From e5129bd396adff665b8a7920b867801c1b142ea0 Mon Sep 17 00:00:00 2001 From: os-warren Date: Fri, 21 Aug 2026 10:48:23 +0000 Subject: [PATCH 1/2] fix(plugin-auth): envelope the better-auth-native /admin/ refusals (#10349) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `/api/v1/auth/admin/` namespace answered the same question in two shapes. ObjectStack's raw mounts refuse an anonymous caller with the ADR-0112 envelope and `code: 'UNAUTHENTICATED'` (`platform-admin-gate.ts`); the routes better-auth serves itself refuse through the vendor's `adminMiddleware` (`APIError.fromStatus('UNAUTHORIZED')`, no body argument), which reaches the client as a 401 announcing `application/json` and carrying the EMPTY STRING. Measured on better-auth 1.7.1 through `AuthManager.handleRequest`, anonymous: ten vendor-lane routes answered `401 ct=application/json len=0 body=""`. `handleRequest` now gives those refusals the declared envelope at the one seam every vendor route passes through, with the code DERIVED from the status by ADR-0112's own `standardErrorCodeForHttpStatus` map — no new error code, no literal to drift. Statuses and admission are unchanged. Scope is the `/admin/` namespace (option C, not option B). The prefix test costs no new concept: `handleRequest` already discriminates on `betterAuthEndpointPath` twice, for `STOP_IMPERSONATING_PATH` and `SESSION_ERASURE_PATHS`. Three narrowings, each pinned: a refusal that already carried a body keeps it byte-for-byte; only 401/403 are named (a bodyless `/admin/oauth2/*` 404 and any semantic 4xx the vendor owns stay as they are); nothing outside `/admin/` is touched. The dogfood sweep's `better-auth-gate` bucket tightens from `[401, 403].includes(anon.status)` to the full ADR-0112 pin — status AND code — which is the fix's own falsifiable assertion. That bucket documented this gap instead of closing it, and nothing tracked closing it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .changeset/admin-vendor-refusal-envelope.md | 58 ++++ .../src/admin-impersonate-endpoint.test.ts | 26 +- .../plugins/plugin-auth/src/auth-manager.ts | 23 +- .../plugin-auth/src/platform-admin-gate.ts | 25 +- .../src/vendor-admin-refusal-envelope.test.ts | 268 ++++++++++++++++++ .../src/vendor-admin-refusal-envelope.ts | 154 ++++++++++ ...min-route-nonadmin-refusal.dogfood.test.ts | 31 +- 7 files changed, 568 insertions(+), 17 deletions(-) create mode 100644 .changeset/admin-vendor-refusal-envelope.md create mode 100644 packages/plugins/plugin-auth/src/vendor-admin-refusal-envelope.test.ts create mode 100644 packages/plugins/plugin-auth/src/vendor-admin-refusal-envelope.ts diff --git a/.changeset/admin-vendor-refusal-envelope.md b/.changeset/admin-vendor-refusal-envelope.md new file mode 100644 index 0000000000..a7788a5489 --- /dev/null +++ b/.changeset/admin-vendor-refusal-envelope.md @@ -0,0 +1,58 @@ +--- +"@objectstack/plugin-auth": minor +--- + +fix(plugin-auth): the better-auth-native `/admin/` routes refuse an anonymous caller with the ADR-0112 envelope (#10349) + +**BREAKING** response-shape change on the `/api/v1/auth/admin/` namespace, +shipped as `minor` under the repo's launch-window convention for breaking +changes. + +`/api/v1/auth/admin/` is served by two implementations and answered the same +question in two shapes. ObjectStack's raw mounts (`create-user`, +`set-user-password`, `unlock-user`, `import-users`, `ban-user`, `unban-user`, +`oauth2/toggle-disabled`, `sso/*`) refuse an anonymous caller through +`judgePlatformAdmin` with the declared envelope and `code: 'UNAUTHENTICATED'`. +The routes better-auth serves itself refuse through the vendor's +`adminMiddleware` — `getAuthoritativeSessionFromCtx(ctx)` then +`APIError.fromStatus('UNAUTHORIZED')`, with no body argument at all. + +Measured on the installed better-auth 1.7.1, anonymous, through +`AuthManager.handleRequest`: ten vendor-lane routes (`impersonate-user`, +`set-role`, `revoke-user-sessions`, `revoke-user-session`, +`list-user-sessions`, `update-user`, `list-users`, `get-user`, +`has-permission`, `stop-impersonating`) answered `401` with a +`content-type: application/json` header and the **empty string** as the body. +A client that believes that header and parses the body throws on the refusal +instead of branching on it, and a client that wants to branch has to know, per +route, which of the two implementations happens to serve it — an +implementation detail, not a contract. + +`AuthManager.handleRequest` now gives those refusals the declared envelope at +the one seam every vendor route passes through. **Statuses are unchanged and +admission is unchanged**: nothing that was refused is now admitted, nothing +that was admitted is now refused, and no status moved. What is added is the +machine-readable `code`, derived from the status by ADR-0112's own +`standardErrorCodeForHttpStatus` map rather than spelled out again — so no new +error code is registered and the vendor lane's anonymous refusal is now +byte-identical to the ObjectStack lane's. + +Scope is the `/admin/` namespace only. Three narrowings hold the rest of the +surface still, and each is pinned: + +- **A refusal that already carried a body keeps it, byte for byte.** The + signed-in non-admin's `403` with the vendor's own + `YOU_ARE_NOT_ALLOWED_TO_*` vocabulary is untouched; this change fills in an + empty body and never rewrites a spoken one. +- **Only the two refusal statuses are named** (`401`, `403`). A bodyless `404` + such as `/admin/oauth2/*` with the `oidcProvider` plugin off, and any + semantic `4xx` the vendor owns, are left exactly as they are. +- **Nothing outside `/admin/` is touched.** `POST /sign-in/email` still answers + `401 {"message":"Invalid email or password","code":"INVALID_EMAIL_OR_PASSWORD"}`, + measured identical on both sides of the change. + +Consumers that branch on the HTTP status are unaffected. Consumers that already +parse the ObjectStack `/admin/*` envelope now get the same shape everywhere in +the namespace, with no per-route knowledge required. + + diff --git a/packages/plugins/plugin-auth/src/admin-impersonate-endpoint.test.ts b/packages/plugins/plugin-auth/src/admin-impersonate-endpoint.test.ts index efde462447..fa021a6b55 100644 --- a/packages/plugins/plugin-auth/src/admin-impersonate-endpoint.test.ts +++ b/packages/plugins/plugin-auth/src/admin-impersonate-endpoint.test.ts @@ -243,15 +243,23 @@ describe('the other direction — a non-entitled caller is still refused', () => // "the gate said no". expect(res.status).toBe(401); - // ⚠️ Measured, and deliberately not dressed up: this refusal carries an - // EMPTY body — no ADR-0112 envelope, no `code`. It comes from the vendor's - // `adminMiddleware` (`APIError.fromStatus('UNAUTHORIZED')`), which runs - // before this endpoint's handler and is byte-identical on stock - // better-auth 1.7.1 for both `/admin/impersonate-user` and - // `/admin/set-role`. This card changes the AUTHORIZATION predicate, not the - // authentication middleware, so the shape is pinned as it is rather than - // asserted to be something it is not. - expect(await res.text()).toBe(''); + // ⚠️ This assertion USED to read `expect(await res.text()).toBe('')` — the + // vendor's `adminMiddleware` (`APIError.fromStatus('UNAUTHORIZED')`, no body + // argument) refused an anonymous caller with the EMPTY STRING under a + // `content-type: application/json` header, on every better-auth-native + // `/admin/` route. #9968 changed the AUTHORIZATION predicate only and pinned + // that shape as it was rather than dressing it up. + // + // #10349 closed it at the ONE seam every vendor route passes through + // (`AuthManager.handleRequest` → `vendor-admin-refusal-envelope.ts`), so the + // anonymous refusal now carries the ADR-0112 envelope — `code` AND `status` + // — and is byte-identical to what the ObjectStack raw `/admin/*` mounts + // answer. Authorization here is still untouched: the 403 assertions above + // and the 200 below are unchanged. + expect(JSON.parse(await res.text())).toEqual({ + success: false, + error: { code: 'UNAUTHENTICATED', message: 'Sign in first' }, + }); }); it('an org owner/admin who is NOT a platform admin is refused', async () => { diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 65dd241024..1044073db0 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -30,6 +30,7 @@ import { type AuthEventAuditSurface, } from './auth-session-audit.js'; import { SESSION_ERASURE_PATHS } from './session-tombstone.js'; +import { envelopeVendorAdminRefusal } from './vendor-admin-refusal-envelope.js'; import { ADMIN_SESSION_COOKIE_KEY, STOP_IMPERSONATING_PATH, @@ -3680,11 +3681,31 @@ export class AuthManager { // is left with an identity that still occupies the org roster and can no // longer sign in. Nothing tells the operator, and there is no way back. const endpointPath = this.betterAuthEndpointPath(request); - const response = + const vendorResponse = endpointPath !== undefined && SESSION_ERASURE_PATHS.has(endpointPath) ? await this.runSubjectErasureAtomically(runHandler) : await runHandler(); + // [#10349] The better-auth-native `/admin/` routes refuse an anonymous + // caller through the vendor's `adminMiddleware` + // (`APIError.fromStatus('UNAUTHORIZED')`, no body argument), so the refusal + // reaches the client as a 401 that announces `application/json` and carries + // the EMPTY STRING — no envelope, nothing to branch on. The ObjectStack raw + // `/admin/*` mounts answer the identical question with the ADR-0112 + // envelope and `code: 'UNAUTHENTICATED'` (`platform-admin-gate.ts`), and + // which of the two a caller gets depends only on which implementation + // happens to serve that route — an implementation detail, not a contract. + // + // This is the ONE seam every vendor route passes through, which is why the + // normalization belongs here and not in ten routes we do not own. It is + // scoped to the `/admin/` NAMESPACE (option C): the prefix test costs no new + // concept, because this method already discriminates on `endpointPath` twice + // above — `STOP_IMPERSONATING_PATH` and `SESSION_ERASURE_PATHS`. + // + // Status and admission are untouched; see the module header for the three + // narrowings and the measurement behind each. + const response = await envelopeVendorAdminRefusal(endpointPath, vendorResponse); + if (response.status >= 500) { try { const body = await response.clone().text(); diff --git a/packages/plugins/plugin-auth/src/platform-admin-gate.ts b/packages/plugins/plugin-auth/src/platform-admin-gate.ts index 1fcb18a1c2..635e882dfd 100644 --- a/packages/plugins/plugin-auth/src/platform-admin-gate.ts +++ b/packages/plugins/plugin-auth/src/platform-admin-gate.ts @@ -40,6 +40,21 @@ export type PlatformAdminVerdict = | { ok: true; actor: PlatformAdminActor } | { ok: false; refusal: PlatformAdminRefusal }; +/** + * The human half of the two refusals, keyed by the status that carries them. + * + * Lifted out of `judgePlatformAdmin` (whose bytes are unchanged) so the + * better-auth-native `/admin/` lane can answer an anonymous caller with the + * SAME body rather than a second string that merely looks the same today — + * see `vendor-admin-refusal-envelope.ts` (#10349). The machine half is not + * duplicated anywhere: it is ADR-0112's own derived-code map, + * `standardErrorCodeForHttpStatus`. + */ +export const PLATFORM_ADMIN_REFUSAL_MESSAGES: Readonly> = { + 401: 'Sign in first', + 403: 'Admin role required', +}; + /** * Is this session user a platform admin under ADR-0068 D2? * @@ -82,7 +97,10 @@ export function judgePlatformAdmin(session: unknown): PlatformAdminVerdict { ok: false, refusal: { status: 401, - body: { success: false, error: { code: 'UNAUTHENTICATED', message: 'Sign in first' } }, + body: { + success: false, + error: { code: 'UNAUTHENTICATED', message: PLATFORM_ADMIN_REFUSAL_MESSAGES[401] }, + }, }, }; } @@ -92,7 +110,10 @@ export function judgePlatformAdmin(session: unknown): PlatformAdminVerdict { ok: false, refusal: { status: 403, - body: { success: false, error: { code: 'PERMISSION_DENIED', message: 'Admin role required' } }, + body: { + success: false, + error: { code: 'PERMISSION_DENIED', message: PLATFORM_ADMIN_REFUSAL_MESSAGES[403] }, + }, }, }; } diff --git a/packages/plugins/plugin-auth/src/vendor-admin-refusal-envelope.test.ts b/packages/plugins/plugin-auth/src/vendor-admin-refusal-envelope.test.ts new file mode 100644 index 0000000000..2ec3c5449a --- /dev/null +++ b/packages/plugins/plugin-auth/src/vendor-admin-refusal-envelope.test.ts @@ -0,0 +1,268 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #10349 — the better-auth-native `/admin/` lane answers an anonymous caller +// with a bodyless 401. This file pins the fix in BOTH directions and at BOTH +// levels: +// +// • the pure normalizer, where the three narrowings are cheap to enumerate +// (empty body only · 401/403 only · `/admin/` only), and +// • the real `AuthManager.handleRequest` seam on the installed better-auth +// 1.7.1, where the vendor's `adminMiddleware` actually produces the refusal. +// +// ⛔ A refusal-only suite is not enough here and the lane has paid for that +// twice: an implementation that refuses EVERYONE passes every refusal +// assertion. So the ADMISSION direction is asserted on the same route with the +// same seam — a platform admin still gets 200 out of `/admin/impersonate-user` +// — and a non-`/admin/` refusal is asserted to come back byte-identical. +// +// ADR-0112 is `code` AND `status`; every refusal assertion below carries both. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { standardErrorCodeForHttpStatus } from '@objectstack/spec/api'; +import { ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; +import { + envelopeVendorAdminRefusal, + isVendorAdminPath, + VENDOR_ADMIN_PATH_PREFIX, +} from './vendor-admin-refusal-envelope'; +import { judgePlatformAdmin } from './platform-admin-gate'; +import { AuthManager } from './auth-manager'; +import { createMemoryEngine } from './impersonation-bearer-rotation.test'; + +// ─────────────────────────────────────────────────────────────────────────── +// The pure normalizer +// ─────────────────────────────────────────────────────────────────────────── + +/** The vendor's shape: a status, `application/json`, and the EMPTY STRING. */ +const vendorBodylessRefusal = (status: number): Response => + new Response('', { status, headers: { 'content-type': 'application/json' } }); + +const bodyOf = async (res: Response) => JSON.parse(await res.text()); + +describe('#10349 — the normalizer fills in a bodyless vendor /admin/ refusal', () => { + it('a bodyless 401 becomes the ADR-0112 envelope, status untouched', async () => { + const out = await envelopeVendorAdminRefusal('/admin/set-role', vendorBodylessRefusal(401)); + + expect(out.status).toBe(401); + expect(await bodyOf(out)).toEqual({ + success: false, + error: { code: 'UNAUTHENTICATED', message: 'Sign in first' }, + }); + }); + + it('a bodyless 403 becomes the envelope too — one rule over both refusal statuses', async () => { + const out = await envelopeVendorAdminRefusal('/admin/set-role', vendorBodylessRefusal(403)); + + expect(out.status).toBe(403); + expect(await bodyOf(out)).toEqual({ + success: false, + error: { code: 'PERMISSION_DENIED', message: 'Admin role required' }, + }); + }); + + it('the code is DERIVED from ADR-0112‘s own map, not written down here', async () => { + // If the derived-code map is ever re-pointed, this file must not be the + // place that keeps the old spelling alive. + for (const status of [401, 403] as const) { + const out = await envelopeVendorAdminRefusal('/admin/x', vendorBodylessRefusal(status)); + expect((await bodyOf(out)).error.code).toBe(standardErrorCodeForHttpStatus(status)); + } + }); + + it('the vendor lane and the ObjectStack lane now answer anonymous BYTE-IDENTICALLY', async () => { + // The asymmetry is the substance of this card, so its disappearance is + // asserted directly rather than inferred from two separate assertions that + // merely happen to agree today. + const objectStackLane = judgePlatformAdmin(null); + expect(objectStackLane.ok).toBe(false); + const raw = objectStackLane.ok ? undefined : objectStackLane.refusal; + + const vendorLane = await envelopeVendorAdminRefusal( + '/admin/impersonate-user', + vendorBodylessRefusal(401), + ); + + expect(vendorLane.status).toBe(raw!.status); + expect(await bodyOf(vendorLane)).toEqual(raw!.body); + }); + + // ── The three narrowings, each asserted by IDENTITY ────────────────────── + // + // `toBe(input)` — the same object, not an equal one. An implementation that + // rebuilt an "equivalent" response on these paths would still be changing + // headers and streams on surfaces this card does not own. + + it('a vendor refusal that DID say something is returned unchanged', async () => { + // The signed-in non-admin's real 403 on this stack. + const spoken = new Response( + JSON.stringify({ message: 'x', code: 'YOU_ARE_NOT_ALLOWED_TO_IMPERSONATE_USERS' }), + { status: 403, headers: { 'content-type': 'application/json' } }, + ); + + expect(await envelopeVendorAdminRefusal('/admin/impersonate-user', spoken)).toBe(spoken); + }); + + it('a non-refusal status is returned unchanged, even bodyless under /admin/', async () => { + // Measured: `/admin/oauth2/resources` is a bodyless 404 when oidcProvider + // is off, and `/admin/remove-user` can answer a semantic 409. Neither is + // this seam's to name. + for (const status of [200, 400, 404, 409, 500]) { + const untouched = vendorBodylessRefusal(status); + expect(await envelopeVendorAdminRefusal('/admin/anything', untouched)).toBe(untouched); + } + }); + + it('a non-/admin/ path is returned unchanged — this is option C, not option B', async () => { + for (const path of ['/sign-in/email', '/get-session', '/organization/add-member', '/admin', undefined]) { + const untouched = vendorBodylessRefusal(401); + expect(await envelopeVendorAdminRefusal(path, untouched)).toBe(untouched); + } + }); + + it('the namespace prefix is a namespace, not a route name', () => { + expect(VENDOR_ADMIN_PATH_PREFIX).toBe('/admin/'); + expect(isVendorAdminPath('/admin/set-role')).toBe(true); + expect(isVendorAdminPath('/admin/sso/register')).toBe(true); + expect(isVendorAdminPath('/admin')).toBe(false); + expect(isVendorAdminPath('/administrator/x')).toBe(false); + expect(isVendorAdminPath(undefined)).toBe(false); + }); + + it('headers the vendor attached survive, and content-length does not lie', async () => { + const withCookie = new Response('', { + status: 401, + headers: { 'content-type': 'application/json', 'content-length': '0', 'set-cookie': 'a=b' }, + }); + + const out = await envelopeVendorAdminRefusal('/admin/set-role', withCookie); + + expect(out.headers.get('set-cookie')).toBe('a=b'); + expect(out.headers.get('content-type')).toBe('application/json'); + const text = await out.text(); + const declared = out.headers.get('content-length'); + expect(declared === null || Number(declared) === new TextEncoder().encode(text).length).toBe(true); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The real seam, on the installed better-auth 1.7.1 +// ─────────────────────────────────────────────────────────────────────────── + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const PASSWORD = 'S3cure!Passw0rd-10349'; +const BASE = 'http://localhost:3000/api/v1/auth'; +const PS_ADMIN = 'ps_admin_full_access'; + +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), + }), + ); + +/** + * Anonymous probes across the vendor `/admin/` lane. Derived by hand rather + * than from the route table on purpose: this is a package-level test with no + * booted stack, and the DERIVED sweep over the live route table is the dogfood + * suite's job (`admin-route-nonadmin-refusal.dogfood.test.ts`). + */ +const VENDOR_ADMIN_PROBES: Array<[string, unknown]> = [ + ['/admin/impersonate-user', { userId: 'usr_probe' }], + ['/admin/set-role', { userId: 'usr_probe', role: 'admin' }], + ['/admin/revoke-user-sessions', { userId: 'usr_probe' }], + ['/admin/list-user-sessions', { userId: 'usr_probe' }], + ['/admin/update-user', { userId: 'usr_probe', data: { name: 'X' } }], +]; + +beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => vi.restoreAllMocks()); + +describe('#10349 — through AuthManager.handleRequest on the real vendor pipeline', () => { + it('every vendor /admin/ route refuses an anonymous caller 401 UNAUTHENTICATED', async () => { + const manager = makeManager(createMemoryEngine()); + + for (const [path, body] of VENDOR_ADMIN_PROBES) { + const res = await post(manager, path, body); + const text = await res.text(); + + // status AND code — a lone status cannot tell the vendor's authentication + // refusal from a validation error, and was green through the defect. + expect(res.status, `${path}: ${text}`).toBe(401); + expect(JSON.parse(text), `${path}`).toEqual({ + success: false, + error: { code: 'UNAUTHENTICATED', message: 'Sign in first' }, + }); + } + }, 60_000); + + it('a non-/admin/ vendor refusal keeps the vendor‘s own body — option C holds at the seam', async () => { + const manager = makeManager(createMemoryEngine()); + + const res = await post(manager, '/sign-in/email', { + email: 'nobody@example.com', + password: 'wrong-password-entirely', + }); + + expect(res.status).toBe(401); + // Measured before AND after the change: the vendor's flat `{message,code}`. + // If option C ever silently became option B, this is what would move. + expect(await bodyOf(res)).toEqual({ + message: 'Invalid email or password', + code: 'INVALID_EMAIL_OR_PASSWORD', + }); + }, 60_000); + + it('ADMISSION is unchanged — a platform admin still gets 200 from /admin/impersonate-user', async () => { + // ⛔ The load-bearing half. Everything above stays green on an + // implementation that refuses every caller; only this fails there. + const engine = createMemoryEngine(); + const manager = makeManager(engine); + + for (const [email, name] of [ + ['padmin.10349@example.com', 'Platform Admin'], + ['target.10349@example.com', 'Target'], + ]) { + await post(manager, '/sign-up/email', { email, password: PASSWORD, name }); + } + const rows = (engine.tables.get('sys_user') ?? []) as any[]; + const idFor = (email: string) => String(rows.find((r) => r.email === email)!.id); + const adminId = idFor('padmin.10349@example.com'); + const targetId = idFor('target.10349@example.com'); + + // ADR-0068 D2 grant — deliberately NOT the legacy `role` scalar. + await engine.insert('sys_permission_set', { id: PS_ADMIN, name: ADMIN_FULL_ACCESS }); + await engine.insert('sys_user_permission_set', { + user_id: adminId, + permission_set_id: PS_ADMIN, + organization_id: null, + }); + + const signIn = await post(manager, '/sign-in/email', { + email: 'padmin.10349@example.com', + password: PASSWORD, + }); + const bearer = signIn.headers.get('set-auth-token'); + expect(bearer, 'sign-in must mint a bearer or this test proves nothing').toBeTruthy(); + + const res = await post(manager, '/admin/impersonate-user', { userId: targetId }, bearer!); + + expect(res.status, await res.clone().text()).toBe(200); + expect((await bodyOf(res))?.user?.id).toBe(targetId); + }, 60_000); +}); diff --git a/packages/plugins/plugin-auth/src/vendor-admin-refusal-envelope.ts b/packages/plugins/plugin-auth/src/vendor-admin-refusal-envelope.ts new file mode 100644 index 0000000000..06ef895bf8 --- /dev/null +++ b/packages/plugins/plugin-auth/src/vendor-admin-refusal-envelope.ts @@ -0,0 +1,154 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * ADR-0112 envelope for the better-auth-native `/admin/` refusals (#10349). + * + * ## The defect, measured + * + * `/api/v1/auth/admin/*` is served by TWO implementations and answers the same + * question in two shapes. ObjectStack's raw Hono mounts (`create-user`, + * `set-user-password`, `unlock-user`, `import-users`, `ban-user`, `unban-user`, + * `oauth2/toggle-disabled`, `sso/*`) run `judgePlatformAdmin` and refuse an + * anonymous caller with the declared envelope — `401 { success: false, error: { + * code: 'UNAUTHENTICATED', message } }` (see `platform-admin-gate.ts`). The + * routes better-auth serves itself refuse the same caller through the vendor's + * `adminMiddleware`, which is `getAuthoritativeSessionFromCtx(ctx)` followed by + * `APIError.fromStatus('UNAUTHORIZED')` — no body argument at all. + * + * Measured on the installed better-auth 1.7.1, anonymous, through + * `AuthManager.handleRequest`, ten vendor-lane routes answered: + * + * POST /admin/impersonate-user -> 401 content-type: application/json body '' + * POST /admin/set-role -> 401 content-type: application/json body '' + * POST /admin/revoke-user-sessions -> 401 content-type: application/json body '' + * POST /admin/revoke-user-session -> 401 content-type: application/json body '' + * POST /admin/list-user-sessions -> 401 content-type: application/json body '' + * POST /admin/update-user -> 401 content-type: application/json body '' + * GET /admin/list-users -> 401 content-type: application/json body '' + * GET /admin/get-user -> 401 content-type: application/json body '' + * POST /admin/has-permission -> 401 content-type: application/json body '' + * POST /admin/stop-impersonating -> 401 content-type: application/json body '' + * + * The body is the EMPTY STRING, not an empty JSON object — which is why this + * module ADDS an envelope and never rewrites one. The `content-type` still + * announces `application/json`, so a client that believes the header and calls + * `JSON.parse` throws on the refusal instead of branching on it. A consumer + * cannot tell 401-means-sign-in from 401-means-anything-else without knowing, + * per route, which of the two implementations happens to serve it — and that + * split is an implementation detail, not a contract. + * + * ## What is normalized, and what is deliberately left alone + * + * Scope is the `/admin/` NAMESPACE only (triage's option C, not option B). + * Statuses are unchanged and admission is unchanged: this module never turns a + * 2xx into a refusal and never turns a refusal into a 2xx. It fills in a body + * that was empty, and nothing else. + * + * Three deliberate narrowings, each of which a broader rule would have broken: + * + * 1. **Empty body only.** A vendor refusal that DID say something keeps saying + * it byte-for-byte — the signed-in non-admin's + * `403 {"message":…,"code":"YOU_ARE_NOT_ALLOWED_TO_IMPERSONATE_USERS"}` is + * the vendor's own denial vocabulary and is what `admin-route-nonadmin- + * refusal.dogfood.test.ts` asserts on. Rewriting it would be a second, + * larger contract change smuggled in behind this one. + * 2. **Refusal statuses only** (401 / 403). `/admin/oauth2/*` answers 404 with + * an empty body when the `oidcProvider` plugin is off, and a 404 discloses + * nothing that needs a code; a semantic 4xx the vendor owns (409, 400) is + * not this seam's to name. + * 3. **The `/admin/` prefix.** Non-`/admin/` vendor routes are untouched, which + * is what makes this option C. `POST /sign-in/email` already answers + * `401 {"message":"Invalid email or password","code":"INVALID_EMAIL_OR_ + * PASSWORD"}` and stays exactly that. + * + * ## Why the code is DERIVED rather than written down here + * + * `standardErrorCodeForHttpStatus` (`@objectstack/spec/api`) is the one place a + * code is spelled for a producer that knows only a status — ADR-0112's own + * derived-code map, `401 -> UNAUTHENTICATED`, `403 -> PERMISSION_DENIED`. Using + * it means this module registers no vocabulary of its own and cannot drift from + * the catalog: there is no string literal here to drift. + * + * The messages come from `platform-admin-gate.ts`, the module that already owns + * ObjectStack's refusal wording. That is not decoration — it is what makes the + * two lanes answer an anonymous caller with BYTE-IDENTICAL bodies rather than + * with two independently-maintained strings that merely look alike today. + */ + +import { standardErrorCodeForHttpStatus } from '@objectstack/spec/api'; +import { PLATFORM_ADMIN_REFUSAL_MESSAGES } from './platform-admin-gate.js'; + +/** + * The `/admin/` namespace, in better-auth's own `ctx.path` spelling — the same + * spelling `AuthManager.betterAuthEndpointPath` returns and the same one + * `SESSION_ERASURE_PATHS` and the stop-impersonating recovery seam are keyed + * on. The trailing slash is load-bearing: it addresses the namespace and not a + * hypothetical route literally named `/admin`. + */ +export const VENDOR_ADMIN_PATH_PREFIX = '/admin/'; + +/** + * The statuses this seam will name. Exactly the two `judgePlatformAdmin` + * itself emits, and exactly the two the `/admin/` dogfood sweep asserts a + * non-admin receives. + * + * 403 is included although no bodyless 403 occurs on the installed vendor + * version (measured: the signed-in non-admin's 403 carries the vendor's + * `YOU_ARE_NOT_ALLOWED_*` body). One rule over both refusal statuses means a + * vendor change that starts refusing bodylessly with the other one cannot + * silently re-open this hole — which is the failure mode the card records: + * the gap was known, documented, and tracked by nothing. + */ +const NORMALIZED_REFUSAL_STATUSES: ReadonlySet = new Set([401, 403]); + +/** Is `endpointPath` inside the better-auth `/admin/` namespace? */ +export function isVendorAdminPath(endpointPath: string | undefined): boolean { + return endpointPath !== undefined && endpointPath.startsWith(VENDOR_ADMIN_PATH_PREFIX); +} + +/** + * Give a bodyless vendor-lane `/admin/` refusal the ADR-0112 envelope. + * + * Returns the response UNCHANGED — the same object, not a copy — whenever any + * of the three narrowings above applies, so the untouched paths are untouched + * by identity and a test can assert that with `toBe`. + * + * Headers are carried over rather than rebuilt: better-auth attaches + * `Set-Cookie` to refusals on some paths, and dropping them would change + * behaviour well outside this card. Only `content-type` is asserted (the + * vendor already claims `application/json`; now it is telling the truth) and + * `content-length` is dropped, since the body length changed. + */ +export async function envelopeVendorAdminRefusal( + endpointPath: string | undefined, + response: Response, +): Promise { + if (!isVendorAdminPath(endpointPath)) return response; + if (!NORMALIZED_REFUSAL_STATUSES.has(response.status)) return response; + + let body: string; + try { + body = await response.clone().text(); + } catch { + // Unreadable body (already-disturbed stream) → leave it exactly as it is. + // A refusal we cannot inspect is not one we may rewrite. + return response; + } + if (body !== '') return response; + + const code = standardErrorCodeForHttpStatus(response.status); + const message = + PLATFORM_ADMIN_REFUSAL_MESSAGES[response.status as 401 | 403] ?? + /* istanbul ignore next — unreachable while the set above holds two members */ + 'Refused'; + + const headers = new Headers(response.headers); + headers.set('content-type', 'application/json'); + headers.delete('content-length'); + + return new Response(JSON.stringify({ success: false, error: { code, message } }), { + status: response.status, + statusText: response.statusText, + headers, + }); +} diff --git a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts index c1d1433459..fadf9f0175 100644 --- a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts +++ b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts @@ -75,7 +75,16 @@ // bridges while SSO is off). That is what proves the member's 403 is a // gate verdict and not a payload the server rejects for everyone. // -// `better-auth-gate` (9 routes) — refusal side only, DELIBERATELY. On this +// `better-auth-gate` (9 routes) — refusal side only, DELIBERATELY. The +// ANONYMOUS half is now the full ADR-0112 pin, identical to the bucket +// above — `401` and `code: 'UNAUTHENTICATED'`. It was `[401, 403].includes( +// status)` until #10349, because the vendor's `adminMiddleware` answered an +// anonymous caller with a bodyless 401 and there was no code to assert; +// `AuthManager.handleRequest` now envelopes that refusal at the one seam +// every vendor route passes through, so the asymmetry this file used to +// DOCUMENT is closed and this bucket pins the closure. +// +// The signed-in-non-admin half stays the vendor's own vocabulary. On this // stack the platform admin is refused these routes too, with the same // `YOU_ARE_NOT_ALLOWED_TO_*` code as the member. That is not a harness // artifact: better-auth's admin plugin authorizes on the legacy @@ -549,11 +558,23 @@ describe('#9482 C9: every derived /admin/ route refuses a non-admin', () => { expect(routes.length, 'no better-auth-gate routes were derived').toBeGreaterThan(0); for (const route of routes) { + // ── #10349: this used to be `[401, 403].includes(anon.status)` ──────── + // + // It had to be, because there was no code to assert: the vendor's + // `adminMiddleware` refused an anonymous caller with a bodyless 401 — + // `content-type: application/json` and the EMPTY STRING — so this suite + // DOCUMENTED the asymmetry against the `objectstack-gate` bucket above + // rather than closing it, and nothing tracked closing it. + // + // `AuthManager.handleRequest` now gives the vendor lane's `/admin/` + // refusals the ADR-0112 envelope (`vendor-admin-refusal-envelope.ts`), + // so the two buckets answer an anonymous caller identically and this + // assertion is the FIX'S OWN falsifiable pin: status AND code, exactly + // as the ObjectStack bucket asserts them thirty lines up. Loosening it + // back re-opens the gap silently. const anon = await fire(route, undefined); - expect( - [401, 403].includes(anon.status), - `${route} anonymous should be refused, got ${anon.status} ${anon.body}`, - ).toBe(true); + expect(anon.status, `${route} anonymous: ${anon.body}`).toBe(401); + expect(anon.code, `${route} anonymous code: ${anon.body}`).toBe('UNAUTHENTICATED'); const member = await fire(route, memberToken); expect( From be9b5a9fdcd94cb5434f7c0542a7e54d674dc806 Mon Sep 17 00:00:00 2001 From: os-warren Date: Fri, 21 Aug 2026 11:39:37 +0000 Subject: [PATCH 2/2] test(dogfood): record the #10792 member-arm exception the tightened bucket exposed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `better-auth-gate` bucket's member-arm vocabulary check was guarded by `if (member.code !== undefined)`, and on every bodyless refusal the code WAS undefined — so for those routes the check had never executed. Giving the vendor lane an envelope made it executable, and it went red on the first run: `/admin/remove-user` answers a SIGNED-IN member `401 UNAUTHENTICATED`, while `set-role` and `update-user` answer the same bearer `403 YOU_ARE_NOT_ALLOWED_*`. Measured on the booted showcase stack and controlled hermetically: the same three fires against the in-memory engine give the member 403, both with no `transaction` on the engine and with a pass-through one. So it is the real erasure transaction (#7724, `SESSION_ERASURE_PATHS`) that the session re-read inside `adminMiddleware` does not survive. Filed as #10792. Recorded here as an ADDITIONAL accepted code for that one route, never as a pin — the same reasoning the platform-admin arm below already carries. Pinning today's 401 would turn the fix red, pinning the 403 is red today, and widening the vocabulary for every route would let the next one drift in silence. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- ...min-route-nonadmin-refusal.dogfood.test.ts | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts index fadf9f0175..8ba8b6f0e7 100644 --- a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts +++ b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts @@ -584,12 +584,33 @@ describe('#9482 C9: every derived /admin/ route refuses a non-admin', () => { // When the vendor answers with a body, it must be its own denial // vocabulary — not a validation error, which would mean the request died // before the gate and this assertion measured nothing. + // + // ⚠️ #10792, found the moment #10349 made this branch executable at all. + // It was guarded by `member.code !== undefined`, and the code WAS + // undefined on every bodyless refusal — so for those routes this check + // had never once run. On the first run where it did, `remove-user` came + // back `401 UNAUTHENTICATED` for a SIGNED-IN member while its siblings + // `set-role` and `update-user` answered the same bearer + // `403 YOU_ARE_NOT_ALLOWED_*`: on that path alone the session is re-read + // inside the #7724 erasure transaction and comes back empty, so + // authentication answers a question authorization should have. + // + // Recorded as an ADDITIONAL accepted code for that one route, never as a + // pin — same reasoning as the platform-admin arm below. Pinning today's + // 401 would turn the fix red; pinning the 403 is red today; and widening + // the vocabulary for EVERY route would let the next route drift into the + // same state in silence. Delete this arm when #10792 closes. + const KNOWN_AUTHN_BEFORE_AUTHZ = 'POST /api/v1/auth/admin/remove-user'; // #10792 + const denialCodes = + route === KNOWN_AUTHN_BEFORE_AUTHZ + ? /^(YOU_ARE_NOT_ALLOWED|UNAUTHENTICATED$)/ + : /^YOU_ARE_NOT_ALLOWED/; if (member.code !== undefined) { expect( member.code, `${route} member: refused with ${member.code}, which is not a denial code. A ` + `VALIDATION_ERROR here means the payload never reached the gate.`, - ).toMatch(/^YOU_ARE_NOT_ALLOWED/); + ).toMatch(denialCodes); } } // ⛔ No allowed-side assertion in this bucket — see the header: the platform