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..cd7844c46c --- /dev/null +++ b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.test.ts @@ -0,0 +1,454 @@ +// 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, assertEngineUpdateDispatch } 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; + }, + // 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); + 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('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(); + + 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..bf0e4617cd --- /dev/null +++ b/packages/plugins/plugin-auth/src/impersonation-bearer-rotation.ts @@ -0,0 +1,329 @@ +// 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 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 + * 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 ─────────────────────────────────── + // 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, + activeOrganizationId ? { activeOrganizationId } : undefined, + 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; + } +}