Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions .changeset/closed-query-param-ingress-policy.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
---
"@objectstack/rest": minor
---

feat(rest): closed query-parameter sets become REST ingress policy, starting with the first tier of data read routes (#7606)

**BREAKING** for tolerated traffic, and deliberately so — see the last section.

## The condition

`rest-server.ts` handlers read the query keys they know and ignore the
remainder, so a misspelled, renamed or invented parameter is **silently
dropped** and the caller gets a plausible-looking `200`. The failure is
undetectable from the response in both directions:

- it **silently widens** — a dropped `?objects=` fans a search across every
object; a dropped `?fields=` returns the whole record. An unfiltered result
is shaped exactly like a genuinely broad match.
- it **silently narrows** — a dropped key inside a filter answers `200` with
zero rows, which is shaped exactly like an object that really is empty.

There is no status, header or field that distinguishes either from a real
answer, which is what makes it worth a policy rather than a bug per endpoint.
An AI caller can detect neither direction at all.

## The policy

A REST route **declares its closed query-parameter set on the day it lands**,
refusing an unrecognised name with a located `400` instead of dropping it.
Adoption is incremental and per lane — data READ routes first — never a
one-shot sweep. The rule, its three measuring constraints and the exclusions
are written up in `packages/rest/src/query-allowlist.ts` and in AGENTS.md's
"Route & surface ownership" section, so it is enforceable at review time.

## The first tier

Three routes, each set **measured from the handler's own read points**:

| route | closed set |
| :--- | :--- |
| `GET /data/:object/:id` | `select`, `expand` |
| `GET /data/:object/export` | `format`, `header`, `limit`, `page`, `filter`, `search`, `searchFields`, `orderby`, `fields`, `locale` |
| `GET /search` | `q`, `query`, `objects`, `limit`, `perObject` |

The refusal is `400` with the ADR-0112 nested body
`{ error: { code: 'VALIDATION_ERROR', message } }` — the same envelope these
routes' existing multiplicity refusals answer, so no route gains a second
dialect. The message names the parameters that were not understood **and lists
the ones that are**, so a caller can fix the request from the response alone.

## What is deliberately NOT closed

`GET /data/:object` (the record list) keeps accepting any name. Its handler
passes the whole query to the normalizer, which lowers every leftover key into
an implicit field-equality predicate — `?status=open` *is* the filter — so the
valid names are the object's own fields and vary per object. That route is
already guarded one layer down and against the right authority: an unknown
**field** is refused there with `400 INVALID_FIELD`. Closing it here would
break every implicit filter.

Its repeated-`?filter=` refusal (`400 INVALID_FILTER`) is untouched, and since
the recognition gate never runs on that route the two guards never meet on one
request.

## Breaking tolerated traffic is the point, and v17 is the window

A caller sending one of these routes a parameter we ignore today starts getting
a `400`. That is not a side effect — it is the change. This is **not a pure bug
fix**: the blast radius cannot be measured from our side, precisely because we
have been dropping the traffic silently, so it was decided rather than
measured (maintainer ruling, 2026-08-12). v17 is the intended window; the
longer it waits the more tolerated traffic there is to break.

Two callers most likely to notice, both on `GET /data/:object/:id`: `?fields=`
and `?populate=` are refused. They are the spec's canonical/alias spellings for
slots this route reads as `select` and `expand`, and it folds no aliases — so
they were being dropped, silently returning the full record. They are left
outside the closed set rather than implemented, because adding them would
advertise a capability the handler does not have; the refusal message names
`select` and `expand` as what the route does accept.

<!-- adr-0087: not-required (no-migration-prescription) HTTP query-string ingress, not stored metadata — the ADR-0087 ledger drives `objectstack migrate meta`, and no metadata migration can rewrite a caller's URL. No authorable spec key, export or config field is removed or renamed by this change. -->
45 changes: 44 additions & 1 deletion AGENTS.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -602,7 +602,7 @@ export class MyPlugin implements Plugin {

## Route & surface ownership

Four rules, each paid for by a real bug. They matter more than usual here because this
Five rules, each paid for by a real bug. They matter more than usual here because this
repo is largely written by agents, and every one of them is a trap that reads as
reasonable code.

Expand DownExpand Up@@ -631,6 +631,49 @@ SDKs, codegen and AI clients. Advertise only what is actually mounted, and mount
everything advertised (ADR-0076 D12) — a wrong answer here propagates into everything
built on top of it.

**5. A new REST route declares its CLOSED query-parameter set on the day it lands.**
Maintainer ruling, 2026-08-12, verbatim and untranslated:

> **裁定:政策 YES —— 闭合查询参数集成为 REST ingress 政策;采纳方式为增量,⛔ 不打大包。**

Open the handler with `refuseUnknownQueryParams(req, res, <EXPORTED_PARAMS>)`
(`packages/rest/src/query-allowlist.ts` — its header is the authority on detail) so an
unrecognised name gets a located `400` instead of being dropped. A handler that reads
the keys it knows and ignores the rest fails **undetectably in both directions**: a
dropped *filter* returns the full set, a dropped key inside `where` returns `200` with
zero rows, and no status, header or field distinguishes either from a real answer — an
AI caller cannot see it at all. This is review-enforceable: a PR adding a `GET` route
that reads `req.query` without declaring a closed set is incomplete.

Three things to get right, all of them measured rather than assumed:

- ⛔ **Measure the set from the handler's ACTUAL read points, never from the docs or
the card.** It is not "the filters" — it is paging, ordering, format, alias spellings
and anything middleware reads. **Forgetting `limit` trades a silent-widening bug for
a loud pagination outage**, which is worse than the defect. ⚠️ **Read the helpers the
handler calls, not just the handler**: the export route's `?locale=` never appears in
its body — it is read a frame down, by `extractLocale` behind the call that localises
the header row — so a set measured from the handler alone would have 400'd every
localised export that works today. Pin **both halves** per route: a refusal pin
(status + nested `error.code` + **the service was never called**) beside a
preservation pin (**the arguments the service actually received**). Neither half is
optional, and a bare status assertion is not a pin — "still 200" is exactly what the
defect looked like, and the refusal's whole point is that the service never ran.
- ⛔ **Routes whose parameter set is genuinely OPEN are excluded, by name.**
`GET /data/:object` hands its whole query to the normalizer, which lowers every
leftover key into an implicit field filter (`?status=open` *is* the filter) — the
valid names are the object's own fields, so any list here would be wrong. It is
already gated one layer down, against the right authority: an unknown **field** is
refused there with `400 INVALID_FIELD`, judged against the object's real field map
(the registry's, including the audit/tenant/owner columns it injects — not the
author's declaration). The test: *if an unrecognised name has a defined meaning on
this route, the set is open* — gate it where the authority for the name lives.
- **Existing routes convert per lane, ⛔ never as one sweep** (data read routes first).
A broad wave with thin pins is the failure mode the ruling rejected.

Recognition runs **before** the arity gate (`refuseRepeatedQueryParams`); both answer
the same nested ADR-0112 `VALIDATION_ERROR`, so composing them adds no dialect.

**Verifying any of this:** "who serves this path" is a question about the composed,
*provisioned* runtime — not about which plugin declares it, not about registration
order, and not about a minimal harness that merely boots. The question has been
Expand Down
80 changes: 80 additions & 0 deletions packages/rest/src/query-allowlist.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -56,6 +56,86 @@
* of "this request is malformed". `VALIDATION_ERROR` is the standard catalog's
* member for 400 (`spec/src/api/errors.zod.ts`); nothing in `packages/spec`
* moves for this.
*
* ═══════════════════════════════════════════════════════════════════════════
* # THE INGRESS POLICY (#7606)
* ═══════════════════════════════════════════════════════════════════════════
*
* Maintainer ruling, 2026-08-12, verbatim and untranslated:
*
* > 裁定:政策 YES —— 闭合查询参数集成为 REST ingress 政策;采纳方式为增量,
* > ⛔ 不打大包。
*
* ## The rule
*
* **A new REST route declares its closed query-parameter set on the day it
* lands**, by opening its handler with {@link refuseUnknownQueryParams} over
* an exported `readonly string[]`. This is review-enforceable: a PR adding a
* `GET` route that reads `req.query` and does not declare a closed set is
* incomplete, and the reviewer should say so.
*
* Existing routes convert **per lane, never as one sweep** — a broad wave with
* thin pins is the failure mode the ruling explicitly rejected. Data READ
* routes convert first: silent widening and narrowing bite hardest there, and
* an AI caller can detect neither direction.
*
* ## Three rules for measuring the set — the part that goes wrong
*
* 1. ⛔ **Measure from the handler's ACTUAL read points. Never guess, and
* never copy the docs.** The set is not "the filters": it includes paging,
* ordering, output format, alias spellings the handler honours, and
* anything a middleware reads off the query before the handler runs. A
* whitelist that forgets `limit` converts a silent-widening bug into a loud
* pagination incident — strictly worse than the defect it fixes.
* 2. **Declare only what the handler really implements.** When a route reads
* an alias but not the canonical spelling (`GET /data/:object/:id` reads
* `select`, never the canonical `fields`), the missing spelling stays
* OUTSIDE the set. Adding it would advertise a capability that does not
* exist; refusing it makes the gap self-reporting.
* 3. **Recognition and arity are different questions** and a name may be
* answered differently by each. A multi-valued parameter belongs in the
* recognition set and stays out of the multiplicity declaration.
*
* ## ⛔ Routes whose parameter set is genuinely OPEN are excluded — by name
*
* The policy is not "every route eventually". Some surfaces accept
* caller-defined names by design, and closing them would be a defect:
*
* - **`GET {basePath}/data/:object` (the record list)** — ⛔ **do not add this
* gate.** The handler hands the WHOLE query record to `findData`, whose
* normalizer lowers every leftover key into an implicit field-equality
* predicate (`?status=open` IS the filter). The valid parameter names are
* therefore the object's own field names, which vary per object and include
* the audit / tenant / owner columns the registry injects — a closed list in
* this file could only ever be wrong. That route is already guarded, one
* layer down and against the right authority: #4134's read-path gate refuses
* an unknown FIELD with `400 INVALID_FIELD` (`assertQueryParamsAreFields`,
* `metadata-protocol`), and #7534 extended the same gate to the explicit
* `where` / `$filter` axes. Adding recognition here would break every
* implicit filter and author a third dialect for a condition that already
* has two correct answers.
*
* The general test: **if an unrecognised name has a defined meaning on this
* route, the set is open and this gate does not belong.** Gate it where the
* authority for the name actually lives.
*
* ## Composition with the sibling gates
*
* Recognition runs FIRST, before {@link refuseRepeatedQueryParams} — see that
* helper and the note on {@link refuseUnknownQueryParams} itself. Both answer
* the same nested `VALIDATION_ERROR` envelope, so composing them adds no
* dialect. The third gate, `assertFilterParamSuppliedOnce` (#7390), answers
* `400 INVALID_FILTER` through the flat `mapDataError` envelope and lives
* ONLY on the list route excluded above — so it and this gate never run on one
* request, and the cross-route code divergence recorded in #8001 is neither
* widened nor resolved by this policy.
*
* ## This breaks tolerated traffic, deliberately
*
* A caller sending a parameter we ignore today starts getting a `400`. That is
* the point, and v17 is the intended window: the traffic is invisible to us
* precisely because we drop it silently, so the blast radius cannot be
* measured from our side — only decided. It was decided above.
*/

/**
Expand Down
Loading
Loading