Skip to content

Commit 0882584

Browse files
committed
fix(service-analytics): require warn on the admission sink and deny the two silent bridge corners
Contract review CHANGES REQUIRED on PR #16860, findings F1, F3 and F9. F1 — `AdmissionLogger` declared `error?` and `warn?`, which is a contract that permits silence (#9754). `warn` is now REQUIRED, so every value of the type carries a destination for a refusal report, and the fail-closed branch reaches for it when `error` is absent instead of dropping the report. F3 — the analytics -> `security` admission bridge collapsed three resolutions into one. A `getService('security')` that THROWS, and a registered service carrying neither `canReadObject` nor `explain`, both returned `undefined` and were then read as "no security service" — admitting the query silently. Those are wired-but-broken providers and `/data`'s middleware does not fall open in either state, so both now DENY and report at `error`. An ABSENT service still admits: that deployment has no object-level gate on `/data` either, so the two doors still agree, which is the property being defended. F9 — `raw-sql-object-routing.test.ts` now asserts that every warning in the fixture IS the deliberate admission-bridge init report, rather than only excluding the routing phrase. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
1 parent 65264f7 commit 0882584

5 files changed

Lines changed: 343 additions & 36 deletions

File tree

.changeset/analytics-inline-dataset-object-read-admission.md

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,27 @@
55
"@objectstack/verify": minor
66
---
77

8+
fix(service-analytics)!: `POST /analytics/dataset/query` asks the OBJECT-level read grant before it serves an inline dataset (#16645)
9+
10+
<!-- adr-0087: not-required (no-migration-prescription) Nothing authorable is renamed, retired or re-typed: no `packages/spec` key changes its name, its type or its optionality, no stored shape moves, and every dataset, dashboard and analytics request body parses byte-identically to before — so `objectstack migrate meta` has nothing to rewrite and this changeset carries no rewrite instructions. What narrows is the ACCEPT SET of a published route at REQUEST time: `POST /analytics/dataset/query` (and the `/analytics/query` and `/analytics/sql` doors) now refuse a caller who holds no object-level read grant on an object the request reads, which is the same verdict `GET /data/<object>` already returns for that caller on that deployment. The remedy for a caller who is refused is a GRANT, held in permission-set data rather than in an authored file: the deployment gives the principal read on the object, exactly as it must today to use `/data`. There is no authored artifact and no stored representation for a migration to act on, and the additions to the contract are additive (a new OPTIONAL `ISecurityService.canReadObject`, new optional keys on three option payloads), which is a widening rather than a retirement. -->
11+
12+
**BREAKING** in the accept-set sense — an accept-set narrowing on a published
13+
route — landing in the launch window as `minor` on all four packages (the
14+
lockstep convention: during the window the bump level is not the carrier, this
15+
banner and the disposition above are). Nothing that was already admitted
16+
becomes refused **except** the requests `GET /data/<object>` refuses today for
17+
the same principal, which is the defect. Nothing that was refused becomes
18+
admitted.
19+
820
`POST /analytics/dataset/query` now asks the OBJECT-level read grant before it serves an inline dataset, so the analytics door and `GET /data/<object>` reach one admission verdict on every driver.
921

1022
The route accepts an inline dataset definition (`body.dataset`) from any authenticated caller. On a SQL driver the compiled statement ran through the driver's raw `execute()`, which is documented as a tenant-isolation bypass and which no middleware sits in front of — so the request reached the database having passed exactly ONE of the three read layers (the row scope, threaded since ADR-0021 D-C). A caller with **no grant of any kind** on an object received its row count, and with `dimensions` its grouped counts by any column, where the `/data` door answered `403 PERMISSION_DENIED` for the same principal on the same deployment. On the memory driver the identical request fell through to the ObjectQL engine, which applies all three layers in one place, and was refused. The exposure is not opt-in and an application cannot decline it: a deployment shipping 0 datasets and 0 dashboards has the identical surface, because the reachable slot is the inline definition rather than a declared one.
1123

12-
**This change NARROWS what the analytics doors accept.** Requests that were already refused by `/data` are now refused by analytics too; nothing that was refused becomes admitted.
24+
**This change NARROWS what the analytics doors accept.** Requests that were already refused by `/data` are now refused by analytics too; nothing that was refused becomes admitted. "Fails closed" is a statement about a WIRED provider: a deployment with no `security` service registered keeps its previous analytics behaviour by design, because on that deployment `/data` carries no object-level gate either and the equivalence is what is being defended.
1325

1426
- **`ISecurityService.canReadObject(object, context)`** (`@objectstack/spec`, optional) — the object-level half of a read, the sibling of `getReadFilter`'s row-level half. It exists because the two are not interchangeable: `getReadFilter` answers "which rows" and answers `undefined` — "no row restriction" — for a caller who may not read the object at all, so a door holding only the filter reads a caller with NO grant as a caller with NO restriction. Fails CLOSED. Absence is a defined state and its fallback is **not** "admit": a consumer composes the same verdict from `explain`, which is not optional.
1527
- **`@objectstack/plugin-security` implements it** as the middleware's own read gate, arm for arm and in its order — the `isSystem` bypass, the "no permission sets resolved" skip, the #3545 fail-closed refusal on an unresolvable object posture, the ADR-0066 D3 `requiredPermissions` capability AND-gate, the `allowRead` CRUD grant, and the ADR-0090 D10 delegator intersection — from the same primitives the middleware calls, and it is exposed on the registered `security` service.
16-
- **`@objectstack/service-analytics` asks it once at the door**, for the base object and every joined object, **ahead of strategy selection**. Placement is the fix: two strategies each enforcing their own copy of three layers is the CAUSE of the divergence, not its remedy, so both strategies — and any strategy added later — inherit one verdict by construction. `AnalyticsServicePlugin` auto-bridges the new `admitObjectRead` hook to the `security` service (`canReadObject`, falling back to `explain`), the same way it already bridges `getReadScope`, and warns loudly at init when no security service is registered.
28+
- **`@objectstack/service-analytics` asks it once at the door**, for the base object and every joined object, **ahead of strategy selection**. Placement is the fix: two strategies each enforcing their own copy of three layers is the CAUSE of the divergence, not its remedy, so both strategies — and any strategy added later — inherit one verdict by construction. `AnalyticsServicePlugin` auto-bridges the new `admitObjectRead` hook to the `security` service (`canReadObject`, falling back to `explain`), the same way it already bridges `getReadScope`, and warns loudly at init when no security service is registered. The bridge tells three resolutions apart: an ABSENT `security` service admits (that deployment has no object-level gate on `/data` either, so the two doors still agree, and this is what keeps a deployment shipping no `plugin-security` working as before); a service that cannot be USED — resolving it throws, or it exposes neither `canReadObject` nor `explain` — DENIES and reports at `error`, because `/data`'s middleware does not fall open in those states.
1729
- **`@objectstack/verify`** gains `bootStack(app, { databaseDriver: 'sqlite-wasm' | 'memory' })`, because a two-driver equivalence property cannot be measured on one driver — which is how the strategies were allowed to disagree.
1830

1931
The refusal is `PERMISSION_DENIED` / 403, the same code and status the engine path already answers, and it names only the object the caller themselves named.
Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* The analytics → `security` admission bridge, and the three resolutions it
5+
* must tell apart.
6+
*
7+
* The object-level gate at the analytics door is only as good as the answer the
8+
* bridge brings back, and the bridge has three outcomes that are easy to
9+
* collapse into one:
10+
*
11+
* - the `security` service is ABSENT — this deployment has no object-level
12+
* gate anywhere, `GET /data/<object>` included, because that gate IS the
13+
* absent middleware. The two doors agree, which is the equivalence property
14+
* the card asks for, so the query is ADMITTED and the state is reported at
15+
* init;
16+
* - resolving the service THREW — a security service exists on this
17+
* deployment and could not be reached;
18+
* - the service resolved but exposes NEITHER `canReadObject` NOR `explain` —
19+
* it exists and cannot answer.
20+
*
21+
* The last two are wired-but-broken providers. `/data`'s middleware does not
22+
* fall open in either state, so admitting here would reopen exactly the
23+
* divergence between the two doors that this gate closes — and would do it
24+
* silently, which is worse than the original defect: the original at least had
25+
* a shape a reader could find in the code. Both DENY, and both say why at
26+
* `error`.
27+
*
28+
* ⛔ The absent case is not a bug to be tightened away. It is the negative
29+
* control that keeps the two deny cases honest: a bridge that denied on absence
30+
* too would refuse every analytics query on every deployment that ships no
31+
* `plugin-security`, which is a strictly different (and wrong) answer from the
32+
* one `/data` gives on that same deployment.
33+
*/
34+
35+
import { describe, it, expect, vi } from 'vitest';
36+
import { DatasetSchema } from '@objectstack/spec/ui';
37+
import type { ExecutionContext } from '@objectstack/spec/kernel';
38+
import { AnalyticsServicePlugin } from '../plugin.js';
39+
import type { AnalyticsService } from '../analytics-service.js';
40+
41+
/** The reported probe's shape: one object, one count measure, no dimensions. */
42+
const probe = DatasetSchema.parse({
43+
name: 'probe_member',
44+
label: 'probe',
45+
object: 'employer_member',
46+
dimensions: [],
47+
measures: [{ name: 'cnt', label: 'Count', aggregate: 'count' }],
48+
});
49+
50+
const CALLER = { userId: 'u_seeker', tenantId: 'org_a' } as ExecutionContext;
51+
52+
/** The SQL posture — the reported path, where nothing else stands in the way. */
53+
const nativeSql = () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false });
54+
55+
/**
56+
* Engine double. Every read it serves is recorded, so a denial can be asserted
57+
* as "the database was never reached" rather than only as a thrown envelope —
58+
* a gate that refuses AFTER running the statement has not refused anything.
59+
*/
60+
function fakeEngine() {
61+
const reads: string[] = [];
62+
return {
63+
reads,
64+
engine: {
65+
execute: async (sql: unknown, options?: { object?: string }) => {
66+
reads.push(`execute:${options?.object ?? String(sql)}`);
67+
return { rows: [{ cnt: 24 }] };
68+
},
69+
aggregate: async (object: string) => {
70+
reads.push(`aggregate:${object}`);
71+
return [{ cnt: 24 }];
72+
},
73+
getObject: (name: string) =>
74+
name === 'employer_member' ? { fields: { id: { type: 'text' } } } : undefined,
75+
resolveEffectiveDatasource: () => undefined,
76+
},
77+
};
78+
}
79+
80+
/**
81+
* Minimal `PluginContext`. `security` is supplied as a THUNK so a fixture can
82+
* make the lookup itself throw — the corner that is otherwise unreachable from
83+
* a plain service map.
84+
*/
85+
function fakePluginContext(opts: {
86+
data: unknown;
87+
security?: () => unknown;
88+
}) {
89+
const registered: Record<string, unknown> = {};
90+
const warn = vi.fn();
91+
const error = vi.fn();
92+
return {
93+
registered,
94+
warn,
95+
error,
96+
ctx: {
97+
getService: (name: string) => {
98+
if (name === 'security') return opts.security ? opts.security() : undefined;
99+
if (name === 'data') return opts.data;
100+
return registered[name];
101+
},
102+
registerService: (name: string, svc: unknown) => { registered[name] = svc; },
103+
replaceService: (name: string, svc: unknown) => { registered[name] = svc; },
104+
logger: { info() {}, warn, error, debug() {} },
105+
},
106+
};
107+
}
108+
109+
async function bootAnalytics(security?: () => unknown) {
110+
const { engine, reads } = fakeEngine();
111+
const { ctx, registered, error } = fakePluginContext({ data: engine, security });
112+
await new AnalyticsServicePlugin({ queryCapabilities: nativeSql }).init(ctx as never);
113+
return { service: registered.analytics as AnalyticsService, reads, error };
114+
}
115+
116+
const runProbe = (service: AnalyticsService) =>
117+
service.queryDataset(probe as never, { measures: ['cnt'] } as never, CALLER);
118+
119+
describe('analytics admission bridge — resolving the "security" service', () => {
120+
// ── The two corners that used to admit silently ────────────────────────────
121+
122+
it('DENIES when resolving the "security" service THROWS', async () => {
123+
const boom = () => { throw new Error('security service is initialising'); };
124+
const { service, reads, error } = await bootAnalytics(boom);
125+
126+
await expect(runProbe(service)).rejects.toMatchObject({
127+
code: 'PERMISSION_DENIED',
128+
status: 403,
129+
});
130+
// The refusal has to happen BEFORE the statement runs, or it is not a gate.
131+
expect(reads).toEqual([]);
132+
// And it has to be findable. A security refusal nobody can see is
133+
// indistinguishable from a gate that never ran.
134+
expect(error.mock.calls.map((c) => String(c[0])).join('\n')).toMatch(
135+
/read admission could not be resolved .* denying the query \(fail-closed\).*threw/s,
136+
);
137+
});
138+
139+
it('DENIES when the "security" service exposes neither canReadObject nor explain', async () => {
140+
// A registered object that is not the contract it claims to be —
141+
// `explain` is NON-optional on `ISecurityService`, so a conforming
142+
// provider never lands here.
143+
const { service, reads, error } = await bootAnalytics(() => ({ getReadFilter: () => undefined }));
144+
145+
await expect(runProbe(service)).rejects.toMatchObject({
146+
code: 'PERMISSION_DENIED',
147+
status: 403,
148+
});
149+
expect(reads).toEqual([]);
150+
expect(error.mock.calls.map((c) => String(c[0])).join('\n')).toMatch(
151+
/read admission could not be resolved .* neither canReadObject\(\) nor explain\(\)/s,
152+
);
153+
});
154+
155+
// ── The negative control: absence is a different state and still ADMITS ────
156+
157+
it('ADMITS when NO "security" service is registered at all', async () => {
158+
// ⛔ Not a corner to tighten. On this deployment `/data` has no
159+
// object-level gate either, so the two doors still agree — which is the
160+
// property being defended. Tightening this to a denial would refuse every
161+
// analytics query on every deployment shipping no `plugin-security`.
162+
const { service, reads } = await bootAnalytics(undefined);
163+
164+
const result = await runProbe(service);
165+
expect(result.rows).toEqual([{ cnt: 24 }]);
166+
expect(reads).toHaveLength(1);
167+
});
168+
169+
// ── The two working spellings, so the deny cases cannot pass by refusing all ─
170+
171+
it('asks canReadObject when the service has it, and serves an ADMITTED caller', async () => {
172+
const canReadObject = vi.fn(() => true);
173+
const { service, reads } = await bootAnalytics(() => ({ canReadObject }));
174+
175+
const result = await runProbe(service);
176+
expect(result.rows).toEqual([{ cnt: 24 }]);
177+
expect(canReadObject).toHaveBeenCalledWith('employer_member', CALLER);
178+
expect(reads).toHaveLength(1);
179+
});
180+
181+
it('refuses through canReadObject when that service answers false', async () => {
182+
const { service, reads } = await bootAnalytics(() => ({ canReadObject: () => false }));
183+
184+
await expect(runProbe(service)).rejects.toMatchObject({
185+
code: 'PERMISSION_DENIED',
186+
status: 403,
187+
});
188+
expect(reads).toEqual([]);
189+
});
190+
191+
it('falls back to explain for a service that predates canReadObject — both verdicts', async () => {
192+
const admitted = await bootAnalytics(() => ({
193+
explain: async () => ({ allowed: true }),
194+
}));
195+
expect((await runProbe(admitted.service)).rows).toEqual([{ cnt: 24 }]);
196+
197+
const refused = await bootAnalytics(() => ({
198+
explain: async () => ({ allowed: false }),
199+
}));
200+
await expect(runProbe(refused.service)).rejects.toMatchObject({
201+
code: 'PERMISSION_DENIED',
202+
status: 403,
203+
});
204+
expect(refused.reads).toEqual([]);
205+
});
206+
});

packages/services/service-analytics/src/__tests__/raw-sql-object-routing.test.ts

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -254,13 +254,23 @@ describe('executeRawSql auto-bridge routes by object (#5033)', () => {
254254
{ stage: 'won', deal_count: 2 },
255255
{ stage: 'lost', deal_count: 1 },
256256
]);
257-
// Scoped to the degradation this case is about, exactly as its sibling
258-
// above spells it — ⛔ not a blanket "no warning at all". The analytics
259-
// plugin now also reports at init when no `security` service is registered
260-
// to answer the OBJECT-LEVEL read grant, and this fixture deliberately
261-
// registers none; a blanket assertion would read that deliberate report as
262-
// a routing regression.
263-
expect(warn.mock.calls.map(String).join('\n')).not.toMatch(/is unavailable/);
257+
// The blanket `expect(warn).not.toHaveBeenCalled()` this replaces could no
258+
// longer hold: the plugin reports at init when no `security` service is
259+
// registered to answer the OBJECT-LEVEL read grant, and this fixture
260+
// deliberately registers none.
261+
//
262+
// ⛔ But the replacement is not "anything except the routing phrase"
263+
// either — that admits every OTHER new warning into a case whose whole job
264+
// is to prove this object's routing did not regress. Every warning that
265+
// fires here must BE the one deliberate report, named; a second one, of any
266+
// wording, fails this case.
267+
const warnings = warn.mock.calls.map((c) => String(c[0]));
268+
expect(warnings).toHaveLength(1);
269+
expect(warnings[0]).toMatch(
270+
/No admitObjectRead configured and no "security" service registered at init/,
271+
);
272+
// …and it is emphatically not the degradation this case is about.
273+
expect(warnings[0]).not.toMatch(/is unavailable/);
264274
});
265275
});
266276

0 commit comments

Comments
 (0)