fix(analytics,rest)!: read-scope 拒收改判 READ_SCOPE_COMPILE_FAILED/500,路由的 message 正则名单完全退休

.changeset/analytics-read-scope-compile-failed-500.md

@@ -0,0 +1,83 @@
+---
+"@objectstack/service-analytics": patch
+"@objectstack/rest": patch
+"@objectstack/spec": patch
+---
+
+fix(analytics,rest)!: an RLS read-scope lowering failure is a `500`, not the caller's `400` — and its policy detail no longer reaches the response (#5367)
+
+**Observable behaviour change — read this if you alert, retry, or assert on status.**
+A request whose dataset carries an RLS read scope that `read-scope-sql.ts` cannot
+lower used to answer `400 DATASET_INVALID` with the refusal message echoed
+verbatim. It now answers `500 ANALYTICS_QUERY_FAILED` with the message withheld
+(`"Internal server error"`); the full text goes to the server log. Monitoring that
+counted these as client errors will see a 4xx disappear and a 5xx appear, and a
+client retrying on 5xx will now retry a request that cannot succeed until an
+administrator fixes the policy. Both follow from the correction below and are
+stated rather than buried.
+
+## What was wrong
+
+These ten fail-closed refusals were the last family `/analytics/dataset/query`
+classified by **prose** — the final entry of the hardcoded message-substring list
+#5352 introduced, which #5367's first PR had already shrunk from six entries to
+one. Two defects in one verdict:
+
+- **Misattribution.** `compileScopedFilterToSql(filter, alias)` receives an RLS
+  `FilterCondition` the security service compiled from an **administrator's**
+  sharing rule / permission set, and a join alias the **dataset compiler**
+  generated. Neither is caller input — the caller's own predicate goes through
+  `filter-normalizer.ts` and has answered `INVALID_FILTER` / 400 since #5352. So
+  what can arrive here is a broken policy, or drift between two of our own
+  components (#5557's `$regex` was literally the second case). For this request's
+  caller both are a **server** fault; `400` told them to fix a request that was
+  never wrong and kept the real fault out of 5xx alerting.
+- **Disclosure.** A 400 echoed the message, so
+  `unsafe field identifier "secret_policy_field"` and
+  `unsupported operator "$regex" on "owner_email"` handed a tenant the field names
+  and comparands of the RLS policy governing them.
+
+The maintainer ruled on 2026-08-06 (option B on #5367's decision card; option A
+was `READ_SCOPE_INVALID` / 422, rejected because no consumer reads a code on this
+path, a 4xx misreports a condition the client cannot fix, and 422 would have left
+the disclosure question to be re-decided message by message).
+
+## What changed
+
+- `read-scope-sql.ts` gains a module-local `readScopeCompileError` — the twin of
+  `filter-normalizer.ts`'s `invalidFilterError`, and likewise **the only way the
+  module refuses**. All ten sites carry `READ_SCOPE_COMPILE_FAILED` / **500**.
+  `:104`'s alias-vs-field split (option C on the card) collapses under B: both
+  branches answer the same verdict, pinned so the collapse is a recorded decision.
+- `rest-server.ts` loses branch ② entirely. **The message-sniffing mechanism is
+  fully retired** — nothing in this catch reads prose any more, and #5367's
+  Prime-Directive-#12 retirement schedule ("declared, loud, tested AND removable
+  on a schedule") is paid off.
+- The route's 5xx branch now withholds the message of any producer that
+  **declares** a server fault (`status >= 500` with a `code`). This was needed
+  rather than inherited: `looksLikeInternalErrorLeak` (#3867/#5520) is a heuristic
+  over SQL/driver *phrasing*, and measured, every read-scope message returns
+  `false` from it — so retiring the list alone would have moved the policy content
+  from a 400 body into a 500 body instead of out of the response. Teaching that
+  heuristic to recognise `[read-scope-sql]` would have been *more* message
+  sniffing, so the rule keys on the ADR-0112 envelope instead. **Undeclared** 5xx
+  errors keep #5667's tiering, so a self-authored fault ("no strategy can handle
+  query …") stays readable.
+- `READ_SCOPE_COMPILE_FAILED` is registered in `ERROR_CODE_LEDGER` under
+  `@objectstack/service-analytics` (ADR-0112 D3) and typed as
+  `RegisteredErrorCode` at the constructor, so an unregistered code is a compile
+  error. It is legible on the wire through the sibling `/analytics/query` exit,
+  which puts a thrown `err.code` in `error.details.code` (#3842).
+
+**Which inputs are refused did not change.** No refusal condition moved: nothing
+that used to lower now throws, and nothing that used to throw now lowers. That is
+pinned input-by-input — refusals *and* accepted read scopes with their compiled
+SQL and bind params — in `read-scope-refusal-envelope.test.ts`, which is green both
+before and after; only the envelope assertions move.
+
+Coverage: `read-scope-refusal-envelope.test.ts` (service-analytics) drives all ten
+sites through the real compiler; `analytics-read-scope-refusal-envelope.test.ts`
+(rest) drives five policy shapes end-to-end through a real `AnalyticsService`,
+asserting the 500, that the body contains no policy detail, and that the withheld
+text is present in the log — plus a positive control and both sides of the
+declared-vs-undeclared withhold.

content/docs/references/api/error-code-ledger.mdx

@@ -270,6 +270,7 @@ const result = ErrorCode.parse(data);
 * `PROJECT_PROVISIONING`
 * `PROJECT_PROVISIONING_FAILED`
 * `RAW_SQL_UNSUPPORTED`
+* `READ_SCOPE_COMPILE_FAILED`
 * `RECORD_GONE`
 * `RECORD_LOCKED`
 * `REPORTS_LIST_FAILED`

packages/rest/src/analytics-dataset-dimension-gate.test.ts

@@ -281,7 +281,7 @@ describe('[#5520] the 500 body no longer ships driver internals', () => {
     expect(String(res.body.error)).toMatch(/no strategy can handle query/);
   });
 
-  it('does not disturb the 4xx branches #5352 and #5367 own', async () => {
+  it('does not disturb the classification branches #5352 and #5367 own', async () => {
     // ① a producer-declared 4xx envelope still passes through with its own code…
     const enveloped = Object.assign(new Error('Unsupported filter operator "$sortOf" on "stage".'), {
       code: 'INVALID_FILTER',
@@ -318,15 +318,28 @@ describe('[#5520] the 500 body no longer ships driver internals', () => {
     expect(b.body.code).toBe('DATASET_INVALID');
     expect(String(b.body.message)).toMatch(/not declared in the dataset/);
 
-    // …③ and the ONE message-list entry #5367 deliberately left in place still
-    // answers 400 for `read-scope-sql`'s bare fail-closed refusals.
+    // …③ RE-JUDGED. This half asserted that the one remaining message-list entry
+    // still answered 400 for `read-scope-sql`'s bare refusals. The maintainer
+    // ruled on 2026-08-06 that the family is a SERVER fault: it now declares
+    // `READ_SCOPE_COMPILE_FAILED` / 500, the list is deleted, and — because the
+    // messages name RLS policy fields — the 5xx branch withholds the text. So
+    // what this half now guards is that #5520's `looksLikeInternalErrorLeak`
+    // withhold and #5367's declared-server-fault withhold COMPOSE rather than
+    // fight: same 500 code, message withheld, log intact.
     const c = await post(
       buildRoute(async () =>
-        throwingAnalytics(new Error('[read-scope-sql] unsupported operator "$regex" on "owner" (fail-closed).')),
+        throwingAnalytics(
+          Object.assign(
+            new Error('[read-scope-sql] unsupported operator "$regex" on "owner_email" (fail-closed).'),
+            { code: 'READ_SCOPE_COMPILE_FAILED', status: 500 },
+          ),
+        ),
       ),
       { dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } },
     );
-    expect(c.statusCode).toBe(400);
-    expect(c.body.code).toBe('DATASET_INVALID');
+    expect(c.statusCode).toBe(500);
+    expect(c.body.code).toBe('ANALYTICS_QUERY_FAILED');
+    expect(c.body.error).toBe(INTERNAL_ERROR_MESSAGE);
+    expect(String(c.body.error)).not.toMatch(/owner_email/);
   });
 });

packages/rest/src/analytics-filter-refusal-envelope.test.ts

@@ -32,11 +32,13 @@
  * Reading the envelope makes this route classify on what the error SAYS about
  * itself. Three regressions would each be worse than the bug:
  *
- *   1. The message list still classifies the ONE family that remains a bare
- *      `Error` — `read-scope-sql` — so deleting its entry would regress it from
- *      `400 DATASET_INVALID` to 500. (#5352 left six entries here. #5367
- *      enveloped five of the six producers and deleted their entries; the block
- *      near the bottom of this file now pins the deletion in both directions.)
+ *   1. ~~The message list still classifies the families that carry no envelope.~~
+ *      **Retired.** #5352 left six entries here; #5367 enveloped all six
+ *      producers and deleted every entry — five as `DATASET_INVALID` / 400 and,
+ *      after the maintainer's 2026-08-06 ruling, `read-scope-sql`'s ten as
+ *      `READ_SCOPE_COMPILE_FAILED` / 500. The block near the bottom of this file
+ *      pins each deletion in both directions, which is now the only thing
+ *      standing between this catch and a fresh message test.
  *   2. A genuine internal fault must still be a 500 with its `logError` line —
  *      "read the envelope" must not become "call everything a 400".
  *   3. A 5xx-status error is NOT passed through, so an internal fault can never
@@ -46,6 +48,7 @@
 import { describe, it, expect, vi } from 'vitest';
 import type { Logger } from '@objectstack/spec/contracts';
 import { AnalyticsService } from '@objectstack/service-analytics';
+import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types';
 import { RestServer } from './rest-server';
 
 // ── harness ──────────────────────────────────────────────────────────────────
@@ -269,24 +272,44 @@ describe('[#5322] empty combinators are boolean identities at the REST face —
   }
 });
 
-describe('[#5352 → #5367] the message-sniffing fallback is down to its last entry', () => {
-  // ── The surviving entry ────────────────────────────────────────────────────
-  // `read-scope-sql.ts`'s ten refusals are still bare `Error`s, and #5367
-  // deliberately left them that way: their inputs are an admin-authored RLS
-  // policy and a compiler-generated join alias, not caller input, so
-  // `DATASET_INVALID` may well be the wrong verdict for them and choosing the
-  // right one is a separate judgement. Until it lands, this entry is all that
-  // stands between them and a 500.
-  it('read-scope-sql: a fail-closed read scope → still 400 DATASET_INVALID by the message list', async () => {
-    const message = '[read-scope-sql] unsupported operator "$regex" on "owner" (fail-closed).';
+describe('[#5352 → #5367] the message-sniffing fallback is GONE', () => {
+  // ── The last entry, retired ────────────────────────────────────────────────
+  // ⚠️ RE-JUDGED. This case used to read "read-scope-sql: a fail-closed read
+  // scope → still 400 DATASET_INVALID by the message list", and the comment
+  // above it said the verdict for that family was a separate judgement still
+  // pending. The maintainer made it on 2026-08-06 (option B on #5367's decision
+  // card): the ten refusals are a SERVER fault, they now declare
+  // `READ_SCOPE_COMPILE_FAILED` / 500 themselves, and the list is deleted.
+  //
+  // So the same input is asserted the other way round — and the bare form, which
+  // is what the list used to rescue, is asserted too. Between them they pin that
+  // no message test survives anywhere in this catch.
+  it('read-scope-sql: the DECLARED 500 → 500 ANALYTICS_QUERY_FAILED, policy content withheld', async () => {
+    const message = '[read-scope-sql] unsupported operator "$regex" on "owner_email" (fail-closed).';
+    const err = Object.assign(new Error(message), { code: 'READ_SCOPE_COMPILE_FAILED', status: 500 });
+    const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(err) }));
+    const res = await post(route, { dataset, selection });
+    expect(res.statusCode).toBe(500);
+    expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED');
+    // The disclosure half: an RLS policy's field name must not come back.
+    expect(String(res.body.error)).not.toMatch(/owner_email/);
+    expect(String(res.body.error)).not.toMatch(/read-scope-sql/);
+  });
+
+  it('read-scope-sql: the same message BARE is not sniffed either (500, and readable)', async () => {
+    // Bare = no producer declaration. It still lands on 500 because the list is
+    // gone, not because anything recognised its prose; and it keeps #5667's
+    // tiering, so an undeclared fault stays readable. That difference is the
+    // point of making the withhold depend on the DECLARATION.
+    const message = '[read-scope-sql] unsupported operator "$regex" on "owner_email" (fail-closed).';
     const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(new Error(message)) }));
     const res = await post(route, { dataset, selection });
-    expect(res.statusCode).toBe(400);
-    expect(res.body.code).toBe('DATASET_INVALID');
-    expect(String(res.body.message)).toMatch(/read-scope-sql/);
+    expect(res.statusCode).toBe(500);
+    expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED');
+    expect(String(res.body.error)).toMatch(/read-scope-sql/);
   });
 
-  // ── The five entries #5367 deleted, pinned in BOTH directions ──────────────
+  // ── The five entries #5367's first PR deleted, pinned in BOTH directions ────
   // These rows used to assert "a bare `Error` with this message → 400", which is
   // precisely the fragility #5367 removed: the status was a property of the
   // wording. Re-asserting it would now be asserting the defect. So each family
@@ -357,7 +380,7 @@ describe('[#5352] reading the envelope did not turn every failure into a 400', (
     expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED');
   });
 
-  it('a 5xx-status error is NOT passed through — an internal fault keeps the 500 envelope', async () => {
+  it('a 5xx-status error is NOT passed through — an internal fault keeps the 500 envelope, message withheld', async () => {
     // Deliberate asymmetry: the passthrough is 4xx-only, so a producer cannot
     // re-label a server fault with a code of its own and slip past the
     // `logError` line that makes it visible to operators.
@@ -370,6 +393,12 @@ describe('[#5352] reading the envelope did not turn every failure into a 400', (
 
     expect(res.statusCode).toBe(500);
     expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED');
+    // [#5367] Second half of the asymmetry, added with the read-scope ruling: a
+    // producer that DECLARES a server fault has declared that the detail is the
+    // operator's, so the message is withheld here and kept in `logError`. This
+    // case is the generic form of the rule the RLS lowering needed — it applies
+    // to any declared 5xx, not to a list of recognised phrasings.
+    expect(res.body.error).toBe(INTERNAL_ERROR_MESSAGE);
   });
 
   it('a HALF envelope (4xx status, no code) is not honoured — this route invents no code', async () => {