-
Notifications
You must be signed in to change notification settings - Fork 875
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: move invalid token to log field #1171
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1171 +/- ##
==========================================
+ Coverage 61.89% 62.70% +0.80%
==========================================
Files 23 23
Lines 1475 1488 +13
==========================================
+ Hits 913 933 +20
+ Misses 476 471 -5
+ Partials 86 84 -2
Continue to review full report at Codecov.
|
😩 Welp, this PR actually does escape the input as a "safe" go-string, which should not trigger CWE-117 (all non-text characters are escaped, including newlines etc.). |
Soo, are we closing this, or are you still looking to continue at some point? The easIER solution here would probably be to just escape it and log it as-is. |
Well, it is escaped (using %q). Not sure how we could escape it in a way that doesn't trigger the security rule. I think it's marginally better in this version, since it's clearly distinguished from an actual log message, but we could also just remove it all together. |
I think I prefer the second option |
Prevents potential log manipulation. Probably not a realistic concern, but fixing it is easier than dismissing the warnings.