diff --git a/.changeset/17416-packages-get-version-scope.md b/.changeset/17416-packages-get-version-scope.md new file mode 100644 index 0000000000..627bd47da4 --- /dev/null +++ b/.changeset/17416-packages-get-version-scope.md @@ -0,0 +1,51 @@ +--- +'@objectstack/runtime': minor +--- + +fix(runtime): `GET /api/v1/packages/:id` honours `?version=` instead of silently ignoring it (#17416) + +The route accepted a `?version=` query parameter and the only surface serving it +never read the parameter. A caller asking for a version that is not installed +was answered `200` with the **installed** row, and nothing in the status, +headers or body distinguished that from a version-scoped read that actually +happened. + +The parameter is not hypothetical traffic: `ScopedEnvironmentClient.packages.get` +(`@objectstack/client`) declares `version?: string` and appends it, so the SDK +has been sending a parameter the runtime dropped. The handler that honoured it +— the REST registrar's twin of this route — was removed with the duplicate +response shape, and the dispatcher's `/packages` domain never had that read to +inherit. + +``` +FROM GET /api/v1/packages/com.acme.crm?version=99.0.0 (1.0.0 installed) + -> 200 { data: { manifest: { version: "1.0.0" }, … } } + +TO GET /api/v1/packages/com.acme.crm?version=99.0.0 + -> 404 { error: { message: "Package 'com.acme.crm' version '99.0.0' not + found — installed version is '1.0.0'" } } +``` + +**What does not change.** The unversioned read is untouched, down to the row and +the writability verdict it stamps — pinned as the lit control beside the new +assertions, because a green on only the scoped path would also pass with the +ordinary read broken. `?version=` naming the installed version is served +exactly as the unversioned read is, and so is `?version=latest`: the deleted +handler read `requested.value || 'latest'` and its store resolved `latest` to +the newest row, so "no version" and "`latest`" named one request there and name +one request here. An id the registry does not hold keeps its existing 404 +wording whether or not `?version=` rode along — a package that is not installed +cannot be at the wrong version. + +**This is request-side only.** The response shape is not touched, so the route +still answers with exactly one body shape; comparison is exact string equality +on the version, the same predicate the durable package store uses (`AND version += ?`), so the two answers to "is this package at version v" cannot drift into +semver-range semantics at one of them. + +A repeated `?version=a&version=b` is no longer resolved by silently choosing +one — it is answered with a refusal naming what was seen. The repo's one rule +for a repeated single-valued parameter answers `400 VALIDATION_ERROR` and is +the right end state for this door too; it is not restated here, because the +helper that owns that rule and its message is not exported from +`@objectstack/rest`. diff --git a/packages/runtime/src/domains/packages-get-version-scope.test.ts b/packages/runtime/src/domains/packages-get-version-scope.test.ts new file mode 100644 index 0000000000..9318d9748c --- /dev/null +++ b/packages/runtime/src/domains/packages-get-version-scope.test.ts @@ -0,0 +1,207 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `GET /api/v1/packages/:id?version=` is a VERSION-SCOPED read, and its answer + * is distinguishable from the unversioned one (#17416). + * + * ## The defect + * + * The route accepted `?version=` and the only surface serving it never read the + * parameter. `ScopedEnvironmentClient.packages.get(id, version)` declares + * `version?: string` and appends it, so `?version=99.0.0` against an installed + * `1.0.0` was answered `200` with the `1.0.0` row — and no status, header or + * field told the caller which of the two reads it got. The handler that + * honoured it (with the store predicate `AND version = ?`) went with the REST + * twin in #14503 / #16628; this dispatcher domain never had it to inherit. + * + * ## What is pinned, and why each half is here + * + * The card's acceptance criterion is a PAIR, because either half alone passes + * for the wrong reason: + * + * - **§1 the discriminating pin** — the same request with and without + * `?version=` must not produce the same + * response. Asserted as `status` PLUS the discriminating field (the error + * message naming the version, and the body carrying no package row), ⛔ + * never as "it returned 200": that reading is precisely what hid this + * defect for the whole of its life. + * - **§2 the lit control** — the unversioned read still answers exactly as it + * did, down to the row it serves. A green that only exercised the new + * scoped path would also be green if the ordinary read were broken, and + * this door is the single implementation of the route. + * + * §3 pins the two requests that mean "the installed row" — no parameter and + * `?version=latest` — as ONE request, which is the contract the deleted + * handler published (`requested.value || 'latest'`). §4 pins the repeated + * parameter as not-a-silent-success: `?version=a&version=b` carries two + * conflicting intents, and the answer names what it saw rather than choosing. + * ⚠️ §4 asserts the DEFECT CLASS is closed (no `200` with the installed row), + * deliberately not the exact status, because the repo's one rule for a repeated + * single-valued parameter answers `400 VALIDATION_ERROR` and is unreachable + * from this package today — see `readRequestedVersion`'s header in + * `packages.ts`. So this pin stays green when that rule lands here. + * + * ## The harness + * + * A real {@link SchemaRegistry} behind the real {@link HttpDispatcher}, the way + * `packages-writable-verdict.test.ts` and `packages-readonly-gate.test.ts` next + * door do it — so the rows under test are what `installPackage` actually + * produces and the answers are the ones the composed door gives. + */ + +import { describe, it, expect } from 'vitest'; +import { SchemaRegistry } from '@objectstack/objectql'; +import { HttpDispatcher } from '../http-dispatcher.js'; + +const PKG = 'com.acme.crm'; +/** The version actually installed — the row every read below can legitimately serve. */ +const INSTALLED = '1.0.0'; +/** The card's own repro value: a version this registry does not hold. */ +const ABSENT = '99.0.0'; + +function make() { + const registry = new SchemaRegistry({ logLevel: 'silent' } as any); + registry.installPackage({ id: PKG, name: PKG, version: INSTALLED, scope: 'project', type: 'app' } as any); + const objectql = { registry, manifests: new Map() }; + const kernel: any = { + context: { getService: (name: string) => (name === 'objectql' ? objectql : null) }, + }; + return new HttpDispatcher(kernel); +} + +/** Holds the ADR-0106 D4 read set; the caller gate is not this file's subject. */ +const reader = (): any => ({ + request: {}, + environmentId: 'pkg-get-version-scope-test', + executionContext: { userId: 'u_admin', isSystem: false, systemPermissions: ['manage_metadata', 'studio.access'] }, +}); + +/** + * `GET /packages/:id` against an EXISTING host. + * + * ⛔ Every case that compares two RESPONSE BODIES must issue both requests + * through this, against ONE `make()`. `SchemaRegistry.installPackage` stamps + * `installedAt` and `updatedAt` from a single `new Date()` per install + * (`packages/objectql/src/registry.ts`), and both are declared record fields + * that `toPackageResponse` carries to the wire. So two hosts hold two rows + * whose stamps differ whenever the installs straddle a millisecond boundary, + * and a whole-body `toEqual` between them fails on the clock rather than on + * anything this door did — a flake that passes on a re-run and comes back. + * + * One host makes it deterministic rather than merely likelier: both responses + * are projections of ONE row, so there is no second install and no second + * clock read to disagree. Nothing on the read path reads a clock at all — + * neither `toPackageResponse` (an allowlist copy) nor `withWritableVerdict` (a + * spread) nor the dispatcher's `success()` envelope — so with one install the + * stamps cannot move, at any scheduling. + * + * ⛔ The repair for such a failure is this shape, ⛔ never dropping the two + * stamps out of the comparison: whole-body equality is what makes «the same + * request» mean the same RESPONSE rather than the same status. + */ +async function read(dispatcher: HttpDispatcher, query: Record | undefined) { + const r = await dispatcher.handlePackages(`/${PKG}`, 'GET', undefined, query, reader()); + return { status: r.response?.status ?? 200, body: r.response?.body }; +} + +/** One request on a host of its own — for the cases that compare against no other body. */ +async function get(query: Record) { + return read(make(), query); +} + +describe('#17416 GET /packages/:id — ?version= scopes the read', () => { + describe('§1 the discriminating pin — with and without ?version= are not the same answer', () => { + it('a non-installed ?version= is NOT answered with the installed row', async () => { + // ONE host: the criterion is «the SAME request with and without + // `?version=`», so both answers have to be about the same row — + // two hosts would let a difference come from the rows instead. + const host = make(); + const scoped = await read(host, { version: ABSENT }); + const unscoped = await read(host, {}); + + // The discriminating field, not the status alone. + expect(scoped.status).toBe(404); + expect(scoped.body?.error?.message).toContain(ABSENT); + expect(scoped.body?.error?.message).toContain(INSTALLED); + // ⛔ No package row rode out on the refusal. + expect(scoped.body?.data).toBeUndefined(); + + // ...and the SAME request without the parameter is a different answer. + expect(unscoped.status).toBe(200); + expect(unscoped.body?.data?.manifest?.version).toBe(INSTALLED); + expect(scoped.status).not.toBe(unscoped.status); + expect(scoped.body).not.toEqual(unscoped.body); + }); + + it('the installed version IS served when it is the one asked for', async () => { + const r = await get({ version: INSTALLED }); + expect(r.status).toBe(200); + expect(r.body?.data?.manifest?.id).toBe(PKG); + expect(r.body?.data?.manifest?.version).toBe(INSTALLED); + }); + + it('an id this registry does not hold keeps the wording packages-single-door pins', async () => { + const dispatcher = make(); + const r = await dispatcher.handlePackages('/com.absent.pkg', 'GET', undefined, { version: ABSENT }, reader()); + expect(r.response?.status).toBe(404); + // ⛔ The id 404 is NOT re-worded by the version scope: a package that + // is not here cannot be "at the wrong version". + expect(r.response?.body?.error?.message).toBe(`Package 'com.absent.pkg' not found`); + }); + }); + + describe('§2 the lit control — the unversioned read is untouched', () => { + it('answers 200 with the installed row and its writability verdict', async () => { + const r = await get({}); + expect(r.status).toBe(200); + expect(r.body?.success).toBe(true); + expect(r.body?.data?.manifest?.id).toBe(PKG); + expect(r.body?.data?.manifest?.version).toBe(INSTALLED); + // The #14375 verdict the door has always stamped, still stamped. + expect(r.body?.data?.writable).toBe(true); + }); + + it('is byte-identical to the read with no query object at all', async () => { + const host = make(); + const withEmpty = await read(host, {}); + const withNone = await read(host, undefined); + expect(withNone.status).toBe(200); + expect(withNone.body).toEqual(withEmpty.body); + }); + }); + + describe('§3 `latest` and absent name the SAME request', () => { + it('?version=latest serves the installed row, exactly as no parameter does', async () => { + const host = make(); + const latest = await read(host, { version: 'latest' }); + const unscoped = await read(host, {}); + expect(latest.status).toBe(200); + expect(latest.body).toEqual(unscoped.body); + }); + }); + + describe('§4 a repeated ?version= is not resolved silently', () => { + it('two conflicting values are not answered 200 with the installed row', async () => { + const r = await get({ version: [ABSENT, INSTALLED] }); + // The defect class: a success carrying a row the caller did not ask for. + expect(r.status).not.toBe(200); + expect(r.body?.data).toBeUndefined(); + // It says what it saw rather than choosing one of the two. + expect(r.body?.error?.message).toContain('supplied 2 times'); + }); + + it('ONE occurrence encoded as a one-element array is one occurrence', async () => { + const host = make(); + const arr = await read(host, { version: [INSTALLED] }); + const str = await read(host, { version: INSTALLED }); + expect(arr.status).toBe(200); + expect(arr.body).toEqual(str.body); + }); + + it('a one-element array naming an absent version still refuses', async () => { + const r = await get({ version: [ABSENT] }); + expect(r.status).toBe(404); + expect(r.body?.error?.message).toContain(ABSENT); + }); + }); +}); diff --git a/packages/runtime/src/domains/packages.ts b/packages/runtime/src/domains/packages.ts index 5716ac29c3..55742fe2e8 100644 --- a/packages/runtime/src/domains/packages.ts +++ b/packages/runtime/src/domains/packages.ts @@ -531,6 +531,102 @@ function withWritableVerdict` and the array arm + * is live on every adapter this repo ships, so `?version=a&version=b` reaches + * this door as `['a','b']` — a well-formed request carrying two conflicting + * intents. Picking one silently is a wrong answer delivered as a success, which + * is the defect class this card is about, so it is not done. The repo's ONE + * rule for this condition answers `400 VALIDATION_ERROR` + * (`refuseRepeatedQueryParams` / `repeatedQueryParamMessage` in + * `packages/rest/src/query-multiplicity.ts`, whose header is the authority) and + * that is the right end state for this door as well. ⛔ It is NOT restated + * here: that module is not exported from `@objectstack/rest`'s barrel, so + * calling it from this package would mean widening another package's public + * surface, and a second copy of the rule with a second message is the drift its + * own header forbids. Until the rule is reachable, this door says what it saw + * and names no version — it does not answer `200` with the installed row. + * + * A one-element array is one occurrence encoded differently by an adapter and + * is unwrapped, per that same rule's stated semantics. + */ +type RequestedVersion = + | { readonly kind: 'unscoped' } + | { readonly kind: 'exact'; readonly value: string } + | { readonly kind: 'repeated'; readonly count: number }; + +function readRequestedVersion(raw: unknown): RequestedVersion { + if (Array.isArray(raw)) { + if (raw.length > 1) return { kind: 'repeated', count: raw.length }; + return readRequestedVersion(raw[0]); + } + if (typeof raw !== 'string') return { kind: 'unscoped' }; + // `latest` is the installed row — see this type's header. + if (raw === 'latest') return { kind: 'unscoped' }; + return { kind: 'exact', value: raw }; +} + +/** The one sentence this door answers a repeated `?version=` with. */ +function repeatedVersionMessage(id: string, count: number): string { + return `Package '${id}' — the "version" query parameter was supplied ${count} times, ` + + 'so this read names no single version. Supply it at most once.'; +} + +/** + * The version the registry's row for a package IS (#17416). + * + * `manifest.version` is read FIRST because it is the field the producer + * actually writes: `SchemaRegistry.installPackage` stores a projection of the + * manifest, and nothing in the registry populates `installedVersion` — which + * `packages/spec/src/kernel/package-registry.zod.ts` declares `.optional()` and + * documents as a mirror («Mirrors manifest.version for quick access»). So the + * mirror is a fallback for a row some other producer filled in, ⛔ never a + * tolerated alias for an off-spec spelling: both keys are declared, and a row + * where they disagree is a producer defect this door cannot repair. + * + * Comparison is exact string equality — the same predicate the durable store + * uses (`AND version = ?` in `@objectstack/service-package`), so the two + * answers about "is this package at version v" cannot drift into semver range + * semantics at one of them. + */ +function installedVersionOf(pkg: unknown): string | undefined { + const src = pkg as { manifest?: { version?: unknown }; installedVersion?: unknown } | null; + const fromManifest = src?.manifest?.version; + if (typeof fromManifest === 'string' && fromManifest !== '') return fromManifest; + const mirror = src?.installedVersion; + return typeof mirror === 'string' && mirror !== '' ? mirror : undefined; +} + export async function handlePackagesRequest(deps: DomainHandlerDeps, path: string, method: string, body: any, query: any, _context: HttpProtocolContext): Promise { const m = method.toUpperCase(); @@ -1207,12 +1303,38 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin } } - // GET /packages/:id → get package + // GET /packages/:id[?version=] → get package, scoped to a version when + // one is asked for (#17416 — see `readRequestedVersion`). if (parts.length === 1 && m === 'GET') { const denied = requireReadCapability(deps, _context); if (denied) return denied; const id = decodeURIComponent(parts[0]); const pkg = registry.getPackage(id); if (!pkg) return { handled: true, response: deps.error(`Package '${id}' not found`, 404) }; + // [#17416] The version scope is applied AFTER the id lookup, so an + // id this registry does not hold keeps answering the wording + // `packages-single-door.test.ts` pins — `Package '' not found`, + // whether or not `?version=` rode along. Only a package that IS + // here can be at the wrong version. + const requested = readRequestedVersion(query?.version); + if (requested.kind === 'repeated') { + return { + handled: true, + response: deps.error(repeatedVersionMessage(id, requested.count), 404), + }; + } + if (requested.kind === 'exact') { + const present = installedVersionOf(pkg); + if (present !== requested.value) { + return { + handled: true, + response: deps.error( + `Package '${id}' version '${requested.value}' not found` + + (present ? ` — installed version is '${present}'` : ''), + 404, + ), + }; + } + } // [#14375] Same verdict, same predicate as the list door — and // [#14309] the same project-then-stamp order, for the same reason. return {