diff --git a/.changeset/21941-access-guards-fail-closed.md b/.changeset/21941-access-guards-fail-closed.md new file mode 100644 index 00000000000..957a7cc5c60 --- /dev/null +++ b/.changeset/21941-access-guards-fail-closed.md @@ -0,0 +1,17 @@ +--- +'@objectstack/runtime': minor +'@objectstack/plugin-auth': minor +--- + +fix(runtime, plugin-auth)!: two access guards refuse, instead of admitting, when their own read cannot answer + +Clause-②: no (narrowing) + + + +**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention: a request that one of these two guards let through only because the guard's own read faulted is now refused. Nothing an author or caller writes changes shape. + +- **`@objectstack/runtime` — the dispatcher's environment-membership gate.** When its `sys_environment_member` read throws, or no ObjectQL engine resolves on the request's kernel, the request is refused with `503 SERVICE_UNAVAILABLE` — the `AuthzStoreUnavailableError` answer the identity step and the domain gates already give an authorization input they could not read. Before, the gate logged at debug level and let the request through. A member is still admitted, and a non-member is still refused with `403 PROJECT_MEMBERSHIP_REQUIRED`. An engine whose registry does not register `sys_environment_member` declares the gate inapplicable, and nothing is read. +- **`@objectstack/plugin-auth` — the organization slug guard** (`organizationHooks.beforeUpdateOrganization`). When its `sys_organization` or `sys_environment` read throws, the organization update is refused with `503 SERVICE_UNAVAILABLE`. Before, the hook ended without refusing and the slug changed. A slug change while an active environment references the organization is still refused (`403 FORBIDDEN`), and any other change is still allowed. An engine that does not register `sys_environment` — the open-source composition, where it is a cloud-provided object — declares the guard inapplicable from its registry (`getSchema`): nothing is read and the update proceeds as before. Without a data engine the guard does not apply either. + +What changes for you: nothing in what you write. A `503 SERVICE_UNAVAILABLE` on these doors is a store outage that used to be hidden behind an admitted request; it clears when the store answers again. diff --git a/packages/plugins/plugin-auth/src/auth-manager.org-slug-guard-system-context.test.ts b/packages/plugins/plugin-auth/src/auth-manager.org-slug-guard-system-context.test.ts index b228ec74297..59fd34e8c30 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.org-slug-guard-system-context.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.org-slug-guard-system-context.test.ts @@ -18,13 +18,19 @@ * 2. the guard KEEPS refusing on an engine that refuses a principal-less, * non-system context — its reads are not one, so the catch below never * turns the refusal into a skipped guard; - * 3. the catches around the two reads are unchanged: a read that throws ends - * the hook without refusing, as it did before. That is pre-existing - * behaviour, pinned as it stands, not endorsed here. + * 3. the guard FAILS CLOSED when a read it makes cannot answer (#21941): a + * read that throws is refused with better-auth's `SERVICE_UNAVAILABLE` + * (503) — never the guard's own `FORBIDDEN` (403), and never "ends the hook + * without refusing" — while an engine that does not register + * `sys_environment` (the open-source composition) declares the guard + * inapplicable without reading anything. The real ObjectQL registry + * answers that question in the last section. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { ObjectQL, assertEngineFindOnePredicate } from '@objectstack/objectql'; import { AuthManager } from './auth-manager'; +import { authIdentityObjects } from './manifest'; vi.mock('better-auth', () => ({ betterAuth: vi.fn(() => ({ handler: vi.fn(), api: {} })), @@ -172,8 +178,51 @@ describe('organization slug guard — on an engine that refuses a principal-less }); }); -describe('organization slug guard — the catches around the reads are unchanged (pre-existing)', () => { - it('an organization read that throws ends the hook without refusing, as before', async () => { +/** + * A read that could not answer, asserted on its envelope: better-auth's + * `SERVICE_UNAVAILABLE` / 503, naming the object whose read failed — and + * explicitly NOT the guard's `FORBIDDEN` / 403, which is a verdict about the + * slug change that the guard never reached. + */ +async function expectReadFaultRefusal(attempt: Promise, object: string) { + const err = (await attempt.then( + () => undefined, + (e: unknown) => e, + )) as { status?: unknown; statusCode?: unknown; body?: { message?: unknown }; cause?: unknown } | undefined; + expect(err, 'the slug change was let through on a read that could not answer').toBeTruthy(); + expect(err?.statusCode).toBe(503); + expect(err?.status).toBe('SERVICE_UNAVAILABLE'); + expect(String(err?.body?.message)).toContain(`\`${object}\``); + return err; +} + +/** The registration answer an ObjectQL engine gives through `getSchema`. */ +const schemaFor = (...registered: string[]) => vi.fn((object: string) => (registered.includes(object) ? { name: object } : undefined)); + +/** + * An engine double that answers the registration question (`getSchema`) beside + * its two reads. Its `findOne` keeps ObjectQL's own predicate contract + * (`assertEngineFindOnePredicate`), so it is no looser than the engine it + * stands in for. + */ +function registeringEngine( + registered: string[], + answers: { findOne: () => Promise; find: () => Promise }, +) { + return { + getSchema: schemaFor(...registered), + findOne: vi.fn(async (object: string, q?: Query) => { + assertEngineFindOnePredicate(object, q as never); + return answers.findOne(); + }), + find: vi.fn(answers.find), + }; +} + +describe('organization slug guard — fails closed when a read cannot answer (#21941)', () => { + it('an organization read that throws is refused (503), and the environment read is never made', async () => { + // SUPERSEDED PIN, quoted — what the guard answered before: + // await expect(update('acme-new')).resolves.toBeUndefined(); // ends the hook without refusing const engine = { findOne: vi.fn(async () => { throw new Error('store unavailable'); @@ -181,11 +230,25 @@ describe('organization slug guard — the catches around the reads are unchanged find: vi.fn(async () => ENVS), }; const update = await slugGuard(engine); - await expect(update('acme-new')).resolves.toBeUndefined(); + const err = await expectReadFaultRefusal(update('acme-new'), 'sys_organization'); + expect((err?.cause as Error | undefined)?.message).toBe('store unavailable'); expect(engine.find).not.toHaveBeenCalled(); }); - it('an environment read that throws ends the hook without refusing, as before', async () => { + it('an environment read that throws is refused (503) — on an engine that registers the object', async () => { + // SUPERSEDED PIN, quoted — `find` threw and the hook resolved `undefined`. + const engine = registeringEngine(['sys_organization', 'sys_environment'], { + findOne: async () => ORG, + find: async () => { + throw new Error('connection reset'); + }, + }); + const update = await slugGuard(engine); + await expectReadFaultRefusal(update('acme-new'), 'sys_environment'); + expect(engine.find).toHaveBeenCalledTimes(1); + }); + + it('an environment read that throws on an engine whose registry cannot be asked is refused too — it is asked, and the fault refuses', async () => { const engine = { findOne: vi.fn(async () => ORG), find: vi.fn(async () => { @@ -193,6 +256,71 @@ describe('organization slug guard — the catches around the reads are unchanged }), }; const update = await slugGuard(engine); + await expectReadFaultRefusal(update('acme-new'), 'sys_environment'); + }); + + it('a healthy read on a registered object still refuses the change while an active environment references the org, and allows it otherwise', async () => { + const refusing = registeringEngine(['sys_organization', 'sys_environment'], { + findOne: async () => ORG, + find: async () => ENVS, + }); + await expectSlugRefusal((await slugGuard(refusing))('acme-new')); + const allowing = registeringEngine(['sys_organization', 'sys_environment'], { + findOne: async () => ORG, + find: async () => [{ id: 'e2', status: 'archived' }], + }); + await expect((await slugGuard(allowing))('acme-new')).resolves.toBeUndefined(); + }); + + it('an engine that does not register `sys_environment` declares the guard inapplicable — nothing is read', async () => { + const engine = registeringEngine(['sys_organization'], { + findOne: async () => { + throw new Error('must not be read'); + }, + find: async () => { + throw new Error('must not be read'); + }, + }); + const update = await slugGuard(engine); + await expect(update('acme-new')).resolves.toBeUndefined(); + expect(engine.getSchema).toHaveBeenCalledWith('sys_environment'); + expect(engine.findOne).not.toHaveBeenCalled(); + expect(engine.find).not.toHaveBeenCalled(); + }); +}); + +describe('organization slug guard — the real ObjectQL registry answers the composition question', () => { + /** A real engine holding exactly the identity objects this package registers — no `sys_environment`. */ + function engineWithAuthObjects(): ObjectQL { + const engine = new ObjectQL({ logger: { debug() {}, info() {}, warn() {}, error() {}, child() { return this; } } } as never); + for (const object of authIdentityObjects) { + engine.registry.registerObject(object as never, '@objectstack/plugin-auth'); + } + return engine; + } + + it('this package registers no `sys_environment`, so on its own object set the guard does not apply and reads nothing', async () => { + const engine = engineWithAuthObjects(); + expect(engine.getSchema('sys_organization'), 'POSITIVE CONTROL: the registry does answer for an object this package registers').toBeTruthy(); + expect(engine.getSchema('sys_environment')).toBeUndefined(); + const findOne = vi.spyOn(engine, 'findOne'); + const find = vi.spyOn(engine, 'find'); + const update = await slugGuard(engine); await expect(update('acme-new')).resolves.toBeUndefined(); + expect(findOne).not.toHaveBeenCalled(); + expect(find).not.toHaveBeenCalled(); + }); + + it('once `sys_environment` is registered the guard reads, and a real engine fault on that read refuses (503)', async () => { + // No driver is registered, so the engine itself cannot serve the read: a + // genuine fault of the real engine, not a double's. + const engine = engineWithAuthObjects(); + engine.registry.registerObject( + { name: 'sys_environment', label: 'Environment', fields: { organization_id: { name: 'organization_id', type: 'text' } } } as never, + '@objectstack/test-cloud-objects', + ); + expect(engine.getSchema('sys_environment')).toBeTruthy(); + const update = await slugGuard(engine); + await expectReadFaultRefusal(update('acme-new'), 'sys_organization'); }); }); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 97a4838a6d0..c9754aef54d 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1275,6 +1275,28 @@ async function smsQuotaExceededApiError(message: string): Promise { return new APIError(SMS_QUOTA_EXCEEDED_CODE, { message }); } +/** + * [#21941] The organization slug guard's answer when one of its own reads + * cannot answer: `503 SERVICE_UNAVAILABLE`, the existing ADR-0112 code for a + * store that is temporarily down. + * + * Not the guard's `403 FORBIDDEN`: a read that could not answer is not a + * verdict about the slug change, so it must not wear the refusal's words — and + * never "allowed", which is what swallowing the fault used to answer. The + * driver's own failure rides `cause`, so its diagnostic is not lost. + */ +async function slugGuardReadFaultApiError(object: string, cause: unknown): Promise { + const { APIError } = await import('better-auth/api'); + const err = new APIError('SERVICE_UNAVAILABLE', { + message: + `The organization slug guard could not read \`${object}\`, so whether an active environment ` + + `still references this organization was never determined, and the slug was not changed. ` + + `This is a server-side outage, not a refusal of the new slug: retry once the data store is reachable.`, + }); + (err as { cause?: unknown }).cause = cause; + return err; +} + export class AuthManager { private auth: Auth | null = null; /** @@ -3334,13 +3356,35 @@ export class AuthManager { const orgId = member?.organizationId; if (!newSlug || !orgId) return; + // [#21941] The guard reads only what this composition registers, + // and FAILS CLOSED when a registered read cannot answer. + // + // - NO DATA ENGINE ⇒ the guard does not apply. Only a standalone + // `AuthManager` lacks one (`AuthPlugin` declares ObjectQL a hard + // dependency), and it then runs on better-auth's in-memory store: + // no engine registers `sys_environment`, so no environment can + // reference the organization. + // - `sys_environment` NOT REGISTERED ⇒ the guard does not apply, + // for the same reason, and nothing is read. It is a cloud-provided + // object (`CLOUD_PROVIDED_OBJECT_NAMES`), so this is the + // open-source composition's answer. The question is the + // registry's own (`getSchema`), never a caught throw; an engine + // that cannot be asked reads as before — it is asked, and a fault + // refuses. + // - A REGISTERED READ THAT THROWS ⇒ refused, `503 + // SERVICE_UNAVAILABLE` (`slugGuardReadFaultApiError`). It used to + // end the hook without refusing, which let the slug change through + // whenever either read faulted. + const rawEngine = this.config.dataEngine; + if (!rawEngine) return; + const schemaOf = (rawEngine as { getSchema?: (object: string) => unknown }).getSchema; + if (typeof schemaOf === 'function' && !schemaOf.call(rawEngine, 'sys_environment')) return; + // Both reads run as the platform (`withSystemContext`): this hook // IS the slug guard — the organization id is the `where`, not the // reader — so neither read reaches the engine with no principal // and no opt-in (the security middleware's principal-less - // hand-off, ADR-0096). The catches below are unchanged. - const rawEngine = this.config.dataEngine; - if (!rawEngine) return; + // hand-off, ADR-0096). const dataEngine = withSystemContext(rawEngine) as any; let currentSlug: string | undefined; @@ -3349,8 +3393,8 @@ export class AuthManager { where: { id: orgId }, }); currentSlug = current?.slug; - } catch { - return; + } catch (err) { + throw await slugGuardReadFaultApiError('sys_organization', err); } if (!currentSlug || currentSlug === newSlug) return; @@ -3362,8 +3406,8 @@ export class AuthManager { activeEnvs = (envs ?? []).filter( (e: any) => e?.status !== 'archived' && e?.status !== 'failed', ).length; - } catch { - return; + } catch (err) { + throw await slugGuardReadFaultApiError('sys_environment', err); } if (activeEnvs > 0) { diff --git a/packages/runtime/src/domains/meta-verb-fallthrough.test.ts b/packages/runtime/src/domains/meta-verb-fallthrough.test.ts index 228f24d3089..2e0d794542a 100644 --- a/packages/runtime/src/domains/meta-verb-fallthrough.test.ts +++ b/packages/runtime/src/domains/meta-verb-fallthrough.test.ts @@ -129,7 +129,12 @@ function boot() { } as any; return { - dispatcher: new HttpDispatcher(kernel), + // The environment-membership gate is not this file's subject, and this + // fixture composes no ObjectQL engine — which the gate refuses as an + // outage (503) rather than letting the request through (#21941). It is + // switched off here the way the dispatcher option documents for tests + // that seed no membership. + dispatcher: new HttpDispatcher(kernel, undefined, { enforceProjectMembership: false }), getMetaItem, saveMetaItem, deleteMetaItem, diff --git a/packages/runtime/src/http-dispatcher.membership-skip-boundary.test.ts b/packages/runtime/src/http-dispatcher.membership-skip-boundary.test.ts index 48aff0c8047..8ed92267e66 100644 --- a/packages/runtime/src/http-dispatcher.membership-skip-boundary.test.ts +++ b/packages/runtime/src/http-dispatcher.membership-skip-boundary.test.ts @@ -51,7 +51,14 @@ function makeGate() { const objectql = { find, getObjects: vi.fn().mockReturnValue({}), - registry: { getObject: vi.fn().mockReturnValue(null), getRegisteredTypes: vi.fn().mockReturnValue([]) }, + // The engine REGISTERS `sys_environment_member`: the gate asks the + // registry before it reads, and an engine that registers no such object + // declares the gate inapplicable (#21941) — which would make "skipped" + // and "checked" both return `null` again. + registry: { + getObject: vi.fn((name: string) => (name === 'sys_environment_member' ? { name } : null)), + getRegisteredTypes: vi.fn().mockReturnValue([]), + }, }; const auth = { getApi: async () => ({ diff --git a/packages/runtime/src/http-dispatcher.membership-system-context.test.ts b/packages/runtime/src/http-dispatcher.membership-system-context.test.ts index e603a922124..131168dba72 100644 --- a/packages/runtime/src/http-dispatcher.membership-system-context.test.ts +++ b/packages/runtime/src/http-dispatcher.membership-system-context.test.ts @@ -18,13 +18,20 @@ * 2. the gate KEEPS its answers on an engine that refuses a principal-less, * non-system context — the read is not one, so the non-member is still * refused rather than waved through by the catch below; - * 3. the catch around the read is unchanged: a read that throws lets the - * request through (`null`), as it did before. That is pre-existing - * behaviour, pinned as it stands, not endorsed here. + * 3. the gate FAILS CLOSED when its read cannot answer (#21941): a read that + * throws, or no engine at all, raises `AuthzStoreUnavailableError` — + * `503 SERVICE_UNAVAILABLE`, naming `sys_environment_member` — instead of + * letting the request through (`null`), and never answers the membership + * `403`; an engine whose registry does not register the object declares + * the gate inapplicable without reading. The last section drives the three + * answers through the real plugin route and its envelope. */ import { describe, it, expect, vi } from 'vitest'; +import { ObjectKernel, isAuthzStoreUnavailableError } from '@objectstack/core'; +import { ApiErrorSchema, BaseResponseSchema } from '@objectstack/spec/api'; import { HttpDispatcher } from './http-dispatcher.js'; +import { createDispatcherPlugin } from './dispatcher-plugin.js'; const USER = 'user-1'; const ENV = 'env-private'; @@ -44,15 +51,23 @@ function isPrincipalLessNonSystem(ctx: Record): boolean { return positions.length === 0 && permissions.length === 0 && !ctx.userId && ctx.isSystem !== true; } -function makeDispatcher(find: (...args: FindCall) => Promise) { - const ql = { find: vi.fn(find) }; +/** + * `registry` is the engine's own registration answer: omitted, the engine is + * one whose registry cannot be asked (it is read as before); given, the gate + * asks it before reading. `ql: null` is a kernel that resolves no engine. + */ +function makeDispatcher( + find: (...args: FindCall) => Promise, + opts: { registry?: { getObject: (name: string) => unknown }; noEngine?: boolean } = {}, +) { + const ql = { find: vi.fn(find), ...(opts.registry ? { registry: opts.registry } : {}) }; const kernel: any = { context: { getService: (name: string) => { if (name === 'auth') { return { getApi: async () => ({ getSession: async () => ({ user: { id: USER } }) }) }; } - if (name === 'objectql') return ql; + if (name === 'objectql') return opts.noEngine ? null : ql; return null; }, }, @@ -114,17 +129,167 @@ describe('environment-membership gate — on an engine that refuses a principal- }); }); -describe('environment-membership gate — the catch around the read is unchanged (pre-existing)', () => { - it('a read that throws lets the request through (null), as before', async () => { - const debug = vi.spyOn(console, 'debug').mockImplementation(() => {}); - try { - const { ql, check } = makeDispatcher(async () => { - throw new Error('control-plane store unavailable'); - }); - expect(await check()).toBeNull(); - expect(ql.find).toHaveBeenCalledTimes(1); - } finally { - debug.mockRestore(); - } +/** Settle to the rejection, or to `undefined` when the call RESOLVED. */ +const rejectionOf = (p: Promise) => p.then(() => undefined, (e: unknown) => e); + +/** The outage the gate raises when its read cannot answer — asserted on its envelope fields, not on the throw alone. */ +function expectStoreUnavailable(err: any) { + expect(err, 'the gate RESOLVED — a read that could not answer let the request through').toBeDefined(); + expect(isAuthzStoreUnavailableError(err)).toBe(true); + expect(err.code).toBe('SERVICE_UNAVAILABLE'); + expect(err.status).toBe(503); + expect(err.object).toBe('sys_environment_member'); +} + +const registered = { getObject: (name: string) => (name === 'sys_environment_member' ? { name } : undefined) }; +const unregistered = { getObject: (_name: string) => undefined }; + +describe('environment-membership gate — fails closed when its read cannot answer (#21941)', () => { + it('a read that throws is refused as a store outage (503), not let through', async () => { + // SUPERSEDED PIN, quoted — what the gate answered before: + // expect(await check()).toBeNull(); // a read that throws lets the request through + const { ql, check } = makeDispatcher(async () => { + throw new Error('control-plane store unavailable'); + }); + const err: any = await rejectionOf(check()); + expectStoreUnavailable(err); + expect(String(err.cause?.message)).toBe('control-plane store unavailable'); + expect(ql.find).toHaveBeenCalledTimes(1); + }); + + it('the same on an engine whose registry registers the object — a fault on a registered read refuses', async () => { + const { ql, check } = makeDispatcher(async () => { + throw new Error('connection reset'); + }, { registry: registered }); + expectStoreUnavailable(await rejectionOf(check())); + expect(ql.find).toHaveBeenCalledTimes(1); + }); + + it('a refusal by the engine itself is not a membership verdict either — it is the outage, not the 403', async () => { + const { check } = makeDispatcher(async () => { + throw Object.assign(new Error('[Security] Access denied'), { code: 'PERMISSION_DENIED', status: 403 }); + }); + expectStoreUnavailable(await rejectionOf(check())); + }); + + it('no engine resolved on the request kernel is refused the same way — the read cannot be made', async () => { + // SUPERSEDED PIN, quoted — `if (!ql) return null; // No QL — cannot enforce; fail open.` + const { ql, check } = makeDispatcher(async () => [], { noEngine: true }); + expectStoreUnavailable(await rejectionOf(check())); + expect(ql.find).not.toHaveBeenCalled(); + }); + + it('a healthy read on a registered object still admits a member and still refuses a non-member', async () => { + const member = makeDispatcher(async () => [{ id: 'm1' }], { registry: registered }); + expect(await member.check()).toBeNull(); + const stranger = makeDispatcher(async () => [], { registry: registered }); + expectMembershipRefusal(await stranger.check()); + }); + + it('an engine that does not register `sys_environment_member` declares the gate inapplicable — nothing is read', async () => { + // The composition's own answer, from the registry: no membership object, + // so no membership to judge. Not a swallowed fault — the read never runs. + const { ql, check } = makeDispatcher(async () => { + throw new Error('must not be read'); + }, { registry: unregistered }); + expect(await check()).toBeNull(); + expect(ql.find).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// The door: the three answers through the real plugin route and its envelope +// --------------------------------------------------------------------------- + +describe('environment-membership gate — on the wire (real plugin route, real envelope)', () => { + type Membership = 'member' | 'non-member' | 'read-faults'; + + /** + * A REAL kernel as a host hands the dispatcher: a resolver that scopes every + * request to ENV, an `auth` service that signs USER in, and an engine that + * answers every identity read with no rows — so the identity step completes + * cleanly and the ONLY read that can fault is the membership one. + */ + function kernelWith(membership: Membership): ObjectKernel { + const kernel = new ObjectKernel({ skipSystemValidation: true, gracefulShutdown: false } as any); + kernel.registerService('kernel-resolver', { + resolveKernel: (ctx: any, k: any) => { + ctx.environmentId = ENV; + return k; + }, + }); + kernel.registerService('auth', { + getApi: async () => ({ getSession: async () => ({ user: { id: USER }, session: { userId: USER } }) }), + }); + kernel.registerService('objectql', { + registry: registered, + find: async (object: string) => { + if (object !== 'sys_environment_member') return []; + if (membership === 'read-faults') throw new Error('control-plane store unavailable'); + return membership === 'member' ? [{ id: 'm1' }] : []; + }, + }); + return kernel; + } + + async function mountOn(kernel: ObjectKernel) { + const handlers: Record any> = {}; + const rec = (verb: string) => (path: string, handler: any) => { + handlers[`${verb} ${path}`] = handler; + }; + const server = { get: rec('GET'), post: rec('POST'), put: rec('PUT'), delete: rec('DELETE'), patch: rec('PATCH') }; + const plugin = createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false, enforceProjectMembership: true }); + await plugin.start?.({ + getKernel: () => kernel, + getService: (n: string) => (n === 'http.server' ? server : undefined), + environmentId: undefined, + logger: { info() {}, warn() {}, error() {}, debug() {} }, + hook: () => {}, + on: () => {}, + } as any); + return handlers; + } + + async function drive(kernel: ObjectKernel) { + const handler = (await mountOn(kernel))[ROUTE]; + expect(handler, 'route must be mounted').toBeTypeOf('function'); + const res: any = { + statusCode: undefined, + body: undefined, + status(c: number) { res.statusCode = c; return res; }, + header() { return res; }, + json(b: any) { res.body = b; return res; }, + end() { return res; }, + }; + await handler({ headers: { cookie: 'session=1' }, query: {} }, res); + return { status: res.statusCode as number, body: res.body }; + } + + // A route behind the gate with no service wired behind it, so a request the + // gate ADMITS answers the domain's own 501 — distinguishable from both of + // the gate's refusals. + const ROUTE = 'GET /api/v1/automation/_status'; + + it('POSITIVE CONTROL: a member is admitted — the domain answers (501, no automation service)', async () => { + const { status, body } = await drive(kernelWith('member')); + expect(status).toBe(501); + expect(body?.error?.code).toBe('NOT_IMPLEMENTED'); + }); + + it('CONTROL: a non-member is refused by the gate — 403 PROJECT_MEMBERSHIP_REQUIRED', async () => { + const { status, body } = await drive(kernelWith('non-member')); + expect(status).toBe(403); + expect(body?.error?.code).toBe('PROJECT_MEMBERSHIP_REQUIRED'); + }); + + it('the membership read faults → 503 with a declared SERVICE_UNAVAILABLE envelope — neither admitted (501) nor the membership 403', async () => { + // The transport withholds 5xx prose from the caller, so the failed read is + // named server-side (the error's `object`, pinned above), not on the wire. + const { status, body } = await drive(kernelWith('read-faults')); + expect(status).toBe(503); + expect(BaseResponseSchema.safeParse(body).success).toBe(true); + expect(body?.success).toBe(false); + expect(ApiErrorSchema.safeParse(body?.error).error?.issues ?? []).toEqual([]); + expect(body?.error?.code).toBe('SERVICE_UNAVAILABLE'); }); }); diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index eaefa74b516..946bc08a022 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -7,6 +7,9 @@ import { // every seam between `resolveAuthzContext` and a door), and the tenancy // read is classified by the REGISTRY's own "never registered" brand. rethrowAuthzStoreUnavailable, isServiceNotRegisteredError, + // [#21941] The environment-membership gate raises the same loud outage + // when its own read cannot answer. + AuthzStoreUnavailableError, } from '@objectstack/core'; import { isMcpServerEnabled, looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE, resolveThrownHttpError, demotedDeclaredCode, declaredUserMessage } from '@objectstack/types'; import { measureServerTiming, allowPerfDisclosure, isPerfDisclosurePrincipal } from '@objectstack/observability'; @@ -249,6 +252,9 @@ function isPathWithinPrefix(path: string, prefix: string): boolean { return Number.isNaN(next) || next === 47 /* '/' */ || next === 63 /* '?' */; } +/** The object the environment-membership gate reads (control plane, cloud-provided). */ +const ENVIRONMENT_MEMBER_OBJECT = 'sys_environment_member'; + /** * The protocol-standard discovery route, and the canonical spelling of the API * root (#17625). @@ -1456,45 +1462,79 @@ export class HttpDispatcher { } // Query sys_environment_member (control plane). - try { - const qlService = await this.getObjectQLService(this.requestKernel(context)); - const ql = qlService ?? await this.resolveService(this.requestKernel(context), 'objectql'); - if (!ql) return null; // No QL — cannot enforce; fail open. + // + // [#21941] The gate FAILS CLOSED when its read cannot answer. It used to + // catch every fault here and return `null` — the caller admitted — so a + // signed-in non-member passed whenever the read faulted. A read that + // could not answer is not a verdict about the caller either way: it is + // refused as the outage it is, with the loud, retriable answer the + // identity step and the domain gates already give an authorization + // input they could not read — `AuthzStoreUnavailableError`, 503 + // `SERVICE_UNAVAILABLE`, thrown out of `dispatch()` to the transport's + // envelope — never as allowed, and never as the membership 403. + // + // Three answers, told apart before and around the read: + // + // - NO ENGINE ⇒ refused. The read cannot be made. Only a host + // `KernelResolver` writes `context.environmentId`, and this point is + // reached only for a caller the `auth` service signed in; this + // repository's `auth` provider (`plugin-auth`) declares ObjectQL a + // hard dependency, so no composition built from it reaches this line + // without an engine. An absent one here is a fault, not a + // composition. + // - OBJECT NOT REGISTERED ⇒ the gate does not apply. A composition + // whose engine registers no `sys_environment_member` (it is a + // cloud-provided object, `CLOUD_PROVIDED_OBJECT_NAMES`) holds no + // environment membership to judge. The question is the registry's own + // `getObject` — the one the engine's verbs ask before refusing an + // unregistered name — never a caught throw. An engine whose registry + // cannot be asked reads as before: it is asked, and a fault refuses. + // - THE READ THROWS ⇒ refused. Anything the registered read raises. + const qlService = await this.getObjectQLService(this.requestKernel(context)); + const ql = qlService ?? await this.resolveService(this.requestKernel(context), 'objectql'); + if (!ql) { + throw new AuthzStoreUnavailableError( + ENVIRONMENT_MEMBER_OBJECT, + new Error('no ObjectQL engine resolved on this request\'s kernel'), + ); + } + if (typeof ql.registry?.getObject === 'function' && !ql.registry.getObject(ENVIRONMENT_MEMBER_OBJECT)) { + return null; + } + let rows: unknown; + try { // The membership read carries the explicit system opt-in. This // gate is the one asking the question — the caller's user id is // the `where`, not the reader — so the read runs as the platform, // never as a context with no principal and no opt-in (the // security middleware's principal-less hand-off, ADR-0096). - let rows = await ql.find('sys_environment_member', { + rows = await ql.find(ENVIRONMENT_MEMBER_OBJECT, { where: { environment_id: environmentId, user_id: userId }, limit: 1, context: { isSystem: true }, } as any); - if (rows && (rows as any).value) rows = (rows as any).value; - const isMember = Array.isArray(rows) && rows.length > 0; - - if (isMember) { - this.membershipCache.set(cacheKey, now); - return null; - } - - // [#3842] `PROJECT_MEMBERSHIP_REQUIRED` was parked in `details.type` - // — the fourth site, and the only one to use that spelling — because - // `error.code` held the status. It travels as `details.code` now, the - // one carrier `buildApiError` promotes into `error.code`; the two - // genuine context fields stay in `details`. - return this.error( - `Forbidden: user ${userId} is not a member of project ${environmentId}`, - 403, - { code: 'PROJECT_MEMBERSHIP_REQUIRED', environmentId, userId }, - ); } catch (err) { - // Control-plane lookup failure — log and fail open rather than - // break the request. Tightening this is deferred to Phase 4. - console.debug('[HttpDispatcher] Membership check failed:', err); + throw new AuthzStoreUnavailableError(ENVIRONMENT_MEMBER_OBJECT, err); + } + if (rows && (rows as any).value) rows = (rows as any).value; + const isMember = Array.isArray(rows) && rows.length > 0; + + if (isMember) { + this.membershipCache.set(cacheKey, now); return null; } + + // [#3842] `PROJECT_MEMBERSHIP_REQUIRED` was parked in `details.type` + // — the fourth site, and the only one to use that spelling — because + // `error.code` held the status. It travels as `details.code` now, the + // one carrier `buildApiError` promotes into `error.code`; the two + // genuine context fields stay in `details`. + return this.error( + `Forbidden: user ${userId} is not a member of project ${environmentId}`, + 403, + { code: 'PROJECT_MEMBERSHIP_REQUIRED', environmentId, userId }, + ); } /** diff --git a/packages/runtime/src/packages-unscoped-environment-binding.test.ts b/packages/runtime/src/packages-unscoped-environment-binding.test.ts index 8e75b6f2db3..e070a2c0df8 100644 --- a/packages/runtime/src/packages-unscoped-environment-binding.test.ts +++ b/packages/runtime/src/packages-unscoped-environment-binding.test.ts @@ -107,7 +107,10 @@ function makeKernel(label: string, opts: { memberOf?: string[]; withRegistry?: b return [{ manifest: { id: `pkg.of.${label}` }, status: 'installed', enabled: true }]; }), getPackage: vi.fn(() => undefined), - getObject: vi.fn(() => null), + // Registers `sys_environment_member` and nothing else: the membership + // gate asks the registry before it reads, and an engine that registers + // no such object declares the gate inapplicable (#21941). + getObject: vi.fn((name: string) => (name === 'sys_environment_member' ? { name } : null)), getRegisteredTypes: vi.fn(() => []), }; diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index de32e797d66..7198f727770 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2716,6 +2716,11 @@ "verb": "findOne", "pinned": 1 }, + { + "file": "packages/plugins/plugin-auth/src/auth-manager.org-slug-guard-system-context.test.ts", + "verb": "findOne", + "pinned": 1 + }, { "file": "packages/plugins/plugin-auth/src/auth-manager.test.ts", "verb": "delete",