diff --git a/.changeset/break-glass-guard-self-service-target.md b/.changeset/break-glass-guard-self-service-target.md new file mode 100644 index 0000000000..b84f1c4d19 --- /dev/null +++ b/.changeset/break-glass-guard-self-service-target.md @@ -0,0 +1,18 @@ +--- +'@objectstack/plugin-auth': patch +--- + +`/delete-user` no longer lets a body-supplied `userId` win over the resolved actor + +`/delete-user` is the vendor's self-service delete: its contract names no +target, the subject IS the authenticated caller. The break-glass +last-local-credential guard's target resolution on that route still preferred +a body-supplied `userId` whenever one was present, so any authenticated +caller could steer the guard's own refusal at a user other than themselves. + +The guard's target on `/delete-user` is now the resolved actor unconditionally +— `body.userId` is never consulted for that route, only as a prior fallback. +`/admin/remove-user` and `/admin/ban-user` are unaffected: target-naming is +their own contract and is untouched here. For every caller acting on +themselves, nothing changes — the same lookup runs and the same outcome +(refuse the last local credential, admit everything else) is returned. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 643ad10106..4b4bdafd2c 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1674,10 +1674,21 @@ export class AuthManager { if (breakGlassActor?.userId) { try { // `/delete-user` names no target in the vendor's own contract: - // the subject IS the authenticated caller, which is now already - // resolved rather than looked up a second time. - let targetId: string | undefined = ctx?.body?.userId ?? ctx?.body?.user_id; - if (!targetId && ctx.path === '/delete-user') targetId = breakGlassActor.userId; + // the subject IS the authenticated caller. That must hold + // UNCONDITIONALLY — a body-supplied `userId` on this + // self-service route is never the target, present or not. + // (A prior `if (!targetId && …)` form let a body value win + // whenever one was supplied, turning the guard's refusal into + // a per-record oracle over the break-glass holder for any + // authenticated caller, not just the caller acting on + // themselves. `/admin/remove-user` and `/admin/ban-user` + // legitimately name a target in their own contract, so only + // this route's resolution is keyed on the actor instead of + // the body.) + const targetId: string | undefined = + ctx.path === '/delete-user' + ? breakGlassActor.userId + : (ctx?.body?.userId ?? ctx?.body?.user_id); if (targetId) { isLastLocalCredential = await isLastLocalCredentialHolder( ctx.context.adapter, diff --git a/packages/plugins/plugin-auth/src/break-glass-guard-self-service-target.test.ts b/packages/plugins/plugin-auth/src/break-glass-guard-self-service-target.test.ts new file mode 100644 index 0000000000..0063ec3901 --- /dev/null +++ b/packages/plugins/plugin-auth/src/break-glass-guard-self-service-target.test.ts @@ -0,0 +1,221 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #11074 — the authenticated residual of #10776's fix. +// +// #10776 (pinned in `break-glass-guard-authentication-order.test.ts`) moved +// the break-glass guard to run only AFTER identity is established, closing +// the disclosure for an ANONYMOUS caller. Inside the guard, target resolution +// on `/delete-user` was left unchanged: a body-supplied `userId` still won +// over the resolved actor whenever one was present. `/delete-user` is the +// vendor's self-service delete — its contract names no target, the subject +// IS the caller — so that preference let ANY authenticated caller (no admin +// role required; `/delete-user` sits outside the `/admin/` lane) turn the +// guard's own refusal into a per-record answer about who else holds the +// break-glass credential. +// +// The fix: on `/delete-user`, the target is the resolved actor +// UNCONDITIONALLY — `ctx?.body?.userId` is never consulted for that path, not +// only as a fallback. `/admin/remove-user` and `/admin/ban-user` are +// unaffected — target-naming is their own contract, per the vendor and the +// ADR-0068 admin gate — and are not touched here. +// +// ── What this file pins, and why as a comparison ───────────────────────────── +// +// The substance of the finding is that the guard's refusal became a +// DISCRIMINATOR for an authenticated non-holder: naming the holder answered +// differently than naming anyone else. Pinning only "naming the holder no +// longer 409s" would not prove that — an implementation that refused +// everyone, or answered every case with a coin flip, could satisfy a lone +// assertion. So every describe-block below asserts the EQUALITY between two +// calls from the same caller, the way `break-glass-guard-authentication-order +// .test.ts` already does for the anonymous case. Real better-auth pipeline +// throughout, same precedent: requests go in as `Request` objects through +// `AuthManager.handleRequest`. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { AuthManager } from './auth-manager'; +import { createMemoryEngine } from './impersonation-bearer-rotation.test'; +import { LAST_LOCAL_CREDENTIAL_CODE } from './last-local-credential'; + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const PASSWORD = 'S3cure!Passw0rd-11074'; +const BASE = 'http://localhost:3000/api/v1/auth'; + +const makeManager = (engine: any) => + new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + dataEngine: engine, + } as any); + +const post = (manager: AuthManager, path: string, body: unknown, bearer?: string) => + manager.handleRequest( + new Request(`${BASE}${path}`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + ...(bearer ? { authorization: `Bearer ${bearer}` } : {}), + }, + body: JSON.stringify(body), + }), + ); + +/** Status + whatever error code the body carries, in either envelope shape. */ +async function verdict(res: Response): Promise<{ status: number; code?: string; text: string }> { + const text = await res.text(); + let code: string | undefined; + try { + const parsed = JSON.parse(text); + code = parsed?.error?.code ?? parsed?.code; + } catch { + /* non-JSON / empty body → no code */ + } + return { status: res.status, code, text }; +} + +/** + * One deployment: + * + * - `owner` holds the ONLY local-password (`credential`) account — the + * break-glass escape hatch this guard exists to protect. + * - `caller` is a second, credential-less (IdP-managed) user — an + * authenticated caller who is NOT the break-glass holder, which is exactly + * the population the finding is about. `/delete-user` needs no admin role, + * so `caller` carries none. + */ +async function seedDeployment() { + const engine = createMemoryEngine(); + const manager = makeManager(engine); + + for (const [email, name] of [ + ['owner.11074@example.com', 'Break Glass Owner'], + ['caller.11074@example.com', 'Ordinary Caller'], + ]) { + const res = await post(manager, '/sign-up/email', { email, password: PASSWORD, name }); + expect(res.status, `sign-up ${email}: ${await res.clone().text()}`).toBe(200); + } + + const users = (engine.tables.get('sys_user') ?? []) as any[]; + const idFor = (email: string) => String(users.find((r) => r.email === email)!.id); + const ownerId = idFor('owner.11074@example.com'); + const callerId = idFor('caller.11074@example.com'); + + const callerSignIn = await post(manager, '/sign-in/email', { + email: 'caller.11074@example.com', + password: PASSWORD, + }); + const callerBearer = callerSignIn.headers.get('set-auth-token'); + expect(callerBearer, 'the probe leg needs the non-holder caller signed in').toBeTruthy(); + + const ownerSignIn = await post(manager, '/sign-in/email', { + email: 'owner.11074@example.com', + password: PASSWORD, + }); + const ownerBearer = ownerSignIn.headers.get('set-auth-token'); + expect(ownerBearer, 'the evasion leg needs the holder signed in').toBeTruthy(); + + // Strip the local password from everyone except `owner`, leaving exactly + // one credential holder — the state the guard exists to protect, and what + // makes `caller` a genuine non-holder. + const accounts = (engine.tables.get('sys_account') ?? []) as any[]; + engine.tables.set( + 'sys_account', + accounts.filter((r) => !(r.provider_id === 'credential' && String(r.user_id ?? '') !== ownerId)), + ); + const remaining = (engine.tables.get('sys_account') ?? []).filter( + (r: any) => r.provider_id === 'credential', + ); + expect( + remaining.map((r: any) => String(r.user_id)), + 'fixture invariant: `owner` must be the SOLE local-credential holder', + ).toEqual([ownerId]); + + return { engine, manager, ownerId, callerId, callerBearer: callerBearer!, ownerBearer: ownerBearer! }; +} + +beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => vi.restoreAllMocks()); + +// ─────────────────────────────────────────────────────────────────────────── +// The disclosure: an authenticated non-holder learns nothing per-record +// ─────────────────────────────────────────────────────────────────────────── + +describe('#11074 — /delete-user: an authenticated non-holder gets no per-record answer', () => { + it('naming the holder vs. naming nobody-in-particular in body.userId: SAME answer', async () => { + const { manager, ownerId, callerBearer } = await seedDeployment(); + + const namedHolder = await verdict(await post(manager, '/delete-user', { userId: ownerId }, callerBearer)); + const namedOther = await verdict( + await post(manager, '/delete-user', { userId: 'usr_no_such_user' }, callerBearer), + ); + + // Both fall through past the guard to the vendor's own disabled + // `/delete-user` handler (#7735) — the caller is deleting THEMSELVES, + // and they are not the break-glass holder. Neither leg is the guard's + // distinctive refusal. + expect(namedHolder.status, namedHolder.text).toBe(404); + expect(namedOther.status, namedOther.text).toBe(404); + expect(namedHolder.code).not.toBe(LAST_LOCAL_CREDENTIAL_CODE); + expect(namedOther.code).not.toBe(LAST_LOCAL_CREDENTIAL_CODE); + + // The comparison IS the finding: pre-fix, naming the holder answered 409 + // here while naming anyone else answered 404 — a per-record oracle for + // ANY authenticated caller, not only one acting on themselves. Restored: + // what the body claims cannot move the answer at all. + expect(namedHolder.status, 'the two arms must be indistinguishable').toBe(namedOther.status); + expect(namedHolder.text, 'the two arms must be indistinguishable').toBe(namedOther.text); + }, 60_000); + + it('naming the holder vs. omitting userId entirely: SAME answer too', async () => { + // A caller need not even guess a real id to have probed under the old + // code — any body.userId that resolved to SOME credential holder would + // have done it. Omitting it entirely is the other natural shape of an + // ordinary self-delete call, and must land the same place. + const { manager, ownerId, callerBearer } = await seedDeployment(); + + const named = await verdict(await post(manager, '/delete-user', { userId: ownerId }, callerBearer)); + const omitted = await verdict(await post(manager, '/delete-user', {}, callerBearer)); + + expect(named.status, named.text).toBe(404); + expect(omitted.status, omitted.text).toBe(404); + expect(named.status).toBe(omitted.status); + expect(named.text).toBe(omitted.text); + }, 60_000); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The load-bearing half: the invariant survives, in BOTH directions +// ─────────────────────────────────────────────────────────────────────────── + +describe('#11074 — the guard still does its job, and cannot be steered by the body', () => { + it('still-refused: the holder deleting themselves via /delete-user is still 409', async () => { + // ⛔ The leg that fails on an implementation that "fixed" the disclosure + // by disabling the guard on this path entirely. + const { manager, ownerBearer } = await seedDeployment(); + + const v = await verdict(await post(manager, '/delete-user', {}, ownerBearer)); + + expect(v.status, v.text).toBe(409); + expect(v.code, v.text).toBe(LAST_LOCAL_CREDENTIAL_CODE); + }, 60_000); + + it('the holder cannot evade the guard by naming a decoy target in body.userId', async () => { + // The other direction of the same bug: unconditional actor-keying also + // means the caller cannot point the guard AWAY from themselves by naming + // someone else. Pre-fix, a holder POSTing `{ userId: }` + // would have had the guard check that OTHER user (who is not the sole + // holder) and let the real self-delete proceed. + const { manager, callerId, ownerBearer } = await seedDeployment(); + + const decoy = await verdict(await post(manager, '/delete-user', { userId: callerId }, ownerBearer)); + const honest = await verdict(await post(manager, '/delete-user', {}, ownerBearer)); + + expect(decoy.status, decoy.text).toBe(409); + expect(decoy.code, decoy.text).toBe(LAST_LOCAL_CREDENTIAL_CODE); + expect(honest.status).toBe(decoy.status); + expect(honest.text).toBe(decoy.text); + }, 60_000); +}); diff --git a/packages/plugins/plugin-auth/src/break-glass-local-credential.test.ts b/packages/plugins/plugin-auth/src/break-glass-local-credential.test.ts index 65f42e3493..380ed0ddcb 100644 --- a/packages/plugins/plugin-auth/src/break-glass-local-credential.test.ts +++ b/packages/plugins/plugin-auth/src/break-glass-local-credential.test.ts @@ -87,6 +87,12 @@ async function buildConfig( * caller's session lookup finds. */ const CALLER_TOKEN = 'tok_admin_caller'; +// [#11074] `/delete-user` now keys its target on the resolved ACTOR, +// unconditionally — never `body.userId`. A synthetic ctx that wants to reach +// the guard's refusal on that path has to authenticate AS the credential +// holder, not merely name one in the body. This second token resolves to a +// different session user so that leg can be driven honestly. +const OWNER_TOKEN = 'tok_owner_caller'; function adapterWithCredentials(holders: string[]) { const rows = holders.map((userId) => ({ userId, providerId: 'credential' })); @@ -94,9 +100,13 @@ function adapterWithCredentials(holders: string[]) { findOne: vi.fn(async ({ model, where }: { model?: string; where: Array<{ field: string; value: unknown }> }) => { if (model === 'session') { const token = where.find((w) => w.field === 'token')?.value; - return token === CALLER_TOKEN - ? { token, userId: 'usr_admin_caller', expiresAt: new Date(Date.now() + 3_600_000) } - : null; + if (token === CALLER_TOKEN) { + return { token, userId: 'usr_admin_caller', expiresAt: new Date(Date.now() + 3_600_000) }; + } + if (token === OWNER_TOKEN) { + return { token, userId: 'usr_owner', expiresAt: new Date(Date.now() + 3_600_000) }; + } + return null; } const userId = where.find((w) => w.field === 'userId')?.value; return rows.find((r) => r.userId === userId) ?? null; @@ -113,9 +123,9 @@ function hookCtx( path: string, body: Record, adapter: unknown, - { signedIn = true }: { signedIn?: boolean } = {}, + { signedIn = true, token = CALLER_TOKEN }: { signedIn?: boolean; token?: string } = {}, ) { - const headers = new Headers(signedIn ? { authorization: `Bearer ${CALLER_TOKEN}` } : {}); + const headers = new Headers(signedIn ? { authorization: `Bearer ${token}` } : {}); return { path, body, headers, context: { adapter } }; } @@ -198,7 +208,10 @@ describe('[#5892] the last local password login survives ban / remove / delete', const before = config.hooks?.before; expect(typeof before).toBe('function'); - for (const path of BAN_PATHS) { + // `/admin/ban-user` and `/admin/remove-user` still resolve their target + // from `body.userId` — an admin acting on someone else, per their own + // vendor contract. + for (const path of ['/admin/ban-user', '/admin/remove-user']) { const adapter = adapterWithCredentials(['usr_owner']); let caught: unknown; try { @@ -211,6 +224,26 @@ describe('[#5892] the last local password login survives ban / remove / delete', expect(api.body.code).toBe('LAST_LOCAL_CREDENTIAL'); expect(api.body.message).toMatch(/identity-\s*provider outage|provider outage/); } + + // [#11074] `/delete-user` is self-service: the target is the ACTOR, + // unconditionally. A body-named target — even the genuine holder — is + // never consulted, so the refusal is only reachable by authenticating AS + // the holder. + { + const adapter = adapterWithCredentials(['usr_owner']); + let caught: unknown; + try { + await before!( + hookCtx('/delete-user', { userId: 'usr_someone_else' }, adapter, { token: OWNER_TOKEN }), + ); + } catch (e) { + caught = e; + } + expect(isAPIError(caught)).toBe(true); + const api = caught as { statusCode: number; body: { code?: string; message?: string } }; + expect(api.body.code).toBe('LAST_LOCAL_CREDENTIAL'); + expect(api.body.message).toMatch(/identity-\s*provider outage|provider outage/); + } }); it('allows it when another user still holds a local password', async () => {