Merge branch 'security-12630-private-system-note-disclosed-in-graphql' into 'master'

Add policy check if cross reference system notes are accessible

See merge request gitlab/gitlab-ee!1281

app/models/discussion.rb

@@ -18,6 +18,7 @@ class Discussion
             :for_merge_request?,
             :to_ability_name,
             :editable?,
+            :visible_for?,
 
             to: :first_note
 

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

changelogs/unreleased/security-12630-private-system-note-disclosed-in-graphql.yml

+---
+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

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

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