fix(metadata-protocol)!: the Data API refuses a malformed or negative page window with a 400 naming the parameter as written

.changeset/25229-page-window-refused.md

@@ -0,0 +1,18 @@
+---
+'@objectstack/metadata-protocol': patch
+---
+
+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
+
+Clause-②: no (narrowing)
+Metadata-protocol: none
+
+<!-- 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. -->
+
+**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.
+
+- **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.
+- **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.
+- **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`.
+
+**The remedy.** Send the window as a whole count of zero or more, or leave the parameter out.

packages/metadata-protocol/src/protocol.page-window.test.ts

@@ -0,0 +1,140 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #25229 — `findData`'s list-query normalizer coerced the page window with a
+ * bare `Number(...)` and no range check.
+ *
+ * Every list door reaches that one statement with the window unvalidated:
+ * `GET /data/:object` hands its querystring over as strings, `POST
+ * /data/:object/query`'s body schema checks that `limit` / `offset` are
+ * numbers but not their sign, and the runtime dispatcher forwards both. So a
+ * `NaN` (`?$top=abc`) or a negative limit reached the driver, which drops a
+ * window it cannot read — every row, under a 200 — and a negative offset
+ * reached knex's `QueryBuilder.offset`, which throws: a 500.
+ *
+ * ## Two assertion classes
+ *
+ *  1. **REFUSAL** — each malformed window, in every spelling, answers `400`
+ *     `INVALID_REQUEST` naming the spelling the caller wrote, and the engine is
+ *     never reached. `code` AND `status` are asserted: the unfixed normalizer
+ *     never threw for these, so a bare `toThrow()` would only be half a pin.
+ *  2. **PRESERVATION** — a well-formed window reaches `engine.find` as the
+ *     integers it always did, `0` included. Green in both directions, so a
+ *     guard, not evidence: it is what a refusal is cheapest to break.
+ *
+ * The door-level half (both REST routes over a real `SqlDriver`) is
+ * `packages/rest/src/data-page-window-door.test.ts`.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { ObjectStackProtocolImplementation } from './protocol.js';
+
+const SCHEMA = {
+    name: 'invoice',
+    nameField: 'name',
+    fields: {
+        name: { name: 'name', type: 'text' },
+        status: { name: 'status', type: 'text' },
+    },
+};
+
+function makeProtocol() {
+    const find = vi.fn(async () => [] as unknown[]);
+    const engine = {
+        registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) },
+        find,
+        aggregate: vi.fn(async () => [] as unknown[]),
+        count: vi.fn(async () => 0),
+    };
+    return { p: new ObjectStackProtocolImplementation(engine as any), find };
+}
+
+/** The option bag `engine.find` was actually handed, for an accepted query. */
+async function optionsFor(query: Record<string, unknown>): Promise<Record<string, unknown>> {
+    const { p, find } = makeProtocol();
+    await p.findData({ object: 'invoice', query } as never);
+    expect(find, `${JSON.stringify(query)} never reached engine.find`).toHaveBeenCalledTimes(1);
+    return (find.mock.calls[0] as unknown[])[1] as Record<string, unknown>;
+}
+
+/** The refusal a rejected query produced, plus proof the engine was not reached. */
+async function refusalFor(query: Record<string, unknown>): Promise<{
+    message: string; status?: number; code?: string; param?: string;
+}> {
+    const { p, find } = makeProtocol();
+    let answered: unknown;
+    try {
+        answered = await p.findData({ object: 'invoice', query } as never);
+    } catch (e) {
+        const err = e as Error & { status?: number; code?: string; param?: string };
+        expect(find, 'the engine was reached before the refusal').not.toHaveBeenCalled();
+        return { message: err.message, status: err.status, code: err.code, param: err.param };
+    }
+    throw new Error(
+        `${JSON.stringify(query)} was ACCEPTED (answered ${JSON.stringify(answered)}) instead of refused`,
+    );
+}
+
+// ---------------------------------------------------------------------------
+// 1. REFUSAL
+// ---------------------------------------------------------------------------
+
+describe('#25229 — a malformed page window is refused, naming the spelling the caller wrote', () => {
+    it.each<[string, Record<string, unknown>, string]>([
+        // [wire spelling, the query, what it used to become]
+        ['$skip', { $skip: '-5' }, 'a knex throw, answered 500'],
+        ['$skip', { $skip: '-1', $top: '3' }, 'a knex throw, answered 500'],
+        ['$skip', { $skip: 'abc' }, 'offset: NaN'],
+        ['skip', { skip: '-1' }, 'a knex throw, answered 500'],
+        ['offset', { offset: -5 }, 'a knex throw, answered 500'],
+        ['offset', { offset: 1.5 }, 'a fractional offset'],
+        ['offset', { offset: '' }, 'offset: 0'],
+        ['$top', { $top: 'abc' }, 'limit: NaN, every row'],
+        ['$top', { $top: '-1' }, 'a dropped window, every row'],
+        ['$top', { $top: '' }, 'limit: 0, an empty page'],
+        ['$top', { $top: '   ' }, 'limit: 0, an empty page'],
+        ['$top', { $top: '1.5' }, 'a fractional limit'],
+        ['top', { top: 'abc' }, 'limit: NaN, every row'],
+        ['limit', { limit: -1 }, 'a dropped window, every row'],
+        ['limit', { limit: 'ten' }, 'limit: NaN, every row'],
+        ['limit', { limit: true }, 'limit: 1'],
+        ['limit', { limit: Number.POSITIVE_INFINITY }, 'an unbounded limit'],
+    ])('refuses %s in %j with 400 INVALID_REQUEST (was: %s)', async (param, query) => {
+        const err = await refusalFor(query);
+
+        expect(err.code).toBe('INVALID_REQUEST');
+        expect(err.status).toBe(400);
+        expect(err.param).toBe(param);
+        expect(err.message).toContain(`Query parameter '${param}' must be a non-negative integer`);
+    });
+
+    it('quotes the value it refused, bounded', async () => {
+        const err = await refusalFor({ $top: 'x'.repeat(500) });
+
+        expect(err.message).toContain('got "xxxx');
+        expect(err.message.length).toBeLessThan(400);
+    });
+});
+
+// ---------------------------------------------------------------------------
+// 2. PRESERVATION
+// ---------------------------------------------------------------------------
+
+describe('#25229 — a well-formed page window reaches the engine unchanged', () => {
+    it.each<[Record<string, unknown>, number | undefined, number | undefined]>([
+        [{ $top: '5', $skip: '2' }, 5, 2],
+        [{ top: '5', skip: '2' }, 5, 2],
+        [{ limit: 5, offset: 2 }, 5, 2],
+        [{ limit: '5', offset: '2' }, 5, 2],
+        [{ $top: '0' }, 0, undefined],
+        [{ $skip: '0' }, undefined, 0],
+        [{}, undefined, undefined],
+    ])('%j → limit %s, offset %s', async (query, limit, offset) => {
+        const options = await optionsFor(query);
+
+        expect(options.limit).toBe(limit);
+        expect(options.offset).toBe(offset);
+        // No window spelling leaks into the implicit-filter bucket.
+        expect(options.where).toBeUndefined();
+    });
+});

packages/metadata-protocol/src/protocol.ts

@@ -4751,6 +4751,52 @@ function repeatedQueryParamError(param: string, count: number): Error {
     return err;
 }
 
+/**
+ * [#25229] The page window (`limit` / `offset`, spelled `$top` / `top` /
+ * `$skip` / `skip` on the wire) as the non-negative integer every driver
+ * needs, or a refusal naming the parameter the caller wrote (#4226).
+ *
+ * The bare `Number(...)` this replaces failed three ways, none of them loud:
+ *
+ *  - `NaN` (`?$top=abc`) and a negative limit (`?$top=-1`) reached the driver,
+ *    which drops a window it cannot read — every row answered under a 200, the
+ *    #7321 class (a malformed window served as if no window was sent);
+ *  - a negative offset (`?$skip=-1`, `{"offset": -1}`) reached knex's
+ *    `QueryBuilder.offset`, which throws — a 500 for a caller's mistake;
+ *  - a blank value (`?$top=`) is `Number('') === 0`, an empty page nobody
+ *    asked for.
+ *
+ * A blank value is refused, not read as absent. The blank `?filter=` is
+ * absent because an empty filter and no filter mean the same thing; an empty
+ * window does not — absent means "every row", so reading `?$top=` that way is
+ * the silent widening above under another name, and reading it as `0` is the
+ * guess this replaces. `0` itself stays valid: `$top=0` asks for the total
+ * alone.
+ *
+ * Only the two arms a door can deliver are read: a number (a JSON body) and a
+ * string (a querystring). Anything else — `true`, which `Number` would quietly
+ * make `1` — is refused like any other value that is not a count.
+ */
+function pageWindowValue(value: unknown, param: string): number {
+    const n = typeof value === 'number'
+        ? value
+        : typeof value === 'string' && value.trim() !== ''
+            ? Number(value)
+            : Number.NaN;
+    if (Number.isSafeInteger(n) && n >= 0) return n;
+    // `String` for a number: `JSON.stringify(Infinity)` is `null`.
+    const shown = typeof value === 'number' ? String(value) : JSON.stringify(value) ?? String(value);
+    const err: any = new Error(
+        `Query parameter '${param}' must be a non-negative integer; got `
+        + `${shown.length > 64 ? `${shown.slice(0, 64)}…` : shown}. The query was not run: a window `
+        + 'the driver cannot read is either dropped, answering every row, or rejected by the database.',
+    );
+    err.status = 400;
+    err.code = 'INVALID_REQUEST';
+    err.param = param;
+    throw err;
+}
+
 /**
  * [#7321] Refuse a repeated occurrence of any list-query parameter this
  * normalizer reads as single-valued, and normalise the benign one-element array
@@ -14624,8 +14670,15 @@ export class ObjectStackProtocolImplementation implements
         // (`options.limit = Number(options.top)` — the alias overwrote the
         // canonical key, so `{top: 1, limit: 3}` answered 1 over HTTP and 3
         // through a direct engine call).
-        if (options.limit != null) options.limit = Number(options.limit);
-        if (options.offset != null) options.offset = Number(options.offset);
+        //
+        // [#25229] The coercion is CHECKED, not bare (see
+        // {@link pageWindowValue}). This is the one statement every list door
+        // reaches with the window still unvalidated — `GET /data/:object`'s
+        // querystring, `POST /data/:object/query`'s body (whose schema checks
+        // the type, not the sign) and the runtime dispatcher — so the refusal
+        // lives here, once.
+        if (options.limit != null) options.limit = pageWindowValue(options.limit, slotParam('limit'));
+        if (options.offset != null) options.offset = pageWindowValue(options.offset, slotParam('offset'));
 
         // Projection: comma-separated string → array. A single-value
         // querystring param arrives as a bare string — `?fields=name` or the

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

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