Skip to content

Commit 1dd7b68

Browse files
committed
fix(auth): make OTP consumption atomic to close concurrent-guess race
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.
1 parent 2562bc1 commit 1dd7b68

2 files changed

Lines changed: 92 additions & 23 deletions

File tree

apps/web/__tests__/unit/verification-token.test.ts

Lines changed: 66 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { verificationTokens } from "@cap/database/schema";
21
import type { SQL } from "drizzle-orm";
32
import { MySqlDialect } from "drizzle-orm/mysql-core";
43
import type { MySql2Database } from "drizzle-orm/mysql2";
@@ -7,41 +6,53 @@ import { DrizzleAdapter } from "../../../../packages/database/auth/drizzle-adapt
76

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

10-
function parsePredicate(condition: SQL) {
9+
function matchesPredicate(condition: SQL, row: TokenRow) {
1110
const query = new MySqlDialect().sqlToQuery(condition);
12-
const match = /^`verification_tokens`\.`(identifier|token)` = \?$/.exec(
13-
query.sql,
14-
);
15-
if (!match?.[1]) throw new Error(`Unexpected predicate: ${query.sql}`);
16-
return {
17-
column: match[1] as "identifier" | "token",
18-
value: query.params[0] as string,
19-
};
11+
const columns = [
12+
...query.sql.matchAll(
13+
/`verification_tokens`\.`(identifier|token)`\s*=\s*\?/g,
14+
),
15+
].map((match) => match[1] as "identifier" | "token");
16+
if (columns.length === 0) {
17+
throw new Error(`Unexpected predicate: ${query.sql}`);
18+
}
19+
return columns.every((column, i) => row[column] === query.params[i]);
2020
}
2121

22-
function fakeDatabase(initialRow: TokenRow) {
22+
function fakeDatabase(
23+
initialRow: TokenRow,
24+
options?: { afterSelect?: () => void },
25+
) {
2326
let row: TokenRow | undefined = initialRow;
2427
const db = {
2528
select: () => ({
2629
from: () => ({
2730
where: (condition: SQL) => ({
2831
limit: async () => {
29-
if (!row) return [];
30-
const { column, value } = parsePredicate(condition);
31-
return row[column] === value ? [row] : [];
32+
const result = row && matchesPredicate(condition, row) ? [row] : [];
33+
options?.afterSelect?.();
34+
return result;
3235
},
3336
}),
3437
}),
3538
}),
3639
delete: () => ({
3740
where: async (condition: SQL) => {
38-
if (!row) return;
39-
const { column, value } = parsePredicate(condition);
40-
if (row[column] === value) row = undefined;
41+
if (row && matchesPredicate(condition, row)) {
42+
row = undefined;
43+
return [{ affectedRows: 1 }];
44+
}
45+
return [{ affectedRows: 0 }];
4146
},
4247
}),
4348
};
44-
return { db: db as unknown as MySql2Database, getRow: () => row };
49+
return {
50+
db: db as unknown as MySql2Database,
51+
getRow: () => row,
52+
setRow: (next: TokenRow) => {
53+
row = next;
54+
},
55+
};
4556
}
4657

4758
describe("useVerificationToken", () => {
@@ -95,4 +106,41 @@ describe("useVerificationToken", () => {
95106

96107
expect(result).toBeNull();
97108
});
109+
110+
it("lets only one of two concurrent correct guesses succeed", async () => {
111+
const { db, getRow } = fakeDatabase({ ...validRow });
112+
const adapter = DrizzleAdapter(db);
113+
114+
const [first, second] = await Promise.all([
115+
adapter.useVerificationToken?.({ identifier, token: "111111" }),
116+
adapter.useVerificationToken?.({ identifier, token: "111111" }),
117+
]);
118+
119+
const successes = [first, second].filter((result) => result !== null);
120+
expect(successes).toHaveLength(1);
121+
expect(getRow()).toBeUndefined();
122+
});
123+
124+
it("does not delete a resent code that replaced the row a guess read", async () => {
125+
const resentRow: TokenRow = {
126+
identifier,
127+
token: "222222",
128+
expires: new Date(Date.now() + 60_000),
129+
};
130+
const { db, getRow, setRow } = fakeDatabase(
131+
{ ...validRow },
132+
{
133+
afterSelect: () => setRow({ ...resentRow }),
134+
},
135+
);
136+
const adapter = DrizzleAdapter(db);
137+
138+
const wrongGuessAgainstStaleCode = await adapter.useVerificationToken?.({
139+
identifier,
140+
token: "000000",
141+
});
142+
143+
expect(wrongGuessAgainstStaleCode).toBeNull();
144+
expect(getRow()).toEqual(resentRow);
145+
});
98146
});

packages/database/auth/drizzle-adapter.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,15 @@ import {
1717
} from "../schema.ts";
1818
import type { ValidatedSsoIdentity } from "./sso.ts";
1919

20+
const getAffectedRows = (result: unknown) => {
21+
if (Array.isArray(result)) {
22+
return (
23+
(result[0] as { affectedRows?: number } | undefined)?.affectedRows ?? 0
24+
);
25+
}
26+
return (result as { affectedRows?: number } | undefined)?.affectedRows ?? 0;
27+
};
28+
2029
type CreateUserData = Parameters<NonNullable<Adapter["createUser"]>>[0];
2130
type LinkAccountData = Parameters<NonNullable<Adapter["linkAccount"]>>[0];
2231
type UnlinkAccountData = Parameters<NonNullable<Adapter["unlinkAccount"]>>[0];
@@ -521,12 +530,24 @@ export function DrizzleAdapter(
521530
console.warn("[useVerificationToken] No token found");
522531
return null;
523532
}
524-
// Delete on every attempt (not just a match) so a wrong guess burns the
525-
// code instead of leaving it guessable for the rest of its TTL.
526-
await db
533+
// Claim the exact row we just read (identifier + token) with a single
534+
// delete, and only proceed if we actually removed it. This makes
535+
// consumption atomic: concurrent requests race on the same delete, so
536+
// at most one of them can claim the row, whether the guess is right or
537+
// wrong. Scoping by token too (not identifier alone) means the delete
538+
// can't clobber a resend that replaced this row after we read it.
539+
const result = await db
527540
.delete(verificationTokens)
528-
.where(eq(verificationTokens.identifier, row.identifier));
529-
541+
.where(
542+
and(
543+
eq(verificationTokens.identifier, row.identifier),
544+
eq(verificationTokens.token, row.token),
545+
),
546+
);
547+
if (getAffectedRows(result) !== 1) {
548+
console.warn("[useVerificationToken] Token already consumed");
549+
return null;
550+
}
530551
if (row.token !== token) {
531552
console.warn("[useVerificationToken] Token mismatch");
532553
return null;

0 commit comments

Comments
 (0)