Skip to content

fix(auth): allow identical login codes for different emails - #3559

Open
yashranaway wants to merge 1 commit into
CapSoftware:mainfrom
yashranaway:fix/email-otp-collisions
Open

yashranaway wants to merge 1 commit into
CapSoftware:mainfrom
yashranaway:fix/email-otp-collisions

Conversation

@yashranaway

@yashranaway yashranaway commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

email login codes are six digits and get hashed with the auth secret, not the email, so two people who happen to be sent the same code get the same hash. the token column had a unique index, so the second person's code failed to save and their sign in email broke. with lots of pending codes that collision isn't rare.

the uniqueness wasn't doing anything: every lookup goes by email, which is already the primary key, and consuming a code deletes the exact email plus token row. so i dropped the index and made the adapter re-read the inserted row by email instead of by token.

needs migration 0052, which drops the unique index on verification_tokens.token.

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge.

Summary

Allows different email addresses to store identical login-code hashes.

  • Different email addresses can use the same sign-in code hash.

No actionable issues found. Review was based on code inspection; tests and migrations were not executed.

Reviews (1) · Last reviewed commit: "fix(auth): allow identical login codes f..." · Reviewed by Greptile

@vercel

vercel Bot commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
cap-status Skipped Skipped Oct 29, 2026 8:50pm UTC

@vercel
vercel Bot temporarily deployed to Preview – cap-status October 29, 2026 20:50 Inactive
@yashranaway

Copy link
Copy Markdown
Contributor Author

@greptileai review

@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).

export const verificationTokens = mysqlTable("verification_tokens", {
identifier: varchar("identifier", { length: 255 }).primaryKey().notNull(),
token: varchar("token", { length: 255 }).unique().notNull(),
token: varchar("token", { length: 255 }).notNull(),

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: Removing the token uniqueness constraint enables cross-account OTP login when two emails receive the same code

Removing the token unique index lets two emails share a code hash; the hash is not email-bound, so a code can authenticate either account.

Keep the unique index and bind the stored hash to the identifier (e.g. HMAC(secret, identifier + code)); then identical codes cannot validate another account.

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/schema.ts">
<violation number="1" location="packages/database/schema.ts:210">
<priority>P1</priority>
<title>Removing the token uniqueness constraint enables cross-account OTP login when two emails receive the same code</title>
<evidence>The schema change removes `.unique()` from `verificationTokens.token` (schema.ts:210), and migration 0052 line 1 drops `verification_tokens_token_unique`. This allows two different identifiers to store the same token hash. NextAuth 4.24.14 computes the stored value as SHA-256 of the six-digit code plus the auth secret, with no email/identifier input, and the new integration test itself stores the same hash for two identifiers (sso-database.test.ts:363-366). The adapter's `useVerificationToken` loads the row by identifier but compares only `row.token !== token` (drizzle-adapter.ts:467-501); it never binds the presented code to the identifier. Consequently, an attacker who has a valid code for their own email can submit that code with a victim's email. If the victim has a pending code with the same six-digit value, the hashes match, the victim's row is consumed, and NextAuth issues a session for the victim. The adapter change at line 461 is part of the same flow but does not add identifier binding.</evidence>
<recommendation>Do not rely on dropping the unique constraint. Keep the unique constraint and make the stored verification value identifier-bound, for example by storing `SHA256(identifier + ':' + token)` (or an equivalent HMAC) in the adapter and applying the same derivation during verification. Add a regression test where two identifiers have the same plaintext code and assert that a code issued for one identifier cannot authenticate the other.</recommendation>
</violation>
</file>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This doesn't allow cross-account login: every lookup reads the row by email (the primary key) and compares that row's hash, and using a code deletes that exact email+token row, so a code only works for the email it was sent to. Two emails sharing a hash just means both were sent the same six digits; knowing your own code doesn't make guessing someone else's easier.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The contributor correctly notes the lookup is scoped by email, but this doesn't eliminate the cross-account risk. With the unique constraint removed, two pending codes can share the same hash. If a victim happens to have a pending code with the same six-digit value, an attacker can submit that colliding code with the victim's email and match the victim's row, because the stored hash isn't bound to the identifier. Please either keep the unique constraint or bind the hash to the identifier (e.g., HMAC(secret, identifier + code)) so a code cannot validate another account even when codes collide.

This branch was previously deployed

1 inactive deployment
Preview – cap-status — a6f9ed1e Deployed Oct 29, 2026 by vercel[bot]
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.

1 participant