From 78f4c4caeda71dc9374e99dc51adc256ce347aa0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 14:33:27 +0000 Subject: [PATCH 1/4] fix(formula): converge the CEL pushdown parser onto the canonical front end, with an rc grace window (#6132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `packages/formula/src/cel-to-filter.ts` — the one canonical CEL → FilterCondition pushdown compiler (ADR-0058 D1/D2/D6) — kept a private, limitless `new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true })` with no `limits`, no stdlib and no `rewriteNullableTernary`, and read `.ast` off it. That made the RLS / sharing pushdown path the one place on the platform answering 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 while the interpreter refused each outright, and `isSupportedRlsExpression` was a thin wrapper over the same env rather than an independent gate. It now parses through `parseCelToAstWithReason`, the canonical entry (#4812), carrying DEFAULT_LIMITS. Within the limits this is behaviour-preserving, measured rather than asserted: across the 710 sources of the pushdown corpus both front ends accept, the only AST difference is `rewriteNullableTernary`'s `dyn(...)` wrap on the null-guard ternaries, and a ternary faults on its own `?:` node before the lowerer descends into a branch — so reason AND detail are byte-identical. Pinned in cel-to-filter-parse-convergence.test.ts, which rebuilds the old env to compare. Over the limits, the maintainer's A' ruling (2026-08-08, on the issue) is implemented as a single dated switch, `CEL_PUSHDOWN_LIMITS_MODE` in the new `cel-pushdown-limits.ts`: - `rc-grace` (shipping default, 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 once per predicate naming the exceeded bound, the platform's value for it, the predicate's own measure, and the GA consequence. - `fail-closed` (v17 GA, one line): the predicate is refused as `{ reason: 'parse-error', detail: 'Exceeded maxAstNodes (256)' }`, which the RLS path already routes to RLS_DENY_FILTER. Both positions run in CI today — in `@objectstack/formula` and in `@objectstack/plugin-security`, which owns the deny sentinel — so the GA half is proven before it ships. Two assertions go red on the flip so it cannot be silent. Sister entrance: `parseCelToAstWithReason` separates "not valid CEL" from "valid CEL, over budget" and names WHICH bound was blown, its platform value, and what the source measures (cel-js's own accounting — the smallest bound it parses under, found by probing the parser, because `maxDepth` counts parenthesised recursion that leaves no AST node behind). Graded by the same by-class/by-code classifier `compile`/`evaluate` use (#6223), never by error prose; the parity is pinned. `parseCelToAst` is unchanged and still collapses every refusal to `null`. `@objectstack/lint` needs no change at either position: its two enforceability gates read `isSupportedRlsExpression` / `compileCelToFilter` and both suites pin "the lint verdict IS the consumer's verdict" in both directions, so authoring reporting flips with the runtime by construction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T --- .changeset/cel-pushdown-limits-converge.md | 88 +++++++ packages/formula/src/cel-engine.ts | 228 +++++++++++++++++- packages/formula/src/cel-parse-reason.test.ts | 144 +++++++++++ packages/formula/src/cel-pushdown-limits.ts | 111 +++++++++ .../formula/src/cel-to-filter-limits.test.ts | 201 +++++++++++++++ .../cel-to-filter-parse-convergence.test.ts | 171 +++++++++++++ packages/formula/src/cel-to-filter.ts | 131 ++++++++-- packages/formula/src/index.ts | 19 ++ .../src/rls-pushdown-limits.test.ts | 163 +++++++++++++ 9 files changed, 1230 insertions(+), 26 deletions(-) create mode 100644 .changeset/cel-pushdown-limits-converge.md create mode 100644 packages/formula/src/cel-parse-reason.test.ts create mode 100644 packages/formula/src/cel-pushdown-limits.ts create mode 100644 packages/formula/src/cel-to-filter-limits.test.ts create mode 100644 packages/formula/src/cel-to-filter-parse-convergence.test.ts create mode 100644 packages/plugins/plugin-security/src/rls-pushdown-limits.test.ts 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..f869d9094b 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -271,14 +271,234 @@ 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` / … */ + limit: CelLimitKey; + /** The platform's value for that bound, i.e. what the source had to stay under. */ + limitValue: number; + /** + * 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 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. + */ +const 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 MEASURE_CAP_FACTOR}× the bound. + */ +function measureOverrun(source: string, key: CelLimitKey, limitValue: number): number | null { + const cap = limitValue * 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 limitValue = limit ? DEFAULT_LIMITS[limit] : Number.NaN; + const summary = parseErr.summary ?? message.split('\n')[0]; + if (!limit) { + // A bounds fault we cannot name. Report it as bounds (the class is not in + // doubt) with no measure — never as a syntax fault, which is the exact + // mislabel this entrance exists to stop. + return { + ok: false, + kind: 'bounds', + message, + overrun: { limit: 'maxAstNodes', limitValue: DEFAULT_LIMITS.maxAstNodes, measured: null, summary }, + unboundedAst: null, + }; + } + 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..f6fc1f9cae --- /dev/null +++ b/packages/formula/src/cel-pushdown-limits.ts @@ -0,0 +1,111 @@ +// 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'; + * ``` + * + * Two tests are written to go red on that line so the flip cannot be a silent + * one: `cel-pushdown-limits.test.ts` pins the shipped default, and + * `cel-to-filter-limits.rc-grace.test.ts` pins the grace-window behaviour. Both + * name this file in their failure text; flipping the const means updating them + * to the GA expectation, which is already written out in + * `cel-to-filter-limits.ga.test.ts` and exercised there today. + * + * 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..70f932f529 --- /dev/null +++ b/packages/formula/src/cel-to-filter-limits.test.ts @@ -0,0 +1,201 @@ +// 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, + }, +]; + +let warn: ReturnType; + +beforeEach(() => { + __resetPushdownLimitWarnings(); + warn = vi.spyOn(console, '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..a30e13b07a 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 { parseCelToAstWithReason } from './cel-engine'; +import type { CelBoundsOverrun } from './cel-engine'; +import { celPushdownLimitsMode } from './cel-pushdown-limits'; + // --------------------------------------------------------------------------- // Public contract // --------------------------------------------------------------------------- @@ -85,14 +88,106 @@ 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); + const measure = overrun.measured === null + ? `over ${overrun.limitValue * 64} (measurement capped)` + : String(overrun.measured); + const shown = source.length > 200 ? `${source.slice(0, 197)}...` : source; + hostConsole()?.warn?.( + `[cel-to-filter] pushdown predicate exceeds the platform CEL bound ${overrun.limit} ` + + `(limit ${overrun.limitValue}, 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 +215,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 +231,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(); + } + }); + } +}); From 52f6af87f7f6b220e83d1bf2d3940fa612d06380 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 15:02:00 +0000 Subject: [PATCH 2/4] fix(formula): spy the WARN sink through globalThis, and correct the flip docblock (#6132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from running the gate list: - `check:type-check-debt` went red: `@objectstack/formula`'s TEST_DEBT ledger records 17 raw tsc errors and the new suite made it 18. The added error was `TS2584: Cannot find name 'console'` — this package compiles with neither the DOM lib nor `@types/node`, which is why the compiler itself reaches the sink through `globalThis`. The test now spies on the same object the compiler writes to, rather than a differently-obtained one, which is also the only version of this spy that cannot go green over a silent sink. Back to 17. - The switch docblock named three test files that do not exist (the suites were consolidated into one). It now names the real file and states the flip's measured blast radius: 10 tests, all in that file, and nothing else. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T --- packages/formula/src/cel-pushdown-limits.ts | 15 +++++++++------ packages/formula/src/cel-to-filter-limits.test.ts | 13 ++++++++++++- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/packages/formula/src/cel-pushdown-limits.ts b/packages/formula/src/cel-pushdown-limits.ts index f6fc1f9cae..00b32ee2d3 100644 --- a/packages/formula/src/cel-pushdown-limits.ts +++ b/packages/formula/src/cel-pushdown-limits.ts @@ -42,12 +42,15 @@ * export const CEL_PUSHDOWN_LIMITS_MODE: CelPushdownLimitsMode = 'fail-closed'; * ``` * - * Two tests are written to go red on that line so the flip cannot be a silent - * one: `cel-pushdown-limits.test.ts` pins the shipped default, and - * `cel-to-filter-limits.rc-grace.test.ts` pins the grace-window behaviour. Both - * name this file in their failure text; flipping the const means updating them - * to the GA expectation, which is already written out in - * `cel-to-filter-limits.ga.test.ts` and exercised there today. + * `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 diff --git a/packages/formula/src/cel-to-filter-limits.test.ts b/packages/formula/src/cel-to-filter-limits.test.ts index 70f932f529..dcc73c6fc8 100644 --- a/packages/formula/src/cel-to-filter-limits.test.ts +++ b/packages/formula/src/cel-to-filter-limits.test.ts @@ -59,11 +59,22 @@ const AT_LIMIT = [ }, ]; +/** + * 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(console, 'warn').mockImplementation(() => {}); + warn = vi.spyOn(hostConsole, 'warn').mockImplementation(() => {}); }); afterEach(() => { From 501c4d6c3a24e1c5cdf46d3fa06be8ab27362b3c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 15:04:43 +0000 Subject: [PATCH 3/4] refactor(formula): name the measurement cap once (#6132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The grace WARN's "measurement capped" branch spelled the cap factor as a literal `64` while `measureOverrun` read it from `MEASURE_CAP_FACTOR` — two copies of one number, in two files, where a drift would make the WARN quote a bound the measurement never used. The constant is now exported from `cel-engine.ts` (not from the package index: it is an implementation detail of the diagnostic, not public surface) and read in both places. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T --- packages/formula/src/cel-engine.ts | 8 ++++---- packages/formula/src/cel-to-filter.ts | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index f869d9094b..fda97c3887 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -294,7 +294,7 @@ export interface CelBoundsOverrun { * 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 MEASURE_CAP_FACTOR}) + * 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. */ @@ -356,7 +356,7 @@ export interface ParseCelToAstOptions { * without a cap, describing a pathological source means parsing it at whatever * size it happens to be. */ -const MEASURE_CAP_FACTOR = 64; +export const CEL_BOUNDS_MEASURE_CAP_FACTOR = 64; /** * The canonical env with ONE bound lifted, used only to measure an overrun. @@ -403,10 +403,10 @@ function parsesUnder(source: string, key: CelLimitKey, value: number): boolean { * 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 MEASURE_CAP_FACTOR}× the bound. + * parses, capped at {@link CEL_BOUNDS_MEASURE_CAP_FACTOR}× the bound. */ function measureOverrun(source: string, key: CelLimitKey, limitValue: number): number | null { - const cap = limitValue * MEASURE_CAP_FACTOR; + 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; diff --git a/packages/formula/src/cel-to-filter.ts b/packages/formula/src/cel-to-filter.ts index a30e13b07a..15aa1d8ac5 100644 --- a/packages/formula/src/cel-to-filter.ts +++ b/packages/formula/src/cel-to-filter.ts @@ -45,7 +45,7 @@ import type { ASTNode } from '@marcbachmann/cel-js'; import type { FilterCondition } from '@objectstack/spec/data'; -import { parseCelToAstWithReason } from './cel-engine'; +import { CEL_BOUNDS_MEASURE_CAP_FACTOR, parseCelToAstWithReason } from './cel-engine'; import type { CelBoundsOverrun } from './cel-engine'; import { celPushdownLimitsMode } from './cel-pushdown-limits'; @@ -173,7 +173,7 @@ function warnOverLimitPushdown(source: string, overrun: CelBoundsOverrun): void if (warnedOverLimit.size >= WARNED_OVER_LIMIT_MAX) warnedOverLimit.clear(); warnedOverLimit.add(source); const measure = overrun.measured === null - ? `over ${overrun.limitValue * 64} (measurement capped)` + ? `over ${overrun.limitValue * CEL_BOUNDS_MEASURE_CAP_FACTOR} (measurement capped)` : String(overrun.measured); const shown = source.length > 200 ? `${source.slice(0, 197)}...` : source; hostConsole()?.warn?.( From 078002f5b41f71bc5d60052c8ec89354929ee244 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 15:08:44 +0000 Subject: [PATCH 4/4] fix(formula): report an unnameable bound as `limit: null`, not as a guessed key (#6132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parseCelToAstWithReason`'s "cel-js raised a limit fault whose key this package cannot read" branch filled the overrun with `maxAstNodes` while its own comment said a guessed name is the thing to avoid. It is unreachable on cel-js 8.0.0 — `Parser#limitExceeded` always phrases it `Exceeded ()` — but a guessed key is worse than none: the author goes and shortens the wrong axis. The branch now reports `{ limit: null, limitValue: null }`, which the types carry, and hands back no unbounded AST, so the pushdown path fails closed on it in either position of the switch rather than compiling something it cannot describe. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T --- packages/formula/src/cel-engine.ts | 25 +++++++++++++++++-------- packages/formula/src/cel-to-filter.ts | 16 +++++++++++----- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index fda97c3887..4801860c5e 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -286,10 +286,15 @@ 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` / … */ - limit: CelLimitKey; + /** + * 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; + 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 @@ -472,20 +477,24 @@ export function parseCelToAstWithReason( if (classifyCelFault(err) !== 'bounds') return { ok: false, kind: 'parse', message }; const parseErr = err as ParseError; const limit = limitKeyOf(parseErr); - const limitValue = limit ? DEFAULT_LIMITS[limit] : Number.NaN; const summary = parseErr.summary ?? message.split('\n')[0]; if (!limit) { - // A bounds fault we cannot name. Report it as bounds (the class is not in - // doubt) with no measure — never as a syntax fault, which is the exact - // mislabel this entrance exists to stop. + // 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: 'maxAstNodes', limitValue: DEFAULT_LIMITS.maxAstNodes, measured: null, summary }, + 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) { diff --git a/packages/formula/src/cel-to-filter.ts b/packages/formula/src/cel-to-filter.ts index 15aa1d8ac5..44d77cbeb4 100644 --- a/packages/formula/src/cel-to-filter.ts +++ b/packages/formula/src/cel-to-filter.ts @@ -172,13 +172,19 @@ function warnOverLimitPushdown(source: string, overrun: CelBoundsOverrun): void if (warnedOverLimit.has(source)) return; if (warnedOverLimit.size >= WARNED_OVER_LIMIT_MAX) warnedOverLimit.clear(); warnedOverLimit.add(source); - const measure = overrun.measured === null - ? `over ${overrun.limitValue * CEL_BOUNDS_MEASURE_CAP_FACTOR} (measurement capped)` - : String(overrun.measured); + // `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} ` + - `(limit ${overrun.limitValue}, this predicate measures ${measure}): ${overrun.summary}. ` + + `[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}`,