diff --git a/.changeset/mapdataerror-5xx-status-passthrough.md b/.changeset/mapdataerror-5xx-status-passthrough.md new file mode 100644 index 0000000000..c8d26ddecd --- /dev/null +++ b/.changeset/mapdataerror-5xx-status-passthrough.md @@ -0,0 +1,27 @@ +--- +'@objectstack/rest': patch +--- + +REST: a declared 5xx status now survives on the CRUD data routes + +`mapDataError`'s explicit-status passthrough accepted only 4xx, while +`resolveErrorResponse` (the door every metadata/UI/discovery/batch route uses) +accepts 400-599. The same thrown error therefore got two different answers +depending on which route caught it, and on the data routes a producer's +declared 5xx was overwritten — the status re-derived from the message text, or +falling through to `500 INTERNAL_ERROR`. + +The passthrough is now 400-599 on both doors, with the same disposition #5437 +already ruled for a declared server fault: **keep the status, keep the +machine-readable `code`, drop the prose**. The `code` half reads +`declaresServerFault` from `@objectstack/types`, so an empty or non-string code +is not mistaken for an ADR-0112 declaration and nothing is invented when the +producer named no code. + +User-visible effect: an aggregate function a SQL backend cannot compile +(`count_distinct` / `array_agg` / `string_agg`) now answers +`501 NOT_IMPLEMENTED` instead of `500 INTERNAL_ERROR`, and an upstream/ +dependency `502` / `503` reaches the caller as itself rather than as a generic +500. The 4xx half is unchanged (wording truncated, `object` retained), no 5xx +message text reaches the client, and the withheld text still reaches the +operator log. diff --git a/packages/rest/src/rest-4xx-message-truncation.test.ts b/packages/rest/src/rest-4xx-message-truncation.test.ts index 48503483e5..630b25dbbd 100644 --- a/packages/rest/src/rest-4xx-message-truncation.test.ts +++ b/packages/rest/src/rest-4xx-message-truncation.test.ts @@ -142,21 +142,44 @@ describe('mapDataError: short 4xx messages are byte-for-byte unchanged (#5423)', .toBe('Request failed'); }); - it('5xx never enters this branch at all (unchanged: sanitizing heuristics own it)', () => { + it('a 5xx never reaches this truncation at all — its message is dropped whole', () => { + // [#5582] This case was "5xx never enters this branch at all + // (unchanged: sanitizing heuristics own it)". The heuristics no longer + // own it: `mapDataError`'s passthrough now runs 400-599, the same door + // `resolveErrorResponse` opens, so the declared 502 IS preserved on + // this direct-call path — the parenthetical the #5489 note left here + // ("out of #5489's scope") is what that issue closed. + // + // What this file is about is unchanged and is the point of keeping the + // case: TRUNCATION is a 4xx disposition only. A 4xx message is the + // caller's remedy and is bounded; a 5xx message is the operator's and + // is dropped whole — never sliced, never ellipsised, never partially + // visible. That is why a 600-character 5xx is asserted here rather than + // a short one. + const r = mapDataError( + Object.assign(new Error('connect ECONNREFUSED 10.0.0.5:5432 '.repeat(20)), { + status: 502, + code: 'UPSTREAM_UNAVAILABLE', + }), + ); + expect(r.status).toBe(502); + expect(r.body.code).toBe('UPSTREAM_UNAVAILABLE'); + // Withheld, not truncated: no prefix of the original, no ellipsis. + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(String(r.body.error).endsWith('…')).toBe(false); + expect(String(r.body.error)).not.toContain('10.0.0.5'); + expect(String(r.body.error)).not.toContain('ECONNREFUSED'); + }); + + it('a 5xx with no declared code keeps its status and gains no invented one', () => { + // The exact shape this section used to carry (no `code`). Pinned here + // too so the file still covers the half-declaration it was written on. const r = mapDataError( Object.assign(new Error('connect ECONNREFUSED 10.0.0.5:5432 '.repeat(20)), { status: 502 }), ); - expect(r.status).not.toBe(502); - // [#5489] `not.toBe(502)` was true of the OLD landing too, and that - // landing was `400` with every byte of the ECONNREFUSED text — host and - // port included — on the wire. The negative assertion could not tell - // the two apart, so what it actually lands on is pinned here: this - // declared 5xx now leaves `mapDataError` through the terminal - // `UNCLASSIFIED_FAULT`, sanitised and in the server band. (The declared - // 502 is still not preserved on this direct-call path — that is - // `resolveErrorResponse`'s branch, and out of #5489's scope.) - expect(r.status).toBe(500); - expect(r.body.code).toBe('INTERNAL_ERROR'); + expect(r.status).toBe(502); + expect(r.body.code).toBeUndefined(); + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); expect(String(r.body.error)).not.toContain('10.0.0.5'); }); }); diff --git a/packages/rest/src/rest-5xx-status-passthrough.test.ts b/packages/rest/src/rest-5xx-status-passthrough.test.ts new file mode 100644 index 0000000000..07ac2aa0b7 --- /dev/null +++ b/packages/rest/src/rest-5xx-status-passthrough.test.ts @@ -0,0 +1,454 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5582] A producer that DECLARES a 5xx keeps that status on the CRUD data +// routes, exactly as it already does on every route that reports through +// `resolveErrorResponse`. +// +// `rest-server.ts` has two error doors, and they gave opposite answers to one +// question — "the producer declared a `status`": +// +// sendError / handleRouteError (metadata, UI, discovery, batch) 400-599 +// mapDataError (the CRUD data routes, ~11 direct 400-499 +// call sites that bypass the above) +// +// So the same thrown error came back as `502` through one door and as +// `500 INTERNAL_ERROR` through the other. #5437 / PR #5464 already ruled how a +// declared 5xx must be answered — keep the status, keep the machine-readable +// `code`, drop the prose — and this file pins `mapDataError` giving the same +// answer. +// +// The live producer is #5907: `driver-sql` / `driver-turso` throw +// `status: 501` + `code: NOT_IMPLEMENTED` for an aggregate function the spec +// declares and the backend cannot compile (`count_distinct` / `array_agg` / +// `string_agg`). Those names clear `metadata-protocol`'s shape gate, so the +// throw travels from the driver to a CRUD data route — `mapDataError`'s +// direct-call territory — where the 4xx-only passthrough skipped it, no text +// heuristic matched it, and `UNCLASSIFIED_FAULT` answered `500 INTERNAL_ERROR`. +// The caller was told "the server fell over" instead of "this backend does not +// implement that declared capability": the ADR-0112 CODE was overwritten too, +// not just the status. +// +// The `code` half is spelled with `declaresServerFault` (`@objectstack/types`, +// PR #6122) — the criterion this repo already uses for "the producer declared a +// server fault" at the analytics route and in `runtime`'s dispatcher — rather +// than a fourth open-coded truthiness check. +// +// --------------------------------------------------------------------------- +// Reverse verification (restore the 4xx-only gate, keep this file). Directions +// were predicted BEFORE running; §3's prediction was WRONG and is recorded as +// measured rather than rewritten to fit: +// +// §1 §2 §5 predicted RED, measured RED (19 failures total) — every +// declared 5xx degrades to 500 INTERNAL_ERROR again and the #5907 +// 501→500 case fails by name. +// §3 predicted GREEN, measured MIXED — 5 of 9 red. The prediction was +// right about WHAT these cases defend (a 5xx arm that starts +// echoing the producer's prose) and wrong about which assertion +// carries it. The CONTAINMENT half (`not.toContain(message)`) is +// genuinely direction-insensitive: it stayed green in all 9, since +// the pre-fix terminal branch sanitised too (#5489). The ENVELOPE +// half (`error === INTERNAL_ERROR_MESSAGE`) moves, because the old +// heuristics answered some of these inputs with a DIFFERENT +// sanitised envelope — raw SQL / a SQLite or Postgres dump became +// `DATA_STORE_FAULT`'s "Internal data error" + `DATABASE_ERROR`, +// and a unique-constraint payload became `409 UNIQUE_VIOLATION`. +// So the revert measures something worth knowing: for a producer +// that DECLARED a 5xx, this branch now takes precedence over those +// text classifiers. That is the intended ordering (a declaration +// outranks a keyword guess about the same error) and matches +// `resolveErrorResponse`, whose door is likewise the first one. +// Undeclared errors are untouched — §4's last two cases pin that. +// §4 predicted GREEN, measured GREEN — the 4xx half and the branches +// above the passthrough are untouched. +// --------------------------------------------------------------------------- + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { mapDataError, RestServer } from './rest-server.js'; + +const DATA_LIST = '/api/v1/data/:object'; + +// --------------------------------------------------------------------------- +// Producer fixtures — copied from the shipping producers, not invented. +// `@objectstack/rest` must not take a dependency on a driver package to run its +// own tests, so the shapes are reproduced here; what matters is that each is +// `status` + `code` + a message carrying something the caller must not read. +// --------------------------------------------------------------------------- + +/** + * `driver-sql`'s `uncompilableAggregateFunctionError` (#5907) — the first live + * producer this issue has. Its wording names the backend's internal compile + * table, which is operator detail rather than caller detail. + */ +function uncompilableAggregateError(func: string) { + return Object.assign( + new Error( + `Aggregate function "${func}" is declared but not implemented by this backend. ` + + `Compiled here: count, sum, avg, min, max. The name is spelled correctly and ` + + `@objectstack/spec AggregationFunction declares it — this is a capability gap in the ` + + `backend, not a mistake in the query (#5907).`, + ), + { code: 'NOT_IMPLEMENTED', status: 501 }, + ); +} + +/** The issue body's own measurement, with the `code` its producer would declare. */ +function upstreamUnreachableError() { + return Object.assign( + new Error('connect ECONNREFUSED 10.0.0.5:5432 (internal pool)'), + { code: 'UPSTREAM_UNAVAILABLE', status: 502 }, + ); +} + +/** `metadata-protocol`'s `metadataStoreUnavailableError` shape (503 + code). */ +function metadataStoreUnavailableError() { + return Object.assign( + new Error('Metadata store is not available: SQLITE_ERROR: no such table: sys_metadata'), + { code: 'SERVICE_UNAVAILABLE', status: 503 }, + ); +} + +// --------------------------------------------------------------------------- +// §1 The declared 5xx envelope: status and code survive, the prose does not +// --------------------------------------------------------------------------- + +describe('[#5582] mapDataError: a declared 5xx keeps its status and its code', () => { + it("#5907's 501 NOT_IMPLEMENTED survives instead of degrading to 500 INTERNAL_ERROR", () => { + const r = mapDataError(uncompilableAggregateError('count_distinct'), 'showcase_account'); + + // The regression this issue is about, in one pair of assertions. + expect(r.status).toBe(501); + expect(r.body.code).toBe('NOT_IMPLEMENTED'); + // ...and what it used to be, pinned as a NEGATIVE so a partial revert + // (status restored, code still overwritten) cannot pass. + expect(r.status).not.toBe(500); + expect(r.body.code).not.toBe('INTERNAL_ERROR'); + }); + + it('the 502 from the issue body keeps its status; the host and port do not ride along', () => { + const r = mapDataError(upstreamUnreachableError(), 'showcase_account'); + + expect(r.status).toBe(502); + expect(r.body.code).toBe('UPSTREAM_UNAVAILABLE'); + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(JSON.stringify(r.body)).not.toContain('10.0.0.5'); + expect(JSON.stringify(r.body)).not.toContain('5432'); + expect(JSON.stringify(r.body)).not.toContain('ECONNREFUSED'); + }); + + it('a 503 stays a 503 — a retryable dependency outage is not a 500', () => { + const r = mapDataError(metadataStoreUnavailableError(), 'showcase_account'); + + expect(r.status).toBe(503); + expect(r.body.code).toBe('SERVICE_UNAVAILABLE'); + // Its text names the metadata table AND trips the missing-relation + // heuristic; both are withheld and neither re-labels the fault. + expect(JSON.stringify(r.body)).not.toContain('sys_metadata'); + expect(r.body.code).not.toBe('OBJECT_NOT_FOUND'); + expect(r.body.code).not.toBe('DATABASE_ERROR'); + }); + + it('the body is exactly { error, code } — no object, no extra keys', () => { + // Same envelope `resolveErrorResponse` emits for the same input, which + // is the whole point of this issue: one condition, one wire answer, + // whichever door caught it. `object` is deliberately absent — every + // other 5xx envelope in this file (DATA_STORE_FAULT, UNCLASSIFIED_FAULT) + // omits it too. + const r = mapDataError(uncompilableAggregateError('array_agg'), 'showcase_account'); + expect(r.body).toEqual({ error: INTERNAL_ERROR_MESSAGE, code: 'NOT_IMPLEMENTED' }); + }); + + it('the whole band passes through, not a hand-picked list of statuses', () => { + for (const status of [500, 501, 502, 503, 504, 507, 599]) { + const r = mapDataError(Object.assign(new Error('internal detail'), { status, code: 'X_FAULT' })); + expect(r.status).toBe(status); + expect(r.body).toEqual({ error: INTERNAL_ERROR_MESSAGE, code: 'X_FAULT' }); + } + }); + + it('a 499 is a client status and keeps its wording; 500 is the first withheld one', () => { + const at499 = mapDataError(Object.assign(new Error('almost server'), { status: 499, code: 'C' })); + expect(at499.body.error).toBe('almost server'); + + const at500 = mapDataError(Object.assign(new Error('almost server'), { status: 500, code: 'C' })); + expect(at500.body.error).toBe(INTERNAL_ERROR_MESSAGE); + }); + + it('a status outside 400-599 is not a declaration this branch reads', () => { + // 600 mirrors `resolveErrorResponse`'s upper bound exactly: it falls to + // the heuristics, which recognise nothing here, so the terminal + // sanitised 500 answers (#5489). + const r = mapDataError(Object.assign(new Error('nonsense status'), { status: 600, code: 'X' })); + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + }); +}); + +// --------------------------------------------------------------------------- +// §2 The half-declaration: a 5xx with no `code` +// --------------------------------------------------------------------------- + +describe('[#5582] a 5xx that declares no code passes its status and invents nothing', () => { + it('a bare { status: 502 } lands as 502 with NO code at all', () => { + // This is the exact shape the two pins this issue named were written + // on, and the exact shape `resolveErrorResponse` already answers this + // way ("a dynamically-assigned status is treated identically (no code + // declared)", `rest-5xx-message-sanitization.test.ts`): ADR-0112 says + // the PRODUCER names the condition, so the half that was declared is + // honoured and the half that was not is left empty. Inventing + // `INTERNAL_ERROR` here would put a code on the wire nobody wrote — + // and overwriting the 502 with a 500 would re-derive a declared status + // from message text, which is what #5437 ruled against one door over. + const r = mapDataError( + Object.assign(new Error('connect ECONNREFUSED 10.0.0.5:5432 (internal pool)'), { status: 502 }), + ); + + expect(r.status).toBe(502); + expect(r.body.code).toBeUndefined(); + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(JSON.stringify(r.body)).not.toContain('10.0.0.5'); + }); + + it('an EMPTY-string code is not a declaration either — nothing empty reaches the wire', () => { + const r = mapDataError(Object.assign(new Error('boom'), { status: 503, code: '' })); + expect(r.status).toBe(503); + expect(r.body.code).toBeUndefined(); + }); + + it('a NON-STRING code is not a declaration — a driver errno is not an ADR-0112 code', () => { + // `declaresServerFault` requires a string, which is what keeps MySQL's + // numeric `errno`-style codes off the wire as if they were catalog + // entries. The status is still the producer's own. + const r = mapDataError(Object.assign(new Error('boom'), { status: 502, code: 1062 })); + expect(r.status).toBe(502); + expect(r.body.code).toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// §3 Sanitization — the opposite overreach. +// +// GREEN under a revert BY DESIGN (the pre-fix terminal branch sanitised too, +// #5489). These exist so a future 5xx arm cannot start echoing the producer's +// words, which is what the 4xx-only gate was originally defending against. +// --------------------------------------------------------------------------- + +describe('[#5582] nothing of a 5xx message reaches the client', () => { + const leaky = [ + ['raw SQL', 'insert into showcase_account (name, email) values (?, ?)'], + ['a SQLite dump', 'SQLITE_ERROR: no such table: sys_metadata'], + ['a Postgres dump', 'relation "public.showcase_account" does not exist'], + ['a connection string', 'connect ECONNREFUSED 10.0.0.5:5432 (internal pool)'], + ['a unique-constraint payload', 'UNIQUE constraint failed: sys_user.email'], + ['an operator sentence', 'retry without options.atomic, or probe capabilities.transactionalBatch'], + ['an RLS policy field name', '[read-scope-sql] unsafe field identifier "secret_policy_field"'], + ] as const; + + it.each(leaky)('withholds %s', (_label, message) => { + const r = mapDataError( + Object.assign(new Error(message), { status: 500, code: 'DEPENDENCY_FAULT' }), + 'showcase_account', + ); + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(JSON.stringify(r.body)).not.toContain(message); + }); + + it('length is not the criterion — a 600-character 5xx is withheld, not truncated', () => { + const r = mapDataError( + Object.assign(new Error(`x${'y'.repeat(600)}`), { status: 503, code: 'SERVICE_UNAVAILABLE' }), + ); + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(String(r.body.error).endsWith('…')).toBe(false); + expect(String(r.body.error)).not.toContain('yyy'); + }); + + it('a missing message changes nothing — the generic sentence is unconditional', () => { + const r = mapDataError({ status: 502, code: 'UPSTREAM_UNAVAILABLE' }); + expect(r.body).toEqual({ error: INTERNAL_ERROR_MESSAGE, code: 'UPSTREAM_UNAVAILABLE' }); + }); +}); + +// --------------------------------------------------------------------------- +// §4 Non-regression: the 4xx half and the branches ABOVE the passthrough +// --------------------------------------------------------------------------- + +describe('[#5582] the 4xx half and the structured branches are untouched', () => { + it('a short 4xx is still byte-for-byte verbatim, with its object', () => { + const msg = 'FORBIDDEN: insufficient privileges to update showcase_inquiry rec1'; + const r = mapDataError(Object.assign(new Error(msg), { code: 'FORBIDDEN', status: 403 }), 'showcase_inquiry'); + expect(r.status).toBe(403); + expect(r.body).toEqual({ error: msg, code: 'FORBIDDEN', object: 'showcase_inquiry' }); + }); + + it('a long 4xx is still TRUNCATED rather than withheld (#5423)', () => { + const r = mapDataError(Object.assign(new Error('z'.repeat(600)), { status: 400, code: 'INVALID_FILTER' })); + expect(r.status).toBe(400); + expect(String(r.body.error)).toHaveLength(500); + expect(String(r.body.error).endsWith('…')).toBe(true); + expect(r.body.error).not.toBe(INTERNAL_ERROR_MESSAGE); + }); + + it('OBJECT_NOT_FOUND keeps its canonical 404 even when it declares a 5xx status', () => { + // That branch sits ABOVE the passthrough, which is how `mapDataError` + // reaches the same conclusion `resolveErrorResponse` spells out by + // excluding the code from its passthrough gate: one condition, one + // wire code, whichever layer detected it (#3770). + const r = mapDataError( + Object.assign(new Error('gone'), { code: 'OBJECT_NOT_FOUND', status: 503 }), + 'showcase_account', + ); + expect(r.status).toBe(404); + expect(r.body.code).toBe('OBJECT_NOT_FOUND'); + }); + + it('a 5xx-declaring DELETE_RESTRICTED still answers 409 with its structured fields', () => { + const r = mapDataError( + Object.assign(new Error('dependents exist'), { + code: 'DELETE_RESTRICTED', status: 500, dependentCount: 3, + }), + 'sys_position', + ); + expect(r.status).toBe(409); + expect(r.body.dependentCount).toBe(3); + }); + + it('an error with NO declared status is still judged by the heuristics', () => { + // The overwhelming majority: raw driver errors carry no `status` at + // all, so the classifiers still own them and still SUPPLY a code. + const leak = mapDataError(new Error('SQLITE_ERROR: no such table: some_aux_table'), 'showcase_account'); + expect(leak.status).toBe(500); + expect(leak.body.code).toBe('DATABASE_ERROR'); + + const unknown = mapDataError(new Error('nothing recognisable here'), 'showcase_account'); + expect(unknown.status).toBe(500); + expect(unknown.body.code).toBe('INTERNAL_ERROR'); + }); + + it('an UNDECLARED unique violation is still a 409 — every dialect, unchanged', () => { + // The case the reverse verification surfaced as worth pinning. A + // declared 5xx now outranks the text classifiers (that is the point of + // the branch), so the guard that matters is that real driver conflicts + // — which carry a driver `code` and NO `status` — never enter it and + // still answer `409 UNIQUE_VIOLATION` (#6250). + const dialects = [ + Object.assign(new Error('UNIQUE constraint failed: sys_user.email'), { code: 'SQLITE_CONSTRAINT_UNIQUE' }), + Object.assign(new Error('duplicate key value violates unique constraint "sys_user_email_key"'), { code: '23505' }), + Object.assign(new Error("ER_DUP_ENTRY: Duplicate entry 'a@b.com' for key 'idx_email_unique'"), { code: 'ER_DUP_ENTRY' }), + ]; + for (const err of dialects) { + const r = mapDataError(err, 'sys_user'); + expect(r.status).toBe(409); + expect(r.body.code).toBe('UNIQUE_VIOLATION'); + } + }); +}); + +// --------------------------------------------------------------------------- +// §5 Walked through a real CRUD data route +// +// The unit cases above call `mapDataError` directly. This section proves the +// wire answer on the route the issue is about — the direct-call territory that +// bypasses `resolveErrorResponse` entirely. +// --------------------------------------------------------------------------- + +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 setup(protocolOverrides: Record = {}) { + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([{ name: 'showcase_account' }]), + getMetaItem: vi.fn().mockResolvedValue({}), + findData: vi.fn().mockResolvedValue([]), + ...protocolOverrides, + }; + const rest = new RestServer( + createMockServer() as any, + protocol, + { api: { requireAuth: false } } as any, + ); + (rest as any).resolveExecCtx = async () => ({ userId: 'u1' }); + rest.registerRoutes(); + return rest; +} + +async function callDataList(rest: any, object: string) { + const route = rest.getRoutes().find((r: any) => r.method === 'GET' && r.path === DATA_LIST); + if (!route) throw new Error(`GET ${DATA_LIST} route not registered`); + const res = makeRes(); + await route.handler({ method: 'GET', params: { object }, query: {}, headers: {} }, res); + return res; +} + +let errorSpy: ReturnType; +const loggedText = (needle: string) => + errorSpy.mock.calls.some((call: unknown[]) => JSON.stringify(call.map(String)).includes(needle)); + +beforeEach(() => { errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); }); +afterEach(() => { errorSpy.mockRestore(); }); + +describe('[#5582] the CRUD data route, in process', () => { + it('an aggregate the backend cannot compile answers 501 NOT_IMPLEMENTED on the wire', async () => { + const rest = setup({ + findData: vi.fn().mockRejectedValue(uncompilableAggregateError('string_agg')), + }); + + const res = await callDataList(rest, 'showcase_account'); + + expect(res.statusCode).toBe(501); + expect(res.body).toEqual({ error: INTERNAL_ERROR_MESSAGE, code: 'NOT_IMPLEMENTED' }); + // The wording the driver wrote for the operator stays with the operator. + expect(JSON.stringify(res.body)).not.toContain('Compiled here'); + }, 60_000); + + it('a 502 reaches the client as a 502, and the operator still gets the words', async () => { + const rest = setup({ findData: vi.fn().mockRejectedValue(upstreamUnreachableError()) }); + + const res = await callDataList(rest, 'showcase_account'); + + expect(res.statusCode).toBe(502); + expect(res.body.code).toBe('UPSTREAM_UNAVAILABLE'); + expect(JSON.stringify(res.body)).not.toContain('10.0.0.5'); + // No blind spot. 502 is an `isExpectedDataStatus` lifecycle outcome, so + // it gets no "[REST] Unhandled error" line — `logWithheldServerFault` + // (#5437) is what covers it, and `logUnexpectedRouteError` calls it on + // exactly this path. (Green either side of the fix: before it, the + // degraded 500 was logged through the unhandled channel instead. The + // invariant is that the withheld text is never lost, not which channel + // carries it.) + expect(loggedText('ECONNREFUSED')).toBe(true); + }, 60_000); + + it('a 4xx on the same route is unchanged — wording and object still land', async () => { + const rest = setup({ + findData: vi.fn().mockRejectedValue( + Object.assign(new Error('Filter operator "$nope" is not declared'), { + status: 400, code: 'INVALID_FILTER', + }), + ), + }); + + const res = await callDataList(rest, 'showcase_account'); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FILTER'); + expect(res.body.error).toBe('Filter operator "$nope" is not declared'); + expect(res.body.object).toBe('showcase_account'); + }, 60_000); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 2b4c788e12..de85c7eb0f 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -786,11 +786,88 @@ export function mapDataError(error: any, object?: string): { status: number; bod // HTTP status (e.g. plugin-sharing's record-scope denial: status 403 + // code FORBIDDEN) — mirrors sendError's `.status` handling, which the // generic data routes bypass by calling mapDataError directly (#2926 ⑦). - // Placed AFTER the structured-code branches above (409s carry rich - // fields this envelope would drop) and deliberately limited to 4xx: - // 5xx messages keep going through the sanitizing heuristics below so - // internal/SQL details never reach the client verbatim. - if (typeof error?.status === 'number' && error.status >= 400 && error.status < 500) { + // Placed AFTER the structured-code branches above (409s carry rich fields + // this envelope would drop). + // + // [#5582] The range is 400–599, the same door {@link resolveErrorResponse} + // opens. It used to stop at 4xx, argued as "5xx messages keep going through + // the sanitizing heuristics below so internal/SQL details never reach the + // client verbatim" — which was the right FEAR and the wrong CURE, and + // #5437/#5464 already ruled on it one door over. Two consequences, both + // measured: + // + // - **The declaration was destroyed to protect the prose.** The two + // doors gave opposite answers to one question ("the producer declared a + // status"): a `502` reporting an unreachable upstream came back as + // `500 INTERNAL_ERROR` on every CRUD data route and as `502` on every + // metadata/UI/discovery route. 502/503 are not synonyms of 500 — they + // are `isExpectedDataStatus` lifecycle outcomes, and proxies and retry + // policies read them differently. + // - **The status was then re-derived from the message TEXT**, which is + // exactly what {@link resolveErrorResponse}'s docblock forbids: an error + // that declared its own condition had that condition overwritten by a + // keyword heuristic, or (matching none) by `UNCLASSIFIED_FAULT`. Since + // #5907 that is live rather than theoretical: `driver-sql` and + // `driver-turso` throw `status: 501` / `code: NOT_IMPLEMENTED` for a + // spec-declared aggregate function the backend cannot compile + // (`count_distinct` / `array_agg` / `string_agg`), those functions clear + // the protocol's shape gate, and the throw reaches these routes — so the + // caller was told `500 INTERNAL_ERROR` ("the server fell over") instead + // of `501 NOT_IMPLEMENTED` ("this backend does not implement that + // declared capability"). The ADR-0112 code was overwritten, not just the + // status. + // + // The fear is answered structurally instead, by the arm below: in the 5xx + // band the message is dropped UNCONDITIONALLY, so no phrasing a producer + // can pick — deliberately or by accident — carries driver text past this + // boundary. Sanitising here is strictly tighter than the old fallthrough, + // which shipped a 5xx's raw words verbatim whenever they tripped no + // keyword (`connect ECONNREFUSED 10.0.0.5:5432` did exactly that until + // #5489 turned the terminal branch into a sanitised 500). + // + // Not a diagnostics loss: every caller pairs this with + // `logUnexpectedRouteError`, whose `logWithheldServerFault` half (#5437) + // fires precisely when a response dropped the error's own message — so the + // 502/503 band that `isExpectedRouteError` keeps quiet still leaves the + // operator a line carrying the full original error. + if (typeof error?.status === 'number' && error.status >= 400 && error.status < 600) { + // [#5582] A declared server fault: keep the status, keep the + // machine-readable `code`, drop the prose. Byte-identical to + // {@link resolveErrorResponse}'s 5xx arm — one condition, one wire + // answer, whichever door caught it. + // + // The `code` rides along on {@link declaresServerFault}, the criterion + // `@objectstack/types` already owns for "this producer DECLARED a + // server fault" (`status >= 500` *and* a non-empty string `code`; PR + // #6122, pinned by `error-leak.test.ts`, read by the analytics route + // here and by `runtime`'s dispatcher). Inside this branch its status + // half is already true, so what it adds is the `code` half — and it + // adds it as a TESTED predicate rather than a fourth open-coded + // truthiness check, which is what keeps a numeric driver `errno` or an + // empty string from landing on the wire as an ADR-0112 code. + // + // A 5xx with NO code passes its status through carrying no code at all, + // deliberately: ADR-0112 says the PRODUCER names the condition, so a + // half-declaration is honoured for the half that was declared and + // nothing is invented for the half that was not. That is the answer + // `resolveErrorResponse` already gives the same shape + // (`rest-5xx-message-sanitization.test.ts` §"a dynamically-assigned + // status is treated identically"), and inventing `INTERNAL_ERROR` here + // would put a code on the wire the producer never wrote — while + // re-deriving the status from the message text is the defect this + // branch exists to remove. + if (error.status >= 500) { + return { + status: error.status, + body: { + error: INTERNAL_ERROR_MESSAGE, + ...(declaresServerFault(error) ? { code: error.code as string } : {}), + }, + }; + } + // [#5423] The 4xx arm is UNCHANGED by #5582: a 4xx message is addressed + // TO the caller and is the remedy, so it keeps its wording, its + // `object`, and the bound as a TRUNCATION rather than a replacement. // An over-long message is TRUNCATED, not swapped for generic text // (#5423) — see {@link truncateClientMessage}. A missing or empty one // still degrades to `'Request failed'`: there is nothing to truncate. diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 00689e2f3e..718346699a 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -2329,20 +2329,49 @@ describe('mapDataError — schema/constraint envelopes', () => { expect(r.body.error).toContain('insufficient privileges'); }); - it('does NOT pass through an explicit 5xx status (message stays sanitized)', () => { + // [#5582] This case was "does NOT pass through an explicit 5xx status + // (message stays sanitized)", and BOTH halves of that title have now been + // ruled on separately — which is why it is two cases. + // + // The status half was the defect: `mapDataError`'s passthrough stopped at + // 4xx while `resolveErrorResponse`'s went to 599, so one declared error got + // two answers depending on which door caught it, and on the data routes a + // declared 502/503 was rewritten to 500 by a message-text heuristic. The + // passthrough is now the same 400-599 both sides (#5437 / PR #5464 is the + // ruling it now matches). + // + // The message half was never in question and is unchanged: a 5xx's own words + // never reach the client. + it('passes an explicit 5xx status through, with its code and WITHOUT its message', () => { + const r = mapDataError( + Object.assign(new Error('connect ECONNREFUSED 10.0.0.5:5432 (internal pool)'), { + status: 502, + code: 'UPSTREAM_UNAVAILABLE', + }), + ); + // The declared status and the machine-readable code both survive... + expect(r.status).toBe(502); + expect(r.body.code).toBe('UPSTREAM_UNAVAILABLE'); + // ...and neither is the degraded answer this used to give. + expect(r.status).not.toBe(500); + expect(r.body.code).not.toBe('INTERNAL_ERROR'); + // The sanitization half of the old assertion, kept verbatim. + expect(String(r.body.error)).not.toContain('ECONNREFUSED'); + expect(JSON.stringify(r.body)).not.toContain('10.0.0.5'); + }); + + it('a 5xx that declares NO code passes its status and invents no code', () => { + // The shape this case originally carried. `declaresServerFault` — the + // `@objectstack/types` criterion the 5xx arm reads for the `code` half — + // needs a non-empty string `code`, and ADR-0112 says the PRODUCER names the + // condition: the declared half is honoured, the undeclared half is left + // empty rather than filled with an invented `INTERNAL_ERROR`. Same answer + // `resolveErrorResponse` already gives this shape. const r = mapDataError( Object.assign(new Error('connect ECONNREFUSED 10.0.0.5:5432 (internal pool)'), { status: 502 }), ); - // 5xx bypasses the passthrough and falls into the sanitizing heuristics. - expect(r.status).not.toBe(502); - expect(r.body.code).not.toBe('FORBIDDEN'); - // [#5489] Both negatives above were satisfied by the OLD landing as well — - // a 400 carrying `connect ECONNREFUSED 10.0.0.5:5432 (internal pool)` - // verbatim, which is a server fault wearing a client-error status AND the - // leak the sanitizing heuristics were supposed to stop. Pinned positively - // now that the terminal branch is a sanitised 500. - expect(r.status).toBe(500); - expect(r.body.code).toBe('INTERNAL_ERROR'); + expect(r.status).toBe(502); + expect(r.body.code).toBeUndefined(); expect(String(r.body.error)).not.toContain('ECONNREFUSED'); });