diff --git a/.changeset/package-door-withholds-leaky-5xx-message.md b/.changeset/package-door-withholds-leaky-5xx-message.md new file mode 100644 index 0000000000..db0a1310bf --- /dev/null +++ b/.changeset/package-door-withholds-leaky-5xx-message.md @@ -0,0 +1,50 @@ +--- +"@objectstack/rest": patch +--- + +fix(rest): the direct-mount package door withholds a leaky 5xx message, like its two siblings already do (#8086) + +A driver failure under `/api/v1/packages` returned the driver's own line to the +API client. Reproduced end to end through a real `ObjectQL` engine and a real +`ObjectStackProtocolImplementation`, with the driver failing the `sys_metadata` +read the way a missing table does — `DELETE /api/v1/packages/:id` answered: + +``` +HTTP 500 +{"success":false,"error":{"code":"INTERNAL_ERROR", + "message":"SQLITE_ERROR: no such table: sys_metadata"}} +``` + +That path is not exotic: a full uninstall (no `?version=`) routes to +`protocol.deletePackage`, whose first database touch sits outside its own +per-item `try`, so the driver line propagates whole into this registrar's +catch-all and onto the wire. + +**Not a new rule — the rule this surface already followed, at the door that was +missed.** Both siblings sanitize: the dispatcher twin (`HttpDispatcher.error`) +has replaced a leaky 5xx message with the generic text since #3867, and +`rest-server.ts` runs the same predicate at three call sites. The earlier fix +for this class (#5437) reached neither this registrar nor could it, because +this door does not go through `resolveErrorResponse` at all. `/api/v1/packages` +has **two** HTTP doors and this one mounts first in the production stack, so +the unfiltered answer was the live one — one deployment, two different answers +to the same failure. + +**Only the prose is withheld.** `status`, `code` and `details` are untouched, +so a client can still branch on the code and the coded-refusal mapping added in +#8016 still answers. The full text still reaches the server log and the error +reporter. + +**4xx is deliberately untouched.** A refusal's message is caller-facing by +design — `[tenant_scope_required]` names the exact parameter to pass, a `409 +DESTRUCTIVE_CHANGE` names the remedy — and it is disclosed where disclosure +costs nothing, because the caller supplied the input. Withholding those would +delete the self-correcting sentence that makes the refusal actionable. + +**Known ceiling, stated so a green suite is not mistaken for full coverage.** +The shared predicate is a heuristic over the message and does not recognise +Postgres's `relation "…" does not exist` phrasing, so that dialect's line still +travels — here and through the dispatcher twin alike, since both run the same +predicate. Widening it at one door would re-create the divergence this closes. +The cure is to stop interpolating driver text into client-facing messages at +the producer, which is tracked separately. diff --git a/packages/rest/src/package-door-5xx-message-sanitization.test.ts b/packages/rest/src/package-door-5xx-message-sanitization.test.ts new file mode 100644 index 0000000000..ab01860720 --- /dev/null +++ b/packages/rest/src/package-door-5xx-message-sanitization.test.ts @@ -0,0 +1,582 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8086] The direct-mount package door withholds a LEAKY 5xx message. + * + * ## The gap this closes + * + * `packages/rest/src/package-routes.ts` writes every error through the shared + * `sendError`, and neither it nor `sendError` applied any leak heuristic. Both + * siblings on this surface already do: + * + * - the dispatcher twin — `HttpDispatcher.error` + * (`packages/runtime/src/http-dispatcher.ts`) replaces the message with + * `INTERNAL_ERROR_MESSAGE` when `httpStatus >= 500 && + * looksLikeInternalErrorLeak(message)` (#3867); + * - `rest-server.ts` — three call sites run the same predicate (#5437 / + * PR #5464), which closed exactly this class one seam over and never + * reached this registrar, because it does not go through + * `resolveErrorResponse` at all. + * + * So one door of `/api/v1/packages` withheld a leaky 5xx and the other did + * not, on the same deployment — and this registrar is the one production + * serves for the routes both declare (first-match-wins, see the module note in + * `package-routes.ts`). + * + * This is option **B** of the three the card recorded, and the only one ruled: + * apply the rule this surface already follows, at the door that was missed. + * Option A (put it inside the shared `sendError`) is escalated and NOT taken — + * that would make a disclosure rule a property of the envelope writer, which + * its own module note disclaims, and it reaches 7+ route modules. Option C + * (stop `metadata-protocol` interpolating driver text into client-facing + * messages) is the real cure and is a separate card; this is the interim that + * stops the bleeding. + * + * ## Reachability was MEASURED, not assumed + * + * The card was filed `Unverified`: grep proved the *filter was absent*, which + * is a different claim from the *leak being reachable*. Section 1 settles it by + * observation — a REAL `ObjectQL` engine, a REAL + * `ObjectStackProtocolImplementation`, and a driver that fails every + * `sys_metadata` access the way a missing table does, driven through the route + * a client calls. The producer walked is `protocol.deletePackage`'s + * `engine.find('sys_metadata', …)`, which sits OUTSIDE that method's per-item + * `try`/`catch` and so propagates whole. + * + * Before this change that request answered, verbatim: + * + * HTTP 500 + * {"success":false,"error":{"code":"INTERNAL_ERROR", + * "message":"SQLITE_ERROR: no such table: sys_metadata"}} + * + * ## Reverse verification, direction predicted BEFORE running + * + * Deleting the two-line withhold in `sendThrownError` turns the section-1 and + * section-2 leak cases RED — they assert the positive sanitized shape, so the + * driver line reappears in the diff — and leaves every pass-through case + * (section 3) and every 4xx case (section 4) GREEN, because the predicate is + * what decides and neither of those trips it. That is the ordinary direction, + * and it was confirmed by running it (quoted in the PR). + * + * ## What is deliberately NOT asserted + * + * That the message merely "changed", or that it "no longer contains the table + * name". Both pass for any rewrite, including a worse one. Every case below + * asserts the POSITIVE shape — `INTERNAL_ERROR_MESSAGE` — plus the full + * ADR-0112 envelope (`code` AND `status`), because one alone is not an answer: + * a status without the code leaves the client unable to branch, a code without + * the status makes every proxy and retry policy read a refusal as a fault. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ApiErrorSchema, BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; +import type { RouteHandler } from '@objectstack/spec/contracts'; +import { ObjectQL } from '@objectstack/objectql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { INTERNAL_ERROR_MESSAGE, looksLikeInternalErrorLeak } from '@objectstack/types'; +import { registerPackageRoutes } from './package-routes.js'; + +const PKGS = '/api/v1/packages'; + +/** The driver line a missing `sys_metadata` produces on each dialect. */ +const SQLITE_NO_TABLE = 'SQLITE_ERROR: no such table: sys_metadata'; +const PG_NO_RELATION = 'relation "sys_metadata" does not exist'; + +interface Captured { + status: number; + body: any; +} + +/** A caller holding every capability these routes gate on. */ +const CLEARS_THE_GATE = async () => ({ + userId: 'u_pkg', + systemPermissions: ['manage_metadata', 'studio.access', 'setup.access'], +}); + +function mount(svc: Record, options: Record = {}) { + const routes = new Map(); + const server = { + get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); }, + post: (p: string, h: RouteHandler) => { routes.set(`POST:${p}`, h); }, + put: (p: string, h: RouteHandler) => { routes.set(`PUT:${p}`, h); }, + delete: (p: string, h: RouteHandler) => { routes.set(`DELETE:${p}`, h); }, + patch: () => {}, + use: () => {}, + listen: async () => {}, + close: async () => {}, + } as any; + registerPackageRoutes(server, () => svc as any, '/api/v1', { + resolveExecutionContext: CLEARS_THE_GATE, + ...options, + } as any); + return routes; +} + +async function drive( + routes: Map, + method: string, + path: string, + req: Record = {}, +): Promise { + const handler = routes.get(`${method}:${path}`); + if (!handler) throw new Error(`no handler for ${method} ${path}`); + const captured: Captured = { status: 0, body: undefined }; + const res: any = { + json(data: any) { captured.body = data; }, + send() {}, + status(code: number) { captured.status = code; return res; }, + header() { return res; }, + }; + await handler( + { params: {}, query: {}, body: undefined, headers: {}, method, path, ...req } as any, + res, + ); + return captured; +} + +/** + * Every assertion an answer from this door must satisfy, spelled once and + * IMPORTED from `packages/spec` rather than restated — a body that parses here + * is one the wire contract accepts, including `code` being a member of the + * closed ADR-0112 vocabulary. + */ +function expectDeclaredEnvelope(captured: Captured): any { + expect(BaseResponseSchema.safeParse(captured.body).success).toBe(true); + expect(envelopeViolations(captured.body)).toEqual([]); + expect(captured.body?.success).toBe(false); + const parsed = ApiErrorSchema.safeParse(captured.body?.error); + expect(parsed.error?.issues ?? []).toEqual([]); + expect(parsed.success).toBe(true); + return captured.body.error; +} + +/** A thrown error carrying whatever a producer declares on it. */ +function thrown(message: string, carried: Record): Error { + return Object.assign(new Error(message), carried); +} + +// --------------------------------------------------------------------------- +// 1. The real producer, end to end — the card's "Unverified" half +// --------------------------------------------------------------------------- +// +// Nothing is hand-built here: the engine, the protocol and the driver text all +// come from shipping code, and the route is the one a client calls. +// +// `DELETE /api/v1/packages/:id` with no `?version=` routes to +// `protocol.deletePackage` (`package-routes.ts`, the `!version && typeof +// options.protocol?.deletePackage === 'function'` branch). That method's FIRST +// database touch is `this.engine.find('sys_metadata', { where })`, outside any +// `try` — its per-item `catch` only wraps the `deleteMetaItem` loop below it. +// So a driver failure on the overlay read propagates whole, out of the +// protocol, into this registrar's catch-all, and onto the wire. + +function failingDriver(dbError: string) { + const boom = () => { throw new Error(dbError); }; + const driver: any = { + name: 'memory-broken', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find() { boom(); }, async findOne() { boom(); }, + async create() { boom(); }, async update() { boom(); }, async delete() { boom(); }, + async upsert() { boom(); }, async count() { boom(); }, + async bulkCreate() { boom(); }, async bulkUpdate() { boom(); }, async bulkDelete() { boom(); }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return driver; +} + +async function bootRealProtocol(dbError: string): Promise { + const engine = new ObjectQL(); + engine.registerDriver(failingDriver(dbError), true); + await engine.init(); + 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. + const protocol = await bootRealProtocol(SQLITE_NO_TABLE); + + await expect( + protocol.deletePackage({ packageId: 'com.acme.crm', allTenants: true }), + ).rejects.toThrow(SQLITE_NO_TABLE); + }, 60_000); + + it('the driver line does not appear anywhere in the client body', async () => { + const protocol = await bootRealProtocol(SQLITE_NO_TABLE); + + const captured = await drive( + mount({ delete: async () => ({ success: true }) }, { protocol }), + 'DELETE', + `${PKGS}/:id`, + { params: { id: 'com.acme.crm' } }, + ); + + 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'); + + const wire = JSON.stringify(captured.body); + expect(wire).not.toContain('SQLITE_ERROR'); + expect(wire).not.toContain('no such table'); + expect(wire).not.toContain('sys_metadata'); + }, 60_000); + + /** + * ⚠️ The CEILING of option B, measured and pinned rather than papered over. + * + * The ruled fix applies the SHARED predicate, which is a heuristic over the + * message and recognises no Postgres "relation … does not exist" phrasing — + * measured false, asserted below. So that dialect's line still travels + * through this door after this change, exactly as it travels through the + * dispatcher twin, which runs the same predicate (#3867). The two doors + * therefore still AGREE, which is what this card was about; what remains is a + * property of the heuristic, shared by every boundary that applies it. + * + * This is not an argument for widening the predicate here — that would be a + * new rule at one door, re-creating the divergence this closes. It is the + * argument for **option C**: `metadata-protocol` should not interpolate + * driver text into client-facing messages at all, which is the only fix that + * does not depend on recognising a dialect's phrasing. Filed separately. + * + * This case goes RED the day the shared predicate learns this phrasing or C + * lands — which is precisely when a reader should come back and re-read the + * paragraph above, instead of consuming a green suite as proof that the door + * is covered. + */ + it('the residual: Postgres phrasing trips no keyword, so it still travels (option C is the cure)', async () => { + expect(looksLikeInternalErrorLeak(PG_NO_RELATION)).toBe(false); + + const protocol = await bootRealProtocol(PG_NO_RELATION); + const captured = await drive( + mount({ delete: async () => ({ success: true }) }, { protocol }), + 'DELETE', + `${PKGS}/:id`, + { params: { id: 'com.acme.crm' } }, + ); + + const error = expectDeclaredEnvelope(captured); + expect(captured.status).toBe(500); + expect(error.code).toBe('INTERNAL_ERROR'); + // Stated as the fact it is: withheld would be better, and the predicate + // cannot tell. Asserted positively so the day it changes is visible. + expect(error.message).toContain('does not exist'); + expect(error.message).not.toBe(INTERNAL_ERROR_MESSAGE); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// 2. Every catch site in this registrar, and the whole 5xx band +// --------------------------------------------------------------------------- +// +// The live half above proves the door. These prove it is the DOOR and not one +// route: all four handlers exit through the same `sendThrownError`, so each is +// driven separately rather than assumed to share the fix. +// +// `GET /packages` is different BY DESIGN: both of its data sources sit in their +// own inner `try { … } catch {}`, so nothing below reaches the outer catch. +// What does is the gate — `refusePackageRequest` calls +// `options.resolveExecutionContext(req)`, and a resolver that throws +// SYNCHRONOUSLY throws before the `.catch(() => undefined)` is attached. + +interface Site { + name: string; + run: (error: unknown) => Promise<{ captured: Captured; reached: () => boolean }>; +} + +const MANIFEST = { id: 'com.acme.crm', version: '1.0.0' }; + +const SITES: Site[] = [ + { + name: 'POST /packages/publish — packageService.publish throws', + run: async (error: unknown) => { + const publish = vi.fn(async () => { throw error; }); + const captured = await drive( + mount({ publish }), + 'POST', + `${PKGS}/publish`, + { body: { manifest: MANIFEST, metadata: { author: 'acme' } } }, + ); + return { captured, reached: () => publish.mock.calls.length === 1 }; + }, + }, + { + name: 'GET /packages — the capability gate resolver throws', + run: async (error: unknown) => { + const resolveExecutionContext = vi.fn(() => { throw error; }); + const captured = await drive( + mount({ list: async () => [] }, { resolveExecutionContext }), + 'GET', + PKGS, + ); + return { captured, reached: () => resolveExecutionContext.mock.calls.length === 1 }; + }, + }, + { + name: 'GET /packages/:id — packageService.get throws', + run: async (error: unknown) => { + const get = vi.fn(async () => { throw error; }); + const captured = await drive( + mount({ get }), + 'GET', + `${PKGS}/:id`, + { params: { id: 'com.acme.crm' } }, + ); + return { captured, reached: () => get.mock.calls.length === 1 }; + }, + }, + { + name: 'DELETE /packages/:id — packageService.delete throws', + run: async (error: unknown) => { + const del = vi.fn(async () => { throw error; }); + const captured = await drive( + mount({ delete: del }), + 'DELETE', + `${PKGS}/:id`, + { params: { id: 'com.acme.crm' } }, + ); + return { captured, reached: () => del.mock.calls.length === 1 }; + }, + }, +]; + +describe('[#8086] a leaky 5xx is withheld at every catch site, across the band', () => { + /** + * Deliberately spans the band and both `code` postures: an UNDECLARED fault + * (the 500 arm, where the code derives from the status) and a DECLARED 5xx + * carrying its own registered code. The code and status must survive in both + * — the withhold is about the prose only, and #8016's mapping must not be + * undone by it. + */ + const LEAKS: Array<{ name: string; error: unknown; status: number; code: string }> = [ + { + name: 'a bare driver throw (undeclared ⇒ 500 INTERNAL_ERROR)', + error: new Error(SQLITE_NO_TABLE), + status: 500, + code: 'INTERNAL_ERROR', + }, + { + // The producer #5437 named by line, copied verbatim — `status` ASSIGNED + // to an already-constructed error rather than written as a `status:` + // literal, which is the shape a grep does not find. + name: 'the metadata-protocol overlay producer, verbatim', + error: thrown(`Failed to delete customization overlay: ${SQLITE_NO_TABLE}`, { status: 500 }), + status: 500, + code: 'INTERNAL_ERROR', + }, + { + name: 'a constraint dump naming physical columns', + error: new Error('UNIQUE constraint failed: sys_metadata.name'), + status: 500, + code: 'INTERNAL_ERROR', + }, + { + name: 'a bare statement prefix, the shape drivers put in front of their message', + error: new Error('SELECT * FROM sys_packages WHERE id = ? — near "FROM": syntax error'), + status: 500, + code: 'INTERNAL_ERROR', + }, + { + name: 'a DECLARED 503 with a registered code keeps both and loses only the prose', + error: thrown('SQLSTATE 08006: connection failure to the metadata store', { + status: 503, + code: 'SERVICE_UNAVAILABLE', + }), + status: 503, + code: 'SERVICE_UNAVAILABLE', + }, + ]; + + for (const site of SITES) { + for (const leak of LEAKS) { + it(`${site.name}: ${leak.name}`, async () => { + // Premise guard for the shaped half: these really are messages the + // predicate calls leaks. A case that quietly stopped tripping it would + // otherwise "pass" section 3's pass-through rule instead. + expect(looksLikeInternalErrorLeak((leak.error as Error).message)).toBe(true); + + const { captured, reached } = await site.run(leak.error); + expect(reached(), 'the throwing seam was never called').toBe(true); + + const error = expectDeclaredEnvelope(captured); + expect(error.message).toBe(INTERNAL_ERROR_MESSAGE); + expect(captured.status).toBe(leak.status); + expect(error.code).toBe(leak.code); + expect(JSON.stringify(captured.body)).not.toContain('sys_'); + }); + } + } +}); + +// --------------------------------------------------------------------------- +// 3. The PREDICATE decides — not a blanket 5xx replacement +// --------------------------------------------------------------------------- +// +// Without this section the whole file is satisfied by `if (status >= 500) +// message = INTERNAL_ERROR_MESSAGE`, which is a different rule: it would delete +// every self-authored server-fault sentence this door has (`This deployment +// serves no marketplace publish surface…`-class prose, a 501's stated remedy), +// and it would silently diverge from the twin, whose whole point is that the +// two doors answer alike. + +describe('[#8086] a 5xx that does NOT look like a leak passes through unchanged', () => { + const PASS_THROUGH: Array<{ name: string; error: unknown; status: number; code: string }> = [ + { + name: 'a plain undeclared fault from our own code', + error: new Error('kaboom'), + status: 500, + code: 'INTERNAL_ERROR', + }, + { + name: "the atomic-batch refusal's stated remedy (501, declared code)", + error: thrown( + "Atomic batch on 'showcase_account' requires engine transaction support; this runtime cannot roll back.", + { status: 501, code: 'NOT_IMPLEMENTED' }, + ), + status: 501, + code: 'NOT_IMPLEMENTED', + }, + { + name: 'a 503 whose prose names no internals', + error: thrown('The marketplace registry is warming up; retry shortly.', { + status: 503, + code: 'SERVICE_UNAVAILABLE', + }), + status: 503, + code: 'SERVICE_UNAVAILABLE', + }, + ]; + + for (const site of SITES) { + for (const shape of PASS_THROUGH) { + it(`${site.name}: ${shape.name}`, async () => { + expect(looksLikeInternalErrorLeak((shape.error as Error).message)).toBe(false); + + const { captured, reached } = await site.run(shape.error); + expect(reached(), 'the throwing seam was never called').toBe(true); + + const error = expectDeclaredEnvelope(captured); + expect(error.message).toBe((shape.error as Error).message); + expect(error.message).not.toBe(INTERNAL_ERROR_MESSAGE); + expect(captured.status).toBe(shape.status); + expect(error.code).toBe(shape.code); + }); + } + } +}); + +// --------------------------------------------------------------------------- +// 4. The OVER-BLOCK guard: 4xx is untouched +// --------------------------------------------------------------------------- +// +// A 4xx refusal's message is caller-facing BY DESIGN — it is the self-correcting +// sentence #4277 exists for, and #5436's truncation deliberately preserves its +// head for the same reason. Sanitizing those would destroy the one thing that +// tells an author how to fix their request, and it would do it precisely where +// the message costs nothing to disclose (the caller supplied the input). +// +// The cases are chosen to TRIP the predicate on purpose: their prose contains +// the words the heuristic keys on. Only the status keeps them intact, which is +// exactly the half a "sanitize by message alone" implementation would lose. + +describe('[#8086] a 4xx message is never withheld, even when it trips the predicate', () => { + const FOUR_XX: Array<{ name: string; error: unknown; status: number; code: string }> = [ + { + name: "the protocol's TENANT_SCOPE_REQUIRED refusal, wording that trips the predicate", + error: thrown( + "[tenant_scope_required] Refusing to uninstall 'com.acme.crm': foreign key rows in sys_metadata " + + 'would be orphaned — pass organizationId to scope it, or allTenants: true to confirm.', + { status: 400, code: 'TENANT_SCOPE_REQUIRED' }, + ), + status: 400, + code: 'TENANT_SCOPE_REQUIRED', + }, + { + name: 'the established 409 DESTRUCTIVE_CHANGE, naming the tables it would drop', + error: thrown( + 'Uninstalling drops 3 tables; unique constraint on showcase_account would be lost. ' + + 'Pass force: true to confirm.', + { status: 409, code: 'DESTRUCTIVE_CHANGE' }, + ), + status: 409, + code: 'DESTRUCTIVE_CHANGE', + }, + { + name: 'a 499 — the last client status, the bound from below', + error: thrown('UNIQUE constraint failed: sys_packages.id', { status: 499 }), + status: 499, + // `HttpStatusErrorCodeMap` does not name 499, so the code falls to the + // client-error bucket — `standardErrorCodeForHttpStatus`'s `< 500` arm. + code: 'VALIDATION_ERROR', + }, + ]; + + for (const site of SITES) { + for (const refusal of FOUR_XX) { + it(`${site.name}: ${refusal.name}`, async () => { + // These would every one of them be withheld if the rule read the + // message alone — that is the point of choosing them. + expect(looksLikeInternalErrorLeak((refusal.error as Error).message)).toBe(true); + + const { captured, reached } = await site.run(refusal.error); + expect(reached(), 'the throwing seam was never called').toBe(true); + + const error = expectDeclaredEnvelope(captured); + expect(error.message).toBe((refusal.error as Error).message); + expect(captured.status).toBe(refusal.status); + expect(error.code).toBe(refusal.code); + }); + } + } + + it('the bound itself, from both sides: 499 verbatim, 500 withheld', async () => { + // An off-by-one here either starts withholding 4xx bodies or leaves 500 + // open — the two ways this fix can be wrong, pinned in one case. + const message = 'UNIQUE constraint failed: sys_packages.id'; + for (const [status, expected] of [[499, message], [500, INTERNAL_ERROR_MESSAGE]] as const) { + const { captured } = await SITES[0].run(thrown(message, { status })); + expect(captured.status, `status ${status}`).toBe(status); + expect(captured.body?.error?.message, `status ${status}`).toBe(expected); + } + }); +}); + +// --------------------------------------------------------------------------- +// 5. #8016 must not regress +// --------------------------------------------------------------------------- +// +// This change rewrote the expression #8016 landed, so its half is re-pinned at +// the same seam rather than trusted: the coded refusal mapping is what makes a +// 4xx a 4xx here, and section 4 is only meaningful while it holds. + +describe('[#8086] the #8016 coded mapping still answers (non-regression)', () => { + it('a coded 409 keeps its status, its code AND its message', async () => { + const { captured } = await SITES[3].run( + thrown('Uninstalling drops 3 tables', { status: 409, code: 'DESTRUCTIVE_CHANGE' }), + ); + expect(captured.status).toBe(409); + expect(captured.body?.error?.code).toBe('DESTRUCTIVE_CHANGE'); + expect(captured.body?.error?.message).toBe('Uninstalling drops 3 tables'); + }); + + it('structured `details` survive the withhold on a leaky 5xx', async () => { + // The withhold is scoped to the MESSAGE. `details` carries `issues`/`fields` + // the UI maps back to inputs; dropping it here would be a second, unruled + // change riding along. + const { captured } = await SITES[0].run( + thrown(SQLITE_NO_TABLE, { status: 500, issues: [{ path: 'manifest.id', message: 'Required' }] }), + ); + expect(captured.status).toBe(500); + expect(captured.body?.error?.message).toBe(INTERNAL_ERROR_MESSAGE); + expect(captured.body?.error?.details?.issues).toHaveLength(1); + }); +}); diff --git a/packages/rest/src/package-routes.ts b/packages/rest/src/package-routes.ts index cdfbc9fb34..2efd8593d3 100644 --- a/packages/rest/src/package-routes.ts +++ b/packages/rest/src/package-routes.ts @@ -10,7 +10,17 @@ import { OBJECT_SCHEMA_READ_ONLY_EXEMPT_CAPABILITIES } from '@objectstack/metada import type { PackageService } from '@objectstack/service-package'; // The declared envelope is written in ONE place for the whole platform (#3973), // and so (#8016) is the rule that reads an HTTP answer off a THROWN error. -import { sendOk, sendError, resolveThrownHttpError } from '@objectstack/types'; +// [#8086] `looksLikeInternalErrorLeak` / `INTERNAL_ERROR_MESSAGE` come from the +// same package for the same reason: "do not ship driver internals to clients" +// is a property of the HTTP boundary, not of one router, so every boundary +// applies ONE predicate in its own envelope (#3867). +import { + sendOk, + sendError, + resolveThrownHttpError, + looksLikeInternalErrorLeak, + INTERNAL_ERROR_MESSAGE, +} from '@objectstack/types'; import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js'; import { readSingleQueryValue, repeatedQueryParamMessage } from './query-multiplicity.js'; @@ -122,19 +132,64 @@ async function refusePackageRequest( * leaving nothing on the default arm would trade one wrong answer for another * and hide real faults. * - * ⚠️ The message is passed through as thrown. Unlike the dispatcher twin, this - * door applies no `looksLikeInternalErrorLeak` withholding to 5xx bodies — that - * gap predates this change and is unchanged by it (filed separately); nothing - * here newly exposes a message that was not already exposed, because the 500 - * arm shipped `(error as Error).message` verbatim before. + * ## [#8086] …and a leaky 5xx message is withheld + * + * The paragraph that stood here recorded the gap as still open: "this door + * applies no `looksLikeInternalErrorLeak` withholding to 5xx bodies — that gap + * predates this change and is unchanged by it (filed separately)". Filed as + * #8086, and closed here. + * + * It was reachable, not theoretical, and was reproduced through this door + * before being fixed — a real `ObjectQL` engine and a real + * `ObjectStackProtocolImplementation` whose driver fails the `sys_metadata` + * read the way a missing table does. `DELETE /api/v1/packages/:id` with no + * `?version=` routes to `protocol.deletePackage`, whose FIRST database touch + * (`engine.find('sys_metadata', { where })`) sits outside that method's + * per-item `try`, so the driver line propagates whole and arrived here: + * + * HTTP 500 + * {"success":false,"error":{"code":"INTERNAL_ERROR", + * "message":"SQLITE_ERROR: no such table: sys_metadata"}} + * + * This is NOT a new rule — it is the rule this surface already follows, at the + * door that was missed. The dispatcher twin (`HttpDispatcher.error`, + * `packages/runtime/src/http-dispatcher.ts`) has run exactly this expression + * since #3867, and `rest-server.ts` runs the same predicate at three call + * sites. #5437 / PR #5464 closed this class one seam over and never reached + * this registrar, because it does not go through `resolveErrorResponse` at all. + * Two doors serve `/api/v1/packages` and this one mounts FIRST in the + * production stack, so the unfiltered answer was the live one. + * + * Scoped to 5xx, deliberately: a 4xx message is a caller-facing answer by + * design — the protocol's `[tenant_scope_required]` refusal names the very + * parameter to pass, a `409 DESTRUCTIVE_CHANGE` names the remedy — and + * withholding those would delete the self-correcting sentence, at exactly the + * boundary where disclosure costs nothing because the caller supplied the + * input. Only the PROSE is withheld: `status`, `code` and `details` are + * untouched, so #8016's mapping still answers and a client can still branch. + * + * ⚠️ Ceiling, stated because a green suite must not read as full coverage: + * `looksLikeInternalErrorLeak` is a heuristic over the message and recognises + * no Postgres `relation "…" does not exist` phrasing, so that dialect's line + * still travels — through this door and through the twin alike, since both run + * the same predicate. Widening it HERE would be a new rule at one door and + * would re-create the divergence this closes. The cure is option C — the + * producer (`metadata-protocol`) not interpolating driver text into + * client-facing messages at all — which is a separate card. Pinned as a live + * case in `package-door-5xx-message-sanitization.test.ts` so it goes red the + * day either lands. */ function sendThrownError(res: any, error: unknown): void { const thrown = resolveThrownHttpError(error); + // The dispatcher twin's expression, byte for byte — one rule, two doors. + const message = thrown.status >= 500 && looksLikeInternalErrorLeak(thrown.message) + ? INTERNAL_ERROR_MESSAGE + : thrown.message; sendError( res, thrown.status, thrown.code, - thrown.message, + message, thrown.details ? { details: thrown.details } : undefined, ); } diff --git a/packages/runtime/src/package-door-error-parity.test.ts b/packages/runtime/src/package-door-error-parity.test.ts index 726097fbfc..3dadc76d26 100644 --- a/packages/runtime/src/package-door-error-parity.test.ts +++ b/packages/runtime/src/package-door-error-parity.test.ts @@ -47,13 +47,24 @@ * * ## What is deliberately NOT asserted: the message * - * The dispatcher withholds a 5xx message that looks like a driver/internal leak - * (`looksLikeInternalErrorLeak`, #3867); the REST package door applies no such - * filter and ships the thrown message verbatim. That asymmetry predates #8016 - * and is filed separately — it is a DISCLOSURE rule, not a mapping rule, so - * pinning `status` + `code` here is the whole of what "the two doors agree" - * means today. Asserting message parity would pin the gap shut instead of - * leaving it visible. + * [#8086] The asymmetry this paragraph used to record is CLOSED, so the reason + * has changed and the sentence that stood here would now be false. It read: + * "the REST package door applies no such filter and ships the thrown message + * verbatim. That asymmetry predates #8016 and is filed separately." It was + * filed as #8086 and fixed — `sendThrownError` + * (`packages/rest/src/package-routes.ts`) now runs the SAME + * `looksLikeInternalErrorLeak` / `INTERNAL_ERROR_MESSAGE` expression the + * dispatcher has run since #3867, so the two doors no longer disagree about + * disclosure either. + * + * Message parity is still not asserted HERE, for the ordinary reason rather + * than as a gap left visible: disclosure is a property each boundary applies in + * its own envelope, and each door pins its own half against the shared + * predicate — this file's job is the MAPPING rule (`status` + `code`), and + * `packages/rest/src/package-door-5xx-message-sanitization.test.ts` is where + * the REST door's disclosure behaviour is pinned. Widening this file to the + * message would make one suite the owner of two rules that are deliberately + * separate. */ import { describe, it, expect } from 'vitest';