From 20cb4f7ab567062fd67ccd40cd29ff1d2e85d8f0 Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Wed, 25 Sep 2019 18:25:40 +0200
Subject: [PATCH] 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.
---
app/models/merge_request.rb | 8 +++
app/services/auto_merge/base_service.rb | 7 +-
.../merge_requests/assigns_merge_params.rb | 24 +++++++
app/services/merge_requests/base_service.rb | 14 ++++
app/services/merge_requests/build_service.rb | 2 +
app/services/merge_requests/create_service.rb | 1 -
app/services/merge_requests/update_service.rb | 4 --
...vl-validate-force-remove-branch-on-mrs.yml | 6 ++
spec/requests/api/merge_requests_spec.rb | 32 +++++++++
spec/services/auto_merge/base_service_spec.rb | 3 +-
...rge_when_pipeline_succeeds_service_spec.rb | 7 +-
.../assigns_merge_params_spec.rb | 70 +++++++++++++++++++
.../merge_requests/build_service_spec.rb | 3 +-
.../merge_requests/update_service_spec.rb | 24 +++++++
14 files changed, 191 insertions(+), 14 deletions(-)
create mode 100644 app/services/concerns/merge_requests/assigns_merge_params.rb
create mode 100644 changelogs/unreleased/security-bvl-validate-force-remove-branch-on-mrs.yml
create mode 100644 spec/services/concerns/merge_requests/assigns_merge_params_spec.rb
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index cd8ede3905a..67f666a89b2 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 06ee25eac2a..456cc589477 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -24,6 +24,8 @@ module MergeRequests
merge_request.source_project.remove_source_branch_after_merge?
end
+ 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 721998ede6a..1d482f3c5bd 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 c396539cf56..1bebeacb4d4 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 46e86d5b4cb..9b358839c06 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_truthy
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 8c796475de0..741420d76a7 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
--
2.30.9