diff --git a/.changeset/analytics-cube-gate-and-error-leak.md b/.changeset/analytics-cube-gate-and-error-leak.md new file mode 100644 index 0000000000..8511180016 --- /dev/null +++ b/.changeset/analytics-cube-gate-and-error-leak.md @@ -0,0 +1,71 @@ +--- +"@objectstack/types": minor +"@objectstack/service-analytics": minor +"@objectstack/runtime": minor +"@objectstack/rest": patch +--- + +fix(analytics,runtime,types): gate cube auto-inference on object existence; stop the dispatcher boundary returning raw SQL (#3867) + +Two independent defects on the `/analytics` surface, found while verifying #3770 +against a real server. On an authenticated CRM dev server, before this change: + +``` +POST /api/v1/analytics/query {"cube":"sqlite_master","measures":["count"],"dimensions":["type"]} +→ 200 {"rows":[{"type":"index","count":262},{"type":"table","count":71},{"type":"view","count":1}], + "sql":"SELECT type AS \"type\", COUNT(*) AS \"count\" FROM \"sqlite_master\" GROUP BY type"} +``` + +That is SQLite's internal schema table — never a registered object — read +successfully through the analytics endpoint. Not merely "the name reaches the +driver and errors": **any table the connection can see was readable.** + +**① The cube name reached the driver as a table name.** `AnalyticsService.ensureCube` +auto-infers a minimal Cube when none is registered, with `cube.sql = `. That is the intended "metric over an object" path — an `object-metric` KPI +widget queries `crm_account` with no authored Cube — but it accepted *any* string, +so the endpoint could aggregate over an arbitrary physical table. The +analytics-side twin of the data-path gap #3770 closed, and it was not covered by +that fix: #3770 gated the protocol's `analyticsQuery`, which is the *degraded +fallback*; a deployment with `@objectstack/service-analytics` installed runs the +real engine instead (`ctx.replaceService`). + +Inference is now gated on the same schema registry the data path consults, via a +new optional `AnalyticsServiceConfig.isRegisteredObject` that `plugin.ts` wires +from the `data` engine's `getObject`. Three-way rule: a registered Cube runs +untouched (its `sql` is whatever it declares); an unregistered name that IS an +object still auto-infers exactly as before; neither → `CUBE_NOT_FOUND` / 404 +raised before any SQL exists, naming both ways to make the request valid. With no +probe configured the gate stands down and warns once — the same tiering #3770 +took for a missing registry. `generateSql` (`/analytics/sql`) is gated too. + +**② The dispatcher boundary returned `err.message` verbatim.** `errorResponseBase` +is the single error exit for *every* route the dispatcher plugin mounts — +`/analytics`, `/packages`, `/i18n`, `/storage`, `/automation`, `/auth`, +`/notifications`, `/mcp`. `@objectstack/rest` has guarded its data routes against +driver dumps forever (`mapDataError`); this boundary guarded nothing, so any +driver error on any of those routes shipped its SQL to the client. Unlike ①, this +half is unconditional — it does not depend on the cube being invalid. + +The leak heuristic moved out of `rest-server.ts` into `@objectstack/types` as +`looksLikeInternalErrorLeak` (both packages already depend on it) and is now +applied at both boundaries — one predicate, one place to widen when a new +dialect's phrasing shows up. `mapDataError`'s behaviour is unchanged. At the +dispatcher it applies **only to 5xx**: a 4xx message is a deliberate +business/validation answer and must reach the caller intact. Sanitising costs no +diagnostics — the untouched error still reaches `errorReporter` through the +existing `__obsRecordedError` side-channel. + +**Also fixed in the same function:** `errorResponseBase` read only +`err.statusCode`, while domain errors across this codebase carry `status` (and +`HttpDispatcher.errorFromThrown` already reads `status` first). Every deliberate +4xx thrown through a dispatcher route — including #3770's `OBJECT_NOT_FOUND` on +the analytics fallback path — was rendered as a **500**. It now reads `status` +then `statusCode`. + +**Behaviour change.** `/analytics/query` and `/analytics/sql` return 404 +`CUBE_NOT_FOUND` for a cube that is neither registered nor a registered object; +previously the name was passed to the driver. Dashboards and KPI widgets pointed +at real objects or authored cubes are unaffected. A 5xx on a dispatcher route +whose message looks like a driver dump now reads `Internal server error` — check +server logs or your error reporter for the original. diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 02c89873ab..af35d91c9a 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -4,7 +4,7 @@ import { IHttpServer, resolveAuthzContext, resolveLocalizationContext, isAuthGateAllowlisted, shouldDenyAnonymous, ANONYMOUS_DENY_BODY, ANONYMOUS_DENY_STATUS, } from '@objectstack/core'; -import { isMcpServerEnabled } from '@objectstack/types'; +import { isMcpServerEnabled, looksLikeInternalErrorLeak } 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'; @@ -401,17 +401,14 @@ export function mapDataError(error: any, object?: string): { status: number; bod // Default: do NOT leak raw SQL or driver internals. If the message // looks like a SQL/driver dump, replace it with a generic envelope // and rely on server logs for the full diagnostic. - const looksLikeSqlLeak = - lower.includes('sqlite_') || - lower.includes('sqlstate') || - lower.startsWith('insert into ') || - lower.startsWith('update ') || - lower.startsWith('select ') || - lower.startsWith('delete from ') || - lower.includes('constraint failed') || - lower.includes('unique constraint') || - lower.includes('foreign key'); - if (looksLikeSqlLeak) { + // + // [#3867] The heuristic itself now lives in `@objectstack/types` + // (`looksLikeInternalErrorLeak`) so the OTHER HTTP boundary — the + // dispatcher-plugin routes (`/analytics`, `/packages`, `/i18n`, …) — can + // apply the same rule. Before #3867 that boundary applied none and + // returned raw SQL to clients. Behaviour here is unchanged; only the + // predicate's home moved. + if (looksLikeInternalErrorLeak(raw)) { // Surface unique-constraint violations as a structured 409 so // the UI can map them to "this value already exists". if (lower.includes('unique constraint') || lower.includes('unique violation')) { diff --git a/packages/runtime/src/dispatcher-plugin.error-envelope.test.ts b/packages/runtime/src/dispatcher-plugin.error-envelope.test.ts new file mode 100644 index 0000000000..bba27aad76 --- /dev/null +++ b/packages/runtime/src/dispatcher-plugin.error-envelope.test.ts @@ -0,0 +1,156 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3867 — the dispatcher-plugin error exit. + * + * `errorResponseBase` is the single error exit for EVERY route this plugin + * mounts (`/analytics`, `/packages`, `/i18n`, `/storage`, `/automation`, + * `/auth`, `/notifications`, `/mcp`, …): each handler catches and calls it + * rather than re-throwing. Two defects lived there, and neither is visible + * until a real error actually reaches it: + * + * 1. It returned `err.message` verbatim. `@objectstack/rest` has guarded its + * data routes against driver dumps forever (`mapDataError`); this boundary + * guarded nothing, so `POST /analytics/query` on an unresolvable cube + * answered with a real SQL statement in the body. + * 2. It read only `err.statusCode`, while domain errors across this codebase + * carry `status` (and `HttpDispatcher.errorFromThrown` already reads + * `status` first). A deliberate 404 rendered as a 500. + * + * These drive the REAL route handler the plugin registers, through the real + * dispatcher, so the assertions are about what an HTTP client receives. + */ + +import { describe, it, expect } from 'vitest'; + +import { createDispatcherPlugin } from './dispatcher-plugin.js'; + +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'), + }, + }; +} + +/** A kernel whose `analytics` service throws whatever the test hands it. */ +function makeCtx(fakeServer: any, analyticsError: unknown) { + const analytics = { + query: async () => { throw analyticsError; }, + getMeta: async () => ({ cubes: [] }), + generateSql: async () => ({ sql: null }), + }; + 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 `POST /analytics/query` with an analytics service that throws `err`. */ +async function postAnalyticsQuery(err: unknown) { + const { server, handlers } = makeFakeServer(); + const plugin = createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false }); + await plugin.start?.(makeCtx(server, err)); + + 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: { cube: 'x', query: {} }, query: {} }, res); + return res; +} + +describe('#3867 — dispatcher-plugin error envelope', () => { + it('does not return raw SQL to the client (the message that motivated the issue)', async () => { + const res = await postAnalyticsQuery( + new Error('SELECT FROM "sqlite_sequence" - near "FROM": syntax error'), + ); + + expect(res.statusCode).toBe(500); + expect(res.body.success).toBe(false); + expect(res.body.error.message).toBe('Internal server error'); + // The specifics a client must never see. + expect(String(res.body.error.message)).not.toContain('SELECT'); + expect(String(res.body.error.message)).not.toContain('sqlite_sequence'); + }); + + it('still hands the UNSANITISED error to the observability side-channel', async () => { + // Sanitising the response must not cost server-side diagnostics: the + // error reporter reads `__obsRecordedError`, not the response body. + const original = new Error('UNIQUE constraint failed: sys_user.email'); + const res = await postAnalyticsQuery(original); + + expect(res.statusCode).toBe(500); + expect(res.body.error.message).toBe('Internal server error'); + expect((res as any).__obsRecordedError).toBe(original); + }); + + it('leaves an ordinary 5xx message alone — only leaks are replaced', async () => { + const res = await postAnalyticsQuery(new Error('analytics engine unavailable')); + + expect(res.statusCode).toBe(500); + expect(res.body.error.message).toBe('analytics engine unavailable'); + }); + + it('honours `status` (not just `statusCode`) so a domain 404 is not a 500', async () => { + // What the #3867 cube gate throws, and the shape every protocol-layer + // domain error uses (`OBJECT_NOT_FOUND`, `RECORD_NOT_FOUND`, …). + const err = Object.assign(new Error("Cube 'ghost' not found: no cube is registered"), { + code: 'CUBE_NOT_FOUND', + status: 404, + }); + const res = await postAnalyticsQuery(err); + + expect(res.statusCode).toBe(404); + // A 4xx message is a deliberate answer — it must reach the caller intact. + expect(res.body.error.message).toContain("Cube 'ghost' not found"); + }); + + it('still honours `statusCode` for callers that use it', async () => { + const err = Object.assign(new Error('bad request'), { statusCode: 400 }); + const res = await postAnalyticsQuery(err); + + expect(res.statusCode).toBe(400); + expect(res.body.error.message).toBe('bad request'); + }); + + it('does not sanitise a 4xx even when its message resembles SQL', async () => { + // Anti-regression for the tier: the guard is scoped to 5xx precisely so + // a deliberate client-facing answer is never swallowed. + const err = Object.assign(new Error('unique constraint on email — pick another'), { + status: 409, + }); + const res = await postAnalyticsQuery(err); + + expect(res.statusCode).toBe(409); + expect(res.body.error.message).toBe('unique constraint on email — pick another'); + }); +}); diff --git a/packages/runtime/src/dispatcher-plugin.ts b/packages/runtime/src/dispatcher-plugin.ts index 59f6350b5d..f3d899c019 100644 --- a/packages/runtime/src/dispatcher-plugin.ts +++ b/packages/runtime/src/dispatcher-plugin.ts @@ -1,6 +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 { HttpDispatcher, HttpDispatcherResult } from './http-dispatcher.js'; import { buildSecurityHeaders, @@ -347,8 +348,38 @@ function sendResultBase( }); } +/** + * The single error exit for EVERY dispatcher-plugin route — `/analytics`, + * `/packages`, `/i18n`, `/storage`, `/automation`, `/auth`, `/notifications`, + * `/mcp`, … Each handler catches and calls here rather than re-throwing. + * + * [#3867] Two things were wrong with it, both invisible until a driver error + * actually reached this path: + * + * 1. **It only honoured `statusCode`.** Domain errors across this codebase + * carry their HTTP status as `status` (the protocol layer's + * `OBJECT_NOT_FOUND`/`RECORD_NOT_FOUND`/`CLONE_DISABLED`, plugin-sharing's + * `FORBIDDEN`, …); `HttpDispatcher.errorFromThrown` already reads `status` + * first, `statusCode` second. Here a deliberate 404 was rendered as a + * **500** — the wrong code, and it dragged the message through the + * sanitiser below for no reason. Now aligned with `errorFromThrown`. + * + * 2. **It returned `err.message` verbatim.** `@objectstack/rest` has guarded + * its data routes against driver dumps since forever (`mapDataError`), but + * this boundary had no equivalent, so `POST /analytics/query` on an + * unresolvable cube answered with a real SQL statement in the body. The + * shared predicate now applies here too — but ONLY on a 5xx: a 4xx message + * is a deliberate business/validation answer and must reach the caller + * intact. + * + * Sanitising costs no diagnostics: the untouched error is still handed to + * `errorReporter` through the `__obsRecordedError` side-channel below. + */ function errorResponseBase(err: any, res: any, securityHeaders?: Record): void { - const code = err.statusCode || 500; + const code = + (typeof err?.status === 'number' ? err.status : undefined) ?? + (typeof err?.statusCode === 'number' ? err.statusCode : undefined) ?? + 500; res.status(code); if (securityHeaders) { for (const [k, v] of Object.entries(securityHeaders)) { @@ -366,9 +397,14 @@ function errorResponseBase(err: any, res: any, securityHeaders?: Record= 500 && looksLikeInternalErrorLeak(raw) + ? INTERNAL_ERROR_MESSAGE + : raw || 'Internal Server Error'; res.json({ success: false, - error: { message: err.message || 'Internal Server Error', code }, + error: { message, code }, }); } diff --git a/packages/services/service-analytics/src/__tests__/analytics-service.test.ts b/packages/services/service-analytics/src/__tests__/analytics-service.test.ts index 3bf2f4833d..790f4c60d8 100644 --- a/packages/services/service-analytics/src/__tests__/analytics-service.test.ts +++ b/packages/services/service-analytics/src/__tests__/analytics-service.test.ts @@ -725,7 +725,15 @@ describe('AnalyticsService — auto-inferred cube log level', () => { it('logs at debug (not warn) for a scalar metric over an unregistered cube', async () => { const logger = { info: vi.fn(), debug: vi.fn(), warn: vi.fn(), error: vi.fn(), child: vi.fn().mockReturnThis() } as any; await makeService(logger).query({ cube: 'showcase_task', measures: ['count'] }); - expect(logger.warn).not.toHaveBeenCalled(); + // [#3867] Asserted against THIS message rather than "warn was never called + // at all". These services are built without `isRegisteredObject`, so the + // cube-existence gate correctly stands down and warns once about being + // inactive — a different message, and not what this pair is about. The + // contract here is the LEVEL of the no-cube-registered log, which is + // exactly how the sibling case below already asserts it. + expect(logger.warn).not.toHaveBeenCalledWith( + expect.stringContaining('No cube registered for "showcase_task"'), + ); expect(logger.debug).toHaveBeenCalledWith(expect.stringContaining('No cube registered for "showcase_task"')); }); diff --git a/packages/services/service-analytics/src/__tests__/cube-inference-gate.test.ts b/packages/services/service-analytics/src/__tests__/cube-inference-gate.test.ts new file mode 100644 index 0000000000..354a34b3bb --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/cube-inference-gate.test.ts @@ -0,0 +1,144 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3867 — the cube auto-inference existence gate. + * + * `ensureCube` infers a minimal Cube when none is registered under the queried + * name, and that inferred cube's `sql` IS the name. That is the intended + * "metric over an object" path (an `object-metric` KPI widget queries + * `crm_account` with no authored Cube) — but it accepted ANY string, so + * `POST /analytics/query` could aggregate over an arbitrary physical table. + * Live repro on a CRM dev server before the fix, using a table that genuinely + * exists but is not a registered object: + * + * ``` + * POST /analytics/query {"cube":"sqlite_sequence","query":{"measures":["count"]}} + * → 500 {"error":{"message":"SELECT FROM \"sqlite_sequence\" - near \"FROM\": syntax error"}} + * ``` + * + * The name reached the driver as a table. This is the analytics-side twin of + * the data-path gap closed in #3770, and these cases pin the same three-way + * rule: registered cube → run; unregistered name that IS an object → infer and + * run (the intended path, unchanged); neither → 404 before any SQL exists. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { Cube } from '@objectstack/spec/data'; +import { AnalyticsService } from '../analytics-service.js'; + +const silentLogger = { + info: vi.fn(), + debug: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + child: vi.fn().mockReturnThis(), +} as any; + +const authoredCube: Cube = { + name: 'authored_cube', + title: 'Authored', + sql: 'some_physical_table', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: {}, + public: false, +}; + +/** Records which object each aggregate ran against, so we can assert none ran. */ +function makeService(opts: { + knownObjects?: string[]; + cubes?: Cube[]; + wireRegistry?: boolean; +} = {}) { + const aggregated: string[] = []; + const service = new AnalyticsService({ + logger: silentLogger, + ...(opts.cubes ? { cubes: opts.cubes } : {}), + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async (objectName: string) => { + aggregated.push(objectName); + return [{ count: 1 }]; + }, + ...(opts.wireRegistry === false + ? {} + : { isRegisteredObject: (n: string) => (opts.knownObjects ?? []).includes(n) }), + }); + return { service, aggregated }; +} + +const CUBE_NOT_FOUND = { code: 'CUBE_NOT_FOUND', status: 404 }; + +describe('#3867 — cube auto-inference existence gate', () => { + it('rejects an unregistered name that is not an object, before any SQL exists', async () => { + const { service, aggregated } = makeService({ knownObjects: ['crm_account'] }); + + await expect( + service.query({ cube: 'sqlite_sequence', measures: ['count'] } as any), + ).rejects.toMatchObject(CUBE_NOT_FOUND); + + // The whole point: the name never became a table. + expect(aggregated).toEqual([]); + }); + + it('names the object in the rejection so the caller can act on it', async () => { + const { service } = makeService({ knownObjects: ['crm_account'] }); + + await expect( + service.query({ cube: 'sqlite_sequence', measures: ['count'] } as any), + ).rejects.toMatchObject({ cube: 'sqlite_sequence' }); + }); + + it('does not poison the registry with the rejected cube', async () => { + // Pre-fix `ensureCube` registered the inferred cube before anything + // could object; a rejected name must leave no trace, or the second + // attempt would find a "registered" cube and sail through the gate. + const { service, aggregated } = makeService({ knownObjects: ['crm_account'] }); + + await expect(service.query({ cube: 'ghost', measures: ['count'] } as any)).rejects.toThrow(); + expect(service.cubeRegistry.get('ghost')).toBeUndefined(); + await expect(service.query({ cube: 'ghost', measures: ['count'] } as any)) + .rejects.toMatchObject(CUBE_NOT_FOUND); + expect(aggregated).toEqual([]); + }); + + it('still auto-infers for a REGISTERED object — the intended KPI path is unchanged', async () => { + const { service, aggregated } = makeService({ knownObjects: ['crm_account'] }); + + const result = await service.query({ cube: 'crm_account', measures: ['count'] } as any); + + expect(result).toBeTruthy(); + expect(aggregated).toEqual(['crm_account']); + expect(service.cubeRegistry.get('crm_account')).toBeTruthy(); + }); + + it('never gates an authored cube — its `sql` is whatever it declares', async () => { + // `authored_cube` is not an object name, and its physical table is a + // third name entirely. A registered Cube was authored deliberately, so + // the gate must not second-guess it. + const { service, aggregated } = makeService({ knownObjects: [], cubes: [authoredCube] }); + + await service.query({ cube: 'authored_cube', measures: ['count'] } as any); + + expect(aggregated).toEqual(['some_physical_table']); + }); + + it('gates generateSql too, not just query', async () => { + // `/analytics/sql` runs the same `ensureCube`; leaving it ungated would + // hand back SQL naming an arbitrary table. + const { service } = makeService({ knownObjects: ['crm_account'] }); + + await expect( + service.generateSql({ cube: 'sqlite_sequence', measures: ['count'] } as any), + ).rejects.toMatchObject(CUBE_NOT_FOUND); + }); + + it('stands down when no registry probe is configured — nothing to consult', async () => { + // Same tiering as #3770's `assertObjectRegistered`: with no source of + // truth the question cannot be answered, and failing closed would break + // every embedding that runs analytics without a data engine. + const { service, aggregated } = makeService({ wireRegistry: false }); + + await service.query({ cube: 'anything_at_all', measures: ['count'] } as any); + + expect(aggregated).toEqual(['anything_at_all']); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 8fdd01ae1d..b45b3e058e 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -183,6 +183,24 @@ export interface AnalyticsServiceConfig { * `StrategyContext.isExternalObject`. */ isExternalObject?: (objectName: string) => boolean; + /** + * [#3867] Is `name` a registered object in this kernel's schema registry? + * + * Consulted by {@link AnalyticsService.ensureCube} on the auto-inference + * path only. When no Cube is registered under the queried name, the service + * infers a minimal one whose `sql` IS that name — the intended "metric over + * an object" path (an `object-metric` KPI widget queries `crm_account` + * without anyone authoring a Cube). Without this hook that inference accepts + * ANY string, so an arbitrary physical table name reached the driver: the + * analytics-side twin of the data-path gap closed in #3770. + * + * Optional, and absence means "skip the check" — same tiering as #3770's + * `assertObjectRegistered`: with no registry to consult the question cannot + * be answered, and failing closed would break every embedding that runs + * analytics without a data engine. The production bridge in `plugin.ts` + * always wires it. + */ + isRegisteredObject?: (name: string) => boolean; /** * ADR-0021 — optional object-graph resolver used when compiling datasets: * `(baseObject, relationshipName) => relatedObjectName | undefined`. When @@ -265,6 +283,10 @@ export class AnalyticsService implements IAnalyticsService { private readonly labelResolver?: DimensionLabelDeps; /** ADR-0037 P3: pending-seed row resolver for draft data preview. */ private readonly draftRowsResolver?: AnalyticsServiceConfig['draftRowsResolver']; + /** [#3867] Schema-registry probe gating cube auto-inference. */ + private readonly isRegisteredObject?: AnalyticsServiceConfig['isRegisteredObject']; + /** [#3867] One-shot flag for the {@link assertInferableCube} stand-down warning. */ + private warnedNoObjectRegistry = false; readonly cubeRegistry: CubeRegistry; private readonly logger: Logger; @@ -282,6 +304,7 @@ export class AnalyticsService implements IAnalyticsService { this.measureCurrency = config.measureCurrency; this.labelResolver = config.labelResolver; this.draftRowsResolver = config.draftRowsResolver; + this.isRegisteredObject = config.isRegisteredObject; // Compile + register pre-defined datasets (ADR-0021). if (config.datasets) { @@ -803,6 +826,13 @@ export class AnalyticsService implements IAnalyticsService { let cube = this.cubeRegistry.get(name); if (!cube) { + // [#3867] Auto-inference below sets `cube.sql = name`, so from here on + // the queried string IS a physical table name. Verify it names a + // registered object BEFORE that happens — otherwise `/analytics/query` + // is a way to aggregate over any table the connection can see, exactly + // the hole #3770 closed on the data path. A registered Cube needs no + // such check: it was authored, and its `sql` is whatever it declares. + this.assertInferableCube(name); cube = this.inferCubeFromQuery(query); this.cubeRegistry.register(cube); // A scalar query — only measures, no grouping (no `dimensions`/ @@ -845,6 +875,44 @@ export class AnalyticsService implements IAnalyticsService { } } + /** + * [#3867] Gate on the cube auto-inference path: a name with no registered + * Cube may only be inferred into one if it is a registered object. + * + * Rejects with `status: 404` / `code: 'CUBE_NOT_FOUND'` so the HTTP boundary + * answers "no such cube" instead of letting the name reach the driver as a + * table and surfacing whatever the driver says about it. The message names + * both ways the request could be made valid, because from here the two are + * genuinely indistinguishable: register a Cube, or register the object. + * + * Skips when `isRegisteredObject` was not supplied — see the config field's + * doc for why that tier is a deliberate stand-down and not a hole. + */ + private assertInferableCube(name: string): void { + const isRegisteredObject = this.isRegisteredObject; + if (!isRegisteredObject) { + if (!this.warnedNoObjectRegistry) { + this.warnedNoObjectRegistry = true; + this.logger.warn( + '[Analytics] no object-registry hook configured — the cube-inference existence gate ' + + '(#3867) is INACTIVE for this service; an unregistered cube name reaches the driver ' + + 'as a raw table name.', + ); + } + return; + } + if (isRegisteredObject(name)) return; + const err = new Error( + `Cube '${name}' not found: no cube is registered under that name, and it is not a ` + + `registered object either (a cube can only be auto-inferred from a registered object). ` + + `Define a Cube in your stack, or check the object name.`, + ) as Error & { code?: string; status?: number; cube?: string }; + err.code = 'CUBE_NOT_FOUND'; + err.status = 404; + err.cube = name; + throw err; + } + /** Build a minimal Cube from the fields referenced by an AnalyticsQuery. */ private inferCubeFromQuery(query: AnalyticsQuery): Cube { const cubeName = query.cube!; diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index 170c25d02d..44d7e9646e 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -499,6 +499,20 @@ export class AnalyticsServicePlugin implements Plugin { const obj = dataEngine()?.getObject?.(objectName); return !!(obj && obj.external != null); }, + // [#3867] Existence probe for the cube auto-inference gate. Reads the + // same schema registry the data path's #3770 gate consults, through the + // engine accessor this bridge already uses above — so "which objects + // exist" has one answer across /data and /analytics. + // + // `dataEngine()` resolves lazily and may be absent entirely (analytics + // installed without a data engine). Reporting `false` there would 404 + // every cube, so an unresolvable engine reports `true` — "cannot answer, + // do not block" — mirroring the tiering #3770 took on the data path. + isRegisteredObject: (name: string) => { + const engine = dataEngine(); + if (!engine) return true; + return engine.getObject?.(name) != null; + }, draftRowsResolver, }; diff --git a/packages/types/src/error-leak.test.ts b/packages/types/src/error-leak.test.ts new file mode 100644 index 0000000000..691b0f1e73 --- /dev/null +++ b/packages/types/src/error-leak.test.ts @@ -0,0 +1,58 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3867 — the shared internal-leak predicate. + * + * It was extracted from `@objectstack/rest`'s `mapDataError` so the OTHER HTTP + * boundary (the dispatcher-plugin routes) applies the same rule. These cases + * pin both halves of its job: catch driver/SQL dumps, and leave deliberate + * business messages alone — the second half matters because a false positive + * on a 4xx would replace a real answer with "Internal server error". + */ + +import { describe, it, expect } from 'vitest'; +import { looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from './error-leak.js'; + +describe('looksLikeInternalErrorLeak', () => { + it('catches the message that motivated #3867 (raw SQL from /analytics/query)', () => { + expect( + looksLikeInternalErrorLeak('SELECT FROM "sqlite_sequence" - near "FROM": syntax error'), + ).toBe(true); + }); + + it.each([ + ['sqlite dialect code', 'SQLITE_CONSTRAINT_NOTNULL: NOT NULL constraint failed: t.c'], + ['postgres SQLSTATE', 'error: duplicate key value violates unique constraint (SQLSTATE 23505)'], + ['bare INSERT', 'insert into `sys_team` (`id`) values (?) - some driver detail'], + ['bare UPDATE', 'update `sys_team` set `name` = ? - failed'], + ['bare DELETE', 'delete from `sys_team` where `id` = ? - failed'], + ['constraint dump', 'NOT NULL constraint failed: sys_team.organization_id'], + ['unique violation', 'UNIQUE constraint failed: sys_user.email'], + ['foreign key', 'FOREIGN KEY constraint failed'], + ])('catches %s', (_label, message) => { + expect(looksLikeInternalErrorLeak(message)).toBe(true); + }); + + it.each([ + ['a business rule thrown by a hook', '删除被阻断:该客户下仍有未结订单'], + ['a validation message', 'name is required'], + ['a not-found message', "Object 'ghost' is not registered"], + ['a permission denial', '[Security] Access denied: operation on object is not permitted'], + // Anchored deliberately: a message may MENTION a verb without being SQL. + ['a sentence merely mentioning update', 'Cannot update this record while it is locked'], + ['a sentence merely mentioning select', 'Please select at least one row before exporting'], + ])('leaves %s alone', (_label, message) => { + expect(looksLikeInternalErrorLeak(message)).toBe(false); + }); + + it('is null-safe — an error with no message is not a leak', () => { + expect(looksLikeInternalErrorLeak(undefined)).toBe(false); + expect(looksLikeInternalErrorLeak(null)).toBe(false); + expect(looksLikeInternalErrorLeak('')).toBe(false); + }); + + it('exposes a replacement message that names nothing internal', () => { + expect(INTERNAL_ERROR_MESSAGE).toBe('Internal server error'); + expect(looksLikeInternalErrorLeak(INTERNAL_ERROR_MESSAGE)).toBe(false); + }); +}); diff --git a/packages/types/src/error-leak.ts b/packages/types/src/error-leak.ts new file mode 100644 index 0000000000..5bc29f4e9f --- /dev/null +++ b/packages/types/src/error-leak.ts @@ -0,0 +1,61 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Shared "does this error message leak server internals?" heuristic (#3867). + * + * ObjectStack has more than one HTTP boundary. `@objectstack/rest` guards the + * REST data routes inside `mapDataError`; the dispatcher-plugin routes + * (`/analytics`, `/packages`, `/i18n`, `/storage`, `/automation`, …) exit + * through `errorResponseBase`. Before #3867 only the first of those sanitised + * anything, so a driver error raised under `/analytics/query` reached the + * client verbatim — a real SQL statement in the response body: + * + * ``` + * {"success":false,"error":{"message":"SELECT FROM \"sqlite_sequence\" - near \"FROM\": syntax error","code":500}} + * ``` + * + * "Do not ship driver internals to clients" is a property of the HTTP + * boundary, not of one router, so the predicate lives here — the package both + * `@objectstack/rest` and `@objectstack/runtime` already depend on — and each + * boundary applies it in its own envelope. One heuristic, one place to widen + * when a new dialect's phrasing shows up. + * + * Deliberately a *heuristic over the message*, not a driver taxonomy: these + * errors arrive as plain `Error`s from a half-dozen dialects with no shared + * shape. It is applied only where the outcome is already a 5xx, so a false + * 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. + */ + +/** Generic replacement text for a message that trips {@link looksLikeInternalErrorLeak}. */ +export const INTERNAL_ERROR_MESSAGE = 'Internal server error'; + +/** + * Whether `message` looks like a raw SQL statement or driver/engine dump that + * must not be returned to an API client. + * + * Matches: dialect error codes (`SQLSTATE`, `sqlite_*`), bare statements + * (a message that *starts* as `SELECT`/`INSERT INTO`/`UPDATE`/`DELETE FROM` — + * drivers prefix the offending SQL to their message), and constraint-violation + * dumps, which name physical tables and columns. + * + * Does NOT match ordinary business or validation messages, which is why the + * statement forms are anchored with `startsWith`: a legitimate message may + * *mention* "update" without being one. + */ +export function looksLikeInternalErrorLeak(message: string | undefined | null): boolean { + if (!message) return false; + const lower = String(message).toLowerCase(); + return ( + lower.includes('sqlite_') || + lower.includes('sqlstate') || + lower.startsWith('insert into ') || + lower.startsWith('update ') || + lower.startsWith('select ') || + lower.startsWith('delete from ') || + lower.includes('constraint failed') || + lower.includes('unique constraint') || + lower.includes('foreign key') + ); +} diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 121395d7ad..5274def0ce 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -2,6 +2,7 @@ export * from './degraded-boot.js'; export * from './env.js'; +export * from './error-leak.js'; export * from './module-not-found.js'; // Placeholder for Kernel interface to avoid circular dependency