From 09b1ed49949747bc15d72f42179534377612b6a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 04:13:32 +0000 Subject: [PATCH] fix(service-analytics): gate the /analytics/query SQL echo on debug (#8286) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/v1/analytics/query returned the executed statement to callers in data.sql on every deployment, NODE_ENV=production included, with no debug flag requested. The contract has always declared the field debug-only (AnalyticsResultResponseSchema: `sql: z.string().optional().describe('Executed SQL (if debug enabled)')`) — no implementation ever read a switch. The gate lands at the response-assembly seam, AnalyticsService.query, which is the single point every strategy's result leaves through: NativeSQLStrategy returns the statement it ran, ObjectQLStrategy renders a representative one, and FallbackDelegateStrategy passes through whatever the delegated service minted. queryDataset reaches the same seam via DatasetExecutor. generateSql — the dedicated /analytics/sql dry-run route — is deliberately not gated. New host switch `debugSql` (AnalyticsServicePlugin -> AnalyticsServiceConfig). Unset resolves to NODE_ENV === 'development' and nothing else: an unset NODE_ENV counts as production, matching how os start / os serve / os doctor read that absence. No request field: a caller-settable flag would let any tenant reopen the disclosure. Kept separate from the plugin's `debug` log-verbosity option so raising log level cannot widen what travels to a tenant. Tests: every absence pin is paired with a presence pin on the same cube, query and rows, differing only in the switch, and each arm captures the statement server-side so absence means withheld rather than never-minted. Three existing suites state the precondition they now depend on — including the #7598 cross-field pin, whose absence assertion would otherwise have stopped measuring the renderer's decline and started measuring the gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --- .changeset/analytics-sql-echo-debug-gate.md | 63 +++ .../src/__tests__/analytics-service.test.ts | 9 +- .../cross-field-engine-fallback.test.ts | 10 + .../dataset-selection-window.test.ts | 33 +- .../src/__tests__/sql-echo-debug-gate.test.ts | 384 ++++++++++++++++++ .../src/analytics-service.ts | 86 +++- .../services/service-analytics/src/plugin.ts | 16 + 7 files changed, 591 insertions(+), 10 deletions(-) create mode 100644 .changeset/analytics-sql-echo-debug-gate.md create mode 100644 packages/services/service-analytics/src/__tests__/sql-echo-debug-gate.test.ts diff --git a/.changeset/analytics-sql-echo-debug-gate.md b/.changeset/analytics-sql-echo-debug-gate.md new file mode 100644 index 0000000000..616f478c58 --- /dev/null +++ b/.changeset/analytics-sql-echo-debug-gate.md @@ -0,0 +1,63 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): gate the `/analytics/query` SQL echo on debug, as the contract has always declared (#8286) + +`POST /api/v1/analytics/query` returned the executed statement to the caller in +`data.sql` on every deployment, `NODE_ENV=production` included, with no debug +flag requested and none available to request. The contract had declared the +field debug-only since it was introduced — `AnalyticsResultResponseSchema` +(`spec/api/analytics.zod.ts`) types it `optional()` and describes it as +"Executed SQL (if debug enabled)" — but no implementation ever read a debug +switch. This restores declared = enforced. **The contract is unchanged; the +response now matches it.** + +**What was disclosed.** More than table and column names. The echoed statement +carries the compiled read scope, so it describes the SHAPE of the tenant +isolation predicate: on the reported deployment it showed that `sys_user` is +walled by an enumerated `"sys_user"."id" IN ($2, $3, …)` member list rather than +by an `organization_id` comparison — that is, which column the wall is built on +and how — plus the bound-parameter arity, which counts the caller's own +organization's membership and hands a prober the exact query surface to work +against. + +**No wall was breached.** This is information disclosure and nothing more. The +reporter ran the isolation probes on the same deployment and every one held: +cross-tenant read answered 404, cross-tenant update and delete answered 403 at +row-level security, a `filter`/`where` naming another organization came back +empty, a batch write by foreign id answered per-row `PERMISSION_DENIED`, and the +audit log and activity stream were partitioned cleanly. The wall works; it +simply should not have been describing itself to callers. + +**The gate is one gate.** It lives at the response-assembly seam — +`AnalyticsService.query`, the single point every strategy's result leaves +through — not on any one strategy. `NativeSQLStrategy` returns the statement it +ran, `ObjectQLStrategy` renders a representative one, and the fallback delegate +passes through whatever the service it delegates to minted (the in-memory +analytics service always echoes); gating one of the three would have left the +others serving. `queryDataset` reaches the same seam through `DatasetExecutor`, +so dataset-backed dashboard and report responses inherit the verdict without a +second gate to keep in step. + +**The switch, and its default.** New `debugSql` option on +`AnalyticsServicePlugin` (forwarded to `AnalyticsServiceConfig`). Unset means no +host choice, which resolves to `NODE_ENV === 'development'` — and only that: an +**unset** `NODE_ENV` counts as production and the echo stays off, matching how +`os start`, `os serve` and `os doctor` already read that absence. Of the two ways +to be wrong, disclosing on a production deployment whose operator forgot the +variable is the dangerous one. + +It is deliberately a HOST switch with no request field behind it: a +caller-settable debug flag would let any tenant reopen the disclosure on demand, +which is the shape of the defect rather than a fix for it. It is also +deliberately separate from the plugin's existing `debug` option, which stays +server-side log verbosity only — raising log level on a live deployment must not +widen what travels to a tenant. + +**Unaffected.** `POST /api/v1/analytics/sql` — the dedicated dry-run route that +exists to hand back a statement — is not gated and behaves exactly as before; it +is where an author debugging a widget should look. Rows, `fields`, `totals`, +drill-through metadata, error envelopes and every gate on the query path are +untouched, and no shipped consumer read the echo (the Studio console does not +render it). 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 73f35e3724..cf7b0dd46a 100644 --- a/packages/services/service-analytics/src/__tests__/analytics-service.test.ts +++ b/packages/services/service-analytics/src/__tests__/analytics-service.test.ts @@ -422,11 +422,18 @@ describe('AnalyticsService', () => { logger: silentLogger, queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: true, inMemory: false }), executeRawSql: vi.fn().mockResolvedValue(mockRows), + // [#8286] The claim under test is ROUTING — which strategy served — and + // the echo is only the witness it reads: `ObjectQLStrategy` would have + // died here for want of an aggregate bridge, so a statement coming back + // means NativeSQL won. Since #8286 that witness has a precondition, so + // the precondition is stated. The echo's OWN behaviour, both sides of + // this switch, is pinned in `sql-echo-debug-gate.test.ts`. + debugSql: true, }); const result = await service.query(baseQuery); expect(result.rows).toEqual(mockRows); - expect(result.sql).toBeDefined(); // NativeSQL always includes sql + expect(result.sql).toBeDefined(); // with the echo enabled, NativeSQL reports its statement }); it('should fall back to ObjectQLStrategy when nativeSql is false', async () => { diff --git a/packages/services/service-analytics/src/__tests__/cross-field-engine-fallback.test.ts b/packages/services/service-analytics/src/__tests__/cross-field-engine-fallback.test.ts index d652784844..070608d93e 100644 --- a/packages/services/service-analytics/src/__tests__/cross-field-engine-fallback.test.ts +++ b/packages/services/service-analytics/src/__tests__/cross-field-engine-fallback.test.ts @@ -110,6 +110,16 @@ describe('[#7598] cross-field `$field` on the analytics face — served via the readScope = null; service = new AnalyticsService({ cubes: [CUBE], + // [#8286] The response-level SQL echo is now debug-gated and OFF by + // default outside development. It is enabled here on purpose, and the + // reason is the same one the header gives for declaring both + // capabilities: an assertion has to be a MEASUREMENT. The pin below — + // "`/analytics/query` serves the query while the echo declines" — would + // pass against a service that never echoes anything at all, i.e. it would + // stop measuring the renderer's decline and start measuring the gate. + // With the echo on, `sql` being absent again means what this file says it + // means: `generateSql` refused and `execute()` swallowed the refusal. + debugSql: true, // BOTH paths available — see the header. Native SQL wins unless it declines. queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: true, inMemory: false }), executeRawSql: async (_object, sql) => { diff --git a/packages/services/service-analytics/src/__tests__/dataset-selection-window.test.ts b/packages/services/service-analytics/src/__tests__/dataset-selection-window.test.ts index 87bb028ece..b96eabb6c4 100644 --- a/packages/services/service-analytics/src/__tests__/dataset-selection-window.test.ts +++ b/packages/services/service-analytics/src/__tests__/dataset-selection-window.test.ts @@ -41,14 +41,26 @@ const accounts = DatasetSchema.parse({ type AggCall = { object: string; options: Record }; -/** ObjectQL-aggregate service; captures every `executeAggregate` call. */ -function aggService(rows: Record[], calls: AggCall[] = []) { +/** + * ObjectQL-aggregate service; captures every `executeAggregate` call. + * + * [#8286] `debugSql` is the response-level SQL echo, off by default outside + * development. Only the tests that read `result.sql` — the block asserting the + * echo tells the truth — need it on; the rest of this file measures the CALL + * the executor made, which the gate does not touch. + */ +function aggService( + rows: Record[], + calls: AggCall[] = [], + options: { debugSql?: boolean } = {}, +) { const svc = new AnalyticsService({ queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), - executeAggregate: async (object, options) => { - calls.push({ object, options: options as Record }); + executeAggregate: async (object, opts) => { + calls.push({ object, options: opts as Record }); return rows; }, + debugSql: options.debugSql, }); return { svc, calls }; } @@ -327,9 +339,16 @@ describe('#3588 — ordering never corrupts a multi-query selection', () => { }); }); +/** + * [#8286] Every service in this block enables the echo explicitly. The claim — + * that the echoed statement is an honest account of the query that ran — is + * unchanged and still worth pinning; what changed is that the echo now has a + * precondition, and a block whose whole subject is the echo's CONTENT must + * state it rather than inherit whatever `NODE_ENV` the runner happens to have. + */ describe('#3588 — the echoed SQL tells the truth on the ObjectQL path', () => { it('renders date_trunc for a bucketed dimension instead of the bare column', async () => { - const { svc } = aggService([{ created_at: '2026-06', account_count: 4 }]); + const { svc } = aggService([{ created_at: '2026-06', account_count: 4 }], [], { debugSql: true }); const result = await svc.queryDataset( accounts, { dimensions: ['created_at'], measures: ['account_count'], dateGranularity: 'month' }, @@ -341,7 +360,7 @@ describe('#3588 — the echoed SQL tells the truth on the ObjectQL path', () => }); it('renders the ordering and window that the response rows actually reflect', async () => { - const { svc } = aggService([{ industry: 'Tech', annual_revenue_sum: 1 }]); + const { svc } = aggService([{ industry: 'Tech', annual_revenue_sum: 1 }], [], { debugSql: true }); const result = await svc.queryDataset( accounts, { dimensions: ['industry'], measures: ['annual_revenue_sum'], order: { annual_revenue_sum: 'desc' }, limit: 10 }, @@ -352,7 +371,7 @@ describe('#3588 — the echoed SQL tells the truth on the ObjectQL path', () => }); it('parameterizes filter values rather than inlining them into the echoed statement', async () => { - const { svc } = aggService([{ industry: 'Tech', account_count: 1 }]); + const { svc } = aggService([{ industry: 'Tech', account_count: 1 }], [], { debugSql: true }); const result = await svc.queryDataset( accounts, { dimensions: ['industry'], measures: ['account_count'], runtimeFilter: { industry: 'Tech' } }, diff --git a/packages/services/service-analytics/src/__tests__/sql-echo-debug-gate.test.ts b/packages/services/service-analytics/src/__tests__/sql-echo-debug-gate.test.ts new file mode 100644 index 0000000000..5568b902cf --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/sql-echo-debug-gate.test.ts @@ -0,0 +1,384 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8286] `/api/v1/analytics/query` echoed the executed statement to every + * caller, on production deployments included — while the contract had declared + * the echo debug-only all along (`AnalyticsResultResponseSchema.data.sql`, + * "Executed SQL (if debug enabled)"). This file pins declared = enforced. + * + * ## Why every absence pin here is PAIRED + * + * "`sql` is absent" is trivially green against any fixture that never minted a + * statement — a query that errored early, a strategy with no renderer, a cube + * that does not exist. A whole suite of absence assertions can be vacuous and + * still read as thorough. So no absence claim stands alone here: + * + * - every arm captures, SERVER-SIDE, the statement the run actually produced + * (`executeRawSql` / the delegated service / the ObjectQL renderer), and + * asserts it is a real statement, so the fixture is proven capable of + * disclosure before absence is claimed of it; and + * - every absence arm has a presence twin on the SAME cube, the SAME query and + * the SAME rows, differing in the debug switch and nothing else. + * + * The vocabulary is `cross-field-engine-fallback.test.ts`'s, deliberately: + * absence is `toBeUndefined()`, never falsiness — an empty string would satisfy + * "no SQL" while still being a key on the wire. + * + * ## Every strategy, because the gate is one gate + * + * `NativeSQLStrategy` returns the statement it ran, `ObjectQLStrategy` renders + * a representative one, and `FallbackDelegateStrategy` passes through whatever + * the delegated service minted. All three are driven below on both sides of the + * switch: a gate bolted onto one of them would leave the others serving. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { Cube, FilterCondition } from '@objectstack/spec/data'; +import type { + AnalyticsQuery, + AnalyticsResult, + IAnalyticsService, +} from '@objectstack/spec/contracts'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { DatasetSchema } from '@objectstack/spec/ui'; +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; + +/** The card's cube: a member-walled directory object. */ +const usersCube: Cube = { + name: 'sys_user', + title: 'Users', + sql: 'sys_user', + measures: { + count: { name: 'count', label: 'Count', type: 'count', sql: '*' }, + }, + dimensions: { + id: { name: 'id', label: 'Id', type: 'string', sql: 'id' }, + }, + public: false, +}; + +/** + * The reported request, verbatim — no debug field of any kind, because the + * request contract has none to set (see `AnalyticsServiceConfig.debugSql` for + * why that is deliberate rather than an omission). + */ +const REPRO_QUERY: AnalyticsQuery = { + cube: 'sys_user', + measures: ['count'], + dimensions: [], +}; + +const ROWS = [{ count: '2' }]; + +/** The enumerated member list the card says the echo disclosed the shape of. */ +const MEMBER_SCOPE: FilterCondition = { + id: { $in: ['usr_1', 'usr_2', 'usr_3'] }, +} as FilterCondition; + +/** + * A native-SQL service over {@link usersCube}. `executed` collects the + * statements the driver was actually asked to run — the server-side witness + * that makes an absence assertion mean "withheld" rather than "never existed". + */ +function nativeService(options: { debugSql?: boolean; readScope?: FilterCondition } = {}) { + const executed: string[] = []; + const svc = new AnalyticsService({ + cubes: [usersCube], + logger: silentLogger, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async (_object, sql) => { + executed.push(sql); + return ROWS; + }, + ...(options.readScope ? { getReadScope: () => options.readScope! } : {}), + // Explicit on BOTH sides on purpose: an arm that let the default decide + // would pass or fail with the shell's `NODE_ENV`, which is not a property + // of this code. The default itself is pinned in its own block below. + debugSql: options.debugSql, + }); + return { svc, executed }; +} + +// ───────────────────────────────────────────────────────────────── +// NativeSQLStrategy — the reported path +// ───────────────────────────────────────────────────────────────── + +describe('[#8286] NativeSQLStrategy — the echo is gated, the query is not', () => { + it('debug ON: the response carries the statement that ran', async () => { + const { svc, executed } = nativeService({ debugSql: true }); + + const result = await svc.query(REPRO_QUERY); + + expect(result.rows).toEqual(ROWS); + expect(executed).toHaveLength(1); + expect(executed[0]).toContain('SELECT'); + expect(executed[0]).toContain('"sys_user"'); + // Not merely "a string": the echo is the statement the driver ran, which is + // the only version of it worth handing a debugger. + expect(result.sql).toBe(executed[0]); + }); + + it('debug OFF: the same query, the same rows — the echo is absent', async () => { + const { svc, executed } = nativeService({ debugSql: false }); + + const result = await svc.query(REPRO_QUERY); + + // The pair's whole point: this run DID mint a statement, on the same cube + // and the same query as the arm above. Absence here is a withholding. + expect(executed).toHaveLength(1); + expect(executed[0]).toContain('SELECT'); + expect(result.rows).toEqual(ROWS); + expect(result.sql, 'the echo must be absent, not half-rendered').toBeUndefined(); + }); + + it('debug OFF: the isolation predicate does not travel at all', async () => { + const { svc, executed } = nativeService({ debugSql: false, readScope: MEMBER_SCOPE }); + + const result = await svc.query(REPRO_QUERY); + + // The read scope reached the statement — the disclosure the card describes + // is real, and this fixture reproduces it. + expect(executed[0]).toMatch(/"sys_user"\."id" IN \(/); + // …and none of it reaches the caller: not the predicate's shape (an + // enumerated `id IN (…)` member list rather than an `organization_id` + // comparison), not its parameter arity (which counts the caller's own org), + // not the physical table name. + expect(result.sql).toBeUndefined(); + const onTheWire = JSON.stringify(result); + expect(onTheWire).not.toContain('sys_user'); + expect(onTheWire).not.toContain('IN ('); + expect(onTheWire).not.toContain('SELECT'); + }); + + it('debug ON: the isolation predicate is what the echo shows — same fixture', async () => { + const { svc, executed } = nativeService({ debugSql: true, readScope: MEMBER_SCOPE }); + + const result = await svc.query(REPRO_QUERY); + + expect(result.sql).toBe(executed[0]); + expect(result.sql).toMatch(/"sys_user"\."id" IN \(/); + }); +}); + +// ───────────────────────────────────────────────────────────────── +// FallbackDelegateStrategy — the pass-through path +// ───────────────────────────────────────────────────────────────── + +/** + * A delegated analytics service that always echoes — the real shape of + * `MemoryAnalyticsService` (`@objectstack/driver-memory`), which is what the + * dev stack registers and what `FallbackDelegateStrategy` hands results back + * from untouched. + */ +const DELEGATED_SQL = 'SELECT COUNT(*) AS "count" FROM sys_user'; + +function fallbackService(debugSql: boolean | undefined) { + const delegate: IAnalyticsService = { + query: async (): Promise => ({ + rows: ROWS, + fields: [{ name: 'count', type: 'number' }], + sql: DELEGATED_SQL, + }), + getMeta: vi.fn(), + } as unknown as IAnalyticsService; + + return new AnalyticsService({ + cubes: [usersCube], + logger: silentLogger, + // No native SQL, no aggregate bridge — the delegate is the only strategy + // that can handle this query, so what is measured below is genuinely its + // pass-through and not some other path answering. + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: false, inMemory: false }), + fallbackService: delegate, + debugSql, + }); +} + +describe('[#8286] FallbackDelegateStrategy — the delegate\'s echo is gated too', () => { + it('debug ON: the delegated statement reaches the caller', async () => { + const result = await fallbackService(true).query(REPRO_QUERY); + + expect(result.rows).toEqual(ROWS); + expect(result.sql).toBe(DELEGATED_SQL); + }); + + it('debug OFF: the same delegate, the same rows — the echo is absent', async () => { + const result = await fallbackService(false).query(REPRO_QUERY); + + // The delegate minted the statement either way (the arm above is the proof + // on this very fixture); the seam is what withholds it. A gate bolted onto + // `NativeSQLStrategy` alone would leave this arm serving. + expect(result.rows).toEqual(ROWS); + expect(result.sql, 'the echo must be absent, not half-rendered').toBeUndefined(); + }); +}); + +// ───────────────────────────────────────────────────────────────── +// ObjectQLStrategy — the rendered-echo path +// ───────────────────────────────────────────────────────────────── + +function objectqlService(debugSql: boolean | undefined) { + return new AnalyticsService({ + cubes: [usersCube], + logger: silentLogger, + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async () => ROWS, + debugSql, + }); +} + +describe('[#8286] ObjectQLStrategy — the rendered echo is gated', () => { + it('debug ON: the representative statement reaches the caller', async () => { + const result = await objectqlService(true).query(REPRO_QUERY); + + expect(result.rows).toEqual(ROWS); + expect(result.sql).toContain('SELECT'); + expect(result.sql).toContain('sys_user'); + }); + + it('debug OFF: the same query, the same rows — the echo is absent', async () => { + const result = await objectqlService(false).query(REPRO_QUERY); + + expect(result.rows).toEqual(ROWS); + expect(result.sql, 'the echo must be absent, not half-rendered').toBeUndefined(); + }); +}); + +// ───────────────────────────────────────────────────────────────── +// The production default — the repro, with nothing configured +// ───────────────────────────────────────────────────────────────── + +/** + * The service resolves the default ONCE, in its constructor, so each arm builds + * its service inside its own environment. + */ +async function queryUnderNodeEnv(value: string | undefined): Promise { + const previous = process.env.NODE_ENV; + try { + if (value === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = value; + // `debugSql` is not passed AT ALL — this is the deployment in the report: a + // host that configured nothing, serving a request that asked for nothing. + const svc = new AnalyticsService({ + cubes: [usersCube], + logger: silentLogger, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => ROWS, + }); + return await svc.query(REPRO_QUERY); + } finally { + if (previous === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = previous; + } +} + +describe('[#8286] the default posture — no host choice, no request field', () => { + it('NODE_ENV=production: the echo is absent (the reported deployment)', async () => { + const result = await queryUnderNodeEnv('production'); + expect(result.rows).toEqual(ROWS); + expect(result.sql, 'the echo must be absent, not half-rendered').toBeUndefined(); + }); + + it('NODE_ENV unset: absent too — an unset variable is not development', async () => { + // The 2026-08-06 maintainer ruling for machine-readable environment + // answers, inherited here: of the two ways to be wrong, disclosing on a + // production deployment whose operator forgot the variable is the dangerous + // one. `os start` / `os serve` / `os doctor` all read absence as production. + const result = await queryUnderNodeEnv(undefined); + expect(result.sql, 'the echo must be absent, not half-rendered').toBeUndefined(); + }); + + it('NODE_ENV=development: present — the default is a default, not a removal', async () => { + // The presence half of the default pair. Without it, the two arms above + // would be satisfied by a build that had simply deleted the echo, and this + // suite could not tell a gate from a deletion. + const result = await queryUnderNodeEnv('development'); + expect(result.sql).toContain('SELECT'); + expect(result.sql).toContain('"sys_user"'); + }); + + it('an explicit host choice outranks the environment, in both directions', async () => { + const previous = process.env.NODE_ENV; + try { + process.env.NODE_ENV = 'production'; + const on = await nativeService({ debugSql: true }).svc.query(REPRO_QUERY); + expect(on.sql).toContain('SELECT'); + + process.env.NODE_ENV = 'development'; + const off = await nativeService({ debugSql: false }).svc.query(REPRO_QUERY); + expect(off.sql, 'the echo must be absent, not half-rendered').toBeUndefined(); + } finally { + if (previous === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = previous; + } + }); +}); + +// ───────────────────────────────────────────────────────────────── +// The other caller-facing face, and the one that is NOT gated +// ───────────────────────────────────────────────────────────────── + +const usersDataset = DatasetSchema.parse({ + name: 'user_headcount', + label: 'User headcount', + object: 'sys_user', + include: [], + dimensions: [{ name: 'id', field: 'id', type: 'string' }], + measures: [{ name: 'user_count', aggregate: 'count' }], +}); + +const DATASET_CTX = { tenantId: 'org_A' } as ExecutionContext; + +function datasetService(debugSql: boolean | undefined) { + return new AnalyticsService({ + logger: silentLogger, + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async () => [{ id: 'usr_1', user_count: 2 }], + debugSql, + }); +} + +describe('[#8286] queryDataset inherits the verdict — one gate, not two', () => { + it('debug ON: the dataset response carries the echo', async () => { + const result = await datasetService(true).queryDataset( + usersDataset, + { dimensions: ['id'], measures: ['user_count'] }, + DATASET_CTX, + ); + expect(result.rows).toHaveLength(1); + expect(result.sql).toContain('SELECT'); + }); + + it('debug OFF: the same dataset, the same rows — the echo is absent', async () => { + const result = await datasetService(false).queryDataset( + usersDataset, + { dimensions: ['id'], measures: ['user_count'] }, + DATASET_CTX, + ); + expect(result.rows).toHaveLength(1); + expect(result.sql, 'the echo must be absent, not half-rendered').toBeUndefined(); + }); +}); + +describe('[#8286] the dry-run route keeps its echo — it IS the surface for it', () => { + it('generateSql answers with the statement even with the echo gated off', async () => { + // `/api/v1/analytics/sql` exists to hand back a statement; gating it would + // not narrow an over-serving response, it would delete a declared route. + // The debugging author is meant to come here, which is why the query face + // can afford to say nothing. + const { svc } = nativeService({ debugSql: false, readScope: MEMBER_SCOPE }); + + const { sql } = await svc.generateSql(REPRO_QUERY); + + expect(sql).toContain('SELECT'); + expect(sql).toMatch(/"sys_user"\."id" IN \(/); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 369992e432..e7e1761613 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -16,7 +16,7 @@ import type { Dataset } from '@objectstack/spec/ui'; // differently from objectui's `pickLocalized` with neither end erroring. import { resolveI18nLabel } from '@objectstack/spec/ui'; import type { Logger } from '@objectstack/spec/contracts'; -import { createLogger, bucketKeyToCalendarRange, zonedDateStartToUtcMs } from '@objectstack/core'; +import { createLogger, getEnv, bucketKeyToCalendarRange, zonedDateStartToUtcMs } from '@objectstack/core'; // [#6615] The Postgres `"x" of relation "y"` phrase, owned once. This is the // only reason this package depends on `@objectstack/types` — see the module's // docblock for why the edge is acyclic and why it was worth adding. @@ -575,6 +575,43 @@ export interface AnalyticsServiceConfig { objectName: string, context?: ExecutionContext, ) => Promise[] | null>; + + /** + * [#8286] Echo the executed statement back to the CALLER in + * `AnalyticsResult.sql`. **Off unless this host opts in.** + * + * The contract has always declared the echo debug-only — + * `AnalyticsResultResponseSchema.data.sql` (`spec/api/analytics.zod.ts`) is + * `optional()` and describes itself as "Executed SQL (if debug enabled)" — + * but no implementation ever read a debug switch, so every `/analytics/query` + * response carried the statement, on production deployments included. This + * field is that switch: declared = enforced, restored at the one seam the + * response leaves through ({@link AnalyticsService.query}). + * + * **Default** — `NODE_ENV === 'development'`, and nothing else. In + * particular an UNSET `NODE_ENV` counts as production and the echo stays + * off: that is the maintainer's 2026-08-06 ruling for machine-readable + * environment answers (see `resolveDiscoveryEnvironment` and the note at + * `runtime/src/http-dispatcher.ts`), and of the two ways to be wrong, + * disclosing on a production deployment whose operator forgot the variable + * is the dangerous one. `os start` forces `NODE_ENV='production'` when + * unset, `os serve` resolves `NODE_ENV || 'production'`, `os doctor` derives + * the same expression — this switch now reads the absence the same way. + * + * **Why not a request field.** There is none, deliberately: a caller-set + * debug flag would let any tenant re-open the disclosure on demand, which is + * the shape of the defect rather than a fix for it. The echo is a HOST + * decision, and the caller-facing surface for "show me the SQL" already + * exists as the dedicated dry-run route `/api/v1/analytics/sql` + * (`generateSql`), which this switch does not touch. + * + * **Why not the plugin's `debug` (log) option.** Server-side log verbosity + * and what travels to a caller are different decisions with different blast + * radii; folding them together means a support engineer raising log level on + * a live deployment silently reopens the disclosure. Two switches, named for + * what they open. + */ + debugSql?: boolean; } /** @@ -632,6 +669,11 @@ export class AnalyticsService implements IAnalyticsService { private readonly isExternalObject?: AnalyticsServiceConfig['isExternalObject']; /** [#3867] One-shot flag for the {@link assertInferableCube} stand-down warning. */ private warnedNoObjectRegistry = false; + /** + * [#8286] Does the executed statement travel back to the caller? + * See {@link AnalyticsServiceConfig.debugSql} for the switch and its default. + */ + private readonly debugSql: boolean; readonly cubeRegistry: CubeRegistry; private readonly logger: Logger; @@ -653,6 +695,10 @@ export class AnalyticsService implements IAnalyticsService { this.getObjectFieldNames = config.getObjectFieldNames; this.getObjectDatasource = config.getObjectDatasource; this.isExternalObject = config.isExternalObject; + // [#8286] Resolved ONCE, at construction, from the host's explicit choice + // or from `NODE_ENV`. An unset `NODE_ENV` is not development — see the + // field's doc for the ruling this inherits. + this.debugSql = config.debugSql ?? (getEnv('NODE_ENV') === 'development'); // Compile + register pre-defined datasets (ADR-0021). if (config.datasets) { @@ -810,7 +856,14 @@ export class AnalyticsService implements IAnalyticsService { const strategy = this.resolveStrategy(query, ctx, skip); this.logger.debug(`[Analytics] Query on cube "${query.cube}" → ${strategy.name}`); try { - return await strategy.execute(query, ctx); + // [#8286] ONE gate, every strategy. This is the single seam a strategy + // result leaves the service through — `NativeSQLStrategy` returns the + // statement it ran, `ObjectQLStrategy` renders a representative one, + // and `FallbackDelegateStrategy` passes through whatever the delegated + // service minted (e.g. `MemoryAnalyticsService`, which always echoes). + // Gating any one of those would leave the others serving, which is the + // shape the defect already had. + return this.applySqlEchoPolicy(await strategy.execute(query, ctx)); } catch (e) { if ((e as { code?: string })?.code === 'RAW_SQL_UNSUPPORTED') { this.logger.warn( @@ -824,6 +877,35 @@ export class AnalyticsService implements IAnalyticsService { } } + /** + * [#8286] Withhold the executed statement unless this host enabled the echo. + * + * Applied at {@link query}, which is the response-assembly seam for BOTH + * faces that serve callers: `/api/v1/analytics/query` calls it directly, and + * `queryDataset` reaches it through `DatasetExecutor`, so a dataset response + * inherits the same verdict without a second gate to keep in step. + * `generateSql` — the dedicated `/api/v1/analytics/sql` dry-run route — is + * deliberately NOT gated: asking for the statement is that route's entire + * purpose, and it is the surface a debugging author is meant to use. + * + * What the echo disclosed, and why "it is only a table name" understates it: + * the statement carries the compiled read scope, i.e. the SHAPE of the + * isolation predicate (`"sys_user"."id" IN ($2, $3, …)` rather than an + * `organization_id` comparison) plus its bound-parameter arity, which counts + * the caller's own org membership. No wall was breached by it — the echo is + * information disclosure, and this is the disclosure closing. + */ + private applySqlEchoPolicy(result: AnalyticsResult): AnalyticsResult { + if (this.debugSql || result?.sql === undefined) return result; + // Copy-and-delete rather than mutate: the strategy (or a delegated + // fallback service) owns the object it returned, and a cached result on + // the other side of that boundary must not lose a field because this + // service handed it to a caller once. + const withheld: AnalyticsResult = { ...result }; + delete withheld.sql; + return withheld; + } + /** * Compile a `dataset` (ADR-0021) and register its Cube + join allowlist so it * can be queried by name. Idempotent (re-registering overwrites). Returns the diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index 2dd6cbcbb7..651547f979 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -154,6 +154,17 @@ export interface AnalyticsServicePluginOptions { getAllowedRelationships?: (cubeName: string) => Set | undefined; /** Enable debug logging. */ debug?: boolean; + /** + * [#8286] Echo the executed statement back to CALLERS in + * `AnalyticsResult.sql` (`/api/v1/analytics/query`). + * + * Distinct from {@link AnalyticsServicePluginOptions.debug} above, which is + * server-side log verbosity only: raising log level must never widen what + * travels to a tenant. Default and rationale live with the service config — + * see `AnalyticsServiceConfig.debugSql`. Undefined here means "no host + * choice", which the service resolves to development-only. + */ + debugSql?: boolean; } /** @@ -568,6 +579,11 @@ export class AnalyticsServicePlugin implements Plugin { coerceTemporalFilterColumn, relationshipResolver, labelResolver, + // [#8286] Passed through as authored — `undefined` is "this host did not + // choose", which the service resolves to development-only. Defaulting it + // here would be a second copy of that decision, drifting the moment one + // of the two moves. + debugSql: this.options.debugSql, // Source-field metadata behind the display chains on result columns: // ADR-0053 currency (`currencyConfig.defaultCurrency`) and percent scale // (`max`, which is what marks whole-percent storage — objectui#3136).