From 10f9463cbbc1c3dabf3b50733f208aa7c165b07c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 10:08:31 +0000 Subject: [PATCH] fix(rest): give POST /packages/publish an owner on every boot (#7563) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /api/v1/packages/publish` answered 405 with `Allow: DELETE, GET, HEAD, PATCH` on a live showcase boot. Not one of those verbs belongs to the publish surface — POST is the only verb it has ever had. They are `/packages/:id`'s method set, offered because with the publish route unmounted that pattern was the only registration still matching the path, with `id = "publish"`. The caller was told "this path exists, use another method", and every method on offer would have operated on a package literally named `publish`. Two facts produced it, and both are repaired. The REST package registrar was gated on `ctx.getService('package')` resolving at the single instant `RestApiPlugin.start()` ran. `objectstack serve` registers the capability providers (`requires: ['marketplace']` → PackageServicePlugin) AFTER `createRestApiPlugin`, and start order follows registration order for plugins with no dependency edge between them — so the deployments that DO compose a package service are exactly the ones that answered "no" at mount time. The service is now handed to the registrar as a resolver and read per request, which makes the answer independent of composition order instead of silently encoding it. And `POST /packages/publish` has no dispatcher twin, so "not mounted" never degraded to the 404 `direct-mount-composition.ts` documented — it degraded to a sibling's 405. It therefore mounts unconditionally and answers its own honest 404, naming the surface rather than a package id, where no package service is composed. The other three package routes deliberately do NOT follow: each shadows a live dispatcher twin at a byte-identical pattern, so mounting them without a service would replace three working routes with a degraded refusal. The #7526 route-ledger <-> live-mount parity gate did not flag this: the row was PINNED in UNEXERCISED_BY_THIS_BOOT, reasoned as "the registrar is service-gated and this boot composes none". The reason was true and the conclusion was wrong — an unmounted route is not automatically an unanswered one. The pin is deleted (the route is observable for real now), and the pin rule is tightened: a pinned path that any OTHER pattern matches now fails the gate. That probe runs across every verb on purpose, because this class is invisible to a method-scoped one — nothing registers POST on `/packages/:id`, so `resolveMountedRoute('POST', ...)` reports a clean absence while the adapter's `allowedMethodsForPath()` ignores the request method and builds the 405 from whatever else matches the path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Wpp7qrGdj3e8vdX5LmkmM5 --- .changeset/packages-publish-mount-or-404.md | 41 +++ ...e-ledger-live-mount-parity.dogfood.test.ts | 101 ++++++- packages/rest/src/direct-mount-composition.ts | 90 ++++-- .../src/direct-mount-introspection.test.ts | 49 +++- packages/rest/src/direct-mount.ts | 12 +- .../src/package-envelope.conformance.test.ts | 2 +- .../rest/src/package-publish-mount.test.ts | 268 ++++++++++++++++++ .../package-routes-query-multiplicity.test.ts | 2 +- packages/rest/src/package-routes.ts | 104 ++++++- packages/rest/src/rest-route-ledger.ts | 4 +- 10 files changed, 610 insertions(+), 63 deletions(-) create mode 100644 .changeset/packages-publish-mount-or-404.md create mode 100644 packages/rest/src/package-publish-mount.test.ts diff --git a/.changeset/packages-publish-mount-or-404.md b/.changeset/packages-publish-mount-or-404.md new file mode 100644 index 0000000000..3e69b9dab4 --- /dev/null +++ b/.changeset/packages-publish-mount-or-404.md @@ -0,0 +1,41 @@ +--- +"@objectstack/rest": patch +--- + +Give `POST /api/v1/packages/publish` an owner on every boot (#7563) + +On a live showcase boot, `POST /api/v1/packages/publish` answered **405** with +`Allow: DELETE, GET, HEAD, PATCH`. Not one of those verbs belongs to the publish +surface — `POST` is the only verb it has ever had. They are `/packages/:id`'s +method set, offered because with the publish route unmounted that pattern was +the only registration still matching the path, with `id = "publish"`. A caller +was told "this path exists, use another method", and every method on offer would +have operated on a package literally named `publish`. + +Two facts produced it, and both are repaired. + +The REST package registrar was gated on `ctx.getService('package')` resolving at +the single instant `RestApiPlugin.start()` ran. `objectstack serve` registers the +capability providers (`requires: ['marketplace']` → `PackageServicePlugin`) +*after* `createRestApiPlugin`, and start order follows registration order for +plugins with no dependency edge between them — so the deployments that do +compose a package service are precisely the ones that answered "no" at mount +time. The service is now handed to the registrar as a resolver and read per +request, which makes the answer independent of composition order instead of +silently encoding it. + +And `POST /packages/publish` has no dispatcher twin, so "not mounted" never +degraded to the 404 the composition documented — it degraded to a sibling's 405. +It therefore mounts unconditionally and answers its own honest 404, naming the +surface rather than a package id, where no package service is composed. The +other three package routes deliberately do **not** follow: each shadows a live +dispatcher twin at a byte-identical pattern, so mounting them without a service +would replace three working routes with a degraded refusal. + +The route-ledger ↔ live-mount parity gate (#7526) had this row **pinned** as +unobservable, reasoned as "the registrar is service-gated and this boot composes +none". The reason was true and the conclusion was wrong: an unmounted route is +not automatically an unanswered one. The pin is deleted (the route is now +observable for real), and the pin rule itself is tightened — a pinned path that +some *other* pattern answers now fails the gate, because that is the disguise +the gate already refuses for every unpinned row. diff --git a/packages/qa/dogfood/test/route-ledger-live-mount-parity.dogfood.test.ts b/packages/qa/dogfood/test/route-ledger-live-mount-parity.dogfood.test.ts index 6740bb0714..0285544316 100644 --- a/packages/qa/dogfood/test/route-ledger-live-mount-parity.dogfood.test.ts +++ b/packages/qa/dogfood/test/route-ledger-live-mount-parity.dogfood.test.ts @@ -113,20 +113,33 @@ function collectLedgerRows(): LedgerRow[] { * Rows this boot structurally cannot observe, each with the reason. * * ⚠️ READ THIS BEFORE ADDING A LINE. A pin is NOT "this route is allowed to be - * missing" — it is "this boot cannot see it", and the gate asserts BOTH - * directions of that claim: a pinned row that turns out to be reachable fails - * too, so the set can only shrink by accident and never grow by accident. The - * moment a pin starts meaning "we know it is broken", it has become the - * declaration-instead-of-observation this whole file exists to end. A route - * that is broken gets fixed or gets an issue, not a line here. + * missing" — it is "NOTHING answers this path on this boot", and the gate + * asserts BOTH halves of that claim: a pinned row that turns out to be + * reachable fails, and (since #7563) so does one whose path is answered by a + * DIFFERENT pattern. So the set can only shrink by accident and never grow by + * accident. The moment a pin starts meaning "we know it is broken", it has + * become the declaration-instead-of-observation this whole file exists to end. + * A route that is broken gets fixed or gets an issue, not a line here. + * + * ## What #7563 taught this list + * + * `POST /api/v1/packages/publish` used to be pinned here, reasoned as "the + * registrar is service-gated and this boot composes no `package` service". + * That reason was true and the conclusion was wrong: an unmounted route is not + * automatically an UNANSWERED one. With nobody owning the path, the + * dispatcher's `/packages/:id` matched it (`id = "publish"`) and the router + * answered 405 with THAT route's `Allow` set — the "LEDGERED BUT NOT MOUNTED, + * and DISGUISED" failure this gate spells out for every unpinned row, invisible + * for the one class that had been excused from the check. A conditional mount + * was therefore the one shape the gate could not model, so the pin's second + * half below is now checked as strictly as the first. The route itself mounts + * unconditionally as of #7563 and needs no pin at all. */ const UNEXERCISED_BY_THIS_BOOT: Record = { '* /api/v1/auth/**': 'plugin-auth mounts one rawApp.all() catch-all on Hono directly, so no auth route ever passes through the IHttpServer port. Audited against better-auth\'s live auth.api table by auth-route-ledger.conformance.test.ts instead', '* /api/v1/apps/**': 'the ADR-0121 declarative-endpoint carve-out is a setFallbackHandler seam, not a route — being invisible to a route table is the property that makes it incapable of shadowing one (#5040 §1-C)', - 'POST /api/v1/packages/publish': - 'the marketplace publish registrar mounts only when a `package` service occupies the slot (direct-mount-composition.ts); the showcase registers none. Its presence half is already guarded by rest-route-ledger.conformance.test.ts against a capably-mocked RestServer', }; /** Segments a probe path uses for `:params` — must match no literal segment. */ @@ -282,6 +295,57 @@ describe('route ledger ↔ live mount parity (#7526)', () => { expect(stale, `\n${stale.join('\n')}\n`).toEqual([]); }); + // ── …and a pin means NOTHING answers, not "something else answers" (#7563) ─ + // + // The half the pin list was missing. "This boot does not mount it" and "this + // boot does not ANSWER it" are different claims, and only the second one + // makes a pin safe: a pinned path some other registration matches hands the + // caller that route's answer, which is strictly more misleading than the 404 + // the pin implies. `POST /api/v1/packages/publish` was pinned here and + // absorbed by `/packages/:id` for exactly that reason (#7563). + // + // ⚠️ THE PROBE IS ACROSS ALL VERBS, and that is the whole subject. The way + // this class actually surfaced was NOT a same-method disguise: nothing + // registers POST on `/packages/:id`, so `resolveMountedRoute('POST', …)` + // answers `undefined` and a method-scoped check sees a clean absence. The + // adapter's 405 seam does not work that way — `allowedMethodsForPath()` + // matches the concrete PATH against every registered pattern IGNORING the + // request's method, and answers 405 with whatever verbs that turns up. So a + // pinned path is only genuinely unanswered when NO verb matches it; one that + // matches under some other verb answers 405 + `Allow`, naming another route's + // methods, which is the defect this file's pin list shipped. + // + // A row genuinely served by a broader pattern is not this: the ledger says so + // with `servedBy`, and direction 1 checks it there. + const PROBED_VERBS = ['GET', 'POST', 'PUT', 'PATCH', 'DELETE'] as const; + + it('no pinned path is matched by any OTHER pattern, under any verb (#7563)', () => { + const disguised: string[] = []; + + for (const [key, reason] of Object.entries(UNEXERCISED_BY_THIS_BOOT)) { + const sp = key.indexOf(' '); + const pattern = key.slice(sp + 1); + // A `**` row names a prefix family rather than one resolvable path, so + // there is no concrete probe to build; the check above already asserts + // nothing is mounted under the prefix. + if (pattern.includes('*')) continue; + + const path = probePath(pattern); + for (const verb of PROBED_VERBS) { + const resolved = server.resolveMountedRoute!(verb, path); + if (!resolved || resolved.pattern === pattern) continue; + disguised.push( + `${key} — pinned as unobservable ("${reason}"), but \`${resolved.pattern}\` matches ${path} under ` + + `${verb}. The pin claims a caller gets nothing here; a caller actually gets that route's answer — ` + + `its 405 + \`Allow\` when the verbs differ, its body when they do not. Mount an owner for this path ` + + '(so it can 404 for itself), or declare `servedBy` if that pattern legitimately serves it.', + ); + } + } + + expect(disguised, `\n${disguised.join('\n')}\n`).toEqual([]); + }); + // ── Direction 2: every live mount is ledgered ───────────────────────────── it('every mounted route is ledgered', () => { const exact = new Set(ledgerRows.filter((r) => !isWildcardRow(r)).map((r) => `${r.method} ${r.pattern}`)); @@ -325,6 +389,27 @@ describe('route ledger ↔ live mount parity (#7526)', () => { .toEqual({ method: 'GET', pattern: '/api/v1/meta/:type' }); }); + // The publish path, pinned in both the currencies that matter: which + // registration the router hands it to, and what a caller actually receives. + it('POST /packages/publish is owned by the publish route, not absorbed by /packages/:id (#7563)', async () => { + // `/packages/:id` is mounted (by the dispatcher) and would match this path + // under GET/DELETE/PATCH — which is the whole reason the 405 was built from + // its method set. The publish registration has to win the POST. + expect(mounted.map((m) => `${m.method} ${m.pattern}`)).toContain('GET /api/v1/packages/:id'); + expect(server.resolveMountedRoute!('POST', '/api/v1/packages/publish')) + .toEqual({ method: 'POST', pattern: '/api/v1/packages/publish' }); + + // …and on the wire. This boot composes no `package` service, so the honest + // answer is the publish route's own 404 naming the surface — never a 405 + // advertising `DELETE, GET, HEAD, PATCH`, which are `/packages/:id`'s verbs + // over a package whose id is the literal string `publish`. + const token = await stack.signIn(); + const res = await stack.apiAs(token, 'POST', '/packages/publish', {}); + expect(res.status).toBe(404); + expect(res.headers.get('Allow')).toBeNull(); + expect((await res.json())?.error?.message).toContain('marketplace publish surface'); + }, 60_000); + // The other two defects, pinned as live-router facts rather than as prose. it('the three #7526 routes resolve to themselves and not to a catch-all sibling', () => { expect(server.resolveMountedRoute!('GET', '/api/v1/meta/object/lead/published')) diff --git a/packages/rest/src/direct-mount-composition.ts b/packages/rest/src/direct-mount-composition.ts index 0406675f92..d8c6da7e01 100644 --- a/packages/rest/src/direct-mount-composition.ts +++ b/packages/rest/src/direct-mount-composition.ts @@ -20,18 +20,26 @@ * Each registrar returns the array it iterated to mount, and that array is what * gets recorded on the `RestServer`. So: * - * - a registrar this boot called ⇒ its routes are enumerable through - * `getRoutes()` and appear in `GET {apiPath}/openapi.json`; - * - a registrar this boot skipped (no `package` service) ⇒ nothing is - * recorded, nothing is documented, and the 404 a caller would get from that - * deployment is what the document says too. + * - a route this boot mounted ⇒ it is enumerable through `getRoutes()` and + * appears in `GET {apiPath}/openapi.json`; + * - a route this boot skipped (a package route needing a `package` service + * that is not there) ⇒ nothing is recorded, nothing is documented, and the + * 404 a caller would get from that deployment is what the document says too. * - * The service gate stays exactly where it was — here, at composition — and the - * record follows it rather than restating it. What is deliberately NOT recorded - * is any verdict about a service that a later phase could still contradict: the - * federation routes mount unconditionally and decide per request whether the - * `external-datasource` service is there (503 if not), so this file records - * them as mounted and says nothing about federation being available. + * [#7563] That second bullet promised a 404 and, for `POST /packages/publish`, + * did not get one: with no owner for the path, the dispatcher's + * `/packages/:id` matched it (`id = "publish"`) and the router answered 405 + * with THAT route's `Allow` set. The publish route therefore mounts on every + * boot and answers its own honest 404 — see `package-routes.ts` for why the + * other three must not follow it. + * + * The service gate stays exactly where it was — around the package registrar's + * routes — and the record follows it rather than restating it. What is + * deliberately NOT recorded is any verdict about a service that a later phase + * could still contradict: the federation routes mount unconditionally and + * decide per request whether the `external-datasource` service is there (503 if + * not), so this file records them as mounted and says nothing about federation + * being available. */ import type { PluginContext } from '@objectstack/core'; @@ -73,27 +81,49 @@ export function mountAndRecordDirectRoutes(composition: DirectMountComposition): const enableProjectScoping = composition.enableProjectScoping ?? false; const projectResolution = composition.projectResolution ?? 'auto'; - // Package management routes — only when the service backing them exists. + // Package management routes. [#7563] The registrar is called on EVERY boot + // and the `package` service is handed to it as a RESOLVER, not as a + // resolved instance — the gate did not move, it stopped being a + // boot-instant snapshot. `objectstack serve` registers the capability + // providers (`requires: ['marketplace']` → `PackageServicePlugin`) after + // `createRestApiPlugin`, and start order follows registration order for + // plugins with no edge between them, so asking once here answered "no + // package service" on precisely the deployments that have one. + // + // `registerPackageRoutes` decides what that resolver's answer means per + // route: `POST /packages/publish` mounts either way (nobody else serves it, + // and an unowned path is answered by a `/packages/:id` sibling's 405 + // instead of a 404 — #7563), the other three only when a service is there + // (they shadow live dispatcher twins). It reports back exactly what it + // mounted, so the record still follows the gate rather than restating it. + const resolvePackageService = () => { + try { + return ctx.getService('package'); + } catch { + // Not registered (yet) — an absence, not a failure. + return undefined; + } + }; try { - const packageService = ctx.getService('package'); - if (packageService) { - // `required` scoping serves ONLY the scoped variant; `auto` serves - // both. Unchanged from the pre-#5822 plugin — expressed as the list - // of bases so the mount and the record cannot disagree about it. - const scopedBase = `${versionedBase}/environments/:environmentId`; - const bases = enableProjectScoping - ? (projectResolution === 'required' ? [scopedBase] : [versionedBase, scopedBase]) - : [versionedBase]; - for (const base of bases) { - recorder.recordDirectMountedRoutes( - registerPackageRoutes(server, packageService, base, { protocol, resolveExecutionContext }), - ); - } - ctx.logger.info('Package management routes registered'); + // `required` scoping serves ONLY the scoped variant; `auto` serves + // both. Unchanged from the pre-#5822 plugin — expressed as the list + // of bases so the mount and the record cannot disagree about it. + const scopedBase = `${versionedBase}/environments/:environmentId`; + const bases = enableProjectScoping + ? (projectResolution === 'required' ? [scopedBase] : [versionedBase, scopedBase]) + : [versionedBase]; + for (const base of bases) { + recorder.recordDirectMountedRoutes( + registerPackageRoutes(server, resolvePackageService, base, { protocol, resolveExecutionContext }), + ); } - } catch (e) { - // Package service not available, skip - ctx.logger.debug('Package service not available, package routes skipped'); + ctx.logger.info('Package management routes registered'); + } catch (e: any) { + // Nothing is recorded on this path, for the same reason the federation + // arm below records nothing when it throws: a registrar that failed + // part-way may have mounted some rows, and under-claiming is the safe + // direction. + ctx.logger.warn('Package management routes registration failed', { error: e?.message }); } // External Datasource Federation routes (ADR-0015): catalog / draft / diff --git a/packages/rest/src/direct-mount-introspection.test.ts b/packages/rest/src/direct-mount-introspection.test.ts index f4727070f4..a25cf75cee 100644 --- a/packages/rest/src/direct-mount-introspection.test.ts +++ b/packages/rest/src/direct-mount-introspection.test.ts @@ -19,12 +19,23 @@ * mounted ⇒ enumerable, and documented * not mounted ⇒ absent from both * - * The second direction has a real trigger: the package registrar is gated on - * the `package` service, so a deployment without it serves no `packages.*` - * route — and must not document one. The federation registrar is NOT gated (it + * The second direction has a real trigger: three of the four package routes are + * gated on the `package` service, so a deployment without it serves none of + * them — and must not document them. The federation registrar is NOT gated (it * mounts always and answers 503 per request), so "mounted" is unconditional * there and the document says so. * + * [#7563] `POST /packages/publish` joined the unconditional cohort, and for a + * reason the second direction is about rather than an exception to it: leaving + * it unmounted did NOT produce the 404 this rule assumes. Nothing else serves + * that verb+path, so the dispatcher's `/packages/:id` matched it with + * `id = "publish"` and the router answered 405 built from THAT route's method + * set. "Not mounted ⇒ absent from the document" is honest only while "not + * mounted" also means "not answered"; where it cannot, the route mounts and + * 404s for itself. The three gated routes have dispatcher twins at their own + * patterns and so keep the original treatment — that split is pinned in + * `package-publish-mount.test.ts`. + * * Both are driven through the REAL composition: `mountAndRecordDirectRoutes` * for the server-level facts, and `createRestApiPlugin().start()` for the * end-to-end one, because the plugin is where a wiring regression would @@ -101,7 +112,11 @@ function createCtx(services: Record) { /** The ledger's own list of the nine, split by registrar. */ const LEDGER_DIRECT_MOUNT = REST_ROUTE_LEDGER.filter((e) => e.source === 'direct-mount').map((e) => e.route); -const PACKAGE_ROUTES = LEDGER_DIRECT_MOUNT.filter((r) => r.includes('/packages')); +const ALL_PACKAGE_ROUTES = LEDGER_DIRECT_MOUNT.filter((r) => r.includes('/packages')); +/** [#7563] Mounted on every boot — no dispatcher twin to fall back to. */ +const PUBLISH_ROUTE = 'POST /api/v1/packages/publish'; +/** The `package`-service-gated three, each shadowing a dispatcher twin. */ +const PACKAGE_ROUTES = ALL_PACKAGE_ROUTES.filter((r) => r !== PUBLISH_ROUTE); const FEDERATION_ROUTES = LEDGER_DIRECT_MOUNT.filter((r) => r.includes('/external')); /** `VERB /path` for every route the server reports as mounted. */ @@ -155,7 +170,7 @@ function documented(body: any, route: string): boolean { describe('#5822 — a registrar describes exactly what it mounted', () => { it('package routes: the returned array matches the registration calls, one for one', () => { const server = createMockServer(); - const returned = registerPackageRoutes(server as any, packageServiceStub() as any, '/api/v1'); + const returned = registerPackageRoutes(server as any, () => packageServiceStub() as any, '/api/v1'); const mounted: string[] = []; for (const verb of ['get', 'post', 'put', 'patch', 'delete'] as const) { @@ -263,7 +278,15 @@ describe('#5822 — mounted direct-mount routes are enumerable and documented', // --------------------------------------------------------------------------- describe('#5822 — an unmounted registrar is reported by nothing', () => { - it('a boot without the `package` service enumerates and documents no packages route', async () => { + it('the publish row this file splits out is a row the ledger really has', () => { + // Without this, renaming the route in the ledger would quietly move it into + // PACKAGE_ROUTES and make the gated-cohort cases below assert the opposite + // of what they are named after. + expect(ALL_PACKAGE_ROUTES).toContain(PUBLISH_ROUTE); + expect(PACKAGE_ROUTES).toHaveLength(ALL_PACKAGE_ROUTES.length - 1); + }); + + it('a boot without the `package` service enumerates and documents no service-backed packages route', async () => { const { rest, server } = bootWith({}); const keys = mountedKeys(rest); for (const route of PACKAGE_ROUTES) { @@ -280,6 +303,17 @@ describe('#5822 — an unmounted registrar is reported by nothing', () => { } }); + it('…but publish IS mounted, enumerable and documented on that same boot (#7563)', async () => { + // The counterpart the header explains: this route has no dispatcher twin, + // so being absent handed the path to `/packages/:id` and a 405 built from + // its verbs. Mounted, it answers its own 404 — and a document that names it + // is describing a route that really is there. + const { rest, server } = bootWith({}); + expect(mountedKeys(rest)).toContain(PUBLISH_ROUTE); + expect(server.post.mock.calls.map((args: unknown[]) => args[0])).toContain('/api/v1/packages/publish'); + expect(documented(await serveOpenApi(server), PUBLISH_ROUTE)).toBe(true); + }); + it('the federation routes are still there — they mount unconditionally', async () => { // The gate that does NOT exist, stated so the previous test cannot be // "fixed" by dropping every direct-mount row when a service is missing: @@ -317,10 +351,11 @@ describe('#5822 — the REST plugin records what it mounts', () => { } }); - it('publishes the five, and only the five, when it is not', async () => { + it('publishes the five federation routes plus publish, and nothing else, when it is not', async () => { const { server } = await bootPlugin({}); const body = await serveOpenApi(server); for (const route of FEDERATION_ROUTES) expect(documented(body, route)).toBe(true); + expect(documented(body, PUBLISH_ROUTE), `${PUBLISH_ROUTE} mounts unconditionally (#7563)`).toBe(true); for (const route of PACKAGE_ROUTES) expect(documented(body, route)).toBe(false); }); }); diff --git a/packages/rest/src/direct-mount.ts b/packages/rest/src/direct-mount.ts index 5512be97a0..f8a80cf17d 100644 --- a/packages/rest/src/direct-mount.ts +++ b/packages/rest/src/direct-mount.ts @@ -46,10 +46,14 @@ * * - **mounted ⇒ enumerable.** The array that mounted the routes is the array * that is returned; a registrar cannot mount a route it does not describe. - * - **not mounted ⇒ not enumerable.** A registrar that is never called returns - * nothing, so a boot without `packageService` reports no `packages.*` route - * and publishes none. The service gate stays where it was — at the - * composition step — and the record follows it. + * - **not mounted ⇒ not enumerable.** A route the registrar leaves out is not + * in the array it returns, so a boot without `packageService` reports the + * three `package`-service-backed `packages.*` routes nowhere and publishes + * none of them. The service gate stays where it was and the record follows + * it. Since #7563 the gate covers three of the four rather than all four: + * `POST /packages/publish` mounts unconditionally (it has no dispatcher twin + * to fall back to, so leaving it unowned produced another route's 405 rather + * than a 404) — see `package-routes.ts`. * * One deliberate asymmetry: if the host server throws part-way through mounting, * the exception propagates and the caller records NOTHING, even though some rows diff --git a/packages/rest/src/package-envelope.conformance.test.ts b/packages/rest/src/package-envelope.conformance.test.ts index 8aec8621ca..7f116fbd11 100644 --- a/packages/rest/src/package-envelope.conformance.test.ts +++ b/packages/rest/src/package-envelope.conformance.test.ts @@ -72,7 +72,7 @@ function mount(svc: Svc, options: any = {}) { // the gate (holding both the read and write capability); a test can override // `resolveExecutionContext` to exercise the gate itself. The gate itself is // pinned in the `packages authz` describe at the bottom of this file. - registerPackageRoutes(server, svc as any, '/api/v1', { + registerPackageRoutes(server, () => svc as any, '/api/v1', { resolveExecutionContext: async () => ({ userId: 'u_pkg', systemPermissions: ['manage_metadata', 'studio.access', 'setup.access'], }), diff --git a/packages/rest/src/package-publish-mount.test.ts b/packages/rest/src/package-publish-mount.test.ts new file mode 100644 index 0000000000..d95465e1d6 --- /dev/null +++ b/packages/rest/src/package-publish-mount.test.ts @@ -0,0 +1,268 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `POST /packages/publish` HAS AN OWNER ON EVERY BOOT (#7563). + * + * ## The defect this pins shut + * + * On a live showcase boot, `POST /api/v1/packages/publish` answered **405** + * with `Allow: DELETE, GET, HEAD, PATCH`. Not one of those verbs belongs to the + * publish surface — `POST` is the only verb it has ever had. They are + * `/packages/:id`'s method set, and the router offered them because with the + * publish route unmounted that pattern was the only thing left matching the + * path, with `id = "publish"`. So a caller was told "this path exists, use + * another method", and every method on offer would have operated on a package + * literally named `publish`. + * + * Two facts made it possible, and this file pins the repair of both: + * + * 1. The registrar was gated on `ctx.getService('package')` resolving at the + * ONE instant `RestApiPlugin.start()` ran. `objectstack serve` registers + * capability providers (`requires: ['marketplace']` → `PackageServicePlugin`) + * AFTER `createRestApiPlugin`, and start order follows registration order + * for plugins with no edge between them — so the deployments that DO + * compose a package service are exactly the ones that answered "no" at + * mount time. The service is now resolved per request. + * 2. `POST /packages/publish` has no dispatcher twin, so "not mounted" did not + * degrade to a 404 the way the composition's own doc promised — it degraded + * to a sibling's 405. It therefore mounts unconditionally and answers its + * own honest 404 where no package service exists. + * + * ## The asymmetry is load-bearing, and is pinned too + * + * The other three package routes must NOT follow. Each shadows a live + * dispatcher twin at a byte-identical pattern; mounting them without a service + * would replace three working routes with a degraded refusal. A future edit + * that "makes the registrar consistent" by mounting all four unconditionally + * fails the `keeps its hands off the three dispatcher twins` case below. + */ + +// `.js` on the relative imports: under `moduleResolution: nodenext` an +// extension-less one does not resolve and every symbol it names becomes `any`. +import { describe, it, expect, vi } from 'vitest'; +import { registerPackageRoutes } from './package-routes.js'; +import { mountAndRecordDirectRoutes } from './direct-mount-composition.js'; + +// --------------------------------------------------------------------------- +// harness +// --------------------------------------------------------------------------- + +function createMockServer() { + return { + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; +} + +/** A caller who clears the write gate, so every case reaches the route body. */ +const AUTHED = { + resolveExecutionContext: async () => ({ userId: 'u_pkg', systemPermissions: ['manage_metadata'] }), +}; + +function packageServiceStub() { + return { + list: vi.fn().mockResolvedValue([]), + get: vi.fn().mockResolvedValue(null), + publish: vi.fn().mockResolvedValue({ success: true }), + delete: vi.fn().mockResolvedValue({ success: true }), + }; +} + +/** Capture one response the way the direct-mount handlers write it. */ +function captureRes() { + const out: { status?: number; body?: any } = {}; + const res: any = { + status(code: number) { out.status = code; return res; }, + json(body: any) { out.body = body; return res; }, + send(body: any) { out.body = body; return res; }, + header() { return res; }, + }; + return { res, out }; +} + +/** Drive the mounted publish handler with `body`. */ +async function callPublish( + server: ReturnType, + body: unknown = {}, + path = '/api/v1/packages/publish', +) { + const call = server.post.mock.calls.find((args: unknown[]) => args[0] === path); + expect(call, `POST ${path} must be mounted for this case to mean anything`).toBeDefined(); + const { res, out } = captureRes(); + await (call as any)[1]({ method: 'POST', body, params: {}, query: {}, headers: {} }, res); + return out; +} + +/** Every `VERB /path` the registrar asked the server to mount. */ +function mountedOn(server: ReturnType): string[] { + const keys: string[] = []; + for (const verb of ['get', 'post', 'put', 'patch', 'delete'] as const) { + for (const [path] of (server[verb] as any).mock.calls) keys.push(`${verb.toUpperCase()} ${path}`); + } + return keys; +} + +// --------------------------------------------------------------------------- +// the publish path always has an owner +// --------------------------------------------------------------------------- + +describe('#7563 — POST /packages/publish is mounted with or without a `package` service', () => { + it('mounts on a boot that resolves no package service at all', () => { + const server = createMockServer(); + const returned = registerPackageRoutes(server as any, () => undefined, '/api/v1', AUTHED); + + expect(mountedOn(server)).toContain('POST /api/v1/packages/publish'); + // #5822's identity holds on this branch too: what is reported IS what was + // mounted, not a table describing a different set. + expect(returned.map((r) => `${r.method} ${r.path}`)).toEqual(['POST /api/v1/packages/publish']); + }); + + it('keeps its hands off the three dispatcher twins when there is no service', () => { + // The asymmetry, stated as a test so it cannot be "tidied up": these three + // patterns are served by the runtime dispatcher on a package-service-less + // stack, and a degraded REST shadow registered ahead of them would take + // three working routes away. + const server = createMockServer(); + registerPackageRoutes(server as any, () => undefined, '/api/v1', AUTHED); + + const keys = mountedOn(server); + expect(keys).not.toContain('GET /api/v1/packages'); + expect(keys).not.toContain('GET /api/v1/packages/:id'); + expect(keys).not.toContain('DELETE /api/v1/packages/:id'); + }); + + it('mounts the full surface when a package service is there', () => { + const server = createMockServer(); + registerPackageRoutes(server as any, () => packageServiceStub() as any, '/api/v1', AUTHED); + + expect(mountedOn(server).sort()).toEqual([ + 'DELETE /api/v1/packages/:id', + 'GET /api/v1/packages', + 'GET /api/v1/packages/:id', + 'POST /api/v1/packages/publish', + ]); + }); +}); + +// --------------------------------------------------------------------------- +// what it answers, on each side of the gate +// --------------------------------------------------------------------------- + +describe('#7563 — the publish route answers for itself instead of borrowing a 405', () => { + it('404s with a message about THIS surface when no package service is composed', async () => { + const server = createMockServer(); + registerPackageRoutes(server as any, () => undefined, '/api/v1', AUTHED); + + const out = await callPublish(server, { manifest: { id: 'com.acme.crm', version: '1.0.0' }, metadata: {} }); + expect(out.status).toBe(404); + expect(out.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + // Names the SURFACE, not a package id — the two 404s this module can emit + // must not read alike. + expect(out.body?.error?.message).toContain('marketplace publish surface'); + expect(out.body?.error?.message).not.toContain('com.acme.crm'); + }); + + it('refuses an anonymous caller before disclosing whether the surface exists here', async () => { + // The 404 is a fact about how the deployment is composed. Answering it to + // an unauthenticated prober would make this seam a capability oracle, so + // the anonymous floor runs first — same order as every other route here. + const server = createMockServer(); + registerPackageRoutes(server as any, () => undefined, '/api/v1', { + resolveExecutionContext: async () => ({ userId: null }), + }); + + const out = await callPublish(server); + expect(out.status).toBe(401); + }); + + it('runs the real handler when the service arrives AFTER the mount decision', async () => { + // The `objectstack serve` ordering, reproduced: the registrar is composed + // while `getService('package')` still throws, and the service shows up + // before the first request. Resolving per request is what makes this the + // ordinary case rather than a silently degraded one. + const server = createMockServer(); + let svc: ReturnType | undefined; + registerPackageRoutes(server as any, () => svc as any, '/api/v1', AUTHED); + + // Composed with nothing there… + expect(mountedOn(server)).toContain('POST /api/v1/packages/publish'); + + // …the capability provider registers… + svc = packageServiceStub(); + + // …and the handler publishes for real. + const out = await callPublish(server, { manifest: { id: 'com.acme.crm', version: '1.0.0' }, metadata: {} }); + expect(out.status).toBe(200); + expect(svc.publish).toHaveBeenCalledWith({ + manifest: { id: 'com.acme.crm', version: '1.0.0' }, + metadata: {}, + }); + expect(out.body?.data?.message).toBe('Published com.acme.crm@1.0.0'); + }); + + it('still validates the request body ahead of anything else it can refuse', async () => { + const server = createMockServer(); + registerPackageRoutes(server as any, () => packageServiceStub() as any, '/api/v1', AUTHED); + + const out = await callPublish(server, {}); + expect(out.status).toBe(400); + expect(out.body?.error?.code).toBe('MISSING_REQUIRED_FIELD'); + }); +}); + +// --------------------------------------------------------------------------- +// through the composition production calls +// --------------------------------------------------------------------------- + +describe('#7563 — the composition mounts publish on a service-less boot', () => { + function compose(services: Record, extra: Record = {}) { + const server = createMockServer(); + const recorded: Array<{ method: string; path: string }> = []; + mountAndRecordDirectRoutes({ + server: server as any, + recorder: { recordDirectMountedRoutes: (routes) => { recorded.push(...routes.map((r) => ({ method: r.method, path: r.path }))); } }, + ctx: { + getService: (name: string) => { + if (name in services) return services[name]; + throw new Error(`Service '${name}' not found`); + }, + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + } as any, + versionedBase: '/api/v1', + ...extra, + }); + return { server, recorded: recorded.map((r) => `${r.method} ${r.path}`) }; + } + + it('records the publish route — and only it — when `package` is absent', () => { + const { server, recorded } = compose({}); + expect(mountedOn(server)).toContain('POST /api/v1/packages/publish'); + expect(recorded.filter((r) => r.includes('/packages'))).toEqual(['POST /api/v1/packages/publish']); + }); + + it('records all four when `package` is present', () => { + const { recorded } = compose({ package: packageServiceStub() }); + expect(recorded.filter((r) => r.includes('/packages')).sort()).toEqual([ + 'DELETE /api/v1/packages/:id', + 'GET /api/v1/packages', + 'GET /api/v1/packages/:id', + 'POST /api/v1/packages/publish', + ]); + }); + + it('mirrors the publish route under the scoped base, on both bases, with no service', () => { + // The scoped mirror used to exist only inside the service gate, so a + // project-scoped deployment without the service had the same unowned path + // twice. + const { server } = compose({}, { enableProjectScoping: true, projectResolution: 'auto' }); + const keys = mountedOn(server); + expect(keys).toContain('POST /api/v1/packages/publish'); + expect(keys).toContain('POST /api/v1/environments/:environmentId/packages/publish'); + }); +}); diff --git a/packages/rest/src/package-routes-query-multiplicity.test.ts b/packages/rest/src/package-routes-query-multiplicity.test.ts index 67193d31ad..12107a8b86 100644 --- a/packages/rest/src/package-routes-query-multiplicity.test.ts +++ b/packages/rest/src/package-routes-query-multiplicity.test.ts @@ -91,7 +91,7 @@ function harness(options: { protocol?: boolean } = {}) { // (`manage_metadata`) — every case here then reaches the multiplicity rule it // is named after. The gate itself is pinned in // `package-envelope.conformance.test.ts`'s `packages authz` describe. - registerPackageRoutes(server, svc as any, '/api/v1', { + registerPackageRoutes(server, () => svc as any, '/api/v1', { resolveExecutionContext: async () => ({ userId: 'u_pkg', systemPermissions: ['manage_metadata', 'studio.access', 'setup.access'], }), diff --git a/packages/rest/src/package-routes.ts b/packages/rest/src/package-routes.ts index 949bacbfb4..b46d7b79d2 100644 --- a/packages/rest/src/package-routes.ts +++ b/packages/rest/src/package-routes.ts @@ -88,6 +88,23 @@ async function refusePackageRequest( * carries the full argument for why repetition is refused rather than resolved. */ +/** + * Resolve the `package` service AT REQUEST TIME. + * + * [#7563] Deliberately a function and not a resolved instance. The composition + * step used to ask `ctx.getService('package')` ONCE, during + * `RestApiPlugin.start()`, and mount nothing when the answer was "not yet" — + * which is a different question from "not composed". `objectstack serve` + * registers the capability providers (`requires: ['marketplace']` → + * `PackageServicePlugin`) AFTER `createRestApiPlugin`, and plugin start order + * follows registration order for plugins with no edge between them + * (`plugin-order.ts`), so on every showcase-shaped deployment the service is + * present at request time and absent at the one instant the mount decision was + * taken. Resolving per request makes the answer independent of composition + * order instead of silently encoding it. + */ +export type PackageServiceResolver = () => PackageService | undefined; + /** * Options for package route registration. */ @@ -132,9 +149,7 @@ export interface PackageRoutesOptions { * * Returns the routes it mounted, so the caller can record them on the * `RestServer` that owns the surface (#5822) — the returned array IS the array - * that was iterated to mount, never a second, hand-kept table. A boot without a - * `package` service never calls this registrar, so nothing is mounted and - * nothing is reported; see `direct-mount.ts`. + * that was iterated to mount, never a second, hand-kept table. * * Routes: * - POST /api/v1/packages/publish - Publish a package to the marketplace registry @@ -150,6 +165,35 @@ export interface PackageRoutesOptions { * dispatcher's own `POST /packages/:id/publish` (ADR-0033 draft publish) * is two segments — different shape, no clash. * + * ## Which of these four mount, and why they differ (#7563) + * + * `POST /packages/publish` mounts UNCONDITIONALLY; the other three stay gated + * on the `package` service. That asymmetry is not a compromise — it is the one + * shape that is honest for each: + * + * - The three gated routes have DISPATCHER TWINS at byte-identical patterns + * (`packages/runtime/src/domains/packages.ts` — `GET /packages`, + * `GET /packages/:id`, `DELETE /packages/:id`), mounted unconditionally. + * This registrar shadows them when it runs (first-match-wins). Mounting them + * without a `package` service would replace three WORKING routes with a + * degraded refusal, so absence keeps them where they are. + * - `POST /packages/publish` has NO twin. Nobody else serves that verb+path, + * so when this registrar sits out, the request does not 404 — it is absorbed + * by the dispatcher's `/packages/:id` (with `id = "publish"`), and the + * router answers `405` with `Allow: DELETE, GET, HEAD, PATCH`: ANOTHER + * route's method set, describing verbs that would each operate on a package + * literally named `publish` (#7563). "Use a different method" is the one + * answer that misinforms here, because `POST` is the only verb this surface + * ever had. Mounting it always means the path has an owner that can tell the + * truth — the handler when a package service is reachable, and an honest + * 404 naming this surface when none is. + * + * The degraded answer is 404 and not 503: a deployment that composed no + * marketplace capability is not going to grow one on retry, and 503 invites + * exactly that retry. It is also what `direct-mount-composition.ts` has always + * documented as the answer for a skipped registrar — until #7563 that promise + * was simply not true on the wire for this one path. + * * ## Where this module's error codes came from * * This was the *partially* converted module when #3843 was filed, which is @@ -181,19 +225,16 @@ export interface PackageRoutesOptions { */ export function registerPackageRoutes( server: IHttpServer, - packageService: PackageService, + resolvePackageService: PackageServiceResolver, basePath: string = '/api/v1', options: PackageRoutesOptions = {}, ): readonly DirectMountedRoute[] { const packagesPath = `${basePath}/packages`; /** - * ONE declaration of this registrar's surface (#5822): the array below is - * what gets mounted on the host server AND what is handed back as the - * description of what was mounted. There is no second table to keep in sync — - * see `direct-mount.ts` for why that identity is the whole point. + * The always-mounted half — see "Which of these four mount" above. */ - const routes: readonly DirectMountedRoute[] = [ + const publishRoute: DirectMountedRoute = // POST /api/v1/packages/publish - Publish a package to the marketplace { method: 'POST', @@ -202,6 +243,25 @@ export function registerPackageRoutes( handler: async (req, res) => { try { if (await refusePackageRequest(options, req, res, 'write')) return; + // Resolved HERE, not at composition (#7563). Authorization runs first so + // an anonymous prober cannot read a deployment's capability composition + // off this seam. + const packageService = resolvePackageService(); + if (!packageService) { + // The honest answer for a surface this host does not serve. It names + // the surface rather than a package id, so it cannot be confused with + // the `RESOURCE_NOT_FOUND` a real publish emits for a missing package, + // and it can never be the `405` of a route that merely shares the + // `/packages` prefix. + sendError( + res, + 404, + 'RESOURCE_NOT_FOUND', + 'This deployment serves no marketplace publish surface — it composes no `package` service. ' + + "Add the `marketplace` capability to the app's `requires` to enable publishing.", + ); + return; + } const { manifest, metadata } = req.body || {}; if (!manifest || !metadata) { @@ -232,8 +292,19 @@ export function registerPackageRoutes( sendError(res, 500, 'INTERNAL_ERROR', (error as Error).message); } }, - }, + }; + /** + * The service-gated half — mounted only when a `package` service is + * reachable, because each of these three SHADOWS a live dispatcher twin at + * the same pattern and a degraded shadow is worse than no shadow. + * + * These take the RESOLVED service, not the resolver: the gate below already + * decided on presence, and handing them an optional they would each have to + * re-check would add three branches no deployment can reach. Their bodies are + * unchanged from before #7563. + */ + const serviceGatedRoutes = (packageService: PackageService): readonly DirectMountedRoute[] => [ // GET /api/v1/packages - List all packages (merges registry + database) { method: 'GET', @@ -414,5 +485,18 @@ export function registerPackageRoutes( }, ]; + /** + * ONE declaration of this registrar's surface (#5822): the array below is + * what gets mounted on the host server AND what is handed back as the + * description of what was mounted. There is no second table to keep in sync — + * see `direct-mount.ts` for why that identity is the whole point. The gate is + * inside the declaration rather than around the call, so "what was mounted" + * stays the array that mounted it on both branches. + */ + const packageService = resolvePackageService(); + const routes: readonly DirectMountedRoute[] = packageService + ? [publishRoute, ...serviceGatedRoutes(packageService)] + : [publishRoute]; + return mountDirectRoutes(server, routes); } diff --git a/packages/rest/src/rest-route-ledger.ts b/packages/rest/src/rest-route-ledger.ts index 08e9c1cc66..e59cfd716a 100644 --- a/packages/rest/src/rest-route-ledger.ts +++ b/packages/rest/src/rest-route-ledger.ts @@ -307,9 +307,9 @@ export const REST_ROUTE_LEDGER: readonly RestRouteLedgerEntry[] = [ { route: 'POST /api/v1/data/:object/updateMany', family: 'batch', source: 'route-manager', disposition: 'sdk', client: 'data.updateMany' }, { route: 'POST /api/v1/data/:object/deleteMany', family: 'batch', source: 'route-manager', disposition: 'sdk', client: 'data.deleteMany' }, - // ── packages (direct-mount registrar, service-gated) ────────────────────── + // ── packages (direct-mount registrar; the three `:id` rows service-gated) ── { route: 'POST /api/v1/packages/publish', family: 'packages', source: 'direct-mount', disposition: 'server-only', - note: 'marketplace registry publish ({manifest, metadata}) — publisher tooling, not app-SDK surface. Moved off the bare POST /packages in #3610: that verb+path is the dispatcher install route, and REST registering it first swallowed every packages.install call with a 400.' }, + note: 'marketplace registry publish ({manifest, metadata}) — publisher tooling, not app-SDK surface. Moved off the bare POST /packages in #3610: that verb+path is the dispatcher install route, and REST registering it first swallowed every packages.install call with a 400. Mounted UNCONDITIONALLY since #7563 — it has no dispatcher twin, so while it was service-gated the path was absorbed by /packages/:id and answered 405 with THAT route\'s Allow set; it now resolves the `package` service per request and answers an honest 404 on a deployment that composes none.' }, { route: 'GET /api/v1/packages', family: 'packages', source: 'direct-mount', disposition: 'sdk', client: 'packages.list', note: 'shadows the dispatcher twin (registered first); merges registry + database packages' }, { route: 'GET /api/v1/packages/:id', family: 'packages', source: 'direct-mount', disposition: 'sdk', client: 'packages.get',