From 4aa94cf0100dfd0adf7f8043e77f95a616b88ab6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 01:44:46 +0000 Subject: [PATCH 1/3] fix(rest): refuse a repeated ?version= on GET/DELETE /packages/:id (#6307) --- .../src/package-envelope.conformance.test.ts | 24 ++ .../package-routes-query-multiplicity.test.ts | 233 ++++++++++++++++++ packages/rest/src/package-routes.ts | 83 ++++++- 3 files changed, 337 insertions(+), 3 deletions(-) create mode 100644 packages/rest/src/package-routes-query-multiplicity.test.ts diff --git a/packages/rest/src/package-envelope.conformance.test.ts b/packages/rest/src/package-envelope.conformance.test.ts index 1ebb4f26a5..8cc0702df0 100644 --- a/packages/rest/src/package-envelope.conformance.test.ts +++ b/packages/rest/src/package-envelope.conformance.test.ts @@ -341,6 +341,30 @@ describe('packages envelope (#3843) — error bodies', () => { expect(body.data.packages).toHaveLength(1); }); + it('a repeated `?version=` is refused identically on both verbs (#6307)', async () => { + // The rule is one rule, so the two verbs must answer the SAME code, status + // and message — two answers for one parameter would be a new inconsistency. + const get = await drive( + mount({ get: async () => ({ id: 'com.acme.crm', manifest: MANIFEST }) }), + 'GET', + `${PKGS}/:id`, + { params: { id: 'com.acme.crm' }, query: { version: ['1.0.0', '2.0.0'] } }, + ); + const del = await drive( + mount({ delete: async () => ({ success: true }) }), + 'DELETE', + `${PKGS}/:id`, + { params: { id: 'com.acme.crm' }, query: { version: ['1.0.0', '2.0.0'] } }, + ); + expect(get.status).toBe(400); + expect(del.status).toBe(400); + expect(get.body).toEqual(del.body); + expect(get.body.error.code).toBe('VALIDATION_ERROR'); + expect(get.body.error.message).toContain('"version"'); + expect(envelopeViolations(get.body)).toEqual([]); + expect(BaseResponseSchema.safeParse(get.body).success).toBe(true); + }); + it('a partial uninstall keeps its per-item detail under `error.details`', async () => { const { body } = await drive( mount({}, { diff --git a/packages/rest/src/package-routes-query-multiplicity.test.ts b/packages/rest/src/package-routes-query-multiplicity.test.ts new file mode 100644 index 0000000000..7665400d57 --- /dev/null +++ b/packages/rest/src/package-routes-query-multiplicity.test.ts @@ -0,0 +1,233 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `?version=` multiplicity on `/api/v1/packages/:id` (#6307). + * + * `IHttpRequest.query` is declared `Record`, so a + * repeated query parameter arrives as an ARRAY. Both handlers used it as a + * string and handed the array straight to `PackageService`, whose parameter is + * `version?: string`. Measured on `origin/main` before the fix: + * + * GET ?version=1.0.0&version=2.0.0 → packageService.get(id, ['1.0.0','2.0.0']) + * DELETE ?version=1.0.0&version=2.0.0 → packageService.delete(id, ['1.0.0','2.0.0']) + * …and `protocol.deletePackage` NOT called, + * answering 200 "Deleted com.acme.crm@1.0.0,2.0.0" + * + * The DELETE line is the sharp one: `if (!version && protocol.deletePackage)` + * gates the FULL uninstall (metadata rows + the durable `sys_packages` record + + * the registered data-plane cleanups, #2747). A truthy `version` skips it, so a + * repeated parameter silently narrowed the operation's SCOPE and still reported + * success. That is a wrong answer on a destructive verb, so the route refuses + * the ambiguity instead of resolving it — see `readSingleQueryValue`. + * + * Observation-class: no user hits this today, because it takes a client that + * repeats the parameter, and the Hono adapter collapses repeats to the first + * value before a handler sees them. The `node:http` adapter does not (measured: + * `NodeHttpServer` hands `['1.0.0','2.0.0']` through over a real socket), which + * is why the consumer has to handle the shape its contract declares rather than + * depend on which server booted. + * + * What these cases pin, in order: the single-value paths behave EXACTLY as + * before (the fix is not allowed to move them), repetition is refused + * identically on both verbs, and the full-uninstall branch is still reached + * when no version is supplied at all. + */ + +import { describe, it, expect } from 'vitest'; +import type { RouteHandler } from '@objectstack/spec/contracts'; +import { registerPackageRoutes } from './package-routes.js'; + +const PKGS = '/api/v1/packages'; +const ID = 'com.acme.crm'; +const MANIFEST = { id: ID, version: '1.0.0' }; + +interface Captured { status: number; body: any } + +/** Records every argument the service/protocol layer is handed. */ +interface Spy { + getVersions: unknown[]; + deleteVersions: unknown[]; + protocolCalls: number; +} + +function harness(options: { protocol?: boolean } = {}) { + const spy: Spy = { getVersions: [], deleteVersions: [], protocolCalls: 0 }; + const svc = { + get: async (_id: string, version?: string) => { + spy.getVersions.push(version); + return { id: ID, manifest: MANIFEST }; + }, + delete: async (_id: string, version?: string) => { + spy.deleteVersions.push(version); + return { success: true }; + }, + }; + const opts = options.protocol + ? { + protocol: { + deletePackage: async () => { + spy.protocolCalls += 1; + return { success: true, deletedCount: 3, failedCount: 0, failed: [], cleanups: [] }; + }, + }, + } + : {}; + + const routes = new Map(); + const server = { + get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); }, + post: (p: string, h: RouteHandler) => { routes.set(`POST:${p}`, h); }, + put: () => {}, + delete: (p: string, h: RouteHandler) => { routes.set(`DELETE:${p}`, h); }, + patch: () => {}, + use: () => {}, + listen: async () => {}, + close: async () => {}, + } as any; + registerPackageRoutes(server, svc as any, '/api/v1', opts); + + const drive = async (method: 'GET' | 'DELETE', query: Record): Promise => { + const handler = routes.get(`${method}:${PKGS}/:id`); + if (!handler) throw new Error(`no handler for ${method}`); + const captured: Captured = { status: 200, body: undefined }; + const res: any = { + json(d: any) { captured.body = d; }, + send() {}, + status(c: number) { captured.status = c; return res; }, + header() { return res; }, + }; + await handler( + { params: { id: ID }, query, body: undefined, headers: {}, method, path: `${PKGS}/:id` } as any, + res, + ); + return captured; + }; + + return { spy, drive }; +} + +describe('#6307 — a single `?version=` behaves exactly as before', () => { + it('GET with one value passes that STRING through and answers the same body', async () => { + const { spy, drive } = harness(); + const { status, body } = await drive('GET', { version: '1.0.0' }); + expect(spy.getVersions).toEqual(['1.0.0']); + expect(status).toBe(200); + expect(body).toEqual({ + success: true, + data: { package: { id: ID, manifest: MANIFEST, source: 'database' } }, + }); + }); + + it('GET with no version still asks for `latest`', async () => { + const { spy, drive } = harness(); + await drive('GET', {}); + expect(spy.getVersions).toEqual(['latest']); + }); + + it('GET with an EMPTY `?version=` still asks for `latest` (falsy, as before)', async () => { + const { spy, drive } = harness(); + await drive('GET', { version: '' }); + expect(spy.getVersions).toEqual(['latest']); + }); + + it('DELETE with one value stays version-scoped and answers the same body', async () => { + const { spy, drive } = harness({ protocol: true }); + const { status, body } = await drive('DELETE', { version: '1.0.0' }); + expect(spy.deleteVersions).toEqual(['1.0.0']); + expect(spy.protocolCalls).toBe(0); + expect(status).toBe(200); + expect(body).toEqual({ success: true, data: { message: `Deleted ${ID}@1.0.0` } }); + }); +}); + +describe('#6307 — the full-uninstall branch is still reached without a version', () => { + it('DELETE with NO version goes through protocol.deletePackage', async () => { + const { spy, drive } = harness({ protocol: true }); + const { status, body } = await drive('DELETE', {}); + expect(spy.protocolCalls).toBe(1); + expect(spy.deleteVersions).toEqual([]); + expect(status).toBe(200); + expect(body).toEqual({ + success: true, + data: { message: `Deleted ${ID}`, deletedCount: 3, cleanups: [] }, + }); + }); + + it('DELETE with an EMPTY `?version=` still uninstalls fully (falsy, as before)', async () => { + const { spy, drive } = harness({ protocol: true }); + await drive('DELETE', { version: '' }); + expect(spy.protocolCalls).toBe(1); + }); + + it('DELETE with the parameter absent from an EMPTY array is no occurrence at all', async () => { + // A contract-legal encoding of "not supplied". It must not be mistaken for + // a version pin — that would silently narrow the uninstall again. + const { spy, drive } = harness({ protocol: true }); + await drive('DELETE', { version: [] }); + expect(spy.protocolCalls).toBe(1); + }); +}); + +describe('#6307 — one occurrence encoded as a one-element array is still one occurrence', () => { + it('GET accepts `[\'1.0.0\']` and unwraps it', async () => { + const { spy, drive } = harness(); + const { status } = await drive('GET', { version: ['1.0.0'] }); + expect(status).toBe(200); + expect(spy.getVersions).toEqual(['1.0.0']); + }); + + it('DELETE accepts `[\'1.0.0\']` and stays version-scoped', async () => { + const { spy, drive } = harness({ protocol: true }); + const { status } = await drive('DELETE', { version: ['1.0.0'] }); + expect(status).toBe(200); + expect(spy.deleteVersions).toEqual(['1.0.0']); + expect(spy.protocolCalls).toBe(0); + }); +}); + +describe('#6307 — a REPEATED `?version=` is refused, not resolved', () => { + it('GET answers 400 VALIDATION_ERROR and never reaches the service', async () => { + const { spy, drive } = harness(); + const { status, body } = await drive('GET', { version: ['1.0.0', '2.0.0'] }); + expect(status).toBe(400); + expect(body.success).toBe(false); + expect(body.error.code).toBe('VALIDATION_ERROR'); + expect(body.error.message).toContain('"version"'); + expect(body.error.message).toContain('2 times'); + // The array never reaches `version?: string`. + expect(spy.getVersions).toEqual([]); + }); + + it('DELETE answers 400 and performs NO deletion of either kind', async () => { + // The defect answered 200 here, having quietly skipped the full uninstall + // and asked the durable registry to delete "1.0.0,2.0.0". + const { spy, drive } = harness({ protocol: true }); + const { status, body } = await drive('DELETE', { version: ['1.0.0', '2.0.0'] }); + expect(status).toBe(400); + expect(body.error.code).toBe('VALIDATION_ERROR'); + expect(spy.deleteVersions).toEqual([]); + expect(spy.protocolCalls).toBe(0); + }); + + it('both verbs answer the identical body — one rule, one answer', async () => { + const g = await harness().drive('GET', { version: ['a', 'b'] }); + const d = await harness({ protocol: true }).drive('DELETE', { version: ['a', 'b'] }); + expect(g.status).toBe(d.status); + expect(g.body).toEqual(d.body); + }); + + it('two IDENTICAL values are still two occurrences, and still refused', async () => { + // Deliberate: the rule is "supply it at most once", which a client can check + // without knowing our semantics. "at most one DISTINCT value" would be a + // de-duplication rule nobody can predict. + const { spy, drive } = harness({ protocol: true }); + const { status } = await drive('DELETE', { version: ['1.0.0', '1.0.0'] }); + expect(status).toBe(400); + expect(spy.protocolCalls).toBe(0); + }); + + it('three or more occurrences are reported by count', async () => { + const { body } = await harness().drive('GET', { version: ['1', '2', '3'] }); + expect(body.error.message).toContain('3 times'); + }); +}); diff --git a/packages/rest/src/package-routes.ts b/packages/rest/src/package-routes.ts index f20c0f6184..04226a227e 100644 --- a/packages/rest/src/package-routes.ts +++ b/packages/rest/src/package-routes.ts @@ -9,6 +9,67 @@ import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js'; /** * Options for package route registration. */ +/** + * The outcome of reading a query parameter that this API declares as + * single-valued. `ok: false` carries the multiplicity so the refusal can say + * what it saw rather than only that it refused. + */ +type SingleQueryRead = + | { readonly ok: true; readonly value: string | undefined } + | { readonly ok: false; readonly count: number }; + +/** + * Read a query parameter the route declares single-valued out of the shape the + * transport contract actually declares (#6307). + * + * `IHttpRequest.query` is `Record` — a repeated + * parameter is an ARRAY, and that is not a hypothetical arm of the union: the + * `node:http` adapter (`@objectstack/http-conformance`'s `NodeHttpServer`) + * hands `?version=a&version=b` through as `['a','b']`, measured over a socket. + * The Hono adapter happens to collapse it to the first value before a handler + * ever sees it, so the two adapters answer one contract-legal request + * differently — which is precisely why the CONSUMER has to handle the shape it + * was told to expect rather than lean on whichever server booted. + * + * ## Why repetition is refused rather than resolved + * + * `?version=1.0.0&version=2.0.0` is a well-formed request carrying two + * conflicting intents. Picking one silently is a wrong answer delivered as a + * success, and on `DELETE` it silently changes the OPERATION'S SCOPE: any + * truthy `version` skips the `protocol.deletePackage` full-uninstall branch, so + * a repeated parameter degraded a full uninstall into a narrow version-delete + * and answered `200`. The server does not get to choose which of a caller's two + * versions it meant; it says so. + * + * The rule is deliberately about MULTIPLICITY, not about shape: the parameter + * may be supplied at most once. A one-element array is one occurrence encoded + * differently by an adapter and is accepted; an empty array is no occurrence. + * Two identical values (`?version=1.0.0&version=1.0.0`) are still two + * occurrences and are still refused — "at most one *distinct* value" would be a + * de-duplication rule no caller can predict, while "supply it at most once" is + * checkable client-side without knowing anything about our semantics. + * + * This is NOT tolerance for off-spec input: the contract already declares the + * array. It is the consumer finally handling a declared shape. + */ +function readSingleQueryValue(raw: string | string[] | undefined): SingleQueryRead { + if (Array.isArray(raw)) { + // length 0 → the parameter was not supplied; length 1 → supplied once. + return raw.length > 1 ? { ok: false, count: raw.length } : { ok: true, value: raw[0] }; + } + return { ok: true, value: raw }; +} + +/** + * The one refusal message for a repeated single-valued parameter, so `GET` and + * `DELETE` answer the SAME rule identically — two different answers for one + * parameter would just be a new inconsistency. + */ +function repeatedQueryParamMessage(name: string, count: number): string { + return `The "${name}" query parameter was supplied ${count} times. Supply it at most once — ` + + `this endpoint will not choose between conflicting values.`; +} + export interface PackageRoutesOptions { /** * Protocol service (ObjectStackProtocol) — provides access to in-memory @@ -77,7 +138,10 @@ export interface PackageRoutesOptions { * * Generic conditions reuse the STANDARD catalog rather than becoming registered * synonyms of it: a missing request field is `MISSING_REQUIRED_FIELD`, an absent - * package is `RESOURCE_NOT_FOUND`, an unexpected throw is `INTERNAL_ERROR`. Only + * package is `RESOURCE_NOT_FOUND`, a request whose own parameters are + * self-contradictory is `VALIDATION_ERROR` (the catalog's generic validation + * failure, and what `HttpStatusErrorCodeMap` already names a bare 400 — see + * `readSingleQueryValue`), an unexpected throw is `INTERNAL_ERROR`. Only * the package-specific outcomes are registered — `PACKAGE_MANIFEST_INVALID`, * `PACKAGE_PUBLISH_FAILED`, `PACKAGE_DELETE_PARTIAL`, `PACKAGE_DELETE_FAILED`. */ @@ -201,7 +265,12 @@ export function registerPackageRoutes( handler: async (req, res) => { try { const packageId = req.params.id; - const version = req.query?.version || 'latest'; + const requested = readSingleQueryValue(req.query?.version); + if (!requested.ok) { + sendError(res, 400, 'VALIDATION_ERROR', repeatedQueryParamMessage('version', requested.count)); + return; + } + const version = requested.value || 'latest'; // Try database first (richer data from publish) const pkg = await packageService.get(packageId, version); @@ -241,7 +310,15 @@ export function registerPackageRoutes( handler: async (req, res) => { try { const packageId = req.params.id; - const version = req.query?.version; + // Refused BEFORE the branch below, because the branch below is exactly + // what a repeated `?version=` silently changed (#6307): the truthiness of + // `version` is what decides full uninstall vs version-scoped delete. + const requested = readSingleQueryValue(req.query?.version); + if (!requested.ok) { + sendError(res, 400, 'VALIDATION_ERROR', repeatedQueryParamMessage('version', requested.count)); + return; + } + const version = requested.value; // [#2747] A FULL uninstall (no version pin) goes through // protocol.deletePackage — one uninstall semantic, not three dialects: From 09dd46b2b813314c0334f717b1055dc22a3bafc0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 01:48:18 +0000 Subject: [PATCH 2/3] chore(changeset): #6307 repeated version query param --- ...age-routes-repeated-version-query-param.md | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 .changeset/package-routes-repeated-version-query-param.md diff --git a/.changeset/package-routes-repeated-version-query-param.md b/.changeset/package-routes-repeated-version-query-param.md new file mode 100644 index 0000000000..e13f4119b0 --- /dev/null +++ b/.changeset/package-routes-repeated-version-query-param.md @@ -0,0 +1,59 @@ +--- +"@objectstack/rest": minor +--- + +fix(rest): a repeated `?version=` on `/packages/:id` is refused, not silently resolved (#6307) + +`IHttpRequest.query` is declared `Record` — a repeated +query parameter arrives as an **array**. Both `/api/v1/packages/:id` handlers read +it as a string and passed it straight to `PackageService.get/delete`, whose +parameter is `version?: string`. Measured on `main` before the fix: + +``` +GET /packages/com.acme.crm?version=1.0.0&version=2.0.0 + → packageService.get('com.acme.crm', ['1.0.0','2.0.0']) +DELETE /packages/com.acme.crm?version=1.0.0&version=2.0.0 + → packageService.delete('com.acme.crm', ['1.0.0','2.0.0']) + → 200 { message: 'Deleted com.acme.crm@1.0.0,2.0.0' } +``` + +The `DELETE` line is the sharp one. `if (!version && protocol.deletePackage)` is +what gates the **full uninstall** (#2747: the package's metadata rows, the durable +`sys_packages` record, and the registered data-plane cleanups — plugin-security +revoking its permission sets and bindings). Any truthy `version` skips it, so a +repeated parameter silently narrowed the *scope of the operation* on a destructive +verb and still reported success. + +**Both verbs now refuse the ambiguity** with `400 VALIDATION_ERROR` +(`The "version" query parameter was supplied 2 times. Supply it at most once — this +endpoint will not choose between conflicting values.`). `?version=a&version=b` is a +well-formed request carrying two conflicting intents; picking one silently is a +wrong answer delivered as a `200`. The rule is identical on both verbs — one +parameter, one answer — and the code comes from ADR-0112's **standard** catalog +rather than a newly registered synonym, because "this request contradicts itself" +is a generic validation condition. + +The rule is about **multiplicity, not shape**: the parameter may be supplied at +most once. A one-element array is one occurrence encoded differently by an adapter +and is accepted; an empty array is no occurrence. Two identical values are still +two occurrences and are still refused — "at most one *distinct* value" would be a +de-duplication rule no client can predict, while "supply it at most once" is +checkable client-side. + +**Not tolerance for off-spec input.** The contract already declared the array; the +consumer simply never handled a shape it was told to expect. + +**Nothing that works today changes.** A single `?version=1.0.0`, no `version` at +all, and an empty `?version=` all behave exactly as before — including the full +uninstall still being reached when no version is supplied. No in-repo caller, +documented example or SDK path repeats the parameter (`client.packages.get` builds +`?version=` from a single `version?: string`), so the new 400 is unreachable from +any supported client. It is `minor` rather than `patch` only because a request +shape that used to answer `200` now answers `400`. + +Adapter note, measured over a real socket: the `node:http` adapter +(`NodeHttpServer`) hands `['1.0.0','2.0.0']` to the handler as the contract +declares, while the Hono adapter collapses a repeat to the first value before any +handler sees it. Both are contract-legal (the union permits either), which is +exactly why the consumer must handle the declared shape rather than depend on +which server booted. From 70b48553483cf9d54bf105a5c5d9d8b838a04d31 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 02:33:18 +0000 Subject: [PATCH 3/3] style(rest): keep the PackageRoutesOptions docstring attached to its interface --- packages/rest/src/package-routes.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/rest/src/package-routes.ts b/packages/rest/src/package-routes.ts index 04226a227e..4cd9dc777e 100644 --- a/packages/rest/src/package-routes.ts +++ b/packages/rest/src/package-routes.ts @@ -6,9 +6,6 @@ import type { PackageService } from '@objectstack/service-package'; import { sendOk, sendError } from '@objectstack/types'; import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js'; -/** - * Options for package route registration. - */ /** * The outcome of reading a query parameter that this API declares as * single-valued. `ok: false` carries the multiplicity so the refusal can say @@ -70,6 +67,9 @@ function repeatedQueryParamMessage(name: string, count: number): string { + `this endpoint will not choose between conflicting values.`; } +/** + * Options for package route registration. + */ export interface PackageRoutesOptions { /** * Protocol service (ObjectStackProtocol) — provides access to in-memory