Skip to content

Commit 95ab93f

Browse files
fix(rest): refuse a ?limit= the door cannot read on import jobs, export, meta history and search (#20061, #20062) (#20137)
Fixes #20061 Fixes #20062 Clause-②: no (narrowing) ## What Four published REST doors read `?limit=` with a bare `Number()`. They substituted, clamped or dropped the result and answered `200`: - `GET /data/import/jobs` - `GET /data/:object/export` - `GET /meta/:type/:name/history` - `GET /search` Each door now reads `limit` against its own declaration and refuses a value outside it. `GET /data/import/jobs` does the same for `offset`, because its declaration bounds `offset` too. The refusal is `400` with the data surface's existing `VALIDATION_FAILED` + `fields[]` envelope, and the service is never called. Nothing a conforming caller sends changes its answer. The change is one private reader in `packages/rest/src/rest-server.ts`, `readDeclaredQueryNumber`, with no new package export: - **import jobs** read through `ListImportJobsRequestSchema.shape.limit` / `.offset`; - **history** reads through `HistoryMetaItemRequestSchema.shape.limit`; - **export** and **search** declare no request schema, so they read through a module-private `z.number().int().optional()`: a whole number, nothing about range. A non-blank numeric string is coerced with `Number()` and handed to the schema. Anything else (`abc`, a blank string, a structured value) goes to the schema as it came, so the declaration refuses it by type. `Number()` never gets to invent a `0` or a `NaN` for it. ## Before and after, measured on the real `RestServer` routes (spy protocol) Every row answered `200` before this PR. | door | request | answered before | answers now | |:--|:--|:--|:--| | import jobs | `?limit=0` / `?limit=-3` | limit 50 / 1 | `400`, `fields[0]` = `limit` / `min_value` | | import jobs | `?limit=201` / `?limit=500` | limit 200 | `400`, `max_value` | | import jobs | `?limit=abc` / `1.5` / `Infinity` / blank | limit 50 / 1.5 / 200 / 50 | `400`, `invalid_type` | | import jobs | `?offset=-1` / `abc` / `1.5` | offset 0 / 0 / 1.5 | `400`, `min_value` / `invalid_type` | | export | `?limit=abc` / empty / blank | `X-Export-Limit: 1` (a one-row export) | `400`, `invalid_type` | | export | `?limit=1.5` / `Infinity` | `X-Export-Limit: 1.5` / `50000` | `400`, `invalid_type` | | meta history | `?limit=abc` / `Infinity` | limit dropped: the whole change log | `400`, `invalid_type` | | meta history | `?limit=` (empty) / blank | limit 0: zero events | `400`, `invalid_type` | | search | `?limit=abc` | `limit: NaN` handed to `searchAll`: no overall cap | `400`, `invalid_type` | | search | `?limit=1.5` / `Infinity` / blank | 1.5 / clamped to 100 / 0 | `400`, `invalid_type` | Unchanged and pinned by the lit controls: - **Import jobs:** absent or empty `limit` → 50, absent or empty `offset` → 0. Conforming values reach `findData` unchanged. - **Export:** absent → 10000. `?limit=25` → 25. The door's own range handling is untouched: `?limit=0` → 1 and `?limit=60000` → 50000. - **History:** absent → no `limit` member. `5`, `0` and `1.5` are forwarded unchanged, because the declaration is `z.number()` with no `int()` and no bounds. - **Search:** absent or empty → `undefined`. `20`, `0` and `500` reach `searchAll` unchanged, and its `[1, 100]` clamp is its own business. The refusal body carries: - `code: 'VALIDATION_FAILED'`; - `fields: [{ field: 'limit', code, message }]`; - an `error` sentence naming the parameter; - `object`, on the export door only. The handler throws `validationFailure` (`@objectstack/types`), and each door's existing catch (`handleRouteError` / `mapDataError`) writes the 400. So `rest-server.ts` gains no response write site, and `check:route-envelope`'s `siblingCode` count on it is unchanged (green). ## PM mechanism assumptions, measured 1. **The `parseIntegerParam` cycle: confirmed.** `packages/runtime/package.json` depends on `@objectstack/rest`. rest's dependencies are `core`, `metadata-core`, `observability`, `platform-objects`, `service-package`, `spec`, `types` (plus `exceljs`, `zod`). Nothing was moved; `packages/runtime/**` and `packages/spec/**` are untouched. 2. **The declared-schema idiom holds, with one measured difference in `fields[].code`.** zod 4.6.1 against the declared schemas, mapped through `zodIssuesToFields`: - `NaN`, a string, `Infinity`, and `1.5` against `int()` all give `invalid_type`; - below `min` gives `min_value`; above `max` gives `max_value`. These are all ADR-0114 catalog members, never zod's own codes. Runtime's `parseIntegerParam` answers `invalid_number` for the same `abc` / `1.5`; see the open question in the report. 3. **The doors match the card, with two corrections.** - Export serves no declared request schema. `CreateExportJobRequestSchema` is the contract of `POST /api/v1/data/:object/export`, which rest does not mount. `ExportRequestSchema` (`contract.zod.ts`) is bound to no route. - The search door holds: the only production caller of `searchAll` is the REST door (`git grep`, tests excluded). So `packages/metadata-protocol/src/protocol.ts` is not in this diff. 4. **The empty-string premise is partly falsified.** Only import jobs and search answered `?limit=` with their default. Export answered a one-row export (`Number('') || 0` → `Math.max(1, 0)`), and history answered zero events (`Number('')` = 0, forwarded). The rule applied is the one `parseEnumParam` (runtime `query-param.ts`) states for the same spelling: - Where the old answer already was the absent answer, empty stays absent. - Where the old answer was an invented `0`, empty is refused. Reading it as absent on export and history would have turned those two into a 10000-row export and the whole change log. That grows the window, the widening this claim stops on, so the reader carries an explicit per-door `emptyIsAbsent`. No door accepts anything it refused before: none refused any single-valued `limit` before this PR. No conforming value's answer changes either, so the declaration stays `no (narrowing)`. ## Tests The new file is `packages/rest/src/rest-server-limit-param-parsing.test.ts` (43 cases). Each refusal pins `status` 400, `code` `VALIDATION_FAILED`, `fields[0].field` and `fields[0].code`, and that the service spy was never called. Each lit control pins the argument the service received. - Against unfixed `6780e34a`: 24 failed (every refusal row answered 200), 19 passed (every lit control). - Against the fix: 43 passed. One existing fixture was corrected in `rest-server-closed-query-params.test.ts`. Its search closed-set loop sent `limit: 'x'` and expected 200; it now takes a valid value per name, as its export twin in the same file already does. Runs at head `74898ed5`, after merging `origin/main`: - **Rest suite:** `pnpm --filter @objectstack/rest exec vitest run --project local --maxWorkers=2` → `Test Files 196 passed (196)`, `Tests 3327 passed | 1 skipped (3328)`. - **Typecheck:** `pnpm --filter @objectstack/rest run typecheck` → exit 0. `tsc --noEmit` passes, and `check:test-typecheck: OK — ... 0 file(s) / 0 error(s)`. Ablations went through `scripts/ablation-replace.mjs` in WRAP mode, run after the fix was committed. Vitest reads `src/` through the relative import, so no dist was involved. - **Import jobs:** restoring `Math.min(200, Math.max(1, Number(q.limit) || 50))` landed on disk (anchor 1 → 0, blob `02ceccd1` → `cab07300`). Result: `Tests 8 failed | 35 passed`, exactly the 8 import-jobs `limit` refusal rows. - **Search:** restoring `req.query?.limit ? Number(req.query.limit) : undefined` landed (blob `02ceccd1` → `9b589c00`). Result: `Tests 4 failed | 39 passed`, exactly the 4 search refusal rows. - Both restores were proven: blob == HEAD `02ceccd1`, `git diff HEAD` empty. Public surface: rest's `dist/index.d.ts` and `dist/index.d.cts`, built from merge-base `bc80e162`'s `rest-server.ts` and from HEAD's, are byte-identical (`cmp`). The restore was proven by blob hash. So no importing package's types can move, and only rest's own tests are owed. ## Gates `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` on the real diff gives 61 commands, identical to the PM's list. All ran at `74898ed5` with exit codes captured before any pipe. `--ran` reconciliation: `61 derived, 59 run, 2 NOT-MEASURED, 0 UNRUN`. - **59 exit 0.** This includes `check:adr-0087-registration --base origin/main` (`not-required (no-migration-prescription)` accepted), `check:changeset-no-major`, `check:empty-changeset`, `check:route-envelope`, `check:nul-bytes`, `check:doc-authoring`, `check:published-files`, `check:issue-citations` and `check:undeclared-dep-imports`. - **NOT MEASURED: `check:dual-build-cjs-loads`**, exit 3 `PREREQUISITE NOT MET`: 43 workspace packages have no `dist/`. Declared narrowing: `require('./dist/index.cjs')` and `import('@objectstack/rest')` both load and expose `RestServer`. - **NOT MEASURED: `check:type-check-debt`**, exit 3 `PREREQUISITE NOT MET`: the full closure is not built. See the `.d.ts` identity above; CI builds the closure before this step. - **`pnpm lint`** (the lane addition) ran as the full union, `eslint . --no-inline-config`, at `74898ed5`: exit 0. ## First-party callers, measured - `@objectstack/client` sends `limit` as written: `data.listImportJobs`, `data.export`, `meta.getHistory`, `search`. - objectui at its pin `f8a9d0fb`: `ImportWizard` sends `limit: 50`, `ResourceHistoryPage` sends `limit: 100`, and the search and export emitters send `limit` only when it is a number greater than 0. None sends a value this PR refuses. ## Acceptance notes - **Same-family reads left out of this diff.** Triage's note 3 keeps them out, and the file is also held by another claim (PR #20125). - Measured on the real routes with a spy protocol, each answering 200: - `GET /meta/:type/:name/history?sinceSeq=abc` drops `sinceSeq` and reads the log from the start (`rest-server.ts:8139`); - `GET /meta/:type/:name/audit?limit=abc` drops `limit`, so the producer's default 100 is served (`:8318`; declared `z.number()`, which refuses `NaN`); - `GET /search?perObject=abc` hands `perObject: NaN` to `searchAll` (`:10709`). - Read only, not measured: `GET /data/approvals/requests` `limit` / `offset` drop a non-numeric value and serve the unpaged list (`:13601`). Export `?page=` falls back to the 500-row chunk (`:10297`), which changes chunking only, not the rows returned. - **History's declared `limit` admits `1.5`, `0` and negatives.** The door forwards them as before. Whether `HistoryMetaItemRequestSchema.limit` should declare `int()` / `min()` is the spec seat's question, and this PR takes no position. - **Export and search range stay as they were:** the export floor of 1 and cap of 50000, and search's `[1, 100]` clamp. Both cards take no position on bounds. - **`GET /data/import/jobs?status=` is not checked against the declared `ImportJobStatus`.** Read only, not measured, out of scope. - **Merge:** `origin/main` was merged (no rebase) after PR #20128 landed on `rest-server.ts`, with no conflict. PR #20125's hunks do not overlap these doors. - **CI-owned, not run locally:** the full `Test Core` shards, `Dogfood`, `Build Core`, `Temporal Conformance`, and the workspace type-check lanes. --- _Generated by [Claude Code](https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent fe677ae commit 95ab93f

4 files changed

Lines changed: 545 additions & 10 deletions

File tree

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
---
2+
'@objectstack/rest': minor
3+
---
4+
5+
fix(rest): four published doors refuse a `?limit=` they cannot read with `400 VALIDATION_FAILED`, instead of substituting, clamping or dropping it and answering `200` (#20061, #20062)
6+
7+
Clause-②: no (narrowing)
8+
9+
**BREAKING**: shipped as `minor` under the launch-window convention
10+
(`check-changeset-no-major` refuses `major` until GA). The banner and the ADR-0087
11+
disposition below carry the breaking-ness, not the level.
12+
13+
`GET /data/import/jobs`, `GET /data/:object/export`, `GET /meta/:type/:name/history`
14+
and `GET /search` read `?limit=` with a bare `Number()`. That coercion does not
15+
fail. It invents a value, and the door served it with a `200`:
16+
- `?limit=0` on the import-job history answered the 50-row default against a
17+
declaration of `min(1).max(200)`;
18+
- `?limit=abc` on the export downloaded one row;
19+
- on the metadata history it returned the whole change log;
20+
- on search it removed the overall cap.
21+
22+
Since `@objectstack/client` sends `limit` exactly as the caller wrote it, the door
23+
is the only place such a value can be refused.
24+
25+
Each door now reads the parameter against its own declaration:
26+
- **`GET /data/import/jobs`** reads `limit` and `offset` through
27+
`ListImportJobsRequestSchema` (`limit` `int().min(1).max(200)`, default 50;
28+
`offset` `int().min(0)`, default 0).
29+
- **`GET /meta/:type/:name/history`** reads `limit` through
30+
`HistoryMetaItemRequestSchema.limit` (`z.number()`, so any finite number, as
31+
before).
32+
- **`GET /data/:object/export`** and **`GET /search`** declare no request schema.
33+
There `limit` must be a whole number. Their range handling is unchanged: the
34+
export floor of 1 and cap of 50000, and search's `[1, 100]` clamp.
35+
36+
A value outside the declaration answers `400` with the data surface's existing
37+
envelope, `{ error, code: 'VALIDATION_FAILED', fields }`. `fields[0].field` names
38+
the parameter. `fields[0].code` is the ADR-0114 member for the failed constraint:
39+
`invalid_type` for a value that is not a number (or not a whole one, where one is
40+
required), `min_value` or `max_value` for one outside a declared bound. The
41+
service is never called.
42+
43+
What changes, per door (every row answered `200` before):
44+
45+
| door | request | answered before | answers now |
46+
|:--|:--|:--|:--|
47+
| `GET /data/import/jobs` | `?limit=0`, `?limit=-3` | 50 rows / 1 row | `400`, `min_value` |
48+
| `GET /data/import/jobs` | `?limit=201`, `?limit=500` | 200 rows | `400`, `max_value` |
49+
| `GET /data/import/jobs` | `?limit=abc`, `?limit=1.5`, `?limit=Infinity` | 50 / 1.5 / 200 rows | `400`, `invalid_type` |
50+
| `GET /data/import/jobs` | `?offset=-1`, `?offset=abc`, `?offset=1.5` | offset 0 / 0 / 1.5 | `400`, `min_value` / `invalid_type` |
51+
| `GET /data/:object/export` | `?limit=abc`, `?limit=` (empty) | a one-row export | `400`, `invalid_type` |
52+
| `GET /data/:object/export` | `?limit=1.5`, `?limit=Infinity` | limit 1.5 / capped to 50000 | `400`, `invalid_type` |
53+
| `GET /meta/:type/:name/history` | `?limit=abc`, `?limit=Infinity` | the whole change log | `400`, `invalid_type` |
54+
| `GET /meta/:type/:name/history` | `?limit=` (empty) | zero events | `400`, `invalid_type` |
55+
| `GET /search` | `?limit=abc` | no overall cap | `400`, `invalid_type` |
56+
| `GET /search` | `?limit=1.5`, `?limit=Infinity` | a cap of 1.5 / 100 | `400`, `invalid_type` |
57+
58+
A blank value such as `?limit=%20` is refused on every door.
59+
60+
**Unchanged:**
61+
- An absent `limit` keeps each door's default: 50 jobs, a 10000-row export, the
62+
full history, search's 20.
63+
- An empty `?limit=` on the import-job history and on search still means absent,
64+
as it always did there.
65+
- Every conforming value reaches the service exactly as before, including the
66+
ranges no card here takes a position on: export `?limit=0` still exports one
67+
row, search `?limit=500` is still clamped to 100, and history still forwards
68+
`0` or `1.5` because its declaration admits them.
69+
70+
**Fix for a caller that now gets the `400`:** send `limit` as a whole number, within
71+
the declared range where the door declares one (`1`–`200` for import jobs), or
72+
omit it to get the door's default.
73+
74+
<!-- adr-0087: not-required (no-migration-prescription) Nothing authored moves: no spec key, metadata property or exported symbol is added, removed or renamed, and `packages/spec` is untouched, so `objectstack migrate meta` has nothing to rewrite and no ADR-0087 registry gains a row. What narrows is the set of HTTP query values four REST doors accept, back to what their declarations (`ListImportJobsRequestSchema`, `HistoryMetaItemRequestSchema`) or a whole-number reading already said. A query string is neither authored nor persisted. The other categories are closed on facts: `@objectstack/rest` publishes (not `unpublished`), no ADR-0087 id covers an HTTP query value (not `registered` / `already-registered`), and the change is runtime behaviour, not a TypeScript declaration (not `runtime-interface-only` / `type-surface-only`). -->

‎packages/rest/src/rest-server-closed-query-params.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -426,9 +426,13 @@ describe('#7606 §3 — GET /search', () => {
426426
});
427427

428428
it('every name in the declared set is accepted — asserted against the export', async () => {
429+
// A VALID value per name, as in the export twin above: `limit` is read
430+
// as a whole number (#20062) and refuses `'x'` with `VALIDATION_FAILED`
431+
// for a reason that has nothing to do with the closed set.
432+
const validValue: Record<string, string> = { limit: '10', perObject: '5' };
429433
for (const name of GLOBAL_SEARCH_PARAMS) {
430434
const { search } = boot();
431-
const answer = await search({ [name]: 'x' });
435+
const answer = await search({ [name]: validValue[name] ?? 'x' });
432436
expect(answer.status, `"${name}" is declared supported but was refused`).toBe(200);
433437
}
434438
});

0 commit comments

Comments
 (0)