Commit da8ac163 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'issue_17302' into 'master'

Fix api leaking notes when user is not authorized to read noteable

fixes #17302

See merge request !4102
parents 98d88e81 5bf49bb6
...@@ -21,6 +21,7 @@ v 8.8.0 (unreleased) ...@@ -21,6 +21,7 @@ v 8.8.0 (unreleased)
- Bump mail_room to 0.7.0 to fix stuck IDLE connections - Bump mail_room to 0.7.0 to fix stuck IDLE connections
- Remove future dates from contribution calendar graph. - Remove future dates from contribution calendar graph.
- Support e-mail notifications for comments on project snippets - Support e-mail notifications for comments on project snippets
- Fix API leak of notes of unauthorized issues, snippets and merge requests
- Use ActionDispatch Remote IP for Akismet checking - Use ActionDispatch Remote IP for Akismet checking
- Fix error when visiting commit builds page before build was updated - Fix error when visiting commit builds page before build was updated
- Add 'l' shortcut to open Label dropdown on issuables and 'i' to create new issue on a project - Add 'l' shortcut to open Label dropdown on issuables and 'i' to create new issue on a project
......
...@@ -19,20 +19,24 @@ module API ...@@ -19,20 +19,24 @@ module API
# GET /projects/:id/issues/:noteable_id/notes # GET /projects/:id/issues/:noteable_id/notes
# GET /projects/:id/snippets/:noteable_id/notes # GET /projects/:id/snippets/:noteable_id/notes
get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do
@noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) @noteable = user_project.send(noteables_str.to_sym).find(params[noteable_id_str.to_sym])
# We exclude notes that are cross-references and that cannot be viewed if can?(current_user, noteable_read_ability_name(@noteable), @noteable)
# by the current user. By doing this exclusion at this level and not # We exclude notes that are cross-references and that cannot be viewed
# at the DB query level (which we cannot in that case), the current # by the current user. By doing this exclusion at this level and not
# page can have less elements than :per_page even if # at the DB query level (which we cannot in that case), the current
# there's more than one page. # page can have less elements than :per_page even if
notes = # there's more than one page.
# paginate() only works with a relation. This could lead to a notes =
# mismatch between the pagination headers info and the actual notes # paginate() only works with a relation. This could lead to a
# array returned, but this is really a edge-case. # mismatch between the pagination headers info and the actual notes
paginate(@noteable.notes). # array returned, but this is really a edge-case.
reject { |n| n.cross_reference_not_visible_for?(current_user) } paginate(@noteable.notes).
present notes, with: Entities::Note reject { |n| n.cross_reference_not_visible_for?(current_user) }
present notes, with: Entities::Note
else
not_found!("Notes")
end
end end
# Get a single +noteable+ note # Get a single +noteable+ note
...@@ -45,13 +49,14 @@ module API ...@@ -45,13 +49,14 @@ module API
# GET /projects/:id/issues/:noteable_id/notes/:note_id # GET /projects/:id/issues/:noteable_id/notes/:note_id
# GET /projects/:id/snippets/:noteable_id/notes/:note_id # GET /projects/:id/snippets/:noteable_id/notes/:note_id
get ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do get ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do
@noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) @noteable = user_project.send(noteables_str.to_sym).find(params[noteable_id_str.to_sym])
@note = @noteable.notes.find(params[:note_id]) @note = @noteable.notes.find(params[:note_id])
can_read_note = can?(current_user, noteable_read_ability_name(@noteable), @noteable) && !@note.cross_reference_not_visible_for?(current_user)
if @note.cross_reference_not_visible_for?(current_user) if can_read_note
not_found!("Note")
else
present @note, with: Entities::Note present @note, with: Entities::Note
else
not_found!("Note")
end end
end end
...@@ -136,5 +141,11 @@ module API ...@@ -136,5 +141,11 @@ module API
end end
end end
end end
helpers do
def noteable_read_ability_name(noteable)
"read_#{noteable.class.to_s.underscore.downcase}".to_sym
end
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, :public, 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) }
...@@ -39,6 +39,7 @@ describe API::API, api: true do ...@@ -39,6 +39,7 @@ describe API::API, api: true do
context "when noteable is an Issue" do context "when noteable is an Issue" do
it "should return an array of issue notes" do it "should return an array of issue notes" do
get api("/projects/#{project.id}/issues/#{issue.id}/notes", user) get api("/projects/#{project.id}/issues/#{issue.id}/notes", user)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first['body']).to eq(issue_note.note) expect(json_response.first['body']).to eq(issue_note.note)
...@@ -46,20 +47,33 @@ describe API::API, api: true do ...@@ -46,20 +47,33 @@ describe API::API, api: true do
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/12345/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
context "that references a private issue" do context "and current user cannot view the notes" do
it "should return an empty array" do it "should return an empty array" do
get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user) get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response).to be_empty expect(json_response).to be_empty
end end
context "and issue is confidential" do
before { ext_issue.update_attributes(confidential: true) }
it "returns 404" do
get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user)
expect(response.status).to eq(404)
end
end
context "and current user can view the note" do context "and current user can view the note" do
it "should return an empty array" do it "should return an empty array" do
get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user) get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first['body']).to eq(cross_reference_note.note) expect(json_response.first['body']).to eq(cross_reference_note.note)
...@@ -71,6 +85,7 @@ describe API::API, api: true do ...@@ -71,6 +85,7 @@ describe API::API, api: true do
context "when noteable is a Snippet" do context "when noteable is a Snippet" do
it "should return an array of snippet notes" do it "should return an array of snippet notes" do
get api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user) get api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first['body']).to eq(snippet_note.note) expect(json_response.first['body']).to eq(snippet_note.note)
...@@ -78,6 +93,13 @@ describe API::API, api: true do ...@@ -78,6 +93,13 @@ describe API::API, api: true do
it "should return a 404 error when snippet id not found" do it "should return a 404 error when snippet id not found" do
get api("/projects/#{project.id}/snippets/42/notes", user) get api("/projects/#{project.id}/snippets/42/notes", user)
expect(response.status).to eq(404)
end
it "returns 404 when not authorized" do
get api("/projects/#{project.id}/snippets/#{snippet.id}/notes", private_user)
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
end end
...@@ -85,6 +107,7 @@ describe API::API, api: true do ...@@ -85,6 +107,7 @@ describe API::API, api: true do
context "when noteable is a Merge Request" do context "when noteable is a Merge Request" do
it "should return an array of merge_requests notes" do it "should return an array of merge_requests notes" do
get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/notes", user) get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/notes", user)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first['body']).to eq(merge_request_note.note) expect(json_response.first['body']).to eq(merge_request_note.note)
...@@ -92,6 +115,13 @@ describe API::API, api: true do ...@@ -92,6 +115,13 @@ describe API::API, api: true do
it "should return a 404 error if merge request id not found" do it "should return a 404 error if merge request id not found" do
get api("/projects/#{project.id}/merge_requests/4444/notes", user) get api("/projects/#{project.id}/merge_requests/4444/notes", user)
expect(response.status).to eq(404)
end
it "returns 404 when not authorized" do
get api("/projects/#{project.id}/merge_requests/4444/notes", private_user)
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
end end
...@@ -101,24 +131,39 @@ describe API::API, api: true do ...@@ -101,24 +131,39 @@ describe API::API, api: true do
context "when noteable is an Issue" do context "when noteable is an Issue" do
it "should return an issue note by id" do it "should return an issue note by id" do
get api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", user) get api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", user)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response['body']).to eq(issue_note.note) expect(json_response['body']).to eq(issue_note.note)
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/12345", 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
context "that references a private issue" do context "and current user cannot view the note" do
it "should return a 404 error" do it "should return a 404 error" do
get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user) get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user)
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
context "when issue is confidential" do
before { issue.update_attributes(confidential: true) }
it "returns 404" do
get api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", private_user)
expect(response.status).to eq(404)
end
end
context "and current user can view the note" do context "and current user can view the note" do
it "should return an issue note by id" do it "should return an issue note by id" do
get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", private_user) get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", private_user)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response['body']).to eq(cross_reference_note.note) expect(json_response['body']).to eq(cross_reference_note.note)
end end
...@@ -129,12 +174,14 @@ describe API::API, api: true do ...@@ -129,12 +174,14 @@ describe API::API, api: true do
context "when noteable is a Snippet" do context "when noteable is a Snippet" do
it "should return a snippet note by id" do it "should return a snippet note by id" do
get api("/projects/#{project.id}/snippets/#{snippet.id}/notes/#{snippet_note.id}", user) get api("/projects/#{project.id}/snippets/#{snippet.id}/notes/#{snippet_note.id}", user)
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response['body']).to eq(snippet_note.note) expect(json_response['body']).to eq(snippet_note.note)
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/12345", 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
...@@ -144,6 +191,7 @@ describe API::API, api: true do ...@@ -144,6 +191,7 @@ describe API::API, api: true do
context "when noteable is an Issue" do context "when noteable is an Issue" do
it "should create a new issue note" do it "should create a new issue note" do
post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!' post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
expect(response.status).to eq(201) expect(response.status).to eq(201)
expect(json_response['body']).to eq('hi!') expect(json_response['body']).to eq('hi!')
expect(json_response['author']['username']).to eq(user.username) expect(json_response['author']['username']).to eq(user.username)
...@@ -151,11 +199,13 @@ describe API::API, api: true do ...@@ -151,11 +199,13 @@ describe API::API, api: true do
it "should return a 400 bad request error if body not given" do it "should return a 400 bad request error if body not given" do
post api("/projects/#{project.id}/issues/#{issue.id}/notes", user) post api("/projects/#{project.id}/issues/#{issue.id}/notes", user)
expect(response.status).to eq(400) expect(response.status).to eq(400)
end end
it "should return a 401 unauthorized error if user not authenticated" do it "should return a 401 unauthorized error if user not authenticated" do
post api("/projects/#{project.id}/issues/#{issue.id}/notes"), body: 'hi!' post api("/projects/#{project.id}/issues/#{issue.id}/notes"), body: 'hi!'
expect(response.status).to eq(401) expect(response.status).to eq(401)
end end
...@@ -164,6 +214,7 @@ describe API::API, api: true do ...@@ -164,6 +214,7 @@ describe API::API, api: true do
creation_time = 2.weeks.ago creation_time = 2.weeks.ago
post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), post api("/projects/#{project.id}/issues/#{issue.id}/notes", user),
body: 'hi!', created_at: creation_time body: 'hi!', created_at: creation_time
expect(response.status).to eq(201) expect(response.status).to eq(201)
expect(json_response['body']).to eq('hi!') expect(json_response['body']).to eq('hi!')
expect(json_response['author']['username']).to eq(user.username) expect(json_response['author']['username']).to eq(user.username)
...@@ -176,6 +227,7 @@ describe API::API, api: true do ...@@ -176,6 +227,7 @@ describe API::API, api: true do
context "when noteable is a Snippet" do context "when noteable is a Snippet" do
it "should create a new snippet note" do it "should create a new snippet note" do
post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user), body: 'hi!' post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user), body: 'hi!'
expect(response.status).to eq(201) expect(response.status).to eq(201)
expect(json_response['body']).to eq('hi!') expect(json_response['body']).to eq('hi!')
expect(json_response['author']['username']).to eq(user.username) expect(json_response['author']['username']).to eq(user.username)
...@@ -183,11 +235,13 @@ describe API::API, api: true do ...@@ -183,11 +235,13 @@ describe API::API, api: true do
it "should return a 400 bad request error if body not given" do it "should return a 400 bad request error if body not given" do
post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user) post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user)
expect(response.status).to eq(400) expect(response.status).to eq(400)
end end
it "should return a 401 unauthorized error if user not authenticated" do it "should return a 401 unauthorized error if user not authenticated" do
post api("/projects/#{project.id}/snippets/#{snippet.id}/notes"), body: 'hi!' post api("/projects/#{project.id}/snippets/#{snippet.id}/notes"), body: 'hi!'
expect(response.status).to eq(401) expect(response.status).to eq(401)
end end
end end
...@@ -227,6 +281,7 @@ describe API::API, api: true do ...@@ -227,6 +281,7 @@ describe API::API, api: true do
it 'should return modified note' do it 'should return modified note' do
put api("/projects/#{project.id}/issues/#{issue.id}/"\ put api("/projects/#{project.id}/issues/#{issue.id}/"\
"notes/#{issue_note.id}", user), body: 'Hello!' "notes/#{issue_note.id}", user), body: 'Hello!'
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response['body']).to eq('Hello!') expect(json_response['body']).to eq('Hello!')
end end
...@@ -234,12 +289,14 @@ describe API::API, api: true do ...@@ -234,12 +289,14 @@ 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}/issues/#{issue.id}/notes/12345", 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
it 'should return a 400 bad request error if body not given' do it 'should return a 400 bad request error if body not given' do
put api("/projects/#{project.id}/issues/#{issue.id}/"\ put api("/projects/#{project.id}/issues/#{issue.id}/"\
"notes/#{issue_note.id}", user) "notes/#{issue_note.id}", user)
expect(response.status).to eq(400) expect(response.status).to eq(400)
end end
end end
...@@ -248,6 +305,7 @@ describe API::API, api: true do ...@@ -248,6 +305,7 @@ describe API::API, api: true do
it 'should return modified note' do it 'should return modified note' do
put api("/projects/#{project.id}/snippets/#{snippet.id}/"\ put api("/projects/#{project.id}/snippets/#{snippet.id}/"\
"notes/#{snippet_note.id}", user), body: 'Hello!' "notes/#{snippet_note.id}", user), body: 'Hello!'
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response['body']).to eq('Hello!') expect(json_response['body']).to eq('Hello!')
end end
...@@ -255,6 +313,7 @@ describe API::API, api: true do ...@@ -255,6 +313,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/12345", user), body: "Hello!" "notes/12345", user), body: "Hello!"
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
end end
...@@ -263,6 +322,7 @@ describe API::API, api: true do ...@@ -263,6 +322,7 @@ describe API::API, api: true do
it 'should return modified note' do it 'should return modified note' do
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\ put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\
"notes/#{merge_request_note.id}", user), body: 'Hello!' "notes/#{merge_request_note.id}", user), body: 'Hello!'
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(json_response['body']).to eq('Hello!') expect(json_response['body']).to eq('Hello!')
end end
...@@ -270,6 +330,7 @@ describe API::API, api: true do ...@@ -270,6 +330,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/12345", user), body: "Hello!" "notes/12345", user), body: "Hello!"
expect(response.status).to eq(404) expect(response.status).to eq(404)
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