From c99402c05f18c6ca8dd7b64c59527abe1e6e80d4 Mon Sep 17 00:00:00 2001 From: Alexandru Croitor Date: Wed, 18 Sep 2019 11:26:20 +0300 Subject: [PATCH] Add policy check if cross reference system notes are accessible --- app/models/discussion.rb | 1 + app/policies/note_policy.rb | 9 ++ ...ivate-system-note-disclosed-in-graphql.yml | 6 ++ spec/graphql/types/issue_type_spec.rb | 79 ++++++++++++++++++ spec/policies/note_policy_spec.rb | 83 +++++++++++++++++++ 5 files changed, 178 insertions(+) create mode 100644 changelogs/unreleased/security-12630-private-system-note-disclosed-in-graphql.yml 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