-
Notifications
You must be signed in to change notification settings - Fork 455
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
improved adhoc string scanning #386
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
calvinli
reviewed
Jan 13, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine overall, just have a few comments
…re-v1-better-adhoc-string-scanning
calvinli
approved these changes
Jan 13, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
jimmyhlee94
pushed a commit
to jimmyhlee94/detect-secrets
that referenced
this pull request
Aug 19, 2021
Follow up of Yelp#379
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Adhoc string scanning is hella useful. e.g.
$ detect-secrets scan --string 'butactuallynotasecret' ... Base64HighEntropyString: False
However, sometimes (especially for the HighEntropyString plugins), we want to see why it failed. This is existing behavior in 0.14.3; this PR carries this functionality forward. With this change, it will be:
$ detect-secrets scan --string 'butactuallynotasecret' ... Base64HighEntropyString: False (3.404)
Reviewer Notes
It was somewhat difficult to surface the non-secret to the scan layer in a generic manner (i.e. without using
if plugin in {Base64HighEntropyString}
), so I had to be creative with theanalyze_line
andanalyze_string
functionality. As a result, for people using theanalyze_string
function directly (like the test case), they would need to be responsible for filtering out the secret themselves.Not ideal, but it's the best way I could find.
This PR also introduces another gotcha -- for our DI system, we now leverage the pythonic fashion of duck-typing to determine whether there are enough required parameters to inject into the function. This needed to be done to support cases where the underlying function had default values for their parameters (e.g.
HighEntropyStringsPlugin.analyze_line
), and as such, we don't need to supply all the parameters that a function needs. However, this also means that if the underlying function raises an uncaughtTypeError
, it will be silenced by the DI system.I guess future improvements could be done to distinguish default parameters from non-default parameters in a function declaration, but I didn't want to dive too deep into Python magic internals for this PR.