Skip to content

Data API paging: a negative $skip/offset answers 500, and a non-numeric or negative $top/limit is silently ignored #25229

Closed

Description

@os-sales

QA-source: #25227 · api-backend.query-contract-matrix · observed beside the clauses

Found by an off-clause boundary probe during the 18.1.0 P0 run; re-measured on a second boot. Not a regression: the same coercion ships in the published @objectstack/metadata-protocol@18.0.0.

Reproduction (stock showcase, admin bearer)

request answer expected
GET /api/v1/data/showcase_account?$skip=-5 500 {"error":"Internal server error","code":"INTERNAL_ERROR"} 400 naming $skip
GET /api/v1/data/showcase_account?$skip=-1&$top=3 500 INTERNAL_ERROR 400
POST /api/v1/data/showcase_account/query {"offset":-5} 500 INTERNAL_ERROR 400 naming offset
GET /api/v1/data/showcase_account?$top=abc 200, all 14 rows (as if no $top) 400 naming $top
GET /api/v1/data/showcase_account?$top=-1 200, all 14 rows 400
POST /api/v1/data/showcase_account/query {"limit":-1} 200, all 14 rows 400
GET /api/v1/data/showcase_account?$top= 200, 0 rows (Number('') is 0) 400, or treated as absent
POST /api/v1/data/showcase_account/query {"limit":"ten"} 400 VALIDATION_FAILED query.limit (correct — the two doors disagree)

Server log on the 500s: [REST] Unhandled error: Error: A non-negative integer must be provided to offset. (knex QueryBuilder.offset, from driver-sql).

Mechanism

packages/metadata-protocol/src/protocol.ts (the list-query normalizer, around the [#3795] slot fold) coerces with options.limit = Number(options.limit) and options.offset = Number(options.offset) and never range-checks: NaN reaches the driver and is dropped (the window is ignored), a negative limit is dropped the same way, and a negative offset reaches knex and throws. The query schema's offset: z.number() has no .nonnegative(), so the POST door passes a negative offset too.

This is the silent-coercion class of #7321 (limit: NaN): a malformed window answers as if no window was sent. The filter is never widened, only the page window.

Suggested fix

Validate the coerced limit/offset (and their $top/$skip/top spellings) as non-negative integers on both doors and refuse otherwise with a 400 that names the parameter the caller wrote (the #4226 spelling discipline), plus a conformance test for $skip=-1, $top=abc, $top=-1 and the empty $top=.

Activity

  1. objectstack-fleet commented on Oct 22, 2026

    @objectstack-fleet
    Contributor

    Claim: PM loop (card thread) · 2026-10-22T15:22Z · p2, bug, domain:engine. Data API paging: a malformed or negative $top/$skip/limit/offset is refused with a 400 naming the caller's spelling on both doors, instead of a 500 or a silently ignored window.
    Session: session_01A3FoRXqj2EgAzPQbXU3Kzh (card thread of domain:engine seat 2, seat session session_01FaVyM6YvQLg9CsjPVCYc5K)
    Account: os-bill
    Branch: claude/project-thread-6i4obw (from origin/main; a cloud thread can push only its designated branch)
    Worktree: the card thread's own checkout
    Domain: domain:engine
    Seat: domain:engine#2 (seat post #20966)
    File surface: packages/metadata-protocol/src/protocol.ts (the list-query normalizer) and its tests; packages/spec query schema only if the measurement requires it (cross-lane, declared on #18549); the REST door in packages/rest only if the shared normalizer cannot see the caller's spelling (declared to its lane); a conformance test; changesets.
    (stop on breach; explain in the report)
    ⛔ Not in scope: driver-sql's knex call (the window never reaches it malformed once refused upstream); any other query parameter.
    Container & model: M, mode:thread, model: TIER_DEFAULT, effort: high. Review: the seat's TIER_DEFAULT review of record; CONTRACT_REVIEW_TIER if a published spec schema narrows.
    Clause-②: no (narrowing) (provisional; the dev confirms from the diff)
    Metadata-protocol: none (provisional)
    Responsibility: the list-query normalizer coerces the window with no range check | the POST body schema checks the type but not the sign | every Data API client that pages
    Thread-read: none
    Serial constraints cleared: open PRs on the normalizer re-checked at claim time.


    Generated by Claude Code

  2. objectstack-fleet commented on Oct 22, 2026

    @objectstack-fleet
    Contributor

    os-dev-report

    {
      "issue": 25229,
      "status": "done",
      "branch": "claude/project-thread-6i4obw",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/25260",
      "session": "session_01A3FoRXqj2EgAzPQbXU3Kzh",
      "premise_still_valid": true,
      "summary": "Every row of the card's table reproduced on main 345f0b154c over a stock showcase boot, plus $skip=abc (200, all rows), offset 1.5 (200, 13 rows), limit 1.5 (200, 1 row), limit 0 (200, 0 rows) and $skip= (200, all rows). findData's list-query normalizer is the one statement GET /data/:object, POST /data/:object/query and the runtime dispatcher all reach with the window unvalidated, so the bare Number() coercion there became pageWindowValue: a non-negative safe integer, or 400 INVALID_REQUEST with param set to the caller's spelling ($top/top/limit/$skip/skip/offset), before any read. A blank value ($top=, $skip=) is refused, not read as absent: an absent window means every row, so a blank one read that way is the silent widening this card closes. 0 stays valid. No packages/spec or packages/rest door change was needed; the POST schema's VALIDATION_FAILED for limit 'ten' is unchanged.",
      "tests": "New packages/metadata-protocol/src/protocol.page-window.test.ts: 25/25 (17 refusals asserting code+status+param+first clause with engine.find never called, 1 bounded-quote case, 7 preservation cases). New packages/rest/src/data-page-window-door.test.ts over a real SqlDriver (sqlite): 4/4 (11 GET and 4 POST refusals with zero reads, VALIDATION_FAILED unchanged, controls page as before). Ablation via scripts/ablation-replace.mjs run mode, the check replaced by the base's bare Number(value), metadata-protocol rebuilt, ablation-dist-preflight marker hit in dist/index.js and dist/index.cjs: unit 18 failed / 7 passed, door 2 failed / 2 passed. Restore: blob == HEAD, git diff HEAD empty, rebuilt, preflight --absent over 22 built files, 25/25 and 4/4 green. Full package runs after building the @objectstack/rest^... closure: metadata-protocol test 257 files passed 3 skipped, 20893 tests passed 19 skipped; rest test 316 files, 6326 passed 326 skipped; typecheck metadata-protocol exit 0, rest exit 0. Live showcase re-measure on the fix: every refused row 400 naming the spelling, controls 200 with 5 rows.",
      "gates": "dispatch-gates --commands at 8845dd244: 68 derived, all exit 0; --ran reconciliation 68 run, 0 NOT-MEASURED, 0 UNRUN. Six were rerun for local causes outside the diff and passed on the rerun: check-issue-citations (board read failed once), check-plugin-teardown-shape --self-test (shallow clone, pinned fixture commit fetched), check:dts-closure and check:dual-build-cjs-loads (ran during a concurrent declaration rebuild), check:query-options-erasure and check:type-check-debt (300 s local timeout). PR check-runs at open: 6 success, 2 skipped, 16 in_progress; not waited on.",
      "dogfood_battery": "8845dd244 · os-verify-lock VERDICT command-exit 0 · Test Files 287 passed, 3 skipped (290) · Tests 2398 passed, 18 skipped (2416) · no failures",
      "mcp_calls": "1, subscribe_pr_activity (session PR watch, not a GitHub write); 0 MCP GitHub calls",
      "api_writes": "3 through the fleet-write relay and post-stamped: POST /repos/objectstack-ai/objectstack/pulls (draft #25260), POST /repos/objectstack-ai/objectstack/issues/25260/assignees (os-bill), POST /repos/objectstack-ai/objectstack/issues/25229/comments (this report). Plus git push of the designated branch.",
      "deviations": [
        "Branch is the platform-designated claude/project-thread-6i4obw, as the claim records.",
        "The PR body's attribution line links this card thread, as the session's required attribution block gives it, not the seat thread link the order quoted.",
        "The conformance test lives in packages/rest as a test file only; no REST door source changed.",
        "Not re-merged with origin/main (9 commits ahead, none touching metadata-protocol, rest or the runtime data domain; local merge-tree clean), so the dogfood head stays the PR head."
      ],
      "files_changed": [
        "packages/metadata-protocol/src/protocol.ts",
        "packages/metadata-protocol/src/protocol.page-window.test.ts",
        "packages/rest/src/data-page-window-door.test.ts",
        ".changeset/25229-page-window-refused.md"
      ],
      "line_budget": "protocol.ts +55/-2; protocol.page-window.test.ts +140; data-page-window-door.test.ts +194; changeset +18",
      "clause2": "no (narrowing): nothing widens and no surface grows; requests answered 200 or 500 before are refused 400, so a published accept set narrows. The changeset carries the BREAKING banner and ADR-0087 not-required.",
      "metadata_protocol": "none: a query window is a request parameter, not authored metadata; changeset level patch.",
      "open_questions": [
        { "question": "Blank ?$skip= answered the right rows before (Number('') is 0, the same as no offset) and is now refused for symmetry with ?$top=. Keep it refused?", "options": ["A refuse both blanks (this PR)", "B refuse blank $top only, read blank $skip as absent"], "recommendation": "A, because one rule per slot pair is predictable and a blank value is a malformed request on either slot; B leaves the two window slots disagreeing on the same input." }
      ],
      "out_of_scope_findings": [
        "carrier: none · a direct in-process engine.find with offset -1, which skips findData, still meets knex's throw; this is not a Data API door and is noted in Acceptance notes only."
      ]
    }

    Delta after the seat's review FAIL (record 6346702112): head 90e4e4d adds one commit, so 6 files change, not 4. The two new ones are packages/rest/src/data-action-routes.ts (the export chunk is Math.trunced where ?page= is read, so ?page=100.5 no longer hands findData a fractional limit) and packages/rest/src/rest-server-query-number-census.test.ts (a 100.5 row plus a whole-count pin on every limit the stub receives; ablation 2 red, restore 16/16). On the new head: 69/69 derived gates exit 0, rest typecheck exit 0. The PR body was corrected through the relay at the seat's request.

    Report taken 2026-10-22T16:59Z.

  3. objectstack-fleet commented on Oct 22, 2026

    @objectstack-fleet
    Contributor

    ACCEPT → PR #25260 comment 6347650135 (contract review PASS 6347561377 on 90e4e4d69, after FAIL 6346702112). Ready and auto-merge follow once every check on that head is green.

  4. objectstack-fleet commented on Oct 22, 2026

    @objectstack-fleet
    Contributor

    Landed · domain:engine seat 2 (seat post #20966) · os-bill · session_01FaVyM6YvQLg9CsjPVCYc5K · 2026-10-22T18:34Z

    Probe Reading
    PR #25260 (Fixes #25229) merged: true at 2026-10-22T18:33Z, merge commit 17f3c65a1, through the merge queue
    origin/main 17f3c65a1 fix(metadata-protocol)!: the Data API refuses a malformed or negative page window with a 400 naming the parameter as written (#25260)
    queue branch gh-readonly-queue/main/pr-25260-* gone; head branch claude/project-thread-6i4obw deleted
    card closed completed by the PR

    Every list door now reaches one window normalizer: a malformed, fractional or negative $top / $skip / limit / offset / ?page= answers 400 naming the parameter as written. Follow-up: the published query schemas still declare limit / offset as plain numbers (declared ≠ enforced) — a domain:spec card the seat files next. State: pm:dispatched removed in this act.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area:apiThe API a customer can call, and integrations — REST, connectors, webhooks, jobsbugSomething isn't workingdomain:enginepriority:p2Medium: important, M3

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions