From 652034b3d8d7be6de870d43174e65d017e4c4b12 Mon Sep 17 00:00:00 2001 From: Ahmed Hemdan Date: Tue, 5 Dec 2023 19:32:38 +0100 Subject: [PATCH 1/4] Check commits for bypass special commit flag --- ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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 5cc78e970b2aad..9eec70447ee6b0 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb @@ -18,6 +18,8 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker LOG_MESSAGES = { secrets_check: 'Detecting secrets...', secrets_not_found: 'Secret detection scan completed with no findings.', + skip_secret_detection: "If you wish to skip secret detection, please include [skip secret detection] " \ + "in one of the commit messages for your changes, and try to push again.\n\n", found_secrets: 'Secret detection scan completed with one or more findings.', 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 ' \ @@ -29,6 +31,7 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker }.freeze BLOB_BYTES_LIMIT = 1.megabyte # Limit is 1MiB to start with. + SPECIAL_COMMIT_FLAG = /\[skip[ _-]secret[ _-]detection\]/i def validate! # Return early and not perform the check if: @@ -43,6 +46,9 @@ def validate! return unless push_rule && project.licensed_feature_available?(:pre_receive_secret_detection) + # Skip if any commit has the special bypass flag `[skip secret detection]` + return if skip_secret_detection? + logger.log_timed(LOG_MESSAGES[:secrets_check]) do # List all blobs via `ListAllBlobs()` based on the existence of a # quarantine directory. If no directory exists, we use `ListBlobs()` instead. @@ -95,6 +101,10 @@ def validate! private + def skip_secret_detection? + changes_access.commits.any? { |commit| commit.safe_message =~ SPECIAL_COMMIT_FLAG } + end + def secret_detection_logger @secret_detection_logger ||= ::Gitlab::SecretDetectionLogger.build end @@ -176,6 +186,7 @@ def build_secrets_found_message(response) response.results.each { |finding| message += build_finding_message(finding) } + message += LOG_MESSAGES[:skip_secret_detection] message += LOG_MESSAGES[:found_secrets_post_message] message end @@ -194,6 +205,7 @@ def build_secrets_found_with_errors_message(response) end end + message += LOG_MESSAGES[:skip_secret_detection] message += LOG_MESSAGES[:found_secrets_post_message] message end -- GitLab From e98716836d83d3abb825b46fd4274d8275113593 Mon Sep 17 00:00:00 2001 From: Ahmed Hemdan Date: Fri, 8 Dec 2023 12:33:34 +0100 Subject: [PATCH 2/4] Update specs --- .../gitlab/checks/push_rules/secrets_check.rb | 4 ++-- .../checks/push_rules/secrets_check_spec.rb | 2 ++ .../secrets_check_shared_contexts.rb | 9 +++++++-- .../gitlab/secrets_check_shared_examples.rb | 20 +++++++++++++++++++ 4 files changed, 31 insertions(+), 4 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 9eec70447ee6b0..a2697954fa67d5 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb @@ -18,8 +18,8 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker LOG_MESSAGES = { secrets_check: 'Detecting secrets...', secrets_not_found: 'Secret detection scan completed with no findings.', - skip_secret_detection: "If you wish to skip secret detection, please include [skip secret detection] " \ - "in one of the commit messages for your changes, and try to push again.\n\n", + skip_secret_detection: "\n\nIf you wish to skip secret detection, please include [skip secret detection] " \ + "in one of the commit messages for your changes.", found_secrets: 'Secret detection scan completed with one or more findings.', 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 ' \ diff --git a/ee/spec/lib/ee/gitlab/checks/push_rules/secrets_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rules/secrets_check_spec.rb index 4c73c5b13b8c1c..d86065a8be2a70 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rules/secrets_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rules/secrets_check_spec.rb @@ -50,6 +50,7 @@ it_behaves_like 'scan failed to initialize' it_behaves_like 'scan failed with invalid input' it_behaves_like 'scan skipped due to invalid status' + it_behaves_like 'scan skipped when a commit has special bypass flag' end end @@ -76,6 +77,7 @@ it_behaves_like 'scan failed to initialize' it_behaves_like 'scan failed with invalid input' it_behaves_like 'scan skipped due to invalid status' + it_behaves_like 'scan skipped when a commit has special bypass flag' end context 'when feature flag is disabled' do 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 f93cfc63c8d22e..fe72d13c954a9b 100644 --- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb +++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb @@ -117,6 +117,11 @@ end # Log messages + let(:skip_secret_detection) do + "\n\nIf you wish to skip secret detection, please include [skip secret detection] " \ + "in one of the commit messages for your changes." + end + let(:secrets_not_found) { 'Secret detection scan completed with no findings.' } let(:found_secrets) { 'Secret detection scan completed with one or more findings.' } let(:found_secrets_post_message) { "\n\nPlease remove the identified secrets in your commits and try again." } @@ -182,11 +187,11 @@ end end -def create_commit(blobs) +def create_commit(blobs, message = 'Add a file') commit = repository.commit_files( user, branch_name: 'a-new-branch', - message: 'Add a file', + message: message, actions: blobs.map do |path, content| { action: :create, 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 3c6e78c03bcd8b..1abbcae7362b3b 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 @@ -197,6 +197,7 @@ 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(skip_secret_detection) expect(error.message).to include(found_secrets_post_message) end end @@ -216,6 +217,7 @@ 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(skip_secret_detection) expect(error.message).to include(found_secrets_post_message) end end @@ -294,6 +296,7 @@ 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(skip_secret_detection) expect(error.message).to include(found_secrets_post_message) end end @@ -476,6 +479,7 @@ 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(skip_secret_detection) expect(error.message).to include(found_secrets_post_message) end end @@ -522,6 +526,7 @@ 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(skip_secret_detection) expect(error.message).to include(found_secrets_post_message) end end @@ -620,3 +625,18 @@ expect { subject.validate! }.not_to raise_error end end + +RSpec.shared_examples 'scan skipped when a commit has special bypass flag' do + include_context 'secrets check context' + + let_it_be(:new_commit) do + create_commit( + { '.env' => 'SECRET=glpat-JUST20LETTERSANDNUMB' }, # gitleaks:allow + 'dummy commit [skip secret detection]' + ) + end + + it 'skips the scanning process' do + expect { subject.validate! }.not_to raise_error + end +end -- GitLab From 75f385aa00b2c9fe47521443f5ace2d26dde6ce8 Mon Sep 17 00:00:00 2001 From: Ahmed Hemdan Date: Thu, 14 Dec 2023 12:46:57 +0100 Subject: [PATCH 3/4] Address reviewer 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 a2697954fa67d5..6c18f72cca470f 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb @@ -31,7 +31,7 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker }.freeze BLOB_BYTES_LIMIT = 1.megabyte # Limit is 1MiB to start with. - SPECIAL_COMMIT_FLAG = /\[skip[ _-]secret[ _-]detection\]/i + SPECIAL_COMMIT_FLAG = /\[skip[ ]secret[ ]detection\]/i def validate! # Return early and not perform the check if: -- GitLab From a21e0a5f28e451d912d1095a5a57aae74adca743 Mon Sep 17 00:00:00 2001 From: Ahmed Hemdan Date: Thu, 14 Dec 2023 15:18:58 +0100 Subject: [PATCH 4/4] Apply more reviewer feedback --- .../gitlab/checks/push_rules/secrets_check.rb | 2 +- .../lib/gitlab/secrets_check_shared_examples.rb | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 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 6c18f72cca470f..a19616ba5ccaec 100644 --- a/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rules/secrets_check.rb @@ -31,7 +31,7 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker }.freeze BLOB_BYTES_LIMIT = 1.megabyte # Limit is 1MiB to start with. - SPECIAL_COMMIT_FLAG = /\[skip[ ]secret[ ]detection\]/i + SPECIAL_COMMIT_FLAG = /\[skip secret detection\]/i def validate! # Return early and not perform the check if: 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 1abbcae7362b3b..33751b6ff416c9 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 @@ -639,4 +639,21 @@ it 'skips the scanning process' do expect { subject.validate! }.not_to raise_error end + + context 'when other commits having secrets in the same push' do + let_it_be(:second_commit_with_secret) do + create_commit('.test.env' => 'TOKEN=glpat-JUST20LETTERSANDNUMB') # gitleaks:allow + end + + let(:changes) do + [ + { oldrev: initial_commit, newrev: new_commit, ref: 'refs/heads/master' }, + { oldrev: initial_commit, newrev: second_commit_with_secret, ref: 'refs/heads/master' } + ] + end + + it 'skips the scanning process still' do + expect { subject.validate! }.not_to raise_error + end + end end -- GitLab