Commit 076632f1 authored by Robert Speicher's avatar Robert Speicher Committed by Robert Speicher

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
parent 23bc10ec
...@@ -5,6 +5,8 @@ module Notes ...@@ -5,6 +5,8 @@ module Notes
note.author = current_user note.author = current_user
note.system = false note.system = false
return unless valid_project?(note)
if note.save if note.save
notification_service.new_note(note) notification_service.new_note(note)
...@@ -28,5 +30,14 @@ module Notes ...@@ -28,5 +30,14 @@ module Notes
note.project.execute_hooks(note_data, :note_hooks) note.project.execute_hooks(note_data, :note_hooks)
note.project.execute_services(note_data, :note_hooks) note.project.execute_services(note_data, :note_hooks)
end end
private
def valid_project?(note)
return false unless project
return true if note.for_commit?
note.noteable.try(:project) == project
end
end end
end end
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe API::API, api: true do describe API::API, api: true do
include ApiHelpers include ApiHelpers
let(:user) { create(:user) } 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!(:issue) { create(:issue, project: project, author: user) }
let!(:merge_request) { create(:merge_request, source_project: project, target_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) } let!(:snippet) { create(:project_snippet, project: project, author: user) }
...@@ -22,7 +22,7 @@ describe API::API, api: true do ...@@ -22,7 +22,7 @@ describe API::API, api: true do
end end
it "should return a 404 error when issue id not found" do 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) expect(response.status).to eq(404)
end end
end end
...@@ -65,7 +65,7 @@ describe API::API, api: true do ...@@ -65,7 +65,7 @@ describe API::API, api: true do
end end
it "should return a 404 error if issue note not found" do 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) expect(response.status).to eq(404)
end end
end end
...@@ -78,7 +78,7 @@ describe API::API, api: true do ...@@ -78,7 +78,7 @@ describe API::API, api: true do
end end
it "should return a 404 error if snippet note not found" do 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) expect(response.status).to eq(404)
end end
end end
...@@ -122,6 +122,27 @@ describe API::API, api: true do ...@@ -122,6 +122,27 @@ describe API::API, api: true do
expect(response.status).to eq(401) expect(response.status).to eq(401)
end end
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 end
describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
...@@ -142,7 +163,7 @@ describe API::API, api: true do ...@@ -142,7 +163,7 @@ describe API::API, api: true do
end end
it 'should return a 404 error when note id not found' do 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!' body: 'Hello!'
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
...@@ -164,7 +185,7 @@ describe API::API, api: true do ...@@ -164,7 +185,7 @@ describe API::API, api: true do
it 'should return a 404 error when note id not found' do it 'should return a 404 error when note id not found' do
put api("/projects/#{project.id}/snippets/#{snippet.id}/"\ put api("/projects/#{project.id}/snippets/#{snippet.id}/"\
"notes/123", user), body: "Hello!" "notes/12345", user), body: "Hello!"
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
end end
...@@ -179,10 +200,9 @@ describe API::API, api: true do ...@@ -179,10 +200,9 @@ describe API::API, api: true do
it 'should return a 404 error when note id not found' do it 'should return a 404 error when note id not found' do
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\ 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) expect(response.status).to eq(404)
end end
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment