From aba91e6b02ed33da110bad7782eb5083e55c9412 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 04:00:12 +0000 Subject: [PATCH] fix(service-analytics): guard fetchRecordLabels with assertReadScopeCannotVacate The record-label hook is a fourth consumer of the same readScopeProvider output the three unified faces guard, and it met neither compileScopedFilterToSql nor the vacancy guard: it $ands the referenced object's scope with `id $in [...]` and hands that straight to executeAggregate. A vacating spelling from an out-of-repo getReadScope producer therefore let a row-granular per-record read run effectively unscoped, surfacing exactly the display names the referenced object's RLS exists to hide. Call the already-exported assertReadScopeCannotVacate before the filter composition, in the same envelope as the siblings (READ_SCOPE_COMPILE_FAILED / 500). Placement mirrors ObjectQLStrategy.resolveFkAttr, this hook's structural twin: after the early returns (a call that reads nothing cannot widen anything) and before the chunk loop (one scope, one verdict). Zero compiler change. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .changeset/tidy-ducks-tickle.md | 9 + .../record-label-read-scope-vacancy.test.ts | 308 ++++++++++++++++++ .../services/service-analytics/src/plugin.ts | 25 ++ 3 files changed, 342 insertions(+) create mode 100644 .changeset/tidy-ducks-tickle.md create mode 100644 packages/services/service-analytics/src/__tests__/record-label-read-scope-vacancy.test.ts diff --git a/.changeset/tidy-ducks-tickle.md b/.changeset/tidy-ducks-tickle.md new file mode 100644 index 0000000000..b258546ec5 --- /dev/null +++ b/.changeset/tidy-ducks-tickle.md @@ -0,0 +1,9 @@ +--- +'@objectstack/service-analytics': patch +--- + +Guard the analytics record-label lookup with `assertReadScopeCannotVacate` — the fourth read-scope door + +`AnalyticsServicePlugin`'s `fetchRecordLabels` hook `$and`s the **referenced** object's read scope with an `id $in [...]` filter and hands the result straight to `executeAggregate`. Unlike the three faces unified previously (the ObjectQL engine merge, the `/analytics/sql` echo merge, and `NativeSQLStrategy.applyReadScope`), it met neither `compileScopedFilterToSql` nor the vacancy guard, so a read scope that lowers to a boolean constant — the `$not`-over-`$in: []` family reachable from any out-of-repo `StrategyContext.getReadScope` producer — let that per-record read run effectively unscoped for the ids in hand, surfacing the display names the referenced object's RLS exists to hide. + +The hook now calls the already-exported `assertReadScopeCannotVacate` on the referenced object's scope before composing the filter, refusing in the same envelope as its siblings (`READ_SCOPE_COMPILE_FAILED` / 500). No behaviour changes for scopes that bind: an ordinary referenced-object scope still narrows the label lookup, and the `$in: []` zero-rows reduction (including the live RLS composite that pairs it with an own-rows grant) still passes through untouched. The read-scope SQL compiler is unchanged. diff --git a/packages/services/service-analytics/src/__tests__/record-label-read-scope-vacancy.test.ts b/packages/services/service-analytics/src/__tests__/record-label-read-scope-vacancy.test.ts new file mode 100644 index 0000000000..1e92d42042 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/record-label-read-scope-vacancy.test.ts @@ -0,0 +1,308 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14329] The FOURTH read-scope door — `AnalyticsServicePlugin`'s + * `fetchRecordLabels` hook — answers the same verdict as the other three. + * + * #13640 guarded the ObjectQL ENGINE merge and #13926 the `/analytics/sql` + * ECHO merge plus `NativeSQLStrategy.applyReadScope`; the three-faces file + * next door pins those. This hook is a FOURTH consumer of the very same + * `readScopeProvider` output, reached by a different route entirely + * (`AnalyticsService.queryDataset` → `resolveScope` → `dimension-labels.ts` → + * `DimensionLabelDeps.fetchRecordLabels`, the closure `plugin.ts` builds), and + * it met NEITHER `compileScopedFilterToSql` nor `assertReadScopeCannotVacate`: + * it `$and`s the REFERENCED object's scope with `id $in [...]` and hands that + * straight to `executeAggregate`. + * + * So a vacating scope spelling from an out-of-repo `getReadScope` producer + * (`StrategyContext.getReadScope` is a spec contract — that population is + * exactly who this contract exists for, and the one with no producer-side + * #13570 guard) let this per-record read run effectively unscoped for the ids + * in hand, surfacing the display names the referenced object's RLS exists to + * hide. The leak is row-granular by construction: `group by (id, name)` is a + * record read dressed as an aggregate. + * + * ## What is measured here, and what is NOT + * + * These cases drive the REAL plugin wiring — `new AnalyticsServicePlugin(...).init(ctx)` + * — so the closure under test is the one `plugin.ts` actually ships, not a + * stub standing in for it. What they do NOT re-measure is the ENGINE's + * lowering of a vacating scope: that table (which spellings come back with the + * whole table, driven against a real `SqliteWasmDriver`) is + * `read-scope-vacancy-three-faces.test.ts`'s, and re-deriving it here would be + * a second copy of one ruling. The fixture engine below therefore honours the + * filter it is handed by a small, deliberately obvious evaluator — which is + * the right authority for THIS seam's question: *does the hook forward a scope + * that a scope-honouring engine can narrow by, and does it refuse the + * spellings that cannot narrow anything at all?* + * + * ## Two label passes, two DIFFERENT dispositions — both fail closed + * + * A refusal from this hook surfaces differently depending on which of + * `queryDataset`'s two label passes raised it, and both are asserted below + * because a reader who checks only one will conclude the other is unguarded: + * + * - **sort-key pass** (`order` on a lookup dimension, #3680) runs inside + * `DatasetExecutor.execute`, whose catch in `queryDataset` re-throws a + * DECLARED ADR-0112 envelope untouched (`hasDeclaredErrorEnvelope`). The + * refusal reaches the caller as itself — `READ_SCOPE_COMPILE_FAILED` / 500. + * - **display pass** (#3602) is wrapped in its own try/catch that degrades to + * a `warn` and leaves raw ids rendering. That is not this card weakening: + * it is the disposition #3602 already chose for this surface one frame up + * (`dimension-labels.ts` skips a dimension's labels rather than fetch + * unscoped when the scope cannot be resolved), and it is fail-CLOSED — no + * name is fetched, so none can leak. + * + * The security property is therefore identical on both passes and is asserted + * as such: **the referenced object is never read at all**. A bare "it threw" + * would not distinguish that from a read that happened and then threw. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import type { FilterCondition } from '@objectstack/spec/data'; +import { AnalyticsService } from '../analytics-service.js'; +import { AnalyticsServicePlugin } from '../plugin.js'; + +const CTX = { tenantId: 'org_A', userId: 'u_me' } as ExecutionContext; + +/** Tasks grouped by a lookup dimension whose target is `crm_account`. */ +const DATASET = DatasetSchema.parse({ + name: 'tasks_by_account', + label: 'Tasks by account', + object: 'task', + dimensions: [{ name: 'account', field: 'account', type: 'lookup', label: 'Account' }], + measures: [{ name: 'cnt', aggregate: 'count' }], +}); + +/** + * Referenced-object fixture rows. `organization_id` is what an ordinary + * tenant scope narrows by; `owner` is what the emptied-membership spellings + * address. `acc2` is the row an ordinary `org_A` scope must NOT surface. + */ +const ACCOUNTS = [ + { id: 'acc1', name: 'Acme Corp', organization_id: 'org_A', owner: 'u_me' }, + { id: 'acc2', name: 'Umbrella Ltd', organization_id: 'org_B', owner: 'u_other' }, +]; + +/** The grouped base aggregate: both FK ids reach the label pass. */ +const TASK_ROWS = [ + { account: 'acc1', cnt: 3 }, + { account: 'acc2', cnt: 1 }, +]; + +/** + * A deliberately small filter evaluator for the FIXTURE rows — equality, + * `$in`, `$and`, `$or`. It exists so "an ordinary scope still narrows" and + * "`$in: []` still reduces to zero rows" are read off real returned rows + * rather than off the filter object, which would only echo the assertion. + * + * ⛔ Not an engine-lowering model, and not where a vacating spelling's row + * consequence is established: an unrecognised operator throws rather than + * quietly matching, so a spelling this cannot judge fails loudly instead of + * manufacturing a comfortable answer. The measured lowering table lives in + * `read-scope-vacancy-three-faces.test.ts`, against a real driver. + */ +function matches(row: Record, filter: unknown): boolean { + if (filter == null) return true; + if (typeof filter !== 'object' || Array.isArray(filter)) { + throw new Error(`[fixture] not a filter node: ${JSON.stringify(filter)}`); + } + return Object.entries(filter as Record).every(([key, value]) => { + if (key === '$and') return (value as unknown[]).every((n) => matches(row, n)); + if (key === '$or') return (value as unknown[]).some((n) => matches(row, n)); + if (value !== null && typeof value === 'object' && !Array.isArray(value)) { + const ops = Object.entries(value as Record); + return ops.every(([op, comparand]) => { + if (op === '$in') return (comparand as unknown[]).includes(row[key]); + throw new Error(`[fixture] unsupported operator ${op} — this evaluator judges no spelling it was not written for`); + }); + } + return row[key] === value; + }); +} + +type EngineCall = { object: string; where?: Record }; + +function fakePluginContext(services: Record) { + const registered: Record = {}; + const warn = vi.fn(); + return { + registered, + warn, + ctx: { + getService: (name: string) => services[name] ?? registered[name], + registerService: (name: string, svc: unknown) => { registered[name] = svc; }, + replaceService: (name: string, svc: unknown) => { registered[name] = svc; }, + logger: { info() {}, warn, error() {}, debug() {} }, + }, + }; +} + +const objectqlOnly = () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }); + +/** + * Drive the label path through the real plugin wiring. + * + * `order` selects WHICH label pass runs: with it, the sort-key pass (#3680) + * resolves labels inside `DatasetExecutor.execute`; without it, only the + * display pass (#3602) does. The two have different refusal dispositions, so + * every case below states which one it is exercising. + */ +async function runLabels(opts: { scope: FilterCondition | undefined; order?: boolean }) { + const seen: EngineCall[] = []; + const engine = { + aggregate: async (object: string, options: Record) => { + seen.push({ object, where: options.where as Record | undefined }); + if (object === 'task') return TASK_ROWS; + return ACCOUNTS.filter((r) => matches(r, options.where)).map((r) => ({ id: r.id, name: r.name, _c: 1 })); + }, + getObject: (name: string) => + name === 'task' + ? { fields: { account: { type: 'lookup', reference: 'crm_account' } } } + : name === 'crm_account' + ? { fields: { name: { type: 'text' } } } + : undefined, + }; + const { ctx, registered, warn } = fakePluginContext({ data: engine }); + + await new AnalyticsServicePlugin({ + queryCapabilities: objectqlOnly, + getReadScope: (object: string) => (object === 'crm_account' ? opts.scope : undefined), + }).init(ctx as never); + + const run = () => + (registered.analytics as AnalyticsService).queryDataset( + DATASET as never, + { + dimensions: ['account'], + measures: ['cnt'], + ...(opts.order ? { order: { account: 'asc' } } : {}), + } as never, + CTX, + ); + + return { run, seen, warn }; +} + +/** Did anything read the REFERENCED object? The security question, directly. */ +const readReferenced = (seen: EngineCall[]) => seen.filter((c) => c.object === 'crm_account'); + +/** + * The vacating family, as measured in `read-scope-sql.ts`'s #13640 section: + * every one of these came back with the whole table from a real engine. + * `$nin: []` is refused at any polarity (matching `compileOperator`'s own + * `$nin` arm); the rest are emptied POSITIVE memberships under an odd number + * of negations, which is what makes them vacate. + */ +const VACATING: Array<[string, FilterCondition]> = [ + ['empty $nin', { owner: { $nin: [] } } as FilterCondition], + ['$not over empty $in', { $not: { owner: { $in: [] } } } as FilterCondition], + ['$not over a bare empty array', { $not: { owner: [] } } as FilterCondition], + ['$not over a multi-key operator object holding an empty $in', { $not: { owner: { $in: [], $ne: 'u_other' } } } as FilterCondition], + ['a vacating arm inside an $or', { $or: [{ $not: { owner: { $in: [] } } }, { owner: 'u_me' }] } as FilterCondition], +]; + +describe('#14329 — a vacating referenced-object scope is refused before the label lookup runs', () => { + it.each(VACATING)('sort-key pass: %s refuses in the sibling envelope', async (_name, scope) => { + const { run, seen } = await runLabels({ scope, order: true }); + + // ADR-0112 envelope, `code` AND `status` — the same two the three sibling + // faces answer with. A bare `toThrow` would stay green against a driver + // throwing a naked `Error`, which is the failure this assertion exists to + // exclude. + const err = await run().then( + () => { throw new Error('expected a refusal, got a result'); }, + (e: unknown) => e as { code?: unknown; status?: unknown; message?: string }, + ); + expect(err.code).toBe('READ_SCOPE_COMPILE_FAILED'); + expect(err.status).toBe(500); + expect(String(err.message)).toContain('read scope for "crm_account"'); + + // The other half of a refusal pin: the referenced object was NEVER read. + // "It threw" alone does not distinguish a guard from a leak followed by a + // throw — and the leak is precisely a read that happened. + expect(readReferenced(seen)).toEqual([]); + // The base aggregate still ran: the refusal is scoped to the label door. + expect(seen.map((c) => c.object)).toEqual(['task']); + }); + + it.each(VACATING)('display pass: %s fails closed to raw ids without reading the target', async (_name, scope) => { + const { run, seen, warn } = await runLabels({ scope }); + + // The display pass has its own catch (analytics-service.ts) that degrades + // to a warn — the #3602 disposition for this surface. So the CALLER sees + // rows, and what matters is that no name was fetched to put in them. + const result = await run() as unknown as { rows: Record[] }; + expect(readReferenced(seen)).toEqual([]); + expect(result.rows.map((r) => r.account)).toEqual(['acc1', 'acc2']); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('dimension label resolution failed')); + }); +}); + +describe('#14329 over-denial controls — the guard refuses ONLY the vacating shapes', () => { + it('an ordinary referenced-object scope still narrows the label lookup', async () => { + const { run, seen } = await runLabels({ scope: { organization_id: 'org_A' } as FilterCondition }); + + const result = await run() as unknown as { rows: Record[] }; + + // Preservation pin — the scope reached the engine `$and`-composed with the + // id filter, never key-merged, so it cannot be displaced by the ids. + const labelCall = readReferenced(seen); + expect(labelCall).toHaveLength(1); + expect(labelCall[0].where).toEqual({ + $and: [{ id: { $in: ['acc1', 'acc2'] } }, { organization_id: 'org_A' }], + }); + + // ...and the NARROWED RESULT SET, not merely "no throw": `acc1` is in the + // tenant and renders its name; `acc2` is out and keeps its raw id, which is + // the whole point of scoping this read. + expect(result.rows.map((r) => r.account)).toEqual(['Acme Corp', 'acc2']); + }); + + it('the `$in: []` zero-rows reduction still yields no labels and no refusal', async () => { + // Positive polarity: the ruled #5322/#5243 reduction to constant FALSE. + // Narrowing at its own arm — the SAFE direction on a read scope — and + // deliberately NOT refused, here or at any sibling door. + const { run, seen } = await runLabels({ scope: { owner: { $in: [] } } as FilterCondition }); + + const result = await run() as unknown as { rows: Record[] }; + + expect(readReferenced(seen)).toHaveLength(1); + expect(readReferenced(seen)[0].where).toEqual({ + $and: [{ id: { $in: ['acc1', 'acc2'] } }, { owner: { $in: [] } }], + }); + // Zero rows came back, so no label overwrites a raw id — and no refusal. + expect(result.rows.map((r) => r.account)).toEqual(['acc1', 'acc2']); + }); + + it('the live #13570 RLS composite keeps own rows flowing', async () => { + // `{ $or: [{ owner: { $in: [] } }, { owner: 'u_me' }] }` — an emptied + // membership beside an own-rows grant, which the RLS compiler really emits + // when a membership set resolves empty. Refusing it would 500 every + // analytics query for such a user, the outcome #13571's verdict rejected. + const { run, seen } = await runLabels({ + scope: { $or: [{ owner: { $in: [] } }, { owner: 'u_me' }] } as FilterCondition, + }); + + const result = await run() as unknown as { rows: Record[] }; + + expect(readReferenced(seen)).toHaveLength(1); + expect(result.rows.map((r) => r.account)).toEqual(['Acme Corp', 'acc2']); + }); + + it('no scope at all still reads the target, unchanged', async () => { + // The `undefined` arm — "no scope for this object" is a legitimate answer + // from the provider contract, and the guard must not turn it into a + // refusal. Without this case a guard that refused everything would pass + // every refusal assertion above. + const { run, seen } = await runLabels({ scope: undefined }); + + const result = await run() as unknown as { rows: Record[] }; + + expect(readReferenced(seen)).toHaveLength(1); + expect(readReferenced(seen)[0].where).toEqual({ id: { $in: ['acc1', 'acc2'] } }); + expect(result.rows.map((r) => r.account)).toEqual(['Acme Corp', 'Umbrella Ltd']); + }); +}); diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index 053e54b0b7..60da818b87 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -9,6 +9,7 @@ import { AnalyticsService } from './analytics-service.js'; import type { AnalyticsServiceConfig } from './analytics-service.js'; import type { AnalyticsDriverCapabilities } from './strategies/types.js'; import { pickDisplayField, type DimensionLabelDeps } from './dimension-labels.js'; +import { assertReadScopeCannotVacate } from './read-scope-sql.js'; /** * The slice of the DECLARED engine contracts this plugin's auto-bridges @@ -518,6 +519,30 @@ export class AnalyticsServicePlugin implements Plugin { const map = new Map(); const displayField = pickDisplayField(dataEngine()?.getObject?.(targetObject)?.fields); if (!displayField || !executeAggregate || ids.length === 0) return map; + // [#14329] The FOURTH read-scope door, and the same guard the other + // three answer with. #13640 guarded the ObjectQL engine merge and + // #13926 the echo merge and `NativeSQLStrategy.applyReadScope`; this + // hook is a fourth consumer of the same `readScopeProvider` output + // that meets NEITHER `compileScopedFilterToSql` nor the guard — the + // `$and` below hands the scope straight to `executeAggregate`, so a + // vacating spelling (`$not` over `$in: []` and its measured siblings, + // reachable from any out-of-repo `getReadScope` producer the + // `StrategyContext` spec contract admits) used to let this per-record + // read run effectively unscoped for the ids in hand — leaking exactly + // the display names the referenced object's RLS exists to hide. + // + // Placement mirrors `ObjectQLStrategy.resolveFkAttr`, this hook's + // structural twin (same id-`$in` `$and` scope, same `executeAggregate`, + // guarded since #13640): AFTER the early returns, because a call that + // reads nothing cannot widen anything and refusing it would be pure + // over-denial; and BEFORE the chunk loop, so one scope gets one verdict + // rather than one per 500 ids. The condition is spelled to match the + // composition on the next line exactly, so the set of scopes guarded + // and the set of scopes `$and`-ed are provably the same set. + // + // ⛔ Zero compiler change: the #13571 lowering residue is ruled and + // untouched. This guard is the walk, not the lowering. + if (scope) assertReadScopeCannotVacate(scope, targetObject); // #3680 — the sort-key pass hands over the PRE-window id set (every // grouped value, not just the displayed page), so a high-cardinality // lookup dimension can push thousands of ids through here. Chunk the