fix(rest): 未分类的路由错误回消毒 5xx,不再把服务端故障说成 400 客户端错误 (#5489)

.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`(契约优先),那是唯一仍会把措辞交给调用方的路径。

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

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');
     });
 });
 

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

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

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,20 +175,28 @@ 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) });
 
         const res = await callMetaItem(rest, { type: 'object', name: 'showcase_account' });
 
         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');
     });
 });
 

packages/rest/src/rest-server.ts

@@ -436,6 +436,59 @@ const DATA_STORE_FAULT = (): { status: number; body: Record<string, unknown> } =
     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: <the raw message> }`. 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<string, unknown> } => ({
+    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<string, unknown> | 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