summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml5
-rw-r--r--changelogs/unreleased/security-fix-unauthenticated-lint.yml5
-rw-r--r--lib/api/lint.rb4
-rw-r--r--lib/api/merge_request_approvals.rb2
-rw-r--r--lib/api/merge_request_diffs.rb4
-rw-r--r--lib/api/merge_requests.rb11
-rw-r--r--lib/api/todos.rb5
-rw-r--r--spec/requests/api/lint_spec.rb168
-rw-r--r--spec/requests/api/merge_request_approvals_spec.rb6
-rw-r--r--spec/requests/api/merge_request_diffs_spec.rb12
-rw-r--r--spec/requests/api/merge_requests_spec.rb30
-rw-r--r--spec/requests/api/todos_spec.rb8
-rw-r--r--spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb23
13 files changed, 227 insertions, 56 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/changelogs/unreleased/security-fix-unauthenticated-lint.yml b/changelogs/unreleased/security-fix-unauthenticated-lint.yml
new file mode 100644
index 00000000000..94521ba7ec9
--- /dev/null
+++ b/changelogs/unreleased/security-fix-unauthenticated-lint.yml
@@ -0,0 +1,5 @@
+---
+title: Updates authorization for linting API
+merge_request:
+author:
+type: security
diff --git a/lib/api/lint.rb b/lib/api/lint.rb
index f1f34622187..2d30754a36d 100644
--- a/lib/api/lint.rb
+++ b/lib/api/lint.rb
@@ -11,6 +11,8 @@ module API
optional :include_merged_yaml, type: Boolean, desc: 'Whether or not to include merged CI config yaml in the response'
end
post '/lint' do
+ unauthorized! unless Gitlab::CurrentSettings.signup_enabled? && current_user
+
result = Gitlab::Ci::YamlProcessor.new(params[:content], user: current_user).execute
status 200
@@ -55,7 +57,7 @@ module API
optional :dry_run, type: Boolean, default: false, desc: 'Run pipeline creation simulation, or only do static check.'
end
post ':id/ci/lint' do
- authorize! :download_code, user_project
+ authorize! :create_pipeline, user_project
result = Gitlab::Ci::Lint
.new(project: user_project, current_user: current_user)
diff --git a/lib/api/merge_request_approvals.rb b/lib/api/merge_request_approvals.rb
index 00f42703731..0cdfd8f94b4 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 ab0e9b95e4a..142ecd0dc1e 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -246,6 +246,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,
@@ -262,7 +264,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
@@ -272,6 +277,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 =
@@ -353,6 +360,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,
@@ -368,6 +377,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/lint_spec.rb b/spec/requests/api/lint_spec.rb
index 2653653c896..2316e702c3e 100644
--- a/spec/requests/api/lint_spec.rb
+++ b/spec/requests/api/lint_spec.rb
@@ -4,91 +4,136 @@ require 'spec_helper'
RSpec.describe API::Lint do
describe 'POST /ci/lint' do
- context 'with valid .gitlab-ci.yaml content' do
- let(:yaml_content) do
- File.read(Rails.root.join('spec/support/gitlab_stubs/gitlab_ci.yml'))
- end
+ context 'when signup settings are disabled' do
+ Gitlab::CurrentSettings.signup_enabled = false
- it 'passes validation without warnings or errors' do
- post api('/ci/lint'), params: { content: yaml_content }
+ context 'when unauthenticated' do
+ it 'returns authentication error' do
+ post api('/ci/lint'), params: { content: 'content' }
- expect(response).to have_gitlab_http_status(:ok)
- expect(json_response).to be_an Hash
- expect(json_response['status']).to eq('valid')
- expect(json_response['warnings']).to eq([])
- expect(json_response['errors']).to eq([])
+ expect(response).to have_gitlab_http_status(:unauthorized)
+ end
end
- it 'outputs expanded yaml content' do
- post api('/ci/lint'), params: { content: yaml_content, include_merged_yaml: true }
+ context 'when authenticated' do
+ it 'returns unauthorized error' do
+ post api('/ci/lint'), params: { content: 'content' }
- expect(response).to have_gitlab_http_status(:ok)
- expect(json_response).to have_key('merged_yaml')
+ expect(response).to have_gitlab_http_status(:unauthorized)
+ end
end
end
- context 'with valid .gitlab-ci.yaml with warnings' do
- let(:yaml_content) { { job: { script: 'ls', rules: [{ when: 'always' }] } }.to_yaml }
+ context 'when signup settings are enabled' do
+ Gitlab::CurrentSettings.signup_enabled = true
- it 'passes validation but returns warnings' do
- post api('/ci/lint'), params: { content: yaml_content }
+ context 'when unauthenticated' do
+ it 'returns authentication error' do
+ post api('/ci/lint'), params: { content: 'content' }
- expect(response).to have_gitlab_http_status(:ok)
- expect(json_response['status']).to eq('valid')
- expect(json_response['warnings']).not_to be_empty
- expect(json_response['status']).to eq('valid')
- expect(json_response['errors']).to eq([])
+ expect(response).to have_gitlab_http_status(:unauthorized)
+ end
+ end
+
+ context 'when authenticated' do
+ let_it_be(:api_user) { create(:user) }
+ it 'returns authentication success' do
+ post api('/ci/lint', api_user), params: { content: 'content' }
+
+ expect(response).to have_gitlab_http_status(:ok)
+ end
end
end
- context 'with an invalid .gitlab_ci.yml' do
- context 'with invalid syntax' do
- let(:yaml_content) { 'invalid content' }
+ context 'when authenticated' do
+ let_it_be(:api_user) { create(:user) }
- it 'responds with errors about invalid syntax' do
- post api('/ci/lint'), params: { content: yaml_content }
+ context 'with valid .gitlab-ci.yaml content' do
+ let(:yaml_content) do
+ File.read(Rails.root.join('spec/support/gitlab_stubs/gitlab_ci.yml'))
+ end
+
+ it 'passes validation without warnings or errors' do
+ post api('/ci/lint', api_user), params: { content: yaml_content }
expect(response).to have_gitlab_http_status(:ok)
- expect(json_response['status']).to eq('invalid')
+ expect(json_response).to be_an Hash
+ expect(json_response['status']).to eq('valid')
expect(json_response['warnings']).to eq([])
- expect(json_response['errors']).to eq(['Invalid configuration format'])
+ expect(json_response['errors']).to eq([])
end
it 'outputs expanded yaml content' do
- post api('/ci/lint'), params: { content: yaml_content, include_merged_yaml: true }
+ post api('/ci/lint', api_user), params: { content: yaml_content, include_merged_yaml: true }
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to have_key('merged_yaml')
end
end
- context 'with invalid configuration' do
- let(:yaml_content) { '{ image: "ruby:2.7", services: ["postgres"], invalid }' }
+ context 'with valid .gitlab-ci.yaml with warnings' do
+ let(:yaml_content) { { job: { script: 'ls', rules: [{ when: 'always' }] } }.to_yaml }
- it 'responds with errors about invalid configuration' do
- post api('/ci/lint'), params: { content: yaml_content }
+ it 'passes validation but returns warnings' do
+ post api('/ci/lint', api_user), params: { content: yaml_content }
expect(response).to have_gitlab_http_status(:ok)
- expect(json_response['status']).to eq('invalid')
- expect(json_response['warnings']).to eq([])
- expect(json_response['errors']).to eq(['jobs invalid config should implement a script: or a trigger: keyword', 'jobs config should contain at least one visible job'])
+ expect(json_response['status']).to eq('valid')
+ expect(json_response['warnings']).not_to be_empty
+ expect(json_response['status']).to eq('valid')
+ expect(json_response['errors']).to eq([])
end
+ end
- it 'outputs expanded yaml content' do
- post api('/ci/lint'), params: { content: yaml_content, include_merged_yaml: true }
+ context 'with an invalid .gitlab_ci.yml' do
+ context 'with invalid syntax' do
+ let(:yaml_content) { 'invalid content' }
- expect(response).to have_gitlab_http_status(:ok)
- expect(json_response).to have_key('merged_yaml')
+ it 'responds with errors about invalid syntax' do
+ post api('/ci/lint', api_user), params: { content: yaml_content }
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response['status']).to eq('invalid')
+ expect(json_response['warnings']).to eq([])
+ expect(json_response['errors']).to eq(['Invalid configuration format'])
+ end
+
+ it 'outputs expanded yaml content' do
+ post api('/ci/lint', api_user), params: { content: yaml_content, include_merged_yaml: true }
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response).to have_key('merged_yaml')
+ end
+ end
+
+ context 'with invalid configuration' do
+ let(:yaml_content) { '{ image: "ruby:2.7", services: ["postgres"] }' }
+
+ it 'responds with errors about invalid configuration' do
+ post api('/ci/lint', api_user), params: { content: yaml_content }
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response['status']).to eq('invalid')
+ expect(json_response['warnings']).to eq([])
+ expect(json_response['errors']).to eq(['jobs config should contain at least one visible job'])
+ end
+
+ it 'outputs expanded yaml content' do
+ post api('/ci/lint', api_user), params: { content: yaml_content, include_merged_yaml: true }
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response).to have_key('merged_yaml')
+ end
end
end
- end
- context 'without the content parameter' do
- it 'responds with validation error about missing content' do
- post api('/ci/lint')
+ context 'without the content parameter' do
+ it 'responds with validation error about missing content' do
+ post api('/ci/lint', api_user)
- expect(response).to have_gitlab_http_status(:bad_request)
- expect(json_response['error']).to eq('content is missing')
+ expect(response).to have_gitlab_http_status(:bad_request)
+ expect(json_response['error']).to eq('content is missing')
+ end
end
end
end
@@ -364,6 +409,18 @@ RSpec.describe API::Lint do
expect(response).to have_gitlab_http_status(:not_found)
end
+
+ context 'when project is public' do
+ before do
+ project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ end
+
+ it 'returns authentication error' do
+ ci_lint
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
+ end
end
context 'when authenticated as non-member' do
@@ -387,13 +444,10 @@ RSpec.describe API::Lint do
context 'when running as dry run' do
let(:dry_run) { true }
- it 'returns pipeline creation error' do
+ it 'returns authentication error' do
ci_lint
- expect(response).to have_gitlab_http_status(:ok)
- expect(json_response['merged_yaml']).to eq(nil)
- expect(json_response['valid']).to eq(false)
- expect(json_response['errors']).to eq(['Insufficient permissions to create a new pipeline'])
+ expect(response).to have_gitlab_http_status(:forbidden)
end
end
@@ -410,7 +464,11 @@ RSpec.describe API::Lint do
)
end
- it_behaves_like 'valid project config'
+ it 'returns authentication error' do
+ ci_lint
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
end
end
end
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 3a3eae73932..a04867658e8 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -1226,6 +1226,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) }
@@ -1402,6 +1408,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
@@ -1427,6 +1439,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
@@ -1502,6 +1520,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'
@@ -1591,6 +1615,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 eaffa49fc9d..00de1ef5964 100644
--- a/spec/requests/api/todos_spec.rb
+++ b/spec/requests/api/todos_spec.rb
@@ -331,6 +331,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
View Lorry Controller Status