Merge branch 'security-10-1' into '10-1-stable'

Security fixes for 10.1 RC

See merge request gitlab/gitlabhq!2209

6 files changed, 99 insertions, 58 deletions

diff --git a/spec/controllers/profiles_controller_spec.rb b/spec/controllers/profiles_controller_spec.rb
index ce5040ff02b..d380978b86e 100644
--- a/spec/controllers/profiles_controller_spec.rb
+++ b/spec/controllers/profiles_controller_spec.rb
@@ -1,9 +1,10 @@
 require('spec_helper')
 
-describe ProfilesController do
-  describe "PUT update" do
-    it "allows an email update from a user without an external email address" do
-      user = create(:user)
+describe ProfilesController, :request_store do
+  let(:user) { create(:user) }
+
+  describe 'PUT update' do
+    it 'allows an email update from a user without an external email address' do
       sign_in(user)
 
       put :update,
@@ -29,7 +30,7 @@ describe ProfilesController do
       expect(user.unconfirmed_email).to eq nil
     end
 
-    it "ignores an email update from a user with an external email address" do
+    it 'ignores an email update from a user with an external email address' do
       stub_omniauth_setting(sync_profile_from_provider: ['ldap'])
       stub_omniauth_setting(sync_profile_attributes: true)
 
@@ -46,7 +47,7 @@ describe ProfilesController do
       expect(ldap_user.unconfirmed_email).not_to eq('john@gmail.com')
     end
 
-    it "ignores an email and name update but allows a location update from a user with external email and name, but not external location" do
+    it 'ignores an email and name update but allows a location update from a user with external email and name, but not external location' do
       stub_omniauth_setting(sync_profile_from_provider: ['ldap'])
       stub_omniauth_setting(sync_profile_attributes: true)
 
@@ -65,4 +66,35 @@ describe ProfilesController do
       expect(ldap_user.location).to eq('City, Country')
     end
   end
+
+  describe 'PUT update_username' do
+    let(:namespace) { user.namespace }
+    let(:project) { create(:project_empty_repo, namespace: namespace) }
+    let(:gitlab_shell) { Gitlab::Shell.new }
+    let(:new_username) { 'renamedtosomethingelse' }
+
+    it 'allows username change' do
+      sign_in(user)
+
+      put :update_username,
+        user: { username: new_username }
+
+      user.reload
+
+      expect(response.status).to eq(302)
+      expect(user.username).to eq(new_username)
+    end
+
+    it 'moves dependent projects to new namespace' do
+      sign_in(user)
+
+      put :update_username,
+        user: { username: new_username }
+
+      user.reload
+
+      expect(response.status).to eq(302)
+      expect(gitlab_shell.exists?(project.repository_storage_path, "#{new_username}/#{project.path}.git")).to be_truthy
+    end
+  end
 end
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 053bd73fee3..ed8088a46f0 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -850,47 +850,48 @@ describe Projects::IssuesController do
 
   describe 'GET #discussions' do
     let!(:discussion) { create(:discussion_note_on_issue, noteable: issue, project: issue.project) }
-
-    before do
-      project.add_developer(user)
-      sign_in(user)
-    end
-
-    it 'returns discussion json' do
-      get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
-
-      expect(JSON.parse(response.body).first.keys).to match_array(%w[id reply_id expanded notes individual_note])
-    end
-
-    context 'with cross-reference system note', :request_store do
-      let(:new_issue) { create(:issue) }
-      let(:cross_reference) { "mentioned in #{new_issue.to_reference(issue.project)}" }
-
+    context 'when authenticated' do
       before do
-        create(:discussion_note_on_issue, :system, noteable: issue, project: issue.project, note: cross_reference)
+        project.add_developer(user)
+        sign_in(user)
       end
 
-      it 'filters notes that the user should not see' do
+      it 'returns discussion json' do
         get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
 
-        expect(JSON.parse(response.body).count).to eq(1)
+        expect(json_response.first.keys).to match_array(%w[id reply_id expanded notes individual_note])
       end
 
-      it 'does not result in N+1 queries' do
-        # Instantiate the controller variables to ensure QueryRecorder has an accurate base count
-        get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
+      context 'with cross-reference system note', :request_store do
+        let(:new_issue) { create(:issue) }
+        let(:cross_reference) { "mentioned in #{new_issue.to_reference(issue.project)}" }
 
-        RequestStore.clear!
+        before do
+          create(:discussion_note_on_issue, :system, noteable: issue, project: issue.project, note: cross_reference)
+        end
 
-        control_count = ActiveRecord::QueryRecorder.new do
+        it 'filters notes that the user should not see' do
           get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
-        end.count
 
-        RequestStore.clear!
+          expect(JSON.parse(response.body).count).to eq(1)
+        end
 
-        create_list(:discussion_note_on_issue, 2, :system, noteable: issue, project: issue.project, note: cross_reference)
+        it 'does not result in N+1 queries' do
+          # Instantiate the controller variables to ensure QueryRecorder has an accurate base count
+          get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
 
-        expect { get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid }.not_to exceed_query_limit(control_count)
+          RequestStore.clear!
+
+          control_count = ActiveRecord::QueryRecorder.new do
+            get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
+          end.count
+
+          RequestStore.clear!
+
+          create_list(:discussion_note_on_issue, 2, :system, noteable: issue, project: issue.project, note: cross_reference)
+
+          expect { get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid }.not_to exceed_query_limit(control_count)
+        end
       end
     end
   end
diff --git a/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js b/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js
index 67166802c70..2ecb64d84b5 100644
--- a/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js
+++ b/spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js
@@ -791,6 +791,29 @@ describe('Filtered Search Visual Tokens', () => {
         expect(tokenValueElement.innerText.trim()).toBe(dummyUser.name);
         const avatar = tokenValueElement.querySelector('img.avatar');
         expect(avatar.src).toBe(dummyUser.avatar_url);
+        expect(avatar.alt).toBe('');
+      })
+      .then(done)
+      .catch(done.fail);
+    });
+
+    it('escapes user name when creating token', (done) => {
+      const dummyUser = {
+        name: '<script>',
+        avatar_url: `${gl.TEST_HOST}/mypics/avatar.png`,
+      };
+      const { tokenValueContainer, tokenValueElement } = findElements(authorToken);
+      const tokenValue = tokenValueElement.innerText;
+      usersCacheSpy = (username) => {
+        expect(`@${username}`).toBe(tokenValue);
+        return Promise.resolve(dummyUser);
+      };
+
+      subject.updateUserTokenAppearance(tokenValueContainer, tokenValueElement, tokenValue)
+      .then(() => {
+        expect(tokenValueElement.innerText.trim()).toBe(dummyUser.name);
+        tokenValueElement.querySelector('.avatar').remove();
+        expect(tokenValueElement.innerHTML.trim()).toBe(_.escape(dummyUser.name));
       })
       .then(done)
       .catch(done.fail);
diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb
index 5f41e28fece..17a620ef603 100644
--- a/spec/lib/banzai/filter/sanitization_filter_spec.rb
+++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb
@@ -217,6 +217,11 @@ describe Banzai::Filter::SanitizationFilter do
         output: '<img>'
       },
 
+      'protocol-based JS injection: Unicode' => {
+        input: %Q(<a href="\u0001java\u0003script:alert('XSS')">foo</a>),
+        output: '<a>foo</a>'
+      },
+
       'protocol-based JS injection: spaces and entities' => {
         input:  '<a href=" &#14;  javascript:alert(\'XSS\');">foo</a>',
         output: '<a href="">foo</a>'
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 2ebf6acd42a..a241d4111bd 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -4,6 +4,7 @@ describe Namespace do
   include ProjectForksHelper
 
   let!(:namespace) { create(:namespace) }
+  let(:gitlab_shell) { Gitlab::Shell.new }
 
   describe 'associations' do
     it { is_expected.to have_many :projects }
@@ -153,25 +154,18 @@ describe Namespace do
     end
   end
 
-  describe '#move_dir' do
+  describe '#move_dir', :request_store do
     let(:namespace) { create(:namespace) }
     let!(:project) { create(:project_empty_repo, namespace: namespace) }
 
-    before do
-      allow(namespace).to receive(:path_changed?).and_return(true)
-    end
-
     it "raises error when directory exists" do
       expect { namespace.move_dir }.to raise_error("namespace directory cannot be moved")
     end
 
     it "moves dir if path changed" do
-      new_path = namespace.full_path + "_new"
+      namespace.update_attributes(path: namespace.full_path + '_new')
 
-      allow(namespace).to receive(:full_path_was).and_return(namespace.full_path)
-      allow(namespace).to receive(:full_path).and_return(new_path)
-      expect(namespace).to receive(:remove_exports!)
-      expect(namespace.move_dir).to be_truthy
+      expect(gitlab_shell.exists?(project.repository_storage_path, "#{namespace.path}/#{project.path}.git")).to be_truthy
     end
 
     context "when any project has container images" do
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index cd473c1f388..0a6ab455abe 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -502,20 +502,6 @@ describe SystemNoteService do
     end
   end
 
-  describe '.cross_reference?' do
-    it 'is truthy when text begins with expected text' do
-      expect(described_class.cross_reference?('mentioned in something')).to be_truthy
-    end
-
-    it 'is truthy when text begins with legacy capitalized expected text' do
-      expect(described_class.cross_reference?('mentioned in something')).to be_truthy
-    end
-
-    it 'is falsey when text does not begin with expected text' do
-      expect(described_class.cross_reference?('this is a note')).to be_falsey
-    end
-  end
-
   describe '.cross_reference_disallowed?' do
     context 'when mentioner is not a MergeRequest' do
       it 'is falsey' do