Skip to content

Commit 587fc91

Browse files
os-zhuangclaude
andauthored
feat(analytics,spec): executeAggregate bridge carries ExecutionContext — ADR-0021 D-C second belt (#3602) (#3651)
The analytics→engine bridge now forwards the request's ExecutionContext to `engine.aggregate`, so the engine's own middleware chain scopes analytics reads independently of the analytics layer's `getReadScope`. `BaseEngineOptions.context` has always been `.optional()`, so nothing forced the bridge to pass it — and it did not. An authenticated aggregate reached the engine with no principal, plugin-security's principal-less fall-open skipped its RLS injection, and the only thing left scoping the query was the strategy remembering to call `getReadScope`. #3597 was a strategy that did not, and both belts were off at once. `getReadScope` stays: the two resolve scope through different paths (engine middleware vs `security.getReadFilter`), and a deployment without plugin-security has only the analytics layer. Depth, not a replacement. - `StrategyContext` gains `context?: ExecutionContext`, bound per call by `AnalyticsService.callCtx` — unconditionally, including when no read-scope provider is configured, since that deployment needs the engine belt most. - `StrategyContext.executeAggregate` and the plugin/service `executeAggregate` config options gain `context?: ExecutionContext`. Additive: a custom bridge that ignores it behaves exactly as before. - `fetchRecordLabels` — the dimension display-label lookup — is row-granular (one row per record, real display names) and ran with neither read scope nor context. Its ids come from a scoped aggregate today, so it leaked nothing, but that was the caller's invariant, not the bridge's. Now ANDs the target object's read scope in (`$and`, never a key merge) and forwards the context. - `ObjectQLStrategy.generateSql` emitted no WHERE at all, so `/analytics/sql` read as an unscoped table scan while the real aggregate was scoped. Now renders the caller's filters and the read scope, and runs the same joined-scope guard `execute()` does. Never executed, so this was misleading output rather than a leak. - `BootOptions.analytics` lets a gate boot with the analytics belt off; the new dogfood case asserts the engine belt alone still scopes a member to their own rows. Verified to bite: reverting the context forwarding makes it count 5 instead of 2. Deliberately not fixed here: `ObjectQLStrategy.execute()` ignores `timeDimensions[].dateRange` entirely, so `generateSql` does not render a BETWEEN either. Filed as #3650. Claude-Session: https://claude.ai/code/session_013st2KArjmLQSuVzpDS91jj Co-authored-by: Claude <noreply@anthropic.com>
1 parent c2ecfda commit 587fc91

11 files changed

Lines changed: 702 additions & 38 deletions

File tree

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/service-analytics": minor
4+
"@objectstack/verify": minor
5+
---
6+
7+
feat(analytics): the executeAggregate bridge carries ExecutionContext — ADR-0021 D-C second belt
8+
9+
The analytics→engine bridge now forwards the request's `ExecutionContext` to
10+
`engine.aggregate`, so the engine's own middleware chain scopes analytics reads
11+
independently of the analytics layer's `getReadScope`.
12+
13+
**Why.** `BaseEngineOptions.context` has always been `.optional()`, so nothing
14+
forced the bridge to pass it — and it did not. An authenticated aggregate
15+
reached the engine with no principal, plugin-security's principal-less fall-open
16+
skipped its RLS injection, and the only thing left scoping the query was the
17+
strategy remembering to call `getReadScope`. #3597 was a strategy that did not,
18+
and both belts were off at once.
19+
20+
`getReadScope` stays: the two resolve scope through different paths (engine
21+
middleware vs `security.getReadFilter`), and a deployment without
22+
plugin-security has only the analytics layer. This is depth, not a replacement.
23+
24+
- `StrategyContext` gains `context?: ExecutionContext`, bound per call by
25+
`AnalyticsService` from `query()` / `generateSql()` / `queryDataset()`.
26+
- `StrategyContext.executeAggregate` and the `AnalyticsServicePlugin` /
27+
`AnalyticsService` `executeAggregate` config options gain `context?:
28+
ExecutionContext`. **Custom bridges should forward it** to their engine; the
29+
built-in auto-bridge does. Purely additive — an existing bridge that ignores
30+
it keeps working exactly as before.
31+
- `DimensionLabelDeps.fetchRecordLabels` and `resolveDimensionLabels` each gain
32+
an optional trailing `context`, beside the `scope` / `resolveScope` that
33+
#3639 added — the same two-belt split as the aggregate path.
34+
- `BootOptions.analytics` (`@objectstack/verify`) overrides the
35+
AnalyticsServicePlugin instance, so a gate can boot with the analytics belt
36+
off and assert the engine-side belt alone still scopes.
37+
38+
**Also fixed on the same seam:**
39+
40+
- `fetchRecordLabels` — the dimension display-label lookup — is row-granular
41+
(one row per record, real display names). #3639 gave it the analytics-layer
42+
belt (the referenced object's own read scope); it now also carries the
43+
context, so the engine scopes the same read independently.
44+
- `ObjectQLStrategy.generateSql` emitted no `WHERE` at all, so the
45+
`/analytics/sql` preview read as an unscoped table scan while the real
46+
aggregate was scoped. It now renders the caller's filters and the read scope.
47+
The preview never executed, so this was misleading output rather than a leak.

packages/qa/dogfood/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
"@objectstack/plugin-auth": "workspace:*",
2121
"@objectstack/plugin-security": "workspace:*",
2222
"@objectstack/plugin-webhooks": "workspace:*",
23+
"@objectstack/service-analytics": "workspace:*",
2324
"@objectstack/service-messaging": "workspace:*",
2425
"@objectstack/service-storage": "workspace:*",
2526
"@objectstack/spec": "workspace:*",

packages/qa/dogfood/test/analytics-rls.dogfood.test.ts

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
2626
import { bootStack, type VerifyStack } from '@objectstack/verify';
27+
import { AnalyticsServicePlugin } from '@objectstack/service-analytics';
2728
import {
2829
rlsFixtureStack,
2930
ownerScopedMemberSet,
@@ -144,3 +145,74 @@ describe('dogfood: analytics aggregates are RLS-scoped to the caller (#3597)', (
144145
expect(memberSees).toBeLessThan(all.length);
145146
});
146147
});
148+
149+
/**
150+
* The SECOND belt, alone (#3602).
151+
*
152+
* Everything above runs the fully-wired stack, where the analytics layer scopes
153+
* the query itself (#3601). That proves the system is safe today; it cannot
154+
* prove the belt BENEATH it works, because the first belt masks it. #3597 was
155+
* exactly a first-belt miss — a strategy that never called `getReadScope` — and
156+
* a third strategy will eventually repeat it.
157+
*
158+
* So boot with the analytics belt explicitly OFF (`getReadScope: () => undefined`)
159+
* and assert the member still counts only their own rows. The only thing left
160+
* scoping the read is the ExecutionContext the `executeAggregate` bridge now
161+
* hands `engine.aggregate`, which the security middleware turns into an RLS
162+
* predicate on `opCtx.ast.where`. Before #3602 the bridge passed no context, the
163+
* middleware's principal-less fall-open skipped its own injection, and this case
164+
* would count every note in the table.
165+
*/
166+
describe('dogfood: engine-side RLS scopes analytics even with the analytics belt off (#3602)', () => {
167+
let stack: VerifyStack;
168+
let memberToken: string;
169+
170+
beforeAll(async () => {
171+
stack = await bootStack(rlsFixtureStack as never, {
172+
security: rlsFixtureSecurity(ownerScopedMemberSet),
173+
analytics: new AnalyticsServicePlugin({ getReadScope: () => undefined }),
174+
});
175+
176+
const adminToken = await stack.signIn();
177+
memberToken = await stack.signUp('analytics-depth@verify.test');
178+
179+
for (let i = 0; i < ADMIN_NOTES; i++) {
180+
const r = await stack.apiAs(adminToken, 'POST', '/data/rls_note', {
181+
name: `admin-note-${i}`,
182+
body: 'owned by admin',
183+
});
184+
expect(r.status).toBeLessThan(300);
185+
}
186+
for (let i = 0; i < MEMBER_NOTES; i++) {
187+
const r = await stack.apiAs(memberToken, 'POST', '/data/rls_note', {
188+
name: `member-note-${i}`,
189+
body: 'owned by member',
190+
});
191+
expect(r.status).toBeLessThan(300);
192+
}
193+
}, 90_000);
194+
195+
afterAll(async () => {
196+
await stack?.stop();
197+
});
198+
199+
it('the member counts ONLY their own rows on the ObjectQL (date-bucketed) path', async () => {
200+
const res = await stack.apiAs(memberToken, 'POST', '/analytics/dataset/query', {
201+
dataset: notesByDay,
202+
selection: { dimensions: ['created'], measures: ['cnt'] },
203+
});
204+
expect(res.status).toBe(200);
205+
const body = (await res.json()) as { rows?: Array<Record<string, number>> };
206+
const seen = (body.rows ?? []).reduce((sum, row) => sum + Number(row.cnt ?? 0), 0);
207+
208+
expect(seen).toBe(MEMBER_NOTES);
209+
});
210+
211+
it('the rows it cannot see DO exist — the belt is scoping, not an empty table', async () => {
212+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
213+
const ql = await stack.kernel.getServiceAsync<any>('objectql');
214+
const all = (await ql.find('rls_note', { context: { isSystem: true } })) as unknown[];
215+
216+
expect(all).toHaveLength(ADMIN_NOTES + MEMBER_NOTES);
217+
});
218+
});

0 commit comments

Comments
 (0)