Commit 6044fd7e authored by Robert Speicher's avatar Robert Speicher Committed by Yorick Peterse

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 c8776b0c
...@@ -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
# Finish the harder work in the background # Finish the harder work in the background
NewNoteWorker.perform_in(2.seconds, note.id, params) NewNoteWorker.perform_in(2.seconds, note.id, params)
...@@ -13,5 +15,14 @@ module Notes ...@@ -13,5 +15,14 @@ module Notes
note note
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) }
...@@ -45,7 +45,7 @@ describe API::API, api: true do ...@@ -45,7 +45,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
...@@ -106,7 +106,7 @@ describe API::API, api: true do ...@@ -106,7 +106,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
...@@ -134,7 +134,7 @@ describe API::API, api: true do ...@@ -134,7 +134,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
...@@ -191,6 +191,27 @@ describe API::API, api: true do ...@@ -191,6 +191,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
...@@ -211,7 +232,7 @@ describe API::API, api: true do ...@@ -211,7 +232,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
...@@ -233,7 +254,7 @@ describe API::API, api: true do ...@@ -233,7 +254,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
...@@ -248,7 +269,7 @@ describe API::API, api: true do ...@@ -248,7 +269,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}/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
...@@ -268,7 +289,7 @@ describe API::API, api: true do ...@@ -268,7 +289,7 @@ describe API::API, api: true do
end end
it 'returns a 404 error when note id not found' do it 'returns a 404 error when note id not found' do
delete api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user) delete api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user)
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
...@@ -288,7 +309,7 @@ describe API::API, api: true do ...@@ -288,7 +309,7 @@ describe API::API, api: true do
it 'returns a 404 error when note id not found' do it 'returns a 404 error when note id not found' do
delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\ delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\
"notes/123", user) "notes/12345", user)
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
...@@ -308,7 +329,7 @@ describe API::API, api: true do ...@@ -308,7 +329,7 @@ describe API::API, api: true do
it 'returns a 404 error when note id not found' do it 'returns a 404 error when note id not found' do
delete api("/projects/#{project.id}/merge_requests/"\ delete api("/projects/#{project.id}/merge_requests/"\
"#{merge_request.id}/notes/123", user) "#{merge_request.id}/notes/12345", user)
expect(response.status).to eq(404) expect(response.status).to eq(404)
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