Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(output): Only fail secret scans when the secret is introduced #1010

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Walz
Copy link
Collaborator

@Walz Walz commented Nov 14, 2024

Context

Related to SPI-526 and will close #1001

We want to only fail secret scans when the secret is introduced.
New fields have been added to the API to automatically detect if the content is a diff and if the secret has been added, deleted or in the context. (GitGuardian/py-gitguardian#122)

What has been done

Make use of the new fields to return success when no secret are added.

TODO:

  • Update outputs
  • Add an argument to enable the previous behavior (fail for all kind of secrets)

Validation

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.04%. Comparing base (b82d7ca) to head (db52127).

Files with missing lines Patch % Lines
...ticals/secret/output/secret_text_output_handler.py 87.50% 2 Missing ⚠️
...gshield/verticals/secret/secret_scan_collection.py 96.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1010      +/-   ##
==========================================
- Coverage   92.04%   92.04%   -0.01%     
==========================================
  Files         181      181              
  Lines        7704     7740      +36     
==========================================
+ Hits         7091     7124      +33     
- Misses        613      616       +3     
Flag Coverage Δ
unittests 92.04% <93.75%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@Walz Walz force-pushed the samuel/spi-526-implement-deletion-commit-identification-in-ggshield branch from 455a373 to ee9c949 Compare November 15, 2024 14:51
Comment on lines 83 to 90
policy_breaks = self.scan.policy_breaks
if diff_kinds is not None and self.scan.is_diff:
policy_breaks = [
policy_break
for policy_break in self.scan.policy_breaks
if policy_break.diff_kind in diff_kinds
]
return policy_breaks
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can be slightly simplified to:

Suggested change
policy_breaks = self.scan.policy_breaks
if diff_kinds is not None and self.scan.is_diff:
policy_breaks = [
policy_break
for policy_break in self.scan.policy_breaks
if policy_break.diff_kind in diff_kinds
]
return policy_breaks
if diff_kinds is not None and self.scan.is_diff:
return [
policy_break
for policy_break in self.scan.policy_breaks
if policy_break.diff_kind in diff_kinds
]
return self.scan.policy_breaks

@@ -70,6 +77,18 @@ def censor(self) -> None:
def has_policy_breaks(self) -> bool:
return self.scan.has_policy_breaks

def policy_breaks(
self, *, diff_kinds: Optional[List[DiffKind]] = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nicer if diff_kinds was a set instead of a list, since we don't want to have duplicate kinds.

@@ -70,6 +77,18 @@ def censor(self) -> None:
def has_policy_breaks(self) -> bool:
return self.scan.has_policy_breaks

def policy_breaks(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: this should be get_policy_breaks().

@Walz Walz force-pushed the samuel/spi-526-implement-deletion-commit-identification-in-ggshield branch from ee9c949 to db52127 Compare November 20, 2024 16:38
Copy link

gitguardian bot commented Nov 20, 2024

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
14567116 Triggered GitGuardian Test Token Checked db52127 tests/unit/cassettes/test_scan_file_secret-True.yaml View secret
14567116 Triggered GitGuardian Test Token Checked db52127 tests/unit/cassettes/test_scan_file_secret_with_validity.yaml View secret
14567116 Triggered GitGuardian Test Token Checked db52127 tests/unit/cassettes/test_scan_file_secret_with_validity.yaml View secret
14567116 Triggered GitGuardian Test Token Checked db52127 tests/unit/cassettes/test_scan_file_secret-True.yaml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Fail only upon adding a secret (not when removing or when a secret is on the line above or below).
2 participants