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

Security fixes for 10.1 RC

See merge request gitlab/gitlabhq!2209

app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js

@@ -123,8 +123,8 @@ class FilteredSearchVisualTokens {
         /* eslint-disable no-param-reassign */
         tokenValueContainer.dataset.originalValue = tokenValue;
         tokenValueElement.innerHTML = `
-          <img class="avatar s20" src="${user.avatar_url}" alt="${user.name}'s avatar">
-          ${user.name}
+          <img class="avatar s20" src="${user.avatar_url}" alt="">
+          ${_.escape(user.name)}
         `;
         /* eslint-enable no-param-reassign */
       })

app/controllers/projects/application_controller.rb

@@ -2,7 +2,6 @@ class Projects::ApplicationController < ApplicationController
   include RoutableActions
   skip_before_action :authenticate_user!
-  before_action :redirect_git_extension
   before_action :project
   before_action :repository
   layout 'project'
@@ -11,15 +10,6 @@ class Projects::ApplicationController < ApplicationController
   private
-  def redirect_git_extension
-    # Redirect from
-    #   localhost/group/project.git
-    # to
-    #   localhost/group/project
-    #
-    redirect_to url_for(params.merge(format: nil)) if params[:format] == 'git'
-  end
-
   def project
     return @project if @project
     return nil unless params[:project_id] || params[:id]

app/controllers/projects_controller.rb

@@ -4,6 +4,7 @@ class ProjectsController < Projects::ApplicationController
   include PreviewMarkdown
   before_action :authenticate_user!, except: [:index, :show, :activity, :refs]
+  before_action :redirect_git_extension, only: [:show]
   before_action :project, except: [:index, :new, :create]
   before_action :repository, except: [:index, :new, :create]
   before_action :assign_ref_vars, only: [:show], if: :repo_exists?
@@ -389,4 +390,13 @@ class ProjectsController < Projects::ApplicationController
   def project_export_enabled
     render_404 unless current_application_settings.project_export_enabled?
   end
+
+  def redirect_git_extension
+    # Redirect from
+    #   localhost/group/project.git
+    # to
+    #   localhost/group/project
+    #
+    redirect_to request.original_url.sub(/\.git\/?\Z/, '') if params[:format] == 'git'
+  end
 end

app/models/concerns/storage/legacy_namespace.rb

@@ -7,6 +7,8 @@ module Storage
         raise Gitlab::UpdatePathError.new('Namespace cannot be moved, because at least one project has tags in container registry')
       end
+      expires_full_path_cache
+
       # Move the namespace directory in all storage paths used by member projects
       repository_storage_paths.each do |repository_storage_path|
         # Ensure old directory exists before moving it

app/models/note.rb

@@ -169,7 +169,7 @@ class Note < ActiveRecord::Base
   end
   def cross_reference?
-    system? && SystemNoteService.cross_reference?(note)
+    system? && matches_cross_reference_regex?
   end
   def diff_note?

app/services/system_note_service.rb

@@ -162,7 +162,6 @@ module SystemNoteService
   #   "changed time estimate to 3d 5h"
   #
   # Returns the created Note object
-
   def change_time_estimate(noteable, project, author)
     parsed_time = Gitlab::TimeTrackingFormatter.output(noteable.time_estimate)
     body = if noteable.time_estimate == 0
@@ -188,7 +187,6 @@ module SystemNoteService
   #   "added 2h 30m of time spent"
   #
   # Returns the created Note object
-
   def change_time_spent(noteable, project, author)
     time_spent = noteable.time_spent
@@ -453,10 +451,6 @@ module SystemNoteService
     end
   end
-  def cross_reference?(note_text)
-    note_text =~ /\A#{cross_reference_note_prefix}/i
-  end
-
   # Check if a cross-reference is disallowed
   #
   # This method prevents adding a "mentioned in !1" note on every single commit
@@ -486,7 +480,6 @@ module SystemNoteService
   # mentioner - Mentionable object
   #
   # Returns Boolean
-
   def cross_reference_exists?(noteable, mentioner)
     # Initial scope should be system notes of this noteable type
     notes = Note.system.where(noteable_type: noteable.class)

changelogs/unreleased/38126-security-username-change.yml

+---
+title: Move project repositories between namespaces when renaming users
+merge_request:
+author:
+type: security

changelogs/unreleased/fix-arbitrary-redirect-vulnerability.yml

+---
+title: Prevent an open redirect on project pages
+merge_request:
+author:
+type: security

changelogs/unreleased/rs-sanitize-unicode-in-protocol.yml

+---
+title: Prevent a persistent XSS in user-provided markup
+merge_request:
+author:
+type: security

lib/banzai/filter/sanitization_filter.rb

@@ -75,9 +75,19 @@ module Banzai
             begin
               node['href'] = node['href'].strip
               uri = Addressable::URI.parse(node['href'])
-              uri.scheme = uri.scheme.downcase if uri.scheme
-              node.remove_attribute('href') if UNSAFE_PROTOCOLS.include?(uri.scheme)
+              return unless uri.scheme
+
+              # Remove all invalid scheme characters before checking against the
+              # list of unsafe protocols.
+              #
+              # See https://tools.ietf.org/html/rfc3986#section-3.1
+              scheme = uri.scheme
+                .strip
+                .downcase
+                .gsub(/[^A-Za-z0-9\+\.\-]+/, '')
+
+              node.remove_attribute('href') if UNSAFE_PROTOCOLS.include?(scheme)
             rescue Addressable::URI::InvalidURIError
               node.remove_attribute('href')
             end

spec/controllers/profiles_controller_spec.rb

 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

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
-        RequestStore.clear!
-        control_count = ActiveRecord::QueryRecorder.new do
+      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)}" }
+        before do
+          create(:discussion_note_on_issue, :system, noteable: issue, project: issue.project, note: cross_reference)
+        end
+        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!
-        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)
+          expect(JSON.parse(response.body).count).to eq(1)
+        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
+          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

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);

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>'

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"
-      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
+      namespace.update_attributes(path: namespace.full_path + '_new')
+      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

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