fix(analytics): 记录级权限范围与 measure 字段校验 (#4467, #4437)

.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.

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<string, any>; orgScoping?: boolean; findOneImpl?: (query: any) => any }) => {
+  const makeMiddlewareCtx = (overrides: { permissionSets: PermissionSet[]; objectFields?: string[]; schemaExtra?: Record<string, any>; orgScoping?: boolean; findOneImpl?: (query: any) => any; sharing?: any }) => {
     const fields: Record<string, any> = {};
     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' });
+      });
+    });
   });
 
   // -------------------------------------------------------------------------