diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-02-10 23:14:50 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-02-10 23:15:15 +0000 |
commit | 9d8e65803b6eba4b689bb442c158755c516c3eab (patch) | |
tree | 63f125a21f3b0744ac4865aceca0c5a10cb1bbdd | |
parent | 0e71ffb85425829c094aa0ff32f9a28048b1403d (diff) | |
download | gitlab-ce-9d8e65803b6eba4b689bb442c158755c516c3eab.tar.gz |
Add latest changes from gitlab-org/security/gitlab@13-6-stable-ee
-rw-r--r-- | changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml | 5 | ||||
-rw-r--r-- | lib/api/merge_request_approvals.rb | 2 | ||||
-rw-r--r-- | lib/api/merge_request_diffs.rb | 4 | ||||
-rw-r--r-- | lib/api/merge_requests.rb | 11 | ||||
-rw-r--r-- | lib/api/todos.rb | 5 | ||||
-rw-r--r-- | spec/requests/api/merge_request_approvals_spec.rb | 6 | ||||
-rw-r--r-- | spec/requests/api/merge_request_diffs_spec.rb | 12 | ||||
-rw-r--r-- | spec/requests/api/merge_requests_spec.rb | 30 | ||||
-rw-r--r-- | spec/requests/api/todos_spec.rb | 8 | ||||
-rw-r--r-- | spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb | 23 |
10 files changed, 106 insertions, 0 deletions
diff --git a/changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml b/changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml new file mode 100644 index 00000000000..c1174904018 --- /dev/null +++ b/changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml @@ -0,0 +1,5 @@ +--- +title: Check user access on API merge request read actions +merge_request: +author: +type: security diff --git a/lib/api/merge_request_approvals.rb b/lib/api/merge_request_approvals.rb index 27ef0b9c7cd..0d496476412 100644 --- a/lib/api/merge_request_approvals.rb +++ b/lib/api/merge_request_approvals.rb @@ -26,6 +26,8 @@ module API # GET /projects/:id/merge_requests/:merge_request_iid/approvals desc 'List approvals for merge request' get 'approvals' do + not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project) + merge_request = find_merge_request_with_access(params[:merge_request_iid]) present_approval(merge_request) diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index 0ffb38438eb..97a6c7075b3 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -23,6 +23,8 @@ module API use :pagination end get ":id/merge_requests/:merge_request_iid/versions" do + not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project) + merge_request = find_merge_request_with_access(params[:merge_request_iid]) present paginate(merge_request.merge_request_diffs.order_id_desc), with: Entities::MergeRequestDiff @@ -39,6 +41,8 @@ module API end get ":id/merge_requests/:merge_request_iid/versions/:version_id" do + not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project) + merge_request = find_merge_request_with_access(params[:merge_request_iid]) present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index d17e451093b..ba90a249d1e 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -243,6 +243,8 @@ module API success Entities::MergeRequest end get ':id/merge_requests/:merge_request_iid' do + not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project) + merge_request = find_merge_request_with_access(params[:merge_request_iid]) present merge_request, @@ -259,7 +261,10 @@ module API success Entities::UserBasic end get ':id/merge_requests/:merge_request_iid/participants' do + not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project) + merge_request = find_merge_request_with_access(params[:merge_request_iid]) + participants = ::Kaminari.paginate_array(merge_request.participants) present paginate(participants), with: Entities::UserBasic @@ -269,6 +274,8 @@ module API success Entities::Commit end get ':id/merge_requests/:merge_request_iid/commits' do + not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project) + merge_request = find_merge_request_with_access(params[:merge_request_iid]) commits = @@ -350,6 +357,8 @@ module API success Entities::MergeRequestChanges end get ':id/merge_requests/:merge_request_iid/changes' do + not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project) + merge_request = find_merge_request_with_access(params[:merge_request_iid]) present merge_request, @@ -365,6 +374,8 @@ module API get ':id/merge_requests/:merge_request_iid/pipelines' do pipelines = merge_request_pipelines_with_access + not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project) + present paginate(pipelines), with: Entities::Ci::PipelineBasic end diff --git a/lib/api/todos.rb b/lib/api/todos.rb index 03850ba1c4e..afc1525cbe2 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -28,6 +28,11 @@ module API end post ":id/#{type}/:#{type_id_str}/todo" do issuable = instance_exec(params[type_id_str], &finder) + + unless can?(current_user, :read_merge_request, issuable.project) + not_found!(type.split("_").map(&:capitalize).join(" ")) + end + todo = TodoService.new.mark_todo(issuable, current_user).first if todo diff --git a/spec/requests/api/merge_request_approvals_spec.rb b/spec/requests/api/merge_request_approvals_spec.rb index fad5c3fb60e..b18f3017e03 100644 --- a/spec/requests/api/merge_request_approvals_spec.rb +++ b/spec/requests/api/merge_request_approvals_spec.rb @@ -21,6 +21,12 @@ RSpec.describe API::MergeRequestApprovals do expect(response).to have_gitlab_http_status(:ok) end + + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals" } + end + end end describe 'POST :id/merge_requests/:merge_request_iid/approve' do diff --git a/spec/requests/api/merge_request_diffs_spec.rb b/spec/requests/api/merge_request_diffs_spec.rb index 2e6cbe7bee7..971fb5e991c 100644 --- a/spec/requests/api/merge_request_diffs_spec.rb +++ b/spec/requests/api/merge_request_diffs_spec.rb @@ -35,6 +35,12 @@ RSpec.describe API::MergeRequestDiffs, 'MergeRequestDiffs' do get api("/projects/#{project.id}/merge_requests/0/versions", user) expect(response).to have_gitlab_http_status(:not_found) end + + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/versions" } + end + end end describe 'GET /projects/:id/merge_requests/:merge_request_iid/versions/:version_id' do @@ -63,5 +69,11 @@ RSpec.describe API::MergeRequestDiffs, 'MergeRequestDiffs' do get api("/projects/#{project.id}/merge_requests/#{non_existing_record_iid}/versions/#{merge_request_diff.id}", user) expect(response).to have_gitlab_http_status(:not_found) end + + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/versions/#{merge_request_diff.id}" } + end + end end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index e7005bd3ec5..b6c85e98bd0 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1087,6 +1087,12 @@ RSpec.describe API::MergeRequests do end end + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}" } + end + end + context 'merge_request_metrics' do let(:pipeline) { create(:ci_empty_pipeline) } @@ -1263,6 +1269,12 @@ RSpec.describe API::MergeRequests do it_behaves_like 'issuable participants endpoint' do let(:entity) { create(:merge_request, :simple, milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, source_branch: 'markdown', title: "Test", created_at: base_time) } end + + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/participants" } + end + end end describe 'GET /projects/:id/merge_requests/:merge_request_iid/commits' do @@ -1288,6 +1300,12 @@ RSpec.describe API::MergeRequests do expect(response).to have_gitlab_http_status(:not_found) end + + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/commits" } + end + end end describe 'GET /projects/:id/merge_requests/:merge_request_iid/:context_commits' do @@ -1363,6 +1381,12 @@ RSpec.describe API::MergeRequests do expect(response).to have_gitlab_http_status(:not_found) end + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes" } + end + end + it_behaves_like 'find an existing merge request' it_behaves_like 'accesses diffs via raw_diffs' @@ -1452,6 +1476,12 @@ RSpec.describe API::MergeRequests do expect(response).to have_gitlab_http_status(:forbidden) end end + + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/pipelines" } + end + end end describe 'POST /projects/:id/merge_requests/:merge_request_iid/pipelines' do diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index bc315c5b3c6..4a86453a09b 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -329,6 +329,14 @@ RSpec.describe API::Todos do expect(response).to have_gitlab_http_status(:not_found) end end + + it 'returns an error if the issuable author does not have access' do + project_1.add_guest(issuable.author) + + post api("/projects/#{project_1.id}/#{issuable_type}/#{issuable.iid}/todo", issuable.author) + + expect(response).to have_gitlab_http_status(:not_found) + end end describe 'POST :id/issuable_type/:issueable_id/todo' do diff --git a/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb b/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb new file mode 100644 index 00000000000..e6f9e5a434c --- /dev/null +++ b/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'rejects user from accessing merge request info' do + let(:project) { create(:project, :private) } + let(:merge_request) do + create(:merge_request, + author: user, + source_project: project, + target_project: project + ) + end + + before do + project.add_guest(user) + end + + it 'returns a 404 error' do + get api(url, user) + + expect(response).to have_gitlab_http_status(:not_found) + expect(json_response['message']).to eq('404 Merge Request Not Found') + end +end |