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

Fix new secret scanning #9

Merged
merged 1 commit into from
Jul 14, 2022
Merged

Fix new secret scanning #9

merged 1 commit into from
Jul 14, 2022

Conversation

bgolding355
Copy link
Contributor

Fix Detect secret scanning

There are two ways to look for new secrets based on the README for Yelp/detect-secrets. In this Github action, I managed to combine both together. While the combination mostly works, I have a (private) PR where it did not behave as expected. The two ways to do this are:

Using detect-secrets

detect-secrets scan --baseline .secrets.baseline

Using detect-secrets-hook

git ls-files -z | xargs -0 detect-secrets-hook --baseline .secrets.baseline

This approach has the benefit of mutating .secrets.baseline, which makes it easier to generate the new file.

I managed to combine these two approaches into:

git ls-files -z | xargs -0 detect-secrets scan $DETECT_SECRET_ADDITIONAL_ARGS --baseline "$BASELINE_FILE"

Testing locally, it seems that detect-secrets is much faster, but when I get around to implementing #6 I will switch to detect-secrets-hook.

@bgolding355
Copy link
Contributor Author

bgolding355 commented Jul 14, 2022

@jsoref an example of the problem can be found in this (private) PR. https://github.com/GarnerCorp/lighthouse/pull/4566

The line:

git ls-files -z | xargs -0 detect-secrets scan $DETECT_SECRET_ADDITIONAL_ARGS --baseline "$BASELINE_FILE"

is creating a .secrets.baseline that is very strange, and the resulting JOBS SUMMARY is wrong. When you have time, I will show you this bug, and explain how this change fixes it.

I explained this in further detail in this (private) comment https://github.com/GarnerCorp/lighthouse/pull/4566#issuecomment-1184760230. I don't want to go into to much detail in a public repo

@bgolding355 bgolding355 merged commit 55e02af into main Jul 14, 2022
@bgolding355 bgolding355 deleted the fix-new-secret-scanning branch July 14, 2022 18: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