fix(rest): a declared 5xx no longer ships its own message to the client (#5437)

.changeset/rest-5xx-message-withheld.md

@@ -0,0 +1,49 @@
+---
+"@objectstack/rest": patch
+---
+
+fix(rest): a declared 5xx no longer ships its own message to the client (#5437)
+
+**Behaviour change — read this if you operate a deployment or parse REST error
+bodies.** An error that carries an explicit `status` of 500 or above now reaches
+the client as `{ "error": "Internal server error", "code": "<the producer's
+code>" }`. The status and the code are unchanged; only the free-text message is
+withheld, and the full original text is written to the server log.
+
+**What was wrong.** `sendError` — the error path of the metadata, UI, discovery
+and batch routes — passed an explicit status straight through for the whole
+400-599 band, so a declared 5xx returned `error.message` verbatim without
+passing through any of the sanitizing heuristics (`isSqlLeak`,
+`looksLikeInternalErrorLeak`, the `Internal data error` envelope). The sibling
+branch in `mapDataError` stops at 4xx on purpose, with the reason written down:
+"5xx messages keep going through the sanitizing heuristics below so
+internal/SQL details never reach the client verbatim". Two opposite verdicts on
+one question, and the routes that report through `sendError` got the permissive
+one.
+
+That was reachable, not theoretical. `metadata-protocol` interpolates the raw
+driver error into two client-facing 500s — the customization-overlay persist and
+delete failures — so a real driver line such as `SQLITE_ERROR: no such table:
+sys_metadata`, `relation "sys_metadata" does not exist`, or a unique-constraint
+payload naming physical columns was returned to whoever made the request. The
+only thing standing in the way was a 500-character bound, and driver errors are
+far shorter than that. Length was never a proxy for leakage; on this side of the
+bound it failed open.
+
+**Accepted cost.** A 5xx message written *for* the caller now reaches them as
+the generic sentence plus its code. Two concrete examples: the overlay-persist
+failure's "In-memory registry was updated but will be lost on restart", and the
+atomic-batch refusal's "retry without options.atomic, or probe
+capabilities.transactionalBatch on /discovery first". Both remain fully readable
+in the server log, and the machine-readable `code` (`OVERLAY_PERSISTENCE_FAILED`,
+`NOT_IMPLEMENTED`) still rides on the response, so a client keying on codes is
+unaffected. If you were surfacing 5xx `error` text in an operator console, read
+it from the log instead — `[REST] Unhandled error` for a genuine fault, and a
+new `[REST] 5xx message withheld from client` line for the 502/503 lifecycle
+statuses that the unhandled-error predicate deliberately keeps quiet.
+
+The message is dropped unconditionally rather than filtered by keyword: a
+predicate would only move the question to "does the heuristic know this
+dialect", which is the failure mode that produced the bug. 4xx behaviour is
+untouched — an over-long client message is still truncated rather than erased
+(#5423 / #5436).

packages/rest/src/rest-4xx-message-truncation.test.ts

@@ -29,6 +29,7 @@
 // mangling messages that were always fine).
 
 import { describe, it, expect, vi } from 'vitest';
+import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types';
 import { mapDataError, RestServer } from './rest-server';
 
 /** The bound both branches use. Unchanged by #5423 — only what happens at it. */
@@ -288,13 +289,17 @@ describe('sendError: the same bound, walked through a real route (#5423)', () =>
         expect(res.body).toEqual({ error: msg, code: 'NO_DRAFT' });
     }, 60_000);
 
-    it('an over-long 5xx is DELIBERATELY still replaced — the asymmetry is the point', async () => {
-        // This branch's passthrough range is 400-599, wider than mapDataError's.
-        // A 4xx message is addressed to the caller and is the remedy; a 5xx
-        // message is a server fault's log diagnostic that happens to be
-        // reachable here, and `mapDataError`'s sibling branch is already
-        // "deliberately limited to 4xx ... so internal/SQL details never reach
-        // the client verbatim". #5423 does not widen 5xx leniency.
+    it('a 5xx is NOT truncated — it is withheld, whatever its length (#5437)', async () => {
+        // This case used to pin the 400-599 passthrough, where an over-long 5xx
+        // became the literal 'Request failed' while a SHORT one went out word
+        // for word. That asymmetry WAS the #5437 leak: length is not a proxy
+        // for "this text is safe to publish", and `metadata-protocol`
+        // interpolates raw driver errors into 500s far shorter than the bound.
+        //
+        // The 4xx/5xx split survives and is still this file's subject; what
+        // changed is the 5xx disposition — "withheld regardless of length"
+        // instead of "withheld only above 500 characters". Full coverage lives
+        // in `rest-5xx-message-sanitization.test.ts`.
         const rest = setup({
             getMetaItem: vi.fn().mockRejectedValue(
                 Object.assign(new Error('z'.repeat(600)), { code: 'INTERNAL', status: 503 }),
@@ -305,6 +310,11 @@ describe('sendError: the same bound, walked through a real route (#5423)', () =>
         });
 
         expect(res.statusCode).toBe(503);
-        expect(res.body.error).toBe('Request failed');
+        expect(res.body.error).toBe(INTERNAL_ERROR_MESSAGE);
+        // Withheld, not truncated: no prefix of the original survives at all.
+        expect(String(res.body.error)).not.toContain('z');
+        // The producer's own code still rides along — a SCREAMING_SNAKE
+        // constant is what the client keys on and is not a leak.
+        expect(res.body.code).toBe('INTERNAL');
     }, 60_000);
 });