diff --git a/.changeset/rls-undeclared-column-denies-in-every-position.md b/.changeset/rls-undeclared-column-denies-in-every-position.md new file mode 100644 index 0000000000..3f6752cbab --- /dev/null +++ b/.changeset/rls-undeclared-column-denies-in-every-position.md @@ -0,0 +1,29 @@ +--- +"@objectstack/plugin-security": minor +"@objectstack/lint": patch +--- + +fix(plugin-security)!: an RLS predicate naming an undeclared column now denies in EVERY position and polarity, on the read face and the write face alike (#17042) + + + +**BREAKING** — a fail-open-to-fail-closed narrowing on row-level security. A policy that widened yesterday denies today. Shipped as `minor` under the launch-window convention, the same grading the insert-side `check` post-image narrowing used. + +A predicate naming a column the object does **not declare** could not narrow, and in a **negation-carrying position** it did not deny either — it **widened** the policy to every row inside the tenant wall, and on the write path it **permitted** the write the policy was authored to refuse. + +⛔ It is **not** a cross-tenant leak. Tenancy is a separate layer and it holds. What was defeated is the narrowing the policy author wrote *inside* the wall — an owner-only or private-record policy silently becoming "every row". + +Two independent sites, each with its own reason, each measured against the same two controls (a real column must still narrow; the *same* phantom column in a **positive** position must still refuse): + +- **Read face.** `extractTargetField` is a **leading-only** `==` / `=` / `in` shape match, so `nope != "x"`, `!(nope == 1)`, `!(nope in ['a'])` and any arm after the first returned `null`; the policy was **kept**, the drop counter never incremented and the deny sentinel never armed. The kept filter then met the settled include-direction ruling — a row that *has* no such column satisfies "column != x". Measured on the matcher: **3 of 3** rows for each negated shape, against **1 of 3** for the real narrowing and **0 of 3** for the same phantom column in a positive position. +- **Write face — the worse one.** `computeWriteCheckFilter` compiled `check` clauses with **no field-existence check at all**, and the ADR-0058 D4 post-image gate evaluates that filter in-process. Measured end to end on both SQL drivers: every negated phantom **permitted** the insert, in both post-image polarities, while a positive phantom refused (by accident of an absent value comparing unequal) — which is why a suite that only ever exercised the positive shape stayed green over the hole. + +**The repair is one seam, not two.** `RLSCompiler.compileFilter` — the single choke point both the read layer and the write gate already pass through — now takes the object's declared-column set and judges every column the policy names on the **compiled** `FilterCondition` tree. That is positional-agnostic by construction: the pushdown compiler lowers `!` to `$not`, `||` to `$or` and `&&` to `$and`, so a column lands as a plain object key whatever position it was authored in, and there is no spelling of negation left for a shape match to miss. Widening the regex instead was rejected: a matcher that must enumerate every spelling of negation is the same "recognises only what it was told about" defect one level over, and it would additionally have broken the ADR-0095 carve-out that *depends* on the regex recognising only the leading shape. A policy dropped this way joins the existing fail-closed path — same deny sentinel, same WARN line — rather than growing a parallel mechanism. + +⛔ **The matcher's include-direction ruling is untouched.** A row lacking a column *does* satisfy "column != x" for an ordinary user query, and re-semanticing every filter in the repo to fix one caller is not the trade. The defect was that a policy compiler lowered an undeclared column into a filter at all; the matcher now never sees a phantom, and a regression test pins the raw matcher still answering 3 of 3 for the same filter so a later reader can see which half moved. + +**Who is affected.** Only a permission set carrying an RLS policy whose predicate names a column its object does not declare — an authoring mistake `@objectstack/lint` already reports on all of these shapes. For such a policy the object now returns **zero rows** for every holder of the set (read) and refuses every governed insert / update (write), where before a negated spelling returned everything and permitted everything. ⚠️ **An installation relying on such a policy to grant access will lose that access at the upgrade, and that is the intended direction**: what it was "granting" was the absence of enforcement. Correct the column name; the linter names the miss and offers the object's real field list. + +**driver-sql, previously unmeasured, is now measured, and it refines the picture.** On the **read** face `driver-sql` and `driver-sqlite-wasm` never widened — they failed closed by **raising** `INVALID_FILTER` / 400 when the phantom column reached the statement builder, so the read-face defect was driver-dependent (in-process matchers widened; SQL raised). On the **write** face they failed open exactly like every other driver, because the `check` is evaluated in-process and never reaches SQL. After this change both faces answer uniformly on both drivers. `driver-mongodb` remains inferred from the shared ruling rather than measured. + +`@objectstack/lint`'s diagnostic for this miss is corrected in the same change. Its **detection is unchanged** — all the negated shapes were already reported. Its consequence text was stale in one half and misattributed in the other: it described the field miss as having two directions decided by position, and it credited the write leg's fail-closed to a safety net that path never had. It now states one direction for both clauses, and records the older runtime's fail-open write behaviour explicitly so an operator reading it against a deployment that predates this guard is not told the wrong thing. diff --git a/packages/lint/src/validate-rls-predicate-enforceability.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.test.ts index 2600e7bc5f..322837d689 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -623,10 +623,20 @@ 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'); - // ⚠️ 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/); + // ⚠️ ONE direction, and the pin says so on purpose. This block used to + // assert the opposite — that the field half had a fail-OPEN leg decided by + // position — which was true of the runtime at the time and is now false: + // column existence is judged on the COMPILED predicate, so position and + // polarity are normalised away before the check runs. An author told "one + // of these directions is fail-OPEN" would now harden against a hole that + // no longer exists, and would not fix the name. + expect(f.message).not.toMatch(/fail-OPEN/); + expect(f.message).toMatch(/judges column existence on the COMPILED predicate/); + expect(f.message).toMatch(/the position and the polarity you wrote it in make no difference/); + // …and every shape the old text split across two directions is named in + // the one direction, so an author recognises their own predicate in it. + expect(f.message).toMatch(/`field != x`/); + expect(f.message).toMatch(/any arm after the first/); // ⛔ …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 @@ -634,17 +644,19 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not // 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/); + // the cost, which is the same cost the variable half carries + expect(f.message).toMatch(/DROP the policy at request time/); expect(f.message).toMatch(/RLS_DENY_FILTER/); expect(f.message).toMatch(/ZERO rows/); - // 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/); + expect(f.message).toMatch(/DISAPPEARS for every holder of this permission set/); + // ⛔ …and NOT the three claims the rewritten text retired. The + // cross-tenant sentence went with them: it was there to bound a leak + // reading that the message no longer makes, and a denial needs no such + // disclaimer. Overstating the old defect was the risk; restating a bound + // on a defect the text does not describe is just noise. + expect(f.message).not.toMatch(/leaves the policy KEPT/); + expect(f.message).not.toMatch(/DEFEATED/); + expect(f.message).not.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"\?/); @@ -673,10 +685,15 @@ 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/); - // 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/); + // ⚠️ The write leg says the SAME thing the read leg does — one direction — + // and it is the leg whose old text was not merely stale but misattributed: + // it credited a fail-closed to the `extractTargetField` safety net, and + // `computeWriteCheckFilter` never had one. The vacuous-permit sentence + // survives as an explicit statement about an OLDER runtime, so an operator + // reading this against a deployment that predates the guard is not told the + // wrong thing. + expect(f.message).toMatch(/On a runtime older than that guard this clause failed OPEN/); + expect(f.message).toMatch(/PERMITTED exactly the writes the policy was written to refuse/); expect(f.message).not.toMatch(/select \/ update \/ delete matches ZERO/); }); }); @@ -803,23 +820,28 @@ describe('validateRlsPredicateEnforceability — the reference pass never throws }); }); -describe('validateRlsPredicateEnforceability — the fail-OPEN field shapes are reported too', () => { +describe('validateRlsPredicateEnforceability — the once-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. + * hole was 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. + * `extractTargetField` matched only a LEADING `field ==` / `=` / `in`, so for + * each shape below the safety net returned `null`, the policy was KEPT, and + * the phantom column lowered to a negated constraint that a row without that + * column satisfies (`noValueSatisfiesNegation`). Measured on the runtime of + * the day: 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 write path being the worse one because it had no + * field-existence net at all. * - * 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. + * ⚠️ The runtime has since been repaired: `RLSCompiler.compileFilter` judges + * column existence on the COMPILED predicate, which both faces pass through, + * so all five shapes now fail CLOSED. That does NOT retire these cases — + * `noValueSatisfiesNegation` is deliberately unchanged, so the shapes are + * still exactly the ones whose miss used to invert, and DETECTING them is + * still this rule's job. A rule that only caught the leading position would + * satisfy the card and miss the half that was dangerous. */ it.each([ ['a bare negation', 'nope_a != "x"'], diff --git a/packages/lint/src/validate-rls-predicate-enforceability.ts b/packages/lint/src/validate-rls-predicate-enforceability.ts index e1b5efaf0b..03439c52c9 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -502,12 +502,22 @@ function filterFieldPaths(filter: Record | null): Set { /** * 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. + * ⚠️ This text was rewritten once, and the reason it was wrong is worth keeping: + * it said the field half had TWO directions — fail-closed in the leading + * position `extractTargetField` recognises, fail-OPEN everywhere else — and it + * attributed the WRITE leg's fail-closed to that same safety net. Both halves + * were wrong to leave standing. The runtime now judges column existence on the + * COMPILED predicate, inside `RLSCompiler.compileFilter`, which both the read + * layer and the ADR-0058 D4 write gate pass through: position and polarity are + * normalised away before the check runs, so a miss fails CLOSED everywhere, on + * both clauses. And the write path never had an `extractTargetField` net to + * credit — `computeWriteCheckFilter` compiled `check` with no field-existence + * check at all, which is why a negated miss there PERMITTED the write until the + * compiler-side guard landed. + * + * ⇒ Both KINDS now have one direction each, and it is the same direction. What + * this text still owes an author is that the miss is not a harmless typo: it + * turns the policy into a blanket refusal for every holder of the set. */ function referenceConsequence(clause: 'using' | 'check', kind: 'field' | 'variable'): string { if (kind === 'variable') { @@ -529,39 +539,31 @@ function referenceConsequence(clause: 'using' | 'check', kind: 'field' | 'variab '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. 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 + // ── The FIELD half. ONE direction, in every position and every polarity, + // since the runtime moved the column check onto the COMPILED predicate. The + // tracker 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 ' + - '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. ' + - '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`. Repairing that is the runtime\'s job, not this rule\'s — what ' + - 'this diagnostic owes you is WHICH direction your predicate is in.' - ); + const dropped = + '`RLSCompiler` judges column existence on the COMPILED predicate, so the position and the polarity ' + + 'you wrote it in make no difference — a leading `field ==`, a negation (`field != x`, ' + + '`!(field == x)`, `!(field in [...])`) and any arm after the first all lower to the same tree and ' + + 'all DROP the policy at request time, with one WARN line as 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: 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 names a column that is not there. 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. ⚠️ On a runtime older ' + + 'than that guard this clause failed OPEN rather than closed — the write path had no ' + + 'field-existence check at all, so a negated miss was satisfied VACUOUSLY by the post-image and ' + + 'PERMITTED exactly the writes the policy was written to refuse, on every driver. Fix the name ' + + 'rather than relying on either behaviour.'; } /** diff --git a/packages/plugins/plugin-security/src/rls-compiler.ts b/packages/plugins/plugin-security/src/rls-compiler.ts index 3c54ac68dc..c121a2067f 100644 --- a/packages/plugins/plugin-security/src/rls-compiler.ts +++ b/packages/plugins/plugin-security/src/rls-compiler.ts @@ -22,7 +22,7 @@ import type { CelFilterFailReason } from '@objectstack/formula'; * {@link isEmptyMembershipFilter} then refuses — same silent denial, so it * joins the same vocabulary rather than staying unnamed. */ -type RlsDropReason = CelFilterFailReason | 'empty-membership'; +type RlsDropReason = CelFilterFailReason | 'empty-membership' | 'unknown-field'; /** A dropped policy's cause: the compiler's reason plus its human `detail`. */ interface RlsDropCause { @@ -72,6 +72,149 @@ interface RLSUserContext { [key: string]: unknown; } +/** + * The declared-column set a compiled policy is judged against. + * + * Supplied by the caller because only the caller can resolve it (the plugin's + * `getObjectFieldNames`, which prefers ObjectQL's live SchemaRegistry over the + * boot-time metadata artifact). `undefined` means "not resolvable here", and a + * compile with no guard behaves exactly as it did before the guard existed — + * a schema that cannot be loaded must not manufacture denials. + */ +export interface RlsFieldGuard { + /** Every column the object declares, exactly as `getObjectFieldNames` builds it. */ + declared: ReadonlySet; +} + +/** {@link judgeCompiledFields}' answer. */ +type RlsFieldVerdict = + | { ok: true } + | { ok: false; detail: string }; + +/** + * Every column a compiled {@link https://www.mongodb.com/docs Mongo-style} + * FilterCondition NAMES — at any depth, in any polarity. `null` means the tree + * contains a shape this walker does not model, which is a REFUSAL, not an + * empty answer. + * + * ## Why the compiled filter and not the predicate text + * + * This is the whole point of the seam. A source-text matcher has to enumerate + * the spellings of negation (`!=`, `!(…)`, `not in`, an arm after the first), + * and the one it has not been told about is the one that gets through — which + * is the defect one level over, not its repair. The compiled tree has no + * spellings: `cel-to-filter.ts` lowers `!` to `$not`, `||` to `$or`, `&&` to + * `$and`, and every column it names lands as a plain object key whatever + * position the author wrote it in. Walking that is positional-agnostic by + * construction, and it stays correct when the pushdown compiler learns a new + * source form, because a new form still has to lower into this same shape. + * + * ## The rules, and where they differ from the ingress collector + * + * Two collectors already answer "which columns does this filter name" — + * `collectFilterFieldKeys` (`@objectstack/metadata-protocol`) and + * `collectFilterFieldNames` (`@objectstack/objectql`). This one keeps their + * first rule verbatim and INVERTS their second, deliberately: + * + * - **`$and` / `$or` / `$not` are combinators, recursed into; a non-`$` key is + * a column.** Same as both. + * - ⚠️ **Any OTHER `$`-prefixed key at NODE level REFUSES here** (`null`), + * where the ingress collectors skip it without descending. Their direction is + * right for a gate that must not invent 400s on caller input; it is wrong + * here, because the input is not caller input — it is THIS compiler's own + * output, which emits `$and` / `$or` / `$not` and nothing else at node level. + * An unmodelled combinator therefore means the tree grew a shape this guard + * has not been taught, and leaving the columns beneath it unexamined is + * precisely the fail-open this guard exists to close. + * - **A field key's value is scanned only for `{ $field: … }` references** — + * the field-to-field comparison `cel-to-filter.ts` emits — which name a + * second real column. Other nested keys are a cross-object condition the RLS + * compiler refuses at lowering time (`classify` throws `unsupported` on any + * `.`-chain), so they cannot appear; they are ignored rather than refused so + * this walker never denies on a shape it merely does not produce. + */ +function collectRlsFilterColumns( + node: unknown, + out: Set = new Set(), + depth = 0, +): Set | null { + // A self-referential filter must not hang the read path. Unlike the ingress + // collector — which returns what it has — overrunning the bound REFUSES, + // because an unexamined subtree is an unguarded column. + if (depth > 32) return null; + if (node === null || typeof node !== 'object' || Array.isArray(node)) return null; + for (const [key, value] of Object.entries(node as Record)) { + if (key === '$and' || key === '$or') { + if (!Array.isArray(value)) return null; + for (const arm of value) { + if (collectRlsFilterColumns(arm, out, depth + 1) === null) return null; + } + continue; + } + if (key === '$not') { + if (collectRlsFilterColumns(value, out, depth + 1) === null) return null; + continue; + } + if (key.startsWith('$')) return null; + out.add(key); + collectFieldReferences(value, out, depth + 1); + } + return out; +} + +/** Collect `{ $field: 'other_column' }` right-hand references out of an operator bag. */ +function collectFieldReferences(spec: unknown, out: Set, depth: number): void { + if (depth > 32 || spec === null || typeof spec !== 'object') return; + if (Array.isArray(spec)) { + for (const item of spec) collectFieldReferences(item, out, depth + 1); + return; + } + for (const [key, value] of Object.entries(spec as Record)) { + if (key === '$field') { + if (typeof value === 'string' && value !== '') out.add(value); + continue; + } + collectFieldReferences(value, out, depth + 1); + } +} + +/** + * Does every column this compiled policy names exist on the object? + * + * The security question underneath: a predicate naming a column the object does + * not declare cannot narrow anything, and in a NEGATION-carrying position it + * does not merely fail to narrow — it WIDENS, because a row that has no such + * column satisfies "column != x" under the settled include-direction ruling + * (`noValueSatisfiesNegation`, driver-memory / driver-mongodb). That ruling is + * correct for an ordinary user query and is deliberately untouched; what is + * wrong is lowering an undeclared column into a filter AT ALL from a policy + * compiler. So the phantom is stopped here, before any matcher sees it. + */ +function judgeCompiledFields( + filter: Record, + guard: RlsFieldGuard, +): RlsFieldVerdict { + const named = collectRlsFilterColumns(filter); + if (named === null) { + return { + ok: false, + detail: + 'the compiled predicate contains a filter shape this guard does not model, so the columns it ' + + `names could not be enumerated and the policy was refused rather than trusted (compiled to ${JSON.stringify(filter)})`, + }; + } + const missing = [...named].filter((column) => !guard.declared.has(column)); + if (missing.length === 0) return { ok: true }; + return { + ok: false, + detail: + `the predicate names ${missing.length === 1 ? 'a column' : 'columns'} the object does not declare ` + + `(${missing.map((m) => `"${m}"`).join(', ')}), so it cannot narrow anything; in a negation-carrying ` + + 'position it would instead WIDEN the policy to every row the tenant wall admits, so the policy was ' + + 'refused', + }; +} + /** * Sentinel filter used when applicable RLS policies exist but none can * be compiled against the current execution context (typically because a @@ -261,11 +404,22 @@ export class RLSCompiler { * active organization). The caller must treat this as "deny by * default" — its `id` comparison naturally yields zero rows on * select/update/delete, which is the safe fail-closed answer. + * + * `fieldGuard` is the object's declared-column set. When supplied, a policy + * whose compiled predicate names a column the object does not declare is + * DROPPED — in every position and every polarity, judged on the compiled + * tree rather than on the predicate's text ({@link judgeCompiledFields}) — + * and joins the same fail-closed path as any other dropped policy. It is the + * ONE seam both faces pass through: the read layer compiles `using` here and + * the ADR-0058 D4 write gate compiles `check` here, so the two can no longer + * disagree about what a phantom column means. Omit it and the compile behaves + * exactly as it did before the guard existed. */ compileFilter( policies: RowLevelSecurityPolicy[], executionContext?: ExecutionContext, clause: 'using' | 'check' = 'using', + fieldGuard?: RlsFieldGuard, ): Record | null { if (policies.length === 0) return null; @@ -314,7 +468,17 @@ export class RLSCompiler { applicable++; const outcome = this.compileExpressionOutcome(predicate, userCtx); if (outcome.filter) { - filters.push(outcome.filter); + // Field existence, judged on the COMPILED tree so no negation spelling + // can route around it. A policy naming an undeclared column joins + // `deniedBy` exactly as an unresolved variable does — same collection, + // same fail-closed sentinel below, same WARN line — because it is the + // same class of fault: an applicable policy that cannot enforce. + const verdict = fieldGuard ? judgeCompiledFields(outcome.filter, fieldGuard) : null; + if (verdict && !verdict.ok) { + deniedBy.push({ policy, cause: { reason: 'unknown-field', detail: verdict.detail } }); + } else { + filters.push(outcome.filter); + } } else if (!isSupportedRlsExpression(predicate)) { // ADR-0056 D4: an UNSUPPORTED-SHAPE predicate (e.g. arithmetic, functions, // subqueries) compiles to nothing and would silently vanish, leaving the diff --git a/packages/plugins/plugin-security/src/rls-phantom-column-negation.test.ts b/packages/plugins/plugin-security/src/rls-phantom-column-negation.test.ts new file mode 100644 index 0000000000..e59deb8cb7 --- /dev/null +++ b/packages/plugins/plugin-security/src/rls-phantom-column-negation.test.ts @@ -0,0 +1,376 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17042] An RLS predicate naming an UNDECLARED column must deny — in every + * position, in every polarity, on BOTH faces. + * + * ## What was measured on `origin/main` @ `91f65c4ea`, before the fix + * + * A predicate naming a column the object does not declare cannot narrow. In a + * NEGATION-carrying position it did not deny either: it WIDENED. Two + * independent sites, each with its own reason, each measured against the same + * two controls — a real column must still narrow, and the SAME phantom column + * in a POSITIVE position must still refuse. A cell whose controls do not + * discriminate is not a reading: the first run of this measurement "refused" + * every write because the driver had failed to boot, and only the controls + * said so. + * + * READ face — `extractTargetField` is a LEADING `==` / `=` / `in` shape match, + * so a negated predicate returned `null`, `if (!targetField) return true` KEPT + * the policy, `dropped` never incremented and the deny sentinel never armed. + * The kept filter then met the settled include-direction ruling + * (`noValueSatisfiesNegation`, driver-memory / driver-mongodb, #13166): a row + * that HAS no such column satisfies "column != x". + * + * `nope != "x"` → `{nope:{$ne:'x'}}` 3 / 3 + * `!(nope == 1)` → `{$not:{nope:1}}` 3 / 3 + * `!(nope in ['a'])` → `{$not:{nope:{$in:['a']}}}` 3 / 3 + * `is_private == false || nope != "x"` → `{$or:[…]}` 3 / 3 + * control `is_private == false` 1 / 3 + * control `nope == false` (positive phantom) 0 / 3 + * + * WRITE face — `computeWriteCheckFilter` compiled `check` clauses with NO + * field-existence net at all, and step 3.6 (ADR-0058 D4) evaluates that filter + * against the post-image. Every negated phantom PERMITTED the write the policy + * was authored to refuse, on both drivers, in both post-image polarities; the + * positive phantom refused, by accident of `looseEq(undefined, value)` being + * false — which is why a suite that only ever tested the positive shape stayed + * green over the hole. + * + * ⚠️ driver-sql and driver-sqlite-wasm were NOT MEASURED when the card was + * filed. They are now: on the READ face they do NOT widen — they fail closed by + * RAISING `INVALID_FILTER` / 400 — so the read face is driver-DEPENDENT. On the + * WRITE face they fail open exactly like every other driver, because the + * `check` is evaluated in-process and never reaches SQL. The write face is the + * worse one and no driver choice mitigates it. + * + * ## What this file pins + * + * The repair is ONE seam: `RLSCompiler.compileFilter` — the single choke point + * both faces already pass through — judges every column the policy names on the + * COMPILED FilterCondition tree. That is positional-agnostic by construction: + * the pushdown compiler lowers `!` to `$not`, `||` to `$or` and `&&` to `$and`, + * so a column lands as a plain object key whatever position it was authored in, + * and there is no spelling of negation left for a shape match to miss. + * + * ⛔ `noValueSatisfiesNegation` is NOT touched and must not be: it is correct + * for an ordinary user query, and re-semanticing every filter in the repo to + * fix one caller is the blast radius this card must not take. The first + * describe below pins that directly — the raw matcher STILL admits 3 of 3 rows + * for the same filter — so a future reader can see that what changed is that + * the policy compiler stopped PRODUCING the filter, not what the matcher does + * with one. + * + * ⭐ The load-bearing control is "a real column still narrows". A fix that made + * every policy deny would pass a naive red→green and break every install, so it + * is asserted on both faces and in both directions on the write face. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { matchesFilterCondition } from '@objectstack/formula'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { RLSCompiler, RLS_DENY_FILTER } from './rls-compiler.js'; +import { SecurityPlugin } from './security-plugin.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +// ── the fixture, transcribed from the card: three rows, one of them public ── + +const ROWS: Array> = [ + { id: 'r1', title: 'one', is_private: false, owner: 'a@e.example' }, + { id: 'r2', title: 'two', is_private: true, owner: 'b@e.example' }, + { id: 'r3', title: 'three', is_private: true, owner: 'c@e.example' }, +]; + +/** Exactly the columns `qa_doc` declares. `nope` is deliberately absent. */ +const DECLARED: ReadonlySet = new Set(['id', 'title', 'is_private', 'owner']); + +const OBJECTS = [ + { + name: 'qa_doc', + label: 'Doc', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + title: { name: 'title', type: 'text' }, + is_private: { name: 'is_private', type: 'boolean' }, + owner: { name: 'owner', type: 'text' }, + }, + }, +]; + +/** + * The four shapes the card measured. Each names `nope`, which `qa_doc` does not + * declare, in a position `extractTargetField` does not recognise. + */ +const PHANTOM_NEGATIONS: Array<[label: string, cel: string]> = [ + ['a bare `!=`', 'nope != "x"'], + ['`!` over an equality', '!(nope == 1)'], + ['`!` over a membership', '!(nope in ["a"])'], + ['a trailing `||` arm, past the leading shape match', 'is_private == false || nope != "x"'], +]; + +/** The narrowing an author actually meant — the over-fix control. */ +const REAL_COLUMN = 'is_private == false'; +/** The same phantom column in a POSITIVE position: refused before and after. */ +const PHANTOM_POSITIVE = 'nope == false'; + +const policy = (clause: 'using' | 'check', cel: string): never => + ({ object: 'qa_doc', operation: 'select', [clause]: cel }) as never; + +const CTX = { userId: 'usr_a', tenantId: 'org1', positions: ['reader'] } as never; + +// ── 1. the matcher ruling, pinned as UNCHANGED ───────────────────────────── + +describe('[#17042] the include-direction ruling is untouched — the compiler stopped producing the filter', () => { + it('a row with no such column STILL satisfies a negation at the matcher (3 of 3)', () => { + // ⛔ This is `noValueSatisfiesNegation`'s reading (#13166), shared with + // driver-mongodb, and it is deliberately preserved: it is correct for an + // ordinary user query. If this cell ever goes to 0 the fix was applied in + // the wrong place and every filter in the repo has been re-semanticed. + for (const filter of [ + { nope: { $ne: 'x' } }, + { $not: { nope: 1 } }, + { $not: { nope: { $in: ['a'] } } }, + { $or: [{ is_private: false }, { nope: { $ne: 'x' } }] }, + ]) { + expect(ROWS.filter((r) => matchesFilterCondition(r as never, filter as never)).length).toBe(3); + } + // …and the two controls the 3/3 depends on to be a reading. + expect(ROWS.filter((r) => matchesFilterCondition(r as never, { is_private: false } as never)).length).toBe(1); + expect(ROWS.filter((r) => matchesFilterCondition(r as never, { nope: false } as never)).length).toBe(0); + }); +}); + +// ── 2. the seam itself ───────────────────────────────────────────────────── + +describe('[#17042] RLSCompiler.compileFilter — a phantom column denies in every position', () => { + const compiler = new RLSCompiler(); + const guard = { declared: DECLARED }; + + for (const clause of ['using', 'check'] as const) { + for (const [label, cel] of PHANTOM_NEGATIONS) { + it(`${clause}: ${label} → RLS_DENY_FILTER`, () => { + const filter = compiler.compileFilter([policy(clause, cel)], CTX, clause, guard); + expect(filter).toEqual(RLS_DENY_FILTER); + // The sentinel is a REFUSAL, and a refusal admits nothing. + expect(ROWS.filter((r) => matchesFilterCondition(r as never, filter as never)).length).toBe(0); + }); + } + + it(`${clause}: the SAME phantom in a positive position also denies`, () => { + expect(compiler.compileFilter([policy(clause, PHANTOM_POSITIVE)], CTX, clause, guard)).toEqual(RLS_DENY_FILTER); + }); + + it(`${clause}: ⭐ a REAL column still narrows — the over-fix control`, () => { + const filter = compiler.compileFilter([policy(clause, REAL_COLUMN)], CTX, clause, guard); + expect(filter).toEqual({ is_private: false }); + expect(ROWS.filter((r) => matchesFilterCondition(r as never, filter as never)).length).toBe(1); + }); + } + + it('a phantom named on the RIGHT of a field-to-field comparison denies too', () => { + // `cel-to-filter` emits `{ title: { $eq: { $field: 'nope' } } }` here, so + // the column is not a key at all — the walker has to find it inside the + // operator bag. A guard that only read node keys would miss this. + expect(compiler.compileFilter([policy('using', 'title == nope')], CTX, 'using', guard)).toEqual(RLS_DENY_FILTER); + // control: the same shape between two REAL columns compiles. + expect(compiler.compileFilter([policy('using', 'title == owner')], CTX, 'using', guard)).not.toEqual(RLS_DENY_FILTER); + }); + + it('a policy naming a phantom does not survive by riding alongside a valid sibling', () => { + // The valid sibling still grants, so this is NOT a deny — but the phantom + // arm must contribute nothing rather than OR-ing in an allow-all. + const filter = compiler.compileFilter( + [policy('using', REAL_COLUMN), policy('using', 'nope != "x"')], + CTX, + 'using', + guard, + ); + expect(filter).toEqual({ is_private: false }); + expect(ROWS.filter((r) => matchesFilterCondition(r as never, filter as never)).length).toBe(1); + }); + + it('⛔ WITHOUT a guard the compile is byte-identical to before — a schema that will not load makes no denials', () => { + // `getObjectFieldNames` answers `null` at boot before the registry is + // populated. That must keep every policy, exactly as it did, rather than + // manufacturing a deny for an object whose columns are simply not known yet. + for (const [, cel] of PHANTOM_NEGATIONS) { + expect(compiler.compileFilter([policy('using', cel)], CTX)).not.toEqual(RLS_DENY_FILTER); + } + }); +}); + +// ── 3. both faces, end to end, on both drivers ───────────────────────────── + +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; +const SYS_CTX = { isSystem: true, userId: 'usr_system' }; +const CALLER = { + userId: 'usr_a', + email: 'a@e.example', + positions: ['reader'], + permissions: ['qa_reader'], + posture: 'MEMBER', +}; + +const engines: ObjectQL[] = []; +afterEach(async () => { + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +function permissionSet(using: string, check?: string): PermissionSet { + return PermissionSetSchema.parse({ + name: 'qa_reader', + objects: { qa_doc: { allowRead: true, allowCreate: true, allowEdit: true } }, + rowLevelSecurity: [ + { name: 'qa_doc_policy', object: 'qa_doc', operation: 'all', using, ...(check ? { check } : {}) }, + ], + }); +} + +async function boot(makeDriver: () => unknown, ps: PermissionSet): Promise { + const engine = new ObjectQL(); + engine.registerDriver(makeDriver() as never, true); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.rls-phantom-column-negation-17042', + name: 'RLS phantom column negation', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: OBJECTS, + } as never); + await engine.syncSchemas(); + engines.push(engine); + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (_type: string, name: string) => engine.getSchema(name) ?? null, + list: async () => [MEMBER_DEFAULT, ps], + }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx as never); + await plugin.start(ctx as never); + // The expected refusals log at WARN through the engine's own logger. + vi.spyOn((engine as unknown as { logger: { warn: () => void } }).logger, 'warn').mockImplementation(() => undefined); + await engine.insert('qa_doc', ROWS as never, { context: SYS_CTX } as never); + return engine; +} + +const DRIVERS: Array<[string, () => unknown]> = [ + ['driver-sql (better-sqlite3 :memory:)', + () => new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never)], + ['driver-sqlite-wasm (:memory:)', () => new SqliteWasmDriver({ filename: ':memory:' } as never)], +]; + +interface Outcome { ok: boolean; code?: string; status?: number } + +const attempt = async (run: () => Promise): Promise => { + try { + await run(); + return { ok: true }; + } catch (e) { + const err = e as { code?: string; statusCode?: number; status?: number }; + return { ok: false, code: err.code, status: err.statusCode ?? err.status }; + } +}; + +for (const [driverName, makeDriver] of DRIVERS) { + describe(`[#17042] READ face, end to end — ${driverName}`, () => { + for (const [label, cel] of PHANTOM_NEGATIONS) { + it(`${label} → zero rows, and no raise`, async () => { + const engine = await boot(makeDriver, permissionSet(cel)); + // ⚠️ Before the fix this THREW `INVALID_FILTER` / 400 on both SQL + // drivers — the phantom column reached the statement builder. A zero + // here is the deny sentinel doing its job, and the controls below are + // what separate it from "the harness returns nothing". + const rows = (await engine.find('qa_doc', { context: CALLER } as never)) as unknown[]; + expect(rows).toHaveLength(0); + }); + } + + it('⭐ a REAL column still narrows to 1 of 3 — the over-fix control', async () => { + const engine = await boot(makeDriver, permissionSet(REAL_COLUMN)); + const rows = (await engine.find('qa_doc', { context: CALLER } as never)) as Array>; + expect(rows).toHaveLength(1); + expect(rows[0]!.id).toBe('r1'); + }); + + it('the same phantom in a positive position is still zero rows', async () => { + const engine = await boot(makeDriver, permissionSet(PHANTOM_POSITIVE)); + expect((await engine.find('qa_doc', { context: CALLER } as never)) as unknown[]).toHaveLength(0); + }); + }); + + describe(`[#17042] WRITE face, end to end — ${driverName}`, () => { + /** + * ⚠️ A SINGLE object, never an array. Step 3.6 is guarded by + * `!Array.isArray(opCtx.data)`, so a bulk payload skips the check gate + * entirely and every cell below would read "permitted" for a reason that + * has nothing to do with this card. + */ + const insert = (engine: ObjectQL, isPrivate: boolean) => + engine.insert( + 'qa_doc', + { id: 'w1', title: 'w', is_private: isPrivate, owner: 'a@e.example' } as never, + { context: CALLER } as never, + ); + + const stored = async (engine: ObjectQL) => + ((await engine.find('qa_doc', { where: { id: 'w1' }, context: SYS_CTX } as never)) as unknown[]).length; + + for (const [label, cel] of PHANTOM_NEGATIONS) { + for (const isPrivate of [false, true]) { + it(`check ${label}, post-image is_private=${isPrivate} → refused, nothing stored`, async () => { + const engine = await boot(makeDriver, permissionSet(REAL_COLUMN, cel)); + const outcome = await attempt(() => insert(engine, isPrivate)); + // Asserted on the ADR-0112 envelope, never on a bare throw: a driver + // raising a raw `Error` would satisfy `toThrow()` and prove nothing. + expect(outcome.ok).toBe(false); + expect(outcome.code).toBe('PERMISSION_DENIED'); + expect(outcome.status).toBe(403); + // "the gate refused" and "nothing landed" are separate facts. + expect(await stored(engine)).toBe(0); + }); + } + } + + it('⭐ a REAL column check still ADMITS the post-image that satisfies it', async () => { + const engine = await boot(makeDriver, permissionSet(REAL_COLUMN, REAL_COLUMN)); + const outcome = await attempt(() => insert(engine, false)); + expect(outcome.ok).toBe(true); + expect(await stored(engine)).toBe(1); + }); + + it('⭐ …and still REFUSES the post-image that violates it', async () => { + const engine = await boot(makeDriver, permissionSet(REAL_COLUMN, REAL_COLUMN)); + const outcome = await attempt(() => insert(engine, true)); + expect(outcome.ok).toBe(false); + expect(outcome.code).toBe('PERMISSION_DENIED'); + expect(await stored(engine)).toBe(0); + }); + + it('the same phantom in a positive position is still refused', async () => { + const engine = await boot(makeDriver, permissionSet(REAL_COLUMN, PHANTOM_POSITIVE)); + const outcome = await attempt(() => insert(engine, false)); + expect(outcome.ok).toBe(false); + expect(outcome.code).toBe('PERMISSION_DENIED'); + expect(await stored(engine)).toBe(0); + }); + }); +} diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 29b0eb04a5..b6dc708a7c 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -6016,9 +6016,32 @@ export class SecurityPlugin implements Plugin { ? collected.filter((p) => !isPlatformOwnershipFloorPolicy(p)) : collected; if (allRlsPolicies.length > 0) { - // Field-existence safety: a wildcard policy targeting a column the object - // lacks is a *deny* contribution (fail-closed), unless the object opted - // out of tenancy (skip). Schema-lookup failure keeps all policies. + // Field-existence safety, in TWO passes that answer two different + // questions. Schema-lookup failure runs neither: a schema that cannot + // be loaded must not manufacture denials. + // + // Pass 1, HERE — the ADR-0095 delta c carve-out, and only that. A + // wildcard `organization_id`-leading policy on an object that opted OUT + // of tenancy is NOT APPLICABLE to it, so it is skipped WITHOUT + // contributing to the deny sentinel; a leading miss on any other column + // is a deny contribution. `extractTargetField`'s leading `==` / `=` / + // `in` shape match is exactly the right instrument for that carve-out, + // because the shape it recognises is the shape the platform's own + // tenant policy is authored in. + // + // Pass 2, in the COMPILER — every column named ANYWHERE in the + // predicate, in any position and any polarity. ⚠️ This pass is what + // pass 1 cannot do and must not be widened to do: a predicate naming an + // undeclared column in a NEGATION-carrying position (`nope != "x"`, + // `!(nope == 1)`, `!(nope in […])`, or a trailing `||` arm) returned + // `null` from `extractTargetField`, so the policy was KEPT, `dropped` + // never incremented, the sentinel never armed — and a row that has no + // such column SATISFIES the negation, so the authored narrowing widened + // to every row inside the tenant wall instead of denying. Teaching the + // regex more spellings would only move the same defect one level over; + // the compiler judges the compiled tree, where a negation is a `$not` + // node and has no spelling left to hide behind. The matcher's + // include-direction ruling is untouched — it never sees the phantom. let dropped = 0; const compilable = objectFields ? allRlsPolicies.filter((p) => { @@ -6032,7 +6055,12 @@ export class SecurityPlugin implements Plugin { return false; }) : allRlsPolicies; - layer1 = this.rlsCompiler.compileFilter(compilable, context); + layer1 = this.rlsCompiler.compileFilter( + compilable, + context, + 'using', + objectFields ? { declared: objectFields } : undefined, + ); // Every applicable policy dropped for a missing field → deny sentinel. if (layer1 == null && dropped > 0) { layer1 = { ...RLS_DENY_FILTER }; @@ -6266,7 +6294,26 @@ export class SecurityPlugin implements Plugin { // throwing resolver or an unresolved key still drop the policy and still // fail closed, on this path as on the read path. await this.stageRlsMembership(context); - return this.rlsCompiler.compileFilter(withCheck, context, 'check'); + // [#17042] The write face's field-existence net — which this path had NONE + // of. `computeWriteCheckFilter` compiled `check` clauses with no column + // check at all, so a phantom column in a negated position + // (`nope != "x"`, `!(nope == 1)`, `!(nope in […])`, a trailing `||` arm) + // was satisfied VACUOUSLY by the post-image and PERMITTED the write the + // policy was authored to refuse — measured on both SQL drivers, the write + // being driver-independent because the check is evaluated in-process + // against the post-image (`matchesFilterCondition`, step 3.6). Only a + // POSITIVE phantom refused, by accident of `looseEq(undefined, value)` + // being false, which is why the hole was invisible to a suite that tested + // the positive shape. The guard is the SAME one the read layer passes, so + // the two faces can never again disagree about what an undeclared column + // means. `null` (schema not loadable) passes no guard and behaves as before. + const objectFields = await this.getObjectFieldNames(this.metadata, object, this.ql); + return this.rlsCompiler.compileFilter( + withCheck, + context, + 'check', + objectFields ? { declared: objectFields } : undefined, + ); } /** @@ -7650,7 +7697,20 @@ export class SecurityPlugin implements Plugin { /** * Extract the left-hand field name from a simple RLS expression like * `field = current_user.x` or `field IN (current_user.y)`. Returns - * `null` for unsupported shapes (in which case we keep the policy). + * `null` for unsupported shapes. + * + * ⚠️ [#17042] What a `null` MEANS here changed, and the change is the whole + * point: this is no longer the field-existence net, it is the ADR-0095 delta + * c carve-out's instrument. `null` still means "this policy is not the + * leading-`organization_id` shape the carve-out is about", so the caller + * keeps it — but the policy is now judged for column existence a second time + * inside `RLSCompiler.compileFilter`, on the COMPILED tree, where position + * and polarity have been normalised away. ⛔ Do not widen this regex to + * recognise `!=` / `!` / `not in`: a shape match that must enumerate every + * spelling of negation is the same "recognises only what it was told about" + * defect the compiler-side pass exists to end, and widening it here would + * ALSO turn the carve-out into a denial for the tenancy-disabled case it + * deliberately skips. */ private extractTargetField(using?: string): string | null { if (!using) return null;