Skip to content

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

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
mini0n-ai wants to merge 1 commit into
base: main
Choose a base branch
from
+241 −23
Open

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

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
196 changes: 196 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,196 @@
import type { MySql2Database } from "drizzle-orm/mysql2";
import { describe, expect, it } from "vitest";
import { DrizzleAdapter } from "../../../../packages/database/auth/drizzle-adapter";

interface VerificationTokenRow {
identifier: string;
token: string;
expires: Date;
}

function createMockDb(initialRows: VerificationTokenRow[]) {
let table = [...initialRows];
let deletePredicate: unknown = null;

const db = {
select: () => ({
from: () => ({
where: () => ({
limit: async () => table.slice(0, 1),
}),
}),
}),
delete: () => ({
where: (pred: unknown) => {
deletePredicate = pred;
const initialCount = table.length;
table = table.filter(
(row) =>
!(
row.identifier.toLowerCase() === "user@example.com" &&
row.token === "123456"
),
);
const rowsAffected = initialCount - table.length;
return Promise.resolve({ rowsAffected });
},
}),
transaction: async (cb: (tx: unknown) => Promise<unknown>) => cb(db),
getTable: () => table,
getDeletePredicate: () => deletePredicate,
};

return db;
}

describe("useVerificationToken", () => {
it("burns the token on wrong guess and returns null", async () => {
const mockDb = createMockDb([
{
identifier: "user@example.com",
token: "123456",
expires: new Date(Date.now() + 600000),
},
]);

const adapter = DrizzleAdapter(mockDb as unknown as MySql2Database);
const result = await adapter.useVerificationToken?.({
identifier: "USER@example.com",
token: "999999",
});

expect(result).toBeNull();
expect(mockDb.getDeletePredicate()).not.toBeNull();
expect(mockDb.getTable()).toHaveLength(0);
});

it("returns token and invalidates it on correct guess", async () => {
const mockDb = createMockDb([
{
identifier: "user@example.com",
token: "123456",
expires: new Date(Date.now() + 600000),
},
]);

const adapter = DrizzleAdapter(mockDb as unknown as MySql2Database);
const result = await adapter.useVerificationToken?.({
identifier: "USER@example.com",
token: "123456",
});

expect(result).not.toBeNull();
expect(result?.identifier).toBe("user@example.com");
expect(result?.token).toBe("123456");
expect(mockDb.getDeletePredicate()).not.toBeNull();
expect(mockDb.getTable()).toHaveLength(0);
});

it("returns null if token does not exist", async () => {
const mockDb = createMockDb([]);

const adapter = DrizzleAdapter(mockDb as unknown as MySql2Database);
const result = await adapter.useVerificationToken?.({
identifier: "nonexistent@example.com",
token: "123456",
});

expect(result).toBeNull();
expect(mockDb.getDeletePredicate()).toBeNull();
});

it("prevents race condition by checking rowsAffected on token consumption", async () => {
let table = [
{
identifier: "user@example.com",
token: "123456",
expires: new Date(Date.now() + 600000),
},
];

const mockDb = {
select: () => ({
from: () => ({
where: () => ({
limit: async () => table.slice(0, 1),
}),
}),
}),
delete: () => ({
where: () => {
const initialCount = table.length;
table = [];
const rowsAffected = initialCount;
return Promise.resolve({ rowsAffected });
},
}),
transaction: async (cb: (tx: unknown) => Promise<unknown>) => cb(mockDb),
} as unknown as MySql2Database;

const adapter = DrizzleAdapter(mockDb);

const firstResult = await adapter.useVerificationToken?.({
identifier: "USER@example.com",
token: "123456",
});

expect(firstResult).not.toBeNull();
expect(firstResult?.token).toBe("123456");

const secondResult = await adapter.useVerificationToken?.({
identifier: "USER@example.com",
token: "123456",
});

expect(secondResult).toBeNull();
});

it("deletes only the selected token instance and preserves replacement tokens for the same user", async () => {
let table = [
{
identifier: "user@example.com",
token: "123456",
expires: new Date(Date.now() + 600000),
},
{
identifier: "user@example.com",
token: "replacement_token",
expires: new Date(Date.now() + 600000),
},
];

const mockDb = {
select: () => ({
from: () => ({
where: () => ({
limit: async () => [table[0]],
}),
}),
}),
delete: () => ({
where: () => {
const initialCount = table.length;
table = table.filter(
(row) =>
!(
row.identifier === "user@example.com" && row.token === "123456"
),
);
const rowsAffected = initialCount - table.length;
return Promise.resolve({ rowsAffected });
},
}),
transaction: async (cb: (tx: unknown) => Promise<unknown>) => cb(mockDb),
} as unknown as MySql2Database;

const adapter = DrizzleAdapter(mockDb);
const result = await adapter.useVerificationToken?.({
identifier: "USER@example.com",
token: "999999",
});

expect(result).toBeNull();
expect(table.some((r) => r.token === "123456")).toBe(false);
expect(table.some((r) => r.token === "replacement_token")).toBe(true);
});
});
68 changes: 45 additions & 23 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,53 @@ export function DrizzleAdapter(
return row;
},
async useVerificationToken({ identifier, token }) {
const rows = await db
.select()
.from(verificationTokens)
.where(eq(verificationTokens.token, token))
.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;

const execute = async (tx: typeof db) => {
const rows = await tx
.select()
.from(verificationTokens)
.where(eq(verificationTokens.identifier, normalizedIdentifier))
.limit(1);
const row = rows[0];
if (!row) {
console.warn("[useVerificationToken] No token found");
return null;
}
const storedIdentifier = row.identifier?.toLowerCase() ?? "";

@superagent-security superagent-security Bot Sep 10, 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.

P2: The OTP deletion is scoped only by identifier and can invalidate a different token during rotation

DELETE filters only by email, so OTP rotation or concurrent requests can consume the wrong row or replay one OTP.

Atomically consume the selected token by its unique key and verify the affected-row count; add 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:528">
<priority>P2</priority>
<title>The OTP deletion is scoped only by identifier and can invalidate a different token during rotation</title>
<evidence>After selecting one row by normalized identifier, the new deletion uses only eq(verificationTokens.identifier, row.identifier). A token generated for the same email between the SELECT and DELETE, or another concurrently stored token for that email, can therefore be deleted even though it was not the row selected for this request. The separate SELECT and DELETE also leave concurrent verification requests able to both observe and return the same valid row before either deletion is committed, so this does not provide the claimed replay/race protection.</evidence>
<recommendation>Consume the exact selected row atomically: use a transaction with row locking, or a single conditional DELETE/UPDATE that identifies the row by its unique token/key and returns the consumed row. Check the affected-row/result count before returning success, and add concurrency and token-rotation tests.</recommendation>
</violation>
</file>

// Invalidate the specific token instance that was selected. This burns wrong guesses
// while scoping deletion to both identifier AND row.token to protect newly issued replacement tokens.
const result = await tx
.delete(verificationTokens)
.where(
and(
eq(verificationTokens.identifier, row.identifier),
eq(verificationTokens.token, row.token),
),
);

// If database reports 0 rows affected, token was consumed or rotated concurrently
const rowsAffected = (result as { rowsAffected?: number })?.rowsAffected;
if (rowsAffected === 0) {
console.warn(
"[useVerificationToken] Token already consumed or invalid during deletion.",
);
return null;
}

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

return { ...row, identifier: storedIdentifier };
};

if (typeof db.transaction === "function") {
return await db.transaction(async (tx) => execute(tx as unknown as typeof db));
}
await db
.delete(verificationTokens)
.where(
and(
eq(verificationTokens.token, token),
eq(verificationTokens.identifier, row.identifier),
),
);
return { ...row, identifier: storedIdentifier };
return await execute(db);
},
};
}