diff --git a/.changeset/two-factor-reenrollment-verified-flag.md b/.changeset/two-factor-reenrollment-verified-flag.md new file mode 100644 index 0000000000..1860c2d6b6 --- /dev/null +++ b/.changeset/two-factor-reenrollment-verified-flag.md @@ -0,0 +1,36 @@ +--- +"@objectstack/plugin-auth": patch +--- + +`POST /api/v1/auth/two-factor/enable` no longer leaves `sys_two_factor.verified` +describing the enrollment *before* the secret it stores. + +better-auth's enable handler computes the row it writes as +`verified: existingTwoFactor != null && existingTwoFactor.verified === true` +(measured on the installed 1.7.1, `dist/plugins/two-factor/index.mjs`), and +`sys_two_factor` declares `user_id` unique — so a second `enable` on an account +that already has a confirmed factor rewrites that one row with a brand-new +secret while inheriting the old enrollment's flag. The flag then said +"user-confirmed" about a secret nobody had ever confirmed, and the sign-in +challenge honoured it. + +The vendor already gates the challenge on that flag, in both places it matters: +`totp/index.mjs` refuses an unconfirmed factor with `TOTP_NOT_ENABLED` before +any lockout bookkeeping, and the post-sign-in hook offers `totp` among +`twoFactorMethods` only when the flag is not `false`. That gate is exactly what +a *first* enrollment relies on. Re-enrollment was the one path that slipped past +it — not because the gate was missing, but because the value handed to it was +inherited. So the fix restores the flag rather than adding a second gate: +after a successful `method: 'totp'` enable, `verified` is set to `false`, and +the freshly issued secret becomes live only once the caller proves possession of +it through `/two-factor/verify-totp`. + +This is a tightening. The request body, the response shape and the status are +unchanged, a first-time enrollment is unaffected (better-auth already wrote +`false` there), and a rotation is still reachable and still completes — it now +takes the same confirmation step a first enrollment takes. What changes is that +a secret the endpoint hands out is no longer accepted at the next sign-in until +it has been confirmed. Clients that re-enroll and then rely on the new +authenticator working immediately at sign-in must call `/two-factor/verify-totp` +with the live session first, which is the flow first-time enrollment already +uses. diff --git a/content/docs/permissions/authentication.mdx b/content/docs/permissions/authentication.mdx index a7aa962e34..80cfad85e6 100644 --- a/content/docs/permissions/authentication.mdx +++ b/content/docs/permissions/authentication.mdx @@ -547,6 +547,18 @@ A complete opt-in 2FA UX still needs to handle: - the `twoFactorRedirect` response returned by password sign-in, and - backup-code recovery. + + **`/two-factor/enable` always issues a secret that must be confirmed — including + on re-enrollment.** Calling it on an account that already has 2FA active replaces + the stored secret and marks the enrollment unconfirmed, so the new secret is + **not** accepted at the sign-in challenge (`400 TOTP_NOT_ENABLED`) and `totp` is + not offered in `twoFactorMethods` until `/two-factor/verify-totp` succeeds with + the live session. The replaced secret stops working as soon as `enable` returns, + so a re-enrollment UI must run the confirmation step in the same session, and + must show the backup codes from that response — they are the recovery path if + the authenticator was never captured. + + For custom account UIs, enable the backend plugin in configuration: ```typescript diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index a512c8ae51..ce4e2711ed 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -38,6 +38,7 @@ import { withBearerAdminSessionRecovery, } from './impersonation-bearer-rotation.js'; import { echoInstalledSessionToken } from './two-factor-rotated-token-echo.js'; +import { resetVerifiedOnTwoFactorReenrollment } from './two-factor-reenrollment-verified-reset.js'; import { applyPlatformAdminImpersonation, } from './admin-impersonate-endpoint.js'; @@ -1716,6 +1717,18 @@ export class AuthManager { // corrects the echoed VALUE only; resolver precedence is untouched. await echoInstalledSessionToken(ctx); + // ── #10700: `verified` must describe the secret stored beside it ── + // A second `/two-factor/enable` on an already-confirmed account + // rewrites the TOTP secret on the one `sys_two_factor` row the + // account has and INHERITS `verified` from the enrollment before it, + // so a secret nobody confirmed is honoured at the sign-in challenge. + // better-auth already gates that challenge on the flag — a first + // enrollment is inert until the session-lane verify flips it — so + // restoring the flag restores the gate rather than adding a second + // one. See `two-factor-reenrollment-verified-reset.ts`, whose header + // also states what this deliberately does NOT do. + await resetVerifiedOnTwoFactorReenrollment(ctx); + // ── ADR-0069 D2: account lockout (counter) ────────────────── // better-auth catches an INVALID_EMAIL_OR_PASSWORD APIError and runs // the after-hook with it on `ctx.context.returned`; a success leaves diff --git a/packages/plugins/plugin-auth/src/two-factor-reenrollment-verified-reset.test.ts b/packages/plugins/plugin-auth/src/two-factor-reenrollment-verified-reset.test.ts new file mode 100644 index 0000000000..a9761e4911 --- /dev/null +++ b/packages/plugins/plugin-auth/src/two-factor-reenrollment-verified-reset.test.ts @@ -0,0 +1,422 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #10700 — a second `/two-factor/enable` on an already-confirmed account +// rewrote the TOTP secret on the account's one `sys_two_factor` row and +// INHERITED `verified` from the enrollment before it. +// +// The shape of the defect dictates the shape of these tests, twice over. +// +// ① `verified` is a FLAG, and a suite that asserts only the flag is the exact +// mistake that produced the defect: the flag is not the control, it is the +// INPUT to the control. Every pin below that names the flag is paired with +// the question the runtime actually asks — what does the next sign-in +// challenge accept? — and the pair is asserted in the same test. +// +// ② The endpoint answered 200 before the fix and answers 200 after it. So a +// test asserting "enable still works", or "a totpURI came back", or "the +// secret changed" would have been GREEN against the bug. Nothing here ends +// at a status on `/two-factor/enable`. +// +// And because the fix is a REFUSAL, the still-works legs are load-bearing: an +// implementation that simply refused every `enable` would satisfy a +// one-directional suite while breaking every enrollment shipping today. Three +// legs here fail against such an implementation — a first enrollment completes +// end to end, a legitimate rotation completes end to end, and the backup codes +// the enable response issues still complete a sign-in. +// +// Real better-auth pipeline throughout, following +// `two-factor-rotated-token-echo.test.ts`: requests go in as `Request` objects +// through `AuthManager.handleRequest`, the secrets are the ones better-auth +// minted, and "who does this resolve to" is asked through +// `auth.api.getSession` — the same seam +// `runtime/src/security/resolve-session-principal.ts` calls. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { createHmac } from 'node:crypto'; +import { AuthManager } from './auth-manager'; +// The SAME in-memory engine the #8243 and #10701 harnesses drive. A third fake +// would be a third looseness risk and a new `check:engine-double-contract` +// ledger entry, for no added fidelity. +import { createMemoryEngine } from './impersonation-bearer-rotation.test'; + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const PASSWORD = 'S3cure!Passw0rd-10700'; +const EMAIL = 'reenroller@example.com'; +const BASE = 'http://localhost:3000/api/v1/auth'; + +// ── RFC 6238 TOTP ────────────────────────────────────────────────────────── +// Hand-rolled for the reason `two-factor-rotated-token-echo.test.ts` gives: +// `@better-auth/utils/otp` is a transitive dependency and taking a direct +// dependency on it to generate six digits would tie this suite to an internal +// package's resolution. better-auth's defaults are the RFC's (SHA-1, 6 digits, +// 30s). + +function base32Decode(input: string): Buffer { + const ALPHABET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ234567'; + const clean = input.replace(/=+$/, '').toUpperCase(); + let bits = 0; + let value = 0; + const out: number[] = []; + for (const char of clean) { + const idx = ALPHABET.indexOf(char); + if (idx === -1) throw new Error(`invalid base32 character: ${char}`); + value = (value << 5) | idx; + bits += 5; + if (bits >= 8) { + out.push((value >>> (bits - 8)) & 0xff); + bits -= 8; + } + } + return Buffer.from(out); +} + +/** The 6-digit TOTP for `secret` at the current 30-second step. */ +function totp(secret: Buffer): string { + const counter = Math.floor(Date.now() / 30_000); + const buf = Buffer.alloc(8); + buf.writeBigUInt64BE(BigInt(counter)); + const digest = createHmac('sha1', secret).update(buf).digest(); + const offset = digest[digest.length - 1] & 0x0f; + const code = + ((digest[offset] & 0x7f) << 24) | + ((digest[offset + 1] & 0xff) << 16) | + ((digest[offset + 2] & 0xff) << 8) | + (digest[offset + 3] & 0xff); + return String(code % 1_000_000).padStart(6, '0'); +} + +const cookieHeader = (res: Response): string => + (res.headers.getSetCookie?.() ?? []).map((c) => c.split(';')[0]).join('; '); + +const makeManager = (engine: any) => + new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + dataEngine: engine, + plugins: { twoFactor: true }, + } as any); + +const post = ( + manager: AuthManager, + path: string, + body: unknown, + headers: Record = {}, +) => + manager.handleRequest( + new Request(`${BASE}${path}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...headers }, + body: JSON.stringify(body ?? {}), + }), + ); + +const userIdFor = (engine: any, email: string): string => { + const row = ((engine.tables.get('sys_user') ?? []) as any[]).find((r) => r.email === email); + if (!row) throw new Error(`no sys_user row for ${email}`); + return String(row.id); +}; + +/** The account's one `sys_two_factor` row, straight out of the engine. */ +const enrolmentRow = (engine: any, userId: string): any => { + const rows = ((engine.tables.get('sys_two_factor') ?? []) as any[]).filter( + (r) => String(r.user_id) === userId, + ); + // `sys_two_factor` declares `user_id` unique. If that ever stops holding, + // every "the stored secret" assertion below is reading an arbitrary row, so + // it is checked rather than assumed. + expect(rows.length, 'sys_two_factor is meant to hold exactly one row per user').toBe(1); + return rows[0]; +}; + +/** + * Assert `verified` at BOTH spellings that matter, because they are not the + * same value and only one of them decides anything. + * + * better-auth's challenge gate compares STRICTLY against `false` + * (`totp/index.mjs`, and the post-sign-in hook that populates + * `twoFactorMethods`). What it compares is the value its own adapter hands + * back — and this repo's adapter declares `supportsBooleans: false` + * (`objectql-adapter.ts`), so better-auth stores the column as 0/1 and + * converts it back to a boolean on read. Measured, not assumed: reading the + * engine table directly returns `0` where the gate sees `false`. + * + * So the load-bearing assertion is the adapter-side one; the column-side one + * is pinned beside it so a driver change that started handing the raw integer + * to a strict `=== false` could not slip through green. + */ +const expectVerified = async ( + manager: AuthManager, + engine: any, + userId: string, + expected: boolean, + why: string, +): Promise => { + const auth: any = await manager.getAuthInstance(); + const context = await auth.$context; + const asTheGateSeesIt = await context.adapter.findOne({ + model: 'twoFactor', + where: [{ field: 'userId', value: userId }], + }); + expect(asTheGateSeesIt, 'better-auth cannot read back the enrolment row').toBeTruthy(); + expect(asTheGateSeesIt.verified, why).toBe(expected); + expect(enrolmentRow(engine, userId).verified, `${why} (stored column)`).toBe(expected ? 1 : 0); +}; + +const principalFor = async ( + manager: AuthManager, + headers: Record, +): Promise => { + const auth: any = await manager.getAuthInstance(); + const session = await auth.api.getSession({ headers: new Headers(headers) }).catch(() => null); + const id = session?.user?.id ?? session?.session?.userId; + return typeof id === 'string' && id.length > 0 ? id : null; +}; + +/** The base32 secret better-auth just handed out, decoded. */ +const secretFromEnableResponse = async (res: Response): Promise => { + const { totpURI } = (await res.clone().json()) as { totpURI: string }; + const uriSecret = new URL(totpURI.replace('otpauth://', 'https://')).searchParams.get('secret'); + expect(uriSecret, 'no secret in the otpauth URI').toBeTruthy(); + return base32Decode(String(uriSecret)); +}; + +/** + * A password sign-in that stops at the 2FA challenge. + * + * Returns the challenge cookie AND the methods the challenge offered — the + * second is half the answer to "what does the challenge accept", and it is + * derived from the same flag on the same row. + */ +const beginChallenge = async (manager: AuthManager) => { + const res = await post(manager, '/sign-in/email', { email: EMAIL, password: PASSWORD }); + expect(res.status, `sign-in: ${await res.clone().text()}`).toBe(200); + const body = (await res.clone().json()) as { twoFactorRedirect?: boolean; twoFactorMethods?: string[] }; + expect( + body.twoFactorRedirect, + 'sign-in did not stop at the 2FA challenge — every assertion after this would be measuring the session lane', + ).toBe(true); + const cookie = cookieHeader(res); + expect(cookie, 'sign-in returned no two-factor cookie').toBeTruthy(); + return { cookie, methods: body.twoFactorMethods ?? [] }; +}; + +/** ADR-0112: a refusal is a `code` AND a `status`, never a status alone. */ +const refusal = async (res: Response): Promise<{ status: number; code: unknown }> => { + const body = await res.clone().json().catch(() => ({}) as any); + return { status: res.status, code: (body as any)?.code ?? (body as any)?.error?.code }; +}; + +/** + * A brand-new account that has completed a FIRST enrollment end to end, and + * whose confirmed secret is proven to complete a sign-in challenge. + * + * Both halves are load-bearing. The completion is the still-works leg for + * first-time enrollment; the proven challenge is the positive control without + * which "the challenge refuses the re-enrolled secret" could not be told apart + * from "this harness never completes a challenge at all". + */ +const arrangeConfirmedEnrolment = async () => { + const engine = createMemoryEngine(); + const manager = makeManager(engine); + + const signedUp = await post(manager, '/sign-up/email', { + email: EMAIL, + password: PASSWORD, + name: 'Re-enrolling User', + }); + expect(signedUp.status, `sign-up: ${await signedUp.clone().text()}`).toBe(200); + const userId = userIdFor(engine, EMAIL); + + const enabled = await post( + manager, + '/two-factor/enable', + { password: PASSWORD }, + { cookie: cookieHeader(signedUp) }, + ); + expect(enabled.status, `two-factor/enable (first): ${await enabled.clone().text()}`).toBe(200); + const firstSecret = await secretFromEnableResponse(enabled); + + // A first enrollment is INERT until confirmed — better-auth's own posture, + // and the behaviour re-enrollment was skipping. + await expectVerified(manager, engine, userId, false, 'a fresh enrollment must not read as confirmed'); + + const confirmed = await post( + manager, + '/two-factor/verify-totp', + { code: totp(firstSecret) }, + { cookie: cookieHeader(signedUp) }, + ); + expect(confirmed.status, `verify-totp (first enrolment): ${await confirmed.clone().text()}`).toBe(200); + await expectVerified(manager, engine, userId, true, 'confirming must flip the flag'); + + // verify-totp rotates the session on the first-enrolment lane (#10701), so + // the live cookie is the one IT installed, not the sign-up one. + const sessionCookie = cookieHeader(confirmed); + expect(sessionCookie, 'verify-totp installed no session cookie').toContain('session_token='); + expect(await principalFor(manager, { cookie: sessionCookie })).toBe(userId); + + return { engine, manager, userId, firstSecret, sessionCookie }; +}; + +beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => vi.restoreAllMocks()); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#10700 — first enrollment still completes (the still-works floor)', () => { + it('enable → confirm → the challenge accepts the confirmed secret and signs the user in', async () => { + const { manager, userId, firstSecret } = await arrangeConfirmedEnrolment(); + + const { cookie, methods } = await beginChallenge(manager); + expect(methods, 'the challenge must offer totp for a confirmed enrollment').toContain('totp'); + + const completed = await post(manager, '/two-factor/verify-totp', { code: totp(firstSecret) }, { cookie }); + expect(completed.status, `verify-totp (challenge): ${await completed.clone().text()}`).toBe(200); + // Ends at the principal, not at the status: a 200 that installs nobody's + // session is exactly what a broken challenge looks like. + expect(await principalFor(manager, { cookie: cookieHeader(completed) })).toBe(userId); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#10700 — a re-enrolled secret is inert until it is confirmed', () => { + it('the flag describes the STORED secret, and the challenge refuses the unconfirmed one', async () => { + const { engine, manager, userId, firstSecret, sessionCookie } = await arrangeConfirmedEnrolment(); + + const reenrolled = await post( + manager, + '/two-factor/enable', + { password: PASSWORD }, + { cookie: sessionCookie }, + ); + expect(reenrolled.status, `two-factor/enable (re-enroll): ${await reenrolled.clone().text()}`).toBe(200); + const secondSecret = await secretFromEnableResponse(reenrolled); + expect( + secondSecret.equals(firstSecret), + 'the premise: re-enrolling hands out a DIFFERENT secret', + ).toBe(false); + + // ① The flag. Against the unfixed handler this read `true` — inherited + // from the enrollment that confirmed the PREVIOUS secret. + await expectVerified( + manager, + engine, + userId, + false, + 'verified must describe the secret stored beside it, not the enrollment before it', + ); + + // ② What the challenge accepts — the half the flag alone cannot show. + const { cookie, methods } = await beginChallenge(manager); + expect( + methods, + 'the challenge must not offer a factor nobody has confirmed', + ).not.toContain('totp'); + + const withNewSecret = await post(manager, '/two-factor/verify-totp', { code: totp(secondSecret) }, { cookie }); + // ADR-0112: code AND status. Against the unfixed handler this was a 200 + // that installed a full session for a secret no one had ever confirmed. + expect(await refusal(withNewSecret)).toEqual({ status: 400, code: 'TOTP_NOT_ENABLED' }); + expect( + await principalFor(manager, { cookie: cookieHeader(withNewSecret) }), + 'a refused challenge must install nobody', + ).toBeNull(); + }); + + it('confirming the re-enrolled secret makes it — and only it — live at the challenge', async () => { + const { engine, manager, userId, firstSecret, sessionCookie } = await arrangeConfirmedEnrolment(); + + const reenrolled = await post( + manager, + '/two-factor/enable', + { password: PASSWORD }, + { cookie: sessionCookie }, + ); + expect(reenrolled.status).toBe(200); + const secondSecret = await secretFromEnableResponse(reenrolled); + + // The still-works leg for rotation: the confirmation step is reachable + // with the session the caller already holds. An implementation that just + // refused `enable` never gets here. + const confirmed = await post( + manager, + '/two-factor/verify-totp', + { code: totp(secondSecret) }, + { cookie: sessionCookie }, + ); + expect(confirmed.status, `verify-totp (re-enrol confirmation): ${await confirmed.clone().text()}`).toBe(200); + await expectVerified(manager, engine, userId, true, 'confirming a rotation must flip the flag back'); + + const { cookie, methods } = await beginChallenge(manager); + expect(methods).toContain('totp'); + const completed = await post(manager, '/two-factor/verify-totp', { code: totp(secondSecret) }, { cookie }); + expect(completed.status, `verify-totp (challenge, rotated secret): ${await completed.clone().text()}`).toBe(200); + expect(await principalFor(manager, { cookie: cookieHeader(completed) })).toBe(userId); + + // And the rotation is real: the superseded secret is not a second live + // credential. Without this, "the rotation works" would be satisfiable by + // an implementation that never rotated anything. + const stale = await beginChallenge(manager); + const withOldSecret = await post(manager, '/two-factor/verify-totp', { code: totp(firstSecret) }, { cookie: stale.cookie }); + // Measured, and deliberately a DIFFERENT envelope from the one above: a + // superseded secret is now merely a wrong code (`401 INVALID_CODE`, from + // `verify-two-factor.mjs`'s `invalid()`), whereas an unconfirmed factor is + // refused by the gate before any code is checked (`400 TOTP_NOT_ENABLED`). + // Asserting the pair keeps "refused" from collapsing into one status. + expect(await refusal(withOldSecret)).toEqual({ status: 401, code: 'INVALID_CODE' }); + expect(await principalFor(manager, { cookie: cookieHeader(withOldSecret) })).toBeNull(); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The availability window, pinned as it ACTUALLY is rather than as one would +// like it to be. `/two-factor/enable` rewrites the account's single +// `sys_two_factor` row unconditionally, so the previously confirmed secret +// stops working the moment the call returns — that is true before this change +// and after it, and this fix does not claim otherwise. What the fix changes is +// WHERE the caller finds out: with a live session in hand rather than at the +// next sign-in with none. These two pins hold the floor that does exist, so a +// later change that quietly removes the recovery path turns red here. +describe('#10700 — the window between re-enrolling and confirming', () => { + it('the superseded secret is gone from the challenge, and the fresh backup codes are the way back in', async () => { + const { manager, userId, firstSecret, sessionCookie } = await arrangeConfirmedEnrolment(); + + const reenrolled = await post( + manager, + '/two-factor/enable', + { password: PASSWORD }, + { cookie: sessionCookie }, + ); + expect(reenrolled.status).toBe(200); + const { backupCodes } = (await reenrolled.clone().json()) as { backupCodes: string[] }; + expect(Array.isArray(backupCodes) && backupCodes.length > 0, 're-enrolling must issue backup codes').toBe(true); + + // The superseded secret: refused, and refused for the reason the flag + // gives — the factor is unconfirmed, not merely mistyped. + const stale = await beginChallenge(manager); + const withOldSecret = await post( + manager, + '/two-factor/verify-totp', + { code: totp(firstSecret) }, + { cookie: stale.cookie }, + ); + expect(await refusal(withOldSecret)).toEqual({ status: 400, code: 'TOTP_NOT_ENABLED' }); + + // The floor: the caller is not locked out. The codes THIS response handed + // over complete the sign-in. + const recovery = await beginChallenge(manager); + const rescued = await post( + manager, + '/two-factor/verify-backup-code', + { code: backupCodes[0] }, + { cookie: recovery.cookie }, + ); + expect(rescued.status, `verify-backup-code: ${await rescued.clone().text()}`).toBe(200); + expect( + await principalFor(manager, { cookie: cookieHeader(rescued) }), + 'the backup code issued by the re-enrollment must sign the user in', + ).toBe(userId); + }); +}); diff --git a/packages/plugins/plugin-auth/src/two-factor-reenrollment-verified-reset.ts b/packages/plugins/plugin-auth/src/two-factor-reenrollment-verified-reset.ts new file mode 100644 index 0000000000..f9887a991f --- /dev/null +++ b/packages/plugins/plugin-auth/src/two-factor-reenrollment-verified-reset.ts @@ -0,0 +1,171 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #10700 — a SECOND `/two-factor/enable` on an account that already has a + * confirmed factor rewrote the stored TOTP secret and carried `verified` over + * from the enrollment before it. + * + * ## The defect, at the seam + * + * better-auth's enable handler computes the row it is about to write as + * + * verified: existingTwoFactor != null && existingTwoFactor.verified === true + * || !!options?.skipVerificationOnEnable + * + * and, when a row already exists, `update`s that onto it + * (`dist/plugins/two-factor/index.mjs`, measured 2026-08-22 against the + * installed better-auth `1.7.1`). `secret` is the freshly generated one; + * `verified` is inherited. So the flag describes the enrollment that came + * BEFORE the secret sitting next to it. + * + * `sys_two_factor` declares `user_id` unique + * (`packages/platform-objects/src/identity/sys-two-factor.object.ts`), so this + * is an in-place rewrite of the one row the account has — there is no second + * row and no second secret. + * + * ## Why the flag alone is the whole defect here + * + * The vendor ALREADY gates the sign-in challenge on this flag, in two places + * (same dist tree): + * + * • `totp/index.mjs` — `if (isSignIn && twoFactor.verified === false) throw + * TOTP_NOT_ENABLED`, before any lockout bookkeeping; + * • `index.mjs`'s post-sign-in hook — `totp` is offered in + * `twoFactorMethods` only when `userTotpSecret.verified !== false`. + * + * Both are STRICT `false` comparisons, which hold here because `verified` is + * declared `type: 'boolean'` in the plugin's own schema and this repo's adapter + * runs with `supportsBooleans: false` (`objectql-adapter.ts`), so better-auth's + * factory converts the stored 0/1 back to a real boolean on read. + * + * That gate is exactly what a FIRST enrollment relies on: enable writes + * `verified: false`, the factor is inert at sign-in, and the session-lane + * `/two-factor/verify-totp` flips it true. Re-enrollment is the one path that + * skipped it — not because the gate is missing, but because the flag handed to + * the gate was inherited. Restoring the flag therefore restores the gate; it + * does not add a second one. A duplicate gate here would be a second owner of + * the same decision (AGENTS.md · Route & surface ownership #1). + * + * ## What this does + * + * After a successful `method: 'totp'` enable, force `verified = false` on the + * account's row. On a first enrollment the vendor already wrote `false` and + * this is a no-op; on a re-enrollment it is the correction. Nothing else about + * the endpoint changes — same request body, same response shape, same status. + * The endpoint accepts strictly LESS than it did: a secret it hands out is no + * longer live at the challenge until the caller proves possession of it. + * + * ⚠️ **Scope, stated so review can see the edge of it.** This closes the + * integrity half of the card — `verified` describes the stored secret at every + * point in the flow — and it moves the mis-scan discovery point from "the next + * sign-in, with no session" to "immediately, while the caller still holds a + * live session". It does NOT make the previous secret survive the call: enable + * rewrites the one row unconditionally, so the prior secret dies when enable + * returns, exactly as before. Keeping it alive would need somewhere to park an + * unconfirmed secret, and every way to do that either widens what + * `/two-factor/verify-totp` accepts or adds persisted state to a + * `managedBy: 'better-auth'` table — neither is a call this lane may make on + * its own. The recovery path that does hold across the window is pinned in the + * tests: the backup codes the same enable response issues complete a sign-in. + * + * ## `skipVerificationOnEnable` + * + * Deliberately honoured rather than overridden. That option means "activate + * without a confirmation step", so under it `verified: true` beside a fresh + * secret is the operator's declared intent, not a stale flag. `AuthManager` + * never sets it — the option is not exposed in this repo's plugin config — so + * this branch is defensive, not a live path. + */ + +/** The published endpoint whose write this repairs. */ +export const TWO_FACTOR_ENABLE_PATH = '/two-factor/enable'; + +/** better-auth's logical model name for `sys_two_factor`. */ +const TWO_FACTOR_MODEL = 'twoFactor'; + +/** Did this request succeed, and did it enroll a TOTP secret? */ +async function enrolledTotpSecret(ctx: any): Promise { + const returned = ctx?.context?.returned; + if (!returned || typeof returned !== 'object') return false; + try { + const { isAPIError } = await import('better-auth/api'); + if (isAPIError(returned)) return false; + } catch { + if (returned instanceof Error) return false; + } + // `method: 'otp'` enables email/SMS codes and never touches the secret row. + return (returned as any).method === 'totp'; +} + +/** Is the operator running with the confirmation step deliberately disabled? */ +function verificationDeliberatelySkipped(ctx: any): boolean { + const plugins = ctx?.context?.options?.plugins; + if (!Array.isArray(plugins)) return false; + const twoFactorPlugin = plugins.find((p: any) => p?.id === 'two-factor'); + return twoFactorPlugin?.options?.skipVerificationOnEnable === true; +} + +/** + * Force `verified = false` on the row a successful TOTP enable just wrote, so + * the flag describes the secret stored beside it rather than the enrollment + * before it. + * + * Never throws. The enrollment itself succeeded and the caller is holding the + * `totpURI` and backup codes the response handed over; turning that into a 500 + * would strand them with credentials the account may or may not have kept. + * A failure to correct the flag is loud in the log instead — this is the + * silent-data-loss class, not the noisy-degradation class, because the symptom + * of getting it wrong is an account whose second factor nobody confirmed. + */ +export async function resetVerifiedOnTwoFactorReenrollment(ctx: any): Promise { + try { + if (ctx?.path !== TWO_FACTOR_ENABLE_PATH) return; + if (!(await enrolledTotpSecret(ctx))) return; + if (verificationDeliberatelySkipped(ctx)) return; + + const userId: unknown = ctx?.context?.session?.user?.id; + if (typeof userId !== 'string' || !userId) { + console.error( + '[AuthManager] /two-factor/enable succeeded with no resolvable session user; ' + + 'could not confirm that sys_two_factor.verified describes the stored secret (#10700).', + ); + return; + } + + const adapter = ctx?.context?.adapter; + if (!adapter?.findOne || !adapter?.update) return; + + const row = await adapter.findOne({ + model: TWO_FACTOR_MODEL, + where: [{ field: 'userId', value: userId }], + }); + // #3807 — an absent row is not "not mine". A successful TOTP enable always + // writes one, so a miss means the read did not see what the handler wrote; + // say so rather than reading the null as "nothing to correct". + if (!row) { + console.error( + '[AuthManager] /two-factor/enable succeeded but no sys_two_factor row was ' + + `readable for user ${userId}; sys_two_factor.verified could not be confirmed (#10700).`, + ); + return; + } + // Skip only on the exact value the vendor's challenge gate keys on + // (`verified === false`, strict, in both `totp/index.mjs` and the + // post-sign-in hook). Anything else — `true`, an un-converted `1`, an + // absent column — is a value that gate would NOT refuse, so it gets the + // write. Fail-closed: the cost of a redundant update is one statement; the + // cost of skipping one is a live unconfirmed secret. + if (row.verified === false) return; + + await adapter.update({ + model: TWO_FACTOR_MODEL, + update: { verified: false }, + where: [{ field: 'id', value: row.id }], + }); + } catch (err: any) { + console.error( + '[AuthManager] could not reset sys_two_factor.verified after /two-factor/enable ' + + `(#10700): ${err?.message ?? err}`, + ); + } +}