From 2562bc18c517043c68ed7cc764656324d62290b6 Mon Sep 17 00:00:00 2001 From: addyCooks Date: Sat, 5 Sep 2026 21:25:09 +0530 Subject: [PATCH 1/4] fix(auth): invalidate email OTP on a failed guess, not just a match 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. --- .../__tests__/unit/verification-token.test.ts | 98 +++++++++++++++++++ packages/database/auth/drizzle-adapter.ts | 25 +++-- 2 files changed, 109 insertions(+), 14 deletions(-) create mode 100644 apps/web/__tests__/unit/verification-token.test.ts diff --git a/apps/web/__tests__/unit/verification-token.test.ts b/apps/web/__tests__/unit/verification-token.test.ts new file mode 100644 index 0000000000..54822e4483 --- /dev/null +++ b/apps/web/__tests__/unit/verification-token.test.ts @@ -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(); + }); +}); diff --git a/packages/database/auth/drizzle-adapter.ts b/packages/database/auth/drizzle-adapter.ts index 16f7e10453..57e663f2ca 100644 --- a/packages/database/auth/drizzle-adapter.ts +++ b/packages/database/auth/drizzle-adapter.ts @@ -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 .delete(verificationTokens) - .where( - and( - eq(verificationTokens.token, token), - eq(verificationTokens.identifier, row.identifier), - ), - ); - return { ...row, identifier: storedIdentifier }; + .where(eq(verificationTokens.identifier, row.identifier)); + + if (row.token !== token) { + console.warn("[useVerificationToken] Token mismatch"); + return null; + } + return { ...row, identifier: normalizedIdentifier }; }, }; } From 1dd7b68300a139162db3f980a16df98cfbf30b93 Mon Sep 17 00:00:00 2001 From: addyCooks Date: Sun, 6 Sep 2026 13:27:32 +0530 Subject: [PATCH 2/4] 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. --- .../__tests__/unit/verification-token.test.ts | 84 +++++++++++++++---- packages/database/auth/drizzle-adapter.ts | 31 +++++-- 2 files changed, 92 insertions(+), 23 deletions(-) diff --git a/apps/web/__tests__/unit/verification-token.test.ts b/apps/web/__tests__/unit/verification-token.test.ts index 54822e4483..ea7c963578 100644 --- a/apps/web/__tests__/unit/verification-token.test.ts +++ b/apps/web/__tests__/unit/verification-token.test.ts @@ -1,4 +1,3 @@ -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"; @@ -7,41 +6,53 @@ import { DrizzleAdapter } from "../../../../packages/database/auth/drizzle-adapt type TokenRow = { identifier: string; token: string; expires: Date }; -function parsePredicate(condition: SQL) { +function matchesPredicate(condition: SQL, row: TokenRow) { 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, - }; + const columns = [ + ...query.sql.matchAll( + /`verification_tokens`\.`(identifier|token)`\s*=\s*\?/g, + ), + ].map((match) => match[1] as "identifier" | "token"); + if (columns.length === 0) { + throw new Error(`Unexpected predicate: ${query.sql}`); + } + return columns.every((column, i) => row[column] === query.params[i]); } -function fakeDatabase(initialRow: TokenRow) { +function fakeDatabase( + initialRow: TokenRow, + options?: { afterSelect?: () => void }, +) { 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] : []; + const result = row && matchesPredicate(condition, row) ? [row] : []; + options?.afterSelect?.(); + return result; }, }), }), }), delete: () => ({ where: async (condition: SQL) => { - if (!row) return; - const { column, value } = parsePredicate(condition); - if (row[column] === value) row = undefined; + if (row && matchesPredicate(condition, row)) { + row = undefined; + return [{ affectedRows: 1 }]; + } + return [{ affectedRows: 0 }]; }, }), }; - return { db: db as unknown as MySql2Database, getRow: () => row }; + return { + db: db as unknown as MySql2Database, + getRow: () => row, + setRow: (next: TokenRow) => { + row = next; + }, + }; } describe("useVerificationToken", () => { @@ -95,4 +106,41 @@ describe("useVerificationToken", () => { expect(result).toBeNull(); }); + + it("lets only one of two concurrent correct guesses succeed", async () => { + const { db, getRow } = fakeDatabase({ ...validRow }); + const adapter = DrizzleAdapter(db); + + const [first, second] = await Promise.all([ + adapter.useVerificationToken?.({ identifier, token: "111111" }), + adapter.useVerificationToken?.({ identifier, token: "111111" }), + ]); + + const successes = [first, second].filter((result) => result !== null); + expect(successes).toHaveLength(1); + expect(getRow()).toBeUndefined(); + }); + + it("does not delete a resent code that replaced the row a guess read", async () => { + const resentRow: TokenRow = { + identifier, + token: "222222", + expires: new Date(Date.now() + 60_000), + }; + const { db, getRow, setRow } = fakeDatabase( + { ...validRow }, + { + afterSelect: () => setRow({ ...resentRow }), + }, + ); + const adapter = DrizzleAdapter(db); + + const wrongGuessAgainstStaleCode = await adapter.useVerificationToken?.({ + identifier, + token: "000000", + }); + + expect(wrongGuessAgainstStaleCode).toBeNull(); + expect(getRow()).toEqual(resentRow); + }); }); diff --git a/packages/database/auth/drizzle-adapter.ts b/packages/database/auth/drizzle-adapter.ts index 57e663f2ca..882fd950b9 100644 --- a/packages/database/auth/drizzle-adapter.ts +++ b/packages/database/auth/drizzle-adapter.ts @@ -17,6 +17,15 @@ import { } from "../schema.ts"; import type { ValidatedSsoIdentity } from "./sso.ts"; +const getAffectedRows = (result: unknown) => { + if (Array.isArray(result)) { + return ( + (result[0] as { affectedRows?: number } | undefined)?.affectedRows ?? 0 + ); + } + return (result as { affectedRows?: number } | undefined)?.affectedRows ?? 0; +}; + type CreateUserData = Parameters>[0]; type LinkAccountData = Parameters>[0]; type UnlinkAccountData = Parameters>[0]; @@ -521,12 +530,24 @@ export function DrizzleAdapter( console.warn("[useVerificationToken] No token found"); 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 + // Claim the exact row we just read (identifier + token) with a single + // delete, and only proceed if we actually removed it. This makes + // consumption atomic: concurrent requests race on the same delete, so + // at most one of them can claim the row, whether the guess is right or + // wrong. Scoping by token too (not identifier alone) means the delete + // can't clobber a resend that replaced this row after we read it. + const result = await db .delete(verificationTokens) - .where(eq(verificationTokens.identifier, row.identifier)); - + .where( + and( + eq(verificationTokens.identifier, row.identifier), + eq(verificationTokens.token, row.token), + ), + ); + if (getAffectedRows(result) !== 1) { + console.warn("[useVerificationToken] Token already consumed"); + return null; + } if (row.token !== token) { console.warn("[useVerificationToken] Token mismatch"); return null; From 6473cc94ccdd011ba31df641140a1f23da1928b2 Mon Sep 17 00:00:00 2001 From: addyCooks Date: Sun, 6 Sep 2026 13:47:10 +0530 Subject: [PATCH 3/4] test(auth): cover OTP concurrent-guess atomicity against real MySQL 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. --- .../integration/sso-database.test.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/apps/web/__tests__/integration/sso-database.test.ts b/apps/web/__tests__/integration/sso-database.test.ts index 5d4c6ce485..3f9d619881 100644 --- a/apps/web/__tests__/integration/sso-database.test.ts +++ b/apps/web/__tests__/integration/sso-database.test.ts @@ -6,6 +6,7 @@ import { organizationSso, organizations, users, + verificationTokens, } from "@cap/database/schema"; import { Organisation, User } from "@cap/web-domain"; import { and, eq } from "drizzle-orm"; @@ -344,5 +345,31 @@ describe.runIf(Boolean(databaseUrl))( .where(eq(organizationMembers.userId, userId)), ).toHaveLength(0); }); + + it("burns an email OTP exactly once under concurrent guesses", async () => { + const identifier = `${id()}@example.com`; + const token = "482913"; + const adapter = DrizzleAdapter(database()); + if (!adapter.useVerificationToken) { + throw new Error("Missing useVerificationToken adapter."); + } + await database() + .insert(verificationTokens) + .values({ identifier, token, expires: new Date(Date.now() + 600_000) }); + + const attempts = await Promise.all( + Array.from({ length: 8 }, () => + adapter.useVerificationToken({ identifier, token }), + ), + ); + + expect(attempts.filter((result) => result !== null)).toHaveLength(1); + expect( + await database() + .select() + .from(verificationTokens) + .where(eq(verificationTokens.identifier, identifier)), + ).toHaveLength(0); + }); }, ); From 54e0bd53a6f2c158c9d48a5dd7be129a0da1a8e2 Mon Sep 17 00:00:00 2001 From: addyCooks Date: Sun, 6 Sep 2026 14:07:54 +0530 Subject: [PATCH 4/4] test(auth): cover mixed correct/incorrect OTP guesses racing on real 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. --- .../integration/sso-database.test.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/apps/web/__tests__/integration/sso-database.test.ts b/apps/web/__tests__/integration/sso-database.test.ts index 3f9d619881..55c2b5a68f 100644 --- a/apps/web/__tests__/integration/sso-database.test.ts +++ b/apps/web/__tests__/integration/sso-database.test.ts @@ -371,5 +371,36 @@ describe.runIf(Boolean(databaseUrl))( .where(eq(verificationTokens.identifier, identifier)), ).toHaveLength(0); }); + + it("never authenticates a wrong guess when it races the correct one", async () => { + const identifier = `${id()}@example.com`; + const token = "531942"; + const adapter = DrizzleAdapter(database()); + if (!adapter.useVerificationToken) { + throw new Error("Missing useVerificationToken adapter."); + } + await database() + .insert(verificationTokens) + .values({ identifier, token, expires: new Date(Date.now() + 600_000) }); + + const guesses = ["000000", "111111", token, "222222", "333333"]; + const attempts = await Promise.all( + guesses.map((guess) => + adapter.useVerificationToken({ identifier, token: guess }), + ), + ); + + const successes = attempts.filter((result) => result !== null); + expect(successes.length).toBeLessThanOrEqual(1); + for (const success of successes) { + expect(success).toMatchObject({ identifier, token }); + } + expect( + await database() + .select() + .from(verificationTokens) + .where(eq(verificationTokens.identifier, identifier)), + ).toHaveLength(0); + }); }, );