fix(rest): the hand-built write routes answer a sandboxed crash and a declared-5xx refusal as the doors do (#22764)

.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.

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 '<name>' 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<string, unknown> } | 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<string, unknown> }): {
     status: number;

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');
         }
       },
     },

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.

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;
         };