From 6510c81a177ea56e20775b1f356e664c64f12b26 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Fri, 13 Sep 2024 13:12:23 -0500
Subject: [PATCH 1/4] Check gitaly protocol for diff scanning
Only scan diffs if http or ssh protocol. If web, scan entire file
Changelog: changed
---
ee/lib/gitlab/checks/secrets_check.rb | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb
index 74c1b8e2fc6cc4..f012b3cb24503b 100644
--- a/ee/lib/gitlab/checks/secrets_check.rb
+++ b/ee/lib/gitlab/checks/secrets_check.rb
@@ -99,6 +99,14 @@ def validate!
private
+ def http_or_ssh_protocol
+ changes_access.protocol == "http" || changes_access.protocol == "ssh"
+ end
+
+ def use_diff_scan
+ Feature.enabled?(:spp_scan_diffs, project) && http_or_ssh_protocol
+ end
+
def run_pre_receive_secret_detection?
Gitlab::CurrentSettings.current_application_settings.pre_receive_secret_detection_enabled &&
(enabled_for_non_dedicated_project? || enabled_for_dedicated_project?)
--
GitLab
From 4650a7dcb244a355365fa16e27b64c8d1e447c38 Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Wed, 18 Sep 2024 19:00:07 -0500
Subject: [PATCH 2/4] Doc protocol differences
---
.../secret_detection/secret_push_protection/index.md | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/doc/user/application_security/secret_detection/secret_push_protection/index.md b/doc/user/application_security/secret_detection/secret_push_protection/index.md
index 62b1f744802fd8..5e384c6fe668b5 100644
--- a/doc/user/application_security/secret_detection/secret_push_protection/index.md
+++ b/doc/user/application_security/secret_detection/secret_push_protection/index.md
@@ -117,6 +117,10 @@ This can cause a [push to be blocked unexpectedly](#push-blocked-unexpectedly) w
You can enable the `spp_scan_diffs` [feature flag](../../../../administration/feature_flags.md) for your project,
which modifies Secret Push Protection to only scan newly committed changes (or diffs), and not the rest of the file.
+When `spp_scan_diffs` is enabled, Secret Push Protection scans the diffs for CLI-based pushes via HTTP/SSH.
+Changes committed via the WebIDE still result in the entire file being scanned due to a technical limitation.
+[Issue 491282](https://gitlab.com/gitlab-org/gitlab/-/issues/491282) addresses the limitation so only the diffs are scanned for WebIDE changes.
+
## Resolve a blocked push
When secret push protection blocks a push, you can either:
@@ -210,6 +214,7 @@ Secret Push Protection scans all contents of modified files. This can cause a pu
if a modified file contains a secret, even if the secret is not part of the diff.
[Enable the `spp_scan_diffs` feature flag](#diff-scanning) to ensure that only newly committed changes are scanned.
+To push a WebIDE change to a file that contains a secret, you need to [skip secret push protection](#skip-secret-push-protection).
### File was not scanned
--
GitLab
From d7ea47eba167cfcc1fcf10f8e4a217fda390c65b Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Thu, 3 Oct 2024 01:06:12 -0500
Subject: [PATCH 3/4] Add specs, use use_diff_scan
---
ee/lib/gitlab/checks/secrets_check.rb | 2 +-
.../lib/gitlab/checks/secrets_check_spec.rb | 25 +++++++++++++++++++
.../gitlab/secrets_check_shared_examples.rb | 2 ++
3 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb
index f012b3cb24503b..e80467a1014ef8 100644
--- a/ee/lib/gitlab/checks/secrets_check.rb
+++ b/ee/lib/gitlab/checks/secrets_check.rb
@@ -68,7 +68,7 @@ def validate!
end
logger.log_timed(LOG_MESSAGES[:secrets_check]) do
- if Feature.enabled?(:spp_scan_diffs, project)
+ if use_diff_scan
payloads = get_diffs
# Pass payloads to gem for scanning.
response = ::Gitlab::SecretDetection::ScanDiffs
diff --git a/ee/spec/lib/gitlab/checks/secrets_check_spec.rb b/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
index dd0ed5a964dc8c..3de864051dbd8e 100644
--- a/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
+++ b/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
@@ -67,6 +67,31 @@
it_behaves_like 'diff scan skipped when a commit has special bypass flag'
it_behaves_like 'diff scan skipped when secret_push_protection.skip_all push option is passed'
+ context 'when the protocol is web' do
+ let(:changes_access_web) do
+ Gitlab::Checks::ChangesAccess.new(
+ changes,
+ project: project,
+ user_access: user_access,
+ protocol: 'web',
+ logger: logger,
+ push_options: push_options
+ )
+ end
+
+ subject(:secrets_check) { described_class.new(changes_access_web) }
+
+ it_behaves_like 'scan passed'
+ it_behaves_like 'scan detected secrets'
+ it_behaves_like 'scan detected secrets but some errors occured'
+ it_behaves_like 'scan timed out'
+ 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'
+ it_behaves_like 'scan skipped when secret_push_protection.skip_all push option is passed'
+ end
+
context 'when the scan diff feature flag is not enabled' do
before do
stub_feature_flags(spp_scan_diffs: false)
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 c32ef179a65034..396e48f3674067 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
@@ -1136,6 +1136,8 @@
)
end
+ subject(:secrets_check) { described_class.new(changes_access) }
+
let_it_be(:new_commit) do
create_commit(
{ '.env' => 'SECRET=glpat-JUST20LETTERSANDNUMB' } # gitleaks:allow
--
GitLab
From 15333566ea51be12cd42928ab4d7c79df7ca299f Mon Sep 17 00:00:00 2001
From: Serena Fang <sfang@gitlab.com>
Date: Thu, 3 Oct 2024 10:18:58 -0500
Subject: [PATCH 4/4] Apply reviewer suggestions
---
ee/lib/gitlab/checks/secrets_check.rb | 10 +--
.../lib/gitlab/checks/secrets_check_spec.rb | 71 ++++++++++---------
.../secrets_check_shared_contexts.rb | 11 +++
3 files changed, 54 insertions(+), 38 deletions(-)
diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb
index e80467a1014ef8..7c54afbcb96a91 100644
--- a/ee/lib/gitlab/checks/secrets_check.rb
+++ b/ee/lib/gitlab/checks/secrets_check.rb
@@ -68,7 +68,7 @@ def validate!
end
logger.log_timed(LOG_MESSAGES[:secrets_check]) do
- if use_diff_scan
+ if use_diff_scan?
payloads = get_diffs
# Pass payloads to gem for scanning.
response = ::Gitlab::SecretDetection::ScanDiffs
@@ -99,12 +99,12 @@ def validate!
private
- def http_or_ssh_protocol
- changes_access.protocol == "http" || changes_access.protocol == "ssh"
+ def http_or_ssh_protocol?
+ %w[http ssh].include?(changes_access.protocol)
end
- def use_diff_scan
- Feature.enabled?(:spp_scan_diffs, project) && http_or_ssh_protocol
+ def use_diff_scan?
+ Feature.enabled?(:spp_scan_diffs, project) && http_or_ssh_protocol?
end
def run_pre_receive_secret_detection?
diff --git a/ee/spec/lib/gitlab/checks/secrets_check_spec.rb b/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
index 3de864051dbd8e..00b19bf89527a1 100644
--- a/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
+++ b/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
@@ -56,40 +56,31 @@
it_behaves_like 'skips the push check'
end
- it_behaves_like 'diff scan passed'
- it_behaves_like 'diff scan detected secrets'
- it_behaves_like 'diff scan detected secrets but some errors occured'
- it_behaves_like 'diff scan timed out'
- it_behaves_like 'diff scan failed to initialize'
- it_behaves_like 'diff scan failed with invalid input'
- it_behaves_like 'diff scan handles malformed blobs'
- it_behaves_like 'diff scan skipped due to invalid status'
- it_behaves_like 'diff scan skipped when a commit has special bypass flag'
- it_behaves_like 'diff scan skipped when secret_push_protection.skip_all push option is passed'
-
- context 'when the protocol is web' do
- let(:changes_access_web) do
- Gitlab::Checks::ChangesAccess.new(
- changes,
- project: project,
- user_access: user_access,
- protocol: 'web',
- logger: logger,
- push_options: push_options
- )
+ context 'when the spp_scan_diffs flag is enabled' do
+ it_behaves_like 'diff scan passed'
+ it_behaves_like 'diff scan detected secrets'
+ it_behaves_like 'diff scan detected secrets but some errors occured'
+ it_behaves_like 'diff scan timed out'
+ it_behaves_like 'diff scan failed to initialize'
+ it_behaves_like 'diff scan failed with invalid input'
+ it_behaves_like 'diff scan handles malformed blobs'
+ it_behaves_like 'diff scan skipped due to invalid status'
+ it_behaves_like 'diff scan skipped when a commit has special bypass flag'
+ it_behaves_like 'diff scan skipped when secret_push_protection.skip_all push option is passed'
+
+ context 'when the protocol is web' do
+ subject(:secrets_check) { described_class.new(changes_access_web) }
+
+ it_behaves_like 'scan passed'
+ it_behaves_like 'scan detected secrets'
+ it_behaves_like 'scan detected secrets but some errors occured'
+ it_behaves_like 'scan timed out'
+ 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'
+ it_behaves_like 'scan skipped when secret_push_protection.skip_all push option is passed'
end
-
- subject(:secrets_check) { described_class.new(changes_access_web) }
-
- it_behaves_like 'scan passed'
- it_behaves_like 'scan detected secrets'
- it_behaves_like 'scan detected secrets but some errors occured'
- it_behaves_like 'scan timed out'
- 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'
- it_behaves_like 'scan skipped when secret_push_protection.skip_all push option is passed'
end
context 'when the scan diff feature flag is not enabled' do
@@ -106,6 +97,20 @@
it_behaves_like 'scan skipped due to invalid status'
it_behaves_like 'scan skipped when a commit has special bypass flag'
it_behaves_like 'scan skipped when secret_push_protection.skip_all push option is passed'
+
+ context 'when the protocol is web' do
+ subject(:secrets_check) { described_class.new(changes_access_web) }
+
+ it_behaves_like 'scan passed'
+ it_behaves_like 'scan detected secrets'
+ it_behaves_like 'scan detected secrets but some errors occured'
+ it_behaves_like 'scan timed out'
+ 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'
+ it_behaves_like 'scan skipped when secret_push_protection.skip_all push option is passed'
+ end
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 ed9ba59030851e..e6ed8c88120f24 100644
--- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
+++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
@@ -88,6 +88,17 @@
)
end
+ let(:changes_access_web) do
+ Gitlab::Checks::ChangesAccess.new(
+ changes,
+ project: project,
+ user_access: user_access,
+ protocol: 'web',
+ logger: logger,
+ push_options: push_options
+ )
+ end
+
let(:delete_changes_access) do
Gitlab::Checks::ChangesAccess.new(
delete_changes,
--
GitLab