Skip to content

Commit ca2b2f6

Browse files
os-zhuangclaude
andauthored
fix(plugin-security): #2936 — RLS field-existence/tenancyDisabled nets recognize canonical == (#2942)
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. Claude-Session: https://claude.ai/code/session_019QRUvVfpvSycAHMMF2xTxs Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2ae78c6 commit ca2b2f6

4 files changed

Lines changed: 93 additions & 11 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@objectstack/plugin-security": patch
3+
---
4+
5+
fix(plugin-security): #2936 — RLS field-existence / tenancy-disabled safety nets now recognize canonical `==`
6+
7+
`extractTargetField` (the lightweight left-hand-field parser feeding the Layer 1
8+
**field-existence** net and the **tenancy-disabled** skip net in
9+
`computeLayeredRlsFilter`) only matched the legacy single-`=` / `IN` shape. It
10+
returned `null` for canonical CEL `==` (`field == …`), which is how real seeds
11+
and business policies author equality. A `null` target field means "keep the
12+
policy", so both safety nets were **inert** for every `==` policy: a wildcard
13+
policy targeting a column the object lacks was NOT failed closed, and a
14+
`==`-form `organization_id` policy on a `tenancy.enabled:false` object was NOT
15+
skipped. The regex now recognizes `==` (listed before `=` so the ordered
16+
alternation does not mis-match the first `=`), alongside the existing `=`/`IN`.
17+
Recognition is only **extended** — the net semantics are unchanged, and
18+
`!=`/`>`/`<`/`>=`/`<=` still return `null` (conservative keep), matching prior
19+
behavior for any unmatched shape.
20+
21+
Behavior delta (fail-closed strengthening, same effective visibility): the
22+
wildcard `owner_only_writes` / `owner_only_deletes` seed policies
23+
(`created_by == current_user.id`) now correctly fail closed on an object that
24+
lacks a `created_by` column (platform-global / system tables). Previously they
25+
slipped the net and compiled to a phantom `{ created_by: … }` filter against a
26+
missing column — a driver-dependent, effectively-deny result; now the net drops
27+
the sole applicable write policy and yields the deny sentinel. A member could not
28+
by-id write such a column-less object either way, so the visible/writable row set
29+
is unchanged; only the mechanism is now an explicit fail-closed deny. All ordinary
30+
tenant/business objects carry `created_by`, so they are unaffected (proven green by
31+
the dogfood authz-conformance + RLS matrices). The tenancy-disabled skip net has no
32+
effect on any current seed (no `==` seed policy targets `organization_id` on a
33+
tenancy-disabled object). The tenant wall itself is Layer 0 (`tenant-layer.ts`),
34+
which never used this parser, so tenant isolation is unaffected (ADR-0095 D1).

packages/plugins/plugin-security/src/authz-matrix-gate.test.ts

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -203,11 +203,22 @@ const EXPECTED_MATRIX: Record<string, Record<string, { read: unknown; write: unk
203203
org_admin: { read: null, write: 'BYPASS(no-write-filter)' },
204204
// [D1 accepted delta (c)] A member reading a `tenancy.enabled:false` global
205205
// object: Layer 0 treats it as a NON-tenant object (no phantom org filter) →
206-
// the global catalog is VISIBLE (read: null). The write drops the dead
207-
// org-disjunct to plain owner scope (same visibility, no org column exists).
208-
member: { read: null, write: [{ created_by: 'u1' }] },
209-
// [D1 accepted delta (c)] no-org member likewise sees the global catalog (was deny-sentinel).
210-
no_org_member: { read: null, write: [{ created_by: 'u2' }] },
206+
// the global catalog is VISIBLE (read: null).
207+
// [#2936] WRITE now fail-closes to the DENY sentinel. `sys_package` has no
208+
// `created_by` column, and the wildcard `owner_only_writes` policy authors
209+
// its predicate as `created_by == current_user.id` (canonical `==`).
210+
// Pre-#2936 `extractTargetField` did not recognize `==`, so the
211+
// field-existence net let the policy through and it compiled to a phantom
212+
// `{ created_by: … }` filter against a column the object lacks — a
213+
// driver-dependent, effectively-deny result. Now the net recognizes `==`,
214+
// sees `created_by` is absent, and drops the only applicable write policy →
215+
// deny sentinel (fail-closed). SAME effective visibility (a member cannot
216+
// by-id write a column-less global object either way); the mechanism is now
217+
// an explicit fail-closed deny instead of a phantom-column filter.
218+
member: { read: null, write: [{ id: DENY }] },
219+
// [#2936] no-org member: same fail-closed deny on the write (see above); the
220+
// global catalog stays VISIBLE on read (Layer 0 non-tenant, delta (c)).
221+
no_org_member: { read: null, write: [{ id: DENY }] },
211222
},
212223
better_auth: {
213224
// [W2] better-auth-managed posture → superuser read bypass → null.

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -702,6 +702,34 @@ describe('SecurityPlugin', () => {
702702
expect(filter).toEqual(RLS_DENY_FILTER);
703703
});
704704

705+
it('[#2936] fail-closed on a CANONICAL `==` wildcard policy targeting a missing column → deny sentinel', async () => {
706+
// The `=`-form twin above is the L692 test. This is its canonical-CEL
707+
// sibling: pre-#2936 `extractTargetField` did not recognize `==`, so the
708+
// field-existence net was INERT for it — the policy slipped through and
709+
// compiled to a phantom `{ organization_id: … }` filter against a column
710+
// the object lacks. The net now recognizes `==` and fails closed here,
711+
// exactly as it always did for the legacy `=` form. Real seeds/business
712+
// policies author equality as `==`, so this is the path that mattered.
713+
const eqPolicySet: PermissionSet = {
714+
name: 'member_default',
715+
label: 'Member',
716+
objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } },
717+
rowLevelSecurity: [
718+
{ name: 'tenant_isolation', object: '*', operation: 'all', using: 'organization_id == current_user.organization_id' },
719+
],
720+
} as any;
721+
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
722+
const harness = makeMiddlewareCtx({
723+
permissionSets: [eqPolicySet],
724+
objectFields: ['id', 'name'], // no organization_id, tenancy not opted out
725+
orgScoping: true,
726+
});
727+
await plugin.init(harness.ctx);
728+
await plugin.start(harness.ctx);
729+
const filter = await plugin.getReadFilter('task', { userId: 'u1', tenantId: 'org-1', positions: [], permissions: [] });
730+
expect(filter).toEqual(RLS_DENY_FILTER);
731+
});
732+
705733
it('tenancy opt-out → undefined (not denied), matching the find-path', async () => {
706734
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
707735
const harness = makeMiddlewareCtx({

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

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2487,12 +2487,21 @@ export class SecurityPlugin implements Plugin {
24872487
*/
24882488
private extractTargetField(using?: string): string | null {
24892489
if (!using) return null;
2490-
// Match `field =` or `field IN`/`in`. Note: `\b` is omitted after `=`
2491-
// because `=` is non-word and the next char (space) is non-word too —
2492-
// a word boundary cannot exist between two non-word chars, so `=\b`
2493-
// would never match. We instead require the alternation token to be
2494-
// followed by whitespace or `(`.
2495-
const m = using.match(/^\s*([a-z_][a-z0-9_]*)\s*(?:=|IN|in)(?=\s|\()/);
2490+
// Match `field ==` (canonical CEL), `field =` (legacy SQL-ish), or
2491+
// `field IN`/`in`. [#2936] `==` MUST be listed before `=` in the
2492+
// alternation: alternation is ordered, so a bare `=` branch would match
2493+
// the first `=` of `==` and then the `(?=\s|\()` lookahead — seeing the
2494+
// SECOND `=`, not whitespace — would fail, leaving `==` unrecognized (the
2495+
// original bug: real seeds/business policies author equality as `==`, so
2496+
// the field-existence and tenancy-disabled safety nets that consume this
2497+
// were inert for them). `\b` is omitted after the operator because `=` is
2498+
// non-word and the next char (space) is non-word too — a word boundary
2499+
// cannot exist between two non-word chars — so we require the operator to
2500+
// be followed by whitespace or `(` instead. `!=`/`>`/`<`/`>=`/`<=` are
2501+
// deliberately NOT recognized: returning `null` keeps such a policy (the
2502+
// conservative default), matching the pre-#2936 behavior for any shape the
2503+
// regex did not match.
2504+
const m = using.match(/^\s*([a-z_][a-z0-9_]*)\s*(?:==|=|IN|in)(?=\s|\()/);
24962505
return m ? m[1] : null;
24972506
}
24982507
}

0 commit comments

Comments
 (0)