From 3a1e2a1a5d9409ff3e07fa2f6ce01151887807d6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 03:06:15 +0000 Subject: [PATCH 1/5] feat(lint): rls-predicate-unknown-field / -unknown-user-variable WIP: the reference half of the RLS predicate gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- .../validate-rls-predicate-enforceability.ts | 369 +++++++++++++++++- 1 file changed, 361 insertions(+), 8 deletions(-) diff --git a/packages/lint/src/validate-rls-predicate-enforceability.ts b/packages/lint/src/validate-rls-predicate-enforceability.ts index 70025bb36b..bd902dba52 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -152,13 +152,24 @@ */ import { + compileCelToFilter, isPushdownableCel, isSupportedRlsExpression, parseCelToAstWithReason, sqlPredicateToCel, } from '@objectstack/formula'; import type { CelBoundsOverrun } from '@objectstack/formula'; -import { recordsOf } from './object-graph.js'; +import { RESERVED_RLS_MEMBERSHIP_KEYS } from '@objectstack/spec/contracts'; +import { + describeFieldPathVerdict, + indexObjectGraph, + isUnjudgeable, + listNames, + recordsOf, + resolveFieldPath, + suggestName, + type ObjectGraph, +} from './object-graph.js'; /** A predicate outside the pushdown subset — the policy enforces nothing. */ export const RLS_PREDICATE_UNENFORCEABLE = 'rls-predicate-unenforceable'; @@ -166,6 +177,16 @@ export const RLS_PREDICATE_UNENFORCEABLE = 'rls-predicate-unenforceable'; export const RLS_PREDICATE_UNPARSEABLE = 'rls-predicate-unparseable'; /** Valid CEL that overruns a platform parse bound (`maxAstNodes`, `maxDepth`, …). */ export const RLS_PREDICATE_OVER_BUDGET = 'rls-predicate-over-budget'; +/** + * A predicate whose SHAPE is fine but which names a field the policy's object + * does not declare — the reference half of the same failure (#16119). + */ +export const RLS_PREDICATE_UNKNOWN_FIELD = 'rls-predicate-unknown-field'; +/** + * A predicate referencing a `current_user.*` value nothing pre-resolves — the + * variable half of the same failure (#16119). + */ +export const RLS_PREDICATE_UNKNOWN_USER_VARIABLE = 'rls-predicate-unknown-user-variable'; export type RlsPredicateSeverity = 'error' | 'warning'; @@ -242,6 +263,317 @@ function consequence(clause: 'using' | 'check'): string { '`PermissionDeniedError`. The policy reads as a write rule and behaves as a blanket refusal.'; } +/* ──────────────────────────────────────────────────────────────────────────── + * #16119 — the REFERENCE half. The three ids above judge a predicate's SHAPE + * (does it parse, does it lower, does it fit the bounds) and nothing judges + * what it POINTS AT, so `is_private_nope == false || owner_id == current_user.id` + * and `is_private == false || owner_id == current_user.nope` were both reported + * by NOTHING — measured at the same site, in the same run, that reported the two + * shape faults twice. + * + * Both miss directions fail CLOSED, which is why they survived: an unknown + * field is what a column RENAME leaves behind, and an unresolved `current_user.*` + * compiles to nothing — so the authored narrowing becomes a blanket refusal and + * every holder of the permission set loses the object, with lint and CI green. + * + * ## Why this is a SECOND pair of ids and not a widening of the three above + * + * Same disposition `security-fls-unknown-field` took beside + * `security-fls-unqualified-key`: those ids say *unenforceable* / *unparseable* + * and are correct inside that scope, the prescriptions differ (rewrite the + * predicate / fix the name / pre-resolve the variable), and an author who + * suppresses one must not thereby suppress the other. These run only where the + * shape check PASSED, so the guards are disjoint by construction — a predicate + * is judged by the shape ids or by these, never both. + */ + +/** + * The `current_user.*` keys the platform itself resolves, read from the + * contract that declares them rather than transcribed. + * + * {@link RESERVED_RLS_MEMBERSHIP_KEYS} (`@objectstack/spec/contracts`) is the + * list `IRlsMembershipResolver` is forbidden to supply *because the kernel + * already owns them* — "keys … must not collide with the named context fields + * (`id`, `organization_id`, `positions`, `org_user_ids`, `accessible_org_ids`, + * `email`) — the compiler never lets a membership key clobber those." That is + * the same set `RLSCompiler.compileFilter` builds its `RLSUserContext` from, + * and it is in `@objectstack/spec`, which this package may read (the RLS + * compiler itself is a runtime it may not). + * + * ⛔ Deliberately NOT hotcrm's five-name guard, and ⛔ not `RLSUserContextSchema` + * in `packages/spec/src/security/rls.zod.ts` — that schema still spells the org + * key `tenantId` and carries `department` / `attributes` the RLS compiler never + * binds, so reading it would judge authored policies against a shape the + * runtime does not have. + */ +const PRERESOLVED_USER_KEYS: ReadonlySet = new Set(RESERVED_RLS_MEMBERSHIP_KEYS); + +/** + * Probe values bound in place of the real request context. + * + * An ARRAY is the value that lowers in EVERY position the pushdown subset has: + * `lowerMembership` requires `Array.isArray` on the right of `in`, and + * `lowerComparison` accepts any value at all — so binding every known key to one + * array lets a well-formed predicate compile without a request. The SCALAR is + * the discriminator described on {@link userVariableIsScalarPositioned}. + */ +const PROBE_ARRAY: readonly string[] = ['__objectstack_lint_probe__']; +const PROBE_SCALAR = '__objectstack_lint_probe__'; + +/** `variable "current_user.nope" is undefined` → `current_user.nope`. */ +function unresolvedVariablePath(detail: string): string | null { + const m = /variable "([^"]+)"/.exec(detail); + return m ? m[1] : null; +} + +type UserProbe = Record; + +function baseUserProbe(): UserProbe { + const probe: UserProbe = {}; + for (const key of PRERESOLVED_USER_KEYS) probe[key] = PROBE_ARRAY; + return probe; +} + +function compileWithProbe(bridged: string, probe: UserProbe) { + return compileCelToFilter(bridged, { variables: { current_user: probe } }); +} + +/** + * Is this `current_user.` reference in a position only a SCALAR can fill? + * + * This is the whole reason the variable rule can exist without false-positiving + * the platform's own documented feature. §7.3.1 dynamic membership lets an app + * stage ARBITRARY keys into `ExecutionContext.rlsMembership` and reference them + * as `field in current_user.` — the existing `rls-predicate-unparseable` + * hint *recommends exactly that shape* — so a key this linter has never heard of + * is, in an `in` position, indistinguishable from a correct §7.3.1 reference and + * must NOT be reported (the object graph's `unknowable` discipline, one axis + * over). + * + * What makes the other positions decidable is that the merge is array-only: + * `compileFilter` stages a membership entry only `if (Array.isArray(value))`, + * and it never lets one clobber a named field. So the complete set of values + * `current_user.` can EVER hold at runtime is "some array" — and an + * array is the one thing a scalar position cannot use. A key compared with + * `==` / `!=` / `<` / `>` or handed to `startsWith` therefore resolves to + * nothing on every request there will ever be. + * + * The question is asked of the COMPILER, not of a model of it: bind the key to + * a scalar and re-run `compileCelToFilter`. If the predicate still lowers, the + * key sat in a scalar position; if the compiler refuses (`in` requires an + * array/list on the right), it sat in a membership position and is left alone. + * A key used in BOTH positions in one predicate takes the membership answer and + * is skipped — the conservative direction for a new rule. + */ +function userVariableIsScalarPositioned(bridged: string, probe: UserProbe, path: string): boolean { + const scalarProbe: UserProbe = { ...probe }; + setProbePath(scalarProbe, path, PROBE_SCALAR); + const res = compileWithProbe(bridged, scalarProbe); + if (res.ok) return true; + // Still unresolved, but about a DIFFERENT variable: this one resolved as a + // scalar before the compiler reached the next miss. + if (res.reason === 'unresolved-variable') return unresolvedVariablePath(res.detail) !== path; + return false; +} + +/** Bind `current_user.a.b` inside the probe, creating the intermediate records. */ +function setProbePath(probe: UserProbe, path: string, value: unknown): void { + const segments = path.split('.').slice(1); // drop the `current_user` root + if (segments.length === 0) return; + let cursor: UserProbe = probe; + for (let i = 0; i < segments.length - 1; i++) { + const next = cursor[segments[i]]; + const rec = next && typeof next === 'object' && !Array.isArray(next) ? (next as UserProbe) : {}; + cursor[segments[i]] = rec; + cursor = rec; + } + cursor[segments[segments.length - 1]] = value; +} + +/** Is this whole path exactly one pre-resolved key (`current_user.id`)? */ +function isPreresolvedPath(path: string): boolean { + const segments = path.split('.'); + return segments.length === 2 && segments[0] === 'current_user' && PRERESOLVED_USER_KEYS.has(segments[1]); +} + +/** + * Compile the predicate against a probe context, reporting each unresolvable + * `current_user.*` reference on the way, and return the lowered filter — whose + * KEYS are the field paths the runtime will push down (ADR-0055: every one a + * single column). + * + * The loop exists because `resolveValue` throws on the FIRST miss it reaches, so + * each discovered key is bound to a probe value before the next compile. Bounded + * rather than `while (true)`: a linter must terminate on input it did not + * anticipate, and stopping early only costs a finding. + */ +function resolveReferences( + bridged: string, +): { filter: Record | null; unresolvedScalars: string[] } { + const probe = baseUserProbe(); + const unresolvedScalars: string[] = []; + for (let pass = 0; pass < 32; pass++) { + const res = compileWithProbe(bridged, probe); + if (res.ok) return { filter: res.filter as Record, unresolvedScalars }; + if (res.reason !== 'unresolved-variable') { + // The predicate passed `isSupportedRlsExpression`, so a shape refusal here + // can only be a probe value the position cannot take (an array handed to + // `startsWith`). Nothing further is decidable; report what was found. + return { filter: null, unresolvedScalars }; + } + const path = unresolvedVariablePath(res.detail); + if (!path || !path.startsWith('current_user.') || isPreresolvedPath(path)) { + return { filter: null, unresolvedScalars }; + } + if (userVariableIsScalarPositioned(bridged, probe, path)) { + unresolvedScalars.push(path); + setProbePath(probe, path, PROBE_SCALAR); + } else { + // A membership position — an app-staged §7.3.1 key is indistinguishable + // from a typo here, so this is `unknowable`, never a finding. + setProbePath(probe, path, PROBE_ARRAY); + } + } + return { filter: null, unresolvedScalars }; +} + +/** Collect every `{ $field: '' }` reference nested anywhere under a value. */ +function collectFieldRefs(value: unknown, out: Set): void { + if (Array.isArray(value)) { + for (const item of value) collectFieldRefs(item, out); + return; + } + if (!value || typeof value !== 'object') return; + for (const [key, nested] of Object.entries(value as Record)) { + if (key === '$field' && typeof nested === 'string' && nested) out.add(nested); + else collectFieldRefs(nested, out); + } +} + +/** + * The field paths a lowered FilterCondition addresses. + * + * Read off the COMPILER'S OWN OUTPUT rather than re-walked from the source: + * every producer of a field key in `cel-to-filter.ts` (`emit`, `lowerMembership`, + * `lowerStringMethod`) writes the path as the condition's key, so this reads + * exactly the columns the driver will be handed. A second parse of the predicate + * here would be the fork this file's docblock refuses. + */ +function filterFieldPaths(filter: Record | null): Set { + const fields = new Set(); + const walk = (node: unknown): void => { + if (Array.isArray(node)) { + for (const item of node) walk(item); + return; + } + if (!node || typeof node !== 'object') return; + for (const [key, value] of Object.entries(node as Record)) { + if (key === '$and' || key === '$or' || key === '$not') { + walk(value); + continue; + } + if (key.startsWith('$')) continue; + fields.add(key); + collectFieldRefs(value, fields); + } + }; + walk(filter); + return fields; +} + +/** What a reference miss costs at request time, per clause. Measured, not inferred. */ +function referenceConsequence(clause: 'using' | 'check', kind: 'field' | 'variable'): string { + const dropped = + kind === 'field' + ? '`SecurityPlugin`\'s field-existence safety net DROPS the policy before the compiler sees it — it ' + + 'reads the predicate\'s LEADING `field ==` / `=` / `in` and refuses a column the object lacks — and ' + + 'a miss anywhere further along instead reaches the driver as a phantom column (`no such column`, or ' + + 'zero rows with no error at all). ' + : 'The pushdown compiler answers `unresolved-variable`, so `RLSCompiler` DROPS the policy at request ' + + 'time — one WARN line is the only signal, and nothing reports it at authoring time. '; + return clause === 'using' + ? dropped + + 'When it is the only applicable policy for that object and operation the layer falls back to the ' + + '`RLS_DENY_FILTER` sentinel, which is AND-ed onto the where clause: every select / update / delete ' + + 'matches ZERO rows, so the object DISAPPEARS for every holder of this permission set — not because ' + + 'they were denied, but because the narrowing they were granted resolves to nothing. When other ' + + 'policies also apply, this one vanishes from the OR and grants none of the access it appears to.' + : dropped + + 'On the ADR-0058 D4 write path the post-image `check` can then never be satisfied: every insert / ' + + 'update the policy governs fails with `PermissionDeniedError`. The policy reads as a write rule and ' + + 'behaves as a blanket refusal for every holder of this permission set.'; +} + +/** + * The reference pass: every finding a SHAPE-VALID predicate earns. + * + * Ordered fields-then-variables and deduplicated per name, so a predicate + * naming one missing column twice earns one finding rather than one per + * occurrence. + */ +function referenceFindings( + graph: ObjectGraph, + source: string, + clause: 'using' | 'check', + where: string, + path: string, + object: string, +): RlsPredicateFinding[] { + const findings: RlsPredicateFinding[] = []; + const bridged = sqlPredicateToCel(source); + const { filter, unresolvedScalars } = resolveReferences(bridged); + + for (const fieldPath of filterFieldPaths(filter)) { + const verdict = resolveFieldPath(graph, object, fieldPath); + if (isUnjudgeable(verdict) || !verdict) continue; + const account = describeFieldPathVerdict(verdict, fieldPath, `RLS ${clause} predicate field`); + if (!account) continue; + findings.push({ + severity: 'error', + rule: RLS_PREDICATE_UNKNOWN_FIELD, + where, + path, + message: + `RLS ${clause} \`${quote(source)}\` lowers correctly but does not name a real column: ` + + `${account.message} ` + referenceConsequence(clause, 'field'), + hint: + `${account.detail} Point the predicate at a column the object really declares, or delete the ` + + `policy if the narrowing is gone — a policy that can never match is not protection, it is an ` + + `outage. If the field was RENAMED, this policy has been denying since that rename; if it was ` + + `meant to live on another object, RLS cannot join to it (ADR-0055) — denormalise the value onto ` + + `"${object}" (a formula/rollup field) and test that column instead.`, + }); + } + + for (const variablePath of unresolvedScalars) { + const key = variablePath.slice('current_user.'.length); + findings.push({ + severity: 'error', + rule: RLS_PREDICATE_UNKNOWN_USER_VARIABLE, + where, + path, + message: + `RLS ${clause} \`${quote(source)}\` reads \`${variablePath}\`, which nothing pre-resolves. The ` + + `kernel-resolved \`current_user\` keys are exactly ${listNames(PRERESOLVED_USER_KEYS)}, and the ` + + `only other keys that can EVER appear are §7.3.1 membership sets, which the runtime stages as ` + + `ARRAYS and which are therefore usable only as \`field in current_user.\` — this reference ` + + `is in a scalar position, so no request can ever supply it. ` + + referenceConsequence(clause, 'variable'), + hint: + `Use one of the pre-resolved context values (${listNames(PRERESOLVED_USER_KEYS)})${suggestName( + key, + PRERESOLVED_USER_KEYS, + )} — \`current_user.organization_id\` is the tenant, \`current_user.id\` the acting user, ` + + `\`current_user.email\` their unique address. If "${key}" is meant to be an app-resolved set, it ` + + `must be staged into \`ExecutionContext.rlsMembership\` by an \`IRlsMembershipResolver\` that ` + + `DECLARES the key, and it can then only be tested with \`in\` (\` in ${variablePath}\`), ` + + `never compared with \`==\`: the runtime stages membership sets as arrays and never as scalars.`, + }); + } + + return findings; +} + /** * Gate every stack-declared RLS predicate on the ONE thing the runtime does * with it: lower it to a FilterCondition (ADR-0056 D4). @@ -253,6 +585,15 @@ export function validateRlsPredicateEnforceability(stack: unknown): RlsPredicate const findings: RlsPredicateFinding[] = []; const cfg = (stack ?? {}) as AnyRec; + // [#16119] The object graph for the reference pass, built ONCE: `indexObjectGraph` + // walks every object's whole field map, and a stack with N permission sets would + // otherwise pay for that walk N times to answer the same question. It is the + // SHARED index every field-existence rule in this package resolves through, so + // the three skips (an object this stack does not define, an object with no + // readable field map, registry-injected system columns) are the graph's and not + // re-derived here. + const graph: ObjectGraph = indexObjectGraph(cfg); + recordsOf(cfg.permissions).forEach((ps, psIndex) => { recordsOf(ps.rowLevelSecurity).forEach((policy, pIndex) => { for (const clause of ['using', 'check'] as const) { @@ -266,7 +607,23 @@ export function validateRlsPredicateEnforceability(stack: unknown): RlsPredicate // it: `RLSCompiler.compileFilter` calls the SAME `isSupportedRlsExpression` // to decide whether a dropped policy warrants its WARN. There is no // heuristic here to drift. - if (isSupportedRlsExpression(source)) continue; + const psNameEarly = str(ps.name) || String(psIndex); + const policyNameEarly = str(policy.name) || String(pIndex); + const objectEarly = str(policy.object); + const whereEarly = + `permission set "${psNameEarly}" policy "${policyNameEarly}"` + + (objectEarly ? ` on object "${objectEarly}"` : ''); + const pathEarly = `permissions[${psIndex}].rowLevelSecurity[${pIndex}].${clause}`; + + if (isSupportedRlsExpression(source)) { + // [#16119] The shape is fine, so the REFERENCE pass owns this predicate. + // Disjoint from everything below by construction: the three shape ids + // only ever run on predicates this branch has already returned from. + findings.push( + ...referenceFindings(graph, source, clause, whereEarly, pathEarly, objectEarly), + ); + continue; + } // ── The explanation. Re-derived only to tell the author WHICH fix they // need; the red/green boundary above never consults it. (Both agree by @@ -280,12 +637,8 @@ export function validateRlsPredicateEnforceability(stack: unknown): RlsPredicate // the two are separated here, and only here. const overrun = parseError ? boundsOverrunOf(bridged) : null; - const psName = str(ps.name) || String(psIndex); - const policyName = str(policy.name) || String(pIndex); - const object = str(policy.object); - const where = - `permission set "${psName}" policy "${policyName}"` + (object ? ` on object "${object}"` : ''); - const path = `permissions[${psIndex}].rowLevelSecurity[${pIndex}].${clause}`; + const where = whereEarly; + const path = pathEarly; if (overrun) { // `limit` is null only for a bounds fault this package cannot NAME From 65ec07e3e05bb5fbe6e6897a1114c315b6e9cc1c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 03:11:07 +0000 Subject: [PATCH 2/5] test(lint): pin the RLS predicate reference pass Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- packages/lint/src/index.ts | 6 + ...idate-rls-predicate-enforceability.test.ts | 293 ++++++++++++++++++ 2 files changed, 299 insertions(+) diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 8968a1e940..13589e4329 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -346,11 +346,17 @@ export type { // as an authorization and behaves as a blanket refusal. The verdict is // `isSupportedRlsExpression` — the runtime's own, hoisted into // `@objectstack/formula` in the same change so lint can reach it. +// [#16119] …and the REFERENCE half of the same gate: the three ids above judge a +// predicate's SHAPE and never what it POINTS AT, so a policy naming a renamed +// column or an un-pre-resolved `current_user.*` value was reported by nothing +// while failing closed on the whole object for every holder of the set. export { validateRlsPredicateEnforceability, RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNPARSEABLE, RLS_PREDICATE_OVER_BUDGET, + RLS_PREDICATE_UNKNOWN_FIELD, + RLS_PREDICATE_UNKNOWN_USER_VARIABLE, } from './validate-rls-predicate-enforceability.js'; export type { RlsPredicateFinding, diff --git a/packages/lint/src/validate-rls-predicate-enforceability.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.test.ts index fe27b311fb..67ba68dc7c 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -3,11 +3,15 @@ import { describe, it, expect, afterEach } from 'vitest'; import { isSupportedRlsExpression, setCelPushdownLimitsModeForTests } from '@objectstack/formula'; +import { RESERVED_RLS_MEMBERSHIP_KEYS } from '@objectstack/spec/contracts'; + import { validateRlsPredicateEnforceability, RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNPARSEABLE, RLS_PREDICATE_OVER_BUDGET, + RLS_PREDICATE_UNKNOWN_FIELD, + RLS_PREDICATE_UNKNOWN_USER_VARIABLE, } from './validate-rls-predicate-enforceability.js'; import { AUTHORING_RULES, runAuthoringRules } from './authoring-rules.js'; @@ -507,3 +511,292 @@ describe('validateRlsPredicateEnforceability — a bounds overrun is its own id .toEqual([RLS_PREDICATE_OVER_BUDGET]); }); }); + +// ── #16119: the REFERENCE half — a predicate that lowers but points at nothing + +/** + * The card's site, reproduced: hotcrm's `opportunity_private_owner_only` on + * `crm_opportunity`, whose shipped `using` is + * `is_private == false || owner_id == current_user.id`. + * + * The object is declared here because that is the whole point — the three shape + * ids never needed one, and these two cannot exist without it. + */ +const CRM_OPPORTUNITY = { + name: 'crm_opportunity', + label: 'Opportunity', + ownership: 'user', + fields: [ + { name: 'name', type: 'text' }, + { name: 'is_private', type: 'boolean' }, + { name: 'owner_id', type: 'user', reference: 'sys_user' }, + { name: 'assigned_to_id', type: 'user', reference: 'sys_user' }, + { name: 'amount', type: 'number' }, + { name: 'status', type: 'text' }, + { name: 'organization_id', type: 'text' }, + ], +}; + +/** The card's site with one clause swapped, over a stack that DECLARES the object. */ +const siteWith = (clause: 'using' | 'check', source: unknown, object = 'crm_opportunity') => ({ + objects: [CRM_OPPORTUNITY], + permissions: [ + { + name: 'sales_manager', + label: 'Sales Manager', + rowLevelSecurity: [ + { + name: 'opportunity_private_owner_only', + object, + operation: 'all', + ...(clause === 'using' ? {} : { using: 'true' }), + [clause]: source, + }, + ], + }, + ], +}); + +describe('validateRlsPredicateEnforceability — the four injections at ONE site (#16119)', () => { + /** + * The card's measurement, restated as a table so the two halves stay + * comparable. The two CONTROLS must keep firing under their EXISTING ids and + * must NOT acquire either new one — a rule that swallowed them would look + * like an improvement and would be a regression of #4983/#6778. + */ + it.each([ + ['CONTROL — not pushdownable', 'billing_address.country == "US"', [RLS_PREDICATE_UNENFORCEABLE]], + ['CONTROL — unparseable', 'is_private == = false', [RLS_PREDICATE_UNPARSEABLE]], + ['was SILENT — unknown field', 'is_private_nope == false || owner_id == current_user.id', [RLS_PREDICATE_UNKNOWN_FIELD]], + ['was SILENT — unknown user variable', 'is_private == false || owner_id == current_user.nope', [RLS_PREDICATE_UNKNOWN_USER_VARIABLE]], + ])('%s', (_label, source, expected) => { + expect(validateRlsPredicateEnforceability(siteWith('using', source)).map((f) => f.rule)).toEqual(expected); + }); + + it('leaves the shipped predicate at that site silent — the negative control', () => { + // Without this, an always-fires implementation satisfies the table above. + expect(ids(siteWith('using', 'is_private == false || owner_id == current_user.id'))).toEqual([]); + }); + + it('is disjoint from the three SHAPE ids by construction', () => { + // A shape fault never earns a reference id and vice versa: the reference + // pass only runs where `isSupportedRlsExpression` already said yes. + for (const source of ['size(record.tags) > 0', 'a = current_user.id AND b = 1', 'is_private == = false']) { + const rules = ids(siteWith('using', source)); + expect(rules).not.toContain(RLS_PREDICATE_UNKNOWN_FIELD); + expect(rules).not.toContain(RLS_PREDICATE_UNKNOWN_USER_VARIABLE); + expect(rules).toHaveLength(1); + } + }); + + /** + * The "before" half, mechanical rather than asserted in prose — the same + * construction #4983's own test uses. If some future rule grows to cover + * these shapes, this fails and someone decides which of the two owns it. + */ + it('NOTHING else in the whole rule table reports either miss', () => { + for (const source of [ + 'is_private_nope == false || owner_id == current_user.id', + 'is_private == false || owner_id == current_user.nope', + ]) { + const stack = siteWith('using', source); + const findings = runAuthoringRules('lint', { normalized: stack, parsed: stack }); + const mine = findings.filter( + (f) => f.rule === RLS_PREDICATE_UNKNOWN_FIELD || f.rule === RLS_PREDICATE_UNKNOWN_USER_VARIABLE, + ); + expect(mine).toHaveLength(1); + // Every other finding the table produces is about something else entirely + // (the object declares no `sharingModel`), and is identical for the + // SHIPPED predicate — so it is background, not a second report of this. + const others = findings.filter((f) => !f.rule.startsWith('rls-predicate')).map((f) => f.rule); + const shipped = siteWith('using', 'is_private == false || owner_id == current_user.id'); + const baseline = runAuthoringRules('lint', { normalized: shipped, parsed: shipped }).map((f) => f.rule); + expect(others).toEqual(baseline); + } + }); +}); + +describe('validateRlsPredicateEnforceability — the messages name the COST, not just the miss', () => { + it('says the object disappears for every holder of the set (unknown field)', () => { + const [f] = validateRlsPredicateEnforceability(siteWith('using', 'is_private_nope == false')); + expect(f.rule).toBe(RLS_PREDICATE_UNKNOWN_FIELD); + expect(f.where).toBe('permission set "sales_manager" policy "opportunity_private_owner_only" on object "crm_opportunity"'); + expect(f.path).toBe('permissions[0].rowLevelSecurity[0].using'); + expect(f.severity).toBe('error'); + expect(f.message).toMatch(/field-existence safety net DROPS the policy/); + expect(f.message).toMatch(/RLS_DENY_FILTER/); + expect(f.message).toMatch(/ZERO rows/); + expect(f.message).toMatch(/DISAPPEARS for every holder of this permission set/); + // …and the miss itself, with the platform's own "did you mean". + expect(f.message).toMatch(/"is_private_nope" is not a field on object "crm_opportunity"/); + expect(f.message).toMatch(/Did you mean "is_private"\?/); + // The hint lists the columns that DO exist and names the rename story. + expect(f.hint).toMatch(/Fields on "crm_opportunity": amount, assigned_to_id/); + expect(f.hint).toMatch(/RENAMED/); + }); + + it('says the same for an unresolved `current_user.*`, and prescribes the §7.3.1 route', () => { + const [f] = validateRlsPredicateEnforceability(siteWith('using', 'owner_id == current_user.nope')); + expect(f.rule).toBe(RLS_PREDICATE_UNKNOWN_USER_VARIABLE); + expect(f.message).toMatch(/reads `current_user\.nope`, which nothing pre-resolves/); + expect(f.message).toMatch(/scalar position, so no request can ever supply it/); + expect(f.message).toMatch(/DISAPPEARS for every holder of this permission set/); + expect(f.hint).toMatch(/IRlsMembershipResolver/); + expect(f.hint).toMatch(/never compared with `==`/); + }); + + it('names the WRITE consequence on a `check` clause, not the read one', () => { + const [f] = validateRlsPredicateEnforceability(siteWith('check', 'nope_field == 1')); + expect(f.rule).toBe(RLS_PREDICATE_UNKNOWN_FIELD); + expect(f.path).toBe('permissions[0].rowLevelSecurity[0].check'); + expect(f.message).toMatch(/PermissionDeniedError/); + expect(f.message).toMatch(/blanket refusal/); + expect(f.message).not.toMatch(/select \/ update \/ delete matches ZERO rows/); + }); +}); + +describe('validateRlsPredicateEnforceability — the `current_user` set is DERIVED, not transcribed', () => { + /** + * The known set is {@link RESERVED_RLS_MEMBERSHIP_KEYS} from + * `@objectstack/spec/contracts` — the contract that declares which + * `current_user.*` keys the kernel owns and an `IRlsMembershipResolver` may + * therefore never supply. Asserting the BEHAVIOUR against that import (rather + * than a literal list retyped here) is what makes this rule follow the + * platform: a key added to the contract stops being reported the same day, + * with no edit in this package. + * + * ⛔ The card's own five-name list is hotcrm's local guard, and + * `RLSUserContextSchema` in `packages/spec/src/security/rls.zod.ts` is a + * different, stale shape (`tenantId`, `department`, `attributes`) the RLS + * compiler never binds — neither is the authority. + */ + it('accepts every kernel-resolved key, in the position that key supports', () => { + expect(RESERVED_RLS_MEMBERSHIP_KEYS.length).toBeGreaterThan(0); + for (const key of RESERVED_RLS_MEMBERSHIP_KEYS) { + // A scalar comparison and a membership test between them cover both + // positions, so the assertion does not need to know which kind each key is. + const scalar = ids(siteWith('using', `owner_id == current_user.${key}`)); + const member = ids(siteWith('using', `owner_id in current_user.${key}`)); + expect({ key, silent: scalar.length === 0 || member.length === 0 }).toEqual({ key, silent: true }); + } + }); + + it('reports a key the contract does not name — the firing control beside that zero', () => { + for (const key of ['nope', 'roles', 'organizationId', 'department', 'tenantId']) { + expect(RESERVED_RLS_MEMBERSHIP_KEYS).not.toContain(key); + expect(ids(siteWith('using', `owner_id == current_user.${key}`))).toEqual([ + RLS_PREDICATE_UNKNOWN_USER_VARIABLE, + ]); + } + }); +}); + +describe('validateRlsPredicateEnforceability — §7.3.1 membership keys stay UNKNOWABLE', () => { + /** + * The false-positive this rule exists on the edge of. An app stages arbitrary + * keys into `ExecutionContext.rlsMembership` and references them as + * `field in current_user.`; `RowLevelSecurityPolicySchema` documents the + * pattern and the EXISTING `rls-predicate-unparseable` hint recommends it. In + * an `in` position an unknown key is indistinguishable from a correct one, so + * it must never be reported. + * + * It is decidable in the other positions only because the merge is array-only + * (`compileFilter` stages an entry `if (Array.isArray(value))`), so the sole + * value an app-staged key can ever hold is an array — which a scalar position + * cannot use, on any request. + */ + it.each([ + ['a team set', 'assigned_to_id in current_user.team_member_ids'], + ['a territory set', 'owner_id in current_user.territory_account_ids'], + ['the spec doc example', 'assigned_to_id in current_user.team_ids'], + ['bridged from legacy SQL', 'assigned_to_id IN (current_user.team_member_ids)'], + ['composed with a real clause', 'is_private == false || assigned_to_id in current_user.team_member_ids'], + ])('%s is silent', (_label, source) => { + expect(ids(siteWith('using', source))).toEqual([]); + }); + + it('a key used in BOTH positions takes the membership answer (the conservative direction)', () => { + expect(ids(siteWith('using', 'owner_id in current_user.zzz && status == current_user.zzz'))).toEqual([]); + }); +}); + +describe('validateRlsPredicateEnforceability — the graph\'s three skips, not this rule\'s', () => { + it('skips an object this stack does not define', () => { + expect(ids(siteWith('using', 'whatever == current_user.id', 'not_in_this_stack'))).toEqual([]); + }); + + it('skips an object that declares no readable field map', () => { + const stack = { + objects: [{ name: 'ext_thing', label: 'Ext', external: true }], + permissions: [ + { name: 'p', rowLevelSecurity: [{ name: 'r', object: 'ext_thing', using: 'whatever == current_user.id' }] }, + ], + }; + expect(ids(stack)).toEqual([]); + }); + + it('skips a registry-injected system column', () => { + // `created_at` appears in no authored `fields` and is real at runtime. + expect(ids(siteWith('using', 'created_at != null'))).toEqual([]); + // …and `owner_id` resolves through the declared map on an `ownership: user` + // object, so the injected path and the declared path agree here. + expect(ids(siteWith('using', 'owner_id == current_user.id'))).toEqual([]); + }); + + it('skips a policy that names no object at all', () => { + const stack = { + objects: [CRM_OPPORTUNITY], + permissions: [{ name: 'p', rowLevelSecurity: [{ name: 'r', using: 'whatever == current_user.id' }] }], + }; + expect(ids(stack)).toEqual([]); + }); +}); + +describe('validateRlsPredicateEnforceability — the reference pass never throws', () => { + it.each([ + ['an empty stack', {}], + ['objects but no permissions', { objects: [CRM_OPPORTUNITY] }], + ['a permission set with no policies', { objects: [CRM_OPPORTUNITY], permissions: [{ name: 'p' }] }], + ['a null member in `objects`', { objects: [null, CRM_OPPORTUNITY], permissions: [] }], + ['a null member in `rowLevelSecurity`', { objects: [CRM_OPPORTUNITY], permissions: [{ name: 'p', rowLevelSecurity: [null] }] }], + ])('%s', (_label, stack) => { + expect(() => validateRlsPredicateEnforceability(stack)).not.toThrow(); + expect(validateRlsPredicateEnforceability(stack)).toEqual([]); + }); + + it('still resolves through the name-keyed map spelling of `objects`', () => { + const stack = { + objects: { crm_opportunity: { fields: { is_private: { name: 'is_private', type: 'boolean' } } } }, + permissions: [ + { name: 'p', rowLevelSecurity: [{ name: 'r', object: 'crm_opportunity', using: 'nope_field == 1' }] }, + ], + }; + expect(ids(stack)).toEqual([RLS_PREDICATE_UNKNOWN_FIELD]); + }); +}); + +describe('validateRlsPredicateEnforceability — the field set is read off the COMPILER\'s output', () => { + it.each([ + ['a leading miss (the safety net\'s own position)', 'nope_one == 1', 1], + ['a trailing miss (past the safety net)', 'is_private == false || nope_two == 1', 1], + ['a miss on the right of a field-to-field comparison', 'owner_id == nope_three', 1], + ['a miss inside a membership test', "nope_four in ['a', 'b']", 1], + ['a miss inside a string method', "nope_five.startsWith('AC')", 1], + ['a miss under a negation', '!(nope_six == 1)', 1], + ['two distinct misses in one predicate', 'nope_seven == 1 && nope_eight == 2', 2], + ])('%s', (_label, source, expected) => { + const findings = validateRlsPredicateEnforceability(siteWith('using', source)); + expect(findings.map((f) => f.rule)).toEqual(new Array(expected).fill(RLS_PREDICATE_UNKNOWN_FIELD)); + }); + + it('reports one finding per missing NAME, not one per occurrence', () => { + expect(ids(siteWith('using', 'nope_dup == 1 || nope_dup == 2'))).toEqual([RLS_PREDICATE_UNKNOWN_FIELD]); + }); + + it('reports both halves when a predicate carries both misses', () => { + expect(ids(siteWith('using', 'nope_field == current_user.nope_var'))).toEqual([ + RLS_PREDICATE_UNKNOWN_FIELD, + RLS_PREDICATE_UNKNOWN_USER_VARIABLE, + ]); + }); +}); From 90e54a08329ea879420a6218f812d7009a7dc7bc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 03:13:13 +0000 Subject: [PATCH 3/5] docs(changeset): cover the RLS predicate reference rules Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- .changeset/rls-predicate-references.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 .changeset/rls-predicate-references.md diff --git a/.changeset/rls-predicate-references.md b/.changeset/rls-predicate-references.md new file mode 100644 index 0000000000..a53ca91116 --- /dev/null +++ b/.changeset/rls-predicate-references.md @@ -0,0 +1,16 @@ +--- +"@objectstack/lint": minor +--- + +Two new gating rules — `rls-predicate-unknown-field` and `rls-predicate-unknown-user-variable`: an RLS predicate that lowers correctly but names a column the object does not declare, or a `current_user.*` value nothing pre-resolves, is now an authoring-time `error`. + +The three shipped `rls-predicate-*` rules judge a predicate's **shape** — does it parse, does it lower, does it fit the platform's CEL bounds. Nothing judged what it **points at**. Measured as four injections at one site, in one run: `billing_address.country == "US"` reported `rls-predicate-unenforceable` and `is_private == = false` reported `rls-predicate-unparseable`, while `is_private_nope == false || owner_id == current_user.id` and `is_private == false || owner_id == current_user.nope` reported **nothing at all** — from the same site the linter had just reported twice. + +Both silent shapes fail **closed**, which is what makes them expensive rather than cosmetic. An unknown field is what a column rename leaves behind: `SecurityPlugin`'s field-existence safety net reads the predicate's leading `field ==` / `=` / `in`, refuses a column the object lacks and drops the policy, and when it was the only applicable policy the layer falls back to the `RLS_DENY_FILTER` sentinel — every select / update / delete matches zero rows. An unresolved `current_user.*` reaches the same sentinel one step earlier, through the compiler's `unresolved-variable` refusal. Either way the object **disappears for every holder of the permission set**, not because they were denied but because the narrowing they were granted resolves to nothing — with lint green and CI green until someone opens it and finds it empty. + +- **Two rules beside the three, not a widening of them.** The existing ids say *unenforceable* / *unparseable* / *over-budget* and are correct inside that scope; they are untouched, and the two controls above still report under them and under neither new id. The prescriptions differ (rewrite the predicate / fix the column name / pre-resolve the variable), and an author who suppresses one must not thereby suppress the other. The guards are disjoint by construction: the reference pass runs only where `isSupportedRlsExpression` has already said yes. +- **Where the existence answer comes from.** Field paths are read off the pushdown compiler's **own output** — the lowered `FilterCondition`'s keys are the columns the driver will be handed — and resolved through `object-graph.ts`, the shared index every field-existence rule in this package already uses. No new input path, no second parse of the predicate. The rule therefore inherits that module's three skips, each the difference between a finding and a false one: an object this stack does not define, an object with no readable field map (an ADR-0015 `external` object, an introspected datasource), and registry-injected system columns such as `created_at`, which are real at runtime and appear in no authored `fields`. +- **The `current_user` set is derived, not transcribed.** It is `RESERVED_RLS_MEMBERSHIP_KEYS` from `@objectstack/spec/contracts` — the keys an `IRlsMembershipResolver` may never supply *because the kernel already owns them*. A key added there stops being reported the same day, with no edit in this package. +- **§7.3.1 membership keys are left alone, and that boundary is the reason this rule can exist.** An app stages arbitrary sets into `ExecutionContext.rlsMembership` and references them as `field in current_user.`; the spec documents the pattern and `rls-predicate-unparseable`'s own hint recommends it. In an `in` position an unknown key is indistinguishable from a correct one and is never reported. It is decidable in the other positions only because the merge is array-only — the sole value an app-staged key can ever hold is an array, which a scalar position cannot use on any request — so `owner_id == current_user.nope` is refused while `assigned_to_id in current_user.team_member_ids` stays silent. A key used in both positions takes the membership answer. + +**What moves for consumers.** A stack whose RLS predicate names a renamed column or an un-pre-resolved context value built clean before and now fails `os validate` / `os lint` / `os compile`. That is the point — the policy had already stopped enforcing what it was written to enforce, and was denying the whole object instead. A stack whose predicates all resolve is byte-identically clean: measured on the shipped showcase (44 objects, 6 RLS policies over two objects) at zero findings, on `plugin-security`'s seed permission sets at zero, and on hotcrm's built-permissions fixture at zero — each with a firing control beside it (one injected dangling column and one injected unknown variable at the same site produce exactly one finding each) and a nonsense control (an injected `in current_user.` and a real-field/real-variable predicate stay silent). From 52c27ab97d4755025ba48d44ec7468822e2d8641 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 04:01:47 +0000 Subject: [PATCH 4/5] fix(lint): state the measured consequence of an unknown RLS field, both directions The rule's detection was right; what it SAID the miss costs was not. The consequence prose claimed both reference misses fail closed. That holds for an unresolved current_user value, which the compiler refuses in every position, and it does not hold for a missing field: extractTargetField recognises only a leading `field ==` / `=` / `in`, so a negation or any later arm leaves the policy kept, and a row without that column satisfies the negated constraint. Measured 3/3 rows against a 1/3 real narrowing and a 0/3 phantom positive, on the read path and on matchesFilterCondition alike. The message, the docblock and the changeset now say which direction applies, and say it with the limits intact: not a cross-tenant leak, driver-sql not measured. The runtime repair is tracked separately and is not attempted here. Also tightens the current_user position pin, which asserted a disjunction that passed on whichever position happened to be silent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- .changeset/rls-predicate-references.md | 12 +- ...idate-rls-predicate-enforceability.test.ts | 73 +++++++++-- .../validate-rls-predicate-enforceability.ts | 114 +++++++++++++----- 3 files changed, 161 insertions(+), 38 deletions(-) diff --git a/.changeset/rls-predicate-references.md b/.changeset/rls-predicate-references.md index a53ca91116..feb36afed1 100644 --- a/.changeset/rls-predicate-references.md +++ b/.changeset/rls-predicate-references.md @@ -6,11 +6,19 @@ Two new gating rules — `rls-predicate-unknown-field` and `rls-predicate-unknow The three shipped `rls-predicate-*` rules judge a predicate's **shape** — does it parse, does it lower, does it fit the platform's CEL bounds. Nothing judged what it **points at**. Measured as four injections at one site, in one run: `billing_address.country == "US"` reported `rls-predicate-unenforceable` and `is_private == = false` reported `rls-predicate-unparseable`, while `is_private_nope == false || owner_id == current_user.id` and `is_private == false || owner_id == current_user.nope` reported **nothing at all** — from the same site the linter had just reported twice. -Both silent shapes fail **closed**, which is what makes them expensive rather than cosmetic. An unknown field is what a column rename leaves behind: `SecurityPlugin`'s field-existence safety net reads the predicate's leading `field ==` / `=` / `in`, refuses a column the object lacks and drops the policy, and when it was the only applicable policy the layer falls back to the `RLS_DENY_FILTER` sentinel — every select / update / delete matches zero rows. An unresolved `current_user.*` reaches the same sentinel one step earlier, through the compiler's `unresolved-variable` refusal. Either way the object **disappears for every holder of the permission set**, not because they were denied but because the narrowing they were granted resolves to nothing — with lint green and CI green until someone opens it and finds it empty. +Both silent shapes are expensive rather than cosmetic, and they do **not** fail in the same direction — which is the part the card's own measurement did not reach. + +An unresolved `current_user.*` is refused by the pushdown compiler in **every** position, including under `!` and in a trailing `||` arm, so that half always fails **closed**: `RLSCompiler` drops the policy, the layer falls back to the `RLS_DENY_FILTER` sentinel, and the object disappears for every holder of the permission set — not because they were denied but because the narrowing they were granted resolves to nothing. + +An unknown **field** takes its direction from **position**, and one of the two is fail-**open**. `SecurityPlugin`'s field-existence safety net recognises only a *leading* `field ==` / `=` / `in` (`extractTargetField` is that shape match), so a miss there drops the policy and arms the deny sentinel — zero rows. A miss the net does not recognise — a negation (`nope != "x"`, `!(nope == 1)`, `!(nope in ['a'])`) or any arm after the first — leaves the policy **kept**, and the phantom column lowers to a negated constraint that a row without that column *satisfies* (`noValueSatisfiesNegation`: `$ne` / `$nin` / `$notContains`). The authored narrowing is then **defeated rather than enforced**: measured at 3 of 3 rows, against 1 of 3 for the real narrowing and 0 of 3 for the same phantom column in a positive position, on the read path and on the write path's `matchesFilterCondition` alike. + +⛔ That is **not** a cross-tenant leak — tenancy is a separate layer and it holds; what is defeated is the narrowing authored inside the wall. Measured on driver-memory; driver-mongodb follows the same shared ruling; **driver-sql is NOT MEASURED** and is expected to fail closed by raising `no such column`. The runtime repair is tracked separately as #17042 and is deliberately not attempted here — these rules report the miss, in both directions, and the diagnostic says which direction applies so an author is not told "this denies everything" about a predicate that in fact matches everything. - **Two rules beside the three, not a widening of them.** The existing ids say *unenforceable* / *unparseable* / *over-budget* and are correct inside that scope; they are untouched, and the two controls above still report under them and under neither new id. The prescriptions differ (rewrite the predicate / fix the column name / pre-resolve the variable), and an author who suppresses one must not thereby suppress the other. The guards are disjoint by construction: the reference pass runs only where `isSupportedRlsExpression` has already said yes. - **Where the existence answer comes from.** Field paths are read off the pushdown compiler's **own output** — the lowered `FilterCondition`'s keys are the columns the driver will be handed — and resolved through `object-graph.ts`, the shared index every field-existence rule in this package already uses. No new input path, no second parse of the predicate. The rule therefore inherits that module's three skips, each the difference between a finding and a false one: an object this stack does not define, an object with no readable field map (an ADR-0015 `external` object, an introspected datasource), and registry-injected system columns such as `created_at`, which are real at runtime and appear in no authored `fields`. - **The `current_user` set is derived, not transcribed.** It is `RESERVED_RLS_MEMBERSHIP_KEYS` from `@objectstack/spec/contracts` — the keys an `IRlsMembershipResolver` may never supply *because the kernel already owns them*. A key added there stops being reported the same day, with no edit in this package. - **§7.3.1 membership keys are left alone, and that boundary is the reason this rule can exist.** An app stages arbitrary sets into `ExecutionContext.rlsMembership` and references them as `field in current_user.`; the spec documents the pattern and `rls-predicate-unparseable`'s own hint recommends it. In an `in` position an unknown key is indistinguishable from a correct one and is never reported. It is decidable in the other positions only because the merge is array-only — the sole value an app-staged key can ever hold is an array, which a scalar position cannot use on any request — so `owner_id == current_user.nope` is refused while `assigned_to_id in current_user.team_member_ids` stays silent. A key used in both positions takes the membership answer. -**What moves for consumers.** A stack whose RLS predicate names a renamed column or an un-pre-resolved context value built clean before and now fails `os validate` / `os lint` / `os compile`. That is the point — the policy had already stopped enforcing what it was written to enforce, and was denying the whole object instead. A stack whose predicates all resolve is byte-identically clean: measured on the shipped showcase (44 objects, 6 RLS policies over two objects) at zero findings, on `plugin-security`'s seed permission sets at zero, and on hotcrm's built-permissions fixture at zero — each with a firing control beside it (one injected dangling column and one injected unknown variable at the same site produce exactly one finding each) and a nonsense control (an injected `in current_user.` and a real-field/real-variable predicate stay silent). +**What moves for consumers.** A stack whose RLS predicate names a renamed column or an un-pre-resolved context value built clean before and now fails `os validate` / `os lint` / `os compile`. That is the point — the policy had already stopped doing what it was written to do, denying the whole object in one position and granting every row in the other. + +A stack whose predicates all resolve is byte-identically clean. The reading is the shipped showcase: 3 RLS clauses, all 3 judgeable against declared objects, **zero** findings — with three firing controls at the real site (an injected dangling column, an injected unknown variable, and an injected fail-open negation shape each produce exactly one finding) and two nonsense controls (an injected membership test against an unknown key, and a real-field/real-variable predicate, stay silent). `plugin-security`'s seed sets and hotcrm's built-permissions fixture also emit zero, but ⛔ **those two are not readings**: every policy target in the seeds is an object that package does not declare, and the hotcrm fixture carries no `objects` key at all, so all 71 and all 4 clauses respectively are skipped by construction. Declaring one of their objects makes the fixture report 2 — which is what a control is for. diff --git a/packages/lint/src/validate-rls-predicate-enforceability.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.test.ts index 67ba68dc7c..315dc773d8 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -623,10 +623,22 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not expect(f.where).toBe('permission set "sales_manager" policy "opportunity_private_owner_only" on object "crm_opportunity"'); expect(f.path).toBe('permissions[0].rowLevelSecurity[0].using'); expect(f.severity).toBe('error'); - expect(f.message).toMatch(/field-existence safety net DROPS the policy/); + // ⚠️ BOTH directions, because they are not the same and the fail-OPEN one is + // the dangerous half: an author told "this denies everything" about a + // predicate that in fact matches everything hardens the wrong thing. + expect(f.message).toMatch(/one of the two directions is fail-OPEN/); + expect(f.message).toMatch(/#17042/); + // closed leg — the LEADING position the safety net recognises + expect(f.message).toMatch(/fails CLOSED/); expect(f.message).toMatch(/RLS_DENY_FILTER/); expect(f.message).toMatch(/ZERO rows/); - expect(f.message).toMatch(/DISAPPEARS for every holder of this permission set/); + // open leg — a negation or any arm after the first + expect(f.message).toMatch(/leaves the policy KEPT/); + expect(f.message).toMatch(/SATISFIES the negated constraint/); + expect(f.message).toMatch(/DEFEATED/); + // …and the limits, stated rather than overstated + expect(f.message).toMatch(/NOT a cross-tenant leak/); + expect(f.message).toMatch(/driver-sql is NOT MEASURED/); // …and the miss itself, with the platform's own "did you mean". expect(f.message).toMatch(/"is_private_nope" is not a field on object "crm_opportunity"/); expect(f.message).toMatch(/Did you mean "is_private"\?/); @@ -640,7 +652,12 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not expect(f.rule).toBe(RLS_PREDICATE_UNKNOWN_USER_VARIABLE); expect(f.message).toMatch(/reads `current_user\.nope`, which nothing pre-resolves/); expect(f.message).toMatch(/scalar position, so no request can ever supply it/); + // The variable half really is fail-closed in EVERY position — the compiler + // refuses it under `!` and in a trailing `||` arm alike — so unlike the + // field half it may say so without qualification. + expect(f.message).toMatch(/unresolved-variable` in EVERY position/); expect(f.message).toMatch(/DISAPPEARS for every holder of this permission set/); + expect(f.message).not.toMatch(/fail-OPEN/); expect(f.hint).toMatch(/IRlsMembershipResolver/); expect(f.hint).toMatch(/never compared with `==`/); }); @@ -650,8 +667,11 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not expect(f.rule).toBe(RLS_PREDICATE_UNKNOWN_FIELD); expect(f.path).toBe('permissions[0].rowLevelSecurity[0].check'); expect(f.message).toMatch(/PermissionDeniedError/); - expect(f.message).toMatch(/blanket refusal/); - expect(f.message).not.toMatch(/select \/ update \/ delete matches ZERO rows/); + // The write path has the SAME asymmetry, measured against the same controls: + // a positive phantom constraint refuses the post-image, a negated one is + // satisfied vacuously and permits the write the policy was written to refuse. + expect(f.message).toMatch(/permits exactly the writes it was written to refuse/); + expect(f.message).not.toMatch(/select \/ update \/ delete matches ZERO/); }); }); @@ -670,14 +690,16 @@ describe('validateRlsPredicateEnforceability — the `current_user` set is DERIV * different, stale shape (`tenantId`, `department`, `attributes`) the RLS * compiler never binds — neither is the authority. */ - it('accepts every kernel-resolved key, in the position that key supports', () => { + it('accepts every kernel-resolved key in BOTH positions', () => { expect(RESERVED_RLS_MEMBERSHIP_KEYS.length).toBeGreaterThan(0); for (const key of RESERVED_RLS_MEMBERSHIP_KEYS) { - // A scalar comparison and a membership test between them cover both - // positions, so the assertion does not need to know which kind each key is. + // ⚠️ BOTH, asserted separately. An `a.length === 0 || b.length === 0` + // here would pass on whichever position happened to be silent, and this + // rule is silent in both — so the disjunction pinned nothing about + // position at all and would have survived a position-blind rewrite. const scalar = ids(siteWith('using', `owner_id == current_user.${key}`)); const member = ids(siteWith('using', `owner_id in current_user.${key}`)); - expect({ key, silent: scalar.length === 0 || member.length === 0 }).toEqual({ key, silent: true }); + expect({ key, scalar, member }).toEqual({ key, scalar: [], member: [] }); } }); @@ -775,6 +797,41 @@ describe('validateRlsPredicateEnforceability — the reference pass never throws }); }); +describe('validateRlsPredicateEnforceability — the fail-OPEN field shapes are reported too', () => { + /** + * The half the card's escalation clause did not name. It asked for a + * fail-OPEN *variable*, and the compiler refuses those in every position; the + * hole is field-shaped instead. + * + * `extractTargetField` matches only a LEADING `field ==` / `=` / `in`, so for + * each shape below the safety net returns `null`, the policy is KEPT, and the + * phantom column lowers to a negated constraint that a row without that + * column satisfies (`noValueSatisfiesNegation`). Measured: 3 of 3 rows, + * against 1 of 3 for the real narrowing and 0 of 3 for the same phantom + * column in a positive position — read path and write path alike. + * + * The runtime repair is #17042 and is deliberately NOT attempted here. What + * this rule owes is that the miss is REPORTED in these positions too, which + * is what these cases pin: a rule that only caught the leading position would + * satisfy the card and miss the dangerous half entirely. + */ + it.each([ + ['a bare negation', 'nope_a != "x"'], + ['a negated equality', '!(nope_b == 1)'], + ['a negated membership', "!(nope_c in ['a'])"], + ['a trailing `||` arm behind a REAL leading field', 'is_private == false || nope_d != "x"'], + ['a trailing `&&` arm behind a REAL leading field', 'is_private == false && nope_e != "x"'], + ])('%s is reported', (_label, source) => { + expect(ids(siteWith('using', source))).toEqual([RLS_PREDICATE_UNKNOWN_FIELD]); + }); + + it('the shipped predicate in the same shapes stays silent — the negative control', () => { + for (const source of ['is_private != true', '!(is_private == true)', 'is_private == false || owner_id != "x"']) { + expect(ids(siteWith('using', source))).toEqual([]); + } + }); +}); + describe('validateRlsPredicateEnforceability — the field set is read off the COMPILER\'s output', () => { it.each([ ['a leading miss (the safety net\'s own position)', 'nope_one == 1', 1], diff --git a/packages/lint/src/validate-rls-predicate-enforceability.ts b/packages/lint/src/validate-rls-predicate-enforceability.ts index bd902dba52..8953816331 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -271,10 +271,28 @@ function consequence(clause: 'using' | 'check'): string { * by NOTHING — measured at the same site, in the same run, that reported the two * shape faults twice. * - * Both miss directions fail CLOSED, which is why they survived: an unknown - * field is what a column RENAME leaves behind, and an unresolved `current_user.*` - * compiles to nothing — so the authored narrowing becomes a blanket refusal and - * every holder of the permission set loses the object, with lint and CI green. + * Both were invisible, and they are NOT the same failure. The card measured + * both as fail-CLOSED and that reading is right for the shapes it measured; it + * does not generalise. An unresolved `current_user.*` really is refused by the + * compiler in every position, so that half always fails closed: the authored + * narrowing becomes a blanket refusal and every holder of the permission set + * loses the object. A missing FIELD takes its direction from POSITION, and one + * of the two is fail-OPEN: + * + * - Leading `field ==` / `=` / `in` — the only shape `extractTargetField` + * recognises — is dropped by the field-existence safety net and arms the + * deny sentinel. Fail closed. + * - A negation (`nope != "x"`, `!(nope == 1)`, `!(nope in ['a'])`) or any arm + * after the first is NOT recognised, so the policy is KEPT and the phantom + * column lowers to a negated constraint that a row without that column + * SATISFIES (`noValueSatisfiesNegation`: `$ne` / `$nin` / `$notContains`). + * The narrowing is DEFEATED — measured at 3 of 3 rows against a 1-of-3 real + * narrowing and a 0-of-3 phantom positive, on the read path and on the + * write path's `matchesFilterCondition` alike. ⛔ Not a cross-tenant leak: + * tenancy is a separate layer and holds. The runtime half is #17042 and is + * NOT this rule's to fix — this rule reports the miss, in both directions. + * + * Either way it is what a column RENAME leaves behind, with lint and CI green. * * ## Why this is a SECOND pair of ids and not a widening of the three above * @@ -481,27 +499,65 @@ function filterFieldPaths(filter: Record | null): Set { return fields; } -/** What a reference miss costs at request time, per clause. Measured, not inferred. */ +/** + * What a reference miss costs at request time, per clause. Measured, not inferred. + * + * ⚠️ The two KINDS do not have the same failure direction, and the field half + * does not have ONE direction. An unresolved variable is refused by the compiler + * in every position, so it always fails closed. A missing FIELD fails closed or + * fails OPEN depending on where in the predicate it sits, and the message says + * which — an author told "this denies everything" about a predicate that in fact + * matches everything would harden exactly the wrong thing. + */ function referenceConsequence(clause: 'using' | 'check', kind: 'field' | 'variable'): string { - const dropped = - kind === 'field' - ? '`SecurityPlugin`\'s field-existence safety net DROPS the policy before the compiler sees it — it ' + - 'reads the predicate\'s LEADING `field ==` / `=` / `in` and refuses a column the object lacks — and ' + - 'a miss anywhere further along instead reaches the driver as a phantom column (`no such column`, or ' + - 'zero rows with no error at all). ' - : 'The pushdown compiler answers `unresolved-variable`, so `RLSCompiler` DROPS the policy at request ' + - 'time — one WARN line is the only signal, and nothing reports it at authoring time. '; - return clause === 'using' - ? dropped + - 'When it is the only applicable policy for that object and operation the layer falls back to the ' + - '`RLS_DENY_FILTER` sentinel, which is AND-ed onto the where clause: every select / update / delete ' + - 'matches ZERO rows, so the object DISAPPEARS for every holder of this permission set — not because ' + - 'they were denied, but because the narrowing they were granted resolves to nothing. When other ' + - 'policies also apply, this one vanishes from the OR and grants none of the access it appears to.' - : dropped + - 'On the ADR-0058 D4 write path the post-image `check` can then never be satisfied: every insert / ' + - 'update the policy governs fails with `PermissionDeniedError`. The policy reads as a write rule and ' + - 'behaves as a blanket refusal for every holder of this permission set.'; + if (kind === 'variable') { + const dropped = + 'The pushdown compiler answers `unresolved-variable` in EVERY position — including under `!` and in ' + + 'a trailing `||` arm — so `RLSCompiler` DROPS the policy at request time, and one WARN line is the ' + + 'only signal. '; + return clause === 'using' + ? dropped + + 'When it is the only applicable policy for that object and operation the layer falls back to the ' + + '`RLS_DENY_FILTER` sentinel, which is AND-ed onto the where clause: every select / update / ' + + 'delete matches ZERO rows, so the object DISAPPEARS for every holder of this permission set — ' + + 'not because they were denied, but because the narrowing they were granted resolves to nothing. ' + + 'When other policies also apply, this one vanishes from the OR and grants none of the access it ' + + 'appears to.' + : dropped + + 'On the ADR-0058 D4 write path that leaves the post-image `check` unsatisfiable: every insert / ' + + 'update the policy governs fails with `PermissionDeniedError`. The policy reads as a write rule ' + + 'and behaves as a blanket refusal for every holder of this permission set.'; + } + + // ── The FIELD half. Which direction it takes is decided by position, and one + // of the two is fail-OPEN (#17042). + const closed = + clause === 'using' + ? 'the field-existence safety net in `SecurityPlugin` DROPS the policy and, when it was the only ' + + 'applicable one, arms the `RLS_DENY_FILTER` sentinel — every select / update / delete matches ZERO ' + + 'rows and the object disappears for every holder of this permission set' + : 'the post-image can never satisfy the constraint, so every insert / update the policy governs ' + + 'fails with `PermissionDeniedError` — a blanket refusal for every holder of this permission set'; + const open = + clause === 'using' + ? 'every row inside the tenant wall SATISFIES the negated constraint, so the policy stops narrowing ' + + 'and matches everything the wall admits' + : 'the post-image SATISFIES the negated constraint vacuously, so the check permits exactly the ' + + 'writes it was written to refuse'; + return ( + 'What it costs depends on WHERE the miss sits, and one of the two directions is fail-OPEN (#17042). ' + + 'The safety net recognises only a LEADING `field ==` / `=` / `in` — `extractTargetField` is that ' + + `shape match — so a miss THERE fails CLOSED: ${closed}. A miss the net does NOT recognise — a ` + + 'negation (`field != x`, `!(field == x)`, `!(field in [...])`), or any arm after the first — leaves ' + + 'the policy KEPT, and a row that has no such column satisfies a negation: ' + + `${open}. The authored narrowing is then DEFEATED rather than enforced. ` + + 'Measured on the driver-memory matcher and on `matchesFilterCondition` (the write path), each against ' + + 'two controls: the real narrowing selects 1 of 3 rows, the SAME phantom column in a positive position ' + + 'selects 0 of 3, and each negation shape selects 3 of 3. ⛔ It is NOT a cross-tenant leak — tenancy is ' + + 'a separate layer and holds; what is defeated is the narrowing authored INSIDE the wall. ' + + 'driver-mongodb follows the same shared ruling; driver-sql is NOT MEASURED and is expected to fail ' + + 'closed by raising `no such column`. Fixing the runtime is #17042; this rule only reports the miss.' + ); } /** @@ -538,10 +594,12 @@ function referenceFindings( `${account.message} ` + referenceConsequence(clause, 'field'), hint: `${account.detail} Point the predicate at a column the object really declares, or delete the ` + - `policy if the narrowing is gone — a policy that can never match is not protection, it is an ` + - `outage. If the field was RENAMED, this policy has been denying since that rename; if it was ` + - `meant to live on another object, RLS cannot join to it (ADR-0055) — denormalise the value onto ` + - `"${object}" (a formula/rollup field) and test that column instead.`, + `policy if the narrowing is gone — a policy naming a column that does not exist is not ` + + `protection either way: it is an outage in one position and an open door in the other. If the ` + + `field was RENAMED, this policy has been wrong since that rename — check WHICH way before you ` + + `judge the blast radius, because a negated or non-leading miss has been granting, not denying. ` + + `If the value was meant to live on another object, RLS cannot join to it (ADR-0055) — ` + + `denormalise it onto "${object}" (a formula/rollup field) and test that column instead.`, }); } From 530469ff4ce8c690570e5196dc4102ac1ca7c1e6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 04:12:39 +0000 Subject: [PATCH 5/5] fix(lint): keep the tracker id out of the diagnostic string check:doc-authoring is right: a runtime string reaches authors, operators and generated surfaces, none of whom can resolve a bare issue number. The id moves to the adjacent comment, where the reader who can resolve it is already looking, and a test pins that neither the message nor the hint carries one again. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- .../src/validate-rls-predicate-enforceability.test.ts | 8 +++++++- .../lint/src/validate-rls-predicate-enforceability.ts | 10 +++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/lint/src/validate-rls-predicate-enforceability.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.test.ts index 315dc773d8..2600e7bc5f 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -627,7 +627,13 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not // the dangerous half: an author told "this denies everything" about a // predicate that in fact matches everything hardens the wrong thing. expect(f.message).toMatch(/one of the two directions is fail-OPEN/); - expect(f.message).toMatch(/#17042/); + // ⛔ …and NOT by citing a tracker id. This string reaches authors, + // operators and generated surfaces, none of whom can resolve `#NNNN` + // (`check:doc-authoring`); the id lives in the adjacent `//` comment, which + // the reader who CAN resolve it is already reading. Pinned here so the next + // author does not re-add it and learn this from CI instead. + expect(f.message).not.toMatch(/#\d{3,}/); + expect(f.hint).not.toMatch(/#\d{3,}/); // closed leg — the LEADING position the safety net recognises expect(f.message).toMatch(/fails CLOSED/); expect(f.message).toMatch(/RLS_DENY_FILTER/); diff --git a/packages/lint/src/validate-rls-predicate-enforceability.ts b/packages/lint/src/validate-rls-predicate-enforceability.ts index 8953816331..e1b5efaf0b 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -530,7 +530,10 @@ function referenceConsequence(clause: 'using' | 'check', kind: 'field' | 'variab } // ── The FIELD half. Which direction it takes is decided by position, and one - // of the two is fail-OPEN (#17042). + // of the two is fail-OPEN. The runtime repair is tracked in #17042; the id + // stays HERE and never in the returned string, because that string reaches + // authors, operators and generated surfaces, none of whom can resolve + // `#NNNN` (`check:doc-authoring`, maintainer ruling 2026-08-12). const closed = clause === 'using' ? 'the field-existence safety net in `SecurityPlugin` DROPS the policy and, when it was the only ' + @@ -545,7 +548,7 @@ function referenceConsequence(clause: 'using' | 'check', kind: 'field' | 'variab : 'the post-image SATISFIES the negated constraint vacuously, so the check permits exactly the ' + 'writes it was written to refuse'; return ( - 'What it costs depends on WHERE the miss sits, and one of the two directions is fail-OPEN (#17042). ' + + 'What it costs depends on WHERE the miss sits, and one of the two directions is fail-OPEN. ' + 'The safety net recognises only a LEADING `field ==` / `=` / `in` — `extractTargetField` is that ' + `shape match — so a miss THERE fails CLOSED: ${closed}. A miss the net does NOT recognise — a ` + 'negation (`field != x`, `!(field == x)`, `!(field in [...])`), or any arm after the first — leaves ' + @@ -556,7 +559,8 @@ function referenceConsequence(clause: 'using' | 'check', kind: 'field' | 'variab 'selects 0 of 3, and each negation shape selects 3 of 3. ⛔ It is NOT a cross-tenant leak — tenancy is ' + 'a separate layer and holds; what is defeated is the narrowing authored INSIDE the wall. ' + 'driver-mongodb follows the same shared ruling; driver-sql is NOT MEASURED and is expected to fail ' + - 'closed by raising `no such column`. Fixing the runtime is #17042; this rule only reports the miss.' + 'closed by raising `no such column`. Repairing that is the runtime\'s job, not this rule\'s — what ' + + 'this diagnostic owes you is WHICH direction your predicate is in.' ); }