From b74225280d1aee356390cfb37e4ac52c84786768 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 07:41:03 +0000 Subject: [PATCH 1/3] fix(rest): refuse a ?limit= the door cannot read instead of substituting it GET /data/import/jobs, GET /data/:object/export, GET /meta/:type/:name/history and GET /search read ?limit= with a bare Number() and answered 200 with a substituted, clamped or dropped window. Each now reads the parameter through its declared schema (ListImportJobsRequestSchema, HistoryMetaItemRequestSchema) or, where the door declares none, as a whole number, and refuses anything else with 400 VALIDATION_FAILED + fields[] via the shared error mapper. Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn Co-authored-by: Claude --- .../rest-server-limit-param-parsing.test.ts | 331 ++++++++++++++++++ packages/rest/src/rest-server.ts | 144 +++++++- 2 files changed, 466 insertions(+), 9 deletions(-) create mode 100644 packages/rest/src/rest-server-limit-param-parsing.test.ts diff --git a/packages/rest/src/rest-server-limit-param-parsing.test.ts b/packages/rest/src/rest-server-limit-param-parsing.test.ts new file mode 100644 index 00000000000..93d07bc0632 --- /dev/null +++ b/packages/rest/src/rest-server-limit-param-parsing.test.ts @@ -0,0 +1,331 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20061 / #20062] A `?limit=` a door cannot read is REFUSED — never + * substituted, clamped into range, or dropped. + * + * Four published doors read `?limit=` with a bare `Number(...)` and answered + * `200` whatever came back: + * + * | door | unfixed read | what a bad value did | + * | :---------------------------- | :---------------------------------------------- | :-------------------------------- | + * | `GET /data/import/jobs` | `Math.min(200, Math.max(1, Number(q.limit) \|\| 50))` | `0` → 50 rows, `500` → 200 rows | + * | `GET /data/:object/export` | `Math.max(1, Number(q.limit) \|\| 0)` | `abc` / empty → a ONE-row export | + * | `GET /meta/:type/:name/history` | `Number(q.limit)`, dropped unless finite | `abc` → the WHOLE change log | + * | `GET /search` | `q.limit ? Number(q.limit) : undefined` | `abc` → `NaN`, so no overall cap | + * + * Each door now reads the parameter against its own DECLARATION and answers a + * value outside it with the data surface's `400 VALIDATION_FAILED` + `fields[]` + * envelope. `fields[].code` is the ADR-0114 catalog member the declared schema's + * failure maps to (`invalid_type`, `min_value`, `max_value`). + * + * ## 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 preservation case (the lit control) pins the ARGUMENT the + * service received for a conforming value, so a fix that refused everything + * could not pass either. + * + * ## What is deliberately NOT asserted + * + * The bounds of the three doors whose request declares none (export, search, + * history) — both cards take no position on them. So `?limit=0` still reaches + * export's own `Math.max(1, …)` and search's `[1, 100]` clamp exactly as it did + * (the preservation rows below pin those answers UNCHANGED), and history still + * forwards whatever finite number its declared `z.number()` admits. + */ + +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 DATA = '/api/v1/data'; + +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. `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 + * protocol call the preservation cases inspect. + */ +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', + }), + historyMetaItem: vi.fn().mockResolvedValue({ events: [] }), + searchAll: vi.fn().mockResolvedValue({ + query: 'acme', hits: [], pages: [], totalObjects: 0, totalHits: 0, truncated: false, + }), + findData: vi.fn().mockResolvedValue({ records: [] }), + }; + + 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 route = (method: string, path: string) => { + 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}`); + return found; + }; + + const drive = async (method: string, path: string, req: Record = {}) => { + const res = mockRes(); + await route(method, path).handler( + { method, path, params: {}, query: {}, headers: {}, body: {}, ...req } as any, + res, + ); + return { + status: res.statusCode, + body: res.json.mock.calls.at(-1)?.[0], + headers: res._headers as Record, + }; + }; + + return { protocol, drive }; +} + +type Answer = { status: number; body: any }; + +/** The refusal half: status, top-level code, and the named field with its catalog code. */ +function expectLimitRefusal(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); +} + +// ───────────────────────────────────────────────────────────────────────────── +// 1. GET /data/import/jobs — `ListImportJobsRequestSchema`: +// limit `int().min(1).max(200).default(50)`, offset `int().min(0).default(0)` +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20061 — GET /data/import/jobs holds its declared `limit` / `offset` bounds', () => { + const jobs = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('GET', `${DATA}/import/jobs`, { query }); + return { answer, protocol }; + }; + + it.each([ + ['0', 'min_value'], // was: `0 || 50` → the default 50 rows + ['-3', 'min_value'], // was: clamped up to 1 + ['201', 'max_value'], // was: clamped down to 200 + ['500', 'max_value'], // was: clamped down to 200 + ['abc', 'invalid_type'], // was: `NaN || 50` → 50 rows + ['1.5', 'invalid_type'], // was: handed to the engine as 1.5 + ['Infinity', 'invalid_type'], // was: clamped down to 200 + [' ', 'invalid_type'], // was: `0 || 50` → 50 rows + ])('?limit=%j is refused as %s and no query runs', async (limit, fieldCode) => { + const { answer, protocol } = await jobs({ limit }); + expectLimitRefusal(answer, 'limit', fieldCode); + expect(protocol.findData, 'no import-job query may have been issued').not.toHaveBeenCalled(); + }); + + it.each([ + ['-1', 'min_value'], // was: clamped up to 0 + ['abc', 'invalid_type'], // was: `NaN || 0` → 0 + ['1.5', 'invalid_type'], // was: handed to the engine as 1.5 + ])('?offset=%j is refused as %s and no query runs', async (offset, fieldCode) => { + const { answer, protocol } = await jobs({ offset }); + expectLimitRefusal(answer, 'offset', fieldCode); + expect(protocol.findData).not.toHaveBeenCalled(); + }); + + it.each([ + [{ limit: '1' }, 1, 0], + [{ limit: '200' }, 200, 0], + [{ limit: '25', offset: '0' }, 25, 0], + [{ limit: '25', offset: '40' }, 25, 40], + ])('lit control: %j reaches the engine as limit %i, offset %i', async (query, limit, offset) => { + const { answer, protocol } = await jobs(query); + expect(answer.status).toBe(200); + expect(protocol.findData).toHaveBeenCalledTimes(1); + expect(protocol.findData.mock.calls[0][0].query).toMatchObject({ limit, offset }); + }); + + it.each([ + ['absent', {}], + ['empty', { limit: '', offset: '' }], + ])('%s limit/offset keep the declared defaults (50, 0)', async (_label, query) => { + const { answer, protocol } = await jobs(query); + expect(answer.status).toBe(200); + expect(protocol.findData.mock.calls[0][0].query).toMatchObject({ limit: 50, offset: 0 }); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 2. GET /data/:object/export — no declared request schema; a row count must +// be a whole number. Range stays the door's own clamp, untouched. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20062 — GET /data/:object/export refuses a `limit` it cannot read', () => { + const exportRows = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('GET', `${DATA}/:object/export`, { + params: { object: 'account' }, + query: { format: 'csv', ...query }, + }); + return { answer, protocol }; + }; + + it.each([ + ['abc', 'invalid_type'], // was: `NaN || 0` → a ONE-row export + ['', 'invalid_type'], // was: `Number('') || 0` → a ONE-row export + [' ', 'invalid_type'], // was: a ONE-row export + ['1.5', 'invalid_type'], // was: `X-Export-Limit: 1.5` + ['Infinity', 'invalid_type'], // was: silently capped to 50000 + ])('?limit=%j is refused as %s and no export query runs', async (limit, fieldCode) => { + const { answer, protocol } = await exportRows({ limit }); + expectLimitRefusal(answer, 'limit', fieldCode); + expect(protocol.findData, 'no export query may have been issued').not.toHaveBeenCalled(); + }); + + it.each([ + ['25', '25'], + // The door's own range handling is out of this card's scope and must + // come through UNCHANGED: a floor of 1 and the 50000 hard cap. + ['0', '1'], + ['60000', '50000'], + ])('lit control: ?limit=%j exports with X-Export-Limit %s', async (limit, effective) => { + const { answer, protocol } = await exportRows({ limit }); + expect(answer.status).toBe(200); + expect(answer.headers['X-Export-Limit']).toBe(effective); + expect(protocol.findData).toHaveBeenCalled(); + }); + + it('an absent limit keeps the 10000 default', async () => { + const { answer } = await exportRows({}); + expect(answer.status).toBe(200); + expect(answer.headers['X-Export-Limit']).toBe('10000'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 3. GET /meta/:type/:name/history — `HistoryMetaItemRequestSchema.limit` is +// `z.number().optional()`: any finite number, no int, no bounds. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20062 — GET /meta/:type/:name/history refuses what its declaration refuses', () => { + const history = async (query: Record) => { + const { drive, protocol } = boot(); + const answer = await drive('GET', `${META}/:type/:name/history`, { + params: { type: 'view', name: 'all_accounts' }, + query, + }); + return { answer, protocol }; + }; + + it.each([ + ['abc', 'invalid_type'], // was: NaN dropped → the WHOLE change log + ['Infinity', 'invalid_type'], // was: dropped → the whole change log + ['', 'invalid_type'], // was: `Number('')` → 0 → zero events + [' ', 'invalid_type'], // was: 0 → zero events + ])('?limit=%j is refused as %s and the log is never read', async (limit, fieldCode) => { + const { answer, protocol } = await history({ limit }); + expectLimitRefusal(answer, 'limit', fieldCode); + expect(protocol.historyMetaItem).not.toHaveBeenCalled(); + }); + + it.each([ + ['5', 5], + // Declared `z.number()` admits these; the door forwards them exactly as + // before rather than inventing bounds the declaration does not carry. + ['0', 0], + ['1.5', 1.5], + ])('lit control: ?limit=%j reaches historyMetaItem as %s', async (limit, expected) => { + const { answer, protocol } = await history({ limit }); + expect(answer.status).toBe(200); + expect(protocol.historyMetaItem).toHaveBeenCalledTimes(1); + expect(protocol.historyMetaItem.mock.calls[0][0].limit).toBe(expected); + }); + + it('an absent limit still reads the full log (no limit member at all)', async () => { + const { answer, protocol } = await history({}); + expect(answer.status).toBe(200); + expect('limit' in protocol.historyMetaItem.mock.calls[0][0]).toBe(false); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 4. GET /search — no declared request schema; a result cap must be a whole +// number. The producer's own [1, 100] clamp is untouched. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#20062 — GET /search refuses a `limit` 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 overall cap never triggered + ['1.5', 'invalid_type'], // was: an overall cap of 1.5 (two hits) + ['Infinity', 'invalid_type'], // was: silently clamped to 100 + [' ', 'invalid_type'], // was: 0 → clamped to 1 + ])('?limit=%j is refused as %s and no search runs', async (limit, fieldCode) => { + const { answer, protocol } = await search({ limit }); + expectLimitRefusal(answer, 'limit', fieldCode); + expect(protocol.searchAll).not.toHaveBeenCalled(); + }); + + it.each([ + ['20', 20], + // Range is the producer's business and reaches it unchanged. + ['0', 0], + ['500', 500], + ])('lit control: ?limit=%j reaches searchAll as %s', async (limit, expected) => { + const { answer, protocol } = await search({ limit }); + expect(answer.status).toBe(200); + expect(protocol.searchAll.mock.calls[0][0].limit).toBe(expected); + }); + + it.each([ + ['absent', {}], + ['empty', { limit: '' }], + ])('%s limit 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].limit).toBeUndefined(); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 5004bb98b64..d9ccb884922 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -31,6 +31,9 @@ import { // `@objectstack/types` because the producer is a PLUGIN and rest cannot // import one. strandedDecisionDetails, + // [#20061] The thrown `VALIDATION_FAILED` + `fields[]` shape every catch in + // this file already maps to `400` — see `readDeclaredQueryNumber` below. + validationFailure, } from '@objectstack/types'; import { allowPerfDisclosure, @@ -97,8 +100,11 @@ import { BatchEndpointsConfigSchema, RouteGenerationConfigSchema, } from '@objectstack/spec/api'; -import type { z } from 'zod'; +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'; // [#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`). @@ -647,6 +653,96 @@ export const GLOBAL_SEARCH_PARAMS: readonly string[] = [ 'q', 'query', 'objects', 'limit', 'perObject', ]; +/** + * [#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 + * `@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(); + +/** + * [#20061 / #20062] Read ONE numeric query parameter against the door's own + * declared schema for it, and refuse — never substitute — what that schema + * refuses. + * + * The defect this closes is the bare coercion: `Number(q.limit)` does not + * fail, it INVENTS a value and the door serves it. Measured on four published + * doors, each answering `200`: `GET /data/import/jobs` turned `?limit=0` into + * its 50-row default and clamped `?limit=500` to 200 against a declaration of + * `min(1).max(200)`; `GET /data/:object/export` turned `?limit=abc` into a + * ONE-row export; `GET /meta/:type/:name/history` dropped it and returned the + * whole change log; `GET /search` handed `NaN` to `searchAll`, whose overall + * cap then never triggered. + * + * ## How a query string meets a `z.number()` declaration + * + * A query string carries no types, so the declared schema cannot parse it + * directly — and `Number()` alone is the defect. The one coercion made here is + * the faithful one: a non-blank string whose `Number()` is not `NaN` is parsed + * AS that number (`'50'` → 50, `'1.5'` → 1.5, `'Infinity'` → Infinity), and + * everything else — `'abc'`, a blank string, a structured value — is handed to + * the schema AS IT CAME, so the declaration refuses it by type rather than + * after `Number()` has already invented a `0` or a `NaN` for it. The schema, + * not this function, decides what is legal: `int()`, `min()` / `max()` and + * zod's own refusal of non-finite numbers all apply exactly as declared, and an + * absent parameter meets the schema's own `.default()` / `.optional()`. + * + * ## The empty string — the one per-door judgement + * + * `?limit=` is present-but-empty, and what it means is decided from what the + * door answered for it before, exactly as `parseEnumParam` (runtime + * `query-param.ts`) decides the same spelling: where the old answer already + * WAS the absent answer (import jobs' `Number('') || 50`, search's falsy + * guard), it stays absent (`emptyIsAbsent: true`) so a defensible answer does + * not become a new `400`; where the old answer was an invented `0` — a + * one-row export, a zero-event history — refusing it strictly improves on it. + * + * ## 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. + * + * 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. + */ +function readDeclaredQueryNumber( + query: Record | undefined, + param: string, + declared: z.ZodType, + opts: { readonly emptyIsAbsent: boolean }, +): number | undefined { + const raw = query?.[param]; + let input: unknown = raw; + if (raw === undefined || raw === null || (raw === '' && opts.emptyIsAbsent)) { + input = undefined; + } else if (typeof raw === 'string' && raw.trim() !== '') { + const coerced = Number(raw); + if (!Number.isNaN(coerced)) input = coerced; + } + const parsed = declared.safeParse(input); + if (parsed.success) return parsed.data; + const fields = zodIssuesToFields( + parsed.error.issues.map((issue) => ({ ...issue, path: [param, ...issue.path] })), + { [param]: input }, + ); + throw validationFailure( + `Invalid \`${param}\` query parameter: ${fields[0]?.message ?? 'not a readable number'}`, + fields, + ); +} + /** * [#16674] Which `services.*` slot each `routes.*` key is the address OF. * @@ -7706,9 +7802,15 @@ export class RestServer { const sinceSeq = req.query?.sinceSeq !== undefined ? Number(req.query.sinceSeq) : undefined; - const limit = req.query?.limit !== undefined - ? Number(req.query.limit) - : undefined; + // [#20062] `limit` is parsed through its DECLARATION + // (`HistoryMetaItemRequestSchema.limit`, `z.number().optional()`), + // not coerced: `?limit=abc` used to be `NaN`, dropped by the + // spread below, and answered with the WHOLE change log; `?limit=` + // became `Number('')` = 0, a zero-event answer. Both are refused + // now. The declaration carries no `int()` and no bounds, so any + // finite number is still forwarded exactly as before. + const limit = readDeclaredQueryNumber(req.query, 'limit', + HistoryMetaItemRequestSchema.shape.limit, { emptyIsAbsent: false }); // [#13406] STATE THE ORG PARTITION. `sys_metadata_history` // is a per-org log — `SysMetadataRepository.history()` // filters `organization_id = this.organizationId` by strict @@ -7792,7 +7894,9 @@ export class RestServer { ...(environmentId ? { environmentId } : {}), ...(historyOrganizationId ? { organizationId: historyOrganizationId } : {}), ...(sinceSeq !== undefined && Number.isFinite(sinceSeq) ? { sinceSeq } : {}), - ...(limit !== undefined && Number.isFinite(limit) ? { limit } : {}), + // Already finite or absent — the declared parse above refuses + // anything else, so no `Number.isFinite` drop is left here. + ...(limit !== undefined ? { limit } : {}), }; const result = await p.historyMetaItem(historyRequest); res.json(result); @@ -9711,8 +9815,15 @@ export class RestServer { const filter: Record = {}; if (typeof q.object === 'string' && q.object) filter.object_name = q.object; if (typeof q.status === 'string' && q.status) filter.status = q.status; - const limit = Math.min(200, Math.max(1, Number(q.limit) || 50)); - const offset = Math.max(0, Number(q.offset) || 0); + // [#20061] Parsed through the DECLARED `ListImportJobsRequestSchema` + // (limit `int().min(1).max(200).default(50)`, offset + // `int().min(0).default(0)`) rather than clamped: `?limit=0` used to + // answer the 50-row default and `?limit=500` 200 rows, both `200`. + // Absent and empty keep the declared defaults, as they always did. + const limit = readDeclaredQueryNumber(q, 'limit', + ListImportJobsRequestSchema.shape.limit, { emptyIsAbsent: true }); + const offset = readDeclaredQueryNumber(q, 'offset', + ListImportJobsRequestSchema.shape.offset, { emptyIsAbsent: true }); const jobsListRequest: ServerScopedDataRequest = { object: IMPORT_JOB_OBJECT, // [#16337] Canonical QueryAST, not the wire dialect this @@ -9838,7 +9949,14 @@ export class RestServer { // heavier than a bare value dump, so cap it well below HARD_CAP; // above this the export still succeeds, just without colours. const STYLE_ROW_CAP = 10_000; - const requestedLimit = q.limit != null ? Math.max(1, Number(q.limit) || 0) : 10_000; + // [#20062] Read, not coerced: `?limit=abc` (and `?limit=`) used to + // become `Number(…) || 0` → `Math.max(1, 0)` → a ONE-row export + // answered `200`. No request schema is declared for this door, so + // 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 }); + const requestedLimit = limitParam !== undefined ? Math.max(1, limitParam) : 10_000; const limit = Math.min(requestedLimit, HARD_CAP); 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 @@ -10240,10 +10358,18 @@ export class RestServer { const objects = typeof objectsParam === 'string' ? objectsParam.split(',').map((s: string) => s.trim()).filter(Boolean) : Array.isArray(objectsParam) ? objectsParam : undefined; + // [#20062] Read, not coerced: `?limit=abc` used to reach + // `searchAll` as `NaN`, and `hits.length >= NaN` is never true, + // so the overall cap was silently gone. No request schema is + // declared for this door, so the reading is "a whole number"; + // `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 }); const result = await searchAll.call(p, { q, objects, - limit: req.query?.limit ? Number(req.query.limit) : undefined, + limit, perObject: req.query?.perObject ? Number(req.query.perObject) : undefined, ...(context ? { context } : {}), }); From 3d07f062998d395f13a575a6ce382d7fc31d9a6a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 07:46:46 +0000 Subject: [PATCH 2/3] fix(rest): give the search closed-set fixture a readable limit; rename the reader's bag parameter The closed-set acceptance loop for GET /search sent limit=x, an unreadable value the door now refuses; it takes a valid value per name, as the export twin already does. The reader's parameter is renamed so the canonical query-slot census does not read it as a query slot. Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn Co-authored-by: Claude --- packages/rest/src/rest-server-closed-query-params.test.ts | 6 +++++- packages/rest/src/rest-server.ts | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/rest/src/rest-server-closed-query-params.test.ts b/packages/rest/src/rest-server-closed-query-params.test.ts index 3079cb0fd21..e713bc7114b 100644 --- a/packages/rest/src/rest-server-closed-query-params.test.ts +++ b/packages/rest/src/rest-server-closed-query-params.test.ts @@ -426,9 +426,13 @@ describe('#7606 §3 — GET /search', () => { }); it('every name in the declared set is accepted — asserted against the export', async () => { + // A VALID value per name, as in the export twin above: `limit` is read + // as a whole number (#20062) and refuses `'x'` with `VALIDATION_FAILED` + // for a reason that has nothing to do with the closed set. + const validValue: Record = { limit: '10', perObject: '5' }; for (const name of GLOBAL_SEARCH_PARAMS) { const { search } = boot(); - const answer = await search({ [name]: 'x' }); + const answer = await search({ [name]: validValue[name] ?? 'x' }); expect(answer.status, `"${name}" is declared supported but was refused`).toBe(200); } }); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index d9ccb884922..02ceccd1373 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -718,12 +718,12 @@ const UNDECLARED_ROW_COUNT_PARAM = z.number().int().optional(); * reaches this function is a single string or nothing. */ function readDeclaredQueryNumber( - query: Record | undefined, + queryParams: Record | undefined, param: string, declared: z.ZodType, opts: { readonly emptyIsAbsent: boolean }, ): number | undefined { - const raw = query?.[param]; + const raw = queryParams?.[param]; let input: unknown = raw; if (raw === undefined || raw === null || (raw === '' && opts.emptyIsAbsent)) { input = undefined; From eef1a66894d357f9095387db4f154ae4d6a905cb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 07:49:16 +0000 Subject: [PATCH 3/3] chore(changeset): rest ?limit= parsing narrowing (minor, BREAKING) Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn Co-authored-by: Claude --- .changeset/20061-rest-limit-parsing.md | 74 ++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 .changeset/20061-rest-limit-parsing.md diff --git a/.changeset/20061-rest-limit-parsing.md b/.changeset/20061-rest-limit-parsing.md new file mode 100644 index 00000000000..5ce6226de2b --- /dev/null +++ b/.changeset/20061-rest-limit-parsing.md @@ -0,0 +1,74 @@ +--- +'@objectstack/rest': minor +--- + +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) + +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. + +`GET /data/import/jobs`, `GET /data/:object/export`, `GET /meta/:type/:name/history` +and `GET /search` read `?limit=` with a bare `Number()`. That coercion does not +fail. It invents a value, and the door served it with a `200`: +- `?limit=0` on the import-job history answered the 50-row default against a + declaration of `min(1).max(200)`; +- `?limit=abc` on the export downloaded one row; +- on the metadata history it returned the whole change log; +- on search it removed the overall cap. + +Since `@objectstack/client` sends `limit` exactly as the caller wrote it, the door +is the only place such a value can be refused. + +Each door now reads the parameter against its own declaration: +- **`GET /data/import/jobs`** reads `limit` and `offset` through + `ListImportJobsRequestSchema` (`limit` `int().min(1).max(200)`, default 50; + `offset` `int().min(0)`, default 0). +- **`GET /meta/:type/:name/history`** reads `limit` through + `HistoryMetaItemRequestSchema.limit` (`z.number()`, so any finite number, as + before). +- **`GET /data/:object/export`** and **`GET /search`** declare no request schema. + There `limit` must be a whole number. Their range handling is unchanged: the + export floor of 1 and cap of 50000, and search's `[1, 100]` clamp. + +A value outside the declaration answers `400` with the data surface's existing +envelope, `{ error, code: 'VALIDATION_FAILED', fields }`. `fields[0].field` names +the parameter. `fields[0].code` is the ADR-0114 member for the failed constraint: +`invalid_type` for a value that is not a number (or not a whole one, where one is +required), `min_value` or `max_value` for one outside a declared bound. The +service is never called. + +What changes, per door (every row answered `200` before): + +| door | request | answered before | answers now | +|:--|:--|:--|:--| +| `GET /data/import/jobs` | `?limit=0`, `?limit=-3` | 50 rows / 1 row | `400`, `min_value` | +| `GET /data/import/jobs` | `?limit=201`, `?limit=500` | 200 rows | `400`, `max_value` | +| `GET /data/import/jobs` | `?limit=abc`, `?limit=1.5`, `?limit=Infinity` | 50 / 1.5 / 200 rows | `400`, `invalid_type` | +| `GET /data/import/jobs` | `?offset=-1`, `?offset=abc`, `?offset=1.5` | offset 0 / 0 / 1.5 | `400`, `min_value` / `invalid_type` | +| `GET /data/:object/export` | `?limit=abc`, `?limit=` (empty) | a one-row export | `400`, `invalid_type` | +| `GET /data/:object/export` | `?limit=1.5`, `?limit=Infinity` | limit 1.5 / capped to 50000 | `400`, `invalid_type` | +| `GET /meta/:type/:name/history` | `?limit=abc`, `?limit=Infinity` | the whole change log | `400`, `invalid_type` | +| `GET /meta/:type/:name/history` | `?limit=` (empty) | zero events | `400`, `invalid_type` | +| `GET /search` | `?limit=abc` | no overall cap | `400`, `invalid_type` | +| `GET /search` | `?limit=1.5`, `?limit=Infinity` | a cap of 1.5 / 100 | `400`, `invalid_type` | + +A blank value such as `?limit=%20` is refused on every door. + +**Unchanged:** +- An absent `limit` keeps each door's default: 50 jobs, a 10000-row export, the + full history, search's 20. +- An empty `?limit=` on the import-job history and on search 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: export `?limit=0` still exports one + row, search `?limit=500` is still clamped to 100, and history still forwards + `0` or `1.5` because its declaration admits them. + +**Fix for a caller that now gets the `400`:** send `limit` as a whole number, within +the declared range where the door declares one (`1`–`200` for import jobs), or +omit it to get the door's default. + +