From d19f07072c68a95958dbb0c8790e3683cf3f21ec Mon Sep 17 00:00:00 2001
From: Vasilii Iakliushin <viakliushin@gitlab.com>
Date: Tue, 11 Mar 2025 12:09:25 +0100
Subject: [PATCH] Scan diffs for changes commits via UI
Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/491282
**Problem**
Currently, diff scan is applied only to git pushes via ssh/https
protocol, because there was no way to distiguishing between RPC actions
that triggered the scan.
For example, we want to skip the check for merge commit creation but
verify commits created via WebIDE.
https://gitlab.com/gitlab-org/gitlab/-/issues/423992 introduces an
option to pass additional context from Gitaly RPC endpoint to the
checker code.
**Solution**
* Provide `enable_secrets_check` to selected RPC endpoints
* Run diff scan for checks triggered by calls to these endpoints
---
.../secret_checks_for_web_requests.yml | 9 ++++
ee/lib/gitlab/checks/secrets_check.rb | 9 +++-
.../lib/gitlab/checks/secrets_check_spec.rb | 51 +++++++++++--------
.../secrets_check_shared_contexts.rb | 12 +++++
lib/gitlab/gitaly_client/conflicts_service.rb | 10 +++-
lib/gitlab/gitaly_client/operation_service.rb | 26 +++++++---
6 files changed, 88 insertions(+), 29 deletions(-)
create mode 100644 config/feature_flags/gitlab_com_derisk/secret_checks_for_web_requests.yml
diff --git a/config/feature_flags/gitlab_com_derisk/secret_checks_for_web_requests.yml b/config/feature_flags/gitlab_com_derisk/secret_checks_for_web_requests.yml
new file mode 100644
index 000000000000..97a7566eb09a
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/secret_checks_for_web_requests.yml
@@ -0,0 +1,9 @@
+---
+name: secret_checks_for_web_requests
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/491282
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178726
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/521677
+milestone: '17.10'
+group: group::secret detection
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb
index 1d36cd42ce1c..0f4d39c0eadb 100644
--- a/ee/lib/gitlab/checks/secrets_check.rb
+++ b/ee/lib/gitlab/checks/secrets_check.rb
@@ -204,7 +204,14 @@ def http_or_ssh_protocol?
end
def use_diff_scan?
- Feature.enabled?(:spp_scan_diffs, project) && http_or_ssh_protocol?
+ Feature.enabled?(:spp_scan_diffs, project) && (http_or_ssh_protocol? || secrets_check_enabled_for_web_requests?)
+ end
+
+ def secrets_check_enabled_for_web_requests?
+ return false if Feature.disabled?(:secret_checks_for_web_requests, project)
+ return false if changes_access.gitaly_context.nil?
+
+ changes_access.gitaly_context['enable_secrets_check'] == true
end
def includes_full_revision_history?
diff --git a/ee/spec/lib/gitlab/checks/secrets_check_spec.rb b/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
index 9fcf99308803..e7a129f150e3 100644
--- a/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
+++ b/ee/spec/lib/gitlab/checks/secrets_check_spec.rb
@@ -111,6 +111,7 @@
it_behaves_like 'diff scan passed'
it_behaves_like 'scan detected secrets in diffs'
it_behaves_like 'detects secrets with special characters in diffs'
+ it_behaves_like 'processes hunk headers'
it 'tracks and recovers errors when getting diff' do
expect(repository).to receive(:diff_blobs).and_raise(::GRPC::InvalidArgument)
@@ -126,32 +127,40 @@
context 'when the protocol is web' do
subject(:secrets_check) { described_class.new(changes_access_web) }
- it_behaves_like 'entire file 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'
- it_behaves_like 'scan discarded secrets because they match exclusions'
- it_behaves_like 'detects secrets with special characters in full files'
- end
-
- context 'when the spp_scan_diffs flag is enabled' do
- it_behaves_like 'diff scan passed'
- it_behaves_like 'scan detected secrets in diffs'
- it_behaves_like 'processes hunk headers'
- it_behaves_like 'detects secrets with special characters in diffs'
-
- context 'when the protocol is web' do
- subject(:secrets_check) { described_class.new(changes_access_web) }
+ context 'when the secret_checks_for_web_requests feature flag is disabled' do
+ before do
+ stub_feature_flags(secret_checks_for_web_requests: false)
+ end
it_behaves_like 'entire file 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'
+ it_behaves_like 'scan discarded secrets because they match exclusions'
it_behaves_like 'detects secrets with special characters in full files'
end
+
+ context 'when the secret_checks_for_web_requests feature flag is enabled' do
+ context 'when changes_access.gitaly_context enable_secrets_check is false' do
+ it_behaves_like 'entire file scan passed'
+ it_behaves_like 'scan detected secrets'
+ it_behaves_like 'detects secrets with special characters in full files'
+ end
+
+ # TODO: Remove this block when `spp_scan_diffs` is removed and cleaned up.
+ context 'when changes_access.gitaly_context enable_secrets_check is true' do
+ subject(:secrets_check) { described_class.new(changes_access_web_secrets_check_enabled) }
+
+ it_behaves_like 'diff scan passed'
+ it_behaves_like 'scan detected secrets in diffs'
+ it_behaves_like 'detects secrets with special characters in diffs'
+ end
+ 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 66486c55f776..21d67e1495a9 100644
--- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
+++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
@@ -119,6 +119,18 @@
)
end
+ let(:changes_access_web_secrets_check_enabled) do
+ Gitlab::Checks::ChangesAccess.new(
+ changes,
+ project: project,
+ user_access: user_access,
+ protocol: 'web',
+ logger: logger,
+ push_options: push_options,
+ gitaly_context: { 'enable_secrets_check' => true }
+ )
+ end
+
let(:delete_changes_access) do
Gitlab::Checks::ChangesAccess.new(
delete_changes,
diff --git a/lib/gitlab/gitaly_client/conflicts_service.rb b/lib/gitlab/gitaly_client/conflicts_service.rb
index 831c5ca1305a..35763960424b 100644
--- a/lib/gitlab/gitaly_client/conflicts_service.rb
+++ b/lib/gitlab/gitaly_client/conflicts_service.rb
@@ -55,7 +55,15 @@ def resolve_conflicts(target_repository, resolution, source_branch, target_branc
end
end
- response = gitaly_client_call(@repository.storage, :conflicts_service, :resolve_conflicts, req_enum, remote_storage: target_repository.storage, timeout: GitalyClient.long_timeout)
+ response = gitaly_client_call(
+ @repository.storage,
+ :conflicts_service,
+ :resolve_conflicts,
+ req_enum,
+ remote_storage: target_repository.storage,
+ timeout: GitalyClient.long_timeout,
+ gitaly_context: { 'enable_secrets_check' => true }
+ )
if response.resolution_error.present?
raise Gitlab::Git::Conflict::Resolver::ResolutionError, response.resolution_error
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index fe352ade7978..c68191e6ec36 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -284,7 +284,8 @@ def user_cherry_pick(
:user_cherry_pick,
request,
remote_storage: start_repository.storage,
- timeout: GitalyClient.long_timeout
+ timeout: GitalyClient.long_timeout,
+ gitaly_context: { 'enable_secrets_check' => true }
)
Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
@@ -368,7 +369,8 @@ def rebase(user, rebase_id, branch:, branch_sha:, remote_repository:, remote_bra
:user_rebase_confirmable,
request_enum.each,
timeout: GitalyClient.long_timeout,
- remote_storage: remote_repository.storage
+ remote_storage: remote_repository.storage,
+ gitaly_context: { 'enable_secrets_check' => true }
)
# First request
@@ -535,8 +537,14 @@ def user_commit_files(
end
response = gitaly_client_call(
- @repository.storage, :operation_service, :user_commit_files, req_enum,
- timeout: GitalyClient.long_timeout, remote_storage: start_repository&.storage)
+ @repository.storage,
+ :operation_service,
+ :user_commit_files,
+ req_enum,
+ timeout: GitalyClient.long_timeout,
+ remote_storage: start_repository&.storage,
+ gitaly_context: { 'enable_secrets_check' => true }
+ )
if (pre_receive_error = response.pre_receive_error.presence)
raise Gitlab::Git::PreReceiveError, pre_receive_error
@@ -588,8 +596,14 @@ def user_commit_patches(user, branch_name:, patches:, target_sha: nil)
end
end
- response = gitaly_client_call(@repository.storage, :operation_service,
- :user_apply_patch, chunks, timeout: GitalyClient.long_timeout)
+ response = gitaly_client_call(
+ @repository.storage,
+ :operation_service,
+ :user_apply_patch,
+ chunks,
+ timeout: GitalyClient.long_timeout,
+ gitaly_context: { 'enable_secrets_check' => true }
+ )
Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
end
--
GitLab