fix(security): enforce referenced-object RLS/FLS on $expand (#2850)

.changeset/security-2850-expand-rls-fls.md

@@ -0,0 +1,20 @@
+---
+'@objectstack/objectql': patch
+'@objectstack/plugin-security': patch
+---
+
+fix(security): enforce referenced-object RLS/FLS on $expand (#2850)
+
+`expandRelatedRecords` resolved lookup/master_detail/user references via the
+driver directly, so the referenced object's row- and field-level security never
+ran — any API/session caller who could read a base row could `?expand=` a
+foreign key and receive RLS-hidden rows and FLS-masked fields (tenant isolation
+was the only surviving boundary).
+
+The expand batch now routes through the engine's own `find`, so the security
+middleware applies the referenced object's RLS + FLS to the `id $in [...]` batch
+(one query per level, no N+1). The sub-read carries a server-set `__expandRead`
+marker: the middleware waives only the object-level CRUD / requiredPermissions
+gate for PUBLIC referenced objects (already broadly readable — avoids
+over-blocking common status/owner lookups), while PRIVATE referenced objects
+keep the full gate. Covers the list and single-record REST/protocol surfaces.

packages/objectql/src/engine.test.ts

@@ -642,15 +642,17 @@ describe('ObjectQL Engine', () => {
             expect(result[0].assignee).toEqual({ id: 'u1', name: 'Alice' });
             expect(result[1].assignee).toEqual({ id: 'u2', name: 'Bob' });
 
-            // Verify the expand query used $in
+            // Verify the expand query used $in. The expand sub-read now routes
+            // through the secured `find` path ([#2850]), so the referenced
+            // read carries the `__expandRead` marker in its context.
             expect(mockDriver.find).toHaveBeenCalledTimes(2);
             expect(mockDriver.find).toHaveBeenLastCalledWith(
                 'user',
                 expect.objectContaining({
                     object: 'user',
                     where: { id: { $in: ['u1', 'u2'] } },
                 }),
-                undefined,
+                expect.objectContaining({ context: { __expandRead: true } }),
             );
         });
 
@@ -686,7 +688,7 @@ describe('ObjectQL Engine', () => {
             expect(mockDriver.find).toHaveBeenLastCalledWith(
                 'sys_user',
                 expect.objectContaining({ where: { id: { $in: ['u1'] } } }),
-                undefined,
+                expect.objectContaining({ context: { __expandRead: true } }),
             );
         });
 
@@ -867,6 +869,45 @@ describe('ObjectQL Engine', () => {
             expect(mockDriver.find).toHaveBeenCalledTimes(1);
         });
 
+        it('[#2850] routes the expand sub-read through the middleware for the referenced object (RLS/FLS can apply), tagged __expandRead', async () => {
+            // Regression: expand used to call the driver directly for the
+            // referenced object, so the REFERENCED object's security middleware
+            // (RLS/FLS/CRUD) never ran — a caller could read masked/owner-hidden
+            // related records via `?expand=`. The sub-read now re-enters
+            // `this.find`, so any registered middleware sees it.
+            vi.mocked(SchemaRegistry.getObject).mockImplementation((name) => {
+                if (name === 'task') return {
+                    name: 'task',
+                    fields: { assignee: { type: 'lookup', reference: 'user' }, title: { type: 'text' } },
+                } as any;
+                if (name === 'user') return { name: 'user', fields: { name: { type: 'text' } } } as any;
+                return undefined;
+            });
+
+            vi.mocked(mockDriver.find)
+                .mockResolvedValueOnce([{ id: 't1', title: 'Task 1', assignee: 'u1' }])
+                .mockResolvedValueOnce([{ id: 'u1', name: 'Alice' }]);
+
+            const seen: Array<{ object: string; operation: string; expandRead: boolean }> = [];
+            engine.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
+                seen.push({
+                    object: opCtx.object,
+                    operation: opCtx.operation,
+                    expandRead: opCtx.context?.__expandRead === true,
+                });
+                await next();
+            });
+
+            await engine.find('task', { expand: { assignee: { object: 'user' } } });
+
+            // The base read runs through the middleware WITHOUT the marker…
+            expect(seen).toContainEqual({ object: 'task', operation: 'find', expandRead: false });
+            // …and the referenced object's expand sub-read runs through it WITH
+            // the __expandRead marker set — this is the hook the security plugin
+            // uses to apply the referenced object's RLS + FLS.
+            expect(seen).toContainEqual({ object: 'user', operation: 'find', expandRead: true });
+        });
+
         it('should drop formula fields from driver projection and evaluate them after fetch', async () => {
             // Regression: planFormulaProjection used to add ALL schema fields
             // (including the formula fields themselves) back to projected,
@@ -1028,7 +1069,7 @@ describe('ObjectQL Engine', () => {
                 expect.objectContaining({
                     where: { id: { $in: ['u1', 'u2'] } },
                 }),
-                undefined,
+                expect.objectContaining({ context: { __expandRead: true } }),
             );
             expect(result[0].assignee).toEqual({ id: 'u1', name: 'Alice' });
             expect(result[1].assignee).toEqual({ id: 'u1', name: 'Alice' });

packages/objectql/src/engine.ts

@@ -1905,25 +1905,36 @@ export class ObjectQL implements IDataEngine {
           ? { $and: [idFilter, nestedAST.where] }
           : idFilter;
 
-        const relatedQuery: QueryAST = {
-          object: referenceObject,
-          where,
-          ...(nestedAST.fields ? { fields: nestedAST.fields } : {}),
-          ...(nestedAST.orderBy ? { orderBy: nestedAST.orderBy } : {}),
-          // NOTE: nestedAST.limit/offset are intentionally NOT forwarded here.
-          // This path batch-loads every parent's related records in a single
-          // $in query, so a *per-parent* limit/offset can't be expressed — a
-          // global cap on the batch would silently drop records other parents
-          // need. Paginate by querying the related object directly instead.
-        };
-
-        const driver = this.getDriver(referenceObject);
-        // Propagate tenantId so cross-object expansion respects isolation —
-        // e.g. a contact expansion only resolves IDs visible to the caller's
-        // tenant. Without this the driver returns the raw FK target which
-        // would let a maliciously crafted FK reach across tenants.
-        const expandOpts = this.buildDriverOptions(execCtx);
-        const relatedRecords = await driver.find(referenceObject, relatedQuery, expandOpts) ?? [];
+        // [#2850] Resolve the referenced object through the engine's own read
+        // path (`this.find`) rather than the raw driver, so the security
+        // middleware applies the REFERENCED object's RLS + FLS to the expanded
+        // batch — not merely tenant isolation. A single `find` over the
+        // collected `id $in [...]` batch re-enters the middleware exactly once
+        // (no N+1), preserving the batched load. `nestedAST.limit/offset` are
+        // still intentionally NOT forwarded: this path batch-loads every
+        // parent's related records in one query, so a *per-parent* limit/offset
+        // can't be expressed — a global cap would silently drop records other
+        // parents need. Paginate by querying the related object directly.
+        //
+        // The `__expandRead` marker (set here, never from client input —
+        // `executionContext` is server-built) tells the security layer this is
+        // an expansion sub-read: it waives ONLY the object-level CRUD /
+        // requiredPermissions gate for PUBLIC referenced objects (already
+        // broadly readable, so a common status/owner lookup isn't over-blocked),
+        // while PRIVATE referenced objects keep the full RLS + CRUD treatment
+        // (you may expand only rows you could read directly). FLS masking
+        // applies to both. `expand` is intentionally omitted from this query so
+        // `find` does not re-expand — nested relations recurse below under the
+        // depth guard.
+        const relatedRecords = await this.find(
+          referenceObject,
+          {
+            where,
+            ...(nestedAST.fields ? { fields: nestedAST.fields as any } : {}),
+            ...(nestedAST.orderBy ? { orderBy: nestedAST.orderBy as any } : {}),
+            context: { ...(execCtx ?? {}), __expandRead: true } as ExecutionContext,
+          },
+        ) ?? [];
 
         // Build a lookup map: id → record
         const recordMap = new Map<string, any>();

packages/plugins/plugin-security/src/security-plugin.test.ts

@@ -459,6 +459,80 @@ describe('SecurityPlugin', () => {
       await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' });
     });
 
+    // ── [#2850] $expand RLS/FLS bypass — sub-read gate relaxation ────────────
+    // The engine's expand path re-enters `find` for the referenced object with
+    // a server-set `__expandRead` marker. For a PUBLIC referenced object the
+    // object-level CRUD / requiredPermissions gate is waived (the row is already
+    // broadly readable, so applying it would over-block a common status/owner
+    // lookup) — but RLS + FLS still run. A PRIVATE referenced object keeps the
+    // full gate: expansion may reveal only rows the caller could read directly.
+    it('[#2850] __expandRead WAIVES the CRUD gate for a PUBLIC referenced object', async () => {
+      const noGrantSet: PermissionSet = {
+        name: 'member_default', label: 'Member', objects: {},
+      } as any;
+      const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
+      const harness = makeMiddlewareCtx({
+        permissionSets: [noGrantSet],
+        objectFields: ['id', 'organization_id', 'name'],
+        orgScoping: true,
+      });
+      await plugin.init(harness.ctx);
+      await plugin.start(harness.ctx);
+      const base = (): any => ({
+        object: 'task', operation: 'find', ast: { where: undefined },
+        context: { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [] },
+      });
+      // Control: a DIRECT read with no grant on the object is denied.
+      await expect(harness.run(base())).rejects.toMatchObject({ name: 'PermissionDeniedError' });
+      // The SAME read tagged as an expand sub-read is allowed — the gate is waived
+      // for the (public) referenced object.
+      const expandCtx = base();
+      expandCtx.context.__expandRead = true;
+      await expect(harness.run(expandCtx)).resolves.toBeDefined();
+    });
+
+    it('[#2850] __expandRead does NOT waive the gate for a PRIVATE referenced object', async () => {
+      // Same shape as "DENIES a non-admin on a private object", but as an expand
+      // sub-read: private objects stay strict — expand may not reveal a row the
+      // caller could not have queried directly.
+      const plainWildcard: PermissionSet = {
+        name: 'member_default', label: 'Member',
+        objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } },
+      } as any;
+      const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
+      const harness = makeMiddlewareCtx({
+        permissionSets: [plainWildcard],
+        objectFields: ['id', 'organization_id', 'signed_token'],
+        schemaExtra: { access: { default: 'private' } },
+        orgScoping: true,
+      });
+      await plugin.init(harness.ctx);
+      await plugin.start(harness.ctx);
+      const opCtx: any = {
+        object: 'task', operation: 'find', ast: { where: undefined },
+        context: { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], __expandRead: true },
+      };
+      await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' });
+    });
+
+    it('[#2850] __expandRead still injects RLS on the expand sub-read (only the CRUD gate is waived)', async () => {
+      const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
+      const harness = makeMiddlewareCtx({
+        permissionSets: [tenantPolicySet],
+        orgScoping: true,
+      });
+      await plugin.init(harness.ctx);
+      await plugin.start(harness.ctx);
+      const opCtx: any = {
+        object: 'task', operation: 'find', ast: { where: undefined },
+        context: { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], __expandRead: true },
+      };
+      await harness.run(opCtx);
+      // The tenant wall is still AND-injected — waiving the CRUD gate on an
+      // expand sub-read does NOT waive row-level scoping on the referenced object.
+      expect(opCtx.ast.where).toEqual({ $and: [{ organization_id: 'org-1' }, { organization_id: 'org-1' }] });
+    });
+
     it('DENIES a caller missing the required capability (D3 AND-gate)', async () => {
       const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
       const harness = makeMiddlewareCtx({

packages/plugins/plugin-security/src/security-plugin.ts

@@ -697,12 +697,29 @@ export class SecurityPlugin implements Plugin {
           ? await this.getObjectSecurityMeta(opCtx.object)
           : { isPrivate: false, tenancyDisabled: false, isBetterAuthManaged: false, requiredPermissions: EMPTY_REQUIRED_PERMISSIONS, fieldRequiredPermissions: {} as Record<string, string[]> };
 
+      // [#2850] $expand sub-read gate relaxation. The engine's expand path
+      // re-enters `find` for a referenced object carrying `__expandRead` (a
+      // server-set marker; `executionContext` is never client-built). For a
+      // PUBLIC referenced object — covered by the '*' wildcard grant and thus
+      // already broadly readable — applying the object-level CRUD /
+      // requiredPermissions gate to the EXPANSION would only surface "never
+      // designed for expand" modeling gaps (over-blocking a legitimate
+      // status/owner lookup) without adding protection, since the row is
+      // already visible. So waive those two throw-gates for PUBLIC expand
+      // sub-reads only. RLS injection (step 3) and FLS masking (step 4) still
+      // run, and a PRIVATE referenced object keeps the FULL gate — expansion
+      // may reveal only rows the caller could have read directly.
+      const expandSkipCrud =
+        opCtx.operation === 'find' &&
+        opCtx.context?.__expandRead === true &&
+        !secMeta.isPrivate;
+
       // 1.5. [ADR-0066 D3/⑤] requiredPermissions AND-gate — a capability
       //      prerequisite checked BEFORE the CRUD grant (ADR §Precedence): a
       //      caller missing any required capability is denied regardless of how
       //      permissive their grants are. Per-operation (⑤): only the caps for
       //      THIS operation's CRUD class (plus any all-operations caps) apply.
-      if (permissionSets.length > 0) {
+      if (permissionSets.length > 0 && !expandSkipCrud) {
         const required = requiredCapsForOperation(secMeta.requiredPermissions, opCtx.operation);
         if (required.length > 0) {
           const held = this.permissionEvaluator.getSystemPermissions(permissionSets);
@@ -730,7 +747,7 @@ export class SecurityPlugin implements Plugin {
       }
 
       // 2. CRUD permission check
-      if (permissionSets.length > 0) {
+      if (permissionSets.length > 0 && !expandSkipCrud) {
         const allowed = this.permissionEvaluator.checkObjectPermission(
           opCtx.operation,
           opCtx.object,