-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
refactor(secret): add warning about large files #7085
refactor(secret): add warning about large files #7085
Conversation
pkg/fanal/analyzer/secret/secret.go
Outdated
@@ -165,6 +166,9 @@ func (a *SecretAnalyzer) Required(filePath string, fi os.FileInfo) bool { | |||
return false | |||
} | |||
|
|||
if size := fi.Size(); size > 209_715_200 { // 200MB |
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.
I'm not sure about the size
@knqyf263 wdyt?
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.
I feel even 10 MB is large for plain text.
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.
yes, I probably overdid it when I chose 200 MB 😄
updated in c3d5e4c
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.
Can we have this configurable i.e if value is passed through flag than we can consider it and skip the scanning?
If not provided then we can show the warning and proceed with the scanning.
--file-size-threshold 10mb
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.
I'm not sure new flag is good idea, because we will only be using it for secret
scanner.
But we can think about adding this option to secret config file
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.
Sure, anything would be fine, since this would be helpful a lot.
We can easily add a config and scan whole registry and based on the threshold, trivy will skip the scanning.
Otherwise we will have to manually add code logic to look for the warnings.
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.
But for standalone trivy scanning, IMO flag would make sense.
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.
The size varies depending on scanner (vulnerability, misconfiguration and secret) and analyzer (npm and go binary). Adding the flag needs to be carefully designed. We'll merge this PR with the current implementation as this warning is at least useful.
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.
Should I go ahead and add a feature enhancement Issue?
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.
Yes, you can open a discussion.
@DmitriyLewen I realized it even shows a warning for binary files. We should show it after checking the file type as a binary file is skipped regardless of the file size. |
Nice catch! |
Description
Show warnings about large files so users can skip them.
See #7078 for more details.
example:
Related discussions
Checklist