fix(auth): invalidate email OTP on a failed guess, not just a match - #2224
fix(auth): invalidate email OTP on a failed guess, not just a match#2224addyCooks wants to merge 1 commit into
Conversation
useVerificationToken looked up the row by the guessed token, so a wrong 6-digit guess simply found no row and left the real code untouched, guessable for the rest of its 10-minute TTL. Look the row up by identifier instead (mirrors the mobile login path) and delete it on every attempt, so a wrong guess burns the code immediately.
| await db | ||
| .delete(verificationTokens) | ||
| .where( | ||
| and( | ||
| eq(verificationTokens.token, token), | ||
| eq(verificationTokens.identifier, row.identifier), | ||
| ), | ||
| ); | ||
| return { ...row, identifier: storedIdentifier }; | ||
| .where(eq(verificationTokens.identifier, row.identifier)); |
There was a problem hiding this comment.
Concurrent attempts reuse tokens
Concurrent verification requests can select the same token before either deletion finishes. Because the deletion result is ignored, each request can then authenticate using its previously selected row. A correct request may succeed even after a racing wrong guess was supposed to burn the token, and concurrent correct requests may both succeed. A resend racing this sequence can also have its newly issued token deleted while the old token authenticates. The lookup, invalidation, and match decision must be atomic, with concurrency covered by the existing real-MySQL integration-test facility.
How this was verified: The callback accepts concurrent requests, while the adapter performs an unlocked SELECT followed by a separate DELETE and returns the previously selected row without checking the deletion result.
Knowledge Base Used: Data and identity platform
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/database/auth/drizzle-adapter.ts
Line: 526-528
Comment:
**Concurrent attempts reuse tokens**
Concurrent verification requests can select the same token before either deletion finishes. Because the deletion result is ignored, each request can then authenticate using its previously selected row. A correct request may succeed even after a racing wrong guess was supposed to burn the token, and concurrent correct requests may both succeed. A resend racing this sequence can also have its newly issued token deleted while the old token authenticates. The lookup, invalidation, and match decision must be atomic, with concurrency covered by the existing real-MySQL integration-test facility.
**How this was verified:** The callback accepts concurrent requests, while the adapter performs an unlocked SELECT followed by a separate DELETE and returns the previously selected row without checking the deletion result.
**Knowledge Base Used:** [Data and identity platform](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/data-and-identity-platform.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| } | ||
| // Delete on every attempt (not just a match) so a wrong guess burns the | ||
| // code instead of leaving it guessable for the rest of its TTL. | ||
| await db |
There was a problem hiding this comment.
P1: OTP consumption is vulnerable to concurrent stale-row authentication
Separate SELECT/DELETE/compare steps can let concurrent requests authenticate from the same stale OTP row.
Atomically consume the identifier's token and authorize only when the conditional operation claims the row; add MySQL race tests.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="packages/database/auth/drizzle-adapter.ts">
<violation number="1" location="packages/database/auth/drizzle-adapter.ts:526">
<priority>P1</priority>
<title>OTP consumption is vulnerable to concurrent stale-row authentication</title>
<evidence>The changed method selects a token by identifier, then performs a separate DELETE and finally compares the previously selected row's token. The deletion result is ignored. Concurrent verification requests can therefore both read the same row before either delete completes and return success based on stale state, allowing multiple uses of a one-time code or allowing a correct attempt to succeed after a racing wrong attempt. A resend racing this sequence can also be affected because deletion is keyed only by identifier.</evidence>
<recommendation>Make lookup, single-use invalidation, and match decision atomic at the database level: use a transaction with appropriate row locking or an atomic conditional DELETE/consume operation whose affected-row result determines whether the request may authenticate. Ensure a replacement token cannot be deleted by an in-flight consume operation, and add a real-MySQL concurrency test covering simultaneous correct and incorrect attempts and resend races.</recommendation>
</violation>
</file>
Summary
Fixes #2221.
useVerificationTokenin the NextAuth Drizzle adapter only invalidated the verification row on a successful code match. A wrong 6-digit guess found no matching row and simply returnednull, leaving the real code untouched and guessable for the rest of its 10-minute TTL a brute-forceable account takeover.Root cause
useVerificationTokenqueriedverification_tokensby the guessed token value. Since the row is keyed byidentifier, a wrong guess never matched, so there was nothing to invalidate on failure. The mobile login path (apps/web/app/api/mobile/[...route]/route.ts) already avoided this by looking the row up byidentifierfirst and deleting it on any outcome — the web path had no equivalent.Fix
In
packages/database/auth/drizzle-adapter.ts:identifier(the key an attacker doesn't control) instead of by the guessedtoken.This mirrors the correct mobile behavior and closes the gap without depending on rate limiting (
AUTH_OTP_VERIFY/AUTH_OTP_SENDremain unwired and, even wired, are Vercel-Firewall-only and fail open on self-hosted deployments so this fix is the durable, hosting-agnostic one, as noted in the issue).Testing
apps/web/__tests__/unit/verification-token.test.ts(no prior test coverage existed for this adapter method), covering:null.apps/webunit suite (pnpm test): 2542 passed. 3 pre-existing failures (Slack manifest brand-color drift, anagent-api-handlerhook timeout, ansso-login-pagesrender timeout) are unrelated verified none of those files reference the changed code.tsc --noEmitonpackages/databaseis clean.Related
Greptile Summary
The PR changes email OTP consumption to locate a token by normalized email identifier, delete it on either a matching or incorrect guess, and return it only on a match. It also adds unit coverage for sequential mismatch, successful consumption, and missing-token behavior.
Confidence Score: 3/5
The PR should not merge until OTP lookup, invalidation, and validation enforce the single-attempt guarantee atomically under concurrent requests.
The sequential fix works, but concurrent requests can select the same token before deletion and then authenticate using stale state, while a resend racing the consume path can also have its replacement token deleted.
Files Needing Attention: packages/database/auth/drizzle-adapter.ts, apps/web/tests/unit/verification-token.test.ts
Security Review
The sequential failed-guess behavior is improved, but concurrent callback requests can read the same token before deletion and authenticate from stale state. This leaves a race in the single-attempt OTP security invariant and can also interfere with a concurrently issued replacement token.
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(auth): invalidate email OTP on a fai..." | Re-trigger Greptile
Context used: