diff --git a/.changeset/break-glass-guard-after-auth.md b/.changeset/break-glass-guard-after-auth.md new file mode 100644 index 0000000000..2030eaddb4 --- /dev/null +++ b/.changeset/break-glass-guard-after-auth.md @@ -0,0 +1,17 @@ +--- +'@objectstack/plugin-auth': patch +--- + +Run the break-glass last-local-credential guard after authentication + +The guard that refuses removal of the last local-password login was registered +as a better-auth `before` hook, which runs ahead of the endpoint middleware that +establishes identity. It therefore decided — and answered — a question about a +named user for a caller who had not been authenticated, while every neighbouring +route on the same lane answers with the ordinary "please log in" refusal. + +The guard now runs only once the acting user is resolved. An unauthenticated +caller falls through to the ordinary refusal and learns nothing about the named +user. For an authenticated caller nothing changes: the same lookup runs and the +same `LAST_LOCAL_CREDENTIAL` conflict is returned, so the lockout protection is +unaffected. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index ce4e2711ed..2e63bcada1 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1643,22 +1643,49 @@ export class AuthManager { // the mount is conditional on the admin plugin, and because the // guard itself now lives in ONE module both call sites share — // `last-local-credential.ts`, whose header records this trap. + // ── [#10776] AUTHENTICATE FIRST ──────────────────────────── + // A `hooks.before` runs AHEAD of the endpoint's own + // `use: [adminMiddleware]`, and that middleware is the only layer + // establishing identity on this lane. So this guard used to read an + // unauthenticated request's body, ask the database a question about + // the named user, and answer it — a per-record answer to a caller + // nobody had authenticated, where every sibling `/admin/` route + // answers 401. Resolving the actor first makes the guard's decision + // (and its distinctive refusal) reachable only once the caller has + // an identity; an anonymous caller falls through to the vendor's own + // `sessionMiddleware`/`adminMiddleware` and hears the ordinary 401. + // + // Maintainer ruling 2026-08-22 (decision-inbox digest, accepted + // verbatim 「接受所有」): option A, authentication before the guard. + // + // ⚠️ This changes WHEN the guard decides, never WHAT it decides: + // an authenticated caller reaches exactly the same lookup and the + // same `CONFLICT`. `break-glass-guard-authentication-order.test.ts` + // pins both halves — the disclosure closing AND the invariant + // surviving — because closing the first by deleting the guard would + // satisfy a disclosure-only suite. + // + // Same shape as the `/oauth2/authorize` gate above: unauthenticated + // → fall through, never a refusal invented here. + const breakGlassActor = await this.resolveActor(ctx); + let isLastLocalCredential = false; - try { - let targetId: string | undefined = ctx?.body?.userId ?? ctx?.body?.user_id; - if (!targetId && ctx.path === '/delete-user') { - const { getSessionFromCtx } = await import('better-auth/api'); - const s: any = await getSessionFromCtx(ctx as any).catch(() => null); - targetId = s?.user?.id ?? s?.session?.userId; - } - if (targetId) { - isLastLocalCredential = await isLastLocalCredentialHolder( - ctx.context.adapter, - targetId, - ); + 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; + if (targetId) { + isLastLocalCredential = await isLastLocalCredentialHolder( + ctx.context.adapter, + targetId, + ); + } + } catch { + // Fail-open — never block a legitimate op on a lookup error. } - } catch { - // Fail-open — never block a legitimate op on a lookup error. } if (isLastLocalCredential) { const { APIError } = await import('better-auth/api'); diff --git a/packages/plugins/plugin-auth/src/break-glass-guard-authentication-order.test.ts b/packages/plugins/plugin-auth/src/break-glass-guard-authentication-order.test.ts new file mode 100644 index 0000000000..3bd8c08269 --- /dev/null +++ b/packages/plugins/plugin-auth/src/break-glass-guard-authentication-order.test.ts @@ -0,0 +1,290 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #10776 — the break-glass last-local-credential guard must run AFTER identity +// is established, not before it. +// +// The guard is registered as a better-auth `hooks.before` keyed on `ctx.path`. +// A `before` hook runs ahead of the endpoint's own `use: [adminMiddleware]`, +// which is the only layer that establishes identity on that lane. The guard +// therefore decided — and answered — a per-record question for a caller nobody +// had authenticated. Maintainer ruling 2026-08-22 (decision-inbox digest, +// accepted verbatim 「接受所有」): **Option A, authentication before the +// guard**, so an unauthenticated caller hears only the ordinary refusal that +// every other route on this lane gives. Option B (keep the guard early and +// disguise its answer) was the fallback and is not taken; option C (accept the +// disclosure) is not taken. +// +// ── Why this file drives the REAL seam ────────────────────────────────────── +// +// `break-glass-local-credential.test.ts` drives the before-hook directly with a +// synthetic `ctx`. That is the right shape for the guard's own predicate, and +// it is structurally blind to the defect this file pins: hook ORDER relative to +// endpoint middleware does not exist in a synthetic call. So every assertion +// here goes through `AuthManager.handleRequest` on the installed better-auth +// 1.7.1, where the vendor's own middleware really runs, and reads a status and +// a code off a real `Response`. +// +// ── The load-bearing half ─────────────────────────────────────────────────── +// +// ⛔ An implementation that simply DELETED the guard would satisfy every +// disclosure assertion below while destroying the protection the guard exists +// for. Two describe-blocks exist to make that impossible to pass vacuously: +// the still-refused leg (an AUTHENTICATED admin removing the genuine last +// local credential still gets 409 `LAST_LOCAL_CREDENTIAL`) and the admission +// leg (the same admin removing an ordinary user still succeeds, so the +// still-refused leg cannot be satisfied by refusing everyone). +// +// ADR-0112 is `code` AND `status`; every refusal assertion below carries both. + +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-10776'; +const BASE = 'http://localhost:3000/api/v1/auth'; + +const makeManager = (engine: any) => + new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + dataEngine: engine, + plugins: { admin: true }, + } 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); + // ObjectStack's ADR-0112 envelope nests it; better-auth's flat shape does not. + code = parsed?.error?.code ?? parsed?.code; + } catch { + /* non-JSON body → no code */ + } + return { status: res.status, code, text }; +} + +/** + * One deployment, staged to the exact posture the guard exists to protect: + * + * - `owner` holds the ONLY local-password (`credential`) account — the + * break-glass escape hatch itself. + * - `admin` is an IdP-managed platform admin holding NO local credential + * (their credential row is removed after they sign in, which is what + * enforced SSO looks like: a managed team with a live session and no + * password). Their session keeps working, so they can act as the + * authenticated caller. + * - `ordinary` is a second credential-less managed user — the removable one, + * so the admission direction is testable on the same fixture. + * + * `/admin/remove-user` is better-auth's own endpoint and still authorizes on + * the legacy `role` scalar (only `/admin/impersonate-user` was re-pointed at + * ObjectStack's predicate, see `auth-manager.ts`), so the scalar is what is + * set here. + */ +async function seedDeployment() { + const engine = createMemoryEngine(); + const manager = makeManager(engine); + + for (const [email, name] of [ + ['owner.10776@example.com', 'Break Glass Owner'], + ['admin.10776@example.com', 'Managed Admin'], + ['ordinary.10776@example.com', 'Ordinary User'], + ]) { + 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.10776@example.com'); + const adminId = idFor('admin.10776@example.com'); + const ordinaryId = idFor('ordinary.10776@example.com'); + + // The vendor `/admin/` lane's own authorization scalar. + users.find((r) => String(r.id) === adminId)!.role = 'admin'; + + const signIn = await post(manager, '/sign-in/email', { + email: 'admin.10776@example.com', + password: PASSWORD, + }); + const bearer = signIn.headers.get('set-auth-token'); + expect(bearer, 'sign-in must mint a bearer or the authenticated legs prove nothing').toBeTruthy(); + + const ownerSignIn = await post(manager, '/sign-in/email', { + email: 'owner.10776@example.com', + password: PASSWORD, + }); + const ownerBearer = ownerSignIn.headers.get('set-auth-token'); + expect(ownerBearer, 'the self-service leg needs the owner signed in').toBeTruthy(); + + // Strip the local password from everyone except `owner`, leaving exactly one + // credential holder. This is the state the guard guards. + 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, adminId, ordinaryId, bearer: bearer!, ownerBearer: ownerBearer! }; +} + +beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => vi.restoreAllMocks()); + +// ─────────────────────────────────────────────────────────────────────────── +// The disclosure: an unauthenticated caller learns nothing per-record +// ─────────────────────────────────────────────────────────────────────────── + +describe('#10776 — an anonymous caller gets the ordinary refusal, never a per-record answer', () => { + it('arm 1: naming the break-glass holder answers 401 UNAUTHENTICATED, not the guard‘s 409', async () => { + const { manager, ownerId } = await seedDeployment(); + + const v = await verdict(await post(manager, '/admin/remove-user', { userId: ownerId })); + + // Status AND code (ADR-0112). The status alone was the whole defect: a 409 + // where every sibling route answers 401 IS the per-record answer. + expect(v.status, v.text).toBe(401); + expect(v.code, v.text).toBe('UNAUTHENTICATED'); + expect(v.code, 'the guard‘s code must not reach an unauthenticated caller').not.toBe( + LAST_LOCAL_CREDENTIAL_CODE, + ); + expect(v.status, 'the guard‘s status must not reach an unauthenticated caller').not.toBe(409); + }, 60_000); + + it('arm 2: naming an ordinary user answers 401 UNAUTHENTICATED — measured, not read off the source', async () => { + // The card filed this arm as an unmeasured reading. It is measured here so + // the before/after is a real comparison: if this arm did NOT already land + // on 401, the disclosure would be wider than the card states. + const { manager, ordinaryId } = await seedDeployment(); + + const v = await verdict(await post(manager, '/admin/remove-user', { userId: ordinaryId })); + + expect(v.status, v.text).toBe(401); + expect(v.code, v.text).toBe('UNAUTHENTICATED'); + }, 60_000); + + it('arm 2b: a userId nobody holds answers the same 401 — no existence oracle either', async () => { + const { manager } = await seedDeployment(); + + const v = await verdict(await post(manager, '/admin/remove-user', { userId: 'usr_no_such_user' })); + + expect(v.status, v.text).toBe(401); + expect(v.code, v.text).toBe('UNAUTHENTICATED'); + }, 60_000); + + it('the two arms are INDISTINGUISHABLE to the anonymous caller', async () => { + // The substance of the card asserted directly rather than inferred from two + // assertions that merely happen to agree today: whatever the platform says, + // it must say the SAME thing for the break-glass holder and for anyone else. + const { manager, ownerId, ordinaryId } = await seedDeployment(); + + const holder = await verdict(await post(manager, '/admin/remove-user', { userId: ownerId })); + const other = await verdict(await post(manager, '/admin/remove-user', { userId: ordinaryId })); + + expect(holder.status).toBe(other.status); + expect(holder.text).toBe(other.text); + }, 60_000); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The load-bearing half: the invariant survives the move +// ─────────────────────────────────────────────────────────────────────────── + +describe('#10776 — the break-glass invariant is unchanged for an AUTHENTICATED admin', () => { + it('still-refused: removing the genuine last local credential is still 409 LAST_LOCAL_CREDENTIAL', async () => { + // ⛔ The leg that fails on an implementation that "fixed" the disclosure by + // deleting the guard. Everything in the block above stays green there. + const { manager, ownerId, bearer } = await seedDeployment(); + + const v = await verdict( + await post(manager, '/admin/remove-user', { userId: ownerId }, bearer), + ); + + expect(v.status, v.text).toBe(409); + expect(v.code, v.text).toBe(LAST_LOCAL_CREDENTIAL_CODE); + }, 60_000); + + it('admission: the same admin removing an ordinary user still succeeds', async () => { + // Without this, the still-refused leg above is satisfiable by refusing + // every caller — the failure mode this lane has already paid for twice. + const { manager, ordinaryId, bearer } = await seedDeployment(); + + const res = await post(manager, '/admin/remove-user', { userId: ordinaryId }, bearer); + const v = await verdict(res); + + expect(v.status, v.text).toBe(200); + expect(v.code, v.text).not.toBe(LAST_LOCAL_CREDENTIAL_CODE); + }, 60_000); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The self-service path, whose TIMING moves with the guard +// ─────────────────────────────────────────────────────────────────────────── + +describe('#10776 — /delete-user sits under the same guard and is covered here', () => { + it('anonymous: no per-record answer, and specifically not the guard‘s 409', async () => { + const { manager, ownerId } = await seedDeployment(); + + const v = await verdict(await post(manager, '/delete-user', { userId: ownerId })); + + // Measured: the vendor's own session middleware refuses first, in + // better-auth's flat envelope. That is deliberate and is NOT the #10349 + // envelope's business — `/delete-user` is not an `/admin/` path, and that + // card's normalizer is scoped to `/admin/` on purpose. What matters here is + // that the answer is an AUTHENTICATION refusal and carries nothing about + // the named user. + expect(v.status, v.text).toBe(401); + expect(v.code, v.text).toBe('UNAUTHORIZED'); + expect(v.code, v.text).not.toBe(LAST_LOCAL_CREDENTIAL_CODE); + expect(v.status, v.text).not.toBe(409); + }, 60_000); + + it('anonymous: the self-service path is INDISTINGUISHABLE across the two arms too', async () => { + const { manager, ownerId, ordinaryId } = await seedDeployment(); + + const holder = await verdict(await post(manager, '/delete-user', { userId: ownerId })); + const other = await verdict(await post(manager, '/delete-user', { userId: ordinaryId })); + + expect(holder.status).toBe(other.status); + expect(holder.text).toBe(other.text); + }, 60_000); + + it('authenticated: the OUTCOME for the break-glass holder is unchanged — still 409', async () => { + // Triage asked whether the ordering change alters this path's outcome or + // only its timing. This is that question, pinned. + const { manager, ownerId, ownerBearer } = await seedDeployment(); + + const v = await verdict(await post(manager, '/delete-user', { userId: ownerId }, ownerBearer)); + + expect(v.status, v.text).toBe(409); + expect(v.code, v.text).toBe(LAST_LOCAL_CREDENTIAL_CODE); + }, 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 10b39c2eeb..65f42e3493 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 @@ -24,6 +24,12 @@ * (`LAST_LOCAL_CREDENTIAL`). Under enforced SSO the managed team has no * local credential at all, so that one account IS the escape hatch. * + * [#10776] The guard now runs only once the caller's identity is + * established. That moved WHEN it decides, not WHAT it decides, so the + * cases below still pin the same verdicts — they just have to present a + * resolvable credential to reach the guard at all, and the last case pins + * that an anonymous caller never reaches it. + * * The middleware is driven directly with a synthetic `ctx` — the same shape * better-auth passes it (`path`, `body`, `context.adapter`) — because the * decision under test is entirely a function of those three, and standing up @@ -73,11 +79,25 @@ async function buildConfig( /** * The `account` rows a deployment holds. `findOne` answers the target's own * credential lookup; `findMany` answers "who else holds one". + * + * [#10776] It also answers the `session` lookup the hook now makes BEFORE the + * guard: the guard runs only for a caller whose identity is established, so a + * synthetic ctx that wants to reach the guard has to carry one. Anything the + * fake is not asked about answers `null`, which is exactly what an anonymous + * caller's session lookup finds. */ +const CALLER_TOKEN = 'tok_admin_caller'; + function adapterWithCredentials(holders: string[]) { const rows = holders.map((userId) => ({ userId, providerId: 'credential' })); return { - findOne: vi.fn(async ({ where }: { where: Array<{ field: string; value: unknown }> }) => { + 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; + } const userId = where.find((w) => w.field === 'userId')?.value; return rows.find((r) => r.userId === userId) ?? null; }), @@ -85,6 +105,20 @@ function adapterWithCredentials(holders: string[]) { }; } +/** + * A ctx the way better-auth hands it to the global before-hook, carrying a + * resolvable credential. `signedIn: false` is the anonymous caller. + */ +function hookCtx( + path: string, + body: Record, + adapter: unknown, + { signedIn = true }: { signedIn?: boolean } = {}, +) { + const headers = new Headers(signedIn ? { authorization: `Bearer ${CALLER_TOKEN}` } : {}); + return { path, body, headers, context: { adapter } }; +} + let consoleSpy: ReturnType; let warnSpy: ReturnType; const prevMcp = process.env.OS_MCP_SERVER_ENABLED; @@ -168,7 +202,7 @@ describe('[#5892] the last local password login survives ban / remove / delete', const adapter = adapterWithCredentials(['usr_owner']); let caught: unknown; try { - await before!({ path, body: { userId: 'usr_owner' }, context: { adapter } }); + await before!(hookCtx(path, { userId: 'usr_owner' }, adapter)); } catch (e) { caught = e; } @@ -184,11 +218,7 @@ describe('[#5892] the last local password login survives ban / remove / delete', const adapter = adapterWithCredentials(['usr_owner', 'usr_second_admin']); await expect( - config.hooks!.before!({ - path: '/admin/ban-user', - body: { userId: 'usr_owner' }, - context: { adapter }, - }), + config.hooks!.before!(hookCtx('/admin/ban-user', { userId: 'usr_owner' }, adapter)), ).resolves.toBeUndefined(); }); @@ -199,11 +229,7 @@ describe('[#5892] the last local password login survives ban / remove / delete', const adapter = adapterWithCredentials(['usr_owner']); await expect( - config.hooks!.before!({ - path: '/admin/ban-user', - body: { userId: 'usr_managed' }, - context: { adapter }, - }), + config.hooks!.before!(hookCtx('/admin/ban-user', { userId: 'usr_managed' }, adapter)), ).resolves.toBeUndefined(); // Only the target's own lookup ran — the whole-table scan is skipped. expect(adapter.findMany).not.toHaveBeenCalled(); @@ -211,8 +237,17 @@ describe('[#5892] the last local password login survives ban / remove / delete', it('fails OPEN on a lookup error — the opposite direction from the ban guard, on purpose', async () => { const { config } = await buildConfig({ ssoOnlyMode: true }); + // The session lookup succeeds — the identity is established — and it is the + // GUARD's own lookup that fails. Otherwise this would be measuring the + // #10776 fall-through instead of the fail-open direction. const adapter = { - findOne: vi.fn(async () => { + 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; + } throw new Error('account table unreadable'); }), findMany: vi.fn(async () => []), @@ -222,11 +257,34 @@ describe('[#5892] the last local password login survives ban / remove / delete', // is the cost in `last-admin-ban-guard.ts`. Different failure modes, // different directions — see that file's header. await expect( - config.hooks!.before!({ - path: '/admin/ban-user', - body: { userId: 'usr_owner' }, - context: { adapter }, - }), + config.hooks!.before!(hookCtx('/admin/ban-user', { userId: 'usr_owner' }, adapter)), ).resolves.toBeUndefined(); }); + + // ── [#10776] the guard is now downstream of authentication ─────────────── + + it('does not run AT ALL for an unauthenticated caller — no throw, and no lookup', async () => { + // The disclosure this closes is not only the 409: it is that the guard + // queried the database about a user named by a caller nobody had + // authenticated. So the absence of the lookup is asserted, not just the + // absence of the refusal. The status-level half is pinned end-to-end in + // `break-glass-guard-authentication-order.test.ts`. + const { config } = await buildConfig({ ssoOnlyMode: true }); + + for (const path of BAN_PATHS) { + const adapter = adapterWithCredentials(['usr_owner']); + + await expect( + config.hooks!.before!(hookCtx(path, { userId: 'usr_owner' }, adapter, { signedIn: false })), + ).resolves.toBeUndefined(); + + const askedAboutAccounts = adapter.findOne.mock.calls.some( + ([q]: [{ model?: string }]) => q?.model === 'account', + ); + expect(askedAboutAccounts, `${path}: the guard must not query for an anonymous caller`).toBe( + false, + ); + expect(adapter.findMany).not.toHaveBeenCalled(); + } + }); });