summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAlexandru Croitor <acroitor@gitlab.com>2019-09-18 11:26:20 +0300
committerAlexandru Croitor <acroitor@gitlab.com>2019-09-25 11:56:19 +0300
commitc01ad00ebd7e6f5979e426b735582632d54f36f8 (patch)
tree1f1aee9dda0767acbbcaa63cbf25996dec3049a7
parent0be35b492ea2c2f447c11c7b94e81da0db7a7d56 (diff)
downloadgitlab-ce-c01ad00ebd7e6f5979e426b735582632d54f36f8.tar.gz
Add policy check if cross reference system notes are accessible
-rw-r--r--app/models/discussion.rb1
-rw-r--r--app/policies/note_policy.rb9
-rw-r--r--changelogs/unreleased/security-12630-private-system-note-disclosed-in-graphql.yml6
-rw-r--r--spec/graphql/types/issue_type_spec.rb79
-rw-r--r--spec/policies/note_policy_spec.rb83
5 files changed, 178 insertions, 0 deletions
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index dd896f77084..0d066d0d99f 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -18,6 +18,7 @@ class Discussion
:for_merge_request?,
:to_ability_name,
:editable?,
+ :visible_for?,
to: :first_note
diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb
index 8d23e3abed3..b2af6c874c7 100644
--- a/app/policies/note_policy.rb
+++ b/app/policies/note_policy.rb
@@ -11,6 +11,8 @@ class NotePolicy < BasePolicy
condition(:can_read_noteable) { can?(:"read_#{@subject.to_ability_name}") }
+ condition(:is_visible) { @subject.visible_for?(@user) }
+
rule { ~editable }.prevent :admin_note
# If user can't read the issue/MR/etc then they should not be allowed to do anything to their own notes
@@ -27,6 +29,13 @@ class NotePolicy < BasePolicy
enable :resolve_note
end
+ rule { ~is_visible }.policy do
+ prevent :read_note
+ prevent :admin_note
+ prevent :resolve_note
+ prevent :award_emoji
+ end
+
rule { is_noteable_author }.policy do
enable :resolve_note
end
diff --git a/changelogs/unreleased/security-12630-private-system-note-disclosed-in-graphql.yml b/changelogs/unreleased/security-12630-private-system-note-disclosed-in-graphql.yml
new file mode 100644
index 00000000000..03658c931a3
--- /dev/null
+++ b/changelogs/unreleased/security-12630-private-system-note-disclosed-in-graphql.yml
@@ -0,0 +1,6 @@
+---
+title: Add a policy check for system notes that may not be visible due to cross references
+ to private items
+merge_request:
+author:
+type: security
diff --git a/spec/graphql/types/issue_type_spec.rb b/spec/graphql/types/issue_type_spec.rb
index 799a8662b94..734e5af3cd8 100644
--- a/spec/graphql/types/issue_type_spec.rb
+++ b/spec/graphql/types/issue_type_spec.rb
@@ -17,4 +17,83 @@ describe GitlabSchema.types['Issue'] do
expect(described_class).to have_graphql_field(field_name)
end
end
+
+ describe "issue notes" do
+ let(:user) { create(:user) }
+ let(:project) { create(:project, :public) }
+ let(:issue) { create(:issue, project: project) }
+ let(:confidential_issue) { create(:issue, :confidential, project: project) }
+ let(:private_note_body) { "mentioned in issue #{confidential_issue.to_reference(project)}" }
+ let!(:note1) { create(:note, system: true, noteable: issue, author: user, project: project, note: private_note_body) }
+ let!(:note2) { create(:note, system: true, noteable: issue, author: user, project: project, note: 'public note') }
+
+ let(:query) do
+ %(
+ query {
+ project(fullPath:"#{project.full_path}"){
+ issue(iid:"#{issue.iid}"){
+ descriptionHtml
+ notes{
+ edges{
+ node{
+ bodyHtml
+ author{
+ username
+ }
+ body
+ }
+ }
+ }
+ }
+ }
+ }
+ )
+ end
+
+ context 'query issue notes' do
+ subject { GitlabSchema.execute(query, context: { current_user: current_user }).as_json }
+
+ shared_examples_for 'does not include private notes' do
+ it "does not return private notes" do
+ notes = subject.dig("data", "project", "issue", "notes", 'edges')
+ notes_body = notes.map {|n| n.dig('node', 'body')}
+
+ expect(notes.size).to eq 1
+ expect(notes_body).not_to include(private_note_body)
+ expect(notes_body).to include('public note')
+ end
+ end
+
+ shared_examples_for 'includes private notes' do
+ it "returns all notes" do
+ notes = subject.dig("data", "project", "issue", "notes", 'edges')
+ notes_body = notes.map {|n| n.dig('node', 'body')}
+
+ expect(notes.size).to eq 2
+ expect(notes_body).to include(private_note_body)
+ expect(notes_body).to include('public note')
+ end
+ end
+
+ context 'when user signed in' do
+ let(:current_user) { user }
+
+ it_behaves_like 'does not include private notes'
+
+ context 'when user member of the project' do
+ before do
+ project.add_developer(user)
+ end
+
+ it_behaves_like 'includes private notes'
+ end
+ end
+
+ context 'when user is anonymous' do
+ let(:current_user) { nil }
+
+ it_behaves_like 'does not include private notes'
+ end
+ end
+ end
end
diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb
index bcf021f1dfd..d18ded8bce9 100644
--- a/spec/policies/note_policy_spec.rb
+++ b/spec/policies/note_policy_spec.rb
@@ -152,6 +152,89 @@ describe NotePolicy do
it_behaves_like 'a discussion with a private noteable'
end
end
+
+ context 'when it is a system note' do
+ let(:developer) { create(:user) }
+ let(:any_user) { create(:user) }
+
+ shared_examples_for 'user can read the note' do
+ it 'allows the user to read the note' do
+ expect(policy).to be_allowed(:read_note)
+ end
+ end
+
+ shared_examples_for 'user can act on the note' do
+ it 'allows the user to read the note' do
+ expect(policy).not_to be_allowed(:admin_note)
+ expect(policy).to be_allowed(:resolve_note)
+ expect(policy).to be_allowed(:award_emoji)
+ end
+ end
+
+ shared_examples_for 'user cannot read or act on the note' do
+ it 'allows user to read the note' do
+ expect(policy).not_to be_allowed(:admin_note)
+ expect(policy).not_to be_allowed(:resolve_note)
+ expect(policy).not_to be_allowed(:read_note)
+ expect(policy).not_to be_allowed(:award_emoji)
+ end
+ end
+
+ context 'when noteable is a public issue' do
+ let(:note) { create(:note, system: true, noteable: noteable, author: user, project: project) }
+
+ before do
+ project.add_developer(developer)
+ end
+
+ context 'when user is project member' do
+ let(:policy) { described_class.new(developer, note) }
+
+ it_behaves_like 'user can read the note'
+ it_behaves_like 'user can act on the note'
+ end
+
+ context 'when user is not project member' do
+ let(:policy) { described_class.new(any_user, note) }
+
+ it_behaves_like 'user can read the note'
+ end
+
+ context 'when user is anonymous' do
+ let(:policy) { described_class.new(nil, note) }
+
+ it_behaves_like 'user can read the note'
+ end
+ end
+
+ context 'when it is a system note referencing a confidential issue' do
+ let(:confidential_issue) { create(:issue, :confidential, project: project) }
+ let(:note) { create(:note, system: true, noteable: issue, author: user, project: project, note: "mentioned in issue #{confidential_issue.to_reference(project)}") }
+
+ before do
+ project.add_developer(developer)
+ end
+
+ context 'when user is project member' do
+ let(:policy) { described_class.new(developer, note) }
+
+ it_behaves_like 'user can read the note'
+ it_behaves_like 'user can act on the note'
+ end
+
+ context 'when user is not project member' do
+ let(:policy) { described_class.new(any_user, note) }
+
+ it_behaves_like 'user cannot read or act on the note'
+ end
+
+ context 'when user is anonymous' do
+ let(:policy) { described_class.new(nil, note) }
+
+ it_behaves_like 'user cannot read or act on the note'
+ end
+ end
+ end
end
end
end