From 87b47f4b3d8d32b79486682d9109ccb596d69262 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:22:14 +0800 Subject: [PATCH] fix(analytics): fail closed on cross-object aggregation the ObjectQL path cannot join (#3654) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit engine.aggregate() has no join — it never expands a lookup and the SQL driver's aggregate emits no JOIN. A dotted dimension/measure like account.region reaching ObjectQLStrategy (the fallback NativeSQL declines on: date-granularity bucketing, in-memory driver, federated objects) failed SILENTLY: the in-memory path bucketed every row under one (null) group and summed the whole table into it — a plausible number that is actually a mislabelled full-table total; the native path errored on the unresolved column. ObjectQLStrategy now rejects any cross-object reference outright, with a clear message, before the query reaches the engine. This generalizes the #3597 guard (assertJoinedScopesEnforceable), which only rejected when the joined object carried a read scope AND early-returned when no read-scope provider was configured — so the silent (null) bucket still shipped on unsecured / in-memory setups. The new guard is unconditional and subsumes #3597: a rejected query never loads the joined object, so nothing is left unscoped. Cross-object datasets are unaffected on NativeSQLStrategy, which hand-compiles the LEFT JOINs (and scopes each). This only changes the fallback path — a silent wrong answer becomes a loud, actionable error. Full lookup-traversal in the aggregate path is left as follow-up (#3654). Tests: three cross-object cases (joined-object scope / base-only scope / NO provider — the #3654 silent-(null) case) now reject; reverting the guard turns all three red. The engine-level in-memory repro is in #3654. Co-Authored-By: Claude --- .changeset/objectql-crossobj-fail-closed.md | 26 ++++++ .../src/__tests__/objectql-read-scope.test.ts | 44 ++++++---- .../src/strategies/objectql-strategy.ts | 80 +++++++++++-------- 3 files changed, 101 insertions(+), 49 deletions(-) create mode 100644 .changeset/objectql-crossobj-fail-closed.md diff --git a/.changeset/objectql-crossobj-fail-closed.md b/.changeset/objectql-crossobj-fail-closed.md new file mode 100644 index 000000000..e6ac68963 --- /dev/null +++ b/.changeset/objectql-crossobj-fail-closed.md @@ -0,0 +1,26 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(analytics): fail closed on cross-object aggregation the ObjectQL path cannot join (#3654) + +`engine.aggregate()` has no join — it never expands a lookup and the SQL driver's +aggregate emits no `JOIN`. So a dotted dimension/measure like `account.region` +reaching `ObjectQLStrategy` (the fallback NativeSQL declines: date-granularity +bucketing, in-memory driver, federated objects) failed SILENTLY: the in-memory +path bucketed every row under one `(null)` group and summed the whole table into +it (a plausible number that is actually a mislabelled full-table total), and the +native path errored on the unresolved column. + +`ObjectQLStrategy` now rejects any cross-object reference outright, with a clear +message, before the query reaches the engine. This generalizes the #3597 guard +(which only rejected when the joined object carried a read scope, and skipped the +check entirely when no read-scope provider was configured — so the silent +`(null)` bucket still shipped on unsecured/in-memory setups) into an +unconditional one, and subsumes it: a rejected query never loads the joined +object, so there is nothing left unscoped. + +Cross-object datasets are unaffected on `NativeSQLStrategy`, which hand-compiles +the LEFT JOINs (and scopes each). This only changes the fallback path, turning a +silent wrong answer into a loud, actionable error. Full lookup-traversal support +in the aggregate path is left as follow-up (see #3654). diff --git a/packages/services/service-analytics/src/__tests__/objectql-read-scope.test.ts b/packages/services/service-analytics/src/__tests__/objectql-read-scope.test.ts index d1f7583e9..2ad14fd1a 100644 --- a/packages/services/service-analytics/src/__tests__/objectql-read-scope.test.ts +++ b/packages/services/service-analytics/src/__tests__/objectql-read-scope.test.ts @@ -218,7 +218,7 @@ describe('ObjectQLStrategy — read scope (ADR-0021 D-C, #3597)', () => { }); }); -describe('ObjectQLStrategy — joined-object scope is fail-closed (#3597)', () => { +describe('ObjectQLStrategy — cross-object references are fail-closed (#3654, subsumes #3597)', () => { const joined = DatasetSchema.parse({ name: 'sales_by_account', label: 'Sales by account', @@ -228,32 +228,42 @@ describe('ObjectQLStrategy — joined-object scope is fail-closed (#3597)', () = measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], }); - function makeJoinedService(scopeFor: (o: string) => FilterCondition | undefined) { + function makeJoinedService(scopeFor?: (o: string) => FilterCondition | undefined) { const compiled = compileDataset(joined); - return new AnalyticsService({ + // A stub that would happily "succeed" with a garbage single-(null) bucket if + // the guard didn't fire — so a passing test proves the REJECTION, not luck. + let called = false; + const svc = new AnalyticsService({ cubes: [compiled.cube], queryCapabilities: objectqlOnly, - executeAggregate: async () => [], - getReadScope: (o: string) => scopeFor(o), + executeAggregate: async () => { called = true; return [{ 'account.region': null, revenue: 999 }]; }, + getReadScope: scopeFor ? (o: string) => scopeFor(o) : undefined, getAllowedRelationships: () => compiled.allowedRelationships, }); + return { svc, wasExecuted: () => called }; } - it('denies the query when a referenced joined object carries a scope', async () => { - const service = makeJoinedService(() => ({ organization_id: 'org_A' })); + const q = { cube: 'sales_by_account', dimensions: ['region'], measures: ['revenue'] }; - await expect( - service.query({ cube: 'sales_by_account', dimensions: ['region'], measures: ['revenue'] }, ctxA), - ).rejects.toThrow(/cannot enforce the read scope of joined object\(s\) "account"/); + it('rejects a cross-object grouping the engine cannot join — even with a joined-object scope', async () => { + const { svc, wasExecuted } = makeJoinedService(() => ({ organization_id: 'org_A' })); + await expect(svc.query(q, ctxA)).rejects.toThrow( + /cannot group or aggregate across a relationship .*"account\.region"/, + ); + expect(wasExecuted()).toBe(false); // never reached the engine }); - it('allows the query when only the base object carries a scope', async () => { - const service = makeJoinedService((o) => - o === 'opportunity' ? { organization_id: 'org_A' } : undefined, - ); + it('rejects when only the base object carries a scope (the query is still unjoinable)', async () => { + const { svc } = makeJoinedService((o) => (o === 'opportunity' ? { organization_id: 'org_A' } : undefined)); + await expect(svc.query(q, ctxA)).rejects.toThrow(/cannot group or aggregate across a relationship/); + }); - await expect( - service.query({ cube: 'sales_by_account', dimensions: ['region'], measures: ['revenue'] }, ctxA), - ).resolves.toBeDefined(); + it('rejects with NO read-scope provider at all — the #3654 silent-(null) case (security off)', async () => { + // Before #3654 this path ran unguarded (the guard early-returned without a + // getReadScope), and the member got a single mislabelled `(null)` bucket + // summing the whole table. Now it fails loud. + const { svc, wasExecuted } = makeJoinedService(/* no provider */); + await expect(svc.query(q)).rejects.toThrow(/cannot group or aggregate across a relationship/); + expect(wasExecuted()).toBe(false); }); }); diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 8436e53d8..63024bcd4 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -79,10 +79,17 @@ export class ObjectQLStrategy implements AnalyticsStrategy { } } - // ADR-0021 D-C — the read scope (tenant + RLS) MUST be ANDed in before the - // query leaves the strategy. Rejects the query outright when a joined object - // carries a scope this path cannot express (#3597). - this.assertJoinedScopesEnforceable(cube, query, groupBy, filter, ctx); + // Reject cross-object grouping/aggregation this path cannot perform — the + // engine has no join, so it would silently mis-bucket or error (#3654). This + // runs BEFORE scope injection and subsumes the #3597 joined-scope concern: + // a rejected query never loads the joined object, so nothing is left + // unscoped. Independent of read scope, so it fires with security off too. + this.assertNoCrossObjectReferences(cube, query, groupBy, filter); + + // ADR-0021 D-C — the base object's read scope (tenant + RLS) MUST be ANDed + // in before the query leaves the strategy (#3597). + // (`assertNoCrossObjectReferences` above guarantees `objectName` is the only + // object in play, so a single base-object scope is sufficient here.) const rows = await ctx.executeAggregate!(objectName, { // Structured groupBy items ({field, dateGranularity}) pass through the @@ -185,58 +192,67 @@ export class ObjectQLStrategy implements AnalyticsStrategy { } /** - * Fail-closed guard for cross-object queries (#3597). + * Fail-closed guard for cross-object references on the ObjectQL path + * (#3654, subsuming #3597). * - * `engine.aggregate`'s `where` addresses the BASE object. A dotted member - * (`account.region`) is traversed by the engine through the lookup field, but - * there is no place in this call shape to hang a predicate on the JOINED - * object — so a joined object's read scope cannot be enforced here. + * `engine.aggregate()` has NO join: it never expands a lookup, and the SQL + * driver's aggregate emits no `JOIN`. A dotted member like `account.region` + * therefore reaches the engine as a bare column that no table in the query + * provides. The failure is SILENT and wrong, not loud: + * - native SQL path → "column account.region does not exist" (a hard error); + * - in-memory path → `row['account.region']` is always `undefined`, so every + * row collapses into ONE `(null)` bucket and the measure is summed across + * the whole table — a plausible-looking number that is actually a + * full-table total mislabelled `(null)`. * - * `NativeSQLStrategy` can express it (alias-qualified WHERE per join) and does. - * When this path cannot, we reject rather than run a partially-scoped query: - * the same posture as `resolveReadScopes` (throws rather than emit unscoped - * SQL) and `compileScopedFilterToSql` (throws rather than drop a predicate). + * So we reject any cross-object reference OUTRIGHT — regardless of read scope. + * This also subsumes the #3597 concern: because the joined object is never + * loaded on this path, there is nothing to leave unscoped. Cross-object + * datasets are served by `NativeSQLStrategy`, which hand-compiles the LEFT + * JOINs (and scopes each one); this path is the fallback NativeSQL declines + * (date-granularity bucketing, in-memory driver, federated objects), and it + * cannot join, so "loud rejection" beats "silent wrong answer". * - * Only joins the query ACTUALLY references are considered — the scope map is - * a deliberate superset of what gets scanned, so keying off the map alone - * would reject queries that never touch the joined table. + * Detection is on the RESOLVED field names (post-`resolveFieldName`), so a + * dotted dimension the cube flattens to a real column is NOT flagged — only + * genuinely-unresolved relationship traversals are. */ - private assertJoinedScopesEnforceable( + private assertNoCrossObjectReferences( cube: Cube, query: AnalyticsQuery, groupBy: Array, filter: Record, - ctx: StrategyContext, ): void { - if (typeof ctx.getReadScope !== 'function') return; - - // Field names as they will reach the engine. A dotted name is a relationship - // traversal; its first segment is the join alias. + // Field names as they will reach the engine. A dotted name whose first + // segment names a DIFFERENT object is a relationship traversal the engine + // cannot perform in an aggregate. const referenced = [ ...groupBy.map((g) => (typeof g === 'string' ? g : g.field)), ...Object.keys(filter), ...(query.measures ?? []).map((m) => this.resolveMeasureAggregation(cube, m).field), ]; + const baseObject = this.extractObjectName(cube); const offending = new Set(); for (const fieldName of referenced) { if (!fieldName.includes('.')) continue; const alias = fieldName.split('.')[0]; const joinedObject = cube.joins?.[alias]?.name ?? alias; - if (joinedObject === this.extractObjectName(cube)) continue; - const scope = ctx.getReadScope(joinedObject); - if (scope !== undefined && scope !== null) offending.add(joinedObject); + if (joinedObject === baseObject) continue; + offending.add(fieldName); } if (offending.size === 0) return; throw new Error( - `[Analytics] ObjectQLStrategy cannot enforce the read scope of joined ` + - `object(s) ${[...offending].map((o) => `"${o}"`).join(', ')} — denying the ` + - `query (fail-closed, ADR-0021 D-C). This path reaches the joined table ` + - `through the base object's lookup, where a per-join security predicate ` + - `cannot be expressed. Run this query on a driver that supports native SQL ` + - `(NativeSQLStrategy scopes each join), or drop the cross-object ` + - `dimension/measure from the query.`, + `[Analytics] ObjectQLStrategy cannot group or aggregate across a ` + + `relationship (cross-object reference(s): ` + + `${[...offending].map((f) => `"${f}"`).join(', ')}). ` + + `engine.aggregate() does not join, so the referenced object is never ` + + `loaded — this path would silently bucket every row under a single ` + + `"(null)" group (or error outright on a SQL driver), and could not scope ` + + `the joined object's rows. Run this query where NativeSQLStrategy handles ` + + `it (a raw-SQL/JOIN-capable driver with no date-granularity bucketing), or ` + + `drop the cross-object dimension/measure from the query.`, ); }