diff --git a/.changeset/23491-patch-data-request-gate.md b/.changeset/23491-patch-data-request-gate.md index 686fac2c406..26968ee7ec7 100644 --- a/.changeset/23491-patch-data-request-gate.md +++ b/.changeset/23491-patch-data-request-gate.md @@ -22,6 +22,6 @@ What changes for you, wherever `@objectstack/rest` or the runtime dispatcher ser How it renders: `RestServer` answers `{ error, code: 'VALIDATION_FAILED', fields, object }`, the envelope its request gate already used. The runtime dispatcher returns its own `400 VALIDATION_FAILED` envelope, `fields` under `error.details`, instead of throwing, so a `@objectstack/hono` `createHonoApp` host renders it as `400`. -Unchanged: a record body, `{}` included; a string `expectedVersion` (judged; `409` on a mismatch); `If-Match` and the `DELETE`'s `?expectedVersion`, which are strings by transport; and a JSON `null` body on a `POST` create, which both doors still read as an empty record and answer `201`. +Unchanged: a record body, `{}` included; a string `expectedVersion` (judged; `409` on a mismatch); and `If-Match` and the `DELETE`'s `?expectedVersion`, which are strings by transport. A JSON `null` body on a `POST` create is refused too, on both doors, in the companion entry for the `/data` create and query requests. To send a patch or a row, send the fields as a JSON object. To guard a write, send the version you read (the record's `updated_at`) as a string. The `@objectstack/client` SDK's `data.update` sends the version as an `If-Match` header string, so an SDK caller is affected only when the `data` it passes is not an object, or carries a number `expectedVersion`. diff --git a/.changeset/23574-data-request-contract-remainder.md b/.changeset/23574-data-request-contract-remainder.md new file mode 100644 index 00000000000..26c746ade5d --- /dev/null +++ b/.changeset/23574-data-request-contract-remainder.md @@ -0,0 +1,29 @@ +--- +'@objectstack/rest': minor +'@objectstack/runtime': minor +--- + +fix(rest,runtime)!: a JSON `null` body on a `/data` create or query is refused `400 VALIDATION_FAILED` on both doors; the runtime dispatcher's `POST /data/:object/query` validates `FindDataRequestSchema`; and the dispatcher asks an object's API exposure before any request gate + +`RestServer` read a JSON `null` body on `POST /data/:object` as an empty record and created an empty row, and on `POST /data/:object/query` as no query and answered the full record set. The runtime dispatcher's `/data` domain, the only answer on a host that mounts just the `@objectstack/hono` catch-all, did the same for the create, and validated no query at all: a mistyped clause or a retired key reached the engine, which dropped it and answered the full record set. `CreateDataRequestSchema.data` declares a record and `FindDataRequestSchema.query` declares a QueryAST or nothing, never `null`. Both doors now validate the assembled request as the caller sent it, forward that same request, and answer each case alike. On an object whose API exposure denies the verb, the dispatcher used to answer a `PATCH` body that is not a record, or a create body that is an array, a string or a number, with its request gate's `400`, where `RestServer` answered the exposure refusal; it now asks the exposure gate before each request gate, as `RestServer` does. + +Clause-②: no (narrowing) + + + +**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes. + +What changes for you, wherever `@objectstack/rest` or the runtime dispatcher serves these routes: + +- Both doors, `POST /data/:object` with a JSON `null` body: FROM `201` and an empty row created, TO `400 VALIDATION_FAILED` naming `data`, with no row created. +- Both doors, `POST /data/:object/query` with a JSON `null` body: FROM `200` with the full record set, TO `400 VALIDATION_FAILED` naming `query`, the engine never asked. +- The runtime dispatcher, `POST /data/:object/query` with a mistyped clause (`{ "limit": "ten" }`), the retired `joins` key, or a number as the body: FROM `200` with the full record set, the clause dropped, TO `400 VALIDATION_FAILED` naming the member (`query.limit`, `query.joins`, `query`). `RestServer` already answered this. +- The runtime dispatcher, `POST /data/:object/query` with a string or an array as the body: FROM the engine's `400 INVALID_FIELD` for numbered field names, TO `400 VALIDATION_FAILED` naming `query`. `RestServer` already answered this. +- The runtime dispatcher, `POST /data/:object/query` with a body that nests the query under a `query` key (`{ "query": { "where": … } }`): FROM read as the nested query (`200`), TO read as `RestServer` reads it, the body itself being the query, so `query` is a field the object does not have: `400 INVALID_FIELD`. +- The runtime dispatcher, `PATCH /data/:object/:id` with a body that is not a record (an array, a string, a number or `null`), and `POST /data/:object` with an array, a string or a number as the body, on an object whose API exposure denies the verb: FROM `400 VALIDATION_FAILED`, TO the exposure refusal `RestServer` answers (`404` when the object sets `apiEnabled: false`, `405` when its `apiMethods` leaves the verb out). The request was refused before and is refused now; only the answer changes. A JSON `null` create on such an object already answered the exposure refusal (the old gate read it as `{}`), and so did every query on it: the query route had no request gate, and its new one runs after the exposure gate, as `RestServer` orders it. + +How it renders: `RestServer` answers `{ error, code: 'VALIDATION_FAILED', fields, object }`, the envelope its request gates already use. The runtime dispatcher returns its own `400 VALIDATION_FAILED` envelope, `fields` under `error.details`, instead of throwing, so a `@objectstack/hono` `createHonoApp` host renders it as `400`. + +Unchanged: an empty request body, which every host hands over as `{}` (an empty record creates a row; an empty query answers every record); a record body; a conforming query; and a body `object` key on a query, which the path object still overrides. + +To create a row, send its fields as a JSON object, `{}` for all defaults. To query, send the QueryAST itself as the body, `{}` for every record. The `@objectstack/client` SDK's `data.create` and `data.query` send exactly these, so an SDK caller is affected only when the value it passes is `null`, or not the shape the request declares. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 431411d1e59..4d85111fe98 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -175,7 +175,7 @@ One of the largest consumers of the flag; | # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor | |:--|:---|:---|:---|:---| -| 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes | `packages/runtime/src/action-execution.ts#callData` | +| 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes. The gate is `refuseUnexposedDataAction`, which `callData` asks first and the `/data` domain asks before its request gate; `callData` reads the same bypass once more for each nested-relation condition's target | `packages/runtime/src/action-execution.ts#callData`, `#refuseUnexposedDataAction` | | 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `packages/runtime/src/action-execution.ts#actionPermissionError` | | 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability (the permission-set clone door's save included) — and, because REST's save door and the stored-version read doors ask one save verdict, an app's full stored version on `/layers`, `?layers=true` and `/diff`, whatever its entry gates withhold | `packages/runtime/src/domains/meta.ts#handleMetadataRequest`, `packages/rest/src/rest-server.ts#registerMetadataEndpointsInner`, `packages/rest/src/rest-server.ts#metaSaveVerdict`, `packages/runtime/src/domains/permission-set-clone.ts#handlePermissionSetCloneWrite` | | 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `packages/metadata-core/src/meta-write-capability.ts#metaWriteCapabilityVerdict` | diff --git a/packages/rest/src/request-schema-gate.conformance.test.ts b/packages/rest/src/request-schema-gate.conformance.test.ts index bbbf3fb9f5b..6d082a4329b 100644 --- a/packages/rest/src/request-schema-gate.conformance.test.ts +++ b/packages/rest/src/request-schema-gate.conformance.test.ts @@ -117,6 +117,9 @@ const CASES: GateCase[] = [ // fallback and an ordinary sum on every SQL datasource — one query, two // numbers, both plausible. { aggregations: [{ function: 'sum', field: 'amount', alias: 'total', distinct: true }] }, + // #23574 — `null` was read as no query (`req.body ?? {}`) and answered + // the full record set; `query` declares a QueryAST or nothing. + null, ], valid: { where: { status: 'active' }, limit: 5 }, spy: 'findData', @@ -124,7 +127,9 @@ const CASES: GateCase[] = [ { route: 'POST /api/v1/data/:object', params: PARAMS, - violating: ['garbage', 42, [{ name: 'x' }]], + // #23574 — `null` was read as an empty record (`req.body ?? {}`) and + // created an empty row. + violating: ['garbage', 42, [{ name: 'x' }], null], valid: { name: 'ACME' }, spy: 'createData', }, diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index a22d641127c..f6b71f228bf 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -9032,8 +9032,18 @@ export class RestServer { // reaching the engine as `data`. Per-field checks stay // downstream (object metadata / validation rules); this // gate is about the SHAPE the contract declares. + // + // [#23574] …the body AS THE CALLER SENT IT. It was + // `req.body ?? {}`, so a JSON `null` body parsed as an + // empty record and created an empty row (`201`). `null` + // is not the record `data` declares, so it is refused + // like an array, a string or a number, and the runtime + // dispatcher's twin (`domains/data.ts`) asks this gate + // over the same assembled request. An empty request + // body is not `null`: the HTTP adapter hands it over + // as `{}`, a record. const { CreateDataRequestSchema } = await import('@objectstack/spec/api'); - const createInput = { object: req.params.object, data: req.body ?? {} }; + const createInput = { object: req.params.object, data: req.body }; const parsedCreate = (CreateDataRequestSchema as any).safeParse(createInput); if (!parsedCreate.success) { res.status(400).json({ @@ -9046,7 +9056,7 @@ export class RestServer { } const createRequest: ServerScopedDataRequest = { object: req.params.object, - data: req.body ?? {}, + data: req.body, ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), }; @@ -9096,11 +9106,21 @@ export class RestServer { // ORIGINAL body is forwarded, not the parse output, so the // schema cannot inject defaults the engine did not receive // before (the analytics-entry precedent, #3878). + // + // [#23574] …the body AS THE CALLER SENT IT. It was + // `req.body ?? {}`, so a JSON `null` body read as no + // query and answered the full record set (`200`). + // `query` declares a QueryAST or nothing, never `null`, + // so `null` is refused like a string or a number, and + // the runtime dispatcher's twin (`domains/data.ts`) now + // asks this gate over the same assembled request. An + // empty request body is not `null`: the HTTP adapter + // hands it over as `{}`, an empty query. const { FindDataRequestSchema } = await import('@objectstack/spec/api'); - const rawQuery = req.body ?? {}; + const rawQuery = req.body; const query = (rawQuery && typeof rawQuery === 'object' && !Array.isArray(rawQuery)) ? { ...rawQuery, object: req.params.object } - : rawQuery; // non-object bodies go to the schema as-is and fail with `query: invalid_type` + : rawQuery; // non-object bodies (`null` included) go to the schema as-is and fail with `query: invalid_type` const findInput = { object: req.params.object, query }; const parsedFind = (FindDataRequestSchema as any).safeParse(findInput); if (!parsedFind.success) { diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index de6cf91945e..e1777f48edd 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -275,6 +275,43 @@ async function resolveEngineQuietly(deps: ActionExecutionDeps, requestContext: H } } +/** + * ── Object-level API exposure gate (ADR-0049, #1889) ───── + * Honour the object's `apiEnabled` / `apiMethods` declarations for external + * traffic: throws the refusal ({@link checkApiExposure}'s `404` / `405`) when + * `object` does not expose `action`, and returns otherwise. System/internal + * contexts bypass — these flags govern API *exposure*, not internal engine + * self-writes. + * + * [#23574] Extracted from {@link callData}, which still asks it first for + * every caller, so that the runtime dispatcher's `/data` domain can ask the + * SAME question before its own request-schema gate (`domains/data.ts`): a + * caller the object refuses is answered the refusal, never a shape verdict + * about the body it sent. `RestServer` asks `enforceApiAccess` before its + * twin gate for the same reason. Moved, not copied: this is the one place the + * dispatcher's exposure decision is resolved. + */ +export async function refuseUnexposedDataAction(deps: ActionExecutionDeps, + requestContext: HttpProtocolContext, + action: string, + object: unknown, + scopeId?: string, + executionContext?: ExecutionContext, +): Promise { + if (executionContext?.isSystem || !object) return; + let def: any; + try { + const meta = await deps.resolveService(requestContext, 'metadata', scopeId); + def = await (meta as any)?.getObject?.(object); + } catch { + def = undefined; // fall open to schema defaults (apiEnabled=true) + } + const gate = checkApiExposure(def, action); + if (!gate.allowed) { + throw { statusCode: gate.status ?? 403, message: gate.reason ?? 'API access denied' }; + } +} + export async function callData(deps: ActionExecutionDeps, requestContext: HttpProtocolContext, action: string, @@ -283,27 +320,12 @@ export async function callData(deps: ActionExecutionDeps, scopeId?: string, executionContext?: ExecutionContext, ): Promise { - // ── Object-level API exposure gate (ADR-0049, #1889) ───── - // Honour the object's `apiEnabled` / `apiMethods` declarations for - // external traffic. System/internal contexts bypass — these flags - // govern API *exposure*, not internal engine self-writes. + // The object-level API exposure gate ({@link refuseUnexposedDataAction}). // [#22823] Decided ONCE, here: the same bypass covers each nested-relation // condition's target, asked below on the two arms that hand the engine a // caller's `where` ({@link refuseUnservedRelationTargets}). const exposureGated = !executionContext?.isSystem; - if (exposureGated && params?.object) { - let def: any; - try { - const meta = await deps.resolveService(requestContext, 'metadata', scopeId); - def = await (meta as any)?.getObject?.(params.object); - } catch { - def = undefined; // fall open to schema defaults (apiEnabled=true) - } - const gate = checkApiExposure(def, action); - if (!gate.allowed) { - throw { statusCode: gate.status ?? 403, message: gate.reason ?? 'API access denied' }; - } - } + await refuseUnexposedDataAction(deps, requestContext, action, params?.object, scopeId, executionContext); const protocol = await deps.resolveService(requestContext, 'protocol', scopeId); const qlService = dataDriver ?? await deps.getObjectQL(requestContext, scopeId); diff --git a/packages/runtime/src/domains/data-path-object.test.ts b/packages/runtime/src/domains/data-path-object.test.ts index 0c1f6edec72..ab87d39018c 100644 --- a/packages/runtime/src/domains/data-path-object.test.ts +++ b/packages/runtime/src/domains/data-path-object.test.ts @@ -222,11 +222,18 @@ describe('POST /data/:object/query binds to the object in the path (#3946)', () expect(req.query).toMatchObject({ where: { status: 'open' }, limit: 5 }); }); - it('still honours an explicit `query` envelope', async () => { + // [#23574] Flipped. This pinned the body's `query` key being unwrapped as + // the query itself, a second spelling `RestServer` never read (there the + // body IS the QueryAST, as the catalog declares) and one the request gate + // cannot see: `{ query: { limit: 'ten' } }` would pass the gate and be + // served as the full set. The key now reaches the protocol as the field it + // names, which the real protocol refuses `400 INVALID_FIELD` on both doors + // (`data-write-request-gate.test.ts`, on a real store). + it('a body `query` key is the field it names, never a second spelling of the query', async () => { const { deps, context, findData } = setup(); await post(deps, context, 'crm_account/query', { query: { where: { status: 'open' } } }); - expect(findData.mock.calls[0][0].query).toEqual({ where: { status: 'open' } }); + expect(findData.mock.calls[0][0].query).toEqual({ query: { where: { status: 'open' } } }); }); it('threads the caller execution context through unchanged', async () => { diff --git a/packages/runtime/src/domains/data-write-request-gate.test.ts b/packages/runtime/src/domains/data-write-request-gate.test.ts index 7667fc84dab..4bf3c26a3e8 100644 --- a/packages/runtime/src/domains/data-write-request-gate.test.ts +++ b/packages/runtime/src/domains/data-write-request-gate.test.ts @@ -7,6 +7,9 @@ * answer on a host that mounts just `@objectstack/hono`'s `${prefix}/*` * catch-all. * + * [#23574] …and the `POST /data/:object/query` read, and a JSON `null` create, + * and both doors ask the object's API exposure before their request gate. + * * ## What was wrong * * `RestServer` validates `PATCH /data/:object/:id` as `UpdateDataRequestSchema` @@ -28,10 +31,27 @@ * * The `null` body now answers `400` on both `PATCH` doors: the declared type * is a record, and one door reading it as `{}` was the consumer-side leniency - * the contract-first rule forbids. A `null` CREATE is the one shape left as it - * was: `RestServer`'s create reads it as `{}`, the dispatcher's gate asks the - * same assembled request, and the two doors answer it alike (pinned below as - * parity only). + * the contract-first rule forbids. + * + * [#23574] Three more, measured on this rig before the fix: + * + * 1. A `null` CREATE answered `201` and an EMPTY ROW on both doors: each read + * it as `{}` (`req.body ?? {}`, and the dispatcher mirrored it statement + * for statement). Both now refuse it like the rest. + * 2. The dispatcher's `POST /data/:object/query` had no + * `FindDataRequestSchema` gate: `{ limit: "ten" }`, the retired + * `{ joins: [] }`, a number and a `null` answered `200` with the FULL + * record set, and a string or an array answered the engine's `400 + * INVALID_FIELD` for numbered keys. `RestServer` refused each `400 + * VALIDATION_FAILED`, except `null`, which it too read as `{}` (a full + * read). Both now gate the same assembled request, and forward it: a body + * `query` key, which the dispatcher used to read as a nested query, is + * now the unknown field it is through `RestServer`. + * 3. On an object whose API exposure denies the verb, a body that is not a + * record answered `400` on the dispatcher (its request gate ran before + * `callData`, where the exposure gate lives) and the exposure refusal + * (`404` / `405`) on `RestServer`, whose `enforceApiAccess` runs first. + * The dispatcher now asks the same exposure gate first. * * ## The rig * @@ -42,6 +62,14 @@ * the dispatcher through `HttpDispatcher.dispatch()` with the body parsed the * way the catch-all parses it (`JSON.parse` of the request text, any failure * `{}`). Every row asserts the store, not just the door's word. + * + * [#23574] Both doors read each object's exposure from the one definition the + * engine's registry holds: `RestServer` through the protocol's object list + * (`getMetaItems`, which this rig's protocol cannot read without a metadata + * store, so it is answered from the registry), and the dispatcher through the + * kernel's `metadata` service (`getObject`, answered from the same registry). + * Without them each door's exposure gate fails open, and every exposure row + * below would read as an allowed object. */ import { describe, it, expect, vi, afterEach } from 'vitest'; @@ -58,14 +86,22 @@ const DATA = `${PREFIX}/data`; const TEST_PACKAGE_ID = 'objectstack-test'; const CALLER = { userId: 'u_member', isSystem: false }; -const TASK = { - name: 'task', - label: 'Task', - fields: { - id: { name: 'id', type: 'text' as const, primaryKey: true, label: 'ID' }, - name: { name: 'name', type: 'text' as const, label: 'Name' }, - }, +const FIELDS = { + id: { name: 'id', type: 'text' as const, primaryKey: true, label: 'ID' }, + name: { name: 'name', type: 'text' as const, label: 'Name' }, }; +const TASK = { name: 'task', label: 'Task', fields: FIELDS }; + +/** + * [#23574] Two objects whose declared API exposure refuses every verb below: + * `locked` exposes `get` alone (`405` for `create`, `update` and `list`), and + * `hidden` is not exposed at all (`404`). + */ +const EXPOSURE_DENIED: ReadonlyArray<{ object: { name: string; label: string; fields: typeof FIELDS; enable: Record }; status: number }> = [ + { object: { name: 'locked', label: 'Locked', fields: FIELDS, enable: { apiMethods: ['get'] } }, status: 405 }, + { object: { name: 'hidden', label: 'Hidden', fields: FIELDS, enable: { apiEnabled: false } }, status: 404 }, +]; +const OBJECTS = [TASK, ...EXPOSURE_DENIED.map((d) => d.object)]; const BEFORE = 'first'; const AFTER = 'second'; @@ -79,10 +115,10 @@ afterEach(async () => { } }); -/** What the store holds after a request: `r1`'s name and version, and the row count. */ +/** What the store holds after a request: `r1`'s name and version, and the row count, of one object. */ interface StoreState { name: unknown; version: unknown; rows: number } -/** One real store with one `task` row, `r1`, named {@link BEFORE}. */ +/** One real store with one row, `r1`, named {@link BEFORE}, in each of {@link OBJECTS}. */ async function bootStore() { const engine = new ObjectQL(); liveEngines.push(engine); @@ -92,30 +128,37 @@ async function bootStore() { useNullAsDefault: true, }), true); await engine.init(); - engine.registry.registerObject(TASK as any, TEST_PACKAGE_ID); + for (const object of OBJECTS) engine.registry.registerObject(object as any, TEST_PACKAGE_ID); await engine.syncSchemas(); const protocol = new ObjectStackProtocolImplementation(engine as any); - await protocol.createData({ object: 'task', data: { id: 'r1', name: BEFORE } } as any); + for (const object of OBJECTS) { + await protocol.createData({ object: object.name, data: { id: 'r1', name: BEFORE } } as any); + } const updateData = vi.spyOn(protocol, 'updateData'); const createData = vi.spyOn(protocol, 'createData'); - const state = async (): Promise => { - const row: any = await engine.findOne('task', { where: { id: 'r1' } }); - const rows = ((await engine.find('task', {})) as unknown[]).length; + const findData = vi.spyOn(protocol, 'findData'); + const state = async (object = 'task'): Promise => { + const row: any = await engine.findOne(object, { where: { id: 'r1' } }); + const rows = ((await engine.find(object, {})) as unknown[]).length; const v = row?.updated_at; return { name: row?.name, version: v instanceof Date ? v.toISOString() : v, rows }; }; - return { protocol, updateData, createData, state }; + /** Each object's definition as the registry holds it: the one exposure source both doors read. */ + const definition = (name: string) => engine.registry.getObject(name); + return { protocol, updateData, createData, findData, state, definition }; } // ── The two doors ───────────────────────────────────────────────────────────── -interface Answer { status: number; code?: string; fields?: string[] } +interface Answer { status: number; code?: string; fields?: string[]; records?: number } interface Door { - patch(body: unknown): Promise; - create(body: unknown): Promise; - state(): Promise; + patch(body: unknown, object?: string): Promise; + create(body: unknown, object?: string): Promise; + query(body: unknown, object?: string): Promise; + state(object?: string): Promise; updateCalls(): number; createCalls(): number; + findCalls(): number; } /** The refusal's `code` in either door's envelope: nested under `error`, or beside a string `error`. */ @@ -126,6 +169,11 @@ const fieldsOf = (body: any): string[] | undefined => { const fields = Array.isArray(body?.fields) ? body.fields : body?.error?.details?.fields; return Array.isArray(fields) ? fields.map((f: any) => f?.field).sort() : undefined; }; +/** How many records a query answered: `RestServer`'s `records`, the dispatcher's `data.records`. */ +const recordsOf = (body: any): number | undefined => { + const records = body?.records ?? body?.data?.records; + return Array.isArray(records) ? records.length : undefined; +}; function makeRes() { const res: any = { statusCode: 200, body: undefined, headers: {} as Record }; @@ -140,7 +188,11 @@ function makeRes() { } async function bootRestServer(): Promise { - const { protocol, updateData, createData, state } = await bootStore(); + const { protocol, updateData, createData, findData, state, definition } = await bootStore(); + // `enforceApiAccess` reads the object list through the protocol; see the rig note above. + (protocol as any).getMetaItems = async (request: any) => request?.type === 'object' + ? { type: 'object', items: OBJECTS.map((o) => definition(o.name)) } + : { type: request?.type, items: [] }; const noop = () => {}; const server = { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; const rest: any = new RestServer(server as any, protocol as any, { api: { requireAuth: false } } as any); @@ -153,18 +205,21 @@ async function bootRestServer(): Promise { }; const update = routeOf('PATCH', `${DATA}/:object/:id`); const create = routeOf('POST', `${DATA}/:object`); + const find = routeOf('POST', `${DATA}/:object/query`); const drive = async (route: any, path: string, params: Record, body: unknown): Promise => { const res = makeRes(); // `plugin-hono-server` hands a JSON body over as `c.req.json()` parsed it: `null` stays `null`. await route.handler({ method: route.method, path, params, headers: { 'content-type': 'application/json' }, query: {}, body: clone(body) }, res); - return { status: res.statusCode, code: codeOf(res.body), fields: fieldsOf(res.body) }; + return { status: res.statusCode, code: codeOf(res.body), fields: fieldsOf(res.body), records: recordsOf(res.body) }; }; return { state, updateCalls: () => updateData.mock.calls.length, createCalls: () => createData.mock.calls.length, - patch: (body) => drive(update, `${DATA}/task/r1`, { object: 'task', id: 'r1' }, body), - create: (body) => drive(create, `${DATA}/task`, { object: 'task' }, body), + findCalls: () => findData.mock.calls.length, + patch: (body, object = 'task') => drive(update, `${DATA}/${object}/r1`, { object, id: 'r1' }, body), + create: (body, object = 'task') => drive(create, `${DATA}/${object}`, { object }, body), + query: (body, object = 'task') => drive(find, `${DATA}/${object}/query`, { object }, body), }; } @@ -189,7 +244,7 @@ async function catchAll(dispatcher: HttpDispatcher, method: string, path: string try { const res: any = await dispatcher.dispatch(method, path, parsed, {}, { request: raw } as any, PREFIX); const out = res.response?.body; - return { status: res.response?.status ?? 0, code: codeOf(out), fields: fieldsOf(out) }; + return { status: res.response?.status ?? 0, code: codeOf(out), fields: fieldsOf(out), records: recordsOf(out) }; } catch (e) { // A throw out of `dispatch()` is rendered by the host's exit; read it // through the one rule the dispatcher's exits answer a throw with. @@ -199,8 +254,10 @@ async function catchAll(dispatcher: HttpDispatcher, method: string, path: string } async function bootCatchAll(): Promise { - const { protocol, updateData, createData, state } = await bootStore(); - const services: Record = { protocol }; + const { protocol, updateData, createData, findData, state, definition } = await bootStore(); + // `callData`'s exposure gate reads the kernel's `metadata` service; see the rig note above. + const metadata = { getObject: async (name: string) => definition(name) }; + const services: Record = { protocol, metadata }; const get = (n: string) => services[n] ?? null; const kernel: any = { context: { getService: get }, getService: get, getServiceAsync: async (n: string) => get(n) }; const dispatcher = new HttpDispatcher(kernel); @@ -211,8 +268,10 @@ async function bootCatchAll(): Promise { state, updateCalls: () => updateData.mock.calls.length, createCalls: () => createData.mock.calls.length, - patch: (body) => catchAll(dispatcher, 'PATCH', '/data/task/r1', body), - create: (body) => catchAll(dispatcher, 'POST', '/data/task', body), + findCalls: () => findData.mock.calls.length, + patch: (body, object = 'task') => catchAll(dispatcher, 'PATCH', `/data/${object}/r1`, body), + create: (body, object = 'task') => catchAll(dispatcher, 'POST', `/data/${object}`, body), + query: (body, object = 'task') => catchAll(dispatcher, 'POST', `/data/${object}/query`, body), }; } @@ -271,12 +330,12 @@ describe('[#23491] the two PATCH doors refuse each non-record body alike', () => // ── POST /data/:object ──────────────────────────────────────────────────────── -/** The create's non-record bodies `RestServer`'s assembled request refuses; `null` is read as `{}` by both doors. */ -const CREATE_REFUSED = NOT_A_RECORD.filter(([, body]) => body !== null); - +// [#23574] Location 1: `null` is refused like the rest. Both doors used to read +// it as `{}` and create an empty row (`201`), so it was left out of this list +// and pinned as parity only. for (const [doorName, boot] of DOORS) { - describe(`[#23491] ${doorName}: POST /data/:object refuses a body that is not a record`, () => { - for (const [shape, body] of CREATE_REFUSED) { + describe(`[#23491] [#23574] ${doorName}: POST /data/:object refuses a body that is not a record`, () => { + for (const [shape, body] of NOT_A_RECORD) { it(`${shape}: 400 VALIDATION_FAILED on data, createData never asked, no row created`, async () => { const door = await boot(); const before = await door.state(); @@ -293,6 +352,14 @@ for (const [doorName, boot] of DOORS) { expect(answer.status, JSON.stringify(answer)).toBe(201); expect((await door.state()).rows).toBe(before.rows + 1); }, 60_000); + + it('control: an empty record body (what every host hands over for an empty request body) creates one row (201)', async () => { + const door = await boot(); + const before = await door.state(); + const answer = await door.create({}); + expect(answer.status, JSON.stringify(answer)).toBe(201); + expect((await door.state()).rows).toBe(before.rows + 1); + }, 60_000); }); } @@ -310,3 +377,138 @@ describe('[#23491] the two POST doors answer each non-record body alike', () => }, 60_000); } }); + +// ── POST /data/:object/query ────────────────────────────────────────────────── + +/** + * [#23574] Location 2: bodies `FindDataRequestSchema` refuses, each with the + * member the refusal names. `RestServer` refused all but `null` before this + * card; the dispatcher refused none of them with this answer. + */ +const QUERY_REFUSED: ReadonlyArray = [ + ['a mistyped clause, `limit: "ten"`', { limit: 'ten' }, 'query.limit'], + ['the retired `joins`', { joins: [] }, 'query.joins'], + ['a string', 'garbage', 'query'], + ['an array', [{}], 'query'], + ['a number', 42, 'query'], + ['null', null, 'query'], +]; + +for (const [doorName, boot] of DOORS) { + describe(`[#23574] ${doorName}: POST /data/:object/query refuses a request FindDataRequestSchema does not declare`, () => { + for (const [shape, body, field] of QUERY_REFUSED) { + it(`${shape}: 400 VALIDATION_FAILED on ${field}, findData never asked`, async () => { + const door = await boot(); + const answer = await door.query(body); + expect({ status: answer.status, code: answer.code, fields: answer.fields }, JSON.stringify(answer)) + .toEqual({ status: 400, code: 'VALIDATION_FAILED', fields: [field] }); + expect(door.findCalls()).toBe(0); + }, 60_000); + } + + it('control: an empty query answers every record (200)', async () => { + const door = await boot(); + await door.create({ name: AFTER }); + const answer = await door.query({}); + expect({ status: answer.status, records: answer.records }, JSON.stringify(answer)).toEqual({ status: 200, records: 2 }); + }, 60_000); + + it('control: a conforming query is served as asked (200, the filtered set)', async () => { + const door = await boot(); + await door.create({ name: AFTER }); + const answer = await door.query({ where: { name: AFTER }, limit: 5 }); + expect({ status: answer.status, records: answer.records }, JSON.stringify(answer)).toEqual({ status: 200, records: 1 }); + expect(door.findCalls()).toBe(1); + }, 60_000); + }); +} + +describe('[#23574] the two query doors answer each request alike', () => { + const BODIES: ReadonlyArray = [ + ...QUERY_REFUSED.map(([shape, body]) => [shape, body] as const), + // What the gate parses is what both doors forward, so a body `query` + // key is no nested query on either: the engine refuses the field it + // names. The dispatcher used to read it as the query (`200`). + ['a body `query` key', { query: { where: { name: 'nope' } } }], + ['a body `object` (the path object wins)', { object: 'hidden' }], + ]; + for (const [shape, body] of BODIES) { + it(`${shape}: the same status, code, fields and record count`, async () => { + const answers: Answer[] = []; + for (const [, boot] of DOORS) answers.push(await (await boot()).query(body)); + const [rest, catchAllAnswer] = answers; + expect(catchAllAnswer).toEqual(rest); + }, 60_000); + } + + it('a body `query` key: 400 INVALID_FIELD on both, never a nested query', async () => { + for (const [doorName, boot] of DOORS) { + const answer = await (await boot()).query({ query: { where: { name: 'nope' } } }); + expect({ status: answer.status, code: answer.code }, `${doorName}: ${JSON.stringify(answer)}`) + .toEqual({ status: 400, code: 'INVALID_FIELD' }); + } + }, 60_000); +}); + +// ── Exposure before the request gate ────────────────────────────────────────── + +/** + * [#23574] Location 3: on an object whose API exposure denies the verb, every + * body is answered the exposure refusal, a record or not: the request gate is + * never what tells a caller the object refuses how its ingress reads a body. + * Statuses only across doors: the two doors' exposure envelopes carry + * different codes, which this card does not change. + */ +const EXPOSURE_BODIES: ReadonlyArray = [ + ...NOT_A_RECORD, + ['a record', { name: AFTER }], +]; +const EXPOSURE_QUERY_BODIES: ReadonlyArray = [ + ['a mistyped clause, `limit: "ten"`', { limit: 'ten' }], + ['null', null], + ['an empty query', {}], +]; + +type Verb = 'patch' | 'create' | 'query'; +const EXPOSURE_CASES: ReadonlyArray, (d: Door) => number]> = [ + ['patch', EXPOSURE_BODIES, (d) => d.updateCalls()], + ['create', EXPOSURE_BODIES, (d) => d.createCalls()], + ['query', EXPOSURE_QUERY_BODIES, (d) => d.findCalls()], +]; + +for (const [doorName, boot] of DOORS) { + describe(`[#23574] ${doorName}: an exposure-denied object is answered its exposure refusal before any request gate`, () => { + for (const { object, status } of EXPOSURE_DENIED) { + for (const [verb, bodies, engineCalls] of EXPOSURE_CASES) { + it(`${verb} on \`${object.name}\`: ${status} for every body, the engine never asked, the row unchanged`, async () => { + const door = await boot(); + const before = await door.state(object.name); + for (const [shape, body] of bodies) { + const answer = await door[verb](body, object.name); + expect(answer.status, `${shape}: ${JSON.stringify(answer)}`).toBe(status); + } + expect(engineCalls(door)).toBe(0); + expect(await door.state(object.name)).toEqual(before); + }, 60_000); + } + } + }); +} + +describe('[#23574] the two doors answer an exposure-denied object with the same status, whatever the body', () => { + for (const { object } of EXPOSURE_DENIED) { + for (const [verb, bodies] of EXPOSURE_CASES) { + it(`${verb} on \`${object.name}\``, async () => { + const statuses: number[][] = []; + for (const [, boot] of DOORS) { + const door = await boot(); + const seen: number[] = []; + for (const [, body] of bodies) seen.push((await door[verb](body, object.name)).status); + statuses.push(seen); + } + const [rest, catchAllStatuses] = statuses; + expect(catchAllStatuses).toEqual(rest); + }, 60_000); + } + } +}); diff --git a/packages/runtime/src/domains/data.ts b/packages/runtime/src/domains/data.ts index b2eaf6bfffa..1de19c7caf4 100644 --- a/packages/runtime/src/domains/data.ts +++ b/packages/runtime/src/domains/data.ts @@ -40,7 +40,8 @@ import { assertFilterParamSuppliedOnce, repeatedQueryParamMessage } from '@objec // assembled protocol request does not parse returns the same way. import { validationFailure, fieldsFromZodIssues, VALIDATION_FAILED_STATUS } from '../validation-failure.js'; // [#23491] The request contracts `RestServer`'s twin routes validate (#3899). -import { CreateDataRequestSchema, UpdateDataRequestSchema } from '@objectstack/spec/api'; +// [#23574] …the query's too. +import { CreateDataRequestSchema, FindDataRequestSchema, UpdateDataRequestSchema } from '@objectstack/spec/api'; /** * [#23265] The request's `If-Match`, from either header shape a host hands @@ -104,15 +105,20 @@ function versionTokenRefusal(token: unknown, spelling: string): string | undefin * created rows. RETURNED, not thrown, for the reason `versionTokenRefusal`'s * refusal is: a throw out of `dispatch()` reaches a `createHonoApp` host's * catch-all, which renders only some declared statuses. + * + * [#23574] …and the `POST /data/:object/query` read's + * (`FindDataRequestSchema`), which had no gate either. Every caller asks the + * object's API exposure first ({@link actionExec.refuseUnexposedDataAction}), + * so this refusal never answers for an object the caller may not address. */ -function writeRequestRefusal( +function requestSchemaRefusal( deps: DomainHandlerDeps, - write: 'update' | 'create', + request: 'update' | 'create' | 'query', issues: Array<{ path: Array; code: string; message: string }>, input: unknown, ): HttpDispatcherResult { const fields = fieldsFromZodIssues(issues, input); - const message = `Invalid ${write} request: ${fields.map((f) => `${f.field}: ${f.message}`).join('; ')}`; + const message = `Invalid ${request} request: ${fields.map((f) => `${f.field}: ${f.message}`).join('; ')}`; return { handled: true, response: deps.errorFromThrown(validationFailure(message, fields), VALIDATION_FAILED_STATUS), @@ -210,7 +216,47 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m // data (`data: body`, `query: normalized`) instead of splatting it, // and the GET-by-id branch even allowlists its query params against // exactly this kind of parameter pollution. - const result = await actionExec.callData(deps, _context, 'query', { ...body, object: objectName }, _context.dataDriver, _context.environmentId, _context.executionContext); + // + // [#23574] The assembled request is validated as + // `FindDataRequestSchema`, the gate `RestServer`'s twin has asked + // since #3899, over the request that twin assembles, statement for + // statement: a record body is the query, the path object written + // into it last; any other body is the query as sent. This branch + // had no gate, so a mistyped clause (`limit: "ten"`) or the retired + // `joins` reached the engine, which dropped it and answered `200` + // with the full record set; a number or a JSON `null` spread to no + // query at all, another full read; and a string or an array was + // spread into numbered keys the engine refused as unknown fields. + // `RestServer` refused all of these `400 VALIDATION_FAILED`, and + // both doors now do; `null` is refused like the rest, on both + // (`query` declares a QueryAST or nothing, never `null`). + // + // The object's API exposure is asked first, as the twin's + // `enforceApiAccess` is: a caller the object refuses is answered + // that refusal, never a verdict on the shape of its body. + await actionExec.refuseUnexposedDataAction(deps, _context, 'query', objectName, _context.environmentId, _context.executionContext); + const query = (body && typeof body === 'object' && !Array.isArray(body)) + ? { ...body, object: objectName } + : body; + const findInput = { object: objectName, ...(query !== undefined ? { query } : {}) }; + const parsedFind = FindDataRequestSchema.safeParse(findInput); + if (!parsedFind.success) return requestSchemaRefusal(deps, 'query', parsedFind.error.issues, findInput); + // What is forwarded is the query the gate parsed (never the parse + // output, as on the twin), handed to `callData` as `query`, the + // path object riding as the request's `object` and not inside it: + // the engine fallback (no protocol registered) serves a query's + // clauses only. A body that passes the gate is a record (or + // absent), and its clauses reach the engine exactly as they did + // before. One body does not: a `query` key used to be unwrapped + // here as the query itself (`callData` reads `params.query` + // first), a second spelling the gate above never sees, so + // `{ query: { limit: "ten" } }` would pass it and be read as the + // full set. The catalog declares the body to be the QueryAST, so + // a `query` key is the field it names, refused by the engine as + // it is through `RestServer`. + const { object: _pathObject, ...clauses } = (query ?? {}) as Record; + const result = await actionExec.callData(deps, _context, 'query', { object: objectName, query: clauses }, + _context.dataDriver, _context.environmentId, _context.executionContext); return { handled: true, response: deps.success(result) }; } @@ -282,6 +328,14 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m // declares no room for was forwarded and judged `409`. // `versionTokenRefusal` above already refuses it, so what reaches // this gate and the forward below is a non-blank string. + // + // [#23574] The object's API exposure is asked before this gate, + // where the twin asks `enforceApiAccess`: between its token + // refusal and its request gate. This gate used to run first, so + // on an object whose exposure denies the update a body that is not + // a record answered `400` here and `404` / `405` there, telling a + // caller the object refuses how its ingress reads a body. + await actionExec.refuseUnexposedDataAction(deps, _context, 'update', objectName, _context.environmentId, _context.executionContext); const updateInput = { object: objectName, id, @@ -289,7 +343,7 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m ...(expectedVersion != null ? { expectedVersion } : {}), }; const parsedUpdate = UpdateDataRequestSchema.safeParse(updateInput); - if (!parsedUpdate.success) return writeRequestRefusal(deps, 'update', parsedUpdate.error.issues, updateInput); + if (!parsedUpdate.success) return requestSchemaRefusal(deps, 'update', parsedUpdate.error.issues, updateInput); // Spec: returns UpdateDataResponse = { object, id, record } const result = await actionExec.callData(deps, _context, 'update', updateInput, _context.dataDriver, _context.environmentId, _context.executionContext); @@ -389,12 +443,19 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m // row (`201`), and a string answered the engine's own unknown-field // `400`; `RestServer`'s twin refused all three `400 // VALIDATION_FAILED` (#3899). The assembled request is the twin's, - // statement for statement, `null` read as `{}` included: both doors - // answer a JSON `null` create alike (an empty row), and that - // leniency is retired on both at once or not at all. - const createInput = { object: objectName, data: body ?? {} }; + // statement for statement. + // + // [#23574] …the body AS THE CALLER SENT IT. Both doors read a JSON + // `null` as `{}` (`body ?? {}`) and created an empty row (`201`); + // `data` declares a record, so `null` is refused like an array, a + // string or a number, on both doors at once. An empty request body + // is not `null`: every host hands it over as `{}`, a record. And + // the object's API exposure is asked before this gate, as the + // twin's `enforceApiAccess` is (see the `PATCH` above). + await actionExec.refuseUnexposedDataAction(deps, _context, 'create', objectName, _context.environmentId, _context.executionContext); + const createInput = { object: objectName, data: body }; const parsedCreate = CreateDataRequestSchema.safeParse(createInput); - if (!parsedCreate.success) return writeRequestRefusal(deps, 'create', parsedCreate.error.issues, createInput); + if (!parsedCreate.success) return requestSchemaRefusal(deps, 'create', parsedCreate.error.issues, createInput); // Spec: returns CreateDataResponse = { object, id, record } const result = await actionExec.callData(deps, _context, 'create', createInput, _context.dataDriver, _context.environmentId, _context.executionContext); const res = deps.success(result);