Fix reference checks

Because some system notes may have more than one references, we
need to check if all references are visible to user, not only
if there is any visible reference.

app/models/note.rb

@@ -545,7 +545,8 @@ class Note < ApplicationRecord
       # 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?
+      refs = all_references(user)
+      refs.all.any? && refs.stateful_not_visible_counter == 0
     end
   end
 

changelogs/unreleased/security-reference-check.yml

+---
+title: Make sure that only system notes where all references are visible to user are exposed in GraphQL API.
+merge_request:
+author:
+type: security

lib/banzai/reference_parser/base_parser.rb

@@ -201,12 +201,14 @@ module Banzai
         gather_references(nodes)
       end
 
-      # Gathers the references for the given HTML nodes.
+      # Gathers the references for the given HTML nodes.  Returns visible
+      # references and a list of nodes which are not visible to the user
       def gather_references(nodes)
         nodes = nodes_user_can_reference(current_user, nodes)
-        nodes = nodes_visible_to_user(current_user, nodes)
+        visible = nodes_visible_to_user(current_user, nodes)
+        not_visible = nodes - visible
 
-        referenced_by(nodes)
+        { visible: referenced_by(visible), not_visible: not_visible }
       end
 
       # Returns a Hash containing the projects for a given list of HTML nodes.

lib/gitlab/reference_extractor.rb

@@ -6,11 +6,16 @@ module Gitlab
     REFERABLES = %i(user issue label milestone mentioned_user mentioned_group mentioned_project
                     merge_request snippet commit commit_range directly_addressed_user epic).freeze
     attr_accessor :project, :current_user, :author
+    # This counter is increased by a number of references filtered out by
+    # banzai reference exctractor. Note that this counter is stateful and
+    # not idempotent and is increased whenever you call `references`.
+    attr_reader :stateful_not_visible_counter
 
     def initialize(project, current_user = nil)
       @project = project
       @current_user = current_user
       @references = {}
+      @stateful_not_visible_counter = 0
 
       super()
     end
@@ -20,11 +25,15 @@ module Gitlab
     end
 
     def references(type)
-      super(type, project, current_user)
+      refs = super(type, project, current_user)
+      @stateful_not_visible_counter += refs[:not_visible].count
+
+      refs[:visible]
     end
 
     def reset_memoized_values
       @references = {}
+      @stateful_not_visible_counter = 0
       super()
     end
 

spec/lib/banzai/reference_parser/mentioned_group_parser_spec.rb

@@ -19,7 +19,7 @@ describe Banzai::ReferenceParser::MentionedGroupParser do
         it 'returns empty array' do
           link['data-group'] = project.group.id.to_s
 
-          expect(subject.gather_references([link])).to eq([])
+          expect_gathered_references(subject.gather_references([link]), [], 1)
         end
       end
 
@@ -30,7 +30,7 @@ describe Banzai::ReferenceParser::MentionedGroupParser do
         end
 
         it 'returns groups' do
-          expect(subject.gather_references([link])).to eq([group])
+          expect_gathered_references(subject.gather_references([link]), [group], 0)
         end
       end
 
@@ -38,7 +38,7 @@ describe Banzai::ReferenceParser::MentionedGroupParser do
         it 'returns an empty Array' do
           link['data-group'] = 'test-non-existing'
 
-          expect(subject.gather_references([link])).to eq([])
+          expect_gathered_references(subject.gather_references([link]), [], 1)
         end
       end
     end

spec/lib/banzai/reference_parser/mentioned_project_parser_spec.rb

@@ -19,7 +19,7 @@ describe Banzai::ReferenceParser::MentionedProjectParser do
         it 'returns empty Array' do
           link['data-project'] = project.id.to_s
 
-          expect(subject.gather_references([link])).to eq([])
+          expect_gathered_references(subject.gather_references([link]), [], 1)
         end
       end
 
@@ -30,7 +30,7 @@ describe Banzai::ReferenceParser::MentionedProjectParser do
         end
 
         it 'returns an Array of referenced projects' do
-          expect(subject.gather_references([link])).to eq([project])
+          expect_gathered_references(subject.gather_references([link]), [project], 0)
         end
       end
 
@@ -38,7 +38,7 @@ describe Banzai::ReferenceParser::MentionedProjectParser do
         it 'returns an empty Array' do
           link['data-project'] = 'inexisting-project-id'
 
-          expect(subject.gather_references([link])).to eq([])
+          expect_gathered_references(subject.gather_references([link]), [], 1)
         end
       end
     end

spec/lib/banzai/reference_parser/mentioned_user_parser_spec.rb

@@ -22,7 +22,7 @@ describe Banzai::ReferenceParser::MentionedUserParser do
         end
 
         it 'returns empty list of users' do
-          expect(subject.gather_references([link])).to eq([])
+          expect_gathered_references(subject.gather_references([link]), [], 0)
         end
       end
     end
@@ -35,7 +35,7 @@ describe Banzai::ReferenceParser::MentionedUserParser do
         end
 
         it 'returns empty list of users' do
-          expect(subject.gather_references([link])).to eq([])
+          expect_gathered_references(subject.gather_references([link]), [], 0)
         end
       end
     end
@@ -44,7 +44,7 @@ describe Banzai::ReferenceParser::MentionedUserParser do
       it 'returns an Array of users' do
         link['data-user'] = user.id.to_s
 
-        expect(subject.referenced_by([link])).to eq([user])
+        expect_gathered_references(subject.gather_references([link]), [user], 0)
       end
     end
   end

spec/lib/banzai/reference_parser/project_parser_spec.rb

@@ -17,7 +17,7 @@ describe Banzai::ReferenceParser::ProjectParser do
         it 'returns an Array of projects' do
           link['data-project'] = project.id.to_s
 
-          expect(subject.gather_references([link])).to eq([project])
+          expect_gathered_references(subject.gather_references([link]), [project], 0)
         end
       end
 
@@ -25,7 +25,7 @@ describe Banzai::ReferenceParser::ProjectParser do
         it 'returns an empty Array' do
           link['data-project'] = ''
 
-          expect(subject.gather_references([link])).to eq([])
+          expect_gathered_references(subject.gather_references([link]), [], 1)
         end
       end
 
@@ -35,7 +35,7 @@ describe Banzai::ReferenceParser::ProjectParser do
 
           link['data-project'] = private_project.id.to_s
 
-          expect(subject.gather_references([link])).to eq([])
+          expect_gathered_references(subject.gather_references([link]), [], 1)
         end
 
         it 'returns an Array when authorized' do
@@ -43,7 +43,7 @@ describe Banzai::ReferenceParser::ProjectParser do
 
           link['data-project'] = private_project.id.to_s
 
-          expect(subject.gather_references([link])).to eq([private_project])
+          expect_gathered_references(subject.gather_references([link]), [private_project], 0)
         end
       end
     end

spec/lib/gitlab/reference_extractor_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Gitlab::ReferenceExtractor do
-  let(:project) { create(:project) }
+  let_it_be(:project) { create(:project) }
 
   before do
     project.add_developer(project.creator)
@@ -293,4 +293,34 @@ describe Gitlab::ReferenceExtractor do
       end
     end
   end
+
+  describe '#references' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:issue) { create(:issue, project: project) }
+    let(:text) { "Ref. #{issue.to_reference}" }
+
+    subject { described_class.new(project, user) }
+
+    before do
+      subject.analyze(text)
+    end
+
+    context 'when references are visible' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'returns visible references of given type' do
+        expect(subject.references(:issue)).to eq([issue])
+      end
+
+      it 'does not increase stateful_not_visible_counter' do
+        expect { subject.references(:issue) }.not_to change { subject.stateful_not_visible_counter }
+      end
+    end
+
+    it 'increases stateful_not_visible_counter' do
+      expect { subject.references(:issue) }.to change { subject.stateful_not_visible_counter }.by(1)
+    end
+  end
 end

spec/models/note_spec.rb

@@ -350,12 +350,12 @@ describe Note do
   end
 
   describe "cross_reference_not_visible_for?" do
-    let(:private_user)    { create(:user) }
-    let(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
-    let(:private_issue)   { create(:issue, project: private_project) }
+    let_it_be(:private_user)    { create(:user) }
+    let_it_be(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
+    let_it_be(:private_issue)   { create(:issue, project: private_project) }
 
-    let(:ext_proj)  { create(:project, :public) }
-    let(:ext_issue) { create(:issue, project: ext_proj) }
+    let_it_be(:ext_proj)  { create(:project, :public) }
+    let_it_be(:ext_issue) { create(:issue, project: ext_proj) }
 
     shared_examples "checks references" do
       it "returns true" do
@@ -393,10 +393,24 @@ describe Note do
       it_behaves_like "checks references"
     end
 
-    context "when there are two references in note" do
+    context "when there is a reference to a label" do
+      let_it_be(:private_label) { create(:label, project: private_project) }
       let(:note) do
         create :note,
           noteable: ext_issue, project: ext_proj,
+          note: "added label #{private_label.to_reference(ext_proj)}",
+          system: true
+      end
+      let!(:system_note_metadata) { create(:system_note_metadata, note: note, action: :label) }
+
+      it_behaves_like "checks references"
+    end
+
+    context "when there are two references in note" do
+      let_it_be(:ext_issue2) { create(:issue, project: ext_proj) }
+      let(:note) do
+        create :note,
+          noteable: ext_issue2, project: ext_proj,
           note: "mentioned in issue #{private_issue.to_reference(ext_proj)} and " \
                 "public issue #{ext_issue.to_reference(ext_proj)}",
           system: true

spec/support/helpers/reference_parser_helpers.rb

@@ -5,6 +5,11 @@ module ReferenceParserHelpers
     Nokogiri::HTML.fragment('<a></a>').children[0]
   end
 
+  def expect_gathered_references(result, visible, not_visible_count)
+    expect(result[:visible]).to eq(visible)
+    expect(result[:not_visible].count).to eq(not_visible_count)
+  end
+
   shared_examples 'no project N+1 queries' do
     it 'avoids N+1 queries in #nodes_visible_to_user', :request_store do
       context = Banzai::RenderContext.new(project, user)