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 dd0d470032648fb1e2735057b9e05b407b0dde94..59e621a56998fa651962778cb81a5580f72b0712 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
@@ -93,13 +93,13 @@ To enable secret push protection in a project:
## Coverage
-Secret push protection checks the content of each commit when it is pushed to GitLab.
+Secret push protection checks the content of each commit diff when it is pushed to GitLab.
However, the following exclusions apply.
Secret push protection does not check a file in a commit when:
- The file is a binary file.
-- The file is larger than 1 MiB.
+- The diff patch for the file is larger than 1 MiB.
- The file was renamed, deleted, or moved without changes to the content.
- The content of the file is identical to the content of another file in the source code.
- The file is contained in the initial push that created the repository.
@@ -186,20 +186,3 @@ To skip secret push protection when using any Git client:
For example, you are using the GitLab Web IDE and have several commits that are blocked from being
pushed because one of them contains a secret. To skip secret push protection, edit the latest
commit message and add `[skip secret push protection]`, then push the commits.
-
-## Troubleshooting
-
-When working with secret push protection, you may encounter the following situations.
-
-### Push blocked unexpectedly
-
-Secret Push Protection scans all contents of modified files. This can cause a push to be
-unexpectedly blocked if a modified file contains a secret, even if the secret is not part of the diff.
-
-To push a change to a file that contains a secret, you need to [skip secret push protection](#skip-secret-push-protection).
-
-[Issue 469161](https://gitlab.com/gitlab-org/gitlab/-/issues/469161) proposes to change the scanning logic to scan only diffs.
-
-### File was not scanned
-
-Some files are excluded from scanning. For a list of exclusions, see [coverage](#coverage).
diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb
index 66b52e94a011c69fdfc239e2a1c29f07294e467b..581ccfe9a8c8d60f4131d73847fd002f5a2929d5 100644
--- a/ee/lib/gitlab/checks/secrets_check.rb
+++ b/ee/lib/gitlab/checks/secrets_check.rb
@@ -18,7 +18,7 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker
LOG_MESSAGES = {
secrets_check: 'Detecting secrets...',
secrets_not_found: 'Secret detection scan completed with no findings.',
- skip_secret_detection: "\n\nTo skip secret push protection, add the following Git push option" \
+ skip_secret_detection: "\n\nTo skip secret push protection, add the following Git push option " \
"to your push command: `-o secret_push_protection.skip_all`",
found_secrets: "\nPUSH BLOCKED: Secrets detected in code changes",
found_secrets_post_message: "\n\nTo push your changes you must remove the identified secrets.",
@@ -34,7 +34,7 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker
found_secrets_footer: "\n--------------------------------------------------\n\n"
}.freeze
- BLOB_BYTES_LIMIT = 1.megabyte # Limit is 1MiB to start with.
+ DIFF_PATCH_BYTES_LIMIT = 1.megabyte # https://handbook.gitlab.com/handbook/engineering/architecture/design-documents/secret_detection/#target-types
SPECIAL_COMMIT_FLAG = /\[skip secret push protection\]/i
DOCUMENTATION_PATH = 'user/application_security/secret_detection/secret_push_protection/index.html'
DOCUMENTATION_PATH_ANCHOR = 'resolve-a-blocked-push'
@@ -68,17 +68,12 @@ def validate!
end
logger.log_timed(LOG_MESSAGES[:secrets_check]) do
- blobs = ::Gitlab::Checks::ChangedBlobs.new(
- project, revisions, bytes_limit: BLOB_BYTES_LIMIT + 1
- ).execute(timeout: logger.time_left)
+ diffs = get_diffs
- # Filter out larger than BLOB_BYTES_LIMIT blobs and binary blobs.
- blobs.reject! { |blob| blob.size > BLOB_BYTES_LIMIT || blob.binary }
-
- # Pass blobs to gem for scanning.
+ # Pass diffs to gem for scanning.
response = ::Gitlab::SecretDetection::Scan
.new(logger: secret_detection_logger)
- .secrets_scan(blobs, timeout: logger.time_left)
+ .secrets_scan(diffs, timeout: logger.time_left)
# Handle the response depending on the status returned.
format_response(response)
@@ -149,13 +144,36 @@ def track_spp_skipped(skip_method)
)
end
+ def get_diffs
+ # Get new commits
+ commits = project.repository.new_commits(revisions)
+
+ # Get changed paths
+ paths = project.repository.find_changed_paths(commits, merge_commit_diff_mode: :all_parents)
+ pairs = paths.map { |p| { new_blob_id: p.new_blob_id, old_blob_id: p.old_blob_id } }
+
+ # Get blob pairs
+ blob_pairs = pairs.map do |pair|
+ Gitaly::DiffBlobsRequest::BlobPair.new(left_blob: pair[:old_blob_id], right_blob: pair[:new_blob_id])
+ end
+
+ diffs = project.repository.diff_blobs(
+ blob_pairs,
+ diff_mode: Gitlab::GitalyClient::DiffService::DIFF_MODES[:unspecified],
+ whitespace_changes: Gitlab::GitalyClient::DiffService::WHITESPACE_CHANGES[:unspecified],
+ patch_bytes_limit: DIFF_PATCH_BYTES_LIMIT + 1
+ ).to_a.flatten
+
+ # Filter out diffs above 1.Megabyte and binary diffs.
+ diffs.reject { |diff| diff.over_patch_bytes_limit || diff.binary }
+ end
+
def format_response(response)
- # Try to retrieve file path and commit sha for the blobs found.
+ # Try to retrieve file path and commit sha for the diffs found.
if [
::Gitlab::SecretDetection::Status::FOUND,
::Gitlab::SecretDetection::Status::FOUND_WITH_ERRORS
].include?(response.status)
- # TODO: filter out revisions not related to found secrets
results = transform_findings(response)
end
@@ -182,8 +200,8 @@ def format_response(response)
# Entire scan timed out, we log and skip the check for now.
secret_detection_logger.error(message: ERROR_MESSAGES[:scan_timeout_error])
when ::Gitlab::SecretDetection::Status::INPUT_ERROR
- # Scan failed to invalid input. We skip the check because an input error
- # could be due to not having `blobs` being empty (i.e. no new blobs to scan).
+ # Scan failed due to invalid input. We skip the check because an input error
+ # could be due to not having `diffs` being empty (i.e. no new diffs to scan).
secret_detection_logger.error(message: ERROR_MESSAGES[:invalid_input_error])
else
# Invalid status returned by the scanning service/gem, we don't
@@ -231,10 +249,8 @@ def build_secrets_found_message(results, with_errors: false)
}
)
- # Commenting these out as the WebIDE is failing on displaying this message due to length.
- # message += LOG_MESSAGES[:skip_secret_detection]
- # message += LOG_MESSAGES[:found_secrets_footer]
- # Also shortened up the found_secrets: message
+ message += LOG_MESSAGES[:skip_secret_detection]
+ message += LOG_MESSAGES[:found_secrets_footer]
message
end
@@ -252,7 +268,7 @@ def build_finding_message(finding, type)
end
when ::Gitlab::SecretDetection::Status::SCAN_ERROR
format(ERROR_MESSAGES[:failed_to_scan_regex_error], finding.to_h)
- when ::Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+ when ::Gitlab::SecretDetection::Status::DIFF_TIMEOUT
format(ERROR_MESSAGES[:blob_timed_out_error], finding.to_h)
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 1dbc00d87ca9f607d0157a582570380b9eaafd2a..585bb0e22f7391e2593d534688175f588681828d 100644
--- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
+++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb
@@ -6,7 +6,7 @@
let_it_be(:user) { create(:user) }
# Project is created with an empty repository, so
- # we create an initial commit to have a blob committed.
+ # we create an initial commit to have a commit with some diffs.
let_it_be(:project) { create(:project, :empty_repo) }
let_it_be(:repository) { project.repository }
let_it_be(:initial_commit) do
@@ -21,12 +21,12 @@
)
end
- # Create a default `new_commit` for use cases in which we don't care much about blobs.
+ # Create a default `new_commit` for use cases in which we don't care much about diffs.
let_it_be(:new_commit) { create_commit('.env' => 'BASE_URL=https://foo.bar') }
# Define blob references as follows:
- # 1. old reference is used as the blob id for the initial commit.
- # 2. new reference is used as the blob id for commits created in before_all statements elsewhere.
+ # 1. old reference is used as the left blob id in diff_blob objects.
+ # 2. new reference is used as the right blob id in diff_blob objects.
let(:old_blob_reference) { 'f3ac5ae18d057a11d856951f27b9b5b8043cf1ec' }
let(:new_blob_reference) { 'fe29d93da4843da433e62711ace82db601eb4f8f' }
let(:changes) do
@@ -39,6 +39,19 @@
]
end
+ let_it_be(:commits) do
+ [
+ Gitlab::Git::Commit.find(repository, new_commit)
+ ]
+ end
+
+ let(:expected_tree_args) do
+ {
+ repository: repository, sha: new_commit,
+ recursive: true, rescue_not_found: false
+ }
+ end
+
# repository.blank_ref is used to denote a delete commit
let(:delete_changes) do
[
@@ -79,12 +92,6 @@
)
end
- # We cannot really get the same Gitlab::Git::Blob objects even if we call `list_all_blobs` or `list_blobs`
- # directly in any of the specs (which is also not a very good idea) as the object ids will always
- # be different, so we expect the attributes of the returned object to match.
- let(:old_blob) { have_attributes(class: Gitlab::Git::Blob, id: old_blob_reference, size: 23) }
- let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 33) }
-
# Used for mocking calls to `tree_entries` methods.
let(:gitaly_pagination_cursor) { Gitaly::PaginationCursor.new(next_cursor: "") }
let(:tree_entries) do
@@ -142,12 +149,19 @@
let(:finding_path) { '.env' }
let(:finding_line_number) { 1 }
let(:finding_description) { 'GitLab Personal Access Token' }
- let(:finding_path2) { 'test.txt' }
- let(:finding_line_number2) { 2 }
- let(:finding_description2) { 'GitLab Runner Authentication Token' }
+ let(:another_finding_path) { 'test.txt' }
+ let(:another_finding_line_number) { 2 }
+ let(:another_finding_description) { 'GitLab Runner Authentication Token' }
let(:finding_message_header) { format(log_messages[:finding_message_occurrence_header], { sha: new_commit }) }
+ let(:another_finding_message_header) do
+ format(log_messages[:finding_message_occurrence_header], { sha: another_new_commit })
+ end
+
let(:finding_message_path) { format(log_messages[:finding_message_occurrence_path], { path: finding_path }) }
- let(:finding_message_path2) { format(log_messages[:finding_message_occurrence_path], { path: finding_path2 }) }
+ let(:another_finding_message_path) do
+ format(log_messages[:finding_message_occurrence_path], { path: another_finding_path })
+ end
+
let(:finding_message_occurrence_line) do
format(
log_messages[:finding_message_occurrence_line],
@@ -158,12 +172,12 @@
)
end
- let(:finding_message_occurrence_line2) do
+ let(:another_finding_message_occurrence_line) do
format(
log_messages[:finding_message_occurrence_line],
{
line_number: finding_line_number,
- description: finding_description2
+ description: another_finding_description
}
)
end
@@ -183,7 +197,11 @@
message = finding_message_header
message += finding_message_path
message += finding_message_occurrence_line
- message += format(log_messages[:finding_message_occurrence_header], { sha: commit_with_same_blob })
+ message += finding_message_path
+ message += finding_message_occurrence_line
+ message += another_finding_message_header
+ message += finding_message_path
+ message += finding_message_occurrence_line
message += finding_message_path
message += finding_message_occurrence_line
message
@@ -193,8 +211,18 @@
message = finding_message_header
message += finding_message_path
message += finding_message_occurrence_line
- message += finding_message_path2
- message += finding_message_occurrence_line2
+ message += another_finding_message_path
+ message += another_finding_message_occurrence_line
+ message
+ end
+
+ let(:finding_message_multiple_findings_multiple_commits_occurrence_lines) do
+ message = finding_message_header
+ message += finding_message_path
+ message += finding_message_occurrence_line
+ message += another_finding_message_header
+ message += another_finding_message_path
+ message += another_finding_message_occurrence_line
message
end
@@ -233,26 +261,12 @@
end
end
-RSpec.shared_context 'quarantine directory exists' do
- let(:git_env) { { 'GIT_OBJECT_DIRECTORY_RELATIVE' => 'objects' } }
+RSpec.shared_context 'with gitaly commit client' do
let(:gitaly_commit_client) { instance_double(Gitlab::GitalyClient::CommitService) }
- let(:object_existence_map) do
- {
- old_blob_reference.to_s => true,
- new_blob_reference.to_s => false
- }
- end
-
before do
- allow(Gitlab::Git::HookEnv).to receive(:all).with(repository.gl_repository).and_return(git_env)
-
- # Since all blobs are committed to the repository, we mock the gitaly commit
- # client and `object_existence_map` in such way only some of them are considered new.
+ # We mock the gitaly commit client to have it return tree entries.
allow(repository).to receive(:gitaly_commit_client).and_return(gitaly_commit_client)
- allow(gitaly_commit_client).to receive(:object_existence_map).and_return(object_existence_map)
-
- # We also want to have the client return the tree entries.
allow(gitaly_commit_client).to receive(:tree_entries).and_return([tree_entries, gitaly_pagination_cursor])
end
end
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 d52b634d9e4f34e5ca86016dd0e4a53a04caaed6..b768147f23caad09ee1bead12b7b8af41627c1c8 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
@@ -18,39 +18,56 @@
include_context 'secrets check context'
let(:passed_scan_response) { ::Gitlab::SecretDetection::Response.new(Gitlab::SecretDetection::Status::NOT_FOUND) }
- let(:new_blob_reference) { 'da66bef46dbf0ad7fdcbeec97c9eaa24c2846dda' }
- let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 24) }
-
- context 'with quarantine directory' do
- include_context 'quarantine directory exists'
+ let(:new_blob_reference) do
+ 'da66bef46dbf0ad7fdcbeec97c9eaa24c2846dda'
+ end
- it 'lists all blobs of a repository' do
- expect(repository).to receive(:list_all_blobs)
- .with(
- bytes_limit: Gitlab::Checks::SecretsCheck::BLOB_BYTES_LIMIT + 1,
- dynamic_timeout: kind_of(Float),
- ignore_alternate_object_directories: true
- )
- .once
- .and_return([old_blob, new_blob])
- .and_call_original
+ let(:diff_blob_no_secrets) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+BASE_URL=https://foo.bar\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ end
- expect(secret_detection_logger).to receive(:info)
- .once
- .with(message: log_messages[:secrets_not_found])
+ let(:diff_blob_no_secrets_response) do
+ [
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+BASE_URL=https://foo.bar\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ ]
+ end
- expect { subject.validate! }.not_to raise_error
+ context 'when there is no secret in the commit' do
+ before do
+ allow(repository).to receive(:new_commits).and_return(commits)
end
- it 'filters existing blobs out' do
- expect_next_instance_of(::Gitlab::Checks::ChangedBlobs) do |instance|
- # old blob is expected to be filtered out
- expect(instance).to receive(:filter_existing!)
+ it 'returns passed scan response' do
+ expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
+ expect(instance).to receive(:secrets_scan)
.with(
- array_including(old_blob, new_blob)
+ [diff_blob_no_secrets],
+ timeout: kind_of(Float)
)
.once
- .and_return(new_blob)
+ .and_return(passed_scan_response)
+ .and_call_original
+ end
+
+ expect_next_instance_of(described_class) do |instance|
+ expect(instance).to receive(:get_diffs)
+ .once
+ .and_return(diff_blob_no_secrets_response)
.and_call_original
end
@@ -62,18 +79,46 @@
end
end
- context 'with no quarantine directory' do
- it 'list new blobs' do
- expect(repository).to receive(:list_blobs)
- .with(
- ['--not', '--all', '--not'] + changes.pluck(:newrev),
- bytes_limit: Gitlab::Checks::SecretsCheck::BLOB_BYTES_LIMIT + 1,
- with_paths: false,
- dynamic_timeout: kind_of(Float)
+ context 'when there is an existing secret in the file but not in the commit diffs' do
+ let_it_be(:commit_with_secret) { create_commit('.env' => 'SECRET=glpat-JUST20LETTERSANDNUMB') } # gitleaks:allow
+ let_it_be(:blob_reference_with_secret) { 'fe29d93da4843da433e62711ace82db601eb4f8f' }
+
+ 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
+
+ before do
+ allow(repository).to receive(:new_commits).and_return(commits)
+ end
+
+ it 'returns passed scan response' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
.once
- .and_return(new_blob)
+ .and_return(diff_blob_no_secrets_response)
.and_call_original
+ end
+
+ expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
+ expect(instance).to receive(:secrets_scan)
+ .with(
+ [diff_blob_no_secrets],
+ timeout: kind_of(Float)
+ )
+ .once
+ .and_return(passed_scan_response)
+ .and_call_original
+ end
expect(secret_detection_logger).to receive(:info)
.once
@@ -82,37 +127,39 @@
expect { subject.validate! }.not_to raise_error
end
end
-
- it 'scans blobs' do
- expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
- expect(instance).to receive(:secrets_scan)
- .with(
- [new_blob],
- timeout: kind_of(Float)
- )
- .once
- .and_return(passed_scan_response)
- .and_call_original
- end
-
- expect_next_instance_of(described_class) do |instance|
- expect(instance).to receive(:format_response)
- .with(passed_scan_response)
- .once
- .and_call_original
- end
-
- expect(secret_detection_logger).to receive(:info)
- .once
- .with(message: log_messages[:secrets_not_found])
-
- expect { subject.validate! }.not_to raise_error
- end
end
RSpec.shared_examples 'scan detected secrets' do
include_context 'secrets check context'
+ let_it_be(:new_commit) { create_commit('.env' => 'SECRET=glpat-JUST20LETTERSANDNUMB') } # gitleaks:allow
+ let_it_be(:new_blob_reference) { 'fe29d93da4843da433e62711ace82db601eb4f8f' }
+
+ let(:diff_blob) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ end
+
+ let(:diff_blob_response) do
+ [
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ ]
+ end
+
let(:successful_scan_response) do
::Gitlab::SecretDetection::Response.new(
Gitlab::SecretDetection::Status::FOUND,
@@ -128,46 +175,23 @@
)
end
- # The new commit must have a secret, so create a commit with one.
- let_it_be(:new_commit) { create_commit('.env' => 'SECRET=glpat-JUST20LETTERSANDNUMB') } # gitleaks:allow
-
- let(:expected_tree_args) do
- {
- repository: repository, sha: new_commit,
- recursive: true, rescue_not_found: false
- }
- end
-
- context 'with quarantine directory' do
- include_context 'quarantine directory exists'
-
- it 'lists all blobs of a repository' do
- expect(repository).to receive(:list_all_blobs)
- .with(
- bytes_limit: Gitlab::Checks::SecretsCheck::BLOB_BYTES_LIMIT + 1,
- dynamic_timeout: kind_of(Float),
- ignore_alternate_object_directories: true
- )
+ context 'when there is a secret in the commit' do
+ it 'scans diffs' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
.once
- .and_return([old_blob, new_blob])
+ .and_return(diff_blob_response)
.and_call_original
+ end
- expect(secret_detection_logger).to receive(:info)
- .once
- .with(message: log_messages[:found_secrets])
-
- expect { subject.validate! }.to raise_error(::Gitlab::GitAccess::ForbiddenError)
- end
-
- it 'filters existing blobs out' do
- expect_next_instance_of(::Gitlab::Checks::ChangedBlobs) do |instance|
- # old blob is expected to be filtered out
- expect(instance).to receive(:filter_existing!)
+ expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
+ expect(instance).to receive(:secrets_scan)
.with(
- array_including(old_blob, new_blob)
+ [diff_blob],
+ timeout: kind_of(Float)
)
.once
- .and_return(new_blob)
+ .and_return(successful_scan_response)
.and_call_original
end
@@ -175,61 +199,28 @@
.once
.with(message: log_messages[:found_secrets])
- expect { subject.validate! }.to raise_error(::Gitlab::GitAccess::ForbiddenError)
- end
- end
-
- context 'with no quarantine directory' do
- it 'list new blobs' do
- expect(repository).to receive(:list_blobs)
- .with(
- ['--not', '--all', '--not'] + changes.pluck(:newrev),
- bytes_limit: Gitlab::Checks::SecretsCheck::BLOB_BYTES_LIMIT + 1,
- with_paths: false,
- dynamic_timeout: kind_of(Float)
+ expect { subject.validate! }.to raise_error do |error|
+ expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
+ expect(error.message).to include(
+ log_messages[:found_secrets],
+ finding_message_header,
+ finding_message_path,
+ finding_message_occurrence_line,
+ log_messages[:found_secrets_post_message],
+ found_secrets_docs_link
)
- .once
- .and_return(new_blob)
- .and_call_original
-
- expect(secret_detection_logger).to receive(:info)
- .once
- .with(message: log_messages[:found_secrets])
-
- expect { subject.validate! }.to raise_error(::Gitlab::GitAccess::ForbiddenError)
+ end
end
end
- it 'scans blobs' do
- expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
- expect(instance).to receive(:secrets_scan)
- .with(
- [new_blob],
- timeout: kind_of(Float)
- )
- .once
- .and_return(successful_scan_response)
- .and_call_original
- end
-
- expect(secret_detection_logger).to receive(:info)
+ it 'loads tree entries of the new commit' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
.once
- .with(message: log_messages[:found_secrets])
-
- expect { subject.validate! }.to raise_error do |error|
- expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
- expect(error.message).to include(
- log_messages[:found_secrets],
- finding_message_header,
- finding_message_path,
- finding_message_occurrence_line,
- log_messages[:found_secrets_post_message],
- found_secrets_docs_link
- )
+ .and_return(diff_blob_response)
+ .and_call_original
end
- end
- it 'loads tree entries of the new commit' do
expect(::Gitlab::Git::Tree).to receive(:tree_entries)
.once
.with(**expected_tree_args)
@@ -255,6 +246,13 @@
context 'when no tree entries exist or cannot be loaded' do
it 'gracefully raises an error with existing information' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
+ .once
+ .and_return(diff_blob_response)
+ .and_call_original
+ end
+
expect(::Gitlab::Git::Tree).to receive(:tree_entries)
.once
.with(**expected_tree_args)
@@ -280,6 +278,13 @@
let(:gitaly_pagination_cursor) { Gitaly::PaginationCursor.new(next_cursor: "abcdef") }
it 'logs an error and continue to raise and present findings' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
+ .once
+ .and_return(diff_blob_response)
+ .and_call_original
+ end
+
expect(::Gitlab::Git::Tree).to receive(:tree_entries)
.once
.with(**expected_tree_args)
@@ -308,14 +313,32 @@
type: :blob,
mode: '100644',
name: '.env',
- path: 'config/.env',
- flat_path: 'config/.env',
+ path: finding_path,
+ flat_path: finding_path,
commit_id: new_commit
)
]
end
it 'loads tree entries of the new commit in subdirectories' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
+ .once
+ .and_return(diff_blob_response)
+ .and_call_original
+ end
+
+ expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
+ expect(instance).to receive(:secrets_scan)
+ .with(
+ [diff_blob],
+ timeout: kind_of(Float)
+ )
+ .once
+ .and_return(successful_scan_response)
+ .and_call_original
+ end
+
expect(::Gitlab::Git::Tree).to receive(:tree_entries)
.once
.with(**expected_tree_args)
@@ -340,15 +363,42 @@
end
end
- context 'when a blob has multiple secrets' do
+ context 'when there are multiple secrets in a commit' do
+ let_it_be(:secret1) { 'SECRET=glpat-JUST20LETTERSANDNUMB' } # gitleaks:allow
+ let_it_be(:secret2) { 'TOKEN=glpat-JUST20LETTERSANDNUMB' } # gitleaks:allow
+
let_it_be(:new_commit) do
- create_commit('.env' => "SECRET=glpat-JUST20LETTERSANDNUMB\nTOKEN=glpat-JUST20LETTERSANDNUMB") # gitleaks:allow
+ create_commit('.env' => "#{secret1}\n#{secret2}")
end
let(:new_blob_reference) { '59ef300b246861163ee1e2ab4146e16144e4770f' }
- let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 66) }
- let(:successful_with_multiple_findings_scan_response) do
+ let(:diff_blob_multiple_secrets_in_commit) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1,2 @@\n+#{secret1}\n+#{secret2}\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ end
+
+ let(:diff_blob_multiple_secrets_in_commit_response) do
+ [
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1,2 @@\n+#{secret1}\n+#{secret2}\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ ]
+ end
+
+ let(:successful_scan_response) do
::Gitlab::SecretDetection::Response.new(
Gitlab::SecretDetection::Status::FOUND,
[
@@ -370,49 +420,77 @@
)
end
- it 'displays all findings with their corresponding commit sha/filepath' do
- expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
- expect(instance).to receive(:secrets_scan)
- .with(
- [new_blob],
- timeout: kind_of(Float)
- )
+ context 'when there is a secret in the commit' do
+ it 'scans diffs' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
.once
- .and_return(successful_with_multiple_findings_scan_response)
+ .and_return(diff_blob_multiple_secrets_in_commit_response)
.and_call_original
- end
-
- expect(secret_detection_logger).to receive(:info)
- .once
- .with(message: log_messages[:found_secrets])
-
- expect { subject.validate! }.to raise_error do |error|
- expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
- expect(error.message).to include(
- log_messages[:found_secrets],
- finding_message_header,
- finding_message_multiple_occurrence_lines,
- log_messages[:found_secrets_post_message],
- found_secrets_docs_link
- )
+ end
+
+ expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
+ expect(instance).to receive(:secrets_scan)
+ .with(
+ [diff_blob_multiple_secrets_in_commit],
+ timeout: kind_of(Float)
+ )
+ .once
+ .and_return(successful_scan_response)
+ .and_call_original
+ end
+
+ expect(secret_detection_logger).to receive(:info)
+ .once
+ .with(message: log_messages[:found_secrets])
+
+ expect { subject.validate! }.to raise_error do |error|
+ expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
+ expect(error.message).to include(
+ log_messages[:found_secrets],
+ finding_message_header,
+ finding_message_path,
+ finding_message_multiple_occurrence_lines,
+ log_messages[:found_secrets_post_message],
+ found_secrets_docs_link
+ )
+ end
end
end
end
context 'when a blob is present in multiple commits' do
- let_it_be(:new_commit) do
- create_commit('.env' => "SECRET=glpat-JUST20LETTERSANDNUMB") # gitleaks:allow
- end
-
- let_it_be(:commit_with_same_blob) do
+ let_it_be(:another_new_commit) do
create_commit(
{ '.env' => "SECRET=glpat-JUST20LETTERSANDNUMB" }, # gitleaks:allow
'Same commit different message'
)
end
- let(:new_blob_reference) { 'fe29d93da4843da433e62711ace82db601eb4f8f' }
- let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 33) }
+ let(:diff_blob_same_in_multiple_commits) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ end
+
+ let(:diff_blob_same_in_multiple_commits_response) do
+ [
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ ]
+ end
let(:tree_entries) do
[
@@ -432,21 +510,21 @@
name: '.env',
path: '.env',
flat_path: '.env',
- commit_id: commit_with_same_blob
+ commit_id: another_new_commit
)
]
end
let(:changes) do
[
- { oldrev: initial_commit, newrev: commit_with_same_blob, ref: 'refs/heads/master' }
+ { oldrev: initial_commit, newrev: another_new_commit, ref: 'refs/heads/master' }
]
end
let(:commits) do
[
Gitlab::Git::Commit.find(repository, new_commit),
- Gitlab::Git::Commit.find(repository, commit_with_same_blob)
+ Gitlab::Git::Commit.find(repository, another_new_commit)
]
end
@@ -474,20 +552,16 @@
before do
allow(repository).to receive(:new_commits)
- .with([commit_with_same_blob])
+ .with([another_new_commit])
.and_return(commits)
end
it 'displays the findings with their corresponding commit sha/file path' do
- expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
- expect(instance).to receive(:secrets_scan)
- .with(
- [new_blob],
- timeout: kind_of(Float)
- )
- .once
- .and_return(successful_with_same_blob_in_multiple_commits_scan_response)
- .and_call_original
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
+ .once
+ .and_return(diff_blob_same_in_multiple_commits_response)
+ .and_call_original
end
expect(::Gitlab::Git::Tree).to receive(:tree_entries)
@@ -497,11 +571,22 @@
.and_call_original
expect(::Gitlab::Git::Tree).to receive(:tree_entries)
- .with(**expected_tree_args.merge(sha: commit_with_same_blob))
+ .with(**expected_tree_args.merge(sha: another_new_commit))
.once
.and_return([tree_entries, gitaly_pagination_cursor])
.and_call_original
+ expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
+ expect(instance).to receive(:secrets_scan)
+ .with(
+ [diff_blob_same_in_multiple_commits, diff_blob_same_in_multiple_commits],
+ timeout: kind_of(Float)
+ )
+ .once
+ .and_return(successful_with_same_blob_in_multiple_commits_scan_response)
+ .and_call_original
+ end
+
expect(secret_detection_logger).to receive(:info)
.once
.with(message: log_messages[:found_secrets])
@@ -519,15 +604,42 @@
end
context 'when a blob has multiple secrets on the same line' do
+ let_it_be(:secret1) { 'SECRET=glpat-JUST20LETTERSANDNUMB' } # gitleaks:allow
+ let_it_be(:secret2) { 'TOKEN=GR1348941JUST20LETTERSANDNUMB' } # gitleaks:allow
+
let_it_be(:new_commit) do
- create_commit('.env' => "SECRET=glpat-JUST20LETTERSANDNUMB;TOKEN=GR1348941JUST20LETTERSANDNUMB") # gitleaks:allow)
+ create_commit('.env' => "#{secret1};#{secret2}")
end
let(:new_blob_reference) { '13a31e7c93bbe8781f341e24e8ef26ef717d0da2' }
- let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 69) }
let(:second_finding_description) { 'GitLab Runner Registration Token' }
+ let(:diff_blob_secrets_same_line) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+#{secret1};#{secret2}\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ end
+
+ let(:diff_blob_secrets_same_line_response) do
+ [
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+#{secret1};#{secret2}\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ ]
+ end
+
let(:successful_with_multiple_findings_on_same_line_scan_response) do
::Gitlab::SecretDetection::Response.new(
Gitlab::SecretDetection::Status::FOUND,
@@ -551,10 +663,17 @@
end
it 'displays the findings with their corresponding commit sha/filepath' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
+ .once
+ .and_return(diff_blob_secrets_same_line_response)
+ .and_call_original
+ end
+
expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
expect(instance).to receive(:secrets_scan)
.with(
- [new_blob],
+ [diff_blob_secrets_same_line],
timeout: kind_of(Float)
)
.once
@@ -579,15 +698,189 @@
end
end
- context 'when multiple blobs contain secrets in a commit' do
+ context 'when multiple commits contain secrets' do
+ let_it_be(:another_new_commit) { create_commit('test.txt' => 'TOKEN=glrt-12312312312312312312') } # gitleaks:allow
+
+ let(:another_blob_reference) { 'e10edae379797ad5649a65ad364f6c940ee5bbc3' }
+
+ let(:commits) do
+ [
+ Gitlab::Git::Commit.find(repository, new_commit),
+ Gitlab::Git::Commit.find(repository, another_new_commit)
+ ]
+ end
+
+ let(:another_diff_blob) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: another_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+TOKEN=glrt-12312312312312312312\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ end
+
+ let(:diff_blob_multiple_commits_response) do
+ [
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: another_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+TOKEN=glrt-12312312312312312312\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ ]
+ end
+
+ let(:successful_with_multiple_commits_contain_secrets_response) do
+ ::Gitlab::SecretDetection::Response.new(
+ Gitlab::SecretDetection::Status::FOUND,
+ [
+ Gitlab::SecretDetection::Finding.new(
+ new_blob_reference,
+ Gitlab::SecretDetection::Status::FOUND,
+ 1,
+ "gitlab_personal_access_token",
+ "GitLab Personal Access Token"
+ ),
+ Gitlab::SecretDetection::Finding.new(
+ another_blob_reference,
+ Gitlab::SecretDetection::Status::FOUND,
+ 1,
+ "gitlab_runner_auth_token",
+ "GitLab Runner Authentication Token"
+ )
+ ]
+ )
+ end
+
+ 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
+ ),
+ Gitlab::Git::Tree.new(
+ id: another_blob_reference,
+ type: :blob,
+ mode: '100644',
+ name: 'test.txt',
+ path: 'test.txt',
+ flat_path: 'test.txt',
+ commit_id: another_new_commit
+ )
+ ]
+ end
+
+ before do
+ allow(repository).to receive(:new_commits).and_return(commits)
+ end
+
+ it 'successful scans findings in multiple commits' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
+ .once
+ .and_return(diff_blob_multiple_commits_response)
+ .and_call_original
+ end
+
+ 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: another_new_commit))
+ .once
+ .and_return([tree_entries, gitaly_pagination_cursor])
+ .and_call_original
+
+ expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
+ expect(instance).to receive(:secrets_scan)
+ .with(
+ [diff_blob, another_diff_blob],
+ timeout: kind_of(Float)
+ )
+ .once
+ .and_return(successful_with_multiple_commits_contain_secrets_response)
+ .and_call_original
+ end
+
+ expect(secret_detection_logger).to receive(:info)
+ .once
+ .with(message: log_messages[:found_secrets])
+
+ expect { subject.validate! }.to raise_error do |error|
+ expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError)
+ expect(error.message).to include(
+ log_messages[:found_secrets],
+ finding_message_header,
+ finding_message_path,
+ finding_message_multiple_findings_multiple_commits_occurrence_lines,
+ log_messages[:found_secrets_post_message],
+ found_secrets_docs_link
+ )
+ end
+ end
+ end
+
+ context 'when multiple diffs contain secrets in a commit' do
let_it_be(:new_commit) do
create_commit('.env' => "SECRET=glpat-JUST20LETTERSANDNUMB", # gitleaks:allow
'test.txt' => "SECRET=glrt-JUST20LETTERSANDNUMB") # gitleaks:allow
end
- let(:new_blob_reference2) { '5f571267577ed6e0b4b24fb87f7a8218d5912eb9' }
- let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 33) }
- let(:new_blob2) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference2, size: 32) }
+ let(:another_new_blob_reference) { '5f571267577ed6e0b4b24fb87f7a8218d5912eb9' }
+
+ let(:another_diff_blob) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: another_new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glrt-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ end
+
+ let(:diff_blobs_multiple_blobs_with_secrets_response) do
+ [
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ Gitaly::DiffBlobsResponse.new(
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: another_new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glrt-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ ]
+ end
let(:successful_with_multiple_files_findings_scan_response) do
::Gitlab::SecretDetection::Response.new(
@@ -601,9 +894,9 @@
"GitLab Personal Access Token"
),
Gitlab::SecretDetection::Finding.new(
- new_blob_reference2,
+ another_new_blob_reference,
Gitlab::SecretDetection::Status::FOUND,
- 2,
+ 1,
"gitlab_runner_authentication_token",
"GitLab Runner Authentication Token"
)
@@ -611,16 +904,23 @@
)
end
- it 'displays all findings with their corresponding commit sha/filepath' do
+ it 'displays the findings with their corresponding commit sha/filepath' do
+ expect_next_instance_of(described_class) do |secrets_push_check|
+ expect(secrets_push_check).to receive(:get_diffs)
+ .once
+ .and_return(diff_blobs_multiple_blobs_with_secrets_response)
+ .and_call_original
+ end
+
expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
expect(instance).to receive(:secrets_scan)
.with(
- array_including(new_blob, new_blob2),
+ [diff_blob, another_diff_blob],
timeout: kind_of(Float)
)
- .once.and_call_original do |*args|
- expect(instance.secrets_scan(*args)).to eq(successful_with_multiple_files_findings_scan_response)
- end
+ .once
+ .and_return(successful_with_multiple_files_findings_scan_response)
+ .and_call_original
end
expect(secret_detection_logger).to receive(:info)
@@ -648,17 +948,6 @@
let(:category) { described_class.name }
before do
- allow_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
- allow(instance).to receive(:secrets_scan)
- .with(
- [new_blob],
- timeout: kind_of(Float)
- )
- .once
- .and_return(successful_scan_response)
- .and_call_original
- end
-
allow(secret_detection_logger).to receive(:info)
end
@@ -684,7 +973,7 @@
),
Gitlab::SecretDetection::Finding.new(
timed_out_blob_reference,
- Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+ Gitlab::SecretDetection::Status::DIFF_TIMEOUT
),
Gitlab::SecretDetection::Finding.new(
failed_to_scan_blob_reference,
@@ -698,10 +987,6 @@
let_it_be(:timed_out_commit) { create_commit('.test.env' => 'TOKEN=glpat-JUST20LETTERSANDNUMB') } # gitleaks:allow
let_it_be(:failed_to_scan_commit) { create_commit('.dev.env' => 'GLPAT=glpat-JUST20LETTERSANDNUMB') } # gitleaks:allow
- let(:expected_tree_args) do
- { repository: repository, recursive: true, rescue_not_found: false }
- end
-
let(:changes) do
[
{ oldrev: initial_commit, newrev: new_commit, ref: 'refs/heads/master' },
@@ -713,132 +998,53 @@
let(:timed_out_blob_reference) { 'eaf3c09526f50b5e35a096ef70cca033f9974653' }
let(:failed_to_scan_blob_reference) { '4fbec77313fd240d00fc37e522d0274b8fb54bd1' }
- let(:timed_out_blob) { have_attributes(class: Gitlab::Git::Blob, id: timed_out_blob_reference, size: 32) }
- let(:failed_to_scan_blob) { have_attributes(class: Gitlab::Git::Blob, id: failed_to_scan_blob_reference, size: 32) }
-
- # Used for the quarantine directory context below.
- let(:object_existence_map) do
- {
- old_blob_reference.to_s => true,
- new_blob_reference.to_s => false,
- timed_out_blob_reference.to_s => false,
- failed_to_scan_blob_reference.to_s => false
- }
+ let(:diff_blob) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+SECRET=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
end
- context 'with quarantine directory' do
- include_context 'quarantine directory exists'
-
- it 'lists all blobs of a repository' do
- expect(repository).to receive(:list_all_blobs)
- .with(
- bytes_limit: Gitlab::Checks::SecretsCheck::BLOB_BYTES_LIMIT + 1,
- dynamic_timeout: kind_of(Float),
- ignore_alternate_object_directories: true
- )
- .once
- .and_return(
- [old_blob, new_blob, timed_out_blob, failed_to_scan_blob]
- )
- .and_call_original
-
- expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
- expect(instance).to receive(:secrets_scan)
- .with(
- array_including(new_blob, timed_out_blob, failed_to_scan_blob),
- timeout: kind_of(Float)
- )
- .and_return(successful_scan_with_errors_response)
- end
-
- expect(secret_detection_logger).to receive(:info)
- .once
- .with(message: log_messages[:found_secrets_with_errors])
-
- expect { subject.validate! }.to raise_error(::Gitlab::GitAccess::ForbiddenError)
- end
-
- it 'filters existing blobs out' do
- expect_next_instance_of(::Gitlab::Checks::ChangedBlobs) do |instance|
- # old blob is expected to be filtered out
- expect(instance).to receive(:filter_existing!)
- .with(
- array_including(old_blob, new_blob, timed_out_blob, failed_to_scan_blob)
- )
- .once
- .and_return(
- array_including(new_blob, timed_out_blob, failed_to_scan_blob)
- )
- .and_call_original
- end
-
- expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
- expect(instance).to receive(:secrets_scan)
- .with(
- array_including(new_blob, timed_out_blob, failed_to_scan_blob),
- timeout: kind_of(Float)
- )
- .and_return(successful_scan_with_errors_response)
- end
-
- expect(secret_detection_logger).to receive(:info)
- .once
- .with(message: log_messages[:found_secrets_with_errors])
-
- expect { subject.validate! }.to raise_error(::Gitlab::GitAccess::ForbiddenError)
- end
+ let(:timed_out_diff_blob) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: timed_out_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+TOKEN=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
end
- context 'with no quarantine directory' do
- it 'list new blobs' do
- expect(repository).to receive(:list_blobs)
- .with(
- ['--not', '--all', '--not'] + changes.pluck(:newrev),
- bytes_limit: Gitlab::Checks::SecretsCheck::BLOB_BYTES_LIMIT + 1,
- with_paths: false,
- dynamic_timeout: kind_of(Float)
- )
- .once
- .and_return(
- array_including(new_blob, old_blob, timed_out_blob, failed_to_scan_blob)
- )
- .and_call_original
-
- expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
- expect(instance).to receive(:secrets_scan)
- .with(
- array_including(new_blob, timed_out_blob, failed_to_scan_blob),
- timeout: kind_of(Float)
- )
- .and_return(successful_scan_with_errors_response)
- end
-
- expect(secret_detection_logger).to receive(:info)
- .once
- .with(message: log_messages[:found_secrets_with_errors])
-
- expect { subject.validate! }.to raise_error(::Gitlab::GitAccess::ForbiddenError)
- end
+ let(:failed_to_scan_diff_blob) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: failed_to_scan_blob_reference,
+ patch: "@@ -0,0 +1 @@\n+GLPAT=glpat-JUST20LETTERSANDNUMB\n\\ No newline at end of file\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
end
- it 'scans blobs' do
+ it 'scans diffs' do
expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
expect(instance).to receive(:secrets_scan)
.with(
- array_including(new_blob, timed_out_blob, failed_to_scan_blob),
+ array_including(diff_blob, timed_out_diff_blob, failed_to_scan_diff_blob),
timeout: kind_of(Float)
)
.once
.and_return(successful_scan_with_errors_response)
end
- expect_next_instance_of(described_class) do |instance|
- expect(instance).to receive(:format_response)
- .with(successful_scan_with_errors_response)
- .once
- .and_call_original
- end
-
expect(secret_detection_logger).to receive(:info)
.once
.with(message: log_messages[:found_secrets_with_errors])
@@ -862,7 +1068,7 @@
expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
expect(instance).to receive(:secrets_scan)
.with(
- array_including(new_blob, timed_out_blob, failed_to_scan_blob),
+ array_including(diff_blob, timed_out_diff_blob, failed_to_scan_diff_blob),
timeout: kind_of(Float)
)
.once
@@ -907,12 +1113,25 @@
end
context 'when a blob has multiple secrets' do
+ let_it_be(:secret1) { 'SECRET=glpat-JUST20LETTERSANDNUMB' } # gitleaks:allow
+ let_it_be(:secret2) { 'TOKEN=glpat-JUST20LETTERSANDNUMB' } # gitleaks:allow
+
let_it_be(:new_commit) do
- create_commit('.env' => "SECRET=glpat-JUST20LETTERSANDNUMB\nTOKEN=glpat-JUST20LETTERSANDNUMB") # gitleaks:allow
+ create_commit('.env' => "#{secret1}\n#{secret2}")
end
let(:new_blob_reference) { '59ef300b246861163ee1e2ab4146e16144e4770f' }
- let(:new_blob) { have_attributes(class: Gitlab::Git::Blob, id: new_blob_reference, size: 66) }
+ let(:diff_blob) do
+ have_attributes(
+ class: Gitlab::GitalyClient::DiffBlob,
+ left_blob_id: Gitlab::Git::SHA1_BLANK_SHA,
+ right_blob_id: new_blob_reference,
+ patch: "@@ -0,0 +1,2 @@\n+#{secret1}\n+#{secret2}\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
+ end
let(:successful_scan_with_multiple_findings_and_errors_response) do
::Gitlab::SecretDetection::Response.new(
@@ -934,7 +1153,7 @@
),
Gitlab::SecretDetection::Finding.new(
timed_out_blob_reference,
- Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+ Gitlab::SecretDetection::Status::DIFF_TIMEOUT
),
Gitlab::SecretDetection::Finding.new(
failed_to_scan_blob_reference,
@@ -948,20 +1167,13 @@
expect_next_instance_of(::Gitlab::SecretDetection::Scan) do |instance|
expect(instance).to receive(:secrets_scan)
.with(
- array_including(new_blob, timed_out_blob, failed_to_scan_blob),
+ array_including(diff_blob, timed_out_diff_blob, failed_to_scan_diff_blob),
timeout: kind_of(Float)
)
.once
.and_return(successful_scan_with_multiple_findings_and_errors_response)
end
- expect_next_instance_of(described_class) do |instance|
- expect(instance).to receive(:format_response)
- .with(successful_scan_with_multiple_findings_and_errors_response)
- .once
- .and_call_original
- end
-
expect(secret_detection_logger).to receive(:info)
.once
.with(message: log_messages[:found_secrets_with_errors])
@@ -1082,7 +1294,7 @@
let_it_be(:new_commit) do
create_commit(
{ '.env' => 'SECRET=glpat-JUST20LETTERSANDNUMB' }, # gitleaks:allow
- 'dummy commit [skip secret push protection]'
+ 'skip scanning [skip secret push protection]'
)
end
diff --git a/gems/gitlab-secret_detection/README.md b/gems/gitlab-secret_detection/README.md
index 9fc770a8bf5b6dccad9426a382ce40d369c097a2..ede38a5a01a07837acf871b8bcd19bf9cac1da87 100644
--- a/gems/gitlab-secret_detection/README.md
+++ b/gems/gitlab-secret_detection/README.md
@@ -1,6 +1,6 @@
# Gitlab::SecretDetection
-The gitlab-secret_detection gem performs keyword and regex matching on git blobs that may include secrets. The gem accepts one or more git blobs, matches them against a defined ruleset of regular expressions, and returns scan results.
+The gitlab-secret_detection gem performs keyword and regex matching on git diffs that may include secrets. The gem accepts one or more git diffs, matches them against a defined ruleset of regular expressions, and returns scan results.
##### Scan parameters
@@ -10,10 +10,10 @@ accepts the following parameters:
| Parameter | Type | Required | Default | Description |
|----------------|---------|----------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| `blobs` | Array | Yes | NA | Array of blobs with each blob to have `id` and `data` properties. `id` represents the uniqueness of the blob in the given array and `data` is the content of the blob to scan. |
+| `diffs` | Array | Yes | NA | Array of diffs. Each diff has attributes: left_blob_id, right_blob_id, patch, status, binary, and over_patch_bytes_limit. |
| `timeout` | Number | No | [`60s`](https://gitlab.com/gitlab-org/gitlab/-/blob/5dfcf7431bfff25519c05a7e66c0cbb8d7b362be/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb#L22) | The maximum duration allowed for the scan to run on a commit request comprising multiple blobs. If the specified timeout elapses, the scan is automatically terminated. The timeout duration is specified in seconds but can also accept floating-point values to denote smaller units. For instance, use `0.5` to represent `500ms`. |
-| `blob_timeout` | Number | No | [`5s`](https://gitlab.com/gitlab-org/gitlab/-/blob/5dfcf7431bfff25519c05a7e66c0cbb8d7b362be/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb#L24) | The maximum duration allowed for the scan to run on an individual blob. Upon expiration of the specified timeout, the scan is interrupted for the current blob and advances to the next blob in the request. The timeout duration is specified in seconds but can also accept floating-point values to denote smaller units. For instance, use `0.5` to represent `500ms`. |
-| `subprocess` | Boolean | No | [`true`](https://gitlab.com/gitlab-org/gitlab/-/blob/5dfcf7431bfff25519c05a7e66c0cbb8d7b362be/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb#L34) | Runs the scan operation within a subprocess rather than the main process. This design aims to mitigate memory overconsumption issues that may arise from scanning multiple large blobs within a single subprocess. Check [here](https://docs.gitlab.com/ee/architecture/blueprints/secret_detection/decisions/002_run_scan_within_subprocess.html) for more details. |
+| `diff_timeout` | Number | No | [`5s`](https://gitlab.com/gitlab-org/gitlab/-/blob/5dfcf7431bfff25519c05a7e66c0cbb8d7b362be/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb#L24) | The maximum duration allowed for the scan to run on an individual diff. Upon expiration of the specified timeout, the scan is interrupted for the current diff and advances to the next diff in the request. The timeout duration is specified in seconds but can also accept floating-point values to denote smaller units. For instance, use `0.5` to represent `500ms`. |
+| `subprocess` | Boolean | No | [`true`](https://gitlab.com/gitlab-org/gitlab/-/blob/5dfcf7431bfff25519c05a7e66c0cbb8d7b362be/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb#L34) | Runs the scan operation within a subprocess rather than the main process. This design aims to mitigate memory overconsumption issues that may arise from scanning multiple large diffs within a single subprocess. Check [here](https://docs.gitlab.com/ee/architecture/blueprints/secret_detection/decisions/002_run_scan_within_subprocess.html) for more details. |
##### Scan Constraints
diff --git a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb
index 3918d584ccd64d56befaae78c3861aafc8289bae..0b58bc4213efecd82de94bb04bd84b00827a9479 100644
--- a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb
+++ b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb
@@ -20,14 +20,14 @@ class Scan
# default time limit(in seconds) for running the scan operation per invocation
DEFAULT_SCAN_TIMEOUT_SECS = 60
- # default time limit(in seconds) for running the scan operation on a single blob
- DEFAULT_BLOB_TIMEOUT_SECS = 5
+ # default time limit(in seconds) for running the scan operation on a single diff
+ DEFAULT_DIFF_TIMEOUT_SECS = 5
# file path where the secrets ruleset file is located
RULESET_FILE_PATH = File.expand_path('../../gitleaks.toml', __dir__)
# Max no of child processes to spawn per request
# ref: https://gitlab.com/gitlab-org/gitlab/-/issues/430160
MAX_PROCS_PER_REQUEST = 5
- # Minimum cumulative size of the blobs required to spawn and
+ # Minimum cumulative size of the diffs required to spawn and
# run the scan within a new subprocess.
MIN_CHUNK_SIZE_PER_PROC_BYTES = 2_097_152 # 2MiB
# Whether to run scan in subprocesses or not. Default is true.
@@ -46,23 +46,24 @@ def initialize(logger: Logger.new($stdout), ruleset_path: RULESET_FILE_PATH)
@pattern_matcher = build_pattern_matcher(rules)
end
- # Runs Secret Detection scan on the list of given blobs. Both the total scan duration and
- # the duration for each blob is time bound via +timeout+ and +blob_timeout+ respectively.
+ # Runs Secret Detection scan on the list of given diffs. Both the total scan duration and
+ # the duration for each diff is time bound via +timeout+ and +diff_timeout+ respectively.
#
- # +blobs+:: Array of blobs with each blob to have `id` and `data` properties.
+ # +diffs+:: Array of diffs between diff pairs. Each diff has attributes: left_blob_id, right_blob_id,
+ # patch, status, binary, and over_patch_bytes_limit.
# +timeout+:: No of seconds(accepts floating point for smaller time values) to limit the total scan duration
- # +blob_timeout+:: No of seconds(accepts floating point for smaller time values) to limit
- # the scan duration on each blob
+ # +diff_timeout+:: No of seconds(accepts floating point for smaller time values) to limit
+ # the scan duration on each diff
# +subprocess+:: If passed true, the scan is performed within subprocess instead of main process.
- # To avoid over-consuming memory by running scan on multiple large blobs within a single subprocess,
- # it instead groups the blobs into smaller array where each array contains blobs with cumulative size of
+ # To avoid over-consuming memory by running scan on multiple large diffs within a single subprocess,
+ # it instead groups the diffs into smaller array where each array contains diffs with cumulative size of
# +MIN_CHUNK_SIZE_PER_PROC_BYTES+ bytes and each group runs in a separate sub-process. Default value
# is true.
#
# NOTE:
# Running the scan in fork mode primarily focuses on reducing the memory consumption of the scan by
- # offloading regex operations on large blobs to sub-processes. However, it does not assure the improvement
- # in the overall latency of the scan, specifically in the case of smaller blob sizes, where the overhead of
+ # offloading regex operations on large diffs to sub-processes. However, it does not assure the improvement
+ # in the overall latency of the scan, specifically in the case of smaller diff sizes, where the overhead of
# forking a new process adds to the overall latency of the scan instead. More reference on Subprocess-based
# execution is found here: https://gitlab.com/gitlab-org/gitlab/-/issues/430160.
#
@@ -73,23 +74,25 @@ def initialize(logger: Logger.new($stdout), ruleset_path: RULESET_FILE_PATH)
# }
#
def secrets_scan(
- blobs,
+ diffs,
timeout: DEFAULT_SCAN_TIMEOUT_SECS,
- blob_timeout: DEFAULT_BLOB_TIMEOUT_SECS,
+ diff_timeout: DEFAULT_DIFF_TIMEOUT_SECS,
subprocess: RUN_IN_SUBPROCESS
)
- return SecretDetection::Response.new(SecretDetection::Status::INPUT_ERROR) unless validate_scan_input(blobs)
+
+ return SecretDetection::Response.new(SecretDetection::Status::INPUT_ERROR) unless validate_scan_input(diffs)
Timeout.timeout(timeout) do
- matched_blobs = filter_by_keywords(blobs)
+ matched_diffs = filter_by_keywords(diffs)
- next SecretDetection::Response.new(SecretDetection::Status::NOT_FOUND) if matched_blobs.empty?
+ next SecretDetection::Response.new(SecretDetection::Status::NOT_FOUND) if matched_diffs.empty?
- secrets = if subprocess
- run_scan_within_subprocess(matched_blobs, blob_timeout)
- else
- run_scan(matched_blobs, blob_timeout)
- end
+ secrets =
+ if subprocess
+ run_scan_within_subprocess(matched_diffs, diff_timeout)
+ else
+ run_scan(matched_diffs, diff_timeout)
+ end
scan_status = overall_scan_status(secrets)
@@ -143,104 +146,137 @@ def create_keywords(rules)
secrets_keywords.flatten.compact.to_set
end
- # returns only those blobs that contain at least one of the keywords
+ # returns only those diffs that contain at least one of the keywords
# from the keywords list
- def filter_by_keywords(blobs)
- matched_blobs = []
+ def filter_by_keywords(diffs)
+ matched_diffs = []
- blobs.each do |blob|
- matched_blobs << blob if keywords.any? { |keyword| blob.data.include?(keyword) }
+ diffs.each do |diff|
+ matched_diffs << diff if keywords.any? { |keyword| diff.patch.include?(keyword) }
end
- matched_blobs.freeze
+ matched_diffs.freeze
end
- def run_scan(blobs, blob_timeout)
- found_secrets = blobs.flat_map do |blob|
- Timeout.timeout(blob_timeout) do
- find_secrets(blob)
+ def run_scan(diffs, diff_timeout)
+ found_secrets = diffs.flat_map do |diff|
+ Timeout.timeout(diff_timeout) do
+ find_secrets(diff)
end
rescue Timeout::Error => e
- logger.error "Secret Detection scan timed out on the blob(id:#{blob.id}): #{e}"
- SecretDetection::Finding.new(blob.id,
- SecretDetection::Status::BLOB_TIMEOUT)
+ logger.error "Secret Detection scan timed out on the diff(id:#{diff.right_blob_id}): #{e}"
+ SecretDetection::Finding.new(diff.right_blob_id,
+ SecretDetection::Status::DIFF_TIMEOUT)
end
found_secrets.freeze
end
- def run_scan_within_subprocess(blobs, blob_timeout)
- blob_sizes = blobs.map(&:size)
- grouped_blob_indicies = group_by_chunk_size(blob_sizes)
+ def run_scan_within_subprocess(diffs, diff_timeout)
+ diff_sizes = diffs.map { |diff| diff.patch.length }
+ grouped_diff_indicies = group_by_chunk_size(diff_sizes)
- grouped_blobs = grouped_blob_indicies.map { |idx_arr| idx_arr.map { |i| blobs[i] } }
+ grouped_diffs = grouped_diff_indicies.map { |idx_arr| idx_arr.map { |i| diffs[i] } }
found_secrets = Parallel.flat_map(
- grouped_blobs,
+ grouped_diffs,
in_processes: MAX_PROCS_PER_REQUEST,
isolation: true # do not reuse sub-processes
- ) do |grouped_blob|
- grouped_blob.flat_map do |blob|
- Timeout.timeout(blob_timeout) do
- find_secrets(blob)
+ ) do |grouped_diff|
+ grouped_diff.flat_map do |diff|
+ Timeout.timeout(diff_timeout) do
+ find_secrets(diff)
end
rescue Timeout::Error => e
- logger.error "Secret Detection scan timed out on the blob(id:#{blob.id}): #{e}"
- SecretDetection::Finding.new(blob.id,
- SecretDetection::Status::BLOB_TIMEOUT)
+ logger.error "Secret Detection scan timed out on the diff(id:#{diff.right_blob_id}): #{e}"
+ SecretDetection::Finding.new(diff.right_blob_id,
+ SecretDetection::Status::DIFF_TIMEOUT)
end
end
found_secrets.freeze
end
- # finds secrets in the given blob with a timeout circuit breaker
- def find_secrets(blob)
+ # finds secrets in the given diff with a timeout circuit breaker
+ def find_secrets(diff)
+ line_number_offset = 0
secrets = []
- blob.data.each_line.with_index do |line, index|
- patterns = pattern_matcher.match(line, exception: false)
-
- next unless patterns.any?
-
- line_number = index + 1
- patterns.each do |pattern|
- type = rules[pattern]["id"]
- description = rules[pattern]["description"]
-
- secrets << SecretDetection::Finding.new(
- blob.id,
- SecretDetection::Status::FOUND,
- line_number,
- type,
- description
- )
+ lines = diff.patch.split("\n")
+
+ # The following section parses the diff patch.
+ #
+ # If the line starts with @@, it is the hunk header, used to calculate the line number.
+ # If the line starts with +, it is newly added in this diff, and we
+ # scan the line for newly added secrets. Also increment line number.
+ # If the line starts with -, it is removed in this diff, do not increment line number.
+ # If the line starts with \\, it is the no newline marker, do not increment line number.
+ # If the line starts with a space character, it is a context line, just increment the line number.
+ #
+ # A context line that starts with an important character would still be treated
+ # like a context line, as shown below:
+ # @@ -1,5 +1,5 @@
+ # context line
+ # -removed line
+ # +added line
+ # @@this context line has a @@ but starts with a space so isnt a header
+ # +this context line has a + but starts with a space so isnt an addition
+ # -this context line has a - but starts with a space so isnt a removal
+ lines.each do |line|
+ # Parse hunk header for start line
+ if line.start_with?("@@")
+ hunk_info = line.match(/@@ -\d+(,\d+)? \+(\d+)(,\d+)? @@/)
+ start_line = hunk_info[2].to_i
+ line_number_offset = start_line - 1
+ # Line added in this commit
+ elsif line.start_with?('+')
+ line_number_offset += 1
+ # Remove leading +
+ line_content = line[1..]
+
+ patterns = pattern_matcher.match(line_content, exception: false)
+ next unless patterns.any?
+
+ patterns.each do |pattern|
+ type = rules[pattern]["id"]
+ description = rules[pattern]["description"]
+
+ secrets << SecretDetection::Finding.new(
+ diff.right_blob_id,
+ SecretDetection::Status::FOUND,
+ line_number_offset,
+ type,
+ description
+ )
+ end
+ # Line not added in this commit, just increment line number
+ elsif line.start_with?(' ')
+ line_number_offset += 1
+ # Line removed in this commit or no newline marker, do not increment line number
+ elsif line.start_with?('-', '\\')
+ # No increment
end
end
secrets
rescue StandardError => e
- logger.error "Secret Detection scan failed on the blob(id:#{blob.id}): #{e}"
+ logger.error "Secret Detection scan failed on the diff(id:#{diff.right_blob_id}): #{e}"
- SecretDetection::Finding.new(blob.id, SecretDetection::Status::SCAN_ERROR)
+ SecretDetection::Finding.new(diff.right_blob_id, SecretDetection::Status::SCAN_ERROR)
end
- def validate_scan_input(blobs)
- return false if blobs.nil? || !blobs.instance_of?(Array)
+ def validate_scan_input(diffs)
+ return false if diffs.nil? || !diffs.instance_of?(Array)
- blobs.all? do |blob|
- next false unless blob.respond_to?(:id) || blob.respond_to?(:data)
-
- blob.data.freeze # freeze blobs to avoid additional object allocations on strings
- end
+ diffs.each { |diff| diff.patch.freeze }
end
def overall_scan_status(found_secrets)
return SecretDetection::Status::NOT_FOUND if found_secrets.empty?
- timed_out_blobs = found_secrets.count { |el| el.status == SecretDetection::Status::BLOB_TIMEOUT }
+ timed_out_diffs = found_secrets.count { |el| el.status == SecretDetection::Status::DIFF_TIMEOUT }
- case timed_out_blobs
+ case timed_out_diffs
when 0
SecretDetection::Status::FOUND
when found_secrets.length
@@ -250,15 +286,15 @@ def overall_scan_status(found_secrets)
end
end
- # This method accepts an array of blob sizes(in bytes) and groups them into an array
+ # This method accepts an array of diff sizes(in bytes) and groups them into an array
# of arrays structure where each element is the group of indicies of the input
- # array whose cumulative blob sizes has at least +MIN_CHUNK_SIZE_PER_PROC_BYTES+
- def group_by_chunk_size(blob_size_arr)
+ # array whose cumulative diff sizes has at least +MIN_CHUNK_SIZE_PER_PROC_BYTES+
+ def group_by_chunk_size(diff_size_arr)
cumulative_size = 0
chunk_indexes = []
chunk_idx_start = 0
- blob_size_arr.each_with_index do |size, index|
+ diff_size_arr.each_with_index do |size, index|
cumulative_size += size
next unless cumulative_size >= MIN_CHUNK_SIZE_PER_PROC_BYTES
@@ -268,11 +304,11 @@ def group_by_chunk_size(blob_size_arr)
cumulative_size = 0
end
- if cumulative_size.positive? && (chunk_idx_start < blob_size_arr.length)
- chunk_indexes << if chunk_idx_start == blob_size_arr.length - 1
+ if cumulative_size.positive? && (chunk_idx_start < diff_size_arr.length)
+ chunk_indexes << if chunk_idx_start == diff_size_arr.length - 1
[chunk_idx_start]
else
- (chunk_idx_start..blob_size_arr.length - 1).to_a
+ (chunk_idx_start..diff_size_arr.length - 1).to_a
end
end
diff --git a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/status.rb b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/status.rb
index 200294fc2e73c3a55ebd1ce202dc4ec8e32754c2..85d60b86009a02e99597c36c18e316498317cb59 100644
--- a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/status.rb
+++ b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/status.rb
@@ -8,7 +8,7 @@ class Status
FOUND = 1 # When scan operation completes with one or more findings
FOUND_WITH_ERRORS = 2 # When scan operation completes with one or more findings along with some errors
SCAN_TIMEOUT = 3 # When the scan operation runs beyond given time out
- BLOB_TIMEOUT = 4 # When the scan operation on a blob runs beyond given time out
+ DIFF_TIMEOUT = 4 # When the scan operation on a diff runs beyond given time out
SCAN_ERROR = 5 # When the scan operation fails due to regex error
INPUT_ERROR = 6 # When the scan operation fails due to invalid input
end
diff --git a/gems/gitlab-secret_detection/spec/lib/gitlab/secret_detection/scan_spec.rb b/gems/gitlab-secret_detection/spec/lib/gitlab/secret_detection/scan_spec.rb
index 71899cd77a11c63626ca33ba6ead08b11c5cbb87..8cba55865d1812f30363b9a5050ae07254cf413f 100644
--- a/gems/gitlab-secret_detection/spec/lib/gitlab/secret_detection/scan_spec.rb
+++ b/gems/gitlab-secret_detection/spec/lib/gitlab/secret_detection/scan_spec.rb
@@ -5,10 +5,13 @@
RSpec.describe Gitlab::SecretDetection::Scan, feature_category: :secret_detection do
subject(:scan) { described_class.new }
- def new_blob(id:, data:)
- Struct.new(:id, :data).new(id, data)
+ let(:diff_blob) do
+ Struct.new(:left_blob_id, :right_blob_id, :patch, :status, :binary, :over_patch_bytes_limit, keyword_init: true)
end
+ let(:sha1_blank_sha) { ('0' * 40).freeze }
+ let(:sample_blob_id) { 'fe29d93da4843da433e62711ace82db601eb4f8f' }
+
let(:ruleset) do
{
"title" => "gitleaks config",
@@ -64,47 +67,108 @@ def new_blob(id:, data:)
allow(scan).to receive(:parse_ruleset).and_return(ruleset)
end
- context 'when the blob does not contain a secret' do
- let(:blobs) do
+ context 'when the diff does not contain a secret' do
+ let(:diffs) do
[
- new_blob(id: 1234, data: "no secrets")
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1 @@\n+BASE_URL=https://foo.bar\n\\ No newline at end of file\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
]
end
it "does not match" do
expected_response = Gitlab::SecretDetection::Response.new(Gitlab::SecretDetection::Status::NOT_FOUND)
- expect(scan.secrets_scan(blobs)).to eq(expected_response)
+ expect(scan.secrets_scan(diffs)).to eq(expected_response)
end
- it "attempts to keyword match returning no blobs for further scan" do
+ it "attempts to keyword match returning no diffs for further scan" do
expect(scan).to receive(:filter_by_keywords)
- .with(blobs)
+ .with(diffs)
.and_return([])
- scan.secrets_scan(blobs)
+ scan.secrets_scan(diffs)
end
it "does not attempt to regex match" do
expect(scan).not_to receive(:match_rules_bulk)
- scan.secrets_scan(blobs)
+ scan.secrets_scan(diffs)
end
end
- context "when multiple blobs contains secrets" do
- let(:blobs) do
+ context "when multiple diffs contains secrets" do
+ let(:diffs) do
[
- new_blob(id: 111, data: "glpat-12312312312312312312"), # gitleaks:allow
- new_blob(id: 222, data: "\n\nglptt-1231231231231231231212312312312312312312"), # gitleaks:allow
- new_blob(id: 333, data: "data with no secret"),
- new_blob(id: 444,
- data: "GR134894112312312312312312312\nglft-12312312312312312312"), # gitleaks:allow
- new_blob(id: 555, data: "data with no secret"),
- new_blob(id: 666, data: "data with no secret"),
- new_blob(id: 777, data: "\nglptt-1231231231231231231212312312312312312312"), # gitleaks:allow
- new_blob(id: 888,
- data: "glpat-12312312312312312312;GR134894112312312312312312312") # gitleaks:allow
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1 @@\n+glpat-12312312312312312312\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,3 @@\n+\n+\n+glptt-1231231231231231231212312312312312312312\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1 @@\n+data with no secret\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,2 @@\n+GR134894112312312312312312312\n+glft-12312312312312312312\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1 @@\n+data with no secret\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1 @@\n+data with no secret\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1 @@\n+glptt-1231231231231231231212312312312312312312\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,2 @@\n+glpat-12312312312312312312\n+GR134894112312312312312312312\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
]
end
@@ -113,51 +177,51 @@ def new_blob(id:, data:)
Gitlab::SecretDetection::Status::FOUND,
[
Gitlab::SecretDetection::Finding.new(
- blobs[0].id,
+ diffs[0].right_blob_id,
Gitlab::SecretDetection::Status::FOUND,
1,
ruleset['rules'][0]['id'],
ruleset['rules'][0]['description']
),
Gitlab::SecretDetection::Finding.new(
- blobs[1].id,
+ diffs[1].right_blob_id,
Gitlab::SecretDetection::Status::FOUND,
3,
ruleset['rules'][1]['id'],
ruleset['rules'][1]['description']
),
Gitlab::SecretDetection::Finding.new(
- blobs[3].id,
+ diffs[3].right_blob_id,
Gitlab::SecretDetection::Status::FOUND,
1,
ruleset['rules'][2]['id'],
ruleset['rules'][2]['description']
),
Gitlab::SecretDetection::Finding.new(
- blobs[3].id,
+ diffs[3].right_blob_id,
Gitlab::SecretDetection::Status::FOUND,
2,
ruleset['rules'][3]['id'],
ruleset['rules'][3]['description']
),
Gitlab::SecretDetection::Finding.new(
- blobs[6].id,
+ diffs[6].right_blob_id,
Gitlab::SecretDetection::Status::FOUND,
- 2,
+ 1,
ruleset['rules'][1]['id'],
ruleset['rules'][1]['description']
),
Gitlab::SecretDetection::Finding.new(
- blobs[7].id,
+ diffs[7].right_blob_id,
Gitlab::SecretDetection::Status::FOUND,
1,
ruleset['rules'][0]['id'],
ruleset['rules'][0]['description']
),
Gitlab::SecretDetection::Finding.new(
- blobs[7].id,
+ diffs[7].right_blob_id,
Gitlab::SecretDetection::Status::FOUND,
- 1,
+ 2,
ruleset['rules'][2]['id'],
ruleset['rules'][2]['description']
)
@@ -165,90 +229,93 @@ def new_blob(id:, data:)
)
end
- it "attempts to keyword match returning only filtered blobs for further scan" do
- expected = blobs.filter { |b| b.data != "data with no secret" }
+ it "attempts to keyword match returning only filtered diffs for further scan" do
+ expected = diffs.reject { |d| d.patch.include?("data with no secret") }
expect(scan).to receive(:filter_by_keywords)
- .with(blobs)
+ .with(diffs)
.and_return(expected)
- scan.secrets_scan(blobs)
+ scan.secrets_scan(diffs)
end
it "matches multiple rules when running in main process" do
- expect(scan.secrets_scan(blobs, subprocess: false)).to eq(expected_response)
- end
-
- context "in subprocess" do
- let(:dummy_lines) do
- 10_000
- end
-
- let(:large_blobs) do
- dummy_data = "\nrandom data" * dummy_lines
- [
- new_blob(id: 111, data: "glpat-12312312312312312312#{dummy_data}"), # gitleaks:allow
- new_blob(id: 222, data: "\n\nglptt-1231231231231231231212312312312312312312#{dummy_data}"), # gitleaks:allow
- new_blob(id: 333, data: "data with no secret#{dummy_data}"),
- new_blob(id: 444,
- data: "GR134894112312312312312312312\nglft-12312312312312312312#{dummy_data}"), # gitleaks:allow
- new_blob(id: 555, data: "data with no secret#{dummy_data}"),
- new_blob(id: 666, data: "data with no secret#{dummy_data}"),
- new_blob(id: 777, data: "#{dummy_data}\nglptt-1231231231231231231212312312312312312312") # gitleaks:allow
- ]
- end
-
- it "matches multiple rules" do
- expect(scan.secrets_scan(blobs, subprocess: true)).to eq(expected_response)
- end
-
- it "allocates less memory than when running in main process" do
- forked_stats = Benchmark::Malloc.new.run { scan.secrets_scan(large_blobs, subprocess: true) }
- non_forked_stats = Benchmark::Malloc.new.run { scan.secrets_scan(large_blobs, subprocess: false) }
-
- max_processes = Gitlab::SecretDetection::Scan::MAX_PROCS_PER_REQUEST
-
- forked_memory = forked_stats.allocated.total_memory
- non_forked_memory = non_forked_stats.allocated.total_memory
- forked_obj_allocs = forked_stats.allocated.total_objects
- non_forked_obj_allocs = non_forked_stats.allocated.total_objects
-
- expect(non_forked_memory).to be >= forked_memory * max_processes
- expect(non_forked_obj_allocs).to be >= forked_obj_allocs * max_processes
- end
+ expect(scan.secrets_scan(diffs, subprocess: false)).to eq(expected_response)
end
end
context "when configured with time out" do
- let(:each_blob_timeout_secs) { 0.000_001 } # 1 micro-sec to intentionally timeout large blob
+ let(:each_diff_timeout_secs) { 0.000_001 } # 1 micro-sec to intentionally timeout large diff
let(:large_data) do
- ("large data with a secret glpat-12312312312312312312\n" * 10_000_000).freeze # gitleaks:allow
+ ("\n+large data with a secret glpat-12312312312312312312" * 10_000_000).freeze # gitleaks:allow
end
- let(:blobs) do
+ let(:diffs) do
[
- new_blob(id: 111, data: "GR134894112312312312312312312"), # gitleaks:allow
- new_blob(id: 333, data: "data with no secret"),
- new_blob(id: 333, data: large_data)
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,2 @@\n+GR134894112312312312312312312\n", # gitleaks:allow
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,2 @@\n+data with no secret\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,10000001 @@\n#{large_data}\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
]
end
- let(:all_large_blobs) do
+ let(:all_large_diffs) do
[
- new_blob(id: 111, data: large_data),
- new_blob(id: 222, data: large_data),
- new_blob(id: 333, data: large_data)
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,10000001 @@\n#{large_data}\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,10000001 @@\n#{large_data}\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ ),
+ diff_blob.new(
+ left_blob_id: sha1_blank_sha,
+ right_blob_id: sample_blob_id,
+ patch: "@@ -0,0 +1,10000001 @@\n#{large_data}\n",
+ status: :STATUS_END_OF_PATCH,
+ binary: false,
+ over_patch_bytes_limit: false
+ )
]
end
it "whole secret detection scan operation times out" do
- scan_timeout_secs = 0.000_001 # 1 micro-sec to intentionally timeout large blob
+ scan_timeout_secs = 0.000_001 # 1 micro-sec to intentionally timeout large diff
expected_response = Gitlab::SecretDetection::Response.new(Gitlab::SecretDetection::Status::SCAN_TIMEOUT)
begin
- response = scan.secrets_scan(blobs, timeout: scan_timeout_secs)
+ response = scan.secrets_scan(diffs, timeout: scan_timeout_secs)
expect(response).to eq(expected_response)
rescue ArgumentError
# When RSpec's main process terminates and attempts to clean up child processes upon completion, it terminates
@@ -264,50 +331,50 @@ def new_blob(id:, data:)
end
end
- it "one of the blobs times out while others continue to get scanned" do
+ it "one of the diffs times out while others continue to get scanned" do
expected_response = Gitlab::SecretDetection::Response.new(
Gitlab::SecretDetection::Status::FOUND_WITH_ERRORS,
[
Gitlab::SecretDetection::Finding.new(
- blobs[0].id,
+ diffs[0].right_blob_id,
Gitlab::SecretDetection::Status::FOUND,
1,
ruleset['rules'][2]['id'],
ruleset['rules'][2]['description']
),
Gitlab::SecretDetection::Finding.new(
- blobs[2].id,
- Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+ diffs[2].right_blob_id,
+ Gitlab::SecretDetection::Status::DIFF_TIMEOUT
)
]
)
- expect(scan.secrets_scan(blobs, blob_timeout: each_blob_timeout_secs)).to eq(expected_response)
+ expect(scan.secrets_scan(diffs, diff_timeout: each_diff_timeout_secs)).to eq(expected_response)
end
- it "all the blobs time out" do
- # scan status changes to SCAN_TIMEOUT when *all* the blobs time out
+ it "all the diffs time out" do
+ # scan status changes to SCAN_TIMEOUT when *all* the diffs time out
expected_scan_status = Gitlab::SecretDetection::Status::SCAN_TIMEOUT
expected_response = Gitlab::SecretDetection::Response.new(
expected_scan_status,
[
Gitlab::SecretDetection::Finding.new(
- all_large_blobs[0].id,
- Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+ all_large_diffs[0].right_blob_id,
+ Gitlab::SecretDetection::Status::DIFF_TIMEOUT
),
Gitlab::SecretDetection::Finding.new(
- all_large_blobs[1].id,
- Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+ all_large_diffs[1].right_blob_id,
+ Gitlab::SecretDetection::Status::DIFF_TIMEOUT
),
Gitlab::SecretDetection::Finding.new(
- all_large_blobs[2].id,
- Gitlab::SecretDetection::Status::BLOB_TIMEOUT
+ all_large_diffs[2].right_blob_id,
+ Gitlab::SecretDetection::Status::DIFF_TIMEOUT
)
]
)
- expect(scan.secrets_scan(all_large_blobs, blob_timeout: each_blob_timeout_secs)).to eq(expected_response)
+ expect(scan.secrets_scan(all_large_diffs, diff_timeout: each_diff_timeout_secs)).to eq(expected_response)
end
end
end