Skip to content

fix(auth): invalidate email OTP on a failed guess, not just a match - #2224

Open
addyCooks wants to merge 1 commit into
CapSoftware:mainfrom
addyCooks:fix/otp-verification-token-brute-force
Open

fix(auth): invalidate email OTP on a failed guess, not just a match#2224
addyCooks wants to merge 1 commit into
CapSoftware:mainfrom
addyCooks:fix/otp-verification-token-brute-force

Conversation

@addyCooks

@addyCooks addyCooks commented Sep 5, 2026

Copy link
Copy Markdown

Summary

Fixes #2221. useVerificationToken in 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 returned null, leaving the real code untouched and guessable for the rest of its 10-minute TTL a brute-forceable account takeover.

Root cause

useVerificationToken queried verification_tokens by the guessed token value. Since the row is keyed by identifier, 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 by identifier first and deleting it on any outcome — the web path had no equivalent.

Fix

In packages/database/auth/drizzle-adapter.ts:

  • Look up the verification row by identifier (the key an attacker doesn't control) instead of by the guessed token.
  • Delete the row on every attempt match or mismatch before returning, so a wrong guess burns the code immediately instead of leaving it valid for reuse.
  • Return the row only when the stored token actually matches the guess.

This mirrors the correct mobile behavior and closes the gap without depending on rate limiting (AUTH_OTP_VERIFY/AUTH_OTP_SEND remain 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

  • Added apps/web/__tests__/unit/verification-token.test.ts (no prior test coverage existed for this adapter method), covering:
    • A wrong guess deletes the stored code; a subsequent correct guess against the same code also fails (proves it's burned, not just rejected).
    • A correct guess consumes and returns the row.
    • No code requested for an identifier returns null.
  • Ran the full apps/web unit suite (pnpm test): 2542 passed. 3 pre-existing failures (Slack manifest brand-color drift, an agent-api-handler hook timeout, an sso-login-pages render timeout) are unrelated verified none of those files reference the changed code.
  • tsc --noEmit on packages/database is 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.

  • Correctly closes the straightforward sequential brute-force window left by failed guesses.
  • Leaves selection, invalidation, and validation as separate database operations, so concurrent attempts do not reliably enforce the intended single-attempt guarantee.

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

Filename Overview
packages/database/auth/drizzle-adapter.ts Changes OTP consumption to burn tokens on mismatches, but the unlocked select-delete-compare sequence remains vulnerable to concurrent stale-row authentication.
apps/web/tests/unit/verification-token.test.ts Adds useful sequential OTP-consumption tests, though its single-row fake cannot cover the adapter's concurrency-sensitive database behavior.
Prompt To Fix All With AI
### Issue 1
packages/database/auth/drizzle-adapter.ts:526-528
**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.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix(auth): invalidate email OTP on a fai..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

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.
Comment on lines 526 to +528
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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security 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.

@superagent-security superagent-security Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superagent found 1 security concern(s).

}
// 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Email OTP login has no attempt limit: useVerificationToken doesn't invalidate the code on a wrong guess (brute-forceable account takeover)

1 participant