Merge branch 'data_leak' into 'master'

Confidential notes data leak

Fixes part of https://gitlab.com/gitlab-org/gitlab-ee/issues/575

See merge request !1967

4 files changed, 40 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 0ed726f0696..d00d3fab9a4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -15,6 +15,7 @@ v 8.8.3
   - Fix incorrect links on pipeline page when merge request created from fork.  !4376
   - Use downcased path to container repository as this is expected path by Docker. !4420
   - Fix wiki project clone address error (chujinjin). !4429
+  - In search results, only show notes on confidential issues that the user has access to
 
 v 8.8.2
   - Added remove due date button. !4209
diff --git a/app/models/note.rb b/app/models/note.rb
index 55b98557244..29f38539116 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -77,14 +77,30 @@ class Note < ActiveRecord::Base
     #
     # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
     #
-    # query - The search query as a String.
+    # query   - The search query as a String.
+    # as_user - Limit results to those viewable by a specific user
     #
     # Returns an ActiveRecord::Relation.
-    def search(query)
+    def search(query, as_user: nil)
       table   = arel_table
       pattern = "%#{query}%"
 
-      where(table[:note].matches(pattern))
+      found_notes = joins('LEFT JOIN issues ON issues.id = noteable_id').
+        where(table[:note].matches(pattern))
+
+      if as_user
+        found_notes.where('
+          issues.confidential IS NULL
+          OR issues.confidential IS FALSE
+          OR (issues.confidential IS TRUE
+            AND (issues.author_id = :user_id
+            OR issues.assignee_id = :user_id
+            OR issues.project_id IN(:project_ids)))',
+          user_id: as_user.id,
+          project_ids: as_user.authorized_projects.select(:id))
+      else
+        found_notes.where('issues.confidential IS NULL OR issues.confidential IS FALSE')
+      end
     end
 
     def grouped_awards
diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb
index 71c5b6801fb..183bd10d6a3 100644
--- a/lib/gitlab/project_search_results.rb
+++ b/lib/gitlab/project_search_results.rb
@@ -74,7 +74,7 @@ module Gitlab
     end
 
     def notes
-      project.notes.user.search(query).order('updated_at DESC')
+      project.notes.user.search(query, as_user: @current_user).order('updated_at DESC')
     end
 
     def commits
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 5d916f0e6a6..64a5bab7cb1 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -111,6 +111,25 @@ describe Note, models: true do
     it 'returns notes with matching content regardless of the casing' do
       expect(described_class.search('WOW')).to eq([note])
     end
+
+    context "confidential issues" do
+      let(:user) { create :user }
+      let(:confidential_issue) { create(:issue, :confidential, author: user) }
+      let(:confidential_note) { create :note, note: "Random", noteable: confidential_issue }
+
+      it "returns notes with matching content if user can see the issue" do
+        expect(described_class.search(confidential_note.note, as_user: user)).to eq([confidential_note])
+      end
+
+      it "does not return notes with matching content if user can not see the issue" do
+        user = create :user
+        expect(described_class.search(confidential_note.note, as_user: user)).to be_empty
+      end
+
+      it "does not return notes with matching content for unauthenticated users" do
+        expect(described_class.search(confidential_note.note)).to be_empty
+      end
+    end
   end
 
   describe '.grouped_awards' do