From 55886f1ec0d19bc3bbf38c15d21762e78a1fc305 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 07:40:17 +0000 Subject: [PATCH] fix(plugin-security): getReadFilter applies the controlled_by_parent derivation (#5815) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getReadFilter` is the read-scope provider bound by the analytics / raw-SQL path — the one read surface that bypasses the engine and has no other source of scope. Its contract is to return the same filter the engine middleware ANDs into every find; that middleware injects three layers (RLS, the ADR-0055 controlled_by_parent derivation, plugin-sharing's OWD/record-share filter) and this method composed only the first and third. For a controlled_by_parent object the gap was total, not partial: such an object carries no authored RLS by design and maps to `public` in plugin-sharing's effectiveSharingModel, so both composed layers returned null and the scope came back `undefined` — no predicate. A caller who could not read a single master row could still COUNT(*) / GROUP BY its detail rows. The derivation is now ANDed into the same composition, resolved from the permission sets the method already resolved (no second resolution). It is internally fail-closed and a throw reaches the method's existing fail-closed handler. The delegated (onBehalfOf) branch already denied outright (#2852) and is untouched, as is the derivation's own semantics (#5386). Tests extend the #5386 fixture with the analytics read face, pinning that the middleware-injected `ast.where` and `getReadFilter` yield the same visible row set — with one row excluded on both, so the agreement carries information. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- .../getreadfilter-controlled-by-parent.md | 42 +++++++ .../src/controlled-by-parent-sharing.test.ts | 116 +++++++++++++++++- .../plugin-security/src/security-plugin.ts | 53 +++++++- 3 files changed, 206 insertions(+), 5 deletions(-) create mode 100644 .changeset/getreadfilter-controlled-by-parent.md diff --git a/.changeset/getreadfilter-controlled-by-parent.md b/.changeset/getreadfilter-controlled-by-parent.md new file mode 100644 index 0000000000..e5fe513196 --- /dev/null +++ b/.changeset/getreadfilter-controlled-by-parent.md @@ -0,0 +1,42 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): `getReadFilter` applies the `controlled_by_parent` derivation — the analytics read scope was missing the master half entirely + +`getReadFilter` is the read-scope provider bound by the analytics / raw-SQL +path: the one read surface that bypasses the engine and therefore has no other +source of scope. Its contract is that it returns **the same filter the engine +middleware ANDs into every find**. That middleware injects three things — the +RLS filter, the ADR-0055 `controlled_by_parent` derivation (`masterFK IN +(accessible master ids)`), and plugin-sharing's OWD / record-share filter. +`getReadFilter` composed only the first and third; `computeControlledByParentFilter` +was never called on that path at all. + +For an object whose `sharingModel` is `controlled_by_parent` that is not a +partial gap but a total one, because the two layers it *did* compose both stand +down on exactly that object by design: such an object carries no authored RLS +(the whole point of the model is that access is derived rather than authored), +and it maps to `public` in plugin-sharing's `effectiveSharingModel`, so +`buildReadFilter` returns `null`. Both halves returned `null`, the composition +returned `undefined`, and the analytics path ran with **no predicate**. A caller +who could not read a single master row through `/data` could still `COUNT(*)` +and `GROUP BY` its detail rows through `/analytics` — and line-item objects are +the usual shape here, so the grouped values are per-line prices and discounts. + +The derivation is now composed into the same AND on that path, resolved from the +permission sets `getReadFilter` had already resolved (no second resolution), so +the two read surfaces enforce identical scoping — which is why +`computeControlledByParentFilter` was extracted and shared in the first place. +Failures deny: the derivation is internally fail-closed, and a throw propagates +to the method's existing fail-closed handler rather than widening the read. The +delegated (`onBehalfOf`) branch already denied outright on this path (#2852) and +is unchanged. + +This is the same failure shape #4467 fixed for the OWD/sharing layer of this +method, one layer over; #5386 fixed *which inputs* the derivation folds in, not +*whether it runs* on this surface. + +**Impact.** A deployment with `controlled_by_parent` objects and an analytics / +raw-SQL consumer will see those queries return fewer rows — the rows the caller +was never entitled to aggregate. No authoring change is required. diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts index a596074a11..ca585aac6e 100644 --- a/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts +++ b/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts @@ -229,6 +229,18 @@ async function boot(options: BootOptions = {}) { .map((r) => String(r.id)); }; + /** + * [#5815] The ANALYTICS read face: the scope `getReadFilter` hands the + * raw-SQL path, applied to the same rows. No middleware runs here — this + * method IS the whole enforcement on that surface. + */ + const analyticsVisibleContacts = async (context?: any): Promise => { + const filter = await plugin.getReadFilter('crm_contact', context ?? repContext()); + return (store.rows.crm_contact ?? []) + .filter((r) => matchesFilterCondition(r, (filter ?? null) as any)) + .map((r) => String(r.id)); + }; + /** The WRITE face: a by-id update of one detail row. Resolves or throws. */ const updateContact = async (id: string): Promise => { const opCtx: any = { @@ -254,7 +266,16 @@ async function boot(options: BootOptions = {}) { return out; }; - return { store, ctx, visibleContacts, updateContact, writableContacts }; + return { + store, + ctx, + plugin, + repContext, + visibleContacts, + analyticsVisibleContacts, + updateContact, + writableContacts, + }; } describe('[#5386] controlled_by_parent folds the master\'s ownership and share grants in', () => { @@ -353,3 +374,96 @@ describe('[#5386] controlled_by_parent folds the master\'s ownership and share g await expect(h.updateContact('ct_us')).rejects.toThrow(/record sharing/); }); }); + +// --------------------------------------------------------------------------- + +/** + * [#5815] The THIRD face of the same contract — `getReadFilter`, the read-scope + * provider bound by the analytics / raw-SQL path. + * + * The suite above pins the engine middleware and the by-id write gate against + * one fixture precisely so a third, quieter answer cannot hide between them. + * There WAS one: `getReadFilter` composed the RLS layer and the sharing layer + * and never called the controlled_by_parent derivation at all. On this exact + * fixture both of those layers legitimately return `null` — the app authors no + * RLS, and plugin-sharing maps `controlled_by_parent` to `public` — so the + * composed scope came back `undefined`: NO predicate. A caller who could not + * read `acct_eu` could still `COUNT(*)` and `GROUP BY` its contacts. + * + * These cases therefore assert AGREEMENT, not a filter shape. The two read + * surfaces may compose their layers in any order and produce different ASTs; + * what they may never do is disagree about which rows are visible. + */ +describe('[#5815] getReadFilter enforces the same read scope as the engine middleware', () => { + it('PARITY: the analytics read scope and the middleware-injected `ast.where` see the same rows', async () => { + const h = await boot({ shareLevel: 'edit' }); + const viaMiddleware = await h.visibleContacts(); + const viaReadFilter = await h.analyticsVisibleContacts(); + expect(viaReadFilter.sort()).toEqual(viaMiddleware.sort()); + // …and the agreement carries information: one row is excluded on BOTH + // surfaces. Without this, "identical" would be satisfied by two surfaces + // that each return everything — which is exactly the broken state. + expect(viaReadFilter).not.toContain('ct_eu'); + expect(viaReadFilter).toHaveLength(2); + }); + + it('the returned scope is a real predicate, not `undefined` — the defect measured', async () => { + const h = await boot({ shareLevel: 'edit' }); + const filter = await h.plugin.getReadFilter('crm_contact', h.repContext()); + // Before the fix this was `undefined` (RLS null AND sharing null), which + // the raw-SQL path spreads into a WHERE that constrains nothing. + expect(filter).toBeDefined(); + expect(JSON.stringify(filter)).toContain('acct_own'); + expect(JSON.stringify(filter)).not.toContain('acct_eu'); + }); + + it('PARITY with no grant at all: both surfaces narrow to the caller-owned master', async () => { + const h = await boot({ shareLevel: null }); + expect(await h.analyticsVisibleContacts()).toEqual(await h.visibleContacts()); + expect(await h.analyticsVisibleContacts()).toEqual(['ct_own']); + }); + + it('PARITY on a READ-level grant: the read share widens the analytics face too', async () => { + const h = await boot({ shareLevel: 'read' }); + expect(await h.analyticsVisibleContacts()).toEqual(['ct_us', 'ct_own']); + expect(await h.analyticsVisibleContacts()).toEqual(await h.visibleContacts()); + }); + + it('PARITY without plugin-sharing: neither surface narrows, for the same reason', async () => { + // Both faces stand down together — there is no owner scope and there are no + // grants anywhere in the deployment, so a direct find of the master returns + // every row. Parity must hold in the WIDE direction as well, or the pin + // would merely be asserting that the analytics face is always narrower. + const h = await boot({ shareLevel: 'edit', sharing: 'none' }); + expect(await h.analyticsVisibleContacts()).toEqual(['ct_us', 'ct_eu', 'ct_own']); + expect(await h.analyticsVisibleContacts()).toEqual(await h.visibleContacts()); + }); + + it('fail-closed: a sharing service that throws denies the analytics face too', async () => { + const h = await boot({ shareLevel: 'edit', sharing: 'throws' }); + expect(await h.analyticsVisibleContacts()).toEqual([]); + expect(await h.analyticsVisibleContacts()).toEqual(await h.visibleContacts()); + }); + + it('fail-closed: the derivation resolves the master ONCE per call, never re-entering the detail', async () => { + const h = await boot({ shareLevel: 'edit' }); + h.store.find.mockClear(); + await h.analyticsVisibleContacts(); + const reads = h.store.find.mock.calls.map((c: any[]) => String(c[0])); + // Direct evidence the derivation (and its sharing half) ran on THIS path + // rather than a filter merely coming back from somewhere. + expect(reads).toContain('sys_record_share'); + expect(reads.filter((o) => o === 'crm_account')).toHaveLength(1); + expect(reads).not.toContain('crm_contact'); + }); + + it('a delegated (on-behalf-of) read still denies outright — unchanged by the added layer', async () => { + // [#2852] The D10 delegator intersection is not implemented on this path, + // so it fails closed BEFORE any layer is composed. Pinned here because the + // added layer must not become a reason to compute a scope for a context + // this method refuses to scope at all. + const h = await boot({ shareLevel: 'edit' }); + const delegated = { ...h.repContext(), onBehalfOf: { userId: OTHER } }; + expect(await h.analyticsVisibleContacts(delegated)).toEqual([]); + }); +}); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 57b42b305d..48be2e6327 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2398,6 +2398,33 @@ export class SecurityPlugin implements Plugin { } } + /** + * The read scope for `object` under `context` — the filter the analytics / + * raw-SQL path ANDs into its query, being the one surface that bypasses the + * engine and so has no other source of scope. + * + * Its contract is agreement: this must be **the same filter the engine + * middleware ANDs into every find**. That chain injects THREE things, and + * this method composes the same three: + * + * 1. {@link computeRlsFilter} — tenant Layer 0 + RLS policies; + * 2. {@link computeControlledByParentFilter} — ADR-0055, `masterFK IN + * (accessible master ids)`; + * 3. {@link resolveSharingReadFilter} — plugin-sharing's OWD / record-share + * visibility filter, contributed by the sibling middleware. + * + * Each layer has been missing here at some point, and each absence had the + * same shape: a caller who cannot read a row through `/data` could still + * `COUNT(*)` / `GROUP BY` it through `/analytics`. Layer 3 was #4467; layer 2 + * was #5815 — for a `controlled_by_parent` object, halves 1 and 3 BOTH + * commonly return `null` by design (such an object carries no authored RLS, + * and maps to `public` in plugin-sharing's `effectiveSharingModel`), so the + * composed scope was `undefined` — no predicate at all over what are usually + * line-item rows. + * + * Fails CLOSED on any resolution failure: a dropped predicate here is the + * leak, so an unresolvable layer denies (zero rows) rather than widening. + */ async getReadFilter( object: string, context?: any, @@ -2427,6 +2454,11 @@ export class SecurityPlugin implements Plugin { // already 401s without a token). Mirrors the middleware's early `return next()` // — which is the RLS middleware's early exit only, so the sharing predicate // resolved above still applies. + // [#5815] The controlled_by_parent derivation is deliberately NOT resolved + // ahead of this branch the way the sharing predicate is: it stands down + // without a `userId` (`computeControlledByParentFilter` returns null), and + // this branch is reached only when there is none — so the middleware adds + // nothing here either. Agreement holds by both sides standing down. if (positions.length === 0 && explicit.length === 0 && !context?.userId) { return sharingFilter ?? undefined; } @@ -2454,10 +2486,23 @@ export class SecurityPlugin implements Plugin { try { const permissionSets = await this.resolvePermissionSetsForContext(context); const filter = await this.computeRlsFilter(permissionSets, object, 'find', context); - // [#4467] RLS AND sharing — the same AND-composition the two middlewares - // achieve by both writing into `ast.where`. Either half may be absent; - // `andComposeLayers` returns the other, or null when neither constrains. - return andComposeLayers(filter, sharingFilter) ?? undefined; + // [#5815] ADR-0055 — the SECOND thing the middleware ANDs into `ast.where` + // for a read. Resolved from the sets already resolved above, exactly as + // the middleware feeds it its own, so the derived master id set is + // computed for this identity once and never re-resolved. It stands down + // (null) for any object that is not controlled_by_parent, and it is + // internally fail-closed; a THROW propagates to the catch below, which + // denies — the same posture as the surrounding layers. + const cbpFilter = await this.computeControlledByParentFilter( + permissionSets, + object, + context, + ); + // [#4467] RLS AND controlled-by-parent AND sharing — the same + // AND-composition the middlewares achieve by all writing into `ast.where`. + // Any layer may be absent; `andComposeLayers` returns the others, or null + // when none constrains. + return andComposeLayers(andComposeLayers(filter, cbpFilter), sharingFilter) ?? undefined; } catch (e) { // Fail CLOSED — a resolution failure must deny (zero rows), never expose // every tenant's data through the raw-SQL analytics path.