diff --git a/.changeset/22764-rest-write-fault-withheld.md b/.changeset/22764-rest-write-fault-withheld.md new file mode 100644 index 00000000000..f776207c820 --- /dev/null +++ b/.changeset/22764-rest-write-fault-withheld.md @@ -0,0 +1,29 @@ +--- +'@objectstack/rest': patch +--- + +fix(rest): the hand-built REST write routes answer a sandboxed hook that crashed, or that declared a server error, as the data routes do (#22764) + +Clause-②: no + +**What was wrong.** Two kinds of sandboxed hook failure reach a route as faults, not refusals. One is a hook body that crashed (`ctx.input.title.trim()` on a number): the route receives the QuickJS debug wrapper on `.message`, for example `hook 'guard' threw: TypeError: boom`. The other is a hook that declared a server-band status (`status: 503`). The `/data` routes answer both as faults and withhold the text: a crash answers `500 INTERNAL_ERROR` with `Internal server error`, and a declared `503` answers `503` with its own code and `Internal server error`. The routes below build their error answer by hand, and they sent the wrapper and the native error text to the caller. The four external-datasource routes also answered a crash as `400`, the caller's mistake. + +**What changes on the wire, for those two failures only.** + +| routes | before | after | +| --- | --- | --- | +| `POST /sharing/rules`, `DELETE /sharing/rules/:idOrName`, `POST /sharing/rules/:idOrName/evaluate` | `500 RULE_*_FAILED`, flat `{ code, error }`, the wrapper | the `/data` answer, nested `{ success: false, error: { code, message } }` | +| `POST /security/suggested-bindings/:id/confirm` and `/dismiss` | `500 SUGGESTION_*_FAILED`, the wrapper | the `/data` answer, `{ error: { code, message } }` | +| the nine `POST /approvals/requests/:id/*` writes (`approve`, `reject`, `recall`, `revise`, `resubmit`, `reassign`, `remind`, `request-info`, `comment`) | `500 APPROVAL_*_FAILED`, flat `{ code, error }`, the wrapper | the `/data` answer, nested | +| `POST /packages/publish` | a crash: `500 INTERNAL_ERROR` with the wrapper; a declared `503`: `503` with the hook's sentence | the `/data` answer | +| `POST /datasources/:name/external/tables/:remote/draft` and `/import`, `/external/refresh-catalog`, `/external/validate` | `400 EXTERNAL_DATASOURCE_ERROR` / `400 EXTERNAL_IMPORT_ERROR`, the wrapper for a crash, the sentence for a declared `503` | the `/data` answer, nested | +| `POST /security/explain` | `500 EXPLAIN_FAILED`, the wrapper | the `/data` answer, nested | +| `POST /analytics/dataset/query`, a crash only (a declared 5xx already answered as `/data` does) | `500 ANALYTICS_QUERY_FAILED`, the wrapper | the `/data` answer, in this route's flat `{ code, error }` | + +"The `/data` answer" is `500 INTERNAL_ERROR` with `Internal server error` for a crash, whatever status it declared. For a refusal that declared a 5xx, it is that status, the declared code and `Internal server error`. The prose is released only when the hook marked the declaration a refusal, and a `userMessage` the hook set rides the answer. The withheld text goes to the server log. The answer is computed by the same classification the `/data` routes use, so it cannot drift from theirs. + +The same arms serve the read routes of the sharing-rule, suggested-binding and external-datasource families (`GET /sharing/rules`, `GET /sharing/rules/:idOrName`, `GET /security/suggested-bindings`, `GET /datasources/:name/external/tables`), so they answer these two failures the same way. + +**Unchanged.** A hook's refusal keeps its sentence and status on every route. A refusal that declared a 4xx on the external-datasource routes still answers `400` with that family's code. A fault that is not from a sandboxed hook keeps each route's own answer and code. A service-declared `503` on `POST /packages/publish` is one such fault. The per-record share routes already withheld the text at their own `500 SHARE_*_FAILED` and are unchanged. + +**Upgrading.** Nothing to change. A client that branched on `RULE_*_FAILED`, `APPROVAL_*_FAILED`, `SUGGESTION_*_FAILED`, `EXPLAIN_FAILED`, `ANALYTICS_QUERY_FAILED` or a `400` from an external-datasource route to detect a broken hook reads `INTERNAL_ERROR` (or the declared 5xx code) instead. Those codes still answer every other fault. diff --git a/packages/rest/src/error-response.ts b/packages/rest/src/error-response.ts index 0283b592d4e..c8cda356bdc 100644 --- a/packages/rest/src/error-response.ts +++ b/packages/rest/src/error-response.ts @@ -678,6 +678,61 @@ export function classifiedRefusalAnswer( return resolved.status < 500 ? resolved : undefined; } +/** + * [#22764] The `/data` door's answer for a SANDBOXED body it answers as a + * FAULT, or `undefined` for every other throw, which stays the catching + * route's own. + * + * ## Why it exists + * + * {@link classifiedRefusalAnswer} answers a sandbox's REFUSAL and declines + * every 5xx, so a hand-built family that asks it still owns the two sandbox + * shapes the doors answer as faults, and each family's own terminal relayed + * `.message` for them: + * + * - a body that CRASHED ({@link isSandboxCrash}, #7543). `.message` is the + * QuickJS wrapper around the runtime's native text, so the approvals, + * sharing-rule, suggested-binding, publish and external-datasource doors + * answered `hook '' threw: TypeError: …`, and the four external ones + * at `400`, a fault answered as the caller's mistake; + * - a refusal that DECLARED a server-band status. The doors keep the status + * and withhold the prose (#5437) unless the producer declared it a refusal + * (#16146); the families folded it into their own `500` with the wrapper, + * or relayed the sentence at the declared status. + * + * ## One answer, not a second fault terminal + * + * The answer IS {@link resolveErrorResponse}'s — the classification door + * `sendThrownError` and `handleRouteError` send for the same throw — so its + * status, `code`, withheld sentence and declared channels (`declaredCode`, + * `userMessage`) cannot drift from the doors'. A crash takes that function's + * crash terminal ahead of any declared status (#17273), exactly as there. + * The caller re-dresses it in its own envelope, as it does a classified + * refusal ({@link nestedRefusalEnvelope} for the nested families), and logs + * the original error first: the client no longer reads the words, so the + * operator must. + * + * ## What it deliberately leaves alone + * + * - A throw that is not a sandbox's. A producer-declared 5xx from a service + * keeps the catching route's terminal: the publish door's prose withhold is + * byte for byte its dispatcher twin's heuristic (#8086), and widening it + * at one door would mint the divergence that note closes. No measured + * producer behind these families declares one in any case. + * - A sandboxed refusal declaring a 4xx. That is the refusal half's, and the + * four external routes answer it at their own `400`, unchanged. + */ +export function sandboxFaultAnswer( + error: any, +): { status: number; body: Record } | undefined { + if (!isSandboxOrigin(error)) return undefined; + if (!isSandboxCrash(error)) { + const declared = declaredHttpStatus(error); + if (declared === undefined || declared < 500) return undefined; + } + return resolveErrorResponse(error); +} + /** * [#22719] {@link classifiedRefusalAnswer}'s answer, re-dressed for a door that * publishes the NESTED ADR-0112 D5 envelope (`{ error: { code, message } }`) @@ -713,6 +768,10 @@ export function classifiedRefusalAnswer( * decision, not a rename. `extra` is absent when there is nothing to carry, * so an emitter writes exactly `{ code, message }` for a refusal that declared * nothing. + * + * [#22764] The same families re-dress {@link sandboxFaultAnswer}'s answer + * through it too: that answer is phrased in the same flat classification, so + * the derivation above holds unchanged for a fault. */ export function nestedRefusalEnvelope(refusal: { status: number; body: Record }): { status: number; diff --git a/packages/rest/src/external-datasource-routes.ts b/packages/rest/src/external-datasource-routes.ts index 667bdc2638f..722a84d4add 100644 --- a/packages/rest/src/external-datasource-routes.ts +++ b/packages/rest/src/external-datasource-routes.ts @@ -18,7 +18,11 @@ import type { // The declared envelope is written in ONE place for the whole platform (#3973). // [#22719] …and so is the read of a sandboxed hook's refusal sentence. import { sendOk, sendError, sandboxBusinessMessage } from '@objectstack/types'; +// [#22764] …and so is the `/data` door's answer for a sandboxed body it +// answers as a fault. +import { nestedRefusalEnvelope, sandboxFaultAnswer } from './error-response.js'; import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js'; +import { logError } from './log.js'; /** * External Datasource Federation REST routes (ADR-0015 §6.2). @@ -396,23 +400,51 @@ export function registerExternalDatasourceRoutes( * `.message`. It answers `undefined` for anything that is not a sandboxed * refusal, so every other message here is byte-identical. * - * ⛔ Only the SENTENCE moves. Both arms keep their status and their code: - * `EXTERNAL_IMPORT_ERROR` is what the import route answers for a refusal it - * relays from the metadata door (a declared `409 DESTRUCTIVE_CHANGE` - * included, which the import dogfood pins at `400`), so this family's status - * is its own decision and the shared classification is not asked here. + * ⛔ For a refusal only the SENTENCE moves. Both arms keep their status and + * their code: `EXTERNAL_IMPORT_ERROR` is what the import route answers for a + * refusal it relays from the metadata door (a declared `409 + * DESTRUCTIVE_CHANGE` included, which the import dogfood pins at `400`), so + * this family's refusal status is its own decision and the shared refusal + * classification is not asked here. A FAULT is the other question, answered + * by {@link answerThrown} below. */ const refusalSentence = (err: unknown): string => sandboxBusinessMessage(err) ?? (err instanceof Error ? err.message : String(err)); + /** + * [#22764] Both catch arms' answer. A sandboxed body the doors answer as a + * FAULT — one that CRASHED, or a refusal that DECLARED a 5xx — answers as the + * doors do ({@link sandboxFaultAnswer}), logged first because the caller no + * longer reads its words. Before, both reached this family's `400`: a crash + * as the caller's mistake, with the QuickJS wrapper around the native error + * text, and a declared `503` as a client error carrying its sentence. + * + * ⛔ Every other throw keeps this family's own `400` and code, a declared 4xx + * included: a sandboxed refusal declaring one, and the metadata door's + * `409 DESTRUCTIVE_CHANGE` the import dogfood pins at `400`. That status is + * a separate decision this does not take. + */ + const answerThrown = ( + res: any, + err: unknown, + code: 'EXTERNAL_DATASOURCE_ERROR' | 'EXTERNAL_IMPORT_ERROR', + ) => { + const fault = sandboxFaultAnswer(err); + if (fault) { + logError('[REST] external-datasource error:', err); + const nested = nestedRefusalEnvelope(fault); + return sendError(res, nested.status, nested.code, nested.message, nested.extra); + } + return sendError(res, 400, code, refusalSentence(err)); + }; + /** * A refusal from the external-datasource service — same code as the * admin-routes path (#4249). Named after the service, not one operation: * since #4264 every route here except the import (which has its own * registered code) answers its catch with this. */ - const refused = (res: any, err: unknown) => - sendError(res, 400, 'EXTERNAL_DATASOURCE_ERROR', refusalSentence(err)); + const refused = (res: any, err: unknown) => answerThrown(res, err, 'EXTERNAL_DATASOURCE_ERROR'); /** * ONE declaration of this registrar's surface (#5822): the array below is @@ -489,7 +521,7 @@ export function registerExternalDatasourceRoutes( ); sendOk(res, { object: result }, 201); } catch (err) { - sendError(res, 400, 'EXTERNAL_IMPORT_ERROR', refusalSentence(err)); + answerThrown(res, err, 'EXTERNAL_IMPORT_ERROR'); } }, }, diff --git a/packages/rest/src/package-routes.ts b/packages/rest/src/package-routes.ts index 1b067d9676f..409d574fefb 100644 --- a/packages/rest/src/package-routes.ts +++ b/packages/rest/src/package-routes.ts @@ -17,8 +17,9 @@ import { sandboxBusinessMessage, INTERNAL_ERROR_MESSAGE, } from '@objectstack/types'; -import { classifiedRefusalAnswer } from './error-response.js'; +import { classifiedRefusalAnswer, nestedRefusalEnvelope, sandboxFaultAnswer } from './error-response.js'; import { mountDirectRoutes, type DirectMountedRoute } from './direct-mount.js'; +import { logError } from './log.js'; /** * [#7033 / #7023] The authorization gate for the REST package transport. @@ -202,8 +203,27 @@ async function refusePackageRequest( * mapping, which is why the classification's body is not re-dressed here the * way the hand-built families in `rest-server.ts` re-dress it: their 500 arm * read nothing, this door already reads everything but the sandbox. + * + * ## [#22764] …and a sandboxed body the doors answer as a fault reads as one + * + * The two reads above leave the sandbox's FAULT shapes to this door's own + * mapping, and both shipped words the doors withhold: a body that CRASHED + * answered `500 INTERNAL_ERROR` with the QuickJS wrapper around the native + * error text (the business read declines a crash, so the raw message stood), + * and a refusal that DECLARED `503` answered at its status with its sentence. + * {@link sandboxFaultAnswer} is the doors' answer for exactly those two, so it + * is asked first and re-dressed whole, the way the nested families re-dress + * it. The original error is logged first, the one place its words now reach. + * Every other throw is answered below exactly as before. */ function sendThrownError(res: any, error: unknown): void { + const fault = sandboxFaultAnswer(error); + if (fault) { + logError('[REST] package publish error:', error); + const nested = nestedRefusalEnvelope(fault); + sendError(res, nested.status, nested.code, nested.message, nested.extra); + return; + } const thrown = resolveThrownHttpError(error, classifiedRefusalAnswer(error)?.status ?? 500); // The dispatcher twin's withhold, byte for byte — one rule, two doors. Its // other limb is the shared sandbox read (#22719), not the raw message. diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 0d3fa6c9bfd..80f0e594db9 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -414,6 +414,7 @@ import { sandboxBusinessMessage, classifiedRefusalAnswer, nestedRefusalEnvelope, + sandboxFaultAnswer, boundedDeclaredUserMessage, boundedDeclaredRefusalMessage, declaredHttpStatus, @@ -11242,7 +11243,17 @@ export class RestServer { // now carry is the one they declare and the one the sibling // face has always shipped. `ANALYTICS_QUERY_FAILED` remains // this route's answer for an UNDECLARED fault, below. - const declaredFault = declaredServerFaultAnswer(error); + // + // [#22764] A sandboxed body the doors answer as a FAULT is + // asked first, for the order the doors ask it in: a body + // that CRASHED is a fault whatever status it declared + // (#17273), so it must not reach the declared-5xx relay, and + // it never reaches ③b, whose `clientMsg` is the QuickJS + // wrapper around the native error text for a crash (the + // business read declines one). A sandboxed refusal that + // declared a 5xx answers here too, the same bytes ③a gives + // it: one classification, `/data`'s. + const declaredFault = sandboxFaultAnswer(error) ?? declaredServerFaultAnswer(error); if (declaredFault) { return res.status(declaredFault.status).json({ ...declaredFault.body, ...markExtra }); } @@ -11498,6 +11509,15 @@ export class RestServer { return respondError(res, refusal.status, code, String(refusal.body.error ?? '')); } logError('[REST] Security explain error:', error); + // [#22764] A sandboxed body the doors answer as a fault (a + // crash, or a refusal declaring a 5xx) answers as the doors do, + // instead of the QuickJS wrapper relayed below. Status, code + // and message only, the refusal arm's scope above. + const fault = sandboxFaultAnswer(error); + if (fault) { + const nested = nestedRefusalEnvelope(fault); + return respondError(res, nested.status, nested.code, nested.message); + } // The 500 arm keeps its 500-char cap: an unexpected fault's // message is not a contract, and truncating it stays a // sanitization step — only the position of the words moves. @@ -12117,6 +12137,16 @@ export class RestServer { return sendEnvelopeError(res, nested.status, nested.code, nested.message, nested.extra); } logError(`[REST] sharing-rule ${defaultCode}:`, err); + // [#22764] …and a sandboxed body the doors answer as a FAULT — one + // that crashed, or a refusal that declared a 5xx — answers as the + // doors do, logged above and withheld from the caller. The 500 + // below relayed the QuickJS wrapper around the native error text. + // Nested, for the classified arm's reason. + const fault = sandboxFaultAnswer(err); + if (fault) { + const nested = nestedRefusalEnvelope(fault); + return sendEnvelopeError(res, nested.status, nested.code, nested.message, nested.extra); + } return res.status(500).json({ code: defaultCode, error: msg.slice(0, 500) }); }; @@ -12324,6 +12354,14 @@ export class RestServer { return respondError(res, nested.status, nested.code, nested.message, nested.extra); } logError(`[REST] suggested-bindings ${defaultCode}:`, err); + // [#22764] A sandboxed body the doors answer as a fault (a crash, or + // a refusal declaring a 5xx) answers as the doors do, through this + // family's one emitter, instead of the wrapper relayed below. + const fault = sandboxFaultAnswer(err); + if (fault) { + const nested = nestedRefusalEnvelope(fault); + return respondError(res, nested.status, nested.code, nested.message, nested.extra); + } // The 500 arm keeps its 500-char cap: an unexpected fault's message // is not a contract, and truncating it stays a sanitization step. return respondError(res, 500, defaultCode, String(err?.message ?? err).slice(0, 500)); @@ -12535,6 +12573,19 @@ export class RestServer { sendEnvelopeError(res, nested.status, nested.code, nested.message, nested.extra); return true; } + // [#22764] A sandboxed body the doors answer as a FAULT — one that + // crashed, or a refusal that declared a 5xx — answers as the doors + // do rather than reaching the caller's outer `500 + // APPROVAL_*_FAILED`, which relayed the QuickJS wrapper around the + // native error text. Logged here because answering it skips the + // outer arm's log line, and the operator is now the only reader. + const fault = sandboxFaultAnswer(err); + if (fault) { + logError('[REST] approval error:', err); + const nested = nestedRefusalEnvelope(fault); + sendEnvelopeError(res, nested.status, nested.code, nested.message, nested.extra); + return true; + } return false; }; diff --git a/packages/rest/src/rest-write-route-hook-refusal-sentence.ledger.test.ts b/packages/rest/src/rest-write-route-hook-refusal-sentence.ledger.test.ts index b1576462298..ee024979aca 100644 --- a/packages/rest/src/rest-write-route-hook-refusal-sentence.ledger.test.ts +++ b/packages/rest/src/rest-write-route-hook-refusal-sentence.ledger.test.ts @@ -40,6 +40,14 @@ * (`.message` the wrapper, `.innerMessage` the sentence), reproduced here as * `rest-hook-refusal-message-parity.test.ts` does, because this package does * not depend on `@objectstack/runtime`. + * + * ## [#22764] The fault side, over the same population + * + * §6 drives every row §2 drives with the two sandbox shapes the doors answer + * as FAULTS — a body that crashed, and a refusal that declared a 5xx — and + * holds each to the doors' answer: their status and code, their withheld + * sentence, and no trace of the native text or the wrapper. §2 stays the + * control: a refusal keeps its sentence on the same rows. */ import { describe, it, expect } from 'vitest'; @@ -464,8 +472,10 @@ describe('[#22718] §4 a sandboxed body the doors answer as a fault reads as tha /** * The fault terminal each repaired row keeps: the code its own arm answers a - * throw that is NOT a refusal. External datasources answer every throw at - * `400` with their own code and are listed with that answer. + * throw that is NOT a refusal. External datasources answer such a throw at + * `400` with their own code and are listed with that answer. [#22764] A + * sandboxed body the doors answer as a fault is not such a throw: §6 holds + * every row to the doors' answer for it. */ const OWN_TERMINAL: Record = { 'POST /api/v1/sharing/rules': { status: 500, code: 'RULE_DEFINE_FAILED' }, @@ -549,3 +559,72 @@ describe('[#22719] §5 the repaired rows: a fault keeps its own terminal, a refu }, 60_000); } }); + +// ─── [#22764] the fault side: every driven row answers as the doors do ────── + +/** + * The two sandbox shapes the doors answer as FAULTS, the status and code they + * answer each with, and the text that must not reach the caller: the native + * error a crash carries, and the prose a declared 5xx withholds. + */ +const SANDBOX_FAULTS = [ + ['a body that CRASHED', sandboxCrash, { status: 500, code: 'INTERNAL_ERROR' }, /TypeError|boom/], + ['a refusal that declared a 5xx', sandboxDeclared5xx, { status: 503, code: 'SERVICE_UNAVAILABLE' }, /ledger/], +] as const; + +/** + * Driven rows that answer a sandboxed fault at their OWN terminal rather than + * the doors' status and code, with the reason. Closed. The words are withheld + * on both all the same, and §6 still holds them to the doors' sentence. + */ +const OWN_FAULT_TERMINAL: ReadonlyMap = new Map([ + ['POST /api/v1/data/:object/:id/shares', { + status: 500, code: 'SHARE_GRANT_FAILED', + reason: "the record-share family withholds a sandbox's words at its own 500 (`sharingFaultMessage`) and " + + 'folds a declared 5xx into that terminal by a recorded choice (#11683, #12693); neither is a ' + + 'leak, and its status and code are a decision this card does not take', + }], + ['DELETE /api/v1/data/:object/:id/shares/:shareId', { + status: 500, code: 'SHARE_REVOKE_FAILED', + reason: 'the record-share family, as its grant row', + }], +]); + +describe('[#22764] §6 every driven write row answers a sandboxed fault as the doors do, its words withheld', () => { + it('the reference: the `/data` door answers each shape at the pinned status and code', async () => { + for (const [label, error, doors, leak] of SANDBOX_FAULTS) { + const res = await drive(boot(error), 'POST /api/v1/data/:object', { name: 'x' }); + expect(res.statusCode, label).toBe(doors.status); + expect(codeOf(res.body), label).toBe(doors.code); + expect(JSON.stringify(res.body), label).not.toMatch(leak); + } + }, 60_000); + + it('every row naming its own terminal is a driven row', () => { + for (const route of OWN_FAULT_TERMINAL.keys()) expect(Object.keys(ANSWERS_THE_SENTENCE), route).toContain(route); + }); + + for (const [route, driver] of Object.entries(ANSWERS_THE_SENTENCE)) { + for (const [label, error, doors, leak] of SANDBOX_FAULTS) { + it(`${route}: ${label}`, async () => { + const door = await drive(boot(error), 'POST /api/v1/data/:object', { name: 'x' }); + const b = boot(error, driver.protocol?.(error)); + const res = await drive(b, route, driver.body ?? {}); + // Read first: the async job's worker writes after the `201`. + const answer = driver.answer ? await driver.answer(res, b) : sentenceOf(res.body); + + expect(b.calls, `${driver.called} was never called`).toContain(driver.called); + expect(answer).toBe(sentenceOf(door.body)); + for (const text of [JSON.stringify(res.body ?? null), String(answer)]) { + expect(text).not.toMatch(leak); + expect(text).not.toMatch(WRAPPER_RE); + } + // The import rows answer per row, inside a `200` / `201`. + if (driver.answer) return; + const expected = OWN_FAULT_TERMINAL.get(route) ?? doors; + expect(res.statusCode).toBe(expected.status); + expect(codeOf(res.body)).toBe(expected.code); + }, 60_000); + } + } +});