Skip to content

Commit d533029

Browse files
committed
fix(plugin-auth): 2FA verification echoes the session it installed (#10701)
`/two-factor/verify-totp` answered 200 with two credentials that disagreed: the `Set-Cookie` named the caller's rotated session, while the JSON `token` named the session row the same request had just deleted. better-auth's `verifyTwoFactor` resolves the caller's session once at entry and closes over it, so `valid(ctx)` echoes the PRE-rotation token on the enrolment lane, where the route rotates the session before answering. Because `bearer()` overwrites the request's session cookie with whatever the Authorization header carries, a client that stored the echoed token did not merely fail to authenticate with it -- presenting it destroyed the still-valid rotated cookie and dropped the request to anonymous. The echoed value is now read back out of the response's own session cookie. Shape and meaning of the field are unchanged; only the value moves, from a deleted row to the live one. Resolver precedence is untouched, and two pins hold that line: anonymous is still refused, and a bogus bearer still overrides a valid cookie and still fails closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
1 parent 376b3dc commit d533029

4 files changed

Lines changed: 554 additions & 0 deletions

File tree

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
---
4+
5+
fix(plugin-auth): a 2FA verification echoes the session it INSTALLED, not the one it deleted (#10701)
6+
7+
`POST /api/v1/auth/two-factor/verify-totp` answered `200` with two credentials
8+
that disagreed. The `Set-Cookie` named the caller's new session; the JSON
9+
`token` named a session row the same request had just deleted.
10+
11+
The cause is upstream and mechanical. better-auth's `verifyTwoFactor` helper
12+
resolves the caller's session once, at entry, and closes over it:
13+
14+
```js
15+
valid: async (ctx) => ctx.json({ token: session.session.token, ... })
16+
```
17+
18+
On the enrolment lane — a signed-in user confirming a new TOTP factor — the
19+
route rotates that session before it answers: it mints a new session, installs
20+
it with `setSessionCookie`, and deletes the caller's original session row. Only
21+
then does it call `valid(ctx)`, which still holds the pre-rotation session and
22+
echoes the token of the row that no longer exists. (Measured on the installed
23+
better-auth 1.7.1: `dist/plugins/two-factor/verify-two-factor.mjs` and
24+
`dist/plugins/two-factor/totp/index.mjs`.)
25+
26+
Every other auth response in this repo echoes `token` as the unsigned token of
27+
a live session, and `bearer()` accepts exactly that — presented without a
28+
signature it signs the value itself before verifying. Measured on
29+
`/sign-up/email`, the body's `token` resolves to the user as a bearer. So a
30+
client following that contract after enrolling in 2FA stored a revoked token.
31+
32+
That did not merely fail to authenticate. `bearer()`'s before-hook OVERWRITES
33+
the request's session cookie with whatever the `Authorization` header carries,
34+
so a request presenting the still-valid rotated cookie *and* the dead token
35+
resolved to nobody. Measured before the fix, on one enrolment: the cookie alone
36+
resolved to the user (`get-totp-uri` `200`); the echoed token alone resolved to
37+
nobody (`get-session` `200` and empty, `get-totp-uri` `401`); and the two
38+
together also resolved to nobody (`401`). Fail-closed — no privilege was
39+
available to gain — but a legitimate user was locked out of a session they
40+
still held, which is the point of the report.
41+
42+
The echoed value is now read back out of the response's own session cookie, so
43+
the `token` names the session the response actually installed. This restores
44+
the contract rather than changing it: the field keeps its shape (the unsigned
45+
session token) and its meaning ("the session you now hold"), and only the value
46+
moves, from a deleted row to the live one. Shipped as `patch` for that reason —
47+
no consumer expression has to be rewritten, and the previous value was not a
48+
usable credential for anything, so nothing could have depended on it.
49+
50+
The repair is keyed on the mechanism, not on the enrolment branch: it applies
51+
only when the response staged a session cookie whose token differs from the one
52+
being echoed. On the sign-in-challenge lane, where the route mints the session
53+
it echoes, the two agree and this is a no-op — pinned, along with the cookie
54+
lane, so that fixing the broken lane could not quietly rewrite the others.
55+
`/two-factor/verify-otp` carries the byte-identical rotate-then-answer block and
56+
is covered by the same guard; `/two-factor/verify-backup-code` does not rotate
57+
and is unaffected.
58+
59+
Resolver precedence is deliberately untouched. Having the resolver fall back to
60+
the cookie when a bearer is unusable was the other repair direction named in the
61+
report, and it was ruled out of scope: it would stop an invalid credential from
62+
failing loud. Two pins hold that line — anonymous is still refused, and a bogus
63+
bearer still overrides a valid cookie and still fails closed — so an attempt to
64+
loosen the resolver later reddens this suite instead of passing it.

‎packages/plugins/plugin-auth/src/auth-manager.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import {
3737
rotateCallerBearerOnImpersonation,
3838
withBearerAdminSessionRecovery,
3939
} from './impersonation-bearer-rotation.js';
40+
import { echoInstalledSessionToken } from './two-factor-rotated-token-echo.js';
4041
import {
4142
applyPlatformAdminImpersonation,
4243
} from './admin-impersonate-endpoint.js';
@@ -1704,6 +1705,17 @@ export class AuthManager {
17041705
// rotating that token, `/admin/impersonate-user` is a 200 no-op.
17051706
await rotateCallerBearerOnImpersonation(ctx);
17061707

1708+
// ── #10701: a 2FA verification must echo the session it INSTALLED ─
1709+
// On the enrolment lane `/two-factor/verify-totp` rotates the
1710+
// caller's session and then echoes the PRE-rotation token — the row
1711+
// it just deleted. A client that stores it (the console's
1712+
// `auth-session-token` pattern) is holding a revoked credential, and
1713+
// because `bearer()` overwrites the request cookie with whatever the
1714+
// Authorization header carries, presenting it destroys the valid
1715+
// rotated cookie too. See `two-factor-rotated-token-echo.ts`. This
1716+
// corrects the echoed VALUE only; resolver precedence is untouched.
1717+
await echoInstalledSessionToken(ctx);
1718+
17071719
// ── ADR-0069 D2: account lockout (counter) ──────────────────
17081720
// better-auth catches an INVALID_EMAIL_OR_PASSWORD APIError and runs
17091721
// the after-hook with it on `ctx.context.returned`; a success leaves

0 commit comments

Comments
 (0)