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

Mmillet/spi 515 add all secrets option to ggshield secret scans #1024

Conversation

gg-mmill
Copy link
Contributor

@gg-mmill gg-mmill commented Nov 27, 2024

  • Add the --all-secrets option, such that ignore options are "ignored": all secrets are returned, with is_excluded / exclude_reason (sic) set appropriately in the policy breaks.
  • Adds a message when excluded secrets are found, to advertise the feature
  • Serialize secret options in a header, for option usage monitoring

What has been done

  • add --all-secret option
  • update output handlers
    • json output is always extended
    • text output
      • when option is not passed, add line below file info showing number of hidden secrets
      • when option is passed, show all secrets, and add Exclude reason when appropriate

Validation

Run ggshield scans on a file containing secrets with ignored / resolved issues, with and without --all-secrets option

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.

@gg-mmill gg-mmill requested a review from a team as a code owner November 27, 2024 15:04
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.05%. Comparing base (ee56436) to head (cb39b68).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1024      +/-   ##
==========================================
+ Coverage   92.03%   92.05%   +0.01%     
==========================================
  Files         181      181              
  Lines        7708     7726      +18     
==========================================
+ Hits         7094     7112      +18     
  Misses        614      614              
Flag Coverage Δ
unittests 92.05% <100.00%> (+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.

@gg-mmill gg-mmill force-pushed the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch 2 times, most recently from 6f89d8d to c6b96ce Compare November 29, 2024 09:48
@gg-mmill gg-mmill force-pushed the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch 2 times, most recently from 1047a26 to b6cb659 Compare December 2, 2024 09:42
@agateau-gg agateau-gg marked this pull request as draft December 2, 2024 09:58
@gg-mmill gg-mmill force-pushed the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch from b6cb659 to f9bb794 Compare December 2, 2024 18:22
@gg-mmill gg-mmill self-assigned this Dec 2, 2024
@gg-mmill gg-mmill force-pushed the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch from f9bb794 to 0e48da5 Compare December 3, 2024 14:17
- remove `ignore_known_secrets=True`, which didn't make sense
- add `all_secrets=True`, to get all secrets found by the backend

Notes:
- Since we add `all_secrets=True`, we also need to filter-out secrets                                                                                                          [13:30:5         excluded by the backend
- Since we changed the query params, all secret-related cassettes
  have been rewritten
@gg-mmill gg-mmill force-pushed the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch from 0e48da5 to 9c8dba5 Compare December 3, 2024 14:26
@gg-mmill gg-mmill marked this pull request as ready for review December 3, 2024 14:50
@gg-mmill gg-mmill force-pushed the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch 5 times, most recently from f63eef8 to 8f8252c Compare December 4, 2024 16:53
Copy link
Collaborator

@agateau-gg agateau-gg left a comment

Choose a reason for hiding this comment

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

Minor remarks, but this looks good, and the addition of factories is great!

tests/unit/verticals/secret/test_secret_scanner.py Outdated Show resolved Hide resolved
ggshield/verticals/secret/secret_scan_collection.py Outdated Show resolved Hide resolved
ggshield/verticals/secret/secret_scan_collection.py Outdated Show resolved Hide resolved
ggshield/verticals/secret/secret_scan_collection.py Outdated Show resolved Hide resolved
ggshield/verticals/secret/secret_scan_collection.py Outdated Show resolved Hide resolved
tests/factories.py Outdated Show resolved Hide resolved
tests/unit/verticals/secret/test_secret_scan_collection.py Outdated Show resolved Hide resolved
ggshield/core/config/user_config.py Outdated Show resolved Hide resolved
@gg-mmill gg-mmill force-pushed the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch 7 times, most recently from bb30607 to 8ae13a4 Compare December 9, 2024 10:34
@gg-mmill gg-mmill force-pushed the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch from 8ae13a4 to cb39b68 Compare December 9, 2024 10:36
Copy link
Collaborator

@agateau-gg agateau-gg left a comment

Choose a reason for hiding this comment

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

Looks good!

@gg-mmill gg-mmill merged commit 6d0d8b8 into main Dec 9, 2024
33 checks passed
@gg-mmill gg-mmill deleted the mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans_bis branch December 9, 2024 10:55
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.

2 participants