diff --git a/.changeset/analytics-query-declared-fault-withhold.md b/.changeset/analytics-query-declared-fault-withhold.md new file mode 100644 index 0000000000..bcd5662951 --- /dev/null +++ b/.changeset/analytics-query-declared-fault-withhold.md @@ -0,0 +1,87 @@ +--- +"@objectstack/types": minor +"@objectstack/runtime": minor +"@objectstack/rest": patch +--- + +fix(runtime,types)!: `/analytics/query` no longer echoes RLS policy field names — the declared-server-fault withhold is shared by both HTTP boundaries (#5811) + +**Observable behaviour change — read this if you read, log, or assert on +`error.message` from a dispatcher-plugin route.** An error that **declares a +server fault** in the ADR-0112 envelope (`status >= 500` *and* a non-empty +`code`) now leaves `dispatcher-plugin.errorResponseBase` with its message +replaced by `"Internal server error"`. It previously reached the caller verbatim +unless it happened to *sound* like a SQL/driver dump. This applies to every route +that plugin mounts — `/analytics`, `/packages`, `/i18n`, `/automation`, `/auth`, +`/notifications`, `/mcp`, … — not only the one that motivated it. Nothing a +machine reads changed: the producer's `code` still arrives in the response +(`error.code`, promoted there from `details` by the shared envelope builder, +#3842), the status is untouched, and the full original text still goes to the +server log and `errorReporter` via `__obsRecordedError`. + +## What was wrong + +#5367 (maintainer ruling 2026-08-06) made `read-scope-sql.ts`'s ten fail-closed +RLS lowering refusals `READ_SCOPE_COMPILE_FAILED` / 500 and taught +`POST /analytics/dataset/query` to withhold their message, because those messages +name the field names and comparands of an **administrator's** sharing rule: + +``` +[read-scope-sql] unsafe field identifier "secret_policy_field" — refusing to +build read scope (fail-closed). +``` + +The caller never wrote that field name and must not be able to read it out of an +error body. But the **sibling** analytics face was never closed. +`compileScopedFilterToSql` runs on both `NativeSQLStrategy.applyReadScope` and +`ObjectQLStrategy`'s echoed SQL, both of which serve `POST /analytics/query`, +which exits through `dispatcher-plugin.errorResponseBase`. That exit's only +message guard was `looksLikeInternalErrorLeak` — a heuristic over SQL/driver +*phrasing* — and all eleven read-scope message shapes return `false` from it. +Measured at that boundary: **11 of 11 echoed verbatim**, at 500, with the policy +content in `error.message`. A real reachable disclosure, not a theoretical one. + +## What changed + +- **`@objectstack/types` gains `declaresServerFault(err)`**, exported from + `error-leak.ts` beside `looksLikeInternalErrorLeak`. The heuristic asks whether + a message *sounds* internal; the declaration asks whether the producer *said + so*. `error-leak.ts`'s own file header already states the principle — "do not + ship driver internals to clients" is a property of the HTTP boundary, not of + one router — and this is the second predicate that principle asks for. +- **Both boundaries read it.** `dispatcher-plugin.errorResponseBase` gains the + withhold (the fix); `rest-server.ts`'s `/analytics/dataset/query` catch drops + its in-line copy of the same test in favour of the shared one. #5808 wrote that + rule in-line on purpose — promoting a rule with one consumer is a speculative + surface — and this is the second consumer, so it was promoted rather than + duplicated (`#3843`/`#3867` paid for the two-implementations shape twice). + The REST face's verdict is unchanged in every case: same `status >= 500` plus + non-empty `code` test, over the same two fields. + +## What deliberately did NOT change + +- ⛔ **This is not "withhold every 5xx".** #5667 kept **undeclared** 5xx errors + legible on purpose: a bare `Error` from our own code ("no strategy can handle + query …") is the operator's own bug report, names nothing tenant-sensitive, and + still falls to `looksLikeInternalErrorLeak` alone. A 5xx carrying only half an + envelope (a status with no code) is likewise still readable — inventing the + withhold for it would be the consumer-side leniency Prime Directive #12 removes. +- **4xx is untouched.** `declaresServerFault` requires `status >= 500`, so a + deliberate business/validation answer can never be swallowed by it. +- **`statusCode` is not accepted as a substitute for `status`.** `status` is the + channel ADR-0112 declares; making a disclosure rule depend on which spelling a + producer reached for would be the same leniency in a different place. +- **The heuristic was not taught to recognise `[read-scope-sql]`.** That would be + more prose sniffing — the mechanism #5352/#5367 exist to remove — and would only + ever cover the family someone remembered to add. + +Coverage: `analytics-query-read-scope-withhold.test.ts` (runtime) drives six RLS +policy shapes end-to-end through a **real** `AnalyticsService` on the real +native-SQL path and the real mounted route, asserting the 500, that the whole +serialized body contains no policy detail, that `error.code` still carries +`READ_SCOPE_COMPILE_FAILED`, and that the full text is still on the +`__obsRecordedError` side-channel — plus a positive control and both sides of the +declared-vs-undeclared tiering. `error-leak.test.ts` (types) pins the predicate +directly, including that all eleven read-scope shapes stay invisible to the +heuristic. The REST face's existing `analytics-read-scope-refusal-envelope.test.ts` +is green before and after, unchanged, which is the pin on the refactor. diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 9237dbcd1f..90b76329dd 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -4,7 +4,12 @@ import { IHttpServer, resolveAuthzContext, resolveLocalizationContext, isAuthGateAllowlisted, shouldDenyAnonymous, ANONYMOUS_DENY_BODY, ANONYMOUS_DENY_STATUS, } from '@objectstack/core'; -import { isMcpServerEnabled, looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { + isMcpServerEnabled, + looksLikeInternalErrorLeak, + declaresServerFault, + INTERNAL_ERROR_MESSAGE, +} from '@objectstack/types'; import { allowPerfDisclosure, isPerfDisclosurePrincipal } from '@objectstack/observability'; import { RouteManager } from './route-manager.js'; import { RestServerConfig, RestApiConfig, CrudEndpointsConfig, MetadataEndpointsConfig, BatchEndpointsConfig, RouteGenerationConfig } from '@objectstack/spec/api'; @@ -6979,10 +6984,21 @@ export class RestServer { // a bare `Error` still goes through the heuristic, so a // self-authored fault ("no strategy can handle query …") // stays readable. + // + // [#5811] That rule is no longer written here. The sibling + // face — `/analytics/query`, exiting through + // `dispatcher-plugin.errorResponseBase` — had the identical + // leak (measured: 11/11 read-scope messages echoed verbatim), + // so the criterion was promoted to `declaresServerFault` in + // `@objectstack/types`, beside the heuristic it complements, + // and both boundaries read it. #5808 deliberately left it + // in-line while there was one consumer; this is the second. + // The verdict here is unchanged in every case — the predicate + // is the same `status >= 500` + non-empty `code` test, reading + // the same two fields ① derives `envelopeStatus`/`envelopeCode` + // from. logError('[REST] Analytics dataset query error:', error); - const declaredServerFault = - envelopeStatus !== undefined && envelopeStatus >= 500 && envelopeCode !== undefined; - const outward = declaredServerFault || looksLikeInternalErrorLeak(msg) + const outward = declaresServerFault(error) || looksLikeInternalErrorLeak(msg) ? INTERNAL_ERROR_MESSAGE : msg.slice(0, 500); res.status(500).json({ code: 'ANALYTICS_QUERY_FAILED', error: outward }); diff --git a/packages/runtime/package.json b/packages/runtime/package.json index 38693fffff..e5e0ca55b2 100644 --- a/packages/runtime/package.json +++ b/packages/runtime/package.json @@ -47,6 +47,7 @@ "devDependencies": { "@objectstack/platform-objects": "workspace:*", "@objectstack/plugin-hono-server": "workspace:*", + "@objectstack/service-analytics": "workspace:*", "@objectstack/service-datasource": "workspace:*", "@objectstack/service-job": "workspace:*", "@objectstack/service-messaging": "workspace:*", diff --git a/packages/runtime/src/analytics-query-read-scope-withhold.test.ts b/packages/runtime/src/analytics-query-read-scope-withhold.test.ts new file mode 100644 index 0000000000..c0be935beb --- /dev/null +++ b/packages/runtime/src/analytics-query-read-scope-withhold.test.ts @@ -0,0 +1,286 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5811] `POST /analytics/query` — a read-scope lowering failure reaches the + * caller as a 500 **with the RLS policy withheld**, and the full text reaches the + * operator's log / error reporter. + * + * ## What this closes + * + * #5367 (maintainer ruling 2026-08-06) made `read-scope-sql.ts`'s ten fail-closed + * RLS refusals `READ_SCOPE_COMPILE_FAILED` / 500, and taught + * `/analytics/dataset/query` — served by `@objectstack/rest` — to withhold the + * message of any producer that DECLARES a server fault. The reason was a + * disclosure: those messages name the field names and comparands of the sharing + * rule the tenant is being filtered by. + * + * ``` + * [read-scope-sql] unsafe field identifier "secret_policy_field" — refusing to + * build read scope (fail-closed). + * ``` + * + * The caller never wrote that field name — an administrator did, and the security + * service compiled it. But the SIBLING analytics face was never closed. Both + * `NativeSQLStrategy.applyReadScope` and `ObjectQLStrategy`'s echoed SQL run + * `compileScopedFilterToSql`, and both serve `/analytics/query`, which exits + * through `dispatcher-plugin.errorResponseBase`. That exit's only message guard + * was `looksLikeInternalErrorLeak` — a heuristic over SQL/driver PHRASING — and + * measured, all eleven read-scope message shapes return FALSE from it. #5811 + * measured the result at this boundary: **11/11 echoed verbatim**, at 500, with + * the policy content in `error.message`. + * + * ## Why the rule is DECLARED, not sniffed + * + * Teaching the heuristic to recognise `[read-scope-sql]` would be more message + * sniffing — the mechanism #5352/#5367 exist to remove — and it would only ever + * cover the family someone remembered to add. So #5808's in-line criterion was + * promoted to `declaresServerFault` in `@objectstack/types`, beside the heuristic + * it complements, and BOTH boundaries read it. One rule, one implementation + * (#3843/#3867 paid for the alternative twice). + * + * ⛔ It is NOT "withhold every 5xx". #5667 deliberately kept UNDECLARED 5xx + * legible — a bare `Error` from our own code is the operator's own bug report and + * names nothing tenant-sensitive. That tiering is pinned below, because "the + * withhold got broader" and "the withhold swallowed everything" are one edit + * apart. + * + * ## Why this file boots the REAL analytics service + * + * Producer and boundary are different facts, and a hand-written `Object.assign(new + * Error(...), { status: 500, code: '…' })` fixture proves only that the boundary + * withholds what the TEST declared. Here a real `AnalyticsService` compiles a real + * dataset with a real RLS scope on the real native-SQL path, so the error entering + * `errorResponseBase` is the one `compileScopedFilterToSql` actually threw — the + * same discipline `packages/rest`'s `analytics-read-scope-refusal-envelope.test.ts` + * applies to the sibling face. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { Logger } from '@objectstack/spec/contracts'; +import { AnalyticsService } from '@objectstack/service-analytics'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; + +import { createDispatcherPlugin } from './dispatcher-plugin.js'; + +// ── harness (the shape `dispatcher-plugin.error-envelope.test.ts` uses) ─────── + +function makeFakeServer() { + const handlers: Record any> = {}; + const rec = (verb: string) => (path: string, handler: any) => { + handlers[`${verb} ${path}`] = handler; + }; + return { + handlers, + server: { + get: rec('GET'), + post: rec('POST'), + put: rec('PUT'), + delete: rec('DELETE'), + patch: rec('PATCH'), + }, + }; +} + +function makeCtx(fakeServer: any, analytics: unknown) { + const kernel = { + getService: (name: string) => (name === 'analytics' ? analytics : undefined), + getServiceAsync: async (name: string) => (name === 'analytics' ? analytics : undefined), + }; + return { + getKernel: () => kernel, + getService: (name: string) => (name === 'http.server' ? fakeServer : undefined), + environmentId: undefined, + logger: { info() {}, warn() {}, error() {}, debug() {} }, + hook: () => {}, + on: () => {}, + } as any; +} + +function makeRes() { + const res: any = { + statusCode: undefined as number | undefined, + body: undefined as any, + status(c: number) { res.statusCode = c; return res; }, + header() { return res; }, + json(b: any) { res.body = b; return res; }, + }; + return res; +} + +/** Drive the REAL `POST /api/v1/analytics/query` route against `analytics`. */ +async function postAnalyticsQuery(analytics: unknown, body: unknown) { + const { server, handlers } = makeFakeServer(); + const plugin = createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false }); + await plugin.start?.(makeCtx(server, analytics)); + + const handler = handlers['POST /api/v1/analytics/query']; + expect(handler, 'POST /api/v1/analytics/query must be mounted').toBeTypeOf('function'); + + const res = makeRes(); + await handler({ body, query: {} }, res); + return res; +} + +const silent: Logger = { debug() {}, info() {}, warn() {}, error() {} }; + +/** + * A real `AnalyticsService` on the raw-SQL path whose read scope is `scope`. + * + * `getReadScope` is the seam an administrator's policy arrives through: in + * production it resolves to the security service's `getReadFilter`, i.e. to a + * sharing rule / permission set the tenant's admin authored. Handing it a shape + * `read-scope-sql.ts` cannot lower is exactly the live condition, and + * `NativeSQLStrategy.applyReadScope` calls it for the base table of every query. + */ +function analyticsWithScope(scope: unknown): AnalyticsService { + const svc = new AnalyticsService({ + logger: silent, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [{ stage: 'won', revenue: 100 }], + getReadScope: () => scope as never, + isRegisteredObject: () => true, + }); + // Registering the dataset publishes its Cube, which is what `query({ cube })` + // resolves — the `/analytics/query` face queries cubes by name. + svc.registerDataset(dataset as never); + return svc; +} + +const dataset = { + name: 'pipeline', + label: 'Pipeline', + object: 'crm_opportunity', + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}; +const query = { cube: 'pipeline', measures: ['revenue'], dimensions: ['stage'] }; + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5811] POST /analytics/query — a read-scope failure says nothing about the policy', () => { + let logSpy: ReturnType; + beforeEach(() => { logSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); }); + afterEach(() => { logSpy.mockRestore(); }); + + /** + * One row per shape of RLS policy the lowering refuses, each carrying a + * `secret` — the policy detail the message names and the response must not. + * Mirrors the sibling face's case table so the two boundaries are pinned + * against the same inputs. + */ + const CASES: Array<{ name: string; scope: unknown; secret: string }> = [ + { + name: 'a policy field name the identifier guard refuses', + scope: { 'secret policy field': 'u1' }, + secret: 'secret policy field', + }, + { + name: 'an operator the lowering cannot express', + scope: { owner_email: { $regex: 'admin@internal' } }, + secret: 'owner_email', + }, + { + name: 'a nested / relation value a flat read scope cannot join', + scope: { approved_by_manager: { manager_id: 'usr_ceo' } }, + secret: 'approved_by_manager', + }, + { + name: 'an $in whose comparand is not an array', + scope: { restricted_region: { $in: 'emea' } }, + secret: 'restricted_region', + }, + { + name: 'a combinator whose operand is not an array', + scope: { $and: { owner_id: 'u1' } }, + secret: '$and', + }, + { + name: 'a bare array value the lowering refuses to guess at', + scope: { classified_tier: ['red', 'amber'] }, + secret: 'classified_tier', + }, + ]; + + for (const c of CASES) { + it(`${c.name} → 500, body carries no policy detail`, async () => { + const res = await postAnalyticsQuery(analyticsWithScope(c.scope), query); + + // Classification: a server fault, and the producer's code survives. + expect(res.statusCode).toBe(500); + expect(res.body.success).toBe(false); + // [#3842] `errorResponseBase` puts `err.code` in `details`, and the + // shared builder (`buildApiError` → `splitSemanticCode`) then PROMOTES + // it into the declared `error.code` field, leaving `details` empty and + // therefore omitted. So the code a machine reads arrives at + // `error.code`, not `error.details.code` — asserted where it actually + // lands, since what matters is that the classification survives the + // withhold untouched. + expect(res.body.error.code).toBe('READ_SCOPE_COMPILE_FAILED'); + + // Disclosure: the policy detail is gone from the body — asserted over + // the WHOLE body, not just `message`, because a leak that moved to + // another key would still be a leak. + expect(res.body.error.message).toBe(INTERNAL_ERROR_MESSAGE); + const wire = JSON.stringify(res.body); + expect(wire).not.toContain(c.secret); + expect(wire).not.toMatch(/read-scope-sql/); + expect(wire).not.toMatch(/fail-closed/); + + // …and the untouched error still reaches the operator: the + // observability wrapper hands `__obsRecordedError` to `errorReporter`. + // Asserted rather than assumed — "withheld" is only acceptable + // because the full text is still somewhere. + const recorded = (res as any).__obsRecordedError; + expect(String(recorded?.message)).toContain('read-scope-sql'); + expect(String(recorded?.message)).toContain(c.secret); + expect(recorded?.code).toBe('READ_SCOPE_COMPILE_FAILED'); + }); + } + + it('POSITIVE control: a lowerable read scope still scopes the query → 200 with rows', async () => { + // Without this, every case above could pass for any reason that makes the + // route 500 — including the read scope never being consulted at all. + const res = await postAnalyticsQuery(analyticsWithScope({ organization_id: 'org_A' }), query); + expect(res.statusCode).toBe(200); + expect(res.body.success).toBe(true); + expect(res.body.data.rows).toEqual([{ stage: 'won', revenue: 100 }]); + }); + + it('an UNDECLARED 5xx keeps #5667 tiering — a self-authored fault stays readable', async () => { + // The other side of the declared-withhold rule, and the reason it is scoped + // to producers that DECLARE a server fault rather than to every 500: a bare + // `Error` from our own code is the operator's own bug report, carries + // nothing tenant-sensitive, and #5667 deliberately kept it legible. + // Widening the withhold to all 500s would delete that decision. + const res = await postAnalyticsQuery( + { query: async () => { throw new Error('[Analytics] no strategy can handle query for cube "pipeline"'); } }, + query, + ); + expect(res.statusCode).toBe(500); + expect(String(res.body.error.message)).toMatch(/no strategy can handle query/); + }); + + it('a 5xx with only HALF an envelope stays readable — a code is required, not just a status', async () => { + // Guards the predicate's second half at the boundary. A producer that + // ships a status without a code has not declared anything; inventing the + // withhold for it would be the consumer-side leniency PD #12 removes. + const err = Object.assign(new Error('analytics engine unavailable'), { status: 503 }); + const res = await postAnalyticsQuery({ query: async () => { throw err; } }, query); + expect(res.statusCode).toBe(503); + expect(res.body.error.message).toBe('analytics engine unavailable'); + }); + + it('a DECLARED 4xx is untouched — the withhold is 5xx-only', async () => { + // The message here names the caller's own typo and is theirs to read. + // `declaresServerFault` can never reach it (it requires status >= 500), + // and this pins that at the boundary rather than only in the unit test. + const err = Object.assign(new Error('Unsupported filter operator "$sortOf" on "stage".'), { + code: 'INVALID_FILTER', + status: 400, + }); + const res = await postAnalyticsQuery({ query: async () => { throw err; } }, query); + expect(res.statusCode).toBe(400); + expect(String(res.body.error.message)).toMatch(/\$sortOf/); + expect(res.body.error.code).toBe('INVALID_FILTER'); + }); +}); diff --git a/packages/runtime/src/dispatcher-plugin.ts b/packages/runtime/src/dispatcher-plugin.ts index 8da2a5f8df..9085c1f08c 100644 --- a/packages/runtime/src/dispatcher-plugin.ts +++ b/packages/runtime/src/dispatcher-plugin.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { Plugin, PluginContext, IHttpServer } from '@objectstack/core'; -import { looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { looksLikeInternalErrorLeak, declaresServerFault, INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; import { DispatcherErrorCode } from '@objectstack/spec/api'; import type { IAuthService, IMetadataService } from '@objectstack/spec/contracts'; import type { CounterStore } from '@objectstack/plugin-auth'; @@ -441,6 +441,32 @@ function sendResultBase( * the dispatcher can highlight the field the way a form served by /data can. * `details` is only emitted for that shape — everything else keeps the exact * two-key body it had. + * + * [#5811] A fourth, and the reason (2)'s "shared predicate" is now two of them. + * `looksLikeInternalErrorLeak` is a heuristic over SQL/driver PHRASING, so it + * closes this exit only against faults that *sound* like a driver. It never saw + * `service-analytics`' fail-closed read-scope refusals — measured, all eleven + * shapes return FALSE — and those messages name the field names and comparands of + * the RLS POLICY the tenant is being filtered by: + * + * ``` + * POST /analytics/query (tenant caller, object with a broken sharing rule) + * → 500 {"success":false,"error":{"message":"[read-scope-sql] unsafe field + * identifier \"secret_policy_field\" — refusing to build read scope + * (fail-closed).","code":"READ_SCOPE_COMPILE_FAILED"}} + * ``` + * + * The sibling face — `/analytics/dataset/query` in `@objectstack/rest` — closed + * this in #5367/#5808 by keying on the DECLARATION rather than the prose, but the + * rule was written in-line there because one consumer does not justify a shared + * surface. This exit is the second consumer, so it was promoted: + * {@link declaresServerFault}, next to the heuristic it complements, read by both + * boundaries. ⛔ It is NOT "withhold every 5xx" — #5667 kept UNDECLARED 5xx + * legible on purpose, and a bare `Error` still goes through the heuristic alone. + * + * The code still travels: `details.code` (#3842, below) carries + * `READ_SCOPE_COMPILE_FAILED` to the client untouched, so what a machine reads is + * unchanged and only the prose is withheld — into `errorReporter` and the log. */ function errorResponseBase(err: any, res: any, securityHeaders?: Record): void { const validation = validationFailureDetails(err); @@ -466,8 +492,12 @@ function errorResponseBase(err: any, res: any, securityHeaders?: Record= 500`, so it can never + // reach a 4xx answer the caller is entitled to read. const message = - httpStatus >= 500 && looksLikeInternalErrorLeak(raw) + declaresServerFault(err) || (httpStatus >= 500 && looksLikeInternalErrorLeak(raw)) ? INTERNAL_ERROR_MESSAGE : raw || 'Internal Server Error'; // [#3842] A thrown error's own `.code` finally has somewhere to go. This diff --git a/packages/types/src/error-leak.test.ts b/packages/types/src/error-leak.test.ts index 691b0f1e73..e84f29ef62 100644 --- a/packages/types/src/error-leak.test.ts +++ b/packages/types/src/error-leak.test.ts @@ -11,7 +11,27 @@ */ import { describe, it, expect } from 'vitest'; -import { looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from './error-leak.js'; +import { looksLikeInternalErrorLeak, declaresServerFault, INTERNAL_ERROR_MESSAGE } from './error-leak.js'; + +/** + * The eleven message shapes `service-analytics`' `read-scope-sql.ts` can refuse + * with, verbatim in structure (identifiers substituted for readable stand-ins). + * Shared by both describe blocks below, which is the point: they are the family + * the heuristic cannot see and the declaration can. + */ +const READ_SCOPE_REFUSALS = [ + '[read-scope-sql] unsafe field identifier "secret policy field" — refusing to build read scope (fail-closed).', + '[read-scope-sql] unsafe alias identifier "crm opportunity" — refusing to build read scope (fail-closed).', + '[read-scope-sql] read scope must be a filter object (fail-closed).', + '[read-scope-sql] "$and" requires an array (fail-closed).', + '[read-scope-sql] unsupported top-level operator "$nor" (fail-closed).', + '[read-scope-sql] bare array value for "restricted_region" — use { $in: [...] } (fail-closed).', + '[read-scope-sql] "approved_by_manager" has a nested/relation value which is not supported in a read scope (fail-closed).', + '[read-scope-sql] $in for "restricted_region" needs an array (fail-closed).', + '[read-scope-sql] $nin for "restricted_region" needs an array (fail-closed).', + '[read-scope-sql] $between for "deal_amount" needs [min,max] (fail-closed).', + '[read-scope-sql] unsupported operator "$regex" on "owner_email" (fail-closed).', +]; describe('looksLikeInternalErrorLeak', () => { it('catches the message that motivated #3867 (raw SQL from /analytics/query)', () => { @@ -55,4 +75,118 @@ describe('looksLikeInternalErrorLeak', () => { expect(INTERNAL_ERROR_MESSAGE).toBe('Internal server error'); expect(looksLikeInternalErrorLeak(INTERNAL_ERROR_MESSAGE)).toBe(false); }); + + /** + * [#5811] The measurement that motivated the second predicate, kept here as a + * pin rather than a paragraph. If someone later "helpfully" teaches the + * heuristic to recognise `[read-scope-sql]` (direction C, explicitly + * discouraged), this goes red and points at `declaresServerFault` instead. + */ + it.each(READ_SCOPE_REFUSALS)('does NOT recognise a read-scope refusal: %s', (message) => { + expect(looksLikeInternalErrorLeak(message)).toBe(false); + }); +}); + +/** + * [#5811] The declaration half. `looksLikeInternalErrorLeak` asks whether a + * message SOUNDS internal; this asks whether the producer SAID it was a server + * fault. The read-scope RLS refusals are the family that needs the second + * question — they carry policy field names while sounding like ordinary prose. + */ +describe('declaresServerFault', () => { + it('recognises the shape `read-scope-sql.ts` throws (500 + READ_SCOPE_COMPILE_FAILED)', () => { + const err = Object.assign( + new Error( + '[read-scope-sql] unsafe field identifier "secret_policy_field" — refusing to build read scope (fail-closed).', + ), + { code: 'READ_SCOPE_COMPILE_FAILED', status: 500 }, + ); + expect(declaresServerFault(err)).toBe(true); + // …and it is exactly the family the heuristic cannot see, which is why + // both predicates exist. + expect(looksLikeInternalErrorLeak(err.message)).toBe(false); + }); + + it.each(READ_SCOPE_REFUSALS)('covers every read-scope message shape: %s', (message) => { + const err = Object.assign(new Error(message), { + code: 'READ_SCOPE_COMPILE_FAILED', + status: 500, + }); + expect(declaresServerFault(err)).toBe(true); + }); + + it.each([ + ['503 with a code', 503, 'SERVICE_UNAVAILABLE'], + ['500 with a code', 500, 'INTERNAL_ERROR'], + ['599 with a code', 599, 'WEIRD_BUT_DECLARED'], + ])('is true for %s', (_label, status, code) => { + expect(declaresServerFault(Object.assign(new Error('x'), { status, code }))).toBe(true); + }); + + /** + * ⛔ The load-bearing half. #5667 deliberately kept UNDECLARED 5xx errors + * readable — a bare `Error` from our own code is the operator's own bug + * report and names nothing tenant-sensitive. Widening this predicate to "any + * 5xx" would delete that decision in one character, and these cases are what + * makes that edit fail. + */ + it.each([ + ['a bare Error with no envelope at all', new Error('no strategy can handle query for cube "pipeline"')], + [ + 'a 5xx status with NO code — half an envelope is not a declaration', + Object.assign(new Error('no strategy can handle query for cube "pipeline"'), { status: 500 }), + ], + [ + 'a 5xx status with an EMPTY code', + Object.assign(new Error('boom'), { status: 500, code: '' }), + ], + [ + 'a code with NO status', + Object.assign(new Error('boom'), { code: 'READ_SCOPE_COMPILE_FAILED' }), + ], + [ + 'a declared 4xx — the message is the caller\'s to read', + Object.assign(new Error('Unsupported filter operator "$sortOf" on "stage".'), { + status: 400, + code: 'INVALID_FILTER', + }), + ], + [ + 'a declared 404', + Object.assign(new Error("Cube 'ghost' not found"), { status: 404, code: 'CUBE_NOT_FOUND' }), + ], + [ + 'a non-numeric status', + Object.assign(new Error('boom'), { status: '500', code: 'INTERNAL_ERROR' }), + ], + [ + 'a non-string code', + Object.assign(new Error('boom'), { status: 500, code: 500 }), + ], + ])('is false for %s', (_label, err) => { + expect(declaresServerFault(err)).toBe(false); + }); + + /** + * Reads `status`, never `statusCode`. `status` is the channel ADR-0112 + * declares; accepting the alternate spelling would make a disclosure rule + * depend on which one a producer happened to reach for — the consumer-side + * leniency PD #12 removes. Pinned because "be a bit more tolerant" is the + * single most likely well-meant edit to this function. + */ + it('does not accept `statusCode` as a substitute for `status`', () => { + expect( + declaresServerFault(Object.assign(new Error('boom'), { statusCode: 500, code: 'INTERNAL_ERROR' })), + ).toBe(false); + }); + + it('is safe on anything a `catch` can actually receive', () => { + expect(declaresServerFault(undefined)).toBe(false); + expect(declaresServerFault(null)).toBe(false); + expect(declaresServerFault('a thrown string')).toBe(false); + expect(declaresServerFault(500)).toBe(false); + // A thrown plain object is a real shape in this repo (`throw { statusCode: 503, … }`). + expect(declaresServerFault({ status: 500, code: 'INTERNAL_ERROR' })).toBe(true); + expect(declaresServerFault({ statusCode: 503, message: 'Data service not available' })).toBe(false); + }); }); diff --git a/packages/types/src/error-leak.ts b/packages/types/src/error-leak.ts index df55b1ef02..f484a9960c 100644 --- a/packages/types/src/error-leak.ts +++ b/packages/types/src/error-leak.ts @@ -26,6 +26,10 @@ * positive costs a caller nothing but detail on a response that was a server * fault anyway — while the full text still reaches server logs and the * error reporter. + * + * [#5811] {@link declaresServerFault} joins it here for the same reason and + * answers the other half of the question: the heuristic asks whether a message + * *sounds* internal, the declaration asks whether the producer *said so*. */ /** Generic replacement text for a message that trips {@link looksLikeInternalErrorLeak}. */ @@ -59,3 +63,55 @@ export function looksLikeInternalErrorLeak(message: string | undefined | null): lower.includes('foreign key') ); } + +/** + * Whether the thrown error **declares a server fault** in the ADR-0112 envelope: + * `status >= 500` *and* a non-empty `code`. + * + * The counterpart to {@link looksLikeInternalErrorLeak}, and deliberately not a + * message test at all. Some server faults are dangerous to echo while saying + * nothing a phrasing heuristic can recognise — the motivating family is + * `service-analytics`' `read-scope-sql.ts`, whose ten fail-closed RLS lowering + * refusals name the FIELD NAMES AND COMPARANDS OF THE RLS POLICY: + * + * ``` + * [read-scope-sql] unsafe field identifier "secret_policy_field" — refusing to + * build read scope (fail-closed). + * ``` + * + * That text comes from an administrator's sharing rule compiled by the security + * service; the tenant who receives it never wrote it and must not be able to read + * it out of an error body. Measured, all eleven of its message shapes return + * FALSE from `looksLikeInternalErrorLeak` — they look nothing like a driver dump — + * so a boundary that only ran the heuristic echoed every one of them verbatim + * (#5811 measured 11/11 through `errorResponseBase`). Teaching the heuristic to + * recognise `[read-scope-sql]` would have been *more* message sniffing, which is + * the mechanism #5352/#5367 exist to remove. So the withhold keys on the + * DECLARATION instead: a producer that says `status >= 500` with a `code` has + * declared that this is the server's fault, and a server fault's detail belongs in + * the operator's log, not in the caller's body. + * + * **Both halves are required, and it is deliberately NOT "any 5xx".** #5667 kept + * UNDECLARED 5xx errors legible on purpose — a bare `Error` from our own code + * ("no strategy can handle query …") is the operator's own bug report, carries + * nothing tenant-sensitive, and still falls to `looksLikeInternalErrorLeak`. + * Widening this to every 500 would delete that decision. + * + * **Reads `status`, not `statusCode`.** `status` is the channel ADR-0112 declares; + * `statusCode` is an alternate spelling some boundaries tolerate when *deriving* + * an HTTP status. Accepting it here would make the disclosure rule depend on which + * spelling a producer happened to use — consumer-side leniency of exactly the kind + * Prime Directive #12 removes. A producer that wants its detail withheld declares + * the envelope. + * + * Costs no diagnostics: every boundary that applies this still logs the untouched + * error and hands it to the error reporter. + * + * @param err - the thrown value, of any shape (a non-object is simply not a + * declaration). + */ +export function declaresServerFault(err: unknown): boolean { + if (typeof err !== 'object' || err === null) return false; + const { status, code } = err as { status?: unknown; code?: unknown }; + return typeof status === 'number' && status >= 500 && typeof code === 'string' && code.length > 0; +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9fedcc9690..7313c203c6 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1970,6 +1970,9 @@ importers: '@objectstack/plugin-hono-server': specifier: workspace:* version: link:../plugins/plugin-hono-server + '@objectstack/service-analytics': + specifier: workspace:* + version: link:../services/service-analytics '@objectstack/service-job': specifier: workspace:* version: link:../services/service-job