diff --git a/app/models/note.rb b/app/models/note.rb index bea02d69b65..1b595ef60b4 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -38,10 +38,12 @@ class Note < ActiveRecord::Base alias_attribute :last_edited_at, :updated_at alias_attribute :last_edited_by, :updated_by - # Attribute containing rendered and redacted Markdown as generated by - # Banzai::ObjectRenderer. + # Number of user visible references as generated by Banzai::ObjectRenderer attr_accessor :redacted_note_html + # Total of all references as generated by Banzai::ObjectRenderer + attr_accessor :total_reference_count + # An Array containing the number of visible references as generated by # Banzai::ObjectRenderer attr_accessor :user_visible_reference_count @@ -288,15 +290,7 @@ class Note < ActiveRecord::Base end def cross_reference_not_visible_for?(user) - cross_reference? && !has_referenced_mentionables?(user) - end - - def has_referenced_mentionables?(user) - if user_visible_reference_count.present? - user_visible_reference_count > 0 - else - referenced_mentionables(user).any? - end + cross_reference? && !all_referenced_mentionables_allowed?(user) end def award_emoji? @@ -466,9 +460,18 @@ class Note < ActiveRecord::Base self.discussion_id ||= discussion_class.discussion_id(self) end + def all_referenced_mentionables_allowed?(user) + if user_visible_reference_count.present? && total_reference_count.present? + # if they are not equal, then there are private/confidential references as well + user_visible_reference_count > 0 && user_visible_reference_count == total_reference_count + else + referenced_mentionables(user).any? + end + end + def force_cross_reference_regex_check? return unless system? - SystemNoteMetadata::TYPES_WITH_CROSS_REFERENCES.include?(system_note_metadata&.action) + system_note_metadata&.cross_reference_types&.include?(system_note_metadata&.action) end end diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 6fadbcefa53..d555ebe5322 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -9,6 +9,7 @@ class SystemNoteMetadata < ActiveRecord::Base TYPES_WITH_CROSS_REFERENCES = %w[ commit cross_reference close duplicate + moved ].freeze ICON_TYPES = %w[ @@ -26,4 +27,8 @@ class SystemNoteMetadata < ActiveRecord::Base def icon_types ICON_TYPES end + + def cross_reference_types + TYPES_WITH_CROSS_REFERENCES + end end diff --git a/changelogs/unreleased/security-fix-leaking-private-project-namespace.yml b/changelogs/unreleased/security-fix-leaking-private-project-namespace.yml new file mode 100644 index 00000000000..589d16c0c35 --- /dev/null +++ b/changelogs/unreleased/security-fix-leaking-private-project-namespace.yml @@ -0,0 +1,5 @@ +--- +title: Properly filter private references from system notes +merge_request: +author: +type: security diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb index a176f1e261b..7137c1da57d 100644 --- a/lib/banzai/object_renderer.rb +++ b/lib/banzai/object_renderer.rb @@ -38,6 +38,7 @@ module Banzai redacted_data = redacted[index] object.__send__("redacted_#{attribute}_html=", redacted_data[:document].to_html(save_options).html_safe) # rubocop:disable GitlabSecurity/PublicSend object.user_visible_reference_count = redacted_data[:visible_reference_count] if object.respond_to?(:user_visible_reference_count) + object.total_reference_count = redacted_data[:total_reference_count] if object.respond_to?(:total_reference_count) end end diff --git a/lib/banzai/redactor.rb b/lib/banzai/redactor.rb index 28928d6f376..e77bee78496 100644 --- a/lib/banzai/redactor.rb +++ b/lib/banzai/redactor.rb @@ -37,7 +37,13 @@ module Banzai all_document_nodes.each do |entry| nodes_for_document = entry[:nodes] - doc_data = { document: entry[:document], visible_reference_count: nodes_for_document.count } + + doc_data = { + document: entry[:document], + total_reference_count: nodes_for_document.count, + visible_reference_count: nodes_for_document.count + } + metadata << doc_data nodes_for_document.each do |node| diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 947be44c903..1783dd3206b 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -231,33 +231,60 @@ describe Note do let(:ext_proj) { create(:project, :public) } let(:ext_issue) { create(:issue, project: ext_proj) } - let(:note) do - create :note, - noteable: ext_issue, project: ext_proj, - note: "mentioned in issue #{private_issue.to_reference(ext_proj)}", - system: true + shared_examples "checks references" do + it "returns true" do + expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy + end + + it "returns false" do + expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy + end + + it "returns false if user visible reference count set" do + note.user_visible_reference_count = 1 + note.total_reference_count = 1 + + expect(note).not_to receive(:reference_mentionables) + expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy + end + + it "returns true if ref count is 0" do + note.user_visible_reference_count = 0 + + expect(note).not_to receive(:reference_mentionables) + expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy + end end - it "returns true" do - expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy + context "when there is one reference in note" do + let(:note) do + create :note, + noteable: ext_issue, project: ext_proj, + note: "mentioned in issue #{private_issue.to_reference(ext_proj)}", + system: true + end + + it_behaves_like "checks references" end - it "returns false" do - expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy - end + context "when there are two references in note" do + let(:note) do + create :note, + noteable: ext_issue, project: ext_proj, + note: "mentioned in issue #{private_issue.to_reference(ext_proj)} and " \ + "public issue #{ext_issue.to_reference(ext_proj)}", + system: true + end - it "returns false if user visible reference count set" do - note.user_visible_reference_count = 1 + it_behaves_like "checks references" - expect(note).not_to receive(:reference_mentionables) - expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy - end + it "returns true if user visible reference count set and there is a private reference" do + note.user_visible_reference_count = 1 + note.total_reference_count = 2 - it "returns true if ref count is 0" do - note.user_visible_reference_count = 0 - - expect(note).not_to receive(:reference_mentionables) - expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy + expect(note).not_to receive(:reference_mentionables) + expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy + end end end @@ -269,7 +296,7 @@ describe Note do end context 'when the note might contain cross references' do - SystemNoteMetadata::TYPES_WITH_CROSS_REFERENCES.each do |type| + SystemNoteMetadata.new.cross_reference_types.each do |type| let(:note) { create(:note, :system) } let!(:metadata) { create(:system_note_metadata, note: note, action: type) }