From 902d566690fac810a5678e43c20a6bfb2320770d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 06:19:59 +0000 Subject: [PATCH] =?UTF-8?q?fix(security):=20`controlled=5Fby=5Fparent`=20?= =?UTF-8?q?=E6=8A=98=E5=85=A5=E4=B8=BB=E6=A1=A3=E7=9A=84=E5=BD=92=E5=B1=9E?= =?UTF-8?q?=E4=B8=8E=E5=85=B1=E4=BA=AB=E6=8E=88=E6=9D=83=20(#5386)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 主档可达性此前只走 `computeRlsFilter`(租户 Layer 0 + RLS 策略),归属与 `sys_record_share` 授权由 plugin-sharing 贡献,而它对非 `private` 有效共享模型 返回 null —— `controlled_by_parent` 恰好映射为 public,两半从未相遇。主档没写 RLS 的应用因此拿到不受限的主档 id 集,声明的收窄什么也没收窄;写这半更甚,主档 写 RLS 为空时整段行检查被跳过。 读:`computeControlledByParentFilter` 把主档读 RLS 与 `resolveSharingReadFilter` (`getReadFilter` 已在用的 OWD/共享半边)AND 起来再解析主档 id 集。 写:`assertControlledByParentWrite` 无条件追问 plugin-sharing 的单记录写闸 `canEdit`。两侧解析失败一律 fail closed。 v1 单层语义不变,未装 plugin-sharing 的部署行为不变。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- .../controlled-by-parent-master-sharing.md | 41 ++ .../src/controlled-by-parent-sharing.test.ts | 355 ++++++++++++++++++ .../plugin-security/src/security-plugin.ts | 147 +++++++- 3 files changed, 529 insertions(+), 14 deletions(-) create mode 100644 .changeset/controlled-by-parent-master-sharing.md create mode 100644 packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts diff --git a/.changeset/controlled-by-parent-master-sharing.md b/.changeset/controlled-by-parent-master-sharing.md new file mode 100644 index 0000000000..756fed92d5 --- /dev/null +++ b/.changeset/controlled-by-parent-master-sharing.md @@ -0,0 +1,41 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(security): `controlled_by_parent` 现在真的跟随主档访问 —— 折入主档的归属与共享授权 (#5386) + +**这是一次安全收紧。** 升级后,此前被越权看到 / 写到的明细行会读不到、写不了 —— 那正是 +声明本来就要求的边界。 + +ADR-0055 的 `controlled_by_parent` 对作者的承诺是「子记录跟随父记录的访问」。实现只兑现了 +一半:派生用的主档 id 集来自 `computeRlsFilter(master, 'find')`,即只有 Layer 0(租户)与 +Layer 1(`rowLevelSecurity` 策略)。归属(owner scope)与 `sys_record_share` 授权由**另一个 +插件** `plugin-sharing` 的 `buildReadFilter` 贡献,而它对「有效共享模型不是 `private`」的对象 +返回 `null` —— `controlled_by_parent` 在那边恰好映射为 `public`。于是记录级访问的两半在派生 +对象上从未相遇。 + +后果比文档里那句「sharing grants 未折入」读起来严重得多: + +- 主档上**没有写任何 `rowLevelSecurity`** 的应用,得到的是一个**不受限的主档 id 集**,派生 + 过滤器等于什么都没收窄 —— 只要持有对象级 read,全部明细行可读。行项目类对象(报价行、 + 发票行)是这个形状的常客,而它们携带逐行定价与折扣。 +- 在主档上补写 RLS 也不是绕法:RLS 与 sharing 过滤器是 **AND**,补写会连同被共享进来的行 + 一起切掉。 +- 写这半有同样的洞,而且是从另一侧来的:`assertControlledByParentWrite` 只在主档的写 RLS + 编译出非空过滤器时才检查主档行,主档没写 RLS 时**整段跳过** —— 持有 `allowEdit` 的调用者 + 可以改自己根本看不到的父记录下的明细。 + +**修复**:主档可达性改走与「直接读 / 直接写主档」完全相同的路径,复用既有合成点,不在 +plugin-security 里重刻一份 sharing 语义。 + +- 读:`computeControlledByParentFilter` 现在把主档的读 RLS 与 `resolveSharingReadFilter` + (`getReadFilter` 已经在用的那个 OWD/共享半边)AND 起来再解析主档 id 集。哪一半生效由 + **主档自己的有效共享模型**决定,因此派生出的可见集与直接 find 主档逐点一致。 +- 写:`assertControlledByParentWrite` 在原有的 CRUD `update` + 写 RLS 之外,**无条件**追问 + plugin-sharing 的单记录写闸 `canEdit`(归属按写深度放宽、`edit` 级共享、 + `modifyAllRecords` 旁路)—— 无条件,正因为写 RLS 那一半在常见情形下会被整段跳过。 +- 两侧解析失败一律**fail closed**(主档 id 集为空 / 拒绝写),而不是悄悄放宽回全员可见。 + +未变更的部分:v1 的**单层**语义 —— 主档自身的 `controlled_by_parent` 仍不递归下钻;没有装 +`plugin-sharing` 的部署行为不变(那种部署里主档本身也没有归属与共享可言,派生集依旧与直接 +读主档相等);`read` 级共享仍然只开读不开写,与直接访问主档的逐动词答案一致。 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 new file mode 100644 index 0000000000..a596074a11 --- /dev/null +++ b/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts @@ -0,0 +1,355 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5386] `controlled_by_parent` (ADR-0055) must resolve the MASTER's +// accessibility through the SAME path a direct read / write of that master +// takes — owner scope and `sys_record_share` grants folded in, not the master's +// RLS policies alone. +// +// ## What was measured before this suite existed +// +// The derivation ran `computeRlsFilter(master, 'find')` and nothing else, under +// a system context. Owner scope and record shares are contributed by a SIBLING +// plugin (`plugin-sharing`), whose `buildReadFilter` returns `null` for any +// object whose effective sharing model is not `private` — and +// `controlled_by_parent` maps to `public` there. So the two halves of +// record-level access never met on a derived object: +// +// • an app that authors NO `rowLevelSecurity` on the master got an +// UNRESTRICTED master id set, i.e. the declared narrowing narrowed nothing +// and every detail row was readable by any holder of object-level read; +// • authoring RLS on the master was not a workaround, because RLS is ANDed +// with the sharing filter rather than OR-ed into it, so it also cut off the +// rows a grant had shared in; +// • the write half had the same hole from the other side — its master row +// check was SKIPPED WHOLE when the master's write RLS compiled to `null`. +// +// ## Why one fixture drives both faces +// +// The read filter and the write assertion are two implementation faces of ONE +// contract ("a child follows its parent's access"). A suite that exercised them +// on separate fixtures could not see them disagree, which is the failure this +// family produces: a third, quieter answer where one face allows what the other +// refuses. Every case below runs on the SAME three-account fixture — a master +// owned by someone else and shared to the caller, a master owned by someone +// else and NOT shared (the excluded row, without which "consistent" would prove +// nothing), and a master the caller owns. + +import { describe, it, expect, vi } from 'vitest'; +import { SecurityPlugin } from './security-plugin.js'; +import { SharingService, type SharingEngine } from '@objectstack/plugin-sharing'; +import { matchesFilterCondition } from '@objectstack/formula'; +import type { PermissionSet } from '@objectstack/spec/security'; + +const REP = 'usr_rep'; +const OTHER = 'usr_other'; + +/** The MASTER — owner-scoped by OWD (`private`), with no authored RLS at all. */ +const ACCOUNT_SCHEMA = { + name: 'crm_account', + sharingModel: 'private', + fields: { + id: { name: 'id', type: 'text' }, + name: { name: 'name', type: 'text' }, + owner_id: { name: 'owner_id', type: 'lookup', reference: 'sys_user' }, + }, +}; + +/** The DETAIL — access derived from `crm_account` through the master_detail FK. */ +const CONTACT_SCHEMA = { + name: 'crm_contact', + sharingModel: 'controlled_by_parent', + fields: { + id: { name: 'id', type: 'text' }, + name: { name: 'name', type: 'text' }, + account: { name: 'account', type: 'master_detail', required: true, reference: 'crm_account' }, + }, +}; + +const SHARE_SCHEMA = { + name: 'sys_record_share', + isSystem: true, + fields: { + id: { name: 'id', type: 'text' }, + object_name: { name: 'object_name', type: 'text' }, + record_id: { name: 'record_id', type: 'text' }, + recipient_type: { name: 'recipient_type', type: 'text' }, + recipient_id: { name: 'recipient_id', type: 'text' }, + access_level: { name: 'access_level', type: 'text' }, + owner_id: { name: 'owner_id', type: 'lookup', reference: 'sys_user' }, + }, +}; + +/** + * The app's permission set: full CRUD on both objects (so requests reach the + * record layer instead of being refused by RBAC) and — deliberately — NO + * `rowLevelSecurity` anywhere. That is the shape the issue measured: the app + * expresses its record boundary entirely through OWD + sharing, and expects + * `controlled_by_parent` to follow it. + */ +const REP_SET: PermissionSet = { + name: 'crm_rep', + label: 'CRM Rep', + objects: { + crm_account: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + crm_contact: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, +} as unknown as PermissionSet; + +type Row = Record; + +/** + * In-memory store standing in for the engine's READ surface only — `find`, + * `findOne`, `getSchema`. It declares NO write verb: nothing under test writes + * through it, and a double that does not have a verb cannot be looser than the + * engine on that verb (the `check:engine-double-contract` family, #4434/#5480). + * Filtering runs through `matchesFilterCondition`, the same evaluator the + * security plugin itself uses, so a filter this suite asserts on is a filter + * that was really applied rather than one merely inspected. + */ +function makeStore(rows: Record) { + const schemas: Record = { + crm_account: ACCOUNT_SCHEMA, + crm_contact: CONTACT_SCHEMA, + sys_record_share: SHARE_SCHEMA, + }; + return { + rows, + getSchema: (object: string) => schemas[object], + find: vi.fn(async (object: string, options: any = {}) => { + const all = rows[object] ?? []; + const hits = all.filter((r) => matchesFilterCondition(r, options?.where ?? null)); + return typeof options?.limit === 'number' ? hits.slice(0, options.limit) : hits; + }), + findOne: vi.fn(async (object: string, options: any = {}) => { + const all = rows[object] ?? []; + return all.find((r) => matchesFilterCondition(r, options?.where ?? null)) ?? null; + }), + }; +} + +/** The fixture rows — identical for every case; only the grant level varies. */ +function fixtureRows(shareLevel: 'read' | 'edit' | null): Record { + return { + crm_account: [ + { id: 'acct_us', name: 'US Corp', owner_id: OTHER }, // shared to the rep + { id: 'acct_eu', name: 'EU Corp', owner_id: OTHER }, // NOT shared — the excluded row + { id: 'acct_own', name: 'Own Corp', owner_id: REP }, // the rep's own + ], + crm_contact: [ + { id: 'ct_us', name: 'US contact', account: 'acct_us' }, + { id: 'ct_eu', name: 'EU contact', account: 'acct_eu' }, + { id: 'ct_own', name: 'Own contact', account: 'acct_own' }, + ], + sys_record_share: shareLevel + ? [ + { + id: 'shr_1', + object_name: 'crm_account', + record_id: 'acct_us', + recipient_type: 'user', + recipient_id: REP, + access_level: shareLevel, + }, + ] + : [], + }; +} + +interface BootOptions { + /** The single grant's level, or `null` for a fixture with no grant at all. */ + shareLevel?: 'read' | 'edit' | null; + /** `'none'` boots a deployment WITHOUT plugin-sharing; `'throws'` a broken one. */ + sharing?: 'real' | 'none' | 'throws'; +} + +async function boot(options: BootOptions = {}) { + const shareLevel = options.shareLevel === undefined ? 'edit' : options.shareLevel; + const store = makeStore(fixtureRows(shareLevel)); + + let middleware: any; + const ql = { + registerMiddleware: (mw: any) => { + if (!middleware) middleware = mw; + }, + getSchema: store.getSchema, + find: store.find, + findOne: store.findOne, + }; + + const services: Record = { + manifest: { register: vi.fn() }, + objectql: ql, + metadata: { get: async (n: string) => store.getSchema(n), list: async () => [REP_SET] }, + }; + if ((options.sharing ?? 'real') === 'real') { + // The REAL sharing service over the same store — the point of the fix is + // that the derivation reuses this exact producer instead of re-deriving + // owner/share semantics inside plugin-security. + services.sharing = new SharingService({ engine: store as unknown as SharingEngine }); + } else if (options.sharing === 'throws') { + services.sharing = { + buildReadFilter: async () => { + throw new Error('sharing store unreachable'); + }, + canEdit: async () => { + throw new Error('sharing store unreachable'); + }, + }; + } + + const ctx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ + defaultPermissionSets: [REP_SET], + fallbackPermissionSet: 'crm_rep', + }); + await plugin.init(ctx); + await plugin.start(ctx); + + const repContext = () => ({ userId: REP, tenantId: 'org-1', positions: [], permissions: [] }); + + /** The READ face: run the middleware, then apply the filter it injected. */ + const visibleContacts = async (): Promise => { + const opCtx: any = { + object: 'crm_contact', + operation: 'find', + ast: {}, + options: {}, + context: repContext(), + }; + await middleware(opCtx, async () => {}); + return (store.rows.crm_contact ?? []) + .filter((r) => matchesFilterCondition(r, opCtx.ast.where ?? null)) + .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 = { + object: 'crm_contact', + operation: 'update', + data: { id, name: 'renamed' }, + options: { where: { id } }, + context: repContext(), + }; + await middleware(opCtx, async () => {}); + }; + + const writableContacts = async (): Promise => { + const out: string[] = []; + for (const row of store.rows.crm_contact ?? []) { + try { + await updateContact(String(row.id)); + out.push(String(row.id)); + } catch { + /* denied */ + } + } + return out; + }; + + return { store, ctx, visibleContacts, updateContact, writableContacts }; +} + +describe('[#5386] controlled_by_parent folds the master\'s ownership and share grants in', () => { + // ── READ face ──────────────────────────────────────────────────────────── + it('READ: only children of masters the caller can actually read are visible', async () => { + const h = await boot({ shareLevel: 'edit' }); + // acct_own is reachable by OWNERSHIP, acct_us by the single GRANT, + // acct_eu by neither — so its contact must not appear. + expect(await h.visibleContacts()).toEqual(['ct_us', 'ct_own']); + }); + + it('READ: with no grant at all, only the caller-owned master\'s children survive', async () => { + const h = await boot({ shareLevel: null }); + expect(await h.visibleContacts()).toEqual(['ct_own']); + }); + + it('READ: the derived master id set equals what a direct find of the master returns', async () => { + const h = await boot({ shareLevel: 'edit' }); + // The master half, resolved exactly as a direct read would: owner-match + // OR-ed with the caller's grants. This is the set the child filter must + // quantify over — asserted here so a future change that widens the derived + // set without widening the master's own read is caught at the seam. + const sharing = new SharingService({ engine: h.store as unknown as SharingEngine }); + const masterFilter = await sharing.buildReadFilter('crm_account', { + userId: REP, + tenantId: 'org-1', + } as any); + const directlyReadable = (h.store.rows.crm_account ?? []) + .filter((r) => matchesFilterCondition(r, masterFilter as any)) + .map((r) => String(r.id)); + expect(directlyReadable).toEqual(['acct_us', 'acct_own']); + }); + + // ── WRITE face ─────────────────────────────────────────────────────────── + it('WRITE: a by-id update of a child under an unreachable master is denied', async () => { + const h = await boot({ shareLevel: 'edit' }); + await expect(h.updateContact('ct_eu')).rejects.toThrow(/requires edit access to its master/); + }); + + it('WRITE: children of an owned master and of an edit-shared master stay writable', async () => { + const h = await boot({ shareLevel: 'edit' }); + await expect(h.updateContact('ct_own')).resolves.toBeUndefined(); + await expect(h.updateContact('ct_us')).resolves.toBeUndefined(); + }); + + // ── the invariant the two faces owe each other ─────────────────────────── + it('INVARIANT: read-visible and by-id-writable agree on the same fixture', async () => { + const h = await boot({ shareLevel: 'edit' }); + const readable = await h.visibleContacts(); + const writable = await h.writableContacts(); + expect(readable.sort()).toEqual(writable.sort()); + // …and the agreement is not the empty agreement: exactly one row is + // excluded, so "consistent" carries information. + expect(readable).not.toContain('ct_eu'); + expect(readable).toHaveLength(2); + }); + + it('a READ-level grant opens the child for reading but NOT for writing — the master\'s own verb boundary', async () => { + const h = await boot({ shareLevel: 'read' }); + // Same asymmetry a DIRECT access of the master has: a `read` share widens + // rows for reads only; `canEdit` refuses it. The derived faces must track + // that per-verb answer rather than collapsing to one of them. + expect(await h.visibleContacts()).toEqual(['ct_us', 'ct_own']); + expect(await h.writableContacts()).toEqual(['ct_own']); + await expect(h.updateContact('ct_us')).rejects.toThrow(/record sharing/); + }); + + // ── boundary conditions ────────────────────────────────────────────────── + it('the fold reads the grant table, resolves the master ONCE, and never re-enters the detail', async () => { + const h = await boot({ shareLevel: 'edit' }); + h.store.find.mockClear(); + await h.visibleContacts(); + const reads = h.store.find.mock.calls.map((c: any[]) => String(c[0])); + // The grant table was consulted — direct evidence the sharing half ran and + // not merely that some filter came back. + expect(reads).toContain('sys_record_share'); + // The master id set is resolved by ONE system-context read. More than one + // would mean the derivation re-entered the middleware (v1 is single-level, + // ADR-0055 — the master's own controlled_by_parent is not walked). + expect(reads.filter((o) => o === 'crm_account')).toHaveLength(1); + expect(reads).not.toContain('crm_contact'); + }); + + it('a deployment WITHOUT plugin-sharing is unchanged — RLS-only derivation, nothing to fold', async () => { + // No sharing service means no owner scope and no grants anywhere in the + // deployment, so a direct find of the master returns every row too: the + // derived set stays point-for-point equal to it. + const h = await boot({ shareLevel: 'edit', sharing: 'none' }); + expect(await h.visibleContacts()).toEqual(['ct_us', 'ct_eu', 'ct_own']); + await expect(h.updateContact('ct_eu')).resolves.toBeUndefined(); + }); + + it('fail-closed: a sharing service that throws denies BOTH faces', async () => { + const h = await boot({ shareLevel: 'edit', sharing: 'throws' }); + expect(await h.visibleContacts()).toEqual([]); + await expect(h.updateContact('ct_us')).rejects.toThrow(/record sharing/); + }); +}); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 4eef31d1f2..57b42b305d 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2298,6 +2298,7 @@ export class SecurityPlugin implements Plugin { private async resolveSharingReadFilter( object: string, context: any, + resolvedSets?: PermissionSet[], ): Promise | null> { const sharing = this.resolveKernelService?.('sharing') as | { buildReadFilter?: (o: string, c: any) => Promise } @@ -2306,9 +2307,12 @@ export class SecurityPlugin implements Plugin { // Mirror the middleware's ADR-0057 D1 depth stash. `getEffectiveScope` // needs the resolved sets and the object's posture — the same two inputs // the middleware feeds it — so the owner-match widens identically here. + // [#5386] `resolvedSets` lets an in-middleware caller (the + // controlled_by_parent derivation) pass the sets it already resolved for + // THIS identity, instead of re-resolving them from the context. let readScope: string | undefined; try { - const permissionSets = await this.resolvePermissionSetsForContext(context); + const permissionSets = resolvedSets ?? (await this.resolvePermissionSetsForContext(context)); if (permissionSets.length > 0) { const meta = await this.getObjectSecurityMeta(object); readScope = this.permissionEvaluator.getEffectiveScope( @@ -2331,6 +2335,69 @@ export class SecurityPlugin implements Plugin { return (filter ?? null) as Record | null; } + /** + * [#5386] The OWD / record-sharing half of a SINGLE-RECORD write gate — the + * write analogue of {@link resolveSharingReadFilter}. + * + * plugin-sharing's own middleware gates a by-id `update` on `canEdit(object, + * id, ctx)` (ownership widened by write DEPTH, an `edit`-level + * `sys_record_share` grant, or the `modifyAllRecords` bypass). This resolves + * exactly that gate through the late-bound `sharing` service, so a derived + * check ("may this caller edit the MASTER?") answers with the same predicate a + * direct write of the master would face, instead of a hand-rolled copy that + * drifts. + * + * Returns `true` when the sharing layer imposes nothing — no plugin-sharing in + * the deployment, a bypass/public object, an object with no owner field. + * `canEdit` answers all of those itself. FAILS CLOSED (`false`) when the probe + * throws: a dropped record-share gate is the leak, not the denial. + */ + private async resolveSharingCanEdit( + object: string, + recordId: string, + context: any, + resolvedSets?: PermissionSet[], + ): Promise { + const sharing = this.resolveKernelService?.('sharing') as + | { canEdit?: (o: string, id: string, c: any) => Promise } + | undefined; + if (!sharing || typeof sharing.canEdit !== 'function') return true; + // ADR-0057 D1 depth stash, resolved for THIS object — the context may still + // carry the DETAIL's `__writeScope` from the middleware, and the master's + // own grant is what widens the master's owner-match. Always written (even as + // undefined) so the detail's value can never leak in through the spread. + let writeScope: string | undefined; + try { + const permissionSets = resolvedSets ?? (await this.resolvePermissionSetsForContext(context)); + if (permissionSets.length > 0) { + const meta = await this.getObjectSecurityMeta(object); + writeScope = this.permissionEvaluator.getEffectiveScope( + 'write', + object, + permissionSets, + { isPrivate: meta.isPrivate }, + ); + } + } catch { + // Depth is a WIDENING input: unresolved leaves the owner-match at its + // narrowest ('own'), the safe direction. The gate below still runs. + writeScope = undefined; + } + try { + return ( + (await sharing.canEdit(object, recordId, { ...context, __writeScope: writeScope })) === true + ); + } catch (e) { + this.logger.error?.( + `[security] controlled_by_parent write gate could not resolve the sharing (OWD) edit ` + + `check for '${object}' record '${recordId}' (user ${context?.userId ?? 'unknown'}) — ` + + `denying (fail-closed, #5386)`, + e instanceof Error ? e : new Error(String(e)), + ); + return false; + } + } + async getReadFilter( object: string, context?: any, @@ -3200,17 +3267,33 @@ export class SecurityPlugin implements Plugin { * * For an object whose `sharingModel` is `controlled_by_parent`, access is * derived from the master: return a filter `masterFK IN ()`. The id set is resolved by running the MASTER's own read RLS - * (reused via `computeRlsFilter`) under a system context — no middleware - * re-entry, so no recursion. An empty set yields `{ masterFK: { $in: [] } }`, - * which matches no rows (fail closed). A misconfigured object (no - * master_detail/lookup to derive from) denies all reads (defense-in-depth; - * spec validation should prevent authoring it). Returns null when the object is - * not controlled_by_parent. + * can read>)`. The id set is resolved under a system context — no middleware + * re-entry, so no recursion — against BOTH halves of the master's own read + * scope, the same two the engine ANDs into a direct `find` of the master: + * + * 1. the master's read RLS (`computeRlsFilter` — tenant Layer 0 + policies), and + * 2. plugin-sharing's OWD / record-share visibility filter + * (`resolveSharingReadFilter` — the owner-match widened by READ depth, + * OR-ed with the caller's `sys_record_share` grants). + * + * [#5386] Half 2 used to be missing, and its absence was not a narrow gap: an + * app that authors NO `rowLevelSecurity` on the master got an UNRESTRICTED + * master id set, so the declared narrowing restricted nothing at all — every + * detail row was readable by any holder of object-level read. Authoring RLS on + * the master was no workaround either, since RLS is ANDed with (not OR-ed + * into) the sharing filter and so cuts off the very rows a grant shared in. + * Which half applies is decided by the MASTER's own effective sharing model — + * `buildReadFilter` returns null for a non-`private` master — so the derived + * set stays point-for-point equal to what a direct find of the master returns. + * + * An empty set yields `{ masterFK: { $in: [] } }`, which matches no rows (fail + * closed), as does a failure to resolve the sharing half. A misconfigured + * object (no master_detail/lookup to derive from) denies all reads + * (defense-in-depth; spec validation should prevent authoring it). Returns null + * when the object is not controlled_by_parent. * * v1 scope (ADR-0055): single level — the master's OWN controlled_by_parent is - * not traversed transitively; master accessibility is the master's RLS filter - * (sharing-service grants on the master are not folded in). + * NOT traversed transitively. */ private async computeControlledByParentFilter( permissionSets: PermissionSet[], @@ -3225,7 +3308,24 @@ export class SecurityPlugin implements Plugin { const rel = this.resolveCbpRelation(object); if (!rel) return { ...RLS_DENY_FILTER }; - const masterFilter = await this.computeRlsFilter(permissionSets, rel.master, 'find', context); + const masterRlsFilter = await this.computeRlsFilter(permissionSets, rel.master, 'find', context); + // [#5386] The OWD / record-share half, resolved through the SAME helper + // `getReadFilter` uses, so the derived path and the direct path cannot + // drift. A resolution failure denies (empty master set) rather than + // silently widening the children back to everyone. + let masterSharingFilter: Record | null; + try { + masterSharingFilter = await this.resolveSharingReadFilter(rel.master, context, permissionSets); + } catch (e) { + this.logger.error?.( + `[security] controlled_by_parent derivation could not resolve the sharing (OWD) read ` + + `scope of master '${rel.master}' for '${object}' (user ${context?.userId ?? 'unknown'}) ` + + `— denying (fail-closed, #5386)`, + e instanceof Error ? e : new Error(String(e)), + ); + return { [rel.fk]: { $in: [] } }; + } + const masterFilter = andComposeLayers(masterRlsFilter, masterSharingFilter); let masterIds: string[] = []; try { const rows = await this.ql.find(rel.master, { @@ -3247,12 +3347,25 @@ export class SecurityPlugin implements Plugin { * * A by-id write (insert/update/delete) to a controlled_by_parent detail * requires EDIT access to its master: the caller must hold CRUD `update` on the - * master object AND the master row must be visible under the master's write RLS. + * master object AND the master row must be reachable under BOTH halves of the + * master's own record-level write gate — + * + * 1. the master's write RLS (`computeRlsFilter(master, 'update')`), and + * 2. plugin-sharing's per-record edit gate (`resolveSharingCanEdit` → + * `canEdit`: ownership widened by write DEPTH, an `edit`-level + * `sys_record_share` grant, or the `modifyAllRecords` bypass). + * * This is the write-side companion to the read derivation — the RLS read filter * never applies to a by-id write (the #1994 class), so without this a member * could mutate a detail under a master they cannot edit. Throws on denial; * no-op when the object is not controlled_by_parent. * + * [#5386] Half 2 used to be missing, and half 1 is CONDITIONAL: a master with + * no authored write RLS compiles to a null filter, which skipped the row check + * entirely — so any holder of object-level `update` on the master could write + * details under masters they could neither read nor edit. The sharing gate is + * therefore asked UNCONDITIONALLY, not only when half 1 produced a filter. + * * v1 scope: single-id writes. Bulk writes flow through the AST and are already * scoped by the controlled-by-parent READ filter (to readable masters). */ @@ -3292,8 +3405,8 @@ export class SecurityPlugin implements Plugin { } if (masterId == null) deny('detail record has no master reference'); - // Master edit access = CRUD update on the master AND master row visible under - // the master's write RLS. + // Master edit access = CRUD update on the master AND the master row reachable + // under BOTH halves of its own write gate (write RLS + record sharing). if (!this.permissionEvaluator.checkObjectPermission('update', rel!.master, permissionSets)) { deny(`no edit permission on master '${rel!.master}'`, masterId); } @@ -3310,6 +3423,12 @@ export class SecurityPlugin implements Plugin { } if (!visible) deny(`master '${rel!.master}' not editable by this user (row-level security)`, masterId); } + // [#5386] The OWD / record-share half — asked UNCONDITIONALLY, because the + // RLS half above is skipped whole when the master authors no write policy, + // which is exactly the common case this closes. + if (!(await this.resolveSharingCanEdit(rel!.master, String(masterId), context, permissionSets))) { + deny(`master '${rel!.master}' not editable by this user (record sharing)`, masterId); + } } /**