Skip to content

Commit 17f3c65

Browse files
fix(metadata-protocol)!: the Data API refuses a malformed or negative page window with a 400 naming the parameter as written (#25260)
<!-- claude-projects-attribution: {"github_login":"os-bill"} --> _Requested by **Bill** · [project thread](https://claude.ai/code/project/chan_016iMXWSZDzqWRWvWzdNo8wg?thread=cmsg_016iMXWSZDzqWRWvWzdNo8wgRKKHew7xesRk21mGXrtFzY)_ Fixes #25229 Clause-②: no (narrowing) Metadata-protocol: none ## What changes `findData`'s list-query normalizer (`packages/metadata-protocol/src/protocol.ts`) coerced the page window with a bare `Number(...)`. It now goes through `pageWindowValue`, which accepts only a non-negative safe integer and otherwise throws `400 INVALID_REQUEST` naming the parameter the caller wrote (`$top`, `top`, `limit`, `$skip`, `skip`, `offset`), before any read. That statement is the one place every list door meets with the window still unvalidated: `GET /data/:object` hands its querystring straight in, `POST /data/:object/query` validates its body against `FindDataRequestSchema` (which checks the type, not the sign) and then hands the same body in, and the runtime dispatcher's data list forwards its query the same way. So there is one check and no second validator on one door. The spec query schema is not touched: the shared normalizer sees every spelling, so the narrowing the order allowed in `packages/spec` was not needed. One REST door file changes, and it is not a second validator. `GET /data/:object/export` (`packages/rest/src/data-action-routes.ts`) feeds its own `findData` loop a chunk read from `?page=`, clamped to 50..5000 but not floored, so `?page=100.5` became a `limit` of 100.5. The new check would refuse that with a 400 naming `limit`, a parameter the export caller never wrote, where the base streamed the export. The chunk is now `Math.trunc`ed where `page` is read. That keeps the door's ledgered exemption (no `page` value turns a correct export into a 400). ## Measured (base `345f0b154c`, stock showcase boot, `showcase_account`, 14 rows, admin session) | request | base | this PR | |:--|:--|:--| | `GET ?$skip=-5` | 500 `INTERNAL_ERROR` | 400 `INVALID_REQUEST`, names `$skip` | | `GET ?$skip=-1&$top=3` | 500 `INTERNAL_ERROR` | 400, names `$skip` | | `POST {"offset":-5}` | 500 `INTERNAL_ERROR` | 400, names `offset` | | `GET ?$top=abc` | 200, 14 rows | 400, names `$top` | | `GET ?$top=-1` | 200, 14 rows | 400, names `$top` | | `POST {"limit":-1}` | 200, 14 rows | 400, names `limit` | | `GET ?$top=` | 200, 0 rows | 400, names `$top` | | `POST {"limit":"ten"}` | 400 `VALIDATION_FAILED` `query.limit` | unchanged | | `GET ?$skip=abc` | 200, 14 rows | 400, names `$skip` | | `POST {"offset":1.5}` | 200, 13 rows | 400, names `offset` | | `POST {"limit":1.5}` | 200, 1 row | 400, names `limit` | | `POST {"limit":0}` | 200, 0 rows | unchanged | | `GET ?$skip=` | 200, 14 rows | 400, names `$skip` | | `GET ?$top=5&$skip=2` | 200, 5 rows | unchanged | | `POST {"limit":5,"offset":2}` | 200, 5 rows | unchanged | The base 500s log `A non-negative integer must be provided to offset.` from knex `QueryBuilder.offset` (driver-sql). Every row of the card's table reproduced on today's `main`. ## The blank value: refused, not read as absent `?$top=` and `?$skip=` answer 400. The blank `?filter=` is read as absent because an empty filter and no filter mean the same thing. An empty window does not: an absent window means every row, so reading `?$top=` as absent would be the silent widening this card closes, and reading it as `0` (the base) answers an empty page nobody asked for. `?$skip=` used to answer the right rows by coincidence (`Number('')` is `0`, the same as no offset); it is refused too, so one rule covers both slots. `0` stays valid on both (`$top=0` asks for the total alone). ## Error code `INVALID_REQUEST`, status 400, `param` set to the caller's spelling: the same code and shape the same normalizer already answers for a repeated window parameter (`?$top=1&$top=2`). No new code, so the error-code ledger does not move. ## Clause-② and Metadata-protocol - `Clause-②: no (narrowing)`. Nothing is widened and no public surface grows. A request the doors used to answer (200 with every row, 200 with a rounded window, or 500) is now refused, so a published accept set narrows: the changeset carries the BREAKING banner and its ADR-0087 disposition (`not-required`, no migration). - `Metadata-protocol: none`. A query window is a request parameter, not metadata an author writes, so the changeset is `patch`. ## Tests - `packages/metadata-protocol/src/protocol.page-window.test.ts` (new). Refusal: 17 spellings and values, each asserting `code`, `status`, `param` and the message's first clause, with the engine never called. Plus the bounded quote of the refused value. Preservation: 7 well-formed windows reach `engine.find` as the same integers, `0` included. - `packages/rest/src/data-page-window-door.test.ts` (new, the door-level conformance test). Both routes over a real `SqlDriver` (SQLite): 11 querystring refusals and 4 body refusals answer 400 `INVALID_REQUEST` naming the spelling, with zero reads of the object. `{"limit":"ten"}` is still the schema's `VALIDATION_FAILED`. Controls page as before on both doors (`$top=2&$skip=1`, `limit: 2, offset: 1`, `top`/`skip`, no window, `limit: 0`, `$skip=0`). - Ablation (`scripts/ablation-replace.mjs`, run mode, then `metadata-protocol` rebuilt and `ablation-dist-preflight` showed the marker in `dist/index.js` and `dist/index.cjs`). The check was replaced by the base's bare `Number(value)`: the unit file went 18 failed / 7 passed (every refusal red, every preservation green), the door file 2 failed / 2 passed (both refusal tests red, both control tests green). Restore: blob equal to HEAD, `git diff HEAD` empty, rebuilt, preflight `--absent` over all 22 built files, both files green again (25/25, 4/4). - `packages/rest/src/rest-server-query-number-census.test.ts` (the export `page` exemption's pin, extended). `'100.5'` joins the loop asserting that every `page` value streams the same rows. A new test asserts that every `limit` the stub `findData` receives is a non-negative safe integer, over `abc`, empty, `1.5`, `100.5`, `Infinity`, `-5`, `50` and `5000`. Ablation with the `Math.trunc` removed: 2 failed / 14 passed (the new pin, and the stream loop at `100.5`). Restored, blob equal to HEAD: 16/16. - Local, after a full build of the `@objectstack/rest^...` closure (head `8845dd244`): `pnpm --filter @objectstack/metadata-protocol typecheck` exit 0, `pnpm --filter @objectstack/rest typecheck` exit 0, `pnpm --filter @objectstack/metadata-protocol test` 257 files passed, 3 skipped, 20893 tests passed, 19 skipped; `pnpm --filter @objectstack/rest test` 316 files, 6326 passed, 326 skipped. ## Gates (local, head `90e4e4d69e`) - On head `90e4e4d69e`, the current head with the export chunk fix: `--commands` derived 69 families, the 68 below plus `check:route-envelope`. All 69 ran with exit 0, and `--ran` reconciled them: 69 run, 0 NOT-MEASURED. `pnpm --filter @objectstack/rest typecheck` exit 0. The full rest suite and dogfood were not re-run for this one-line delta; CI's `Test Core` covers it. - On the first head `8845dd244`: `node scripts/pm/dispatch-gates.mjs --commands` on that diff derived 68 families. All 68 ran with exit 0, and `--ran` reconciled them: 68 run, 0 NOT-MEASURED, 0 UNRUN. Six needed a second run, each for a local reason outside the diff, and each exited 0 on the rerun. `check:issue-citations`: the board read failed once. `check-plugin-teardown-shape --self-test`: the shallow clone lacked its pinned fixture commit, which was then fetched. `check:dts-closure` and `check:dual-build-cjs-loads`: they ran during a concurrent rebuild of declaration files. `check:query-options-erasure` and `check:type-check-debt`: they hit a 300 s local timeout on a shared box. - Full dogfood, required by the `(narrowing)` arm, run on head `8845dd244` exactly as CI runs it (the `@objectstack/dogfood^...` closure built, then `pnpm --filter @objectstack/dogfood test` under the verify lock): VERDICT command-exit 0, 287 files passed and 3 skipped (290), 2398 tests passed and 18 skipped (2416). - `origin/main` is 9 commits ahead of the base. None of them touches `packages/metadata-protocol`, `packages/rest` or the runtime data domain, and a local `merge-tree` is clean, so the branch was not re-merged. CI runs on the merge ref. ## Acceptance notes - The message's second sentence ("a window the driver cannot read is either dropped, answering every row, or rejected by the database") describes the negative and NaN cases. A fractional window was rounded by the driver rather than dropped, which the sentence does not name. It is refused for the same reason (a window the caller did not mean), and the PR does not pin the sentence. - `driver-sql`'s knex call is untouched, as the claim says: the window never reaches it malformed once refused upstream. A direct `engine.find` caller that passes `offset: -1` without going through `findData` still meets knex's throw. That is a different door (in-process, not the Data API) and is not in this card. --- _Generated by [Claude Code](https://claude.ai/code/session_01A3FoRXqj2EgAzPQbXU3Kzh)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 284e048 commit 17f3c65

6 files changed

Lines changed: 429 additions & 4 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
fix(metadata-protocol)!: the Data API refuses a malformed or negative page window with a 400 naming the parameter as written, instead of a 500 or a silently ignored window
6+
7+
Clause-②: no (narrowing)
8+
Metadata-protocol: none
9+
10+
<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, object definition or stored shape is added, removed, renamed or re-shaped, and no exported declaration is removed or narrowed, so there is nothing for `objectstack migrate meta` to rewrite. What changes is the answer the list-query normalizer gives a request whose page window is not a non-negative integer. The remedy is in the request (send a count, or leave the parameter out), never a rewrite of a package source or a stored row. -->
11+
12+
**BREAKING** accept-set narrowing on the list doors (`GET /api/v1/data/:object`, `POST /api/v1/data/:object/query`, and the runtime dispatcher's data list). It changes what a Data API caller can send, not what a metadata author can write, so it ships as `patch` with this banner.
13+
14+
- **Before.** The list-query normalizer coerced the window with a bare `Number(...)`. A negative offset (`?$skip=-1`, `{"offset": -5}`) reached the SQL driver, which threw, and the caller got `500 INTERNAL_ERROR`. A limit that was not a number or was negative (`?$top=abc`, `?$top=-1`, `{"limit": -1}`), and an offset that was not a number (`?$skip=abc`), were dropped, so the caller got every row under a `200`. A blank `?$top=` became `0` and answered an empty page. A fractional window (`{"offset": 1.5}`, `{"limit": 1.5}`) was rounded by the driver.
15+
- **Now.** `limit` / `offset` and their spellings `$top`, `top`, `$skip`, `skip` must be non-negative integers. Anything else is refused before any read with `400 INVALID_REQUEST`, and the message names the parameter the caller wrote: `Query parameter '$skip' must be a non-negative integer; got "-1".` A blank value is refused too (`?$top=`, `?$skip=`), not read as absent: an absent window means every row, so reading a blank one that way would be the silent widening this closes.
16+
- **Unchanged.** A well-formed window pages exactly as before (`?$top=5&$skip=2`, `{"limit": 5, "offset": 2}`), and `0` stays valid on both (`$top=0` asks for the total alone). A non-number in a `POST` body (`{"limit": "ten"}`) is still the body schema's `400 VALIDATION_FAILED`.
17+
18+
**The remedy.** Send the window as a whole count of zero or more, or leave the parameter out.
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #25229 — `findData`'s list-query normalizer coerced the page window with a
5+
* bare `Number(...)` and no range check.
6+
*
7+
* Every list door reaches that one statement with the window unvalidated:
8+
* `GET /data/:object` hands its querystring over as strings, `POST
9+
* /data/:object/query`'s body schema checks that `limit` / `offset` are
10+
* numbers but not their sign, and the runtime dispatcher forwards both. So a
11+
* `NaN` (`?$top=abc`) or a negative limit reached the driver, which drops a
12+
* window it cannot read — every row, under a 200 — and a negative offset
13+
* reached knex's `QueryBuilder.offset`, which throws: a 500.
14+
*
15+
* ## Two assertion classes
16+
*
17+
* 1. **REFUSAL** — each malformed window, in every spelling, answers `400`
18+
* `INVALID_REQUEST` naming the spelling the caller wrote, and the engine is
19+
* never reached. `code` AND `status` are asserted: the unfixed normalizer
20+
* never threw for these, so a bare `toThrow()` would only be half a pin.
21+
* 2. **PRESERVATION** — a well-formed window reaches `engine.find` as the
22+
* integers it always did, `0` included. Green in both directions, so a
23+
* guard, not evidence: it is what a refusal is cheapest to break.
24+
*
25+
* The door-level half (both REST routes over a real `SqlDriver`) is
26+
* `packages/rest/src/data-page-window-door.test.ts`.
27+
*/
28+
29+
import { describe, it, expect, vi } from 'vitest';
30+
import { ObjectStackProtocolImplementation } from './protocol.js';
31+
32+
const SCHEMA = {
33+
name: 'invoice',
34+
nameField: 'name',
35+
fields: {
36+
name: { name: 'name', type: 'text' },
37+
status: { name: 'status', type: 'text' },
38+
},
39+
};
40+
41+
function makeProtocol() {
42+
const find = vi.fn(async () => [] as unknown[]);
43+
const engine = {
44+
registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) },
45+
find,
46+
aggregate: vi.fn(async () => [] as unknown[]),
47+
count: vi.fn(async () => 0),
48+
};
49+
return { p: new ObjectStackProtocolImplementation(engine as any), find };
50+
}
51+
52+
/** The option bag `engine.find` was actually handed, for an accepted query. */
53+
async function optionsFor(query: Record<string, unknown>): Promise<Record<string, unknown>> {
54+
const { p, find } = makeProtocol();
55+
await p.findData({ object: 'invoice', query } as never);
56+
expect(find, `${JSON.stringify(query)} never reached engine.find`).toHaveBeenCalledTimes(1);
57+
return (find.mock.calls[0] as unknown[])[1] as Record<string, unknown>;
58+
}
59+
60+
/** The refusal a rejected query produced, plus proof the engine was not reached. */
61+
async function refusalFor(query: Record<string, unknown>): Promise<{
62+
message: string; status?: number; code?: string; param?: string;
63+
}> {
64+
const { p, find } = makeProtocol();
65+
let answered: unknown;
66+
try {
67+
answered = await p.findData({ object: 'invoice', query } as never);
68+
} catch (e) {
69+
const err = e as Error & { status?: number; code?: string; param?: string };
70+
expect(find, 'the engine was reached before the refusal').not.toHaveBeenCalled();
71+
return { message: err.message, status: err.status, code: err.code, param: err.param };
72+
}
73+
throw new Error(
74+
`${JSON.stringify(query)} was ACCEPTED (answered ${JSON.stringify(answered)}) instead of refused`,
75+
);
76+
}
77+
78+
// ---------------------------------------------------------------------------
79+
// 1. REFUSAL
80+
// ---------------------------------------------------------------------------
81+
82+
describe('#25229 — a malformed page window is refused, naming the spelling the caller wrote', () => {
83+
it.each<[string, Record<string, unknown>, string]>([
84+
// [wire spelling, the query, what it used to become]
85+
['$skip', { $skip: '-5' }, 'a knex throw, answered 500'],
86+
['$skip', { $skip: '-1', $top: '3' }, 'a knex throw, answered 500'],
87+
['$skip', { $skip: 'abc' }, 'offset: NaN'],
88+
['skip', { skip: '-1' }, 'a knex throw, answered 500'],
89+
['offset', { offset: -5 }, 'a knex throw, answered 500'],
90+
['offset', { offset: 1.5 }, 'a fractional offset'],
91+
['offset', { offset: '' }, 'offset: 0'],
92+
['$top', { $top: 'abc' }, 'limit: NaN, every row'],
93+
['$top', { $top: '-1' }, 'a dropped window, every row'],
94+
['$top', { $top: '' }, 'limit: 0, an empty page'],
95+
['$top', { $top: ' ' }, 'limit: 0, an empty page'],
96+
['$top', { $top: '1.5' }, 'a fractional limit'],
97+
['top', { top: 'abc' }, 'limit: NaN, every row'],
98+
['limit', { limit: -1 }, 'a dropped window, every row'],
99+
['limit', { limit: 'ten' }, 'limit: NaN, every row'],
100+
['limit', { limit: true }, 'limit: 1'],
101+
['limit', { limit: Number.POSITIVE_INFINITY }, 'an unbounded limit'],
102+
])('refuses %s in %j with 400 INVALID_REQUEST (was: %s)', async (param, query) => {
103+
const err = await refusalFor(query);
104+
105+
expect(err.code).toBe('INVALID_REQUEST');
106+
expect(err.status).toBe(400);
107+
expect(err.param).toBe(param);
108+
expect(err.message).toContain(`Query parameter '${param}' must be a non-negative integer`);
109+
});
110+
111+
it('quotes the value it refused, bounded', async () => {
112+
const err = await refusalFor({ $top: 'x'.repeat(500) });
113+
114+
expect(err.message).toContain('got "xxxx');
115+
expect(err.message.length).toBeLessThan(400);
116+
});
117+
});
118+
119+
// ---------------------------------------------------------------------------
120+
// 2. PRESERVATION
121+
// ---------------------------------------------------------------------------
122+
123+
describe('#25229 — a well-formed page window reaches the engine unchanged', () => {
124+
it.each<[Record<string, unknown>, number | undefined, number | undefined]>([
125+
[{ $top: '5', $skip: '2' }, 5, 2],
126+
[{ top: '5', skip: '2' }, 5, 2],
127+
[{ limit: 5, offset: 2 }, 5, 2],
128+
[{ limit: '5', offset: '2' }, 5, 2],
129+
[{ $top: '0' }, 0, undefined],
130+
[{ $skip: '0' }, undefined, 0],
131+
[{}, undefined, undefined],
132+
])('%j → limit %s, offset %s', async (query, limit, offset) => {
133+
const options = await optionsFor(query);
134+
135+
expect(options.limit).toBe(limit);
136+
expect(options.offset).toBe(offset);
137+
// No window spelling leaks into the implicit-filter bucket.
138+
expect(options.where).toBeUndefined();
139+
});
140+
});

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4751,6 +4751,52 @@ function repeatedQueryParamError(param: string, count: number): Error {
47514751
return err;
47524752
}
47534753

4754+
/**
4755+
* [#25229] The page window (`limit` / `offset`, spelled `$top` / `top` /
4756+
* `$skip` / `skip` on the wire) as the non-negative integer every driver
4757+
* needs, or a refusal naming the parameter the caller wrote (#4226).
4758+
*
4759+
* The bare `Number(...)` this replaces failed three ways, none of them loud:
4760+
*
4761+
* - `NaN` (`?$top=abc`) and a negative limit (`?$top=-1`) reached the driver,
4762+
* which drops a window it cannot read — every row answered under a 200, the
4763+
* #7321 class (a malformed window served as if no window was sent);
4764+
* - a negative offset (`?$skip=-1`, `{"offset": -1}`) reached knex's
4765+
* `QueryBuilder.offset`, which throws — a 500 for a caller's mistake;
4766+
* - a blank value (`?$top=`) is `Number('') === 0`, an empty page nobody
4767+
* asked for.
4768+
*
4769+
* A blank value is refused, not read as absent. The blank `?filter=` is
4770+
* absent because an empty filter and no filter mean the same thing; an empty
4771+
* window does not — absent means "every row", so reading `?$top=` that way is
4772+
* the silent widening above under another name, and reading it as `0` is the
4773+
* guess this replaces. `0` itself stays valid: `$top=0` asks for the total
4774+
* alone.
4775+
*
4776+
* Only the two arms a door can deliver are read: a number (a JSON body) and a
4777+
* string (a querystring). Anything else — `true`, which `Number` would quietly
4778+
* make `1` — is refused like any other value that is not a count.
4779+
*/
4780+
function pageWindowValue(value: unknown, param: string): number {
4781+
const n = typeof value === 'number'
4782+
? value
4783+
: typeof value === 'string' && value.trim() !== ''
4784+
? Number(value)
4785+
: Number.NaN;
4786+
if (Number.isSafeInteger(n) && n >= 0) return n;
4787+
// `String` for a number: `JSON.stringify(Infinity)` is `null`.
4788+
const shown = typeof value === 'number' ? String(value) : JSON.stringify(value) ?? String(value);
4789+
const err: any = new Error(
4790+
`Query parameter '${param}' must be a non-negative integer; got `
4791+
+ `${shown.length > 64 ? `${shown.slice(0, 64)}…` : shown}. The query was not run: a window `
4792+
+ 'the driver cannot read is either dropped, answering every row, or rejected by the database.',
4793+
);
4794+
err.status = 400;
4795+
err.code = 'INVALID_REQUEST';
4796+
err.param = param;
4797+
throw err;
4798+
}
4799+
47544800
/**
47554801
* [#7321] Refuse a repeated occurrence of any list-query parameter this
47564802
* normalizer reads as single-valued, and normalise the benign one-element array
@@ -14624,8 +14670,15 @@ export class ObjectStackProtocolImplementation implements
1462414670
// (`options.limit = Number(options.top)` — the alias overwrote the
1462514671
// canonical key, so `{top: 1, limit: 3}` answered 1 over HTTP and 3
1462614672
// through a direct engine call).
14627-
if (options.limit != null) options.limit = Number(options.limit);
14628-
if (options.offset != null) options.offset = Number(options.offset);
14673+
//
14674+
// [#25229] The coercion is CHECKED, not bare (see
14675+
// {@link pageWindowValue}). This is the one statement every list door
14676+
// reaches with the window still unvalidated — `GET /data/:object`'s
14677+
// querystring, `POST /data/:object/query`'s body (whose schema checks
14678+
// the type, not the sign) and the runtime dispatcher — so the refusal
14679+
// lives here, once.
14680+
if (options.limit != null) options.limit = pageWindowValue(options.limit, slotParam('limit'));
14681+
if (options.offset != null) options.offset = pageWindowValue(options.offset, slotParam('offset'));
1462914682

1463014683
// Projection: comma-separated string → array. A single-value
1463114684
// querystring param arrives as a bare string — `?fields=name` or the

‎packages/rest/src/data-action-routes.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1020,7 +1020,13 @@ export function registerDataActionEndpoints(this: DataActionRouteHost, basePath:
10201020
// the same rows in the same order, so no value of it can widen or
10211021
// substitute the answer, and refusing one would turn a correct
10221022
// export into a `400`.
1023-
const chunkSize = Math.min(MAX_CHUNK, Math.max(50, q.page != null ? Number(q.page) || 500 : 500));
1023+
//
1024+
// [#25229] …which is why it is TRUNCATED where it is read: the
1025+
// chunk becomes each `findData`'s `limit`, and the normalizer
1026+
// refuses a fractional window (`?page=100.5` was a `take` of
1027+
// 100.5, answered 400 naming `limit`, a parameter the caller
1028+
// never wrote).
1029+
const chunkSize = Math.min(MAX_CHUNK, Math.max(50, q.page != null ? Math.trunc(Number(q.page)) || 500 : 500));
10241030
// Colour cells only for xlsx within the style cap; decided up
10251031
// front (before streaming) since we can't know the true row
10261032
// count until the stream drains.

0 commit comments

Comments
 (0)