diff --git a/.changeset/predicate-guard-caller-scope-and-preimage-dedup.md b/.changeset/predicate-guard-caller-scope-and-preimage-dedup.md new file mode 100644 index 0000000000..c0f134e80f --- /dev/null +++ b/.changeset/predicate-guard-caller-scope-and-preimage-dedup.md @@ -0,0 +1,29 @@ +--- +'@objectstack/plugin-security': patch +--- + +fix(security): scope the bulk-write predicate guard to the caller's own filter, and dedupe pre-image reads (#3018 review follow-ups) + +Two hardening follow-ups from the #3018 adversarial review. + +**Predicate guard is now middleware-order-independent for writes.** #2982 made bulk +`update`/`delete` carry an `opCtx.ast`, which brought them under the step-2.9 +anti-oracle predicate guard for the first time. That guard is documented to run +against the *caller's own* predicate — RLS / sharing filters legitimately reference +fields the caller cannot read (e.g. `owner_id`). But for a bulk write it inspected +`opCtx.ast.where`, which a sibling middleware (`plugin-sharing`) may have already had +an `owner_id` owner-match composed into — and the two middlewares' registration order +is not contractually guaranteed. On an object whose `owner_id` is FLS-hidden, that +could 403 a legitimate bulk write purely because the injected filter named the field. +The guard now inspects `opCtx.options.where` (the caller's untouched predicate) for +`update`/`delete`, so it can never mistake an injected owner/RLS filter for a caller +probe, independent of middleware order. Reads are unchanged (the read seed is the +caller's query verbatim and the guard runs before this middleware's own injection). + +**Pre-image reads deduplicated.** The by-id "read the target row" pattern was inlined +at ~5 gates with slightly divergent shapes; a single `readRowById` helper (fail-closed: +missing engine / null id / thrown read → `null`, which always denies) now backs the +provenance gates, and a memoized `getCallerPreImage` collapses the owner-anchor echo +check (3.5) and the RLS `check` post-image (3.6) — which read the identical +`(object, id, caller-context)` row — into one read per operation. No behavior change; +the read shape can no longer drift across sites. diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 597e324997..0cfa2bd282 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -1223,6 +1223,48 @@ describe('SecurityPlugin', () => { }); }); + it('[#2982 follow-up] bulk-write predicate guard inspects the CALLER predicate, not injected owner/RLS filters', async () => { + // The anti-oracle guard (2.9) must reject a caller filtering on an + // FLS-hidden field, but must NOT reject an owner_id predicate a sibling + // middleware (plugin-sharing) composed onto opCtx.ast — otherwise a bulk + // write on an object whose owner_id is FLS-hidden 403s purely because the + // injected filter mentions it, and the result depends on middleware order. + const ownerHiddenSet: PermissionSet = { + name: 'member_default', + label: 'Member', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + fields: { 'task.owner_id': { readable: false, editable: false }, 'task.ssn': { readable: false, editable: false } }, + } as any; + const boot = async () => { + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ permissionSets: [ownerHiddenSet], objectFields: ['id', 'owner_id', 'name', 'ssn', 'status'] }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + return harness; + }; + const ctx = { userId: 'u1', tenantId: 'org-1', positions: [], permissions: ['member_default'] }; + + // Injected owner_id in the AST, but the caller's own predicate is clean → allowed. + const injected: any = await boot(); + const okCtx: any = { + object: 'task', operation: 'update', data: { name: 'renamed' }, + options: { where: { status: 'open' }, multi: true }, + ast: { where: { $and: [{ status: 'open' }, { owner_id: 'u1' }] } }, // as plugin-sharing would compose + context: ctx, + }; + await injected.run(okCtx); // must NOT throw — owner_id is only in the injected filter + + // The caller's OWN predicate references a hidden field → still rejected. + const probing: any = await boot(); + const denyCtx: any = { + object: 'task', operation: 'update', data: { name: 'renamed' }, + options: { where: { ssn: 'guess' }, multi: true }, + ast: { where: { ssn: 'guess' } }, + context: ctx, + }; + await expect(probing.run(denyCtx)).rejects.toThrow(/filter oracle|not readable/); + }); + it('FLS write — the record echoed back by an update is masked (no read-leak of hidden fields)', async () => { // Regression: a caller with edit-but-not-field-read must not be able to // read a read-protected field back out of the mutation response. The write diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 75244b8d04..b91378ff16 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -1195,7 +1195,7 @@ export class SecurityPlugin implements Plugin { // closes the owner-enumeration oracle a system read would // open (a caller who can't read the row gets null → deny, // indistinguishable from a non-owner). - const pre: any = await this.ql.findOne(opCtx.object, { where: { id: targetId }, context: opCtx.context }); + const pre = await this.getCallerPreImage(opCtx, targetId); unchanged = !!pre && pre.owner_id != null && String(pre.owner_id) === String(row.owner_id); } catch { unchanged = false; // fail closed @@ -1257,13 +1257,10 @@ export class SecurityPlugin implements Plugin { ); postImage = null; } else if (this.ql) { - let pre: any = null; - try { - pre = await this.ql.findOne(opCtx.object, { where: { id: targetId }, context: opCtx.context }); - } catch { - pre = null; - } - if (pre && typeof pre === 'object') postImage = { ...(pre as Record), ...(opCtx.data as Record) }; + // Shares the memoized caller pre-image with the step-3.5 owner + // echo check — the identical (object, id, caller-context) row. + const pre = await this.getCallerPreImage(opCtx, targetId); + if (pre) postImage = { ...pre, ...(opCtx.data as Record) }; } } if (postImage && !checkParts.every((f) => matchesFilterCondition(postImage as any, f as any))) { @@ -1366,9 +1363,10 @@ export class SecurityPlugin implements Plugin { // sorting / grouping on it (row presence is the oracle; the objectui // /data surface makes URL-driven predicates first-class). Reject such // queries outright — silent predicate dropping would change query - // semantics unpredictably. MUST run against the CALLER's AST, before - // the RLS injection below: RLS policies legitimately reference fields - // the caller cannot read (e.g. owner_id). + // semantics unpredictably. MUST run against the CALLER's OWN predicate, + // before the RLS injection below: RLS / sharing filters legitimately + // reference fields the caller cannot read (e.g. owner_id) and must not be + // rejected. if (opCtx.ast) { let guardPerms = this.permissionEvaluator.getFieldPermissions(opCtx.object, permissionSets); guardPerms = this.foldFieldRequiredPermissions(guardPerms, secMeta.fieldRequiredPermissions, permissionSets); @@ -1380,7 +1378,22 @@ export class SecurityPlugin implements Plugin { guardPerms = intersectFieldMasks(guardPerms, delGuard); } if (Object.keys(guardPerms).length > 0) { - assertReadableQueryFields(opCtx.ast as unknown as Record, guardPerms, opCtx.object); + // [#2982 follow-up] For a bulk WRITE the caller's own predicate is + // `opCtx.options.where` (untouched); `opCtx.ast.where` may ALREADY + // carry an owner-match that plugin-sharing's write branch composed in + // — and plugin-sharing is a SIBLING middleware whose registration + // order relative to this one is not guaranteed. Guarding the injected + // AST would 403 a legitimate bulk write on an object whose owner_id is + // FLS-hidden, purely because a sibling filter mentioned it. Inspect + // the caller's own predicate so the guard is independent of middleware + // order and never mistakes an injected filter for a probe. Reads keep + // guarding the ast: the read seed is the caller's query verbatim and + // this middleware runs before its own RLS injection. + const guardTarget: Record = + opCtx.operation === 'update' || opCtx.operation === 'delete' + ? { where: opCtx.options?.where } + : (opCtx.ast as unknown as Record); + assertReadableQueryFields(guardTarget, guardPerms, opCtx.object); } } @@ -2156,9 +2169,7 @@ export class SecurityPlugin implements Plugin { return; } - const existing = await this.ql - .findOne('sys_permission_set', { where: { id: targetId }, context: { isSystem: true } }) - .catch(() => null); + const existing = await this.readRowById('sys_permission_set', targetId, { isSystem: true }); if (existing && (existing as Record).managed_by === 'package') { const row = existing as Record; throw new PermissionDeniedError( @@ -2264,9 +2275,7 @@ export class SecurityPlugin implements Plugin { return; } - const existing = await this.ql - .findOne(opCtx.object, { where: { id: targetId }, context: { isSystem: true } }) - .catch(() => null); + const existing = await this.readRowById(opCtx.object, targetId, { isSystem: true }); const existingManagedBy = existing ? String((existing as Record).managed_by ?? '') : ''; @@ -2297,6 +2306,40 @@ export class SecurityPlugin implements Plugin { return null; } + /** + * By-id row read with the standard FAIL-CLOSED contract (missing engine, + * null id, or a thrown read → `null`), shared by every provenance / pre-image + * gate. Centralising the read SHAPE keeps a future change (soft-delete filter, + * field projection) from drifting across the ~5 call sites that used to + * inline it (#3018 review — Reuse). A `null` return always DENIES downstream; + * it never bypasses a gate. + */ + private async readRowById(object: string, id: unknown, context: any): Promise | null> { + if (id == null || typeof this.ql?.findOne !== 'function') return null; + try { + const row = await this.ql.findOne(object, { where: { id }, context }); + return row && typeof row === 'object' ? (row as Record) : null; + } catch { + return null; + } + } + + /** + * The single-id write PRE-IMAGE read under the CALLER's context, memoized per + * operation. The owner-anchor echo check (step 3.5) and the RLS `check` + * post-image (step 3.6) read the IDENTICAL `(object, id, caller-context)` row; + * this collapses the two reads into one. Safe: no write to the row happens + * between the gates (the driver write runs after the whole middleware pass), + * so the pre-image is stable within the operation. + */ + private async getCallerPreImage(opCtx: any, id: unknown): Promise | null> { + if (id == null) return null; + if (opCtx.__preImage && opCtx.__preImage.id === id) return opCtx.__preImage.row; + const row = await this.readRowById(opCtx.object, id, opCtx.context); + opCtx.__preImage = { id, row }; + return row; + } + /** * [ADR-0095 D1] Compute the effective row filter for (object, operation) as * `Layer0(tenant) AND Layer1(business RLS)`. @@ -2597,14 +2640,9 @@ export class SecurityPlugin implements Plugin { } else { const targetId = this.extractSingleId(opCtx); if (targetId == null) return; // bulk write — scoped by the read filter on the AST - let row: any = null; - try { - row = await this.ql.findOne(object, { where: { id: targetId }, context: { isSystem: true } }); - } catch { - row = null; - } + const row = await this.readRowById(object, targetId, { isSystem: true }); if (!row) deny('target record not found', targetId); - masterId = row[rel!.fk]; + masterId = row![rel!.fk]; } if (masterId == null) deny('detail record has no master reference');