From e56e3cdc62f96541b9bd8b7814204e92f1909253 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 9 May 2016 19:35:37 -0300 Subject: [PATCH 1/4] Fix api leaking notes when user is not authorized to read noteable --- CHANGELOG | 1 + lib/api/notes.rb | 29 +++++++++++++++++------------ spec/requests/api/notes_spec.rb | 19 +++++++++++++++++++ 3 files changed, 37 insertions(+), 12 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 63077523aaa..2c44944f66b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -15,6 +15,7 @@ v 8.8.0 (unreleased) - Bump mail_room to 0.7.0 to fix stuck IDLE connections - Remove future dates from contribution calendar graph. - 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 - 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 diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 71a53e6f0d6..4ac08a3e8c4 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -20,19 +20,24 @@ module API # GET /projects/:id/snippets/:noteable_id/notes get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) + read_ability_name = "read_#{@noteable.class.to_s.underscore.downcase}".to_sym - # We exclude notes that are cross-references and that cannot be viewed - # by the current user. By doing this exclusion at this level and not - # at the DB query level (which we cannot in that case), the current - # page can have less elements than :per_page even if - # there's more than one page. - notes = - # paginate() only works with a relation. This could lead to a - # mismatch between the pagination headers info and the actual notes - # array returned, but this is really a edge-case. - paginate(@noteable.notes). - reject { |n| n.cross_reference_not_visible_for?(current_user) } - present notes, with: Entities::Note + if can?(current_user, read_ability_name, @noteable) + # We exclude notes that are cross-references and that cannot be viewed + # by the current user. By doing this exclusion at this level and not + # at the DB query level (which we cannot in that case), the current + # page can have less elements than :per_page even if + # there's more than one page. + notes = + # paginate() only works with a relation. This could lead to a + # mismatch between the pagination headers info and the actual notes + # array returned, but this is really a edge-case. + paginate(@noteable.notes). + reject { |n| n.cross_reference_not_visible_for?(current_user) } + present notes, with: Entities::Note + else + render_api_error!("Not found.", 404) + end end # Get a single +noteable+ note diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 49091fc0f49..f5b31be1ba3 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -57,6 +57,15 @@ describe API::API, api: true do expect(json_response).to be_empty 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 it "should return an empty array" do get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user) @@ -80,6 +89,11 @@ describe API::API, api: true do 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) + end end context "when noteable is a Merge Request" do @@ -94,6 +108,11 @@ describe API::API, api: true do 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) + end end end From 93ca5c9964a26fbf31fcc794348b30193f4dff9f Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 10 May 2016 16:06:02 -0300 Subject: [PATCH 2/4] Fix notes API calls symbol convertions --- lib/api/notes.rb | 6 +++--- spec/requests/api/notes_spec.rb | 31 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 4ac08a3e8c4..f0116acd90f 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -19,7 +19,7 @@ module API # GET /projects/:id/issues/:noteable_id/notes # GET /projects/:id/snippets/:noteable_id/notes 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]) read_ability_name = "read_#{@noteable.class.to_s.underscore.downcase}".to_sym if can?(current_user, read_ability_name, @noteable) @@ -36,7 +36,7 @@ module API reject { |n| n.cross_reference_not_visible_for?(current_user) } present notes, with: Entities::Note else - render_api_error!("Not found.", 404) + not_found!("Notes") end end @@ -50,7 +50,7 @@ module API # GET /projects/:id/issues/: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 - @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]) if @note.cross_reference_not_visible_for?(current_user) diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index f5b31be1ba3..f9bfee94424 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -39,6 +39,7 @@ describe API::API, api: true do context "when noteable is an Issue" do it "should return an array of issue notes" do get api("/projects/#{project.id}/issues/#{issue.id}/notes", user) + expect(response.status).to eq(200) expect(json_response).to be_an Array expect(json_response.first['body']).to eq(issue_note.note) @@ -46,12 +47,14 @@ describe API::API, api: true do it "should return a 404 error when issue id not found" do get api("/projects/#{project.id}/issues/12345/notes", user) + expect(response.status).to eq(404) end context "that references a private issue" do it "should return an empty array" do get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user) + expect(response.status).to eq(200) expect(json_response).to be_an Array expect(json_response).to be_empty @@ -62,6 +65,7 @@ describe API::API, api: true do it "returns 404" do get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user) + expect(response.status).to eq(404) end end @@ -69,6 +73,7 @@ describe API::API, api: true do context "and current user can view the note" do it "should return an empty array" do get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user) + expect(response.status).to eq(200) expect(json_response).to be_an Array expect(json_response.first['body']).to eq(cross_reference_note.note) @@ -80,6 +85,7 @@ describe API::API, api: true do context "when noteable is a Snippet" do it "should return an array of snippet notes" do get api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user) + expect(response.status).to eq(200) expect(json_response).to be_an Array expect(json_response.first['body']).to eq(snippet_note.note) @@ -87,11 +93,13 @@ describe API::API, api: true do it "should return a 404 error when snippet id not found" do 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) end end @@ -99,6 +107,7 @@ describe API::API, api: true do context "when noteable is a Merge Request" do it "should return an array of merge_requests notes" do get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/notes", user) + expect(response.status).to eq(200) expect(json_response).to be_an Array expect(json_response.first['body']).to eq(merge_request_note.note) @@ -106,11 +115,13 @@ describe API::API, api: true do it "should return a 404 error if merge request id not found" do 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) end end @@ -120,24 +131,28 @@ describe API::API, api: true do context "when noteable is an Issue" do it "should return an issue note by id" do get api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", user) + expect(response.status).to eq(200) expect(json_response['body']).to eq(issue_note.note) end it "should return a 404 error if issue note not found" do get api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user) + expect(response.status).to eq(404) end context "that references a private issue" do it "should return a 404 error" do get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user) + expect(response.status).to eq(404) end context "and current user can view the note" 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) + expect(response.status).to eq(200) expect(json_response['body']).to eq(cross_reference_note.note) end @@ -148,12 +163,14 @@ describe API::API, api: true do context "when noteable is a Snippet" do it "should return a snippet note by id" do get api("/projects/#{project.id}/snippets/#{snippet.id}/notes/#{snippet_note.id}", user) + expect(response.status).to eq(200) expect(json_response['body']).to eq(snippet_note.note) end it "should return a 404 error if snippet note not found" do get api("/projects/#{project.id}/snippets/#{snippet.id}/notes/12345", user) + expect(response.status).to eq(404) end end @@ -163,6 +180,7 @@ describe API::API, api: true do context "when noteable is an Issue" do it "should create a new issue note" do post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!' + expect(response.status).to eq(201) expect(json_response['body']).to eq('hi!') expect(json_response['author']['username']).to eq(user.username) @@ -170,11 +188,13 @@ describe API::API, api: true do it "should return a 400 bad request error if body not given" do post api("/projects/#{project.id}/issues/#{issue.id}/notes", user) + expect(response.status).to eq(400) end it "should return a 401 unauthorized error if user not authenticated" do post api("/projects/#{project.id}/issues/#{issue.id}/notes"), body: 'hi!' + expect(response.status).to eq(401) end @@ -183,6 +203,7 @@ describe API::API, api: true do creation_time = 2.weeks.ago post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!', created_at: creation_time + expect(response.status).to eq(201) expect(json_response['body']).to eq('hi!') expect(json_response['author']['username']).to eq(user.username) @@ -195,6 +216,7 @@ describe API::API, api: true do context "when noteable is a Snippet" do it "should create a new snippet note" do post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user), body: 'hi!' + expect(response.status).to eq(201) expect(json_response['body']).to eq('hi!') expect(json_response['author']['username']).to eq(user.username) @@ -202,11 +224,13 @@ describe API::API, api: true do it "should return a 400 bad request error if body not given" do post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user) + expect(response.status).to eq(400) end it "should return a 401 unauthorized error if user not authenticated" do post api("/projects/#{project.id}/snippets/#{snippet.id}/notes"), body: 'hi!' + expect(response.status).to eq(401) end end @@ -246,6 +270,7 @@ describe API::API, api: true do it 'should return modified note' do put api("/projects/#{project.id}/issues/#{issue.id}/"\ "notes/#{issue_note.id}", user), body: 'Hello!' + expect(response.status).to eq(200) expect(json_response['body']).to eq('Hello!') end @@ -253,12 +278,14 @@ describe API::API, api: true do it 'should return a 404 error when note id not found' do put api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user), body: 'Hello!' + expect(response.status).to eq(404) end it 'should return a 400 bad request error if body not given' do put api("/projects/#{project.id}/issues/#{issue.id}/"\ "notes/#{issue_note.id}", user) + expect(response.status).to eq(400) end end @@ -267,6 +294,7 @@ describe API::API, api: true do it 'should return modified note' do put api("/projects/#{project.id}/snippets/#{snippet.id}/"\ "notes/#{snippet_note.id}", user), body: 'Hello!' + expect(response.status).to eq(200) expect(json_response['body']).to eq('Hello!') end @@ -274,6 +302,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/12345", user), body: "Hello!" + expect(response.status).to eq(404) end end @@ -282,6 +311,7 @@ describe API::API, api: true do it 'should return modified note' do put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\ "notes/#{merge_request_note.id}", user), body: 'Hello!' + expect(response.status).to eq(200) expect(json_response['body']).to eq('Hello!') end @@ -289,6 +319,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/12345", user), body: "Hello!" + expect(response.status).to eq(404) end end From c9be74e24797c1dab5b443728349bb0c5ce969c3 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 16 May 2016 16:43:19 -0300 Subject: [PATCH 3/4] Fix single note api request --- lib/api/helpers.rb | 4 ++++ lib/api/notes.rb | 10 +++++----- spec/requests/api/notes_spec.rb | 17 ++++++++++++++--- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 40c967453fb..1003b596aec 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -397,5 +397,9 @@ module API error!(errors[:access_level], 422) if errors[:access_level].any? not_found!(errors) end + + def noteable_ability_name(noteable) + "read_#{noteable.class.to_s.underscore.downcase}".to_sym + end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index f0116acd90f..c49b107d1d9 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -20,9 +20,8 @@ module API # GET /projects/:id/snippets/:noteable_id/notes get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do @noteable = user_project.send(noteables_str.to_sym).find(params[noteable_id_str.to_sym]) - read_ability_name = "read_#{@noteable.class.to_s.underscore.downcase}".to_sym - if can?(current_user, read_ability_name, @noteable) + if can?(current_user, noteable_ability_name(@noteable), @noteable) # We exclude notes that are cross-references and that cannot be viewed # by the current user. By doing this exclusion at this level and not # at the DB query level (which we cannot in that case), the current @@ -52,11 +51,12 @@ module API get ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do @noteable = user_project.send(noteables_str.to_sym).find(params[noteable_id_str.to_sym]) @note = @noteable.notes.find(params[:note_id]) + can_read_note = can?(current_user, noteable_ability_name(@noteable), @noteable) && !@note.cross_reference_not_visible_for?(current_user) - if @note.cross_reference_not_visible_for?(current_user) - not_found!("Note") - else + if can_read_note present @note, with: Entities::Note + else + not_found!("Note") end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index f9bfee94424..ed1ed5aeb95 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, :public, 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) } @@ -51,7 +51,7 @@ describe API::API, api: true do expect(response.status).to eq(404) end - context "that references a private issue" do + context "and current user cannot view the notes" do it "should return an empty array" do get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user) @@ -142,13 +142,24 @@ describe API::API, api: true do expect(response.status).to eq(404) end - context "that references a private issue" do + context "and current user cannot view the note" do it "should return a 404 error" do get api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user) expect(response.status).to eq(404) 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 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) From 5bf49bb63d88b1cce2d9a44716b54acfa63ea657 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 17 May 2016 21:41:53 -0500 Subject: [PATCH 4/4] Move note helper method to notes entity file --- lib/api/helpers.rb | 4 ---- lib/api/notes.rb | 10 ++++++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 1003b596aec..40c967453fb 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -397,9 +397,5 @@ module API error!(errors[:access_level], 422) if errors[:access_level].any? not_found!(errors) end - - def noteable_ability_name(noteable) - "read_#{noteable.class.to_s.underscore.downcase}".to_sym - end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index c49b107d1d9..d4fcfd3d4d3 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -21,7 +21,7 @@ module API get ":id/#{noteables_str}/:#{noteable_id_str}/notes" do @noteable = user_project.send(noteables_str.to_sym).find(params[noteable_id_str.to_sym]) - if can?(current_user, noteable_ability_name(@noteable), @noteable) + if can?(current_user, noteable_read_ability_name(@noteable), @noteable) # We exclude notes that are cross-references and that cannot be viewed # by the current user. By doing this exclusion at this level and not # at the DB query level (which we cannot in that case), the current @@ -51,7 +51,7 @@ module API get ":id/#{noteables_str}/:#{noteable_id_str}/notes/:note_id" do @noteable = user_project.send(noteables_str.to_sym).find(params[noteable_id_str.to_sym]) @note = @noteable.notes.find(params[:note_id]) - can_read_note = can?(current_user, noteable_ability_name(@noteable), @noteable) && !@note.cross_reference_not_visible_for?(current_user) + can_read_note = can?(current_user, noteable_read_ability_name(@noteable), @noteable) && !@note.cross_reference_not_visible_for?(current_user) if can_read_note present @note, with: Entities::Note @@ -141,5 +141,11 @@ module API end end end + + helpers do + def noteable_read_ability_name(noteable) + "read_#{noteable.class.to_s.underscore.downcase}".to_sym + end + end end end