Prevent unauthorised comments on merge requests

* Prevent creating notes on inaccessible MRs

This applies the notes rules at the MR scope. Rather than adding extra
rules to the Project level policy, preventing :create_note here is
better since it only prevents creating notes on MRs.

* Prevent creating notes in inaccessible Issues

without this policy, non-team-members are allowed to comment on issues
even when the project has the private-issues policy set. This means that
without this change, users are allowed to comment on issues that they
cannot read.

* Add CHANGELOG entry

app/policies/issue_policy.rb

@@ -5,6 +5,8 @@ class IssuePolicy < IssuablePolicy
   # Make sure to sync this class checks with issue.rb to avoid security problems.
   # Check commit 002ad215818450d2cbbc5fa065850a953dc7ada8 for more information.
 
+  extend ProjectPolicy::ClassMethods
+
   desc "User can read confidential issues"
   condition(:can_read_confidential) do
     @user && IssueCollection.new([@subject]).visible_to(@user).any?
@@ -14,13 +16,12 @@ class IssuePolicy < IssuablePolicy
   condition(:confidential, scope: :subject) { @subject.confidential? }
 
   rule { confidential & ~can_read_confidential }.policy do
-    prevent :read_issue
+    prevent(*create_read_update_admin_destroy(:issue))
     prevent :read_issue_iid
-    prevent :update_issue
-    prevent :admin_issue
-    prevent :create_note
   end
 
+  rule { ~can?(:read_issue) }.prevent :create_note
+
   rule { locked }.policy do
     prevent :reopen_issue
   end

app/policies/merge_request_policy.rb

@@ -4,4 +4,10 @@ class MergeRequestPolicy < IssuablePolicy
   rule { locked }.policy do
     prevent :reopen_merge_request
   end
+
+  # Only users who can read the merge request can comment.
+  # Although :read_merge_request is computed in the policy context,
+  # it would not be safe to prevent :create_note there, since
+  # note permissions are shared, and this would apply too broadly.
+  rule { ~can?(:read_merge_request) }.prevent :create_note
 end

changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml

+---
+title: Ensure only authorised users can create notes on Merge Requests and Issues
+type: security

spec/policies/issue_policy_spec.rb

@@ -172,6 +172,34 @@ describe IssuePolicy do
       expect(permissions(assignee, issue_locked)).to be_disallowed(:admin_issue, :reopen_issue)
     end
 
+    context 'when issues are private' do
+      before do
+        project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE)
+      end
+      let(:issue) { create(:issue, project: project, author: author) }
+      let(:visitor) { create(:user) }
+      let(:admin) { create(:user, :admin) }
+
+      it 'forbids visitors from viewing issues' do
+        expect(permissions(visitor, issue)).to be_disallowed(:read_issue)
+      end
+      it 'forbids visitors from commenting' do
+        expect(permissions(visitor, issue)).to be_disallowed(:create_note)
+      end
+      it 'allows guests to view' do
+        expect(permissions(guest, issue)).to be_allowed(:read_issue)
+      end
+      it 'allows guests to comment' do
+        expect(permissions(guest, issue)).to be_allowed(:create_note)
+      end
+      it 'allows admins to view' do
+        expect(permissions(admin, issue)).to be_allowed(:read_issue)
+      end
+      it 'allows admins to comment' do
+        expect(permissions(admin, issue)).to be_allowed(:create_note)
+      end
+    end
+
     context 'with confidential issues' do
       let(:confidential_issue) { create(:issue, :confidential, project: project, assignees: [assignee], author: author) }
       let(:confidential_issue_no_assignee) { create(:issue, :confidential, project: project) }

spec/policies/merge_request_policy_spec.rb

@@ -6,6 +6,7 @@ describe MergeRequestPolicy do
   let(:guest) { create(:user) }
   let(:author) { create(:user) }
   let(:developer) { create(:user) }
+  let(:non_team_member) { create(:user) }
   let(:project) { create(:project, :public) }
 
   def permissions(user, merge_request)
@@ -18,6 +19,78 @@ describe MergeRequestPolicy do
     project.add_developer(developer)
   end
 
+  MR_PERMS = %i[create_merge_request_in
+                create_merge_request_from
+                read_merge_request
+                create_note].freeze
+
+  shared_examples_for 'a denied user' do
+    let(:perms) { permissions(subject, merge_request) }
+
+    MR_PERMS.each do |thing|
+      it "cannot #{thing}" do
+        expect(perms).to be_disallowed(thing)
+      end
+    end
+  end
+
+  shared_examples_for 'a user with access' do
+    let(:perms) { permissions(subject, merge_request) }
+
+    MR_PERMS.each do |thing|
+      it "can #{thing}" do
+        expect(perms).to be_allowed(thing)
+      end
+    end
+  end
+
+  context 'when merge requests have been disabled' do
+    let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
+
+    before do
+      project.project_feature.update(merge_requests_access_level: ProjectFeature::DISABLED)
+    end
+
+    describe 'the author' do
+      subject { author }
+      it_behaves_like 'a denied user'
+    end
+
+    describe 'a guest' do
+      subject { guest }
+      it_behaves_like 'a denied user'
+    end
+
+    describe 'a developer' do
+      subject { developer }
+      it_behaves_like 'a denied user'
+    end
+
+    describe 'any other user' do
+      subject { non_team_member }
+      it_behaves_like 'a denied user'
+    end
+  end
+
+  context 'when merge requests are private' do
+    let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) }
+
+    before do
+      project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+      project.project_feature.update(merge_requests_access_level: ProjectFeature::PRIVATE)
+    end
+
+    describe 'a non-team-member' do
+      subject { non_team_member }
+      it_behaves_like 'a denied user'
+    end
+
+    describe 'a developer' do
+      subject { developer }
+      it_behaves_like 'a user with access'
+    end
+  end
+
   context 'when merge request is unlocked' do
     let(:merge_request) { create(:merge_request, :closed, source_project: project, target_project: project, author: author) }
 
@@ -48,6 +121,22 @@ describe MergeRequestPolicy do
     it 'prevents guests from reopening merge request' do
       expect(permissions(guest, merge_request_locked)).to be_disallowed(:reopen_merge_request)
     end
+
+    context 'when the user is not a project member' do
+      let(:user) { create(:user) }
+
+      it 'cannot create a note' do
+        expect(permissions(user, merge_request_locked)).to be_disallowed(:create_note)
+      end
+    end
+
+    context 'when the user is project member, with at least guest access' do
+      let(:user) { guest }
+
+      it 'can create a note' do
+        expect(permissions(user, merge_request_locked)).to be_allowed(:create_note)
+      end
+    end
   end
 
   context 'with external authorization enabled' do