Skip to content

Commit 8b27dd7

Browse files
os-zhuangzhuangjianguoclaude
authored
fix(security): enforce referenced-object RLS/FLS on $expand (#2850) (#2961)
* fix(security): enforce referenced-object RLS/FLS on $expand (#2850) `expandRelatedRecords` resolved lookup/master_detail/user references by calling the driver directly, so the REFERENCED object's row- and field-level security never ran — only tenant isolation survived. Any API/session caller who could read a base row could `?expand=` a foreign key and receive the full referenced record, including RLS-hidden rows and FLS-masked fields. Route the expand batch through the engine's own `find` so the security middleware applies the referenced object's RLS + FLS to the `id $in [...]` batch (one query, no N+1). The read is tagged with a server-set `__expandRead` context marker; the security 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. RLS injection and FLS masking run for both. Covers the list and single-record REST/protocol surfaces (find + findOne). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vh6cpe9MV7bMiXvooeWLH9 * chore: add changeset for #2850 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vh6cpe9MV7bMiXvooeWLH9 --------- Co-authored-by: Claude <zhuangjianguo@steedos.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 750901c commit 8b27dd7

5 files changed

Lines changed: 188 additions & 25 deletions

File tree

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

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

‎packages/objectql/src/engine.test.ts‎

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -642,15 +642,17 @@ describe('ObjectQL Engine', () => {
642642
expect(result[0].assignee).toEqual({ id: 'u1', name: 'Alice' });
643643
expect(result[1].assignee).toEqual({ id: 'u2', name: 'Bob' });
644644

645-
// Verify the expand query used $in
645+
// Verify the expand query used $in. The expand sub-read now routes
646+
// through the secured `find` path ([#2850]), so the referenced
647+
// read carries the `__expandRead` marker in its context.
646648
expect(mockDriver.find).toHaveBeenCalledTimes(2);
647649
expect(mockDriver.find).toHaveBeenLastCalledWith(
648650
'user',
649651
expect.objectContaining({
650652
object: 'user',
651653
where: { id: { $in: ['u1', 'u2'] } },
652654
}),
653-
undefined,
655+
expect.objectContaining({ context: { __expandRead: true } }),
654656
);
655657
});
656658

@@ -686,7 +688,7 @@ describe('ObjectQL Engine', () => {
686688
expect(mockDriver.find).toHaveBeenLastCalledWith(
687689
'sys_user',
688690
expect.objectContaining({ where: { id: { $in: ['u1'] } } }),
689-
undefined,
691+
expect.objectContaining({ context: { __expandRead: true } }),
690692
);
691693
});
692694

@@ -867,6 +869,45 @@ describe('ObjectQL Engine', () => {
867869
expect(mockDriver.find).toHaveBeenCalledTimes(1);
868870
});
869871

872+
it('[#2850] routes the expand sub-read through the middleware for the referenced object (RLS/FLS can apply), tagged __expandRead', async () => {
873+
// Regression: expand used to call the driver directly for the
874+
// referenced object, so the REFERENCED object's security middleware
875+
// (RLS/FLS/CRUD) never ran — a caller could read masked/owner-hidden
876+
// related records via `?expand=`. The sub-read now re-enters
877+
// `this.find`, so any registered middleware sees it.
878+
vi.mocked(SchemaRegistry.getObject).mockImplementation((name) => {
879+
if (name === 'task') return {
880+
name: 'task',
881+
fields: { assignee: { type: 'lookup', reference: 'user' }, title: { type: 'text' } },
882+
} as any;
883+
if (name === 'user') return { name: 'user', fields: { name: { type: 'text' } } } as any;
884+
return undefined;
885+
});
886+
887+
vi.mocked(mockDriver.find)
888+
.mockResolvedValueOnce([{ id: 't1', title: 'Task 1', assignee: 'u1' }])
889+
.mockResolvedValueOnce([{ id: 'u1', name: 'Alice' }]);
890+
891+
const seen: Array<{ object: string; operation: string; expandRead: boolean }> = [];
892+
engine.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
893+
seen.push({
894+
object: opCtx.object,
895+
operation: opCtx.operation,
896+
expandRead: opCtx.context?.__expandRead === true,
897+
});
898+
await next();
899+
});
900+
901+
await engine.find('task', { expand: { assignee: { object: 'user' } } });
902+
903+
// The base read runs through the middleware WITHOUT the marker…
904+
expect(seen).toContainEqual({ object: 'task', operation: 'find', expandRead: false });
905+
// …and the referenced object's expand sub-read runs through it WITH
906+
// the __expandRead marker set — this is the hook the security plugin
907+
// uses to apply the referenced object's RLS + FLS.
908+
expect(seen).toContainEqual({ object: 'user', operation: 'find', expandRead: true });
909+
});
910+
870911
it('should drop formula fields from driver projection and evaluate them after fetch', async () => {
871912
// Regression: planFormulaProjection used to add ALL schema fields
872913
// (including the formula fields themselves) back to projected,
@@ -1028,7 +1069,7 @@ describe('ObjectQL Engine', () => {
10281069
expect.objectContaining({
10291070
where: { id: { $in: ['u1', 'u2'] } },
10301071
}),
1031-
undefined,
1072+
expect.objectContaining({ context: { __expandRead: true } }),
10321073
);
10331074
expect(result[0].assignee).toEqual({ id: 'u1', name: 'Alice' });
10341075
expect(result[1].assignee).toEqual({ id: 'u1', name: 'Alice' });

‎packages/objectql/src/engine.ts‎

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1905,25 +1905,36 @@ export class ObjectQL implements IDataEngine {
19051905
? { $and: [idFilter, nestedAST.where] }
19061906
: idFilter;
19071907

1908-
const relatedQuery: QueryAST = {
1909-
object: referenceObject,
1910-
where,
1911-
...(nestedAST.fields ? { fields: nestedAST.fields } : {}),
1912-
...(nestedAST.orderBy ? { orderBy: nestedAST.orderBy } : {}),
1913-
// NOTE: nestedAST.limit/offset are intentionally NOT forwarded here.
1914-
// This path batch-loads every parent's related records in a single
1915-
// $in query, so a *per-parent* limit/offset can't be expressed — a
1916-
// global cap on the batch would silently drop records other parents
1917-
// need. Paginate by querying the related object directly instead.
1918-
};
1919-
1920-
const driver = this.getDriver(referenceObject);
1921-
// Propagate tenantId so cross-object expansion respects isolation —
1922-
// e.g. a contact expansion only resolves IDs visible to the caller's
1923-
// tenant. Without this the driver returns the raw FK target which
1924-
// would let a maliciously crafted FK reach across tenants.
1925-
const expandOpts = this.buildDriverOptions(execCtx);
1926-
const relatedRecords = await driver.find(referenceObject, relatedQuery, expandOpts) ?? [];
1908+
// [#2850] Resolve the referenced object through the engine's own read
1909+
// path (`this.find`) rather than the raw driver, so the security
1910+
// middleware applies the REFERENCED object's RLS + FLS to the expanded
1911+
// batch — not merely tenant isolation. A single `find` over the
1912+
// collected `id $in [...]` batch re-enters the middleware exactly once
1913+
// (no N+1), preserving the batched load. `nestedAST.limit/offset` are
1914+
// still intentionally NOT forwarded: this path batch-loads every
1915+
// parent's related records in one query, so a *per-parent* limit/offset
1916+
// can't be expressed — a global cap would silently drop records other
1917+
// parents need. Paginate by querying the related object directly.
1918+
//
1919+
// The `__expandRead` marker (set here, never from client input —
1920+
// `executionContext` is server-built) tells the security layer this is
1921+
// an expansion sub-read: it waives ONLY the object-level CRUD /
1922+
// requiredPermissions gate for PUBLIC referenced objects (already
1923+
// broadly readable, so a common status/owner lookup isn't over-blocked),
1924+
// while PRIVATE referenced objects keep the full RLS + CRUD treatment
1925+
// (you may expand only rows you could read directly). FLS masking
1926+
// applies to both. `expand` is intentionally omitted from this query so
1927+
// `find` does not re-expand — nested relations recurse below under the
1928+
// depth guard.
1929+
const relatedRecords = await this.find(
1930+
referenceObject,
1931+
{
1932+
where,
1933+
...(nestedAST.fields ? { fields: nestedAST.fields as any } : {}),
1934+
...(nestedAST.orderBy ? { orderBy: nestedAST.orderBy as any } : {}),
1935+
context: { ...(execCtx ?? {}), __expandRead: true } as ExecutionContext,
1936+
},
1937+
) ?? [];
19271938

19281939
// Build a lookup map: id → record
19291940
const recordMap = new Map<string, any>();

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

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,80 @@ describe('SecurityPlugin', () => {
459459
await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' });
460460
});
461461

462+
// ── [#2850] $expand RLS/FLS bypass — sub-read gate relaxation ────────────
463+
// The engine's expand path re-enters `find` for the referenced object with
464+
// a server-set `__expandRead` marker. For a PUBLIC referenced object the
465+
// object-level CRUD / requiredPermissions gate is waived (the row is already
466+
// broadly readable, so applying it would over-block a common status/owner
467+
// lookup) — but RLS + FLS still run. A PRIVATE referenced object keeps the
468+
// full gate: expansion may reveal only rows the caller could read directly.
469+
it('[#2850] __expandRead WAIVES the CRUD gate for a PUBLIC referenced object', async () => {
470+
const noGrantSet: PermissionSet = {
471+
name: 'member_default', label: 'Member', objects: {},
472+
} as any;
473+
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
474+
const harness = makeMiddlewareCtx({
475+
permissionSets: [noGrantSet],
476+
objectFields: ['id', 'organization_id', 'name'],
477+
orgScoping: true,
478+
});
479+
await plugin.init(harness.ctx);
480+
await plugin.start(harness.ctx);
481+
const base = (): any => ({
482+
object: 'task', operation: 'find', ast: { where: undefined },
483+
context: { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [] },
484+
});
485+
// Control: a DIRECT read with no grant on the object is denied.
486+
await expect(harness.run(base())).rejects.toMatchObject({ name: 'PermissionDeniedError' });
487+
// The SAME read tagged as an expand sub-read is allowed — the gate is waived
488+
// for the (public) referenced object.
489+
const expandCtx = base();
490+
expandCtx.context.__expandRead = true;
491+
await expect(harness.run(expandCtx)).resolves.toBeDefined();
492+
});
493+
494+
it('[#2850] __expandRead does NOT waive the gate for a PRIVATE referenced object', async () => {
495+
// Same shape as "DENIES a non-admin on a private object", but as an expand
496+
// sub-read: private objects stay strict — expand may not reveal a row the
497+
// caller could not have queried directly.
498+
const plainWildcard: PermissionSet = {
499+
name: 'member_default', label: 'Member',
500+
objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } },
501+
} as any;
502+
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
503+
const harness = makeMiddlewareCtx({
504+
permissionSets: [plainWildcard],
505+
objectFields: ['id', 'organization_id', 'signed_token'],
506+
schemaExtra: { access: { default: 'private' } },
507+
orgScoping: true,
508+
});
509+
await plugin.init(harness.ctx);
510+
await plugin.start(harness.ctx);
511+
const opCtx: any = {
512+
object: 'task', operation: 'find', ast: { where: undefined },
513+
context: { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], __expandRead: true },
514+
};
515+
await expect(harness.run(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' });
516+
});
517+
518+
it('[#2850] __expandRead still injects RLS on the expand sub-read (only the CRUD gate is waived)', async () => {
519+
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
520+
const harness = makeMiddlewareCtx({
521+
permissionSets: [tenantPolicySet],
522+
orgScoping: true,
523+
});
524+
await plugin.init(harness.ctx);
525+
await plugin.start(harness.ctx);
526+
const opCtx: any = {
527+
object: 'task', operation: 'find', ast: { where: undefined },
528+
context: { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], __expandRead: true },
529+
};
530+
await harness.run(opCtx);
531+
// The tenant wall is still AND-injected — waiving the CRUD gate on an
532+
// expand sub-read does NOT waive row-level scoping on the referenced object.
533+
expect(opCtx.ast.where).toEqual({ $and: [{ organization_id: 'org-1' }, { organization_id: 'org-1' }] });
534+
});
535+
462536
it('DENIES a caller missing the required capability (D3 AND-gate)', async () => {
463537
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
464538
const harness = makeMiddlewareCtx({

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -697,12 +697,29 @@ export class SecurityPlugin implements Plugin {
697697
? await this.getObjectSecurityMeta(opCtx.object)
698698
: { isPrivate: false, tenancyDisabled: false, isBetterAuthManaged: false, requiredPermissions: EMPTY_REQUIRED_PERMISSIONS, fieldRequiredPermissions: {} as Record<string, string[]> };
699699

700+
// [#2850] $expand sub-read gate relaxation. The engine's expand path
701+
// re-enters `find` for a referenced object carrying `__expandRead` (a
702+
// server-set marker; `executionContext` is never client-built). For a
703+
// PUBLIC referenced object — covered by the '*' wildcard grant and thus
704+
// already broadly readable — applying the object-level CRUD /
705+
// requiredPermissions gate to the EXPANSION would only surface "never
706+
// designed for expand" modeling gaps (over-blocking a legitimate
707+
// status/owner lookup) without adding protection, since the row is
708+
// already visible. So waive those two throw-gates for PUBLIC expand
709+
// sub-reads only. RLS injection (step 3) and FLS masking (step 4) still
710+
// run, and a PRIVATE referenced object keeps the FULL gate — expansion
711+
// may reveal only rows the caller could have read directly.
712+
const expandSkipCrud =
713+
opCtx.operation === 'find' &&
714+
opCtx.context?.__expandRead === true &&
715+
!secMeta.isPrivate;
716+
700717
// 1.5. [ADR-0066 D3/⑤] requiredPermissions AND-gate — a capability
701718
// prerequisite checked BEFORE the CRUD grant (ADR §Precedence): a
702719
// caller missing any required capability is denied regardless of how
703720
// permissive their grants are. Per-operation (⑤): only the caps for
704721
// THIS operation's CRUD class (plus any all-operations caps) apply.
705-
if (permissionSets.length > 0) {
722+
if (permissionSets.length > 0 && !expandSkipCrud) {
706723
const required = requiredCapsForOperation(secMeta.requiredPermissions, opCtx.operation);
707724
if (required.length > 0) {
708725
const held = this.permissionEvaluator.getSystemPermissions(permissionSets);
@@ -730,7 +747,7 @@ export class SecurityPlugin implements Plugin {
730747
}
731748

732749
// 2. CRUD permission check
733-
if (permissionSets.length > 0) {
750+
if (permissionSets.length > 0 && !expandSkipCrud) {
734751
const allowed = this.permissionEvaluator.checkObjectPermission(
735752
opCtx.operation,
736753
opCtx.object,

0 commit comments

Comments
 (0)