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); } /**