From 3fc0564ae09a9edf87a71a8c85ff9bf8ad35121d Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 5 Jan 2018 17:55:37 +0000 Subject: [PATCH] Merge branch '41567-projectfix' into 'security-10-3' check project access on MR create See merge request gitlab/gitlabhq!2273 (cherry picked from commit 1fe2325d6ef2bced4c5e97b57691c894f38b2834) 43e85f49 check project access on MR create --- app/services/merge_requests/create_service.rb | 28 +++++++-- changelogs/unreleased/projectfix.yml | 6 ++ spec/features/cycle_analytics_spec.rb | 1 + .../microsoft_teams_service_spec.rb | 4 ++ spec/requests/api/merge_requests_spec.rb | 26 +++++--- spec/requests/api/v3/merge_requests_spec.rb | 26 +++++--- .../merge_requests/create_service_spec.rb | 61 +++++++++++++++++++ ...attermost_notifications_shared_examples.rb | 1 + 8 files changed, 133 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/projectfix.yml diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 49cf534dc0d..634bf3bd690 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -1,15 +1,11 @@ module MergeRequests class CreateService < MergeRequests::BaseService def execute - # @project is used to determine whether the user can set the merge request's - # assignee, milestone and labels. Whether they can depends on their - # permissions on the target project. - source_project = @project - @project = Project.find(params[:target_project_id]) if params[:target_project_id] + set_projects! merge_request = MergeRequest.new merge_request.target_project = @project - merge_request.source_project = source_project + merge_request.source_project = @source_project merge_request.source_branch = params[:source_branch] merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) @@ -58,5 +54,25 @@ module MergeRequests pipelines.order(id: :desc).first end + + def set_projects! + # @project is used to determine whether the user can set the merge request's + # assignee, milestone and labels. Whether they can depends on their + # permissions on the target project. + @source_project = @project + @project = Project.find(params[:target_project_id]) if params[:target_project_id] + + # make sure that source/target project ids are not in + # params so it can't be overridden later when updating attributes + # from params when applying quick actions + params.delete(:source_project_id) + params.delete(:target_project_id) + + unless can?(current_user, :read_project, @source_project) && + can?(current_user, :read_project, @project) + + raise Gitlab::Access::AccessDeniedError + end + end end end diff --git a/changelogs/unreleased/projectfix.yml b/changelogs/unreleased/projectfix.yml new file mode 100644 index 00000000000..4d8ebe6194a --- /dev/null +++ b/changelogs/unreleased/projectfix.yml @@ -0,0 +1,6 @@ +--- +title: Check user authorization for source and target projects when creating a merge + request. +merge_request: +author: +type: security diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index d36954954b6..510677ecf56 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -113,6 +113,7 @@ feature 'Cycle Analytics', :js do context "as a guest" do before do + project.add_developer(user) project.add_guest(guest) allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) diff --git a/spec/models/project_services/microsoft_teams_service_spec.rb b/spec/models/project_services/microsoft_teams_service_spec.rb index 6a5d0decfec..733086e258f 100644 --- a/spec/models/project_services/microsoft_teams_service_spec.rb +++ b/spec/models/project_services/microsoft_teams_service_spec.rb @@ -92,6 +92,10 @@ describe MicrosoftTeamsService do service.hook_data(merge_request, 'open') end + before do + project.add_developer(user) + end + it "calls Microsoft Teams API" do chat_service.execute(merge_sample_data) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 4eae3e50602..8e2982f1a5d 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -754,16 +754,28 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(400) end - context 'when target_branch is specified' do + context 'when target_branch and target_project_id is specified' do + let(:params) do + { title: 'Test merge_request', + target_branch: 'master', + source_branch: 'markdown', + author: user2, + target_project_id: unrelated_project.id } + end + it 'returns 422 if targeting a different fork' do - post api("/projects/#{forked_project.id}/merge_requests", user2), - title: 'Test merge_request', - target_branch: 'master', - source_branch: 'markdown', - author: user2, - target_project_id: unrelated_project.id + unrelated_project.add_developer(user2) + + post api("/projects/#{forked_project.id}/merge_requests", user2), params + expect(response).to have_gitlab_http_status(422) end + + it 'returns 403 if targeting a different fork which user can not access' do + post api("/projects/#{forked_project.id}/merge_requests", user2), params + + expect(response).to have_gitlab_http_status(403) + end end it "returns 201 when target_branch is specified and for the same project" do diff --git a/spec/requests/api/v3/merge_requests_spec.rb b/spec/requests/api/v3/merge_requests_spec.rb index b8b7d9d1c40..6b748369f0d 100644 --- a/spec/requests/api/v3/merge_requests_spec.rb +++ b/spec/requests/api/v3/merge_requests_spec.rb @@ -371,16 +371,28 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(400) end - context 'when target_branch is specified' do + context 'when target_branch and target_project_id is specified' do + let(:params) do + { title: 'Test merge_request', + target_branch: 'master', + source_branch: 'markdown', + author: user2, + target_project_id: unrelated_project.id } + end + it 'returns 422 if targeting a different fork' do - post v3_api("/projects/#{forked_project.id}/merge_requests", user2), - title: 'Test merge_request', - target_branch: 'master', - source_branch: 'markdown', - author: user2, - target_project_id: unrelated_project.id + unrelated_project.add_developer(user2) + + post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params + expect(response).to have_gitlab_http_status(422) end + + it 'returns 403 if targeting a different fork which user can not access' do + post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params + + expect(response).to have_gitlab_http_status(403) + end end it "returns 201 when target_branch is specified and for the same project" do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index dd8c803a2f7..5d226f34d2d 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -263,5 +263,66 @@ describe MergeRequests::CreateService do expect(issue_ids).to match_array([first_issue.id, second_issue.id]) end end + + context 'when source and target projects are different' do + let(:target_project) { create(:project) } + + let(:opts) do + { + title: 'Awesome merge_request', + source_branch: 'feature', + target_branch: 'master', + target_project_id: target_project.id + } + end + + context 'when user can not access source project' do + before do + target_project.add_developer(assignee) + target_project.add_master(user) + end + + it 'raises an error' do + expect { described_class.new(project, user, opts).execute } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + + context 'when user can not access target project' do + before do + target_project.add_developer(assignee) + target_project.add_master(user) + end + + it 'raises an error' do + expect { described_class.new(project, user, opts).execute } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + end + + context 'when user sets source project id' do + let(:another_project) { create(:project) } + + let(:opts) do + { + title: 'Awesome merge_request', + source_branch: 'feature', + target_branch: 'master', + source_project_id: another_project.id + } + end + + before do + project.add_developer(assignee) + project.add_master(user) + end + + it 'ignores source_project_id' do + merge_request = described_class.new(project, user, opts).execute + + expect(merge_request.source_project_id).to eq(project.id) + end + end end end diff --git a/spec/support/slack_mattermost_notifications_shared_examples.rb b/spec/support/slack_mattermost_notifications_shared_examples.rb index 17f3a861ba8..e827a8da0b7 100644 --- a/spec/support/slack_mattermost_notifications_shared_examples.rb +++ b/spec/support/slack_mattermost_notifications_shared_examples.rb @@ -57,6 +57,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do @issue = issue_service.execute @issues_sample_data = issue_service.hook_data(@issue, 'open') + project.add_developer(user) opts = { title: 'Awesome merge_request', description: 'please fix', -- GitLab