From 875b02311dc84f74299737630b058c9e18a11112 Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Tue, 9 Sep 2025 15:16:45 +1000 Subject: [PATCH 1/7] Remove ignored and_return --- .../lib/gitlab/secrets_check_shared_examples.rb | 4 ---- 1 file changed, 4 deletions(-) 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 ce7da2cc8fdd75..b03d1196de72c5 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 @@ -252,7 +252,6 @@ exclusions: kind_of(Hash) ) .once - .and_return(passed_scan_response) .and_call_original end @@ -870,19 +869,16 @@ expect(::Gitlab::Git::Tree).to receive(:tree_entries) .with(**expected_tree_args.merge(sha: new_commit)) .once - .and_return([tree_entries, gitaly_pagination_cursor]) .and_call_original expect(::Gitlab::Git::Tree).to receive(:tree_entries) .with(**expected_tree_args.merge(sha: timed_out_commit)) .once - .and_return([[], nil]) .and_call_original expect(::Gitlab::Git::Tree).to receive(:tree_entries) .with(**expected_tree_args.merge(sha: failed_to_scan_commit)) .once - .and_return([[], nil]) .and_call_original expect { subject.validate! }.to raise_error do |error| -- GitLab From 76d3dd1f72dbda63bef9e6128ebda24f2c555e70 Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Tue, 9 Sep 2025 15:54:49 +1000 Subject: [PATCH 2/7] Catch all unexpected exceptions --- .../secret_push_protection/secrets_check.rb | 12 ++++++++++ .../secrets_check_spec.rb | 24 +++++++++++++++++++ 2 files changed, 36 insertions(+) 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 ee178133e3ae14..725a88d80720a0 100644 --- a/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb +++ b/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb @@ -19,6 +19,18 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker def validate! run_validation_dark_launch! if should_run_dark_launch? run_validation! + rescue ::Gitlab::GitAccess::ForbiddenError + raise + rescue StandardError => e + ::Gitlab::ErrorTracking.track_exception(e) + # Log the error but don't re-raise to prevent blocking pushes + secret_detection_logger.error( + build_structured_payload( + message: "Secret detection validation failed: #{e.message}", + class: self.class.name, + error_class: e.class.name + ) + ) end private diff --git a/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb b/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb index 47851b2e3edf6c..80652f4dcb7e91 100644 --- a/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb +++ b/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb @@ -14,6 +14,30 @@ end describe '#validate!' do + context 'when an unhandled exception is thrown' do + it 'catch the exception and logs the exception' do + exception_scenarios = [ + StandardError.new('Standard error'), + RuntimeError.new('Runtime error'), + ArgumentError.new('Argument error'), + NoMethodError.new('No method error'), + ::GRPC::Unavailable.new('Service unavailable'), + ThreadError.new('Thread error') + ] + + exception_scenarios.each do |exception| + # Mock run_validation! to raise the exception + allow(secrets_check).to receive(:run_validation!).and_raise(exception) + + # Expect error tracking/logging for the exception + expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(exception) + + # The main assertion: validate! should never raise an exception + expect { secrets_check.validate! }.not_to raise_error + end + end + end + context 'when secret_detection_enable_spp_for_public_projects is disabled' do before do stub_feature_flags(secret_detection_enable_spp_for_public_projects: false) -- GitLab From fba1df8d288c245da8e5e81fe557e9801d06a59d Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Fri, 12 Sep 2025 11:40:52 +1000 Subject: [PATCH 3/7] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Ahmed Hemdan --- ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 725a88d80720a0..e17f87289c5d25 100644 --- a/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb +++ b/ee/lib/gitlab/checks/secret_push_protection/secrets_check.rb @@ -26,7 +26,7 @@ def validate! # Log the error but don't re-raise to prevent blocking pushes secret_detection_logger.error( build_structured_payload( - message: "Secret detection validation failed: #{e.message}", + message: "Secret push protection failed: #{e.message}", class: self.class.name, error_class: e.class.name ) -- GitLab From 87aded0af516f4611ecb9b3b74d24d4a226e5d95 Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Fri, 12 Sep 2025 12:13:48 +1000 Subject: [PATCH 4/7] Remove unused variables --- .../secrets_check_shared_contexts.rb | 16 ---------------- 1 file changed, 16 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 19e2a26c88765d..0490573f1336fd 100644 --- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb +++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb @@ -159,22 +159,6 @@ ) end - # 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( - 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) } -- GitLab From ec95f14ed13aaca31b2a5503ef332153a570b975 Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Fri, 12 Sep 2025 12:21:54 +1000 Subject: [PATCH 5/7] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Ahmed Hemdan --- .../gitlab/checks/secret_push_protection/secrets_check_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb b/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb index 80652f4dcb7e91..ac5ea3fc7efe03 100644 --- a/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb +++ b/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb @@ -15,7 +15,7 @@ describe '#validate!' do context 'when an unhandled exception is thrown' do - it 'catch the exception and logs the exception' do + it 'catches and logs the exception' do exception_scenarios = [ StandardError.new('Standard error'), RuntimeError.new('Runtime error'), -- GitLab From 24bf4803cf4bc675f27a9f2f2b9adf2c77240e5d Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Fri, 12 Sep 2025 13:35:11 +1000 Subject: [PATCH 6/7] Remove incorrect log --- .../secret_push_protection/secrets_check_spec.rb | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb b/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb index ac5ea3fc7efe03..c6d039dc7bfbbb 100644 --- a/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb +++ b/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb @@ -26,13 +26,19 @@ ] exception_scenarios.each do |exception| - # Mock run_validation! to raise the exception allow(secrets_check).to receive(:run_validation!).and_raise(exception) - # Expect error tracking/logging for the exception expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(exception) - # The main assertion: validate! should never raise an exception + expected_payload = { + message: "Secret push protection failed: #{exception.message}", + class: 'Gitlab::Checks::SecretPushProtection::SecretsCheck', + error_class: exception.class.name + } + expect(secrets_check).to receive(:build_structured_payload) + .with(expected_payload).and_return(expected_payload) + expect(secret_detection_logger).to receive(:error).with(expected_payload) + expect { secrets_check.validate! }.not_to raise_error end end -- GitLab From 7a44af9d881c97ab36eb95bd0289c0bbb929b421 Mon Sep 17 00:00:00 2001 From: Craig Smith <5344211-craigmsmith@users.noreply.gitlab.com> Date: Fri, 12 Sep 2025 13:39:50 +1000 Subject: [PATCH 7/7] Fix test --- .../checks/secret_push_protection/secrets_check_spec.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb b/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb index c6d039dc7bfbbb..fca91548167fb7 100644 --- a/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb +++ b/ee/spec/lib/gitlab/checks/secret_push_protection/secrets_check_spec.rb @@ -31,12 +31,10 @@ expect(::Gitlab::ErrorTracking).to receive(:track_exception).with(exception) expected_payload = { - message: "Secret push protection failed: #{exception.message}", - class: 'Gitlab::Checks::SecretPushProtection::SecretsCheck', - error_class: exception.class.name + 'message' => "Secret push protection failed: #{exception.message}", + 'class' => 'Gitlab::Checks::SecretPushProtection::SecretsCheck', + 'error_class' => anything } - expect(secrets_check).to receive(:build_structured_payload) - .with(expected_payload).and_return(expected_payload) expect(secret_detection_logger).to receive(:error).with(expected_payload) expect { secrets_check.validate! }.not_to raise_error -- GitLab