diff --git a/.changeset/security-2850-expand-rls-fls.md b/.changeset/security-2850-expand-rls-fls.md new file mode 100644 index 0000000000..952b988d63 --- /dev/null +++ b/.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. diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index 708c0b4154..ed4048d014 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -642,7 +642,9 @@ 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', @@ -650,7 +652,7 @@ describe('ObjectQL Engine', () => { 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) => { + 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' }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index a2095b6449..e57fb7a1ad 100644 --- a/packages/objectql/src/engine.ts +++ b/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(); diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 1aae460437..f34dd6e014 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/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({ diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 757a75e2bb..2ffbd87180 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/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 }; + // [#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,