Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
addyCooks wants to merge 1 commit into
base: main
Choose a base branch
from
+109 −14
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions apps/web/__tests__/unit/verification-token.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import { verificationTokens } from "@cap/database/schema";
import type { SQL } from "drizzle-orm";
import { MySqlDialect } from "drizzle-orm/mysql-core";
import type { MySql2Database } from "drizzle-orm/mysql2";
import { describe, expect, it } from "vitest";
import { DrizzleAdapter } from "../../../../packages/database/auth/drizzle-adapter";

type TokenRow = { identifier: string; token: string; expires: Date };

function parsePredicate(condition: SQL) {
const query = new MySqlDialect().sqlToQuery(condition);
const match = /^`verification_tokens`\.`(identifier|token)` = \?$/.exec(
query.sql,
);
if (!match?.[1]) throw new Error(`Unexpected predicate: ${query.sql}`);
return {
column: match[1] as "identifier" | "token",
value: query.params[0] as string,
};
}

function fakeDatabase(initialRow: TokenRow) {
let row: TokenRow | undefined = initialRow;
const db = {
select: () => ({
from: () => ({
where: (condition: SQL) => ({
limit: async () => {
if (!row) return [];
const { column, value } = parsePredicate(condition);
return row[column] === value ? [row] : [];
},
}),
}),
}),
delete: () => ({
where: async (condition: SQL) => {
if (!row) return;
const { column, value } = parsePredicate(condition);
if (row[column] === value) row = undefined;
},
}),
};
return { db: db as unknown as MySql2Database, getRow: () => row };
}

describe("useVerificationToken", () => {
const identifier = "person@example.com";
const validRow: TokenRow = {
identifier,
token: "111111",
expires: new Date(Date.now() + 60_000),
};

it("burns the code on a wrong guess instead of leaving it guessable", async () => {
const { db, getRow } = fakeDatabase({ ...validRow });
const adapter = DrizzleAdapter(db);

const wrongGuess = await adapter.useVerificationToken?.({
identifier,
token: "000000",
});

expect(wrongGuess).toBeNull();
expect(getRow()).toBeUndefined();

const correctGuessAfterward = await adapter.useVerificationToken?.({
identifier,
token: "111111",
});
expect(correctGuessAfterward).toBeNull();
});

it("returns the row and deletes it on a correct guess", async () => {
const { db, getRow } = fakeDatabase({ ...validRow });
const adapter = DrizzleAdapter(db);

const result = await adapter.useVerificationToken?.({
identifier,
token: "111111",
});

expect(result).toMatchObject({ identifier, token: "111111" });
expect(getRow()).toBeUndefined();
});

it("returns null when no code was ever requested for the identifier", async () => {
const { db } = fakeDatabase({ ...validRow });
const adapter = DrizzleAdapter(db);

const result = await adapter.useVerificationToken?.({
identifier: "nobody@example.com",
token: "111111",
});

expect(result).toBeNull();
});
});
25 changes: 11 additions & 14 deletions packages/database/auth/drizzle-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,31 +510,28 @@ export function DrizzleAdapter(
return row;
},
async useVerificationToken({ identifier, token }) {
const normalizedIdentifier = identifier?.toLowerCase() ?? "";
const rows = await db
.select()
.from(verificationTokens)
.where(eq(verificationTokens.token, token))
.where(eq(verificationTokens.identifier, normalizedIdentifier))
.limit(1);
const row = rows[0];
if (!row) {
console.warn("[useVerificationToken] No token found");
return null;
}
const normalizedIdentifier = identifier?.toLowerCase() ?? "";
const storedIdentifier = row.identifier?.toLowerCase() ?? "";
if (normalizedIdentifier !== storedIdentifier) {
console.warn("[useVerificationToken] Identifier mismatch");
return null;
}
// 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

@superagent-security superagent-security Bot Sep 5, 2026

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>

.delete(verificationTokens)
.where(
and(
eq(verificationTokens.token, token),
eq(verificationTokens.identifier, row.identifier),
),
);
return { ...row, identifier: storedIdentifier };
.where(eq(verificationTokens.identifier, row.identifier));
Comment on lines 526 to +528

@greptile-apps greptile-apps Bot Sep 5, 2026

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.


if (row.token !== token) {
console.warn("[useVerificationToken] Token mismatch");
return null;
}
return { ...row, identifier: normalizedIdentifier };
},
};
}