Skip to content

fix(auth): the rotating 2FA verify routes echo the live user row, not the pre-rotation snapshot - #17000

Merged
os-trump merged 4 commits into
mainfrom
claude/issue-16535-two-factor-user-echo
Sep 9, 2026
Merged

fix(auth): the rotating 2FA verify routes echo the live user row, not the pre-rotation snapshot#17000
os-trump merged 4 commits into
mainfrom
claude/issue-16535-two-factor-user-echo

Conversation

@os-trump

@os-trump os-trump commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16535

Clause-②: no

On the enrolment lane, better-auth's verifyTwoFactor resolves the caller's session once, at entry, and closes over it. /two-factor/verify-totp then writes twoFactorEnabled: true, mints a new session, installs it and deletes the old row — and only afterwards calls that closure, which serialises parseUserOutput(session.user) from the pre-rotation snapshot. So a successful enrolment answered user.twoFactorEnabled: false to the very caller who had just switched 2FA on.

two-factor-rotated-token-echo.ts (#10701) 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. user is stale for the same reason, so it is repaired under the same predicate rather than a new one. No new decision logic.

What changed

packages/plugins/plugin-auth/src/two-factor-rotated-token-echo.ts

  • freshEchoedUser(ctx, echoed) re-reads the row and returns the echo with its values refreshed.
  • echoInstalledSessionToken applies it after the token repair, inside the same try/catch.

Two 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 (filterOutputFields) is a deny-list — forwarding the raw row would put every column it happens to carry on the wire. The echoed key set is the ceiling: the repair corrects values, never the shape.
  2. The row is read through internalAdapter.findUserById, by the id the response already published. Same seam and same output transform that produced the echo, so a driver that stores booleans as 1/0 cannot change a member's wire type; and reading by the echoed id means the repair can never substitute a different principal — the mirror of [security-sensitive] 2FA verify-totp token is not accepted on the bearer seam and overrides a valid cookie (fail-closed lockout) #10701 reading its token back out of the response's own cookie.

packages/client/src/index.ts drops the AuthTwoFactorVerificationResult.user warning that told callers to re-read the session for the live flag, and the matching note on AuthWireUser.twoFactorEnabled. The declared wire shape is unchanged.

验收备注

Triage's four conditions, verbatim, each with the evidence that answers it.

1. In one successful verify-totp response, user.twoFactorEnabled must equal the real value of that row in the adapter. ⛔ Assert against the row you read back — not against a literal true.

Two pins, one per drive leg, both reading the row at rest — the memory engine's own table, and at driver level under SqlDriver — deliberately not through better-auth's adapter, which is the seam the fix itself reads (that would certify the fix against itself):

  • condition ①: user.twoFactorEnabled equals the value stored for that row (in-memory engine)
  • condition ①, at driver level: the echo equals the stored column (real ObjectQL + SqlDriver on better-sqlite3)

The expected value is computed from a fresh read of the row, never written as a literal; the only normalisation is on the row side, across spellings of the same boolean (SQLite 1/0 vs true/false), and the echo is never normalised. The property a literal true cannot distinguish is measured directly by follows the row DOWN as well as up — the echo tracks the row, not a literal: the row says OFF and the echo must say OFF, on the very lane the repair rewrites.

2. verify-otp travels the same vendor rotation block and is already in the same path table. ⛔ Do not leave a known same-shaped hole.

Verified in the installed artifact: otp/index.mjs carries the byte-identical updateUsercreateSessionsetSessionCookiedeleteSessionvalid(ctx) block. The repair keys on ROTATING_TWO_FACTOR_VERIFY_PATHS, so it covers both entries by construction, and the pin is table-driven — it.each([...ROTATING_TWO_FACTOR_VERIFY_PATHS])('repairs user on %s') — so a third rotating path added later is covered the day it enters the table. It is measured at the hook rather than end to end because this manager builds twoFactor() with no OTP transport, exactly as the module header already records.

3. verify-backup-code must be unaffected — the negative control against a blanket re-read.

It does not rotate, is in neither path list, and its row is never read. Pinned on both drive legs, in the same session as the enrolment it follows:

  • condition ③ (negative control): verify-backup-code still echoes the live row
  • condition ③, at driver level: verify-backup-code still echoes the live row
  • plus the seam-level twin leaves a path OUTSIDE the table alone, user included

That the pairing is doing work is measured, not asserted — see ablation A below: the user alignment is removed and these three stay green while six others go red.

4. Keep the file's failure posture — a failure while tidying the body must ⛔ never turn an already-successful verification into a failure.

Measured through the real pipeline, because the claim is about the response and not about the helper: findUserById is poisoned to throw in the window between enable and the rotating verify-totp (on the signed-in lane it is the only thing that reads it, so the poison isolates the repair from the request it measures). The verification still answers 200, the token repair — which runs first — survives, and user degrades to the vendor's own pre-flip echo rather than to a 500 or a missing member. Two seam-level pins cover the other degradations: a throwing read and a row that cannot be found.

Verification

Both drive legs, before and after, on a tree fetched here (origin/main b834b48e7, merged forward to fe2b7554a):

leg before after
in-memory engine, verify-totp echo vs stored row false vs stored truestale true vs stored true
real SqlDriver (better-sqlite3) through real ObjectQL, at driver level false vs stored column truestale true vs stored true
verify-backup-code on both legs (control) live row — correct live row — still correct

The "before" column is the ablation reading, not a recollection: the assertion text under ablation A is verify-totp echoed the PRE-rotation user snapshot: expected false to be true.

Anchors, re-measured on a tree fetched here

Triage's readings reproduced against the commit object at origin/main b834b48e7 — not against a commit id copied from a comment:

git grep -c twoFactorEnabled origin/main -- .../two-factor-rotated-token-echo.ts   ->  0   (exit 1, no matches)
git grep -c token            origin/main -- .../two-factor-rotated-token-echo.ts   -> 21
git grep -l twoFactorEnabled origin/main -- packages/plugins/plugin-auth/src/      ->  5 files
        (auth-manager.test.ts, auth-manager.ts, auth-schema-config.ts,
         managed-extension-fields.test.ts, objectql-adapter.test.ts)

The zero is a real zero: the positive control answers 21 on the same file and the repo control proves the identifier is spelled that way and does exist in the package.

Ablation — three legs, one per enforcement point

Each leg: mutate, prove the mutation on disk (anchor grep counts plus git hash-object differing from the HEAD blob, with a no-op mutation refused outright), run the pins, restore with git checkout HEAD -- ABSPATH under a trap ... EXIT INT TERM, and prove restoration by blob equality and a zero-byte git diff HEAD. HEAD blob throughout: f7c9df74a73ce6a4203badc5ae82a00eb1fa2e16.

A — remove the user alignment. Mutated blob 49c346e836; anchor const fresh = await freshEchoedUser 1 to 0, planted marker 0 to 1. 6 failed, 27 passed. Red: condition ① on both drive legs, both table paths, the follows-the-row-down pin, the no-widening pin. Green: both verify-backup-code controls, the vendor-shape parity pins, and all six #10701 pins. That pairing is the whole point — a blanket re-read would have reddened the controls too.

B — remove the failure posture (catch rethrows). Mutated blob 7fb0d730b3. 2 failed, 31 passed, with AssertionError: verify-totp (enrolment): expected 500 to be 200 — condition 4 measured rather than asserted: without the posture, an adapter that throws turns a completed verification into a 500.

C — remove the key-set ceiling (blanket forward of the row). Mutated blob f2062771cc. Exactly 1 failed, 32 passed: only never widens the echoed payload with columns the vendor did not publish. Condition ① and both verify-backup-code controls stay green — which is precisely the difference between this repair and "re-read the row and forward it", and why that pin is written at the seam (the fixture rows carry no surplus column, so the end-to-end parity pins cannot see it).

All three restored clean: blob back to f7c9df74a7, git diff HEAD 0 bytes.

Gates

Union derived with scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack and reconciled with --ran, on the final head after merging origin/main (git rev-parse --short HEAD at the union run: 6d19f4839):

✓ dispatch-gates --ran: 59 derived famil(ies) accounted for — 59 run, 0 NOT-MEASURED.
  • The first pass produced three PREREQUISITE NOT MET answers (check:skill-examples exit 1, check:dual-build-cjs-loads exit 3, check:type-check-debt exit 3). Those are not passes and are not reported as any: the closure named by each was built (turbo run build --filter='./packages/*' --filter='./packages/*/*') and all three re-ran to a real exit 0.
  • pnpm check:route-envelope run explicitly (it sits in the silent bucket, dispatch-gates: a whole-tree-walk gate whose workflow names: lists only its CURRENT members is placed Silent, so it is never derived for the card that adds a new member — measured on check:route-envelope / PR #16730 #16828): exit 0. It did not ask for a module declaration — this change writes no response body, it corrects one member of the vendor's own returned object inside an after-hook, and plugin-auth's route file is already ruled exempt/vendor wire. No vendorWire, no exempt, no const hoist: maintainer-only territory was not entered.
  • Five roster gates whose allowlist sits under a path in this diff, read rather than assumed silent: check:auth-mount-ledger, check:error-code-casing, check:authz-resolver, check:filter-alias-parity, check-changeset-fixed — all exit 0.
  • pnpm lint over the whole repo, unnarrowed: 6391 files, 0 errors, 0 warnings, exit 0.
  • pnpm --filter @objectstack/plugin-auth --filter @objectstack/client typecheck — exit 0.
  • pnpm --filter @objectstack/plugin-auth test106 files, 2231 tests passed. pnpm --filter @objectstack/client test35 files, 454 tests passed.
  • Every exit code captured before any pipe.

Out of scope, noted not filed

  • The module is still named two-factor-rotated-token-echo although it now repairs two members. Renaming it touches the import in auth-manager.ts, the cross-reference in packages/client, and two sibling test headers, for no behaviour change — a move, not a fix, and not a rider on this card.
  • arrangeCompletedEnrolment now returns preEnrolmentCookie, which no pin reads yet; it is the natural handle for a future non-rotating case and costs nothing.

Authored by Claude Code — session https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37


Generated by Claude Code

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
@github-actions github-actions Bot added the size/l label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

4 anchor(s) derived from 2 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 2fd714f4fb21be92186cec5c0259dadbe2ffa966packageMentionDocs.

Which tree this was computed on

This run read content/docs from 083f74e29175dd81bf9dd0994bdb168bcd76f026 — the merge of head 6d19f483905542a0a08e5a9d0591e6b3dd2f8696 into base 2fd714f4fb21be92186cec5c0259dadbe2ffa966, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 083f74e29175dd81bf9dd0994bdb168bcd76f026 && git checkout 083f74e29175dd81bf9dd0994bdb168bcd76f026
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 2fd714f4fb21be92186cec5c0259dadbe2ffa966 6d19f483905542a0a08e5a9d0591e6b3dd2f8696 && git checkout -B drift-repro 2fd714f4fb21be92186cec5c0259dadbe2ffa966 && git merge --no-ff 6d19f483905542a0a08e5a9d0591e6b3dd2f8696

node scripts/docs-audit/affected-docs.mjs --json 2fd714f4fb21be92186cec5c0259dadbe2ffa966

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 9, 2026
@os-trump
os-trump marked this pull request as ready for review September 9, 2026 01:23
@os-trump
os-trump enabled auto-merge September 9, 2026 01:23
@os-trump
os-trump added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit ab1c585 Sep 9, 2026
46 of 47 checks passed
@os-trump
os-trump deleted the claude/issue-16535-two-factor-user-echo branch September 9, 2026 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants