From 54658aa459c0e18c09f02bbf3bd604797a1a7861 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 23:45:01 +0000 Subject: [PATCH 1/2] test(rest): pin the remaining numeric query reads' refusals (red on base) Per-door pins for the #20139 family members: history sinceSeq, audit limit, diff from/to, search perObject, approvals limit/offset. Claude-Session: https://claude.ai/code/session_01UYBdGBzWSrAMzpW8ah3GbP Co-authored-by: Claude --- .../rest-server-query-number-reads.test.ts | 406 ++++++++++++++++++ 1 file changed, 406 insertions(+) create mode 100644 packages/rest/src/rest-server-query-number-reads.test.ts diff --git a/packages/rest/src/rest-server-query-number-reads.test.ts b/packages/rest/src/rest-server-query-number-reads.test.ts new file mode 100644 index 00000000000..3b42f7d799c --- /dev/null +++ b/packages/rest/src/rest-server-query-number-reads.test.ts @@ -0,0 +1,406 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20139] The rest of `rest-server.ts`'s numeric query reads: a value the door + * cannot read is REFUSED — never dropped, substituted or handed on as `NaN`. + * + * The family's first four doors (`?limit=` on import jobs, export, meta history + * and search) were closed by `readDeclaredQueryNumber` + * (`rest-server-limit-param-parsing.test.ts`). These are the remaining members, + * found by the census (`rest-server-query-number-census.test.ts`), each of which + * read its parameter with a bare `Number(...)` and answered `200`: + * + * | door | parameter | what an unreadable value did | + * | :------------------------------------- | :--------------------- | :------------------------------------------------ | + * | `GET /meta/:type/:name/history` | `sinceSeq` | dropped: the log was read from the start | + * | `GET /meta/:type/:name/audit` | `limit` | dropped: the producer's default 100 was served | + * | `GET /meta/:type/:name/diff` | `from` / `to` (+alias) | dropped: a DIFFERENT pair of versions was diffed | + * | `GET /search` | `perObject` | `NaN` handed to `searchAll`: no per-object cap | + * | `GET /approvals/requests` | `limit` / `offset` | dropped: the unpaged 500-row window / first page | + * + * `/diff` is not on the card's list. The census found it: its `Number(raw)` sat + * in a local `parseV` helper, one frame away from the `req.query` it read, so a + * search for `Number(req.query` never saw it. + * + * Each door now reads the parameter against its own DECLARATION where one + * exists (`HistoryMetaItemRequestSchema.sinceSeq`, + * `AuditMetaItemRequestSchema.limit`, both `z.number().optional()`) and as a + * whole number where none does (`/diff`, `/search`, approvals). The refusal is + * the data surface's `400 VALIDATION_FAILED` + `fields[]` envelope, with + * `fields[0].field` naming the parameter as the caller spelled it and + * `fields[0].code` the ADR-0114 catalog member. + * + * ## Every case asserts BOTH halves + * + * A refusal case pins `status` + `code` + the named field AND that the service + * was never called: "still answered something" is exactly what the defect + * looked like. A lit control pins the ARGUMENT the service received for a + * conforming value, so a fix that refused everything could not pass either. + * + * ## The empty string, per door + * + * Decided the way the family's reader decides it: from what the door answered + * for `?x=` before. Where that already WAS the absent answer (search's falsy + * guard, `/diff`'s `parseV('')`), empty stays absent. Where `Number('')` + * invented a `0` that changed the answer (history's `sinceSeq: 0`, which the + * SQL repository's `event_seq <= 0` test turns into a filter; audit's one-event + * clamp; approvals' one-row page or paged mode), empty is refused. + * + * ## What is deliberately NOT asserted + * + * Bounds. No card here takes a position on them: `0`, negatives and (where the + * declaration has no `int()`) fractions reach each service exactly as before, + * and each service's own clamp is untouched. + */ + +import { describe, it, expect, vi } from 'vitest'; +// `.js` on purpose — NodeNext resolution requires the extension. +import { RestServer } from './rest-server.js'; + +const META = '/api/v1/meta'; +const APPROVALS = '/api/v1/approvals/requests'; + +function mockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; +} + +function mockRes() { + const res: any = { + statusCode: 200, + _headers: {} as Record, + json: vi.fn(function (this: any, body: any) { this._body = body; return this; }), + send: vi.fn(function (this: any) { return this; }), + write: vi.fn(function (this: any) { return true; }), + end: vi.fn(function (this: any) { return this; }), + setHeader: vi.fn(function (this: any) { return this; }), + status: vi.fn(function (this: any, code: number) { this.statusCode = code; return this; }), + header: vi.fn(function (this: any, k: string, v: string) { this._headers[k] = v; return this; }), + }; + return res; +} + +/** + * The real `RestServer` over a spy protocol and a spy approvals service. + * `isSystem` clears the capability gates that run BEFORE the query is read, + * so every request reaches the read it is named after, and a request that is + * not refused runs all the way to the service call the lit controls inspect. + * `view` is a type no per-caller gate judges, so the event and diff doors + * reach their protocol verb with no extra read in between. + */ +function boot() { + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaItem: vi.fn().mockResolvedValue({ + type: 'view', name: 'all_accounts', item: { name: 'all_accounts' }, lock: 'none', + }), + historyMetaItem: vi.fn().mockResolvedValue({ events: [] }), + auditMetaItem: vi.fn().mockResolvedValue({ events: [] }), + diffMetaItem: vi.fn().mockResolvedValue({ + type: 'view', name: 'all_accounts', fromVersion: null, toVersion: null, + added: [], removed: [], changed: [], + }), + searchAll: vi.fn().mockResolvedValue({ + query: 'acme', hits: [], pages: [], totalObjects: 0, totalHits: 0, truncated: false, + }), + findData: vi.fn().mockResolvedValue({ records: [] }), + }; + const approvals = { + listRequests: vi.fn().mockResolvedValue([{ id: 'req_1' }, { id: 'req_2' }]), + countRequests: vi.fn().mockResolvedValue(2), + }; + + const rest = new RestServer( + mockServer() as any, + protocol as any, + { api: { requireAuth: false } } as any, + undefined, // kernelManager + undefined, // envRegistry + undefined, // defaultEnvironmentIdProvider + undefined, // authServiceProvider + undefined, // objectQLProvider + undefined, // emailServiceProvider + undefined, // sharingServiceProvider + undefined, // reportsServiceProvider + async () => approvals, // approvalsServiceProvider + ); + (rest as any).resolveExecCtx = async () => ({ isSystem: true, userId: 'u1' }); + rest.registerRoutes(); + + const drive = async (method: string, path: string, req: Record = {}) => { + const found = (rest as any).getRoutes().find( + (r: any) => r.method === method && r.path === path, + ); + if (!found) throw new Error(`route not registered: ${method} ${path}`); + const res = mockRes(); + await found.handler( + { method, path, params: {}, query: {}, headers: {}, body: {}, ...req } as any, + res, + ); + return { status: res.statusCode, body: res.json.mock.calls.at(-1)?.[0] }; + }; + + return { protocol, approvals, drive }; +} + +type Answer = { status: number; body: any }; + +/** The refusal half: status, top-level code, and the named field with its catalog code. */ +function expectRefusal(answer: Answer, param: string, fieldCode: string) { + expect( + answer.status, + `expected a 400 refusal for ${param}, got ${answer.status} with body ${JSON.stringify(answer.body)}`, + ).toBe(400); + expect(answer.body?.code).toBe('VALIDATION_FAILED'); + expect(Array.isArray(answer.body?.fields), 'fields[] must be present').toBe(true); + expect(answer.body.fields[0]?.field).toBe(param); + expect(answer.body.fields[0]?.code).toBe(fieldCode); +} + +const ITEM = { type: 'view', name: 'all_accounts' }; + +// ───────────────────────────────────────────────────────────────────────────── +// 1. GET /meta/:type/:name/history — `HistoryMetaItemRequestSchema.sinceSeq` +// is `z.number().optional()`: any finite number, no int, no bounds. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20139 — GET /meta/:type/:name/history refuses a `sinceSeq` its declaration refuses', () => { + const history = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('GET', `${META}/:type/:name/history`, { params: ITEM, query }); + return { answer, protocol }; + }; + + it.each([ + ['abc', 'invalid_type'], // was: `NaN`, dropped → the log read from the start + ['Infinity', 'invalid_type'], // was: dropped → the log read from the start + ['', 'invalid_type'], // was: `Number('')` → `sinceSeq: 0`, a filter nobody asked for + [' ', 'invalid_type'], // was: `sinceSeq: 0` + ])('?sinceSeq=%j is refused as %s and the log is never read', async (sinceSeq, fieldCode) => { + const { answer, protocol } = await history({ sinceSeq }); + expectRefusal(answer, 'sinceSeq', fieldCode); + expect(protocol.historyMetaItem, 'the change log may not have been read').not.toHaveBeenCalled(); + }); + + it.each([ + ['5', 5], + // Declared `z.number()` admits these; forwarded exactly as before. + ['0', 0], + ['1.5', 1.5], + ])('lit control: ?sinceSeq=%j reaches historyMetaItem as %s', async (sinceSeq, expected) => { + const { answer, protocol } = await history({ sinceSeq }); + expect(answer.status).toBe(200); + expect(protocol.historyMetaItem).toHaveBeenCalledTimes(1); + expect(protocol.historyMetaItem.mock.calls[0][0].sinceSeq).toBe(expected); + }); + + it('an absent sinceSeq still reads from the beginning (no sinceSeq member at all)', async () => { + const { answer, protocol } = await history({}); + expect(answer.status).toBe(200); + expect('sinceSeq' in protocol.historyMetaItem.mock.calls[0][0]).toBe(false); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 2. GET /meta/:type/:name/audit — `AuditMetaItemRequestSchema.limit` is +// `z.number().optional()`; the implementation's [1, 500] clamp is its own. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20139 — GET /meta/:type/:name/audit refuses a `limit` its declaration refuses', () => { + const audit = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('GET', `${META}/:type/:name/audit`, { params: ITEM, query }); + return { answer, protocol }; + }; + + it.each([ + ['abc', 'invalid_type'], // was: `NaN`, dropped → the producer default 100 + ['Infinity', 'invalid_type'], // was: dropped → 100 + ['', 'invalid_type'], // was: `Number('')` → 0 → clamped to ONE event + [' ', 'invalid_type'], // was: 0 → one event + ])('?limit=%j is refused as %s and the trail is never read', async (limit, fieldCode) => { + const { answer, protocol } = await audit({ limit }); + expectRefusal(answer, 'limit', fieldCode); + expect(protocol.auditMetaItem, 'the audit trail may not have been read').not.toHaveBeenCalled(); + }); + + it.each([ + ['25', 25], + // Declared `z.number()` admits these; the implementation's clamp is its own business. + ['0', 0], + ['1.5', 1.5], + ['900', 900], + ])('lit control: ?limit=%j reaches auditMetaItem as %s', async (limit, expected) => { + const { answer, protocol } = await audit({ limit }); + expect(answer.status).toBe(200); + expect(protocol.auditMetaItem).toHaveBeenCalledTimes(1); + expect(protocol.auditMetaItem.mock.calls[0][0].limit).toBe(expected); + }); + + it('an absent limit leaves the default to the producer (no limit member at all)', async () => { + const { answer, protocol } = await audit({}); + expect(answer.status).toBe(200); + expect('limit' in protocol.auditMetaItem.mock.calls[0][0]).toBe(false); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 3. GET /meta/:type/:name/diff — no declared request schema; a history +// version must be a whole number. `from`/`to` win over their +// `fromVersion`/`toVersion` spellings, exactly as before. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20139 — GET /meta/:type/:name/diff refuses a version it cannot read', () => { + const diff = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('GET', `${META}/:type/:name/diff`, { params: ITEM, query }); + return { answer, protocol }; + }; + + it.each([ + [{ from: 'abc' }, 'from'], // was: dropped → "the version before `to`" diffed instead + [{ from: '1.5' }, 'from'], // was: forwarded; no version 1.5 exists → a diff against nothing + [{ from: 'Infinity' }, 'from'], // was: dropped + [{ from: ' ' }, 'from'], // was: `Number(' ')` → version 0 + [{ to: 'abc' }, 'to'], // was: dropped → the CURRENT body diffed instead + [{ to: '2.5' }, 'to'], + [{ fromVersion: 'abc' }, 'fromVersion'], + [{ toVersion: 'abc' }, 'toVersion'], + [{ from: '2', to: 'abc' }, 'to'], + ])('%j is refused naming %s, and no diff is computed', async (query, param) => { + const { answer, protocol } = await diff(query); + expectRefusal(answer, param, 'invalid_type'); + expect(protocol.diffMetaItem, 'no diff may have been computed').not.toHaveBeenCalled(); + }); + + it.each([ + [{ from: '2', to: '3' }, { fromVersion: 2, toVersion: 3 }], + [{ fromVersion: '1', toVersion: '4' }, { fromVersion: 1, toVersion: 4 }], + // `from` wins over `fromVersion`, as the old `??` had it. + [{ from: '2', fromVersion: '7' }, { fromVersion: 2 }], + // No position on bounds: a whole number reaches the verb unchanged. + [{ from: '0' }, { fromVersion: 0 }], + ])('lit control: %j reaches diffMetaItem as %j', async (query, expected) => { + const { answer, protocol } = await diff(query); + expect(answer.status).toBe(200); + expect(protocol.diffMetaItem).toHaveBeenCalledTimes(1); + expect(protocol.diffMetaItem.mock.calls[0][0]).toMatchObject(expected); + }); + + it.each([ + ['absent', {}], + // `parseV('')` answered `undefined`: empty already meant absent here. + ['empty', { from: '', to: '' }], + ])('%s from/to still mean previous-vs-current (no version members)', async (_label, query) => { + const { answer, protocol } = await diff(query); + expect(answer.status).toBe(200); + const request = protocol.diffMetaItem.mock.calls[0][0]; + expect('fromVersion' in request).toBe(false); + expect('toVersion' in request).toBe(false); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 4. GET /search — no declared request schema; `perObject` is a result cap +// and must be a whole number. `searchAll`'s own [1, 25] clamp is untouched. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20139 — GET /search refuses a `perObject` it cannot read', () => { + const search = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('GET', '/api/v1/search', { query: { q: 'acme', ...query } }); + return { answer, protocol }; + }; + + it.each([ + ['abc', 'invalid_type'], // was: `NaN` → the per-object cap never applied + ['1.5', 'invalid_type'], // was: a per-object cap of 1.5 + ['Infinity', 'invalid_type'], // was: silently clamped to 25 + [' ', 'invalid_type'], // was: `Number(' ')` → 0 → clamped to 1 + ])('?perObject=%j is refused as %s and no search runs', async (perObject, fieldCode) => { + const { answer, protocol } = await search({ perObject }); + expectRefusal(answer, 'perObject', fieldCode); + expect(protocol.searchAll).not.toHaveBeenCalled(); + }); + + it.each([ + ['5', 5], + // Range is the producer's business and reaches it unchanged. + ['0', 0], + ['50', 50], + ])('lit control: ?perObject=%j reaches searchAll as %s', async (perObject, expected) => { + const { answer, protocol } = await search({ perObject }); + expect(answer.status).toBe(200); + expect(protocol.searchAll.mock.calls[0][0].perObject).toBe(expected); + }); + + it.each([ + ['absent', {}], + // The old falsy guard read `?perObject=` as absent; it stays so. + ['empty', { perObject: '' }], + ])('%s perObject leaves the cap to the producer default (undefined)', async (_label, query) => { + const { answer, protocol } = await search(query); + expect(answer.status).toBe(200); + expect(protocol.searchAll.mock.calls[0][0].perObject).toBeUndefined(); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 5. GET /approvals/requests — no declared request schema; `limit` / `offset` +// must be whole numbers. The service's own [1, 200] clamp is untouched. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20139 — GET /approvals/requests refuses a `limit` / `offset` it cannot read', () => { + const list = async (query: Record) => { + const { drive, approvals } = boot(); + const answer = await drive('GET', APPROVALS, { query }); + return { answer, approvals }; + }; + + it.each([ + [{ limit: 'abc' }, 'limit'], // was: dropped → the unpaged 500-row window, no `total` + [{ limit: '1.5' }, 'limit'], // was: 1.5 handed to the engine + [{ limit: 'Infinity' }, 'limit'], // was: dropped → unpaged + [{ limit: '' }, 'limit'], // was: `Number('')` → 0 → a ONE-row page + [{ limit: ' ' }, 'limit'], // was: 0 → a one-row page + [{ offset: 'abc' }, 'offset'], // was: dropped → the first page + [{ offset: '1.5' }, 'offset'], // was: 1.5 handed to the engine + [{ offset: '' }, 'offset'], // was: 0 → paged mode (50 rows + total) the caller never asked for + [{ limit: '10', offset: 'abc' }, 'offset'], // was: page 1 served for "page N" + ])('%j is refused naming %s and no list is read', async (query, param) => { + const { answer, approvals } = await list(query); + expectRefusal(answer, param, 'invalid_type'); + expect(approvals.listRequests, 'no approval list may have been read').not.toHaveBeenCalled(); + expect(approvals.countRequests).not.toHaveBeenCalled(); + }); + + it.each([ + [{ limit: '50', offset: '0' }, 50, 0], + [{ limit: '25', offset: '50' }, 25, 50], + // No position on bounds: the service's own clamp decides these. + [{ limit: '0' }, 0, undefined], + [{ offset: '-1' }, undefined, -1], + ])('lit control: %j reaches listRequests as limit %s, offset %s', async (query, limit, offset) => { + const { answer, approvals } = await list(query); + expect(answer.status).toBe(200); + expect(approvals.listRequests).toHaveBeenCalledTimes(1); + const filter = approvals.listRequests.mock.calls[0][0]; + expect(filter.limit).toBe(limit); + expect(filter.offset).toBe(offset); + }); + + it('absent limit/offset keep the unpaged list (both undefined, no `total`)', async () => { + const { answer, approvals } = await list({}); + expect(answer.status).toBe(200); + const filter = approvals.listRequests.mock.calls[0][0]; + expect(filter.limit).toBeUndefined(); + expect(filter.offset).toBeUndefined(); + expect(answer.body).toEqual({ data: [{ id: 'req_1' }, { id: 'req_2' }] }); + }); +}); From 5ef104dd1d3960a882fda17a19b7b751d182af42 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 23:58:58 +0000 Subject: [PATCH 2/2] fix(rest)!: the remaining numeric query reads refuse what they cannot read, held by a census History sinceSeq and audit limit read through their declarations; diff from/to, search perObject and approvals limit/offset read as whole numbers, all through readDeclaredQueryNumber. A census test classifies every numeric coercion in rest-server.ts so a new bare Number() over a query value reddens its PR. Export page and rollback toVersion are the ledgered exemptions, each pinned. The publish route comment names 404 NO_DRAFT instead of the retired [no_draft] opener. Claude-Session: https://claude.ai/code/session_01UYBdGBzWSrAMzpW8ah3GbP Co-authored-by: Claude --- .changeset/20139-rest-query-number-census.md | 79 +++ .../rest-server-query-number-census.test.ts | 452 ++++++++++++++++++ .../rest-server-query-number-reads.test.ts | 2 +- packages/rest/src/rest-server.ts | 175 +++++-- 4 files changed, 661 insertions(+), 47 deletions(-) create mode 100644 .changeset/20139-rest-query-number-census.md create mode 100644 packages/rest/src/rest-server-query-number-census.test.ts diff --git a/.changeset/20139-rest-query-number-census.md b/.changeset/20139-rest-query-number-census.md new file mode 100644 index 00000000000..af02e22856e --- /dev/null +++ b/.changeset/20139-rest-query-number-census.md @@ -0,0 +1,79 @@ +--- +'@objectstack/rest': minor +--- + +fix(rest): the remaining numeric query reads refuse a value they cannot read with `400 VALIDATION_FAILED`, instead of dropping it or handing on `NaN` and answering `200` (#20139) + +Clause-②: no (narrowing) + +**BREAKING**: shipped as `minor` under the launch-window convention +(`check-changeset-no-major` refuses `major` until GA). The banner and the ADR-0087 +disposition below carry the breaking-ness, not the level. + +Five published doors still read a numeric query parameter with a bare `Number()`. +That coercion does not fail. It invents `NaN` or `0`, and the door dropped it or +served it with a `200`: +- `GET /meta/:type/:name/history?sinceSeq=abc` read the change log from the start; +- `GET /meta/:type/:name/audit?limit=abc` served the producer's default 100 events; +- `GET /meta/:type/:name/diff?from=abc` diffed a different pair of versions; +- `GET /search?perObject=abc` removed the per-object cap; +- `GET /approvals/requests?limit=abc` served the unpaged 500-row window instead of a page. + +Each door now reads the parameter the way `?limit=` on import jobs, export, history and +search already does: +- **`GET /meta/:type/:name/history`** reads `sinceSeq` through + `HistoryMetaItemRequestSchema.sinceSeq` (`z.number()`, so any finite number, as before). +- **`GET /meta/:type/:name/audit`** reads `limit` through + `AuditMetaItemRequestSchema.limit` (`z.number()`; the implementation's `[1, 500]` + clamp is unchanged). +- **`GET /meta/:type/:name/diff`** (`from` / `to`, and their `fromVersion` / + `toVersion` spellings), **`GET /search`** (`perObject`) and + **`GET /approvals/requests`** (`limit` / `offset`) declare no request schema. There the + value must be a whole number. Each service's own range handling is unchanged. + +A value the door cannot read answers `400` with the data surface's existing envelope, +`{ error, code: 'VALIDATION_FAILED', fields }`. `fields[0].field` names the parameter as +the caller spelled it, and `fields[0].code` is `invalid_type`. The service is never +called. On `GET /approvals/requests` this is a `400`, not the route's +`500 APPROVAL_REQUEST_LIST_FAILED`. + +What changes, per door (every row answered `200` before): + +| door | request | answered before | answers now | +|:--|:--|:--|:--| +| `GET /meta/:type/:name/history` | `?sinceSeq=abc`, `?sinceSeq=Infinity` | the change log from the start | `400`, `invalid_type` | +| `GET /meta/:type/:name/history` | `?sinceSeq=` (empty) | `sinceSeq: 0` applied as a cursor | `400`, `invalid_type` | +| `GET /meta/:type/:name/audit` | `?limit=abc`, `?limit=Infinity` | the default 100 events | `400`, `invalid_type` | +| `GET /meta/:type/:name/audit` | `?limit=` (empty) | one event | `400`, `invalid_type` | +| `GET /meta/:type/:name/diff` | `?from=abc`, `?from=Infinity` | the version before `to`, diffed instead | `400`, `invalid_type` | +| `GET /meta/:type/:name/diff` | `?to=abc` | the current body, diffed instead | `400`, `invalid_type` | +| `GET /meta/:type/:name/diff` | `?from=1.5`, `?to=2.5` | a diff against a version that cannot exist | `400`, `invalid_type` | +| `GET /search` | `?perObject=abc` | no per-object cap | `400`, `invalid_type` | +| `GET /search` | `?perObject=1.5`, `?perObject=Infinity` | a cap of 1.5 / clamped to 25 | `400`, `invalid_type` | +| `GET /approvals/requests` | `?limit=abc`, `?limit=Infinity` | the unpaged 500-row list, no `total` | `400`, `invalid_type` | +| `GET /approvals/requests` | `?limit=` (empty) | a one-row page | `400`, `invalid_type` | +| `GET /approvals/requests` | `?offset=abc` | the first page | `400`, `invalid_type` | +| `GET /approvals/requests` | `?offset=` (empty) | the service's 50-row paged mode | `400`, `invalid_type` | +| `GET /approvals/requests` | `?limit=1.5`, `?offset=1.5` | 1.5 handed to the engine | `400`, `invalid_type` | + +A blank value such as `?sinceSeq=%20` is refused on every one of these doors. + +**Unchanged:** +- An absent parameter keeps each door's default: the history log from the start, the + audit trail's 100 events, previous-vs-current on `/diff`, search's 5 per object, and the + unpaged approvals list. +- An empty `?perObject=`, `?from=` or `?to=` still means absent, as it always did there. +- Every conforming value reaches the service exactly as before, including the ranges no + card here takes a position on: history still forwards `sinceSeq=0` or `1.5`, audit still + forwards `limit=0` or `900` to its own clamp, `/diff` still forwards `from=0`, search + `perObject=50` is still clamped to 25, and approvals `limit=0` / `offset=-1` still reach + the service's own clamp. +- `GET /data/:object/export?page=` is unchanged. It sets only the export's chunk size, and + no value of it changes the rows exported. +- `POST /meta/:type/:name/rollback` `toVersion` is unchanged. It already refused an + unreadable value with `400 INVALID_REQUEST`. + +**Fix for a caller that now gets the `400`:** send the parameter as a number (a whole +number on `/diff`, search and approvals), or omit it to get the door's default. + + diff --git a/packages/rest/src/rest-server-query-number-census.test.ts b/packages/rest/src/rest-server-query-number-census.test.ts new file mode 100644 index 00000000000..778c2d8b7e5 --- /dev/null +++ b/packages/rest/src/rest-server-query-number-census.test.ts @@ -0,0 +1,452 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20139] The census that keeps the bare-`Number()` query-read family closed + * in `rest-server.ts`. + * + * ## The family + * + * A published door read a numeric query parameter with a bare `Number(...)`. + * That coercion does not fail: it INVENTS a value (`NaN`, `0`, a fallback) and + * the door answered `200` with a widened or substituted window. The family was + * closed one door at a time — four `?limit=` reads by `readDeclaredQueryNumber` + * (`rest-server-limit-param-parsing.test.ts`), the rest by this card + * (`rest-server-query-number-reads.test.ts`) — and a one-door-at-a-time close + * is exactly how it reopens: the next handler copies the idiom from a + * neighbour. This file makes that a red PR instead. + * + * ## What it counts — every numeric coercion, not "the ones that look like + * query reads" + * + * It parses the file with the TypeScript AST and finds EVERY numeric coercion: + * a call or `new` of `Number` / `parseInt` / `parseFloat` (bare or as + * `Number.parseInt` / `Number.parseFloat`), one of those passed or assigned as + * a value (`.map(Number)`), and unary `+`. Each site must be classified in + * {@link LEDGER} below as one of: + * + * - `reader` — the one faithful coercion inside `readDeclaredQueryNumber`; + * - `not-a-query-value` — the value comes from somewhere else (a stored row, + * stored metadata), and the reason names where; + * - `exempt` — a request query value deliberately left coerced, and the reason + * says why refusing it would be wrong. Each exemption is also PINNED below, + * so the reason is a measured fact rather than a sentence. + * + * Deciding "is this a query value?" by pattern — `Number(req.query.x)`, + * `Number(q.x)` — was tried against this card and fails on the one member the + * card itself missed: `/diff`'s `Number(raw)` sat in a local `parseV` helper, + * one frame away from the `req.query` it read, and no textual heuristic sees + * through that frame. A person classifying each site once, with a written + * reason, does. The cost is one ledger row per non-query coercion added to + * this file, which is rare (ten sites in the whole file today). + * + * Out of scope by construction: arithmetic that coerces as a side effect + * (`x * 1`, `x - 0`, `Math.trunc(x)`). None exists over a query value today; + * a new one is a review question, not a census row. + * + * ## Keys + * + * A site is keyed ` » `: the anchor is the route it sits + * in (`GET ${dataPath}/:object/export`, read from the registration's `method` + * and `path`) or else the named function or method around it. Line numbers are + * not used — this file is edited several times a day. Editing a ledgered + * coercion changes its key and reddens this file on purpose: an edited read + * is re-judged, not carried over. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; +// `.js` on purpose — NodeNext resolution requires the extension. +import { RestServer } from './rest-server.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const SOURCE_FILE = 'rest-server.ts'; +const SOURCE = readFileSync(resolve(HERE, SOURCE_FILE), 'utf8'); + +// ───────────────────────────────────────────────────────────────────────────── +// The walker +// ───────────────────────────────────────────────────────────────────────────── + +const COERCING_CALLEES: ReadonlySet = new Set([ + 'Number', 'parseInt', 'parseFloat', 'Number.parseInt', 'Number.parseFloat', +]); + +interface CoercionSite { + readonly key: string; + readonly line: number; +} + +const collapse = (text: string): string => text.replace(/\s+/g, ' ').trim(); +const unquote = (text: string): string => text.replace(/^[`'"]|[`'"]$/g, ''); + +function propertyInitializer(literal: ts.ObjectLiteralExpression, name: string): ts.Expression | undefined { + for (const property of literal.properties) { + if (ts.isPropertyAssignment(property) && property.name.getText() === name) return property.initializer; + } + return undefined; +} + +/** The route registration (`{ method, path, handler }`) or named function around a site. */ +function anchorOf(node: ts.Node, sf: ts.SourceFile): string { + for (let at: ts.Node | undefined = node.parent; at; at = at.parent) { + if (ts.isObjectLiteralExpression(at)) { + const method = propertyInitializer(at, 'method'); + const path = propertyInitializer(at, 'path'); + if (method && path && propertyInitializer(at, 'handler')) { + return `${unquote(method.getText(sf))} ${unquote(path.getText(sf))}`; + } + } + if ((ts.isFunctionDeclaration(at) || ts.isMethodDeclaration(at)) && at.name) { + return `${ts.isFunctionDeclaration(at) ? 'function' : 'method'} ${at.name.getText(sf)}`; + } + } + return 'module scope'; +} + +function isCoercion(node: ts.Node, sf: ts.SourceFile): boolean { + if ((ts.isCallExpression(node) || ts.isNewExpression(node)) + && COERCING_CALLEES.has(node.expression.getText(sf))) { + return true; + } + if (ts.isPrefixUnaryExpression(node) && node.operator === ts.SyntaxKind.PlusToken) return true; + // `Number` / `parseInt` / `parseFloat` handed on as a VALUE (`.map(Number)`, + // `const toN = Number`) — the callee position is the arm above, a member + // access (`Number.isFinite`) is not a coercion, and a type is not a value. + if (ts.isIdentifier(node) && COERCING_CALLEES.has(node.text)) { + const parent = node.parent; + if ((ts.isCallExpression(parent) || ts.isNewExpression(parent)) && parent.expression === node) return false; + if (ts.isPropertyAccessExpression(parent) || ts.isTypeReferenceNode(parent)) return false; + return true; + } + return false; +} + +function coercionSites(sourceText: string, fileName: string): CoercionSite[] { + const sf = ts.createSourceFile(fileName, sourceText, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const sites: CoercionSite[] = []; + const visit = (node: ts.Node): void => { + if (isCoercion(node, sf)) { + sites.push({ + key: `${anchorOf(node, sf)} » ${collapse(node.getText(sf))}`, + line: sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1, + }); + } + ts.forEachChild(node, visit); + }; + visit(sf); + return sites; +} + +// ───────────────────────────────────────────────────────────────────────────── +// The ledger — every numeric coercion in `rest-server.ts`, classified +// ───────────────────────────────────────────────────────────────────────────── + +type Disposition = 'reader' | 'not-a-query-value' | 'exempt'; + +interface LedgerRow { + readonly site: string; + readonly disposition: Disposition; + readonly reason: string; +} + +const LEDGER: readonly LedgerRow[] = [ + { + site: 'function readDeclaredQueryNumber » Number(raw)', + disposition: 'reader', + reason: 'The family\'s one faithful coercion: a non-blank string whose `Number()` is not `NaN` ' + + 'is parsed AS that number and everything else reaches the declared schema as it came, ' + + 'so the declaration — never this call — decides what is refused.', + }, + ...[ + 'Number(row?.total_rows ?? 0)', + 'Number(row?.processed_rows ?? 0)', + 'Number(row?.created_count ?? 0)', + 'Number(row?.updated_count ?? 0)', + 'Number(row?.skipped_count ?? 0)', + 'Number(row?.error_count ?? 0)', + ].map((text): LedgerRow => ({ + site: `function importJobToProgress » ${text}`, + disposition: 'not-a-query-value', + reason: 'A counter column of a persisted `sys_import_job` row, read back from the store and ' + + 'mapped to the ImportJobProgress DTO — no request value reaches it.', + })), + { + site: 'GET ${basePath}/forms/:slug/lookup/:field » Number(picker.maxResults)', + disposition: 'not-a-query-value', + reason: '`picker` is `fieldCfg.publicPicker`, the STORED form metadata\'s picker config — ' + + 'the author\'s declared result cap, not anything on the request. (The request\'s own ' + + '`?q=` is read as a string two lines below.)', + }, + { + site: 'POST ${metaPath}/:type/:name/rollback » Number(toVersionRaw)', + disposition: 'exempt', + reason: 'Read body-first (`body.toVersion ?? body.version ?? req.query.toVersion`) and then ' + + 'CHECKED, not served: a non-finite or below-1 result is already refused `400` before ' + + '`rollbackMetaItem` runs, so an unreadable value is never dropped or substituted — ' + + 'outside this family by its own definition. Its refusal is the door\'s own ' + + '`INVALID_REQUEST`; moving it onto `VALIDATION_FAILED` would be a wire change to a door ' + + 'that already refuses, which no card here decides. Pinned below.', + }, + { + site: 'GET ${dataPath}/:object/export » Number(q.page)', + disposition: 'exempt', + reason: '`page` sets only the CHUNK size of the export\'s own `findData` loop (clamped to ' + + '[50, 5000], default 500). An unreadable value (`abc`, empty, blank) is answered exactly ' + + 'as an absent one, and no chunk size adds, drops or reorders a streamed row — so no ' + + 'value of it widens or substitutes the answer, and refusing one would turn a correct ' + + 'export into a `400`. Pinned below.', + }, +]; + +// ───────────────────────────────────────────────────────────────────────────── +// 1. The walker can see every spelling it claims to count +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20139 census — the walker', () => { + it('finds every coercion spelling, and nothing that is not one', () => { + const probe = [ + 'function probe(req: any, q: any) {', + ' const a = Number(req.query.a);', + ' const b = parseInt(q.b, 10);', + ' const c = parseFloat(q.c);', + ' const d = +q.d;', + ' const e = [q.e].map(Number);', + ' const f = new Number(q.f);', + ' const g = Number.parseInt(q.g, 10);', + ' const h = Number.parseFloat(q.h);', + ' const toN = Number;', + // Not coercions: a member of `Number`, a type, arithmetic, a method named alike. + ' const ok = Number.isFinite(a) && Number.isInteger(b);', + ' let typed: Number | undefined;', + ' const sum = a + b;', + ' const other = q.parseInt(q.i);', + ' return [c, d, e, f, g, h, toN, ok, typed, sum, other];', + '}', + ].join('\n'); + expect(coercionSites(probe, 'probe.ts').map((s) => s.key)).toEqual([ + 'function probe » Number(req.query.a)', + 'function probe » parseInt(q.b, 10)', + 'function probe » parseFloat(q.c)', + 'function probe » +q.d', + 'function probe » Number', + 'function probe » new Number(q.f)', + 'function probe » Number.parseInt(q.g, 10)', + 'function probe » Number.parseFloat(q.h)', + 'function probe » Number', + ]); + }); + + it('anchors a site in a route handler to the route, however deep the helper that holds it', () => { + const probe = [ + 'class Probe {', + ' register(routes: any) {', + ' routes.register({', + " method: 'GET',", + ' path: `${base}/things/:id/diff`,', + ' handler: async (req: any) => {', + ' const parseV = (raw: any) => Number(raw);', + ' return parseV(req.query.from);', + ' },', + ' });', + ' }', + '}', + ].join('\n'); + expect(coercionSites(probe, 'probe.ts').map((s) => s.key)).toEqual([ + 'GET ${base}/things/:id/diff » Number(raw)', + ]); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 2. Every numeric coercion in rest-server.ts is routed or ledgered +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20139 census — every numeric coercion in rest-server.ts is classified', () => { + const sites = coercionSites(SOURCE, SOURCE_FILE); + const ledgered = new Map(LEDGER.map((row) => [row.site, row])); + + it('the ledger names each site once', () => { + expect(ledgered.size, 'a ledger row is duplicated').toBe(LEDGER.length); + }); + + it('no coercion is unclassified — a new bare `Number(req.query.x)` lands here', () => { + const unclassified = sites + .filter((site) => !ledgered.has(site.key)) + .map((site) => [ + `${SOURCE_FILE}:${site.line} — \`${site.key}\` is a numeric coercion the census has not classified.`, + ' A request query value: read it through `readDeclaredQueryNumber` against its declared', + ' schema (or `UNDECLARED_WHOLE_NUMBER_PARAM` where none is declared) — never a bare coercion,', + ' which invents `NaN` / `0` and answers 200.', + ' Anything else: add a LEDGER row in this file — `not-a-query-value` naming where the value', + ' comes from, or `exempt` with the reason refusing it would be wrong, pinned by a test.', + ].join('\n')); + expect(unclassified, `\n${unclassified.join('\n\n')}\n`).toEqual([]); + }); + + it('each site appears exactly once, and no ledger row outlives its site', () => { + const counts = new Map(); + for (const site of sites) counts.set(site.key, (counts.get(site.key) ?? 0) + 1); + const repeated = [...counts].filter(([key, n]) => n > 1 && ledgered.has(key)).map(([key, n]) => `${key} ×${n}`); + expect(repeated, 'a ledgered coercion was copied: classify the copy on its own').toEqual([]); + const stale = LEDGER.filter((row) => !counts.has(row.site)).map((row) => row.site); + expect(stale, 'these rows name no site any more — delete them (or re-judge the edited read)').toEqual([]); + }); + + it('the reader is the only `reader`, and every other row carries its reason', () => { + expect(LEDGER.filter((row) => row.disposition === 'reader').map((row) => row.site)) + .toEqual(['function readDeclaredQueryNumber » Number(raw)']); + for (const row of LEDGER) { + expect(row.reason.length, `${row.site} needs a reason a reviewer can check`).toBeGreaterThan(60); + } + }); + + it('positive control: the census sees the reader and both exemptions in the real file', () => { + const keys = new Set(sites.map((s) => s.key)); + for (const row of LEDGER.filter((r) => r.disposition !== 'not-a-query-value')) { + expect(keys.has(row.site), row.site).toBe(true); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 3. Each exemption's reason, measured on the real routes +// ───────────────────────────────────────────────────────────────────────────── + +function mockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; +} + +function mockRes() { + const res: any = { + statusCode: 200, + _headers: {} as Record, + _written: [] as string[], + json: vi.fn(function (this: any, body: any) { this._body = body; return this; }), + send: vi.fn(function (this: any) { return this; }), + write: vi.fn(function (this: any, chunk: any) { this._written.push(String(chunk)); return true; }), + end: vi.fn(function (this: any, chunk?: any) { if (chunk != null) this._written.push(String(chunk)); return this; }), + setHeader: vi.fn(function (this: any) { return this; }), + status: vi.fn(function (this: any, code: number) { this.statusCode = code; return this; }), + header: vi.fn(function (this: any, k: string, v: string) { this._headers[k] = v; return this; }), + }; + return res; +} + +/** 1200 rows, so every chunk size in play here takes more than one `findData` call. */ +const ROWS = Array.from({ length: 1200 }, (_, i) => ({ id: `r${i}`, n: i })); + +function boot() { + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaItem: vi.fn().mockResolvedValue({ + type: 'object', name: 'account', item: { name: 'account', fields: {} }, lock: 'none', + }), + findData: vi.fn(async ({ query }: any) => ({ + records: ROWS.slice(query.offset, query.offset + query.limit), + })), + rollbackMetaItem: vi.fn().mockResolvedValue({ + success: true, version: 'v2', seq: 9, restoredFromVersion: 3, + }), + }; + const rest = new RestServer( + mockServer() as any, + protocol as any, + { api: { requireAuth: false } } as any, + ); + (rest as any).resolveExecCtx = async () => ({ isSystem: true, userId: 'u1' }); + rest.registerRoutes(); + + const drive = async (method: string, path: string, req: Record = {}) => { + const found = (rest as any).getRoutes().find( + (r: any) => r.method === method && r.path === path, + ); + if (!found) throw new Error(`route not registered: ${method} ${path}`); + const res = mockRes(); + await found.handler( + { method, path, params: {}, query: {}, headers: {}, body: {}, ...req } as any, + res, + ); + return { + status: res.statusCode as number, + body: res.json.mock.calls.at(-1)?.[0], + streamed: (res._written as string[]).join(''), + }; + }; + + return { protocol, drive }; +} + +describe('#20139 census — exemption: export `?page=` changes chunking, never the rows', () => { + const exportWith = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('GET', '/api/v1/data/:object/export', { + params: { object: 'account' }, + query: { format: 'json', limit: String(ROWS.length), ...query }, + }); + const takes = protocol.findData.mock.calls.map((call: any[]) => call[0].query.limit); + return { ...answer, takes }; + }; + + it('streams the same rows, in the same order, whatever `page` holds', async () => { + const absent = await exportWith({}); + expect(absent.status).toBe(200); + expect(JSON.parse(absent.streamed)).toHaveLength(ROWS.length); + for (const page of ['abc', '', ' ', '1.5', 'Infinity', '-5', '50', '5000']) { + const answer = await exportWith({ page }); + expect(answer.status, `?page=${JSON.stringify(page)}`).toBe(200); + expect(answer.streamed, `?page=${JSON.stringify(page)} changed the exported rows`).toBe(absent.streamed); + } + }); + + it('an unreadable `page` is answered exactly as an absent one: the default 500-row chunk', async () => { + const absent = await exportWith({}); + expect(absent.takes).toEqual([500, 500, 200]); + for (const page of ['abc', '', ' ']) { + expect((await exportWith({ page })).takes, `?page=${JSON.stringify(page)}`).toEqual(absent.takes); + } + }); + + it('lit control: a readable `page` really is read — it sets the chunk, and only the chunk', async () => { + const fifty = await exportWith({ page: '50' }); + expect(fifty.takes).toHaveLength(ROWS.length / 50); + expect(new Set(fifty.takes)).toEqual(new Set([50])); + }); +}); + +describe('#20139 census — exemption: rollback `toVersion` is already refused, never substituted', () => { + const rollback = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('POST', '/api/v1/meta/:type/:name/rollback', { + params: { type: 'view', name: 'all_accounts' }, + query, + }); + return { answer, protocol }; + }; + + it.each([ + ['abc'], // `NaN` → refused + [''], // `Number('')` = 0 → below 1 → refused + ['Infinity'], // non-finite → refused + ])('?toVersion=%j answers 400 INVALID_REQUEST and nothing is restored', async (toVersion) => { + const { answer, protocol } = await rollback({ toVersion }); + expect(answer.status).toBe(400); + expect(answer.body?.code).toBe('INVALID_REQUEST'); + expect(protocol.rollbackMetaItem).not.toHaveBeenCalled(); + }); + + it('lit control: ?toVersion=3 reaches rollbackMetaItem as 3', async () => { + const { answer, protocol } = await rollback({ toVersion: '3' }); + expect(answer.status).toBe(200); + expect(protocol.rollbackMetaItem).toHaveBeenCalledTimes(1); + expect(protocol.rollbackMetaItem.mock.calls[0][0].toVersion).toBe(3); + }); +}); diff --git a/packages/rest/src/rest-server-query-number-reads.test.ts b/packages/rest/src/rest-server-query-number-reads.test.ts index 3b42f7d799c..a0f664ff56f 100644 --- a/packages/rest/src/rest-server-query-number-reads.test.ts +++ b/packages/rest/src/rest-server-query-number-reads.test.ts @@ -371,7 +371,7 @@ describe('#20139 — GET /approvals/requests refuses a `limit` / `offset` it can [{ limit: ' ' }, 'limit'], // was: 0 → a one-row page [{ offset: 'abc' }, 'offset'], // was: dropped → the first page [{ offset: '1.5' }, 'offset'], // was: 1.5 handed to the engine - [{ offset: '' }, 'offset'], // was: 0 → paged mode (50 rows + total) the caller never asked for + [{ offset: '' }, 'offset'], // was: 0 → the service's 50-row paged mode, never asked for [{ limit: '10', offset: 'abc' }, 'offset'], // was: page 1 served for "page N" ])('%j is refused naming %s and no list is read', async (query, param) => { const { answer, approvals } = await list(query); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index f27986fa886..9505bebcd42 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -103,9 +103,14 @@ import { } from '@objectstack/spec/api'; import { z } from 'zod'; import { DataProtocol, MetadataProtocol } from '@objectstack/spec/api'; -// [#20061 / #20062] The DECLARED request schemas two query-reading doors parse -// their numeric parameters through — see `readDeclaredQueryNumber` below. -import { ListImportJobsRequestSchema, HistoryMetaItemRequestSchema } from '@objectstack/spec/api'; +// [#20061 / #20062 / #20139] The DECLARED request schemas three query-reading +// doors parse their numeric parameters through — see `readDeclaredQueryNumber` +// below. +import { + ListImportJobsRequestSchema, + HistoryMetaItemRequestSchema, + AuditMetaItemRequestSchema, +} 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 // being smuggled past it with `as any` (see `TransportScopedMetaRequest`). @@ -670,16 +675,20 @@ export const GLOBAL_SEARCH_PARAMS: readonly string[] = [ ]; /** - * [#20062] The reading of a ROW-COUNT query parameter on a door whose request - * has no declared schema (`GET /data/:object/export`, `GET /search`): a whole - * number, and nothing about range. Range stays each door's own business — the - * export route's `Math.max(1, …)` floor and 50000 cap, `searchAll`'s `[1, 100]` - * clamp — because neither card this closes takes a position on bounds; it only - * refuses a value the door cannot read as a count at all. The same rule + * [#20062 / #20139] The reading of a numeric query parameter on a door whose + * request has no declared schema: a whole number, and nothing about range. + * Every such parameter in this file counts or addresses whole things — rows + * (`GET /data/:object/export` `limit`; `GET /search` `limit` / `perObject`; + * `GET /approvals/requests` `limit` / `offset`) or history versions + * (`GET /meta/:type/:name/diff` `from` / `to`). Range stays each door's own + * business — the export route's `Math.max(1, …)` floor and 50000 cap, + * `searchAll`'s `[1, 100]` / `[1, 25]` clamps, the approvals service's + * `[1, 200]` — because no card this closes takes a position on bounds; it only + * refuses a value the door cannot read as a whole number at all. The same rule * `@objectstack/runtime`'s `parseIntegerParam` applies without `bounds`, which * this package cannot import (runtime depends on rest). */ -const UNDECLARED_ROW_COUNT_PARAM = z.number().int().optional(); +const UNDECLARED_WHOLE_NUMBER_PARAM = z.number().int().optional(); /** * [#20061 / #20062] Read ONE numeric query parameter against the door's own @@ -721,17 +730,28 @@ const UNDECLARED_ROW_COUNT_PARAM = z.number().int().optional(); * ## The refusal * * THROWN as `validationFailure` (`@objectstack/types`), never written here: - * every door that calls this already sends its catch through - * `handleRouteError` / `mapDataError`, which answer `400` with the data - * surface's `VALIDATION_FAILED` + `fields[]` envelope — the same shape the - * declared-schema body doors in this file answer. `fields[].code` comes from - * `zodIssuesToFields`, so it is the ADR-0114 D3 catalog member for the failed - * constraint (`invalid_type`, `min_value`, `max_value`), with `field` naming - * the parameter. + * every door that calls this sends the throw through `handleRouteError` / + * `mapDataError`, which answer `400` with the data surface's + * `VALIDATION_FAILED` + `fields[]` envelope — the same shape the + * declared-schema body doors in this file answer. A door whose own catch maps + * something else (`GET /approvals/requests` answers every throw + * `500 APPROVAL_REQUEST_LIST_FAILED`) catches the read itself and hands it to + * `handleRouteError`. `fields[].code` comes from `zodIssuesToFields`, so it is + * the ADR-0114 D3 catalog member for the failed constraint (`invalid_type`, + * `min_value`, `max_value`), with `field` naming the parameter. * * Call it AFTER `refuseRepeatedQueryParams` has run for `param`: that gate * refuses a repeated occurrence and unwraps a one-element array, so what * reaches this function is a single string or nothing. + * + * ## The census that keeps the family closed + * + * [#20139] `rest-server-query-number-census.test.ts` finds every numeric + * coercion in this file (`Number(…)`, `parseInt` / `parseFloat`, unary `+`) + * and fails on any it has not classified: this function's own `Number(raw)`, a + * value that is not a request query value, or a ledgered exemption with its + * reason. A new bare `Number(req.query.x)` therefore reddens its PR instead of + * reopening the family one door at a time. */ function readDeclaredQueryNumber( queryParams: Record | undefined, @@ -7485,9 +7505,16 @@ export class RestServer { // `?limit=` silently returned the UNLIMITED history instead // of the page the caller asked for. if (refuseRepeatedQueryParams(req, res, ['sinceSeq', 'limit'])) return; - const sinceSeq = req.query?.sinceSeq !== undefined - ? Number(req.query.sinceSeq) - : undefined; + // [#20139] `sinceSeq` is parsed through its DECLARATION + // (`HistoryMetaItemRequestSchema.sinceSeq`, `z.number().optional()`), + // the way `limit` is just below: `?sinceSeq=abc` used to be `NaN`, + // dropped by the spread below, and answered with the log read from + // the START; `?sinceSeq=` became `Number('')` = 0, a cursor the + // repository applies (`event_seq <= 0` rows skipped) that the + // caller never sent. Both are refused now; any finite number is + // still forwarded exactly as before. + const sinceSeq = readDeclaredQueryNumber(req.query, 'sinceSeq', + HistoryMetaItemRequestSchema.shape.sinceSeq, { emptyIsAbsent: false }); // [#20062] `limit` is parsed through its DECLARATION // (`HistoryMetaItemRequestSchema.limit`, `z.number().optional()`), // not coerced: `?limit=abc` used to be `NaN`, dropped by the @@ -7592,9 +7619,9 @@ export class RestServer { name: req.params.name, ...(environmentId ? { environmentId } : {}), ...(historyOrganizationId ? { organizationId: historyOrganizationId } : {}), - ...(sinceSeq !== undefined && Number.isFinite(sinceSeq) ? { sinceSeq } : {}), - // Already finite or absent — the declared parse above refuses - // anything else, so no `Number.isFinite` drop is left here. + // Both already finite or absent — the declared parses above + // refuse anything else, so no `Number.isFinite` drop is left here. + ...(sinceSeq !== undefined ? { sinceSeq } : {}), ...(limit !== undefined ? { limit } : {}), }; const result = await p.historyMetaItem(historyRequest); @@ -7677,9 +7704,16 @@ export class RestServer { // [#6877] Same `Number(...)` → `NaN` → dropped-limit shape as // the history twin above. if (refuseRepeatedQueryParams(req, res, ['limit'])) return; - const limit = req.query?.limit !== undefined - ? Number(req.query.limit) - : undefined; + // [#20139] Parsed through its DECLARATION + // (`AuditMetaItemRequestSchema.limit`, `z.number().optional()`), + // the history twin's reading: `?limit=abc` used to be `NaN`, + // dropped by the spread below, and answered with the producer's + // default 100 events; `?limit=` became `Number('')` = 0, which the + // implementation clamps to ONE event. Both are refused now; any + // finite number is still forwarded, and the implementation's own + // `[1, 500]` clamp is unchanged. + const limit = readDeclaredQueryNumber(req.query, 'limit', + AuditMetaItemRequestSchema.shape.limit, { emptyIsAbsent: false }); // [#20156] The history twin's gate, for the same reason: the // audit trail of an item the plain read refuses this caller // is refused with the plain read's answer. See @@ -7736,7 +7770,9 @@ export class RestServer { type: req.params.type, name: req.params.name, organizationId: ctx?.tenantId ?? null, - ...(limit !== undefined && Number.isFinite(limit) ? { limit } : {}), + // Already finite or absent — the declared parse above + // refuses anything else. + ...(limit !== undefined ? { limit } : {}), }; const result = await p.auditMetaItem(auditRequest); res.json(result); @@ -7751,7 +7787,7 @@ export class RestServer { }); // POST /meta/:type/:name/publish — promote the pending draft - // overlay to live. 404 [no_draft] when nothing to publish. + // overlay to live. 404 `NO_DRAFT` when nothing to publish. registerPerItemRoute({ method: 'POST', path: `${metaPath}/:type/:name/publish`, @@ -8084,18 +8120,28 @@ export class RestServer { }); return; } - const parseV = (raw: any): number | undefined => { - if (raw === undefined || raw === null || raw === '') return undefined; - const n = Number(raw); - return Number.isFinite(n) ? n : undefined; - }; - // [#6877] `parseV` returns `undefined` for `NaN`, and the - // spreads below then omit the bound entirely — so a repeated - // `?from=` quietly diffed a different pair of versions and - // answered 200. + // [#6877] A repeated `?from=` became `NaN`, the spreads below + // omitted the bound, and the door quietly diffed a different + // pair of versions and answered 200. if (refuseRepeatedQueryParams(req, res, ['from', 'fromVersion', 'to', 'toVersion'])) return; - const fromVersion = parseV(req.query?.from ?? req.query?.fromVersion); - const toVersion = parseV(req.query?.to ?? req.query?.toVersion); + // [#20139] The same drop for a SINGLE unreadable value: the + // `parseV` helper that stood here answered `undefined` for + // `?from=abc` (and `Infinity`), so `diffMetaItem` substituted + // "the version before `to`" — or, for `?to=abc`, the CURRENT + // body — and the door answered 200 with a comparison nobody + // asked for; `?from=1.5` was forwarded and diffed against a + // version that cannot exist. No request schema is declared for + // this door, so a version reads as a whole number. The name the + // caller used is the one read (and the one a refusal names): + // `from` / `to` win over `fromVersion` / `toVersion` exactly as + // the old `??` had it, and an empty value stays absent, as + // `parseV('')` already answered. + const fromParam = req.query?.from != null ? 'from' : 'fromVersion'; + const toParam = req.query?.to != null ? 'to' : 'toVersion'; + const fromVersion = readDeclaredQueryNumber(req.query, fromParam, + UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: true }); + const toVersion = readDeclaredQueryNumber(req.query, toParam, + UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: true }); // [#20156] A diff discloses BOTH versions' values — for a // doc its `content`, for an app its `navigation`, for an // object its `fields` — so it owes the plain read's @@ -9810,9 +9856,17 @@ export class RestServer { // the reading is "a whole number"; the floor and the cap below // are this door's own and are unchanged. const limitParam = readDeclaredQueryNumber(q, 'limit', - UNDECLARED_ROW_COUNT_PARAM, { emptyIsAbsent: false }); + UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: false }); const requestedLimit = limitParam !== undefined ? Math.max(1, limitParam) : 10_000; const limit = Math.min(requestedLimit, HARD_CAP); + // [#20139] Deliberately NOT read through `readDeclaredQueryNumber` + // — the one ledgered exemption of the family's census + // (`rest-server-query-number-census.test.ts`, which also pins + // why). `page` sets only the CHUNK size of the `findData` loop + // below: whatever it holds, readable or not, the export streams + // the same rows in the same order, so no value of it can widen or + // substitute the answer, and refusing one would turn a correct + // export into a `400`. const chunkSize = Math.min(MAX_CHUNK, Math.max(50, q.page != null ? Number(q.page) || 500 : 500)); // Colour cells only for xlsx within the style cap; decided up // front (before streaming) since we can't know the true row @@ -10220,12 +10274,19 @@ export class RestServer { // `searchAll`'s own `[1, 100]` clamp is unchanged, and an empty // `?limit=` stays absent as the old falsy guard had it. const limit = readDeclaredQueryNumber(req.query, 'limit', - UNDECLARED_ROW_COUNT_PARAM, { emptyIsAbsent: true }); + UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: true }); + // [#20139] `perObject`, the same way: `?perObject=abc` reached + // `searchAll` as `NaN`, its `Math.max(1, Math.min(25, NaN))` is + // `NaN`, and the per-object cap was silently gone. Whole number; + // the `[1, 25]` clamp stays `searchAll`'s own, and an empty + // `?perObject=` stays absent as the old falsy guard had it. + const perObject = readDeclaredQueryNumber(req.query, 'perObject', + UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: true }); const result = await searchAll.call(p, { q, objects, limit, - perObject: req.query?.perObject ? Number(req.query.perObject) : undefined, + perObject, ...(context ? { context } : {}), }); res.json(result); @@ -12777,8 +12838,30 @@ export class RestServer { .flatMap((s: any) => String(s).split(',')) .map((s: string) => s.trim()) .filter(Boolean); - const limit = q.limit != null ? Number(q.limit) : undefined; - const offset = q.offset != null ? Number(q.offset) : undefined; + // [#20139] Read, not coerced. `?limit=abc` (or `Infinity`) was + // `NaN`, dropped by an `isFinite` guard, and answered with the + // UNPAGED 500-row window and no `total` — a caller asking for a + // page got a different shape of answer; `?offset=abc` was dropped + // to the first page. `?limit=` / `?offset=` became `Number('')` = + // 0: a ONE-row page, or the service's 50-row paged mode the caller + // never asked for — so empty is refused here, not read as absent. + // No request schema is declared for this door, so both read as + // whole numbers; the service's own `[1, 200]` clamp is unchanged. + // + // Caught HERE, not by the catch below: that one answers every + // throw `500 APPROVAL_REQUEST_LIST_FAILED`, and a query value the + // door cannot read is the caller's `400`. + let limit: number | undefined; + let offset: number | undefined; + try { + limit = readDeclaredQueryNumber(q, 'limit', + UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: false }); + offset = readDeclaredQueryNumber(q, 'offset', + UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: false }); + } catch (refusal: any) { + handleRouteError(res, refusal); + return; + } const listFilter = { object: q.object, recordId: q.recordId ?? q.record_id, @@ -12786,8 +12869,8 @@ export class RestServer { approverId: approverIds.length ? approverIds : undefined, submitterId: q.submitterId ?? q.submitter_id, q: typeof q.q === 'string' ? q.q : undefined, - limit: Number.isFinite(limit) ? limit : undefined, - offset: Number.isFinite(offset) ? offset : undefined, + limit, + offset, }; const rows = await svc.listRequests(listFilter, context ?? {}); // `total` only when the caller pages — counting costs a