diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-08-26 07:43:01 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-08-26 07:43:01 +0000 |
commit | 71636fed6e048b41cc595871bea412d6e75c56ea (patch) | |
tree | 39f54a2b617026c04b62a325a975e7954c8431f5 | |
parent | ffb237a45b3232d0deebcbfbbc5daf3515038dfd (diff) | |
parent | 0b0ee2bc9df042ba7a7d4dc9135c45170ca65c93 (diff) | |
download | gitlab-ce-71636fed6e048b41cc595871bea412d6e75c56ea.tar.gz |
Merge branch 'security-mr-head-pipeline-leak-12-1' into '12-1-stable'
Permission fix for MergeRequestsController#pipeline_status
See merge request gitlab/gitlabhq!3278
3 files changed, 39 insertions, 5 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index c9a1f28f87e..a2e6f878f90 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -189,7 +189,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 @@ -239,6 +239,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 3555d12b41c..64ab1e9e01a 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -1097,17 +1097,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 @@ -1115,7 +1137,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 |