Merge branch '29364-private-projects-mr-fix' into 'security'

app/services/merge_requests/build_service.rb

@@ -6,7 +6,7 @@ module MergeRequests
       # Set MR attributes
       merge_request.can_be_created = true
       merge_request.compare_commits = []
-      merge_request.source_project = project unless merge_request.source_project
+      merge_request.source_project = find_source_project
 
       merge_request.target_project = nil unless can?(current_user, :read_project, merge_request.target_project)
 
@@ -31,6 +31,12 @@ module MergeRequests
 
     private
 
+    def find_source_project
+      return source_project if source_project.present? && can?(current_user, :read_project, source_project)
+
+      project
+    end
+
     def validate_branches(merge_request)
       messages = []
 

changelogs/unreleased/29364-private-projects-mr-fix.yml

+---
+title: Don’t show source project name when user does not have access
+merge_request:
+author:

spec/features/merge_requests/create_new_mr_spec.rb

@@ -41,6 +41,18 @@ feature 'Create New Merge Request', feature: true, js: true do
       visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { target_project_id: private_project.id })
 
       expect(page).not_to have_content private_project.path_with_namespace
+      expect(page).to have_content project.path_with_namespace
+    end
+  end
+
+  context 'when source project cannot be viewed by the current user' do
+    it 'does not leak the private project name & namespace' do
+      private_project = create(:project, :private)
+
+      visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_project_id: private_project.id })
+
+      expect(page).not_to have_content private_project.path_with_namespace
+      expect(page).to have_content project.path_with_namespace
     end
   end
 

spec/services/merge_requests/build_service_spec.rb

@@ -4,6 +4,8 @@ describe MergeRequests::BuildService, services: true do
   include RepoHelpers
 
   let(:project) { create(:project) }
+  let(:source_project) { nil }
+  let(:target_project) { nil }
   let(:user) { create(:user) }
   let(:issue_confidential) { false }
   let(:issue) { create(:issue, project: project, title: 'A bug', confidential: issue_confidential) }
@@ -20,7 +22,9 @@ describe MergeRequests::BuildService, services: true do
     MergeRequests::BuildService.new(project, user,
                                     description: description,
                                     source_branch: source_branch,
-                                    target_branch: target_branch)
+                                    target_branch: target_branch,
+                                    source_project: source_project,
+                                    target_project: target_project)
   end
 
   before do
@@ -254,5 +258,41 @@ describe MergeRequests::BuildService, services: true do
         )
       end
     end
+
+    context 'target_project is set and accessible by current_user' do
+      let(:target_project) { create(:project, :public, :repository)}
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.target_project).to eq(target_project)
+      end
+    end
+
+    context 'target_project is set but not accessible by current_user' do
+      let(:target_project) { create(:project, :private, :repository)}
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.target_project).to eq(project)
+      end
+    end
+
+    context 'source_project is set and accessible by current_user' do
+      let(:source_project) { create(:project, :public, :repository)}
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.source_project).to eq(source_project)
+      end
+    end
+
+    context 'source_project is set but not accessible by current_user' do
+      let(:source_project) { create(:project, :private, :repository)}
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.source_project).to eq(project)
+      end
+    end
   end
 end