Merge branch 'security-commit-private-related-mr-11-8' into '11-8-stable'

Don't allow non-members to see private related MRs

See merge request gitlab/gitlabhq!2930

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

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

changelogs/unreleased/security-commit-private-related-mr.yml

+---
+title: Don't allow non-members to see private related MRs.
+merge_request:
+author:
+type: security

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
     end
   end

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

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,5 +1456,16 @@ 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
 end