From 5f6fd8be8eaa24f74b39e9394fde9db6287f397d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 00:14:28 +0000 Subject: [PATCH 1/3] wip(auth): #16535 user echo alignment + pins --- .../src/two-factor-rotated-token-echo.test.ts | 532 +++++++++++++++++- .../src/two-factor-rotated-token-echo.ts | 76 ++- 2 files changed, 594 insertions(+), 14 deletions(-) diff --git a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts index 391c12c22c..bd32b62b1e 100644 --- a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts +++ b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts @@ -27,7 +27,14 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { createHmac } from 'node:crypto'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; import { AuthManager } from './auth-manager'; +import { authIdentityObjects } from './manifest.js'; +import { + echoInstalledSessionToken, + ROTATING_TWO_FACTOR_VERIFY_PATHS, +} from './two-factor-rotated-token-echo.js'; // The SAME in-memory engine the #8243 harness drives, deliberately: a second // fake would be a second looseness risk and a new `check:engine-double-contract` // ledger entry, for no added fidelity. @@ -100,10 +107,106 @@ const post = (manager: AuthManager, path: string, body: unknown, headers: Record ); const sessionRows = (engine: any) => (engine.tables.get('sys_session') ?? []) as any[]; -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); + +/** + * [#16535] A backend to drive the arrangement against, plus the way to read the + * `sys_user` row it stores — read AT REST, below better-auth's adapter and below + * the echo being measured. + * + * The read is part of the harness rather than a helper on the side because the + * #16535 assertion is an EQUALITY between the echo and the row: a reader that + * went through the same seam the fix reads would certify the fix against itself. + */ +type EnrolmentBackend = { + engine: any; + /** The stored `sys_user` row, in the column spelling that backend stores. */ + readUserRow: (email: string) => Promise>; + /** `sys_user.two_factor_enabled` as it stands in storage, however spelled. */ + readTwoFactorEnabled: (email: string) => Promise; +}; + +/** The in-memory engine leg — the one every #10701 pin already drives. */ +const memoryBackend = (): EnrolmentBackend => { + const engine = createMemoryEngine(); + const read = async (email: string): Promise> => { + 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 row as Record; + }; + return { + engine, + readUserRow: read, + readTwoFactorEnabled: async (email) => (await read(email)).two_factor_enabled, + }; +}; + +/** + * [#16535] The real-driver leg — `ObjectQL` over `@objectstack/driver-sql` on + * better-sqlite3 `:memory:`, the backend `credential-at-rest-posture.test.ts` + * already uses. The card measured the defect twice for a reason: a test that + * never reaches an adapter cannot tell "the echo tracks the row" from "the echo + * happens to say true". + */ +const engines: ObjectQL[] = []; +const sqlBackend = async (): Promise => { + const engine = new ObjectQL(); + engines.push(engine); + engine.registerDriver( + new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), + true, + ); + await engine.init(); + for (const object of authIdentityObjects) { + engine.registry.registerObject(object as never, '@objectstack/plugin-auth'); + } + await engine.syncSchemas(); + + // Read at DRIVER level — below ObjectQL's read mask and below better-auth's + // adapter, which is the seam the fix under test reads through. + const read = async (email: string): Promise> => { + const driver = ( + engine as unknown as { getDriver(o: string): { find(o: string, q: unknown): Promise } } + ).getDriver('sys_user'); + const found = await driver.find('sys_user', { where: {} }); + const rows = (Array.isArray(found) ? found : [found]).filter(Boolean) as Record[]; + const row = rows.find((r) => r.email === email); + if (!row) throw new Error(`no sys_user row for ${email}`); + return row; + }; + return { + engine, + readUserRow: read, + readTwoFactorEnabled: async (email) => (await read(email)).two_factor_enabled, + }; +}; + +/** + * [#16535] The after-hook's own inputs, spelled out — the seam-level harness + * for the coverage and failure-posture cases the real pipeline cannot reach + * (`/two-factor/verify-otp` has no OTP transport configured here) or cannot + * reach deterministically (an adapter that throws). + */ +const fakeRotatingCtx = ( + path: string, + returned: unknown, + findUserById: (id: string) => Promise, +) => { + const headers = new Headers(); + headers.append('set-cookie', 'session_token=installed-token.sig; Path=/; HttpOnly'); + return { + path, + context: { + returned, + responseHeaders: headers, + authCookies: { sessionToken: { name: 'session_token' } }, + internalAdapter: { findUserById }, + options: {}, + }, + }; }; /** @@ -142,9 +245,21 @@ const EMAIL = 'enroller@example.com'; * Returns everything the three cases need: the token the response echoed, the * cookie it installed, and the token the caller was holding BEFORE enrolment * (the value that used to be echoed, kept so the pins can name it exactly). + * + * [#16535] Also returns the whole `verify-totp` body (its `user` member is the + * second stale echo), the backup codes `enable` minted (the negative control + * needs one), and the backend, so a pin can read the row the echo describes. */ -const arrangeCompletedEnrolment = async () => { - const engine = createMemoryEngine(); +const arrangeCompletedEnrolment = async ( + backend: EnrolmentBackend = memoryBackend(), + /** + * [#16535] Runs after `enable` and immediately before the rotating + * `verify-totp` — the only window in which the condition-④ pin can poison the + * seam the repair reads without also breaking the request under measurement. + */ + beforeVerify?: (manager: AuthManager) => Promise, +) => { + const { engine } = backend; const manager = makeManager(engine); const signedUp = await post(manager, '/sign-up/email', { @@ -152,10 +267,10 @@ const arrangeCompletedEnrolment = async () => { password: PASSWORD, name: 'Enrolling User', }); - expect(signedUp.status).toBe(200); + expect(signedUp.status, `sign-up/email: ${await signedUp.clone().text()}`).toBe(200); const preEnrolmentCookie = cookieHeader(signedUp); const preEnrolmentToken = String(((await signedUp.json()) as any).token); - const userId = userIdFor(engine, EMAIL); + const userId = String((await backend.readUserRow(EMAIL)).id); // The premise: before enrolling, the echoed token IS an accepted bearer. // Without this, a green suite could never tell "we fixed the echo" from @@ -164,26 +279,55 @@ const arrangeCompletedEnrolment = async () => { const enabled = await post(manager, '/two-factor/enable', { password: PASSWORD }, { cookie: preEnrolmentCookie }); expect(enabled.status, `two-factor/enable: ${await enabled.clone().text()}`).toBe(200); - const { totpURI } = (await enabled.json()) as { totpURI: string }; + const { totpURI, backupCodes } = (await enabled.json()) as { + totpURI: string; + backupCodes: string[]; + }; const uriSecret = new URL(totpURI.replace('otpauth://', 'https://')).searchParams.get('secret'); expect(uriSecret, 'no secret in the otpauth URI').toBeTruthy(); const secret = base32Decode(String(uriSecret)); + await beforeVerify?.(manager); + const verified = await post(manager, '/two-factor/verify-totp', { code: totp(secret) }, { cookie: preEnrolmentCookie }); expect(verified.status, `verify-totp (enrolment): ${await verified.clone().text()}`).toBe(200); - const echoedToken = String(((await verified.clone().json()) as any).token); + const body = (await verified.clone().json()) as { token: unknown; user: Record }; + const echoedToken = String(body.token); + const echoedUser = body.user; const rotatedCookie = cookieHeader(verified); expect(rotatedCookie, 'verify-totp installed no session cookie').toContain('session_token='); - return { engine, manager, userId, secret, preEnrolmentToken, echoedToken, rotatedCookie }; + return { + engine, + backend, + manager, + userId, + secret, + backupCodes, + preEnrolmentCookie, + preEnrolmentToken, + echoedToken, + echoedUser, + rotatedCookie, + }; }; beforeEach(() => { vi.spyOn(console, 'warn').mockImplementation(() => {}); vi.spyOn(console, 'error').mockImplementation(() => {}); }); -afterEach(() => vi.restoreAllMocks()); +afterEach(async () => { + vi.restoreAllMocks(); + while (engines.length) { + const e = engines.pop(); + try { + await (e as unknown as { destroy?(): Promise })?.destroy?.(); + } catch { + /* noop */ + } + } +}); // ─────────────────────────────────────────────────────────────────────────── describe('#10701 — the three cases from the card, one arrangement', () => { @@ -309,3 +453,367 @@ describe('#10701 — the sign-in-challenge lane is untouched', () => { expect(signInToken).not.toBe(rotatedCookie); }, 60_000); }); + +// ═══════════════════════════════════════════════════════════════════════════ +// #16535 — the SAME stale closure, the OTHER member of the same body. +// +// `valid(ctx)` echoes `{ token, user }` out of the session it resolved at +// ENTRY. #10701 repaired `token`; `user` still comes from the pre-rotation +// snapshot, so a successful enrolment answers `twoFactorEnabled: false` to the +// very caller who just switched it on. +// +// What makes these pins non-vacuous — and what would make them worthless: +// +// • The assertion is an EQUALITY WITH THE STORED ROW, never `toBe(true)`. +// A literal `true` passes just as well when the echo has stopped tracking +// the row altogether (the vendor hard-coding it, a blanket `user.x = true` +// in the hook), which is the very failure mode this card is about. +// • The row is read AT REST — the memory engine's own table, and at DRIVER +// level under `SqlDriver` — not through better-auth's adapter, which is the +// seam the fix itself reads. Reading the row the fix's own way would make +// the equality certify the fix against itself. +// • `verify-backup-code` is a NEGATIVE CONTROL, not decoration. It does not +// rotate and it echoes the live row correctly TODAY. An unconditional +// re-read would "fix" the broken lane and leave this one just as green — +// so it is measured on both sides of the same enrolment. +// • The failure posture is measured by making the row read THROW, not by +// reading the code: a repair that turns a completed verification into a +// 500 is a worse defect than the stale flag it set out to fix. +// ═══════════════════════════════════════════════════════════════════════════ + +/** + * `sys_user.two_factor_enabled` AS STORED, as a truth value the echo can be + * compared against. + * + * The normalisation is on the ROW side only, and only across spellings of the + * same boolean (SQLite stores `1`/`0`, the memory engine `true`/`false`); the + * echo is never normalised, and no literal is ever substituted for the row. If + * the row said "off", every caller below would demand the echo say "off" too — + * which is the difference between pinning the row and pinning `true`. The + * seam-level pin `follows the row DOWN as well as up` measures that directly. + */ +const storedTwoFactorEnabled = async (backend: EnrolmentBackend): Promise => { + const stored = await backend.readTwoFactorEnabled(EMAIL); + expect( + stored === true || stored === false || stored === 1 || stored === 0, + `sys_user.two_factor_enabled is not a boolean at rest: ${String(stored)}`, + ).toBe(true); + return stored === true || stored === 1; +}; + +/** + * The premise the card's differential rests on, asserted as a fact about + * STORAGE and never about the echo: the flag really did flip during the + * request. Without it, `echo === row` is satisfiable by "both are false" — + * which is precisely the defect. + */ +const expectRowSaysEnabled = async (backend: EnrolmentBackend): Promise => { + const value = await storedTwoFactorEnabled(backend); + expect(value, 'the enrolment never wrote the flag — the differential is gone').toBe(true); + return value; +}; + +// ─────────────────────────────────────────────────────────────────────────── +describe('#16535 — the echoed `user` describes the row, on the in-memory engine', () => { + it('condition ①: `user.twoFactorEnabled` equals the value stored for that row', async () => { + const { backend, echoedUser } = await arrangeCompletedEnrolment(); + + // The premise, stated as a fact about STORAGE, never about the echo: the + // flag really did flip during this request. Without it the equality below + // is satisfiable by "both are false", which is the bug. + const rowValue = await expectRowSaysEnabled(backend); + + expect( + echoedUser.twoFactorEnabled, + 'verify-totp echoed the PRE-rotation user snapshot', + ).toBe(rowValue); + }, 60_000); + + it('condition ③ (negative control): `verify-backup-code` still echoes the live row', async () => { + // The lane that does NOT rotate. It was right before this card and must be + // right after it — an unconditional re-read is indistinguishable from the + // targeted fix on `verify-totp` alone, and only shows itself here. + const { manager, backend, backupCodes, rotatedCookie } = await arrangeCompletedEnrolment(); + const rowValue = await expectRowSaysEnabled(backend); + + expect(Array.isArray(backupCodes) && backupCodes.length > 0, 'enable minted no backup codes').toBe(true); + const consumed = await post( + manager, + '/two-factor/verify-backup-code', + { code: backupCodes[0] }, + { cookie: rotatedCookie }, + ); + expect(consumed.status, `verify-backup-code: ${await consumed.clone().text()}`).toBe(200); + + const body = (await consumed.clone().json()) as { user: Record }; + expect(body.user.twoFactorEnabled, 'the non-rotating lane stopped echoing the live row').toBe(rowValue); + + // …and it is still the SAME body shape the vendor writes: this lane is not + // supposed to be touched by the repair at all. + expect(body.user.id).toBe((await backend.readUserRow(EMAIL)).id); + }, 60_000); + + it('the repaired `user` is the vendor shape, not a widened one', async () => { + // The repair re-serialises with better-auth's own `parseUserOutput`, so it + // cannot leak a column the vendor's own echo hides. Measured against the + // lane the repair does NOT touch: the two bodies must carry the same key + // set, or the "repair" has changed the published payload shape. + const { manager, backupCodes, rotatedCookie, echoedUser } = await arrangeCompletedEnrolment(); + + const consumed = await post( + manager, + '/two-factor/verify-backup-code', + { code: backupCodes[0] }, + { cookie: rotatedCookie }, + ); + expect(consumed.status).toBe(200); + const untouched = ((await consumed.clone().json()) as any).user as Record; + + expect(Object.keys(echoedUser).sort()).toEqual(Object.keys(untouched).sort()); + + // Shape is more than a key set. The repaired members come from a row read, + // and a row read is where a boolean becomes `1` — which would silently + // change the WIRE TYPE of `AuthWireUser.emailVerified` while every key-set + // assertion stayed green. So each member's type is compared against the + // lane the repair does not touch. + for (const key of Object.keys(untouched)) { + expect( + typeof echoedUser[key], + `\`user.${key}\` changed wire type: ${JSON.stringify(echoedUser[key])} vs ${JSON.stringify(untouched[key])}`, + ).toBe(typeof untouched[key]); + } + + // The one thing that is deliberately NOT hidden: no credential material + // rides along on either lane. + for (const forbidden of ['password', 'twoFactorSecret', 'backupCodes']) { + expect(Object.keys(echoedUser)).not.toContain(forbidden); + } + }, 60_000); +}); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#16535 — condition ②: every rotating path in the table is covered', () => { + // `/two-factor/verify-otp` travels the byte-identical rotate-then-`valid(ctx)` + // block (`otp/index.mjs`) and is already in the table the `token` repair keys + // on. It cannot be driven end-to-end here — the manager builds `twoFactor()` + // with no OTP transport, exactly as the file header records — so this is + // measured where the coverage decision actually lives: the hook, driven over + // EVERY entry of the table. A third rotating path added later is covered by + // this pin the day it is added to the table. + it.each([...ROTATING_TWO_FACTOR_VERIFY_PATHS])('repairs `user` on %s', async (path) => { + const fresh = { id: 'user_1', email: EMAIL, twoFactorEnabled: true }; + const returned: any = { + token: 'stale-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + const ctx = fakeRotatingCtx(path, returned, async () => fresh); + + await echoInstalledSessionToken(ctx); + + expect(returned.token).toBe('installed-token'); + expect(returned.user.twoFactorEnabled).toBe(true); + }); + + it('follows the row DOWN as well as up — the echo tracks the row, not a literal', async () => { + // The assertion condition ① forbids is `toBe(true)`: it passes just as well + // when the echo has stopped describing the row at all. This is the direct + // measurement of the property `true` cannot distinguish — the row says OFF + // and the echo must say OFF, on the very lane the repair rewrites. + const returned: any = { + token: 'stale-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: true }, + }; + const ctx = fakeRotatingCtx('/two-factor/verify-totp', returned, async () => ({ + id: 'user_1', + email: EMAIL, + twoFactorEnabled: false, + })); + + await echoInstalledSessionToken(ctx); + + expect(returned.user.twoFactorEnabled).toBe(false); + }); + + it('never substitutes a different principal into the response', async () => { + // The row is re-read BY THE ID THE RESPONSE ALREADY PUBLISHED. An adapter + // that answers with some other row is a bug, not an opportunity: the echo + // must stay as the vendor wrote it rather than start describing a stranger. + const returned: any = { + token: 'stale-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + const ctx = fakeRotatingCtx('/two-factor/verify-totp', returned, async () => ({ + id: 'someone_else', + email: 'intruder@example.com', + twoFactorEnabled: true, + })); + + await echoInstalledSessionToken(ctx); + + expect(returned.user.id).toBe('user_1'); + expect(returned.user.email).toBe(EMAIL); + expect(returned.user.twoFactorEnabled).toBe(false); + }); + + it('never widens the echoed payload with columns the vendor did not publish', async () => { + // better-auth's own output filter is a DENY-list, so forwarding a raw row + // would put every column it happens to carry on the wire. The echoed key + // set is the ceiling. + const returned: any = { + token: 'stale-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + const ctx = fakeRotatingCtx('/two-factor/verify-totp', returned, async () => ({ + id: 'user_1', + email: EMAIL, + twoFactorEnabled: true, + two_factor_secret: 'JBSWY3DPEHPK3PXP', + internalRiskScore: 42, + })); + + await echoInstalledSessionToken(ctx); + + expect(returned.user.twoFactorEnabled).toBe(true); + expect(Object.keys(returned.user).sort()).toEqual(['email', 'id', 'twoFactorEnabled']); + }); + + it('leaves a path OUTSIDE the table alone, `user` included', async () => { + // `/two-factor/verify-backup-code` does not rotate and is in neither list. + // This is the seam-level twin of the end-to-end negative control above. + const returned: any = { + token: 'stale-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + const ctx = fakeRotatingCtx('/two-factor/verify-backup-code', returned, async () => ({ + id: 'user_1', + email: EMAIL, + twoFactorEnabled: true, + })); + + await echoInstalledSessionToken(ctx); + + expect(returned.token).toBe('stale-token'); + expect(returned.user.twoFactorEnabled).toBe(false); + }); + + it('leaves `user` alone when no session was rotated', async () => { + // The sign-in-challenge lane: the installed token IS the echoed one, so the + // predicate is false and nothing — token or user — is rewritten. + const returned: any = { + token: 'installed-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + let reads = 0; + const ctx = fakeRotatingCtx('/two-factor/verify-totp', returned, async () => { + reads += 1; + return { id: 'user_1', email: EMAIL, twoFactorEnabled: true }; + }); + + await echoInstalledSessionToken(ctx); + + expect(returned.user.twoFactorEnabled).toBe(false); + expect(reads, 'the row was read on a lane that installed no new session').toBe(0); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#16535 — condition ④: the failure posture is inherited', () => { + it('a row read that THROWS leaves the old echo and never fails the verification', async () => { + // Measured through the REAL pipeline, because the claim is about the + // RESPONSE, not about the helper: the verification already succeeded, the + // caller's rotated cookie is valid, and a hook that cannot tidy the body + // must not convert that into a failure. + // + // `findUserById` is the seam the repair reads and — on the signed-in lane — + // the ONLY thing that reads it, so poisoning it isolates the repair from + // the request it is measuring. (`verifyTwoFactor`'s signed-in branch + // resolves through `findSession`; `findUserById` is the sign-in-challenge + // branch's.) + const arranged = await arrangeCompletedEnrolment(memoryBackend(), async (manager) => { + const auth = (await manager.getAuthInstance()) as any; + const authContext: any = await auth.$context; + authContext.internalAdapter.findUserById = async () => { + throw new Error('adapter is down'); + }; + }); + + // The arrangement itself asserts the 200 and the installed cookie — i.e. + // the verification was NOT turned into a failure. + const rowValue = await expectRowSaysEnabled(arranged.backend); + expect(rowValue).toBe(true); + + // The #10701 repair, which runs FIRST, is not lost with the #16535 one… + expect(arranged.echoedToken).not.toBe(arranged.preEnrolmentToken); + expect( + decodeURIComponent(String(/session_token=([^;]+)/.exec(arranged.rotatedCookie)?.[1])).split('.')[0], + ).toBe(arranged.echoedToken); + + // …and `user` degrades to the vendor's own echo — the pre-flip snapshot — + // rather than to a 500 or to a missing member. + expect(arranged.echoedUser, 'the user member was dropped rather than left alone').toBeTruthy(); + expect(arranged.echoedUser.id).toBe(arranged.userId); + expect( + arranged.echoedUser.twoFactorEnabled, + 'degraded to something other than the vendor echo', + ).toBe(false); + }, 60_000); + + it('a row read that throws still leaves the #10701 token repair in place', async () => { + // The two repairs are independent: #16535 must not be able to undo #10701. + const returned: any = { + token: 'stale-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + const ctx = fakeRotatingCtx('/two-factor/verify-totp', returned, async () => { + throw new Error('adapter is down'); + }); + + await expect(echoInstalledSessionToken(ctx)).resolves.toBeUndefined(); + + expect(returned.token, 'the token repair was lost with the user repair').toBe('installed-token'); + expect(returned.user.twoFactorEnabled).toBe(false); + }); + + it('a row that cannot be found leaves the vendor echo untouched', async () => { + const returned: any = { + token: 'stale-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + const ctx = fakeRotatingCtx('/two-factor/verify-totp', returned, async () => null); + + await echoInstalledSessionToken(ctx); + + expect(returned.token).toBe('installed-token'); + expect(returned.user.twoFactorEnabled).toBe(false); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#16535 — the same measurement on a real SqlDriver', () => { + it('condition ①, at driver level: the echo equals the stored column', async () => { + // The card measured this twice for a reason: a test that never reaches an + // adapter cannot tell "the echo tracks the row" from "the echo happens to + // say true". Here the row is read through the SQL driver itself, below + // ObjectQL's read mask and below better-auth's adapter. + const backend = await sqlBackend(); + const { echoedUser, backend: used } = await arrangeCompletedEnrolment(backend); + + const rowValue = await expectRowSaysEnabled(used); + expect(echoedUser.twoFactorEnabled).toBe(rowValue); + }, 120_000); + + it('condition ③, at driver level: `verify-backup-code` still echoes the live row', async () => { + const backend = await sqlBackend(); + const { manager, backupCodes, rotatedCookie, backend: used } = await arrangeCompletedEnrolment(backend); + const rowValue = await expectRowSaysEnabled(used); + + const consumed = await post( + manager, + '/two-factor/verify-backup-code', + { code: backupCodes[0] }, + { cookie: rotatedCookie }, + ); + expect(consumed.status, `verify-backup-code: ${await consumed.clone().text()}`).toBe(200); + expect(((await consumed.clone().json()) as any).user.twoFactorEnabled).toBe(rowValue); + }, 120_000); +}); diff --git a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts index 6b352697ea..9bc9d2d0c5 100644 --- a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts +++ b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts @@ -69,6 +69,21 @@ * the one this repo's plugin wiring can exercise without OTP transport config. * `/two-factor/verify-backup-code` does NOT rotate and is unaffected either * way; it is listed for neither. + * + * ## #16535 — the same stale closure, the body's OTHER member + * + * `valid(ctx)` echoes `{ token, user }` out of that one entry-time session, so + * `user` is stale for exactly the same reason `token` was. On the enrolment + * lane the vendor writes `twoFactorEnabled: true` BEFORE calling the closure, + * so a successful `/two-factor/verify-totp` reported the flag as still `false` + * to the caller who had just switched it on. Measured differentially: + * `/two-factor/verify-backup-code` in the same session — which does not rotate + * — echoed `true`, proving the row had flipped and the `false` was a snapshot. + * + * The repair is the same predicate applied to the other member, and it is + * narrowed twice over: only the members the vendor already echoed are written + * (the payload's shape is a published contract), and the row is re-read by the + * id the response itself published. See `freshEchoedUser` below. */ /** The 2FA verification routes whose vendor implementation rotates the session. */ @@ -141,8 +156,59 @@ async function echoedTokenPayload(ctx: any): Promise<{ token: string } | undefin } /** - * Repair the `token` a 2FA verification echoes, so it names the session the - * same response installed rather than the one it deleted. + * [#16535] The user the response is already describing, re-read as the row + * stands NOW — or `undefined` when it cannot be read. + * + * Two deliberate narrowings, each of which is the whole safety argument for + * one hazard: + * + * 1. **Only the members the vendor already echoed are written.** The echoed + * `user` is a published wire shape (`AuthWireUser` in `@objectstack/client`), + * and better-auth's own output filter is a DENY-list — handing the raw row + * forward would put every column the row happens to carry on the wire. So + * the echoed key set is the ceiling: this corrects VALUES, never the shape. + * 2. **The row is read through `internalAdapter`, by the id the response + * already published.** Same seam and same output transform that produced the + * echo in the first place, so a value's representation cannot drift; and + * reading by the echoed id means the repair can never substitute a different + * principal into a response — the mirror of #10701 reading its token back + * out of the response's own cookie. + */ +async function freshEchoedUser(ctx: any, echoed: unknown): Promise | undefined> { + if (!echoed || typeof echoed !== 'object') return undefined; + const id = (echoed as Record).id; + if (typeof id !== 'string' || !id) return undefined; + + const row = await ctx?.context?.internalAdapter?.findUserById?.(id); + if (!row || typeof row !== 'object' || (row as Record).id !== id) return undefined; + + const fresh = row as Record; + const repaired: Record = { ...(echoed as Record) }; + for (const key of Object.keys(repaired)) { + if (Object.hasOwn(fresh, key)) repaired[key] = fresh[key]; + } + return repaired; +} + +/** + * Repair what a 2FA verification echoes, so the body describes the state the + * same response installed rather than the state it left behind. + * + * Two members, one defect, one predicate: + * + * - `token` (#10701) named the session row the route had just DELETED. + * - `user` (#16535) is the PRE-rotation snapshot of the caller. On the + * enrolment lane the vendor writes `twoFactorEnabled: true` and only then + * calls the `valid(ctx)` closure it built at entry, so a successful + * enrolment answers `twoFactorEnabled: false` to the user who just switched + * 2FA on. `/two-factor/verify-otp` carries the byte-identical block, which is + * why the repair keys on the path TABLE and not on one route. + * + * Both are gated on the same mechanism — the response staged a session cookie + * whose token differs from the one echoed — so the sign-in-challenge lane, + * where `valid()` mints the session it echoes, stays a byte-for-byte no-op, and + * `/two-factor/verify-backup-code`, which does not rotate and already echoes + * the live row, is in neither list and is not read, let alone rewritten. * * A no-op for every other path, for a failed verification, for a response that * installs no session cookie, and — the common case — whenever the echoed token @@ -152,6 +218,9 @@ async function echoedTokenPayload(ctx: any): Promise<{ token: string } | undefin * a failure because the response body could not be tidied; the caller's cookie * is valid either way, and this repair only widens which credentials from the * response work. Any unexpected shape therefore leaves the payload untouched. + * That posture is inherited by the row read: an adapter that throws or answers + * nothing degrades to the vendor's own echo — never to a failed verification, + * and never to a lost `token` repair, which is written first for that reason. */ export async function echoInstalledSessionToken(ctx: any): Promise { try { @@ -161,6 +230,9 @@ export async function echoInstalledSessionToken(ctx: any): Promise { const installed = await installedSessionToken(ctx); if (!installed || installed === payload.token) return; payload.token = installed; + + const fresh = await freshEchoedUser(ctx, (payload as Record).user); + if (fresh) (payload as Record).user = fresh; } catch { /* leave the payload exactly as the vendor route wrote it */ } From 7ad71ef3da1dc38e0e3aeb3a237ad85071bdd5f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 00:40:17 +0000 Subject: [PATCH 2/3] wip(auth): #16535 lib-target fix, changeset, client doc --- .../two-factor-verify-echoes-live-user-row.md | 16 +++++++++++ packages/client/src/index.ts | 17 ++++++----- .../src/two-factor-rotated-token-echo.test.ts | 28 +++++++++++++++++++ .../src/two-factor-rotated-token-echo.ts | 4 ++- 4 files changed, 55 insertions(+), 10 deletions(-) create mode 100644 .changeset/two-factor-verify-echoes-live-user-row.md diff --git a/.changeset/two-factor-verify-echoes-live-user-row.md b/.changeset/two-factor-verify-echoes-live-user-row.md new file mode 100644 index 0000000000..f6a18df22c --- /dev/null +++ b/.changeset/two-factor-verify-echoes-live-user-row.md @@ -0,0 +1,16 @@ +--- +"@objectstack/plugin-auth": patch +"@objectstack/client": patch +--- + +`POST /two-factor/verify-totp` and `/two-factor/verify-otp` now echo the user row as it stands when the response is written, instead of the pre-rotation snapshot the vendor closes over. + +On the enrolment lane — a signed-in caller confirming a new factor — better-auth writes `twoFactorEnabled: true`, rotates the session, and only then calls the `valid(ctx)` closure it built at entry. That closure still holds the pre-rotation session, so a successful verification answered `user.twoFactorEnabled: false` to the very caller who had just switched 2FA on. An account portal reading that body renders the factor as still OFF right after enrolment, and a bearer client that caches the echoed user carries the wrong flag until its next `get-session`. + +`two-factor-rotated-token-echo` already repaired the body's other stale member, `token`, on exactly these routes and on exactly this predicate — the response staged a session cookie whose token differs from the one echoed. The `user` member is stale for the same reason, so it is repaired under the same predicate rather than a new one. + +- **Two narrowings, both load-bearing.** Only the members the vendor already echoed are written, so the published payload shape (`AuthWireUser`) cannot widen — better-auth's own output filter is a deny-list, and forwarding a raw row would put every column it happens to carry on the wire. And the row is re-read through `internalAdapter` by the id the response itself published, so the repair travels the same output transform that produced the echo (a driver that stores booleans as `1`/`0` cannot change a member's wire type) and can never substitute a different principal into a response. +- **`/two-factor/verify-backup-code` is untouched.** It does not rotate and already echoed the live row; it is in neither path list, its row is not read, and it is pinned as a negative control on both the in-memory engine and a real `SqlDriver` — an unconditional re-read would have "fixed" the broken lane and quietly rewritten one that was already right. +- **The failure posture is inherited.** A row read that throws or answers nothing degrades to the vendor's own echo, never to a failed verification and never to a lost `token` repair, which is written first for that reason. + +`@objectstack/client` drops the `AuthTwoFactorVerificationResult.user` warning that told callers to re-read the session for the live flag; the wire shape it declares is unchanged. diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 82613482f8..f36c361269 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -1068,11 +1068,7 @@ export interface AuthWireUser { createdAt: string; /** ISO-8601. */ updatedAt: string; - /** - * `twoFactor` plugin only. ⚠️ On the enrolment lane of `verifyTotp` this - * is echoed from the pre-flip snapshot — see - * {@link AuthTwoFactorVerificationResult}. - */ + /** `twoFactor` plugin only. */ twoFactorEnabled?: boolean; /** * `admin` plugin only. An open string: the vocabulary is the deployment's @@ -1160,10 +1156,13 @@ export interface AuthTwoFactorVerificationResult { */ token: string; /** - * ⚠️ On the ENROLMENT lane of `verifyTotp` the vendor echoes the user from - * its pre-rotation snapshot, so `twoFactorEnabled` reads `false` here - * although the flag has just flipped server-side (measured on a real SQL - * driver). Re-read the session for the live value. + * The caller, as the row stands when the response is written. The vendor + * echoes the user from its PRE-rotation snapshot on the enrolment lane, so + * `twoFactorEnabled` used to read `false` here although the flag had just + * flipped server-side; plugin-auth's `two-factor-rotated-token-echo` + * repairs that member from the row on the same rotating routes it repairs + * `token` on, so no second read is needed. The payload's shape is + * unchanged — the repair corrects values only. */ user: AuthWireUser; } diff --git a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts index bd32b62b1e..7ef6983efc 100644 --- a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts +++ b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts @@ -816,4 +816,32 @@ describe('#16535 — the same measurement on a real SqlDriver', () => { expect(consumed.status, `verify-backup-code: ${await consumed.clone().text()}`).toBe(200); expect(((await consumed.clone().json()) as any).user.twoFactorEnabled).toBe(rowValue); }, 120_000); + + it('the repaired `user` keeps the vendor wire shape AND its wire types on SQL', async () => { + // This is the leg where a shape regression would actually appear: SQLite + // stores booleans as `1`/`0`, so a repair that forwarded row values without + // travelling the adapter's output transform would put a NUMBER where + // `AuthWireUser.emailVerified` declares a boolean — invisible to any + // assertion about `twoFactorEnabled` alone. + const backend = await sqlBackend(); + const { manager, backupCodes, rotatedCookie, echoedUser } = await arrangeCompletedEnrolment(backend); + + const consumed = await post( + manager, + '/two-factor/verify-backup-code', + { code: backupCodes[0] }, + { cookie: rotatedCookie }, + ); + expect(consumed.status).toBe(200); + const untouched = ((await consumed.clone().json()) as any).user as Record; + + expect(Object.keys(echoedUser).sort()).toEqual(Object.keys(untouched).sort()); + for (const key of Object.keys(untouched)) { + expect( + typeof echoedUser[key], + `\`user.${key}\` changed wire type: ${JSON.stringify(echoedUser[key])} vs ${JSON.stringify(untouched[key])}`, + ).toBe(typeof untouched[key]); + } + expect(typeof echoedUser.twoFactorEnabled, 'the repaired member is not a JSON boolean').toBe('boolean'); + }, 120_000); }); diff --git a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts index 9bc9d2d0c5..f7c9df74a7 100644 --- a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts +++ b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts @@ -185,7 +185,9 @@ async function freshEchoedUser(ctx: any, echoed: unknown): Promise; const repaired: Record = { ...(echoed as Record) }; for (const key of Object.keys(repaired)) { - if (Object.hasOwn(fresh, key)) repaired[key] = fresh[key]; + // `hasOwnProperty.call`, not `in`: a member the row does not carry keeps + // the value the vendor echoed, and no prototype member is ever adopted. + if (Object.prototype.hasOwnProperty.call(fresh, key)) repaired[key] = fresh[key]; } return repaired; } From 1e021adc3b8979cced934bf186d8348f09a09d5a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 00:46:21 +0000 Subject: [PATCH 3/3] fix(auth): echo the live user row from the rotating 2FA verify routes On the enrolment lane better-auth writes `twoFactorEnabled: true`, rotates the session and only then calls the `valid(ctx)` closure it built at entry, so a successful `/two-factor/verify-totp` answered `user.twoFactorEnabled: false` to the caller who had just switched 2FA on. `two-factor-rotated-token-echo` already repaired the body's other stale member, `token`, on exactly these routes and on exactly this predicate; `user` is stale for the same reason and is now repaired under the same one. Narrowed twice: only the members the vendor already echoed are written, so the published payload shape cannot widen; and the row is re-read through `internalAdapter` by the id the response itself published, so the repair travels the same output transform that produced the echo and can never substitute a different principal. `/two-factor/verify-backup-code` does not rotate, is in neither path list, and is pinned as a negative control on both drive legs. A row read that throws degrades to the vendor's own echo, never to a failed verification and never to a lost `token` repair. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- .../src/two-factor-rotated-token-echo.test.ts | 87 +++++++++++-------- 1 file changed, 49 insertions(+), 38 deletions(-) diff --git a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts index 7ef6983efc..4343e86f39 100644 --- a/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts +++ b/packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.test.ts @@ -614,6 +614,55 @@ describe('#16535 — condition ②: every rotating path in the table is covered' expect(returned.user.twoFactorEnabled).toBe(true); }); + it('leaves a path OUTSIDE the table alone, `user` included', async () => { + // `/two-factor/verify-backup-code` does not rotate and is in neither list. + // This is the seam-level twin of the end-to-end negative control above. + const returned: any = { + token: 'stale-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + const ctx = fakeRotatingCtx('/two-factor/verify-backup-code', returned, async () => ({ + id: 'user_1', + email: EMAIL, + twoFactorEnabled: true, + })); + + await echoInstalledSessionToken(ctx); + + expect(returned.token).toBe('stale-token'); + expect(returned.user.twoFactorEnabled).toBe(false); + }); + + it('leaves `user` alone when no session was rotated', async () => { + // The sign-in-challenge lane: the installed token IS the echoed one, so the + // predicate is false and nothing — token or user — is rewritten. + const returned: any = { + token: 'installed-token', + user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, + }; + let reads = 0; + const ctx = fakeRotatingCtx('/two-factor/verify-totp', returned, async () => { + reads += 1; + return { id: 'user_1', email: EMAIL, twoFactorEnabled: true }; + }); + + await echoInstalledSessionToken(ctx); + + expect(returned.user.twoFactorEnabled).toBe(false); + expect(reads, 'the row was read on a lane that installed no new session').toBe(0); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#16535 — the two narrowings the repair is built on', () => { + // Neither is decoration: ablating the key-set ceiling reddens exactly ONE pin + // in this file — the widening one — while condition ① and both + // `verify-backup-code` controls stay green. That single pin is the whole + // difference between this repair and a blanket "re-read the row and forward + // it", which is why it is measured at the seam rather than only end to end + // (the fixture rows happen to carry no surplus column, so the end-to-end + // parity pins cannot see it). + it('follows the row DOWN as well as up — the echo tracks the row, not a literal', async () => { // The assertion condition ① forbids is `toBe(true)`: it passes just as well // when the echo has stopped describing the row at all. This is the direct @@ -676,44 +725,6 @@ describe('#16535 — condition ②: every rotating path in the table is covered' expect(returned.user.twoFactorEnabled).toBe(true); expect(Object.keys(returned.user).sort()).toEqual(['email', 'id', 'twoFactorEnabled']); }); - - it('leaves a path OUTSIDE the table alone, `user` included', async () => { - // `/two-factor/verify-backup-code` does not rotate and is in neither list. - // This is the seam-level twin of the end-to-end negative control above. - const returned: any = { - token: 'stale-token', - user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, - }; - const ctx = fakeRotatingCtx('/two-factor/verify-backup-code', returned, async () => ({ - id: 'user_1', - email: EMAIL, - twoFactorEnabled: true, - })); - - await echoInstalledSessionToken(ctx); - - expect(returned.token).toBe('stale-token'); - expect(returned.user.twoFactorEnabled).toBe(false); - }); - - it('leaves `user` alone when no session was rotated', async () => { - // The sign-in-challenge lane: the installed token IS the echoed one, so the - // predicate is false and nothing — token or user — is rewritten. - const returned: any = { - token: 'installed-token', - user: { id: 'user_1', email: EMAIL, twoFactorEnabled: false }, - }; - let reads = 0; - const ctx = fakeRotatingCtx('/two-factor/verify-totp', returned, async () => { - reads += 1; - return { id: 'user_1', email: EMAIL, twoFactorEnabled: true }; - }); - - await echoInstalledSessionToken(ctx); - - expect(returned.user.twoFactorEnabled).toBe(false); - expect(reads, 'the row was read on a lane that installed no new session').toBe(0); - }); }); // ───────────────────────────────────────────────────────────────────────────