Fixed permissions in comments

When creating comments, sending different noteable IDs for target_id and
note[:noteable_id] would allow you to bypass comment creation security
if the user had creation permissions for target_id. The comment would be
created in note[:noteable_id].

Also made it so that users cannot edit/delete their comments on a
noteable that becomes unreadable to them (if it gets flagged
confidential and they don't have read access for example)

app/controllers/concerns/notes_actions.rb

@@ -6,6 +6,7 @@ module NotesActions
   extend ActiveSupport::Concern
 
   included do
+    prepend_before_action :normalize_create_params, only: [:create]
     before_action :set_polling_interval_header, only: [:index]
     before_action :require_noteable!, only: [:index, :create]
     before_action :authorize_admin_note!, only: [:update, :destroy]
@@ -247,6 +248,15 @@ module NotesActions
     DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: ProjectNoteEntity)
   end
 
+  # Avoids checking permissions in the wrong object - this ensures that the object we checked permissions for
+  # is the object we're actually creating a note in.
+  def normalize_create_params
+    params[:note].try do |note|
+      note[:noteable_id] = params[:target_id]
+      note[:noteable_type] = params[:target_type].classify
+    end
+  end
+
   def note_project
     strong_memoize(:note_project) do
       next nil unless project

app/mailers/emails/notes.rb

@@ -28,7 +28,7 @@ module Emails
       mail_answer_note_thread(@merge_request, @note, note_thread_options(recipient_id))
     end
 
-    def note_snippet_email(recipient_id, note_id)
+    def note_project_snippet_email(recipient_id, note_id)
       setup_note_mail(note_id, recipient_id)
 
       @snippet = @note.noteable

app/models/note.rb

@@ -324,7 +324,7 @@ class Note < ActiveRecord::Base
   end
 
   def to_ability_name
-    for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore
+    for_snippet? ? noteable.class.name.underscore : noteable_type.underscore
   end
 
   def can_be_discussion_note?

app/policies/commit_policy.rb

@@ -2,4 +2,6 @@
 
 class CommitPolicy < BasePolicy
   delegate { @subject.project }
+
+  rule { can?(:download_code) }.enable :read_commit
 end

app/policies/note_policy.rb

@@ -9,8 +9,17 @@ class NotePolicy < BasePolicy
 
   condition(:editable, scope: :subject) { @subject.editable? }
 
+  condition(:can_read_noteable) { can?(:"read_#{@subject.to_ability_name}") }
+
   rule { ~editable }.prevent :admin_note
 
+  # If user can't read the issue/MR/etc then they should not be allowed to do anything to their own notes
+  rule { ~can_read_noteable }.policy do
+    prevent :read_note
+    prevent :admin_note
+    prevent :resolve_note
+  end
+
   rule { is_author }.policy do
     enable :read_note
     enable :admin_note

changelogs/unreleased/security-guest-comments.yml

+---
+title: Fixed ability to comment on locked/confidential issues.
+merge_request:
+author:
+type: security

changelogs/unreleased/security-guest-comments_2.yml

+---
+title: Fixed ability of guest users to edit/delete comments on locked or confidential issues.
+merge_request:
+author:
+type: security

ee/app/controllers/groups/epics/notes_controller.rb

@@ -3,6 +3,10 @@ class Groups::Epics::NotesController < Groups::ApplicationController
   include NotesHelper
   include ToggleAwardEmoji
 
+  # Re-defining the before_action set in NotesActions here without prepending pushes it farther down the callback stack
+  # and we do this here since we need variables instantiated in other before_actions
+  before_action :normalize_create_params, only: [:create]
+
   before_action :epic
   before_action :authorize_create_note!, only: [:create]
 
@@ -39,4 +43,11 @@ class Groups::Epics::NotesController < Groups::ApplicationController
   def note_serializer
     EpicNoteSerializer.new(project: nil, noteable: noteable, current_user: current_user)
   end
+
+  def normalize_create_params
+    params[:note].try do |note|
+      note[:noteable_id] = epic.id
+      note[:noteable_type] = 'Epic'
+    end
+  end
 end

spec/controllers/projects/notes_controller_spec.rb

@@ -283,14 +283,14 @@ describe Projects::NotesController do
 
       def post_create(extra_params = {})
         post :create, {
-               note: { note: 'some other note' },
-               namespace_id: project.namespace,
-               project_id: project,
-               target_type: 'merge_request',
-               target_id: merge_request.id,
-               note_project_id: forked_project.id,
-               in_reply_to_discussion_id: existing_comment.discussion_id
-             }.merge(extra_params)
+          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,
+          in_reply_to_discussion_id: existing_comment.discussion_id
+        }.merge(extra_params)
       end
 
       context 'when the note_project_id is not correct' do
@@ -324,6 +324,30 @@ describe Projects::NotesController do
       end
     end
 
+    context 'when target_id and noteable_id do not match' 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 },
+          target_type: 'issue',
+          target_id: issue.id,
+          project_id: project,
+          namespace_id: project.namespace
+        }
+
+        expect { post :create, request_params }.to change { issue.notes.count }.by(1)
+          .and change { locked_issue.notes.count }.by(0)
+        expect(response).to have_gitlab_http_status(302)
+      end
+    end
+
     context 'when the merge request discussion is locked' do
       before do
         project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
@@ -376,35 +400,60 @@ describe Projects::NotesController do
   end
 
   describe 'PUT update' do
-    let(:request_params) do
-      {
-        namespace_id: project.namespace,
-        project_id: project,
-        id: note,
-        format: :json,
-        note: {
-          note: "New comment"
+    context "should update the note with a valid issue" do
+      let(:request_params) do
+        {
+          namespace_id: project.namespace,
+          project_id: project,
+          id: note,
+          format: :json,
+          note: {
+            note: "New comment"
+          }
         }
-      }
-    end
+      end
 
-    before do
-      sign_in(note.author)
-      project.add_developer(note.author)
+      before do
+        sign_in(note.author)
+        project.add_developer(note.author)
+      end
+
+      it "updates the note" do
+        expect { put :update, request_params }.to change { note.reload.note }
+      end
     end
+    context "doesnt update the note" do
+      let(:issue)   { create(:issue, :confidential, project: project) }
+      let(:note)    { create(:note, noteable: issue, project: project) }
 
-    it "updates the note" do
-      expect { put :update, request_params }.to change { note.reload.note }
+      before do
+        sign_in(user)
+        project.add_guest(user)
+      end
+
+      it "disallows edits when the issue is confidential and the user has guest permissions" do
+        request_params = {
+          namespace_id: project.namespace,
+          project_id: project,
+          id: note,
+          format: :json,
+          note: {
+            note: "New comment"
+          }
+        }
+        expect { put :update, request_params }.not_to change { note.reload.note }
+        expect(response).to have_gitlab_http_status(404)
+      end
     end
   end
 
   describe 'DELETE destroy' do
     let(:request_params) do
       {
-        namespace_id: project.namespace,
-        project_id: project,
-        id: note,
-        format: :js
+          namespace_id: project.namespace,
+          project_id: project,
+          id: note,
+          format: :js
       }
     end
 

spec/helpers/issuables_helper_spec.rb

@@ -212,7 +212,7 @@ describe IssuablesHelper do
         issuableRef: "&#{epic.iid}",
         markdownPreviewPath: "/groups/#{@group.full_path}/preview_markdown",
         markdownDocsPath: '/help/user/markdown',
-        markdownVersion: 11,
+        markdownVersion: CacheMarkdownField::CACHE_COMMONMARK_VERSION,
         issuableTemplates: nil,
         groupPath: @group.path,
         initialTitleHtml: epic.title,

spec/mailers/notify_spec.rb

@@ -522,7 +522,7 @@ describe Notify do
       let(:project_snippet) { create(:project_snippet, project: project) }
       let(:project_snippet_note) { create(:note_on_project_snippet, project: project, noteable: project_snippet) }
 
-      subject { described_class.note_snippet_email(project_snippet_note.author_id, project_snippet_note.id) }
+      subject { described_class.note_project_snippet_email(project_snippet_note.author_id, project_snippet_note.id) }
 
       it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
         let(:model) { project_snippet }

spec/models/note_spec.rb

@@ -517,7 +517,7 @@ describe Note do
 
   describe '#to_ability_name' do
     it 'returns snippet for a project snippet note' do
-      expect(build(:note_on_project_snippet).to_ability_name).to eq('snippet')
+      expect(build(:note_on_project_snippet).to_ability_name).to eq('project_snippet')
     end
 
     it 'returns personal_snippet for a personal snippet note' do

spec/policies/note_policy_spec.rb

@@ -10,11 +10,50 @@ describe NotePolicy, mdoels: true do
       return @policies if @policies
 
       noteable ||= issue
-      note = create(:note, noteable: noteable, author: user, project: project)
+      note = if noteable.is_a?(Commit)
+               create(:note_on_commit, commit_id: noteable.id, author: user, project: project)
+             else
+               create(:note, noteable: noteable, author: user, project: project)
+             end
 
       @policies = described_class.new(user, note)
     end
 
+    shared_examples_for 'a discussion with a private noteable' do
+      let(:noteable) { issue }
+      let(:policy) { policies(noteable) }
+
+      context 'when the note author can no longer see the noteable' do
+        it 'can not edit nor read the note' do
+          expect(policy).to be_disallowed(:admin_note)
+          expect(policy).to be_disallowed(:resolve_note)
+          expect(policy).to be_disallowed(:read_note)
+        end
+      end
+
+      context 'when the note author can still see the noteable' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'can edit the note' do
+          expect(policy).to be_allowed(:admin_note)
+          expect(policy).to be_allowed(:resolve_note)
+          expect(policy).to be_allowed(:read_note)
+        end
+      end
+    end
+
+    context 'when the project is private' do
+      let(:project) { create(:project, :private, :repository) }
+
+      context 'when the noteable is a commit' do
+        it_behaves_like 'a discussion with a private noteable' do
+          let(:noteable) { project.repository.head_commit }
+        end
+      end
+    end
+
     context 'when the project is public' do
       context 'when the note author is not a project member' do
         it 'can edit a note' do
@@ -24,14 +63,48 @@ describe NotePolicy, mdoels: true do
         end
       end
 
-      context 'when the noteable is a snippet' do
+      context 'when the noteable is a project snippet' do
+        it 'can edit note' do
+          policies = policies(create(:project_snippet, :public, project: project))
+
+          expect(policies).to be_allowed(:admin_note)
+          expect(policies).to be_allowed(:resolve_note)
+          expect(policies).to be_allowed(:read_note)
+        end
+
+        context 'when it is private' do
+          it_behaves_like 'a discussion with a private noteable' do
+            let(:noteable) { create(:project_snippet, :private, project: project) }
+          end
+        end
+      end
+
+      context 'when the noteable is a personal snippet' do
         it 'can edit note' do
-          policies = policies(create(:project_snippet, project: project))
+          policies = policies(create(:personal_snippet, :public))
 
           expect(policies).to be_allowed(:admin_note)
           expect(policies).to be_allowed(:resolve_note)
           expect(policies).to be_allowed(:read_note)
         end
+
+        context 'when it is private' do
+          it 'can not edit nor read the note' do
+            policies = policies(create(:personal_snippet, :private))
+
+            expect(policies).to be_disallowed(:admin_note)
+            expect(policies).to be_disallowed(:resolve_note)
+            expect(policies).to be_disallowed(:read_note)
+          end
+        end
+      end
+
+      context 'when a discussion is confidential' do
+        before do
+          issue.update_attribute(:confidential, true)
+        end
+
+        it_behaves_like 'a discussion with a private noteable'
       end
 
       context 'when a discussion is locked' do