Skip to content

Commit 765adb3

Browse files
committed
fix(metadata-protocol): refuse a malformed or negative page window with a 400 naming the caller's spelling
`findData`'s list-query normalizer coerced `limit` / `offset` with a bare `Number(...)`: a NaN or negative limit reached the driver and was dropped (every row, 200), a negative offset reached knex and threw (500), and a blank `$top=` became an empty page. The coercion now accepts only a non-negative safe integer and otherwise refuses `400 INVALID_REQUEST` naming `$top` / `top` / `limit` / `$skip` / `skip` / `offset` as written, before any read, on every door that reaches the normalizer. Claude-Session: https://claude.ai/code/session_01A3FoRXqj2EgAzPQbXU3Kzh Co-authored-by: Claude <noreply@anthropic.com>
1 parent 345f0b1 commit 765adb3

3 files changed

Lines changed: 378 additions & 2 deletions

File tree

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
Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#25229] The Data API's page window — `$top` / `top` / `limit` and `$skip` /
5+
* `skip` / `offset` — is a non-negative integer on BOTH list doors
6+
* (`GET /data/:object`'s querystring, `POST /data/:object/query`'s body), or a
7+
* `400 INVALID_REQUEST` naming the parameter the caller wrote, before any read;
8+
* over a real `SqlDriver` (SQLite), five rows.
9+
*
10+
* Measured on the base through these two routes (the stock showcase answered
11+
* the same, `showcase_account`, admin bearer):
12+
*
13+
* MEASURED_TABLE
14+
*
15+
* The refusal lives once, in the shared normalizer both doors hand their
16+
* request to (`metadata-protocol`'s `findData`); the per-spelling pin with a
17+
* recording engine is that package's `protocol.page-window.test.ts`. This file
18+
* is the door-level half: the status and `code` each door answers, the
19+
* spelling it names, that the driver was never asked, and the controls paging
20+
* exactly as before.
21+
*/
22+
23+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
24+
import { ObjectQL } from '@objectstack/objectql';
25+
import { SqlDriver } from '@objectstack/driver-sql';
26+
import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
27+
import { RestServer } from './rest-server';
28+
import { installDefaultOrganization } from './default-organization.testkit.js';
29+
30+
const OBJECT = 'rest_page_window_25229';
31+
32+
const ITEMS = {
33+
name: OBJECT,
34+
label: 'Items 25229',
35+
fields: {
36+
label: { name: 'label', type: 'text' as const },
37+
},
38+
};
39+
40+
const LABELS = ['a', 'b', 'c', 'd', 'e'];
41+
42+
/** name · the querystring `GET /data/:object` receives · the spelling the refusal names. */
43+
const REFUSED_GET: ReadonlyArray<readonly [string, Record<string, string>, string]> = [
44+
['$skip=-5 (the card)', { $skip: '-5' }, '$skip'],
45+
['$skip=-1&$top=3 (the card)', { $skip: '-1', $top: '3' }, '$skip'],
46+
['$skip=abc', { $skip: 'abc' }, '$skip'],
47+
['$top=abc (the card)', { $top: 'abc' }, '$top'],
48+
['$top=-1 (the card)', { $top: '-1' }, '$top'],
49+
['$top= blank (the card)', { $top: '' }, '$top'],
50+
['$top=1.5', { $top: '1.5' }, '$top'],
51+
['top=abc', { top: 'abc' }, 'top'],
52+
['skip=-1', { skip: '-1' }, 'skip'],
53+
['limit=-1', { limit: '-1' }, 'limit'],
54+
['offset=-1', { offset: '-1' }, 'offset'],
55+
];
56+
57+
/** name · the JSON body `POST /data/:object/query` receives · the spelling the refusal names. */
58+
const REFUSED_POST: ReadonlyArray<readonly [string, Record<string, unknown>, string]> = [
59+
['offset: -5 (the card)', { offset: -5 }, 'offset'],
60+
['limit: -1 (the card)', { limit: -1 }, 'limit'],
61+
['offset: 1.5', { offset: 1.5 }, 'offset'],
62+
['limit: 1.5', { limit: 1.5 }, 'limit'],
63+
];
64+
65+
function createMockServer() {
66+
const noop = () => {};
67+
return { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} };
68+
}
69+
70+
function makeRes() {
71+
const res: any = {
72+
write: () => true, end: () => {},
73+
header: () => res,
74+
status: (code: number) => { res._status = code; return res; },
75+
json: (body: any) => { res._json = body; return res; },
76+
};
77+
return res;
78+
}
79+
80+
describe('[#25229] the page window on both list doors — sqlite', () => {
81+
let engine: ObjectQL;
82+
let driver: any;
83+
const reads = { n: 0 };
84+
let list: (query: Record<string, string>) => Promise<{ status: number; body: any }>;
85+
let query: (body: Record<string, unknown>) => Promise<{ status: number; body: any }>;
86+
87+
beforeAll(async () => {
88+
driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as any);
89+
engine = new ObjectQL();
90+
engine.registerDriver(driver, true);
91+
await engine.init();
92+
engine.registry.registerObject(ITEMS as any);
93+
await engine.syncSchemas();
94+
// A real `single` install's Default Organization: the system writes below
95+
// derive it (ADR-0131 D9), as a fresh NOT NULL tenant column requires (D1).
96+
await installDefaultOrganization(engine);
97+
for (const label of LABELS) await engine.insert(OBJECT, { id: `r${label}`, label } as any);
98+
99+
// Reads of THIS object — the protocol's own metadata traffic is not the question.
100+
for (const verb of ['find', 'findOne', 'count', 'aggregate'] as const) {
101+
const real = driver[verb].bind(driver);
102+
driver[verb] = (o: string, ...rest: unknown[]) => { if (o === OBJECT) reads.n += 1; return real(o, ...rest); };
103+
}
104+
105+
const protocol = new ObjectStackProtocolImplementation(engine as any);
106+
const rest = new RestServer(createMockServer() as any, protocol as any, { api: { requireAuth: false } } as any);
107+
(rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
108+
rest.registerRoutes();
109+
const getRoute = rest.getRoutes().find((r: any) => r.method === 'GET' && r.path === '/api/v1/data/:object');
110+
const postRoute = rest.getRoutes().find((r: any) => r.method === 'POST' && r.path === '/api/v1/data/:object/query');
111+
expect(getRoute).toBeDefined();
112+
expect(postRoute).toBeDefined();
113+
list = async (q) => {
114+
const res = makeRes();
115+
// What a querystring carries: strings.
116+
await getRoute!.handler({ params: { object: OBJECT }, query: { $orderby: 'label', ...q }, headers: {} } as any, res);
117+
return { status: res._status ?? 200, body: res._json };
118+
};
119+
query = async (body) => {
120+
const res = makeRes();
121+
// What the wire carries: JSON.
122+
await postRoute!.handler({ params: { object: OBJECT }, body: JSON.parse(JSON.stringify({ orderBy: [{ field: 'label', order: 'asc' }], ...body })), query: {}, headers: {} } as any, res);
123+
return { status: res._status ?? 200, body: res._json };
124+
};
125+
});
126+
127+
afterAll(async () => {
128+
try { await engine?.destroy(); } catch { /* noop */ }
129+
});
130+
131+
const labelsOf = (body: any): string[] => (body?.records ?? []).map((r: any) => r.label);
132+
133+
it('GET: 400 INVALID_REQUEST naming the querystring spelling — no read', async () => {
134+
const before = reads.n;
135+
for (const [name, q, param] of REFUSED_GET) {
136+
const res = await list(q);
137+
expect(res.status, `${name}: ${JSON.stringify(res.body)}`).toBe(400);
138+
expect(res.body.code, name).toBe('INVALID_REQUEST');
139+
expect(res.body.error, name).toContain(`Query parameter '${param}' must be a non-negative integer`);
140+
}
141+
expect(reads.n - before, 'no read of the object — every refusal precedes the driver').toBe(0);
142+
});
143+
144+
it('POST: 400 INVALID_REQUEST naming the body key — no read', async () => {
145+
const before = reads.n;
146+
for (const [name, body, param] of REFUSED_POST) {
147+
const res = await query(body);
148+
expect(res.status, `${name}: ${JSON.stringify(res.body)}`).toBe(400);
149+
expect(res.body.code, name).toBe('INVALID_REQUEST');
150+
expect(res.body.error, name).toContain(`Query parameter '${param}' must be a non-negative integer`);
151+
}
152+
expect(reads.n - before, 'no read of the object — every refusal precedes the driver').toBe(0);
153+
});
154+
155+
it('POST: a non-number window is still the schema door\'s 400 VALIDATION_FAILED, unchanged', async () => {
156+
const res = await query({ limit: 'ten' });
157+
expect(res.status, JSON.stringify(res.body)).toBe(400);
158+
expect(res.body.code).toBe('VALIDATION_FAILED');
159+
});
160+
161+
it('controls: a well-formed window pages exactly as before on both doors', async () => {
162+
const get = await list({ $top: '2', $skip: '1' });
163+
expect(get.status, JSON.stringify(get.body)).toBe(200);
164+
expect(labelsOf(get.body)).toEqual(['b', 'c']);
165+
166+
const post = await query({ limit: 2, offset: 1 });
167+
expect(post.status, JSON.stringify(post.body)).toBe(200);
168+
expect(labelsOf(post.body)).toEqual(['b', 'c']);
169+
170+
const bare = await list({ top: '2', skip: '3' });
171+
expect(labelsOf(bare.body)).toEqual(['d', 'e']);
172+
173+
const noWindow = await list({});
174+
expect(labelsOf(noWindow.body)).toEqual(LABELS);
175+
176+
// `0` is a count, not a malformed window: no rows, and still a 200.
177+
const zero = await query({ limit: 0 });
178+
expect(zero.status, JSON.stringify(zero.body)).toBe(200);
179+
expect(labelsOf(zero.body)).toEqual([]);
180+
const zeroSkip = await list({ $skip: '0', $top: '1' });
181+
expect(labelsOf(zeroSkip.body)).toEqual(['a']);
182+
});
183+
});

0 commit comments

Comments
 (0)