diff options
| -rw-r--r-- | app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js | 4 | ||||
| -rw-r--r-- | app/controllers/projects/application_controller.rb | 10 | ||||
| -rw-r--r-- | app/controllers/projects_controller.rb | 10 | ||||
| -rw-r--r-- | app/models/concerns/storage/legacy_namespace.rb | 2 | ||||
| -rw-r--r-- | app/models/note.rb | 2 | ||||
| -rw-r--r-- | app/services/system_note_service.rb | 7 | ||||
| -rw-r--r-- | changelogs/unreleased/38126-security-username-change.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/fix-arbitrary-redirect-vulnerability.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/rs-sanitize-unicode-in-protocol.yml | 5 | ||||
| -rw-r--r-- | lib/banzai/filter/sanitization_filter.rb | 14 | ||||
| -rw-r--r-- | spec/controllers/profiles_controller_spec.rb | 44 | ||||
| -rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 57 | ||||
| -rw-r--r-- | spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js | 23 | ||||
| -rw-r--r-- | spec/lib/banzai/filter/sanitization_filter_spec.rb | 5 | ||||
| -rw-r--r-- | spec/models/namespace_spec.rb | 14 | ||||
| -rw-r--r-- | spec/services/system_note_service_spec.rb | 14 |
16 files changed, 141 insertions, 80 deletions
diff --git a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js b/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js index dd24fc44d2a..d2f92929b8a 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js +++ b/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 */ }) diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index d7dd8ddcb7d..9e79852e378 100644 --- a/app/controllers/projects/application_controller.rb +++ b/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] diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index e90b75672ae..592706f4e0e 100644 --- a/app/controllers/projects_controller.rb +++ b/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 diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index 5ab5c80a2f5..b3020484738 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/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 diff --git a/app/models/note.rb b/app/models/note.rb index ceded9f2aef..8939e590ef1 100644 --- a/app/models/note.rb +++ b/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? diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 0bce20ae5b7..69bd19c1977 100644 --- a/app/services/system_note_service.rb +++ b/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) diff --git a/changelogs/unreleased/38126-security-username-change.yml b/changelogs/unreleased/38126-security-username-change.yml new file mode 100644 index 00000000000..6b0dfc3dca2 --- /dev/null +++ b/changelogs/unreleased/38126-security-username-change.yml @@ -0,0 +1,5 @@ +--- +title: Move project repositories between namespaces when renaming users +merge_request: +author: +type: security diff --git a/changelogs/unreleased/fix-arbitrary-redirect-vulnerability.yml b/changelogs/unreleased/fix-arbitrary-redirect-vulnerability.yml new file mode 100644 index 00000000000..8d456b57963 --- /dev/null +++ b/changelogs/unreleased/fix-arbitrary-redirect-vulnerability.yml @@ -0,0 +1,5 @@ +--- +title: Prevent an open redirect on project pages +merge_request: +author: +type: security diff --git a/changelogs/unreleased/rs-sanitize-unicode-in-protocol.yml b/changelogs/unreleased/rs-sanitize-unicode-in-protocol.yml new file mode 100644 index 00000000000..093c99943e2 --- /dev/null +++ b/changelogs/unreleased/rs-sanitize-unicode-in-protocol.yml @@ -0,0 +1,5 @@ +--- +title: Prevent a persistent XSS in user-provided markup +merge_request: +author: +type: security diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index d8c8deea628..6786b9d07b6 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/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 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="  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 |