From d30a90a354f3dc015093d80f9de9dc15b38ff2a0 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <alex.kalderimis@gmail.com>
Date: Tue, 4 Jun 2019 23:30:54 -0400
Subject: [PATCH] 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                  |   9 +-
 app/policies/merge_request_policy.rb          |   6 +
 ...-60465-prevent-comments-on-private-mrs.yml |   3 +
 .../projects/notes_controller_spec.rb         | 311 ++++++++++++++----
 spec/policies/issue_policy_spec.rb            |  28 ++
 spec/policies/merge_request_policy_spec.rb    |  89 +++++
 6 files changed, 371 insertions(+), 75 deletions(-)
 create mode 100644 changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml

diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb
index dd8c5d49cf4..fa252af55e4 100644
--- a/app/policies/issue_policy.rb
+++ b/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
diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb
index a3692857ff4..5ad7bdabdff 100644
--- a/app/policies/merge_request_policy.rb
+++ b/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
diff --git a/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml b/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml
new file mode 100644
index 00000000000..ba970162447
--- /dev/null
+++ b/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml
@@ -0,0 +1,3 @@
+---
+title: Ensure only authorised users can create notes on Merge Requests and Issues
+type: security
diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index 9ab565dc2e8..99808dce016 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -212,40 +212,232 @@ describe Projects::NotesController do
   describe 'POST create' do
     let(:merge_request) { create(:merge_request) }
     let(:project) { merge_request.source_project }
+    let(:note_text) { 'some note' }
     let(:request_params) do
       {
-        note: { note: 'some note', noteable_id: merge_request.id, noteable_type: 'MergeRequest' },
+        note: { note: note_text, noteable_id: merge_request.id, noteable_type: 'MergeRequest' },
         namespace_id: project.namespace,
         project_id: project,
         merge_request_diff_head_sha: 'sha',
         target_type: 'merge_request',
         target_id: merge_request.id
-      }
+      }.merge(extra_request_params)
+    end
+    let(:extra_request_params) { {} }
+
+    let(:project_visibility) { Gitlab::VisibilityLevel::PUBLIC }
+    let(:merge_requests_access_level) { ProjectFeature::ENABLED }
+
+    def create!
+      post :create, params: request_params
     end
 
     before do
+      project.update_attribute(:visibility_level, project_visibility)
+      project.project_feature.update(merge_requests_access_level: merge_requests_access_level)
       sign_in(user)
-      project.add_developer(user)
     end
 
-    it "returns status 302 for html" do
-      post :create, params: request_params
+    describe 'making the creation request' do
+      before do
+        create!
+      end
 
-      expect(response).to have_gitlab_http_status(302)
+      context 'the project is publically available' do
+        context 'for HTML' do
+          it "returns status 302" do
+            expect(response).to have_gitlab_http_status(302)
+          end
+        end
+
+        context 'for JSON' do
+          let(:extra_request_params) { { format: :json } }
+
+          it "returns status 200 for json" do
+            expect(response).to have_gitlab_http_status(200)
+          end
+        end
+      end
+
+      context 'the project is a private project' do
+        let(:project_visibility) { Gitlab::VisibilityLevel::PRIVATE }
+
+        [{}, { format: :json }].each do |extra|
+          context "format is #{extra[:format]}" do
+            let(:extra_request_params) { extra }
+
+            it "returns status 404" do
+              expect(response).to have_gitlab_http_status(404)
+            end
+          end
+        end
+      end
     end
 
-    it "returns status 200 for json" do
-      post :create, params: request_params.merge(format: :json)
+    context 'the user is a developer on a private project' do
+      let(:project_visibility) { Gitlab::VisibilityLevel::PRIVATE }
 
-      expect(response).to have_gitlab_http_status(200)
+      before do
+        project.add_developer(user)
+      end
+
+      context 'HTML requests' do
+        it "returns status 302 (redirect)" do
+          create!
+
+          expect(response).to have_gitlab_http_status(302)
+        end
+      end
+
+      context 'JSON requests' do
+        let(:extra_request_params) { { format: :json } }
+
+        it "returns status 200" do
+          create!
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+
+      context 'the return_discussion param is set' do
+        let(:extra_request_params) { { format: :json, return_discussion: 'true' } }
+
+        it 'returns discussion JSON when the return_discussion param is set' do
+          create!
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response).to have_key 'discussion'
+          expect(json_response.dig('discussion', 'notes', 0, 'note')).to eq(request_params[:note][:note])
+        end
+      end
+
+      context 'when creating a note with quick actions' do
+        context 'with commands that return changes' do
+          let(:note_text) { "/award :thumbsup:\n/estimate 1d\n/spend 3h" }
+          let(:extra_request_params) { { format: :json } }
+
+          it 'includes changes in commands_changes ' do
+            create!
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['commands_changes']).to include('emoji_award', 'time_estimate', 'spend_time')
+            expect(json_response['commands_changes']).not_to include('target_project', 'title')
+          end
+        end
+
+        context 'with commands that do not return changes' do
+          let(:issue) { create(:issue, project: project) }
+          let(:other_project) { create(:project) }
+          let(:note_text) { "/move #{other_project.full_path}\n/title AAA" }
+          let(:extra_request_params) { { format: :json, target_id: issue.id, target_type: 'issue' } }
+
+          before do
+            other_project.add_developer(user)
+          end
+
+          it 'does not include changes in commands_changes' do
+            create!
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(json_response['commands_changes']).not_to include('target_project', 'title')
+          end
+        end
+      end
     end
 
-    it 'returns discussion JSON when the return_discussion param is set' do
-      post :create, params: request_params.merge(format: :json, return_discussion: 'true')
+    context 'when the internal project prohibits non-members from accessing merge requests' do
+      let(:project_visibility) { Gitlab::VisibilityLevel::INTERNAL }
+      let(:merge_requests_access_level) { ProjectFeature::PRIVATE }
 
-      expect(response).to have_gitlab_http_status(200)
-      expect(json_response).to have_key 'discussion'
-      expect(json_response['discussion']['notes'][0]['note']).to eq(request_params[:note][:note])
+      it "prevents a non-member user from creating a note on one of the project's merge requests" do
+        create!
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+
+      context 'when the user is a team member' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'can add comments' do
+          expect { create! }.to change { project.notes.count }.by(1)
+        end
+      end
+
+      # Illustration of the attack vector for posting comments to discussions that should
+      # be inaccessible.
+      #
+      # This relies on posting a note to a commit that is not necessarily even in the
+      # merge request, with a value of :in_reply_to_discussion_id that points to a
+      # discussion on a merge_request that should not be accessible.
+      context 'when the request includes a :in_reply_to_discussion_id designed to fool us' do
+        let(:commit) { create(:commit, project: project) }
+
+        let(:existing_comment) do
+          create(:note_on_commit,
+                 note: 'first',
+                 project: project,
+                 commit_id: merge_request.commit_shas.first)
+        end
+
+        let(:discussion) { existing_comment.discussion }
+
+        # see !60465 for details of the structure of this request
+        let(:request_params) do
+          { "utf8" => "✓",
+            "authenticity_token" => "1",
+            "view" => "inline",
+            "line_type" => "",
+            "merge_request_diff_head_sha" => "",
+            "in_reply_to_discussion_id" => discussion.id,
+            "note_project_id" => project.id,
+            "project_id" => project.id,
+            "namespace_id" => project.namespace,
+            "target_type" => "commit",
+            "target_id" => commit.id,
+            "note" => {
+              "noteable_type" => "",
+              "noteable_id" => "",
+              "commit_id" => "",
+              "type" => "",
+              "line_code" => "",
+              "position" => "",
+              "note" => "ThisReplyWillGoToMergeRequest"
+            } }
+        end
+
+        it 'prevents the request from adding notes to the spoofed discussion' do
+          expect { create! }.not_to change { discussion.notes.count }
+        end
+
+        it 'returns an error to the user' do
+          create!
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
+
+    context 'when the public project prohibits non-members from accessing merge requests' do
+      let(:project_visibility) { Gitlab::VisibilityLevel::PUBLIC }
+      let(:merge_requests_access_level) { ProjectFeature::PRIVATE }
+
+      it "prevents a non-member user from creating a note on one of the project's merge requests" do
+        create!
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+
+      context 'when the user is a team member' do
+        before do
+          project.add_developer(user)
+          create!
+        end
+
+        it 'can add comments' do
+          expect(response).to be_redirect
+        end
+      end
     end
 
     context 'when merge_request_diff_head_sha present' do
@@ -262,7 +454,7 @@ describe Projects::NotesController do
       end
 
       it "returns status 302 for html" do
-        post :create, params: request_params
+        create!
 
         expect(response).to have_gitlab_http_status(302)
       end
@@ -285,7 +477,7 @@ describe Projects::NotesController do
     end
 
     context 'when creating a commit comment from an MR fork' do
-      let(:project) { create(:project, :repository) }
+      let(:project) { create(:project, :repository, :public) }
 
       let(:forked_project) do
         fork_project(project, nil, repository: true)
@@ -299,45 +491,59 @@ describe Projects::NotesController do
         create(:note_on_commit, note: 'a note', project: forked_project, commit_id: merge_request.commit_shas.first)
       end
 
-      def post_create(extra_params = {})
-        post :create, params: {
+      let(:note_project_id) do
+        forked_project.id
+      end
+
+      let(:request_params) do
+        {
           note: { note: 'some other note', noteable_id: merge_request.id },
           namespace_id: project.namespace,
           project_id: project,
           target_type: 'merge_request',
           target_id: merge_request.id,
-          note_project_id: forked_project.id,
+          note_project_id: note_project_id,
           in_reply_to_discussion_id: existing_comment.discussion_id
-        }.merge(extra_params)
+        }
+      end
+
+      let(:fork_visibility) { Gitlab::VisibilityLevel::PUBLIC }
+
+      before do
+        forked_project.update_attribute(:visibility_level, fork_visibility)
       end
 
       context 'when the note_project_id is not correct' do
-        it 'returns a 404' do
-          post_create(note_project_id: Project.maximum(:id).succ)
+        let(:note_project_id) do
+          project.id && Project.maximum(:id).succ
+        end
 
+        it 'returns a 404' do
+          create!
           expect(response).to have_gitlab_http_status(404)
         end
       end
 
       context 'when the user has no access to the fork' do
-        it 'returns a 404' do
-          post_create
+        let(:fork_visibility) { Gitlab::VisibilityLevel::PRIVATE }
 
+        it 'returns a 404' do
+          create!
           expect(response).to have_gitlab_http_status(404)
         end
       end
 
       context 'when the user has access to the fork' do
-        let(:discussion) { forked_project.notes.find_discussion(existing_comment.discussion_id) }
+        let!(:discussion) { forked_project.notes.find_discussion(existing_comment.discussion_id) }
+        let(:fork_visibility) { Gitlab::VisibilityLevel::PUBLIC }
 
-        before do
-          forked_project.add_developer(user)
-
-          existing_comment
+        it 'is successful' do
+          create!
+          expect(response).to have_gitlab_http_status(302)
         end
 
         it 'creates the note' do
-          expect { post_create }.to change { forked_project.notes.count }.by(1)
+          expect { create! }.to change { forked_project.notes.count }.by(1)
         end
       end
     end
@@ -346,11 +552,6 @@ describe Projects::NotesController do
       let(:locked_issue) { create(:issue, :locked, project: project) }
       let(:issue) {create(:issue, project: project)}
 
-      before do
-        project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
-        project.project_member(user).destroy
-      end
-
       it 'uses target_id and ignores noteable_id' do
         request_params = {
           note: { note: 'some note', noteable_type: 'Issue', noteable_id: locked_issue.id },
@@ -368,7 +569,6 @@ describe Projects::NotesController do
 
     context 'when the merge request discussion is locked' do
       before do
-        project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
         merge_request.update_attribute(:discussion_locked, true)
       end
 
@@ -382,6 +582,10 @@ describe Projects::NotesController do
       end
 
       context 'when a user is a team member' do
+        before do
+          project.add_developer(user)
+        end
+
         it 'returns 302 status for html' do
           post :create, params: request_params
 
@@ -400,10 +604,6 @@ describe Projects::NotesController do
       end
 
       context 'when a user is not a team member' do
-        before do
-          project.project_member(user).destroy
-        end
-
         it 'returns 404 status' do
           post :create, params: request_params
 
@@ -415,37 +615,6 @@ describe Projects::NotesController do
         end
       end
     end
-
-    context 'when creating a note with quick actions' do
-      context 'with commands that return changes' do
-        let(:note_text) { "/award :thumbsup:\n/estimate 1d\n/spend 3h" }
-
-        it 'includes changes in commands_changes ' do
-          post :create, params: request_params.merge(note: { note: note_text }, format: :json)
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response['commands_changes']).to include('emoji_award', 'time_estimate', 'spend_time')
-          expect(json_response['commands_changes']).not_to include('target_project', 'title')
-        end
-      end
-
-      context 'with commands that do not return changes' do
-        let(:issue) { create(:issue, project: project) }
-        let(:other_project) { create(:project) }
-        let(:note_text) { "/move #{other_project.full_path}\n/title AAA" }
-
-        before do
-          other_project.add_developer(user)
-        end
-
-        it 'does not include changes in commands_changes' do
-          post :create, params: request_params.merge(note: { note: note_text }, target_type: 'issue', target_id: issue.id, format: :json)
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response['commands_changes']).not_to include('target_project', 'title')
-        end
-      end
-    end
   end
 
   describe 'PUT update' do
diff --git a/spec/policies/issue_policy_spec.rb b/spec/policies/issue_policy_spec.rb
index b149dbcf871..25267d36ab8 100644
--- a/spec/policies/issue_policy_spec.rb
+++ b/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) }
diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb
index 81279225d61..87205f56589 100644
--- a/spec/policies/merge_request_policy_spec.rb
+++ b/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