fix(rest): the direct-mount package door withholds a leaky 5xx message (#8086) - #8130
Conversation
#8086) A driver failure under `/api/v1/packages` returned the driver's own line to the API client. Reproduced end to end before fixing — 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 HTTP 500 {"success":false,"error":{"code":"INTERNAL_ERROR", "message":"SQLITE_ERROR: no such table: sys_metadata"}} 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 follows, at the door that was missed. The dispatcher twin (`HttpDispatcher.error`) has run this expression since #3867 and `rest-server.ts` runs the same predicate at three call sites; #5437 / PR #5464 closed the class one seam over and could not reach this registrar, which does not go through `resolveErrorResponse` at all. Two doors serve `/api/v1/packages` and this one mounts first in production. Only the prose is withheld: `status`, `code` and `details` are untouched, so #8016's coded mapping still answers. 4xx is deliberately untouched — a refusal's message is caller-facing by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 3 release-owned page(s) also reference the affected code. These are read-only:
|
PM review —
|
Fixes #8086
Applies the 5xx leak rule in
sendThrownError(packages/rest/src/package-routes.ts) — option B, and only option B, per the PM ruling on the card.STEP 1 — the premise was Unverified, and it is now verified
The card said plainly: "No end-to-end reproduction: I did not boot a REST server and force a
sys_metadatawrite failure throughPOST /packages/publish." Grep proved the filter was absent, which is a different claim from the leak being reachable. Only the second justifies a fix, so it was measured first.Reproduced in process against unfixed code, with a real
ObjectQLengine, a realObjectStackProtocolImplementation, and a driver that fails everysys_metadataaccess the way a missing table does. Nothing hand-built: the message, its interpolated driver text and its status all come from shipping code, and the route is the one a client calls.DELETE /api/v1/packages/:idanswered, verbatim:The path, off the captured stack:
The producer is not the one the card named, and the real one is better. The card pointed at
Failed to persist customization overlay to sys_metadata: ...andFailed to delete customization overlay: .... The first of those no longer exists inprotocol.ts— it survives only as prose in a docblock, its branch having been deleted by #5264. The second exists but sits behinddeleteMetaItem. What actually carries a driver line through this door is one level up: a full uninstall (no?version=) routes toprotocol.deletePackage, whose first database touch,this.engine.find('sys_metadata', { where }), sits outside that method's per-itemtry— itscatchonly wraps thedeleteMetaItemloop below it. So the driver error propagates whole rather than being collected intofailed[]. The premise holds; the specific producer named in the card had moved.STEP 2 — the ruled fix
Same predicate and same replacement constant (
packages/types/src/error-leak.ts) as the two siblings on this surface:HttpDispatcher.error(#3867) andrest-server.ts's three call sites. Deliberately not a new rule — the rule this surface already follows, applied at the door #5437 / PR #5464 missed because this registrar does not go throughresolveErrorResponseat all.Scope honoured:
packages/types/src/response-envelope.tsuntouched — that is option A, escalated, not ruled.metadata-protocolproducers untouched — that is option C, a separate card.error.codechanged;detailsuntouched. Message only.STEP 3 — pins
New:
packages/rest/src/package-door-5xx-message-sanitization.test.ts, 50 cases. Every case asserts the positive sanitized shape (INTERNAL_ERROR_MESSAGE) plus the full ADR-0112 envelope (codeANDstatus) — never merely that the message "changed" or "no longer contains the table name", both of which pass for any rewrite including a worse one.if (status >= 500), which is a different rule that would delete every self-authored server-fault sentence.[tenant_scope_required]prose naming a foreign key, a409 DESTRUCTIVE_CHANGEnaming a unique constraint — so only the status keeps them intact. Plus the bound from both sides: 499 verbatim, 500 withheld.detailssurvive the withhold.STEP 4 — reverse verification, direction predicted before running
Predicted: the leak cases go red, and every pass-through and 4xx case stays green, because the predicate is what decides and neither trips it.
Observed with the two-line withhold removed — 23 failed | 27 passed, and the 23 are exactly
1 live + (4 sites x 5 leaks) + the 500 half of the bound case + the details case. All 12 pass-through and all 12 over-block cases stayed green.The raw driver text is visible in the failure output, which is the point:
Restored;
sha256sum -cconfirms both files byte-identical to before the removal.Known ceiling, stated so a green suite is not read as full coverage
looksLikeInternalErrorLeakis a heuristic over the message and recognises no Postgresrelation "..." does not existphrasing — measured false. That dialect's line therefore still travels, through this door and through the dispatcher twin alike, since both run the same predicate. The two doors still agree, which is what this card was about; what remains is a property of the shared heuristic.Widening it here would be a new rule at one door and would re-create the divergence this closes. The cure is option C. This is pinned as a live case that goes red the day either the predicate learns the phrasing or C lands — deliberately, so a reader comes back and re-reads it rather than consuming a green suite as proof the door is covered.
Also updated
packages/runtime/src/package-door-error-parity.test.ts— comment only. Its docblock asserted "the REST package door applies no such filter and ships the thrown message verbatim", which this change makes false. Corrected in place with the reason it still does not assert message parity (disclosure is each boundary's own rule; each door pins its own half).Verification
pnpm --filter @objectstack/rest testpnpm --filter @objectstack/rest typecheckpnpm check:type-check-debt@objectstack/restmeasured at its recorded 155, absent from the "can be lowered" list, so the new test file added zero tsc errors against a zero-margin ceiling (full closure built first, as the gate demands)pnpm check:route-envelope--self-test—package-routes.tsstays at its0/0/0ratchet; every body still goes through the sharedsendErrornode scripts/check-nul-bytes.mjscheck-empty-changeset/check-adr-0087-registration/check-changeset-no-major/check-error-code-casingeslint --no-inline-configon changed filesruntime/package-door-error-parity.test.tsChangeset included — user-visible response-message change.
Generated by Claude Code