From 1c7c91806d4b9866f512f50f36c9c74b48cb8229 Mon Sep 17 00:00:00 2001 From: drew cimino Date: Mon, 22 Jul 2019 14:16:15 -0400 Subject: Permission fix for MergeRequestsController#pipeline_status - Use set_pipeline_variables to filter for visible pipelines - Mimic response of nonexistent pipeline if not found - Provide set_pipeline_variables as a before_filter for other actions --- .../projects/merge_requests_controller.rb | 9 ++++++- .../unreleased/security-mr-head-pipeline-leak.yml | 5 ++++ .../projects/merge_requests_controller_spec.rb | 30 +++++++++++++++++++--- 3 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/security-mr-head-pipeline-leak.yml diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index f4d381244d9..ee755d68cf1 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -188,7 +188,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo def pipeline_status render json: PipelineSerializer .new(project: @project, current_user: @current_user) - .represent_status(@merge_request.head_pipeline) + .represent_status(head_pipeline) end def ci_environments_status @@ -238,6 +238,13 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo private + def head_pipeline + strong_memoize(:head_pipeline) do + pipeline = @merge_request.head_pipeline + pipeline if can?(current_user, :read_pipeline, pipeline) + end + end + def ci_environments_status_on_merge_result? params[:environment_target] == 'merge_commit' end diff --git a/changelogs/unreleased/security-mr-head-pipeline-leak.yml b/changelogs/unreleased/security-mr-head-pipeline-leak.yml new file mode 100644 index 00000000000..b15b353ff41 --- /dev/null +++ b/changelogs/unreleased/security-mr-head-pipeline-leak.yml @@ -0,0 +1,5 @@ +--- +title: Check permissions before responding in MergeController#pipeline_status +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 b1dc6a65dd4..8a5fdbc98f4 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -1052,17 +1052,39 @@ describe Projects::MergeRequestsController do let(:status) { pipeline.detailed_status(double('user')) } - before do + it 'returns a detailed head_pipeline status in json' do get_pipeline_status - end - it 'return a detailed head_pipeline status in json' do expect(response).to have_gitlab_http_status(:ok) expect(json_response['text']).to eq status.text expect(json_response['label']).to eq status.label expect(json_response['icon']).to eq status.icon expect(json_response['favicon']).to match_asset_path "/assets/ci_favicons/#{status.favicon}.png" end + + context 'with project member visibility on a public project' do + let(:user) { create(:user) } + let(:project) { create(:project, :repository, :public, :builds_private) } + + it 'returns pipeline data to project members' do + project.add_developer(user) + + get_pipeline_status + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['text']).to eq status.text + expect(json_response['label']).to eq status.label + expect(json_response['icon']).to eq status.icon + expect(json_response['favicon']).to match_asset_path "/assets/ci_favicons/#{status.favicon}.png" + end + + it 'returns blank OK response to non-project-members' do + get_pipeline_status + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_empty + end + end end context 'when head_pipeline does not exist' do @@ -1070,7 +1092,7 @@ describe Projects::MergeRequestsController do get_pipeline_status end - it 'return empty' do + it 'returns blank OK response' do expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_empty end -- cgit v1.2.3