Commit 531b60ff authored by Bob Van Landuyt's avatar Bob Van Landuyt

Merge branch 'security-fix-leaking-private-project-namespace' into 'master'

[master] Fix leaking private project namespace

Closes gitlabhq#2708

See merge request gitlab/gitlab-ee!672
parents 412e7af8 4423f262
......@@ -45,10 +45,12 @@ class Note < ActiveRecord::Base
# Banzai::ObjectRenderer.
attr_accessor :redacted_note_html
# An Array containing the number of visible references as generated by
# Banzai::ObjectRenderer
# Number of user visible references as generated by Banzai::ObjectRenderer
attr_accessor :user_visible_reference_count
# Total of all references as generated by Banzai::ObjectRenderer
attr_accessor :total_reference_count
# Attribute used to store the attributes that have been changed by quick actions.
attr_accessor :commands_changes
......@@ -296,15 +298,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?
......@@ -474,9 +468,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
......@@ -12,6 +12,7 @@ class SystemNoteMetadata < ActiveRecord::Base
commit cross_reference
close duplicate
relate unrelate
moved
].freeze
ICON_TYPES = %w[
......@@ -29,4 +30,8 @@ class SystemNoteMetadata < ActiveRecord::Base
def icon_types
ICON_TYPES
end
def cross_reference_types
TYPES_WITH_CROSS_REFERENCES
end
end
......@@ -8,9 +8,20 @@ module EE
epic_issue_moved issue_changed_epic epic_date_changed
].freeze
EE_TYPES_WITH_CROSS_REFERENCES = %w[
relate unrelate
epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
epic_issue_moved issue_changed_epic
].freeze
override :icon_types
def icon_types
@icon_types ||= (super + EE_ICON_TYPES).freeze
end
override :cross_reference_types
def cross_reference_types
@cross_reference_types ||= (super + EE_TYPES_WITH_CROSS_REFERENCES).freeze
end
end
end
---
title: Properly filter private references from system notes
merge_request:
author:
type: security
......@@ -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
......
......@@ -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|
......
......@@ -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
end
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 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" do
expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy
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 false if user visible reference count set" do
note.user_visible_reference_count = 1
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
expect(note).not_to receive(:reference_mentionables)
expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy
it_behaves_like "checks references"
end
it "returns true if ref count is 0" do
note.user_visible_reference_count = 0
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_behaves_like "checks references"
expect(note).not_to receive(:reference_mentionables)
expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
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
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) }
......
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