Skip to content

Commit 0a6fb1e

Browse files
os-zhuangclaude
andauthored
fix(analytics): make the read-scope auto-bridge order-independent (#3618) (#3619)
`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: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7ffc3d3 commit 0a6fb1e

3 files changed

Lines changed: 196 additions & 6 deletions

File tree

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
fix(analytics): the read-scope auto-bridge no longer depends on plugin order (#3618)
6+
7+
`getReadScope` was only wired when the `security` service already existed at this
8+
plugin's `init()`. The closure itself resolved lazily, but the ASSIGNMENT was
9+
gated on an init-time probe — so a kernel that registers `AnalyticsServicePlugin`
10+
before the security plugin got **no read-scope provider at all**, and every
11+
analytics strategy ran unscoped with only a WARN to show for it.
12+
13+
Both sibling bridges (`executeAggregate`, `executeRawSql`) are wired
14+
unconditionally and resolve at call time, and this one's own comment claimed the
15+
same. Now it actually does: the probe only decides the log wording.
16+
17+
The CLI (`os serve`) registers security before analytics, so that path was
18+
already correct. The exposure was for embedders composing their own kernel — and
19+
for this repo's own `bootStack` harness, which registers analytics first, meaning
20+
the entire dogfood/verify suite had analytics RLS silently disabled and any RLS
21+
assertion written there passed vacuously.
22+
23+
Also corrects the WARN text: with no provider, scoping is absent on ALL paths and
24+
ALL objects, not just "the raw-SQL path" and "joined objects" as it claimed.
25+
26+
Adds `analytics-rls.dogfood.test.ts`: an owner-scoped RLS fixture driven over real
27+
HTTP as a real non-admin, asserting the rows a member's aggregate actually
28+
returns. Reverting either this fix or the #3597 strategy fix turns it red.
Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// END-TO-END gate for #3597 — "analytics aggregates are scoped to the caller",
4+
// proven through the REAL stack: HTTP → REST execution context → SecurityPlugin
5+
// → the AnalyticsServicePlugin auto-bridges → ObjectQL/NativeSQL strategies.
6+
//
7+
// #3597 was a cross-tenant disclosure: ObjectQLStrategy never consumed
8+
// `getReadScope`, and the `executeAggregate` bridge passes no ExecutionContext,
9+
// so plugin-security's principal-less fall-open skipped its own RLS injection
10+
// too. Both belts were off at once and an authenticated non-admin received
11+
// aggregates computed over every row in the table.
12+
//
13+
// Every pre-existing analytics RLS test injects `getReadScope` as a fake into a
14+
// hand-built AnalyticsService. NONE booted the real plugin, so the
15+
// `getReadScope → security.getReadFilter` auto-bridge (plugin.ts:291-305) had
16+
// zero coverage — which is how the gap shipped. This test closes that: it
17+
// asserts on ROWS RETURNED to a real non-admin over HTTP, not on a filter object.
18+
//
19+
// Owner-scoped (`created_by`), not org-scoped, on purpose: a wildcard
20+
// `organization_id` policy is STRIPPED when the enterprise `org-scoping` service
21+
// is absent, which it is in this OSS workspace. An owner policy references
22+
// `current_user.id` and survives single-tenant boots — so this gate actually
23+
// bites on every CI run instead of silently passing.
24+
25+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
26+
import { bootStack, type VerifyStack } from '@objectstack/verify';
27+
import {
28+
rlsFixtureStack,
29+
ownerScopedMemberSet,
30+
rlsFixtureSecurity,
31+
} from './fixtures/rls-owner-fixture.js';
32+
33+
const ADMIN_NOTES = 3;
34+
const MEMBER_NOTES = 2;
35+
36+
/**
37+
* Date-bucketed dataset. The granularity is what makes this interesting:
38+
* `NativeSQLStrategy.canHandle` DECLINES any query carrying a
39+
* `dateGranularity` (native-sql-strategy.ts:30), so this routes to
40+
* ObjectQLStrategy — the leaking path — even on a SQL-capable driver.
41+
*/
42+
const notesByDay = {
43+
name: 'notes_by_day',
44+
label: 'Notes by day',
45+
object: 'rls_note',
46+
dimensions: [
47+
{ name: 'created', label: 'Created', field: 'created_at', type: 'date', dateGranularity: 'day' },
48+
],
49+
measures: [{ name: 'cnt', label: 'Count', aggregate: 'count' }],
50+
};
51+
52+
/** Same object, NO granularity → stays on NativeSQLStrategy. The control. */
53+
const notesTotal = {
54+
name: 'notes_total',
55+
label: 'Notes total',
56+
object: 'rls_note',
57+
dimensions: [],
58+
measures: [{ name: 'cnt', label: 'Count', aggregate: 'count' }],
59+
};
60+
61+
describe('dogfood: analytics aggregates are RLS-scoped to the caller (#3597)', () => {
62+
let stack: VerifyStack;
63+
let adminToken: string;
64+
let memberToken: string;
65+
66+
beforeAll(async () => {
67+
stack = await bootStack(rlsFixtureStack as never, {
68+
security: rlsFixtureSecurity(ownerScopedMemberSet),
69+
});
70+
71+
// First user is the seeded dev admin; every later sign-up is a plain member
72+
// that falls back to the owner-scoped fixture permission set.
73+
adminToken = await stack.signIn();
74+
memberToken = await stack.signUp('analytics-rls@verify.test');
75+
76+
// Author through HTTP as each principal so `created_by` is stamped with the
77+
// real caller — the whole point of an owner policy.
78+
for (let i = 0; i < ADMIN_NOTES; i++) {
79+
const r = await stack.apiAs(adminToken, 'POST', '/data/rls_note', {
80+
name: `admin-note-${i}`,
81+
body: 'owned by admin',
82+
});
83+
expect(r.status).toBeLessThan(300);
84+
}
85+
for (let i = 0; i < MEMBER_NOTES; i++) {
86+
const r = await stack.apiAs(memberToken, 'POST', '/data/rls_note', {
87+
name: `member-note-${i}`,
88+
body: 'owned by member',
89+
});
90+
expect(r.status).toBeLessThan(300);
91+
}
92+
93+
// Premise check — if the owner policy is not actually biting on the plain
94+
// read path, an analytics assertion below would pass for the wrong reason.
95+
const list = await stack.apiAs(memberToken, 'GET', '/data/rls_note');
96+
const listed = (await list.json()) as { records?: Array<{ name: string }> };
97+
expect(listed.records ?? []).toHaveLength(MEMBER_NOTES);
98+
expect((listed.records ?? []).every((r) => r.name.startsWith('member-note'))).toBe(true);
99+
}, 90_000);
100+
101+
afterAll(async () => {
102+
await stack?.stop();
103+
});
104+
105+
async function totalFor(token: string, dataset: unknown, dimensions: string[]): Promise<number> {
106+
const res = await stack.apiAs(token, 'POST', '/analytics/dataset/query', {
107+
dataset,
108+
selection: { dimensions, measures: ['cnt'] },
109+
});
110+
expect(res.status).toBe(200);
111+
const body = (await res.json()) as { rows?: Array<Record<string, number>> };
112+
return (body.rows ?? []).reduce((sum, row) => sum + Number(row.cnt ?? 0), 0);
113+
}
114+
115+
it('ObjectQL path (date-bucketed): a member counts ONLY their own rows', async () => {
116+
// Before #3601 this returned ADMIN_NOTES + MEMBER_NOTES — the member saw a
117+
// count over rows RLS forbids them from reading.
118+
const seen = await totalFor(memberToken, notesByDay, ['created']);
119+
expect(seen).toBe(MEMBER_NOTES);
120+
});
121+
122+
it('NativeSQL path (no granularity): a member counts ONLY their own rows', async () => {
123+
const seen = await totalFor(memberToken, notesTotal, []);
124+
expect(seen).toBe(MEMBER_NOTES);
125+
});
126+
127+
it('the two strategies agree for the same principal', async () => {
128+
const viaObjectql = await totalFor(memberToken, notesByDay, ['created']);
129+
const viaNativeSql = await totalFor(memberToken, notesTotal, []);
130+
expect(viaObjectql).toBe(viaNativeSql);
131+
});
132+
133+
it('the rows the member cannot see DO exist — scoping, not an empty table', async () => {
134+
// Ground truth straight from the engine under a system context: the table
135+
// really holds every note. Without this, "member counts 2" would also pass
136+
// on a broken setup that simply never persisted the admin's rows.
137+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
138+
const ql = await stack.kernel.getServiceAsync<any>('objectql');
139+
const all = (await ql.find('rls_note', { context: { isSystem: true } })) as unknown[];
140+
expect(all).toHaveLength(ADMIN_NOTES + MEMBER_NOTES);
141+
142+
const memberSees = await totalFor(memberToken, notesByDay, ['created']);
143+
expect(memberSees).toBe(MEMBER_NOTES);
144+
expect(memberSees).toBeLessThan(all.length);
145+
});
146+
});

packages/services/service-analytics/src/plugin.ts

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,7 @@ export class AnalyticsServicePlugin implements Plugin {
290290
}
291291
let getReadScope = this.options.getReadScope;
292292
let autoBridgedReadScope = false;
293+
let securityPresentAtInit = false;
293294
if (!getReadScope) {
294295
const trySecurity = (): SecurityReadFilter | undefined => {
295296
try {
@@ -299,10 +300,17 @@ export class AnalyticsServicePlugin implements Plugin {
299300
return undefined;
300301
}
301302
};
302-
if (trySecurity()) {
303-
getReadScope = (object, context) => trySecurity()?.getReadFilter(object, context);
304-
autoBridgedReadScope = true;
305-
}
303+
// ALWAYS wire the bridge — resolution happens at call time, mirroring the
304+
// executeAggregate / executeRawSql auto-bridges above. Gating the
305+
// ASSIGNMENT on an init-time probe (as this did) made analytics RLS
306+
// silently plugin-ORDER-DEPENDENT: a kernel that registers this plugin
307+
// before the security plugin got NO read-scope provider at all, so every
308+
// strategy ran unscoped and only a WARN marked it. The repo's own
309+
// `bootStack` harness registers in exactly that order, which is why no
310+
// dogfood test could ever observe analytics RLS.
311+
securityPresentAtInit = !!trySecurity();
312+
getReadScope = (object, context) => trySecurity()?.getReadFilter(object, context);
313+
autoBridgedReadScope = true;
306314
}
307315

308316
// ADR-0021 — relationship → target-object resolver. A dataset's `include`
@@ -457,12 +465,20 @@ export class AnalyticsServicePlugin implements Plugin {
457465
draftRowsResolver,
458466
};
459467

460-
if (autoBridgedReadScope) {
468+
if (autoBridgedReadScope && securityPresentAtInit) {
461469
ctx.logger.info('[Analytics] Auto-bridged getReadScope → "security" service (getReadFilter)');
470+
} else if (autoBridgedReadScope) {
471+
// The bridge IS wired and will resolve at call time — this is only a
472+
// heads-up that security had not registered yet at our init. It becomes a
473+
// real problem only if no security service ever appears.
474+
ctx.logger.info(
475+
'[Analytics] getReadScope bridged to the "security" service; that service is not ' +
476+
'registered yet at init and will be resolved per query (plugin order is not significant).',
477+
);
462478
} else if (!getReadScope) {
463479
ctx.logger.warn(
464480
'[Analytics] No getReadScope configured and no "security" service with getReadFilter found — ' +
465-
'the raw-SQL analytics path will NOT enforce tenant/RLS scoping on joined objects (ADR-0021 D-C). ' +
481+
'analytics queries will NOT enforce tenant/RLS scoping (ADR-0021 D-C). ' +
466482
'Supply getReadScope or register a security service in multi-tenant deployments.',
467483
);
468484
}

0 commit comments

Comments
 (0)