diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2019-09-25 18:25:40 +0200 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2019-10-23 10:29:07 +0200 |
commit | b271eb42861c8067fc640a83a957742184d1221c (patch) | |
tree | bace1610d92f28a7bb5cec88042f0c3a93a723da | |
parent | 1425a56c75beecaa289ad59587d636f8f469509e (diff) | |
download | gitlab-ce-b271eb42861c8067fc640a83a957742184d1221c.tar.gz |
Only assign merge params when allowed
When a user updates a merge request coming from a fork, they should
not be able to set `force_remove_source_branch` if they cannot push
code to the source project.
Otherwise developers of the target project could remove the source
branch of the source project by setting this flag through the API.
14 files changed, 191 insertions, 15 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 7cdaa3e3ca7..32741046f39 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -69,6 +69,14 @@ class MergeRequest < ApplicationRecord has_many :merge_request_assignees has_many :assignees, class_name: "User", through: :merge_request_assignees + KNOWN_MERGE_PARAMS = [ + :auto_merge_strategy, + :should_remove_source_branch, + :force_remove_source_branch, + :commit_message, + :squash_commit_message, + :sha + ].freeze serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize after_create :ensure_merge_request_diff diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb index e06659a39cd..e08b4ac2260 100644 --- a/app/services/auto_merge/base_service.rb +++ b/app/services/auto_merge/base_service.rb @@ -3,12 +3,13 @@ module AutoMerge class BaseService < ::BaseService include Gitlab::Utils::StrongMemoize + include MergeRequests::AssignsMergeParams def execute(merge_request) - merge_request.merge_params.merge!(params) + assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy)) + merge_request.auto_merge_enabled = true merge_request.merge_user = current_user - merge_request.auto_merge_strategy = strategy return :failed unless merge_request.save @@ -21,7 +22,7 @@ module AutoMerge end def update(merge_request) - merge_request.merge_params.merge!(params) + assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy)) return :failed unless merge_request.save diff --git a/app/services/concerns/merge_requests/assigns_merge_params.rb b/app/services/concerns/merge_requests/assigns_merge_params.rb new file mode 100644 index 00000000000..bd870d9a1e7 --- /dev/null +++ b/app/services/concerns/merge_requests/assigns_merge_params.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module MergeRequests + module AssignsMergeParams + def self.included(klass) + raise "#{self} can not be included in #{klass} without implementing #current_user" unless klass.method_defined?(:current_user) + end + + def assign_allowed_merge_params(merge_request, merge_params) + known_merge_params = merge_params.to_h.with_indifferent_access.slice(*MergeRequest::KNOWN_MERGE_PARAMS) + + # Not checking `MergeRequest#can_remove_source_branch` as that includes + # other checks that aren't needed here. + known_merge_params.delete(:force_remove_source_branch) unless current_user.can?(:push_code, merge_request.source_project) + + merge_request.merge_params.merge!(known_merge_params) + + # Delete the known params now that they're assigned, so we don't try to + # assign them through an `#assign_attributes` later. + # They could be coming in as strings or symbols + merge_params.to_h.with_indifferent_access.except!(*MergeRequest::KNOWN_MERGE_PARAMS) + end + end +end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 7d4227e4a41..aacc3d6831e 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -2,6 +2,8 @@ module MergeRequests class BaseService < ::IssuableBaseService + include MergeRequests::AssignsMergeParams + def create_note(merge_request, state = merge_request.state) SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, state, nil) end @@ -29,6 +31,18 @@ module MergeRequests private + def create(merge_request) + self.params = assign_allowed_merge_params(merge_request, params) + + super + end + + def update(merge_request) + self.params = assign_allowed_merge_params(merge_request, params) + + super + end + def handle_wip_event(merge_request) if wip_event = params.delete(:wip_event) # We update the title that is provided in the params or we use the mr title diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 214f145d09b..bf4da01723b 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -10,13 +10,14 @@ module MergeRequests # TODO: this should handle all quick actions that don't have side effects # https://gitlab.com/gitlab-org/gitlab-foss/issues/53658 merge_quick_actions_into_params!(merge_request, only: [:target_branch]) - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) if params.has_key?(:force_remove_source_branch) # Assign the projects first so we can use policies for `filter_params` merge_request.author = current_user merge_request.source_project = find_source_project merge_request.target_project = find_target_project + self.params = assign_allowed_merge_params(merge_request, params) + filter_params(merge_request) # merge_request.assign_attributes(...) below is a Rails diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 1c730232abb..9a37a0330fc 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -9,7 +9,6 @@ module MergeRequests merge_request.target_project = @project merge_request.source_project = @source_project merge_request.source_branch = params[:source_branch] - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) create(merge_request) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index ae678d4c036..7c9abb12b6e 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -16,10 +16,6 @@ module MergeRequests params.delete(:force_remove_source_branch) end - if params.has_key?(:force_remove_source_branch) - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) - end - handle_wip_event(merge_request) update_task_event(merge_request) || update(merge_request) end diff --git a/changelogs/unreleased/security-bvl-validate-force-remove-branch-on-mrs.yml b/changelogs/unreleased/security-bvl-validate-force-remove-branch-on-mrs.yml new file mode 100644 index 00000000000..50dc9c32c5d --- /dev/null +++ b/changelogs/unreleased/security-bvl-validate-force-remove-branch-on-mrs.yml @@ -0,0 +1,6 @@ +--- +title: Don't allow maintainers of a target project to delete the source branch of + a merge request from a fork +merge_request: +author: +type: security diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 8179da2f97c..05160a33e61 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1737,6 +1737,38 @@ describe API::MergeRequests do expect(json_response['state']).to eq('closed') expect(json_response['force_remove_source_branch']).to be_truthy end + + context 'with a merge request across forks' do + let(:fork_owner) { create(:user) } + let(:source_project) { fork_project(project, fork_owner) } + let(:target_project) { project } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: target_project, + source_branch: 'fixes', + merge_params: { 'force_remove_source_branch' => false }) + end + + it 'is true for an authorized user' do + put api("/projects/#{target_project.id}/merge_requests/#{merge_request.iid}", fork_owner), params: { state_event: 'close', remove_source_branch: true } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['state']).to eq('closed') + expect(json_response['force_remove_source_branch']).to be true + end + + it 'is false for an unauthorized user' do + expect do + put api("/projects/#{target_project.id}/merge_requests/#{merge_request.iid}", target_project.owner), params: { state_event: 'close', remove_source_branch: true } + end.not_to change { merge_request.reload.merge_params } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['state']).to eq('closed') + expect(json_response['force_remove_source_branch']).to be false + end + end end context "to close a MR" do diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index a409f21a7e4..0a6bcb1badc 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -51,7 +51,7 @@ describe AutoMerge::BaseService do expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'") expect(merge_request.merge_params['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18') expect(merge_request.merge_params['should_remove_source_branch']).to eq(false) - expect(merge_request.merge_params['squash']).to eq(false) + expect(merge_request.squash).to eq(false) expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md') end end @@ -108,7 +108,6 @@ describe AutoMerge::BaseService do 'commit_message' => "Merge branch 'patch-12' into 'master'", 'sha' => "200fcc9c260f7219eaf0daba87d818f0922c5b18", 'should_remove_source_branch' => false, - 'squash' => false, 'squash_commit_message' => "Update README.md" } end diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 931b52470c4..ccbb4e7c30d 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -59,8 +59,9 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do it 'sets the params, merge_user, and flag' do expect(merge_request).to be_valid expect(merge_request.merge_when_pipeline_succeeds).to be_truthy - expect(merge_request.merge_params).to include commit_message: 'Awesome message' + expect(merge_request.merge_params).to include 'commit_message' => 'Awesome message' expect(merge_request.merge_user).to be user + expect(merge_request.auto_merge_strategy).to eq AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS end it 'creates a system note' do @@ -73,7 +74,7 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do end context 'already approved' do - let(:service) { described_class.new(project, user, new_key: true) } + let(:service) { described_class.new(project, user, should_remove_source_branch: true) } let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } before do @@ -90,7 +91,7 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do expect(SystemNoteService).not_to receive(:merge_when_pipeline_succeeds) service.execute(mr_merge_if_green_enabled) - expect(mr_merge_if_green_enabled.merge_params).to have_key(:new_key) + expect(mr_merge_if_green_enabled.merge_params).to have_key('should_remove_source_branch') end end end diff --git a/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb b/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb new file mode 100644 index 00000000000..5b653aa331c --- /dev/null +++ b/spec/services/concerns/merge_requests/assigns_merge_params_spec.rb @@ -0,0 +1,70 @@ +require 'spec_helper' + +describe MergeRequests::AssignsMergeParams do + it 'raises an error when used from an instance that does not respond to #current_user' do + define_class = -> { Class.new { include MergeRequests::AssignsMergeParams }.new } + + expect { define_class.call }.to raise_error %r{can not be included in (.*) without implementing #current_user} + end + + describe '#assign_allowed_merge_params' do + let(:merge_request) { build(:merge_request) } + + let(:params) do + { commit_message: 'Commit Message', + 'should_remove_source_branch' => true, + unknown_symbol: 'Unknown symbol', + 'unknown_string' => 'Unknown String' } + end + + subject(:merge_request_service) do + Class.new do + attr_accessor :current_user + + include MergeRequests::AssignsMergeParams + + def initialize(user) + @current_user = user + end + end + end + + it 'only assigns known parameters to the merge request' do + service = merge_request_service.new(merge_request.author) + + service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.merge_params).to eq('commit_message' => 'Commit Message', 'should_remove_source_branch' => true) + end + + it 'returns a hash without the known merge params' do + service = merge_request_service.new(merge_request.author) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(result).to eq({ 'unknown_symbol' => 'Unknown symbol', 'unknown_string' => 'Unknown String' }) + end + + context 'the force_remove_source_branch param' do + let(:params) { { force_remove_source_branch: true } } + + it 'assigns the param if the user is allowed to do that' do + service = merge_request_service.new(merge_request.author) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.force_remove_source_branch?).to be true + expect(result).to be_empty + end + + it 'only removes the param if the user is not allowed to do that' do + service = merge_request_service.new(build(:user)) + + result = service.assign_allowed_merge_params(merge_request, params) + + expect(merge_request.force_remove_source_branch?).to be_falsy + expect(result).to be_empty + end + end + end +end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index d546a092680..68e53553043 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -83,8 +83,9 @@ describe MergeRequests::BuildService do expect(merge_request.force_remove_source_branch?).to be_falsey end - context 'with force_remove_source_branch parameter' do + context 'with force_remove_source_branch parameter when the user is authorized' do let(:mr_params) { params.merge(force_remove_source_branch: '1') } + let(:source_project) { fork_project(project, user) } let(:merge_request) { described_class.new(project, user, mr_params).execute } it 'assigns force_remove_source_branch' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index d3c4c436901..d31f5dc0176 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -646,5 +646,29 @@ describe MergeRequests::UpdateService, :mailer do expect(merge_request.allow_collaboration).to be_truthy end end + + context 'updating `force_remove_source_branch`' do + let(:target_project) { create(:project, :repository, :public) } + let(:source_project) { fork_project(target_project, nil, repository: true) } + let(:user) { target_project.owner } + let(:merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'fixes', + target_project: target_project) + end + + it "cannot be done by members of the target project when they don't have access" do + expect { update_merge_request(force_remove_source_branch: true) } + .not_to change { merge_request.reload.force_remove_source_branch? }.from(nil) + end + + it 'can be done by members of the target project if they can push to the source project' do + source_project.add_developer(user) + + expect { update_merge_request(force_remove_source_branch: true) } + .to change { merge_request.reload.force_remove_source_branch? }.from(nil).to(true) + end + end end end |