Merge branch 'port-ee-3435' into 'security-10-0'

[10.0 CE] Prevent "Related Issues" from leaking confidential issues

See merge request gitlab/gitlabhq!2193

4 files changed, 50 insertions, 23 deletions

diff --git a/app/models/note.rb b/app/models/note.rb
index d0e3bc0bfed..9a4f201f678 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -144,7 +144,7 @@ class Note < ActiveRecord::Base
   end
 
   def cross_reference?
-    system? && SystemNoteService.cross_reference?(note)
+    system? && matches_cross_reference_regex?
   end
 
   def diff_note?
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 1763f64a4e4..d442eb27f74 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -162,7 +162,6 @@ module SystemNoteService
   #   "changed time estimate to 3d 5h"
   #
   # Returns the created Note object
-
   def change_time_estimate(noteable, project, author)
     parsed_time = Gitlab::TimeTrackingFormatter.output(noteable.time_estimate)
     body = if noteable.time_estimate == 0
@@ -188,7 +187,6 @@ module SystemNoteService
   #   "added 2h 30m of time spent"
   #
   # Returns the created Note object
-
   def change_time_spent(noteable, project, author)
     time_spent = noteable.time_spent
 
@@ -451,10 +449,6 @@ module SystemNoteService
     end
   end
 
-  def cross_reference?(note_text)
-    note_text =~ /\A#{cross_reference_note_prefix}/i
-  end
-
   # Check if a cross-reference is disallowed
   #
   # This method prevents adding a "mentioned in !1" note on every single commit
@@ -484,7 +478,6 @@ module SystemNoteService
   # mentioner - Mentionable object
   #
   # Returns Boolean
-
   def cross_reference_exists?(noteable, mentioner)
     # Initial scope should be system notes of this noteable type
     notes = Note.system.where(noteable_type: noteable.class)
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index bdee3894a13..b227d0bb807 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -226,7 +226,7 @@ describe Projects::IssuesController do
           id: issue.iid,
           issue: { assignee_ids: [assignee.id] },
           format: :json
-        body = JSON.parse(response.body)
+        body = json_response
 
         expect(body['assignees'].first.keys)
           .to match_array(%w(id name username avatar_url state web_url))
@@ -877,4 +877,52 @@ describe Projects::IssuesController do
                                   format: :json
     end
   end
+
+  describe 'GET #discussions' do
+    let!(:discussion) { create(:discussion_note_on_issue, noteable: issue, project: issue.project) }
+    context 'when authenticated' do
+      before do
+        project.add_developer(user)
+        sign_in(user)
+      end
+
+      it 'returns discussion json' do
+        get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
+
+        expect(json_response.first.keys).to match_array(%w[id reply_id expanded notes individual_note])
+      end
+    end
+
+    context 'with cross-reference system note', :request_store do
+      let(:new_issue) { create(:issue) }
+      let(:cross_reference) { "mentioned in #{new_issue.to_reference(issue.project)}" }
+
+      before do
+        create(:discussion_note_on_issue, :system, noteable: issue, project: issue.project, note: cross_reference)
+      end
+
+      it 'filters notes that the user should not see' do
+        get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
+
+        expect(JSON.parse(response.body).count).to eq(1)
+      end
+
+      it 'does not result in N+1 queries' do
+        # Instantiate the controller variables to ensure QueryRecorder has an accurate base count
+        get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
+
+        RequestStore.clear!
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
+        end.count
+
+        RequestStore.clear!
+
+        create_list(:discussion_note_on_issue, 2, :system, noteable: issue, project: issue.project, note: cross_reference)
+
+        expect { get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid }.not_to exceed_query_limit(control_count)
+      end
+    end
+  end
 end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 8f1eb4863d9..5781c62afbc 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -530,20 +530,6 @@ describe SystemNoteService do
     end
   end
 
-  describe '.cross_reference?' do
-    it 'is truthy when text begins with expected text' do
-      expect(described_class.cross_reference?('mentioned in something')).to be_truthy
-    end
-
-    it 'is truthy when text begins with legacy capitalized expected text' do
-      expect(described_class.cross_reference?('mentioned in something')).to be_truthy
-    end
-
-    it 'is falsey when text does not begin with expected text' do
-      expect(described_class.cross_reference?('this is a note')).to be_falsey
-    end
-  end
-
   describe '.cross_reference_disallowed?' do
     context 'when mentioner is not a MergeRequest' do
       it 'is falsey' do