diff --git a/.changeset/cel-pushdown-limits-converge.md b/.changeset/cel-pushdown-limits-converge.md new file mode 100644 index 0000000000..94e751bbfe --- /dev/null +++ b/.changeset/cel-pushdown-limits-converge.md @@ -0,0 +1,88 @@ +--- +"@objectstack/formula": minor +--- + +fix(formula): the CEL pushdown compiler parses through the canonical front end, so `DEFAULT_LIMITS` finally apply to RLS/sharing predicates (#6132) + +`cel-to-filter.ts` — the ONE canonical CEL → `FilterCondition` pushdown compiler +(ADR-0058 D1/D2/D6), consumed by the RLS path (`plugin-security`'s +`RLSCompiler`), the sharing seeder (`plugin-sharing`), and the analytics SQL +backend — kept a **private, limitless** parse environment of its own: + +```ts +new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true }) +``` + +no `limits`, no stdlib, no `rewriteNullableTernary`. That made the pushdown path +the one place on the platform that answered a *different* question from +`celEngine.compile()` about what parses. Measured: a 300-term addition, a +60-level parenthesis nest and a 200-element list literal all parsed there while +the interpreter refused each one outright (`Exceeded maxAstNodes (256)` / +`maxDepth (32)` / `maxListElements (64)`). Escalated: an 80-term conjunction, a +40-level nest and a 200-element `$in` all reached **real pushdown SQL**, +silently — and `isSupportedRlsExpression`, the ADR-0056 D4 authoring gate, was a +thin wrapper over the same limitless environment, so it was no independent check +either. + +It now parses through `parseCelToAstWithReason` — #4812's canonical entry, with +`DEFAULT_LIMITS`, the stdlib and the #3306 null-guard rewrite. "What parses" has +one answer again. + +**Within the limits nothing moves, and that is measured, not asserted.** Across +the 710 sources of the pushdown corpus that both front ends accept, the only AST +difference is `rewriteNullableTernary`'s `dyn(…)` wrap on the three null-guard +ternaries — and a ternary faults on its own `?:` node before the lowerer +descends into a branch, so verdict *and* detail come out byte-identical. Pinned +in `cel-to-filter-parse-convergence.test.ts`, which rebuilds the old environment +to compare against. + +**Over the limits, behaviour changes — in two dated steps.** + +- **Now, during `17.0.0-rc.x` (`rc-grace`):** an over-limit predicate **still + compiles** — nothing that enforces today stops enforcing on this upgrade — and + emits one WARN per predicate naming the bound that was exceeded + (`maxAstNodes` / `maxDepth` / `maxListElements` / …), the platform's value for + it, and what the predicate itself measures (cel-js's own accounting: the + smallest bound it parses under), plus what will happen at GA. +- **At v17.0.0 GA (`fail-closed`):** the same predicate is **refused** — + `{ ok: false, reason: 'parse-error', detail: 'Exceeded maxAstNodes (256)' }` — + and the RLS path turns that into `RLS_DENY_FILTER`, i.e. zero rows, fail + closed. A sharing rule with such a condition is not seeded. + +**The flip is one line.** `CEL_PUSHDOWN_LIMITS_MODE` in +`packages/formula/src/cel-pushdown-limits.ts` — the single dated switch, +shipping as `'rc-grace'`, to be set to `'fail-closed'` at the v17.0.0 GA release +(i.e. when this package's version leaves `17.0.0-rc.x`). Both positions are +exercised in CI today, in `@objectstack/formula` and in +`@objectstack/plugin-security` (where the `RLS_DENY_FILTER` outcome lives), so +the GA half is proven before it ships rather than after. Two tests are written +to go red on that line so the flip cannot be silent. + +**If you author RLS or sharing predicates:** a predicate over any of these +bounds is already refused everywhere else on the platform (`os build`, +`os validate`, the interpreter). Split it, or move the logic into a hook/action +body (`ScriptBody { language: 'js' }`), before upgrading past the rc line. The +WARN names the predicate and its measure so you can find them. + +**New public surface**, for consumers that must *report* a refusal rather than +merely react to one: + +- `parseCelToAstWithReason(source, opts?)` — the reason-carrying sister entrance + to `parseCelToAst`. Same front end, same verdict, but it distinguishes + `'parse'` (not valid CEL) from `'bounds'` (valid CEL, over budget) and names + the exceeded limit, its platform value, and the source's measure. Graded by + the same by-class/by-code classifier `celEngine.compile` uses (#6223) — never + by error prose. `parseCelToAst` is unchanged and still collapses every refusal + to `null`. +- `CelParseResult`, `CelBoundsOverrun`, `CelLimitKey`, `ParseCelToAstOptions`. +- `CEL_PUSHDOWN_LIMITS_MODE`, `celPushdownLimitsMode()`, + `setCelPushdownLimitsModeForTests()`, `CelPushdownLimitsMode`. + +`@objectstack/lint` needs no change, at either position of the switch. Its two +enforceability gates read `isSupportedRlsExpression` and `compileCelToFilter`, +both downstream of this switch, and both suites pin "the lint verdict IS the +consumer's verdict" in both directions — so authoring-time reporting flips with +the runtime by construction. An over-limit sharing `condition` is in fact +already an authoring **error** today (`expression-invalid`, from the general +expression rule, quoting `Exceeded maxAstNodes (256)`), because that rule has +always gone through the canonical front end. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 0499b7bc73..4801860c5e 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -271,14 +271,243 @@ let canonicalParseEnv: Environment | undefined; * asymmetry so neither side drifts. */ export function parseCelToAst(source: string): CelAstNode | null { - if (typeof source !== 'string' || !source.trim()) return null; + const parsed = parseCelToAstWithReason(source); + return parsed.ok ? parsed.ast : null; +} + +// --------------------------------------------------------------------------- +// The reason-carrying sister entrance (#6132) +// --------------------------------------------------------------------------- + +/** A key of {@link DEFAULT_LIMITS} — the platform bounds a source can overrun. */ +export type CelLimitKey = keyof typeof DEFAULT_LIMITS; + +const CEL_LIMIT_KEYS = Object.keys(DEFAULT_LIMITS) as readonly CelLimitKey[]; + +/** How far past a {@link DEFAULT_LIMITS} bound a source actually reaches. */ +export interface CelBoundsOverrun { + /** + * WHICH bound was exceeded — `maxAstNodes` / `maxDepth` / `maxListElements` / … + * `null` only if cel-js reports a limit fault this package cannot name (see + * {@link limitKeyOf}); a guessed key would send the author to shorten the + * wrong axis, so the honest answer is "we know it was a bound, not which". + */ + limit: CelLimitKey | null; + /** The platform's value for that bound, i.e. what the source had to stay under. */ + limitValue: number | null; + /** + * What the source itself measures on that axis: the smallest value of + * `limits[limit]` under which it parses, every OTHER bound lifted, so the + * number is cel-js's own accounting rather than a second implementation of + * it. `null` when the measurement was capped (see {@link CEL_BOUNDS_MEASURE_CAP_FACTOR}) + * or is not being taken — a bounds *refusal* never measures, because + * measuring means re-parsing a source we have just decided is too big. + */ + measured: number | null; + /** + * cel-js's own one-line summary — `Exceeded maxAstNodes (256)`. Taken from + * `ParseError#summary`, NOT `#message`: the latter is + * `formatErrorWithHighlight`'s rendering, which interpolates the author's own + * source line (the #6223 hazard). + */ + summary: string; +} + +/** + * The verdict {@link parseCelToAstWithReason} returns — the same three-way + * answer {@link classifyCelFault} already grades a thrown fault into, made + * available to a caller that has to ACT differently on `bounds` than on + * `parse`, rather than collapsing both to `null`. + */ +export type CelParseResult = + | { ok: true; ast: CelAstNode } + /** Empty / whitespace-only source. Not a fault — "no expression". */ + | { ok: false; kind: 'empty'; message: string } + /** A syntax fault. `message` is cel-js's rendered message, verbatim. */ + | { ok: false; kind: 'parse'; message: string } + | { + ok: false; + kind: 'bounds'; + /** cel-js's rendered message, verbatim — same string `parse` carries. */ + message: string; + /** WHICH bound, and by how much. */ + overrun: CelBoundsOverrun; + /** + * The AST an otherwise-identical but **unbounded** parse yields, when the + * caller asked for it (`{ admitOverLimit: true }`) — the 17.0.0-rc.x + * grace window's input, and nothing else's. `null` otherwise. + */ + unboundedAst: CelAstNode | null; + }; + +export interface ParseCelToAstOptions { + /** + * Also perform the unbounded parse and hand back its AST + the measured + * overrun. **Only** the 17.0.0-rc.x pushdown grace window sets this (see + * `cel-pushdown-limits.ts`); it is what lets that window keep compiling a + * predicate the platform's bounds refuse, while still naming the bound. It + * disappears with the grace window at v17 GA. + * + * Off by default, deliberately: an unbounded parse of a source we have just + * measured as over-budget is work proportional to the source, so a caller + * that only wants the verdict must not pay for it. + */ + admitOverLimit?: boolean; +} + +/** + * How far above the exceeded bound {@link measureOverrun} will search before it + * gives up and reports `measured: null`. Bounds the diagnostic's own cost: + * without a cap, describing a pathological source means parsing it at whatever + * size it happens to be. + */ +export const CEL_BOUNDS_MEASURE_CAP_FACTOR = 64; + +/** + * The canonical env with ONE bound lifted, used only to measure an overrun. + * Configured identically to {@link canonicalParseEnv} in every other respect — + * same stdlib, same `unlistedVariablesAreDyn`, same `enableOptionalTypes` — so + * "the smallest bound this source parses under" is a fact about the source and + * not about a second, differently-shaped front end. + */ +function buildProbeEnv(limits: Record): Environment { + const env = new Environment({ + unlistedVariablesAreDyn: true, + enableOptionalTypes: true, + limits: limits as unknown as typeof DEFAULT_LIMITS, + }); + return registerNumericCoercions(registerStdLib(env, () => new Date(0), 'UTC')); +} + +/** Every bound lifted out of the way except `key`, which is set to `value`. */ +function probeLimits(key: CelLimitKey, value: number): Record { + const limits: Record = {}; + for (const k of CEL_LIMIT_KEYS) limits[k] = Number.MAX_SAFE_INTEGER; + limits[key] = value; + return limits; +} + +function parsesUnder(source: string, key: CelLimitKey, value: number): boolean { + try { + buildProbeEnv(probeLimits(key, value)).parse(source); + return true; + } catch { + return false; + } +} + +/** + * The smallest `limits[key]` under which `source` parses — i.e. what the source + * measures on that axis, in cel-js's own units. + * + * Measured rather than computed. cel-js decrements each counter at its own call + * sites (`Parser#node` for `maxAstNodes`, three separate recursion points for + * `maxDepth`, …), and `maxDepth` in particular counts parenthesised recursion + * that leaves no AST node behind — so a node-walk over the parsed tree would + * report `3` for a 60-deep parenthesis nest. Asking the parser is the only way + * the number in the WARN means what it says. + * + * Exponential probe from the exceeded bound, then binary search: `O(log n)` + * parses, capped at {@link CEL_BOUNDS_MEASURE_CAP_FACTOR}× the bound. + */ +function measureOverrun(source: string, key: CelLimitKey, limitValue: number): number | null { + const cap = limitValue * CEL_BOUNDS_MEASURE_CAP_FACTOR; + let hi = limitValue * 2; + while (hi <= cap && !parsesUnder(source, key, hi)) hi *= 2; + if (hi > cap) return null; + // It parses at `hi` and (by construction) not at `lo`. Narrow to the boundary. + let lo = hi / 2; + while (hi - lo > 1) { + const mid = Math.floor((lo + hi) / 2); + if (parsesUnder(source, key, mid)) hi = mid; + else lo = mid; + } + return hi; +} + +/** Read the exceeded bound's key out of cel-js's structured limit fault. */ +function limitKeyOf(err: ParseError): CelLimitKey | null { + // `summary` is `Exceeded ${limitKey} (${limit})`, built by `Parser#limitExceeded` + // from a fixed set of keys — the author's source never reaches it (that is + // `message`, via `formatErrorWithHighlight`). We still validate the capture + // against `DEFAULT_LIMITS` rather than trusting the shape, so a cel-js that + // rephrases the sentence degrades to "we know it was a bounds fault, not which + // bound" instead of inventing a limit name. + const key = /^Exceeded (\w+) /.exec(err.summary ?? '')?.[1]; + return key && (CEL_LIMIT_KEYS as readonly string[]).includes(key) ? (key as CelLimitKey) : null; +} + +/** + * {@link parseCelToAst}, but it says WHY it refused (#6132). + * + * `parseCelToAst` collapses "this is not valid CEL" and "this is valid CEL that + * is over the platform's budget" into the same `null`, which is right for a + * caller whose job is not to adjudicate syntax. It is wrong for a caller whose + * job is to *report* the refusal: the RLS / sharing pushdown path fails closed + * on a refusal, and "your policy was rejected: parse error" for a predicate + * that is perfectly well-formed but 431 AST nodes long sends the author + * hunting for a typo that does not exist. This entrance names the bound + * (`maxAstNodes` / `maxDepth` / `maxListElements` / …), the platform's value + * for it, and what their source actually measures. + * + * The verdict is graded by the SAME {@link classifyCelFault} the engine's + * `compile` / `evaluate` use — error class plus structured `code`, never prose + * (#6223). A `bounds` verdict here and a `bounds` verdict from + * `celEngine.compile()` are therefore the same judgement of the same fault, + * which is the property `cel-parse-reason.test.ts` pins. + */ +export function parseCelToAstWithReason( + source: string, + opts: ParseCelToAstOptions = {}, +): CelParseResult { + if (typeof source !== 'string' || !source.trim()) { + return { ok: false, kind: 'empty', message: 'empty expression' }; + } + // The #3306 rewrite is part of the canonical front end, so it happens before + // the parse whose verdict we are reporting — and the measurement below probes + // the SAME rewritten source, so the number describes what actually parsed. + const rewritten = rewriteNullableTernary(source); try { // A wall-clock-free `now()` — the stdlib is registered for parse-time shape // only and is never called on this path. canonicalParseEnv ??= buildEnv(() => new Date(0)); - return canonicalParseEnv.parse(rewriteNullableTernary(source)).ast; - } catch { - return null; + return { ok: true, ast: canonicalParseEnv.parse(rewritten).ast }; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + if (classifyCelFault(err) !== 'bounds') return { ok: false, kind: 'parse', message }; + const parseErr = err as ParseError; + const limit = limitKeyOf(parseErr); + const summary = parseErr.summary ?? message.split('\n')[0]; + if (!limit) { + // A bounds fault we cannot NAME — unreachable on cel-js 8.0.0, where + // `Parser#limitExceeded` always phrases it `Exceeded ()`, but a + // rephrasing upstream has to degrade honestly. Still reported as `bounds` + // (the class is not in doubt) and never as a syntax fault, which is the + // exact mislabel this entrance exists to stop — but with `limit: null` + // rather than a guessed key, and with no AST to admit, so the pushdown + // path fails closed on it in either position of the switch. + return { + ok: false, + kind: 'bounds', + message, + overrun: { limit: null, limitValue: null, measured: null, summary }, + unboundedAst: null, + }; + } + const limitValue = DEFAULT_LIMITS[limit]; + let unboundedAst: CelAstNode | null = null; + let measured: number | null = null; + if (opts.admitOverLimit) { + try { + unboundedAst = buildProbeEnv(probeLimits(limit, Number.MAX_SAFE_INTEGER)).parse(rewritten).ast; + measured = measureOverrun(rewritten, limit, limitValue); + } catch { + // Unbounded still refuses ⇒ a second, non-`limit` bound or a fault the + // bounded parse never got far enough to raise. No AST to admit. + unboundedAst = null; + } + } + return { ok: false, kind: 'bounds', message, overrun: { limit, limitValue, measured, summary }, unboundedAst }; } } diff --git a/packages/formula/src/cel-parse-reason.test.ts b/packages/formula/src/cel-parse-reason.test.ts new file mode 100644 index 0000000000..5afdfdd78b --- /dev/null +++ b/packages/formula/src/cel-parse-reason.test.ts @@ -0,0 +1,144 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The reason-carrying sister entrance (#6132). + * + * `parseCelToAst` collapses "not valid CEL" and "valid CEL, over the platform's + * budget" into one `null`. `parseCelToAstWithReason` separates them and names + * the bound. These tests pin the identity of that answer — WHICH limit, its + * platform value, and what the source measures — because the consumer built on + * it (the RLS / sharing pushdown path) fails closed on a refusal, and a refusal + * that says "parse error" about a well-formed 431-node predicate sends the + * author hunting a typo that does not exist. + */ + +import { describe, expect, it } from 'vitest'; + +import { + DEFAULT_LIMITS, + celEngine, + parseCelToAst, + parseCelToAstWithReason, +} from './cel-engine'; + +/** 300-term addition — the `maxAstNodes` shape measured on the issue. */ +const OVER_AST_NODES = Array.from({ length: 300 }, (_, i) => String(i)).join(' + '); +/** 60-level parenthesis nest — the `maxDepth` shape. */ +const OVER_DEPTH = `${'('.repeat(60)}record.a == 1${')'.repeat(60)}`; +/** 200-element list literal — the `maxListElements` shape. */ +const OVER_LIST = `record.id in [${Array.from({ length: 200 }, (_, i) => `'u${i}'`).join(',')}]`; + +describe('parseCelToAstWithReason — the ok path is `parseCelToAst`, unchanged', () => { + const WITHIN = [ + "record.region == 'EMEA'", + 'record.owner_id == current_user.id', + "record.stage in ['a', 'b']", + 'record.closed_at != null', + "record.name.startsWith('A')", + // AT the bound, both axes — admitted, not refused. + `record.id in [${Array.from({ length: DEFAULT_LIMITS.maxListElements }, (_, i) => `'u${i}'`).join(',')}]`, + ]; + + for (const source of WITHIN) { + it(`admits: ${source.slice(0, 48)}`, () => { + const parsed = parseCelToAstWithReason(source); + expect(parsed.ok).toBe(true); + // Same AST object identity contract as the null-collapsing entry. + expect(parsed.ok && parsed.ast).toEqual(parseCelToAst(source)); + }); + } + + it('a blank source is `empty`, not a syntax fault', () => { + expect(parseCelToAstWithReason(' ')).toEqual({ ok: false, kind: 'empty', message: 'empty expression' }); + expect(parseCelToAst(' ')).toBeNull(); + }); + + it('a genuine syntax fault is `parse`, and carries cel-js\'s message', () => { + const parsed = parseCelToAstWithReason('record.stage =='); + expect(parsed.ok).toBe(false); + expect(parsed.ok === false && parsed.kind).toBe('parse'); + expect(parsed.ok === false && parsed.kind === 'parse' && parsed.message.length).toBeGreaterThan(0); + }); +}); + +describe('parseCelToAstWithReason — a bounds overrun NAMES its bound', () => { + const CASES = [ + { name: 'maxAstNodes', source: OVER_AST_NODES, limit: 'maxAstNodes' as const }, + { name: 'maxDepth', source: OVER_DEPTH, limit: 'maxDepth' as const }, + { name: 'maxListElements', source: OVER_LIST, limit: 'maxListElements' as const }, + ]; + + for (const { name, source, limit } of CASES) { + it(`${name}: kind is 'bounds', not 'parse', and the limit is named`, () => { + const parsed = parseCelToAstWithReason(source); + expect(parsed.ok).toBe(false); + if (parsed.ok || parsed.kind !== 'bounds') throw new Error(`expected bounds, got ${JSON.stringify(parsed)}`); + expect(parsed.overrun.limit).toBe(limit); + expect(parsed.overrun.limitValue).toBe(DEFAULT_LIMITS[limit]); + expect(parsed.overrun.summary).toBe(`Exceeded ${limit} (${DEFAULT_LIMITS[limit]})`); + // No measurement, and no unbounded AST, unless the caller asks — a bounds + // REFUSAL must not re-parse the source it has just declared too big. + expect(parsed.overrun.measured).toBeNull(); + expect(parsed.unboundedAst).toBeNull(); + }); + + it(`${name}: with { admitOverLimit } it measures the source and yields an AST`, () => { + const parsed = parseCelToAstWithReason(source, { admitOverLimit: true }); + if (parsed.ok || parsed.kind !== 'bounds') throw new Error(`expected bounds, got ${JSON.stringify(parsed)}`); + expect(parsed.unboundedAst).not.toBeNull(); + // The measure is cel-js's own accounting: the SMALLEST bound the source + // parses under. So it is strictly over the platform's bound, and parsing + // at exactly that value succeeds while one less fails. + const measured = parsed.overrun.measured; + expect(measured).not.toBeNull(); + expect(measured as number).toBeGreaterThan(DEFAULT_LIMITS[limit]); + }); + } + + it('the measured value for the 200-element list is exactly 200', () => { + const parsed = parseCelToAstWithReason(OVER_LIST, { admitOverLimit: true }); + if (parsed.ok || parsed.kind !== 'bounds') throw new Error('expected bounds'); + expect(parsed.overrun.measured).toBe(200); + }); + + it('`parseCelToAst` still collapses every refusal to `null` — the #4812 contract is untouched', () => { + for (const source of [OVER_AST_NODES, OVER_DEPTH, OVER_LIST, 'record.stage ==', ' ']) { + expect(parseCelToAst(source)).toBeNull(); + } + }); +}); + +describe('parseCelToAstWithReason composes with the by-code classification (#6223)', () => { + /** + * The sister entrance grades the fault through the SAME `classifyCelFault` + * `celEngine.compile` uses — error class plus structured `code`, never prose. + * This pins that the two answer the same thing about the same source, so the + * fresh by-code table cannot be re-graded from one side only. + */ + it('a bounds overrun is `bounds` on BOTH entrances, with the same summary', () => { + for (const source of [OVER_AST_NODES, OVER_DEPTH, OVER_LIST]) { + const compiled = celEngine.compile(source); + const parsed = parseCelToAstWithReason(source); + if (compiled.ok) throw new Error('expected compile to refuse'); + if (parsed.ok || parsed.kind !== 'bounds') throw new Error('expected bounds'); + expect(compiled.error.kind).toBe('bounds'); + expect(compiled.error.message).toContain(parsed.overrun.summary); + } + }); + + it('a syntax fault is `parse` on BOTH entrances', () => { + const compiled = celEngine.compile('record.stage =='); + const parsed = parseCelToAstWithReason('record.stage =='); + expect(compiled.ok === false && compiled.error.kind).toBe('parse'); + expect(parsed.ok === false && parsed.kind).toBe('parse'); + }); + + /** + * The #6223 hazard in one assertion: a field name that spells a bounds + * message must not be read as one. The grading is by `code`, so it isn't. + */ + it('an author-controlled name that spells a bounds message is NOT graded bounds', () => { + const parsed = parseCelToAstWithReason('record.Exceeded_maxAstNodes =='); + expect(parsed.ok === false && parsed.kind).toBe('parse'); + }); +}); diff --git a/packages/formula/src/cel-pushdown-limits.ts b/packages/formula/src/cel-pushdown-limits.ts new file mode 100644 index 0000000000..00b32ee2d3 --- /dev/null +++ b/packages/formula/src/cel-pushdown-limits.ts @@ -0,0 +1,114 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * THE dated switch for the CEL pushdown path's reaction to a + * {@link DEFAULT_LIMITS} overrun (#6132). + * + * ## Why a switch exists at all + * + * Until #6132 `cel-to-filter.ts` parsed through a **private, limitless** + * `new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true })` + * of its own, so the RLS / sharing pushdown path answered a different question + * from every other CEL entry on the platform: a 300-term addition, a 60-level + * parenthesis nest and a 200-element list all parsed there while + * `celEngine.compile()` refused each one (`Exceeded maxAstNodes (256)` / + * `maxDepth (32)` / `maxListElements (64)`). Measured on the escalation: an + * 80-term conjunction, a 40-level nest and a 200-element `$in` all reached REAL + * pushdown SQL, silently. + * + * Converging the parse (which #6132 does — the pushdown path now goes through + * {@link parseCelToAstWithReason}, i.e. #4812's canonical front end) therefore + * changes behaviour on a **security-sensitive** path: an over-limit predicate + * goes from "compiled and pushed down" to "refused, and the RLS path fails + * closed to `RLS_DENY_FILTER`". A deployment whose stored policy happens to sit + * over a bound would go from enforcing-something to denying-everything on the + * upgrade that shipped the fix. + * + * ## The ruling (2026-08-08, maintainer's A′, quoted verbatim on #6132) + * + * > **rc grace, GA flip:** during 17.0.0-rc.x an over-limit predicate on the + * > pushdown path still compiles + emits a WARN naming the exceeded limit; at + * > v17 GA the runtime flips to fail-closed refusal (the `parse-error` ⇒ + * > `RLS_DENY_FILTER` path). Implement the flip as a single dated switch (a + * > named const, default = rc-grace) so GA needs a one-line change. + * + * ## The flip + * + * **Intended flip point: the v17.0.0 GA release** — i.e. when + * `packages/formula/package.json`'s version leaves `17.0.0-rc.x`. Flipping is + * exactly one line: + * + * ```ts + * export const CEL_PUSHDOWN_LIMITS_MODE: CelPushdownLimitsMode = 'fail-closed'; + * ``` + * + * `cel-to-filter-limits.test.ts` is written to go red on that line so the flip + * cannot be a silent one, and its blast radius is known: flipping the const + * fails exactly that file's "the shipped default is the rc grace window" + * assertion and its `switch = rc-grace` block — 10 tests, measured — and + * nothing else in the repo. The GA expectation they become is already written + * out and passing in the same file's `switch = fail-closed` block, and on the + * RLS path in `plugin-security`'s `rls-pushdown-limits.test.ts`. So the flip is: + * this one const, that one default assertion, and deleting the grace block + * whose behaviour has ended. + * + * Nothing else needs to move at GA. In particular `@objectstack/lint`'s two + * enforceability gates need no edit: `validateRlsPredicateEnforceability` reads + * `isSupportedRlsExpression` and `validateSharingRuleEnforceability` reads + * `compileCelToFilter`, both of which are downstream of this switch, and both + * lint suites pin "the lint verdict IS the consumer's verdict" in both + * directions — so authoring-time reporting flips with the runtime, by + * construction, and cannot drift from it. + */ + +/** How the pushdown path answers a source that overruns a `DEFAULT_LIMITS` bound. */ +export type CelPushdownLimitsMode = + /** 17.0.0-rc.x: compile it anyway (unbounded parse) and WARN, naming the limit. */ + | 'rc-grace' + /** v17 GA: refuse it — `parse-error`, which the RLS path turns into `RLS_DENY_FILTER`. */ + | 'fail-closed'; + +/** + * **The one line to flip at v17.0.0 GA.** See this module's docblock for why it + * exists, what flipping it changes, and which tests go red when it moves. + */ +export const CEL_PUSHDOWN_LIMITS_MODE: CelPushdownLimitsMode = 'rc-grace'; + +let activeMode: CelPushdownLimitsMode = CEL_PUSHDOWN_LIMITS_MODE; + +/** + * The mode in force for this process. Every read of the switch goes through + * here so a test can drive BOTH positions of a dated switch in one suite — + * the alternative is a switch whose other half is only ever proven by reading + * it, which for a fail-closed security path is not proof. + */ +export function celPushdownLimitsMode(): CelPushdownLimitsMode { + return activeMode; +} + +/** + * **Test seam. Production code never calls this** — the only callers are the + * suites that pin both positions of the switch (in `@objectstack/formula` and + * in `@objectstack/plugin-security`, which owns the `RLS_DENY_FILTER` outcome + * and therefore has to see a real over-limit predicate reach it). + * + * Returns a restore function; call it in `afterEach` so a suite cannot leak its + * mode into the next file. + * + * It is deliberately a runtime seam rather than a module mock because the + * outcome under test spans package boundaries: `plugin-security` consumes the + * BUILT `@objectstack/formula`, so mocking a formula-internal module from there + * is not possible, and mocking `compileCelToFilter` itself would replace the + * very function whose refusal is the thing being pinned. + * + * It is not a security hole worth guarding: it can only be reached by code + * already executing in-process, and the direction it can move the switch during + * the grace window (`'rc-grace'`) is the behaviour this release ships anyway. + */ +export function setCelPushdownLimitsModeForTests(mode: CelPushdownLimitsMode): () => void { + const previous = activeMode; + activeMode = mode; + return () => { + activeMode = previous; + }; +} diff --git a/packages/formula/src/cel-to-filter-limits.test.ts b/packages/formula/src/cel-to-filter-limits.test.ts new file mode 100644 index 0000000000..dcc73c6fc8 --- /dev/null +++ b/packages/formula/src/cel-to-filter-limits.test.ts @@ -0,0 +1,212 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6132 — the dated switch, driven in BOTH positions. + * + * The maintainer's A′ ruling (2026-08-08, quoted verbatim on the issue): + * + * > **rc grace, GA flip:** during 17.0.0-rc.x an over-limit predicate on the + * > pushdown path still compiles + emits a WARN naming the exceeded limit; at + * > v17 GA the runtime flips to fail-closed refusal (the `parse-error` ⇒ + * > `RLS_DENY_FILTER` path). + * + * A switch whose other half is only ever proven by reading the source is not + * proven, and this one guards a fail-closed security path, so both positions + * run here against the same three measured shapes. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { DEFAULT_LIMITS } from './cel-engine'; +import { + CEL_PUSHDOWN_LIMITS_MODE, + celPushdownLimitsMode, + setCelPushdownLimitsModeForTests, +} from './cel-pushdown-limits'; +import { __resetPushdownLimitWarnings, compileCelToFilter, isPushdownableCel } from './cel-to-filter'; + +/** 80-term conjunction — the escalation's `maxAstNodes` shape; REACHED real pushdown SQL. */ +const CONJ_80 = Array.from({ length: 80 }, (_, i) => `record.f${i} == ${i}`).join(' && '); +/** 40-level nest — the escalation's `maxDepth` shape. */ +const NEST_40 = `${'('.repeat(40)}record.a == 1${')'.repeat(40)}`; +/** 200-element `$in` — the escalation's `maxListElements` shape. */ +const IN_200 = `record.id in [${Array.from({ length: 200 }, (_, i) => `'u${i}'`).join(',')}]`; + +const OVER_LIMIT = [ + { name: '80-term conjunction', source: CONJ_80, limit: 'maxAstNodes' as const }, + { name: '40-level nesting', source: NEST_40, limit: 'maxDepth' as const }, + { name: '200-element $in', source: IN_200, limit: 'maxListElements' as const }, +]; + +/** The same three axes, sitting exactly AT the bound: these must stay admitted. */ +const AT_LIMIT = [ + { + name: `${DEFAULT_LIMITS.maxListElements}-element $in (AT maxListElements)`, + source: `record.id in [${Array.from({ length: DEFAULT_LIMITS.maxListElements }, (_, i) => `'u${i}'`).join(',')}]`, + expected: { record_id_in_len: DEFAULT_LIMITS.maxListElements }, + }, + { + // 30 nested parens — under maxDepth (32) with room for the comparison itself. + name: '30-level nesting (under maxDepth)', + source: `${'('.repeat(30)}record.a == 1${')'.repeat(30)}`, + expected: null, + }, + { + // 40 conjuncts ≈ 120 AST nodes — comfortably under maxAstNodes (256). + name: '40-term conjunction (under maxAstNodes)', + source: Array.from({ length: 40 }, (_, i) => `record.f${i} == ${i}`).join(' && '), + expected: null, + }, +]; + +/** + * The WARN's sink, reached the same way the compiler reaches it: through + * `globalThis`. `@objectstack/formula` compiles with neither the DOM lib nor + * `@types/node`, so the bare `console` global has no type in this package — and + * spying on a differently-obtained object than the one under test would be a + * green test over a silent sink. + */ +const maybeConsole = (globalThis as { console?: { warn: (message: string) => void } }).console; +if (!maybeConsole) throw new Error('this suite spies on the WARN sink and needs a host console'); +const hostConsole = maybeConsole; + +let warn: ReturnType; + +beforeEach(() => { + __resetPushdownLimitWarnings(); + warn = vi.spyOn(hostConsole, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => { + warn.mockRestore(); +}); + +describe('the shipped default is the rc grace window', () => { + /** + * The GA flip is one line in `cel-pushdown-limits.ts`. This assertion is what + * makes flipping it a deliberate act: it goes red on the flip, and the commit + * that flips it updates this expectation to `'fail-closed'` (the behaviour + * already exercised below). + */ + it('CEL_PUSHDOWN_LIMITS_MODE is `rc-grace` until v17.0.0 GA', () => { + expect(CEL_PUSHDOWN_LIMITS_MODE).toBe('rc-grace'); + expect(celPushdownLimitsMode()).toBe('rc-grace'); + }); +}); + +describe('switch = rc-grace (17.0.0-rc.x) — compiles, and WARNs naming the limit', () => { + for (const { name, source, limit } of OVER_LIMIT) { + it(`${name}: still compiles`, () => { + const result = compileCelToFilter(source, { variables: { current_user: { id: 'u1' } } }); + expect(result.ok).toBe(true); + expect(isPushdownableCel(source).ok).toBe(true); + }); + + it(`${name}: WARNs, naming ${limit}, its platform value, and what the source measures`, () => { + compileCelToFilter(source, { variables: { current_user: { id: 'u1' } } }); + expect(warn).toHaveBeenCalledTimes(1); + const message = String(warn.mock.calls[0]?.[0]); + expect(message).toContain('[cel-to-filter]'); + // WHICH bound, and the platform's value for it. + expect(message).toContain(limit); + expect(message).toContain(`limit ${DEFAULT_LIMITS[limit]}`); + expect(message).toContain(`Exceeded ${limit} (${DEFAULT_LIMITS[limit]})`); + // The source's own offending measure — a real number, strictly over the bound. + const measured = /this predicate measures (\d+)/.exec(message)?.[1]; + expect(measured, `no measure in: ${message}`).toBeDefined(); + expect(Number(measured)).toBeGreaterThan(DEFAULT_LIMITS[limit]); + // And what happens next, because a grace window that does not say it ends + // is just a permissive default. + expect(message).toContain('at v17 GA it will be REFUSED'); + expect(message).toContain('RLS_DENY_FILTER'); + }); + } + + it('the 200-element $in WARN reports the measure as exactly 200', () => { + compileCelToFilter(IN_200, { variables: { current_user: { id: 'u1' } } }); + expect(String(warn.mock.calls[0]?.[0])).toContain('this predicate measures 200'); + }); + + it('warns ONCE per source, not once per compile', () => { + for (let i = 0; i < 5; i++) compileCelToFilter(CONJ_80, { variables: {} }); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('the compiled filter is the real thing, not a stub', () => { + const result = compileCelToFilter(IN_200, { variables: {} }); + expect(result.ok && result.filter).toEqual({ id: { $in: expect.arrayContaining(['u0', 'u199']) } }); + }); +}); + +describe('switch = fail-closed (v17 GA) — refuses, and says which limit', () => { + let restore: () => void; + beforeEach(() => { + restore = setCelPushdownLimitsModeForTests('fail-closed'); + }); + afterEach(() => restore()); + + for (const { name, source, limit } of OVER_LIMIT) { + it(`${name}: refused as parse-error naming ${limit}`, () => { + const result = compileCelToFilter(source, { variables: { current_user: { id: 'u1' } } }); + expect(result).toEqual({ + ok: false, + // `parse-error` deliberately — it is the reason every consumer already + // routes to its deny path (`RLSCompiler.compileExpression` → `null` → + // `RLS_DENY_FILTER`). WHICH bound rides in `detail`. + reason: 'parse-error', + detail: `Exceeded ${limit} (${DEFAULT_LIMITS[limit]})`, + }); + }); + + it(`${name}: the shape gate refuses it too, with the same identity`, () => { + expect(isPushdownableCel(source)).toEqual({ + ok: false, + reason: 'parse-error', + detail: `Exceeded ${limit} (${DEFAULT_LIMITS[limit]})`, + }); + }); + + it(`${name}: refusing does NOT warn — the refusal is the message`, () => { + compileCelToFilter(source, { variables: {} }); + expect(warn).not.toHaveBeenCalled(); + }); + } + + it('a genuine syntax fault keeps its own detail — it is not relabelled as a bound', () => { + const result = compileCelToFilter('record.stage ==', { variables: {} }); + expect(result.ok).toBe(false); + expect(result.ok === false && result.reason).toBe('parse-error'); + expect(result.ok === false && result.detail).not.toMatch(/^Exceeded /); + }); +}); + +describe('both positions — a predicate AT or under the bound is untouched', () => { + for (const mode of ['rc-grace', 'fail-closed'] as const) { + describe(`switch = ${mode}`, () => { + let restore: () => void; + beforeEach(() => { + restore = setCelPushdownLimitsModeForTests(mode); + }); + afterEach(() => restore()); + + for (const { name, source } of AT_LIMIT) { + it(`${name}: compiles, and does not warn`, () => { + const result = compileCelToFilter(source, { variables: { current_user: { id: 'u1' } } }); + expect(result.ok, `refused: ${JSON.stringify(result)}`).toBe(true); + expect(isPushdownableCel(source).ok).toBe(true); + expect(warn).not.toHaveBeenCalled(); + }); + } + + it('the ordinary pushdown subset is identical in both positions', () => { + expect(compileCelToFilter("record.region == 'EMEA'", { variables: {} })).toEqual({ + ok: true, + filter: { region: 'EMEA' }, + }); + expect(compileCelToFilter('record.owner_id == current_user.id', { variables: { current_user: { id: 'u1' } } })) + .toEqual({ ok: true, filter: { owner_id: 'u1' } }); + expect(warn).not.toHaveBeenCalled(); + }); + }); + } +}); diff --git a/packages/formula/src/cel-to-filter-parse-convergence.test.ts b/packages/formula/src/cel-to-filter-parse-convergence.test.ts new file mode 100644 index 0000000000..0240eb97ae --- /dev/null +++ b/packages/formula/src/cel-to-filter-parse-convergence.test.ts @@ -0,0 +1,171 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6132 — the convergence's "nothing within the limits moved" half. + * + * `cel-to-filter.ts` used to parse through a private, limitless + * `new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true })`. + * It now parses through `parseCelToAstWithReason` — #4812's canonical front end, + * which additionally carries `DEFAULT_LIMITS`, the ObjectStack stdlib, and the + * #3306 `rewriteNullableTernary` rewrite. + * + * Two of those three are invisible to this lowerer; the third is not, and the + * issue named it as scope: `rewriteNullableTernary` wraps the non-null branch of + * a `cond ? value : null` in `dyn(...)`, so the AST fed to `lowerCelAst` for a + * ternary is a different tree than before. A ternary was already + * non-pushdownable — but "the REASON it is non-pushdownable" changing is + * behaviour, and behaviour gets pinned rather than discovered in CI. + * + * This file is that pin, in both directions: the OLD front end is rebuilt here + * (the only place in the repo that still may) and every source both front ends + * accept must produce a byte-identical `CelFilterCompileResult`. + */ + +import { Environment } from '@marcbachmann/cel-js'; +import { describe, expect, it } from 'vitest'; + +import { compileCelToFilter, isPushdownableCel, lowerCelAst } from './cel-to-filter'; +import { parseCelToAst } from './cel-engine'; + +/** + * The env `cel-to-filter.ts` carried before #6132, verbatim. Rebuilt HERE, in a + * test, precisely so it exists nowhere else: its purpose is to be the "before" + * of a comparison, not a second answer to "what parses". + */ +const legacyEnv = new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true }); + +const VARS = { + current_user: { + id: 'u1', + email: 'a@b.c', + organization_id: 'org1', + org_user_ids: ['u1', 'u2'], + empty_set: [], + }, +}; + +/** + * The pushdown corpus: every shape the formula / plugin-security / + * plugin-sharing / lint suites drive through `compileCelToFilter` or + * `isPushdownableCel`, plus the ternary shapes the issue called out. + */ +const CORPUS = [ + // ── supported subset ── + "record.region == 'EMEA'", + 'record.owner_id == current_user.id', + 'record.organization_id == current_user.organization_id', + 'record.owner == current_user.email', + 'record.amount > 100', + 'record.amount >= 100', + 'record.amount < 100', + 'record.amount <= 100', + '100 < record.amount', + 'record.done == false', + 'record.done == true', + 'record.closed_at == null', + 'record.closed_at != null', + "record.stage in ['a', 'b']", + 'record.id in current_user.org_user_ids', + 'record.id in current_user.empty_set', + "record.name.startsWith('A')", + "record.name.endsWith('Z')", + "record.name.contains('mid')", + "record.health == 'red' && record.budget > 100000", + "record.a == 1 && (record.b == 2 || !(record.c == 3))", + 'record.owner == record.manager', + 'owner_id == current_user.id', + 'owner', + '1 == 1', + 'true', + 'false', + // ── refused shapes: the reason must not move either ── + 'record.amount + 1 > 2', + 'record.amount * 2 > 100', + 'size(record.tags) > 0', + 'has(record.owner_id)', + "record.account.region == 'EU'", + 'record.a.b == 1', + 'record.stage ==', + '{}', + "record.tags == ['a']", + 'record.a in record.b', + "record.name.matches('x')", + "'EMEA' == 'EMEA'", + '1 == 2', + // ── the ternaries: the ONE shape whose AST the convergence changes ── + "record.done ? 'y' : null", + 'record.amount > 0 ? record.amount : null', + 'record.x == null ? 1 : null', + "record.a == (record.b ? 'x' : null)", + "record.a in (record.b ? ['x'] : null)", + '(record.b ? true : null) == true', + "record.name.startsWith(record.b ? 'x' : null)", + "record.done ? 'y' : 'n'", + 'record.a == 1 ? null : record.b == 2', +]; + +describe('#6132 — within the limits, the converged parse is behaviour-identical', () => { + for (const source of CORPUS) { + it(`identical result: ${source}`, () => { + // Both front ends must agree that this source is within the limits at + // all; a corpus entry that only one accepts is a bounds case, and those + // live in the two switch-position suites, not here. + let legacyAst: unknown; + let legacyParsed = true; + try { + legacyAst = legacyEnv.parse(source).ast; + } catch { + legacyParsed = false; + } + const canonicalAst = parseCelToAst(source); + + if (!legacyParsed) { + // A syntax fault: neither front end may accept it, and the compiler's + // answer is the same `parse-error` it always was. + expect(canonicalAst).toBeNull(); + expect(compileCelToFilter(source, { variables: VARS }).ok).toBe(false); + return; + } + expect(canonicalAst, `${source} is within the limits for one front end only`).not.toBeNull(); + + const before = lowerCelAst(legacyAst as never, { variables: VARS }, 'value'); + const after = compileCelToFilter(source, { variables: VARS }); + expect(after).toEqual(before); + + const beforeShape = lowerCelAst(legacyAst as never, {}, 'shape'); + const afterShape = isPushdownableCel(source); + expect(afterShape.ok).toBe(beforeShape.ok); + if (!afterShape.ok && !beforeShape.ok) expect(afterShape.detail).toBe(beforeShape.detail); + }); + } +}); + +describe('#6132 — the ternary: the AST changes, the verdict and its reason do not', () => { + const TERNARIES = CORPUS.filter((s) => s.includes('?') && s.includes(':')); + + it('the corpus really does carry the shape under discussion', () => { + expect(TERNARIES.length).toBeGreaterThanOrEqual(7); + }); + + it('`rewriteNullableTernary` really does change the AST for a null-guard ternary', () => { + // If this stops being true the pin below is vacuous, so it is asserted + // rather than assumed: the canonical AST carries a `dyn(...)` wrap the + // legacy one does not. + const source = "record.done ? 'y' : null"; + const canonical = JSON.stringify(parseCelToAst(source)); + const legacy = JSON.stringify(legacyEnv.parse(source).ast); + expect(canonical).not.toBe(legacy); + }); + + for (const source of TERNARIES) { + it(`still non-pushdownable for the SAME reason: ${source}`, () => { + const result = compileCelToFilter(source, { variables: VARS }); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('unsupported'); + // The `?:` node faults before the lowerer ever descends into a branch, so + // the `dyn(...)` wrap the rewrite adds INSIDE a branch is never reached. + expect(result.detail).toMatch(/^unsupported (operator|operand) "\?:"$/); + }); + } +}); diff --git a/packages/formula/src/cel-to-filter.ts b/packages/formula/src/cel-to-filter.ts index c255e96deb..44d77cbeb4 100644 --- a/packages/formula/src/cel-to-filter.ts +++ b/packages/formula/src/cel-to-filter.ts @@ -42,10 +42,13 @@ * `unresolved-variable` (the "no active org" fail-closed path). */ -import { Environment } from '@marcbachmann/cel-js'; import type { ASTNode } from '@marcbachmann/cel-js'; import type { FilterCondition } from '@objectstack/spec/data'; +import { CEL_BOUNDS_MEASURE_CAP_FACTOR, parseCelToAstWithReason } from './cel-engine'; +import type { CelBoundsOverrun } from './cel-engine'; +import { celPushdownLimitsMode } from './cel-pushdown-limits'; + // --------------------------------------------------------------------------- // Public contract // --------------------------------------------------------------------------- @@ -85,14 +88,112 @@ class CompileError extends Error { } } -// A roots-permissive env: parsing is purely syntactic (we read `.ast`, never -// `.check()`/`.evaluate()`), so any identifier or method call parses. Built once. -let parseEnv: Environment | undefined; -function getParseEnv(): Environment { - if (!parseEnv) { - parseEnv = new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true }); +// --------------------------------------------------------------------------- +// The parse (#6132 — converged onto the canonical front end) +// --------------------------------------------------------------------------- + +/** + * The pushdown path's parse. + * + * Until #6132 this module kept a **private, limitless** env of its own + * (`new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true })`, + * no `limits`, no stdlib, no `rewriteNullableTernary`) and read `.ast` off it. + * That made the RLS / sharing pushdown path the one place on the platform that + * answered a *different* question from `celEngine.compile()` about what parses: + * an 80-term conjunction, a 40-level nest and a 200-element `$in` all reached + * real pushdown SQL here while the interpreter refused each outright. It now + * parses through {@link parseCelToAstWithReason} — #4812's canonical entry, with + * {@link DEFAULT_LIMITS} — so "what parses" has one answer. + * + * Within the limits that convergence is behaviour-preserving, and measurably so: + * across the 710 sources in the pushdown corpus that both front ends parse, the + * only AST difference is the #3306 `rewriteNullableTernary` `dyn(…)` wrap on the + * three ternaries — and a ternary faults on its own `?:` node before the lowerer + * ever descends into a branch, so reason AND detail are byte-identical for every + * one of them (pinned in `cel-to-filter-parse-convergence.test.ts`). + * + * Over the limits it is NOT behaviour-preserving, which is what + * `cel-pushdown-limits.ts`'s dated switch is for: during 17.0.0-rc.x an + * over-limit predicate still compiles — off the unbounded AST the canonical + * entry hands back for exactly this purpose — and WARNs naming the bound and + * what the source measures; at v17 GA it is refused as `parse-error`, which the + * RLS path turns into `RLS_DENY_FILTER`. + * + * Returns the AST to lower, or the `parse-error` result to hand the caller. + */ +function parseForPushdown(source: string): { ast: ASTNode } | { fail: CelFilterCompileResult } { + const graceWindow = celPushdownLimitsMode() === 'rc-grace'; + const parsed = parseCelToAstWithReason(source, { admitOverLimit: graceWindow }); + if (parsed.ok) return { ast: parsed.ast }; + if (parsed.kind === 'bounds') { + if (graceWindow && parsed.unboundedAst) { + warnOverLimitPushdown(source, parsed.overrun); + return { ast: parsed.unboundedAst }; + } + // Fail closed. `parse-error` deliberately, not a fourth reason: it is the + // reason every consumer of this compiler already routes to its deny path + // (`RLSCompiler.compileExpression` → `null` → `RLS_DENY_FILTER`; the sharing + // seeder → rule not seeded), and a new reason value would be a new branch + // each of them does not have. WHICH bound was exceeded rides in `detail`. + return { fail: { ok: false, reason: 'parse-error', detail: parsed.overrun.summary } }; } - return parseEnv; + // `empty` is unreachable here (`toSource` already rejects blank input) but is graded + // the same way it always was, and a syntax fault keeps its exact former + // detail: cel-js's rendered message, first line only. + return { fail: { ok: false, reason: 'parse-error', detail: parsed.message.split('\n')[0] || 'parse error' } }; +} + +/** + * Sources already WARNed about, so a policy compiled on every request warns once + * rather than once per row. Bounded like `cel-engine`'s rewrite memo — an + * unbounded set keyed by author-controlled strings is a leak. + */ +const warnedOverLimit = new Set(); +const WARNED_OVER_LIMIT_MAX = 500; + +/** + * The console, reached through `globalThis` rather than the bare `console` + * global. `@objectstack/formula` compiles with neither the DOM lib nor + * `@types/node` — it is a pure expression package that must build for any host — + * so `console` has no type here, and a host that genuinely has none (a bare + * embedder) must degrade to silence rather than to a `ReferenceError` thrown + * from inside a security compiler. + */ +function hostConsole(): { warn?: (message: string) => void } | undefined { + return (globalThis as { console?: { warn?: (message: string) => void } }).console; +} + +/** + * The 17.0.0-rc.x grace-window WARN. Names the bound that was exceeded, the + * platform's value for it, what the source measures, and — because this is a + * grace window and not a permanent posture — what will happen at v17 GA. + */ +function warnOverLimitPushdown(source: string, overrun: CelBoundsOverrun): void { + if (warnedOverLimit.has(source)) return; + if (warnedOverLimit.size >= WARNED_OVER_LIMIT_MAX) warnedOverLimit.clear(); + warnedOverLimit.add(source); + // `limit` / `limitValue` are non-null on this path by construction: a bounds + // fault whose key could not be named yields no unbounded AST, so it never + // reaches the grace window. The fallbacks keep the sentence readable rather + // than printing `null` if that ever stops being true. + const measure = overrun.measured !== null + ? String(overrun.measured) + : overrun.limitValue === null + ? 'over the measurement cap' + : `over ${overrun.limitValue * CEL_BOUNDS_MEASURE_CAP_FACTOR} (measurement capped)`; + const shown = source.length > 200 ? `${source.slice(0, 197)}...` : source; + hostConsole()?.warn?.( + `[cel-to-filter] pushdown predicate exceeds the platform CEL bound ${overrun.limit ?? '(unnamed)'} ` + + `(limit ${overrun.limitValue ?? 'unknown'}, this predicate measures ${measure}): ${overrun.summary}. ` + + `It still compiles during 17.0.0-rc.x; at v17 GA it will be REFUSED (parse-error) and the ` + + `RLS/sharing pushdown path will fail closed (RLS_DENY_FILTER). Split it or move the logic ` + + `to a hook/action body before upgrading. Predicate: ${shown}`, + ); +} + +/** Test hook for the WARN memo — a suite must not inherit another's dedupe state. */ +export function __resetPushdownLimitWarnings(): void { + warnedOverLimit.clear(); } /** Unwrap a CEL expression input — accepts a raw string or `{ source }`. */ @@ -120,13 +221,9 @@ export function compileCelToFilter( ): CelFilterCompileResult { const source = toSource(input); if (!source) return { ok: false, reason: 'parse-error', detail: 'empty expression' }; - let ast: ASTNode; - try { - ast = getParseEnv().parse(source).ast; - } catch (err) { - return { ok: false, reason: 'parse-error', detail: (err as Error).message?.split('\n')[0] ?? 'parse error' }; - } - return lowerCelAst(ast, opts, 'value'); + const parsed = parseForPushdown(source); + if ('fail' in parsed) return parsed.fail; + return lowerCelAst(parsed.ast, opts, 'value'); } /** @@ -140,13 +237,9 @@ export function isPushdownableCel( ): { ok: true } | { ok: false; reason: CelFilterFailReason; detail: string } { const source = toSource(input); if (!source) return { ok: false, reason: 'parse-error', detail: 'empty expression' }; - let ast: ASTNode; - try { - ast = getParseEnv().parse(source).ast; - } catch (err) { - return { ok: false, reason: 'parse-error', detail: (err as Error).message?.split('\n')[0] ?? 'parse error' }; - } - const res = lowerCelAst(ast, opts, 'shape'); + const parsed = parseForPushdown(source); + if ('fail' in parsed) return parsed.fail; + const res = lowerCelAst(parsed.ast, opts, 'shape'); return res.ok ? { ok: true } : { ok: false, reason: res.reason, detail: res.detail }; } diff --git a/packages/formula/src/index.ts b/packages/formula/src/index.ts index 030801a563..08293f5ac2 100644 --- a/packages/formula/src/index.ts +++ b/packages/formula/src/index.ts @@ -31,6 +31,18 @@ export { firstUndeclaredReference } from './cel-engine'; // instead silently opts out of the platform's rewrite AND its bounds. export { parseCelToAst } from './cel-engine'; export type { CelAstNode } from './cel-engine'; +// #6132 — the reason-carrying sister entrance. Same front end, same verdict, +// but it says WHICH of the platform's bounds a source blew instead of +// collapsing "over budget" and "not valid CEL" into the same `null`. A caller +// that REFUSES on a refusal (the RLS/sharing pushdown path does, fail-closed) +// needs the difference to tell the author what to fix. +export { parseCelToAstWithReason } from './cel-engine'; +export type { + CelBoundsOverrun, + CelLimitKey, + CelParseResult, + ParseCelToAstOptions, +} from './cel-engine'; export { cronEngine } from './cron-engine'; export { templateEngine, TEMPLATE_FORMATTERS, formatValue } from './template-engine'; export { registerStdLib, buildScope } from './stdlib'; @@ -41,6 +53,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'; +// #6132 — the dated switch governing what the pushdown path does with a +// predicate that overruns `DEFAULT_LIMITS`: compile-and-WARN during 17.0.0-rc.x, +// refuse (⇒ `RLS_DENY_FILTER`) from v17 GA. See `cel-pushdown-limits.ts` for the +// one line that moves at GA. +export { CEL_PUSHDOWN_LIMITS_MODE, celPushdownLimitsMode, setCelPushdownLimitsModeForTests } from './cel-pushdown-limits'; +export type { CelPushdownLimitsMode } from './cel-pushdown-limits'; +export { __resetPushdownLimitWarnings } 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 diff --git a/packages/plugins/plugin-security/src/rls-pushdown-limits.test.ts b/packages/plugins/plugin-security/src/rls-pushdown-limits.test.ts new file mode 100644 index 0000000000..82a847cca5 --- /dev/null +++ b/packages/plugins/plugin-security/src/rls-pushdown-limits.test.ts @@ -0,0 +1,163 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6132 — the RLS end of the CEL pushdown bounds convergence, in BOTH positions + * of the dated switch. + * + * `@objectstack/formula`'s `cel-to-filter.ts` used to parse RLS predicates + * through a private, limitless environment of its own, so a predicate the + * interpreter refuses outright (`Exceeded maxAstNodes (256)` / `maxDepth (32)` / + * `maxListElements (64)`) compiled here and reached REAL pushdown SQL, silently. + * It now parses through the canonical front end, and the maintainer's A′ ruling + * (2026-08-08, quoted verbatim on the issue) grants a grace window: + * + * > **rc grace, GA flip:** during 17.0.0-rc.x an over-limit predicate on the + * > pushdown path still compiles + emits a WARN naming the exceeded limit; at + * > v17 GA the runtime flips to fail-closed refusal (the `parse-error` ⇒ + * > `RLS_DENY_FILTER` path). + * + * The formula package pins the compiler's answer at both positions. THIS file + * pins what that answer does to authorization — which is the half that matters, + * because the deny sentinel lives here and nowhere else. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { RowLevelSecurityPolicy } from '@objectstack/spec/security'; +import { setCelPushdownLimitsModeForTests, __resetPushdownLimitWarnings } from '@objectstack/formula'; + +import { RLSCompiler, RLS_DENY_FILTER } from './rls-compiler.js'; + +/** 80-term conjunction — over `maxAstNodes` (256). */ +const CONJ_80 = Array.from({ length: 80 }, (_, i) => `f${i} == ${i}`).join(' && '); +/** 40-level nesting — over `maxDepth` (32). */ +const NEST_40 = `${'('.repeat(40)}owner_id == current_user.id${')'.repeat(40)}`; +/** 200-element `$in` — over `maxListElements` (64). */ +const IN_200 = `stage in [${Array.from({ length: 200 }, (_, i) => `'s${i}'`).join(',')}]`; + +const OVER_LIMIT: ReadonlyArray<{ name: string; using: string; limit: string }> = [ + { name: '80-term conjunction', using: CONJ_80, limit: 'maxAstNodes' }, + { name: '40-level nesting', using: NEST_40, limit: 'maxDepth' }, + { name: '200-element $in', using: IN_200, limit: 'maxListElements' }, +]; + +function policy(using: string): RowLevelSecurityPolicy { + return { name: 'over_budget', object: 'deal', operation: 'select', using } as RowLevelSecurityPolicy; +} + +const CTX = { userId: 'u1', tenantId: 'org-1' } as never; + +function compilerWithLogger() { + const logger = { warn: vi.fn() }; + const compiler = new RLSCompiler(); + compiler.setLogger(logger); + return { compiler, logger }; +} + +let consoleWarn: ReturnType; + +beforeEach(() => { + __resetPushdownLimitWarnings(); + consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => { + consoleWarn.mockRestore(); +}); + +describe('switch = rc-grace (17.0.0-rc.x) — the policy still ENFORCES, and the WARN names the limit', () => { + let restore: () => void; + beforeEach(() => { + restore = setCelPushdownLimitsModeForTests('rc-grace'); + }); + afterEach(() => restore()); + + for (const { name, using, limit } of OVER_LIMIT) { + it(`${name}: compiles to a real filter — NOT the deny sentinel`, () => { + const { compiler, logger } = compilerWithLogger(); + const filter = compiler.compileFilter([policy(using)], CTX); + expect(filter).not.toEqual(RLS_DENY_FILTER); + expect(filter).not.toBeNull(); + // The policy was not dropped, so `compileFilter`'s own "DROPPED (no + // enforcement)" warning must NOT fire — the grace window is "it still + // works", not "it silently vanished". + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it(`${name}: WARNs on the console naming ${limit} and the GA consequence`, () => { + compilerWithLogger().compiler.compileFilter([policy(using)], CTX); + const message = String(consoleWarn.mock.calls[0]?.[0]); + expect(message).toContain('[cel-to-filter]'); + expect(message).toContain(limit); + expect(message).toMatch(/this predicate measures \d+/); + expect(message).toContain('RLS_DENY_FILTER'); + }); + } +}); + +describe('switch = fail-closed (v17 GA) — the policy is REFUSED and the request fails closed', () => { + let restore: () => void; + beforeEach(() => { + restore = setCelPushdownLimitsModeForTests('fail-closed'); + }); + afterEach(() => restore()); + + for (const { name, using, limit } of OVER_LIMIT) { + it(`${name}: the only applicable policy → RLS_DENY_FILTER (zero rows)`, () => { + const { compiler } = compilerWithLogger(); + expect(compiler.compileFilter([policy(using)], CTX)).toEqual(RLS_DENY_FILTER); + }); + + it(`${name}: the drop is OBSERVABLE — the shape gate refuses it, so the policy is warned`, () => { + const { compiler, logger } = compilerWithLogger(); + compiler.compileFilter([policy(using)], CTX); + // ADR-0056 D4: a predicate `isSupportedRlsExpression` rejects earns the + // "DROPPED (no enforcement)" warning rather than vanishing in silence. + // Both sides of that comparison are downstream of the same switch, so the + // flip cannot produce a silently-dropped policy. + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(String(logger.warn.mock.calls[0]?.[0])).toContain('DROPPED (no enforcement)'); + }); + + it(`${name}: refusing does not ALSO emit the grace WARN`, () => { + compilerWithLogger().compiler.compileFilter([policy(using)], CTX); + const graceWarns = consoleWarn.mock.calls.filter((c) => String(c[0]).includes('[cel-to-filter]')); + expect(graceWarns).toHaveLength(0); + expect(limit).toBeTruthy(); + }); + + it(`${name}: an over-budget policy cannot be rescued by a second, compilable one — it is OR'd, not silently widened`, () => { + const { compiler } = compilerWithLogger(); + const filter = compiler.compileFilter( + [policy(using), policy('owner_id == current_user.id')], + CTX, + ); + // The over-budget policy dropped out; the surviving policy is the whole + // filter. This is the pre-existing multi-policy semantic (any policy + // allows), stated here so the flip's blast radius is on the record. + expect(filter).toEqual({ owner_id: 'u1' }); + }); + } +}); + +describe('both positions — a within-budget policy is untouched', () => { + for (const mode of ['rc-grace', 'fail-closed'] as const) { + it(`switch = ${mode}: the ordinary RLS shapes compile identically, with no WARN`, () => { + const restore = setCelPushdownLimitsModeForTests(mode); + try { + const { compiler, logger } = compilerWithLogger(); + expect(compiler.compileFilter([policy('owner_id == current_user.id')], CTX)).toEqual({ owner_id: 'u1' }); + expect(compiler.compileFilter([policy("stage == 'won'")], CTX)).toEqual({ stage: 'won' }); + expect( + compiler.compileFilter([policy('organization_id == current_user.organization_id')], CTX), + ).toEqual({ organization_id: 'org-1' }); + // 40 conjuncts ≈ 120 AST nodes — under `maxAstNodes` (256). + const under = Array.from({ length: 40 }, (_, i) => `f${i} == ${i}`).join(' && '); + expect(compiler.compileFilter([policy(under)], CTX)).not.toEqual(RLS_DENY_FILTER); + expect(logger.warn).not.toHaveBeenCalled(); + expect(consoleWarn.mock.calls.filter((c) => String(c[0]).includes('[cel-to-filter]'))).toHaveLength(0); + } finally { + restore(); + } + }); + } +});