Conversation
|
Cursor Agent can help with this pull request. Just |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a randomized password submission delay utility and invokes it at the start of password-related action handlers (login, signup, reset-password, account password) to introduce a short, configurable jitter (default up to 250ms) before further processing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Route as "Route Handler\n(login/signup/reset/me_.password)"
participant Delay as "Password Delay\nUtility"
participant Auth as "Auth / DB"
Client->>Route: submit password request
Route->>Delay: await applyPasswordSubmissionDelay()
Delay-->>Route: delay elapsed
Route->>Auth: perform verify / hash / update
Auth-->>Route: result (success / error)
Route-->>Client: redirect or response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/me_.password.tsx (1)
55-79:⚠️ Potential issue | 🟡 MinorTiming asymmetry between "missing" and "wrong" current password paths.
The delay at line 57 fires before any bcrypt work (since
currentPasswordis absent,verifyPasswordis never called), giving a response time of roughly 0–250 ms. The delay at line 71 fires afterverifyPassword(~100 ms bcrypt), giving ~100–350 ms. An automated prober can distinguish these two branches by timing alone, even without reading the response body.Practical risk is low because the endpoint is authenticated and the two errors already carry distinct messages, but it does undermine the PR's goal of consistent response timing across password paths.
Consider consolidating to a single
applyPasswordSubmissionDelay()call that covers both branches uniformly, or accepting the asymmetry given the authenticated context.💡 One consolidation option
if (existingPassword) { if (typeof currentPassword !== 'string' || !currentPassword) { - await applyPasswordSubmissionDelay() return json<ActionData>( { status: 'error', errors: { currentPassword: 'Current password is required' }, }, 400, ) } - const ok = await verifyPassword({ - password: currentPassword, - hash: existingPassword.hash, - }) + const ok = await verifyPassword({ password: currentPassword, hash: existingPassword.hash }) if (!ok) { - await applyPasswordSubmissionDelay() return json<ActionData>( { status: 'error', errors: { currentPassword: 'Current password is incorrect' }, }, 400, ) } }Then add a single
await applyPasswordSubmissionDelay()immediately before the outerif (passwordError || confirmPasswordError)block and before the success path, so all returns share the same unconditional delay budget regardless of whether bcrypt ran.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/me_.password.tsx` around lines 55 - 79, The code has a timing side-channel between the "missing currentPassword" branch (applyPasswordSubmissionDelay() before bcrypt) and the "wrong currentPassword" branch (delay after verifyPassword()), allowing probes to distinguish them; move the single await applyPasswordSubmissionDelay() out of the inner branches and call it unconditionally for all password-check flows (e.g., call it once before the outer password validation/return logic that includes verifyPassword and the error returns) so verifyPassword(), the currentPassword checks, and both error returns share the same delay budget; update uses of verifyPassword, currentPassword, existingPassword.hash, and the error return sites to rely on that single centralized delay.
🧹 Nitpick comments (3)
app/utils/__tests__/password-submission-delay.test.ts (2)
40-42:vi.restoreAllMocks()is a no-op here — remove it.
vi.restoreAllMocks()only resets mocks created viavi.spyOn(). This test usesvi.fn()directly, so there are no spies to restore. The call adds noise and may mislead future maintainers into thinking spy cleanup is needed.🧹 Proposed cleanup
} finally { vi.useRealTimers() - vi.restoreAllMocks() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/password-submission-delay.test.ts` around lines 40 - 42, The test's finally block calls vi.restoreAllMocks() even though the test uses vi.fn() and no spies are created; remove the vi.restoreAllMocks() call from the finally block and keep vi.useRealTimers() only. Locate the finally block in password-submission-delay.test (the section that currently contains "vi.useRealTimers()" and "vi.restoreAllMocks()") and delete the vi.restoreAllMocks() line to avoid the no-op and reduce noise.
23-44: Add a test for thedelayMs <= 0early-return path inapplyPasswordSubmissionDelay.There's no coverage for the branch inside
applyPasswordSubmissionDelaythat returns immediately whendelayMs <= 0(e.g. whenmaxMs: 0is passed). Adding it would close the gap and confirm the function resolves without touchingsetTimeout.✅ Suggested additional test
+test('applyPasswordSubmissionDelay resolves immediately when maxMs is 0', async () => { + vi.useFakeTimers() + try { + const randomInt = vi.fn() + await applyPasswordSubmissionDelay({ maxMs: 0, randomInt }) + expect(randomInt).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/password-submission-delay.test.ts` around lines 23 - 44, Add a new unit test in password-submission-delay.test.ts that exercises the early-return path of applyPasswordSubmissionDelay by calling applyPasswordSubmissionDelay({ maxMs: 0, randomInt }) with a randomInt that returns 0 (or any non-positive delay), using vi.useFakeTimers(); assert the promise resolves immediately (no need to advance timers) and verify setTimeout was not invoked (e.g., vi.spyOn(globalThis, 'setTimeout') or vi.fn check), then restore timers/mocks in finally; reference the applyPasswordSubmissionDelay function and the existing test file to place the new test alongside the existing "waits for the sampled delay" case.app/utils/password.server.ts (1)
13-13: Rate limiting is a prerequisite for this jitter to be effective.A 0–250ms random delay adds meaningful statistical noise on top of bcrypt's ~100ms base cost, but it doesn't prevent a high-volume attacker from averaging out the jitter across many requests. Without a complementary rate-limiting or account-lockout mechanism on these endpoints (
/login,/signup,/reset-password,/me/password), a determined adversary can still use repeated probing to detect timing differences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/password.server.ts` at line 13, The random delay constant PASSWORD_SUBMISSION_DELAY_MAX_MS alone is insufficient; add rate-limiting/account-lockout to the authentication endpoints (/login, /signup, /reset-password, /me/password) so the jitter can't be averaged out by high-volume probing. Implement or attach a request-rate limiter (or per-account exponential backoff/temporary lockout) to the handlers that call this file (the login, signup, resetPassword and updatePassword request handlers) and ensure the limiter runs server-side before/alongside applying PASSWORD_SUBMISSION_DELAY_MAX_MS; log and test that repeated failed attempts trigger the limiter/lockout and return consistent timing and error responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/routes/me_.password.tsx`:
- Around line 55-79: The code has a timing side-channel between the "missing
currentPassword" branch (applyPasswordSubmissionDelay() before bcrypt) and the
"wrong currentPassword" branch (delay after verifyPassword()), allowing probes
to distinguish them; move the single await applyPasswordSubmissionDelay() out of
the inner branches and call it unconditionally for all password-check flows
(e.g., call it once before the outer password validation/return logic that
includes verifyPassword and the error returns) so verifyPassword(), the
currentPassword checks, and both error returns share the same delay budget;
update uses of verifyPassword, currentPassword, existingPassword.hash, and the
error return sites to rely on that single centralized delay.
---
Nitpick comments:
In `@app/utils/__tests__/password-submission-delay.test.ts`:
- Around line 40-42: The test's finally block calls vi.restoreAllMocks() even
though the test uses vi.fn() and no spies are created; remove the
vi.restoreAllMocks() call from the finally block and keep vi.useRealTimers()
only. Locate the finally block in password-submission-delay.test (the section
that currently contains "vi.useRealTimers()" and "vi.restoreAllMocks()") and
delete the vi.restoreAllMocks() line to avoid the no-op and reduce noise.
- Around line 23-44: Add a new unit test in password-submission-delay.test.ts
that exercises the early-return path of applyPasswordSubmissionDelay by calling
applyPasswordSubmissionDelay({ maxMs: 0, randomInt }) with a randomInt that
returns 0 (or any non-positive delay), using vi.useFakeTimers(); assert the
promise resolves immediately (no need to advance timers) and verify setTimeout
was not invoked (e.g., vi.spyOn(globalThis, 'setTimeout') or vi.fn check), then
restore timers/mocks in finally; reference the applyPasswordSubmissionDelay
function and the existing test file to place the new test alongside the existing
"waits for the sampled delay" case.
In `@app/utils/password.server.ts`:
- Line 13: The random delay constant PASSWORD_SUBMISSION_DELAY_MAX_MS alone is
insufficient; add rate-limiting/account-lockout to the authentication endpoints
(/login, /signup, /reset-password, /me/password) so the jitter can't be averaged
out by high-volume probing. Implement or attach a request-rate limiter (or
per-account exponential backoff/temporary lockout) to the handlers that call
this file (the login, signup, resetPassword and updatePassword request handlers)
and ensure the limiter runs server-side before/alongside applying
PASSWORD_SUBMISSION_DELAY_MAX_MS; log and test that repeated failed attempts
trigger the limiter/lockout and return consistent timing and error responses.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/routes/login.tsxapp/routes/me_.password.tsxapp/routes/reset-password.tsxapp/routes/signup.tsxapp/utils/__tests__/password-submission-delay.test.tsapp/utils/password.server.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/signup.tsx`:
- Line 83: The call to applyPasswordSubmissionDelay() currently runs before
actionId is checked and thus adds 0–250ms latency to all branches (cancel,
requestCode, verifyCode) unnecessarily; move the applyPasswordSubmissionDelay()
call so it runs only in the password-handling path that leads to signUp and
calls getPasswordHash. Concretely, remove or relocate the early
applyPasswordSubmissionDelay() invocation and insert it just before the code
that handles the implicit signUp branch (the block that calls getPasswordHash),
leaving the cancel, requestCode, and verifyCode branches untouched.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/routes/login.tsxapp/routes/me_.password.tsxapp/routes/reset-password.tsxapp/routes/signup.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/routes/reset-password.tsx
- app/routes/me_.password.tsx
| } | ||
|
|
||
| export async function action({ request }: Route.ActionArgs) { | ||
| await applyPasswordSubmissionDelay() |
There was a problem hiding this comment.
Delay fires on all action paths, not just the password-submission path.
applyPasswordSubmissionDelay() is called before actionId is read, so the cancel, requestCode, and verifyCode branches all incur a gratuitous 0–250 ms delay. Those paths don't touch a password, so there is no security benefit — only unnecessary UX latency. Only the implicit signUp branch (line 209+) that calls getPasswordHash needs this guard.
🛡️ Proposed fix — move the delay after the early-return branches
export async function action({ request }: Route.ActionArgs) {
- await applyPasswordSubmissionDelay()
const loginInfoSession = await getLoginInfoSession(request)
const requestText = await request.text()
const form = new URLSearchParams(requestText)
const actionId = form.get('actionId')
if (actionId === actionIds.cancel) {
// ... (early return)
}
if (actionId === actionIds.requestCode) {
// ... (early return)
}
if (actionId === actionIds.verifyCode) {
// ... (early return)
}
+ // Only the signUp path reaches here — guard it with the timing-attack delay.
+ await applyPasswordSubmissionDelay()
const signupEmail = loginInfoSession.getSignupEmail()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/signup.tsx` at line 83, The call to applyPasswordSubmissionDelay()
currently runs before actionId is checked and thus adds 0–250ms latency to all
branches (cancel, requestCode, verifyCode) unnecessarily; move the
applyPasswordSubmissionDelay() call so it runs only in the password-handling
path that leads to signUp and calls getPasswordHash. Concretely, remove or
relocate the early applyPasswordSubmissionDelay() invocation and insert it just
before the code that handles the implicit signUp branch (the block that calls
getPasswordHash), leaving the cancel, requestCode, and verifyCode branches
untouched.
cursor
bot
force-pushed
the
cursor/password-timing-attack-delays-306b
branch
from
a65a634 to
5408ca5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/utils/__tests__/password-submission-delay.test.ts (1)
7-44: Well-structured tests — LGTM.The three tests collectively cover the full documented contract: clamping non-positive
maxMs, the inclusive-upper-bound compensation, and the actual timer-wait behavior. Thetry/finallycleanup in the async test correctly restores real timers even on assertion failures. One optional improvement: sincenormalizeMaxDelayMsexplicitly branches on!Number.isFinite, a quick inline test forNaNandInfinityinputs togetPasswordSubmissionDelayMswould lock in that behavior without needing to export the helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/password-submission-delay.test.ts` around lines 7 - 44, Add a small test to assert getPasswordSubmissionDelayMs handles non-finite maxMs by exercising the normalizeMaxDelayMs branch: call getPasswordSubmissionDelayMs with maxMs: NaN and maxMs: Infinity (and a mocked randomInt) and assert it clamps/returns 0 (or the expected behavior per normalizeMaxDelayMs); reference getPasswordSubmissionDelayMs and normalizeMaxDelayMs so the test file __tests__/password-submission-delay.test.ts adds two expectations that verify the function’s behavior for NaN and Infinity inputs.app/utils/password.server.ts (1)
17-20:normalizeMaxDelayMshas no upper-bound guard forcrypto.randomInt's range limit.
crypto.randomIntrequires that(max - min) < 2^48and both arguments are safe integers.normalizeMaxDelayMscorrectly floors and clamps to[0, +∞), butgetPasswordSubmissionDelayMspassesrandomInt(0, safeMaxMs + 1), so a caller supplyingmaxMs >= 2^48 - 1(≈281 trillion ms) would trigger aRangeErrorfromcrypto.randomIntrather than gracefully clamping.The default 250 ms value is nowhere near that limit, but a small defensive cap on
normalizeMaxDelayMsmakes the contract self-enforcing and avoids a confusing crash path if the constant is ever bumped:🛡️ Proposed defensive fix
+// crypto.randomInt requires (max - min) < 2**48 +const RANDOM_INT_MAX_RANGE = 2 ** 48 - 1 function normalizeMaxDelayMs(maxMs: number) { if (!Number.isFinite(maxMs)) return 0 - return Math.max(0, Math.floor(maxMs)) + return Math.min(RANDOM_INT_MAX_RANGE, Math.max(0, Math.floor(maxMs))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/password.server.ts` around lines 17 - 20, normalizeMaxDelayMs currently clamps negatives but allows values >= crypto.randomInt's range limit, which can cause RangeError in getPasswordSubmissionDelayMs (which calls randomInt(0, safeMaxMs + 1)). Fix normalizeMaxDelayMs to also cap the result to crypto.randomInt's max allowable upper bound by flooring the input, clamping to >= 0, and limiting to (2**48 - 1) (the maximum safe max for randomInt with min 0); update the function (normalizeMaxDelayMs) to return Math.max(0, Math.min(Math.floor(maxMs), MAX_RANDOM_INT_MAX)) where MAX_RANDOM_INT_MAX = 2**48 - 1 so callers like getPasswordSubmissionDelayMs never pass an out-of-range value to crypto.randomInt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/utils/__tests__/password-submission-delay.test.ts`:
- Around line 7-44: Add a small test to assert getPasswordSubmissionDelayMs
handles non-finite maxMs by exercising the normalizeMaxDelayMs branch: call
getPasswordSubmissionDelayMs with maxMs: NaN and maxMs: Infinity (and a mocked
randomInt) and assert it clamps/returns 0 (or the expected behavior per
normalizeMaxDelayMs); reference getPasswordSubmissionDelayMs and
normalizeMaxDelayMs so the test file __tests__/password-submission-delay.test.ts
adds two expectations that verify the function’s behavior for NaN and Infinity
inputs.
In `@app/utils/password.server.ts`:
- Around line 17-20: normalizeMaxDelayMs currently clamps negatives but allows
values >= crypto.randomInt's range limit, which can cause RangeError in
getPasswordSubmissionDelayMs (which calls randomInt(0, safeMaxMs + 1)). Fix
normalizeMaxDelayMs to also cap the result to crypto.randomInt's max allowable
upper bound by flooring the input, clamping to >= 0, and limiting to (2**48 - 1)
(the maximum safe max for randomInt with min 0); update the function
(normalizeMaxDelayMs) to return Math.max(0, Math.min(Math.floor(maxMs),
MAX_RANDOM_INT_MAX)) where MAX_RANDOM_INT_MAX = 2**48 - 1 so callers like
getPasswordSubmissionDelayMs never pass an out-of-range value to
crypto.randomInt.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/routes/login.tsxapp/routes/me_.password.tsxapp/routes/reset-password.tsxapp/routes/signup.tsxapp/utils/__tests__/password-submission-delay.test.tsapp/utils/password.server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/routes/reset-password.tsx
- app/routes/signup.tsx
cursor
bot
force-pushed
the
cursor/password-timing-attack-delays-306b
branch
3 times, most recently
from
f7e1104 to
88820c8
Compare
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
bot
force-pushed
the
cursor/password-timing-attack-delays-306b
branch
from
db3166f to
889a44f
Compare
|
This doesn't actually do anything very useful. We're not going to do this. We're already protected against the things that a timing attack would catch. |
| // `crypto.randomInt` requires (max - min) < 2**48 and both args are safe ints. | ||
| // We call `randomInt(0, safeMaxMs + 1)` (exclusive upper bound), so cap `safeMaxMs` | ||
| // to keep the exclusive max within range. | ||
| const MAX_PASSWORD_SUBMISSION_DELAY_MS = 2 ** 48 - 2 |
There was a problem hiding this comment.
Near-identical constant names risk confusion and future bugs
Low Severity
PASSWORD_SUBMISSION_DELAY_MAX_MS (250, the default delay cap) and MAX_PASSWORD_SUBMISSION_DELAY_MS (2^48−2, the crypto.randomInt safety bound) differ only in word order of "MAX." This makes them extremely easy to confuse, increasing the risk of a future maintainer using the wrong one.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
2 participants
Introduce random delays (0-250ms) on all password submission paths to prevent timing attacks.
Note
Medium Risk
Touches authentication/password flows and adds intentional latency to those requests, which could impact UX and request timing behavior if misconfigured.
Overview
Adds a small randomized delay (0–250ms) to all password submission actions (
login,signup,reset-password, andme_.password) to make timing attacks noisier.Introduces
getPasswordSubmissionDelayMs/applyPasswordSubmissionDelayinpassword.server.tswith unit tests, tweaks MSW Gravatar mocking to avoid external requests outside development, and hardens the Playwright CI job by falling back toapt-getffmpeg if the setup action flakes.Written by Cursor Bugbot for commit 889a44f. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Tests