summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/controllers/projects/commit_controller.rb6
-rw-r--r--app/finders/merge_requests_finder.rb9
-rw-r--r--changelogs/unreleased/security-commit-private-related-mr.yml5
-rw-r--r--lib/api/commits.rb10
-rw-r--r--spec/finders/merge_requests_finder_spec.rb26
-rw-r--r--spec/requests/api/commits_spec.rb15
6 files changed, 65 insertions, 6 deletions
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index b13c0ae3967..939a09d4fd2 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -65,7 +65,11 @@ class Projects::CommitController < Projects::ApplicationController
# rubocop: enable CodeReuse/ActiveRecord
def merge_requests
- @merge_requests = @commit.merge_requests.map do |mr|
+ @merge_requests = MergeRequestsFinder.new(
+ current_user,
+ project_id: @project.id,
+ commit_sha: @commit.sha
+ ).execute.map do |mr|
{ iid: mr.iid, path: merge_request_path(mr), title: mr.title }
end
diff --git a/app/finders/merge_requests_finder.rb b/app/finders/merge_requests_finder.rb
index b645011a3c5..93bee3f1488 100644
--- a/app/finders/merge_requests_finder.rb
+++ b/app/finders/merge_requests_finder.rb
@@ -37,13 +37,20 @@ class MergeRequestsFinder < IssuableFinder
end
def filter_items(_items)
- items = by_source_branch(super)
+ items = by_commit(super)
+ items = by_source_branch(items)
items = by_wip(items)
by_target_branch(items)
end
private
+ def by_commit(items)
+ return items unless params[:commit_sha].presence
+
+ items.by_commit_sha(params[:commit_sha])
+ end
+
def source_branch
@source_branch ||= params[:source_branch].presence
end
diff --git a/changelogs/unreleased/security-commit-private-related-mr.yml b/changelogs/unreleased/security-commit-private-related-mr.yml
new file mode 100644
index 00000000000..c4de200b0d8
--- /dev/null
+++ b/changelogs/unreleased/security-commit-private-related-mr.yml
@@ -0,0 +1,5 @@
+---
+title: Don't allow non-members to see private related MRs.
+merge_request:
+author:
+type: security
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index 8defc59224d..d0a9debda5b 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -318,10 +318,18 @@ module API
use :pagination
end
get ':id/repository/commits/:sha/merge_requests', requirements: API::COMMIT_ENDPOINT_REQUIREMENTS do
+ authorize! :read_merge_request, user_project
+
commit = user_project.commit(params[:sha])
not_found! 'Commit' unless commit
- present paginate(commit.merge_requests), with: Entities::MergeRequestBasic
+ commit_merge_requests = MergeRequestsFinder.new(
+ current_user,
+ project_id: user_project.id,
+ commit_sha: commit.sha
+ ).execute
+
+ present paginate(commit_merge_requests), with: Entities::MergeRequestBasic
end
desc "Get a commit's GPG signature" do
diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb
index 107da08a0a9..79f854cdb96 100644
--- a/spec/finders/merge_requests_finder_spec.rb
+++ b/spec/finders/merge_requests_finder_spec.rb
@@ -31,7 +31,7 @@ describe MergeRequestsFinder do
p
end
end
- let(:project4) { create_project_without_n_plus_1(group: subgroup) }
+ let(:project4) { create_project_without_n_plus_1(:repository, group: subgroup) }
let(:project5) { create_project_without_n_plus_1(group: subgroup) }
let(:project6) { create_project_without_n_plus_1(group: subgroup) }
@@ -68,6 +68,15 @@ describe MergeRequestsFinder do
expect(merge_requests.size).to eq(2)
end
+ it 'filters by commit sha' do
+ merge_requests = described_class.new(
+ user,
+ commit_sha: merge_request5.merge_request_diff.last_commit_sha
+ ).execute
+
+ expect(merge_requests).to contain_exactly(merge_request5)
+ end
+
context 'filtering by group' do
it 'includes all merge requests when user has access' do
params = { group_id: group.id }
@@ -269,6 +278,21 @@ describe MergeRequestsFinder do
expect(merge_requests).to contain_exactly(old_merge_request, new_merge_request)
end
end
+
+ context 'when project restricts merge requests' do
+ let(:non_member) { create(:user) }
+ let(:project) { create(:project, :repository, :public, :merge_requests_private) }
+ let!(:merge_request) { create(:merge_request, source_project: project) }
+
+ it "returns nothing to to non members" do
+ merge_requests = described_class.new(
+ non_member,
+ project_id: project.id
+ ).execute
+
+ expect(merge_requests).to be_empty
+ end
+ end
end
describe '#row_count', :request_store do
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index 066f1d6862a..a132b85b878 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -1430,8 +1430,8 @@ describe API::Commits do
end
describe 'GET /projects/:id/repository/commits/:sha/merge_requests' do
- let!(:project) { create(:project, :repository, :private) }
- let!(:merged_mr) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'feature') }
+ let(:project) { create(:project, :repository, :private) }
+ let(:merged_mr) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'feature') }
let(:commit) { merged_mr.merge_request_diff.commits.last }
it 'returns the correct merge request' do
@@ -1456,6 +1456,17 @@ describe API::Commits do
expect(response).to have_gitlab_http_status(404)
end
+
+ context 'public project' do
+ let(:project) { create(:project, :repository, :public, :merge_requests_private) }
+ let(:non_member) { create(:user) }
+
+ it 'responds 403 when only members are allowed to read merge requests' do
+ get api("/projects/#{project.id}/repository/commits/#{commit.id}/merge_requests", non_member)
+
+ expect(response).to have_gitlab_http_status(403)
+ end
+ end
end
describe 'GET /projects/:id/repository/commits/:sha/signature' do