From f48289f7610ca20a5c0cc354f8700986b7dac0ad Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:54:17 +0800 Subject: [PATCH] fix(analytics): make the read-scope auto-bridge order-independent (#3618) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getReadScope` was only wired when the `security` service already existed at this plugin's init(). The closure resolved lazily, but the ASSIGNMENT was gated on an init-time probe — so a kernel registering AnalyticsServicePlugin before the security plugin got no read-scope provider at all, and every analytics strategy ran unscoped behind nothing but a WARN. Both sibling bridges (executeAggregate, executeRawSql) wire unconditionally and resolve at call time, and this one's own comment claimed it did too. Now it does; the probe only picks the log wording. `os serve` registers security first, so the CLI path was already correct. The exposure was embedders composing their own kernel — and this repo's own bootStack harness, which registers analytics at harness.ts:159 and security at :232. That means the entire dogfood/verify suite has been running with analytics RLS silently disabled, so any analytics RLS assertion written there passed vacuously. Also corrects the WARN text: with no provider nothing is scoped on any path or object, not just "the raw-SQL path" / "joined objects". Adds analytics-rls.dogfood.test.ts — an owner-scoped RLS fixture driven over real HTTP as a real non-admin, asserting the rows a member's aggregate actually returns rather than inspecting a filter object. This is the first test to exercise the getReadScope -> security.getReadFilter auto-bridge at all. Found while writing the end-to-end gate for #3597: the gate went red for THIS reason, not that one. Reverting this fix turns 3 of its 4 cases red; reverting the #3597 strategy fix turns the ObjectQL case red. Co-Authored-By: Claude --- .../analytics-read-scope-bridge-order.md | 28 ++++ .../test/analytics-rls.dogfood.test.ts | 146 ++++++++++++++++++ .../services/service-analytics/src/plugin.ts | 28 +++- 3 files changed, 196 insertions(+), 6 deletions(-) create mode 100644 .changeset/analytics-read-scope-bridge-order.md create mode 100644 packages/qa/dogfood/test/analytics-rls.dogfood.test.ts diff --git a/.changeset/analytics-read-scope-bridge-order.md b/.changeset/analytics-read-scope-bridge-order.md new file mode 100644 index 0000000000..7be6ca0667 --- /dev/null +++ b/.changeset/analytics-read-scope-bridge-order.md @@ -0,0 +1,28 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(analytics): the read-scope auto-bridge no longer depends on plugin order (#3618) + +`getReadScope` was only wired when the `security` service already existed at this +plugin's `init()`. The closure itself resolved lazily, but the ASSIGNMENT was +gated on an init-time probe — so a kernel that registers `AnalyticsServicePlugin` +before the security plugin got **no read-scope provider at all**, and every +analytics strategy ran unscoped with only a WARN to show for it. + +Both sibling bridges (`executeAggregate`, `executeRawSql`) are wired +unconditionally and resolve at call time, and this one's own comment claimed the +same. Now it actually does: the probe only decides the log wording. + +The CLI (`os serve`) registers security before analytics, so that path was +already correct. The exposure was for embedders composing their own kernel — and +for this repo's own `bootStack` harness, which registers analytics first, meaning +the entire dogfood/verify suite had analytics RLS silently disabled and any RLS +assertion written there passed vacuously. + +Also corrects the WARN text: with no provider, scoping is absent on ALL paths and +ALL objects, not just "the raw-SQL path" and "joined objects" as it claimed. + +Adds `analytics-rls.dogfood.test.ts`: an owner-scoped RLS fixture driven over real +HTTP as a real non-admin, asserting the rows a member's aggregate actually +returns. Reverting either this fix or the #3597 strategy fix turns it red. diff --git a/packages/qa/dogfood/test/analytics-rls.dogfood.test.ts b/packages/qa/dogfood/test/analytics-rls.dogfood.test.ts new file mode 100644 index 0000000000..2236be69f6 --- /dev/null +++ b/packages/qa/dogfood/test/analytics-rls.dogfood.test.ts @@ -0,0 +1,146 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// END-TO-END gate for #3597 — "analytics aggregates are scoped to the caller", +// proven through the REAL stack: HTTP → REST execution context → SecurityPlugin +// → the AnalyticsServicePlugin auto-bridges → ObjectQL/NativeSQL strategies. +// +// #3597 was a cross-tenant disclosure: ObjectQLStrategy never consumed +// `getReadScope`, and the `executeAggregate` bridge passes no ExecutionContext, +// so plugin-security's principal-less fall-open skipped its own RLS injection +// too. Both belts were off at once and an authenticated non-admin received +// aggregates computed over every row in the table. +// +// Every pre-existing analytics RLS test injects `getReadScope` as a fake into a +// hand-built AnalyticsService. NONE booted the real plugin, so the +// `getReadScope → security.getReadFilter` auto-bridge (plugin.ts:291-305) had +// zero coverage — which is how the gap shipped. This test closes that: it +// asserts on ROWS RETURNED to a real non-admin over HTTP, not on a filter object. +// +// Owner-scoped (`created_by`), not org-scoped, on purpose: a wildcard +// `organization_id` policy is STRIPPED when the enterprise `org-scoping` service +// is absent, which it is in this OSS workspace. An owner policy references +// `current_user.id` and survives single-tenant boots — so this gate actually +// bites on every CI run instead of silently passing. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { + rlsFixtureStack, + ownerScopedMemberSet, + rlsFixtureSecurity, +} from './fixtures/rls-owner-fixture.js'; + +const ADMIN_NOTES = 3; +const MEMBER_NOTES = 2; + +/** + * Date-bucketed dataset. The granularity is what makes this interesting: + * `NativeSQLStrategy.canHandle` DECLINES any query carrying a + * `dateGranularity` (native-sql-strategy.ts:30), so this routes to + * ObjectQLStrategy — the leaking path — even on a SQL-capable driver. + */ +const notesByDay = { + name: 'notes_by_day', + label: 'Notes by day', + object: 'rls_note', + dimensions: [ + { name: 'created', label: 'Created', field: 'created_at', type: 'date', dateGranularity: 'day' }, + ], + measures: [{ name: 'cnt', label: 'Count', aggregate: 'count' }], +}; + +/** Same object, NO granularity → stays on NativeSQLStrategy. The control. */ +const notesTotal = { + name: 'notes_total', + label: 'Notes total', + object: 'rls_note', + dimensions: [], + measures: [{ name: 'cnt', label: 'Count', aggregate: 'count' }], +}; + +describe('dogfood: analytics aggregates are RLS-scoped to the caller (#3597)', () => { + let stack: VerifyStack; + let adminToken: string; + let memberToken: string; + + beforeAll(async () => { + stack = await bootStack(rlsFixtureStack as never, { + security: rlsFixtureSecurity(ownerScopedMemberSet), + }); + + // First user is the seeded dev admin; every later sign-up is a plain member + // that falls back to the owner-scoped fixture permission set. + adminToken = await stack.signIn(); + memberToken = await stack.signUp('analytics-rls@verify.test'); + + // Author through HTTP as each principal so `created_by` is stamped with the + // real caller — the whole point of an owner policy. + for (let i = 0; i < ADMIN_NOTES; i++) { + const r = await stack.apiAs(adminToken, 'POST', '/data/rls_note', { + name: `admin-note-${i}`, + body: 'owned by admin', + }); + expect(r.status).toBeLessThan(300); + } + for (let i = 0; i < MEMBER_NOTES; i++) { + const r = await stack.apiAs(memberToken, 'POST', '/data/rls_note', { + name: `member-note-${i}`, + body: 'owned by member', + }); + expect(r.status).toBeLessThan(300); + } + + // Premise check — if the owner policy is not actually biting on the plain + // read path, an analytics assertion below would pass for the wrong reason. + const list = await stack.apiAs(memberToken, 'GET', '/data/rls_note'); + const listed = (await list.json()) as { records?: Array<{ name: string }> }; + expect(listed.records ?? []).toHaveLength(MEMBER_NOTES); + expect((listed.records ?? []).every((r) => r.name.startsWith('member-note'))).toBe(true); + }, 90_000); + + afterAll(async () => { + await stack?.stop(); + }); + + async function totalFor(token: string, dataset: unknown, dimensions: string[]): Promise { + const res = await stack.apiAs(token, 'POST', '/analytics/dataset/query', { + dataset, + selection: { dimensions, measures: ['cnt'] }, + }); + expect(res.status).toBe(200); + const body = (await res.json()) as { rows?: Array> }; + return (body.rows ?? []).reduce((sum, row) => sum + Number(row.cnt ?? 0), 0); + } + + it('ObjectQL path (date-bucketed): a member counts ONLY their own rows', async () => { + // Before #3601 this returned ADMIN_NOTES + MEMBER_NOTES — the member saw a + // count over rows RLS forbids them from reading. + const seen = await totalFor(memberToken, notesByDay, ['created']); + expect(seen).toBe(MEMBER_NOTES); + }); + + it('NativeSQL path (no granularity): a member counts ONLY their own rows', async () => { + const seen = await totalFor(memberToken, notesTotal, []); + expect(seen).toBe(MEMBER_NOTES); + }); + + it('the two strategies agree for the same principal', async () => { + const viaObjectql = await totalFor(memberToken, notesByDay, ['created']); + const viaNativeSql = await totalFor(memberToken, notesTotal, []); + expect(viaObjectql).toBe(viaNativeSql); + }); + + it('the rows the member cannot see DO exist — scoping, not an empty table', async () => { + // Ground truth straight from the engine under a system context: the table + // really holds every note. Without this, "member counts 2" would also pass + // on a broken setup that simply never persisted the admin's rows. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const ql = await stack.kernel.getServiceAsync('objectql'); + const all = (await ql.find('rls_note', { context: { isSystem: true } })) as unknown[]; + expect(all).toHaveLength(ADMIN_NOTES + MEMBER_NOTES); + + const memberSees = await totalFor(memberToken, notesByDay, ['created']); + expect(memberSees).toBe(MEMBER_NOTES); + expect(memberSees).toBeLessThan(all.length); + }); +}); diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index bcb2d3438a..6c97fb026f 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -290,6 +290,7 @@ export class AnalyticsServicePlugin implements Plugin { } let getReadScope = this.options.getReadScope; let autoBridgedReadScope = false; + let securityPresentAtInit = false; if (!getReadScope) { const trySecurity = (): SecurityReadFilter | undefined => { try { @@ -299,10 +300,17 @@ export class AnalyticsServicePlugin implements Plugin { return undefined; } }; - if (trySecurity()) { - getReadScope = (object, context) => trySecurity()?.getReadFilter(object, context); - autoBridgedReadScope = true; - } + // ALWAYS wire the bridge — resolution happens at call time, mirroring the + // executeAggregate / executeRawSql auto-bridges above. Gating the + // ASSIGNMENT on an init-time probe (as this did) made analytics RLS + // silently plugin-ORDER-DEPENDENT: a kernel that registers this plugin + // before the security plugin got NO read-scope provider at all, so every + // strategy ran unscoped and only a WARN marked it. The repo's own + // `bootStack` harness registers in exactly that order, which is why no + // dogfood test could ever observe analytics RLS. + securityPresentAtInit = !!trySecurity(); + getReadScope = (object, context) => trySecurity()?.getReadFilter(object, context); + autoBridgedReadScope = true; } // ADR-0021 — relationship → target-object resolver. A dataset's `include` @@ -457,12 +465,20 @@ export class AnalyticsServicePlugin implements Plugin { draftRowsResolver, }; - if (autoBridgedReadScope) { + if (autoBridgedReadScope && securityPresentAtInit) { ctx.logger.info('[Analytics] Auto-bridged getReadScope → "security" service (getReadFilter)'); + } else if (autoBridgedReadScope) { + // The bridge IS wired and will resolve at call time — this is only a + // heads-up that security had not registered yet at our init. It becomes a + // real problem only if no security service ever appears. + ctx.logger.info( + '[Analytics] getReadScope bridged to the "security" service; that service is not ' + + 'registered yet at init and will be resolved per query (plugin order is not significant).', + ); } else if (!getReadScope) { ctx.logger.warn( '[Analytics] No getReadScope configured and no "security" service with getReadFilter found — ' + - 'the raw-SQL analytics path will NOT enforce tenant/RLS scoping on joined objects (ADR-0021 D-C). ' + + 'analytics queries will NOT enforce tenant/RLS scoping (ADR-0021 D-C). ' + 'Supply getReadScope or register a security service in multi-tenant deployments.', ); }