-
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
fix(secret): trim excessively long lines #7192
Conversation
@knqyf263 @DmitriyLewen could you take a look at this PR when yuo have free time? thanks I'm not sure about 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.
Left a comment about max length of line.
The rest looks good to me.
if len(rawLine) > maxLineLength { | ||
strRawLine = lo.Ternary(inCause, matchLine, string(rawLine[:maxLineLength])) | ||
} else { | ||
strRawLine = string(rawLine) | ||
} |
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 like 100 characters is enough.
- For lines before and after secret - we don't need such a big line.
- For secret line - if length of secret with secret > 100 - we use
30 + secret + 20 characters
:
trivy/pkg/fanal/secret/scanner.go
Lines 501 to 504 in 36ea011
if lineEnd-lineStart > 100 { lineStart = lo.Ternary(start-30 < 0, 0, start-30) lineEnd = lo.Ternary(end+20 > len(content), len(content), end+20) }
Now 2000 is a length for backward compatibility with RSA keys.
IIUC you want to include full RSA key to result (-----BEGIN RSA PRIVATE KEY-----
and -----END RSA PRIVATE KEY-----
), but I think this is not necessary.
But when we use 2000, we can see case when secret length is 10, but the user sees 1900 unneeded characters.
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.
@DmitriyLewen I generally agree with you. 2k is too long line.
Content: "----BEGIN RSA PRIVATE KEY-----**************************************************************************************************************************-----END RSA PRIVATE", | ||
Highlighted: "----BEGIN RSA PRIVATE KEY-----**************************************************************************************************************************-----END RSA PRIVATE", |
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.
@DmitriyLewen I've decreased maxLineLength
and updated the test.
it shows before/after.
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
cc. @knqyf263
Description
In this PR, we address the issue of handling excessively long lines in files.
When a line is too lengthy, displaying the entire content is unnecessary and inefficient.
In general cases, such as obfuscated JavaScript code, we truncate the line appropriately to improve readability.
If a long line contains a secret, we only display the segment containing the secret.
Before:
After:
Related issues
Checklist