fix(basic-auth): use jitter and constant-time string comparison#1283
fix(basic-auth): use jitter and constant-time string comparison#1283
Conversation
📝 WalkthroughWalkthroughReworks Basic auth flow: adds timing-safe string comparison and random jitter, tightens Base64 decoding error handling, renames an internal error helper to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/utils/auth.ts`:
- Line 70: The current destructuring const [username, password] =
authDecoded.split(":") drops any colons in the password; change the parsing to
split on the first colon only (e.g., find the first indexOf(":") on authDecoded
and use substring/slice to produce username and password) and handle the
missing-colon case (treat as invalid auth). Update the code around the
authDecoded parsing (the line assigning username/password) so password preserves
everything after the first colon.
- Around line 86-88: The custom validate branch in src/utils/auth.ts calls
opts.validate(username, password) and throws authFailed(event, opts?.realm)
immediately on false, causing inconsistent timing versus the username/password
failure path; change the failure path for the opts.validate false case to await
randomJitter() before throwing so it matches the jitter behavior used for
username/password checks (referencing opts.validate, authFailed, and
randomJitter).
🧹 Nitpick comments (3)
src/utils/internal/auth.ts (1)
18-24: Minor comment inconsistency.The JSDoc states "0-100ms" but
Math.floor(Math.random() * 100)produces values in range[0, 99].📝 Suggested fix
/** - * Add random delay (0-100ms) to prevent timing-based credential inference. + * Add random delay (0-99ms) to prevent timing-based credential inference. */src/utils/auth.ts (2)
58-58: Inconsistent realm parameter.
authFailed(event)is called without the realm here, while all other call sites passopts?.realm. Consider passing the realm consistently for uniformWWW-Authenticateheaders.📝 Suggested fix
- throw authFailed(event); + throw authFailed(event, opts?.realm);
112-120: Unusedeventparameter.The
eventparameter is declared but not used in the function body. Either remove it or document the intended future use.🧹 Suggested fix
-function authFailed(event: HTTPEvent, realm: string = "") { +function authFailed(_event: HTTPEvent, realm: string = "") {Or remove entirely if not needed:
-function authFailed(event: HTTPEvent, realm: string = "") { +function authFailed(realm: string = "") {
This PR makes basic auth utilities safer by using constant-time string comparation to prevent guessing password char by char (v8 fast-fails on first char mismatch)
Also fail error is thrown by a random jitter to make it harder.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.