Skip to content

Commit f3f855a

Browse files
os-helpclaude
andauthored
fix(rest): refuse a repeated single-valued query parameter instead of coercing it (#6877) (#7324)
`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. Claude-Session: https://claude.ai/code/session_0158ZQo7LiHSxGWpYKuPq1wu Co-authored-by: Claude <noreply@anthropic.com>
1 parent a47f338 commit f3f855a

5 files changed

Lines changed: 892 additions & 57 deletions

File tree

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
'@objectstack/rest': patch
3+
---
4+
5+
Refuse a repeated single-valued query parameter instead of silently answering the wrong thing (#6877)
6+
7+
`IHttpRequest.query` is declared `Record< string, string | string[] >`, and the array
8+
arm is produced by a real first-party adapter (`NodeHttpServer` hands `?x=1&x=2`
9+
through as `['1','2']`, measured over a socket). `rest-server.ts` read ~50 of its
10+
query parameters as if the union had one arm, so a repeated parameter was coerced
11+
into a *different* value and served with a `200` rather than refused. None of it was
12+
a type error — every site laundered the array through `any`, `String()` or
13+
`Number()`.
14+
15+
Two of the outcomes were inversions rather than degradations:
16+
17+
- `PUT /meta/:type/:name?force=false&force=false` — the read fell through to
18+
`!!forceRaw`, and a non-empty array is truthy, so repeating an explicit **opt-out**
19+
switched the destructive-change guard **on**.
20+
- `GET /data/:object/export?limit=1&limit=2``Number([...])` is `NaN`, `NaN || 0`
21+
is `0`, `Math.max(1, 0)` is `1`: a **one-row export**, `200 OK`.
22+
23+
Each affected handler now declares which of its parameters are single-valued, and a
24+
repeated one is refused with `400` and the ADR-0112 nested envelope
25+
`{ error: { code: 'VALIDATION_ERROR', message } }` — the same rule and message
26+
#6307 landed on `/api/v1/packages/:id`, now shared rather than duplicated. The rule
27+
counts occurrences, not values: a one-element array is one occurrence and is
28+
accepted (and unwrapped), an empty array is none, two identical values are still two.
29+
30+
**Wire-visible**: requests that used to receive a wrong `200` now receive a `400`.
31+
No well-formed single-value request changes in any way.
32+
33+
Parameters that are genuinely multi-valued are deliberately untouched and pinned by
34+
tests — `select` / `expand` on `GET /data/:object/:id` (whose consumer takes
35+
`string | string[]` by design), `objects` on `/search`, `fields` / `searchFields` on
36+
the export route, and `approverId` on `/approvals/requests`.

packages/rest/src/package-routes.ts

Lines changed: 7 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { PackageService } from '@objectstack/service-package';
66
// The declared envelope is written in ONE place for the whole platform (#3973).
77
import { sendOk, sendError } from '@objectstack/types';
88
import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js';
9+
import { readSingleQueryValue, repeatedQueryParamMessage } from './query-multiplicity.js';
910

1011
/**
1112
* [#7033 / #7023] The authorization gate for the REST package transport.
@@ -73,65 +74,14 @@ async function refusePackageRequest(
7374
}
7475

7576
/**
76-
* The outcome of reading a query parameter that this API declares as
77-
* single-valued. `ok: false` carries the multiplicity so the refusal can say
78-
* what it saw rather than only that it refused.
79-
*/
80-
type SingleQueryRead =
81-
| { readonly ok: true; readonly value: string | undefined }
82-
| { readonly ok: false; readonly count: number };
83-
84-
/**
85-
* Read a query parameter the route declares single-valued out of the shape the
86-
* transport contract actually declares (#6307).
87-
*
88-
* `IHttpRequest.query` is `Record<string, string | string[]>` — a repeated
89-
* parameter is an ARRAY, and that is not a hypothetical arm of the union: the
90-
* `node:http` adapter (`@objectstack/http-conformance`'s `NodeHttpServer`)
91-
* hands `?version=a&version=b` through as `['a','b']`, measured over a socket.
92-
* The Hono adapter happens to collapse it to the first value before a handler
93-
* ever sees it, so the two adapters answer one contract-legal request
94-
* differently — which is precisely why the CONSUMER has to handle the shape it
95-
* was told to expect rather than lean on whichever server booted.
96-
*
97-
* ## Why repetition is refused rather than resolved
98-
*
99-
* `?version=1.0.0&version=2.0.0` is a well-formed request carrying two
100-
* conflicting intents. Picking one silently is a wrong answer delivered as a
101-
* success, and on `DELETE` it silently changes the OPERATION'S SCOPE: any
102-
* truthy `version` skips the `protocol.deletePackage` full-uninstall branch, so
103-
* a repeated parameter degraded a full uninstall into a narrow version-delete
104-
* and answered `200`. The server does not get to choose which of a caller's two
105-
* versions it meant; it says so.
106-
*
107-
* The rule is deliberately about MULTIPLICITY, not about shape: the parameter
108-
* may be supplied at most once. A one-element array is one occurrence encoded
109-
* differently by an adapter and is accepted; an empty array is no occurrence.
110-
* Two identical values (`?version=1.0.0&version=1.0.0`) are still two
111-
* occurrences and are still refused — "at most one *distinct* value" would be a
112-
* de-duplication rule no caller can predict, while "supply it at most once" is
113-
* checkable client-side without knowing anything about our semantics.
77+
* The `?version=` multiplicity rule (#6307), now shared (#6877).
11478
*
115-
* This is NOT tolerance for off-spec input: the contract already declares the
116-
* array. It is the consumer finally handling a declared shape.
79+
* Both helpers moved to `query-multiplicity.ts` when the same rule was applied
80+
* to `rest-server.ts`'s read points — ONE rule and one refusal message across
81+
* the package, rather than a second implementation free to drift. Behaviour
82+
* here is unchanged; only the definitions' home moved. The module's header
83+
* carries the full argument for why repetition is refused rather than resolved.
11784
*/
118-
function readSingleQueryValue(raw: string | string[] | undefined): SingleQueryRead {
119-
if (Array.isArray(raw)) {
120-
// length 0 → the parameter was not supplied; length 1 → supplied once.
121-
return raw.length > 1 ? { ok: false, count: raw.length } : { ok: true, value: raw[0] };
122-
}
123-
return { ok: true, value: raw };
124-
}
125-
126-
/**
127-
* The one refusal message for a repeated single-valued parameter, so `GET` and
128-
* `DELETE` answer the SAME rule identically — two different answers for one
129-
* parameter would just be a new inconsistency.
130-
*/
131-
function repeatedQueryParamMessage(name: string, count: number): string {
132-
return `The "${name}" query parameter was supplied ${count} times. Supply it at most once — `
133-
+ `this endpoint will not choose between conflicting values.`;
134-
}
13585

13686
/**
13787
* Options for package route registration.
Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Query-parameter MULTIPLICITY, for every REST handler in this package (#6877).
5+
*
6+
* `IHttpRequest.query` is declared `Record< string, string | string[] >`
7+
* (`packages/spec/src/contracts/http-server.ts`). A repeated parameter
8+
* (`?package=a&package=b`) is the ARRAY arm, and that arm is not hypothetical:
9+
* the `node:http` adapter (`@objectstack/http-conformance`'s `NodeHttpServer`)
10+
* hands it through as `['a','b']`, measured over a real socket on #6878. The
11+
* production Hono adapter happens to collapse repeats to the first value before
12+
* a handler runs — which is why this class is dormant today, and why it stops
13+
* being dormant the moment that collapse is removed (#6878's ruled route 2).
14+
*
15+
* ## Why the answer is a refusal and not a rule for picking
16+
*
17+
* `?version=1.0.0&version=2.0.0` is a well-formed request carrying two
18+
* conflicting intents. Picking one silently is a wrong answer delivered as a
19+
* success. The measured shapes in `rest-server.ts` were all of that kind, in
20+
* three flavours — none of which `tsc` can see, because each launders the array
21+
* through `any`, `String()` or `Number()`:
22+
*
23+
* - `?force=false&force=false` → `!!['false','false']` → **`force: true`**,
24+
* i.e. a repeated *opt-out* turned the destructive-change guard OFF.
25+
* - `?limit=1&limit=2` on the export route → `Number([...])` is `NaN`,
26+
* `NaN || 0` is `0`, `Math.max(1, 0)` is `1` → a **one-row export**, 200 OK.
27+
* - `?status=open&status=won` → `String([...])` → the single status
28+
* `'open,won'`, a name no record has → an empty list, 200 OK.
29+
*
30+
* So the rule is about the COUNT, not the shape: a parameter this API declares
31+
* single-valued may be supplied **at most once**. A one-element array is one
32+
* occurrence encoded differently by an adapter and is accepted (and unwrapped);
33+
* an empty array is no occurrence. Two identical values are still two
34+
* occurrences and are still refused — "at most one *distinct* value" would be a
35+
* de-duplication rule no caller can predict, while "supply it at most once" is
36+
* checkable client-side without knowing anything about our semantics.
37+
*
38+
* This is NOT tolerance for off-spec input: the contract already declares the
39+
* array. It is the consumer finally handling a declared shape.
40+
*
41+
* ## Single-valued is a per-parameter judgement, never a sweep
42+
*
43+
* Several parameters on this surface are genuinely multi-valued and their
44+
* consumers already read both arms — `select` / `expand` (`getData` accepts
45+
* `string | string[]` and splits the comma form itself), `objects` on
46+
* `/search`, `fields` / `searchFields` on the export route, `approverId` on
47+
* `/approvals/requests`. Those are deliberately absent from every declaration
48+
* below; flattening them would be a real regression, which is why each call
49+
* site names the parameters it declares single-valued instead of gating "every
50+
* key in `req.query`".
51+
*
52+
* #6307 landed the first copy of this rule in `package-routes.ts`. The two pure
53+
* helpers now live here so there is ONE rule and one message, not a second
54+
* implementation that drifts.
55+
*/
56+
57+
/**
58+
* The outcome of reading a query parameter that this API declares as
59+
* single-valued. `ok: false` carries the multiplicity so the refusal can say
60+
* what it saw rather than only that it refused.
61+
*/
62+
export type SingleQueryRead =
63+
| { readonly ok: true; readonly value: string | undefined }
64+
| { readonly ok: false; readonly count: number };
65+
66+
/**
67+
* Read a query parameter the route declares single-valued out of the shape the
68+
* transport contract actually declares (#6307).
69+
*
70+
* See this module's header for why repetition is refused rather than resolved,
71+
* and why the rule counts occurrences instead of inspecting the value.
72+
*/
73+
export function readSingleQueryValue(raw: string | string[] | undefined): SingleQueryRead {
74+
if (Array.isArray(raw)) {
75+
// length 0 → the parameter was not supplied; length 1 → supplied once.
76+
return raw.length > 1 ? { ok: false, count: raw.length } : { ok: true, value: raw[0] };
77+
}
78+
return { ok: true, value: raw };
79+
}
80+
81+
/**
82+
* The one refusal message for a repeated single-valued parameter, so every
83+
* route answers the SAME rule identically — two different answers for one
84+
* parameter would just be a new inconsistency.
85+
*/
86+
export function repeatedQueryParamMessage(name: string, count: number): string {
87+
return `The "${name}" query parameter was supplied ${count} times. Supply it at most once — `
88+
+ `this endpoint will not choose between conflicting values.`;
89+
}
90+
91+
/**
92+
* The gate `rest-server.ts` handlers open with: refuse a repeated occurrence of
93+
* any parameter this route declares single-valued, and normalise the benign
94+
* one-element array away so nothing downstream has to know the union existed.
95+
*
96+
* Returns `true` when it answered the request — the caller `return`s
97+
* immediately, exactly like the capability gates it sits beside.
98+
*
99+
* ## The envelope
100+
*
101+
* `400` with the ADR-0112 **nested** body `{ error: { code, message } }` — the
102+
* position ADR-0112 declares and the one PR #7293 (#7035) just converged this
103+
* file's `/meta` 501 refusals onto. `VALIDATION_ERROR` is not a new code: it is
104+
* the standard catalog's member for 400 (`spec/src/api/errors.zod.ts`,
105+
* `standardErrorCodeForHttpStatus(400)`), and the same code #6307 chose for
106+
* this same condition on `/packages/:id`. Nothing in `packages/spec` moves.
107+
*
108+
* ## Why it also normalises
109+
*
110+
* The rule accepts a one-element array as one occurrence. Accepting it without
111+
* unwrapping would leave `['a']` for the very `String()` / `Number()` / truthy
112+
* reads this exists to protect, so the acceptance would be a hole rather than a
113+
* courtesy. `req.query` is a plain mutable `Record` per the contract, and the
114+
* assignment happens ONLY on the array arm — a well-formed single-valued
115+
* request carries a `string` and is not touched at all, which is what makes the
116+
* preservation half of this change byte-identical.
117+
*
118+
* @param req the handler's request (`IHttpRequest`-shaped; `any` because
119+
* `rest-server.ts` types its handlers that way)
120+
* @param res the handler's response
121+
* @param names the parameters THIS route declares single-valued, in the order
122+
* they should be reported; the first repeated one is refused, so
123+
* the answer is deterministic for a request that repeats two.
124+
*/
125+
export function refuseRepeatedQueryParams(
126+
req: any,
127+
res: any,
128+
names: readonly string[],
129+
): boolean {
130+
const query = req?.query;
131+
if (!query || typeof query !== 'object') return false;
132+
for (const name of names) {
133+
const raw = query[name];
134+
if (!Array.isArray(raw)) continue;
135+
const read = readSingleQueryValue(raw);
136+
if (!read.ok) {
137+
res.status(400).json({
138+
error: { code: 'VALIDATION_ERROR', message: repeatedQueryParamMessage(name, read.count) },
139+
});
140+
return true;
141+
}
142+
// One occurrence encoded as an array (or none): unwrap so every read
143+
// below this line sees the `string | undefined` it was written for.
144+
query[name] = read.value as string;
145+
}
146+
return false;
147+
}

0 commit comments

Comments
 (0)