From c34aa3e2a57307e915240d3ae25c539cbe3567af Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 13:03:30 +0000 Subject: [PATCH] fix(rest): a declared 5xx no longer ships its own message to the client (#5437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveErrorResponse` — the value side of `sendError`, and therefore the error path of every metadata / UI / discovery / batch route — passed an explicit status straight through for the whole 400-599 band. A declared 5xx returned `error.message` verbatim, past `isSqlLeak`, past `looksLikeInternalErrorLeak`, past the `Internal data error` envelope, while `mapDataError`'s sibling branch stops at 4xx on purpose and says why: "5xx messages keep going through the sanitizing heuristics below so internal/SQL details never reach the client verbatim". Two opposite verdicts on one question. `metadata-protocol` interpolates the raw driver error into two client-facing 500s (overlay persist / delete), and a real driver line is far shorter than the 500-character bound that was the only thing standing here, so the whole thing arrived intact. Length was never a proxy for leakage; on this side of the bound it failed open. The 5xx band now drops the message unconditionally and keeps the producer's status and `code`. Unconditional rather than heuristic: a keyword gate only moves the question to "does the predicate know this dialect". Sanitised in the branch rather than by falling through to `mapDataError`, which derives status from message TEXT — measured first, and it answers 404 OBJECT_NOT_FOUND for the overlay 500s, 404 "Object '' is not registered" for the atomic batch's 501, and 400 with the driver text still verbatim for anything its keywords miss. The withheld text still reaches the log: `handleRouteError` already prints a genuine fault, and a new line covers the 502/503 gap its predicate leaves. 4xx truncation (#5423 / #5436) is untouched. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- .changeset/rest-5xx-message-withheld.md | 49 ++ .../src/rest-4xx-message-truncation.test.ts | 26 +- .../src/rest-5xx-message-sanitization.test.ts | 481 ++++++++++++++++++ packages/rest/src/rest-server.ts | 131 ++++- 4 files changed, 659 insertions(+), 28 deletions(-) create mode 100644 .changeset/rest-5xx-message-withheld.md create mode 100644 packages/rest/src/rest-5xx-message-sanitization.test.ts diff --git a/.changeset/rest-5xx-message-withheld.md b/.changeset/rest-5xx-message-withheld.md new file mode 100644 index 0000000000..2183d60f5a --- /dev/null +++ b/.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 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). diff --git a/packages/rest/src/rest-4xx-message-truncation.test.ts b/packages/rest/src/rest-4xx-message-truncation.test.ts index f0fa2568f8..84cf40caa2 100644 --- a/packages/rest/src/rest-4xx-message-truncation.test.ts +++ b/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); }); diff --git a/packages/rest/src/rest-5xx-message-sanitization.test.ts b/packages/rest/src/rest-5xx-message-sanitization.test.ts new file mode 100644 index 0000000000..ed153ef655 --- /dev/null +++ b/packages/rest/src/rest-5xx-message-sanitization.test.ts @@ -0,0 +1,481 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5437] A declared 5xx never ships its own message to the client. +// +// `resolveErrorResponse` — the value side of `sendError`, and therefore the +// error path of every metadata / UI / discovery / batch route — passed an +// explicit status straight through for the whole 400-599 band. `mapDataError`'s +// sibling branch stops at 4xx *on purpose*: "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 — a declared 500 shorter +// than 500 characters was returned word for word, past `isSqlLeak`, past +// `looksLikeInternalErrorLeak`, past `Internal data error`. +// +// The producer that makes this live rather than theoretical is +// `metadata-protocol`, which interpolates the raw driver error into two +// client-facing 500s (`Failed to persist customization overlay to +// sys_metadata: ${dbError.message}`, `Failed to delete customization overlay: +// ${err.message}`). A real driver line — `SQLITE_ERROR: no such table: +// sys_metadata`, `relation "sys_metadata" does not exist`, a unique-constraint +// payload naming columns — is nowhere near 500 characters, so the length bound +// never touched it. Length was never a proxy for leakage; on this side of the +// bound it failed OPEN. +// +// --------------------------------------------------------------------------- +// Reverse verification, direction predicted BEFORE running +// --------------------------------------------------------------------------- +// Restoring the old bound (`error.status < 600` with no 5xx branch) turns every +// "withheld" case here RED — they assert on a body that only exists once the +// message is dropped — and leaves every 4xx case GREEN, because #5436's +// truncation is untouched by this change. That is the ordinary direction, and +// it was confirmed by running it (see the PR). +// +// --------------------------------------------------------------------------- +// Why the fix does NOT delegate to `mapDataError` +// --------------------------------------------------------------------------- +// The obvious shape — drop 5xx out of the passthrough and let it fall through +// to `mapDataError`'s heuristics — was measured first and rejected, because +// `mapDataError` derives its status from the message TEXT: +// +// 500 `...sys_metadata: SQLITE_ERROR: no such table: sys_metadata` +// -> 404 OBJECT_NOT_FOUND ("no such table" trips unknown-object) +// 501 `Atomic batch on 'showcase_account' requires engine transaction ...` +// -> 404 Object 'showcase_account' is not registered +// 500 `Failed to delete customization overlay: connect ECONNREFUSED ...` +// -> 400 with the driver text STILL verbatim (terminal fallback) +// +// So it re-labels a server fault as a client mistake, re-labels a capability +// refusal as a missing object, and — for any 5xx whose wording matches no +// keyword — leaks exactly what it was supposed to stop, now wearing a 4xx. Two +// of those statuses are also `isExpectedDataStatus`, so the fault would stop +// being logged at all. The cure is therefore applied in the branch itself: +// keep the status the producer declared, keep the machine-readable `code`, drop +// the prose. The status-preservation assertions below are what pin that. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { RestServer } from './rest-server'; + +const META_ITEM = '/api/v1/meta/:type/:name'; + +/** 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'; + +// --------------------------------------------------------------------------- +// Harness — the shape #5436 introduced, reused verbatim +// --------------------------------------------------------------------------- + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeRes() { + const res: any = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: any) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); res.send = vi.fn(); + return res; +} + +function mountRest(protocol: any) { + const rest = new RestServer( + createMockServer() as any, + protocol, + { api: { requireAuth: false, enableBatch: true } } as any, + ); + (rest as any).resolveExecCtx = async () => ({ userId: 'u1' }); + rest.registerRoutes(); + return rest; +} + +function setup(protocolOverrides: Record = {}) { + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', endpoints: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([]), + getMetaItem: vi.fn().mockResolvedValue({}), + saveMetaItem: vi.fn().mockResolvedValue({}), + deleteMetaItem: vi.fn().mockResolvedValue({}), + findData: vi.fn().mockResolvedValue([]), + batchData: vi.fn().mockResolvedValue({}), + ...protocolOverrides, + }; + return mountRest(protocol); +} + +async function callRoute(rest: any, method: string, path: string, req: Record = {}) { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} route not registered`); + const res = makeRes(); + await route.handler({ method, params: {}, query: {}, body: {}, headers: {}, ...req }, res); + return res; +} + +/** Every `console.error` argument list this test produced. */ +let logged: unknown[][] = []; +let spy: ReturnType; + +beforeEach(() => { + logged = []; + spy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { logged.push(args); }); +}); +afterEach(() => { spy.mockRestore(); }); + +/** Did ANY log line carry the given text (in its message or its arguments)? */ +function loggedText(needle: string): boolean { + return logged.some((args) => args.some((a) => { + if (a instanceof Error) return a.message.includes(needle); + return typeof a === 'string' && a.includes(needle); + })); +} + +// --------------------------------------------------------------------------- +// 1. The real producer, end to end — the issue's "unverified" half +// --------------------------------------------------------------------------- +// +// The issue was raised from a code read and said so: "没有起 REST server 打真实 +// 请求造一次 `sys_metadata` 写失败". This section does that in process — a REAL +// `ObjectQL` engine, a REAL `ObjectStackProtocolImplementation`, and a driver +// that fails every `sys_metadata` access the way a missing table does. Nothing +// is hand-built: the message, its interpolated driver text and its status all +// come from the shipping protocol code, and the route is the one a client calls. +// +// The producer walked here is `deleteMetaItem`'s catch — `Failed to delete +// customization overlay: ${err.message}`, `(e as any).status = 500`. It is +// deliberately the one sampled live, because its status is ASSIGNED to an +// already-constructed error rather than written as a `status:` literal, and the +// issue flagged exactly that shape as un-enumerated ("动态赋值的路径没清点"). A +// `status:` grep does not find it; this test does. +// +// Its sibling `saveMetaItem` producer (`OVERLAY_PERSISTENCE_FAILED`) is covered +// by message shape in the next section rather than live: reaching its legacy +// raw-engine branch requires a metadata type that is neither overlay-allowed +// nor runtime-creatable AND an artifact-backed item of that type, and a +// runtime-created one is refused earlier with `403 NOT_CREATABLE`. Both +// producers hand `resolveErrorResponse` the same thing — a declared 5xx whose +// message embeds `err.message` — so the live half proves the boundary and the +// shaped half proves the wording. + +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) { + const engine = new ObjectQL(); + engine.registerDriver(failingDriver(dbError), true); + await engine.init(); + const protocol = new ObjectStackProtocolImplementation(engine as any); + return mountRest(protocol as any); +} + +describe('[#5437] a real sys_metadata failure, walked in process', () => { + it('the driver line does not appear anywhere in the client body', async () => { + const rest = await bootRealProtocol(SQLITE_NO_TABLE); + + const res = await callRoute(rest, 'DELETE', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + const wire = JSON.stringify(res.body); + expect(wire).not.toContain('no such table'); + expect(wire).not.toContain('SQLITE_ERROR'); + expect(wire).not.toContain('sys_metadata'); + // Still a server fault on the wire — NOT laundered into a 4xx by a + // heuristic reading of the message. `mapDataError` would have answered + // 404 OBJECT_NOT_FOUND for this exact text. + expect(res.statusCode).toBe(500); + }, 60_000); + + it('the withheld text still reaches the server log, in full', async () => { + // The premise guard and the log guarantee in one assertion: the only + // way this text can reach the log is if the protocol really did + // interpolate the driver error into the message it threw. If the + // producer ever stops doing that, this goes red and the case above + // stops proving anything. + const rest = await bootRealProtocol(SQLITE_NO_TABLE); + + await callRoute(rest, 'DELETE', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + expect(loggedText('no such table')).toBe(true); + expect(loggedText('Failed to delete customization overlay')).toBe(true); + }, 60_000); + + it('Postgres phrasing is withheld too — this is not a SQLite-shaped guard', async () => { + const rest = await bootRealProtocol(PG_NO_RELATION); + + const res = await callRoute(rest, 'DELETE', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + const wire = JSON.stringify(res.body); + expect(wire).not.toContain('does not exist'); + expect(wire).not.toContain('relation'); + expect(loggedText('does not exist')).toBe(true); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// 2. The envelope a declared 5xx now produces +// --------------------------------------------------------------------------- +// +// Built the way `metadata-protocol` builds it at the two producer sites, so the +// assertions read against the exact shape that ships. + +/** `protocol.ts` — `saveMetaItem`'s persistence catch. Status set at build. */ +function overlayPersistenceError(dbError: string) { + const err = new Error( + `Failed to persist customization overlay to sys_metadata: ${dbError}. ` + + `In-memory registry was updated but will be lost on restart.`, + ); + (err as any).code = 'OVERLAY_PERSISTENCE_FAILED'; + (err as any).status = 500; + return err; +} + +/** + * `protocol.ts` — `deleteMetaItem`'s catch. The status is assigned to an + * already-constructed error rather than passed to a constructor or an object + * literal, which is the producer shape the issue flagged as un-enumerated + * ("动态赋值的路径没清点"): a `status:` literal grep does not find it. Same + * band, same boundary, so it is sampled here rather than assumed. + */ +function deleteOverlayError(dbError: string) { + const e = new Error(`Failed to delete customization overlay: ${dbError}`); + (e as any).status = 500; + return e; +} + +describe('[#5437] the 5xx envelope: status and code survive, the prose does not', () => { + it('OVERLAY_PERSISTENCE_FAILED keeps its 500 and its code, loses its text', async () => { + const rest = setup({ + saveMetaItem: vi.fn().mockRejectedValue(overlayPersistenceError(SQLITE_NO_TABLE)), + }); + + const res = await callRoute(rest, 'PUT', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + body: { name: 'showcase_account' }, + }); + + // The producer's own declaration is honoured — this is what routing the + // error through `mapDataError` would have destroyed (it answers 404 + // OBJECT_NOT_FOUND, because "no such table" trips its unknown-object + // heuristic). + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('OVERLAY_PERSISTENCE_FAILED'); + // A machine-readable code is not a leak; the prose is. + expect(res.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(res.body).toEqual({ error: INTERNAL_ERROR_MESSAGE, code: 'OVERLAY_PERSISTENCE_FAILED' }); + }, 60_000); + + it('a dynamically-assigned status is treated identically (no code declared)', async () => { + const rest = setup({ + deleteMetaItem: vi.fn().mockRejectedValue(deleteOverlayError(PG_NO_RELATION)), + }); + + const res = await callRoute(rest, 'DELETE', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + expect(res.statusCode).toBe(500); + expect(res.body.error).toBe(INTERNAL_ERROR_MESSAGE); + // No `code` was declared, so none is invented here — ADR-0112 says the + // PRODUCER names the condition. + expect(res.body.code).toBeUndefined(); + expect(loggedText('does not exist')).toBe(true); + }, 60_000); + + it('a SHORT 5xx is withheld too — length was never the criterion', async () => { + // The whole defect in one case: 46 characters, well under the bound + // that used to be the only thing standing here, and every word of it a + // driver internal. + const short = overlayPersistenceError('UNIQUE constraint failed: sys_metadata.name'); + expect(short.message.length).toBeLessThan(500); + + const rest = setup({ saveMetaItem: vi.fn().mockRejectedValue(short) }); + const res = await callRoute(rest, 'PUT', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + body: { name: 'showcase_account' }, + }); + + expect(res.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(String(res.body.error)).not.toContain('sys_metadata'); + }, 60_000); + + it('a 5xx message that trips NO leak keyword is withheld all the same', async () => { + // The case a keyword gate cannot catch and the reason this is not one: + // `connect ECONNREFUSED :` names infrastructure without + // saying a single word `looksLikeInternalErrorLeak` looks for. + const rest = setup({ + deleteMetaItem: vi.fn().mockRejectedValue( + deleteOverlayError('connect ECONNREFUSED 10.0.0.5:5432'), + ), + }); + + const res = await callRoute(rest, 'DELETE', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + expect(res.statusCode).toBe(500); + expect(String(res.body.error)).not.toContain('10.0.0.5'); + expect(String(res.body.error)).not.toContain('ECONNREFUSED'); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// 3. The rest of the 5xx family +// --------------------------------------------------------------------------- + +describe('[#5437] the whole 5xx band, not just 500', () => { + // `metadata-protocol`'s atomic-batch refusal — the third live producer in + // this band, and the one that shows why the status must be preserved: its + // message names the object, so `mapDataError` answers `404 Object '' + // is not registered` for an object that exists perfectly well and a runtime + // that simply cannot open a transaction. + const NOT_IMPLEMENTED = Object.assign( + new Error( + `Atomic batch on 'showcase_account' requires engine transaction support; this runtime cannot roll back. ` + + `Retry without options.atomic, or probe capabilities.transactionalBatch on /discovery first.`, + ), + { status: 501, code: 'NOT_IMPLEMENTED' }, + ); + + it('a 501 stays a 501 with its code — it does not become a 404 or a 400', async () => { + const rest = setup({ getMetaItem: vi.fn().mockRejectedValue(NOT_IMPLEMENTED) }); + const res = await callRoute(rest, 'GET', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + expect(res.statusCode).toBe(501); + expect(res.body.code).toBe('NOT_IMPLEMENTED'); + expect(res.body.error).toBe(INTERNAL_ERROR_MESSAGE); + // Accepted cost, pinned so it is a decision and not a surprise: this + // sentence is self-authored and carries a genuine remedy. It reaches + // the operator's log, and the client keys on `501 NOT_IMPLEMENTED`. + expect(String(res.body.error)).not.toContain('options.atomic'); + }, 60_000); + + it('502 and 503 are withheld and, being "expected" statuses, still get a log line', async () => { + for (const status of [502, 503]) { + logged = []; + const err = Object.assign( + new Error(`upstream metadata store returned garbage at 10.0.0.5:5432`), + { status, code: 'UPSTREAM_FAILED' }, + ); + const rest = setup({ getMetaItem: vi.fn().mockRejectedValue(err) }); + const res = await callRoute(rest, 'GET', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + expect(res.statusCode, `status ${status}`).toBe(status); + expect(res.body.error, `status ${status}`).toBe(INTERNAL_ERROR_MESSAGE); + // `isExpectedDataStatus` treats 502/503 as lifecycle outcomes and + // keeps "[REST] Unhandled error" quiet for them. Before #5437 the + // operator could still read the cause — off the client's response. + // Now that the body is generic, the log line is the ONLY copy. + expect(loggedText('10.0.0.5'), `status ${status}`).toBe(true); + } + }, 60_000); + + it('a genuine 500 fault logs exactly ONE line, not two', async () => { + // `handleRouteError` already prints the whole error object for a fault + // this size; the withheld-message line must not double it. + const rest = setup({ + saveMetaItem: vi.fn().mockRejectedValue(overlayPersistenceError(SQLITE_NO_TABLE)), + }); + await callRoute(rest, 'PUT', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + body: { name: 'showcase_account' }, + }); + + expect(logged).toHaveLength(1); + expect(loggedText('no such table')).toBe(true); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// 4. #5436 must not regress +// --------------------------------------------------------------------------- +// +// This change rewrote the expression that #5436 landed, so the 4xx half is +// re-pinned here at the same boundary rather than trusted. Full coverage of the +// truncation itself stays in `rest-4xx-message-truncation.test.ts`. + +describe('[#5437] the 4xx half is untouched (#5436 non-regression)', () => { + it('a short 4xx is still byte-for-byte verbatim', async () => { + const msg = '[no_draft] No pending draft exists for object/showcase_account.'; + const rest = setup({ + getMetaItem: vi.fn().mockRejectedValue( + Object.assign(new Error(msg), { code: 'NO_DRAFT', status: 404 }), + ), + }); + const res = await callRoute(rest, 'GET', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + expect(res.statusCode).toBe(404); + expect(res.body).toEqual({ error: msg, code: 'NO_DRAFT' }); + }, 60_000); + + it('a long 4xx is still TRUNCATED, not erased, and keeps its issues', async () => { + const head = 'The main clause a caller must read is right here at the front. '; + const err = Object.assign( + new Error(head + 'x'.repeat(600 - head.length)), + { code: 'INVALID_METADATA', status: 422, issues: [{ path: 'label', message: 'Required' }] }, + ); + const rest = setup({ saveMetaItem: vi.fn().mockRejectedValue(err) }); + const res = await callRoute(rest, 'PUT', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + body: { name: 'showcase_account' }, + }); + + expect(res.statusCode).toBe(422); + expect(res.body.error).toContain(head.trim()); + expect(String(res.body.error)).toHaveLength(500); + expect(String(res.body.error).endsWith('…')).toBe(true); + expect(res.body.issues).toHaveLength(1); + }, 60_000); + + it('a 499 is a client status and passes through; 500 is the first withheld one', async () => { + // The bound itself, from both sides — an off-by-one here would either + // start withholding 4xx bodies (#5436's regression) or leave 500 open + // (this issue's). + for (const [status, expected] of [[499, 'verbatim'], [500, 'withheld']] as const) { + const msg = `driver said: ${SQLITE_NO_TABLE}`; + const rest = setup({ + getMetaItem: vi.fn().mockRejectedValue(Object.assign(new Error(msg), { status })), + }); + const res = await callRoute(rest, 'GET', META_ITEM, { + params: { type: 'object', name: 'showcase_account' }, + }); + + expect(res.statusCode, `status ${status}`).toBe(status); + expect(res.body.error, `status ${status}`).toBe( + expected === 'verbatim' ? msg : INTERNAL_ERROR_MESSAGE, + ); + } + }, 60_000); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 76e274f819..80ce7fb0a5 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -4,7 +4,7 @@ import { IHttpServer, resolveAuthzContext, resolveLocalizationContext, isAuthGateAllowlisted, shouldDenyAnonymous, ANONYMOUS_DENY_BODY, ANONYMOUS_DENY_STATUS, } from '@objectstack/core'; -import { isMcpServerEnabled, looksLikeInternalErrorLeak } from '@objectstack/types'; +import { isMcpServerEnabled, looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; import { allowPerfDisclosure, isPerfDisclosurePrincipal } from '@objectstack/observability'; import { RouteManager } from './route-manager.js'; import { RestServerConfig, RestApiConfig, CrudEndpointsConfig, MetadataEndpointsConfig, BatchEndpointsConfig, RouteGenerationConfig } from '@objectstack/spec/api'; @@ -401,6 +401,12 @@ const CLIENT_MESSAGE_MAX = 500; * `isSqlLeak` before reaching here. Same shape as the drivers' own * `safeShapePreview` (`packages/plugins/driver-sql`), which previews rather * than erases. + * + * [#5437] That last paragraph turned out to be the other branch's bug report: + * `resolveErrorResponse` was applying this same bound to 5xx messages, where + * "short" meant "shipped verbatim" and driver errors are short. Its half of the + * passthrough is now 4xx-only, so this helper is reached only by messages + * written for the caller. Both call sites are therefore 4xx today. */ function truncateClientMessage(message: string): string { return message.length < CLIENT_MESSAGE_MAX @@ -812,8 +818,41 @@ export function mapDataError(error: any, object?: string): { status: number; bod * uniformly across CRUD, batch, metadata, UI and discovery routes. */ function sendError(res: any, error: any, object?: string): void { - const { status, body } = resolveErrorResponse(error, object); - res.status(status).json(body); + const resolved = resolveErrorResponse(error, object); + // [#5437] The client no longer reads a 5xx's own words; the operator must. + logWithheldServerFault(error, resolved); + res.status(resolved.status).json(resolved.body); +} + +/** + * [#5437] Log the ORIGINAL error whenever a server fault's own message was + * withheld from the response body. + * + * This is the other half of "the client does not read it, the log keeps it". + * Sanitising a 5xx is only free of cost while the withheld text is still + * somewhere an operator can find it — otherwise tightening the boundary would + * trade a leak for a blind spot, and the `sys_metadata` persistence failure + * this issue was raised on is exactly the fault an operator must be able to + * diagnose (the in-memory registry has already diverged from the database). + * + * `sendError` had no logging at all, so its 5xx band went from "the client can + * read the driver error" straight to "nobody can" without this. The routes that + * exit through `handleRouteError` already print the whole error object for a + * genuine fault — this fires only in the gap that predicate leaves: 502/503, + * which `isExpectedDataStatus` classifies as normal lifecycle outcomes and + * therefore does not log, and whose message this boundary now drops too. + * + * No-ops when nothing was withheld (the resolved body still carries the error's + * own message), so an untouched passthrough does not gain a log line. + */ +function logWithheldServerFault( + error: any, + resolved: { status: number; body: Record }, +): void { + if (resolved.status < 500) return; + const original = typeof error?.message === 'string' ? error.message : ''; + if (!original || resolved.body?.error === original) return; + logError('[REST] 5xx message withheld from client; original error:', error); } /** @@ -831,26 +870,72 @@ function resolveErrorResponse(error: any, object?: string): { status: number; bo const passThroughStatus = error?.code !== 'OBJECT_NOT_FOUND' && typeof error?.status === 'number' && error.status >= 400 && error.status < 600; if (passThroughStatus) { - // [#5423] Same bound as `mapDataError`'s 4xx passthrough, same cure — - // truncate rather than replace — but applied only to the 4xx half. + // [#5437] A declared 5xx never ships its own message text. + // + // Until now this branch's range was 400-599 while `mapDataError`'s + // sibling branch stopped at 4xx *on purpose* — "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 every route that reports through `sendError` (metadata, UI, + // discovery, batch) got the permissive one: a declared 500 shorter than + // `CLIENT_MESSAGE_MAX` was returned word for word, past `isSqlLeak`, + // past `looksLikeInternalErrorLeak`, past `Internal data error`. + // + // That is not dormant code. `metadata-protocol` interpolates the raw + // driver error into two client-facing 500s — `Failed to persist + // customization overlay to sys_metadata: ${dbError.message}` and + // `Failed to delete customization overlay: ${err.message}` — and a real + // driver line (`SQLITE_ERROR: no such table: sys_metadata`, `relation + // "sys_metadata" does not exist`, a unique-constraint payload naming + // columns) is nowhere near 500 characters, so it arrived intact. Length + // was never a proxy for leakage; on this side of the bound it failed + // OPEN. + // + // The cure is structural rather than another predicate: in the 5xx band + // the message is dropped unconditionally, so there is no phrasing a + // producer can pick — deliberately or by accident — that gets driver + // text past this boundary. A keyword gate would only move the question + // to "does the heuristic know this dialect", which is the failure mode + // that produced this bug. + // + // Sanitising HERE rather than by falling through to `mapDataError` is + // the point: `mapDataError` derives a status from the message TEXT, so + // handing it a declared 5xx re-labels the fault as something else + // entirely — the two overlay 500s come back as `404 OBJECT_NOT_FOUND` + // ("no such table" trips the unknown-object heuristic) and the atomic + // batch's `501 NOT_IMPLEMENTED` as `404 Object '' is not + // registered`, both of which then read as *expected* statuses and stop + // being logged at all. Worse, a 5xx whose text matches no heuristic + // falls out of `mapDataError`'s terminal `{ status: 400, error: raw }` + // — still verbatim, now wearing a client-error status. So: keep the + // status the producer declared, keep the machine-readable `code` (a + // SCREAMING_SNAKE constant is not a leak, and it is what a client keys + // on), drop the prose. // - // This branch's range is 400-599, wider than `mapDataError`'s, and the - // 4xx/5xx split is the one distinction the repo already draws here: - // `mapDataError`'s sibling branch is "deliberately limited to 4xx: 5xx - // messages keep going through the sanitizing heuristics ... so - // internal/SQL details never reach the client verbatim". 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. So - // 5xx keeps the wholesale replacement byte-for-byte — #5423 is about - // rejections a client is meant to read, and widening 5xx leniency is - // not in its scope. + // Accepted cost, recorded so it is not rediscovered as a bug: a + // self-authored 500 body — `OVERLAY_PERSISTENCE_FAILED`'s "In-memory + // registry was updated but will be lost on restart", the atomic + // batch's "retry without options.atomic" — reaches the client as the + // generic sentence plus its `code`. The full text still reaches the + // server log (see `logWithheldServerFault`), which is the side of the + // boundary that sentence was written for. Producers that owe a caller + // an actionable 5xx sentence should say it without interpolating the + // driver's — tracked separately. + if (error.status >= 500) { + return { + status: error.status, + body: { + error: INTERNAL_ERROR_MESSAGE, + ...(error.code ? { code: error.code } : {}), + }, + }; + } + // [#5423] 4xx keeps the bound as a TRUNCATION, not a replacement: a 4xx + // message is addressed TO the caller and is the remedy. Unchanged by + // #5437 — see {@link truncateClientMessage}. const safeMsg = typeof error.message !== 'string' ? 'Request failed' - : error.status < 500 - ? truncateClientMessage(error.message) - : error.message.length < CLIENT_MESSAGE_MAX - ? error.message - : 'Request failed'; + : truncateClientMessage(error.message); return { status: error.status, body: { @@ -936,7 +1021,13 @@ function isExpectedRouteError(status: number, body: Record | un function logUnexpectedRouteError(error: any, resolved: { status: number; body: Record }): void { if (!isExpectedRouteError(resolved.status, resolved.body)) { logError('[REST] Unhandled error:', error); + return; } + // [#5437] An "expected" status can still have had its message withheld — + // 502/503 are lifecycle outcomes this predicate deliberately keeps quiet, + // but a declared one no longer ships its own text either. One line, never + // two: a genuine fault already printed the whole error above. + logWithheldServerFault(error, resolved); } /**