fix(analytics): fail closed on cross-object aggregation the ObjectQL path cannot join (#3654)

.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).

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);
   });
 });

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<string | { field: string; dateGranularity: string }>,
     filter: Record<string, unknown>,
-    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<string>();
     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.`,
     );
   }