Commit 2d5608ac authored by Robert Speicher's avatar 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
parents 2f5394f5 be67a484
......@@ -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
......@@ -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
......@@ -191,6 +191,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
......@@ -211,7 +232,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
......@@ -233,7 +254,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
......@@ -248,7 +269,7 @@ 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
......@@ -268,7 +289,7 @@ describe API::API, api: true do
end
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)
end
......@@ -288,7 +309,7 @@ describe API::API, api: true do
it 'returns a 404 error when note id not found' do
delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\
"notes/123", user)
"notes/12345", user)
expect(response.status).to eq(404)
end
......@@ -308,7 +329,7 @@ describe API::API, api: true do
it 'returns a 404 error when note id not found' do
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)
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