diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-10-29 18:58:12 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-10-29 18:58:12 +0300 |
commit | a0043682b500ce39ff4eba00e8c1cecc64819ea1 (patch) | |
tree | 9c0f3d058052f42075ebbee9ae827023829efeec /app/services | |
parent | af84dec405c3f8d13220ee3f98eb4b2f0276a93d (diff) | |
parent | 20cb4f7ab567062fd67ccd40cd29ff1d2e85d8f0 (diff) |
Merge branch 'security-bvl-validate-force-remove-branch-on-mrs-ce' into 'master'
Only assign merge params when allowed
See merge request gitlab/gitlabhq!3458
Diffstat (limited to 'app/services')
-rw-r--r-- | app/services/auto_merge/base_service.rb | 7 | ||||
-rw-r--r-- | app/services/concerns/merge_requests/assigns_merge_params.rb | 24 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 14 | ||||
-rw-r--r-- | app/services/merge_requests/build_service.rb | 2 | ||||
-rw-r--r-- | app/services/merge_requests/create_service.rb | 1 | ||||
-rw-r--r-- | app/services/merge_requests/update_service.rb | 4 |
6 files changed, 44 insertions, 8 deletions
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 |