Commit 08dbd93b authored by Bob Van Landuyt's avatar Bob Van Landuyt

Validate projects in MR build service

This validates the correct abilities for both projects. Only
`read_project` isn't enough:

For the `source_project` we validate `create_merge_request_from` this
also validates that the user has developer access to the project.

For the `target_project` we validate `create_merge_reqeust_in` this
also validates that the user has access to the project's repository.

To avoid generating diffs for unrelated projects we also validate that
the projects are in the same fork network now.
parent 352af3e5
...@@ -18,7 +18,7 @@ module MergeRequests ...@@ -18,7 +18,7 @@ module MergeRequests
merge_request.source_project = find_source_project merge_request.source_project = find_source_project
merge_request.target_project = find_target_project merge_request.target_project = find_target_project
merge_request.target_branch = find_target_branch 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 only if branches are valid, otherwise
# compare_branches may raise an error # compare_branches may raise an error
...@@ -49,15 +49,19 @@ module MergeRequests ...@@ -49,15 +49,19 @@ module MergeRequests
to: :merge_request to: :merge_request
def find_source_project 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 project
end end
def find_target_project 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 end
def find_target_branch def find_target_branch
...@@ -72,10 +76,11 @@ module MergeRequests ...@@ -72,10 +76,11 @@ module MergeRequests
params[:target_branch].present? params[:target_branch].present?
end 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? return false unless source_branch_specified? || target_branch_specified?
validate_branches validate_projects_and_branches
errors.blank? errors.blank?
end end
...@@ -94,7 +99,12 @@ module MergeRequests ...@@ -94,7 +99,12 @@ module MergeRequests
end end
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 source and target branch') unless branches_present?
add_error('You must select different branches') if same_source_and_target? 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? add_error("Source branch \"#{source_branch}\" does not exist") unless source_branch_exists?
......
---
title: Don't expose cross project repositories through diffs when creating merge reqeusts
merge_request:
author:
type: security
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
...@@ -3,6 +3,7 @@ require 'spec_helper' ...@@ -3,6 +3,7 @@ require 'spec_helper'
describe MergeRequests::BuildService do describe MergeRequests::BuildService do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
include RepoHelpers include RepoHelpers
include ProjectForksHelper
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:source_project) { nil } let(:source_project) { nil }
...@@ -49,7 +50,7 @@ describe MergeRequests::BuildService do ...@@ -49,7 +50,7 @@ describe MergeRequests::BuildService do
describe '#execute' do describe '#execute' do
it 'calls the compare service with the correct arguments' 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) expect(CompareService).to receive(:new)
.with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch) .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch)
.and_call_original .and_call_original
...@@ -393,11 +394,27 @@ describe MergeRequests::BuildService do ...@@ -393,11 +394,27 @@ describe MergeRequests::BuildService do
end end
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 context 'source_project is set and accessible by current_user' do
let(:source_project) { create(:project, :public, :repository)} let(:source_project) { create(:project, :public, :repository)}
let(:commits) { Commit.decorate([commit_1], project) } 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) expect(merge_request.source_project).to eq(source_project)
end end
end end
...@@ -406,11 +423,43 @@ describe MergeRequests::BuildService do ...@@ -406,11 +423,43 @@ describe MergeRequests::BuildService do
let(:source_project) { create(:project, :private, :repository)} let(:source_project) { create(:project, :private, :repository)}
let(:commits) { Commit.decorate([commit_1], project) } 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) expect(merge_request.source_project).to eq(project)
end end
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 context 'when specifying target branch in the description' do
let(:description) { "A merge request targeting another branch\n\n/target_branch with-codeowners" } let(:description) { "A merge request targeting another branch\n\n/target_branch with-codeowners" }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment