From 566a9e1e4853a19fd9e87fec26468f607e069be5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 03:34:09 +0000 Subject: [PATCH 1/4] fix(metadata-protocol): stop interpolating raw driver text into client-facing messages (#8136) --- packages/metadata-protocol/src/protocol.ts | 202 ++++++++++++++++++++- 1 file changed, 198 insertions(+), 4 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 5fb0a1cb0d..7f943bd755 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1486,6 +1486,115 @@ function carryCatalogedErrorCode(target: Error, source: unknown): void { } } +/** + * [#8136] Whether a caught error **declared itself a client-facing refusal** — + * a 4xx `status` in the ADR-0112 envelope — and its sentence may therefore be + * quoted back to the caller. + * + * ## The rule this answers, and why it is a POSITIVE list + * + * This package used to interpolate whatever it caught into the message it + * threw (`Failed to delete customization overlay: ${err.message}`) and into the + * `error` strings it collects for a caller. A driver failure on `sys_metadata` + * therefore reached a client verbatim — `SQLITE_ERROR: no such table: + * sys_metadata` on a 500, and the same text inside `failed[].error` on the + * `PACKAGE_DELETE_PARTIAL` 400, where no message-level withhold at any HTTP + * boundary can reach it because it is not the message. Three downstream + * sanitizers each had a hole because of it. + * + * The cure is Prime Directive #12's: pay the consumer-side tolerance down at + * the producer. So the question asked here is **not** "does this text look like + * a driver dump?" — that is `looksLikeInternalErrorLeak`, a heuristic over + * phrasing, and a phrasing test can only ever know the dialects someone has + * met. It is the inverse and bounded question: **did we author this sentence + * for a caller?** A producer that declared 4xx has said the failure is the + * caller's to fix and has written the remedy into the message — the + * self-correcting refusals `SysMetadataRepository` raises (`[item_locked]`, + * `[writable_package_required]`, `[no_draft]`, …) are exactly that, and they + * must survive intact. Everything else is withheld by DEFAULT, so a dialect + * this repo has never run is handled correctly without anyone having enumerated + * it. + * + * ## ⛔ NOT the complement of `declaresServerFault` + * + * The obvious-looking `!declaresServerFault(err)` (`@objectstack/types`, #5811) + * is the wrong direction and would reinstate the whole defect: a bare `Error` + * from a driver declares NOTHING, so it fails that test and would be quoted — + * and a bare driver `Error` is precisely the case measured here. The two + * predicates answer different halves and neither is the other's negation: + * `declaresServerFault` asks a BOUNDARY whether to withhold a declared fault's + * detail; this asks a PRODUCER whether it is allowed to quote at all, and an + * undeclared error is never allowed. + * + * ## Why `status` alone, when the sibling code rule also checks the catalog + * + * {@link carryCatalogedErrorCode} gates on membership in `StandardErrorCode ∪ + * ERROR_CODE_LEDGER` because it writes `ApiErrorSchema.code`, a closed union a + * driver's own dialect must never enter. A message is free text, so the + * catalog does not bound it, and the two readings coincide on every refusal + * that reaches these exits today (each declares both halves). Where they could + * differ, status-alone is the safe direction: requiring a catalogued code too + * would blank an authored refusal that happens to carry an uncatalogued one — + * deleting a #4277 self-correcting message, which is a usability regression in + * exchange for no disclosure gain. + * + * The withheld text never leaves the server: every call site rides the original + * error on `cause`, which `handleRouteError` / `logWithheldServerFault` print + * whole — the same posture {@link metadataStoreUnavailableError} already takes. + */ +function declaresClientRefusal(err: unknown): boolean { + const status = (err as { status?: unknown } | null | undefined)?.status; + return typeof status === 'number' && status >= 400 && status < 500; +} + +/** + * [#8136] The client-facing sentence for a failed overlay delete: the caller's + * own refusal when they declared one, and otherwise a stable line that names + * the operation and quotes nothing. + * + * Both of {@link ObjectStackProtocolImplementation.deleteMetaItem}'s re-wrap + * exits share it, for the reason {@link carryCatalogedErrorCode} gives about + * `code`: the envelope must not vary by which path served the delete. + * + * The `Failed to delete customization overlay` prefix is unchanged, byte for + * byte — it is the operation description a caller needs and several pins read + * it. What changes is what follows the colon. + */ +function overlayDeleteFailureMessage(err: unknown, type: string, name: string): string { + if (declaresClientRefusal(err)) { + const declared = (err as { message?: unknown } | null | undefined)?.message; + if (typeof declared === 'string' && declared.length > 0) { + return `Failed to delete customization overlay: ${declared}`; + } + } + return `Failed to delete customization overlay for ${type}/${name}. ` + + 'The metadata store rejected the delete; the reason is in the server log. ' + + 'Retry once the metadata database is reachable.'; +} + +/** + * [#8136] The per-item `error` string a collector puts on its response — the + * caller's own refusal when they declared one, otherwise a stable fallback. + * + * The counterpart of {@link overlayDeleteFailureMessage} for the paths that + * report failure as DATA rather than by throwing. That distinction is why the + * producer is the only place this can be fixed: `deletePackage`'s `failed[]` + * and `cleanups[]` ride onto a `PACKAGE_DELETE_PARTIAL` 400 inside `details`, + * so no 5xx message withhold at any HTTP boundary ever sees them. + * + * @param fallback - what to say when nothing may be quoted. Already the + * existing no-message fallback at every call site (`'delete failed'`, + * `'cleanup failed'`), so the withheld case reuses the sentence the caller + * could already receive rather than inventing a second vocabulary. + */ +function clientFacingFailureText(err: unknown, fallback: string): string { + if (declaresClientRefusal(err)) { + const declared = (err as { message?: unknown } | null | undefined)?.message; + if (typeof declared === 'string' && declared.length > 0) return declared; + } + return fallback; +} + /** * A batch row that names no record id for an operation that needs one — a * caller error, so it carries VALIDATION_FAILED / 400 rather than falling @@ -12073,6 +12182,11 @@ export class ObjectStackProtocolImplementation implements }); discarded.push({ type: d.type, name: d.name }); } catch (e: any) { + // [#8136] Same source, same reasoning as `deletePackage`'s + // `failed[]` collector below: this `try` wraps only + // `deleteMetaItem`, whose exits all now either declare a + // refusal or withhold at the source. Clean derivatively; no + // filter of its own. failed.push({ type: d.type, name: d.name, @@ -12230,7 +12344,37 @@ export class ObjectStackProtocolImplementation implements { organization_id: null }, ]; } - const rows = (await this.engine.find('sys_metadata', { where })) as any[]; + // [#8136] This read is the uninstall's FIRST database touch, and until + // now it sat outside every `try` in this method — the per-item `catch` + // below wraps only the `deleteMetaItem` loop. So a driver failure here + // propagated whole, out of the protocol and onto the wire: measured as + // `500 INTERNAL_ERROR / "SQLITE_ERROR: no such table: sys_metadata"` + // from `DELETE /api/v1/packages/:id`, a physical table name shipped to + // a client. + // + // Declared rather than swallowed: {@link metadataStoreUnavailableError} + // is this file's EXISTING answer for "a `sys_metadata` read failed" — + // 503 / `SERVICE_UNAVAILABLE`, a message that quotes nothing, and the + // driver error carried on `cause` so the operator still gets it whole. + // Reusing it rather than minting a second sentence for one condition is + // the point; see its docblock for why the verdict is 503. + // + // ⛔ Deliberately NOT routed through {@link + // rethrowUnlessMetadataStoreUnprovisioned}, which returns normally for + // `isMissingTableError` and would license the caller to treat the + // overlay as absent. On a READ that is right — there are genuinely no + // rows. Here it would turn an unreachable store into `rows = []`, and + // this method reports that as a completed uninstall that deleted + // nothing. An outage answered as "there was nothing to remove" is the + // ADR-0110 D3 confusion in its most damaging direction, on a + // destructive verb. Every failure stays a failure; only the disclosure + // changes. + let rows: any[]; + try { + rows = (await this.engine.find('sys_metadata', { where })) as any[]; + } catch (e) { + throw metadataStoreUnavailableError(e); + } const dropStorage = request.keepData !== true; // Delete drafts before active so an object's table is dropped once (on @@ -12253,6 +12397,27 @@ export class ObjectStackProtocolImplementation implements }); deleted.push({ type: row.type, name: row.name, state }); } catch (e: any) { + // [#8136] NO filter here, deliberately, and this comment is why + // the absence is a decision rather than an oversight. + // + // This `try` wraps exactly one call, and every exit + // `deleteMetaItem` has is now either a refusal it DECLARED + // (its two-tier authorization block and `assertLockAllowsDelete` + // — 4xx with a catalogued code) or one of its two re-wraps, + // which withhold at the source via {@link + // overlayDeleteFailureMessage}. Its one engine touch outside + // its own `try`, `getEffectiveLock`, has been fail-closed + // through `rethrowUnlessMetadataStoreUnprovisioned` since + // #5706, so that arrives as the non-quoting 503 too. + // + // So `failed[].error` — which rides onto the + // `PACKAGE_DELETE_PARTIAL` 400 inside `details`, out of reach + // of any boundary's message withhold — is clean BECAUSE the + // producer is, which is the whole shape of this fix. Adding a + // second filter here would be consumer-side tolerance over a + // producer that no longer needs it (Prime Directive #12), and + // it would blank the per-item refusals that make a partial + // uninstall actionable. failed.push({ type: row.type, name: row.name, @@ -12313,7 +12478,18 @@ export class ObjectStackProtocolImplementation implements ...(r?.error ? { error: r.error } : {}), }); } catch (e: any) { - cleanups.push({ name, success: false, removed: 0, error: e?.message ?? 'cleanup failed' }); + // [#8136] A cleanup is arbitrary plugin code that goes straight + // at the engine (plugin-security deletes `sys_permission_set` + // rows and their bindings), so a driver failure lands here + // verbatim — and this outcome rides on the RESPONSE by design, + // inside `details`, where no boundary's message withhold can + // reach it. Quoted only when the cleanup declared a refusal. + cleanups.push({ + name, + success: false, + removed: 0, + error: clientFacingFailureText(e, 'cleanup failed'), + }); console.warn( `[protocol.deletePackage] uninstall cleanup '${name}' failed for '${request.packageId}': ${e?.message}`, ); @@ -13856,8 +14032,19 @@ export class ObjectStackProtocolImplementation implements (conflict as any).actualHead = err.actualHead; throw conflict; } - const e = new Error(`Failed to delete customization overlay: ${err.message ?? err}`); + // [#8136] The message quotes `err` only when `err` DECLARED + // itself a caller-facing refusal — see {@link + // declaresClientRefusal}. Every engine touch in the `try` above + // (`repo.get`, `repo.delete`, `restoreArtifactRegistryView`, + // `dropObjectStorage`, `recordMetadataAudit`, the projector) + // can land a bare driver `Error` in this catch, and this exit + // used to interpolate it whole. + const e = new Error(overlayDeleteFailureMessage(err, request.type, request.name)); (e as any).status = err?.status ?? 500; + // The withheld text is not lost, it is relocated: `cause` is + // what `handleRouteError` / `logWithheldServerFault` print, the + // same posture {@link metadataStoreUnavailableError} takes. + (e as any).cause = err; // [#7426] …and the SAME treatment for `code`, gated on the // declared vocabulary. This is the exit a control-plane // kernel's repository refusal leaves by — `NOT_OVERRIDABLE` / @@ -13948,8 +14135,15 @@ export class ObjectStackProtocolImplementation implements : `Deleted ${singularTypeForRepo} '${request.name}' — it no longer exists.`, }; } catch (err: any) { - const e = new Error(`Failed to delete customization overlay: ${err.message}`); + // [#8136] Same rule as the repository path's exit above — one + // sentence-selection rule for both, so the envelope does not vary + // by which path served the delete. This path is deliberately + // ungated (#5264), so in practice everything reaching here is a + // fault and nothing is quoted; the shared helper is what keeps that + // true if a declared refusal ever does arrive. + const e = new Error(overlayDeleteFailureMessage(err, request.type, request.name)); (e as any).status = 500; + (e as any).cause = err; // [#7426] The SECOND re-wrap exit, and it gets the same `code` rule // — otherwise the verb would answer an envelope that varies by // which path served the delete, which is harder to reason about From f52a3d07a977e4138e8f47f5033facd64442d597 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 04:38:01 +0000 Subject: [PATCH 2/4] wip(#8136): tests + changeset for the producer-side driver-text withhold --- ...etadata-protocol-driver-text-disclosure.md | 51 ++ .../src/durable-package.test.ts | 15 +- .../protocol.driver-text-disclosure.test.ts | 507 ++++++++++++++++++ ...kage-door-5xx-message-sanitization.test.ts | 123 +++-- 4 files changed, 655 insertions(+), 41 deletions(-) create mode 100644 .changeset/metadata-protocol-driver-text-disclosure.md create mode 100644 packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts diff --git a/.changeset/metadata-protocol-driver-text-disclosure.md b/.changeset/metadata-protocol-driver-text-disclosure.md new file mode 100644 index 0000000000..c44f407adb --- /dev/null +++ b/.changeset/metadata-protocol-driver-text-disclosure.md @@ -0,0 +1,51 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): stop interpolating raw driver text into client-facing messages (#8136) + +Option C of #8086, at the producer. `packages/metadata-protocol` interpolated +raw driver/engine error text into messages and response payloads that reach API +clients. Measured on the uninstall path: `DELETE /api/v1/packages/:id` answered +`500 INTERNAL_ERROR` with the body message `SQLITE_ERROR: no such table: +sys_metadata` — a physical table name on the wire. + +Three downstream sanitizers already existed for this class, and each had a hole +traceable to the producer. Two of those holes are structural, not accidental: + +- The boundary belts run `looksLikeInternalErrorLeak`, a **heuristic over the + message**. It now knows the two dialects this repo runs (#8132 / #8263), but a + phrasing test can only ever know the dialects someone has met — MySQL, MSSQL + and Oracle each phrase "this table is missing" differently again, and all + three are measured invisible to it. +- `deletePackage`'s per-item `failed[]` and `cleanups[]` ride onto a + `PACKAGE_DELETE_PARTIAL` **400** inside `details`. That is data, not a + message, so no 5xx message withhold at any HTTP boundary ever sees it. + +**The rule, now stated once at the producer.** A caught error's sentence is +quoted back to a caller only when that error **declared itself a client-facing +refusal** — a 4xx `status` in the ADR-0112 envelope. Anything undeclared (a bare +`Error` from a driver) or declared a server fault gets a stable sentence naming +the operation that failed, and the original error rides on `cause` so the +operator's log still receives it whole. This is a positive list rather than a +negative heuristic, so a dialect nobody here has run is handled correctly by +default. + +Behaviour changes visible to an API client, all on failure paths: + +- A driver failure on the uninstall's `sys_metadata` read is now refused with the + declared envelope this package already uses for that exact condition — + **503 `SERVICE_UNAVAILABLE`** with the "metadata store could not be read" + sentence — instead of an undeclared 500 carrying the driver's own text. It + remains a failure: an unreachable store is never reported as an uninstall that + removed nothing. +- `deleteMetaItem`'s two failure exits keep the `Failed to delete customization + overlay` prefix and their existing `status`, but no longer append the driver's + message. +- `deletePackage`'s `cleanups[].error` reports `cleanup failed` for a cleanup + that failed without declaring a refusal. + +Self-correcting refusals are deliberately untouched: `[item_locked]`, +`[writable_package_required]`, `[no_draft]`, `[tenant_scope_required]` and the +rest declare a 4xx and still reach the caller verbatim, including inside +`failed[]` on a partial uninstall. diff --git a/packages/metadata-protocol/src/durable-package.test.ts b/packages/metadata-protocol/src/durable-package.test.ts index b94f197b1c..9fbe6fde22 100644 --- a/packages/metadata-protocol/src/durable-package.test.ts +++ b/packages/metadata-protocol/src/durable-package.test.ts @@ -96,11 +96,24 @@ describe('deletePackage — uninstall cleanups (#2747)', () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); try { const { impl } = makeImpl(); + // [#8136] `db down` is a BARE error — it declares no ADR-0112 envelope, + // so the producer no longer quotes it onto the response. `cleanups[]` + // rides onto a `PACKAGE_DELETE_PARTIAL` 400 inside `details`, where no + // HTTP boundary's 5xx message withhold can reach it, so a cleanup that + // failed on a driver fault used to ship the driver's words to the client. + // + // This case's SUBJECT is unchanged and still asserted: a throwing cleanup + // is reported as `success: false` rather than aborting the uninstall. + // Only the text moved, and the counterpart — a cleanup that DECLARES a + // 4xx refusal keeps its sentence verbatim — is pinned in + // `protocol.driver-text-disclosure.test.ts`, so "reported as failed" and + // "reported in the driver's words" cannot collapse into one another. (impl as any).registerUninstallCleanup('boom', async () => { throw new Error('db down'); }); const res: any = await (impl as any).deletePackage({ packageId: 'com.example.orders', allTenants: true }); expect(res.cleanups).toEqual([ - { name: 'boom', success: false, removed: 0, error: 'db down' }, + { name: 'boom', success: false, removed: 0, error: 'cleanup failed' }, ]); + expect(JSON.stringify(res)).not.toContain('db down'); } finally { warn.mockRestore(); } diff --git a/packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts b/packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts new file mode 100644 index 0000000000..084bddcc44 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts @@ -0,0 +1,507 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8136 — option C of #8086: `metadata-protocol` stops interpolating raw driver + * text into client-facing messages. The raw text goes to the log; the caller + * gets a sentence that names the operation and quotes nothing. + * + * ## The defect, as measured before this change + * + * `DELETE /api/v1/packages/:id` answered, verbatim: + * + * ``` + * HTTP 500 + * { "success": false, "error": { "code": "INTERNAL_ERROR", + * "message": "SQLITE_ERROR: no such table: sys_metadata" } } + * ``` + * + * Two carriers in this package, both on the uninstall path: + * + * 1. `deletePackage`'s FIRST database touch — `engine.find('sys_metadata')` — + * sat outside every `try` in the method (the per-item `catch` wraps only + * the `deleteMetaItem` loop), so a driver error propagated whole and + * undeclared. + * 2. `deleteMetaItem`'s two re-wrap exits interpolated `err.message` into + * `Failed to delete customization overlay: ...`, and that string is ALSO + * what `deletePackage` collects into `failed[].error` — which rides onto a + * `PACKAGE_DELETE_PARTIAL` **400** inside `details`. No 5xx message + * withhold at any HTTP boundary can reach it there, which is the argument + * for fixing the producer rather than adding a fourth belt. + * + * ## Why this file does NOT test a phrasing heuristic + * + * Three downstream boundaries run `looksLikeInternalErrorLeak` — a heuristic + * over the message. #8132 measured its hole for Postgres and #8263 taught it + * the two dialects this repo runs. That is an interim by construction: a + * phrasing test can only ever know the dialects someone has met. + * + * So the dialect matrix below deliberately includes engines the predicate does + * NOT recognise, and **asserts that it does not** before asserting the text is + * withheld anyway. That pairing is the whole point of option C: correctness + * that does not depend on having enumerated the world's SQL engines. If a + * future PR teaches the predicate one of these dialects, the `toBe(false)` + * half goes red and the reader is sent back here to re-read why the matrix was + * built that way — it must not be quietly "repaired" by deleting the case. + * + * ## The rule under test, stated once + * + * A caught error's sentence may be quoted to a caller only when that error + * DECLARED itself a client-facing refusal (4xx `status`, ADR-0112). Anything + * undeclared — a bare driver `Error` — or declared a server fault is withheld. + * This is a positive list, not a negative heuristic, so an unmet dialect is + * handled correctly by default. + * + * ## Reverse verification — direction predicted BEFORE running + * + * Predicted, with the fix reverted (`git checkout origin/main -- protocol.ts`): + * + * - RED: every case in sections 1, 2 and 4 — the withhold cases. They assert + * the POSITIVE withheld shape (a declared envelope plus the absence of the + * driver line), so an unfixed producer fails them on the message, not on a + * vague "it changed". + * - GREEN IN BOTH DIRECTIONS — `[GUARD]`, not evidence: section 3, the + * over-block bound. A declared 4xx refusal keeps its sentence verbatim + * before and after. What makes them load-bearing is the OVER-BROAD VARIANT + * (`declaresClientRefusal` returning `false` unconditionally, so nothing is + * ever quoted): **measured, 2 failed | 14 passed**, and the two are exactly + * these. Without them this file is satisfied by a blanket replacement, + * which would delete the self-correcting refusals #4277 exists for. + * - The `looksLikeInternalErrorLeak` measurements are green in both + * directions too: they describe the shared predicate, which this card does + * not touch (⛔ widening it is the explicitly ruled-out route). + * + * Measured: **13 failed | 3 passed** with the producer reverted. The 3 green + * are section 0 and the two `[GUARD]` cases named above. + * + * ⚠️ ONE MISSED PREDICTION, kept rather than tidied away. The declared-5xx + * case was drafted inside section 3 and predicted green-in-both-directions + * with the other guards. It came back RED, and it was right to: the unfixed + * re-wrap interpolated `err.message` UNCONDITIONALLY, so a declared 5xx fault + * was quoted too. That makes it evidence for the fix, not a bound on it, and + * it has been moved into section 2 where the other evidence lives. The + * mislabelling is recorded because it is the useful part — "declared" and + * "declared a CLIENT refusal" are not the same predicate, and drafting them as + * one is exactly the confusion `declaresClientRefusal` exists to prevent. + * + * Never a bare `toThrow()`: the unfixed path already throws, so a throw-only + * assertion is permanently green and cannot tell "refused while disclosing" + * from "refused correctly". Every refusal case asserts `code` AND `status`. + */ +import { describe, expect, it } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/metadata-core'; +import { looksLikeInternalErrorLeak } from '@objectstack/types'; +import { DEFAULT_METADATA_TYPE_REGISTRY } from '@objectstack/spec/kernel'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +// --------------------------------------------------------------------------- +// The dialect matrix +// --------------------------------------------------------------------------- + +/** + * One physical condition — `sys_metadata` is not there — as five engines + * phrase it. `knownToPredicate` records what the SHARED heuristic makes of + * each, measured against the shipping predicate in the first test below rather + * than asserted from memory. + * + * The three `false` rows are the reason this card is not "add the phrasing": + * MySQL, MSSQL and Oracle each say it differently again, and the list of + * engines nobody here has run is unbounded. + */ +const DIALECTS: ReadonlyArray<{ engine: string; text: string; knownToPredicate: boolean }> = [ + { engine: 'sqlite', text: 'SQLITE_ERROR: no such table: sys_metadata', knownToPredicate: true }, + { engine: 'postgres', text: 'relation "sys_metadata" does not exist', knownToPredicate: true }, + { engine: 'mysql', text: "Table 'crm.sys_metadata' doesn't exist", knownToPredicate: false }, + { engine: 'mssql', text: "Invalid object name 'sys_metadata'.", knownToPredicate: false }, + { engine: 'oracle', text: 'ORA-00942: table or view does not exist', knownToPredicate: false }, +]; + +/** Fragments that must never appear anywhere in a client-facing payload. */ +const LEAKED_FRAGMENTS = ['sys_metadata', 'SQLITE_ERROR', 'no such table', 'ORA-00942', 'Invalid object name']; + +function expectNothingLeaked(payload: unknown, dialectText: string): void { + const wire = JSON.stringify(payload); + expect(wire).not.toContain(dialectText); + for (const fragment of LEAKED_FRAGMENTS) expect(wire).not.toContain(fragment); +} + +// --------------------------------------------------------------------------- +// Type tiers, DERIVED from the registry (Prime Directive #8) — never listed +// --------------------------------------------------------------------------- + +/** + * `deleteMetaItem` picks its path from `isOverlayAllowed || isRuntimeCreateAllowed` + * — topology-independent (`useRepoPath`). So the registry decides which of the + * two re-wrap exits a type reaches, and a flag flipped there re-tiers these + * with nothing to keep in sync. + */ +const REPO_PATH_TYPE = DEFAULT_METADATA_TYPE_REGISTRY + .filter((e) => e.allowOrgOverride || e.allowRuntimeCreate) + .map((e) => e.type) + .sort()[0]!; + +/** Code-only (`allowRuntimeCreate: false` and `allowOrgOverride: false`) — the legacy raw-engine path #5264 kept alive. */ +const LEGACY_PATH_TYPE = DEFAULT_METADATA_TYPE_REGISTRY + .filter((e) => !e.allowOrgOverride && !e.allowRuntimeCreate) + .map((e) => e.type) + .sort()[0]!; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +type Verb = 'find' | 'findOne' | 'insert' | 'update' | 'delete'; + +interface Row { + id: string; + type: string; + name: string; + organization_id: string | null; + package_id: string | null; + state: string; + metadata: string; + checksum: string; +} + +const seedRow = (type: string, name: string, packageId: string): Row => ({ + id: `row_${type}_${name}`, + type, + name, + organization_id: null, + package_id: packageId, + state: 'active', + metadata: JSON.stringify({ name, label: 'seeded' }), + checksum: 'sha256_disclosure_fixture', +}); + +/** + * A kernel whose driver fails the named verbs with `dbError`, exactly the way + * a missing table does. Everything else answers normally, so a case can let + * the package read succeed and fail only the per-item delete underneath it — + * which is how the `failed[]` data path is reached. + */ +function makeKernel(opts: { + dbError: string; + failOn: readonly Verb[]; + seed?: Row[]; + environmentId?: string; +}) { + const rows = new Map(); + for (const r of opts.seed ?? []) rows.set(r.id, r); + const fail = new Set(opts.failOn); + const boom = (verb: Verb) => { + if (fail.has(verb)) throw new Error(opts.dbError); + }; + + const engine: any = { + async find(table: string) { + boom('find'); + if (table !== 'sys_metadata') return []; + return Array.from(rows.values()); + }, + async findOne(table: string, o: { where: Record }) { + boom('findOne'); + if (table !== 'sys_metadata') return null; + for (const row of rows.values()) { + const ok = Object.entries(o?.where ?? {}).every(([k, v]) => + v === null || v === undefined + ? (row as any)[k] === null || (row as any)[k] === undefined + : (row as any)[k] === v); + if (ok) return row; + } + return null; + }, + async insert(_table: string, data: Record) { + boom('insert'); + return { id: String(data.id ?? 'r_new') }; + }, + async update() { + boom('update'); + return { id: null }; + }, + async delete(table: string, o?: Record) { + // [#4550] The producer's own delete-verb dispatch contract, so this + // double cannot accept a call `ObjectQL.delete` refuses. + assertEngineDeleteDispatch(o); + boom('delete'); + if (table !== 'sys_metadata') return { deleted: 0 }; + const id = (o as any)?.where?.id; + return { deleted: rows.delete(id) ? 1 : 0 }; + }, + registry: { + registerItem: () => {}, + registerObject: () => {}, + listItems: () => [], + getItem: () => undefined, + getArtifactItem: () => undefined, + removeRuntimeShadow: () => false, + removeOverlayEntry: () => {}, + uninstallPackage: () => {}, + }, + }; + + const protocol = new ObjectStackProtocolImplementation( + engine, + () => new Map(), + opts.environmentId, + ) as any; + return { protocol, rows }; +} + +/** Every refusal assertion in this file goes through here: `code` AND `status`, never one. */ +function expectDeclaredEnvelope(err: any, code: string, status: number): void { + expect(err?.code).toBe(code); + expect(err?.status).toBe(status); +} + +async function captureThrow(run: () => Promise): Promise { + try { + await run(); + } catch (e) { + return e; + } + throw new Error('expected the call to throw, and it resolved'); +} + +// --------------------------------------------------------------------------- +// 0. What the shared predicate actually knows — measured, not recalled +// --------------------------------------------------------------------------- + +describe('[#8136] the shared leak heuristic is dialect-bounded, which is why the cure is at the producer', () => { + it('recognises the two engines this repo runs, and none of the three it does not', () => { + for (const { engine, text, knownToPredicate } of DIALECTS) { + expect(looksLikeInternalErrorLeak(text), `${engine}: ${text}`).toBe(knownToPredicate); + } + // Stated positively so the asymmetry cannot be read as an accident: + // three of five phrasings of ONE condition are invisible to every + // boundary that runs the predicate. + expect(DIALECTS.filter((d) => !d.knownToPredicate)).toHaveLength(3); + }); +}); + +// --------------------------------------------------------------------------- +// 1. Carrier one — `deletePackage`'s first database touch +// --------------------------------------------------------------------------- + +describe('[#8136] a driver failure on the uninstall overlay read is declared, not disclosed', () => { + for (const { engine, text } of DIALECTS) { + it(`withholds the ${engine} phrasing and answers a declared envelope`, async () => { + const { protocol } = makeKernel({ dbError: text, failOn: ['find'] }); + + const err = await captureThrow(() => + protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true })); + + // The POSITIVE shape. This is the file's existing contract for "a + // `sys_metadata` read failed" (`metadataStoreUnavailableError`), + // reused rather than a second sentence minted for one condition. + expectDeclaredEnvelope(err, 'SERVICE_UNAVAILABLE', 503); + expect(String(err.message)).toContain('The metadata store could not be read'); + expectNothingLeaked({ message: err.message, code: err.code }, text); + }); + } + + it('relocates the driver error to `cause` rather than losing it', async () => { + const text = DIALECTS[0]!.text; + const { protocol } = makeKernel({ dbError: text, failOn: ['find'] }); + + const err = await captureThrow(() => + protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true })); + + // The operator half of the contract: withheld from the caller, intact + // for `handleRouteError` / `logWithheldServerFault`. Without this the + // fix would be indistinguishable from deleting the diagnostic. + expect(String((err as any).cause?.message)).toBe(text); + }); + + it('keeps the failure a FAILURE — an unreachable store is never reported as a completed uninstall', async () => { + // The trap in fixing this the other way: routing the read through + // `rethrowUnlessMetadataStoreUnprovisioned` would turn an outage into + // `rows = []`, and this method reports that as an uninstall that + // deleted nothing. ADR-0110 D3 — a miss and an outage are different + // facts — on a destructive verb. + const { protocol } = makeKernel({ dbError: DIALECTS[1]!.text, failOn: ['find'] }); + const err = await captureThrow(() => + protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true })); + expect(err).toBeInstanceOf(Error); + expect((err as any).status).toBeGreaterThanOrEqual(500); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Carrier two — `deleteMetaItem`'s two re-wrap exits +// --------------------------------------------------------------------------- + +describe('[#8136] the overlay-delete re-wraps name the operation without quoting the driver', () => { + it(`repository path (${REPO_PATH_TYPE}) withholds the driver line`, async () => { + const text = DIALECTS[0]!.text; + const { protocol } = makeKernel({ dbError: text, failOn: ['findOne'] }); + + const err = await captureThrow(() => + protocol.deleteMetaItem({ type: REPO_PATH_TYPE, name: 'acct_overlay' })); + + expect(String(err.message)).toContain('Failed to delete customization overlay'); + expectNothingLeaked({ message: err.message }, text); + // `status` is deliberately untouched by this card (#7426 owns it). + expect(err.status).toBe(500); + expect(String((err as any).cause?.message)).toBe(text); + }); + + it(`legacy raw-engine path (${LEGACY_PATH_TYPE}) withholds it too — one rule, both exits`, async () => { + const text = DIALECTS[2]!.text; + // Control-plane kernel (no environmentId) + a code-only type is the one + // topology that reaches the legacy path (#5264). + const { protocol } = makeKernel({ dbError: text, failOn: ['findOne'] }); + + const err = await captureThrow(() => + protocol.deleteMetaItem({ type: LEGACY_PATH_TYPE, name: 'nightly_job' })); + + expect(String(err.message)).toContain('Failed to delete customization overlay'); + expectNothingLeaked({ message: err.message }, text); + expect(err.status).toBe(500); + expect(String((err as any).cause?.message)).toBe(text); + }); + + it('withholds every dialect, including the three the predicate cannot see', async () => { + for (const { engine, text } of DIALECTS) { + const { protocol } = makeKernel({ dbError: text, failOn: ['findOne'] }); + const err = await captureThrow(() => + protocol.deleteMetaItem({ type: REPO_PATH_TYPE, name: 'acct_overlay' })); + expect(String(err.message), engine).not.toContain(text); + expectNothingLeaked({ message: err.message }, text); + } + }); + + /** + * ⚠️ Drafted as a `[GUARD]` in section 3 and MEASURED RED — see the file + * header. It is evidence, not a bound, and lives here for that reason: the + * unfixed re-wrap quoted `err.message` UNCONDITIONALLY, so a declared 5xx + * fault was disclosed exactly as a bare driver error was. + */ + it('withholds a DECLARED 5xx too — a server fault is the operator’s detail, not the caller’s', async () => { + // The reason the rule keys on 4xx rather than on "was anything declared + // at all": a producer that declared a fault has said the detail belongs + // in the log (`declaresServerFault`, #5811). + const fault: any = new Error('relation "sys_metadata" does not exist'); + fault.code = 'SERVICE_UNAVAILABLE'; + fault.status = 503; + + const { protocol } = makeKernel({ dbError: 'unused', failOn: [] }); + protocol.getOverlayRepo = () => ({ + get: async () => { throw fault; }, + delete: async () => { throw fault; }, + }); + + const err = await captureThrow(() => + protocol.deleteMetaItem({ type: REPO_PATH_TYPE, name: 'acct_overlay' })); + + expectNothingLeaked({ message: err.message }, fault.message); + expectDeclaredEnvelope(err, 'SERVICE_UNAVAILABLE', 503); + }); +}); + +// --------------------------------------------------------------------------- +// 3. [GUARD] The over-block bound — a DECLARED refusal keeps its sentence +// --------------------------------------------------------------------------- + +describe('[#8136] [GUARD] a declared 4xx refusal is quoted verbatim — green in BOTH directions, red under the over-broad variant', () => { + /** + * The bound that stops this fix being satisfied by "withhold everything". + * `SysMetadataRepository`'s refusals (`[item_locked]`, + * `[writable_package_required]`, `[no_draft]`, …) name the exact remedy, + * and #4277 is the card that exists so they do. Blanking them would trade a + * usability regression for no disclosure gain — measured red under the + * over-broad variant, see the PR body. + */ + it('keeps a repository refusal intact through the re-wrap', async () => { + const refusal: any = new Error( + "[item_locked] Cannot overlay 'view' in package 'showcase': that package is read-only. " + + 'Edit the source artifact and redeploy.', + ); + refusal.code = 'ITEM_LOCKED'; + refusal.status = 403; + + const { protocol } = makeKernel({ dbError: 'unused', failOn: [] }); + protocol.getOverlayRepo = () => ({ + get: async () => { throw refusal; }, + delete: async () => { throw refusal; }, + }); + + const err = await captureThrow(() => + protocol.deleteMetaItem({ type: REPO_PATH_TYPE, name: 'acct_overlay' })); + + // The prescription survives, whole — this is the half a blanket + // sanitizer would destroy. + expect(String(err.message)).toContain('[item_locked]'); + expect(String(err.message)).toContain('Edit the source artifact and redeploy.'); + // …and the envelope #7426 installed is unchanged. + expectDeclaredEnvelope(err, 'ITEM_LOCKED', 403); + }); + +}); + +// --------------------------------------------------------------------------- +// 4. The DATA path — #8131's half, closed at the producer +// --------------------------------------------------------------------------- + +describe('[#8136] the uninstall response body carries no driver text either', () => { + /** + * This is the half no HTTP boundary can fix. `failed[]` and `cleanups[]` + * ride onto a `PACKAGE_DELETE_PARTIAL` **400** inside `details` — not the + * message — so #8130's 5xx withhold and #8016's mapping both pass over it. + * + * ⛔ Note what is NOT asserted here: a filter inside the collector. There + * is none, deliberately. `failed[].error` is clean because + * `deleteMetaItem` is, which is what "fix it at the producer" means. + */ + it('`failed[].error` reports the item without the driver line', async () => { + const text = DIALECTS[1]!.text; + const { protocol } = makeKernel({ + dbError: text, + // The package read succeeds; only the per-item delete underneath it + // fails, which is the shape that produces a PARTIAL rather than a throw. + failOn: ['findOne'], + seed: [seedRow(REPO_PATH_TYPE, 'acct_overlay', 'com.acme.crm')], + }); + + const result = await protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true }); + + expect(result.failedCount).toBe(1); + expect(result.failed[0]?.name).toBe('acct_overlay'); + // The whole response body, the way the handler ships it in `details`. + expectNothingLeaked(result, text); + }); + + it('`cleanups[].error` withholds a failing cleanup’s driver line', async () => { + const text = DIALECTS[4]!.text; + const { protocol } = makeKernel({ dbError: text, failOn: [] }); + // A cleanup is arbitrary plugin code going straight at the engine — + // plugin-security removes package-owned `sys_permission_set` rows — so + // a driver failure lands in that catch verbatim. + protocol.registerUninstallCleanup('security-grants', async () => { + throw new Error(text); + }); + + const result = await protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true }); + + const cleanup = result.cleanups.find((c: any) => c.name === 'security-grants'); + expect(cleanup?.success).toBe(false); + expect(cleanup?.error).toBe('cleanup failed'); + expectNothingLeaked(result, text); + }); + + it('[GUARD] a cleanup that DECLARED a refusal keeps its sentence', async () => { + const refusal: any = new Error( + '[grant_revocation_blocked] 3 grants are pinned by an active session — retry after it ends.', + ); + refusal.code = 'VALIDATION_FAILED'; + refusal.status = 400; + + const { protocol } = makeKernel({ dbError: 'unused', failOn: [] }); + protocol.registerUninstallCleanup('security-grants', async () => { throw refusal; }); + + const result = await protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true }); + + const cleanup = result.cleanups.find((c: any) => c.name === 'security-grants'); + expect(cleanup?.error).toContain('[grant_revocation_blocked]'); + expect(cleanup?.error).toContain('retry after it ends.'); + }); +}); diff --git a/packages/rest/src/package-door-5xx-message-sanitization.test.ts b/packages/rest/src/package-door-5xx-message-sanitization.test.ts index 8be77af17c..01fc44747f 100644 --- a/packages/rest/src/package-door-5xx-message-sanitization.test.ts +++ b/packages/rest/src/package-door-5xx-message-sanitization.test.ts @@ -193,17 +193,60 @@ async function bootRealProtocol(dbError: string): Promise { return new ObjectStackProtocolImplementation(engine as any); } -describe('[#8086] a real sys_metadata failure, walked in process through this door', () => { - it('the premise guard: the protocol really does throw the driver line', async () => { - // Anti-vacuity for the whole section. If `deletePackage` ever stops letting - // the driver text out — option C, the real cure — this goes RED and the - // cases below stop proving anything, instead of silently passing over a - // path nothing can traverse. +/** + * [#8136] **OPTION C LANDED, AND THIS SECTION IS ITS SIGNAL — INVERTED, NOT + * REPAIRED.** + * + * What used to open this block was an anti-vacuity guard asserting that + * `protocol.deletePackage` really does let the driver line out: + * + * ```ts + * await expect(protocol.deletePackage({ … })).rejects.toThrow(SQLITE_NO_TABLE); + * ``` + * + * It was written to go RED the day the producer stopped disclosing — "option C, + * the real cure" — so that a reader came back and re-read this section instead + * of consuming a green suite as proof the door was covered. #8136 is that day. + * Per the card's own instruction the pin is inverted rather than mended: making + * it green again would mean re-teaching the protocol to leak. + * + * So the subject of this section has moved by one layer, deliberately: + * + * before — "the producer emits a driver line and this DOOR withholds it" + * now — "the producer emits no driver line at all, and the envelope it + * does emit is DECLARED rather than guessed from a bare `Error`" + * + * The end-to-end walk is kept exactly as it was, because it is still the only + * thing here that proves the whole path: a real `ObjectQL`, a real + * `ObjectStackProtocolImplementation`, a driver that fails every `sys_metadata` + * access, driven through the route a client calls. What changed is what it + * observes at the far end. + * + * ⚠️ This does NOT retire the door's withhold, and section 2 onward still pins + * it in full. `sendThrownError` guards every producer that reaches this + * registrar, not just `metadata-protocol`, and #8131's `service-package` + * producer is a separate card still in flight. The belt stays; what changed is + * that this particular producer no longer needs it. + */ +describe('[#8136] a real sys_metadata failure, walked in process through this door', () => { + it('the producer no longer discloses: the driver line never leaves `deletePackage`', async () => { + // The inverted guard. This is the same call the old premise guard made, + // asserting the opposite fact — and it is still the anti-vacuity anchor for + // the section: if the protocol ever starts interpolating driver text again, + // this goes red at the source rather than the door silently covering for it. const protocol = await bootRealProtocol(SQLITE_NO_TABLE); + // The POSITIVE shape first, so this guard cannot pass vacuously — a bare + // `rejects.not.toThrow(...)` is green for a rejection with ANY other + // message, including a different leak, and green-by-accident is the exact + // failure mode this section exists to prevent. + await expect( + protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true }), + ).rejects.toMatchObject({ code: 'SERVICE_UNAVAILABLE', status: 503 }); + await expect( protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true }), - ).rejects.toThrow(SQLITE_NO_TABLE); + ).rejects.not.toThrow(SQLITE_NO_TABLE); }, 60_000); it('the driver line does not appear anywhere in the client body', async () => { @@ -217,13 +260,17 @@ describe('[#8086] a real sys_metadata failure, walked in process through this do ); const error = expectDeclaredEnvelope(captured); - // The POSITIVE shape, not "it changed": this is the same replacement - // constant the dispatcher twin and `rest-server.ts` use. - expect(error.message).toBe(INTERNAL_ERROR_MESSAGE); - // The full ADR-0112 envelope. Still a server fault on the wire — the - // withhold touches the prose and nothing else. - expect(captured.status).toBe(500); - expect(error.code).toBe('INTERNAL_ERROR'); + // [#8136] The envelope is now the producer's own DECLARATION, not this + // door's guess. `metadata-protocol` answers an unreadable metadata store + // with 503 / `SERVICE_UNAVAILABLE` — the contract it already used for that + // exact condition — so the door forwards a declared refusal instead of + // falling to the undeclared-500 default and withholding its prose. + expect(captured.status).toBe(503); + expect(error.code).toBe('SERVICE_UNAVAILABLE'); + // Still the POSITIVE shape, not "it changed": the authored sentence, which + // is safe to ship precisely because it quotes nothing. + expect(error.message).toContain('The metadata store could not be read'); + expect(error.message).not.toBe(INTERNAL_ERROR_MESSAGE); const wire = JSON.stringify(captured.body); expect(wire).not.toContain('SQLITE_ERROR'); @@ -232,30 +279,26 @@ describe('[#8086] a real sys_metadata failure, walked in process through this do }, 60_000); /** - * [#8132] Was the residual; now the second green. - * - * This case was added by #8086 as a deliberately-red-in-future pin: the - * shared predicate was a heuristic over the message and knew no Postgres - * `relation … does not exist` phrasing, so that dialect's line still - * travelled through this door while SQLite's was withheld — the same - * condition, disclosed or not depending on which engine was underneath. It - * asserted that gap positively so the day it closed would be visible. + * [#8132 → #8136] Twice-inverted, and the trail is the point. * - * #8132 closed it in the predicate, where it belonged — so the assertion is - * INVERTED here rather than deleted, and the pair above/below now proves the - * property that actually matters: this door answers the same withheld - * envelope for BOTH dialects of one failure. + * #8086 added this as a deliberately-red-in-future pin: the shared predicate + * knew no Postgres `relation … does not exist`, so that dialect's line + * travelled through this door while SQLite's was withheld. #8132 / #8263 + * closed that IN THE PREDICATE and the case flipped to "withheld too, by the + * shared predicate". #8136 now removes the disclosure at the producer, so + * there is nothing left for the predicate to decide about this path. * - * ⚠️ Still not the structural cure, and this comment is the reason the - * pointer survives the flip. The predicate now recognises the two engines - * this repo runs; it is a phrasing test, and a phrasing test can only ever - * know the dialects someone has met. **Option C** — `metadata-protocol` not - * interpolating driver text into client-facing messages at all — is the fix - * whose correctness does not depend on that, and is tracked as #8136. The - * anti-vacuity guard at the top of this describe block goes red when C - * lands, which is the intended signal to revisit this whole section. + * ⚠️ The predicate assertion is KEPT, and deliberately still asserts `true` — + * it records that the interim belt is real and still standing for every other + * producer. What it no longer does is carry the weight of this path, and that + * is the structural difference option C bought: the body below is clean for a + * dialect the predicate has never met just as surely as for one it has. + * `packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts` + * measures that directly, across five dialects, three of which the predicate + * cannot see. */ - it('the Postgres phrasing of the same failure is withheld too, by the shared predicate', async () => { + it('the Postgres phrasing of the same failure is withheld at the producer now', async () => { + // The interim belt still exists and still recognises this phrasing. expect(looksLikeInternalErrorLeak(PG_NO_RELATION)).toBe(true); const protocol = await bootRealProtocol(PG_NO_RELATION); @@ -267,11 +310,11 @@ describe('[#8086] a real sys_metadata failure, walked in process through this do ); const error = expectDeclaredEnvelope(captured); - expect(captured.status).toBe(500); - expect(error.code).toBe('INTERNAL_ERROR'); - // The same positive shape the SQLite case asserts, which is the point of - // the flip: one door, one envelope, regardless of the engine underneath. - expect(error.message).toBe(INTERNAL_ERROR_MESSAGE); + // One door, one envelope, regardless of the engine underneath — the + // property the flip was always about, now held one layer earlier. + expect(captured.status).toBe(503); + expect(error.code).toBe('SERVICE_UNAVAILABLE'); + expect(error.message).toContain('The metadata store could not be read'); const wire = JSON.stringify(captured.body); expect(wire).not.toContain('does not exist'); From 91b0268d239ccb3841d2266deac93ea4d5842750 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 05:47:09 +0000 Subject: [PATCH 3/4] test(rest): keep #5437's log guarantee measuring after the producer withholds (#8136) --- .../src/rest-5xx-message-sanitization.test.ts | 55 ++++++++++++++++--- 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/packages/rest/src/rest-5xx-message-sanitization.test.ts b/packages/rest/src/rest-5xx-message-sanitization.test.ts index 141fd64d57..2b08febac0 100644 --- a/packages/rest/src/rest-5xx-message-sanitization.test.ts +++ b/packages/rest/src/rest-5xx-message-sanitization.test.ts @@ -139,12 +139,41 @@ beforeEach(() => { }); afterEach(() => { spy.mockRestore(); }); -/** Did ANY log line carry the given text (in its message or its arguments)? */ +/** + * Did ANY log line carry the given text (in its message, its `cause` chain, or + * its arguments)? + * + * [#8136] The `cause` traversal is not a convenience — it is where the driver + * text now lives. This helper used to read `a.message` only, which was + * sufficient while `metadata-protocol` interpolated the driver line INTO the + * message it threw. Option C stopped it doing that: the client-facing sentence + * names the operation and quotes nothing, and the original driver error is + * carried on `cause`. `console.error('[REST] Unhandled error:', err)` still + * receives it in full — Node formats an `Error`'s `[cause]` along with it — so + * the operator's half of the contract is unchanged. + * + * Measured at this seam after the producer fix, so the traversal is not + * speculative: + * + * ``` + * [REST] Unhandled error: Error(Failed to delete customization overlay for + * object/showcase_account. …) <> SQLITE_ERROR: no such table: sys_metadata + * ``` + * + * ⛔ Do NOT "fix" a future red here by deleting the `loggedText` assertions. + * They are the only thing in this file asserting that withholding text from the + * CLIENT did not also delete it from the LOG, which is the failure mode a + * disclosure fix is most likely to introduce. + */ function loggedText(needle: string): boolean { - return logged.some((args) => args.some((a) => { - if (a instanceof Error) return a.message.includes(needle); + const carries = (a: unknown, depth = 0): boolean => { + if (depth > 5) return false; + if (a instanceof Error) { + return a.message.includes(needle) || carries((a as { cause?: unknown }).cause, depth + 1); + } return typeof a === 'string' && a.includes(needle); - })); + }; + return logged.some((args) => args.some((a) => carries(a))); } // --------------------------------------------------------------------------- @@ -231,11 +260,19 @@ describe('[#5437] a real sys_metadata failure, walked in process', () => { }, 60_000); it('the withheld text still reaches the server log, in full', async () => { - // The premise guard and the log guarantee in one assertion: the only - // way this text can reach the log is if the protocol really did - // interpolate the driver error into the message it threw. If the - // producer ever stops doing that, this goes red and the case above - // stops proving anything. + // [#8136] This case's own prediction came true, and it is corrected in + // place rather than deleted. It used to say "the only way this text can + // reach the log is if the protocol really did interpolate the driver + // error into the message it threw. If the producer ever stops doing + // that, this goes red" — and option C is the producer doing exactly + // that. It is no longer a premise guard for the case above. + // + // What survives, and is the reason the case stays, is the LOG + // GUARANTEE: the driver line still reaches the operator in full, now on + // the thrown error's `cause` rather than inside its message. That is + // the half a disclosure fix is most likely to break by accident — + // withholding from the client by simply dropping the diagnostic — so it + // is asserted here at the boundary as well as at the producer. const rest = await bootRealProtocol(SQLITE_NO_TABLE); await callRoute(rest, 'DELETE', META_ITEM, { From 056fb8456cf9cb0a4ce859bc25440897447a14ed Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 06:17:08 +0000 Subject: [PATCH 4/4] test(metadata-protocol): pin the disclosure fake's update() to the producer dispatch contract (#8136) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:engine-double-contract` flagged the engine double in protocol.driver-text-disclosure.test.ts (line 195): its `update()` accepted call shapes `ObjectQL.update` refuses, which is how #4434 shipped a dead REST route with its suite green. The double's `delete()` was already pinned via `assertEngineDeleteDispatch`; only `update` was loose. Open the fake's `update` with `assertEngineUpdateDispatch(data, options)` from `@objectstack/metadata-core` — never `@objectstack/objectql`, which depends on this package, so that import would close a cycle turbo refuses. The file already imported the delete predicate from metadata-core, so this needs no package.json change and no lockfile churn. The shrink-only baseline ledger is untouched: this is the real pin, not an exemption. update doubles go 90 -> 91 pinned. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH --- .../src/protocol.driver-text-disclosure.test.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts b/packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts index 084bddcc44..321c85d401 100644 --- a/packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts +++ b/packages/metadata-protocol/src/protocol.driver-text-disclosure.test.ts @@ -88,7 +88,10 @@ * from "refused correctly". Every refusal case asserts `code` AND `status`. */ import { describe, expect, it } from 'vitest'; -import { assertEngineDeleteDispatch } from '@objectstack/metadata-core'; +// [#5619] The producer's OWN write-verb dispatch decisions (#4550 delete / +// #5480 update). From `@objectstack/metadata-core`, never `@objectstack/objectql` +// — objectql depends on THIS package, so that import would close a cycle. +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; import { looksLikeInternalErrorLeak } from '@objectstack/types'; import { DEFAULT_METADATA_TYPE_REGISTRY } from '@objectstack/spec/kernel'; import { ObjectStackProtocolImplementation } from './protocol.js'; @@ -214,7 +217,10 @@ function makeKernel(opts: { boom('insert'); return { id: String(data.id ?? 'r_new') }; }, - async update() { + async update(_table: string, data: Record, o?: Record) { + // [#5480] The producer's own update-verb dispatch contract, so this + // double cannot accept a call `ObjectQL.update` refuses. + assertEngineUpdateDispatch(data, o); boom('update'); return { id: null }; },