Redact search results based on Ability.allowed?

In order to improve security we want to avoid any chance of leaking
search results that the user should not have access to. This approach
should account for bugs from incorrect queries or stale/incorrect data
in the index.

It happens that this also fixes
https://gitlab.com/gitlab-org/gitlab/issues/33712 for the time being as
we have not yet fixed the root cause of the problem which is that the
query is wrong.

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