From 7fded45254ddbb641289e88ccc7daa67708ac060 Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Wed, 22 Nov 2023 17:47:32 +0100
Subject: [PATCH 1/9] Update secrets push check to display file path and commit
 sha

This commit updates the secrets push check to display blob metadata such
as file path and commit sha. This uses `GetTreeEntries` gitaly RPC under
the hood fetch this information per revision, and cross reference them
with blobs where a secret is found.
---
 .../gitlab/checks/push_rules/secrets_check.rb | 60 +++++++++++++++----
 .../lib/gitlab/secret_detection/finding.rb    |  9 ++-
 2 files changed, 54 insertions(+), 15 deletions(-)

diff --git a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
index ec7b50696c6d7c..579860ac351da2 100644
--- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
+++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
@@ -21,12 +21,8 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker
             found_secrets_post_message: "\n\nPlease remove the identified secrets in your commits and try again.",
             found_secrets_with_errors: 'Secret detection scan completed with one or more findings ' \
                                        'but some errors occured during the scan.',
-            finding_details_message: <<~MESSAGE
-              \n\nBlob id: %{blob_id}
-              -- Line: %{line_number}
-              -- Type: %{type}
-              -- Description: %{description}\n
-            MESSAGE
+            finding_details_message: "\n\n-- Found secret at %{path}:%{line_number} in " \
+                                     "commit %{sha} of type %{type} (%{description})\n\n"
           }.freeze
 
           BLOB_BYTES_LIMIT = 1.megabyte # Limit is 1MiB to start with.
@@ -67,13 +63,7 @@ def validate!
                   # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136896#note_1680680116 for more details.
                   filter_existing(all_blobs)
                 else
-                  revisions = changes_access
-                    .changes
-                    .pluck(:newrev) # rubocop:disable CodeReuse/ActiveRecord -- Array#pluck
-                    .reject { |revision| ::Gitlab::Git.blank_ref?(revision) }
-                    .compact
-
-                  # We add `--not --all --not revisions` to ensure we only get new blobs.
+                  # We use `--not --all --not revisions` to ensure we only get new blobs.
                   project.repository.list_blobs(
                     ['--not', '--all', '--not'] + revisions,
                     bytes_limit: BLOB_BYTES_LIMIT + 1,
@@ -127,6 +117,14 @@ def filter_existing(blobs)
           end
 
           def format_response(response)
+            # Try to retrieve file path and commit sha for the blobs found.
+            if [
+              ::Gitlab::SecretDetection::Status::FOUND,
+              ::Gitlab::SecretDetection::Status::FOUND_WITH_ERRORS
+            ].include?(response.status)
+              get_file_path_and_commit_sha(response)
+            end
+
             case response.status
             when ::Gitlab::SecretDetection::Status::NOT_FOUND
               # No secrets found, we log and skip the check.
@@ -182,6 +180,42 @@ def format_response(response)
               secret_detection_logger.error(message: ERROR_MESSAGES[:invalid_scan_status_code_error])
             end
           end
+
+          def revisions
+            @revisions ||= changes_access
+                            .changes
+                            .pluck(:newrev) # rubocop:disable CodeReuse/ActiveRecord -- Array#pluck
+                            .reject { |revision| ::Gitlab::Git.blank_ref?(revision) }
+                            .compact
+          end
+
+          def get_file_path_and_commit_sha(response)
+            # Scanning had found secrets, let's try to look up their file path and commit id. This can be done
+            # by using `GetTreeEntries()` RPC, and cross examining blobs with ones where secrets where found.
+            tree_entries = revisions.flat_map do |revision|
+              # We don't care for the cursor at the moment, so we only get the entries (first element).
+              entries = ::Gitlab::Git::Tree.tree_entries(project.repository, revision, nil, false, true, {})
+                .first
+                .reject { |entry| entry.type != :blob }
+
+              # Let's grab the `commit_id` and the `path` for that entry, we use the blob id as key.
+              entries.each_with_object({}) do |entry, hash|
+                hash[entry.id] = {
+                  sha: entry.commit_id,
+                  path: entry.path
+                }
+              end
+            end.reduce(&:merge)
+
+            # update response with the commit sha and file path
+            response
+              .results
+              .reject { |finding| finding.status != ::Gitlab::SecretDetection::Status::FOUND }
+              .each do |finding|
+              finding.sha = tree_entries[finding.blob_id].sha
+              finding.path = tree_entries[finding.blob_id].path
+            end
+          end
         end
       end
     end
diff --git a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
index e8d83178b09dc9..e3633fadb2a130 100644
--- a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
+++ b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
@@ -5,13 +5,16 @@ module SecretDetection
     # Finding is a data object representing a secret finding identified within a blob
     class Finding
       attr_reader :blob_id, :status, :line_number, :type, :description
+      attr_accessor :sha, :path
 
-      def initialize(blob_id, status, line_number = nil, type = nil, description = nil)
+      def initialize(blob_id, status, line_number = nil, type = nil, description = nil, sha = nil, path = nil) # rubocop:disable Metrics/ParameterLists
         @blob_id = blob_id
         @status = status
         @line_number = line_number
         @type = type
         @description = description
+        @sha = sha
+        @path = path
       end
 
       def ==(other)
@@ -24,7 +27,9 @@ def to_h
           status: status,
           line_number: line_number,
           type: type,
-          description: description
+          description: description,
+          sha: sha,
+          path: path
         }
       end
 
-- 
GitLab


From 8953c60335e66c2fc930653e257ee2545f095d9c Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Thu, 7 Dec 2023 23:26:21 +0100
Subject: [PATCH 2/9] Clean up and update specs

---
 .../gitlab/checks/push_rules/secrets_check.rb | 74 ++++++++++++-------
 .../secrets_check_shared_contexts.rb          | 37 +++++++---
 .../gitlab/secrets_check_shared_examples.rb   | 73 +++++++++++++++++-
 .../lib/gitlab/secret_detection/finding.rb    |  2 +-
 4 files changed, 143 insertions(+), 43 deletions(-)

diff --git a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
index 579860ac351da2..8336af2e1d912b 100644
--- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
+++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
@@ -21,8 +21,8 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker
             found_secrets_post_message: "\n\nPlease remove the identified secrets in your commits and try again.",
             found_secrets_with_errors: 'Secret detection scan completed with one or more findings ' \
                                        'but some errors occured during the scan.',
-            finding_details_message: "\n\n-- Found secret at %{path}:%{line_number} in " \
-                                     "commit %{sha} of type %{type} (%{description})\n\n"
+            finding_message_commit: "\n\nSecrets leaked in commit: %{sha}",
+            finding_message_details: "\n  -- %{path}:%{line_number} | %{description}"
           }.freeze
 
           BLOB_BYTES_LIMIT = 1.megabyte # Limit is 1MiB to start with.
@@ -122,6 +122,7 @@ def format_response(response)
               ::Gitlab::SecretDetection::Status::FOUND,
               ::Gitlab::SecretDetection::Status::FOUND_WITH_ERRORS
             ].include?(response.status)
+              # TODO: filter out revisions not related to found secrets
               get_file_path_and_commit_sha(response)
             end
 
@@ -131,13 +132,7 @@ def format_response(response)
               secret_detection_logger.info(message: LOG_MESSAGES[:secrets_not_found])
             when ::Gitlab::SecretDetection::Status::FOUND
               # One or more secrets found, generate message with findings and fail check.
-              message = LOG_MESSAGES[:found_secrets]
-
-              response.results.each do |finding|
-                message += format(LOG_MESSAGES[:finding_details_message], finding.to_h)
-              end
-
-              message += LOG_MESSAGES[:found_secrets_post_message]
+              message = generate_found_message(response)
 
               secret_detection_logger.info(message: LOG_MESSAGES[:found_secrets])
 
@@ -145,21 +140,7 @@ def format_response(response)
             when ::Gitlab::SecretDetection::Status::FOUND_WITH_ERRORS
               # One or more secrets found, but with scan errors, so we
               # generate a message with findings and errors, and fail the check.
-
-              message = LOG_MESSAGES[:found_secrets_with_errors]
-
-              response.results.each do |finding|
-                case finding.status
-                when ::Gitlab::SecretDetection::Status::FOUND
-                  message += format(LOG_MESSAGES[:finding_details_message], finding.to_h)
-                when ::Gitlab::SecretDetection::Status::SCAN_ERROR
-                  message += format(ERROR_MESSAGES[:failed_to_scan_regex_error], finding.to_h)
-                when ::Gitlab::SecretDetection::Status::BLOB_TIMEOUT
-                  message += format(ERROR_MESSAGES[:blob_timed_out_error], finding.to_h)
-                end
-              end
-
-              message += LOG_MESSAGES[:found_secrets_post_message]
+              message = generate_found_with_errors_message(response)
 
               secret_detection_logger.info(message: LOG_MESSAGES[:found_secrets_with_errors])
 
@@ -189,12 +170,49 @@ def revisions
                             .compact
           end
 
+          def generate_found_message(response)
+            message = LOG_MESSAGES[:found_secrets]
+
+            response.results.group_by(&:sha).each do |commit_sha, results|
+              message += format(LOG_MESSAGES[:finding_message_commit], { sha: commit_sha })
+
+              results.each do |result|
+                message += format(LOG_MESSAGES[:finding_message_details], result.to_h)
+              end
+            end
+
+            message += LOG_MESSAGES[:found_secrets_post_message]
+            message
+          end
+
+          def generate_found_with_errors_message(response)
+            message = LOG_MESSAGES[:found_secrets_with_errors]
+
+            response.results.group_by(&:sha).each do |commit_sha, results|
+              results.each do |result|
+                case result.status
+                when ::Gitlab::SecretDetection::Status::FOUND
+                  message += format(LOG_MESSAGES[:finding_message_commit], { sha: commit_sha })
+                  message += format(LOG_MESSAGES[:finding_message_details], result.to_h)
+                when ::Gitlab::SecretDetection::Status::SCAN_ERROR
+                  message += format(ERROR_MESSAGES[:failed_to_scan_regex_error], result.to_h)
+                when ::Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+                  message += format(ERROR_MESSAGES[:blob_timed_out_error], result.to_h)
+                end
+              end
+            end
+
+            message += LOG_MESSAGES[:found_secrets_post_message]
+            message
+          end
+
           def get_file_path_and_commit_sha(response)
             # Scanning had found secrets, let's try to look up their file path and commit id. This can be done
             # by using `GetTreeEntries()` RPC, and cross examining blobs with ones where secrets where found.
             tree_entries = revisions.flat_map do |revision|
-              # We don't care for the cursor at the moment, so we only get the entries (first element).
-              entries = ::Gitlab::Git::Tree.tree_entries(project.repository, revision, nil, false, true, {})
+              # We don't care for the cursor at the moment, so we only get the first
+              # element (which is `entries`). Then we also just look at entries of type blob.
+              entries = ::Gitlab::Git::Tree.tree_entries(project.repository, revision, nil, false, true, nil)
                 .first
                 .reject { |entry| entry.type != :blob }
 
@@ -212,8 +230,8 @@ def get_file_path_and_commit_sha(response)
               .results
               .reject { |finding| finding.status != ::Gitlab::SecretDetection::Status::FOUND }
               .each do |finding|
-              finding.sha = tree_entries[finding.blob_id].sha
-              finding.path = tree_entries[finding.blob_id].path
+              finding.sha = tree_entries[finding.blob_id][:sha]
+              finding.path = tree_entries[finding.blob_id][:path]
             end
           end
         end
diff --git a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
index 84637cde1376e4..8c0efb998fc982 100644
--- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
+++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
@@ -60,6 +60,21 @@
   let(:old_blob) { have_attributes(class: Gitlab::Git::Blob, id: old_blob_reference, size: 23) }
   let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 33) }
 
+  # Used for mocking calls to `tree_entries` methods.
+  let(:tree_entries) do
+    [
+      Gitlab::Git::Tree.new(
+        id: new_blob_reference,
+        type: :blob,
+        mode: '100644',
+        name: '.env',
+        path: '.env',
+        flat_path: '.env',
+        commit_id: new_commit
+      )
+    ]
+  end
+
   # Used for mocking calls to logger.
   let(:secret_detection_logger) { instance_double(::Gitlab::SecretDetectionLogger) }
 
@@ -107,25 +122,20 @@
     'Secret detection scan completed with one or more findings but some errors occured during the scan.'
   end
 
+  let(:found_secret_path) { '.env' }
   let(:found_secret_line_number) { '1' }
-  let(:found_secret_type) { 'gitlab_personal_access_token' }
   let(:found_secret_description) { 'GitLab Personal Access Token' }
 
-  let(:found_secrets_message) do
-    message = <<~MESSAGE
-      \nBlob id: %{found_secret_blob_id}
-      -- Line: %{found_secret_line_number}
-      -- Type: %{found_secret_type}
-      -- Description: %{found_secret_description}\n
-    MESSAGE
+  let(:found_message_commit) { format("\n\nSecrets leaked in commit: %{sha}", { sha: new_commit }) }
+  let(:found_message_details) do
+    message = "\n  -- %{path}:%{line_number} | %{description}"
 
     format(
       message,
       {
-        found_secret_blob_id: new_blob_reference,
-        found_secret_line_number: found_secret_line_number,
-        found_secret_type: found_secret_type,
-        found_secret_description: found_secret_description
+        path: found_secret_path,
+        line_number: found_secret_line_number,
+        description: found_secret_description
       }
     )
   end
@@ -149,6 +159,9 @@
     # client and `object_existence_map` in such way only some of them are considered new.
     allow(repository).to receive(:gitaly_commit_client).and_return(gitaly_commit_client)
     allow(gitaly_commit_client).to receive(:object_existence_map).and_return(object_existence_map)
+
+    # We also want to have the client return the tree entries.
+    allow(gitaly_commit_client).to receive(:tree_entries).and_return([tree_entries, nil])
   end
 end
 
diff --git a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
index 7b371fd8fd6d03..ae04b29c700acc 100644
--- a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
+++ b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
@@ -203,7 +203,28 @@
     expect { subject.validate! }.to raise_error do |error|
       expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
       expect(error.message).to include(found_secrets)
-      expect(error.message).to include(found_secrets_message)
+      expect(error.message).to include(found_message_commit)
+      expect(error.message).to include(found_message_details)
+      expect(error.message).to include(found_secrets_post_message)
+    end
+  end
+
+  it 'loads tree entries of the new commit' do
+    expect(::Gitlab::Git::Tree).to receive(:tree_entries)
+      .once
+      .with(repository, new_commit, nil, false, true, nil)
+      .and_return([tree_entries, nil])
+      .and_call_original
+
+    expect(secret_detection_logger).to receive(:info)
+      .once
+      .with(message: found_secrets)
+
+    expect { subject.validate! }.to raise_error do |error|
+      expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
+      expect(error.message).to include(found_secrets)
+      expect(error.message).to include(found_message_commit)
+      expect(error.message).to include(found_message_details)
       expect(error.message).to include(found_secrets_post_message)
     end
   end
@@ -382,7 +403,55 @@
     expect { subject.validate! }.to raise_error do |error|
       expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
       expect(error.message).to include(found_secrets_with_errors)
-      expect(error.message).to include(found_secrets_message)
+      expect(error.message).to include(found_message_commit)
+      expect(error.message).to include(found_message_details)
+      expect(error.message).to include(blob_timed_out_error)
+      expect(error.message).to include(failed_to_scan_regex_error)
+      expect(error.message).to include(found_secrets_post_message)
+    end
+  end
+
+  it 'loads tree entries of the new commit' do
+    expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
+      expect(instance).to receive(:secrets_scan)
+        .with(
+          array_including(new_blob, timed_out_blob, failed_to_scan_blob),
+          timeout: kind_of(Float)
+        )
+        .once
+        .and_return(successful_scan_with_errors_response)
+    end
+
+    expect(::Gitlab::Git::Tree).to receive(:tree_entries)
+      .with(repository, new_commit, nil, false, true, nil)
+      .once
+      .ordered
+      .and_return([tree_entries, nil])
+      .and_call_original
+
+    expect(::Gitlab::Git::Tree).to receive(:tree_entries)
+      .with(repository, timed_out_commit, nil, false, true, nil)
+      .once
+      .ordered
+      .and_return([[], nil])
+      .and_call_original
+
+    expect(::Gitlab::Git::Tree).to receive(:tree_entries)
+      .with(repository, failed_to_scan_commit, nil, false, true, nil)
+      .once
+      .ordered
+      .and_return([[], nil])
+      .and_call_original
+
+    expect(secret_detection_logger).to receive(:info)
+      .once
+      .with(message: found_secrets_with_errors)
+
+    expect { subject.validate! }.to raise_error do |error|
+      expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
+      expect(error.message).to include(found_secrets_with_errors)
+      expect(error.message).to include(found_message_commit)
+      expect(error.message).to include(found_message_details)
       expect(error.message).to include(blob_timed_out_error)
       expect(error.message).to include(failed_to_scan_regex_error)
       expect(error.message).to include(found_secrets_post_message)
diff --git a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
index e3633fadb2a130..1ef321f0a9bd56 100644
--- a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
+++ b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
@@ -7,7 +7,7 @@ class Finding
       attr_reader :blob_id, :status, :line_number, :type, :description
       attr_accessor :sha, :path
 
-      def initialize(blob_id, status, line_number = nil, type = nil, description = nil, sha = nil, path = nil) # rubocop:disable Metrics/ParameterLists
+      def initialize(blob_id, status, line_number = nil, type = nil, description = nil, sha = nil, path = nil) # rubocop:disable Metrics/ParameterLists -- all params are needed
         @blob_id = blob_id
         @status = status
         @line_number = line_number
-- 
GitLab


From 31e06707472950bd3300e449059dffab1a7228f6 Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Wed, 13 Dec 2023 14:56:17 +0100
Subject: [PATCH 3/9] Address feedback from reviewers

---
 .../gitlab/checks/push_rules/secrets_check.rb | 99 +++++++++++--------
 .../secrets_check_shared_contexts.rb          | 24 ++++-
 .../gitlab/secrets_check_shared_examples.rb   | 62 +++++++++---
 .../lib/gitlab/secret_detection/finding.rb    | 10 +-
 4 files changed, 132 insertions(+), 63 deletions(-)

diff --git a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
index 8336af2e1d912b..387492312aeb5b 100644
--- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
+++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
@@ -11,7 +11,8 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker
             scan_timeout_error: 'Secret detection scan timed out.',
             scan_initialization_error: 'Secret detection scan failed to initialize.',
             invalid_input_error: 'Secret detection scan failed due to invalid input.',
-            invalid_scan_status_code_error: 'Invalid secret detection scan status, check passed.'
+            invalid_scan_status_code_error: 'Invalid secret detection scan status, check passed.',
+            too_many_tree_entries_error: 'Too many tree entries exist for commit(sha: %{sha}).'
           }.freeze
 
           LOG_MESSAGES = {
@@ -21,8 +22,10 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker
             found_secrets_post_message: "\n\nPlease remove the identified secrets in your commits and try again.",
             found_secrets_with_errors: 'Secret detection scan completed with one or more findings ' \
                                        'but some errors occured during the scan.',
-            finding_message_commit: "\n\nSecrets leaked in commit: %{sha}",
-            finding_message_details: "\n  -- %{path}:%{line_number} | %{description}"
+            finding_message_occurrence: "\n\nSecret leaked in commit: %{sha}" \
+                                        "\n  -- %{path}:%{line_number} | %{description}",
+            finding_message: "\n\nSecret leaked in blob: %{blob_id}" \
+                             "\n  -- %{line_number} | %{description}"
           }.freeze
 
           BLOB_BYTES_LIMIT = 1.megabyte # Limit is 1MiB to start with.
@@ -64,6 +67,7 @@ def validate!
                   filter_existing(all_blobs)
                 else
                   # We use `--not --all --not revisions` to ensure we only get new blobs.
+                  # TODO: explore using `with_paths: true` as an argument to load blob paths.
                   project.repository.list_blobs(
                     ['--not', '--all', '--not'] + revisions,
                     bytes_limit: BLOB_BYTES_LIMIT + 1,
@@ -148,9 +152,6 @@ def format_response(response)
             when ::Gitlab::SecretDetection::Status::SCAN_TIMEOUT
               # Entire scan timed out, we log and skip the check for now.
               secret_detection_logger.error(message: ERROR_MESSAGES[:scan_timeout_error])
-
-              # TODO: Decide if we should also fail the check here.
-              # raise ::Gitlab::GitAccess::ForbiddenError, ERROR_MESSAGES[:scan_timeout_error]
             when ::Gitlab::SecretDetection::Status::INPUT_ERROR
               # Scan failed to invalid input. We skip the check because an input error
               # could be due to not having `blobs` being empty (i.e. no new blobs to scan).
@@ -173,13 +174,7 @@ def revisions
           def generate_found_message(response)
             message = LOG_MESSAGES[:found_secrets]
 
-            response.results.group_by(&:sha).each do |commit_sha, results|
-              message += format(LOG_MESSAGES[:finding_message_commit], { sha: commit_sha })
-
-              results.each do |result|
-                message += format(LOG_MESSAGES[:finding_message_details], result.to_h)
-              end
-            end
+            response.results.each { |finding| message += generate_finding(finding) }
 
             message += LOG_MESSAGES[:found_secrets_post_message]
             message
@@ -188,17 +183,14 @@ def generate_found_message(response)
           def generate_found_with_errors_message(response)
             message = LOG_MESSAGES[:found_secrets_with_errors]
 
-            response.results.group_by(&:sha).each do |commit_sha, results|
-              results.each do |result|
-                case result.status
-                when ::Gitlab::SecretDetection::Status::FOUND
-                  message += format(LOG_MESSAGES[:finding_message_commit], { sha: commit_sha })
-                  message += format(LOG_MESSAGES[:finding_message_details], result.to_h)
-                when ::Gitlab::SecretDetection::Status::SCAN_ERROR
-                  message += format(ERROR_MESSAGES[:failed_to_scan_regex_error], result.to_h)
-                when ::Gitlab::SecretDetection::Status::BLOB_TIMEOUT
-                  message += format(ERROR_MESSAGES[:blob_timed_out_error], result.to_h)
-                end
+            response.results.each do |finding|
+              case finding.status
+              when ::Gitlab::SecretDetection::Status::FOUND
+                message += generate_finding(finding)
+              when ::Gitlab::SecretDetection::Status::SCAN_ERROR
+                message += format(ERROR_MESSAGES[:failed_to_scan_regex_error], finding.to_h)
+              when ::Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+                message += format(ERROR_MESSAGES[:blob_timed_out_error], finding.to_h)
               end
             end
 
@@ -206,32 +198,61 @@ def generate_found_with_errors_message(response)
             message
           end
 
+          def generate_finding(finding)
+            message = ""
+
+            if finding.occurrences.present?
+              # If we have found the tree entries for those findings, let's display them.
+              finding.occurrences.each do |occurrence|
+                message += format(
+                  LOG_MESSAGES[:finding_message_occurrence],
+                  {
+                    sha: occurrence[:sha],
+                    path: occurrence[:path],
+                    line_number: finding.line_number,
+                    description: finding.description
+                  }
+                )
+              end
+            else
+              # If no occurrences are found, we display a more generic message (using blob id)
+              message += format(LOG_MESSAGES[:finding_message], finding.to_h)
+            end
+
+            message
+          end
+
           def get_file_path_and_commit_sha(response)
             # Scanning had found secrets, let's try to look up their file path and commit id. This can be done
             # by using `GetTreeEntries()` RPC, and cross examining blobs with ones where secrets where found.
-            tree_entries = revisions.flat_map do |revision|
-              # We don't care for the cursor at the moment, so we only get the first
-              # element (which is `entries`). Then we also just look at entries of type blob.
-              entries = ::Gitlab::Git::Tree.tree_entries(project.repository, revision, nil, false, true, nil)
-                .first
-                .reject { |entry| entry.type != :blob }
+            tree_entries = revisions.each_with_object({}) do |revision, hash|
+              # We could try to handle pagination, but it is likely to timeout way earlier given the
+              # huge default limit (100000) of entries, so we log an error if we get too many results.
+              entries, cursor = ::Gitlab::Git::Tree.tree_entries(project.repository, revision, nil, true, true, nil)
+
+              # We don't raise because we could still gracefully display information about the secrets detected.
+              unless cursor.next_cursor.empty?
+                secret_detection_logger.error(
+                  message: format(ERROR_MESSAGES[:too_many_tree_entries_error], { sha: sha })
+                )
+              end
 
               # Let's grab the `commit_id` and the `path` for that entry, we use the blob id as key.
-              entries.each_with_object({}) do |entry, hash|
-                hash[entry.id] = {
-                  sha: entry.commit_id,
-                  path: entry.path
-                }
+              entries.each do |entry|
+                # Skip any entry that isn't a blob.
+                next if entry.type != :blob
+
+                hash[entry.id] = [] unless hash.key?(entry.id)
+                hash[entry.id] << { sha: entry.commit_id, path: entry.path }
               end
-            end.reduce(&:merge)
+            end
 
-            # update response with the commit sha and file path
+            # Update response with the occurrences found.
             response
               .results
               .reject { |finding| finding.status != ::Gitlab::SecretDetection::Status::FOUND }
               .each do |finding|
-              finding.sha = tree_entries[finding.blob_id][:sha]
-              finding.path = tree_entries[finding.blob_id][:path]
+              finding.occurrences = tree_entries[finding.blob_id]
             end
           end
         end
diff --git a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
index 8c0efb998fc982..ead1eca2fee84e 100644
--- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
+++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
@@ -61,6 +61,7 @@
   let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 33) }
 
   # Used for mocking calls to `tree_entries` methods.
+  let(:gitaly_pagination_cursor) { Gitaly::PaginationCursor.new(next_cursor: "") }
   let(:tree_entries) do
     [
       Gitlab::Git::Tree.new(
@@ -126,19 +127,34 @@
   let(:found_secret_line_number) { '1' }
   let(:found_secret_description) { 'GitLab Personal Access Token' }
 
-  let(:found_message_commit) { format("\n\nSecrets leaked in commit: %{sha}", { sha: new_commit }) }
-  let(:found_message_details) do
-    message = "\n  -- %{path}:%{line_number} | %{description}"
+  let(:found_message_occurrence) do
+    message = "\n\nSecret leaked in commit: %{sha}" \
+              "\n  -- %{path}:%{line_number} | %{description}"
 
     format(
       message,
       {
+        sha: new_commit,
         path: found_secret_path,
         line_number: found_secret_line_number,
         description: found_secret_description
       }
     )
   end
+
+  let(:found_message_message) do
+    message = "\n\nSecret leaked in blob: %{blob_id}" \
+              "\n  -- %{line_number} | %{description}"
+
+    format(
+      message,
+      {
+        blob_id: new_blob,
+        line_number: found_secret_line_number,
+        description: found_secret_description
+      }
+    )
+  end
 end
 
 RSpec.shared_context 'quarantine directory exists' do
@@ -161,7 +177,7 @@
     allow(gitaly_commit_client).to receive(:object_existence_map).and_return(object_existence_map)
 
     # We also want to have the client return the tree entries.
-    allow(gitaly_commit_client).to receive(:tree_entries).and_return([tree_entries, nil])
+    allow(gitaly_commit_client).to receive(:tree_entries).and_return([tree_entries, gitaly_pagination_cursor])
   end
 end
 
diff --git a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
index ae04b29c700acc..3b530c43cdf0aa 100644
--- a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
+++ b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
@@ -203,8 +203,7 @@
     expect { subject.validate! }.to raise_error do |error|
       expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
       expect(error.message).to include(found_secrets)
-      expect(error.message).to include(found_message_commit)
-      expect(error.message).to include(found_message_details)
+      expect(error.message).to include(found_message_occurrence)
       expect(error.message).to include(found_secrets_post_message)
     end
   end
@@ -212,8 +211,8 @@
   it 'loads tree entries of the new commit' do
     expect(::Gitlab::Git::Tree).to receive(:tree_entries)
       .once
-      .with(repository, new_commit, nil, false, true, nil)
-      .and_return([tree_entries, nil])
+      .with(repository, new_commit, nil, true, true, nil)
+      .and_return([tree_entries, gitaly_pagination_cursor])
       .and_call_original
 
     expect(secret_detection_logger).to receive(:info)
@@ -223,11 +222,48 @@
     expect { subject.validate! }.to raise_error do |error|
       expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
       expect(error.message).to include(found_secrets)
-      expect(error.message).to include(found_message_commit)
-      expect(error.message).to include(found_message_details)
+      expect(error.message).to include(found_message_occurrence)
       expect(error.message).to include(found_secrets_post_message)
     end
   end
+
+  context 'when new commit has file in subdirectory' do
+    let_it_be(:new_commit) { create_commit('config/.env' => 'SECRET=glpat-JUST20LETTERSANDNUMB') } # gitleaks:allow
+
+    let(:found_secret_path) { 'config/.env' }
+    let(:tree_entries) do
+      [
+        Gitlab::Git::Tree.new(
+          id: new_blob_reference,
+          type: :blob,
+          mode: '100644',
+          name: '.env',
+          path: 'config/.env',
+          flat_path: 'config/.env',
+          commit_id: new_commit
+        )
+      ]
+    end
+
+    it 'loads tree entries of the new commit in subdirectories' do
+      expect(::Gitlab::Git::Tree).to receive(:tree_entries)
+        .once
+        .with(repository, new_commit, nil, true, true, nil)
+        .and_return([tree_entries, gitaly_pagination_cursor])
+        .and_call_original
+
+      expect(secret_detection_logger).to receive(:info)
+        .once
+        .with(message: found_secrets)
+
+      expect { subject.validate! }.to raise_error do |error|
+        expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
+        expect(error.message).to include(found_secrets)
+        expect(error.message).to include(found_message_occurrence)
+        expect(error.message).to include(found_secrets_post_message)
+      end
+    end
+  end
 end
 
 RSpec.shared_examples 'scan detected secrets but some errors occured' do
@@ -403,8 +439,7 @@
     expect { subject.validate! }.to raise_error do |error|
       expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
       expect(error.message).to include(found_secrets_with_errors)
-      expect(error.message).to include(found_message_commit)
-      expect(error.message).to include(found_message_details)
+      expect(error.message).to include(found_message_occurrence)
       expect(error.message).to include(blob_timed_out_error)
       expect(error.message).to include(failed_to_scan_regex_error)
       expect(error.message).to include(found_secrets_post_message)
@@ -423,21 +458,21 @@
     end
 
     expect(::Gitlab::Git::Tree).to receive(:tree_entries)
-      .with(repository, new_commit, nil, false, true, nil)
+      .with(repository, new_commit, nil, true, true, nil)
       .once
       .ordered
-      .and_return([tree_entries, nil])
+      .and_return([tree_entries, gitaly_pagination_cursor])
       .and_call_original
 
     expect(::Gitlab::Git::Tree).to receive(:tree_entries)
-      .with(repository, timed_out_commit, nil, false, true, nil)
+      .with(repository, timed_out_commit, nil, true, true, nil)
       .once
       .ordered
       .and_return([[], nil])
       .and_call_original
 
     expect(::Gitlab::Git::Tree).to receive(:tree_entries)
-      .with(repository, failed_to_scan_commit, nil, false, true, nil)
+      .with(repository, failed_to_scan_commit, nil, true, true, nil)
       .once
       .ordered
       .and_return([[], nil])
@@ -450,8 +485,7 @@
     expect { subject.validate! }.to raise_error do |error|
       expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
       expect(error.message).to include(found_secrets_with_errors)
-      expect(error.message).to include(found_message_commit)
-      expect(error.message).to include(found_message_details)
+      expect(error.message).to include(found_message_occurrence)
       expect(error.message).to include(blob_timed_out_error)
       expect(error.message).to include(failed_to_scan_regex_error)
       expect(error.message).to include(found_secrets_post_message)
diff --git a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
index 1ef321f0a9bd56..728d45d62283f3 100644
--- a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
+++ b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/finding.rb
@@ -5,16 +5,15 @@ module SecretDetection
     # Finding is a data object representing a secret finding identified within a blob
     class Finding
       attr_reader :blob_id, :status, :line_number, :type, :description
-      attr_accessor :sha, :path
+      attr_accessor :occurrences
 
-      def initialize(blob_id, status, line_number = nil, type = nil, description = nil, sha = nil, path = nil) # rubocop:disable Metrics/ParameterLists -- all params are needed
+      def initialize(blob_id, status, line_number = nil, type = nil, description = nil, occurrences = nil) # rubocop:disable Metrics/ParameterLists -- all params are needed
         @blob_id = blob_id
         @status = status
         @line_number = line_number
         @type = type
         @description = description
-        @sha = sha
-        @path = path
+        @occurrences = occurrences
       end
 
       def ==(other)
@@ -28,8 +27,7 @@ def to_h
           line_number: line_number,
           type: type,
           description: description,
-          sha: sha,
-          path: path
+          occurrences: occurrences
         }
       end
 
-- 
GitLab


From e02a4c897db677de42c1b09efc1fe781facf5457 Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Wed, 13 Dec 2023 21:34:18 +0100
Subject: [PATCH 4/9] Update spec to fix undercoverage job

---
 .../gitlab/checks/push_rules/secrets_check.rb |  2 +-
 .../secrets_check_shared_contexts.rb          |  3 ++-
 .../gitlab/secrets_check_shared_examples.rb   | 21 +++++++++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
index 387492312aeb5b..31f66046be6068 100644
--- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
+++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
@@ -233,7 +233,7 @@ def get_file_path_and_commit_sha(response)
               # We don't raise because we could still gracefully display information about the secrets detected.
               unless cursor.next_cursor.empty?
                 secret_detection_logger.error(
-                  message: format(ERROR_MESSAGES[:too_many_tree_entries_error], { sha: sha })
+                  message: format(ERROR_MESSAGES[:too_many_tree_entries_error], { sha: revision })
                 )
               end
 
diff --git a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
index ead1eca2fee84e..a35431afbe0bdd 100644
--- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
+++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
@@ -111,7 +111,8 @@
       scan_timeout_error: 'Secret detection scan timed out.',
       scan_initialization_error: 'Secret detection scan failed to initialize.',
       invalid_input_error: 'Secret detection scan failed due to invalid input.',
-      invalid_scan_status_code_error: 'Invalid secret detection scan status, check passed.'
+      invalid_scan_status_code_error: 'Invalid secret detection scan status, check passed.',
+      too_many_tree_entries_error: format('Too many tree entries exist for commit(sha: %{sha}).', { sha: new_commit })
     }
   end
 
diff --git a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
index 3b530c43cdf0aa..55304f3b500af0 100644
--- a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
+++ b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
@@ -227,6 +227,27 @@
     end
   end
 
+  context 'when tree has too many entries' do
+    let(:gitaly_pagination_cursor) { Gitaly::PaginationCursor.new(next_cursor: "abcdef") }
+
+    it 'logs an error and continue to raise and present findings' do
+      expect(::Gitlab::Git::Tree).to receive(:tree_entries)
+        .once
+        .with(repository, new_commit, nil, true, true, nil)
+        .and_return([tree_entries, gitaly_pagination_cursor])
+
+      expect(secret_detection_logger).to receive(:info)
+        .once
+        .with(message: found_secrets)
+
+      expect(secret_detection_logger).to receive(:error)
+        .once
+        .with(message: error_messages[:too_many_tree_entries_error])
+
+      expect { subject.validate! }.to raise_error(::Gitlab::GitAccess::ForbiddenError)
+    end
+  end
+
   context 'when new commit has file in subdirectory' do
     let_it_be(:new_commit) { create_commit('config/.env' => 'SECRET=glpat-JUST20LETTERSANDNUMB') } # gitleaks:allow
 
-- 
GitLab


From 21ef7c00e55cbda8d1bf20b7689a73fc4f2b76b7 Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Wed, 13 Dec 2023 23:07:41 +0100
Subject: [PATCH 5/9] Add another spec to fix undercoverage job

---
 .../secrets_check_shared_contexts.rb          |  4 +--
 .../gitlab/secrets_check_shared_examples.rb   | 33 +++++++++++++------
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
index a35431afbe0bdd..643a38c8e3d748 100644
--- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
+++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
@@ -143,14 +143,14 @@
     )
   end
 
-  let(:found_message_message) do
+  let(:found_message) do
     message = "\n\nSecret leaked in blob: %{blob_id}" \
               "\n  -- %{line_number} | %{description}"
 
     format(
       message,
       {
-        blob_id: new_blob,
+        blob_id: new_blob_reference,
         line_number: found_secret_line_number,
         description: found_secret_description
       }
diff --git a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
index 55304f3b500af0..3c6e78c03bcd8b 100644
--- a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
+++ b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb
@@ -82,9 +82,9 @@
 
     expect_next_instance_of(described_class) do |instance|
       expect(instance).to receive(:format_response)
-      .with(passed_scan_response)
-      .once
-      .and_call_original
+        .with(passed_scan_response)
+        .once
+        .and_call_original
     end
 
     expect(secret_detection_logger).to receive(:info)
@@ -189,13 +189,6 @@
         .and_call_original
     end
 
-    expect_next_instance_of(described_class) do |instance|
-      expect(instance).to receive(:format_response)
-      .with(successful_scan_response)
-      .once
-      .and_call_original
-    end
-
     expect(secret_detection_logger).to receive(:info)
       .once
       .with(message: found_secrets)
@@ -227,6 +220,26 @@
     end
   end
 
+  context 'when no tree entries exist or cannot be loaded' do
+    it 'gracefully raises an error with existing information' do
+      expect(::Gitlab::Git::Tree).to receive(:tree_entries)
+        .once
+        .with(repository, new_commit, nil, true, true, nil)
+        .and_return([{}, gitaly_pagination_cursor])
+
+      expect(secret_detection_logger).to receive(:info)
+        .once
+        .with(message: found_secrets)
+
+      expect { subject.validate! }.to raise_error do |error|
+        expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
+        expect(error.message).to include(found_secrets)
+        expect(error.message).to include(found_message)
+        expect(error.message).to include(found_secrets_post_message)
+      end
+    end
+  end
+
   context 'when tree has too many entries' do
     let(:gitaly_pagination_cursor) { Gitaly::PaginationCursor.new(next_cursor: "abcdef") }
 
-- 
GitLab


From 02e3c169ff7603c6a4c466b668e968df174f5845 Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Thu, 14 Dec 2023 10:55:23 +0100
Subject: [PATCH 6/9] Do not build tree entries hash and update scan response
 directly instead

---
 .../gitlab/checks/push_rules/secrets_check.rb | 24 ++++++++++---------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
index 31f66046be6068..ceb14d4e574bc8 100644
--- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
+++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
@@ -223,9 +223,14 @@ def generate_finding(finding)
           end
 
           def get_file_path_and_commit_sha(response)
+            # Let's put aside the findings with secrets.
+            findings_with_secrets = response
+              .results
+              .reject { |finding| finding.status != ::Gitlab::SecretDetection::Status::FOUND }
+
             # Scanning had found secrets, let's try to look up their file path and commit id. This can be done
             # by using `GetTreeEntries()` RPC, and cross examining blobs with ones where secrets where found.
-            tree_entries = revisions.each_with_object({}) do |revision, hash|
+            revisions.each do |revision|
               # We could try to handle pagination, but it is likely to timeout way earlier given the
               # huge default limit (100000) of entries, so we log an error if we get too many results.
               entries, cursor = ::Gitlab::Git::Tree.tree_entries(project.repository, revision, nil, true, true, nil)
@@ -242,17 +247,14 @@ def get_file_path_and_commit_sha(response)
                 # Skip any entry that isn't a blob.
                 next if entry.type != :blob
 
-                hash[entry.id] = [] unless hash.key?(entry.id)
-                hash[entry.id] << { sha: entry.commit_id, path: entry.path }
-              end
-            end
+                # Update response with occurrences found.
+                current_entry_finding = findings_with_secrets.find { |finding| finding.blob_id == entry.id }
 
-            # Update response with the occurrences found.
-            response
-              .results
-              .reject { |finding| finding.status != ::Gitlab::SecretDetection::Status::FOUND }
-              .each do |finding|
-              finding.occurrences = tree_entries[finding.blob_id]
+                if current_entry_finding
+                  current_entry_finding.occurrences ||= []
+                  current_entry_finding.occurrences << { sha: entry.commit_id, path: entry.path }
+                end
+              end
             end
           end
         end
-- 
GitLab


From 9f0ed63ac5957b10edb82be79a724dafc0cf1799 Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Thu, 14 Dec 2023 16:12:53 +0100
Subject: [PATCH 7/9] Address more feedback from reviewers

---
 .../gitlab/checks/push_rules/secrets_check.rb | 56 +++++++++----------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
index ceb14d4e574bc8..63b28740c232ce 100644
--- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
+++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
@@ -127,7 +127,7 @@ def format_response(response)
               ::Gitlab::SecretDetection::Status::FOUND_WITH_ERRORS
             ].include?(response.status)
               # TODO: filter out revisions not related to found secrets
-              get_file_path_and_commit_sha(response)
+              collect_findings_occurrences(response)
             end
 
             case response.status
@@ -136,7 +136,7 @@ def format_response(response)
               secret_detection_logger.info(message: LOG_MESSAGES[:secrets_not_found])
             when ::Gitlab::SecretDetection::Status::FOUND
               # One or more secrets found, generate message with findings and fail check.
-              message = generate_found_message(response)
+              message = build_secrets_found_message(response)
 
               secret_detection_logger.info(message: LOG_MESSAGES[:found_secrets])
 
@@ -144,7 +144,7 @@ def format_response(response)
             when ::Gitlab::SecretDetection::Status::FOUND_WITH_ERRORS
               # One or more secrets found, but with scan errors, so we
               # generate a message with findings and errors, and fail the check.
-              message = generate_found_with_errors_message(response)
+              message = build_secrets_found_with_errors_message(response)
 
               secret_detection_logger.info(message: LOG_MESSAGES[:found_secrets_with_errors])
 
@@ -171,22 +171,22 @@ def revisions
                             .compact
           end
 
-          def generate_found_message(response)
+          def build_secrets_found_message(response)
             message = LOG_MESSAGES[:found_secrets]
 
-            response.results.each { |finding| message += generate_finding(finding) }
+            response.results.each { |finding| message += build_finding_message(finding) }
 
             message += LOG_MESSAGES[:found_secrets_post_message]
             message
           end
 
-          def generate_found_with_errors_message(response)
+          def build_secrets_found_with_errors_message(response)
             message = LOG_MESSAGES[:found_secrets_with_errors]
 
             response.results.each do |finding|
               case finding.status
               when ::Gitlab::SecretDetection::Status::FOUND
-                message += generate_finding(finding)
+                message += build_finding_message(finding)
               when ::Gitlab::SecretDetection::Status::SCAN_ERROR
                 message += format(ERROR_MESSAGES[:failed_to_scan_regex_error], finding.to_h)
               when ::Gitlab::SecretDetection::Status::BLOB_TIMEOUT
@@ -198,31 +198,25 @@ def generate_found_with_errors_message(response)
             message
           end
 
-          def generate_finding(finding)
-            message = ""
-
-            if finding.occurrences.present?
-              # If we have found the tree entries for those findings, let's display them.
-              finding.occurrences.each do |occurrence|
-                message += format(
-                  LOG_MESSAGES[:finding_message_occurrence],
-                  {
-                    sha: occurrence[:sha],
-                    path: occurrence[:path],
-                    line_number: finding.line_number,
-                    description: finding.description
-                  }
-                )
-              end
-            else
-              # If no occurrences are found, we display a more generic message (using blob id)
-              message += format(LOG_MESSAGES[:finding_message], finding.to_h)
+          def build_finding_message(finding)
+            # If no occurrences are found, we display a more generic message (using blob id).
+            return format(LOG_MESSAGES[:finding_message], finding.to_h) unless finding.occurrences.present?
+
+            # If we have found the tree entries for those findings, let's display them.
+            finding.occurrences.reduce('') do |message, occurrence|
+              message + format(
+                LOG_MESSAGES[:finding_message_occurrence],
+                {
+                  sha: occurrence[:sha],
+                  path: occurrence[:path],
+                  line_number: finding.line_number,
+                  description: finding.description
+                }
+              )
             end
-
-            message
           end
 
-          def get_file_path_and_commit_sha(response)
+          def collect_findings_occurrences(response)
             # Let's put aside the findings with secrets.
             findings_with_secrets = response
               .results
@@ -235,7 +229,9 @@ def get_file_path_and_commit_sha(response)
               # huge default limit (100000) of entries, so we log an error if we get too many results.
               entries, cursor = ::Gitlab::Git::Tree.tree_entries(project.repository, revision, nil, true, true, nil)
 
-              # We don't raise because we could still gracefully display information about the secrets detected.
+              # TODO: Handle pagination in the upcoming iterations
+              # We don't raise because we could still provide a hint to the user
+              # about the detected secrets even without a commit sha/file path information.
               unless cursor.next_cursor.empty?
                 secret_detection_logger.error(
                   message: format(ERROR_MESSAGES[:too_many_tree_entries_error], { sha: revision })
-- 
GitLab


From 6e48e3c7f8727142d1a4c58d28c5ffc72b7763ee Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Thu, 14 Dec 2023 21:32:24 +0100
Subject: [PATCH 8/9] Apply some more feedback

---
 ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
index 63b28740c232ce..11249f8b45e7a5 100644
--- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
+++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
@@ -220,7 +220,7 @@ def collect_findings_occurrences(response)
             # Let's put aside the findings with secrets.
             findings_with_secrets = response
               .results
-              .reject { |finding| finding.status != ::Gitlab::SecretDetection::Status::FOUND }
+              .select { |finding| finding.status == ::Gitlab::SecretDetection::Status::FOUND }
 
             # Scanning had found secrets, let's try to look up their file path and commit id. This can be done
             # by using `GetTreeEntries()` RPC, and cross examining blobs with ones where secrets where found.
-- 
GitLab


From 6110194c3888c0afb537ed2ccb9dcfed63866d1c Mon Sep 17 00:00:00 2001
From: Ahmed Hemdan <ahemdan@gitlab.com>
Date: Thu, 14 Dec 2023 23:45:01 +0100
Subject: [PATCH 9/9] Add prefix to line when reporting a finding with only
 blob info

---
 ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb             | 2 +-
 .../support/shared_contexts/secrets_check_shared_contexts.rb    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
index 11249f8b45e7a5..5cc78e970b2aad 100644
--- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
+++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb
@@ -25,7 +25,7 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker
             finding_message_occurrence: "\n\nSecret leaked in commit: %{sha}" \
                                         "\n  -- %{path}:%{line_number} | %{description}",
             finding_message: "\n\nSecret leaked in blob: %{blob_id}" \
-                             "\n  -- %{line_number} | %{description}"
+                             "\n  -- line:%{line_number} | %{description}"
           }.freeze
 
           BLOB_BYTES_LIMIT = 1.megabyte # Limit is 1MiB to start with.
diff --git a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
index 643a38c8e3d748..f93cfc63c8d22e 100644
--- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
+++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
@@ -145,7 +145,7 @@
 
   let(:found_message) do
     message = "\n\nSecret leaked in blob: %{blob_id}" \
-              "\n  -- %{line_number} | %{description}"
+              "\n  -- line:%{line_number} | %{description}"
 
     format(
       message,
-- 
GitLab