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

Don’t show source project name when user does not have access

See merge request !2081

4 files changed, 60 insertions, 2 deletions

diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index fdce542bd9e..d45da5180e1 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -21,7 +21,9 @@ module MergeRequests
     delegate :target_branch, :source_branch, :source_project, :target_project, :compare_commits, :wip_title, :description, :errors, to: :merge_request
 
     def find_source_project
-      source_project || project
+      return source_project if source_project.present? && can?(current_user, :read_project, source_project)
+
+      project
     end
 
     def find_target_project
diff --git a/changelogs/unreleased/29364-private-projects-mr-fix.yml b/changelogs/unreleased/29364-private-projects-mr-fix.yml
new file mode 100644
index 00000000000..ab93d6f337b
--- /dev/null
+++ b/changelogs/unreleased/29364-private-projects-mr-fix.yml
@@ -0,0 +1,4 @@
+---
+title: Don’t show source project name when user does not have access
+merge_request:
+author:
diff --git a/spec/features/merge_requests/create_new_mr_spec.rb b/spec/features/merge_requests/create_new_mr_spec.rb
index 8cc0996acab..82f1dea99a0 100644
--- a/spec/features/merge_requests/create_new_mr_spec.rb
+++ b/spec/features/merge_requests/create_new_mr_spec.rb
@@ -43,6 +43,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
 
diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb
index adfa75a524f..f78b4928dc3 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/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
@@ -256,5 +260,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