From 72f3fdc7a116ec79e2450bf9095aeb73d4e1ad31 Mon Sep 17 00:00:00 2001 From: drew cimino Date: Fri, 28 Jun 2019 10:40:34 -0400 Subject: [PATCH] Use MergeRequest#source_project as permissions reference for MergeRequest#all_pipelines MergeRequest#all_pipelines fetches Ci::Pipeline records from the source project, so we should specifically check that project for permissions. This was already happening for intra-project merge requests, but in the event that the target and source projects both have private builds, we should ensure that the project permissions are respected. --- .../merge_requests/application_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 3 +- .../security-mr-pipeline-permissions.yml | 5 + .../merge_requests_controller_spec.rb | 98 ++++++++++++++++++- 4 files changed, 102 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/security-mr-pipeline-permissions.yml diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index eb469d2d714..4a31f1f3d57 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -41,7 +41,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont def set_pipeline_variables @pipelines = - if can?(current_user, :read_pipeline, @project) + if can?(current_user, :read_pipeline, @merge_request.source_project) @merge_request.all_pipelines else Ci::Pipeline.none diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 8f177895b08..ea3d68449bd 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -82,7 +82,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def pipelines - @pipelines = @merge_request.all_pipelines.page(params[:page]).per(30) + set_pipeline_variables + @pipelines = @pipelines.page(params[:page]).per(30) Gitlab::PollingInterval.set_header(response, interval: 10_000) diff --git a/changelogs/unreleased/security-mr-pipeline-permissions.yml b/changelogs/unreleased/security-mr-pipeline-permissions.yml new file mode 100644 index 00000000000..a317c93228c --- /dev/null +++ b/changelogs/unreleased/security-mr-pipeline-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Use source project as permissions reference for MergeRequestsController#pipelines +merge_request: +author: +type: security diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index f4a18dcba51..63b04e9d049 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -598,10 +598,100 @@ describe Projects::MergeRequestsController do format: :json end - it 'responds with serialized pipelines' do - expect(json_response['pipelines']).not_to be_empty - expect(json_response['count']['all']).to eq 1 - expect(response).to include_pagination_headers + context 'with "enabled" builds on a public project' do + let(:project) { create(:project, :repository, :public) } + + context 'for a project owner' do + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for an unassociated user' do + let(:user) { create :user } + + it 'responds with no pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + + context 'with private builds on a public project' do + let(:project) { create(:project, :repository, :public, :builds_private) } + + context 'for a project owner' do + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for an unassociated user' do + let(:user) { create :user } + + it 'responds with no pipelines' do + expect(json_response['pipelines']).to be_empty + expect(json_response['count']['all']).to eq(0) + expect(response).to include_pagination_headers + end + end + + context 'from a project fork' do + let(:fork_user) { create :user } + let(:forked_project) { fork_project(project, fork_user, repository: true) } # Forked project carries over :builds_private + let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: forked_project) } + + context 'with private builds' do + context 'for the target project member' do + it 'does not respond with serialized pipelines' do + expect(json_response['pipelines']).to be_empty + expect(json_response['count']['all']).to eq(0) + expect(response).to include_pagination_headers + end + end + + context 'for the source project member' do + let(:user) { fork_user } + + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + + context 'with public builds' do + let(:forked_project) do + fork_project(project, fork_user, repository: true).tap do |new_project| + new_project.project_feature.update(builds_access_level: ProjectFeature::ENABLED) + end + end + + context 'for the target project member' do + it 'does not respond with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for the source project member' do + let(:user) { fork_user } + + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + end end end -- GitLab