From 435943c704787f615f5784196aeb8e23a9f4f1c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 12:57:48 +0000 Subject: [PATCH 1/2] fix(plugin-auth): rotate the caller's bearer on impersonation, and recover the admin via bearer on exit (#8243) better-auth's bearer() plugin authenticates by overwriting the request's session cookie with the bearer token; /admin/impersonate-user hands the impersonation session over AS a cookie. For any bearer client those two collide: the caller's unchanged Authorization header keeps being converted back into the ADMIN's session, so impersonation returned 200 and did nothing, with every subsequent write attributed to the admin. Rotation, not refusal (maintainer ruling, 2026-08-13): a bearer- authenticated impersonation now invalidates the token the caller holds and hands back a recovery credential, so the impersonated session is the only one that resolves afterwards. stop-impersonating honours that credential on x-admin-session-token, because resolving the admin through the admin_session cookie alone leaves the exit path dead in cookie-blocked deployments. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- .changeset/impersonation-bearer-rotation.md | 53 +++ .../plugins/plugin-auth/src/auth-manager.ts | 47 ++ .../src/impersonation-bearer-rotation.test.ts | 417 ++++++++++++++++++ .../src/impersonation-bearer-rotation.ts | 324 ++++++++++++++ 4 files changed, 841 insertions(+) create mode 100644 .changeset/impersonation-bearer-rotation.md create mode 100644 packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts create mode 100644 packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts diff --git a/.changeset/impersonation-bearer-rotation.md b/.changeset/impersonation-bearer-rotation.md new file mode 100644 index 0000000000..6d6a3e144b --- /dev/null +++ b/.changeset/impersonation-bearer-rotation.md @@ -0,0 +1,53 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): impersonation actually takes effect for bearer clients — rotate the caller's token, and let `stop-impersonating` recover the admin via bearer (#8243) + +`POST /api/v1/auth/admin/impersonate-user` answered **HTTP 200 and did nothing** +for every bearer-authenticated client — the console after every normal sign-in, +and every deployment where cookies are blocked, which is the exact context +better-auth's `bearer()` plugin exists for. + +Two correct pieces of better-auth collided. `bearer()` authenticates a request by +**overwriting the request's session cookie** with the bearer token. The admin +plugin's impersonation route does the opposite: it mints the impersonation +session and hands it over **as a cookie**, parking the admin's own session token +in a signed `admin_session` cookie for the way back. A browser composes those +two; a bearer client cannot. The client kept replaying its unchanged +`Authorization: Bearer` header, that header kept being converted back into the +**admin's** session, and the impersonation cookie was never read. + +Nothing reported this. The endpoint returned success, an impersonation session +row existed, and every subsequent request — including every write, since the +framework's data routes resolve identity through the same seam — was attributed +to the **admin** rather than the impersonated user. + +**Impersonation now rotates the caller's credential.** When the caller +authenticated with a bearer, the token it holds is invalidated as part of +impersonating: a rotated admin session is minted, the caller is handed it as a +recovery credential, and the original admin session is deleted. Afterwards the +only token that resolves is the impersonated one better-auth already emits on +`set-auth-token`. A client that adopts the rotation is the impersonated +principal; a client that ignores it gets a loud 401 on its next request. +"Impersonation succeeded but did not take effect" is no longer expressible. + +Refusing bearer-authenticated impersonation was considered and rejected: it +would leave cookie-blocked deployments unable to impersonate at all. + +**The exit path ships with it.** `POST /admin/stop-impersonating` resolved the +admin through the `admin_session` **cookie alone**, so it was dead in precisely +the deployments this fix is about. The recovery credential is now emitted on a +`set-admin-session-token` response header (exposed via +`Access-Control-Expose-Headers`, alongside `set-auth-token`) and accepted back on +an `x-admin-session-token` request header. Clients that already work through +cookies need no change: a real `admin_session` cookie still wins, and the vendor +route's own checks all still run — this adds a lane, it does not open one. + +For API clients, the flow is the same one `set-auth-token` already asks for: +read both headers off the impersonation response, send `Authorization: Bearer` +with the new token, and send the recovery credential back on +`x-admin-session-token` when leaving impersonation. + +Unaffected: cookie-authenticated impersonation, which is unchanged byte for +byte — a browser caller has no stale credential in hand to invalidate. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index ba62d4024c..b93934449b 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -29,6 +29,12 @@ import { type AuthEventAuditSurface, } from './auth-session-audit.js'; import { SESSION_ERASURE_PATHS } from './session-tombstone.js'; +import { + ADMIN_SESSION_COOKIE_KEY, + STOP_IMPERSONATING_PATH, + rotateCallerBearerOnImpersonation, + withBearerAdminSessionRecovery, +} from './impersonation-bearer-rotation.js'; import { invitationRoleCapFailure, isPlainMemberInvitation, @@ -1557,6 +1563,18 @@ export class AuthManager { } }), after: createAuthMiddleware(async (ctx: any) => { + // ── #8243: impersonation must actually take effect for a bearer ── + // FIRST among the after-hooks that touch the response, because the + // bearer plugin's own after-hook runs behind this one and must see + // what we stage: it re-reads `Access-Control-Expose-Headers` to add + // `set-auth-token`, so the recovery header we expose here survives + // only by being merged before it. See + // `impersonation-bearer-rotation.ts` for the mechanism — the short + // version is that `bearer()` converts the caller's token back into + // the ADMIN's session cookie on every later request, so without + // rotating that token, `/admin/impersonate-user` is a 200 no-op. + await rotateCallerBearerOnImpersonation(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 @@ -3335,6 +3353,35 @@ export class AuthManager { } const auth = await this.getOrCreateAuth(); + + // [#8243] Let a bearer client carry the `admin_session` recovery credential + // back out of impersonation. better-auth's `/admin/stop-impersonating` + // resolves the admin through the `admin_session` COOKIE alone, so in a + // cookie-blocked deployment — the exact context `bearer()` exists for — the + // exit path is dead. We accept the credential on a header and write it into + // the request's own `Cookie` before better-auth sees it; the vendor route + // then runs completely unmodified, checking everything it always checked. + // + // The REQUEST seam, not a before-hook: `bearer()`'s before-hook rebuilds + // the header set from `c.request.headers`, so a `Cookie` injected by any + // hook is clobbered by whichever hook sorts after it. Written into the + // request itself, `bearer()`'s parse-mutate-serialize keeps it. + if (this.betterAuthEndpointPath(request) === STOP_IMPERSONATING_PATH) { + try { + const authContext: any = await (auth as any).$context; + const adminCookieName: string | undefined = + authContext?.createAuthCookie?.(ADMIN_SESSION_COOKIE_KEY)?.name; + if (adminCookieName) { + request = await withBearerAdminSessionRecovery(request, adminCookieName); + } + } catch { + // Cookie name unresolvable (e.g. a dynamic-baseURL context we cannot + // reach here) → leave the request alone. The vendor route then answers + // exactly as it does today for a missing cookie: a loud failure, never + // a silent wrong identity. + } + } + // better-auth's HTTP entrypoint (`createBetterAuth.handler`) wraps execution // in `runWithAdapter` but NOT `runWithRequestState`. Endpoints that read // request-state via `defineRequestState()` (e.g. `should-session-refresh`, diff --git a/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts new file mode 100644 index 0000000000..c48b71479c --- /dev/null +++ b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts @@ -0,0 +1,417 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #8243 — `/admin/impersonate-user` was a 200 NO-OP for every bearer client. +// +// The shape of the defect dictates the shape of these tests. better-auth +// answered 200, set the impersonation cookie, and emitted the impersonated +// token on `set-auth-token` — all of it correct — and then its own `bearer()` +// before-hook converted the caller's unchanged `Authorization: Bearer` back +// into the ADMIN's session cookie on every following request. So a test that +// asserted "impersonation returns 200", or "a `set-auth-token` came back", or +// "an impersonation session row exists" would have been GREEN against the bug. +// +// Every assertion below therefore ends at the same question the runtime asks: +// WHICH PRINCIPAL does the next request resolve to? Both directions are pinned, +// as the ruling requires — a bearer client that impersonates resolves to the +// impersonated user and never to the admin, and `stop-impersonating` recovers +// the admin through the bearer lane. +// +// Real better-auth pipeline throughout (the precedent set by +// `session-of-record.test.ts`): requests go in as `Request` objects through +// `AuthManager.handleRequest`, the tokens are the ones better-auth minted, and +// the resolution path is the real one. Where a test wants the seam the data +// routes actually use, it asks `auth.api.getSession({ headers })` directly — +// that is literally what `runtime/src/security/resolve-session-principal.ts` +// calls. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { AuthManager } from './auth-manager'; +import { + ADMIN_SESSION_RECOVERY_REQUEST_HEADER, + ADMIN_SESSION_RECOVERY_RESPONSE_HEADER, +} from './impersonation-bearer-rotation'; + +/** + * In-memory IDataEngine — same fake as the #4785 session-of-record harness, + * including its two fidelity choices: `fields` really projects, and `delete` + * is pinned to ObjectQL's own dispatch predicate. The delete path is genuinely + * exercised here: rotation deletes the admin's original session row. + */ +const createMemoryEngine = () => { + const tables = new Map(); + const rows = (name: string) => { + if (!tables.has(name)) tables.set(name, []); + return tables.get(name)!; + }; + const eq = (a: any, b: any) => + a instanceof Date || b instanceof Date + ? new Date(a as any).getTime() === new Date(b as any).getTime() + : a === b; + const matches = (row: any, where: Record = {}) => + Object.entries(where).every(([k, v]) => { + const actual = row[k]; + if (v && typeof v === 'object' && !Array.isArray(v) && !(v instanceof Date)) { + if ('$ne' in v) return !eq(actual, v.$ne); + if ('$in' in v) return (v.$in as any[]).some((x) => eq(actual, x)); + if ('$gt' in v) return actual > v.$gt; + if ('$gte' in v) return actual >= v.$gte; + if ('$lt' in v) return actual < v.$lt; + if ('$lte' in v) return actual <= v.$lte; + if ('$regex' in v) return new RegExp(String(v.$regex)).test(String(actual ?? '')); + } + return eq(actual, v); + }); + const project = (row: any, fields?: string[]) => { + if (!Array.isArray(fields) || fields.length === 0) return { ...row }; + const out: any = {}; + for (const f of ['id', ...fields]) if (f in row) out[f] = row[f]; + return out; + }; + let seq = 0; + return { + tables, + async insert(name: string, data: any) { + const row = { id: data.id ?? `row_${++seq}`, ...data }; + rows(name).push(row); + return { ...row }; + }, + async findOne(name: string, q: any = {}) { + const row = rows(name).find((r) => matches(r, q.where)); + return row ? project(row, q.fields) : null; + }, + async find(name: string, q: any = {}) { + let out = rows(name).filter((r) => matches(r, q.where)); + const order = q.orderBy?.[0]; + if (order) { + out = [...out].sort( + (a, b) => (a[order.field] > b[order.field] ? 1 : -1) * (order.order === 'desc' ? -1 : 1), + ); + } + if (q.offset) out = out.slice(q.offset); + if (q.limit) out = out.slice(0, q.limit); + return out.map((r) => project(r, q.fields)); + }, + async count(name: string, q: any = {}) { + return rows(name).filter((r) => matches(r, q.where)).length; + }, + async update(name: string, patch: any) { + const row = rows(name).find((r) => r.id === patch.id); + if (!row) return null; + Object.assign(row, patch); + return { ...row }; + }, + async delete(name: string, q: any = {}) { + assertEngineDeleteDispatch(q); + const table = rows(name); + const keep = table.filter((r) => !matches(r, q.where)); + tables.set(name, keep); + return table.length - keep.length; + }, + }; +}; + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const PASSWORD = 'S3cure!Passw0rd-8243'; +const BASE = 'http://localhost:3000/api/v1/auth'; + +const makeManager = (engine: any) => + new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + dataEngine: engine, + // The impersonation endpoints live on better-auth's `admin` plugin, which + // is opt-in in this repo. + plugins: { admin: true }, + } as any); + +const signUp = (manager: AuthManager, email: string, name: string) => + manager.handleRequest( + new Request(`${BASE}/sign-up/email`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ email, password: PASSWORD, name }), + }), + ); + +const signIn = (manager: AuthManager, email: string) => + manager.handleRequest( + new Request(`${BASE}/sign-in/email`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ email, password: PASSWORD }), + }), + ); + +/** The bearer token better-auth hands a client on a successful sign-in. */ +const bearerFrom = (response: Response): string => { + const token = response.headers.get('set-auth-token'); + if (!token) throw new Error('no set-auth-token on the response'); + return token; +}; + +const userRows = (engine: any) => (engine.tables.get('sys_user') ?? []) as any[]; +const sessionRows = (engine: any) => (engine.tables.get('sys_session') ?? []) as any[]; + +const userIdFor = (engine: any, email: string): string => { + const row = userRows(engine).find((r) => r.email === email); + if (!row) throw new Error(`no sys_user row for ${email}`); + return String(row.id); +}; + +/** better-auth's `admin` plugin gates impersonation on `user.role`. */ +const makePlatformAdmin = (engine: any, email: string) => { + const row = userRows(engine).find((r) => r.email === email); + if (!row) throw new Error(`no sys_user row for ${email}`); + row.role = 'admin'; +}; + +/** + * WHO does a bearer token resolve to, asked through the exact seam the + * framework's data routes use: `auth.api.getSession({ headers })`, which is + * what `runtime/src/security/resolve-session-principal.ts` calls. + * + * `null` for anonymous. Never a status code — better-auth answers a dead + * session with a 200 and a JSON `null`, so a status assertion is blind here. + */ +const principalFor = async ( + manager: AuthManager, + bearer: string, +): Promise => { + const auth: any = await manager.getAuthInstance(); + const session = await auth.api + .getSession({ headers: new Headers({ authorization: `Bearer ${bearer}` }) }) + .catch(() => null); + const id = session?.user?.id ?? session?.session?.userId; + return typeof id === 'string' && id.length > 0 ? id : null; +}; + +const impersonate = (manager: AuthManager, bearer: string, userId: string) => + manager.handleRequest( + new Request(`${BASE}/admin/impersonate-user`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + authorization: `Bearer ${bearer}`, + }, + body: JSON.stringify({ userId }), + }), + ); + +const stopImpersonating = ( + manager: AuthManager, + bearer: string, + recovery?: string, +) => + manager.handleRequest( + new Request(`${BASE}/admin/stop-impersonating`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + authorization: `Bearer ${bearer}`, + ...(recovery ? { [ADMIN_SESSION_RECOVERY_REQUEST_HEADER]: recovery } : {}), + }, + }), + ); + +/** + * A signed-in platform admin plus a target to impersonate, with the admin's + * bearer in hand — the state every test below starts from. + */ +const arrangeAdminAndTarget = async () => { + const engine = createMemoryEngine(); + const manager = makeManager(engine); + + await signUp(manager, 'admin@example.com', 'Impersonating Admin'); + await signUp(manager, 'target@example.com', 'Impersonation Target'); + makePlatformAdmin(engine, 'admin@example.com'); + + const adminId = userIdFor(engine, 'admin@example.com'); + const targetId = userIdFor(engine, 'target@example.com'); + + const signedIn = await signIn(manager, 'admin@example.com'); + expect(signedIn.status).toBe(200); + const adminBearer = bearerFrom(signedIn); + + // The premise: before impersonating, the bearer IS the admin. + expect(await principalFor(manager, adminBearer)).toBe(adminId); + + return { engine, manager, adminId, targetId, adminBearer }; +}; + +beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => vi.restoreAllMocks()); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#8243 pin 1 — a bearer client that impersonates STOPS resolving as the admin', () => { + it('the token the caller was holding no longer resolves to anyone', async () => { + const { manager, adminId, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const response = await impersonate(manager, adminBearer, targetId); + expect(response.status).toBe(200); + + // THE assertion. Against the unfixed vendor this came back as `adminId`: + // 200 from the endpoint, and every subsequent write still attributed to + // the admin. Asserting `!== adminId` alone would be satisfied by a broken + // pipeline that resolves nobody, so pin the exact value too. + expect(await principalFor(manager, adminBearer)).not.toBe(adminId); + expect(await principalFor(manager, adminBearer)).toBeNull(); + void targetId; + }); + + it('the rotated token from `set-auth-token` resolves to the IMPERSONATED user', async () => { + // The other half: rotation that only killed the old token would leave the + // caller unable to impersonate at all — which is the refusal-based + // hardening the maintainer rejected. + const { manager, adminId, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const response = await impersonate(manager, adminBearer, targetId); + expect(response.status).toBe(200); + + const impersonatedBearer = bearerFrom(response); + expect(impersonatedBearer).not.toBe(adminBearer); + expect(await principalFor(manager, impersonatedBearer)).toBe(targetId); + expect(await principalFor(manager, impersonatedBearer)).not.toBe(adminId); + }); + + it('the admin session row the caller was holding is really gone', async () => { + // Corroborates the resolution assertions at the storage layer: this is an + // invalidation, not a client-side convention the next code path could + // quietly stop honouring. + const { engine, manager, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const adminSessionToken = adminBearer.split('.')[0]; + expect(sessionRows(engine).some((r) => r.token === adminSessionToken)).toBe(true); + + expect((await impersonate(manager, adminBearer, targetId)).status).toBe(200); + + expect(sessionRows(engine).some((r) => r.token === adminSessionToken)).toBe(false); + }); + + it('hands back a recovery credential, and exposes it to cross-origin readers', async () => { + const { manager, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const response = await impersonate(manager, adminBearer, targetId); + const recovery = response.headers.get(ADMIN_SESSION_RECOVERY_RESPONSE_HEADER); + expect(recovery).toBeTruthy(); + + // A console on another origin cannot read a header that is not exposed, so + // an unexposed recovery credential is the same as none at all. `bearer()`'s + // own after-hook runs behind ours and must not have dropped it. + const exposed = (response.headers.get('access-control-expose-headers') || '') + .split(',') + .map((entry) => entry.trim()); + expect(exposed).toContain(ADMIN_SESSION_RECOVERY_RESPONSE_HEADER); + expect(exposed).toContain('set-auth-token'); + + // The recovery credential is NOT a bearer-shaped admin credential: replayed + // as one it resolves to nobody. It only means anything to the exit route. + expect(await principalFor(manager, recovery!)).toBeNull(); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#8243 pin 2 — `stop-impersonating` recovers the admin via bearer', () => { + it('the caller is the admin again, and the returned token proves it', async () => { + const { manager, adminId, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const entered = await impersonate(manager, adminBearer, targetId); + const impersonatedBearer = bearerFrom(entered); + const recovery = entered.headers.get(ADMIN_SESSION_RECOVERY_RESPONSE_HEADER)!; + expect(await principalFor(manager, impersonatedBearer)).toBe(targetId); + + const exited = await stopImpersonating(manager, impersonatedBearer, recovery); + expect(exited.status).toBe(200); + const body: any = await exited.json(); + expect(body?.user?.id).toBe(adminId); + + // …and the identity the NEXT request resolves to is the admin, not merely + // an admin-shaped response body. + const restoredBearer = bearerFrom(exited); + expect(await principalFor(manager, restoredBearer)).toBe(adminId); + }); + + it('the impersonation token dies on the way out', async () => { + const { manager, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const entered = await impersonate(manager, adminBearer, targetId); + const impersonatedBearer = bearerFrom(entered); + const recovery = entered.headers.get(ADMIN_SESSION_RECOVERY_RESPONSE_HEADER)!; + + expect((await stopImpersonating(manager, impersonatedBearer, recovery)).status).toBe(200); + + expect(await principalFor(manager, impersonatedBearer)).toBeNull(); + }); + + it('WITHOUT the recovery credential the exit still refuses — this adds a lane, it does not open one', async () => { + // The negative control. If this went green on its own, the recovery header + // would be decorative and the tests above would prove nothing about it. + const { manager, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const entered = await impersonate(manager, adminBearer, targetId); + const impersonatedBearer = bearerFrom(entered); + + const exited = await stopImpersonating(manager, impersonatedBearer); + expect(exited.status).not.toBe(200); + }); + + it('a forged recovery credential is refused', async () => { + const { manager, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const entered = await impersonate(manager, adminBearer, targetId); + const impersonatedBearer = bearerFrom(entered); + + const exited = await stopImpersonating( + manager, + impersonatedBearer, + 'not-a-real-admin-session:.0000000000000000000000000000000000000000000=', + ); + expect(exited.status).not.toBe(200); + // And the forgery bought nothing: the caller is still the impersonated user. + expect(await principalFor(manager, impersonatedBearer)).toBe(targetId); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +describe('#8243 — the cookie lane is untouched', () => { + it('a cookie-authenticated impersonation does not rotate anything', async () => { + // Rotation is scoped to the caller's BEARER, because a cookie caller has no + // stale credential in hand — the route already replaced its session cookie. + // Widening it would change behaviour for every browser deployment for no + // reason, so pin the narrowness. + const engine = createMemoryEngine(); + const manager = makeManager(engine); + + await signUp(manager, 'cookie-admin@example.com', 'Cookie Admin'); + await signUp(manager, 'cookie-target@example.com', 'Cookie Target'); + makePlatformAdmin(engine, 'cookie-admin@example.com'); + const targetId = userIdFor(engine, 'cookie-target@example.com'); + + const signedIn = await signIn(manager, 'cookie-admin@example.com'); + const cookie = (signedIn.headers.getSetCookie?.() ?? []) + .map((c) => c.split(';')[0]) + .filter(Boolean) + .join('; '); + expect(cookie).toBeTruthy(); + + const adminSessionsBefore = sessionRows(engine).map((r) => r.token); + + const response = await manager.handleRequest( + new Request(`${BASE}/admin/impersonate-user`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', cookie }, + body: JSON.stringify({ userId: targetId }), + }), + ); + expect(response.status).toBe(200); + + // No recovery header, and the admin's original session row survives — + // exactly the vendor's behaviour. + expect(response.headers.get(ADMIN_SESSION_RECOVERY_RESPONSE_HEADER)).toBeNull(); + const tokensAfter = sessionRows(engine).map((r) => r.token); + for (const token of adminSessionsBefore) expect(tokensAfter).toContain(token); + }); +}); diff --git a/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts new file mode 100644 index 0000000000..8fa26c6da7 --- /dev/null +++ b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts @@ -0,0 +1,324 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8243 — impersonation must actually take effect for a bearer client. + * + * ## The defect + * + * better-auth's `bearer()` plugin authenticates a request by OVERWRITING the + * request's session cookie with the bearer token (a before-hook calling + * `setRequestCookie(headers, authCookies.sessionToken.name, decodedToken)` — + * `dist/plugins/bearer/index.mjs`, verified in 1.7.0-rc.2). The admin plugin's + * `POST /admin/impersonate-user` does the opposite: it mints an impersonation + * session and hands it over as a *cookie* (`setSessionCookie`), parking the + * admin's own session token in a signed `admin_session` cookie for the exit + * path. + * + * For a cookie client those two compose: the browser replaces its session + * cookie and every later request is the impersonated principal. For a bearer + * client they collide. The client keeps replaying `Authorization: Bearer + * `, the before-hook keeps converting it back into the ADMIN's + * session cookie, and the impersonation cookie never gets a chance to be read. + * The endpoint answers 200 and does nothing — and because the framework's data + * routes resolve identity through the very same seam + * (`runtime/src/security/resolve-session-principal.ts`), every write made + * "while impersonating" is attributed to the admin. + * + * A silent 200 no-op on a security-relevant admin endpoint is the worst + * available shape: nothing anywhere reports that impersonation did not happen. + * + * ## The fix — rotation, not refusal (maintainer ruling, 2026-08-13) + * + * Refusing bearer-authenticated impersonation was considered and REJECTED: it + * would leave cookie-blocked deployments — the exact context `bearer()` exists + * for — permanently unable to impersonate at all. + * + * So we rotate instead. When the caller of `/admin/impersonate-user` + * authenticated with a bearer, the token it is holding is INVALIDATED as part + * of impersonation: + * + * 1. mint a rotated admin session (carrying the old one's fields forward, so + * the admin does not lose e.g. their active organization on the way back); + * 2. hand the caller the rotated session as an `admin_session` RECOVERY + * credential, both as the signed cookie the vendor already sets and — for + * the cookie-blocked lane — as a `set-admin-session-token` response + * header, mirroring how `bearer()` emits `set-auth-token`; + * 3. delete the admin's original session, LAST. + * + * After that the only token that resolves is the impersonated one better-auth + * already emitted via `set-auth-token`. A client that adopts the rotation is + * the impersonated principal; a client that ignores it gets a loud 401 on the + * next request. "Impersonation succeeded but did not take effect" stops being + * expressible. + * + * Step order is the failure design. The rotated session exists and the caller + * has been handed its recovery value BEFORE the old session dies, so any throw + * on the way leaves the admin signed in with impersonation refused — never an + * admin locked out of a session they can no longer name. + * + * ## The exit path — also ruled, and shipped together + * + * `POST /admin/stop-impersonating` resolves the admin through the + * `admin_session` COOKIE only. In a cookie-blocked deployment that cookie + * never comes back, so the exit path is dead in precisely the deployments this + * card is about — an entry path without a working exit is not a fix. So the + * recovery value is accepted back on an `x-admin-session-token` REQUEST + * header and injected into the request's `Cookie` header before better-auth + * sees it; the vendor route then runs completely unmodified. + * + * ## Why these seams + * + * No fork, no vendoring, no patched dependency (ruled). Two wrappers around + * the vendor route: + * + * - Rotation rides better-auth's global `hooks.after`, which is the one place + * where the admin's session (`ctx.context.session`, set by the route's + * `sessionMiddleware`) and the adapter are both in hand after the route has + * succeeded. + * - Recovery ingestion rides the REQUEST seam (`AuthManager.handleRequest`), + * NOT a before-hook, and that is load-bearing. `bearer()`'s own before-hook + * rebuilds the header set from `c.request.headers` — the untouched original + * — so any `Cookie` a before-hook injects is overwritten by whichever hook + * runs later. Injecting into the request itself puts the cookie somewhere + * `bearer()` preserves: its `setRequestCookie` is parse-mutate-serialize and + * keeps every cookie it did not set. This is the same reasoning that put + * `runSubjectErasureAtomically` at the request seam rather than in a route. + * + * Upstream: the overwrite is vendor behaviour, reported upstream — see the + * issue link recorded on #8243. + */ + +/** better-auth's admin plugin, entry path. */ +export const IMPERSONATE_USER_PATH = '/admin/impersonate-user'; + +/** better-auth's admin plugin, exit path. */ +export const STOP_IMPERSONATING_PATH = '/admin/stop-impersonating'; + +/** The vendor's cookie name key for the parked admin session. */ +export const ADMIN_SESSION_COOKIE_KEY = 'admin_session'; + +/** + * Response header carrying the `admin_session` recovery credential, emitted on + * a bearer-authenticated impersonation. Deliberately shaped like `bearer()`'s + * own `set-auth-token`, because the client handling is identical: read it off + * the impersonation response, store it, replay it on the way out. + */ +export const ADMIN_SESSION_RECOVERY_RESPONSE_HEADER = 'set-admin-session-token'; + +/** Request header a bearer client replays the recovery credential on. */ +export const ADMIN_SESSION_RECOVERY_REQUEST_HEADER = 'x-admin-session-token'; + +/** + * The session token a request's `Authorization: Bearer` header resolves to, or + * `undefined` when there is no bearer. + * + * Mirrors `bearer()`'s own decoding so the comparison against the resolved + * session is exact: the plugin percent-decodes the token when it needs to and + * installs it as the session cookie, whose value is `.`. + * An unsigned token has no `.` and survives the split unchanged. + */ +function bearerSessionToken(ctx: any): string | undefined { + const raw: string = + ((ctx?.request?.headers?.get?.('authorization') ?? + ctx?.headers?.get?.('authorization')) as string) || ''; + const match = /^Bearer\s+(.+)$/i.exec(raw); + if (!match?.[1]) return undefined; + let token = match[1].trim(); + if (token.includes('%')) { + try { + token = decodeURIComponent(token); + } catch { + /* not percent-encoded after all — use it verbatim, as bearer() does */ + } + } + return token.split('.')[0] || undefined; +} + +/** Every `Set-Cookie` currently staged on the response, however the runtime spells it. */ +function stagedSetCookies(headers: Headers | undefined): string[] { + if (!headers) return []; + const viaGetter = (headers as any).getSetCookie?.(); + if (Array.isArray(viaGetter)) return viaGetter; + const joined = headers.get('set-cookie'); + return joined ? [joined] : []; +} + +/** + * Drop the vendor's `admin_session` `Set-Cookie`, which names the session we + * are about to delete. Ours is appended when this hook's headers are merged, + * so the response carries exactly one — a stale duplicate would be applied + * first and shadowed, which works but reads as a bug to the next person. + */ +function dropStaleAdminSessionCookie(ctx: any, cookieName: string): void { + const headers: Headers | undefined = ctx?.context?.responseHeaders; + if (!headers) return; + const staged = stagedSetCookies(headers); + if (staged.length === 0) return; + const kept = staged.filter((cookie) => !cookie.startsWith(`${cookieName}=`)); + if (kept.length === staged.length) return; + headers.delete('set-cookie'); + for (const cookie of kept) headers.append('set-cookie', cookie); +} + +/** + * Add a header to `Access-Control-Expose-Headers` without dropping what is + * already there. A cross-origin console reads the recovery credential off the + * response, so an unexposed header is the same as no header at all. `bearer()` + * does exactly this for `set-auth-token` in its own after-hook, which runs + * after ours and therefore preserves what we add here. + */ +function exposeResponseHeader(ctx: any, name: string): void { + const current: string = + (ctx?.context?.responseHeaders?.get?.('access-control-expose-headers') as string) || ''; + const exposed = new Set( + current + .split(',') + .map((entry) => entry.trim()) + .filter(Boolean), + ); + exposed.add(name); + ctx.setHeader('Access-Control-Expose-Headers', Array.from(exposed).join(', ')); +} + +/** Did the impersonation route actually succeed? */ +async function impersonationSucceeded(ctx: any): Promise { + const returned = ctx?.context?.returned; + if (!returned) return false; + try { + const { isAPIError } = await import('better-auth/api'); + if (isAPIError(returned)) return false; + } catch { + if (returned instanceof Error) return false; + } + return Boolean((returned as any)?.session); +} + +/** + * Rotate the caller's bearer as part of a successful `/admin/impersonate-user`, + * so the impersonated session is the only one that resolves afterwards. + * + * A no-op for every other path, for a failed impersonation, and for a caller + * that did NOT authenticate with a bearer — a cookie client's session cookie + * was already replaced by the route, so its behaviour is unchanged byte for + * byte, and there is no stale credential in the client's hands to invalidate. + * + * Throws `APIError` if rotation cannot be completed. That is deliberate: the + * one outcome this card exists to make impossible is a 200 that did nothing, + * so a rotation we could not finish must not be reported as success. + */ +export async function rotateCallerBearerOnImpersonation(ctx: any): Promise { + if (ctx?.path !== IMPERSONATE_USER_PATH) return; + if (!(await impersonationSucceeded(ctx))) return; + + const adminSession = ctx?.context?.session?.session; + const adminToken: unknown = adminSession?.token; + const adminUserId: unknown = adminSession?.userId; + if (typeof adminToken !== 'string' || !adminToken) return; + if (typeof adminUserId !== 'string' || !adminUserId) return; + + // Bearer callers only, and only when the bearer is what authenticated THIS + // request. A bearer that failed better-auth's HMAC check never became the + // session, and rotating on its mere presence would kill a cookie session the + // caller is legitimately using. + if (bearerSessionToken(ctx) !== adminToken) return; + + const { APIError } = await import('better-auth/api'); + const { parseSetCookieHeader } = await import('better-auth/cookies'); + + const secret = ctx.context.secret; + const authCookies = ctx.context.authCookies; + const adminCookie = ctx.context.createAuthCookie(ADMIN_SESSION_COOKIE_KEY); + // Read the same way the vendor route reads it, so `dontRememberMe` survives + // the rotation with the meaning the exit path will give it. + const dontRememberMe = await ctx.getSignedCookie( + authCookies.dontRememberToken.name, + secret, + ); + + // ── 1. mint the rotated admin session ─────────────────────────────────── + // The old session's fields ride along: `createSession` spreads the override + // BEFORE its own generated `token`/`expiresAt`/`createdAt`, so identity- + // shaped state (the active organization, for one) is carried forward while + // the token is genuinely new. The old session is still live at this point — + // a failure here leaves the admin exactly as they were. + const { id: _id, ...carriedForward } = (adminSession ?? {}) as Record; + const rotated = await ctx.context.internalAdapter.createSession( + adminUserId, + !!dontRememberMe, + carriedForward, + false, + ); + const rotatedToken: unknown = rotated?.token; + if (typeof rotatedToken !== 'string' || !rotatedToken || rotatedToken === adminToken) { + throw new APIError('INTERNAL_SERVER_ERROR', { + message: + 'Impersonation could not rotate your session and was refused. ' + + 'Nothing was changed; you are still signed in as yourself.', + code: 'IMPERSONATION_ROTATION_FAILED', + }); + } + + // ── 2. hand the caller its recovery credential ────────────────────────── + // Same value shape the vendor parks in the cookie (`:`), + // signed the same way — so the exit path reads one thing whichever lane it + // arrived on. Note the signed form is NOT usable as a bearer: everything + // before the signature is `:`, which matches no + // session row. + dropStaleAdminSessionCookie(ctx, adminCookie.name); + const serialized: string = await ctx.setSignedCookie( + adminCookie.name, + `${rotatedToken}:${dontRememberMe || ''}`, + secret, + authCookies.sessionToken.attributes, + ); + const signedValue = parseSetCookieHeader(serialized).get(adminCookie.name)?.value; + if (!signedValue) { + throw new APIError('INTERNAL_SERVER_ERROR', { + message: + 'Impersonation could not issue an admin-session recovery credential and ' + + 'was refused. Nothing was changed; you are still signed in as yourself.', + code: 'IMPERSONATION_ROTATION_FAILED', + }); + } + ctx.setHeader(ADMIN_SESSION_RECOVERY_RESPONSE_HEADER, signedValue); + exposeResponseHeader(ctx, ADMIN_SESSION_RECOVERY_RESPONSE_HEADER); + + // ── 3. invalidate the token the caller is holding — last ──────────────── + await ctx.context.internalAdapter.deleteSession(adminToken); +} + +/** + * Accept the `admin_session` recovery credential a bearer client replays on + * `x-admin-session-token`, by writing it into the request's `Cookie` header + * before better-auth sees the request. + * + * The vendor's `/admin/stop-impersonating` then resolves the admin exactly as + * it always has — this adds a lane, it does not loosen a check. A real + * `admin_session` cookie still wins, so cookie deployments are untouched, and + * a bogus recovery value fails the vendor's own session lookup rather than + * anything of ours. + * + * Returns the request unchanged whenever there is nothing to do. + */ +export async function withBearerAdminSessionRecovery( + request: Request, + adminSessionCookieName: string, +): Promise { + const recovery = request.headers.get(ADMIN_SESSION_RECOVERY_REQUEST_HEADER); + if (!recovery) return request; + try { + const { parseCookies, setRequestCookie } = await import('better-auth/cookies'); + // A deployment whose cookies work already has the credential; do not let a + // header override the cookie the browser is holding. + if (parseCookies(request.headers.get('cookie') || '').get(adminSessionCookieName)) { + return request; + } + const headers = new Headers(request.headers); + setRequestCookie(headers, adminSessionCookieName, recovery); + return new Request(request, { headers }); + } catch { + // A request we cannot clone is left alone; the vendor route then answers + // the same way it does for a missing cookie today. + return request; + } +} From 76ecba27265ae5217c2b6a1f97b9c3bae2d7d210 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 13:16:32 +0000 Subject: [PATCH 2/2] test(plugin-auth): pin the rotation fake to ObjectQL's update dispatch, and narrow the carried-forward session state (#8243) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check:engine-double-contract flagged the new fake's update() as looser than the producer. Route it through assertEngineUpdateDispatch, the same way its delete() already routes through assertEngineDeleteDispatch. Auditing the double for looseness BEYOND the dispatch signature turned up a real one on the production side: rotation spread the whole old session row into createSession as an override — a set of keys nobody had reasoned about, which an in-memory double accepts and a real ObjectQL insert can refuse. Carry exactly one field across (the admin's selected organization, the only piece they would notice losing) and let everything else be re-derived for a session minted now. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- .../src/impersonation-bearer-rotation.test.ts | 49 ++++++++++++++++--- .../src/impersonation-bearer-rotation.ts | 23 +++++---- 2 files changed, 57 insertions(+), 15 deletions(-) diff --git a/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts index c48b71479c..cd7844c46c 100644 --- a/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts +++ b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts @@ -25,7 +25,7 @@ // calls. import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; import { AuthManager } from './auth-manager'; import { ADMIN_SESSION_RECOVERY_REQUEST_HEADER, @@ -95,11 +95,22 @@ const createMemoryEngine = () => { async count(name: string, q: any = {}) { return rows(name).filter((r) => matches(r, q.where)).length; }, - async update(name: string, patch: any) { - const row = rows(name).find((r) => r.id === patch.id); - if (!row) return null; - Object.assign(row, patch); - return { ...row }; + // Both write verbs open with the PRODUCER's own dispatch predicate, never a + // hand-mirrored id/multi check — a fake looser than the real engine is how + // a suite goes green over a route that never worked. + async update(name: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); + const table = rows(name); + const targets = + dispatch.kind === 'by-id' + ? table.filter((r) => r.id === dispatch.id) + : table.filter((r) => matches(r, options?.where)); + for (const row of targets) Object.assign(row, data); + return dispatch.kind === 'by-id' + ? targets[0] + ? { ...targets[0] } + : null + : targets.length; }, async delete(name: string, q: any = {}) { assertEngineDeleteDispatch(q); @@ -291,6 +302,32 @@ describe('#8243 pin 1 — a bearer client that impersonates STOPS resolving as t expect(sessionRows(engine).some((r) => r.token === adminSessionToken)).toBe(false); }); + it('the rotated admin session is a well-formed session row, not a smuggled copy', async () => { + // The in-memory engine below accepts any payload, so "rotation worked" in a + // test is not by itself evidence that the real ObjectQL insert would take + // it. This converts that permissiveness into a signal: the rotated row is + // compared, column for column, against a row a plain sign-in produced. A + // key the producer would refuse shows up here as an extra column. + const { engine, manager, adminId, targetId, adminBearer } = await arrangeAdminAndTarget(); + + const signInShape = new Set( + Object.keys(sessionRows(engine).find((r) => r.user_id === adminId) ?? {}), + ); + expect(signInShape.size).toBeGreaterThan(0); + + expect((await impersonate(manager, adminBearer, targetId)).status).toBe(200); + + const rotated = sessionRows(engine).find( + (r) => r.user_id === adminId && r.token !== adminBearer.split('.')[0], + ); + expect(rotated).toBeTruthy(); + expect(Object.keys(rotated!).filter((key) => !signInShape.has(key))).toEqual([]); + + // …and the one field carried across on purpose really is carried across. + const adminRow = sessionRows(engine).find((r) => r.user_id === adminId); + expect(rotated!.active_organization_id).toBe(adminRow!.active_organization_id); + }); + it('hands back a recovery credential, and exposes it to cross-origin readers', async () => { const { manager, targetId, adminBearer } = await arrangeAdminAndTarget(); diff --git a/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts index 8fa26c6da7..bf0e4617cd 100644 --- a/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts +++ b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts @@ -37,8 +37,8 @@ * authenticated with a bearer, the token it is holding is INVALIDATED as part * of impersonation: * - * 1. mint a rotated admin session (carrying the old one's fields forward, so - * the admin does not lose e.g. their active organization on the way back); + * 1. mint a rotated admin session, carrying the admin's selected organization + * across so they do not land somewhere else on the way back; * 2. hand the caller the rotated session as an `admin_session` RECOVERY * credential, both as the signed cookie the vendor already sets and — for * the cookie-blocked lane — as a `set-admin-session-token` response @@ -236,16 +236,21 @@ export async function rotateCallerBearerOnImpersonation(ctx: any): Promise ); // ── 1. mint the rotated admin session ─────────────────────────────────── - // The old session's fields ride along: `createSession` spreads the override - // BEFORE its own generated `token`/`expiresAt`/`createdAt`, so identity- - // shaped state (the active organization, for one) is carried forward while - // the token is genuinely new. The old session is still live at this point — - // a failure here leaves the admin exactly as they were. - const { id: _id, ...carriedForward } = (adminSession ?? {}) as Record; + // Exactly ONE field rides across, deliberately: the active organization the + // admin had selected, which is the only piece of session state they would + // notice losing on the way back. Everything else is re-derived for a session + // minted now — `ipAddress`/`userAgent` from this request, and + // `activeOrganizationId` itself by ADR-0081 D1's `session.create.before` + // stamp when the admin had not switched away from their default. + // + // Spreading the whole old row here instead would be handing the producer a + // set of keys nobody reasoned about — a shape an in-memory test double + // accepts happily and a real ObjectQL insert can refuse. Narrow on purpose. + const activeOrganizationId = (adminSession as any)?.activeOrganizationId; const rotated = await ctx.context.internalAdapter.createSession( adminUserId, !!dontRememberMe, - carriedForward, + activeOrganizationId ? { activeOrganizationId } : undefined, false, ); const rotatedToken: unknown = rotated?.token;