diff options
-rw-r--r-- | app/controllers/concerns/issuable_actions.rb | 2 | ||||
-rw-r--r-- | app/controllers/concerns/notes_actions.rb | 2 | ||||
-rw-r--r-- | app/models/group.rb | 2 | ||||
-rw-r--r-- | app/models/note.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ce-master.yml | 5 | ||||
-rw-r--r-- | lib/api/discussions.rb | 2 | ||||
-rw-r--r-- | lib/api/helpers/notes_helpers.rb | 6 | ||||
-rw-r--r-- | lib/api/notes.rb | 2 | ||||
-rw-r--r-- | spec/factories/sequences.rb | 2 | ||||
-rw-r--r-- | spec/models/ci/pipeline_spec.rb | 5 | ||||
-rw-r--r-- | spec/services/web_hook_service_spec.rb | 33 |
11 files changed, 42 insertions, 23 deletions
diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index b86e4451a7e..1d6a1be402c 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -110,7 +110,7 @@ module IssuableActions end notes = prepare_notes_for_rendering(notes) - notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes = notes.select { |n| n.visible_for?(current_user) } discussions = Discussion.build_collection(notes, issuable) diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index d2a961efff7..80be7095ed3 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -29,7 +29,7 @@ module NotesActions end notes = prepare_notes_for_rendering(notes) - notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes = notes.select { |n| n.visible_for?(current_user) } notes_json[:notes] = if use_note_serializer? diff --git a/app/models/group.rb b/app/models/group.rb index 6c868b1d1f0..61a4802a6ee 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -365,6 +365,8 @@ class Group < Namespace end def max_member_access_for_user(user) + return GroupMember::NO_ACCESS unless user + return GroupMember::OWNER if user.admin? members_with_parents diff --git a/app/models/note.rb b/app/models/note.rb index 79aad5cbff9..3956ec192b1 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -332,6 +332,10 @@ class Note < ApplicationRecord cross_reference? && !all_referenced_mentionables_allowed?(user) end + def visible_for?(user) + !cross_reference_not_visible_for?(user) + end + def award_emoji? can_be_award_emoji? && contains_emoji_only? end diff --git a/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ce-master.yml b/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ce-master.yml new file mode 100644 index 00000000000..c639098721e --- /dev/null +++ b/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ce-master.yml @@ -0,0 +1,5 @@ +--- +title: Filter out old system notes for epics in notes api endpoint response +merge_request: +author: +type: security diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index 6c1acc3963f..9125207167c 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -239,7 +239,7 @@ module API # because notes are redacted if they point to projects that # cannot be accessed by the user. notes = prepare_notes_for_rendering(notes) - notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes.select { |n| n.visible_for?(current_user) } end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index b2bf6bf7417..f445834323d 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -12,7 +12,7 @@ module API end def update_note(noteable, note_id) - note = noteable.notes.find(params[:note_id]) + note = noteable.notes.find(note_id) authorize! :admin_note, note @@ -61,8 +61,8 @@ module API end def get_note(noteable, note_id) - note = noteable.notes.with_metadata.find(params[:note_id]) - can_read_note = !note.cross_reference_not_visible_for?(current_user) + note = noteable.notes.with_metadata.find(note_id) + can_read_note = note.visible_for?(current_user) if can_read_note present note, with: Entities::Note diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 84563d66ee8..16fca9acccb 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -42,7 +42,7 @@ module API # array returned, but this is really a edge-case. notes = paginate(raw_notes) notes = prepare_notes_for_rendering(notes) - notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes = notes.select { |note| note.visible_for?(current_user) } present notes, with: Entities::Note end # rubocop: enable CodeReuse/ActiveRecord diff --git a/spec/factories/sequences.rb b/spec/factories/sequences.rb index b6f2d6d8389..17b54d69372 100644 --- a/spec/factories/sequences.rb +++ b/spec/factories/sequences.rb @@ -7,7 +7,7 @@ FactoryBot.define do sequence(:email_alias) { |n| "user.alias#{n}@example.org" } sequence(:title) { |n| "My title #{n}" } sequence(:filename) { |n| "filename-#{n}.rb" } - sequence(:url) { |n| "http://example#{n}.org" } + sequence(:url) { |n| "http://example#{n}.test" } sequence(:label_title) { |n| "label#{n}" } sequence(:branch) { |n| "my-branch-#{n}" } sequence(:past_time) { |n| 4.hours.ago + (2 * n).seconds } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 78be4a8131a..7d84d094bdf 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe Ci::Pipeline, :mailer do include ProjectForksHelper + include StubRequests let(:user) { create(:user) } set(:project) { create(:project) } @@ -2504,7 +2505,7 @@ describe Ci::Pipeline, :mailer do let(:enabled) { true } before do - WebMock.stub_request(:post, hook.url) + stub_full_request(hook.url, method: :post) end context 'with multiple builds' do @@ -2558,7 +2559,7 @@ describe Ci::Pipeline, :mailer do end def have_requested_pipeline_hook(status) - have_requested(:post, hook.url).with do |req| + have_requested(:post, stubbed_hostname(hook.url)).with do |req| json_body = JSON.parse(req.body) json_body['object_attributes']['status'] == status && json_body['builds'].length == 2 diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 50167a2e059..2a4368868d5 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -55,31 +55,38 @@ describe WebHookService do describe '#execute' do before do project.hooks << [project_hook] - - WebMock.stub_request(:post, project_hook.url) end context 'when token is defined' do let(:project_hook) { create(:project_hook, :token) } it 'POSTs to the webhook URL' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers.merge({ 'X-Gitlab-Token' => project_hook.token }) ).once end end it 'POSTs to the webhook URL' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers ).once end it 'POSTs the data as JSON' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers ).once end @@ -115,7 +122,7 @@ describe WebHookService do end it 'catches exceptions' do - WebMock.stub_request(:post, project_hook.url).to_raise(StandardError.new('Some error')) + stub_full_request(project_hook.url, method: :post).to_raise(StandardError.new('Some error')) expect { service_instance.execute }.to raise_error(StandardError) end @@ -125,20 +132,20 @@ describe WebHookService do exceptions.each do |exception_class| exception = exception_class.new('Exception message') - WebMock.stub_request(:post, project_hook.url).to_raise(exception) + stub_full_request(project_hook.url, method: :post).to_raise(exception) expect(service_instance.execute).to eq({ status: :error, message: exception.to_s }) expect { service_instance.execute }.not_to raise_error end end it 'handles 200 status code' do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') expect(service_instance.execute).to include({ status: :success, http_status: 200, message: 'Success' }) end it 'handles 2xx status codes' do - WebMock.stub_request(:post, project_hook.url).to_return(status: 201, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 201, body: 'Success') expect(service_instance.execute).to include({ status: :success, http_status: 201, message: 'Success' }) end @@ -148,7 +155,7 @@ describe WebHookService do context 'with success' do before do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') service_instance.execute end @@ -165,7 +172,7 @@ describe WebHookService do context 'with exception' do before do - WebMock.stub_request(:post, project_hook.url).to_raise(SocketError.new('Some HTTP Post error')) + stub_full_request(project_hook.url, method: :post).to_raise(SocketError.new('Some HTTP Post error')) service_instance.execute end @@ -182,7 +189,7 @@ describe WebHookService do context 'with unsafe response body' do before do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: "\xBB") + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: "\xBB") service_instance.execute end @@ -202,7 +209,7 @@ describe WebHookService do let(:service_instance) { described_class.new(service_hook, data, 'service_hook') } before do - WebMock.stub_request(:post, service_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(service_hook.url, method: :post).to_return(status: 200, body: 'Success') end it { expect { service_instance.execute }.not_to change(WebHookLog, :count) } |