Commit 30d2af94 authored by John Jarvis's avatar John Jarvis

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

[11.5] Validate projects in MR build service

See merge request gitlab/gitlabhq!2704
parents 6aeaa1bd 83d0a217
......@@ -17,7 +17,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
......@@ -48,15 +48,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
......@@ -71,10 +75,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
......@@ -93,7 +98,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?
......
---
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'
describe MergeRequests::BuildService do
using RSpec::Parameterized::TableSyntax
include RepoHelpers
include ProjectForksHelper
let(:project) { create(:project, :repository) }
let(:source_project) { nil }
......@@ -44,7 +45,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
......@@ -375,11 +376,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
......@@ -388,11 +405,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" }
......
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