diff --git a/.changeset/two-factor-enable-race.md b/.changeset/two-factor-enable-race.md new file mode 100644 index 0000000000..c408a7a03b --- /dev/null +++ b/.changeset/two-factor-enable-race.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +Make enabling two-factor safe under concurrent requests by converging on a single `twoFactor` row per user. diff --git a/packages/better-auth/src/plugins/two-factor/index.ts b/packages/better-auth/src/plugins/two-factor/index.ts index 45614cfb28..e4b4d57816 100644 --- a/packages/better-auth/src/plugins/two-factor/index.ts +++ b/packages/better-auth/src/plugins/two-factor/index.ts @@ -286,6 +286,33 @@ export const twoFactor = (options?: O) => { data: { ...totpData, userId: user.id }, }); } + // `userId` is not unique, so concurrent enrollments can both pass the + // findOne above and both insert. Converge on one row that every racer + // picks identically (verified first, then lowest id) so a later findOne + // cannot return a factor the user never scanned. A racer whose row + // lost returned a URI that will fail verification; re-enabling + // updates the surviving unverified row. + const rows = await ctx.context.adapter.findMany({ + model: opts.twoFactorTable, + where: [{ field: "userId", value: user.id }], + }); + if (rows.length > 1) { + const [, ...duplicates] = rows.sort( + (a, b) => + Number(b.verified !== false) - Number(a.verified !== false) || + (String(a.id) < String(b.id) ? -1 : 1), + ); + await ctx.context.adapter.deleteMany({ + model: opts.twoFactorTable, + where: [ + { + field: "id", + operator: "in", + value: duplicates.map((row) => row.id), + }, + ], + }); + } const totpURI = createOTP(secret, { digits: options?.totpOptions?.digits || 6, period: options?.totpOptions?.period, diff --git a/packages/better-auth/src/plugins/two-factor/two-factor.test.ts b/packages/better-auth/src/plugins/two-factor/two-factor.test.ts index 337389b96a..eb04ab3ad2 100644 --- a/packages/better-auth/src/plugins/two-factor/two-factor.test.ts +++ b/packages/better-auth/src/plugins/two-factor/two-factor.test.ts @@ -2713,3 +2713,65 @@ describe("backup codes storage configurations", () => { }); } }); + +describe("two factor enable concurrency", async () => { + const { auth, db, signInWithTestUser, testUser } = await getTestInstance({ + secret: DEFAULT_SECRET, + plugins: [twoFactor()], + }); + + /** + * @see https://github.com/better-auth/better-auth/issues/10561 + */ + it("should keep a single usable factor when enable is called concurrently", async () => { + const { headers, user } = await signInWithTestUser(); + const adapter = (await auth.$context).adapter; + const create = adapter.create.bind(adapter); + // Park every twoFactor insert until both requests have passed their + // findOne, so both observe "no existing factor" and both insert. + let arrived = 0; + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + vi.spyOn(adapter, "create").mockImplementation(async (input) => { + if (input.model === "twoFactor") { + arrived++; + if (arrived === 2) release(); + await gate; + } + return create(input); + }); + + const results = await Promise.all([ + auth.api.enableTwoFactor({ + body: { password: testUser.password }, + headers, + }), + auth.api.enableTwoFactor({ + body: { password: testUser.password }, + headers, + }), + ]); + vi.restoreAllMocks(); + + const rows = await db.findMany({ + model: "twoFactor", + where: [{ field: "userId", value: user.id }], + }); + expect(rows).toHaveLength(1); + const storedSecret = await symmetricDecrypt({ + key: DEFAULT_SECRET, + data: rows[0]!.secret, + }); + const secretParam = (uri: string) => + new URL(uri).searchParams.get("secret"); + const returnedSecrets = results.map((res) => { + if (res.method !== "totp") throw new Error("expected totp"); + return secretParam(res.totpURI); + }); + expect(returnedSecrets).toContain( + secretParam(createOTP(storedSecret).url("test", user.email)), + ); + }); +});