Merge branch 'security-33689-post-filter-search-results' into 'master'

Filter out search results based on permissions to avoid bugs leaking data

See merge request gitlab/gitlab-ee!1388

app/models/discussion.rb

@@ -16,6 +16,7 @@ class Discussion
             :commit_id,
             :for_commit?,
             :for_merge_request?,
+            :noteable_ability_name,
             :to_ability_name,
             :editable?,
             :visible_for?,

app/models/milestone.rb

@@ -261,6 +261,10 @@ class Milestone < ApplicationRecord
     group || project
   end
 
+  def to_ability_name
+    model_name.singular
+  end
+
   def group_milestone?
     group_id.present?
   end

app/models/note.rb

@@ -361,6 +361,10 @@ class Note < ApplicationRecord
   end
 
   def to_ability_name
+    model_name.singular
+  end
+
+  def noteable_ability_name
     for_snippet? ? noteable.class.name.underscore : noteable_type.demodulize.underscore
   end
 

app/models/project.rb

@@ -1265,6 +1265,10 @@ class Project < ApplicationRecord
     end
   end
 
+  def to_ability_name
+    model_name.singular
+  end
+
   # rubocop: disable CodeReuse/ServiceClass
   def execute_hooks(data, hooks_scope = :push_hooks)
     run_after_commit_or_now do

app/policies/note_policy.rb

@@ -9,7 +9,7 @@ class NotePolicy < BasePolicy
 
   condition(:editable, scope: :subject) { @subject.editable? }
 
-  condition(:can_read_noteable) { can?(:"read_#{@subject.to_ability_name}") }
+  condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") }
 
   condition(:is_visible) { @subject.visible_for?(@user) }
 

app/services/notification_service.rb

@@ -281,7 +281,7 @@ class NotificationService
   end
 
   def send_new_note_notifications(note)
-    notify_method = "note_#{note.to_ability_name}_email".to_sym
+    notify_method = "note_#{note.noteable_ability_name}_email".to_sym
 
     recipients = NotificationRecipientService.build_new_note_recipients(note)
     recipients.each do |recipient|

ee/changelogs/unreleased/security-33689-post-filter-search-results.yml

+---
+title: Redact search results based on Ability.allowed?
+merge_request:
+author:
+type: security

ee/lib/gitlab/elastic/search_results.rb

@@ -167,9 +167,26 @@ module Gitlab
       def eager_load(es_result, page, eager:)
         paginated_base = es_result.page(page).per(per_page)
         relation = paginated_base.records.includes(eager) # rubocop:disable CodeReuse/ActiveRecord
+        filtered_results = []
+        permitted_results = relation.select do |o|
+          ability = :"read_#{o.to_ability_name}"
+          if Ability.allowed?(current_user, ability, o)
+            true
+          else
+            # Redact any search result the user may not have access to. This
+            # could be due to incorrect data in the index or a bug in our query
+            # so we log this as an error.
+            filtered_results << { ability: ability, id: o.id, class_name: o.class.name }
+            false
+          end
+        end
+
+        if filtered_results.any?
+          logger.error(message: "redacted_search_results", filtered: filtered_results, current_user_id: current_user&.id, query: query)
+        end
 
         Kaminari.paginate_array(
-          relation,
+          permitted_results,
           total_count: paginated_base.total_count,
           limit: per_page,
           offset: per_page * (page - 1)
@@ -363,6 +380,10 @@ module Gitlab
       def per_page
         20
       end
+
+      def logger
+        @logger ||= Gitlab::ProjectServiceLogger.build
+      end
     end
   end
 end

ee/spec/features/search/elastic/global_search_spec.rb

@@ -43,8 +43,10 @@ describe 'Global elastic search', :elastic do
       let(:object) { :project }
       let(:creation_args) { { namespace: user.namespace } }
       let(:path) { search_path(search: 'project*', scope: 'projects') }
-      # Each Project requires 4 extra queries: one for each "count" (forks, open MRs, open Issues) and one for access level
-      let(:query_count_multiplier) { 4 }
+      # Each Project requires 5 extra queries: one for each "count" (forks,
+      # open MRs, open Issues) and twice for access level. This should be fixed
+      # per https://gitlab.com/gitlab-org/gitlab/issues/34457
+      let(:query_count_multiplier) { 5 }
 
       it_behaves_like 'an efficient database result'
     end

ee/spec/lib/gitlab/elastic/search_results_spec.rb

@@ -215,6 +215,45 @@ describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need_inlin
       expect(results.objects('notes')).to be_empty
       expect(results.notes_count).to eq 0
     end
+
+    it 'redacts issue comments on public projects where issue has lower access_level' do
+      project_1.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+
+      results = described_class.new(user, 'foo', limit_project_ids)
+
+      expect(results.send(:logger))
+        .to receive(:error)
+        .with(hash_including(message: "redacted_search_results", filtered: array_including([
+          { class_name: "Note", id: @note_1.id, ability: :read_note },
+          { class_name: "Note", id: @note_2.id, ability: :read_note }
+      ])))
+
+      expect(results.notes_count).to eq(2) # 2 because redacting only happens when we instantiate the results
+      expect(results.objects('notes')).to be_empty
+    end
+
+    it 'redacts commit comments when user is a guest on a private project' do
+      project_1.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      project_1.add_guest(user)
+      note_on_commit = create(
+        :note_on_commit,
+        project: project_1,
+        note: 'foo note on commit'
+      )
+
+      Gitlab::Elastic::Helper.refresh_index
+
+      results = described_class.new(user, 'foo', limit_project_ids)
+
+      expect(results.send(:logger))
+        .to receive(:error)
+        .with(hash_including(message: "redacted_search_results", filtered: array_including([
+          { class_name: "Note", id: note_on_commit.id, ability: :read_note }
+      ])))
+
+      expect(results.notes_count).to eq(3) # 3 because redacting only happens when we instantiate the results
+      expect(results.objects('notes')).to match_array([@note_1, @note_2])
+    end
   end
 
   describe 'confidential issues' do
@@ -869,15 +908,20 @@ describe Gitlab::Elastic::SearchResults, :elastic, :sidekiq_might_not_need_inlin
 
       context 'when project_ids is not present' do
         context 'when project_ids is :any' do
-          it 'returns all milestones' do
+          it 'returns all milestones and redacts them when the user has no access' do
             results = described_class.new(user, 'project', :any)
+            expect(results.send(:logger))
+              .to receive(:error)
+              .with(hash_including(message: "redacted_search_results", filtered: [{ class_name: "Milestone", id: milestone_2.id, ability: :read_milestone }]))
+
             milestones = results.objects('milestones')
 
+            expect(results.milestones_count).to eq(4) # 4 because redacting only happens when we instantiate the results
+
             expect(milestones).to include(milestone_1)
-            expect(milestones).to include(milestone_2)
+            expect(milestones).not_to include(milestone_2)
             expect(milestones).to include(milestone_3)
             expect(milestones).to include(milestone_4)
-            expect(results.milestones_count).to eq(4)
           end
         end
 

spec/models/milestone_spec.rb

@@ -227,6 +227,14 @@ describe Milestone do
     end
   end
 
+  describe '#to_ability_name' do
+    it 'returns milestone' do
+      milestone = build(:milestone)
+
+      expect(milestone.to_ability_name).to eq('milestone')
+    end
+  end
+
   describe '.search' do
     let(:milestone) { create(:milestone, title: 'foo', description: 'bar') }
 

spec/models/note_spec.rb

@@ -578,24 +578,30 @@ describe Note do
   end
 
   describe '#to_ability_name' do
-    it 'returns snippet for a project snippet note' do
-      expect(build(:note_on_project_snippet).to_ability_name).to eq('project_snippet')
+    it 'returns note' do
+      expect(build(:note).to_ability_name).to eq('note')
+    end
+  end
+
+  describe '#noteable_ability_name' do
+    it 'returns project_snippet for a project snippet note' do
+      expect(build(:note_on_project_snippet).noteable_ability_name).to eq('project_snippet')
     end
 
     it 'returns personal_snippet for a personal snippet note' do
-      expect(build(:note_on_personal_snippet).to_ability_name).to eq('personal_snippet')
+      expect(build(:note_on_personal_snippet).noteable_ability_name).to eq('personal_snippet')
     end
 
     it 'returns merge_request for an MR note' do
-      expect(build(:note_on_merge_request).to_ability_name).to eq('merge_request')
+      expect(build(:note_on_merge_request).noteable_ability_name).to eq('merge_request')
     end
 
     it 'returns issue for an issue note' do
-      expect(build(:note_on_issue).to_ability_name).to eq('issue')
+      expect(build(:note_on_issue).noteable_ability_name).to eq('issue')
     end
 
-    it 'returns issue for a commit note' do
-      expect(build(:note_on_commit).to_ability_name).to eq('commit')
+    it 'returns commit for a commit note' do
+      expect(build(:note_on_commit).noteable_ability_name).to eq('commit')
     end
   end
 

spec/models/project_spec.rb

@@ -4444,6 +4444,14 @@ describe Project do
     end
   end
 
+  describe '#to_ability_name' do
+    it 'returns project' do
+      project = build(:project_empty_repo)
+
+      expect(project.to_ability_name).to eq('project')
+    end
+  end
+
   describe '#execute_hooks' do
     let(:data) { { ref: 'refs/heads/master', data: 'data' } }
     it 'executes active projects hooks with the specified scope' do