Merge branch 'security-bvl-fix-cross-project-mr-exposure' into 'master'

[master] Validate projects in MR build service

See merge request gitlab/gitlabhq!2678

4 files changed, 111 insertions, 10 deletions

diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index 36767621d74..48419da98ad 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -18,7 +18,7 @@ module MergeRequests
       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  = branches_valid?
+      merge_request.can_be_created  = projects_and_branches_valid?
 
       # compare branches only if branches are valid, otherwise
       # compare_branches may raise an error
@@ -49,15 +49,19 @@ module MergeRequests
              to: :merge_request
 
     def find_source_project
-      return source_project if source_project.present? && can?(current_user, :read_project, source_project)
+      return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project)
 
       project
     end
 
     def find_target_project
-      return target_project if target_project.present? && can?(current_user, :read_project, target_project)
+      return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project)
 
-      project.default_merge_request_target
+      target_project = project.default_merge_request_target
+
+      return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project)
+
+      project
     end
 
     def find_target_branch
@@ -72,10 +76,11 @@ module MergeRequests
       params[:target_branch].present?
     end
 
-    def branches_valid?
+    def projects_and_branches_valid?
+      return false if source_project.nil? || target_project.nil?
       return false unless source_branch_specified? || target_branch_specified?
 
-      validate_branches
+      validate_projects_and_branches
       errors.blank?
     end
 
@@ -94,7 +99,12 @@ module MergeRequests
       end
     end
 
-    def validate_branches
+    def validate_projects_and_branches
+      merge_request.validate_target_project
+      merge_request.validate_fork
+
+      return if errors.any?
+
       add_error('You must select source and target branch') unless branches_present?
       add_error('You must select different branches') if same_source_and_target?
       add_error("Source branch \"#{source_branch}\" does not exist") unless source_branch_exists?
diff --git a/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml b/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml
new file mode 100644
index 00000000000..11aae4428fb
--- /dev/null
+++ b/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml
@@ -0,0 +1,5 @@
+---
+title: Don't expose cross project repositories through diffs when creating merge reqeusts
+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_repository_through_new_mr_spec.rb
new file mode 100644
index 00000000000..9318b5f1ebb
--- /dev/null
+++ b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb
@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe 'Merge Request > Tries to access private repo of public project' do
+  let(:current_user) { create(:user) }
+  let(:private_project) do
+    create(:project, :public, :repository,
+           path: 'nothing-to-see-here',
+           name: 'nothing to see here',
+           repository_access_level: ProjectFeature::PRIVATE)
+  end
+  let(:owned_project) do
+    create(:project, :public, :repository,
+           namespace: current_user.namespace,
+           creator: current_user)
+  end
+
+  context 'when the user enters the querystring info for the other project' do
+    let(:mr_path) do
+      project_new_merge_request_diffs_path(
+        owned_project,
+        merge_request: {
+          source_project_id: private_project.id,
+          source_branch: 'feature'
+        }
+      )
+    end
+
+    before do
+      sign_in current_user
+      visit mr_path
+    end
+
+    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
+  end
+end
diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb
index 1894d8c8d0e..536d0d345a4 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/spec/services/merge_requests/build_service_spec.rb
@@ -3,6 +3,7 @@ require 'spec_helper'
 describe MergeRequests::BuildService do
   using RSpec::Parameterized::TableSyntax
   include RepoHelpers
+  include ProjectForksHelper
 
   let(:project) { create(:project, :repository) }
   let(:source_project) { nil }
@@ -49,7 +50,7 @@ describe MergeRequests::BuildService do
 
   describe '#execute' do
     it 'calls the compare service with the correct arguments' do
-      allow_any_instance_of(described_class).to receive(:branches_valid?).and_return(true)
+      allow_any_instance_of(described_class).to receive(:projects_and_branches_valid?).and_return(true)
       expect(CompareService).to receive(:new)
                                   .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch)
                                   .and_call_original
@@ -393,11 +394,27 @@ describe MergeRequests::BuildService do
       end
     end
 
+    context 'target_project is set but repo is not accessible by current_user' do
+      let(:target_project) do
+        create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE)
+      end
+
+      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
+      before do
+        # To create merge requests _from_ a project the user needs at least
+        # developer access
+        source_project.add_developer(user)
+      end
+
+      it 'sets source project correctly' do
         expect(merge_request.source_project).to eq(source_project)
       end
     end
@@ -406,11 +423,43 @@ describe MergeRequests::BuildService do
       let(:source_project) { create(:project, :private, :repository)}
       let(:commits) { Commit.decorate([commit_1], project) }
 
-      it 'sets target project correctly' do
+      it 'sets source project correctly' do
+        expect(merge_request.source_project).to eq(project)
+      end
+    end
+
+    context 'source_project is set but the user cannot create merge requests from the project' do
+      let(:source_project) do
+        create(:project, :public, :repository, merge_requests_access_level: ProjectFeature::PRIVATE)
+      end
+
+      it 'sets the source_project correctly' do
         expect(merge_request.source_project).to eq(project)
       end
     end
 
+    context 'target_project is not in the fork network of source_project' do
+      let(:target_project) { create(:project, :public, :repository) }
+
+      it 'adds an error to the merge request' do
+        expect(merge_request.errors[:validate_fork]).to contain_exactly('Source project is not a fork of the target project')
+      end
+    end
+
+    context 'target_project is in the fork network of source project but no longer accessible' do
+      let!(:project) { fork_project(target_project, user, namespace: user.namespace, repository: true) }
+      let(:source_project) { project }
+      let(:target_project) { create(:project, :public, :repository) }
+
+      before do
+        target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'sets the target_project correctly' do
+        expect(merge_request.target_project).to eq(project)
+      end
+    end
+
     context 'when specifying target branch in the description' do
       let(:description) { "A merge request targeting another branch\n\n/target_branch with-codeowners" }