From adc922ad7979e2e53827974574aa64dd3b898f10 Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Wed, 22 Oct 2025 11:17:14 +1000 Subject: [PATCH 1/4] Pass timeout to SPP GRPC calls --- .../payload_processor.rb | 21 +++++++++++++++---- .../secret_push_protection/secrets_check.rb | 3 ++- lib/gitlab/git/repository.rb | 8 +++---- lib/gitlab/gitaly_client/commit_service.rb | 8 +++---- lib/gitlab/gitaly_client/diff_service.rb | 9 ++++---- 5 files changed, 32 insertions(+), 17 deletions(-) diff --git a/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb b/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb index d0a7efa7b105e7..8abc8a41bf40dc 100644 --- a/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb +++ b/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb @@ -17,11 +17,18 @@ class PayloadProcessor < ::Gitlab::Checks::SecretPushProtection::Base DIFF_REMOVED_LINE = '-' DIFF_CONTEXT_LINE = ' ' END_OF_DIFF = '\\' + TIME_LEFT_BUFF = 10 # extra time to reduce chance of timeouts LOG_MESSAGES = { invalid_encoding: "Could not convert data to UTF-8 from %{encoding}" }.freeze + def initialize(project:, changes_access:, timed_logger:) + super(project: project, changes_access: changes_access) + + @timed_logger = timed_logger + end + # The `standardize_payloads` method gets payloads containing git diffs # and converts them into a standardized format. Each payload is processed to include # its `id`, `data`, and `offset` (used to calculate the line number that a secret is on). @@ -189,7 +196,8 @@ def diff_blobs(paths_slice) right_blob: path.new_blob_id ) end - project.repository.diff_blobs(blob_pair_ids, patch_bytes_limit: PAYLOAD_BYTES_LIMIT).to_a + project.repository.diff_blobs(blob_pair_ids, patch_bytes_limit: PAYLOAD_BYTES_LIMIT, + timeout: gitaly_request_timeout).to_a end def diff_blobs_with_raw_info(paths_slice) @@ -213,7 +221,8 @@ def diff_blobs_with_raw_info(paths_slice) begin project.repository.diff_blobs_with_raw_info( raw_info_data, - patch_bytes_limit: PAYLOAD_BYTES_LIMIT + patch_bytes_limit: PAYLOAD_BYTES_LIMIT, + timeout: gitaly_request_timeout ).to_a rescue StandardError secret_detection_logger.error( @@ -226,7 +235,7 @@ def diff_blobs_with_raw_info(paths_slice) def get_diffs diffs = [] # Get new commits - commits = project.repository.new_commits(revisions) + commits = project.repository.new_commits(revisions, gitaly_request_timeout) diff_filters = [ :DIFF_STATUS_ADDED, @@ -238,7 +247,7 @@ def get_diffs # Get changed paths paths = project.repository.find_changed_paths(commits, merge_commit_diff_mode: :all_parents, - find_renames: true, diff_filters: diff_filters) + find_renames: true, diff_filters: diff_filters, timeout: gitaly_request_timeout) # Reject diff blob objects from paths that are excluded # -- TODO: pass changed paths with diff blob objects and move this exclusion process into the gem. @@ -276,6 +285,10 @@ def exclusions_manager changes_access: changes_access ) end + + def gitaly_request_timeout + @timed_logger.time_left - TIME_LEFT_BUFF + end end end end diff --git a/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb b/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb index 70b2fb036d50dd..f9ea1a04a83fc9 100644 --- a/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb +++ b/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb @@ -171,7 +171,8 @@ def eligibility_checker def payload_processor @payload_processor = Gitlab::Checks::SecretPushProtection::PayloadProcessor.new( project: project, - changes_access: changes_access + changes_access: changes_access, + timed_logger: logger ) end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 7df6a542fd6be0..13b60963ddedbf 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -397,9 +397,9 @@ def log(options) end end - def new_commits(newrevs) + def new_commits(newrevs, timeout = nil) wrapped_gitaly_errors do - gitaly_commit_client.list_new_commits(Array.wrap(newrevs)) + gitaly_commit_client.list_new_commits(Array.wrap(newrevs), timeout) end end @@ -548,13 +548,13 @@ def diff_stats(left_id, right_id) empty_diff_stats end - def find_changed_paths(treeish_objects, merge_commit_diff_mode: nil, find_renames: false, diff_filters: nil) + def find_changed_paths(treeish_objects, merge_commit_diff_mode: nil, find_renames: false, diff_filters: nil, timeout: nil) processed_objects = treeish_objects.compact return [] if processed_objects.empty? wrapped_gitaly_errors do - gitaly_commit_client.find_changed_paths(processed_objects, merge_commit_diff_mode: merge_commit_diff_mode, find_renames: find_renames, diff_filters: diff_filters) + gitaly_commit_client.find_changed_paths(processed_objects, merge_commit_diff_mode: merge_commit_diff_mode, find_renames: find_renames, diff_filters: diff_filters, timeout: timeout) end rescue CommandError, TypeError, NoRepository [] diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index be44a80e984b74..1b762e9b260353 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -251,12 +251,12 @@ def diff_stats(left_commit_sha, right_commit_sha) # else # defaults to :include_merges behavior # ['foo_bar.rb', 'bar_baz.rb'], # - def find_changed_paths(objects, merge_commit_diff_mode: nil, find_renames: false, diff_filters: nil) + def find_changed_paths(objects, merge_commit_diff_mode: nil, find_renames: false, diff_filters: nil, timeout: nil) request = find_changed_paths_request(objects, merge_commit_diff_mode, find_renames, diff_filters) return [] if request.nil? - response = gitaly_client_call(@repository.storage, :diff_service, :find_changed_paths, request, timeout: GitalyClient.medium_timeout) + response = gitaly_client_call(@repository.storage, :diff_service, :find_changed_paths, request, timeout: timeout || GitalyClient.medium_timeout) response.flat_map do |msg| msg.paths.map do |path| Gitlab::Git::ChangedPath.new( @@ -320,7 +320,7 @@ def list_commits(revisions, params = {}) end # List all commits which are new in the repository. If commits have been pushed into the repo - def list_new_commits(revisions) + def list_new_commits(revisions, timeout = nil) git_env = Gitlab::Git::HookEnv.all(@gitaly_repo.gl_repository) if git_env['GIT_OBJECT_DIRECTORY_RELATIVE'].present? # If we have a quarantine environment, then we can optimize the check @@ -337,7 +337,7 @@ def list_new_commits(revisions) repository: quarantined_repo ) - response = gitaly_client_call(@repository.storage, :commit_service, :list_all_commits, request, timeout: GitalyClient.medium_timeout) + response = gitaly_client_call(@repository.storage, :commit_service, :list_all_commits, request, timeout: timeout || GitalyClient.medium_timeout) quarantined_commits = consume_commits_response(response) quarantined_commit_ids = quarantined_commits.map(&:id) diff --git a/lib/gitlab/gitaly_client/diff_service.rb b/lib/gitlab/gitaly_client/diff_service.rb index ec93bd9364a7bf..7e13c1944c7715 100644 --- a/lib/gitlab/gitaly_client/diff_service.rb +++ b/lib/gitlab/gitaly_client/diff_service.rb @@ -32,7 +32,7 @@ def initialize(repository) # @return [GitalyClient::DiffBlobsStitcher] Streamed diff responses from Gitaly def diff_blobs( blob_pairs, diff_mode: DIFF_MODES[:unspecified], whitespace_changes: WHITESPACE_CHANGES[:unspecified], - patch_bytes_limit: 0 + patch_bytes_limit: 0, timeout: nil ) request = Gitaly::DiffBlobsRequest.new( repository: @gitaly_repo, @@ -43,7 +43,7 @@ def diff_blobs( ) response = gitaly_client_call(@storage, :diff_service, :diff_blobs, request, - timeout: GitalyClient.medium_timeout) + timeout: timeout || GitalyClient.medium_timeout) GitalyClient::DiffBlobsStitcher.new(response) end @@ -60,7 +60,8 @@ def diff_blobs_with_raw_info( raw_info, diff_mode: DIFF_MODES[:unspecified], whitespace_changes: WHITESPACE_CHANGES[:unspecified], - patch_bytes_limit: 0 + patch_bytes_limit: 0, + timeout: nil ) request = Gitaly::DiffBlobsRequest.new( repository: @gitaly_repo, @@ -71,7 +72,7 @@ def diff_blobs_with_raw_info( ) response = gitaly_client_call(@storage, :diff_service, :diff_blobs, request, - timeout: GitalyClient.medium_timeout) + timeout: timeout || GitalyClient.medium_timeout) GitalyClient::DiffBlobsStitcher.new(response) end -- GitLab From 8718304b73772f36cb2324b8ee643c1c4b4dbfbb Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Wed, 22 Oct 2025 15:20:41 +1000 Subject: [PATCH 2/4] Do I need this? Not sure --- app/models/repository.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 0b120ca15b2081..09bfcbdb7149a7 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -179,8 +179,8 @@ def commits_between(from, to, limit: nil) end # Returns a list of commits that are not present in any reference - def new_commits(newrev) - commits = raw.new_commits(newrev) + def new_commits(newrev, timeout = nil) + commits = raw.new_commits(newrev, timeout) ::Commit.decorate(commits, container) end -- GitLab From fc4bee6a4d187d35a48f50584378c1b1d2a966be Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Wed, 22 Oct 2025 16:10:16 +1000 Subject: [PATCH 3/4] WIP --- .../secret_push_protection/payload_processor.rb | 14 ++++++++++++-- .../checks/secret_push_protection/secrets_check.rb | 11 +++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb b/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb index 8abc8a41bf40dc..795c5662518ff9 100644 --- a/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb +++ b/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb @@ -17,7 +17,9 @@ class PayloadProcessor < ::Gitlab::Checks::SecretPushProtection::Base DIFF_REMOVED_LINE = '-' DIFF_CONTEXT_LINE = ' ' END_OF_DIFF = '\\' - TIME_LEFT_BUFF = 10 # extra time to reduce chance of timeouts + + # Note: this shouldn't be required, as GITLAB_RAILS_RACK_TIMEOUT is 60s and INTERNAL_TIMEOUT is 50s + TIME_LEFT_BUFF = 20 # extra time to reduce chance of timeouts LOG_MESSAGES = { invalid_encoding: "Could not convert data to UTF-8 from %{encoding}" @@ -235,6 +237,7 @@ def diff_blobs_with_raw_info(paths_slice) def get_diffs diffs = [] # Get new commits + pp "project.repository.new_commits #{gitaly_request_timeout}" commits = project.repository.new_commits(revisions, gitaly_request_timeout) diff_filters = [ @@ -246,6 +249,7 @@ def get_diffs ] # Get changed paths + pp "project.repository.find_changed_paths #{gitaly_request_timeout}" paths = project.repository.find_changed_paths(commits, merge_commit_diff_mode: :all_parents, find_renames: true, diff_filters: diff_filters, timeout: gitaly_request_timeout) @@ -287,7 +291,13 @@ def exclusions_manager end def gitaly_request_timeout - @timed_logger.time_left - TIME_LEFT_BUFF + timeout = @timed_logger.time_left - TIME_LEFT_BUFF + if timeout <= 0 + raise Gitlab::Git::CommandTimedOut, + "Secret push protection has exceeded its time limit as timeout is #{timeout}" + end + + timeout end end end diff --git a/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb b/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb index f9ea1a04a83fc9..496ec2d07eed29 100644 --- a/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb +++ b/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb @@ -45,12 +45,16 @@ def public_project_without_spp? # Only runs for public projects that don't have SPP enabled or licensed. # See: https://gitlab.com/gitlab-org/gitlab/-/issues/551932 def run_validation_dark_launch! + pp "SHOULD BE HERE!!!" audit_logger.track_spp_scan_executed('dark-launch') logger.log_timed(LOG_MESSAGES[:secrets_check]) do + pp "SHOULD BE HERE!!!1" payloads = payload_processor.standardize_payloads + pp "SHOULD BE HERE!!!2" break unless payloads + pp "SHOULD BE HERE!!!3" extra_headers = { 'x-correlation-id': correlation_id, 'x-request-type': 'dark-launch' @@ -58,8 +62,14 @@ def run_validation_dark_launch! sds_client.send_request_to_sds(payloads, exclusions: exclusions_manager.active_exclusions, extra_headers: extra_headers) + + pp "GOT TO HERE!!!!" + pp logger.time_left + pp "GOT TO HERE!!!!/" end rescue StandardError => e + pp "SHOULD BE HERE ERROR #{e}" + pp e.backtrace ::Gitlab::ErrorTracking.track_exception(e) # Log the error but don't re-raise to prevent blocking pushes secret_detection_logger.error( @@ -69,6 +79,7 @@ def run_validation_dark_launch! error_class: e.class.name ) ) + raise e # re-raise to help with testing end def run_validation! -- GitLab From 56cf0e206fbb009ba04827ff7f8e5c961caadb64 Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Wed, 22 Oct 2025 19:08:51 +1000 Subject: [PATCH 4/4] Add max timeout to ensure we don't give loads of time to a single request (and blow out the default timeout) --- .../gitlab/checks/secret_push_protection/payload_processor.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb b/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb index 795c5662518ff9..e1c17094e71b08 100644 --- a/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb +++ b/ee/lib/gitlab/checks/secret_push_protection/payload_processor.rb @@ -19,7 +19,8 @@ class PayloadProcessor < ::Gitlab::Checks::SecretPushProtection::Base END_OF_DIFF = '\\' # Note: this shouldn't be required, as GITLAB_RAILS_RACK_TIMEOUT is 60s and INTERNAL_TIMEOUT is 50s - TIME_LEFT_BUFF = 20 # extra time to reduce chance of timeouts + TIME_LEFT_BUFF = 10 # extra time to reduce chance of timeouts + MAX_TIMEOUT = 20 LOG_MESSAGES = { invalid_encoding: "Could not convert data to UTF-8 from %{encoding}" @@ -292,6 +293,7 @@ def exclusions_manager def gitaly_request_timeout timeout = @timed_logger.time_left - TIME_LEFT_BUFF + timeout = MAX_TIMEOUT if timeout > MAX_TIMEOUT if timeout <= 0 raise Gitlab::Git::CommandTimedOut, "Secret push protection has exceeded its time limit as timeout is #{timeout}" -- GitLab