From 4372aef4d9eda7a8266f82f2c1cb1c52342e6fdf Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 10:49:58 +0000 Subject: [PATCH 1/3] wip: analytics record-level scoping (#4467) + measure field validation (#4437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the three v17 verification defects on the analytics query path. Both reproduced live on a showcase dev server before the change and re-verified after; regression tests still to be added (hence wip). #4467 — /analytics/query ignored record-level scoping `ISecurityService.getReadFilter` documents itself as "the same filter the engine middleware AND-s into every find", exposed for paths that bypass the middleware (the analytics raw-SQL path has no other source of scope). That middleware chain is TWO siblings: plugin-security's RLS injection and plugin-sharing's owner/share visibility filter. Only the RLS half was ever computed, so the analytics path ran with no owner predicate at all. Live repro (showcase, `showcase_private_note` sharingModel:'private', admin owns 5, member holds 2 shares and no viewAllRecords): GET /data/showcase_private_note member -> total 2 correct POST /analytics/query {measures:[count]} member -> count 5 LEAK ... + dimensions:["title"] member -> all 5 titles getReadFilter now resolves plugin-sharing's buildReadFilter through the late-bound `sharing` service and AND-composes it with the RLS filter, and computes the ADR-0057 D1 `__readScope` depth the middleware normally stashes on the context (no middleware runs on this path). Resolved for every non-system caller ahead of the RLS branches — none of the RLS stand-downs is a reason to drop a sibling middleware's predicate — and a resolution failure denies rather than emitting unscoped SQL. #4437 — a measure naming a missing field 500'd with SQLITE_ERROR `inferMeasure('ghost_sum')` built `SUM(ghost)` with no way to know the field exists; the driver threw `no such column` and the caller got `500 {"code":"SQLITE_ERROR","message":"Internal server error"}` — a driver error class on the wire for a plain typo (ADR-0112). The DATA route has refused the same mistake with a 400 naming the field since #4315/#4254. `ensureCube` now validates each measure's resolved source field against the backing object's field names before any SQL is built, and rejects with the same envelope the data route uses (400 INVALID_FIELD + field/object/param). Gated the same way as the #3867 inference gate: only for a cube whose `sql` is a bare object name, only when the new `getObjectFieldNames` probe answers, and only for measures whose source is a bare column (count(*) and dotted cross-object references pass through). Validation runs before the cube is registered so a rejected query leaves no trace in the registry. Refs #4467, #4437 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../plugin-security/src/security-plugin.ts | 91 +++++++++++++- .../src/analytics-service.ts | 114 ++++++++++++++++++ .../services/service-analytics/src/plugin.ts | 12 ++ 3 files changed, 213 insertions(+), 4 deletions(-) diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 0d37fdf3e4..07e6fdd7ce 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2254,6 +2254,68 @@ export class SecurityPlugin implements Plugin { ); } + /** + * [#4467] The OWD / record-sharing half of the read scope — plugin-sharing's + * `buildReadFilter` for `object` under `context`, resolved through the + * late-bound `sharing` service. + * + * `getReadFilter` promises "the same filter the engine middleware AND-s into + * every find". That chain is TWO sibling middlewares: this plugin's RLS + * injection and plugin-sharing's owner/share visibility filter. Only the RLS + * half was ever computed here, so the analytics raw-SQL path — which bypasses + * the engine and has no other source of scope — ran with no owner predicate at + * all: a member could `COUNT(*)` an owner-private object they hold no share on, + * and `GROUP BY title` read the values themselves out of rows `/data` correctly + * refused them. + * + * The DEPTH the owner-match widens to (ADR-0057 D1) is stashed on the context + * by the middleware as `__readScope` before plugin-sharing reads it; no + * middleware runs on this path, so it is computed here from the SAME evaluator + * call the middleware makes. Without it a caller granted `unit`/`org` read + * depth would be scoped to `own` — safe, but a silent disagreement between + * `/data` and `/analytics` in the other direction. + * + * Returns `null` when the sharing layer imposes nothing (no plugin-sharing, a + * public object, an object with no owner field, a bypass object). THROWS on a + * resolution failure so the caller can fail closed — a dropped sharing + * predicate is exactly the leak this fixes. + */ + private async resolveSharingReadFilter( + object: string, + context: any, + ): Promise | null> { + const sharing = this.resolveKernelService?.('sharing') as + | { buildReadFilter?: (o: string, c: any) => Promise } + | undefined; + if (!sharing || typeof sharing.buildReadFilter !== 'function') return null; + // Mirror the middleware's ADR-0057 D1 depth stash. `getEffectiveScope` + // needs the resolved sets and the object's posture — the same two inputs + // the middleware feeds it — so the owner-match widens identically here. + let readScope: string | undefined; + try { + const permissionSets = await this.resolvePermissionSetsForContext(context); + if (permissionSets.length > 0) { + const meta = await this.getObjectSecurityMeta(object); + readScope = this.permissionEvaluator.getEffectiveScope( + 'read', + object, + permissionSets, + { isPrivate: meta.isPrivate }, + ); + } + } catch { + // Depth is a WIDENING input: failing to resolve it leaves the owner-match + // at its narrowest ('own'), which is the safe direction. The sharing call + // below still runs — and its own failure still denies. + readScope = undefined; + } + const filter = await sharing.buildReadFilter(object, { + ...context, + ...(readScope ? { __readScope: readScope } : {}), + }); + return (filter ?? null) as Record | null; + } + async getReadFilter( object: string, context?: any, @@ -2262,11 +2324,29 @@ export class SecurityPlugin implements Plugin { if (context?.isSystem) return undefined; const positions = context?.positions ?? []; const explicit = context?.permissions ?? []; - // Unauthenticated + position-less + permission-less → no scope (the auth + // [#4467] The OWD/sharing predicate is resolved for EVERY non-system caller, + // ahead of the RLS branches below, because it is a SEPARATE middleware in + // the chain this method mirrors: none of the RLS stand-downs below is a + // reason to drop it. A resolution failure denies outright — running the + // analytics raw-SQL path with a dropped owner predicate is the leak. + let sharingFilter: Record | null; + try { + sharingFilter = await this.resolveSharingReadFilter(object, context); + } catch (e) { + this.logger.error?.( + `[security] getReadFilter could not resolve the sharing (OWD) read scope for object ` + + `'${object}' (user ${context?.userId ?? 'unknown'}) — denying (fail-closed, #4467)`, + e instanceof Error ? e : new Error(String(e)), + ); + return { ...RLS_DENY_FILTER }; + } + // Unauthenticated + position-less + permission-less → no RLS scope (the auth // layer, not RLS, gates anonymous access; the analytics REST endpoint - // already 401s without a token). Mirrors the middleware's early `return next()`. + // already 401s without a token). Mirrors the middleware's early `return next()` + // — which is the RLS middleware's early exit only, so the sharing predicate + // resolved above still applies. if (positions.length === 0 && explicit.length === 0 && !context?.userId) { - return undefined; + return sharingFilter ?? undefined; } // [#2852] D10 delegator intersection is NOT implemented on this path. // The engine middleware (find/count/aggregate) intersects an on-behalf-of @@ -2292,7 +2372,10 @@ export class SecurityPlugin implements Plugin { try { const permissionSets = await this.resolvePermissionSetsForContext(context); const filter = await this.computeRlsFilter(permissionSets, object, 'find', context); - return filter ?? undefined; + // [#4467] RLS AND sharing — the same AND-composition the two middlewares + // achieve by both writing into `ast.where`. Either half may be absent; + // `andComposeLayers` returns the other, or null when neither constrains. + return andComposeLayers(filter, sharingFilter) ?? undefined; } catch (e) { // Fail CLOSED — a resolution failure must deny (zero rows), never expose // every tenant's data through the raw-SQL analytics path. diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 45c1fb26db..81d7525b5a 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -92,6 +92,14 @@ function isMissingSourceError(err: unknown): boolean { ); } +/** + * [#4437] A name that is a plain column/table identifier and nothing else. + * Anything with a dot, a paren, whitespace or an operator is a SQL EXPRESSION + * (or a cross-object reference) whose parts this layer cannot attribute to a + * single field — such measures pass the source-field gate untouched. + */ +const BARE_IDENTIFIER = /^[a-z_][a-z0-9_]*$/i; + /** * Configuration for AnalyticsService. */ @@ -209,6 +217,26 @@ export interface AnalyticsServiceConfig { * always wires it. */ isRegisteredObject?: (name: string) => boolean; + /** + * [#4437] The FIELD NAMES `objectName` declares, or `undefined` when nothing + * authoritative can answer. + * + * Consulted by {@link AnalyticsService.ensureCube} to validate the SOURCE + * FIELD a measure resolves to BEFORE any SQL is built. `inferMeasure` maps a + * suffix convention onto a field name (`ghost_sum` → `SUM(ghost)`) and used + * to accept any spelling, so a typo'd measure reached the driver as a column + * and came back as an opaque `500 SQLITE_ERROR` — a driver error class on the + * wire for a caller-shaped mistake (ADR-0112). The DATA route already refuses + * the same mistake with a `400 INVALID_FIELD` naming the field (#4315/#4254); + * this hook is what lets the ANALYTICS route give the same answer. + * + * Same tiering as {@link isRegisteredObject}: absence means "skip the check" + * (registry-less hosts, engine doubles, external datasources whose columns + * are not mirrored locally). The production bridge in `plugin.ts` wires it + * from the same schema registry the data path's gate reads, so "which fields + * exist" has ONE answer across `/data` and `/analytics`. + */ + getObjectFieldNames?: (objectName: string) => readonly string[] | undefined; /** * ADR-0021 — optional object-graph resolver used when compiling datasets: * `(baseObject, relationshipName) => relatedObjectName | undefined`. When @@ -293,6 +321,8 @@ export class AnalyticsService implements IAnalyticsService { private readonly draftRowsResolver?: AnalyticsServiceConfig['draftRowsResolver']; /** [#3867] Schema-registry probe gating cube auto-inference. */ private readonly isRegisteredObject?: AnalyticsServiceConfig['isRegisteredObject']; + /** [#4437] Field-name probe gating measure source-field resolution. */ + private readonly getObjectFieldNames?: AnalyticsServiceConfig['getObjectFieldNames']; /** [#3867] One-shot flag for the {@link assertInferableCube} stand-down warning. */ private warnedNoObjectRegistry = false; readonly cubeRegistry: CubeRegistry; @@ -313,6 +343,7 @@ export class AnalyticsService implements IAnalyticsService { this.labelResolver = config.labelResolver; this.draftRowsResolver = config.draftRowsResolver; this.isRegisteredObject = config.isRegisteredObject; + this.getObjectFieldNames = config.getObjectFieldNames; // Compile + register pre-defined datasets (ADR-0021). if (config.datasets) { @@ -843,6 +874,11 @@ export class AnalyticsService implements IAnalyticsService { // such check: it was authored, and its `sql` is whatever it declares. this.assertInferableCube(name); cube = this.inferCubeFromQuery(query); + // [#4437] Validate the inferred measures' SOURCE FIELDS before the cube + // is registered — a rejected query must leave no trace in the registry + // (same rule the #3867 gate above keeps), or a retry would find a + // "registered" cube carrying the bogus measure and sail straight to SQL. + this.assertMeasureFields(query, cube, Object.keys(cube.measures)); this.cubeRegistry.register(cube); // A scalar query — only measures, no grouping (no `dimensions`/ // `timeDimensions`) — is the first-class "metric over an object" path @@ -877,10 +913,88 @@ export class AnalyticsService implements IAnalyticsService { ...cube, measures: { ...cube.measures, ...extraMeasures }, }; + // [#4437] The cube's DECLARED measures are the ones a caller may name; + // the suffix-inferred entries just added are a convenience, not a + // vocabulary. Snapshot the declared list BEFORE registering the augmented + // cube so the rejection can suggest what the caller could have meant — + // and so a rejected query leaves the registry as it found it. + this.assertMeasureFields(query, augmented, Object.keys(cube.measures)); this.cubeRegistry.register(augmented); this.logger.debug( `[Analytics] Augmented cube "${name}" with inferred measures: ${Object.keys(extraMeasures).join(',')}`, ); + } else { + // No inference happened — every measure is declared. Still validate: an + // authored cube can declare a measure over a field the object dropped. + this.assertMeasureFields(query, cube, Object.keys(cube.measures)); + } + } + + /** + * [#4437] Reject a measure whose SOURCE FIELD the backing object does not + * have, BEFORE the strategy compiles it into SQL. + * + * `inferMeasure` maps a suffix convention onto a field name and has no way to + * know whether that field exists: `ghost_sum` happily became `SUM(ghost)`, the + * driver threw `no such column`, and the caller got + * `500 {"code":"SQLITE_ERROR","message":"Internal server error"}` — a driver + * error class on the wire, and nothing actionable, for what is a plain typo. + * The DATA route has refused the same mistake with a `400 INVALID_FIELD` + * naming the field since #4315/#4254; this is the analytics half of that + * answer, and it is deliberately the SAME envelope (`code`/`field`/`object`/ + * `param`) so one mistake has one shape across both routes. + * + * What it checks, and what it deliberately does not: + * + * - Only when the cube's `sql` is a bare OBJECT NAME. An authored cube whose + * `sql` is a real SQL expression has no field list to check against. + * - Only when {@link AnalyticsServiceConfig.getObjectFieldNames} answers. + * Absent hook / unknown object → stand down (see the config field's doc). + * - Only measures whose source is a BARE COLUMN. `count(*)` has no source + * field, and a dotted reference (`account.industry`) resolves through a + * join whose target this check cannot see — both pass through untouched. + * - `id` / `created_at` / `updated_at` are admitted unconditionally, matching + * the data path's `resolveQueryFields`: they are engine-assigned rather than + * declared, and a gate stricter than the engine it guards would reject + * queries that used to work. + */ + private assertMeasureFields(query: AnalyticsQuery, cube: Cube, declaredMeasures: string[]): void { + const probe = this.getObjectFieldNames; + if (!probe) return; + const measures = query.measures ?? []; + if (measures.length === 0) return; + + const object = typeof cube.sql === 'string' ? cube.sql.trim() : ''; + if (!object || !BARE_IDENTIFIER.test(object)) return; + const fieldNames = probe(object); + if (!fieldNames || fieldNames.length === 0) return; + const known = new Set([...fieldNames, 'id', 'created_at', 'updated_at']); + + const stripPrefix = (m: string) => (m.includes('.') ? m.split('.').slice(1).join('.') : m); + for (const measure of measures) { + const key = stripPrefix(measure); + const metric = cube.measures[key] as { type?: string; sql?: unknown } | undefined; + if (!metric) continue; + // `count(*)` is the one legitimately field-less aggregate. + if (metric.type === 'count' && (metric.sql === '*' || metric.sql == null)) continue; + const source = typeof metric.sql === 'string' ? metric.sql.trim() : ''; + if (!source || source === '*' || !BARE_IDENTIFIER.test(source)) continue; + if (known.has(source)) continue; + + const err = new Error( + `Measure '${measure}' on cube '${cube.name}' aggregates field '${source}', which object ` + + `'${object}' does not have. ` + + `Valid measures: ${declaredMeasures.join(', ') || '(none declared)'}. ` + + `A '_sum' / '_avg' / '_min' / '_max' / '_count_distinct' measure is inferred from ` + + `the object's own fields, so check the spelling of '${source}'.`, + ) as Error & { code?: string; status?: number; field?: string; object?: string; param?: string; measure?: string }; + err.code = 'INVALID_FIELD'; + err.status = 400; + err.field = source; + err.object = object; + err.param = 'measures'; + err.measure = measure; + throw err; } } diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index 904cd1e4b5..d22fe254b9 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -546,6 +546,18 @@ export class AnalyticsServicePlugin implements Plugin { if (!engine) return true; return engine.getObject?.(name) != null; }, + // [#4437] Field names for the measure source-field gate. Read from the + // SAME schema registry `isRegisteredObject` above consults (and the data + // path's #4315 gate reads), so "which fields exist" has one answer across + // /data and /analytics. `undefined` — no engine, unknown object, or an + // object with no field map (an external datasource whose columns are not + // mirrored locally) — means "cannot answer", and the gate stands down. + getObjectFieldNames: (objectName: string) => { + const fields = dataEngine()?.getObject?.(objectName)?.fields; + if (!fields || typeof fields !== 'object') return undefined; + const names = Object.keys(fields); + return names.length > 0 ? names : undefined; + }, draftRowsResolver, }; From 55fe85156c86a70c49eaec0d6dc0fc7b21fca0a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 10:57:46 +0000 Subject: [PATCH 2/3] test: pin the analytics scoping + measure-field gates (#4467, #4437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression cases for the two fixes in the previous commit, plus a polish to the #4437 rejection message. #4467 — `security-plugin.test.ts` gains an OWD/sharing block under the existing `getReadFilter service` describe: AND-composition with the RLS filter, the sharing predicate surviving alone when RLS contributes nothing, the ADR-0057 D1 `__readScope` depth being passed (no middleware runs on this path to stash it), fail-closed on a sharing-resolution throw, the isSystem bypass, and a deployment without plugin-sharing being unaffected. The harness gains an optional `sharing` service double. #4437 — a new `measure-source-field-gate.test.ts` covering the 400 envelope and its `field`/`object`/`param`/`measure` members, the dotted `total.sum` spelling, registry non-poisoning, every legitimate measure spelling still running, an authored cube whose declared measure lost its field, and the three stand-downs (no probe, an object the probe cannot describe, and a cube whose `sql` is an expression rather than an object name). A dotted cross-object measure is asserted to reach the STRATEGY — the layer that owns that decision — rather than being reported as a missing column here. Polish: the rejection listed the caller's own typo as a valid alternative on the auto-inference path, because `cube.measures` there was inferred from the very query being rejected. The suggestion list now excludes measures that failed the check, and names the object's known fields. Refs #4467, #4437 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../src/security-plugin.test.ts | 155 +++++++++- .../measure-source-field-gate.test.ts | 268 ++++++++++++++++++ .../src/analytics-service.ts | 39 ++- 3 files changed, 451 insertions(+), 11 deletions(-) create mode 100644 packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index a76ff98e31..ce75bd826c 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -140,7 +140,7 @@ describe('SecurityPlugin', () => { // wildcard `current_user.organization_id` RLS policies. Otherwise it // strips them so single-tenant deployments aren't filtered to nothing. // ------------------------------------------------------------------------- - const makeMiddlewareCtx = (overrides: { permissionSets: PermissionSet[]; objectFields?: string[]; schemaExtra?: Record; orgScoping?: boolean; findOneImpl?: (query: any) => any }) => { + const makeMiddlewareCtx = (overrides: { permissionSets: PermissionSet[]; objectFields?: string[]; schemaExtra?: Record; orgScoping?: boolean; findOneImpl?: (query: any) => any; sharing?: any }) => { const fields: Record = {}; for (const f of overrides.objectFields ?? ['id', 'organization_id', 'owner_id', 'name']) { fields[f] = { name: f }; @@ -177,6 +177,10 @@ describe('SecurityPlugin', () => { // Sentinel object — SecurityPlugin only checks truthiness. services['org-scoping'] = { name: 'com.objectstack.org-scoping' }; } + // [#4467] The optional plugin-sharing service. Absent by default, which is + // exactly the deployment shape every case above assumes; supply it to + // exercise the OWD/sharing half of the read scope. + if (overrides.sharing) services['sharing'] = overrides.sharing; const ctx: any = { logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, registerService: vi.fn(), @@ -1355,6 +1359,155 @@ describe('SecurityPlugin', () => { const filter = await plugin.getReadFilter('task', { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [] }); expect(filter).toEqual(RLS_DENY_FILTER); }); + + // ----------------------------------------------------------------------- + // [#4467] The OWD / record-sharing half of the read scope. + // + // `getReadFilter` promises "the same filter the engine middleware AND-s + // into every find". That chain is TWO sibling middlewares — this plugin's + // RLS injection and plugin-sharing's owner/share visibility filter — and + // only the RLS half was ever computed here. The analytics raw-SQL path has + // no other source of scope, so `POST /analytics/query` ran with no owner + // predicate at all. Live repro on showcase before the fix, member holding + // shares on 2 of an admin's 5 private notes and no `viewAllRecords`: + // + // GET /data/showcase_private_note member → total 2 correct + // POST /analytics/query {measures:[count]} member → count 5 LEAK + // ... + dimensions:["title"] member → all 5 titles + // + // The dimension case is why this is a disclosure and not just a bad count: + // grouping returns the VALUES of a column the caller may not read. + // ----------------------------------------------------------------------- + describe('[#4467] OWD / sharing composition', () => { + /** A plugin-sharing double that scopes `task` to owner-or-shared. */ + const ownerOrShared = { + buildReadFilter: vi.fn(async (_object: string, ctx: any) => ({ + $or: [{ owner_id: ctx.userId }, { id: { $in: ['rec-1', 'rec-2'] } }], + })), + }; + + it('AND-composes the sharing predicate with the RLS filter', async () => { + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ + permissionSets: [tenantPolicySet], + sharing: ownerOrShared, + }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + + const filter = await plugin.getReadFilter('task', { + userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], + }); + + // Pre-fix this was `{ organization_id: 'org-1' }` alone — every row of + // the tenant, regardless of ownership. + expect(filter).toEqual({ + $and: [ + { organization_id: 'org-1' }, + { $or: [{ owner_id: 'u1' }, { id: { $in: ['rec-1', 'rec-2'] } }] }, + ], + }); + }); + + it('returns the sharing predicate alone when RLS contributes nothing', async () => { + // An owner-private object in a deployment with no tenant policy: the + // sharing half is then the ONLY thing standing between the caller and + // every row, so it must survive on its own rather than collapsing to + // `undefined` with the empty RLS half. + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ + permissionSets: [{ + name: 'member_default', + label: 'Member', + objects: { '*': { allowRead: true } }, + } as any], + sharing: ownerOrShared, + }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + + const filter = await plugin.getReadFilter('task', { + userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], + }); + + expect(filter).toEqual({ $or: [{ owner_id: 'u1' }, { id: { $in: ['rec-1', 'rec-2'] } }] }); + }); + + it('passes the ADR-0057 D1 read DEPTH the middleware would have stashed', async () => { + // plugin-sharing widens its owner-match from `__readScope`, which the + // engine middleware writes onto the context before the sharing + // middleware runs. No middleware runs on this path, so getReadFilter + // must compute it — otherwise a caller granted `org` read depth is + // silently narrowed to `own` here while `/data` shows them everything. + const capture = { buildReadFilter: vi.fn(async () => null) }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ + permissionSets: [{ + name: 'member_default', + label: 'Member', + objects: { '*': { allowRead: true, readScope: 'unit' } }, + } as any], + sharing: capture, + }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + + await plugin.getReadFilter('task', { + userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], + }); + + expect(capture.buildReadFilter).toHaveBeenCalledWith( + 'task', + expect.objectContaining({ __readScope: 'unit', userId: 'u1' }), + ); + }); + + it('fail-closed: a sharing-resolution throw denies rather than under-scoping', async () => { + // Dropping this predicate is precisely the leak, so an unresolvable + // sharing layer must deny — never fall through to the RLS half alone. + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ + permissionSets: [tenantPolicySet], + sharing: { buildReadFilter: async () => { throw new Error('share store unavailable'); } }, + }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + + const filter = await plugin.getReadFilter('task', { + userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], + }); + + expect(filter).toEqual(RLS_DENY_FILTER); + }); + + it('a system context still bypasses both halves', async () => { + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const sharing = { buildReadFilter: vi.fn(async () => ({ owner_id: 'u1' })) }; + const harness = makeMiddlewareCtx({ permissionSets: [tenantPolicySet], sharing }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + + const filter = await plugin.getReadFilter('task', { isSystem: true, userId: 'u1', tenantId: 'org-1' }); + + expect(filter).toBeUndefined(); + expect(sharing.buildReadFilter).not.toHaveBeenCalled(); + }); + + it('a deployment without plugin-sharing is unaffected', async () => { + // The service is optional; its absence must not change the RLS answer + // (and must not throw on the lookup). + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ permissionSets: [tenantPolicySet] }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + + const filter = await plugin.getReadFilter('task', { + userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], + }); + + expect(filter).toEqual({ organization_id: 'org-1' }); + }); + }); }); // ------------------------------------------------------------------------- diff --git a/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts b/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts new file mode 100644 index 0000000000..d1312c4ee8 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts @@ -0,0 +1,268 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4437 — the measure SOURCE-FIELD gate. + * + * `inferMeasure` maps a suffix convention onto a field name and cannot know + * whether that field exists: `ghost_sum` happily became `SUM(ghost)`, the + * driver threw `no such column`, and the caller got a driver error class on + * the wire for what is a plain typo. Live repro on a showcase dev server + * before the fix: + * + * ``` + * POST /analytics/query {"cube":"showcase_invoice","measures":["ghost_sum"]} + * → 500 {"code":"SQLITE_ERROR","message":"Internal server error"} + * ``` + * + * A dotted spelling took the same path (`"total.sum"` → prefix-strip → + * `inferMeasure('sum')` → `SUM(sum)` → 500). The DATA route has refused the + * same mistake with a `400 INVALID_FIELD` naming the field since #4315/#4254; + * these cases pin the analytics half of that answer, and pin the tiering that + * keeps it from over-reaching (ADR-0112: a driver error class is never the + * `error.code` for a caller-shaped mistake). + */ + +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 INVOICE_FIELDS = ['id', 'total', 'status', 'issued_on', 'account']; + +/** + * A service over one object (`showcase_invoice`) whose columns are known. + * `aggregated` records every object an aggregate actually ran against, so a + * test can assert the rejected query never reached the driver. + */ +function makeService(opts: { cubes?: Cube[]; wireProbe?: boolean; fields?: string[] } = {}) { + 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 }]; + }, + isRegisteredObject: (n: string) => n === 'showcase_invoice', + ...(opts.wireProbe === false + ? {} + : { + getObjectFieldNames: (n: string) => + n === 'showcase_invoice' ? (opts.fields ?? INVOICE_FIELDS) : undefined, + }), + }); + return { service, aggregated }; +} + +/** The envelope the DATA route already produces for the same mistake (#4315). */ +const INVALID_FIELD = { + code: 'INVALID_FIELD', + status: 400, + object: 'showcase_invoice', + param: 'measures', +}; + +describe('#4437 — measure source-field gate', () => { + it('refuses a measure over a missing field with a 400, not a driver 500', async () => { + const { service, aggregated } = makeService(); + + await expect( + service.query({ cube: 'showcase_invoice', measures: ['ghost_sum'] } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'ghost', measure: 'ghost_sum' }); + + // The whole point: the typo never became a column. + expect(aggregated).toEqual([]); + }); + + it('names the missing field in the message so the caller can act on it', async () => { + const { service } = makeService(); + + await expect( + service.query({ cube: 'showcase_invoice', measures: ['ghost_sum'] } as any), + ).rejects.toThrow(/aggregates field 'ghost'/); + }); + + it('does not offer the caller their own typo back as a valid measure', async () => { + // On the auto-inference path the bogus measure is already in + // `cube.measures` (it was inferred from this very query), so echoing the + // cube's measure list verbatim suggested `ghost_sum` — the one + // alternative guaranteed not to work. + const { service } = makeService(); + + const err = await service + .query({ cube: 'showcase_invoice', measures: ['ghost_sum'] } as any) + .catch((e) => e as Error); + + expect(err.message).toMatch(/Valid measures: count\./); + expect(err.message).not.toMatch(/Valid measures:[^.]*ghost_sum/); + }); + + it('refuses the dotted spelling the same way, naming what it stripped to', async () => { + // `total.sum` prefix-strips to `sum`, which infers `SUM(sum)` — a column + // named `sum` that does not exist. Same 500 pre-fix, same 400 now. + const { service, aggregated } = makeService(); + + await expect( + service.query({ cube: 'showcase_invoice', measures: ['total.sum'] } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'sum', measure: 'total.sum' }); + + expect(aggregated).toEqual([]); + }); + + it('does not poison the registry with the rejected cube', async () => { + // Same rule the #3867 inference gate keeps: a rejected query must leave + // no trace, or the retry finds a "registered" cube carrying the bogus + // measure and sails straight into SQL. + const { service, aggregated } = makeService(); + + await expect( + service.query({ cube: 'showcase_invoice', measures: ['ghost_sum'] } as any), + ).rejects.toThrow(); + expect(service.cubeRegistry.get('showcase_invoice')).toBeUndefined(); + + await expect( + service.query({ cube: 'showcase_invoice', measures: ['ghost_sum'] } as any), + ).rejects.toMatchObject(INVALID_FIELD); + expect(aggregated).toEqual([]); + }); + + it('lets every legitimate measure spelling through unchanged', async () => { + const { service, aggregated } = makeService(); + + // `count(*)` — the one legitimately field-less aggregate. + await service.query({ cube: 'showcase_invoice', measures: ['count'] } as any); + // A real field under each inferred suffix. + await service.query({ + cube: 'showcase_invoice', + measures: ['total_sum', 'total_avg', 'total_min', 'total_max', 'total_count_distinct'], + } as any); + // Engine-assigned columns are admitted like the data path admits them. + await service.query({ cube: 'showcase_invoice', measures: ['created_at_max'] } as any); + + expect(aggregated).toEqual(Array(3).fill('showcase_invoice')); + }); + + it('gates generateSql too, not just query', async () => { + // `/analytics/sql` runs the same `ensureCube`; leaving it ungated would + // hand back SQL naming a column that does not exist. + const { service } = makeService(); + + await expect( + service.generateSql({ cube: 'showcase_invoice', measures: ['ghost_sum'] } as any), + ).rejects.toMatchObject(INVALID_FIELD); + }); + + it('validates an AUTHORED cube whose declared measure lost its field', async () => { + // An authored cube is not second-guessed about WHICH table it reads + // (#3867), but a measure it declares over a dropped column is the same + // caller-visible 500 — and here the suggestion list is real. + const authored: Cube = { + name: 'invoice_cube', + title: 'Invoices', + sql: 'showcase_invoice', + measures: { + count: { name: 'count', label: 'Count', type: 'count', sql: '*' }, + revenue: { name: 'revenue', label: 'Revenue', type: 'sum', sql: 'total' }, + legacy: { name: 'legacy', label: 'Legacy', type: 'sum', sql: 'dropped_column' }, + }, + dimensions: {}, + public: false, + }; + const { service, aggregated } = makeService({ cubes: [authored] }); + + await expect( + service.query({ cube: 'invoice_cube', measures: ['legacy'] } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'dropped_column', measure: 'legacy' }); + expect(aggregated).toEqual([]); + + // Its healthy siblings still run, and are what the rejection suggests. + await service.query({ cube: 'invoice_cube', measures: ['revenue'] } as any); + expect(aggregated).toEqual(['showcase_invoice']); + }); + + it('leaves a cube whose `sql` is an expression alone — no field list to check', async () => { + // `sql` is a subquery, not an object name: there is no schema to consult, + // and guessing would reject perfectly good authored analytics. + const derived: Cube = { + name: 'derived_cube', + title: 'Derived', + sql: 'SELECT * FROM showcase_invoice WHERE status = 1', + measures: { anything_sum: { name: 'anything_sum', label: 'x', type: 'sum', sql: 'anything' } }, + dimensions: {}, + public: false, + }; + const { service } = makeService({ cubes: [derived] }); + + await expect( + service.query({ cube: 'derived_cube', measures: ['anything_sum'] } as any), + ).resolves.toBeTruthy(); + }); + + it('leaves a dotted cross-object measure to the layers that own it', async () => { + // `account.balance` resolves through a JOIN this gate cannot see — + // `balance` is not a column of `showcase_invoice` and must not be + // reported as a missing one. Whether the query can run at all is the + // strategy's call (the ObjectQL aggregate path declines cross-object + // measures outright) and the join allowlist's (ADR-0021 D-C); either + // way the answer must not be this gate's INVALID_FIELD. + const joined: Cube = { + name: 'joined_cube', + title: 'Joined', + sql: 'showcase_invoice', + measures: { + remote_sum: { name: 'remote_sum', label: 'Remote', type: 'sum', sql: 'account.balance' }, + }, + dimensions: {}, + public: false, + }; + const { service } = makeService({ cubes: [joined] }); + + const err = await service + .query({ cube: 'joined_cube', measures: ['remote_sum'] } as any) + .catch((e) => e as Error & { code?: string }); + + expect(err).toBeInstanceOf(Error); + expect(err.code).not.toBe('INVALID_FIELD'); + // It got as far as the strategy — i.e. past this gate — and was declined + // there for the strategy's own declared reason. + expect(err.message).toMatch(/cannot evaluate a cross-object measure/); + }); + + it('stands down when no field probe is configured — nothing to consult', async () => { + // Same tiering as the #3867 registry gate and the data path's + // `resolveQueryFields`: 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({ wireProbe: false }); + + await service.query({ cube: 'showcase_invoice', measures: ['ghost_sum'] } as any); + + expect(aggregated).toEqual(['showcase_invoice']); + }); + + it('stands down for an object the probe cannot describe', async () => { + // An external datasource whose columns are not mirrored locally answers + // `undefined` — "cannot answer", not "has no fields". + const external: Cube = { + name: 'external_cube', + title: 'External', + sql: 'remote_table', + measures: { ghost_sum: { name: 'ghost_sum', label: 'x', type: 'sum', sql: 'ghost' } }, + dimensions: {}, + public: false, + }; + const { service } = makeService({ cubes: [external] }); + + await expect( + service.query({ cube: 'external_cube', measures: ['ghost_sum'] } as any), + ).resolves.toBeTruthy(); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 81d7525b5a..70e7202870 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -971,22 +971,41 @@ export class AnalyticsService implements IAnalyticsService { const known = new Set([...fieldNames, 'id', 'created_at', 'updated_at']); const stripPrefix = (m: string) => (m.includes('.') ? m.split('.').slice(1).join('.') : m); - for (const measure of measures) { - const key = stripPrefix(measure); - const metric = cube.measures[key] as { type?: string; sql?: unknown } | undefined; - if (!metric) continue; + /** The source field a measure aggregates, or null when there is nothing to check. */ + const sourceFieldOf = (measure: string): string | null => { + const metric = cube.measures[stripPrefix(measure)] as { type?: string; sql?: unknown } | undefined; + if (!metric) return null; // `count(*)` is the one legitimately field-less aggregate. - if (metric.type === 'count' && (metric.sql === '*' || metric.sql == null)) continue; + if (metric.type === 'count' && (metric.sql === '*' || metric.sql == null)) return null; const source = typeof metric.sql === 'string' ? metric.sql.trim() : ''; - if (!source || source === '*' || !BARE_IDENTIFIER.test(source)) continue; - if (known.has(source)) continue; + if (!source || source === '*' || !BARE_IDENTIFIER.test(source)) return null; + return source; + }; + + // Two passes so the rejection can suggest the measures that WOULD have + // worked. On the auto-inference path `cube.measures` already carries the + // caller's own bogus spelling (it was inferred from the query moments ago), + // so echoing the cube's measure list verbatim would offer the typo back as + // a valid alternative — the one suggestion guaranteed to be wrong. + const invalid = new Set(); + for (const measure of measures) { + const source = sourceFieldOf(measure); + if (source && !known.has(source)) invalid.add(stripPrefix(measure)); + } + if (invalid.size === 0) return; + const usable = declaredMeasures.filter((m) => !invalid.has(m)); + + for (const measure of measures) { + const source = sourceFieldOf(measure); + if (!source || known.has(source)) continue; const err = new Error( `Measure '${measure}' on cube '${cube.name}' aggregates field '${source}', which object ` + `'${object}' does not have. ` + - `Valid measures: ${declaredMeasures.join(', ') || '(none declared)'}. ` + - `A '_sum' / '_avg' / '_min' / '_max' / '_count_distinct' measure is inferred from ` + - `the object's own fields, so check the spelling of '${source}'.`, + `Valid measures: ${usable.join(', ') || '(none)'}. ` + + `Other measures are inferred from the object's OWN fields as ` + + `'_sum' / '_avg' / '_min' / '_max' / '_count_distinct', so check the spelling of ` + + `'${source}' — known fields: ${[...fieldNames].sort().join(', ')}.`, ) as Error & { code?: string; status?: number; field?: string; object?: string; param?: string; measure?: string }; err.code = 'INVALID_FIELD'; err.status = 400; From c3cc1067b278f27953ba240fa83a2292baaaeb95 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 12:10:22 +0000 Subject: [PATCH 3/3] chore: add changeset for the analytics scoping + measure-field fixes (#4467, #4437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both packages are publishable and both changes are observable on a public surface, so this is a real changeset rather than an empty one. Levelled `minor` on both counts. #4467 narrows a public read surface — analytics results a principal could previously read they now cannot, so counts drop and `dimensions` groupings lose rows for non-superuser callers on owner-private objects. #4437 changes the response envelope for a caller-shaped mistake (500 SQLITE_ERROR → 400 INVALID_FIELD), which any caller branching on `error.code` will observe. Neither changes an API signature: `ISecurityService.getReadFilter`'s declaration is untouched, and the implementation merely started honouring the contract it already documented. Refs #4467, #4437 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- ...ytics-record-scoping-and-measure-fields.md | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 .changeset/analytics-record-scoping-and-measure-fields.md diff --git a/.changeset/analytics-record-scoping-and-measure-fields.md b/.changeset/analytics-record-scoping-and-measure-fields.md new file mode 100644 index 0000000000..cf73afa41b --- /dev/null +++ b/.changeset/analytics-record-scoping-and-measure-fields.md @@ -0,0 +1,92 @@ +--- +"@objectstack/plugin-security": minor +"@objectstack/service-analytics": minor +--- + +fix(security,analytics): scope /analytics/query to the caller's readable records, and refuse a measure over a missing field (#4467, #4437) + +Two defects on the analytics query path, both found by the v17 verification run +(#3909 / #4482), both reproduced against a live showcase server before the fix +and re-verified with the same requests after. + +## #4467 — `/analytics/query` applied no record-level scoping + +`ISecurityService.getReadFilter` documents itself as "the same filter the engine +middleware AND-s into every find", and exists precisely for paths that bypass +that middleware — its own doc comment names the analytics raw-SQL path. But the +chain it mirrors is TWO sibling middlewares: plugin-security's RLS injection and +plugin-sharing's owner/share visibility filter (`buildSharingMiddleware` AND-s +`buildReadFilter` into `ast.where` for `find`/`findOne`/`count`/`aggregate`). +Only the RLS half was ever computed here, and analytics has no other source of +scope, so the OWD/share predicate simply never existed on that path. + +Live repro: `showcase_private_note` is `sharingModel: 'private'`; an admin owns +5 notes, a member holds read shares on exactly 2 and no `viewAllRecords`. +`GET /data/showcase_private_note` correctly returned 2 for the member, while +`POST /analytics/query {measures:['count']}` returned 5 — and adding +`dimensions:['title']` returned all five titles, i.e. the VALUES of a column +that caller may not read, not merely a bad count. Any authenticated caller who +could reach `/analytics` could enumerate the field values of every row of any +object exposed as a cube, regardless of OWD, sharing rules, or RLS. + +`getReadFilter` now resolves plugin-sharing's `buildReadFilter` through the +late-bound `sharing` service and AND-composes it with the RLS filter — the same +composition the two middlewares reach by both writing into `ast.where`. It also +computes the ADR-0057 D1 `__readScope` depth that the security middleware +normally stashes on the context for plugin-sharing to widen its owner-match +with, using the same `getEffectiveScope` call the middleware makes: no +middleware runs on this path, and without it a caller granted `unit`/`org` read +depth would be silently narrowed to `own`. The sharing predicate is resolved for +every non-system caller AHEAD of the RLS stand-down branches, because those are +the RLS middleware's own early exits and none of them is a reason to drop a +sibling middleware's predicate; a sharing-resolution failure denies outright +rather than falling through to half a scope. + +**Why `minor` rather than `patch`.** This is an observable behaviour change on a +public read surface, in the narrowing direction: analytics results that a +principal could previously read they now cannot. Counts drop, `dimensions` +groupings lose rows, and any dashboard, report, or export built on +`/analytics/query` over an owner-private object will show smaller numbers for +non-superuser principals — correctly, but visibly. Deployments that had (however +unknowingly) come to depend on the unscoped totals will see them change on +upgrade, so this warrants more than a patch-level note even though it is a +security fix. No API signature changed: `ISecurityService.getReadFilter`'s +declaration is untouched — the implementation merely started honouring the +contract it already documented. + +## #4437 — a measure naming a missing field 500'd with SQLITE_ERROR + +`inferMeasure('ghost_sum')` maps a suffix convention onto a field name and has +no way to know the field exists, so it built `SUM(ghost)`, the driver threw +`no such column`, and the caller got +`500 {"code":"SQLITE_ERROR","message":"Internal server error"}` — a driver error +class as the `error.code` for what is a plain typo, which ADR-0112 forbids. A +dotted spelling took the same path (`measures:['total.sum']` prefix-strips to +`sum` → `SUM(sum)` → 500). The DATA route has refused the identical mistake with +a `400 INVALID_FIELD` naming the field since #4315/#4254. + +`AnalyticsService.ensureCube` now validates each measure's resolved source field +against the backing object's field names before any SQL is built, and rejects +with the same envelope the data route produces (`400 INVALID_FIELD` carrying +`field`, `object`, `param`, `measure`) so one mistake has one shape across +`/data` and `/analytics`. The new `getObjectFieldNames` config hook reads the +same schema registry `isRegisteredObject` already consults and the data path's +own gate reads, so "which fields exist" has a single answer across both routes. + +The gate is tiered exactly like the #3867 cube-inference gate, deliberately +narrow: it applies only when the cube's `sql` is a bare object name (an authored +cube whose `sql` is a real SQL expression has no field list to check against), +only when the probe answers (no data engine, or an external datasource whose +columns are not mirrored locally, stands down), and only to measures whose +source is a bare column — `count(*)` has no source field, and a dotted +cross-object reference resolves through a join this layer cannot see, so both +pass through untouched. `id`/`created_at`/`updated_at` are admitted +unconditionally, matching the data path's `resolveQueryFields`: a gate stricter +than the engine it guards would reject queries that used to work. Validation +runs before the cube is registered, so a rejected query leaves no trace in the +registry — otherwise a retry would find a "registered" cube carrying the bogus +measure and sail straight into SQL. + +This half is `minor` for the same envelope reason: a request that used to return +500 now returns 400 with a different `code`, which is a visible contract change +for any caller branching on the response.