Repository navigation
user: keep usernames out of console format strings - #90
Merged
Merged
Conversation
console.warn and console.error treat their first argument as a format string. With the username interpolated into it, a username holding %o or %s consumed the error argument, so the log showed the username wrapped around the error and dropped the error itself. The username is now a %s argument (CodeQL js/tainted-format-string, alerts 25-27).
There was a problem hiding this comment.
🟢 Approval recommended
All three targeted replacements correctly isolate usernames from format strings while preserving the error argument.
0 open findings
What changed in this PR
Prevents usernames from being interpreted as console format strings in authentication error logs.
Changes:
- Uses constant format strings and passes usernames as
%sarguments in all three affected logs. - Preserves error logging and authentication behavior.
| File | Description |
|---|---|
| lib/user/store/mysql.js | Safely logs usernames on password hash upgrade failure. |
| lib/user/store/file.js | Applies the same fix to file-backed storage. |
| lib/user/credentials.js | Safely logs usernames on PBKDF2 failure. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or 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
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.
console.warnandconsole.errortreat their first argument as a format string. Three login log lines interpolated the username into that string and passederrafter it, so a username containing%oor%sconsumed the error. With username%o%s@example.comthe line came out as… for user 'boom'%s@example.comand the error was dropped.The username is now passed as a
%sargument with a constant format string:lib/user/credentials.js:108(PBKDF2 failure)lib/user/store/file.js:81andlib/user/store/mysql.js:55(password hash upgrade failure)These were the only console calls in the package with an interpolated first argument and more arguments after it. Clears CodeQL
js/tainted-format-stringalerts 25, 26 and 27.Tests pass on mysql (597), json (556) and toml (556).