Merge branch 'fix-guest-access-posting-to-notes' into 'security'

Prevent users from creating notes on resources they can't access

See https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2054

3 files changed, 32 insertions, 10 deletions

diff --git a/changelogs/unreleased/fix-guest-access-posting-to-notes.yml b/changelogs/unreleased/fix-guest-access-posting-to-notes.yml
new file mode 100644
index 00000000000..81377c0c6f0
--- /dev/null
+++ b/changelogs/unreleased/fix-guest-access-posting-to-notes.yml
@@ -0,0 +1,4 @@
+---
+title: Prevent users from creating notes on resources they can't access
+merge_request:
+author:
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index c5c214d4d13..ea57aa8e320 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -74,21 +74,27 @@ module API
           required_attributes! [:body]
 
           opts = {
-           note: params[:body],
-           noteable_type: noteables_str.classify,
-           noteable_id: params[noteable_id_str]
+            note: params[:body],
+            noteable_type: noteables_str.classify,
+            noteable_id: params[noteable_id_str]
           }
 
-          if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user)
-            opts[:created_at] = params[:created_at]
-          end
+          noteable = user_project.send(noteables_str.to_sym).find(opts[:noteable_id])
+
+          if can?(current_user, noteable_read_ability_name(noteable), noteable)
+            if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user)
+              opts[:created_at] = params[:created_at]
+            end
 
-          note = ::Notes::CreateService.new(user_project, current_user, opts).execute
+            note = ::Notes::CreateService.new(user_project, current_user, opts).execute
 
-          if note.valid?
-            present note, with: Entities::const_get(note.class.name)
+            if note.valid?
+              present note, with: Entities::const_get(note.class.name)
+            else
+              not_found!("Note #{note.errors.messages}")
+            end
           else
-            not_found!("Note #{note.errors.messages}")
+            not_found!("Note")
           end
         end
 
diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index d58bedc3bf7..ecc0634e906 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -253,6 +253,18 @@ describe API::API, api: true  do
       end
     end
 
+    context 'when user does not have access to read the noteable' do
+      it 'responds with 404' do
+        project = create(:empty_project, :private) { |p| p.team << [user, :guest] }
+        issue = create(:issue, :confidential, project: project)
+
+        post api("/projects/#{project.id}/issues/#{issue.id}/notes", user),
+          body: 'Foo'
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
     context 'when user does not have access to create noteable' do
       let(:private_issue) { create(:issue, project: create(:project, :private)) }