From 898ffeb292f0e6f0562436795322e5fda30905c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 20:00:34 +0000 Subject: [PATCH] fix(runtime): enforce ListRunsRequestSchema's declared limit range (#8054) `parseIntegerParam` gains optional (min, max) bounds, threaded through from `ListRunsRequestSchema.shape.limit`'s own `.min()`/`.max()` at the one call site that declares a range (GET /automation/:name/runs), rather than re-listing (1, 100) as literals. `?limit=0`/`-5` no longer silently answer "no runs", and `?limit=101` is no longer served uncapped -- both now refused as 400 VALIDATION_FAILED with the ADR-0114 min_value/max_value field code. Callers that pass no bounds (e.g. notifications.ts) are unaffected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V --- .changeset/list-runs-limit-range.md | 34 +++++++ .../automation-runs-query-validation.test.ts | 93 +++++++++++++++---- packages/runtime/src/domains/automation.ts | 35 +++++-- packages/runtime/src/query-param.ts | 69 +++++++++++--- 4 files changed, 194 insertions(+), 37 deletions(-) create mode 100644 .changeset/list-runs-limit-range.md diff --git a/.changeset/list-runs-limit-range.md b/.changeset/list-runs-limit-range.md new file mode 100644 index 0000000000..dfaf285bff --- /dev/null +++ b/.changeset/list-runs-limit-range.md @@ -0,0 +1,34 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): `GET /automation/:name/runs?limit=` now enforces its own declared 1..100 range (#8054) + +`ListRunsRequestSchema.limit` has always declared `.min(1).max(100)`, but the +boundary that reads it (`parseIntegerParam`) only checked that the value was a +whole number, never that it fell inside the declared range. Two measured +symptoms, both a `200` with the wrong answer: + +- `?limit=0` (and any negative value) reached the engine as-is, and + `store.listHistory(flowName, 0).slice(0, 0)` is `[]` — a confidently wrong + "this flow has never run", the same shape #7300 fixed for `?limit=abc`, but + produced by a value that *was* a valid integer. +- `?limit=101` reached the engine uncapped, so the declared upper bound was + decorative. + +`parseIntegerParam` gains an optional third `bounds` argument +(`{ min?, max? }`); every existing caller that omits it is byte-for-byte +unaffected — range enforcement is opt-in, per call site. The one call site with +a declared range (`GET /automation/:name/runs`) now threads +`ListRunsRequestSchema.shape.limit`'s own `.min()`/`.max()` through, rather than +re-listing `(1, 100)` as literals — the #7359 discipline +(`ExecutionStatus.options`) applied to a bounded number instead of a closed set, +so the wire's declared range and the boundary's enforced range cannot drift +apart the next time the schema's bounds change. + +A value outside the range is refused in the same house shape as everything else +in this module: `400` `VALIDATION_FAILED` (ADR-0112) with a `details.fields[]` +entry carrying the ADR-0114 field code the property names already mirror — +`min_value` below 1, `max_value` above 100. Both declared boundary values +(`?limit=1`, `?limit=100`) and every ordinary in-range value stay exactly as +they were. diff --git a/packages/runtime/src/domains/automation-runs-query-validation.test.ts b/packages/runtime/src/domains/automation-runs-query-validation.test.ts index 3f28460096..70e66707fc 100644 --- a/packages/runtime/src/domains/automation-runs-query-validation.test.ts +++ b/packages/runtime/src/domains/automation-runs-query-validation.test.ts @@ -1,18 +1,24 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * #7300 / #7359 — `GET /api/v1/automation/:name/runs`'s query parameters, at - * the boundary that reads them. + * #7300 / #7359 / #8054 — `GET /api/v1/automation/:name/runs`'s query + * parameters, at the boundary that reads them. * * #7300 (below) closed the two parameters this handler already forwarded but - * COERCED. #7359 closed the third, which is the same 200-with-the-wrong-answer - * arrived at from the opposite direction: `status` was declared by + * COERCED. #7359 closed a third shape: `status` was declared by * `ListRunsRequestSchema`, had no slot on `IAutomationService.listRuns`, and * was never built into the handler's option object — so `?status=failed` was * dropped here in silence and the caller was answered with EVERY run of the * flow. #7300 deliberately pinned that ignore-the-key behaviour rather than - * decide it; #7359 took the enforce route, so that one pin is superseded here - * by cases asserting the opposite on the same input. + * decide it; #7359 took the enforce route, so that one pin was superseded by + * cases asserting the opposite on the same input. #8054 is the sibling of + * #7359 on the SAME route's OTHER declared constraint: `limit` was already + * type-checked (#7300) but its declared RANGE (`.min(1).max(100)`) was never + * read, so `?limit=0` answered 200 with zero rows — "this flow has never + * run", confidently, about a flow with runs — and `?limit=101` reached the + * engine with its cap simply not applied. The `?limit=1000`/`?limit=-5`/ + * `?limit=0` preservation rows #7300 pinned are superseded here the same way + * #7359 superseded the `status`-ignored case: same input, opposite behaviour. * * The filed defect is character-for-character #6928's, one file over: * `{ limit: query.limit ? Number(query.limit) : undefined, cursor: query.cursor }`. @@ -35,10 +41,14 @@ * absence of a throw, which is not the defect. The defect is the missing * envelope. * 2. PRESERVATION — every value that had a defensible answer before keeps it, - * byte for byte, at the exact `listRuns(name, options)` call. That includes - * out-of-RANGE numbers (`?limit=1000`), which `ListRunsRequestSchema` bounds - * and the engine slices by: range is the service's declared business and - * stays reachable, unrefused. + * byte for byte, at the exact `listRuns(name, options)` call. As of #8054 + * that no longer includes out-of-RANGE numbers (`?limit=1000`, `?limit=0`): + * `ListRunsRequestSchema` bounds `limit` to 1..100 and the boundary now + * enforces that declared range instead of only the value's type, so those + * inputs moved from PRESERVATION to REFUSAL. An ORDINARY in-range value + * (`?limit=25`) and both declared boundary values (`?limit=1`, + * `?limit=100`) still keep their defensible answer — the over-block guard + * for the new range check. * * The wire mapping of the thrown shape to `400` + `details.fields[]` is not * re-proved here — it is one mapping for every domain handler, pinned at both @@ -197,6 +207,42 @@ describe('#7359 — a `?status=` outside the declared set is refused, not silent }); }); +describe('#8054 — a `?limit=` outside the declared 1..100 range is refused, not silently answered', () => { + // Measured, twice, identical both passes: `?limit=0` answered 200 with + // ZERO rows (a confidently wrong "this flow has never run" — the store + // sliced `.slice(0, 0)`), and `?limit=101` answered 200 with the cap + // simply not applied. `ListRunsRequestSchema` had declared `.min(1).max(100)` + // the whole time; this boundary just never read it. Once the range is + // enforced there is no safe reading for a value outside it — same + // reasoning #7359 already applied to `status`, on a bounded number instead + // of a closed set. + it.each([ + ['0 (the "no runs" trap)', '0', 'min_value'], + ['-5 (negative)', '-5', 'min_value'], + ['101 (one past the declared cap)', '101', 'max_value'], + ['1000 (far past the declared cap — the old preserved case, inverted)', '1000', 'max_value'], + ])('refuses ?limit=%s with 400 VALIDATION_FAILED (%s)', async (_label, raw, expectedCode) => { + const { details, status, listRuns } = await refusalFor({ limit: raw }); + + // ADR-0112: the envelope, not merely the throw — `code` AND `status`. + expect(details?.code).toBe('VALIDATION_FAILED'); + expect(status).toBe(400); + // ADR-0114: `min_value`/`max_value` are the field codes the property + // names already mirror — no new vocabulary minted for this. + expect(details?.fields).toEqual([ + { field: 'limit', code: expectedCode, message: expect.stringContaining('`limit`') }, + ]); + // The whole point: the service is never reached with a limit outside + // its own declared contract, so no caller reads a wrong-but-confident + // "no runs" and no caller gets an uncapped result set. + expect(listRuns).not.toHaveBeenCalled(); + }); + + // The boundary values themselves — `?limit=1` and `?limit=100` — are + // pinned as VALID in the `#7300` preservation block below (they were + // always in range and stay unaffected), so they are not repeated here. +}); + describe('#7300 — every value that had a defensible answer keeps it', () => { async function listWith(query: Record | undefined) { const { dispatcher, listRuns } = makeDispatcher(); @@ -207,19 +253,26 @@ describe('#7300 — every value that had a defensible answer keeps it', () => { it.each([ // [label, query, the exact options object `listRuns` must receive] ['?limit=20', { limit: '20' }, { limit: 20, cursor: undefined, status: undefined }], + // An ordinary in-range value is the over-block guard for #8054: bounds + // threading must not start refusing numbers that were always fine. + ['?limit=25 (ordinary, mid-range)', { limit: '25' }, { limit: 25, cursor: undefined, status: undefined }], ['?limit=1 (the low boundary)', { limit: '1' }, { limit: 1, cursor: undefined, status: undefined }], ['?limit=100 (the declared high boundary)', { limit: '100' }, { limit: 100, cursor: undefined, status: undefined }], - // Out of RANGE is not out of DOMAIN. `ListRunsRequestSchema` bounds - // `limit` to 1..100 and the engine slices by whatever it is handed; - // neither answer is this boundary's to change, so both still arrive. - ['?limit=1000 (over the declared range)', { limit: '1000' }, { limit: 1000, cursor: undefined, status: undefined }], - ['?limit=-5 (under it)', { limit: '-5' }, { limit: -5, cursor: undefined, status: undefined }], - // Falsy spellings meant "no limit here" before this gate existed and - // still do — they must not become a new 400. `'0'` is NOT one of them: - // the string is truthy, so `query.limit ? Number(query.limit) : …` read - // it as the number `0` and passed it on, and that is preserved too. + // Out-of-RANGE numbers used to be preserved here (`?limit=1000`, + // `?limit=-5`, `?limit=0`) on the theory that range was the engine's + // declared business, not this boundary's. #8054 found the one place + // that reasoning was wrong: `ListRunsRequestSchema` had ALWAYS + // declared `limit`'s range, and nothing enforced it, so `?limit=0` + // answered "no runs" and `?limit=101` reached the engine uncapped. + // Those three rows are superseded by the `#8054` refusal block below + // rather than deleted outright — same input, opposite behaviour now. + // + // Falsy spellings still mean "no limit here", unaffected by bounds + // because the falsy gate runs BEFORE the bounds check: absent, `null`, + // `''`, and an in-process (non-string) `0` never reach it. `'0'` as a + // QUERY-STRING value is different — the string is truthy, so it always + // reached `Number()` — and is exercised in the `#8054` block instead. ['?limit= (empty)', { limit: '' }, { limit: undefined, cursor: undefined, status: undefined }], - ['?limit=0', { limit: '0' }, { limit: 0, cursor: undefined, status: undefined }], ['limit: 0 (in-process number)', { limit: 0 }, { limit: undefined, cursor: undefined, status: undefined }], ['limit: null', { limit: null }, { limit: undefined, cursor: undefined, status: undefined }], ['no parameters at all', {}, { limit: undefined, cursor: undefined, status: undefined }], diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 8afe63980d..28c6e30573 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -19,6 +19,7 @@ import { validationFailure, validationFailureDetails, fieldsFromZodIssues, VALIDATION_FAILED_STATUS, } from '../validation-failure.js'; import { ExecutionStatus } from '@objectstack/spec/automation'; +import { ListRunsRequestSchema } from '@objectstack/spec/api'; import { parseEnumParam, parseIntegerParam, parseStringParam } from '../query-param.js'; import { capabilityUnavailable } from './unavailable.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; @@ -817,11 +818,29 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // first implementation that starts honouring cursors must not // be the one that discovers the type was never enforced. // - // Out-of-range numbers are NOT refused — `?limit=1000` still - // reaches the engine as 1000 and is sliced there. Range is the - // service's declared business (`ListRunsRequestSchema` bounds it - // 1..100); this gate only refuses values that were never whole - // numbers. + // [#8054] `limit`'s RANGE — `ListRunsRequestSchema` has always + // declared `.min(1).max(100)`, and until now this gate only + // checked that the value was a whole number at all, never that + // it fell inside that declared range. `?limit=0` reached the + // engine as 0, and `store.listHistory(flowName, 0).slice(0, 0)` + // is `[]` — a confidently wrong "this flow has never run", + // exactly #7300's shape but from a value that WAS a valid + // integer. `?limit=101` reached the engine uncapped, so the + // declared upper bound was decorative. + // + // The bounds are READ off `ListRunsRequestSchema.shape.limit` + // rather than re-listed as `(1, 100)` here — the same + // discipline `status` already applies via + // `ExecutionStatus.options` two lines down. Re-listing the + // literals would make the boundary correct today and silently + // wrong again the moment the schema's own `.min()`/`.max()` + // changes; reading them makes declared == enforced true by + // construction, not by two call sites happening to agree. + // + // A value outside the range is refused in the same house shape + // as everything else in this module — `VALIDATION_FAILED` with + // an ADR-0114 field code, here `min_value` / `max_value`, the + // ones the property names already mirror. // // [#7359] `status` is the THIRD declared parameter, and until // now the only one this handler never read. `ListRunsRequestSchema` @@ -840,9 +859,13 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // rather than a list copied into this file: the wire schema is // built from that same enum, so a future member cannot be // accepted by one and refused by the other. + const limitBounds = ListRunsRequestSchema.shape.limit.unwrap(); const options = query ? { - limit: parseIntegerParam('limit', query.limit), + limit: parseIntegerParam('limit', query.limit, { + min: limitBounds.minValue ?? undefined, + max: limitBounds.maxValue ?? undefined, + }), cursor: parseStringParam('cursor', query.cursor), status: parseEnumParam('status', query.status, ExecutionStatus.options), } diff --git a/packages/runtime/src/query-param.ts b/packages/runtime/src/query-param.ts index 56bcacf868..3e9d3be481 100644 --- a/packages/runtime/src/query-param.ts +++ b/packages/runtime/src/query-param.ts @@ -39,12 +39,23 @@ * spelling either: ADR-0112's registered `VALIDATION_FAILED` and ADR-0114's * closed field-level catalog (`FieldErrorCode`) already say all of this. * - * What these parsers deliberately do NOT do is police RANGE. A value that is + * What these parsers do NOT do, by default, is police RANGE. A value that is * out of range but in domain (`?limit=1000`) is the service's declared business * — the notifications inbox clamps it, the automation engine slices by it — and * a boundary that started refusing those would be changing an answer that was - * already defensible. These gates add a refusal for values that were never of - * the declared type at all. + * already defensible. So range enforcement is opt-in, per call site + * ({@link parseIntegerParam}'s `bounds`), read off the call site's own + * declared schema rather than re-listed — never switched on module-wide. + * + * #8054 is that opt-in's origin case, and the same shape as #7359 one field + * over: `ListRunsRequestSchema` had always declared `limit`'s range + * (`.min(1).max(100)`), and the boundary was not reading it — + * + * ?limit=0 → 200, zero rows → "this flow has never run", confidently + * ?limit=101 → 200, cap ignored → a result-set size nothing had asked for + * + * — so that one call site now threads its own bounds through; every other + * caller of `parseIntegerParam` is unaffected, because it passes none. */ import type { FieldErrorCode } from '@objectstack/spec/api'; @@ -94,9 +105,26 @@ export function parseBooleanParam(param: string, raw: unknown): boolean | undefi throw invalidQueryParam(param, 'invalid_boolean', '`true` or `false`', raw); } +/** + * Inclusive numeric bounds a call site may pass to {@link parseIntegerParam} + * so it can police RANGE on top of type — read off the caller's own declared + * schema (`ExistingSchema.shape.limit.unwrap().minValue` / `.maxValue`), + * never re-listed as literals. That is the #7359 discipline + * ({@link parseEnumParam} reading `ExecutionStatus.options`) applied to a + * bounded number instead of a closed set: the wire's declared range and the + * boundary's enforced range cannot drift, because they are the same read. + */ +export interface IntegerParamBounds { + /** Inclusive lower bound — a value below it is refused as `min_value`. */ + readonly min?: number; + /** Inclusive upper bound — a value above it is refused as `max_value`. */ + readonly max?: number; +} + /** * A whole-number parameter — the window sizes both `?limit=` defects were filed - * against (#6928, #7300). + * against (#6928, #7300), and, once `bounds` is supplied, the RANGE defect + * #8054 filed against the same parameter. * * `Number(query.limit)` answers `NaN` for `?limit=abc`, and NaN then survives * the guards downstream, because the two idioms services use to default a @@ -108,22 +136,41 @@ export function parseBooleanParam(param: string, raw: unknown): boolean | undefi * REFUSED: values that are not a whole number at all — `abc`, `10abc`, `1.5`, * `Infinity`, a repeated `?limit=1&limit=2`, a structured value. * - * NOT refused, deliberately: an out-of-RANGE number. Range is the consuming - * service's declared contract (clamp, slice, or reject with its own message), - * and this gate must not start answering 400 for a value that already had a - * defensible answer. + * RANGE (`bounds`) is opt-in, per call site, and OFF unless a bounds object is + * passed — a caller that omits the third argument is byte-for-byte the + * pre-#8054 gate: an out-of-range number (`?limit=1000`) reaches the service + * unrefused, exactly as before, because range used to be nobody's job at this + * boundary. #8054 found the one call site (`ListRunsRequestSchema`'s `limit`) + * that HAD declared a range and was not enforcing it — `?limit=0` answered + * "no runs" with a 200, and `?limit=101` was served uncapped — so that call + * site now threads its own `.min()`/`.max()` through as `bounds`, and a value + * outside them is refused with the ADR-0114 field code the property name + * already mirrors (`min_value` / `max_value`), the same shape + * {@link parseEnumParam}'s `invalid_option` refusal takes for `status`. * * The falsy gate is the one both call sites already had * (`query.limit ? Number(query.limit) : undefined`): absent, `null`, `''` and - * `0` have always meant "no limit here", and they keep meaning that instead of - * becoming a new 400. + * an in-process (non-string) `0` have always meant "no limit here", and they + * keep meaning that — checked BEFORE `bounds`, so they never become a new 400 + * even when a `min` above 0 is supplied. Only a numeric STRING (`?limit=0`, + * truthy as a string) reaches the bounds check. */ -export function parseIntegerParam(param: string, raw: unknown): number | undefined { +export function parseIntegerParam( + param: string, + raw: unknown, + bounds?: IntegerParamBounds, +): number | undefined { if (!raw) return undefined; const parsed = Number(raw); if (!Number.isInteger(parsed)) { throw invalidQueryParam(param, 'invalid_number', 'a whole number', raw); } + if (bounds?.min !== undefined && parsed < bounds.min) { + throw invalidQueryParam(param, 'min_value', `a whole number >= ${bounds.min}`, raw); + } + if (bounds?.max !== undefined && parsed > bounds.max) { + throw invalidQueryParam(param, 'max_value', `a whole number <= ${bounds.max}`, raw); + } return parsed; }