diff --git a/.changeset/rls-predicate-authoring-gate.md b/.changeset/rls-predicate-authoring-gate.md new file mode 100644 index 0000000000..7d1750c290 --- /dev/null +++ b/.changeset/rls-predicate-authoring-gate.md @@ -0,0 +1,75 @@ +--- +"@objectstack/formula": minor +"@objectstack/lint": minor +"@objectstack/plugin-security": patch +--- + +feat(formula,lint): wire ADR-0056 D4's RLS authoring gate, from the runtime's own predicate (#4983) + +`isSupportedRlsExpression` has carried the same docblock since ADR-0056 D4: +"exposed so an authoring-time gate (`objectstack compile`) can REJECT a +predicate the runtime would silently drop … A `false` here means 'this +predicate will never enforce'." It had **no non-test consumer anywhere** — the +function written to fix declared-but-never-read was itself declared and never +read. This lands the consumer, in two steps that had to happen in this order. + +**1. `sqlPredicateToCel` and `isSupportedRlsExpression` move FROM +`@objectstack/plugin-security` (`src/rls-compiler.ts`) TO `@objectstack/formula` +(`src/rls-predicate.ts`), and are exported from its root.** Executable code +unchanged — a change of address, not of behaviour; `plugin-security` now imports +them from `@objectstack/formula` and keeps no copy, so there is still exactly +one definition. No import path outside the two packages changes: neither symbol +was ever exported from `@objectstack/plugin-security`'s entry point. The move is +what makes step 2 possible at all — `@objectstack/lint` may depend on +`@objectstack/spec` and never on a runtime, so with the predicate living in a +runtime the gate's only other door was copying the SQL→CEL bridge, whose +boundary conditions (quoted literals are never rewritten; canonical CEL passes +through unchanged) *are* the gate's red/green line. A fork drifting by one +character rejects policies the runtime executes correctly — the false-positive +direction, which is worse than the gap. ADR-0058 D1 asks for a single canonical +shape gate; the bridge is part of that gate. + +**2. New `@objectstack/lint` rule `validateRlsPredicateEnforceability`, +`error`, on all three authoring commands**, over +`permissions[].rowLevelSecurity[].using` and `.check`: + +- **`rls-predicate-unenforceable`** — parses as CEL, outside the pushdown + subset: a function call (`size(...)`, `has(...)`), arithmetic, a ternary, a + cross-object path (`record.account.region`). +- **`rls-predicate-unparseable`** — does not parse as CEL even after the legacy + SQL bridge (`=` → `==`, `IN` → `in`): SQL `AND` / `OR` / `LIKE`, a subquery. + Its own id because the fix is different — write CEL (`&&`, `||`), not a + different shape. + +What the gate prevents, measured through `plugin-security` rather than inferred: +`RLSCompiler` drops the policy and logs one request-time WARN. On the read path, +when it is the only applicable policy, `compileFilter` returns the +`RLS_DENY_FILTER` sentinel instead, which is AND-ed onto the where clause — so +every select / update / delete on the object matches **zero rows**. On the +ADR-0058 D4 write path the post-image `check` becomes that same sentinel, which +no record satisfies, so every insert / update fails with `PermissionDeniedError`. +The runtime fails closed, which is why this was survivable: the result is not a +hole but a policy that reads as an authorization and behaves as a blanket +refusal, with nothing at authoring time pointing at the line that caused it. + +Fix a flagged predicate by rewriting it inside the lowerable subset — `==` `!=` +`>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, and +`startsWith` / `endsWith` / `contains` over single-column field paths (ADR-0058 +D2), against a literal or a `current_user.*` value. Two specific migrations: +`has(x)` / `size(x) > 0` → `x != null` (a function call is correct in an object +*validation* rule, which is interpreted, and wrong here, where the predicate is +compiled to a filter); and a related record's field → denormalise it onto this +object (formula/rollup) and test that column, since RLS cannot join (ADR-0055). + +Same construction as the sharing-rule gate (#4698): the rule does not model the +consumer or grep for it — it calls `isSupportedRlsExpression`, the exact +function `RLSCompiler.compileFilter` consults to decide whether a dropped policy +earns its warning, so the two verdicts are one boolean by construction, pinned +in both directions over a shared corpus. Measured before shipping: every RLS +predicate declared anywhere in this repo — the `plugin-security` platform seeds, +the examples, the dogfood fixtures, the authoring skill — is supported, so the +gate turns nothing red that works today. Unlike the sharing-rule gate, CEL +*syntax* is reported here rather than deferred to `expression-invalid`: +`validateStackExpressions` does not walk `rowLevelSecurity` at all, and could not +judge this field correctly if it did, because `owner_id = current_user.id` is a +CEL syntax error and a working RLS predicate at the same time. diff --git a/packages/formula/src/index.ts b/packages/formula/src/index.ts index 08f5c3ecad..f88eebc92a 100644 --- a/packages/formula/src/index.ts +++ b/packages/formula/src/index.ts @@ -25,6 +25,13 @@ export { normalizeExpression, normalizeExpressionTree } from './normalize'; // and plugin-sharing; honours ADR-0055 (no subquery / no cross-object traversal). export { compileCelToFilter, isPushdownableCel, lowerCelAst } from './cel-to-filter'; export type { CelFilterCompileResult, CelFilterCompileOptions, CelFilterFailReason } from './cel-to-filter'; +// ADR-0056 D4 / ADR-0058 D1 — the RLS predicate shape gate and its legacy +// SQL→CEL bridge. Hoisted out of plugin-security in #4983 so the runtime that +// enforces the predicate and the authoring gate that rejects it share ONE +// definition: `@objectstack/lint` may depend on this package and never on a +// runtime, so the alternative was forking the bridge, whose `=`/`IN` boundary +// conditions ARE the red/green line. +export { isSupportedRlsExpression, sqlPredicateToCel } from './rls-predicate'; export { matchesFilterCondition } from './matches-filter'; // ADR-0032 — shared validator + introspection (one validator for build, // registration, and the agent-callable validate_expression tool). diff --git a/packages/formula/src/rls-predicate.test.ts b/packages/formula/src/rls-predicate.test.ts new file mode 100644 index 0000000000..9cc5d11bea --- /dev/null +++ b/packages/formula/src/rls-predicate.test.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The unit tests that travelled with `isSupportedRlsExpression` / + * `sqlPredicateToCel` when #4983 hoisted them out of + * `@objectstack/plugin-security` (`security-plugin.test.ts`, describe block + * "RLSCompiler D4 — uncompilable predicates are surfaced"). The two shape cases + * are reproduced VERBATIM below: the hoist is a change of address, so a moved + * test that also changes its assertions would hide the one thing the move has + * to prove. The consumer-side half — that `RLSCompiler` still warns, still + * fails closed, and still agrees with this predicate — stayed in + * plugin-security, where the consumer is. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { isSupportedRlsExpression, sqlPredicateToCel } from './rls-predicate'; +import { isPushdownableCel } from './cel-to-filter'; + +// --------------------------------------------------------------------------- +// ADR-0056 D4 — RLS predicates that won't compile must not vanish in silence +// (moved verbatim from plugin-security/src/security-plugin.test.ts, #4983) +// --------------------------------------------------------------------------- +describe('isSupportedRlsExpression — the ADR-0056 D4 shape gate', () => { + it('isSupportedRlsExpression accepts the compilable shapes', () => { + // Legacy SQL-ish subset (bridged `=`/`IN`). + expect(isSupportedRlsExpression('owner_id = current_user.id')).toBe(true); + expect(isSupportedRlsExpression('owner = current_user.email')).toBe(true); + expect(isSupportedRlsExpression("status = 'published'")).toBe(true); + expect(isSupportedRlsExpression('id IN (current_user.org_user_ids)')).toBe(true); + expect(isSupportedRlsExpression('1 = 1')).toBe(true); + // ADR-0058: the canonical compiler lowers a broader pushdown subset, so the + // shape gate now (correctly) reports these as enforceable — `==`/`!=`, + // comparisons, and CEL compound predicates all compile to a FilterCondition. + expect(isSupportedRlsExpression('owner == current_user.id')).toBe(true); // `==` + expect(isSupportedRlsExpression('amount > 100')).toBe(true); // comparison + expect(isSupportedRlsExpression('region != null')).toBe(true); // null check + expect(isSupportedRlsExpression('a == 1 && b == 2')).toBe(true); // CEL compound + }); + + it('isSupportedRlsExpression rejects genuinely non-pushdownable shapes', () => { + // These cannot lower to a FilterCondition for ANY input, so the gate must + // reject them (ADR-0055 / ADR-0056 D4) — they fail closed at runtime. + expect(isSupportedRlsExpression('a = current_user.id AND b = 1')).toBe(false); // SQL AND ≠ CEL && (unparseable) + expect(isSupportedRlsExpression('amount + 1 > 2')).toBe(false); // arithmetic + expect(isSupportedRlsExpression('id IN (SELECT id FROM users)')).toBe(false); // subquery + expect(isSupportedRlsExpression('record.a.b == 1')).toBe(false); // cross-object traversal + expect(isSupportedRlsExpression('')).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// The bridge's boundary conditions — the reason a COPY of it was unacceptable +// --------------------------------------------------------------------------- +// +// `sqlPredicateToCel` is a regex rewrite, and its edge cases are precisely the +// red/green line of the authoring gate built on it (#4983). A second +// implementation drifting by one character would make `os validate` reject +// policies the runtime executes correctly — the false-positive direction, which +// is worse than the gap. Pinning them here is what makes ONE definition worth +// insisting on. + +describe('sqlPredicateToCel — the legacy bridge, pinned at its boundaries', () => { + it('rewrites the historically-supported SQL subset', () => { + expect(sqlPredicateToCel('owner_id = current_user.id')).toBe('owner_id == current_user.id'); + expect(sqlPredicateToCel('id IN (current_user.org_user_ids)')).toBe('id in (current_user.org_user_ids)'); + expect(sqlPredicateToCel('1 = 1')).toBe('1 == 1'); + }); + + it('never rewrites inside a quoted string literal', () => { + expect(sqlPredicateToCel("status = 'a = b'")).toBe("status == 'a = b'"); + expect(sqlPredicateToCel("note = 'IN transit'")).toBe("note == 'IN transit'"); + }); + + it('is IDEMPOTENT on canonical CEL — an authored predicate passes through unchanged', () => { + for (const cel of [ + 'owner_id == current_user.id', + 'id in current_user.org_user_ids', + 'amount >= 100', + 'amount <= 100', + 'region != null', + "a == 1 && b == 'x'", + ]) { + expect(sqlPredicateToCel(cel)).toBe(cel); + expect(sqlPredicateToCel(sqlPredicateToCel(cel))).toBe(cel); + } + }); + + it('leaves comparison operators containing `=` alone', () => { + // The lookbehind/lookahead exist for these: `>=`, `<=`, `!=`, `==`. + expect(sqlPredicateToCel('a >= 1')).toBe('a >= 1'); + expect(sqlPredicateToCel('a <= 1')).toBe('a <= 1'); + expect(sqlPredicateToCel('a != 1')).toBe('a != 1'); + }); +}); + +// --------------------------------------------------------------------------- +// The composition the gate depends on +// --------------------------------------------------------------------------- + +describe('isSupportedRlsExpression — composition and dependency direction', () => { + it('is exactly `isPushdownableCel(sqlPredicateToCel(x)).ok` for a non-blank predicate', () => { + const corpus = [ + 'owner_id = current_user.id', + "status = 'published'", + 'id IN (current_user.org_user_ids)', + 'amount > 100', + 'a == 1 && b == 2', + 'amount + 1 > 2', + 'size(record.tags) > 0', + "record.account.region == 'EU'", + 'a = current_user.id AND b = 1', + ]; + for (const source of corpus) { + expect({ source, ok: isSupportedRlsExpression(source) }) + .toEqual({ source, ok: isPushdownableCel(sqlPredicateToCel(source)).ok }); + } + }); + + /** + * #4983's hard constraint: the direction is `plugin-security` → `formula` and + * `lint` → `formula`, NEVER the reverse. `@objectstack/formula` depends on + * `@objectstack/spec` alone (see its package.json), and this module may not + * quietly acquire a runtime import — that would put the hoisted predicate back + * out of `@objectstack/lint`'s reach ("Depends on @objectstack/spec; never on + * a runtime") and undo the whole move. Asserted against the source, because + * a dependency that is only wrong at build time produces no failing assertion. + */ + it('never imports a runtime — the hoist direction is pinned, not just intended', () => { + const here = dirname(fileURLToPath(import.meta.url)); + const source = readFileSync(join(here, 'rls-predicate.ts'), 'utf8'); + const specifiers = [...source.matchAll(/from\s+'([^']+)'/g)].map((m) => m[1]); + expect(specifiers).toEqual(['./cel-to-filter']); + + const pkg = JSON.parse(readFileSync(join(here, '..', 'package.json'), 'utf8')) as { + dependencies?: Record; + }; + expect(Object.keys(pkg.dependencies ?? {}).sort()).toEqual(['@marcbachmann/cel-js', '@objectstack/spec']); + }); +}); diff --git a/packages/formula/src/rls-predicate.ts b/packages/formula/src/rls-predicate.ts new file mode 100644 index 0000000000..7a84735d65 --- /dev/null +++ b/packages/formula/src/rls-predicate.ts @@ -0,0 +1,79 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The RLS predicate shape gate, and the legacy SQL→CEL bridge it stands on. + * + * **Hoisted here from `@objectstack/plugin-security` (`src/rls-compiler.ts`) in + * #4983 — executable code unchanged, address changed.** Both functions were + * already pure `(string) => …` over `isPushdownableCel`; neither ever read a + * runtime service, an `ExecutionContext` or a policy record, so nothing about + * them needed a runtime to live in. What their old address DID do was put the + * one decision procedure for "will this RLS predicate ever enforce?" behind a + * package `@objectstack/lint` is forbidden to import ("Depends on + * @objectstack/spec; never on a runtime"), which left the ADR-0056 D4 + * authoring gate two impossible doors: import a runtime, or fork the bridge. + * A forked bridge is the worse one — `sqlPredicateToCel`'s `=` / `IN` boundary + * conditions (quoted literals are never rewritten; CEL input passes through + * unchanged) ARE the red/green line, so one drifting character makes the linter + * reject policies the runtime executes correctly. Hoisting keeps ONE definition + * and lets the gate call the consumer's own verdict (ADR-0058 D1: a single + * canonical shape gate). + * + * `plugin-security` imports both from here; `@objectstack/lint` imports + * {@link isSupportedRlsExpression} for the authoring gate. The dependency + * direction is security → formula and lint → formula, never the reverse — + * pinned by `rls-predicate.test.ts`'s import-graph assertion. + */ + +import { isPushdownableCel } from './cel-to-filter'; + +/** + * Recognize whether an RLS `using` / `check` expression matches one of the SHAPES + * the compiler can compile (equality against a `current_user.*` var, equality + * against a string literal, set-membership against a `current_user.*` array, or + * the `1 = 1` allow-all). This is SHAPE-only — it does not check whether the + * referenced context variable is populated at runtime. + * + * ADR-0056 D4: exposed so an authoring-time gate (`objectstack compile`) can REJECT + * a predicate the runtime would silently drop — the class of bug where + * `owner == current_user.name` (`==`, unsupported) compiled to nothing and left an + * object unprotected. A `false` here means "this predicate will never enforce". + * + * That gate exists as of #4983: `validateRlsPredicateEnforceability` in + * `@objectstack/lint` calls THIS function on every + * `permissions[].rowLevelSecurity[].using` / `.check`, so the sentence above is + * no longer aspirational. Until then the function had no non-test consumer + * anywhere — a declared-but-never-read helper written to fix + * declared-but-never-read. + */ +export function isSupportedRlsExpression(expression: string): boolean { + if (!expression || !expression.trim()) return false; + // ADR-0058 D1: a single canonical shape gate. We bridge the legacy SQL-ish + // subset (`=`, `IN`) to canonical CEL, then ask the ONE pushdown compiler + // whether the shape lowers to a FilterCondition at all. This is broader than + // the historical 4 forms — comparisons (`amount > 100`) and `==` now ENFORCE + // (the compiler lowers them), so the gate correctly reports them supported. + // It is SHAPE-only: whether a referenced `current_user.*` variable is exposed + // at runtime is a separate availability concern (an unexposed var fails closed + // at resolution — see RLSCompiler.compileExpression). + return isPushdownableCel(sqlPredicateToCel(expression)).ok; +} + +/** + * @deprecated Transitional bridge (ADR-0058 D1). Canonical RLS predicates are + * CEL; this exists ONLY so stored/legacy SQL-ish `using`/`check` keeps compiling + * until it is migrated. Bridge the legacy SQL subset to canonical CEL so it flows + * through the ONE compiler: `=` → `==`, `IN` → `in`. Quoted string literals are + * left untouched. It is IDEMPOTENT on CEL input (a `==`/`in` predicate is + * unchanged), so authored-CEL seeds pass through as no-ops (no deprecation warn). Only this historically-supported subset is bridged — compound + * predicates should be authored in canonical CEL (`&&` / `||`); anything outside + * the subset (subqueries, SQL `AND`/`OR`, `LIKE`) stays unparseable and so fails + * closed, exactly as before. + */ +export function sqlPredicateToCel(expression: string): string { + return expression.replace(/'[^']*'|\bIN\b|(?=!])=(?!=)/gi, (m) => { + if (m[0] === "'") return m; // quoted literal — never rewrite its contents + if (m === '=') return '=='; + return 'in'; // IN / in / In → CEL membership operator + }); +} diff --git a/packages/lint/src/authoring-rules.ts b/packages/lint/src/authoring-rules.ts index dbf41dde34..7e740c6846 100644 --- a/packages/lint/src/authoring-rules.ts +++ b/packages/lint/src/authoring-rules.ts @@ -121,6 +121,7 @@ import { validateVisibilityPredicates } from './validate-visibility-predicates.j import { validateSecurityPosture } from './validate-security-posture.js'; import { validateOrgAxisRedLines } from './validate-org-axis-red-lines.js'; import { validateSharingRuleEnforceability } from './validate-sharing-rule-enforceability.js'; +import { validateRlsPredicateEnforceability } from './validate-rls-predicate-enforceability.js'; import { validateActionLocations } from './validate-action-locations.js'; import { lintFlowPatterns } from './lint-flow-patterns.js'; import { lintLivenessProperties } from './lint-liveness-properties.js'; @@ -821,6 +822,30 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ + 'door should not claim it.', run: (stack) => validateSharingRuleEnforceability(stack), }, + // #4983 — the sibling surface of the rule above, and ADR-0056 D4's gate, + // which had never been wired to anything: `isSupportedRlsExpression` existed + // solely so an authoring command could reject a predicate the runtime drops, + // and no authoring command called it. An unlowerable + // `rowLevelSecurity[].using` is DROPPED by `RLSCompiler` and — when it is the + // only applicable policy — replaced by `RLS_DENY_FILTER`, so the policy reads + // as an authorization and behaves as a blanket refusal. Same construction as + // the sharing-rule entry: the verdict is the runtime's own function, reached + // through `@objectstack/formula` (where #4983 hoisted it), never a model of it. + { + name: 'validateRlsPredicateEnforceability', + tier: 'gating', + input: 'parsed', + commands: ALL, + source: 'packages/lint/src/validate-rls-predicate-enforceability.ts', + surfaces: CLI_ONLY, + surfaceReason: + 'P2 (#4463): the rule reads `stack.permissions[]`, a stack-wide collection the per-write snapshot ' + + 'does not carry, and P1 gates `flow` alone. It is otherwise snapshot-ready — it needs no other ' + + 'collection — so widening it is a `runtimeTypes: [\'permission_set\']` edit once the gate builds ' + + 'that snapshot, not new wiring. Recorded as pending rather than done, because a rule that has ' + + 'never run at a door should not claim it.', + run: (stack) => validateRlsPredicateEnforceability(stack), + }, ]; // ─── Runner ───────────────────────────────────────────────────────── diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 5743f01a25..9b1670b14c 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -209,6 +209,22 @@ export type { SharingRuleEnforceabilitySeverity, } from './validate-sharing-rule-enforceability.js'; +// #4983 — the sibling surface, and ADR-0056 D4's gate finally wired. An RLS +// `using` / `check` the runtime cannot compile is DROPPED (and, when it is the +// only applicable policy, replaced by the deny sentinel), so the policy reads +// 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. +export { + validateRlsPredicateEnforceability, + RLS_PREDICATE_UNENFORCEABLE, + RLS_PREDICATE_UNPARSEABLE, +} from './validate-rls-predicate-enforceability.js'; +export type { + RlsPredicateFinding, + RlsPredicateSeverity, +} from './validate-rls-predicate-enforceability.js'; + export { validateDashboardActionRefs, DASHBOARD_ACTION_TARGET_UNDEFINED, diff --git a/packages/lint/src/validate-rls-predicate-enforceability.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.test.ts new file mode 100644 index 0000000000..a0886cb973 --- /dev/null +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -0,0 +1,309 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { isSupportedRlsExpression } from '@objectstack/formula'; + +import { + validateRlsPredicateEnforceability, + RLS_PREDICATE_UNENFORCEABLE, + RLS_PREDICATE_UNPARSEABLE, +} from './validate-rls-predicate-enforceability.js'; +import { AUTHORING_RULES, runAuthoringRules } from './authoring-rules.js'; + +const ids = (stack: unknown) => validateRlsPredicateEnforceability(stack).map((f) => f.rule); + +/** A complete, spec-shaped permission set with one RLS policy's clause swapped in. */ +const policyWith = (clause: 'using' | 'check', source: unknown) => ({ + permissions: [ + { + name: 'sales_rep', + label: 'Sales Rep', + rowLevelSecurity: [ + { + name: 'own_leads', + object: 'lead', + operation: 'select', + // `using` is required on the schema, so a `check` fixture carries a + // valid `using` alongside it — otherwise the fixture would be red for + // a reason the test is not about. + ...(clause === 'using' ? {} : { using: 'owner_id == current_user.id' }), + [clause]: source, + }, + ], + }, + ], +}); + +// ── Red: policies that authorize nothing ───────────────────────────── +// +// Every source below passes `RowLevelSecurityPolicySchema` (`using` / `check` +// are `z.string()`), `os validate`, `os build` and `os lint` as they stand +// today. The runtime drops each one and answers "no rows" — which is the whole +// defect (#4983). + +describe('validateRlsPredicateEnforceability — predicates the runtime can only drop go RED', () => { + it('flags a function call, naming the policy, the path and the real consequence', () => { + const findings = validateRlsPredicateEnforceability(policyWith('using', 'size(record.tags) > 0')); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ + severity: 'error', + rule: RLS_PREDICATE_UNENFORCEABLE, + path: 'permissions[0].rowLevelSecurity[0].using', + where: 'permission set "sales_rep" policy "own_leads" on object "lead"', + }); + // The message must say what the runtime DOES, not merely "unsupported". + expect(findings[0].message).toMatch(/DROPS the policy at request time/); + expect(findings[0].message).toMatch(/RLS_DENY_FILTER/); + expect(findings[0].message).toMatch(/ZERO rows/); + // …and prescribe the fix that works on THIS surface. + expect(findings[0].hint).toMatch(/field != null/); + expect(findings[0].hint).toMatch(/INTERPRETED/); + }); + + it.each([ + ['a cross-object path', "record.account.region == 'EU'"], + ['arithmetic', 'amount + 1 > 2'], + ['a bare function call', 'has(record.owner_id)'], + ['a ternary', "stage == 'won' ? true : false"], + ])('flags %s as unenforceable', (_label, source) => { + expect(ids(policyWith('using', source))).toEqual([RLS_PREDICATE_UNENFORCEABLE]); + }); + + it.each([ + ['SQL AND — the bridge covers `=`/`IN` only', 'a = current_user.id AND b = 1'], + ['a subquery', 'id IN (SELECT id FROM users)'], + ])('gives %s its own id — the fix is CEL, not a different shape', (_label, source) => { + const findings = validateRlsPredicateEnforceability(policyWith('using', source)); + expect(findings.map((f) => f.rule)).toEqual([RLS_PREDICATE_UNPARSEABLE]); + expect(findings[0].hint).toMatch(/canonical CEL/); + expect(findings[0].hint).toMatch(/`&&` \/ `\|\|`/); + }); + + it('judges `check` with the WRITE-path consequence, not the read one', () => { + const findings = validateRlsPredicateEnforceability(policyWith('check', 'size(record.tags) > 0')); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ + severity: 'error', + rule: RLS_PREDICATE_UNENFORCEABLE, + path: 'permissions[0].rowLevelSecurity[0].check', + }); + // ADR-0058 D4: the post-image check becomes the deny sentinel → every write denied. + expect(findings[0].message).toMatch(/PermissionDeniedError/); + expect(findings[0].message).toMatch(/blanket refusal/); + expect(findings[0].message).not.toMatch(/ZERO rows/); + }); + + it('reports `using` and `check` on one policy separately', () => { + const findings = validateRlsPredicateEnforceability({ + permissions: [ + { + name: 'p', + rowLevelSecurity: [{ name: 'r', using: 'size(a) > 0', check: 'b + 1 > 2' }], + }, + ], + }); + expect(findings.map((f) => f.path)).toEqual([ + 'permissions[0].rowLevelSecurity[0].using', + 'permissions[0].rowLevelSecurity[0].check', + ]); + }); + + it('names each offending policy with its own index', () => { + const findings = validateRlsPredicateEnforceability({ + permissions: [ + { name: 'ok', rowLevelSecurity: [{ name: 'a', using: 'owner_id == current_user.id' }] }, + { + name: 'bad', + rowLevelSecurity: [ + { name: 'fine', using: "status = 'published'" }, + { name: 'fn', using: 'size(tags) > 0' }, + ], + }, + ], + }); + expect(findings.map((f) => [f.rule, f.path])).toEqual([ + [RLS_PREDICATE_UNENFORCEABLE, 'permissions[1].rowLevelSecurity[1].using'], + ]); + }); + + it('judges a DISABLED policy too — it is a landmine, not a dead branch', () => { + // `getApplicablePolicies` skips `enabled: false`, so the consequence is + // dormant rather than live. The day someone flips it on is exactly the day + // nobody re-runs the linter. + expect(ids({ permissions: [{ name: 'p', rowLevelSecurity: [{ name: 'r', enabled: false, using: 'size(a) > 0' }] }] })) + .toEqual([RLS_PREDICATE_UNENFORCEABLE]); + }); + + it('accepts the name-keyed permission-set map as well as the array', () => { + expect(ids({ permissions: { sales: { rowLevelSecurity: [{ name: 'r', using: 'size(a) > 0' }] } } })) + .toEqual([RLS_PREDICATE_UNENFORCEABLE]); + }); +}); + +// ── Green: predicates that really do enforce ───────────────────────── +// +// A false positive here is worse than the gap the rule closes: it rejects +// security metadata that enforces correctly today and hands the author a +// "correction" that would break it. The legacy SQL forms are the sharp edge — +// they are CEL syntax ERRORS and perfectly working RLS predicates, because +// `sqlPredicateToCel` bridges them before the compiler sees them. + +describe('validateRlsPredicateEnforceability — predicates the runtime DOES compile stay green', () => { + it.each([ + // Legacy SQL-ish subset, bridged. + ['owner_id = current_user.id'], + ["status = 'published'"], + ['id IN (current_user.org_user_ids)'], + ['1 = 1'], + // Canonical CEL. + ['owner_id == current_user.id'], + ['organization_id == current_user.organization_id'], + ['id in current_user.org_user_ids'], + ['amount > 100'], + ['region != null'], + ['a == 1 && b == 2'], + ["!(stage in ['draft']) || amount >= 1000"], + ])('accepts %s', (source) => { + expect(validateRlsPredicateEnforceability(policyWith('using', source))).toEqual([]); + }); + + it('a quoted literal containing `=` or `IN` survives the bridge and stays green', () => { + // The bridge's sharpest boundary: rewriting inside a string literal would + // turn a working policy red. This is why the bridge was hoisted rather + // than copied (#4983). + expect(validateRlsPredicateEnforceability(policyWith('using', "note = 'a = b'"))).toEqual([]); + expect(validateRlsPredicateEnforceability(policyWith('using', "note = 'IN transit'"))).toEqual([]); + }); + + /** + * The measured claim behind shipping this as `error`: every RLS predicate + * this repo declares today is supported, so the gate turns nothing red. + * Lifted verbatim from `plugin-security/src/objects/default-permission-sets.ts` + * (the platform seeds — `everyone` / member / self-service sets), + * `examples/app-showcase/src/security/permission-sets.ts`, the dogfood + * fixtures, and `skills/objectstack-data/SKILL.md`'s authoring example. + */ + it('accepts every RLS predicate declared anywhere in this repo', () => { + const shipped = [ + // plugin-security default-permission-sets.ts + 'id == current_user.organization_id', + 'id == current_user.id', + 'id in current_user.org_user_ids', + 'user_id == current_user.id', + 'organization_id == current_user.organization_id', + 'created_by == current_user.id', + // examples/ + dogfood fixtures + 'owner == current_user.email', + 'assignee == current_user.email', + // skills/objectstack-data/SKILL.md + 'owner_id == current_user.id', + ]; + for (const source of shipped) { + expect(validateRlsPredicateEnforceability(policyWith('using', source))).toEqual([]); + expect(validateRlsPredicateEnforceability(policyWith('check', source))).toEqual([]); + } + }); + + it('ignores shapes Zod owns rather than inventing a second complaint', () => { + expect(ids(policyWith('using', undefined))).toEqual([]); + expect(ids(policyWith('using', ''))).toEqual([]); + expect(ids(policyWith('using', ' '))).toEqual([]); + expect(ids(policyWith('using', 42))).toEqual([]); + }); + + it('is a no-op on a stack that declares no RLS', () => { + expect(validateRlsPredicateEnforceability({})).toEqual([]); + expect(validateRlsPredicateEnforceability(undefined)).toEqual([]); + expect(validateRlsPredicateEnforceability({ permissions: [] })).toEqual([]); + expect(validateRlsPredicateEnforceability({ permissions: [{ name: 'p' }] })).toEqual([]); + }); + + it('reads only `permissions` — no alias branch that no spec-valid stack can reach', () => { + // `rowLevelSecurity` is declared on `PermissionSetSchema` alone (ObjectSchema + // has no such key) and `permissions` is the one stack key StackSchema + // declares for permission sets. `permissionSets` / `objects[].rls` are + // rejected by name, so reading them here would be the #4984 defect: a branch + // that only ever fires on metadata the schema already refuses. + expect(ids({ permissionSets: [{ name: 'p', rowLevelSecurity: [{ name: 'r', using: 'size(a) > 0' }] }] })).toEqual([]); + expect(ids({ objects: [{ name: 'o', rowLevelSecurity: [{ name: 'r', using: 'size(a) > 0' }] }] })).toEqual([]); + }); +}); + +// ── The predicate is the runtime's, not a model of it ──────────────── + +describe('validateRlsPredicateEnforceability — the verdict IS the RLSCompiler\'s verdict', () => { + const corpus = [ + 'owner_id = current_user.id', + "status = 'published'", + 'id IN (current_user.org_user_ids)', + '1 = 1', + 'owner_id == current_user.id', + 'amount > 100', + 'region != null', + 'a == 1 && b == 2', + "note = 'a = b'", + 'size(record.tags) > 0', + 'amount + 1 > 2', + "record.account.region == 'EU'", + 'id IN (SELECT id FROM users)', + 'a = current_user.id AND b = 1', + ]; + + it('agrees with `isSupportedRlsExpression` on every source, in both directions', () => { + for (const source of corpus) { + // This is the exact function `RLSCompiler.compileFilter` consults to + // decide whether a dropped policy earns its "DROPPED (no enforcement)" + // WARN — same function, same package, same input. + const runtimeWouldEnforce = isSupportedRlsExpression(source); + const lintIsClean = validateRlsPredicateEnforceability(policyWith('using', source)).length === 0; + expect({ source, lintIsClean }).toEqual({ source, lintIsClean: runtimeWouldEnforce }); + } + }); + + /** + * The "before" half of the proof, kept mechanical rather than asserted in + * prose. #4983's complaint is that an unenforceable RLS policy passes the + * WHOLE toolchain, so it is not enough to show the new rule goes red — it + * must also be shown that nothing else ever did. If some future rule grows to + * cover this shape, this test fails and someone decides which of the two owns + * it, instead of the stack quietly acquiring a duplicate diagnostic. + */ + it('no OTHER author-time rule sees this — which is exactly why the gate was missing', () => { + const offending = { + objects: [ + { + name: 'lead', + label: 'Lead', + sharingModel: 'private', + fields: { + name: { type: 'text', label: 'Name' }, + owner_id: { type: 'text', label: 'Owner' }, + tags: { type: 'text', label: 'Tags' }, + }, + }, + ], + permissions: [ + { + name: 'sales_rep', + label: 'Sales Rep', + rowLevelSecurity: [ + { name: 'own_leads', object: 'lead', operation: 'select', using: 'size(record.tags) > 0' }, + ], + }, + ], + }; + + const findings = runAuthoringRules('validate', { normalized: offending, parsed: offending }); + expect(findings.map((f) => f.rule)).toEqual([RLS_PREDICATE_UNENFORCEABLE]); + + // And the fixture really did travel through every rule — a registry that + // silently stopped running would satisfy the assertion above vacuously. + expect(AUTHORING_RULES.filter((r) => r.commands.includes('validate')).length).toBeGreaterThan(20); + }); + + it('runs identically on the normalized tier — `using` / `check` are plain strings in both', () => { + const stack = policyWith('using', 'size(record.tags) > 0'); + expect(runAuthoringRules('lint', { normalized: stack }).map((f) => f.rule)) + .toContain(RLS_PREDICATE_UNENFORCEABLE); + }); +}); diff --git a/packages/lint/src/validate-rls-predicate-enforceability.ts b/packages/lint/src/validate-rls-predicate-enforceability.ts new file mode 100644 index 0000000000..14cd9a1b66 --- /dev/null +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -0,0 +1,255 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4983 — ADR-0056 D4's RLS authoring gate, finally wired. + * + * `isSupportedRlsExpression`'s TSDoc has always said why it exists: "exposed so + * an authoring-time gate (`objectstack compile`) can REJECT a predicate the + * runtime would silently drop … A `false` here means 'this predicate will never + * enforce'." Until this file it had no non-test consumer anywhere in the repo — + * the function written to fix declared-but-never-read was itself declared and + * never read. This rule is that consumer. + * + * ## The consequence being gated (measured, not inferred) + * + * `permissions[].rowLevelSecurity[].using` / `.check` is an authorable surface + * (`PermissionSetSchema.rowLevelSecurity`). Follow one unlowerable predicate + * through `plugin-security`: + * + * 1. `RLSCompiler.compileExpression` bridges the legacy SQL-ish subset and + * hands the result to `compileCelToFilter`, which returns `!ok`, so the + * method returns `null` and the policy contributes NO filter. + * 2. `compileFilter` notices (`!isSupportedRlsExpression(predicate)`) and logs + * ONE `warn`: "policy '…' … has an uncompilable predicate … and was DROPPED + * (no enforcement)". That line, at request time, is the entire signal. + * 3. **Read path (`using`).** If it was the only applicable policy for that + * object+operation, `compileFilter` returns `RLS_DENY_FILTER` — + * `{ id: '__rls_deny__:00000000-0000-0000-0000-000000000000' }` — which the + * caller AND's onto the where clause. Every `select` / `update` / `delete` + * on that object then matches zero rows. If other policies also applied, + * this one is simply dropped out of the OR and the access it was written to + * grant does not exist. + * 4. **Write path (`check`, ADR-0058 D4).** `computeWriteCheckFilter` collects + * only the policies that declare a `check`; the same drop makes the + * post-image predicate the deny sentinel, `matchesFilterCondition` fails, + * and the write raises `PermissionDeniedError`. + * + * So this is not a hole — the runtime fails CLOSED, which is why it has been + * survivable. It is a policy that reads as an authorization and behaves as a + * blanket refusal, with nothing at authoring time pointing at the line that + * caused it: `os validate`, `os build` and `os lint` are all green today. + * + * ## Why the verdict cannot drift from the runtime's + * + * Same construction as `validate-sharing-rule-enforceability.ts` (#4698/#4985), + * and for the same reason: the rule does not model the consumer, guess at it, + * or grep for it. It calls the consumer's OWN decision procedure — + * `isSupportedRlsExpression` — on the same input. `compileFilter` calls that + * exact function to decide whether a dropped policy is an authoring mistake + * (warn) or the intentional "context variable absent" path (silent), so + * `false` here and "DROPPED (no enforcement)" there are the same boolean. + * + * That was impossible until #4983. The predicate first bridges the legacy + * SQL-ish subset through `sqlPredicateToCel` (`=` → `==`, `IN` → `in`), and + * BOTH functions lived in `plugin-security` — a runtime `@objectstack/lint` may + * never import ("Depends on @objectstack/spec; never on a runtime"). The only + * other door was copying the bridge, which is the worst option available: its + * boundary conditions (quoted literals are never rewritten; canonical CEL + * passes through unchanged) ARE this rule's red/green line, so a fork drifting + * by one character makes the linter reject policies the runtime executes + * correctly — the false-positive direction, which is worse than the gap. #4983 + * hoisted both into `@objectstack/formula`, verbatim; this rule and the + * RLSCompiler now read one definition. + * + * ## Why `error` + * + * The bar `lint-flow-patterns.ts` states: gate when no reading of the metadata + * behaves as written. There is none here. The author wrote a row filter; the + * runtime has no filter to apply and answers "no rows" (or "denied") to + * everything the policy governs. Measured before shipping: every `using` / + * `check` declared anywhere in this repo — `plugin-security`'s + * `default-permission-sets.ts` seeds, the examples, the dogfood fixtures — is + * supported, so the gate turns nothing red that works today. That corpus is + * pinned in the tests rather than asserted here. + * + * ## The two ids, and why not one + * + * The consequence is identical, the FIX is not, and allowlists / `--json` + * consumers key on the id: + * + * - {@link RLS_PREDICATE_UNENFORCEABLE} — the predicate parses as CEL and is + * outside the pushdown subset: a function call (`size(...)`, `has(...)`), + * arithmetic, a ternary, a cross-object path (`record.account.region`). Fix + * = rewrite it inside the subset, or denormalise the value onto this object. + * - {@link RLS_PREDICATE_UNPARSEABLE} — it does not parse as CEL even after + * the legacy SQL bridge: SQL `AND` / `OR` / `LIKE`, a subquery, a stray + * operator. Fix = write CEL (`&&`, `||`), which is a different edit. + * + * Unlike the sharing-rule gate, syntax is reported HERE rather than deferred to + * `validateStackExpressions`: that rule does not walk `rowLevelSecurity` at all, + * so deferring would defer to nobody. It also could not judge this field + * correctly if it did — `owner_id = current_user.id` is a CEL syntax error and a + * perfectly working RLS predicate, because the bridge rewrites it first. + * + * ## Scope + * + * `permissions[].rowLevelSecurity[]` only. `rowLevelSecurity` is declared on + * `PermissionSetSchema` and nowhere else — `ObjectSchema` has no such key — and + * `permissions` is the one stack key `StackSchema` declares for permission sets. + * Walking `objects[].rowLevelSecurity` or `permissionSets` "just in case" would + * add a branch that no spec-valid stack can ever reach: exactly the #4984 defect, + * where a red line read rejected aliases and was therefore inert for every stack + * the schema accepts. Alias tolerance belongs at the schema's refusal, not in a + * consumer (Prime Directive #12). + * + * A policy with `enabled: false` IS judged, on purpose. `getApplicablePolicies` + * skips it today, so the consequence described above is dormant rather than + * live — but a switched-off policy that can never enforce is a policy that will + * grant nothing on the day someone switches it on, and that is precisely the + * moment nobody re-runs the linter. + */ + +import { isPushdownableCel, isSupportedRlsExpression, sqlPredicateToCel } from '@objectstack/formula'; + +/** A predicate outside the pushdown subset — the policy enforces nothing. */ +export const RLS_PREDICATE_UNENFORCEABLE = 'rls-predicate-unenforceable'; +/** A predicate that does not parse as CEL even after the legacy SQL bridge. */ +export const RLS_PREDICATE_UNPARSEABLE = 'rls-predicate-unparseable'; + +export type RlsPredicateSeverity = 'error' | 'warning'; + +export interface RlsPredicateFinding { + severity: RlsPredicateSeverity; + /** Diagnostic rule id (`rls-predicate-*`). */ + rule: string; + /** Human-readable location, e.g. `permission set "sales" policy "own_leads"`. */ + where: string; + /** Config path, e.g. `permissions[2].rowLevelSecurity[0].using`. */ + path: string; + /** What is wrong. */ + message: string; + /** How to fix it. */ + hint: string; +} + +type AnyRec = Record; + +/** Coerce a collection (array or name-keyed map) to an array of records. */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v as AnyRec[]; + if (v && typeof v === 'object') { + return Object.entries(v as AnyRec).map(([name, def]) => ({ name, ...(def as AnyRec) })); + } + return []; +} + +function str(v: unknown): string { + return typeof v === 'string' ? v : ''; +} + +const PUSHDOWN_SUBSET = + 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, ' + + 'and the string methods `startsWith` / `endsWith` / `contains` — over SINGLE-column field paths ' + + '(ADR-0058 D2), compared against a literal or a `current_user.*` value.'; + +/** What the runtime does with a predicate it cannot compile, per clause. */ +function consequence(clause: 'using' | 'check'): string { + const dropped = + 'so `RLSCompiler` DROPS the policy at request time (one WARN line — "has an uncompilable predicate ' + + '… and was DROPPED (no enforcement)" — 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, `compileFilter` returns the ' + + '`RLS_DENY_FILTER` sentinel instead, which is AND-ed onto the where clause: every select / update / ' + + 'delete on the object matches ZERO rows. When other policies also apply, this one just 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` as the `RLS_DENY_FILTER` ' + + 'sentinel, which no record can satisfy: every insert / update the policy governs fails with ' + + '`PermissionDeniedError`. The policy reads as a write rule and behaves as a blanket refusal.'; +} + +/** + * Gate every stack-declared RLS predicate on the ONE thing the runtime does + * with it: lower it to a FilterCondition (ADR-0056 D4). + * + * Pure `(stack) => Finding[]`; tolerates the normalized and the parsed tier + * (`using` / `check` are plain `z.string()`, identical in both). + */ +export function validateRlsPredicateEnforceability(stack: unknown): RlsPredicateFinding[] { + const findings: RlsPredicateFinding[] = []; + const cfg = (stack ?? {}) as AnyRec; + + asArray(cfg.permissions).forEach((ps, psIndex) => { + asArray(ps.rowLevelSecurity).forEach((policy, pIndex) => { + for (const clause of ['using', 'check'] as const) { + const source = str(policy[clause]); + // Absent / blank is Zod's to judge (`using` is required on the schema); + // inventing a second complaint about the shape here would be the + // double-report this rule avoids for everything else. + if (!source.trim()) continue; + + // ── The verdict. This is the consumer's own function, not a model of + // 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; + + // ── The explanation. Re-derived only to tell the author WHICH fix they + // need; the red/green boundary above never consults it. (Both agree by + // construction — pinned in both directions in this rule's tests.) + const why = isPushdownableCel(sqlPredicateToCel(source)); + const detail = why.ok ? '' : why.detail; + const parseError = !why.ok && why.reason === 'parse-error'; + + 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}`; + + if (parseError) { + findings.push({ + severity: 'error', + rule: RLS_PREDICATE_UNPARSEABLE, + where, + path, + message: + `RLS ${clause} \`${source}\` does not parse as CEL even after the legacy SQL bridge ` + + `(\`=\` → \`==\`, \`IN\` → \`in\`) has been applied (${detail}), ` + consequence(clause), + hint: + 'Author the predicate in canonical CEL (ADR-0058 D1). The bridge covers only the historic ' + + 'SQL subset — a bare `=` and `IN` — so everything else must already be CEL: combine with ' + + '`&&` / `||` rather than SQL `AND` / `OR`, negate with `!`, and use `startsWith` / ' + + '`endsWith` / `contains` rather than `LIKE`. A subquery has no CEL spelling at all: RLS ' + + 'cannot join (ADR-0055), so pre-resolve the set into a membership key the runtime exposes ' + + '(`field in current_user.`, ADR-0105 D11) or denormalise the value onto this object.', + }); + continue; + } + + findings.push({ + severity: 'error', + rule: RLS_PREDICATE_UNENFORCEABLE, + where, + path, + message: + `RLS ${clause} \`${source}\` is outside the pushdown subset the runtime can compile ` + + `(${detail}), ` + consequence(clause), + hint: + 'Rewrite the predicate inside the lowerable subset. ' + PUSHDOWN_SUBSET + ' Three traps in ' + + 'particular: (1) a function call — `size(record.tags) > 0`, `has(record.x)` — is correct in an ' + + 'object VALIDATION rule, which is INTERPRETED, and wrong here, where the predicate is ' + + 'COMPILED to a filter; write the null test as `field != null`. (2) A related-record path ' + + '(`record.account.region`) is a join, which the compiler refuses by design (ADR-0055) — ' + + 'denormalise the value onto this object (a formula/rollup field) and test that column. ' + + '(3) Arithmetic on a column (`amount * 2 > 100`) never lowers — precompute it into a field, ' + + 'or compare the column against the literal directly.', + }); + } + }); + }); + + return findings; +} diff --git a/packages/lint/src/validate-sharing-rule-enforceability.ts b/packages/lint/src/validate-sharing-rule-enforceability.ts index 87249a2a07..70d17e2299 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.ts @@ -82,15 +82,16 @@ * concrete field predicate, so a lowering CEL condition cannot produce a * match-all filter. `matchAllIsUnreachable` in the tests pins that claim * against the compiler rather than asserting it in prose. - * - **It does not judge RLS `using` / `check`.** Same class, same compiler, - * and ADR-0056 D4 explicitly asks for this gate — but the RLS decision - * procedure is `isSupportedRlsExpression`, which first bridges the legacy - * SQL-ish subset through `sqlPredicateToCel`, and BOTH live in - * `plugin-security`. Judging RLS here means either importing a runtime - * (forbidden) or copying the bridge (forking the predicate — the thing this - * file's opening paragraph refuses to do). Hoisting `sqlPredicateToCel` into - * `@objectstack/formula` first would make it exact; that is a spec/runtime - * move, not a lint change, and is filed separately. + * - **It does not judge RLS `using` / `check`** — its sibling rule + * `validate-rls-predicate-enforceability.ts` does (#4983). Same class, same + * compiler, but a different decision procedure: RLS asks + * `isSupportedRlsExpression`, which first bridges the legacy SQL-ish subset + * through `sqlPredicateToCel`. Both used to live in `plugin-security`, so + * judging RLS from here meant importing a runtime (forbidden) or copying the + * bridge (forking the predicate — the thing this file's opening paragraph + * refuses to do). #4983 hoisted both into `@objectstack/formula` and then + * wrote the gate against the hoisted predicate, which is why the split is + * two rules over two surfaces and still exactly two definitions. * - **It does not look at flow / hook `condition`s.** Those are INTERPRETED by * the CEL engine, not lowered to a filter, so the whole language is in scope * there and non-pushdownability means nothing. Reusing this predicate on diff --git a/packages/plugins/plugin-security/src/rls-compiler.ts b/packages/plugins/plugin-security/src/rls-compiler.ts index a4d821836b..667bbdc95c 100644 --- a/packages/plugins/plugin-security/src/rls-compiler.ts +++ b/packages/plugins/plugin-security/src/rls-compiler.ts @@ -2,7 +2,13 @@ import type { RowLevelSecurityPolicy } from '@objectstack/spec/security'; import type { ExecutionContext } from '@objectstack/spec/kernel'; -import { compileCelToFilter, isPushdownableCel } from '@objectstack/formula'; +// [ADR-0056 D4 / ADR-0058 D1] `isSupportedRlsExpression` and `sqlPredicateToCel` +// used to be DEFINED in this file. #4983 hoisted them into `@objectstack/formula` +// — verbatim, behaviour-preserving — because `@objectstack/lint` must ask the +// SAME question at authoring time and may never import a runtime. This file is +// now a consumer of that one definition, exactly as the lint gate is; there is +// no second copy for the `=` / `IN` bridge to drift against. +import { compileCelToFilter, isSupportedRlsExpression, sqlPredicateToCel } from '@objectstack/formula'; /** * RLS User Context @@ -59,50 +65,6 @@ export const RLS_DENY_FILTER: Record = Object.freeze({ id: '__rls_deny__:00000000-0000-0000-0000-000000000000', }); -/** - * Recognize whether an RLS `using` / `check` expression matches one of the SHAPES - * the compiler can compile (equality against a `current_user.*` var, equality - * against a string literal, set-membership against a `current_user.*` array, or - * the `1 = 1` allow-all). This is SHAPE-only — it does not check whether the - * referenced context variable is populated at runtime. - * - * ADR-0056 D4: exposed so an authoring-time gate (`objectstack compile`) can REJECT - * a predicate the runtime would silently drop — the class of bug where - * `owner == current_user.name` (`==`, unsupported) compiled to nothing and left an - * object unprotected. A `false` here means "this predicate will never enforce". - */ -export function isSupportedRlsExpression(expression: string): boolean { - if (!expression || !expression.trim()) return false; - // ADR-0058 D1: a single canonical shape gate. We bridge the legacy SQL-ish - // subset (`=`, `IN`) to canonical CEL, then ask the ONE pushdown compiler - // whether the shape lowers to a FilterCondition at all. This is broader than - // the historical 4 forms — comparisons (`amount > 100`) and `==` now ENFORCE - // (the compiler lowers them), so the gate correctly reports them supported. - // It is SHAPE-only: whether a referenced `current_user.*` variable is exposed - // at runtime is a separate availability concern (an unexposed var fails closed - // at resolution — see compileExpression). - return isPushdownableCel(sqlPredicateToCel(expression)).ok; -} - -/** - * @deprecated Transitional bridge (ADR-0058 D1). Canonical RLS predicates are - * CEL; this exists ONLY so stored/legacy SQL-ish `using`/`check` keeps compiling - * until it is migrated. Bridge the legacy SQL subset to canonical CEL so it flows - * through the ONE compiler: `=` → `==`, `IN` → `in`. Quoted string literals are - * left untouched. It is IDEMPOTENT on CEL input (a `==`/`in` predicate is - * unchanged), so authored-CEL seeds pass through as no-ops (no deprecation warn). Only this historically-supported subset is bridged — compound - * predicates should be authored in canonical CEL (`&&` / `||`); anything outside - * the subset (subqueries, SQL `AND`/`OR`, `LIKE`) stays unparseable and so fails - * closed, exactly as before. - */ -export function sqlPredicateToCel(expression: string): string { - return expression.replace(/'[^']*'|\bIN\b|(?=!])=(?!=)/gi, (m) => { - if (m[0] === "'") return m; // quoted literal — never rewrite its contents - if (m === '=') return '=='; - return 'in'; // IN / in / In → CEL membership operator - }); -} - /** * Does this filter consist solely of an empty membership (`{ field: { $in: [] } }`)? * Used to preserve the legacy "empty pre-resolved set drops the policy" semantics diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index ce75bd826c..bc8392c066 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -4,7 +4,12 @@ import { describe, it, expect, vi } from 'vitest'; import { SecurityPlugin } from './security-plugin.js'; import { PermissionEvaluator, crudBucketForOperation } from './permission-evaluator.js'; import { FieldMasker } from './field-masker.js'; -import { RLSCompiler, RLS_DENY_FILTER, isSupportedRlsExpression } from './rls-compiler.js'; +import { RLSCompiler, RLS_DENY_FILTER } from './rls-compiler.js'; +// #4983 — `isSupportedRlsExpression` now lives in @objectstack/formula, and its +// SHAPE unit tests moved with it (`formula/src/rls-predicate.test.ts`). What +// stays here is the consumer-side half: that this compiler's drop / warn / +// fail-closed behaviour really is what that predicate describes. +import { isSupportedRlsExpression } from '@objectstack/formula'; import type { PermissionSet } from '@objectstack/spec/security'; import { RLS } from '@objectstack/spec/security'; @@ -3039,30 +3044,35 @@ describe('RLSCompiler', () => { // ADR-0056 D4 — RLS predicates that won't compile must not vanish in silence // --------------------------------------------------------------------------- describe('RLSCompiler D4 — uncompilable predicates are surfaced', () => { - it('isSupportedRlsExpression accepts the compilable shapes', () => { - // Legacy SQL-ish subset (bridged `=`/`IN`). - expect(isSupportedRlsExpression('owner_id = current_user.id')).toBe(true); - expect(isSupportedRlsExpression('owner = current_user.email')).toBe(true); - expect(isSupportedRlsExpression("status = 'published'")).toBe(true); - expect(isSupportedRlsExpression('id IN (current_user.org_user_ids)')).toBe(true); - expect(isSupportedRlsExpression('1 = 1')).toBe(true); - // ADR-0058: the canonical compiler lowers a broader pushdown subset, so the - // shape gate now (correctly) reports these as enforceable — `==`/`!=`, - // comparisons, and CEL compound predicates all compile to a FilterCondition. - expect(isSupportedRlsExpression('owner == current_user.id')).toBe(true); // `==` - expect(isSupportedRlsExpression('amount > 100')).toBe(true); // comparison - expect(isSupportedRlsExpression('region != null')).toBe(true); // null check - expect(isSupportedRlsExpression('a == 1 && b == 2')).toBe(true); // CEL compound - }); - - it('isSupportedRlsExpression rejects genuinely non-pushdownable shapes', () => { - // These cannot lower to a FilterCondition for ANY input, so the gate must - // reject them (ADR-0055 / ADR-0056 D4) — they fail closed at runtime. - expect(isSupportedRlsExpression('a = current_user.id AND b = 1')).toBe(false); // SQL AND ≠ CEL && (unparseable) - expect(isSupportedRlsExpression('amount + 1 > 2')).toBe(false); // arithmetic - expect(isSupportedRlsExpression('id IN (SELECT id FROM users)')).toBe(false); // subquery - expect(isSupportedRlsExpression('record.a.b == 1')).toBe(false); // cross-object traversal - expect(isSupportedRlsExpression('')).toBe(false); + /** + * The consumer-side statement the hoist (#4983) has to keep true: whatever + * `isSupportedRlsExpression` says about a predicate is what THIS compiler + * does with it. The shape assertions themselves moved to + * `@objectstack/formula`'s `rls-predicate.test.ts`; re-asserting them here + * would be a copy of the tests to go with the copy of the code we refused to + * make. This one runs the corpus through the real compiler instead, which is + * the claim `@objectstack/lint`'s new authoring gate relies on: `false` here + * means the runtime drops the policy and warns. + */ + it('the compiler DROPS exactly the predicates isSupportedRlsExpression rejects', () => { + const corpus = [ + // supported — compile to a FilterCondition against a populated context + 'owner_id = current_user.id', + 'owner_id == current_user.id', + "status = 'published'", + 'id IN (current_user.org_user_ids)', + // unsupported — no input can lower these + 'amount + 1 > 2', + 'id IN (SELECT id FROM users)', + 'record.a.b == 1', + 'a = current_user.id AND b = 1', + ]; + const userCtx = { id: 'u1', status: 'published', org_user_ids: ['u1'] } as Record; + for (const source of corpus) { + const compiler = new RLSCompiler(); + const compiled = compiler.compileExpression(source, userCtx) !== null; + expect({ source, compiled }).toEqual({ source, compiled: isSupportedRlsExpression(source) }); + } }); it('WARNS (does not silently drop) an unsupported-shape policy', () => { diff --git a/packages/qa/dogfood/test/authz-conformance.matrix.ts b/packages/qa/dogfood/test/authz-conformance.matrix.ts index 8c04a546da..d3a42b9883 100644 --- a/packages/qa/dogfood/test/authz-conformance.matrix.ts +++ b/packages/qa/dogfood/test/authz-conformance.matrix.ts @@ -156,7 +156,7 @@ export const AUTHZ_CONFORMANCE: AuthzPrimitive[] = [ { id: 'hierarchy-widening', summary: 'hierarchy widening — a unit + its subordinate units gain access', state: 'enforced', enforcement: 'plugin-sharing/business-unit-graph.ts BusinessUnitGraphService subtree (business_unit recipient) — ADR-0057 D5 re-homed off the never-existent sys_position.parent', proof: 'showcase-bu-hierarchy-sharing.dogfood.test.ts' }, { id: 'rls-compiler-fail-closed', summary: 'uncompilable RLS predicate is surfaced/denied, not dropped', state: 'enforced', - enforcement: 'plugin-security/rls-compiler.ts isSupportedRlsExpression + warn' }, + enforcement: 'plugin-security/rls-compiler.ts compileFilter (drop + warn + RLS_DENY_FILTER) on the shape gate formula/rls-predicate.ts isSupportedRlsExpression — hoisted out of plugin-security in #4983 so lint/validate-rls-predicate-enforceability.ts can REJECT the same predicate at authoring time (ADR-0056 D4), from the one definition' }, { id: 'system-permissions', summary: 'systemPermissions / tab-app gating', state: 'enforced', enforcement: 'rest/rest-server.ts filterAppForUser' }, { id: 'secure-by-default-posture', summary: 'ADR-0066 ④ — sensitive system objects opt out of the wildcard grant (access.default: private)', state: 'enforced',