fix(security)!: an RLS predicate naming an undeclared column denies in every position

.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)
+
+<!-- adr-0087: not-required (no-migration-prescription) an enforcement change with no authorable surface behind it: no Zod schema, no metadata key, no spelling and no stored representation moves, so a stored `sys_permission_set` row needs no conversion and `objectstack migrate meta` has nothing to rewrite. What changes is whether an existing predicate is ENFORCED or silently dropped; the remedy for a newly-denied policy is to correct the column name in the policy, which is authoring, not migration — and the authoring-time detector for exactly that mistake is already shipped in `@objectstack/lint`. -->
+
+**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.

packages/lint/src/validate-rls-predicate-enforceability.test.ts

@@ -623,28 +623,40 @@ 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
     // 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/);
+    // 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"'],