From a5e70a3f058264ef389b618b5bec6a36fdde914b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 19:48:24 +0000 Subject: [PATCH] =?UTF-8?q?fix(rest):=20=E6=9C=AA=E5=88=86=E7=B1=BB?= =?UTF-8?q?=E7=9A=84=E8=B7=AF=E7=94=B1=E9=94=99=E8=AF=AF=E5=9B=9E=E6=B6=88?= =?UTF-8?q?=E6=AF=92=205xx,=E4=B8=8D=E5=86=8D=E6=8A=8A=E6=9C=8D=E5=8A=A1?= =?UTF-8?q?=E7=AB=AF=E6=95=85=E9=9A=9C=E8=AF=B4=E6=88=90=20400=20=E5=AE=A2?= =?UTF-8?q?=E6=88=B7=E7=AB=AF=E9=94=99=E8=AF=AF=20(#5489)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mapDataError` 的终局兜底 —— 所有 code 匹配、显式状态直通、文本启发式全部 放弃之后的那一支 —— 原先答 `{ status: 400, body: { error: <原始 message> } }`。 两半都错在同一个方向: - 400 的语义是「你请求错了」,SDK / 代理 / 重试策略据此判定不要重试。真正落到 这一支的恰恰相反:元数据存储读不到时 `matchEndpoint` 按契约抛错(ADR-0110 D3,抛就是为了让 outage 不伪装成 miss),或者处理器自身的 `TypeError`。 实测 `GET /api/v1/meta/api` 对着抛 `Error('metadata store unreachable')` 的存储:HTTP 400。 - 原文逐字下发,而这是全文件里最没有证据可以下发的一条路径:走到这里的前提 就是 `looksLikeInternalErrorLeak` 什么都没匹配上。 改为 `UNCLASSIFIED_FAULT()`:`500 {error:'Internal server error', code:'INTERNAL_ERROR'}`。`INTERNAL_ERROR` 而非 `DATA_STORE_FAULT` 的 `DATABASE_ERROR` —— 后者用在证据指名了存储故障的地方,而这一支的定义性事实 是没有任何证据;`INTERNAL_ERROR` 是 `standardErrorCodeForHttpStatus(500)` 的取值,不是第三套措辞。 真客户端错误一个未动:改动前先给这一支加桩跑完 rest 全套(48 文件 / 719 用例),落到这里的只有 6 个错误 —— 本单的 outage、两个 502 的 ECONNREFUSED、 三个 TypeError,没有一个是客户端错误;历史上唯一骑这条兜底的客户端错误家族 (driver-sql 的 filter 拒收)已由 #4436 在生产者侧迁走。 - 新增 `rest-unclassified-fault-status.test.ts`:兜底落点、消毒、日志留痕、 以及「真 4xx 全部从各自分支拿到原状态」的边界钉。 - `rest-endpoint-surfaces-served-only.test.ts` 的 outage 用例从 `>=400` 升格为 5xx(#5487 的注释写明了在等本单)。 - `rest.test.ts` / `rest-4xx-message-truncation.test.ts` 里两条只写 `not.toBe(502)` 的否定断言升级为钉住实际落点 —— 它们对旧的 400+原文泄漏 同样成立,分不出两者。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --- ...-unclassified-error-server-fault-status.md | 50 ++++ packages/plugins/driver-sql/src/sql-driver.ts | 6 + .../src/rest-4xx-message-truncation.test.ts | 11 + .../src/rest-5xx-message-sanitization.test.ts | 5 + ...rest-endpoint-surfaces-served-only.test.ts | 17 +- .../src/rest-expected-error-logging.test.ts | 29 +- packages/rest/src/rest-server.ts | 70 ++++- .../rest-unclassified-fault-status.test.ts | 255 ++++++++++++++++++ .../src/rest-unknown-object-heuristic.test.ts | 4 + packages/rest/src/rest.test.ts | 8 + 10 files changed, 436 insertions(+), 19 deletions(-) create mode 100644 .changeset/rest-unclassified-error-server-fault-status.md create mode 100644 packages/rest/src/rest-unclassified-fault-status.test.ts diff --git a/.changeset/rest-unclassified-error-server-fault-status.md b/.changeset/rest-unclassified-error-server-fault-status.md new file mode 100644 index 0000000000..eec0042bf4 --- /dev/null +++ b/.changeset/rest-unclassified-error-server-fault-status.md @@ -0,0 +1,50 @@ +--- +"@objectstack/rest": patch +--- + +fix(rest): an unclassified route error answers a sanitised 500, not a 400 (#5489) + +**升级须知 — 状态码行为变化。** `@objectstack/rest` 的错误映射 `mapDataError` +在所有分类分支都不匹配时,原先的终局兜底是 +`{ status: 400, body: { error: <原始 message> } }`。这一支现在改为一个消毒过的 +服务端故障信封: + +``` +500 {"error":"Internal server error","code":"INTERNAL_ERROR"} +``` + +**为什么。** 400 的语义是「你请求错了」——SDK、fetch 封装、代理和重试策略都据此 +判定「不要重试,调用方得改点什么」。而真正落到这一支的错误恰恰相反:元数据存储 +读不到时 `matchEndpoint` 按契约抛错(它抛就是为了让 outage 不伪装成「没有声明 +任何 endpoint」,ADR-0110 D3),或者干脆是处理器自身的 `TypeError`。两者调用方都 +修不了,且都**应该**重试。实测:`GET /api/v1/meta/api` 对着一个抛 +`Error('metadata store unreachable')` 的存储,返回 HTTP 400。 + +同时,原始 message 是逐字下发的——而这偏偏是全文件里最没有证据表明可以下发的一 +条路径:走到这里的前提就是 `looksLikeInternalErrorLeak` 什么都没匹配上,而 +#5462 已经记过「关键词启发式沉默不等于安全」。实测到的一例:一个声明了 +`status: 502`、message 为 `connect ECONNREFUSED 10.0.0.5:5432 (internal pool)` +的错误,经由数据路由直接调用 `mapDataError` 时,以 400 携带主机与端口下发。 +沿用 #5464 的纪律:原文进服务端日志,不进客户端(500 不在 +`isExpectedDataStatus` 内,`handleRouteError` 会打印完整错误对象)。 + +**真正的客户端错误一个都没有改变。** 改动前先做了测绘:给这一支加桩,跑完 +`@objectstack/rest` 全套(48 文件 / 719 用例),落到这一支的只有 6 个错误——本单 +的存储 outage、两个 502 的 ECONNREFUSED、三个 `TypeError`,没有一个是客户端 +错误。历史上唯一骑在这条兜底上的客户端错误家族(driver-sql 无法编译的 filter +拒绝)已由 #4436 在**生产者侧**声明 `status: 400` + `INVALID_FILTER` 迁走。 +validation / permission / unknown object / unknown field / not-null 漂移 / +unique 冲突 / 沙箱业务拒绝等全部仍由各自分支给出原本的 4xx。 + +**`INTERNAL_ERROR` 而非 `DATABASE_ERROR`。** #5462 的 `DATA_STORE_FAULT` +(`500 DATABASE_ERROR`)用在证据**指名**了存储故障的地方(驱动的 missing-relation +措辞、`looksLikeInternalErrorLeak` 命中);而这一支的定义性事实是「没有任何证据」, +把处理器的 `TypeError` 报成 `DATABASE_ERROR` 会把运维指向一个其实健康的数据库。 +`INTERNAL_ERROR` 是 `standardErrorCodeForHttpStatus(500)` 的取值 +(`@objectstack/spec` 的 `HttpStatusErrorCodeMap`)——目录自己为「500 且无更具体 +code」定义的下限,不是第三套措辞;message 复用的也是 +`resolveErrorResponse` 声明式 5xx 分支已在用的 `INTERNAL_ERROR_MESSAGE`。 + +**如果你的客户端把这条兜底当 400 处理过**:它现在是 5xx,可以重试;若你有生产者 +依赖「不声明 status 即可把 message 原文送达调用方」,请改为在抛出点声明 +`status` 与 `code`(契约优先),那是唯一仍会把措辞交给调用方的路径。 diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 11703e6fe9..2178ec05c2 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -467,6 +467,12 @@ const SQLITE_TIME_EXPR_REFS = 8; * tail (attribution, issue numbers) may be cut. Keep the actionable part — * operator, field, path, what arrived, what the spec declares — at the FRONT. * + * [#5489] The "without a status it reached the client verbatim" half is now + * history: that terminal branch answers a sanitised 500 (`INTERNAL_ERROR`). + * Declaring `status` + `code` at the throw site is therefore the ONLY way a + * refusal's words reach the caller at all — which is the contract-first + * arrangement #4436 wanted, no longer relying on a fallback that leaked. + * * The `[sql-driver]` prefix these messages used to carry is GONE from the text: * it is driver-internal wording, and shipping it to clients is exactly what the * #3867 sanitiser exists to stop. The operator/field/vocabulary detail — the diff --git a/packages/rest/src/rest-4xx-message-truncation.test.ts b/packages/rest/src/rest-4xx-message-truncation.test.ts index 84cf40caa2..579610f9ff 100644 --- a/packages/rest/src/rest-4xx-message-truncation.test.ts +++ b/packages/rest/src/rest-4xx-message-truncation.test.ts @@ -147,6 +147,17 @@ describe('mapDataError: short 4xx messages are byte-for-byte unchanged (#5423)', 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(String(r.body.error)).not.toContain('10.0.0.5'); }); }); diff --git a/packages/rest/src/rest-5xx-message-sanitization.test.ts b/packages/rest/src/rest-5xx-message-sanitization.test.ts index ed153ef655..2f189ef881 100644 --- a/packages/rest/src/rest-5xx-message-sanitization.test.ts +++ b/packages/rest/src/rest-5xx-message-sanitization.test.ts @@ -44,6 +44,11 @@ // -> 404 Object 'showcase_account' is not registered // 500 `Failed to delete customization overlay: connect ECONNREFUSED ...` // -> 400 with the driver text STILL verbatim (terminal fallback) +// [#5489] that terminal fallback is now a sanitised 500, so this +// third row's LEAK is closed at the source. The other two rows are +// untouched — they are mis-classifications by the text heuristics, +// not by the fallback — and the reason this fix stays in the branch +// itself (keep the producer's declared status) is unchanged. // // 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 diff --git a/packages/rest/src/rest-endpoint-surfaces-served-only.test.ts b/packages/rest/src/rest-endpoint-surfaces-served-only.test.ts index 5c4412da12..6ce7d627be 100644 --- a/packages/rest/src/rest-endpoint-surfaces-served-only.test.ts +++ b/packages/rest/src/rest-endpoint-surfaces-served-only.test.ts @@ -265,13 +265,16 @@ describe('#5224 — GET /meta/api announces only what the matcher serves', () => const { rest } = mountRest(ALL_ENUMERATED, outage); const res = await getMetaApi(rest); - // The pin is that the request FAILS rather than answering a set. The exact - // status is not this change's to decide: an unrecognised error reaching - // `handleRouteError` lands on `mapDataError`'s terminal fallback, which - // this route measured at 400 — a pre-existing classification shared by - // every error on the metadata routes, not a consequence of the narrowing. - // Asserting 5xx here would pin someone else's bug as if it were fixed. - expect(res.statusCode).toBeGreaterThanOrEqual(400); + // [#5489] Promoted from `>= 400` to the 5xx band. #5487 deliberately left + // it at `>= 400` because the terminal fallback in `mapDataError` measured + // 400 here, and asserting 5xx would have pinned someone else's bug as if it + // were fixed. #5489 fixed it: an outage the mapper cannot attribute to the + // request is a server fault, which is what an SDK must read to decide that + // retrying is the right move. The route's own pin — that it FAILS rather + // than confidently answering "this deployment declares no endpoints" — is + // unchanged and is the second assertion. + expect(res.statusCode).toBeGreaterThanOrEqual(500); + expect(res.body?.code).toBe('INTERNAL_ERROR'); expect(res.body?.items ?? res.body).not.toEqual([SERVED]); }, 60_000); }); diff --git a/packages/rest/src/rest-expected-error-logging.test.ts b/packages/rest/src/rest-expected-error-logging.test.ts index 6e34d8070e..3dea0373be 100644 --- a/packages/rest/src/rest-expected-error-logging.test.ts +++ b/packages/rest/src/rest-expected-error-logging.test.ts @@ -25,6 +25,13 @@ // OPPOSITE overreach, a predicate widened to "any 4xx is expected", which // would silence the un-coded 400 that `mapDataError` degrades an // unrecognised error (a handler `TypeError`) to. +// +// [#5489] That last sentence describes the world before the unrecognised-error +// fallback became a sanitised 500. The handler-bug case below now asserts 500; +// its adversary is no longer a widened 4xx predicate but any future attempt to +// add 500 to `isExpectedDataStatus`. The invariant it guards — a real handler +// bug is never silent — is the same one, and is now carried by the status band +// rather than by the absence of a `code`. import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { RestServer } from './rest-server'; @@ -168,11 +175,16 @@ describe('metadata routes — genuine faults keep the loud log (#4886)', () => { expect(res.statusCode).toBe(500); }); - it('an UNRECOGNISED error (handler bug) stays loud even though it maps to 400', async () => { - // This is the case a blanket "any 4xx is expected" predicate would - // wrongly silence: `mapDataError` degrades anything it recognises - // nothing about to an UN-CODED 400, and that is where a real handler - // bug lands. Silencing it would be the mirror-image of #4886. + it('an UNRECOGNISED error (handler bug) stays loud — and is a 500, not a 400 (#5489)', async () => { + // The loudness is what #4886 pinned, and it is unchanged. What moved is + // WHY it is structural: this case used to land on `mapDataError`'s + // un-coded 400 fallback, so the guard read "loud even though it maps to + // 400" and its adversary was a predicate widened to "any 4xx is + // expected". #5489 made that fallback a sanitised 500 + // (`UNCLASSIFIED_FAULT`) because a handler bug is not the caller's + // fault and an SDK must not read "do not retry" off it. 500 is outside + // `isExpectedDataStatus` entirely, so the log line no longer depends on + // the predicate staying narrow in the 4xx band. const bug = new TypeError('Cannot read properties of undefined (reading \'name\')'); const { rest } = setup({ getMetaItem: vi.fn().mockRejectedValue(bug) }); @@ -180,8 +192,11 @@ describe('metadata routes — genuine faults keep the loud log (#4886)', () => { expect(unhandledLogs()).toHaveLength(1); expect(unhandledLogs()[0][1]).toBe(bug); - expect(res.statusCode).toBe(400); - expect(res.body?.code).toBeUndefined(); + expect(res.statusCode).toBe(500); + expect(res.body?.code).toBe('INTERNAL_ERROR'); + // The bug's own words are the operator's, not the client's — and the + // log line above is where they went. + expect(JSON.stringify(res.body)).not.toContain('Cannot read properties'); }); }); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 69d72b781d..4adaf64711 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -436,6 +436,59 @@ const DATA_STORE_FAULT = (): { status: number; body: Record } = body: { error: 'Internal data error', code: 'DATABASE_ERROR' }, }); +/** + * [#5489] The envelope for "nothing in this mapper recognised the error": a + * sanitised 500 carrying the catalog's `INTERNAL_ERROR`. + * + * This is `mapDataError`'s TERMINAL branch, and until now it answered + * `{ status: 400, error: }`. Both halves of that were wrong + * in the same direction: + * + * - **400 says the CALLER is at fault**, and an SDK reads it as "do not + * retry, fix the request". The errors that actually reach here are the ones + * no branch above could attribute to the request at all — a metadata store + * that cannot be read (`matchEndpoint` throws rather than answering an empty + * set, precisely so an outage does not masquerade as a miss; ADR-0110 D3), + * or a plain handler bug (`TypeError: x is not a function`). Both are server + * faults that a caller cannot fix and a caller SHOULD retry. Measured on + * `GET /api/v1/meta/api` with a store that throws + * `Error('metadata store unreachable')`: HTTP 400 (#5224 / PR #5487 left the + * assertion at `>= 400` rather than pin this as intended). + * - **The raw message shipped verbatim**, which is the exact discipline + * #5437/#5464 closed one branch up: a declared 5xx drops its prose because + * length was never a proxy for leakage. An error that matched no heuristic + * is the LEAST attributable text in the file — this branch is reached only + * because `looksLikeInternalErrorLeak` said nothing, and #5462 already + * recorded that a negative from a keyword heuristic is not evidence of + * safety. The words still reach the operator: 500 is outside + * `isExpectedDataStatus`, so `handleRouteError` prints `[REST] Unhandled + * error` with the whole error, and `sendError`'s `logWithheldServerFault` + * covers the routes that bypass it. + * + * `INTERNAL_ERROR` rather than {@link DATA_STORE_FAULT}'s `DATABASE_ERROR`, and + * the distinction is deliberate: `DATA_STORE_FAULT` is emitted where the + * evidence NAMES a store failure (a driver's missing-relation phrasing, a + * `looksLikeInternalErrorLeak` hit), so it can honestly say "database". Here + * the defining fact is that there is no evidence of anything — sending a + * handler `TypeError` back as `DATABASE_ERROR` would point an operator at a + * database that is fine. `INTERNAL_ERROR` is not a third vocabulary either: it + * is what `standardErrorCodeForHttpStatus(500)` yields (`HttpStatusErrorCodeMap` + * in `@objectstack/spec`) — the catalog's own floor for "500 with no more + * specific code" — and the message is the same `INTERNAL_ERROR_MESSAGE` the + * declared-5xx branch of {@link resolveErrorResponse} already emits. + * + * What did NOT move: every branch above this one. A client error is a 4xx here + * because a producer DECLARED `status` in the 4xx band or because a branch + * matched it by `code`/name/phrasing — validation, permission, unknown object, + * unknown field, not-null drift, unique violation, the sandbox unwraps. This + * branch is the one that had nothing to go on, and "no idea" is a server-side + * answer, not a client-side one. + */ +const UNCLASSIFIED_FAULT = (): { status: number; body: Record } => ({ + status: 500, + body: { error: INTERNAL_ERROR_MESSAGE, code: 'INTERNAL_ERROR' }, +}); + /** * [#5462] Does a driver's missing-relation message name the very object this * request asked for? @@ -899,7 +952,7 @@ export function mapDataError(error: any, object?: string): { status: number; bod } return DATA_STORE_FAULT(); } - return { status: 400, body: { error: raw || 'Bad request' } }; + return UNCLASSIFIED_FAULT(); } /** @@ -1086,10 +1139,17 @@ function isExpectedQueryRejection(body: Record | undefined): bo * - `isExpectedQueryRejection` — the client-caused 400 vocabulary * - `VALIDATION_FAILED` — the per-field 400 envelope * - * It is deliberately NOT "any 4xx". `mapDataError`'s final fallback degrades an - * error it recognised nothing about to an un-coded 400, and that bucket is - * where a genuine handler bug (a `TypeError`, say) lands — silencing it would - * be the mirror-image of the defect this fixes. + * It is deliberately NOT "any 4xx". [#5489] That used to be argued from + * `mapDataError`'s final fallback, which degraded an error it recognised + * nothing about to an UN-CODED 400 — the bucket a genuine handler bug (a + * `TypeError`, say) landed in, so a predicate widened to "any 4xx is expected" + * would have silenced it. That fallback is now {@link UNCLASSIFIED_FAULT}'s + * 500, which this predicate cannot treat as expected at all + * (`isExpectedDataStatus` names 502/503 and nothing else in the 5xx band), so + * the handler bug is loud STRUCTURALLY rather than by this sentence. The + * narrowness still matters for what remains in the un-coded 4xx band — the + * sandbox unwraps' business-rule 400s — and for the next author tempted to + * simplify the predicate down to a status range. * * [#4886] Every route catch now decides through this one function. Before, the * metadata family logged unconditionally — the designer's `?state=draft` probe diff --git a/packages/rest/src/rest-unclassified-fault-status.test.ts b/packages/rest/src/rest-unclassified-fault-status.test.ts new file mode 100644 index 0000000000..632a547fed --- /dev/null +++ b/packages/rest/src/rest-unclassified-fault-status.test.ts @@ -0,0 +1,255 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5489] An error `mapDataError` can attribute to NOTHING is a server fault. +// +// The mapper's terminal branch — reached only after every `code` match, every +// declared-status passthrough and every message-text heuristic has declined — +// answered `{ status: 400, body: { error: } }`. Both halves +// were wrong in the same direction, and the two failures compound: +// +// 400 is "you sent a bad request". An SDK, a browser fetch wrapper, a proxy +// and a retry policy all read it as "do not retry, the caller must change +// something". The errors that actually land here are the opposite kind: +// `matchEndpoint` throwing because its metadata store cannot be read (it +// throws precisely so an outage does not masquerade as an empty declaration +// set — ADR-0110 D3), or a handler `TypeError`. Those are faults the caller +// cannot fix and SHOULD retry. Measured on `GET /api/v1/meta/api` against a +// store that throws `Error('metadata store unreachable')`: HTTP 400. +// +// The raw message shipped verbatim, on the one path with the least evidence +// that it is safe to ship — this branch is reached BECAUSE +// `looksLikeInternalErrorLeak` matched nothing, and #5462 already recorded +// that a keyword heuristic declining is not evidence of safety. The census +// below is the proof: a producer declaring `status: 502` with the message +// `connect ECONNREFUSED 10.0.0.5:5432 (internal pool)` reached a client as a +// 400 carrying host and port, because `mapDataError`'s own passthrough is +// 4xx-only and the heuristics do not know that phrasing. +// +// --------------------------------------------------------------------------- +// What did NOT move — the regression surface, mapped before the change +// --------------------------------------------------------------------------- +// The risk on this issue was misclassifying a REAL client error as a fault, so +// the question "who depends on the terminal fallback for their 400?" was +// answered empirically before editing: the branch was instrumented to record +// every error reaching it, and the whole `@objectstack/rest` suite (48 files, +// 719 tests) was run. Six errors reached it, and not one is a client error: +// +// `metadata store unreachable` (this issue's outage) +// `connect ECONNREFUSED 10.0.0.5:5432 (internal pool)` status 502, ×2 +// `Cannot read properties of undefined (reading 'name')` TypeError +// `boom` TypeError, ×2 +// +// That matches the static reading: reaching here requires no declared 4xx +// status, none of the ~12 recognised `code`/`name` values, no `innerMessage`, +// and a message matching none of the sandbox / provisioning / record-not-found +// / unknown-column / not-null / missing-relation / unknown-object / SQL-leak +// limbs. The one family that used to ride this branch as a genuine client error +// — driver-sql's uncompilable-filter refusals — was moved off it at the +// PRODUCER by #4436, which is why `sql-driver.ts` now declares `status: 400` + +// `INVALID_FILTER` instead. Contract-first, and the reason nothing 4xx-shaped +// is left here to break. +// +// The `describe` block at the bottom pins that boundary directly: every real +// client error still gets its 4xx, from its own branch. +// +// --------------------------------------------------------------------------- +// Reverse verification, direction predicted BEFORE running +// --------------------------------------------------------------------------- +// Ordinary red: restoring `return { status: 400, body: { error: raw || 'Bad +// request' } };` turns every case in the first two describes RED (they assert +// 500 / `INTERNAL_ERROR` / a withheld message), and leaves the whole "real 4xx +// is untouched" block GREEN — that block exists to catch the opposite +// overreach, a fix that starts promoting client mistakes to server faults. +// Confirmed by running it; see the PR. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { mapDataError, RestServer } from './rest-server'; + +const META_ITEM = '/api/v1/meta/:type/:name'; + +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', endpoints: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([]), + 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, protocol }; +} + +async function callMetaItem(rest: any, params: any) { + const res = makeRes(); + const route = rest.getRoutes().find((r: any) => r.method === 'GET' && r.path === META_ITEM); + if (!route) throw new Error(`GET ${META_ITEM} route not registered`); + await route.handler({ method: 'GET', params, query: {}, headers: {} }, res); + return res; +} + +let errorSpy: ReturnType; +beforeEach(() => { errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); }); +afterEach(() => { errorSpy.mockRestore(); }); + +/** Everything the error channel printed, flattened for substring searching. */ +const loggedText = () => errorSpy.mock.calls.map((c) => JSON.stringify(c.map(String))).join('\n'); + +// --------------------------------------------------------------------------- +// The unit: mapDataError's terminal branch +// --------------------------------------------------------------------------- + +describe('[#5489] an unattributable error maps to a sanitised 500, not a 400', () => { + it('the store-outage error this issue was raised on is a server fault', () => { + const r = mapDataError(new Error('metadata store unreachable')); + + expect(r.status).toBe(500); + expect(r.body).toEqual({ error: INTERNAL_ERROR_MESSAGE, code: 'INTERNAL_ERROR' }); + }); + + it('a handler bug lands in the same envelope — nothing here is data-specific', () => { + const r = mapDataError(new TypeError('x.map is not a function'), 'showcase_account'); + + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + // No `object` key: the branch is reached precisely because nothing + // attributed the failure to the caller's object, so naming it would be + // the same over-claim `DATABASE_ERROR` would be. + expect(r.body.object).toBeUndefined(); + }); + + it('the message is WITHHELD, not truncated — this branch has no evidence it is safe', () => { + const r = mapDataError(new Error('pool exhausted for dsn postgres://svc:hunter2@10.0.0.5/app')); + + expect(String(r.body.error)).toBe(INTERNAL_ERROR_MESSAGE); + expect(JSON.stringify(r.body)).not.toContain('hunter2'); + expect(JSON.stringify(r.body)).not.toContain('10.0.0.5'); + }); + + it('an error with no message at all still answers the same envelope', () => { + // The old branch degraded this to `{ error: 'Bad request' }`, which was + // the least informative 400 in the file and still told the caller they + // were at fault. + expect(mapDataError(new Error(''))).toEqual({ + status: 500, + body: { error: INTERNAL_ERROR_MESSAGE, code: 'INTERNAL_ERROR' }, + }); + expect(mapDataError(undefined).status).toBe(500); + }); + + it('`INTERNAL_ERROR`, not `DATABASE_ERROR` — this branch cannot name a cause', () => { + // `DATA_STORE_FAULT`'s `DATABASE_ERROR` is emitted where the evidence + // NAMES a store failure (a driver's missing-relation phrasing, a + // `looksLikeInternalErrorLeak` hit). Here the defining fact is the + // absence of evidence, and a handler `TypeError` returned as + // `DATABASE_ERROR` points an operator at a database that is fine. + // `INTERNAL_ERROR` is `standardErrorCodeForHttpStatus(500)` — the + // catalog's own floor for "500 with no more specific code", not a third + // vocabulary — and the message is the constant the declared-5xx branch + // of `resolveErrorResponse` already emits. + expect(mapDataError(new Error('who knows')).body.code).toBe('INTERNAL_ERROR'); + expect(mapDataError(new Error('no such table: sys_metadata')).body.code).toBe('DATABASE_ERROR'); + expect(mapDataError(new Error('no such table: sys_metadata')).body.error).toBe('Internal data error'); + }); +}); + +// --------------------------------------------------------------------------- +// Walked through a real route, in-process +// --------------------------------------------------------------------------- + +describe('[#5489] the metadata route answers 5xx for an unreadable store', () => { + it('GET /meta/:type reports a fault the caller may retry, and logs the words', async () => { + const outage = new Error('metadata store unreachable'); + const { rest } = setup({ getMetaItem: vi.fn().mockRejectedValue(outage) }); + + const res = await callMetaItem(rest, { type: 'api', name: 'showcase_task_feed' }); + + expect(res.statusCode).toBe(500); + expect(res.body).toEqual({ error: INTERNAL_ERROR_MESSAGE, code: 'INTERNAL_ERROR' }); + // The other half of withholding: an operator can still read it. 500 is + // outside `isExpectedDataStatus`, so `handleRouteError` prints the + // whole error object. + expect(loggedText()).toContain('metadata store unreachable'); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// The boundary: every real client error keeps its 4xx, from its own branch +// --------------------------------------------------------------------------- + +describe('[#5489] real client errors are untouched — none of them rode the fallback', () => { + it('a declared 4xx status still passes through with its own message and code', () => { + const r = mapDataError( + Object.assign(new Error('Unsupported filter operator "$bogusop" on field "title"'), { + status: 400, code: 'INVALID_FILTER', + }), + 'showcase_task', + ); + + // driver-sql's refusals (#4436) — the one family that historically rode + // the terminal fallback, moved to the producer before this change. + expect(r.status).toBe(400); + expect(r.body.code).toBe('INVALID_FILTER'); + expect(String(r.body.error)).toContain('$bogusop'); + }); + + it.each([ + ['VALIDATION_FAILED', { code: 'VALIDATION_FAILED', fields: [] }, 400], + ['INVALID_FIELD', { code: 'INVALID_FIELD', field: 'nope' }, 400], + ['PERMISSION_DENIED', { code: 'PERMISSION_DENIED' }, 403], + ['FEEDS_DISABLED', { code: 'FEEDS_DISABLED' }, 403], + ['RECORD_NOT_ACCESSIBLE', { code: 'RECORD_NOT_ACCESSIBLE' }, 403], + ['OBJECT_NOT_FOUND', { code: 'OBJECT_NOT_FOUND' }, 404], + ['RECORD_NOT_FOUND', { code: 'RECORD_NOT_FOUND' }, 404], + ['DELETE_RESTRICTED', { code: 'DELETE_RESTRICTED' }, 409], + ['CONCURRENT_UPDATE', { code: 'CONCURRENT_UPDATE' }, 409], + ])('%s keeps its status from its own branch', (_name, props, status) => { + const r = mapDataError(Object.assign(new Error('something the caller can fix'), props), 'acct'); + expect(r.status).toBe(status); + expect(r.body.code).toBe((props as any).code); + }); + + it('the message-text client errors keep their 400s too', () => { + // These have no `code` and no `status` — they are judged by phrasing, + // and they are the reason the terminal branch could not simply be + // "anything un-coded is a fault". + expect(mapDataError(new Error('table acct has no column named ghost'), 'acct').status).toBe(400); + expect(mapDataError(new Error('NOT NULL constraint failed: acct.name'), 'acct').status).toBe(400); + expect(mapDataError(new Error("hook 'beforeInsert' threw: Error: 删除被阻断"), 'acct').status).toBe(400); + expect(mapDataError(Object.assign(new Error('wrapped'), { innerMessage: '删除被阻断' })).status).toBe(400); + }); + + it('a unique violation is still the 409 the UI keys on', () => { + const r = mapDataError( + new Error('SQLITE_CONSTRAINT: UNIQUE constraint failed: acct.email'), + 'acct', + ); + expect(r.status).toBe(409); + expect(r.body.code).toBe('UNIQUE_VIOLATION'); + }); +}); diff --git a/packages/rest/src/rest-unknown-object-heuristic.test.ts b/packages/rest/src/rest-unknown-object-heuristic.test.ts index 243fce89b5..00fc325cfc 100644 --- a/packages/rest/src/rest-unknown-object-heuristic.test.ts +++ b/packages/rest/src/rest-unknown-object-heuristic.test.ts @@ -212,6 +212,10 @@ describe('[#5462] sys_metadata unavailable is a fault, not a missing object', () // falling through to `looksLikeInternalErrorLeak`: it contains no // `sqlite_`, no SQLSTATE, no statement prefix and no constraint dump, // so the terminal fallback would have shipped it verbatim as a 400. + // ([#5489] that fallback is a sanitised 500 now, so today the same + // message would at least land in the right band — but un-attributed, + // as `INTERNAL_ERROR` rather than this branch's `DATABASE_ERROR`. The + // limb below is what makes the verdict say "database".) const rest = await bootRealProtocol(PG_NO_RELATION); const res = await callRoute(rest, 'PUT', META_ITEM, { diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index a1917c8430..d34d4d7858 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -2320,6 +2320,14 @@ describe('mapDataError — schema/constraint envelopes', () => { // 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(String(r.body.error)).not.toContain('ECONNREFUSED'); }); // [#5423] This case used to assert the oversized message became the literal