From cced4e5fd26d50d482bd10aa1615b7b86d37ec61 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 02:02:50 +0000 Subject: [PATCH] =?UTF-8?q?fix(plugin-security):=20#2936=20=E2=80=94=20RLS?= =?UTF-8?q?=20field-existence/tenancyDisabled=20nets=20recognize=20canonic?= =?UTF-8?q?al=20=3D=3D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extractTargetField only matched legacy `field =` / `IN`, returning null for canonical CEL `field ==` (how real seeds/business policies author equality). A null target means "keep the policy", so the Layer 1 field-existence net and the tenancy-disabled skip net in computeLayeredRlsFilter were inert for every `==` policy. Extend the regex to recognize `==` (ordered before `=` so the alternation does not mis-match the first `=` of `==`), alongside `=`/`IN`. Recognition is only extended; net semantics are unchanged and unmatched shapes (!=, >, <, >=, <=) still return null (conservative keep). Delta (fail-closed strengthening, same effective visibility): the wildcard owner_only_writes/deletes seeds (`created_by == current_user.id`) now fail closed on objects lacking a created_by column (platform-global/system tables) instead of compiling to a phantom {created_by:…} filter — same denied outcome, explicit deny sentinel now. Ordinary tenant/business objects carry created_by and are unaffected. Tenancy-disabled skip net affects no current seed (no `==` seed targets organization_id on a tenancy-disabled object). Tenant wall is Layer 0 (tenant-layer.ts), which never used this parser — unaffected. Tests: authz-matrix-gate two platform_global write cells updated to the deny sentinel with [#2936] annotations; added a `==` twin of the L692 field-existence fail-closed guard. plugin-security 392 green; dogfood authz-conformance + RLS matrices green. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_019QRUvVfpvSycAHMMF2xTxs --- .changeset/authz-2936-rls-eq-recognition.md | 34 +++++++++++++++++++ .../src/authz-matrix-gate.test.ts | 21 +++++++++--- .../src/security-plugin.test.ts | 28 +++++++++++++++ .../plugin-security/src/security-plugin.ts | 21 ++++++++---- 4 files changed, 93 insertions(+), 11 deletions(-) create mode 100644 .changeset/authz-2936-rls-eq-recognition.md diff --git a/.changeset/authz-2936-rls-eq-recognition.md b/.changeset/authz-2936-rls-eq-recognition.md new file mode 100644 index 0000000000..217407bb8c --- /dev/null +++ b/.changeset/authz-2936-rls-eq-recognition.md @@ -0,0 +1,34 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): #2936 — RLS field-existence / tenancy-disabled safety nets now recognize canonical `==` + +`extractTargetField` (the lightweight left-hand-field parser feeding the Layer 1 +**field-existence** net and the **tenancy-disabled** skip net in +`computeLayeredRlsFilter`) only matched the legacy single-`=` / `IN` shape. It +returned `null` for canonical CEL `==` (`field == …`), which is how real seeds +and business policies author equality. A `null` target field means "keep the +policy", so both safety nets were **inert** for every `==` policy: a wildcard +policy targeting a column the object lacks was NOT failed closed, and a +`==`-form `organization_id` policy on a `tenancy.enabled:false` object was NOT +skipped. The regex now recognizes `==` (listed before `=` so the ordered +alternation does not mis-match the first `=`), alongside the existing `=`/`IN`. +Recognition is only **extended** — the net semantics are unchanged, and +`!=`/`>`/`<`/`>=`/`<=` still return `null` (conservative keep), matching prior +behavior for any unmatched shape. + +Behavior delta (fail-closed strengthening, same effective visibility): the +wildcard `owner_only_writes` / `owner_only_deletes` seed policies +(`created_by == current_user.id`) now correctly fail closed on an object that +lacks a `created_by` column (platform-global / system tables). Previously they +slipped the net and compiled to a phantom `{ created_by: … }` filter against a +missing column — a driver-dependent, effectively-deny result; now the net drops +the sole applicable write policy and yields the deny sentinel. A member could not +by-id write such a column-less object either way, so the visible/writable row set +is unchanged; only the mechanism is now an explicit fail-closed deny. All ordinary +tenant/business objects carry `created_by`, so they are unaffected (proven green by +the dogfood authz-conformance + RLS matrices). The tenancy-disabled skip net has no +effect on any current seed (no `==` seed policy targets `organization_id` on a +tenancy-disabled object). The tenant wall itself is Layer 0 (`tenant-layer.ts`), +which never used this parser, so tenant isolation is unaffected (ADR-0095 D1). diff --git a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts index 9cde1b32c2..8433a9a6e8 100644 --- a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts +++ b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts @@ -203,11 +203,22 @@ const EXPECTED_MATRIX: Record { expect(filter).toEqual(RLS_DENY_FILTER); }); + it('[#2936] fail-closed on a CANONICAL `==` wildcard policy targeting a missing column → deny sentinel', async () => { + // The `=`-form twin above is the L692 test. This is its canonical-CEL + // sibling: pre-#2936 `extractTargetField` did not recognize `==`, so the + // field-existence net was INERT for it — the policy slipped through and + // compiled to a phantom `{ organization_id: … }` filter against a column + // the object lacks. The net now recognizes `==` and fails closed here, + // exactly as it always did for the legacy `=` form. Real seeds/business + // policies author equality as `==`, so this is the path that mattered. + const eqPolicySet: PermissionSet = { + name: 'member_default', + label: 'Member', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + rowLevelSecurity: [ + { name: 'tenant_isolation', object: '*', operation: 'all', using: 'organization_id == current_user.organization_id' }, + ], + } as any; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ + permissionSets: [eqPolicySet], + objectFields: ['id', 'name'], // no organization_id, tenancy not opted out + orgScoping: true, + }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + const filter = await plugin.getReadFilter('task', { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [] }); + expect(filter).toEqual(RLS_DENY_FILTER); + }); + it('tenancy opt-out → undefined (not denied), matching the find-path', 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 974e5ebfbb..03adc2fbee 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2487,12 +2487,21 @@ export class SecurityPlugin implements Plugin { */ private extractTargetField(using?: string): string | null { if (!using) return null; - // Match `field =` or `field IN`/`in`. Note: `\b` is omitted after `=` - // because `=` is non-word and the next char (space) is non-word too — - // a word boundary cannot exist between two non-word chars, so `=\b` - // would never match. We instead require the alternation token to be - // followed by whitespace or `(`. - const m = using.match(/^\s*([a-z_][a-z0-9_]*)\s*(?:=|IN|in)(?=\s|\()/); + // Match `field ==` (canonical CEL), `field =` (legacy SQL-ish), or + // `field IN`/`in`. [#2936] `==` MUST be listed before `=` in the + // alternation: alternation is ordered, so a bare `=` branch would match + // the first `=` of `==` and then the `(?=\s|\()` lookahead — seeing the + // SECOND `=`, not whitespace — would fail, leaving `==` unrecognized (the + // original bug: real seeds/business policies author equality as `==`, so + // the field-existence and tenancy-disabled safety nets that consume this + // were inert for them). `\b` is omitted after the operator because `=` is + // non-word and the next char (space) is non-word too — a word boundary + // cannot exist between two non-word chars — so we require the operator to + // be followed by whitespace or `(` instead. `!=`/`>`/`<`/`>=`/`<=` are + // deliberately NOT recognized: returning `null` keeps such a policy (the + // conservative default), matching the pre-#2936 behavior for any shape the + // regex did not match. + const m = using.match(/^\s*([a-z_][a-z0-9_]*)\s*(?:==|=|IN|in)(?=\s|\()/); return m ? m[1] : null; } }