Skip to content

fix(rest): refuse a repeated single-valued query parameter instead of coercing it (#6877) - #7324

Merged
os-help merged 1 commit into
mainfrom
claude/issue-6877-rest-query-multiplicity
Aug 10, 2026
Merged

fix(rest): refuse a repeated single-valued query parameter instead of coercing it (#6877)#7324
os-help merged 1 commit into
mainfrom
claude/issue-6877-rest-query-multiplicity

Conversation

@os-help

@os-help os-help commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Fixes #6877

IHttpRequest.query is declared Record< string, string | string[] >
(packages/spec/src/contracts/http-server.ts). The array arm is not hypothetical:
NodeHttpServer hands ?x=1&x=2 through as ['1','2'], measured over a real socket
on #6878. rest-server.ts read its query parameters as if the union had one arm, so a
repeated parameter did not fail — it became a different value and was served with a
200.

tsc reported none of it, which is the card's sharpest observation: every site launders
the array through any, String() or Number(), so packages/rest's type-check DEBT
count reached 0 with this entire class still live.

Per the 2026-08-10 ruling on both threads, this lands before #6878's route 2. The
surface is dormant today only because the production Hono adapter collapses duplicates
before handlers run; route 2 removes that collapse. Landing this first makes route 2 a
safe two-point adapter change instead of switching ~50 read points live at once.

Re-derived inventory (merge base f758cec17, my own count — not the card's)

The card's map held up; the census drifted, as expected.

measure card (2026-08-09) this PR (merge base f758cec17)
req.query token occurrences in packages/rest/src, non-test, excl. package-routes.ts 61 "读取点" 74 occurrences / ~50 distinct read points
… in rest-server.ts 60 lines 72 occurrences, 62 lines
… in external-datasource-routes.ts 2 occurrences (1 read point, already guarded)
already typeof … === 'string'-guarded 9 9 (8 in rest-server.ts, 1 in external-datasource-routes.ts)

Command: grep -oP 'req\?\?\.query' per file (occurrences), read points counted by hand
from the contexts. Occurrences exceed read points because a guarded site spends two
(typeof req.query?.x === 'string' && req.query.x.toLowerCase()).

Result: 24 gate sites covering 63 declared single-valued parameter slots, plus 8
parameters deliberately classified multi-valued and 3 read points deliberately left
alone with the reason recorded in the code.

Per-parameter classification

Classified from what the consumer does with the value, never from the name — the
card explicitly flagged select / expand / objects as suspects, and measurement
moved two of them off the defect list.

Declared SINGLE-valued (gated)

route parameters measured outcome on ['a','b'] before this PR
serveMetaItemLayered (both entry points) package array reached getMetaItemLayered({ packageId })
GET /meta/diagnostics severity, type, package array past an as string cast into getMetaDiagnostics
GET /meta/_drafts packageId, type array reached listDrafts
GET /meta/:type package, preview, object, include String(['a','b']) → view named 'a,b' → switcher silently empty; repeated include stopped equalling 'content' → doc bodies stripped, 200
GET /meta/book/:name/tree package array reached getMetaItems
GET /meta/:type/:name layers, state, preview, package comparisons stopped matching → published-world answer for a draft read
PUT /meta/:type/:name force, package, mode inversion — see below
DELETE /meta/:type/:name state, dropStorage destructive opt-in stopped matching
GET /meta/:type/:name/history sinceSeq, limit Number()NaNisFinite drops the bound → the UNLIMITED history
GET /meta/:type/:name/audit limit same
POST /meta/:type/:name/rollback toVersion already 400, but as INVALID_REQUEST "'toVersion' … is required" — wrong diagnosis
GET /meta/:type/:name/diff from, fromVersion, to, toVersion bound dropped → a different version pair diffed, 200
GET /meta/:type/:section/:name package array reached getMetaItem
PUT /meta/:type/:section/:name package guard dropped it to undefined → the save LANDED somewhere else (env-local overlay)
DELETE /data/:object/:id expectedVersion String()'1,2' → guaranteed OCC conflict on a destructive verb
GET /data/import/jobs object, status, limit, offset filter silently dropped; page bound reset to default
GET /data/:object/export format, header, limit, page, filter, search, orderby inversion (limit) — see below; filter array passed typeof … === 'object' and was handed to findData as the filter
GET /search q, query, limit, perObject String() → the literal term 'a,b'
GET /public-forms/** lookup picker q same
POST /analytics/dataset/query preview stopped equalling 'draft' → Studio preview silently ran on PUBLISHED rows
GET /sharing/rules object, activeOnly array to listRules; activeOnly stopped matching → inactive rules listed too
GET /security/suggested-bindings status, packageId String() join → filtered on a status nothing has
GET /reports object, ownerId arrays reached listReports
GET /approvals/requests object, recordId, record_id, status, submitterId, submitter_id, q, limit, offset mixed: passthrough, typeof drop, Number()NaN

The two inversions — not degradations, wrong answers in the opposite direction:

PUT /meta/:type/:name?force=false&force=false
  the read is:  typeof forceRaw === 'string' ? [...] : Boolean(forceRaw)
  and Boolean(['false','false']) is TRUE   → the destructive-change guard was
                                             switched OFF by a caller repeating
                                             an explicit OPT-OUT

GET /data/:object/export?limit=1&limit=2
  the read is:  q.limit != null ? Math.max(1, Number(q.limit) || 0) : 10_000
  Number([...]) is NaN → NaN || 0 is 0 → Math.max(1, 0) is 1
                                         → a ONE-ROW export, 200 OK, nothing
                                           to indicate it

Genuinely MULTI-valued (deliberately NOT gated) — measured, not assumed

route parameter evidence
GET /data/:object/:id select, expand GetDataRequest.select / .expand are z.array(z.string()) (spec/src/api/protocol.zod.ts), and metadata-protocol's getData signature is { …, expand?: string | string[], select?: string | string[] } — it splits the comma form itself and passes an array straight through. ?select=a&select=b was already correct end to end. The card listed this line under "array flows downstream"; measurement says the downstream was built for it.
GET /search objects the handler's own next line is Array.isArray(objectsParam) ? objectsParam : …
GET /data/:object/export fields, searchFields both already branch on Array.isArray(q.fields); export columns are genuinely a list
GET /approvals/requests approverId, approver_id the handler's comment says it: "accepts a single id, a comma-separated list, or the param repeated (→ array)"

Flattening or refusing any of these would have been the real regression. Each is pinned
by a test so a later "completion" of the pattern fails loudly.

Left alone, with the reason recorded in the code

read point why
extractLocale's ?locale= already typeof-guarded; the helper is shared by ~10 routes and has no res to refuse through, and the worst case is falling back to the default locale. Converted to a comment explaining the asymmetry rather than plumbing res through ten call sites.
GET /security/explain's src.* already safe structurally, not by luck: every field goes through ExplainRequestSchema.safeParse, whose members are z.string(), so an array is refused 400 VALIDATION_FAILED by the schema itself. A schema at the boundary is what the gate imitates.
GET /data/:object (query: req.query) out of scope, filed as #7321. This route hands the whole query record to findData, whose normalizer in packages/metadata-protocol owns every parameter's arity — the $-alias table, the implicit field-equality bucket, and $select / $expand / $searchFields, which are legitimately multi-valued. Declaring an arity list here would be this package guessing at another's contract. One measured line in that normalizer: if (options.limit != null) options.limit = Number(options.limit);?$top=1&$top=2 reaches the driver as limit: NaN.
external-datasource-routes.ts's ?schema= already typeof-guarded, degrades to undefined, single read point in a module check:route-envelope pins at zero hand-built responses.

The fix shape

readSingleQueryValue / repeatedQueryParamMessage moved out of package-routes.ts
(where #6307 landed them) into a new packages/rest/src/query-multiplicity.ts, so there
is one rule and one message rather than a second implementation free to drift.
package-routes.ts imports them; its behaviour is unchanged.

The new refuseRepeatedQueryParams(req, res, names) is what handlers open with. It:

  1. refuses the first repeated declared parameter with 400 and the ADR-0112 nested
    envelope { error: { code: 'VALIDATION_ERROR', message } } — the position PR fix(rest): put the /meta 501 refusals inside the ADR-0112 error envelope (#7035) #7293
    (finding: rest-server.ts 里三个相邻 /meta handler 的错误信封是三种不同形状,其中两种不符合 ADR-0112 #7035) just converged this file's /meta 501 refusals onto, matched deliberately;
  2. normalises a one-element array in place. The rule counts occurrences, so ['a']
    is one supply an adapter chose to encode as an array. Accepting it without unwrapping
    would leave the array for the very String() / Number() / truthy reads this exists
    to protect — the acceptance would be a hole, not a courtesy. A well-formed
    single-value request carries a string and is not touched at all, which is what
    makes preservation byte-identical.

No new error code: VALIDATION_ERROR is already the standard catalog's member for 400
(spec/src/api/errors.zod.ts, standardErrorCodeForHttpStatus(400)) and the code #6307
chose for this same condition. packages/spec is untouched, as ruled. The Hono
adapter and packages/qa/http-conformance are untouched — those are #6878's route 2.

Tests — 33 cases, in four parts

packages/rest/src/rest-server-query-multiplicity.test.ts.

Every refusal case asserts the ADR-0112 pairstatus AND the nested
body.error.code — plus the exact message. A bare toThrow() would be worthless here
and the file says why: these handlers send, they never throw, and on the unfixed code
every one of these requests answered 200, so a throw-shaped assertion would report
"the promise resolved" and could not separate "refused with the wrong envelope" from
"did not refuse at all".

Reverse verification — direction predicted first

Prediction: removing the gate calls from rest-server.ts (keeping the helper module and
the tests) turns the §1 refusal cases red and leaves §2 preservation, §3
multi-valued and §4 helper-unit cases green. Removed with
git checkout origin/main -- packages/rest/src/rest-server.ts (never git stash
shared stack).

Measured: 18 failed | 15 passed, and the 18 are exactly the predicted set —

× PUT /meta/:type/:name?force  … expected a 400 refusal for a repeated "force",
                                 got 200 with body {"success":true}
× GET /data/:object/export?limit … got 200
× GET /meta/:type?object       … got 200 with body {"items":[]}
… 13 more, every single one answering 200
× POST …/rollback?toVersion    … expected undefined to be 'VALIDATION_ERROR'
                                 (status already 400 — the pre-existing
                                  INVALID_REQUEST, i.e. the wrong diagnosis,
                                  exactly as the case's comment predicts)
× a ONE-element array is one occurrence  … packageId: ['com.acme'] not 'com.acme'
× an EMPTY array is no occurrence        … packageId: [] not undefined

The last two are preservation-shaped but belong to the refusal side: they pin the
normalisation, which is part of the fix. Every case that pins genuinely unchanged
behaviour stayed green with the fix removed — that is the byte-identical evidence, and
it is stronger than the argument would have been.

Restored with git apply of the saved patch; all 33 green again.

Verification

pnpm --filter '@objectstack/rest^...' build          # build closure first
pnpm --filter @objectstack/rest test                 # 78 files, 1261 tests, ALL PASS
npx tsc --noEmit -p packages/rest/tsconfig.json      # clean
node scripts/check-nul-bytes.mjs                     # OK, 6675 files
node scripts/check-route-envelope.mjs                # OK (8 modules; rest-server.ts is
                                                     #   not a *-routes.ts, query-
                                                     #   multiplicity.ts correctly not scanned)
node scripts/check-error-code-casing.mjs             # OK, 3393 files
node scripts/check-type-check-coverage.mjs           # OK

TEST_DEBT['@objectstack/rest'] re-measured the way the gate does it (a sibling
extends config with the test globs dropped from exclude, after
turbo run build --filter '@objectstack/rest...'): 155 errors — exactly the recorded
ceiling, zero margin consumed, and zero of them in either new file.
The .js
extensions are on both new imports (TS2835 trap, #7248).

Wire-visible

Requests that used to receive a wrong 200 now receive a 400. No well-formed
single-value request changes in any way. Changeset added
(.changeset/rest-query-param-multiplicity.md, patch).

Out of scope, filed


Generated by Claude Code

… coercing it (#6877)

`IHttpRequest.query` is `Record< string, string | string[] >` and the array arm
is produced by a real first-party adapter (`NodeHttpServer`, measured over a
socket on #6878). `rest-server.ts` read ~50 of its query parameters as if the
union had one arm, so a repeated parameter became a DIFFERENT value and was
served with a 200. `tsc` reported none of it: every site launders the array
through `any`, `String()` or `Number()`, which is why this package's DEBT count
reached 0 with the whole class still live.

Two outcomes were inversions rather than degradations:

  ?force=false&force=false  → `!!['false','false']` is true, so repeating an
                              explicit OPT-OUT switched the destructive-change
                              guard ON
  ?limit=1&limit=2 (export) → Number([...]) is NaN, NaN || 0 is 0,
                              Math.max(1, 0) is 1 → a ONE-ROW export, 200 OK

Each handler now declares which of its parameters are single-valued and refuses
a repeat with 400 + the ADR-0112 nested `{ error: { code, message } }` envelope
(#7035's shape, #6307's rule and message — now shared in
`query-multiplicity.ts` rather than duplicated). The rule counts occurrences,
not values.

Genuinely multi-valued parameters are untouched and pinned: `select`/`expand`
(whose consumer takes `string | string[]` by design), `objects`, `fields`,
`searchFields`, `approverId`. `GET /data/:object` is deliberately ungated — its
arity is the `findData` normalizer's contract, filed as #7321.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0158ZQo7LiHSxGWpYKuPq1wu
@vercel

vercel Bot commented Aug 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 10, 2026 6:42am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest.

9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/ai/connect-mcp.mdx (via @objectstack/rest)
  • content/docs/api/error-handling-server.mdx (via @objectstack/rest)
  • content/docs/api/index.mdx (via @objectstack/rest)
  • content/docs/permissions/authentication.mdx (via @objectstack/rest)
  • content/docs/permissions/system-context.mdx (via packages/rest)
  • content/docs/plugins/index.mdx (via @objectstack/rest)
  • content/docs/plugins/packages.mdx (via @objectstack/rest)
  • content/docs/protocol/kernel/http-protocol.mdx (via @objectstack/rest)
  • content/docs/protocol/kernel/i18n-standard.mdx (via packages/rest)

3 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx (via @objectstack/rest)
  • content/docs/releases/v12.mdx (via @objectstack/rest)
  • content/docs/releases/v17.mdx (via @objectstack/rest)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Aug 10, 2026
@os-help
os-help marked this pull request as ready for review August 10, 2026 06:56
@os-help
os-help added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit f3f855a Aug 10, 2026
27 checks passed
@os-help
os-help deleted the claude/issue-6877-rest-query-multiplicity branch August 10, 2026 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

packages/rest 的其它 req.query.* 读取点同样把 string | string[] 当字符串用(#6307 的未扩大部分)

2 participants