Repository navigation
fix(auth): invalidate email OTP on a failed guess, not just a match - #2224
richiemcilroy merged 7 commits 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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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 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[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| } | ||
| // 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>
useVerificationToken read the row, then deleted it, then compared the token from the stale in-memory copy without checking whether the delete actually removed anything. Concurrent requests could all read the same row before any of them deleted it, letting more than one succeed, and a wrong guess's unconditional delete-by-identifier could also wipe out a code from a resend that landed in between. Scope the delete to the exact (identifier, token) pair just read and only treat the guess as claimed when that delete affects exactly one row, so at most one concurrent request can consume a given code and a racing resend's row is never clobbered.
Adds a case to the existing isolated-MySQL integration facility (gated on CAP_SSO_TEST_DATABASE_URL, same as the SSO tests in this file) firing 8 concurrent useVerificationToken calls for one code and asserting exactly one claims it. Requested in review as the concurrency coverage for the atomic-consumption fix, since the prior unit test only exercised it against an in-memory fake.
…MySQL Extends the isolated-MySQL facility with a case firing one correct guess alongside several wrong ones concurrently, asserting at most one attempt ever authenticates and, when one does, it's the correct guess - never a wrong one that raced ahead of it. Addresses the "simultaneous correct and incorrect attempts" scenario named in a follow-up security scan of the atomic-consumption fix.
|
Thanks, this is the cleanest fix for #2221 and we're consolidating the duplicates here. Your later commits address the concurrency findings, but Greptile last scored an older commit (3/5). Please re-trigger Greptile on the current head and resolve the outdated threads. One thing to consider: burning the code on any wrong guess lets someone who knows a user's email invalidate their live code, so a small per-token attempt limit may be a better trade-off. We need Greptile 5/5 and green CI before we review. |
|
Thanks @addyCooks! Greptile is at 3/5 here. We only merge once Greptile reaches 5/5, so please address its findings in the summary above (or reply to them if you think one is wrong), then comment |
|
@greptileai please review the latest commits (atomic OTP consumption + main merged in). |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
…reaches the race callbacks
|
@greptileai review |
| const successes = attempts.filter((result) => result !== null); | ||
| expect(successes.length).toBeLessThanOrEqual(1); | ||
| for (const success of successes) { | ||
| expect(success).toMatchObject({ identifier, token }); | ||
| } |
There was a problem hiding this comment.
The mixed-guess test does not check which guess succeeded. useVerificationToken returns the stored row, so checking its token against the stored token would also pass if a wrong guess incorrectly returned that row. This leaves a gap in the test, not a defect in the current sign-in code. Keep each result paired with its guess and assert that every wrong guess returns null.
| const successes = attempts.filter((result) => result !== null); | |
| expect(successes.length).toBeLessThanOrEqual(1); | |
| for (const success of successes) { | |
| expect(success).toMatchObject({ identifier, token }); | |
| } | |
| const successes = attempts.filter((result) => result !== null); | |
| expect(successes.length).toBeLessThanOrEqual(1); | |
| for (const [index, result] of attempts.entries()) { | |
| if (guesses[index] !== token) { | |
| expect(result).toBeNull(); | |
| } else if (result !== null) { | |
| expect(result).toMatchObject({ identifier, token }); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/__tests__/integration/sso-database.test.ts
Line: 393-397
Comment:
**Test loses the winning guess**
The mixed-guess test does not check which guess succeeded. `useVerificationToken` returns the stored row, so checking its `token` against the stored token would also pass if a wrong guess incorrectly returned that row. This leaves a gap in the test, not a defect in the current sign-in code. Keep each result paired with its guess and assert that every wrong guess returns `null`.
```suggestion
const successes = attempts.filter((result) => result !== null);
expect(successes.length).toBeLessThanOrEqual(1);
for (const [index, result] of attempts.entries()) {
if (guesses[index] !== token) {
expect(result).toBeNull();
} else if (result !== null) {
expect(result).toMatchObject({ identifier, token });
}
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch was previously deployed
2 participants
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
The PR appears safe to merge, with one non-blocking improvement to the mixed-guess test.Findings
Fix with agent prompt
Summary
Email verification now looks up the code by email address and consumes it before checking the guess.
Reviews (2) · Last reviewed commit: "test(auth): bind the adapter's useVerifi..." · Reviewed by Greptile