Correctly check permissions when creating snippet notes

In the Snippets::NotesController the noteable was resolved and
authorized through the :snippet_id, so by passing a :target_id for a
different snippet it was possible to create a note on a snippet
where the user would be unauthorized to do so otherwise.

This fixes the problem by ignoring the :target_id and :target_type from
the request, and using the same noteable for creation and authorization.

app/controllers/concerns/notes_actions.rb

@@ -203,17 +203,17 @@ module NotesActions
 
       # These params are also sent by the client but we need to set these based on
       # target_type and target_id because we're checking permissions based on that
-      create_params[:noteable_type] = params[:target_type].classify
+      create_params[:noteable_type] = noteable.class.name
 
-      case params[:target_type]
-      when 'commit'
-        create_params[:commit_id] = params[:target_id]
-      when 'merge_request'
-        create_params[:noteable_id] = params[:target_id]
+      case noteable
+      when Commit
+        create_params[:commit_id] = noteable.id
+      when MergeRequest
+        create_params[:noteable_id] = noteable.id
         # Notes on MergeRequest can have an extra `commit_id` context
         create_params[:commit_id] = params.dig(:note, :commit_id)
       else
-        create_params[:noteable_id] = params[:target_id]
+        create_params[:noteable_id] = noteable.id
       end
     end
   end

app/controllers/snippets/notes_controller.rb

@@ -5,8 +5,8 @@ class Snippets::NotesController < ApplicationController
   include ToggleAwardEmoji
 
   skip_before_action :authenticate_user!, only: [:index]
-  before_action :snippet
-  before_action :authorize_read_snippet!, only: [:show, :index, :create]
+  before_action :authorize_read_snippet!, only: [:show, :index]
+  before_action :authorize_create_note!, only: [:create]
 
   private
 
@@ -33,4 +33,8 @@ class Snippets::NotesController < ApplicationController
   def authorize_read_snippet!
     return render_404 unless can?(current_user, :read_personal_snippet, snippet)
   end
+
+  def authorize_create_note!
+    access_denied! unless can?(current_user, :create_note, noteable)
+  end
 end

changelogs/unreleased/security-notes-in-private-snippets.yml

+---
+title: Correctly check permissions when creating snippet notes
+merge_request:
+author:
+type: security

spec/controllers/projects/notes_controller_spec.rb

@@ -252,7 +252,7 @@ describe Projects::NotesController do
       before do
         service_params = ActionController::Parameters.new({
           note: 'some note',
-          noteable_id: merge_request.id.to_s,
+          noteable_id: merge_request.id,
           noteable_type: 'MergeRequest',
           commit_id: nil,
           merge_request_diff_head_sha: 'sha'

spec/controllers/snippets/notes_controller_spec.rb

@@ -119,6 +119,119 @@ describe Snippets::NotesController do
     end
   end
 
+  describe 'POST create' do
+    context 'when a snippet is public' do
+      let(:request_params) do
+        {
+          note: attributes_for(:note_on_personal_snippet, noteable: public_snippet),
+          snippet_id: public_snippet.id
+        }
+      end
+
+      before do
+        sign_in user
+      end
+
+      it 'returns status 302' do
+        post :create, params: request_params
+
+        expect(response).to have_gitlab_http_status(302)
+      end
+
+      it 'creates the note' do
+        expect { post :create, params: request_params }.to change { Note.count }.by(1)
+      end
+    end
+
+    context 'when a snippet is internal' do
+      let(:request_params) do
+        {
+          note: attributes_for(:note_on_personal_snippet, noteable: internal_snippet),
+          snippet_id: internal_snippet.id
+        }
+      end
+
+      before do
+        sign_in user
+      end
+
+      it 'returns status 302' do
+        post :create, params: request_params
+
+        expect(response).to have_gitlab_http_status(302)
+      end
+
+      it 'creates the note' do
+        expect { post :create, params: request_params }.to change { Note.count }.by(1)
+      end
+    end
+
+    context 'when a snippet is private' do
+      let(:request_params) do
+        {
+          note: attributes_for(:note_on_personal_snippet, noteable: private_snippet),
+          snippet_id: private_snippet.id
+        }
+      end
+
+      before do
+        sign_in user
+      end
+
+      context 'when user is not the author' do
+        before do
+          sign_in(user)
+        end
+
+        it 'returns status 404' do
+          post :create, params: request_params
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+
+        it 'does not create the note' do
+          expect { post :create, params: request_params }.not_to change { Note.count }
+        end
+
+        context 'when user sends a snippet_id for a public snippet' do
+          let(:request_params) do
+            {
+              note: attributes_for(:note_on_personal_snippet, noteable: private_snippet),
+              snippet_id: public_snippet.id
+            }
+          end
+
+          it 'returns status 302' do
+            post :create, params: request_params
+
+            expect(response).to have_gitlab_http_status(302)
+          end
+
+          it 'creates the note on the public snippet' do
+            expect { post :create, params: request_params }.to change { Note.count }.by(1)
+            expect(Note.last.noteable).to eq public_snippet
+          end
+        end
+      end
+
+      context 'when user is the author' do
+        before do
+          sign_in(private_snippet.author)
+        end
+
+        it 'returns status 302' do
+          post :create, params: request_params
+
+          expect(response).to have_gitlab_http_status(302)
+        end
+
+        it 'creates the note' do
+          expect { post :create, params: request_params }.to change { Note.count }.by(1)
+        end
+      end
+    end
+  end
+
   describe 'DELETE destroy' do
     let(:request_params) do
       {