diff --git a/.changeset/rest-sub-configs-parsed-not-cast.md b/.changeset/rest-sub-configs-parsed-not-cast.md new file mode 100644 index 0000000000..c0c336dcf8 --- /dev/null +++ b/.changeset/rest-sub-configs-parsed-not-cast.md @@ -0,0 +1,117 @@ +--- +'@objectstack/rest': minor +--- + +**BREAKING (accept-set tightening)**: `RestServer` now parses `config.crud`, +`config.metadata`, `config.batch` and `config.routes` against the schemas that +declare them (`CrudEndpointsConfigSchema`, `MetadataEndpointsConfigSchema`, +`BatchEndpointsConfigSchema`, `RouteGenerationConfigSchema` in +`@objectstack/spec/api`) at construction, instead of casting to them — and +builds the normalized config from the parsed output (#11984). + +The constraints were always declared. `packages/spec/src/api/rest-server.zod.ts` +carries `batch.maxBatchSize: z.number().int().min(1).max(1000)`, the +`routes.nameTransform` and `crud.objectParamStyle` enums and +`metadata.cacheTtl: z.number().int()`, and nothing ran them: both hops into +`@objectstack/rest` are casts, the plugin declares no `configSchema`, and #11637 +deliberately parsed `api` alone so that one narrowing went in front of contract +review rather than five. Measured on the pre-fix tree: `batch.maxBatchSize: 0` +constructed happily and became the live batch cap (`?? 200` does not fire — `0` +is not nullish), and `routes.nameTransform: 'snake_case'` sat in the normalized +config as if it were declared. + +**Newly refused, all at `new RestServer(...)` / `createRestApiPlugin().start()`, +with a message naming the sub-object, the failing key(s) and the declaring +schema** (a construction-time refusal, not an HTTP envelope): + +- `batch.maxBatchSize` outside `1..1000` or not an integer — `0`, `-5`, + `2000`, `2.5`. Refused with zod's own bound text (`expected number to be >=1`, + `<=1000`, `expected int`). +- `routes.nameTransform` outside `'none' | 'plural' | 'kebab-case' | 'camelCase'`. +- `crud.objectParamStyle` outside `'path' | 'query'`. +- `metadata.cacheTtl` that is not an integer (`2.5`, `'60'`). +- A declared key of any of the four written with the wrong type: + `crud.dataPrefix: 42`, `metadata.enableCache: 'yes'`, + `routes.includeObjects: 'account'`, `batch.defaultAtomic: 'yes'`, ... +- An explicit **`null`** at any declared key of the four — + `batch: { maxBatchSize: null }`, `metadata: { cacheTtl: null }`, + `crud: { dataPrefix: null }`. The cast-era `??` chain read `null` as absent + and applied the default; the parse refuses it (`batch.maxBatchSize: Invalid + input: expected number, received null`), because zod's `.default()` fills + `undefined` only. +- A sub-object that is not an object at all — `batch: 'x'`, `routes: []` — + refused at the sub-object root (`batch.(root): Invalid input: expected + object, received string`), where the cast admitted it unchanged and every key + read came back `undefined`, so every key silently took its default. +- `crud.patterns` keyed by an operation outside the CRUD vocabulary + (`patterns: { bogus: {...} }`), or a pattern whose `method` is not an HTTP + method, or a pattern missing its required `path` — `patterns` is an + enum-keyed `z.record`, which zod validates key by key, and + `CrudEndpointPatternSchema.path` is a plain `z.string()`. +- A **partial** `routes.overrides..operations`. That record is + `z.record(CrudOperation, z.boolean())` with a non-optional value, which zod 4 + reads as exhaustive: all five operations must be present. The input TYPE + already demanded all five at typed authoring sites; this is the day the + runtime agrees with `tsc`. #14365 proposes `z.partialRecord` for this record; + when that lands the refusal reverses, and the §A pin for it in + `rest-sub-config-parse-not-cast.test.ts` is deleted with it. + +**Deliberately NOT refused** — the narrowing is exactly what the schemas +declare, and no more: + +- A **negative** `metadata.cacheTtl`. The card that filed this defect listed + "a negative TTL" among the values the parse would refuse; the schema declares + `.int()` only, with no lower bound, so `-1` and `0` stay accepted. A lower + bound is `packages/spec`'s to declare, and is filed separately. +- **Unknown keys inside a sub-object**: all four schemas are non-strict + `z.object()`s, so `batch: { bogus: 1 }` is stripped, not refused — as before, + where the cast simply never read it. +- The retired whole-config key `openApi31` (#4579). Its `retiredKey()` + tombstone lives on `RestServerConfigSchema`, and this seam parses the five + sub-objects rather than the whole config, so the tombstone stays unexecuted: + the key keeps the ignore posture #3963 chose for `api.requireAuth`, and + flipping it into a boot failure is a maintainer's decision, not this seam's. +- `api`: unchanged from #11637 / #12450 (validate-only, `requireAuth` still + `.omit()`ed). + +**The parsed output is now consumed** for the four sub-objects — defaults come +from the schema and unknown keys are stripped — because the decision was +measured per sub-object rather than inherited from `api`: for each of the four, +every key `normalizeConfig` reads is one its schema declares (the key diff is +empty), and none carries a tombstone, so nothing a consumed parse could strip +is anything the runtime honours. The one honoured-but-undeclared key this +family ever had, `metadata.maskObjectFields`, gained its declared seat in +#11983 and is pinned to survive the parse. Defaults are unchanged +(`maxBatchSize` 200, `cacheTtl` 3600, `dataPrefix` `/data`, `prefix` `/meta`, +`nameTransform` `'none'`, every operation/endpoint switch on, masking on per +ADR-0106 D8), and a partial `crud.operations` / `batch.operations` / +`metadata.endpoints` still takes per-key defaults (ADR-0122 author state). The +seam is one table of declared sub-object schemas and one `parseDeclaredSubConfig`; +`api` runs through the same table with its `.omit()`. + +**Migration.** Correct the offending key at its producer; the refusal names the +sub-object, the key, the declared rule and the schema that declares it. A +deployment that meant "no batch cap" wants `enableBatchEndpoint: false` or +`api.enableBatch: false` (the cap's range is the declared policy), and a +partial `routes.overrides..operations` wants all five operations +spelled out (until #14365 lands). + +**In-repo blast radius, measured per sub-object on `origin/main` @ `08e49496f`.** +140 files construct a REST server (`new RestServer(` or +`createRestApiPlugin(`, 277 sites); across all of them, **zero** pass a +`crud` / `metadata` / `batch` / `routes` block carrying any key the four +schemas declare (the `routes: { data: '', ... }` fixtures are `discovery.routes` +payloads, and every `metadata: { ... }` inside those files is endpoint or plugin +metadata — verified by scanning each block for the schema's own keys; positive +control: `rest-server.ts`'s own `NormalizedRestServerConfig` and `normalizeConfig` +blocks hit). +Repo-wide value census of the constrained keys, every file type: `maxBatchSize` +24 lines, 3 out-of-range literals — two are `packages/spec`'s own schema tests +(`0`, `2000`, which never construct a server) and one is a different schema's +key (`tracing.test.ts`); `nameTransform` and `objectParamStyle` 6 lines each, 0 +unknown values; `cacheTtl` 69 lines, 1 non-integer literal (`30.7` in +`packages/runtime`'s endpoint-policy tests — the declarative endpoint's +`cacheTtl`, a different schema). No fixture changes; no in-repo boot path is +affected. + + diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 88108c416a..087009f37f 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -64,7 +64,7 @@ not on any flag. ## How the flag is set `isSystem` is **server-constructed and never client-supplied**. Inbound HTTP -cannot set it (`packages/rest/src/rest-server.ts:1445`, `:1474`), and neither +cannot set it (`packages/rest/src/rest-server.ts:1522`, `:1551`), and neither can an action body (`packages/runtime/src/domains/actions.ts:404`). It is written by internal callers only, as an option on the engine call: @@ -103,7 +103,7 @@ that silently does not happen. | 14 | MCP stdio bridge skips the object API-exposure gate | mcp | Get: the bridge reaches objects whose `apiEnabled` / `apiMethods` would refuse an external caller | `stdio-data-bridge.ts:246` | | 15 | **Read-audit rows are not written** | plugin-audit | Lose: the "a person opened this record" trail. `sudo()` keeps the caller's `userId`, so this flag is the only thing separating a human read from a platform one | `read-audit.ts:556` | | 16 | Approval snapshot payload redaction skipped | plugin-approvals | Get: the whole snapshot on `find` / `findOne` — the audit/replay channel. Lose: field-visibility redaction over approval payloads | `payload-redaction-middleware.ts:115` | -| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1477` | +| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1554` | ### 2. Write pipeline and data integrity @@ -158,7 +158,7 @@ The largest single consumer — **20 of the 109 sites**. |:--|:---|:---|:---|:---| | 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes | `action-execution.ts:136` | | 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:399` | -| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4629`, `:5992`, `:6240`, `:6671`, `:6864` | +| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4716`, `:6079`, `:6327`, `:6758`, `:6951` | | 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 | `meta-write-capability.ts:134` | | 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:411`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:276`, `external-datasource-routes.ts:302`, `package-routes.ts:97` | | 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` | @@ -199,7 +199,7 @@ assuming `isSystem` covers it is a documented source of bugs. | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1516` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:286` | -| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1445`, `:1474`; `domains/actions.ts:404` | +| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1522`, `:1551`; `domains/actions.ts:404` | --- diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 425aef40bc..ce00225a07 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -71,7 +71,7 @@ import { refuseRepeatedQueryParams, assertFilterParamSuppliedOnce } from './quer // ignored filter is the one wrong answer a caller cannot detect. import { refuseUnknownQueryParams } from './query-allowlist.js'; import type { DirectMountedRoute, MountedRouteSource } from './direct-mount.js'; -import { RestServerConfig, RestApiConfig, CrudEndpointsConfig, MetadataEndpointsConfig, BatchEndpointsConfig, RouteGenerationConfig } from '@objectstack/spec/api'; +import { RestServerConfig, RestApiConfig, CrudEndpointsConfigParsed, RouteGenerationConfigParsed } from '@objectstack/spec/api'; // [#11683] The catalog's own floor for "a required `code` and no more specific // one" — see its use in `registerSharingEndpoints`, where the nested ADR-0112 // envelope declares `code` REQUIRED while the flat classification it re-dresses @@ -80,7 +80,14 @@ import { standardErrorCodeForHttpStatus } from '@objectstack/spec/api'; // [#11637] The DECLARED contract for `config.api`, imported as a VALUE rather // than a type. Both hops into this package were casts, so this schema had // never run on any deployment path — see `assertDeclaredApiConfig` below. -import { RestApiConfigSchema } from '@objectstack/spec/api'; +import { + RestApiConfigSchema, + CrudEndpointsConfigSchema, + MetadataEndpointsConfigSchema, + BatchEndpointsConfigSchema, + RouteGenerationConfigSchema, +} from '@objectstack/spec/api'; +import type { z } from 'zod'; import { DataProtocol, MetadataProtocol } from '@objectstack/spec/api'; // [#9741] Declared request shapes for the meta-read doors below — imported so // each door's request literal is compiled against the spec contract instead of @@ -738,7 +745,7 @@ type NormalizedRestServerConfig = { delete: boolean; list: boolean; }; - patterns: CrudEndpointsConfig['patterns']; + patterns: CrudEndpointsConfigParsed['patterns']; dataPrefix: string; objectParamStyle: 'path' | 'query'; }; @@ -774,24 +781,94 @@ type NormalizedRestServerConfig = { includeObjects: string[] | undefined; excludeObjects: string[] | undefined; nameTransform: 'none' | 'plural' | 'kebab-case' | 'camelCase'; - overrides: RouteGenerationConfig['overrides']; + overrides: RouteGenerationConfigParsed['overrides']; + }; +}; + +/** + * The DECLARED contract of each `RestServerConfig` sub-object this seam parses, + * keyed by the sub-object's name — the table `normalizeConfig` runs before it + * builds anything (#11637 for `api`, #11984 for the four siblings). + * + * `api` is the one entry with a subtraction: its retired `requireAuth` + * tombstone is `.omit()`ed because this seam does not own that key's posture + * (see {@link RestServer.assertDeclaredApiConfig}). The four siblings carry no + * tombstone of their own and are taken whole. ⛔ `RestServerConfigSchema` — the + * whole-config schema — is deliberately NOT in this table: its `openApi31` + * tombstone (#4579) is a `retiredKey()` whose parse REFUSES the key, while + * #3963 chose warn-and-ignore for a retired REST config key. Parsing per + * sub-object leaves that tombstone unexecuted, which keeps the posture a + * maintainer would have to flip on purpose. + * + * Built on first use, not at module load: every schema here is a `lazySchema` + * Proxy whose whole point is deferring allocation until someone parses, and + * calling `.omit()` at module top level would resolve `RestApiConfigSchema` on + * every import of this file. Cached because `.omit()` allocates a fresh schema + * and a `RestServer` is constructed per boot (and per test). + */ +function buildDeclaredSubConfigSchemas() { + return { + api: RestApiConfigSchema.omit({ requireAuth: true }), + crud: CrudEndpointsConfigSchema, + metadata: MetadataEndpointsConfigSchema, + batch: BatchEndpointsConfigSchema, + routes: RouteGenerationConfigSchema, }; +} +type DeclaredSubConfigSchemas = ReturnType; +type DeclaredSubConfigName = keyof DeclaredSubConfigSchemas; +let declaredSubConfigSchemasCache: DeclaredSubConfigSchemas | undefined; +function declaredSubConfigSchemas(): DeclaredSubConfigSchemas { + return (declaredSubConfigSchemasCache ??= buildDeclaredSubConfigSchemas()); +} + +/** + * The exported name of each declared schema, for the refusal text: the + * prescription is the payload, and an operator reading a boot failure must be + * able to find the rule that refused them without reading our source. + */ +const DECLARED_SUB_CONFIG_SCHEMA_NAMES: Record = { + api: 'RestApiConfigSchema', + crud: 'CrudEndpointsConfigSchema', + metadata: 'MetadataEndpointsConfigSchema', + batch: 'BatchEndpointsConfigSchema', + routes: 'RouteGenerationConfigSchema', }; /** - * The declared `api` contract, minus the ONE retired key whose posture this seam - * does not own (see {@link RestServer.assertDeclaredApiConfig}). + * Run one sub-object's DECLARED contract and return the parsed output — + * defaults applied, unknown keys stripped (every schema in the table is a + * non-strict `z.object()`). Throws on a value the schema rejects, naming the + * sub-object and every failing key with zod's own issue text. This is a + * construction-time refusal, not an HTTP envelope: nothing has been mounted + * yet, and the operator reading the boot log is the audience. + * + * An absent sub-object parses as `{}` — exactly what the `?? {}` in front of + * the old casts read — so the schema's defaults fill it. * - * Built on first use, not at module load: `RestApiConfigSchema` is a - * `lazySchema` Proxy whose whole point is deferring allocation until someone - * parses, and calling `.omit()` at module top level would resolve it on every - * import of this file. Cached because `.omit()` allocates a fresh schema and a - * `RestServer` is constructed per boot (and per test). + * `rationale` lets a caller append a paragraph the issues justify (the + * `api.version` mount rationale) and ONLY then: appending it unconditionally + * was measured to send an operator to a line of their config they never wrote. */ -function buildDeclaredApiConfigSchema() { - return RestApiConfigSchema.omit({ requireAuth: true }); +function parseDeclaredSubConfig( + name: DeclaredSubConfigName, + schema: T, + value: unknown, + rationale?: (issues: ReadonlyArray) => string, +): z.output { + const result = schema.safeParse(value ?? {}); + if (result.success) return result.data; + + const details = result.error.issues + .map((issue) => ` - ${name}.${issue.path.join('.') || '(root)'}: ${issue.message}`) + .join('\n'); + throw new Error( + `REST API configuration is invalid: \`${name}\` does not satisfy ` + + `\`${DECLARED_SUB_CONFIG_SCHEMA_NAMES[name]}\` (@objectstack/spec/api), the schema that declares it.\n` + + details + + (rationale?.(result.error.issues) ?? ''), + ); } -let declaredApiConfigSchemaCache: ReturnType | undefined; /** * RestServer @@ -3407,55 +3484,59 @@ export class RestServer { * `.omit()` to make some config boot — a strategy outside the enum is * wrong where it is WRITTEN, not where it is read. * - * The sibling sub-objects (`crud`, `metadata`, `batch`, `routes`) are still - * cast, not parsed, and carry declared constraints of their own - * (`batch.maxBatchSize: z.number().int().min(1).max(1000)`, the - * `routes.nameTransform` enum, ...). Same defect class, filed separately: - * this change deliberately puts ONE narrowing in front of contract review - * rather than five. + * The sibling sub-objects (`crud`, `metadata`, `batch`, `routes`) went + * through the same door in [#11984], one narrowing later — parsed by + * `parseDeclaredSubConfig` from the same table, and their parsed output + * CONSUMED. The asymmetry with `api` is measured, not stylistic: for each + * of the four, every key `normalizeConfig` reads is one its schema + * declares (the key diff is empty), and none carries a tombstone, so a + * consumed parse cannot strip anything the runtime honours. `api` keeps + * #11637's validate-only shape here; its `??` chain below duplicates the + * schema's defaults key for key today, and folding it onto the parse is a + * separate, separately-measured change — not a rider on the siblings. */ private assertDeclaredApiConfig(api: unknown): void { - declaredApiConfigSchemaCache ??= buildDeclaredApiConfigSchema(); - const result = declaredApiConfigSchemaCache.safeParse(api ?? {}); - if (result.success) return; - - const details = result.error.issues - .map((issue) => ` - api.${issue.path.join('.') || '(root)'}: ${issue.message}`) - .join('\n'); - // The `version` rationale is appended only when `version` is what - // failed. Measured during this change's own ablation: a - // `projectResolution` refusal printed the whole "an empty version - // mounts the entire API at /api//" paragraph, which reads as a - // diagnosis of a key the operator did not write — worse than no - // rationale, because it sends them to the wrong line of their config. - const versionFailed = result.error.issues.some((issue) => issue.path[0] === 'version'); - throw new Error( - 'REST API configuration is invalid: `api` does not satisfy `RestApiConfigSchema` ' - + '(@objectstack/spec/api), the schema that declares it.\n' - + details - + (versionFailed + parseDeclaredSubConfig('api', declaredSubConfigSchemas().api, api, (issues) => ( + // The `version` rationale is appended only when `version` is what + // failed. Measured during #11637's own ablation: a + // `projectResolution` refusal printed the whole "an empty version + // mounts the entire API at /api//" paragraph, which reads as a + // diagnosis of a key the operator did not write — worse than no + // rationale, because it sends them to the wrong line of their config. + issues.some((issue) => issue.path[0] === 'version') ? '\nThis is refused at construction because `api.version` becomes a path segment in ' + 'EVERY route this server mounts (`getApiBasePath()` = `apiPath ?? ' + '`${basePath}/${version}``) — an empty version mounts the entire API at `/api//`, ' + 'and one carrying `/` splices an extra segment into every route.' - : ''), - ); + : '' + )); } /** * Normalize configuration with defaults */ private normalizeConfig(config: RestServerConfig): NormalizedRestServerConfig { - // [#11637] Parse before the cast, not instead of it: the cast below is - // what makes the rest of this method type-check, and it is only sound - // once the declared contract has actually been run. + // [#11637] `api`: parse BEFORE the cast, not instead of it — the cast + // is what makes the `api` block below type-check, and it is only sound + // once the declared contract has actually been run. Validate-only; see + // `assertDeclaredApiConfig` for why its parsed output is discarded. this.assertDeclaredApiConfig(config.api); const api = (config.api ?? {}) as Partial; - const crud = (config.crud ?? {}) as Partial; - const metadata = (config.metadata ?? {}) as Partial; - const batch = (config.batch ?? {}) as Partial; - const routes = (config.routes ?? {}) as Partial; - + // [#11984] The four siblings: parsed AND consumed. Each used to be + // `(config. ?? {}) as Partial<...>`, so `batch.maxBatchSize: 0` + // was the live batch cap (`0` is not nullish) and + // `routes.nameTransform: 'snake_case'` sat in this config as if it were + // declared. The parsed output is safe to build from because, per + // sub-object, every key read below is one its schema declares + // (measured key by key — the diff is empty for all four), so the + // non-strict parse cannot strip anything the runtime honours, and the + // schema's `.default()`s ARE the defaults: one source, not two. + const schemas = declaredSubConfigSchemas(); + const crud = parseDeclaredSubConfig('crud', schemas.crud, config.crud); + const metadata = parseDeclaredSubConfig('metadata', schemas.metadata, config.metadata); + const batch = parseDeclaredSubConfig('batch', schemas.batch, config.batch); + const routes = parseDeclaredSubConfig('routes', schemas.routes, config.routes); + return { api: { version: api.version ?? 'v1', @@ -3476,8 +3557,11 @@ export class RestServer { crud: { // Per key, not per object: since ADR-0122 `crud.operations` is the // AUTHOR state, so a caller may enable three of the five and leave the - // rest to the schema's own per-key `.default(true)`. `??` on the whole - // object would only have filled it when it was absent entirely. + // rest to the schema's own per-key `.default(true)` — which the parse + // above applies whenever the object is PRESENT. The `??` here covers + // the one case the schema leaves open: `operations` itself is + // `.optional()`, so an absent object arrives as `undefined`, not as + // five defaults. operations: { create: crud.operations?.create ?? true, read: crud.operations?.read ?? true, @@ -3486,13 +3570,13 @@ export class RestServer { list: crud.operations?.list ?? true, }, patterns: crud.patterns, - dataPrefix: crud.dataPrefix ?? '/data', - objectParamStyle: crud.objectParamStyle ?? 'path', + dataPrefix: crud.dataPrefix, + objectParamStyle: crud.objectParamStyle, }, metadata: { - prefix: metadata.prefix ?? '/meta', - enableCache: metadata.enableCache ?? true, - cacheTtl: metadata.cacheTtl ?? 3600, + prefix: metadata.prefix, + enableCache: metadata.enableCache, + cacheTtl: metadata.cacheTtl, // [ADR-0106 D8] Default ON — masking is the platform default and // ships with the current major. The key has a declared seat // (`MetadataEndpointsConfigSchema.maskObjectFields` in @@ -3502,6 +3586,8 @@ export class RestServer { // knob the runtime `/metadata` dispatcher shares (it has no REST // config to read). maskObjectFields: isObjectSchemaMaskingEnabled(metadata.maskObjectFields), + // `endpoints` is `.optional()` like `crud.operations` above: the + // `??` is for the absent object; the parse fills a present one. endpoints: { types: metadata.endpoints?.types ?? true, items: metadata.endpoints?.items ?? true, @@ -3510,20 +3596,21 @@ export class RestServer { }, }, batch: { - maxBatchSize: batch.maxBatchSize ?? 200, - enableBatchEndpoint: batch.enableBatchEndpoint ?? true, + maxBatchSize: batch.maxBatchSize, + enableBatchEndpoint: batch.enableBatchEndpoint, + // `operations` is `.optional()` — same shape as `crud.operations`. operations: { createMany: batch.operations?.createMany ?? true, updateMany: batch.operations?.updateMany ?? true, deleteMany: batch.operations?.deleteMany ?? true, upsertMany: batch.operations?.upsertMany ?? true, }, - defaultAtomic: batch.defaultAtomic ?? true, + defaultAtomic: batch.defaultAtomic, }, routes: { includeObjects: routes.includeObjects, excludeObjects: routes.excludeObjects, - nameTransform: routes.nameTransform ?? 'none', + nameTransform: routes.nameTransform, overrides: routes.overrides, }, }; diff --git a/packages/rest/src/rest-sub-config-parse-not-cast.test.ts b/packages/rest/src/rest-sub-config-parse-not-cast.test.ts new file mode 100644 index 0000000000..8480db2195 --- /dev/null +++ b/packages/rest/src/rest-sub-config-parse-not-cast.test.ts @@ -0,0 +1,355 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11984] The REST server runs the DECLARED contract of its four sibling + * sub-objects — `crud`, `metadata`, `batch`, `routes` — at construction. + * + * #11637 made `RestServer.normalizeConfig` parse `config.api` against + * `RestApiConfigSchema` before the cast the rest of the method type-checks + * against, and deliberately left the four siblings cast-only so that ONE + * narrowing went in front of contract review rather than five. Each sibling + * carries declared constraints that consequently never executed + * (`packages/spec/src/api/rest-server.zod.ts`): + * + * batch.maxBatchSize: z.number().int().min(1).max(1000).default(200) + * routes.nameTransform: z.enum(['none', 'plural', 'kebab-case', 'camelCase']) + * crud.objectParamStyle: z.enum(['path', 'query']) + * metadata.cacheTtl: z.number().int().default(3600) + * + * Measured on the pre-change tree (`origin/main` @ `08e49496f`): every value + * in §A constructed a server. `batch.maxBatchSize: 0` became the live batch + * cap (`maxBatch = batch.maxBatchSize ?? 200` — `0` is not nullish), and + * `routes.nameTransform: 'snake_case'` sat in the normalized config as if it + * were declared. + * + * ⛔ ANTI-VACUITY — the same rule as `rest-config-parse-not-cast.test.ts`: a + * pin that asks the SCHEMA whether it refuses `maxBatchSize: 0` is green on + * every tree (`packages/spec`'s own `rest-server.test.ts` already pins that). + * Every case below drives the REAL `RestServer` construction — or, in §B, the + * real plugin composition — so what it measures is whether the SERVER refuses. + * `refusal()` answers `''` when construction succeeds, and `''` contains no + * key name, so every `toContain` below is its own positive control. + * + * §C bounds the narrowing: the seam refuses exactly what the schema declares + * and nothing this seam invented. Two bounds are named because the card that + * filed this defect guessed them wrong: a NEGATIVE `cacheTtl` is declared + * `.int()` only, so it stays accepted; and an UNKNOWN key inside a sub-object + * is stripped, not refused — all four schemas are non-strict `z.object()`s. + * + * §D pins the consumption decision. For these four sub-objects every key + * `normalizeConfig` reads is declared by the sub-object's schema (measured + * key by key; the diff is empty for all four), so the PARSED output is what + * the normalized config is built from and the schema's own defaults are the + * defaults. `api` keeps #11637's validate-only posture — its `.omit()`ed + * tombstone is the reason — and is not this file's subject. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IHttpServer } from '@objectstack/spec/contracts'; +import type { RestServerConfig } from '@objectstack/spec/api'; +import { RestServer, type RestProtocol } from './rest-server.js'; +import { createRestApiPlugin } from './rest-api-plugin.js'; + +function makeServer(): IHttpServer { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), listen: vi.fn(), close: vi.fn(), + } as unknown as IHttpServer; +} + +function makeProtocol(): RestProtocol { + return { + getMetaItems: vi.fn(async ({ type }: { type: string }) => ({ type, items: [] })), + } as unknown as RestProtocol; +} + +/** Construct the real server with the config as given — the seam under test. */ +function construct(config: RestServerConfig): RestServer { + return new RestServer(makeServer(), makeProtocol(), config); +} + +/** + * The construction refusal's message, or `''` when the server constructed. + * An empty answer fails every `toContain` below on its own, which is what + * makes each of them a positive control for the `not.toContain` beside it. + */ +function refusal(config: RestServerConfig): string { + try { + construct(config); + return ''; + } catch (err) { + return err instanceof Error ? err.message : String(err); + } +} + +/** The part of the normalized config these pins read back. */ +type NormalizedView = { + crud: { + operations: Record<'create' | 'read' | 'update' | 'delete' | 'list', boolean>; + patterns: Record | undefined; + dataPrefix: string; + objectParamStyle: string; + }; + metadata: { + prefix: string; + cacheTtl: number; + maskObjectFields: boolean; + endpoints: Record<'types' | 'items' | 'item' | 'schema', boolean>; + }; + batch: { + maxBatchSize: number; + defaultAtomic: boolean; + operations: Record<'createMany' | 'updateMany' | 'deleteMany' | 'upsertMany', boolean>; + }; + routes: { + includeObjects: string[] | undefined; + nameTransform: string; + }; +}; + +/** Read the normalized config back off a constructed server. */ +function normalized(config: RestServerConfig): NormalizedView { + return (construct(config) as unknown as { config: NormalizedView }).config; +} + +// --------------------------------------------------------------------------- +// §A — the server refuses what each sibling schema declares invalid +// --------------------------------------------------------------------------- + +describe('[#11984] §A RestServer construction runs the four sibling schemas', () => { + it('refuses `batch.maxBatchSize: 0` — the value that became the live batch cap', () => { + const message = refusal({ batch: { maxBatchSize: 0 } }); + expect(message, 'the refusal must name the sub-object AND the key').toContain('batch.maxBatchSize'); + expect(message, 'and the schema that declares the bound').toContain('BatchEndpointsConfigSchema'); + expect(message, "zod's own issue text carries the declared bound").toContain('>=1'); + }); + + it('refuses `batch.maxBatchSize: 2000` — above the declared maximum', () => { + const message = refusal({ batch: { maxBatchSize: 2000 } }); + expect(message).toContain('batch.maxBatchSize'); + expect(message).toContain('<=1000'); + }); + + it('refuses `batch.maxBatchSize: 2.5` — declared `.int()`', () => { + expect(refusal({ batch: { maxBatchSize: 2.5 } })).toContain('batch.maxBatchSize'); + }); + + it('refuses `routes.nameTransform: "snake_case"` — an option outside the declared enum', () => { + const message = refusal({ routes: { nameTransform: 'snake_case' as never } }); + expect(message).toContain('routes.nameTransform'); + expect(message).toContain('RouteGenerationConfigSchema'); + expect(message, 'the declared vocabulary is part of the prescription').toContain('kebab-case'); + }); + + it('refuses `crud.objectParamStyle: "header"` — an option outside the declared enum', () => { + const message = refusal({ crud: { objectParamStyle: 'header' as never } }); + expect(message).toContain('crud.objectParamStyle'); + expect(message).toContain('CrudEndpointsConfigSchema'); + }); + + it('refuses `metadata.cacheTtl: 2.5` — declared `.int()`', () => { + const message = refusal({ metadata: { cacheTtl: 2.5 } }); + expect(message).toContain('metadata.cacheTtl'); + expect(message).toContain('MetadataEndpointsConfigSchema'); + }); + + it('refuses a declared key written with the wrong type', () => { + expect(refusal({ crud: { dataPrefix: 42 as never } })).toContain('crud.dataPrefix'); + expect(refusal({ metadata: { enableCache: 'yes' as never } })).toContain('metadata.enableCache'); + expect(refusal({ routes: { includeObjects: 'account' as never } })).toContain('routes.includeObjects'); + }); + + it('refuses `crud.patterns` keyed by an operation the CRUD vocabulary does not contain', () => { + // `patterns` is `z.record(CrudOperation, ...)`: an enum-keyed record, + // which zod validates key by key — so a pattern for an operation that + // does not exist is refused, not carried along and never matched. + const message = refusal({ crud: { patterns: { bogus: { method: 'GET', path: '/x' } } as never } }); + expect(message).toContain('crud.patterns'); + expect(message).toContain('bogus'); + }); + + it('refuses a partial `routes.overrides..operations` — the declared record is exhaustive over the five operations', () => { + // Same enum-keyed record, with a NON-optional value: zod requires + // every declared operation, so the missing ones are named one by one. + // The input TYPE already demanded all five at typed authoring sites; + // this is the day the runtime agrees with `tsc`. + const message = refusal({ routes: { overrides: { account: { operations: { list: false } as never } } } }); + expect(message).toContain('routes.overrides.account.operations.create'); + expect(message).toContain('routes.overrides.account.operations.read'); + }); + + it('lists every failing key of the sub-object in one refusal', () => { + const message = refusal({ batch: { maxBatchSize: 0, defaultAtomic: 'yes' as never } }); + expect(message).toContain('batch.maxBatchSize'); + expect(message).toContain('batch.defaultAtomic'); + }); + + it('a sibling refusal never diagnoses `api.version` — a key this config did not write', () => { + // #11637 appends an "empty version mounts the API at /api//" rationale + // when `api.version` is what failed. A sibling refusal must not + // inherit it: that paragraph sends the operator to a line they never + // wrote. + const message = refusal({ batch: { maxBatchSize: 0 } }); + expect(message, 'positive control: the refusal is present').toContain('batch.maxBatchSize'); + expect(message).not.toContain('/api//'); + expect(message).not.toContain('api.version'); + }); +}); + +// --------------------------------------------------------------------------- +// §B — the real plugin composition, i.e. BOTH cast hops +// --------------------------------------------------------------------------- + +type StartContext = Parameters['start']>>[0]; + +function bootCtx(): StartContext { + const services: Record = { 'http.server': makeServer(), protocol: makeProtocol() }; + return { + registerService: vi.fn(), + getService: vi.fn((name: string) => { + if (name in services) return services[name]; + throw new Error(`Service '${name}' not found`); + }), + getServices: vi.fn(() => new Map(Object.entries(services))), + hook: vi.fn(), + trigger: vi.fn().mockResolvedValue(undefined), + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + getKernel: vi.fn(), + } as unknown as StartContext; +} + +describe('[#11984] §B the refusal survives the plugin path', () => { + it('CONTROL: this ctx really does boot a REST server', async () => { + // `createRestApiPlugin.start()` returns quietly when `http.server` or + // `protocol` is missing, so a rejection below could otherwise be + // attributed to a thin ctx rather than to the config. + const ctx = bootCtx(); + await expect(createRestApiPlugin({ api: { batch: { maxBatchSize: 200 } } }).start!(ctx)).resolves.toBeUndefined(); + }); + + it('rejects `createRestApiPlugin({ api: { batch: { maxBatchSize: 0 } } }).start()`', async () => { + await expect( + createRestApiPlugin({ api: { batch: { maxBatchSize: 0 } } }).start!(bootCtx()), + ).rejects.toThrow(/batch\.maxBatchSize/); + }); + + it('rejects `createRestApiPlugin({ api: { routes: { nameTransform: "snake_case" } } }).start()`', async () => { + await expect( + createRestApiPlugin({ api: { routes: { nameTransform: 'snake_case' as never } } }).start!(bootCtx()), + ).rejects.toThrow(/routes\.nameTransform/); + }); +}); + +// --------------------------------------------------------------------------- +// §C — REGRESSION GUARDS (green BEFORE this change and after) +// --------------------------------------------------------------------------- + +describe('[#11984] §C regression guards — the narrowing is exactly the declared one', () => { + it('an empty config still constructs, with the declared defaults', () => { + const cfg = normalized({}); + expect(cfg.batch.maxBatchSize).toBe(200); + expect(cfg.metadata.cacheTtl).toBe(3600); + expect(cfg.metadata.prefix).toBe('/meta'); + expect(cfg.crud.dataPrefix).toBe('/data'); + expect(cfg.crud.objectParamStyle).toBe('path'); + expect(cfg.routes.nameTransform).toBe('none'); + }); + + it('accepts the declared `maxBatchSize` bounds inclusively', () => { + expect(normalized({ batch: { maxBatchSize: 1 } }).batch.maxBatchSize).toBe(1); + expect(normalized({ batch: { maxBatchSize: 1000 } }).batch.maxBatchSize).toBe(1000); + }); + + it('accepts every option the declared enums contain, and reads each back', () => { + for (const nameTransform of ['none', 'plural', 'kebab-case', 'camelCase'] as const) { + expect(normalized({ routes: { nameTransform } }).routes.nameTransform, nameTransform).toBe(nameTransform); + } + for (const objectParamStyle of ['path', 'query'] as const) { + expect(normalized({ crud: { objectParamStyle } }).crud.objectParamStyle, objectParamStyle).toBe(objectParamStyle); + } + }); + + it('KEEPS a negative `metadata.cacheTtl` — declared `.int()` only, with no lower bound', () => { + // The bound on the narrowing: the card that filed this defect listed + // "a negative TTL" among the values the parse would refuse, and the + // schema declares no such rule. This seam enforces the contract as + // written; a lower bound is `packages/spec`'s to declare. + expect(normalized({ metadata: { cacheTtl: -1 } }).metadata.cacheTtl).toBe(-1); + expect(normalized({ metadata: { cacheTtl: 0 } }).metadata.cacheTtl).toBe(0); + }); + + it('STRIPS an unknown key inside a sub-object rather than refusing it — the schemas are non-strict', () => { + expect(() => construct({ batch: { bogus: 1 } as never })).not.toThrow(); + expect(() => construct({ routes: { overrides: { account: { enabled: false, bogus: 1 } as never } } })).not.toThrow(); + }); + + it('KEEPS the retired top-level `openApi31` key at its ignore posture — the whole-config tombstone is not run here', () => { + // `RestServerConfigSchema.openApi31` is a `retiredKey()` tombstone + // (#4579) whose parse REFUSES the key. This seam parses the five + // sub-objects, never the whole config, so the tombstone keeps the + // posture #3963 chose for `api.requireAuth`: flipping either into a + // boot failure is that decision's to make, not this seam's. + expect(() => construct({ openApi31: {} } as never)).not.toThrow(); + }); + + it('the `api` sub-object still runs its own declared contract (#11637)', () => { + expect(refusal({ api: { version: '' } })).toContain('api.version'); + }); +}); + +// --------------------------------------------------------------------------- +// §D — the parsed output is CONSUMED: defaults come from the schema, and what +// an author wrote survives it +// --------------------------------------------------------------------------- + +describe('[#11984] §D the four siblings consume the parsed output', () => { + it('a declared in-range value is preserved, not replaced by the default', () => { + expect(normalized({ batch: { maxBatchSize: 500 } }).batch.maxBatchSize).toBe(500); + expect(normalized({ metadata: { cacheTtl: 60 } }).metadata.cacheTtl).toBe(60); + expect(normalized({ crud: { dataPrefix: '/records' } }).crud.dataPrefix).toBe('/records'); + expect(normalized({ routes: { includeObjects: ['account'] } }).routes.includeObjects).toEqual(['account']); + }); + + it('KEEPS `metadata.maskObjectFields: false` — the ADR-0106 D8 opt-out survives the parse', () => { + // The reason this card waited on #11983: before the key had a + // declared seat, a consumed parse would have STRIPPED it and turned + // masking back on for a deployment that turned it off. + expect(normalized({ metadata: { maskObjectFields: false } }).metadata.maskObjectFields).toBe(false); + expect(normalized({}).metadata.maskObjectFields).toBe(true); + }); + + it('KEEPS a partial `crud.operations` — per-key defaults, the AUTHOR state (ADR-0122)', () => { + expect(normalized({ crud: { operations: { list: false } } }).crud.operations).toEqual({ + create: true, read: true, update: true, delete: true, list: false, + }); + expect(normalized({}).crud.operations).toEqual({ + create: true, read: true, update: true, delete: true, list: true, + }); + }); + + it('KEEPS a partial `batch.operations` and `metadata.endpoints` the same way', () => { + expect(normalized({ batch: { operations: { deleteMany: false } } }).batch.operations).toEqual({ + createMany: true, updateMany: true, deleteMany: false, upsertMany: true, + }); + expect(normalized({ metadata: { endpoints: { schema: false } } }).metadata.endpoints).toEqual({ + types: true, items: true, item: true, schema: false, + }); + }); + + it('KEEPS a partial `crud.patterns` — the written pattern survives, and no pattern is invented', () => { + // `patterns` is `z.record(CrudOperation, CrudEndpointPatternSchema.optional())`, + // which zod 4 reads as an EXHAUSTIVE record: the parse walks all five + // operations and writes each one's value into the output, so the four + // an author did not write come back as explicit `undefined` entries — + // exactly the declared shape (`Record`, + // which is also why this fixture needs `as never`: the input TYPE + // demands all five keys while the runtime accepts a partial). That + // key-enumeration quirk is the spec's to settle (`z.partialRecord`), + // filed separately, so this pin asserts only what the contract + // promises whichever way that lands: the one written pattern is + // preserved, and no operation gains a pattern it was not given. + const cfg = normalized({ crud: { patterns: { list: { method: 'GET', path: '/x' } } as never } }); + expect(cfg.crud.patterns?.list).toEqual({ method: 'GET', path: '/x' }); + expect(Object.values(cfg.crud.patterns ?? {}).filter((pattern) => pattern !== undefined)).toHaveLength(1); + }); +});