diff --git a/.changeset/packages-lifecycle-readonly-gate.md b/.changeset/packages-lifecycle-readonly-gate.md new file mode 100644 index 0000000000..65d4c1403b --- /dev/null +++ b/.changeset/packages-lifecycle-readonly-gate.md @@ -0,0 +1,42 @@ +--- +"@objectstack/runtime": patch +"@objectstack/metadata-protocol": patch +"@objectstack/spec": patch +--- + +fix(runtime): refuse to disable or delete a read-only package on the `/packages` lifecycle routes (#7560) + +`PATCH /packages//disable` and `DELETE /packages/` answered **200** on a +platform package, and the `DELETE` really removed it from the running process's +registry listing. One authorized API call took platform functionality out of a +live deployment. Reproduced on two platform packages in the QA run behind #7514. + +**Blast radius, measured.** The card reported that the packages come back after a +restart — true for `DELETE` (they are code-loaded, so nothing is permanently +destroyed), but **not** for `disable`: `setPackageDisabled` persists the choice +to `/package-state/.json`, which `SchemaRegistry` replays at boot. +A disabled platform package stayed disabled across restarts. + +**Two axes, not one.** #7033 / PR #7083 gave the whole `/packages` domain caller +authorization (`manage_metadata` on writes, the ADR-0106 D4 set on reads, an +anonymous floor) — *who may call the route*. This is the second, missing check +on the same routes: *what the route may do once the caller is allowed*. An +authorized admin — and `isSystem` — is now refused, because read-only is a +property of the **package**, not of the caller. The caller gate is unchanged; +tightening it would not have fixed this and would have broken legitimate admins. + +**No new vocabulary.** The refusal is ADR-0070's existing one, reused: `422` / +`WRITABLE_PACKAGE_REQUIRED`, the code `saveMetaItem` already throws when asked to +author *into* a read-only package. The predicate behind it moved out of +`ObjectStackProtocolImplementation`'s private method into +`@objectstack/metadata-protocol`'s exported `isWritablePackage(engine, id)` and +is now **referenced** by both callers — a second hand-kept copy of "which +packages are read-only" is exactly the drift that let `DELETE` remove a platform +package while `saveMetaItem` was refusing to add one field to it. Both read-only +signals are covered: a booted code package (`engine.manifests`) and a +platform-delivered manifest `scope` of `system` / `cloud`. + +Packages an org owns (project-scoped bases, ADR-0048 authoring workspaces) still +disable, re-enable and delete exactly as before — pinned in both directions, on +the registry listing rather than on the status code, since the listing is where +the original defect's harm actually showed. diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index c9b81d5325..31dfb50729 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -63,6 +63,13 @@ export type { MetadataDiagnostics } from './metadata-diagnostics.js'; export type { MetadataHostEngine } from './host-engine.js'; +// [#7560] ADR-0070's read-only-package rule. The authoring path (`saveMetaItem` +// → `WRITABLE_PACKAGE_REQUIRED`) and the `/packages` lifecycle gate in +// `@objectstack/runtime` (`PATCH /:id/disable`, `DELETE /:id`) both ask it, so +// "which packages are read-only" has ONE definition rather than two that drift. +export { isWritablePackage, READ_ONLY_PACKAGE_SCOPES } from './package-writability.js'; +export type { PackageWritabilityEngine } from './package-writability.js'; + // #4556 — the `sys_metadata_history.recorded_by` sentinel → NULL conversion, // as an ADR-0119 D2 migration plan. Run by `os migrate recorded-by`. export { diff --git a/packages/metadata-protocol/src/package-writability.ts b/packages/metadata-protocol/src/package-writability.ts new file mode 100644 index 0000000000..022c598244 --- /dev/null +++ b/packages/metadata-protocol/src/package-writability.ts @@ -0,0 +1,83 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * ADR-0070 — the read-only-package predicate, in ONE place. + * + * A package is either a **writable base** (an org may author into it, and its + * lifecycle is the org's to manage) or **read-only** (it belongs to the + * deployment that ships it). Until #7560 this distinction was a private method + * on {@link ObjectStackProtocolImplementation}, reachable only by the metadata + * authoring path — so `saveMetaItem` refused to author INTO a platform package + * while `PATCH /packages/:id/disable` and `DELETE /packages/:id` happily took + * the whole package out of the running deployment. + * + * The two callers now share this function rather than each spelling the rule: + * a third read-only signal added here reaches the authoring gate and the + * lifecycle gate together, which is the only way the two can't drift apart. + */ + +/** + * The engine surface this predicate reads. Structural on purpose — it is + * satisfied by the real `ObjectQLEngine`, by `MetadataHostEngine`, and by the + * partial doubles the gate tests build, and it keeps this module free of a + * dependency on `@objectstack/objectql`. + */ +export interface PackageWritabilityEngine { + /** Booted code packages, keyed by manifest id (`registerApp` populates it). */ + manifests?: { has?(id: string): boolean }; + registry?: { + getPackage?(id: string): { manifest?: { scope?: string } } | undefined; + }; +} + +/** + * Manifest scopes that mark a package as platform-delivered, hence read-only. + * `system` is the platform's own; `cloud` is marketplace / control-plane + * delivered. Anything else (`project`, or an absent scope) is an org's own. + */ +export const READ_ONLY_PACKAGE_SCOPES: readonly string[] = ['system', 'cloud']; + +/** + * True when `packageId` is a **writable base** — a DB-backed package an org or + * the AI may author *new* metadata into, and whose lifecycle the org owns + * (ADR-0070 D2). The two read-only kinds return `false`: + * + * • **Booted code packages** — they register a manifest into the engine at + * startup (`registerApp` → `engine.manifests`); their items are code-shipped + * artifacts. Only `allowOrgOverride` overlays are allowed (ADR-0005), never + * fresh authored items. + * • **Installed / platform packages** — manifest `scope` is `system` or + * `cloud` (marketplace / platform-delivered). + * + * A project-scoped DB package, or a bare ADR-0048 *authoring-workspace* id with + * no registered manifest, is writable. + * + * NOTE: the code-package signal is the engine manifest map ONLY — we + * deliberately do NOT fall back to "owns ≥1 registered object" (the old + * `isLoadedPackage` heuristic). A writable base accrues registered objects once + * its drafts publish, and that must never flip the base to read-only — that is + * the exact #2252 read-only-after-publish trap ADR-0070 removes. + * + * NOTE: this is a property of the PACKAGE, not of the caller. There is + * deliberately no `isSystem` escape hatch: #7033 decided *who may call* the + * package routes, and #7560 is what those routes may do once the caller is + * allowed. An authorized admin — and the engine itself — still may not disable + * or delete a package the deployment ships. Internal code that legitimately + * tears a code package down calls `registry.uninstallPackage` directly and never + * passes through a gate. + * + * An absent/empty `packageId` is NOT writable: the authoring path treats "no + * base resolved" as a refusal (`WRITABLE_PACKAGE_REQUIRED`), and answering + * "writable" for an unknown would make this predicate fail open. + */ +export function isWritablePackage(engine: unknown, packageId: string | null | undefined): boolean { + if (!packageId) return false; + const e = engine as PackageWritabilityEngine | null | undefined; + // Booted code package → read-only artifact source. + if (e?.manifests?.has?.(packageId)) return false; + // Installed / platform package → read-only by manifest scope. + const scope = e?.registry?.getPackage?.(packageId)?.manifest?.scope; + if (typeof scope === 'string' && READ_ONLY_PACKAGE_SCOPES.includes(scope)) return false; + // Project-scoped base, or unregistered authoring-workspace id → writable. + return true; +} diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7ee497373a..d1cc029a89 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -12,6 +12,9 @@ import { readEnvWithDeprecation, resolveTenancyPosture } from '@objectstack/type import { postureEnforcesWall } from '@objectstack/spec/security'; import type { MetadataHostEngine } from './host-engine.js'; import { evaluateRuntimeAuthoringGate } from './runtime-authoring-gate.js'; +// [#7560] ADR-0070's read-only-package rule, shared with the `/packages` +// lifecycle gate in `@objectstack/runtime` — see `./package-writability.js`. +import { isWritablePackage as isWritablePackageShared } from './package-writability.js'; import type { RuntimeAuthoringIssue } from './runtime-authoring-gate.js'; // [#6418] `sys_metadata`'s overlay-uniqueness indexes: probe-first DDL plus the // ADR-0120 D4 reporting that replaced this file's empty `catch` blocks. @@ -8283,35 +8286,20 @@ export class ObjectStackProtocolImplementation implements /** * True when `packageId` is a **writable base** — a DB-backed package an - * org or the AI may author *new* metadata into (ADR-0070 D2). The two - * read-only kinds return `false`: - * - * • **Booted code packages** — they register a manifest into the engine - * at startup (`registerApp` → `engine.manifests`); their items are - * code-shipped artifacts. Only `allowOrgOverride` overlays are allowed - * (ADR-0005), never fresh authored items. - * • **Installed / platform packages** — manifest `scope` is `system` or - * `cloud` (marketplace / platform-delivered). - * - * A project-scoped DB package, or a bare ADR-0048 *authoring-workspace* id - * with no registered manifest, is writable. - * - * NOTE: the code-package signal is the engine manifest map ONLY — we - * deliberately do NOT fall back to "owns ≥1 registered object" (the old - * `isLoadedPackage` heuristic). A writable base accrues registered objects - * once its drafts publish, and that must never flip the base to read-only - * — that is the exact #2252 read-only-after-publish trap this ADR removes. + * org or the AI may author *new* metadata into (ADR-0070 D2). + * + * [#7560] The rule itself moved to {@link isWritablePackage} in + * `./package-writability.js` because it gained a SECOND caller: the + * `/packages` lifecycle routes, which must refuse to disable or delete a + * read-only package the same way this path refuses to author into one. Two + * hand-kept copies of "which packages are read-only" is precisely the drift + * that let `DELETE /packages/:id` remove a platform package from a live + * deployment while `saveMetaItem` was refusing to add one field to it. This + * method stays as the in-class spelling; the shared function is the + * definition, and its doc comment carries the reasoning. */ private isWritablePackage(packageId: string | null | undefined): boolean { - if (!packageId) return false; - const engine = this.engine as any; - // Booted code package → read-only artifact source. - if (engine?.manifests?.has?.(packageId)) return false; - // Installed / platform package → read-only by manifest scope. - const scope = engine?.registry?.getPackage?.(packageId)?.manifest?.scope; - if (scope === 'system' || scope === 'cloud') return false; - // Project-scoped base, or unregistered authoring-workspace id → writable. - return true; + return isWritablePackageShared(this.engine, packageId); } /** diff --git a/packages/runtime/src/domains/packages-readonly-gate.test.ts b/packages/runtime/src/domains/packages-readonly-gate.test.ts new file mode 100644 index 0000000000..0195563675 --- /dev/null +++ b/packages/runtime/src/domains/packages-readonly-gate.test.ts @@ -0,0 +1,294 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `/packages` lifecycle — the ADR-0070 READ-ONLY gate (#7560). + * + * ## The defect + * + * A QA run drove an AUTHORIZED admin at two platform packages: + * + * • `PATCH /packages//disable` → **200** + * • `DELETE /packages/` → **200**, and the package was + * really gone from `GET /packages` on the running process. + * + * One API call took platform functionality out of a live deployment. `DELETE` + * came back on restart (the packages are code-loaded, so nothing is permanently + * destroyed); `disable` did NOT — `setPackageDisabled` persists the choice to + * `/package-state/.json` and the registry replays it at boot. + * + * ## Two axes, not one + * + * #7033 / PR #7083 gave the whole `/packages` domain CALLER authorization + * (`manage_metadata` on writes, the ADR-0106 D4 set on reads, an anonymous + * floor) — *who may call the route*. That is pinned next door in + * `packages-capability-gate.test.ts` and is NOT what this file is about. This + * file pins *what the route may do once the caller is allowed*: the caller here + * holds `manage_metadata` throughout, and is refused anyway, because read-only + * is a property of the PACKAGE. Tightening the caller gate would not have fixed + * this and would have broken legitimate admins. + * + * ## What is asserted, and why it is not the status code + * + * The original report's harm was not "200 instead of 422" — it was that the + * package LEFT THE REGISTRY LISTING of the running process. So every refusal + * case below asserts the listing observable (`registry.getAllPackages()`), and + * the tests drive a REAL {@link SchemaRegistry}, not a `vi.fn()` double: a mock + * `uninstallPackage` cannot tell you whether the package survived. + * + * ## The gate must not be an outage + * + * A gate that refuses everything is not a fix. Every refusal case is mirrored by + * a writable-package case that still succeeds AND whose effect is observed in + * the same listing. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { SchemaRegistry } from '@objectstack/objectql'; +import { HttpDispatcher } from '../http-dispatcher.js'; + +// ── package ids under test ─────────────────────────────────────────────────── +/** Read-only signal #1: manifest `scope: 'system'` (platform-delivered). */ +const PLATFORM_SCOPED = 'com.objectstack.platform'; +/** Read-only signal #1b: manifest `scope: 'cloud'` (marketplace/control-plane). */ +const CLOUD_SCOPED = 'com.objectstack.cloudpack'; +/** + * Read-only signal #2: a CODE-LOADED package — registered into + * `engine.manifests` by `registerApp` at boot. Its manifest scope is the plain + * default (`project`), so only the manifest-map signal marks it read-only. This + * is the shape the #7560 repro used ("restart — the package is back, confirming + * it is code-loaded"). + */ +const CODE_LOADED = 'com.objectstack.showcase'; +/** The writable base — a DB-installed project package the org owns. */ +const WRITABLE = 'com.acme.myapp'; + +function manifest(id: string, scope?: 'system' | 'cloud' | 'project') { + return { + id, + name: id, + version: '1.0.0', + ...(scope ? { scope } : {}), + } as any; +} + +/** + * A dispatcher over a real `SchemaRegistry`. `objectql` stands in for the + * engine: the route reads `.registry` off it, and the ADR-0070 predicate reads + * `.manifests` — the same two handles `ObjectStackProtocolImplementation` asks + * for on the authoring side. + */ +function make() { + const registry = new SchemaRegistry({ logLevel: 'silent' } as any); + registry.installPackage(manifest(PLATFORM_SCOPED, 'system')); + registry.installPackage(manifest(CLOUD_SCOPED, 'cloud')); + registry.installPackage(manifest(CODE_LOADED, 'project')); + registry.installPackage(manifest(WRITABLE, 'project')); + + // Only the code-loaded package booted from an artifact. + const manifests = new Map([[CODE_LOADED, manifest(CODE_LOADED)]]); + + const objectql = { registry, manifests }; + const protocol = { + // Present so a DELETE that is ALLOWED reaches its persisted half too — + // the allow-path must be exercised end to end, not just to the gate. + deletePackage: async () => ({ deletedCount: 1 }), + }; + const kernel: any = { + context: { + getService: (name: string) => + name === 'objectql' ? objectql : name === 'protocol' ? protocol : null, + }, + }; + return { dispatcher: new HttpDispatcher(kernel), registry }; +} + +/** Authorized under #7033 — holds the write capability on every call below. */ +const admin = (): any => ({ + request: {}, + environmentId: 'pkg-readonly-gate-test', + executionContext: { userId: 'u_admin', isSystem: false, systemPermissions: ['manage_metadata'] }, +}); +/** Engine self-invocation. Read-only is about the package, not the caller. */ +const system = (): any => ({ + request: {}, + environmentId: 'pkg-readonly-gate-test', + executionContext: { isSystem: true }, +}); + +const listedIds = (registry: SchemaRegistry): string[] => + registry.getAllPackages().map((p: any) => p.manifest.id); + +// `setPackageDisabled` writes a real JSON file under OS_HOME on the ALLOW path. +// Point OS_HOME at a temp dir so the suite never touches the developer's +// `~/.objectstack` (and so a leftover disable cannot leak between runs). +let home: string; +let priorHome: string | undefined; +beforeAll(() => { + priorHome = process.env.OS_HOME; + home = mkdtempSync(join(tmpdir(), 'os-pkg-readonly-')); + process.env.OS_HOME = home; +}); +afterAll(() => { + if (priorHome === undefined) delete process.env.OS_HOME; + else process.env.OS_HOME = priorHome; + rmSync(home, { recursive: true, force: true }); +}); + +// ══════════════════════════════════════════════════════════════════════════════ +// 1. REFUSED — a read-only package survives both lifecycle verbs +// ══════════════════════════════════════════════════════════════════════════════ + +const READ_ONLY_PACKAGES: Array<{ label: string; id: string }> = [ + { label: 'platform-scoped (manifest scope: system)', id: PLATFORM_SCOPED }, + { label: 'cloud-scoped (manifest scope: cloud)', id: CLOUD_SCOPED }, + { label: 'code-loaded (booted into engine.manifests)', id: CODE_LOADED }, +]; + +describe('/packages lifecycle — DELETE refuses a read-only package (#7560)', () => { + for (const pkg of READ_ONLY_PACKAGES) { + it(`422s DELETE on the ${pkg.label} package AND leaves it in the registry listing`, async () => { + const { dispatcher, registry } = make(); + const before = listedIds(registry); + + const r = await dispatcher.handlePackages(`/${pkg.id}`, 'DELETE', {}, {}, admin()); + + expect(r.response?.status).toBe(422); + expect(r.response?.body?.error?.code).toBe('WRITABLE_PACKAGE_REQUIRED'); + // THE observable the card was written from: asserting the status + // alone would not have caught the original defect's actual harm. + expect(listedIds(registry)).toEqual(before); + expect(listedIds(registry)).toContain(pkg.id); + expect(registry.getPackage(pkg.id)).toBeDefined(); + }); + } +}); + +describe('/packages lifecycle — PATCH /:id/disable refuses a read-only package (#7560)', () => { + for (const pkg of READ_ONLY_PACKAGES) { + it(`422s disable on the ${pkg.label} package AND leaves it ENABLED`, async () => { + const { dispatcher, registry } = make(); + expect(registry.getPackage(pkg.id)?.enabled).toBe(true); + + const r = await dispatcher.handlePackages(`/${pkg.id}/disable`, 'PATCH', {}, {}, admin()); + + expect(r.response?.status).toBe(422); + expect(r.response?.body?.error?.code).toBe('WRITABLE_PACKAGE_REQUIRED'); + // The lifecycle state is untouched — the refusal ran BEFORE + // `disablePackage`, not after it. + expect(registry.getPackage(pkg.id)?.enabled).toBe(true); + expect(registry.getPackage(pkg.id)?.status).not.toBe('disabled'); + }); + } +}); + +// ══════════════════════════════════════════════════════════════════════════════ +// 2. STILL WORKS — the gate is not an outage +// ══════════════════════════════════════════════════════════════════════════════ + +describe('/packages lifecycle — a WRITABLE package still disables and deletes (#7560)', () => { + it('disables the writable package and the listing reflects it', async () => { + const { dispatcher, registry } = make(); + + const r = await dispatcher.handlePackages(`/${WRITABLE}/disable`, 'PATCH', {}, {}, admin()); + + expect(r.response?.status).toBe(200); + expect(registry.getPackage(WRITABLE)?.enabled).toBe(false); + expect(registry.getPackage(WRITABLE)?.status).toBe('disabled'); + }); + + it('re-enables the writable package (the disable is reversible, as before)', async () => { + const { dispatcher, registry } = make(); + await dispatcher.handlePackages(`/${WRITABLE}/disable`, 'PATCH', {}, {}, admin()); + const r = await dispatcher.handlePackages(`/${WRITABLE}/enable`, 'PATCH', {}, {}, admin()); + + expect(r.response?.status).toBe(200); + expect(registry.getPackage(WRITABLE)?.enabled).toBe(true); + }); + + it('deletes the writable package and it leaves the listing', async () => { + const { dispatcher, registry } = make(); + expect(listedIds(registry)).toContain(WRITABLE); + + const r = await dispatcher.handlePackages(`/${WRITABLE}`, 'DELETE', {}, {}, admin()); + + expect(r.response?.status).toBe(200); + expect(r.response?.body?.data?.registryRemoved).toBe(true); + expect(listedIds(registry)).not.toContain(WRITABLE); + // …and the read-only siblings are all still there: the delete removed + // exactly one package, which is the pre-#7560 behaviour for a package + // the org owns. + expect(listedIds(registry).sort()).toEqual([CLOUD_SCOPED, CODE_LOADED, PLATFORM_SCOPED].sort()); + }); +}); + +// ══════════════════════════════════════════════════════════════════════════════ +// 3. The gate's axis: it keys on the PACKAGE, not on the caller +// ══════════════════════════════════════════════════════════════════════════════ + +describe('/packages lifecycle — read-only is a property of the package (#7560)', () => { + it('refuses an isSystem caller too — the write gate exempts isSystem, this one does not', async () => { + const { dispatcher, registry } = make(); + const r = await dispatcher.handlePackages(`/${PLATFORM_SCOPED}`, 'DELETE', {}, {}, system()); + expect(r.response?.status).toBe(422); + expect(listedIds(registry)).toContain(PLATFORM_SCOPED); + }); + + it('is NOT the #7033 caller gate: the admin is authorized (no 401/403) and still refused', async () => { + const { dispatcher } = make(); + const r = await dispatcher.handlePackages(`/${PLATFORM_SCOPED}/disable`, 'PATCH', {}, {}, admin()); + expect(r.response?.status).not.toBe(401); + expect(r.response?.status).not.toBe(403); + expect(r.response?.status).toBe(422); + }); +}); + +// ══════════════════════════════════════════════════════════════════════════════ +// 4. The refusal's shape — ADR-0070's existing vocabulary, not a new one +// ══════════════════════════════════════════════════════════════════════════════ + +describe('/packages lifecycle — the refusal matches the ADR-0070 authoring refusal (#7560)', () => { + it('answers 422 / WRITABLE_PACKAGE_REQUIRED with the package id and the ADR pointer', async () => { + const { dispatcher } = make(); + const r = await dispatcher.handlePackages(`/${PLATFORM_SCOPED}`, 'DELETE', {}, {}, admin()); + const err = r.response?.body?.error; + expect(r.response?.status).toBe(422); + expect(err?.code).toBe('WRITABLE_PACKAGE_REQUIRED'); + expect(err?.httpStatus).toBe(422); + expect(err?.message).toContain('read-only'); + expect(err?.details?.packageId).toBe(PLATFORM_SCOPED); + expect(err?.details?.docs).toBe('docs/adr/0070-package-first-authoring.md'); + }); + + it('names the verb it refused, so disable and delete are distinguishable', async () => { + const { dispatcher } = make(); + const del = await dispatcher.handlePackages(`/${PLATFORM_SCOPED}`, 'DELETE', {}, {}, admin()); + const dis = await dispatcher.handlePackages(`/${PLATFORM_SCOPED}/disable`, 'PATCH', {}, {}, admin()); + expect(del.response?.body?.error?.message).toContain('delete'); + expect(dis.response?.body?.error?.message).toContain('disable'); + }); +}); + +// ══════════════════════════════════════════════════════════════════════════════ +// 5. The gate is not an existence oracle — an unknown id still 404s +// ══════════════════════════════════════════════════════════════════════════════ + +describe('/packages lifecycle — an unknown package id keeps its 404 (#7560)', () => { + it('404s DELETE on an id nobody installed, rather than re-labelling it 422', async () => { + const { dispatcher } = make(); + const kernel: any = { context: { getService: (n: string) => (n === 'objectql' ? { registry: new SchemaRegistry({ logLevel: 'silent' } as any), manifests: new Map() } : null) } }; + const d = new HttpDispatcher(kernel); + const r = await d.handlePackages('/com.nobody.nothing', 'DELETE', {}, {}, admin()); + expect(r.response?.status).toBe(404); + void dispatcher; + }); + + it('404s disable on an id nobody installed', async () => { + const kernel: any = { context: { getService: (n: string) => (n === 'objectql' ? { registry: new SchemaRegistry({ logLevel: 'silent' } as any), manifests: new Map() } : null) } }; + const d = new HttpDispatcher(kernel); + const r = await d.handlePackages('/com.nobody.nothing/disable', 'PATCH', {}, {}, admin()); + expect(r.response?.status).toBe(404); + }); +}); diff --git a/packages/runtime/src/domains/packages.ts b/packages/runtime/src/domains/packages.ts index 624b191c8d..02ae1263be 100644 --- a/packages/runtime/src/domains/packages.ts +++ b/packages/runtime/src/domains/packages.ts @@ -25,6 +25,12 @@ import { // `OBJECT_SCHEMA_READ_ONLY_EXEMPT_CAPABILITIES` — same value it read before, // no re-ruling of the package cohort as a side effect of #7020. import { OBJECT_SCHEMA_READ_ONLY_EXEMPT_CAPABILITIES } from '@objectstack/metadata-core'; +// [#7560] ADR-0070's read-only-package rule — the SAME predicate the metadata +// authoring path asks before refusing a write INTO a platform package +// (`saveMetaItem` → `WRITABLE_PACKAGE_REQUIRED`). Imported, never re-spelled: +// this defect existed precisely because the lifecycle routes had no copy of it, +// and a second copy would be the next place it drifts. +import { isWritablePackage } from '@objectstack/metadata-protocol'; import { organizationIdForMetaWrite } from '../meta-write-org-scope.js'; import { setPackageDisabled } from '../package-state-store.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; @@ -106,9 +112,74 @@ function requireReadCapability(deps: DomainHandlerDeps, context: HttpProtocolCon return null; } +/** + * ADR-0070 READ-ONLY gate for the destructive `/packages` LIFECYCLE routes + * (#7560). + * + * This is a **second, independent** check from the two gates above, on a + * different axis, and collapsing the two is what produced the defect. #7033 / + * PR #7083 decided *who may call* these routes — writes need `manage_metadata`, + * reads the ADR-0106 D4 set, plus a domain-wide anonymous floor. This decides + * *what the route may do once the caller is allowed*: an authorized admin could + * still `PATCH /packages//disable` → 200 and + * `DELETE /packages/` → 200, and the package really left the + * running registry listing. One API call took platform functionality out of a + * live deployment; `DELETE` came back on restart (the packages are code-loaded), + * `disable` did NOT — {@link setPackageDisabled} persists the disable to + * `/package-state/.json`, which the registry re-reads at boot, so + * a disabled platform package stays disabled across restarts. + * + * The predicate is {@link isWritablePackage} from `@objectstack/metadata-protocol` + * — the ADR-0070 rule the authoring path already enforces, referenced rather + * than re-spelled. The refusal is that path's refusal too: `422` / + * `WRITABLE_PACKAGE_REQUIRED`, the code registered for exactly this condition + * ("the package is read-only — provided by code or an installed app"). No new + * vocabulary is invented here; the sentence is lifecycle-specific because the + * authoring one ("switch to a writable package in the package selector") names + * a remedy that makes no sense for a delete. + * + * ⛔ Deliberately NOT caller-sensitive — there is no `isSystem` bypass, unlike + * {@link requireManageMetadata}. Read-only is a property of the PACKAGE. Engine + * self-invocation has no business uninstalling a code package over HTTP, and + * internal teardown calls `registry.uninstallPackage` directly without passing + * through any of these gates. + * + * Callers MUST run this BEFORE mutating — the same "delete first, refuse + * second is the worst shape here" ordering the write gate records. Returns a + * refusal result to short-circuit on, or `null` to proceed. + * + * A package id that resolves to nothing is treated as WRITABLE, so an unknown + * id still falls through to the route's own 404 rather than being re-labelled + * 422 (and so this gate never becomes an existence oracle of its own). + */ +function requireWritablePackage( + deps: DomainHandlerDeps, + engine: unknown, + id: string, + /** Verb for the message, e.g. `'disable'` / `'delete'`. */ + action: string, +): HttpDispatcherResult | null { + if (isWritablePackage(engine, id)) return null; + return { + handled: true, + response: deps.error( + `[writable_package_required] Cannot ${action} package '${id}': it is read-only ` + + `(provided by code or an installed app). Packages the deployment ships are managed by ` + + `the deployment, not over this API — ${action} a package you own, or duplicate this one ` + + `into a writable base (POST /packages/${encodeURIComponent(id)}/duplicate) and change that.`, + 422, + { + code: 'WRITABLE_PACKAGE_REQUIRED', + packageId: id, + docs: 'docs/adr/0070-package-first-authoring.md', + }, + ), + }; +} + /** * Handles Package Management requests - * + * * REST Endpoints: * - GET /packages → list all installed packages * - GET /packages/:id → get a specific package @@ -242,6 +313,11 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin if (parts.length === 2 && parts[1] === 'disable' && m === 'PATCH') { const denied = requireManageMetadata(deps, _context); if (denied) return denied; const id = decodeURIComponent(parts[0]); + // [#7560] ADR-0070: a platform package is not the operator's to + // switch off. BEFORE `disablePackage`, and before + // `setPackageDisabled` writes the choice to disk — a disable that + // lands is not undone by a restart, it is REPLAYED by one. + const readOnly = requireWritablePackage(deps, qlService, id, 'disable'); if (readOnly) return readOnly; const pkg = registry.disablePackage(id); if (!pkg) return { handled: true, response: deps.error(`Package '${id}' not found`, 404) }; try { @@ -687,6 +763,11 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin if (parts.length === 1 && m === 'DELETE') { const denied = requireManageMetadata(deps, _context); if (denied) return denied; const id = decodeURIComponent(parts[0]); + // [#7560] ADR-0070: refuse BEFORE `uninstallPackage`, which is the + // call that removed a platform package from the running registry + // listing (and with it every object the package registers) until + // the next restart. + const readOnly = requireWritablePackage(deps, qlService, id, 'delete'); if (readOnly) return readOnly; const registryRemoved = registry.uninstallPackage(id); // Persisted removal (AI/runtime packages live in sys_metadata, not diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index 0bcdff0ae5..c389bf8cae 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -181,6 +181,13 @@ export const ERROR_CODE_LEDGER = { 'SIGN_IN_REQUIRED', 'UNSUPPORTED', 'VALIDATION_FAILED', + // [#7560] ADR-0070: the `/packages` LIFECYCLE routes (`PATCH /:id/disable`, + // `DELETE /:id`) refuse a read-only — code- or platform-provided — package. + // Second EMITTER of the code `@objectstack/metadata-protocol` already + // registers for the authoring half (`saveMetaItem`); one condition, one + // vocabulary. Per this file's header, a code emitted by several packages is + // listed once per emitting package — provenance, not identity. + 'WRITABLE_PACKAGE_REQUIRED', 'WRONG_PASSWORD', ], '@objectstack/service-storage': [