Filter params in MR build service

Reusing the existing `IssuableBaseService#filter_params` which uses
the policies to determine what params a user can set, and which values
it can be set to.

This also removed the need for the seperate call to
`IssuableBaseService#ensure_milestone_available`.

The `Issues::BuildService` does not suffer from this because it limits
the params that are assignable to the `title`, `description` and
`milestone_id`.

4 files changed, 83 insertions, 8 deletions

diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index 109c964e577..b28f80939ae 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -11,15 +11,18 @@ module MergeRequests
       # https://gitlab.com/gitlab-org/gitlab-ce/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)
-      merge_request.assign_attributes(params)
 
+      # 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
+
+      filter_params(merge_request)
+      merge_request.assign_attributes(params.to_h.compact)
+
       merge_request.compare_commits = []
-      merge_request.source_project  = find_source_project
-      merge_request.target_project  = find_target_project
-      merge_request.target_branch   = find_target_branch
-      merge_request.can_be_created  = projects_and_branches_valid?
-      ensure_milestone_available(merge_request)
+      merge_request.target_branch = find_target_branch
+      merge_request.can_be_created = projects_and_branches_valid?
 
       # compare branches only if branches are valid, otherwise
       # compare_branches may raise an error
@@ -50,12 +53,14 @@ module MergeRequests
              to: :merge_request
 
     def find_source_project
+      source_project = project_from_params(:source_project)
       return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project)
 
       project
     end
 
     def find_target_project
+      target_project = project_from_params(:target_project)
       return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project)
 
       target_project = project.default_merge_request_target
@@ -65,6 +70,17 @@ module MergeRequests
       project
     end
 
+    def project_from_params(param_name)
+      project_from_params = params.delete(param_name)
+
+      id_param_name = :"#{param_name}_id"
+      if project_from_params.nil? && params[id_param_name]
+        project_from_params = Project.find_by_id(params.delete(id_param_name))
+      end
+
+      project_from_params
+    end
+
     def find_target_branch
       target_branch || target_project.default_branch
     end
diff --git a/changelogs/unreleased/security-bvl-filter-mr-params.yml b/changelogs/unreleased/security-bvl-filter-mr-params.yml
new file mode 100644
index 00000000000..4433ec73b7c
--- /dev/null
+++ b/changelogs/unreleased/security-bvl-filter-mr-params.yml
@@ -0,0 +1,5 @@
+---
+title: Filter merge request params on the new merge request page
+merge_request:
+author:
+type: security
diff --git a/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb b/spec/features/merge_request/user_tries_to_access_private_project_info_through_new_mr_spec.rb
index 9318b5f1ebb..1ebe9e2e409 100644
--- a/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb
+++ b/spec/features/merge_request/user_tries_to_access_private_project_info_through_new_mr_spec.rb
@@ -1,6 +1,8 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
-describe 'Merge Request > Tries to access private repo of public project' do
+describe 'Merge Request > User tries to access private project information through the new mr page' do
   let(:current_user) { create(:user) }
   let(:private_project) do
     create(:project, :public, :repository,
@@ -33,5 +35,22 @@ describe 'Merge Request > Tries to access private repo of public project' do
     it "does not mention the project the user can't see the repo of" do
       expect(page).not_to have_content('nothing-to-see-here')
     end
+
+    context 'when the user enters label information from the private project in the querystring' do
+      let(:inaccessible_label) { create(:label, project: private_project) }
+      let(:mr_path) do
+        project_new_merge_request_path(
+          owned_project,
+          merge_request: {
+            label_ids: [inaccessible_label.id],
+            source_branch: 'feature'
+          }
+        )
+      end
+
+      it 'does not expose the label name' do
+        expect(page).not_to have_content(inaccessible_label.name)
+      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 5c3b209086c..f18239f6d39 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/spec/services/merge_requests/build_service_spec.rb
@@ -1,5 +1,4 @@
 # frozen_string_literal: true
-
 require 'spec_helper'
 
 describe MergeRequests::BuildService do
@@ -225,6 +224,11 @@ describe MergeRequests::BuildService do
             let(:label_ids) { [label2.id] }
             let(:milestone_id) { milestone2.id }
 
+            before do
+              # Guests are not able to assign labels or milestones to an issue
+              project.add_developer(user)
+            end
+
             it 'assigns milestone_id and label_ids instead of issue labels and milestone' do
               expect(merge_request.milestone).to eq(milestone2)
               expect(merge_request.labels).to match_array([label2])
@@ -479,4 +483,35 @@ describe MergeRequests::BuildService do
       end
     end
   end
+
+  context 'when assigning labels' do
+    let(:label_ids) { [create(:label, project: project).id] }
+
+    context 'for members with less than developer access' do
+      it 'is not allowed' do
+        expect(merge_request.label_ids).to be_empty
+      end
+    end
+
+    context 'for users allowed to assign labels' do
+      before do
+        project.add_developer(user)
+      end
+
+      context 'for labels in the project' do
+        it 'is allowed for developers' do
+          expect(merge_request.label_ids).to contain_exactly(*label_ids)
+        end
+      end
+
+      context 'for unrelated labels' do
+        let(:project_label) { create(:label, project: project) }
+        let(:label_ids) { [create(:label).id, project_label.id] }
+
+        it 'only assigns related labels' do
+          expect(merge_request.label_ids).to contain_exactly(project_label.id)
+        end
+      end
+    end
+  end
 end