diff --git a/.changeset/closed-query-param-ingress-policy.md b/.changeset/closed-query-param-ingress-policy.md new file mode 100644 index 0000000000..89feea0504 --- /dev/null +++ b/.changeset/closed-query-param-ingress-policy.md @@ -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. + + diff --git a/AGENTS.md b/AGENTS.md index 7903aa3be9..d47fad31f2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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. @@ -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, )` +(`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 diff --git a/packages/rest/src/query-allowlist.ts b/packages/rest/src/query-allowlist.ts index 4435b33dec..b6f46a92fd 100644 --- a/packages/rest/src/query-allowlist.ts +++ b/packages/rest/src/query-allowlist.ts @@ -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. */ /** diff --git a/packages/rest/src/rest-server-closed-query-params.test.ts b/packages/rest/src/rest-server-closed-query-params.test.ts new file mode 100644 index 0000000000..86637d939c --- /dev/null +++ b/packages/rest/src/rest-server-closed-query-params.test.ts @@ -0,0 +1,548 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7606] The closed query-parameter set on the FIRST TIER of data read + * routes: `GET /data/:object/:id`, `GET /data/:object/export`, `GET /search`. + * + * ## What is being pinned, and why a status assertion alone would not do it + * + * These handlers read the keys they know off the query string and ignore the + * rest, so a misspelled, renamed or invented parameter is silently dropped and + * the caller gets a plausible-looking answer. The failure is undetectable from + * the response in BOTH directions — a dropped `?objects=` fans a search across + * every object, a dropped `?fields=` returns the whole record — and on the + * unfixed code every one of those is an ordinary **200**. So each refusal case + * below asserts three things, per the #7527 template: the ADR-0112 pair + * (`status` AND the NESTED `body.error.code`), the located message, and — the + * assertion that actually matters — **that the service was never called**. + * + * ## The other half: preservation + * + * A whitelist is only correct if the real traffic still passes, and the sharp + * edge is that the closed set is not "the filters": it carries paging, output + * format, ordering, and on the export route one name the handler body never + * mentions (`locale`, read by `extractLocale` behind `translateMetaItem`). + * Forgetting `limit` would trade a silent-widening bug for a loud export + * outage. Every preservation case therefore asserts the ARGUMENT the service + * received, not merely that a 200 came back — "still 200" is exactly what the + * defect looked like. + * + * ## Composition (§4) + * + * Two guards already run on this surface and a request can trip both. The + * order and the envelope are pinned here so the answer is defined rather than + * incidental, and so the #8001 divergence is provably left where it was. + */ + +import { describe, it, expect, vi } from 'vitest'; +// `.js` on purpose — NodeNext resolution requires the extension, and this +// package's TEST_DEBT ceiling has no margin for another TS2835 (#7248). +import { + RestServer, + DATA_RECORD_READ_PARAMS, + DATA_EXPORT_PARAMS, + GLOBAL_SEARCH_PARAMS, +} from './rest-server.js'; +import { unknownQueryParamMessage } from './query-allowlist.js'; + +const TASK = { + name: 'task', + label: 'Task', + fields: { + id: { name: 'id', type: 'text', label: 'ID' }, + title: { name: 'title', type: 'text', label: 'Title' }, + }, +}; + +/** Two rows, so "the unfiltered/unscoped answer came back" is visible. */ +const ROWS = [{ id: '1', title: 'alpha' }, { id: '2', title: 'beta' }]; + +function mockServer() { + const noop = () => { /* routes are driven directly, never through the adapter */ }; + return { + get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, + listen: async () => { /* never started */ }, + close: async () => { /* never started */ }, + }; +} + +function mockRes() { + const chunks: string[] = []; + const res: any = { + statusCode: 200, + write: vi.fn(function (this: any, s: any) { chunks.push(String(s)); return true; }), + end: vi.fn(function (this: any) { return this; }), + header: vi.fn(function (this: any) { return this; }), + setHeader: vi.fn(function (this: any) { return this; }), + send: vi.fn(function (this: any) { return this; }), + json: vi.fn(function (this: any, body: any) { this._body = body; return this; }), + status: vi.fn(function (this: any, code: number) { this.statusCode = code; return this; }), + }; + return { res, chunks }; +} + +/** + * Boot a RestServer with the data protocol stubbed, and return a driver per + * route under test. `isSystem` clears the capability gates that run BEFORE the + * recognition rule, so every request below reaches the gate it is named after. + */ +function boot() { + const getData = vi.fn().mockResolvedValue({ object: 'task', record: ROWS[0] }); + const findData = vi.fn().mockResolvedValue({ object: 'task', records: ROWS, total: 2 }); + const searchAll = vi.fn().mockResolvedValue({ results: [] }); + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue({ items: [TASK] }), + getMetaItem: vi.fn().mockResolvedValue({ type: 'object', name: 'task', item: TASK }), + getData, + findData, + searchAll, + }; + const rest = new RestServer( + mockServer() as any, + protocol as any, + { api: { requireAuth: false } } as any, + ); + (rest as any).resolveExecCtx = async () => ({ isSystem: true, userId: 'u1' }); + rest.registerRoutes(); + + const driver = (method: string, path: string) => async ( + query: Record = {}, + params: Record = { object: 'task' }, + ) => { + const found = (rest as any).getRoutes().find( + (r: any) => r.method === method && r.path === path, + ); + if (!found) throw new Error(`route not registered: ${method} ${path}`); + const out = mockRes(); + await found.handler( + { method, path, params, query, headers: {}, body: {} } as any, + out.res, + ); + return { + status: out.res.statusCode, + body: out.res.json.mock.calls.at(-1)?.[0], + chunks: out.chunks.join(''), + }; + }; + + return { + getData, findData, searchAll, + readRecord: driver('GET', '/api/v1/data/:object/:id'), + exportRows: driver('GET', '/api/v1/data/:object/export'), + search: driver('GET', '/api/v1/search'), + listRecords: driver('GET', '/api/v1/data/:object'), + }; +} + +/** The full ADR-0112 assertion for one recognition refusal. */ +function expectRefusal( + answer: { status: number; body: any }, + supported: readonly string[], + ...unknown: string[] +) { + expect( + answer.status, + `expected a 400 refusal for ${unknown.join(', ')}, got ${answer.status} ` + + `with body ${JSON.stringify(answer.body)}`, + ).toBe(400); + // Nested, per ADR-0112 — the same position and code the multiplicity + // refusal on these same handlers answers with (#6877 / #7527). + expect(typeof answer.body?.error).toBe('object'); + expect(answer.body?.error?.code).toBe('VALIDATION_ERROR'); + expect(answer.body?.error?.message).toBe( + unknownQueryParamMessage(unknown, [...supported].sort()), + ); + // The refusal must not have leaked a row-shaped payload alongside itself. + expect(answer.body?.records).toBeUndefined(); + expect(answer.body?.record).toBeUndefined(); +} + +// ───────────────────────────────────────────────────────────────────────────── +// 1. GET /data/:object/:id — closed set {select, expand} +// ───────────────────────────────────────────────────────────────────────────── + +describe('#7606 §1 — GET /data/:object/:id refuses what it would have dropped', () => { + it('?fields= is refused — the CANONICAL spelling this route never implemented', async () => { + // The spec alias table declares the slot as canonical `fields` with + // alias `select`; this route folds no aliases and reads only `select`, + // so `?fields=title` returned the FULL record with a 200. The refusal + // makes that gap self-reporting instead of silent — whether the fold + // should exist at all is #8039, deliberately not settled here. + const { readRecord, getData } = boot(); + const answer = await readRecord({ fields: 'title' }, { object: 'task', id: '1' }); + expectRefusal(answer, DATA_RECORD_READ_PARAMS, 'fields'); + expect( + getData, + 'the record must not have been read — answering the full record is the defect', + ).not.toHaveBeenCalled(); + }); + + it('?populate= — the expand slot\'s unimplemented alias — is refused the same way', async () => { + const { readRecord, getData } = boot(); + const answer = await readRecord({ populate: 'owner' }, { object: 'task', id: '1' }); + expectRefusal(answer, DATA_RECORD_READ_PARAMS, 'populate'); + expect(getData).not.toHaveBeenCalled(); + }); + + it('the refusal stays a 400 — the catch that rewrites 400→404 must not swallow it', async () => { + // This route's catch turns every 400 into a 404 (a bad id is a miss, + // not a malformed request). The gate RESPONDS rather than throwing + // precisely so a refusal cannot reach the caller as "no such record" — + // the silent-drop defect wearing a different status. If someone later + // converts the gate to `throw`, this is the test that goes red. + const { readRecord } = boot(); + const answer = await readRecord({ nope: '1' }, { object: 'task', id: '1' }); + expect(answer.status).toBe(400); + expect(answer.status).not.toBe(404); + expect(answer.body?.error?.code).toBe('VALIDATION_ERROR'); + }); + + it('several unknown names are all reported, in a deterministic order', async () => { + const { readRecord } = boot(); + const answer = await readRecord( + { zebra: '1', alpha: '2', select: 'title' }, + { object: 'task', id: '1' }, + ); + expectRefusal(answer, DATA_RECORD_READ_PARAMS, 'alpha', 'zebra'); + }); + + it('PRESERVATION: select and expand still reach getData, verbatim', async () => { + const { readRecord, getData } = boot(); + const answer = await readRecord( + { select: 'title', expand: 'owner' }, + { object: 'task', id: '1' }, + ); + expect(answer.status).toBe(200); + expect(getData).toHaveBeenCalledWith( + expect.objectContaining({ object: 'task', id: '1', select: 'title', expand: 'owner' }), + ); + }); + + it('PRESERVATION: the repeated (array) arm of select/expand still flows through', async () => { + // `?select=a&select=b` is ALREADY correct end to end here (#6877's + // measured verdict — `getData` splits the comma form itself and takes + // an array). Recognition must not have quietly flattened or refused it. + const { readRecord, getData } = boot(); + const answer = await readRecord( + { select: ['id', 'title'] }, + { object: 'task', id: '1' }, + ); + expect(answer.status).toBe(200); + expect(getData).toHaveBeenCalledWith( + expect.objectContaining({ select: ['id', 'title'] }), + ); + }); + + it('PRESERVATION: a bare read with no parameters is untouched', async () => { + const { readRecord, getData } = boot(); + const answer = await readRecord({}, { object: 'task', id: '1' }); + expect(answer.status).toBe(200); + expect(getData).toHaveBeenCalledTimes(1); + }); + + it('every name in the declared set is accepted — asserted against the export', async () => { + for (const name of DATA_RECORD_READ_PARAMS) { + const { readRecord } = boot(); + const answer = await readRecord({ [name]: 'x' }, { object: 'task', id: '1' }); + expect(answer.status, `"${name}" is declared supported but was refused`).toBe(200); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 2. GET /data/:object/export — closed set of ten, one of them invisible +// ───────────────────────────────────────────────────────────────────────────── + +describe('#7606 §2 — GET /data/:object/export', () => { + it('an unknown parameter is refused and NOTHING is streamed', async () => { + const { exportRows, findData } = boot(); + const answer = await exportRows({ format: 'csv', pageSize: '10' }); + expectRefusal(answer, DATA_EXPORT_PARAMS, 'pageSize'); + expect( + findData, + 'a refusal must land before the first chunk — a dribbled row ahead of the ' + + 'status code is how a caller ends up with a truncated export', + ).not.toHaveBeenCalled(); + expect(answer.chunks).toBe(''); + }); + + it('the message lists every supported name, so the request is fixable from the response', async () => { + const { exportRows } = boot(); + const { body } = await exportRows({ colums: 'id' }); + const message = String(body?.error?.message); + expect(message).toContain('"colums"'); + for (const name of DATA_EXPORT_PARAMS) { + expect(message, `supported parameter "${name}" must appear in the refusal`) + .toContain(name); + } + }); + + it('PRESERVATION: ?locale= survives — the name the handler body never mentions', async () => { + // THE regression this file exists to prevent. `locale` is read one + // frame down by `extractLocale` behind `translateMetaItem`, so a closed + // set measured from the handler alone omits it — and every localised + // export that works today would start answering 400. A measurement is + // not finished until the helpers the handler calls have been read. + const { exportRows, findData } = boot(); + const answer = await exportRows({ format: 'csv', locale: 'zh-CN' }); + expect( + answer.status, + '?locale= is read via extractLocale and MUST be inside the closed set', + ).toBe(200); + expect(findData).toHaveBeenCalled(); + }); + + it('PRESERVATION: limit and page reach findData — forgetting them is a loud outage', async () => { + // Both land on the SAME derived value — the chunk the read asks for is + // `min(page, limit - exported)` — so the two are separated by choosing + // which one is the smaller. Asserting the value rather than the status + // is what makes this a preservation pin: `limit` silently dropped + // exports the whole table with a perfectly ordinary 200. + + // `limit` is the binding cap here (25 < 100), so $top proves IT arrived. + const capped = boot(); + const byLimit = await capped.exportRows({ format: 'csv', limit: '25', page: '100' }); + expect(byLimit.status).toBe(200); + expect((capped.findData.mock.calls[0][0] as any)?.query?.$top).toBe(25); + + // `page` is the binding cap here (50 < 200), so $top proves IT arrived + // — a default chunk would have read 500. + const chunked = boot(); + const byPage = await chunked.exportRows({ format: 'csv', limit: '200', page: '50' }); + expect(byPage.status).toBe(200); + expect((chunked.findData.mock.calls[0][0] as any)?.query?.$top).toBe(50); + }); + + it('PRESERVATION: the row-selection axes still narrow the export', async () => { + const { exportRows, findData } = boot(); + const answer = await exportRows({ + format: 'json', + filter: JSON.stringify({ title: 'alpha' }), + search: 'alpha', + searchFields: 'title', + orderby: 'title:desc', + fields: 'id,title', + header: 'false', + }); + expect(answer.status).toBe(200); + expect(findData).toHaveBeenCalled(); + const arg = findData.mock.calls[0][0] as any; + const q = arg?.query ?? arg; + // The filter the caller expressed must be the one that ran — a dropped + // filter exports MORE rows than asked for, indistinguishable from a + // genuinely broad match. + expect(JSON.stringify(q)).toContain('alpha'); + }); + + it('every name in the declared set is accepted — asserted against the export', async () => { + // A VALID value per name, so a 400 in this loop can only mean "the + // recognition gate refused it" and never "the value was unusable" — + // `filter` in particular is parsed as JSON and would 400 on `'x'` for + // a reason that has nothing to do with the closed set. + const validValue: Record = { + filter: JSON.stringify({ title: 'alpha' }), + orderby: 'title:desc', + limit: '10', + page: '500', + header: 'true', + format: 'csv', + }; + for (const name of DATA_EXPORT_PARAMS) { + const { exportRows } = boot(); + const answer = await exportRows({ format: 'csv', [name]: validValue[name] ?? 'title' }); + expect( + answer.status, + `"${name}" is declared supported but was refused: ` + + JSON.stringify(answer.body), + ).toBe(200); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 3. GET /search — closed set {q, query, objects, limit, perObject} +// ───────────────────────────────────────────────────────────────────────────── + +describe('#7606 §3 — GET /search', () => { + it('an unknown scope parameter is refused instead of fanning out over everything', async () => { + // The widening case at its worst: `?object=lead` (singular — the + // plausible misspelling of `objects`) searched EVERY object while the + // caller believed they had scoped it to one. + const { search, searchAll } = boot(); + const answer = await search({ q: 'acme', object: 'lead' }); + expectRefusal(answer, GLOBAL_SEARCH_PARAMS, 'object'); + expect( + searchAll, + 'the unscoped search must not have run — its result is shaped exactly ' + + 'like a correctly scoped one', + ).not.toHaveBeenCalled(); + }); + + it('every plausible misspelling of the same question fails the same way', async () => { + for (const name of ['object', 'types', 'scope', 'Objects', 'per_object']) { + const { search, searchAll } = boot(); + const answer = await search({ q: 'acme', [name]: 'lead' }); + expectRefusal(answer, GLOBAL_SEARCH_PARAMS, name); + expect(searchAll).not.toHaveBeenCalled(); + } + }); + + it('PRESERVATION: the scope, both term spellings and both caps reach searchAll', async () => { + const { search, searchAll } = boot(); + const answer = await search({ + q: 'acme', objects: 'lead,account', limit: '20', perObject: '5', + }); + expect(answer.status).toBe(200); + expect(searchAll).toHaveBeenCalledWith(expect.objectContaining({ + q: 'acme', + objects: ['lead', 'account'], + limit: 20, + perObject: 5, + })); + }); + + it('PRESERVATION: the `query` fallback spelling of the term still works', async () => { + const { search, searchAll } = boot(); + const answer = await search({ query: 'acme' }); + expect(answer.status).toBe(200); + expect(searchAll).toHaveBeenCalledWith(expect.objectContaining({ q: 'acme' })); + }); + + it('PRESERVATION: the multi-valued arm of objects is not flattened', async () => { + // `objects` is deliberately absent from the route's multiplicity + // declaration — a cross-object search over a LIST is the whole point. + const { search, searchAll } = boot(); + const answer = await search({ q: 'acme', objects: ['lead', 'account'] }); + expect(answer.status).toBe(200); + expect(searchAll).toHaveBeenCalledWith(expect.objectContaining({ + objects: ['lead', 'account'], + })); + }); + + it('every name in the declared set is accepted — asserted against the export', async () => { + for (const name of GLOBAL_SEARCH_PARAMS) { + const { search } = boot(); + const answer = await search({ [name]: 'x' }); + expect(answer.status, `"${name}" is declared supported but was refused`).toBe(200); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 4. COMPOSITION — two guards on one request must have ONE defined answer +// ───────────────────────────────────────────────────────────────────────────── + +describe('#7606 §4 — how the recognition and arity guards compose', () => { + it('an UNKNOWN parameter outranks a repeated known one, on both tiered routes', async () => { + // "I do not know this parameter" is the more fundamental error, so it + // is the one the caller is told about. Pinned rather than left to the + // order the two calls happen to sit in. + const exp = boot(); + expectRefusal( + await exp.exportRows({ bogus: '1', limit: ['1', '2'] }), + DATA_EXPORT_PARAMS, 'bogus', + ); + expect(exp.findData).not.toHaveBeenCalled(); + + const srch = boot(); + expectRefusal( + await srch.search({ bogus: '1', q: ['a', 'b'] }), + GLOBAL_SEARCH_PARAMS, 'bogus', + ); + expect(srch.searchAll).not.toHaveBeenCalled(); + }); + + it('a request that is BOTH unknown AND repeated on the SAME name answers once', async () => { + // One request, one response — never a refusal racing a second refusal. + const { exportRows } = boot(); + const answer = await exportRows({ bogus: ['1', '2'] }); + expectRefusal(answer, DATA_EXPORT_PARAMS, 'bogus'); + }); + + it('both guards answer ONE envelope, so composing them adds no dialect', async () => { + const { exportRows } = boot(); + const unknown = await exportRows({ bogus: '1' }); + const repeated = await exportRows({ format: ['csv', 'json'] }); + // Same status, same nested position, same code — two flavours of "this + // request is malformed", one machine-readable answer shape. + expect(unknown.status).toBe(400); + expect(repeated.status).toBe(400); + expect(unknown.body?.error?.code).toBe('VALIDATION_ERROR'); + expect(repeated.body?.error?.code).toBe('VALIDATION_ERROR'); + }); + + it('⛔ #8001 is left exactly where it was — a repeated ?filter= still answers as before', async () => { + // `filter` is INSIDE the export route's closed set, so recognition + // passes it through and the multiplicity gate still answers it. The + // divergence #8001 records (this route's VALIDATION_ERROR vs the LIST + // route's INVALID_FILTER, by the #7390 maintainer ruling) is neither + // widened nor resolved here — that is the maintainer's call, and this + // test goes red if a later change makes it unilaterally. + const { exportRows } = boot(); + const answer = await exportRows({ filter: ['{"a":1}', '{"b":2}'] }); + expect(answer.status).toBe(400); + expect(answer.body?.error?.code).toBe('VALIDATION_ERROR'); + expect(answer.body?.error?.code).not.toBe('INVALID_FILTER'); + expect(String(answer.body?.error?.message)).toContain('at most once'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 5. THE EXCLUSION — GET /data/:object must NOT be closed +// ───────────────────────────────────────────────────────────────────────────── + +describe('#7606 §5 — the record LIST route is deliberately left open', () => { + it('an unrecognised name still reaches findData as an implicit field filter', async () => { + // ⛔ The one route in this family that must NOT get the recognition + // gate. Its 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 names are the + // object's own fields (including the audit/tenant/owner columns the + // registry injects), so a closed list here could only ever be wrong. + // The authority for the name lives one layer down: #4134 / #7534 refuse + // an unknown FIELD with 400 INVALID_FIELD, against the real field map. + // + // This test fails the moment someone "completes the sweep" by adding + // the gate here — which would break every implicit filter on the + // platform's most-used read route. + const { listRecords, findData } = boot(); + const answer = await listRecords({ not_a_declared_param: 'x' }); + + expect( + answer.status, + 'GET /data/:object must NOT carry the closed-set gate — an unrecognised ' + + 'name here is an implicit FIELD filter, judged one layer down by #4134', + ).toBe(200); + expect(findData).toHaveBeenCalledTimes(1); + // The name must reach the normalizer intact, since that is the only + // layer holding the object's real field map. + const arg = findData.mock.calls[0][0] as any; + expect(arg?.query).toMatchObject({ not_a_declared_param: 'x' }); + }); + + it('the ordinary implicit field filter still works — the capability being protected', async () => { + const { listRecords, findData } = boot(); + const answer = await listRecords({ status: 'open', limit: '10' }); + expect(answer.status).toBe(200); + expect((findData.mock.calls[0][0] as any)?.query) + .toMatchObject({ status: 'open', limit: '10' }); + }); + + it('#7390\'s repeated-filter refusal on this route is untouched', async () => { + // The serial constraint: PR #8004 landed `400 INVALID_FILTER` here for + // a repeated `?filter=`. This card must not undo or reroute it, and + // since the recognition gate never runs on this route the two guards + // never meet on one request. + const { listRecords, findData } = boot(); + const answer = await listRecords({ filter: ['{"a":1}', '{"b":2}'] }); + expect(answer.status).toBe(400); + // Flat `mapDataError` envelope, per the #7390 ruling — NOT the nested + // VALIDATION_ERROR the recognition gate answers elsewhere. + expect(answer.body?.code).toBe('INVALID_FILTER'); + expect(findData).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 980ace2595..6fcdb14a8f 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -1758,6 +1758,80 @@ export const APPROVAL_REQUEST_LIST_PARAMS: readonly string[] = [ 'limit', 'offset', ]; +/** + * [#7606] The closed query-parameter set of `GET {basePath}/data/:object/:id`. + * + * **Measured**, at `registerCrudEndpoints`' read-record handler, from the ONE + * line that reads the query — `const { select, expand } = req.query || {}`. + * The handler destructures exactly these two names and forwards nothing else, + * so every other parameter on this route is dropped in the fullest sense: it + * never reaches `getData` at all. + * + * ⚠️ The dropped names include the CANONICAL spelling of one slot. The spec's + * alias table (`RPC_QUERY_ALIAS_SLOTS`) declares the fields slot as canonical + * `fields` with alias `select`, and the expand slot as canonical `expand` with + * alias `populate` — but this route folds no aliases, so `?fields=name` + * silently returns the FULL record and `?populate=…` silently expands nothing. + * Both are outside this set on purpose: adding them here would advertise a + * capability the handler does not implement, which is the declared-≠-enforced + * trap in the other direction. Refusing them instead makes the gap + * self-reporting — the located message names `select` / `expand` as what this + * route does accept. Tracked as #8039 (an alias-coverage question for the spec + * table and this handler to settle together) rather than widened here. + */ +export const DATA_RECORD_READ_PARAMS: readonly string[] = ['select', 'expand']; + +/** + * [#7606] The closed query-parameter set of `GET {basePath}/data/:object/export`. + * + * **Measured** from the export handler's own reads of `q = req.query ?? {}`, + * every one of them: the output controls (`format`, `header`), the paging pair + * (`limit`, and `page` — which on this route is the streaming CHUNK size, not + * a page number), the row-selection axes (`filter`, `search`, `searchFields`, + * `orderby`) and the column selection (`fields`). + * + * ⚠️ …and `locale`, **which the handler body never mentions**. It is read one + * frame down, by `extractLocale` behind the `translateMetaItem` call that + * localises the header row — the "anything middleware reads" clause of the + * measuring rule, and the single name on this route that a read of the handler + * alone gets wrong. Omitting it would 400 every `?locale=zh-CN` export that + * works today, turning localised column headers into an outage: precisely the + * silent-widening-traded-for-a-loud-incident failure the policy warns about, + * committed by the change meant to prevent it. The measurement is only + * finished when the helpers the handler calls have been read too. + * + * `fields` and `searchFields` are in this set but are deliberately absent from + * the route's sibling multiplicity declaration: both read their array arm on + * purpose (columns are genuinely a list). Recognition and arity are separate + * questions and a name can be answered differently by each. + * + * ⚠️ Dropping `limit` from this array would convert a silent-widening bug into + * a loud export outage — the preservation half of + * `rest-server-closed-query-params.test.ts` exists to make that impossible to + * land, and pins `locale` by name for the reason above. + */ +export const DATA_EXPORT_PARAMS: readonly string[] = [ + 'format', 'header', + 'limit', 'page', + 'filter', 'search', 'searchFields', 'orderby', + 'fields', + 'locale', +]; + +/** + * [#7606] The closed query-parameter set of `GET {basePath}/search`. + * + * **Measured** from the cross-object search handler: the term under both + * spellings it honours (`q`, and the `query` fallback the very next line + * reads), the object scope (`objects`), and the two result caps (`limit`, + * `perObject`). A dropped `?objects=` here is the widening case in its purest + * form — the search silently fans out across every object instead of the one + * the caller named. + */ +export const GLOBAL_SEARCH_PARAMS: readonly string[] = [ + 'q', 'query', 'objects', 'limit', 'perObject', +]; + /** Platform object backing async import jobs (see sys-import-job.object.ts). */ const IMPORT_JOB_OBJECT = 'sys_import_job'; /** Cap on per-row results persisted on the job (failures first). */ @@ -6834,10 +6908,23 @@ export class RestServer { // flattening it) would be the regression. The card listed // this line under "array flows downstream"; measurement // says the downstream was built for it. - const { select, expand } = req.query || {}; const context = await this.resolveExecCtx(environmentId, req); if (this.enforceAuth(req, res, context)) return; if (await this.enforceApiAccess(req, res, p, environmentId, 'get')) return; + // [#7606] Closed parameter set, AFTER the capability + // gates for the same reason #7527 put it there: which + // parameters a route understands is information, and a + // caller who may not read this object should not learn + // the shape of its ingress before being refused. + // + // This gate RESPONDS rather than throwing, which on this + // route is load-bearing and not a style choice: the catch + // below rewrites every 400 into a 404 (a bad id is a + // miss, not a malformed request). A refusal routed + // through it would reach the caller as "no such record" + // — the silent-drop defect wearing a different status. + if (refuseUnknownQueryParams(req, res, DATA_RECORD_READ_PARAMS)) return; + const { select, expand } = req.query || {}; const result = await p.getData({ object: req.params.object, id: req.params.id, @@ -7698,6 +7785,19 @@ export class RestServer { // `fields` and `searchFields` are NOT listed — both already // read the array arm on purpose (`Array.isArray(q.fields)` // a few lines down), and columns are genuinely a list. + // [#7606] Recognition runs BEFORE arity, per the rule stated + // in `query-allowlist.ts`: "I do not know this parameter" + // outranks "this parameter I do know was supplied twice", so + // a request committing both errors is told the more + // fundamental one. Both gates answer the SAME envelope here + // (nested ADR-0112 `VALIDATION_ERROR`), so composing them + // adds no second dialect to this route — the divergence + // recorded in #8001 is between the LIST route's + // `INVALID_FILTER` and this one, and is left exactly as it + // was: `filter` is inside the closed set below, so a + // repeated `?filter=` still reaches the multiplicity gate + // and still answers what it answered before. + if (refuseUnknownQueryParams(req, res, DATA_EXPORT_PARAMS)) return; if (refuseRepeatedQueryParams(req, res, ['format', 'header', 'limit', 'page', 'filter', 'search', 'orderby'])) return; const q = req.query ?? {}; @@ -8071,6 +8171,12 @@ export class RestServer { // term `'a,b'`. `objects` is NOT listed: the next line reads // its array arm deliberately — a cross-object search over a // LIST of objects is the whole point of the parameter. + // [#7606] Recognition before arity, same order and same + // envelope as the export route — see `query-allowlist.ts`. + // A dropped `?objects=` is the widening case at its worst: + // the term fans out across every searchable object while the + // caller believes they scoped it to one. + if (refuseUnknownQueryParams(req, res, GLOBAL_SEARCH_PARAMS)) return; if (refuseRepeatedQueryParams(req, res, ['q', 'query', 'limit', 'perObject'])) return; const q = String(req.query?.q ?? req.query?.query ?? ''); const objectsParam = req.query?.objects;