fix(two-factor): make enabling two-factor safe under concurrent requests

This commit is contained in:
Bereket Engida
2026-09-04 14:58:40 -07:00
parent 5fe5bc21d1
commit 17e99d670a
3 changed files with 94 additions and 0 deletions
+5
View File
@@ -0,0 +1,5 @@
---
"better-auth": patch
---
Make enabling two-factor safe under concurrent requests by converging on a single `twoFactor` row per user.
@@ -286,6 +286,33 @@ export const twoFactor = <O extends TwoFactorOptions>(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<TwoFactorTable>({
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,
@@ -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<void>((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<TwoFactorTable>({
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)),
);
});
});