Merge branch 'rs-notes-privilege-escalation' into 'master'

Prevent privilege escalation via notes API

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15577

See merge request !1964

2 files changed, 39 insertions, 8 deletions

diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 2bb312bb252..01586994813 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -5,6 +5,8 @@ module Notes
       note.author = current_user
       note.system = false
 
+      return unless valid_project?(note)
+
       if note.save
         # Finish the harder work in the background
         NewNoteWorker.perform_in(2.seconds, note.id, params)
@@ -13,5 +15,14 @@ module Notes
 
       note
     end
+
+    private
+
+    def valid_project?(note)
+      return false unless project
+      return true if note.for_commit?
+
+      note.noteable.try(:project) == project
+    end
   end
 end
diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index 39f9a06fe1b..a66cd5d55fb 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe API::API, api: true  do
   include ApiHelpers
   let(:user) { create(:user) }
-  let!(:project) { create(:project, namespace: user.namespace ) }
+  let!(:project) { create(:project, namespace: user.namespace) }
   let!(:issue) { create(:issue, project: project, author: user) }
   let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) }
   let!(:snippet) { create(:project_snippet, project: project, author: user) }
@@ -45,7 +45,7 @@ describe API::API, api: true  do
       end
 
       it "should return a 404 error when issue id not found" do
-        get api("/projects/#{project.id}/issues/123/notes", user)
+        get api("/projects/#{project.id}/issues/12345/notes", user)
         expect(response.status).to eq(404)
       end
 
@@ -106,7 +106,7 @@ describe API::API, api: true  do
       end
 
       it "should return a 404 error if issue note not found" do
-        get api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user)
+        get api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user)
         expect(response.status).to eq(404)
       end
 
@@ -134,7 +134,7 @@ describe API::API, api: true  do
       end
 
       it "should return a 404 error if snippet note not found" do
-        get api("/projects/#{project.id}/snippets/#{snippet.id}/notes/123", user)
+        get api("/projects/#{project.id}/snippets/#{snippet.id}/notes/12345", user)
         expect(response.status).to eq(404)
       end
     end
@@ -178,6 +178,27 @@ describe API::API, api: true  do
         expect(response.status).to eq(401)
       end
     end
+
+    context 'when user does not have access to create noteable' do
+      let(:private_issue) { create(:issue, project: create(:project, :private)) }
+
+      ##
+      # We are posting to project user has access to, but we use issue id
+      # from a different project, see #15577
+      #
+      before do
+        post api("/projects/#{project.id}/issues/#{private_issue.id}/notes", user),
+             body: 'Hi!'
+      end
+
+      it 'responds with 500' do
+        expect(response.status).to eq 500
+      end
+
+      it 'does not create new note' do
+        expect(private_issue.notes.reload).to be_empty
+      end
+    end
   end
 
   describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
@@ -198,7 +219,7 @@ describe API::API, api: true  do
       end
 
       it 'should return a 404 error when note id not found' do
-        put api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user),
+        put api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user),
                 body: 'Hello!'
         expect(response.status).to eq(404)
       end
@@ -220,7 +241,7 @@ describe API::API, api: true  do
 
       it 'should return a 404 error when note id not found' do
         put api("/projects/#{project.id}/snippets/#{snippet.id}/"\
-                  "notes/123", user), body: "Hello!"
+                  "notes/12345", user), body: "Hello!"
         expect(response.status).to eq(404)
       end
     end
@@ -235,10 +256,9 @@ describe API::API, api: true  do
 
       it 'should return a 404 error when note id not found' do
         put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\
-                  "notes/123", user), body: "Hello!"
+                  "notes/12345", user), body: "Hello!"
         expect(response.status).to eq(404)
       end
     end
   end
-
 end