Commit 20cb4f7a authored by Bob Van Landuyt's avatar Bob Van Landuyt

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.
parent dc0622db
......@@ -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
......
......@@ -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
......
# 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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
---
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
......@@ -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
......
......@@ -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
......
......@@ -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
......
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
......@@ -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
......
......@@ -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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment