fix(auth): harden basic-auth realm handling and credential timing#1468
Conversation
- Carry the configured realm on the missing-header challenge and apply the documented "auth" default consistently (challenge + context). - Skip the empty user-id/password rejection when a custom validate function is provided (RFC 7617 permits empty credentials). - Evaluate username and password comparisons unconditionally so failure timing does not distinguish an unknown user from a wrong password. - Accept 1*SP between the "Basic" scheme and token (RFC 9110). - Sanitize the realm to the RFC 9110 §5.6.4 quoted-string charset instead of JSON.stringify, which threw on non-Latin-1 realms. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBasic authentication now normalizes realms, changes custom-validator handling of empty credentials, applies jitter on failures, and safely formats authentication challenges. Tests cover configured realms, flexible credential spacing, and non-Latin-1 realm values. ChangesBasic authentication behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
pi0x
left a comment
pi0x
left a comment
There was a problem hiding this comment.
Verdict: APPROVE (independent review — I did not author this change).
Reviewed the diff, checked out the branch, and ran the suite: pnpm vitest run test/auth.test.ts → 42 passed; pnpm lint (oxlint + oxfmt) clean. I also reverted only the source (keeping the new tests) and confirmed all three regression tests fail against main, so they are genuine regressions, not tautologies.
What I verified:
-
Constant-shape timing.
usernameOk/passwordOk/validateOkare each computed on their own line before theif, so bothtimingSafeEqualcalls always execute — the only short-circuit (!opts.username || …) depends on static config, not request data, so the timing shape is invariant across requests for a given config. The finalif (!usernameOk || !passwordOk || !validateOk)short-circuits only over already-computed booleans. This correctly removes the old||-chain side-channel (unknown-user skipping the password compare).randomJitter()still fires on failure. Correct. -
Realm sanitizer.
quoteRealmmatches RFC 9110 §5.6.4: strips everything outsideHTAB / SP / %x21-7E / obs-text(%x80-FF)(so control chars, DEL 0x7F, and >0xFF code points go), then escapes DQUOTE and backslash as quoted-pairs. This is the correct qdtext + quoted-pair charset and avoids theJSON.stringifynon-Latin-1 header throw. Correct. -
Empty-credential change does not weaken the default path.
if (!opts.validate && (!username || !password))still rejects empty user-id/password whenever no customvalidateis supplied (including the commonbasicAuth({ password })case). Empty creds only reach a user-providedvalidate, per RFC 7617. Correct. -
Default realm
"auth"is now consistent across the missing-header challenge, all other challenges,context.basicAuth.realm, and the JSDoc (Defaults to "auth"). No existing test regressed.
Minor (non-blocking) notes:
-
The scheme regex
/^basic\s+(.+)$/iis commented as "one or more spaces" but\salso matches TAB/CR/LF/FF/VT — broader than RFC 91101*SP. Harmless in practice (the header is already line-parsed by the HTTP layer and token68 is base64, so atob rejects anything odd), but the comment slightly overstates strictness. Consider[ ]+or[\t ]+if you want to match the comment, or reword the comment. -
Expected semantic overlap with #1463 (UTF-8 decode): both edit the
authFailedwww-authenticateline and the decode block. A merge conflict is expected there. Whichever lands second should reconcile so the final code keeps all three: thequoteRealmsanitizer, theTextDecoder("utf-8")decode, andcharset="UTF-8"in the challenge. Flagging only — not counted against this PR.
Solid, well-tested hardening. Approving.
🤖 Generated with Claude Code
RFC 9110 defines the separator as 1*SP; \s also matched TAB/CR/LF/FF/VT. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Addressed the review nit in 6d3be92: tightened the scheme separator regex to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/auth.ts`:
- Around line 81-89: Update the usernameOk and passwordOk checks in the
authentication validation flow to skip timingSafeEqual only when opts.username
or opts.password is undefined, not when they are empty strings. Preserve the
existing unconditional comparison behavior and failure handling so explicitly
configured empty strings require an exact match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6daebc5-d2de-43f4-9585-389084b1cdb2
📒 Files selected for processing (2)
src/utils/auth.tstest/auth.test.ts
| // Evaluate all comparisons unconditionally so failure timing does not | ||
| // distinguish an unknown user from a wrong password. | ||
| const usernameOk = !opts.username || timingSafeEqual(username, opts.username); | ||
| const passwordOk = !opts.password || timingSafeEqual(password, opts.password); | ||
| const validateOk = !opts.validate || (await opts.validate(username, password)); | ||
| if (!usernameOk || !passwordOk || !validateOk) { | ||
| await randomJitter(); | ||
| throw authFailed(event, opts?.realm); | ||
| throw authFailed(event, realm); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Prevent authentication bypass from empty string configurations.
The truthiness checks (!opts.username, !opts.password) will evaluate to true if a developer explicitly configures them as empty strings (""). This causes the validation to silently skip the timingSafeEqual check, effectively acting as a wildcard that accepts any username or password from the client.
Use explicit undefined checks to ensure empty string configurations securely enforce an exact match.
🔒️ Proposed fix to securely handle empty string options
- const usernameOk = !opts.username || timingSafeEqual(username, opts.username);
- const passwordOk = !opts.password || timingSafeEqual(password, opts.password);
- const validateOk = !opts.validate || (await opts.validate(username, password));
+ const usernameOk = opts.username === undefined || timingSafeEqual(username, opts.username);
+ const passwordOk = opts.password === undefined || timingSafeEqual(password, opts.password);
+ const validateOk = opts.validate === undefined || (await opts.validate(username, password));
if (!usernameOk || !passwordOk || !validateOk) {
await randomJitter();
throw authFailed(event, realm);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Evaluate all comparisons unconditionally so failure timing does not | |
| // distinguish an unknown user from a wrong password. | |
| const usernameOk = !opts.username || timingSafeEqual(username, opts.username); | |
| const passwordOk = !opts.password || timingSafeEqual(password, opts.password); | |
| const validateOk = !opts.validate || (await opts.validate(username, password)); | |
| if (!usernameOk || !passwordOk || !validateOk) { | |
| await randomJitter(); | |
| throw authFailed(event, opts?.realm); | |
| throw authFailed(event, realm); | |
| } | |
| // Evaluate all comparisons unconditionally so failure timing does not | |
| // distinguish an unknown user from a wrong password. | |
| const usernameOk = opts.username === undefined || timingSafeEqual(username, opts.username); | |
| const passwordOk = opts.password === undefined || timingSafeEqual(password, opts.password); | |
| const validateOk = opts.validate === undefined || (await opts.validate(username, password)); | |
| if (!usernameOk || !passwordOk || !validateOk) { | |
| await randomJitter(); | |
| throw authFailed(event, realm); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/auth.ts` around lines 81 - 89, Update the usernameOk and passwordOk
checks in the authentication validation flow to skip timingSafeEqual only when
opts.username or opts.password is undefined, not when they are empty strings.
Preserve the existing unconditional comparison behavior and failure handling so
explicitly configured empty strings require an exact match.
…e contract - Apply randomJitter() inside authFailed so every 401 path (missing header, bad scheme, decode failure, missing colon, empty creds, mismatch) is delayed uniformly, closing the parse-vs-mismatch timing asymmetry - Document that a custom `validate` receives decoded credentials as-is, including empty strings (RFC 7617), and must reject them explicitly - Drop the unreachable `= "auth"` default on authFailed's realm param; the realm is computed once at the call site Co-Authored-By: Claude Opus 4.8 <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/auth.ts (1)
88-91: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winPrevent authentication bypass from empty string configurations.
The truthiness checks (
!opts.username,!opts.password) will evaluate totrueif a developer explicitly configures them as empty strings (""). This causes the validation to silently skip thetimingSafeEqualcheck, effectively acting as a wildcard that accepts any username or password from the client.Use explicit
undefinedchecks to ensure empty string configurations securely enforce an exact match.🔒️ Proposed fix to securely handle empty string options
- const usernameOk = !opts.username || timingSafeEqual(username, opts.username); - const passwordOk = !opts.password || timingSafeEqual(password, opts.password); - const validateOk = !opts.validate || (await opts.validate(username, password)); + const usernameOk = opts.username === undefined || timingSafeEqual(username, opts.username); + const passwordOk = opts.password === undefined || timingSafeEqual(password, opts.password); + const validateOk = opts.validate === undefined || (await opts.validate(username, password));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/auth.ts` around lines 88 - 91, Update the usernameOk and passwordOk checks in the authentication validation flow to bypass timingSafeEqual only when opts.username or opts.password is undefined, not when configured as an empty string. Preserve exact matching for all defined values, including empty strings, while leaving validateOk unchanged.
🧹 Nitpick comments (1)
src/utils/auth.ts (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant optional chaining.
Since
optsis unconditionally accessed earlier on line 49 (opts.validate), it is guaranteed to be defined here. The optional chaining operator?.is redundant and can be safely removed.♻️ Proposed refactor
- const realm = opts?.realm ?? "auth"; + const realm = opts.realm ?? "auth";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/auth.ts` at line 56, Remove the redundant optional chaining from the realm assignment in the auth logic, changing the access to use the already-validated opts object directly. Preserve the existing "auth" fallback via nullish coalescing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/utils/auth.ts`:
- Around line 88-91: Update the usernameOk and passwordOk checks in the
authentication validation flow to bypass timingSafeEqual only when opts.username
or opts.password is undefined, not when configured as an empty string. Preserve
exact matching for all defined values, including empty strings, while leaving
validateOk unchanged.
---
Nitpick comments:
In `@src/utils/auth.ts`:
- Line 56: Remove the redundant optional chaining from the realm assignment in
the auth logic, changing the access to use the already-validated opts object
directly. Preserve the existing "auth" fallback via nullish coalescing.
# Conflicts: # src/utils/auth.ts # test/auth.test.ts
2 participants
Hardens
requireBasicAuth/basicAuthfor realm consistency and timing defense-in-depth. The first challenge (missingAuthorizationheader) now carries the configured realm, and the documented"auth"default is applied consistently to both the challenge andevent.context.basicAuth.realm. Username and password comparisons are now evaluated unconditionally so failure timing no longer distinguishes an unknown user from a wrong password, and empty user-id/password are permitted when a customvalidatefunction is supplied (RFC 7617).Parsing nits: accept
1*SPbetween theBasicscheme and token (RFC 9110), and sanitize the realm to the RFC 9110 §5.6.4 quoted-string charset instead ofJSON.stringify, which previously threw on non-Latin-1 realms. Added regression tests for all three observable behaviors; full suite and lint pass.🤖 Generated with Claude Code
Summary by CodeRabbit
WWW-Authenticateheader.Authorizationheaders that include extra spaces between the scheme and credentials.