refactor: one operator-semantics authority for all backends - #792
Conversation
One authority for filter-operator meaning: a declarative FILTER_OPERATOR_SEMANTICS table (family, negation twin, anchor, comparison range, fold participation) drives a planCondition() lowering that resolves every policy decision - complement law, null equality, in/nin decomposition, case-fold verdicts, anchored pattern derivation, value-shape validation and ITSELF legality - into a small closed plan-node vocabulary. Backends consume it via interpretPlan(); optional handler presence is the declared support matrix and the single featureUnsupported throw site. Filter.accept now derives its per-operator visitor dispatch from the same table (behavior identical); the per-operator IFilterVisitor methods stay untouched. Refs #791
The visitor interprets pre-decided plan primitives instead of re-deriving operator semantics: whereIn, whereAnchored and the per-operator visit methods collapse into one handler per plan node. Rendering is unchanged except negated anchored operators, which now emit a plain not() over the positive pattern (with the null-inclusive complement arm) instead of a negative-lookahead regex. A malformed mod value and a non-array in/nin value now render a never-match constant instead of invalid SQL. Refs #791
The compiler interprets plan primitives; negation, IN decomposition, fold verdicts and value validation arrive pre-decided from core. Binding/quantification stays untouched. nor/not compound groups now compile (plain boolean negation, sql parity) instead of throwing. Refs #791
The same condition must select the same rows on typeorm-over-sqlite and @rapiq/memory; expectations are computed by hand from the fixtures. A coverage test forces at least one parity case per operator of the semantics table, and declared- unsupported conditions (regex on sqlite, size, ITSELF) assert the typed error. Refs #791
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFilter semantics are centralized in core through operator metadata, condition-plan lowering, and interpreter dispatch. Memory and SQL backends now consume the shared plans, with updated negation and matching behavior plus TypeORM parity tests. ChangesFilter planning pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Query
participant planCondition
participant interpretPlan
participant MemoryCompiler
participant SQLFiltersVisitor
Query->>planCondition: filter condition
planCondition-->>interpretPlan: ConditionPlan
interpretPlan->>MemoryCompiler: interpret predicate plan
interpretPlan->>SQLFiltersVisitor: interpret SQL plan
MemoryCompiler-->>Query: memory predicate
SQLFiltersVisitor-->>Query: SQL condition
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/memory/src/parameter/filters/compiler.ts`:
- Around line 133-140: Update the regex branch in match to prevent ReDoS during
record evaluation: validate or reject unsafe plan.pattern.source patterns before
constructing the RegExp, or replace execution with a linear-time/timeout-bounded
evaluator. Preserve the existing featureUnsupported error behavior for patterns
that cannot be safely evaluated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e3972b5-bd79-457b-9530-6bbd126dd9c8
📒 Files selected for processing (13)
packages/core/src/parameter/filters/index.tspackages/core/src/parameter/filters/plan/constants.tspackages/core/src/parameter/filters/plan/index.tspackages/core/src/parameter/filters/plan/module.tspackages/core/src/parameter/filters/plan/types.tspackages/core/src/parameter/filters/record/module.tspackages/core/test/unit/parameter/filters-plan.spec.tspackages/memory/src/parameter/filters/compiler.tspackages/memory/src/parameter/filters/module.tspackages/memory/test/unit/filters/compound.spec.tspackages/sql/src/visitor/filters.tspackages/sql/test/unit/interpreters/regex.spec.tspackages/typeorm/test/unit/parity.spec.ts
| match(plan: MatchPlan) : ConditionEval { | ||
| let regex : RegExp; | ||
| if (plan.pattern.mode === 'regex') { | ||
| try { | ||
| regex = new RegExp(plan.pattern.source, plan.pattern.flags); | ||
| } catch { | ||
| throw AdapterError.featureUnsupported('filters:regex:value'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
recheck library validate regex ReDoS safety usage
💡 Result:
The recheck library is a specialized tool designed to detect Regular Expression Denial of Service (ReDoS) vulnerabilities by identifying patterns susceptible to catastrophic backtracking [1][2]. It is primarily used to analyze regular expressions during development and CI/CD, rather than as a runtime security shield for untrusted patterns [2][3]. Key capabilities of recheck include: - Multi-engine analysis: It combines static analysis (automaton-based) and fuzzing to identify vulnerabilities, supporting complex features like backreferences and look-arounds [2][4]. - Diagnostics: It returns analysis results (safe, vulnerable, or unknown) and provides specific attack patterns (strings that trigger the exponential complexity) when a vulnerability is found [5][4][6]. - Integration: It offers a programmatic API (asynchronous and synchronous), a CLI, and an ESLint plugin to automate detection in project codebases [5][2][4]. Best practices for ReDoS safety include: 1. Use linear-time engines: For processing untrusted input, the most robust defense is using a non-backtracking regex engine that guarantees linear-time performance (e.g., Google's RE2, or built-in non-backtracking modes in languages like.NET or Rust) [3][7]. 2. Harden regex patterns: If you must use a backtracking engine, ensure patterns are unambiguous. Avoid nested quantifiers (e.g., (a+)+), overlapping alternation, and ensure quantifiers are bounded (e.g., {1,64} instead of + or *) [8][3][7]. 3. Input validation and timeouts: Always restrict the length of input processed by regexes. If you cannot rewrite a pattern to be linear, run it in an isolated environment (worker thread or subprocess) with a strict, enforced execution timeout [3][7][9]. 4. Defense in depth: Treat regexes that touch attacker-controlled input as security boundaries. Integrate tools like recheck into your CI pipeline to catch vulnerable patterns before they reach production [3]. Note: Timeouts and library checks are aids for developer-authored patterns; they are not intended to be a secure sandbox for executing completely arbitrary, untrusted regular expression patterns provided by end users [3][9].
Citations:
- 1: https://makenowjust-labs.github.io/recheck/
- 2: https://makenowjust-labs.github.io/recheck/docs/intro/
- 3: https://payloadplayground.com/blog/regular-expression-denial-of-service-redos
- 4: https://registry.npmjs.org/%40makenowjust-labo%2Frecheck
- 5: https://makenowjust-labs.github.io/recheck/docs/usage/as-javascript-library/
- 6: https://github.com/makenowjust-labs/recheck/blob/main/packages/recheck/index.d.ts
- 7: https://www.codereviewlab.com/learning/redos
- 8: https://www.regular-expressions.info/redos.html
- 9: https://github.com/dotnet/docs/blob/main/docs/standard/base-types/best-practices-regex.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby symbols first.
ast-grep outline packages/memory/src/parameter/filters/compiler.ts --view expanded || true
echo "----"
wc -l packages/memory/src/parameter/filters/compiler.ts
echo "---- relevant excerpt ----"
sed -n '1,240p' packages/memory/src/parameter/filters/compiler.ts
echo "---- search for MatchPlan / pattern.mode / regex mode usage ----"
rg -n "pattern\.mode|MatchPlan|filters:regex:value|new RegExp\(" packages -g '*.ts' -g '*.tsx'Repository: tada5hi/rapiq
Length of output: 11572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- MatchPlan lowering ----"
sed -n '330,430p' packages/core/src/parameter/filters/plan/module.ts
echo "---- MatchPlan type ----"
sed -n '150,230p' packages/core/src/parameter/filters/plan/types.ts
echo "---- regex helper ----"
sed -n '1,120p' packages/core/src/parameter/filters/regex.ts
echo "---- parser-mongo regex handling ----"
sed -n '660,740p' packages/parser-mongo/src/parameter/filters/module.ts
echo "---- search for any regex safety checks / limits / alternative engines ----"
rg -n "recheck|RE2|timeout|safe regex|regex.*limit|pattern.*limit|catastrophic|backtracking" packages -g '*.ts' -g '*.tsx'Repository: tada5hi/rapiq
Length of output: 9434
Bound regex evaluation in the memory backend. plan.pattern.source is compiled directly and then run against every record, so a crafted pattern can trigger catastrophic backtracking. The try/catch only rejects invalid syntax; it does not address ReDoS. Consider rejecting unsafe regexes here or enforcing a linear-time/timeout-limited evaluator.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 136-136: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(plan.pattern.source, plan.pattern.flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 136-136: Do not use variable for regular expressions
Context: new RegExp(plan.pattern.source, plan.pattern.flags)
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/memory/src/parameter/filters/compiler.ts` around lines 133 - 140,
Update the regex branch in match to prevent ReDoS during record evaluation:
validate or reject unsafe plan.pattern.source patterns before constructing the
RegExp, or replace execution with a linear-time/timeout-bounded evaluator.
Preserve the existing featureUnsupported error behavior for patterns that cannot
be safely evaluated.
There was a problem hiding this comment.
Pre-existing trust model, not introduced here: master's buildRegex compiled the same variable input, and the plan lowering only moved the construction point. Exposure is limited to applications parsing untrusted mongo-style documents ($regex via @rapiq/parser-mongo) and evaluating with @rapiq/memory — the URL dialects have no wire spelling for the regex operator, and SQL backends receive the same pass-through pattern by settled design. Runtime recheck/timeout evaluation is disproportionate for this layer; the designed gating point is the schema filters.validate hook. Tracking documentation + a possible first-class operator allow-list in #794.
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Closes #791
What
Filter-operator semantics now have one home.
@rapiq/coregains a declarative operator-semantics table plus a lowering pass, and both backends become thin interpreters over pre-decided primitives:FILTER_OPERATOR_SEMANTICS(core/parameter/filters/plan/constants.ts) — one row per operator: family, negation twin (complementOf), visitor dispatch method, anchor placement, comparison range, case-fold participation.Filter.acceptderives its per-operator dispatch from this table (runtime behavior identical; the 19-branch if-chain is gone).planCondition(condition, { caseSensitive })lowers any condition tree into ~9 plan-node kinds with every policy decision already made: negation twins resolved tonegatedleaf flags (complement law),eq/ne(null)→ null checks, in/nin decomposed (empty list → constant, null members extracted, non-array → constant), fold verdicts computed (string value +caseSensitiveopt-out, composed through elemMatch prefixes), anchored operators derived into escaped positive patterns, mod/size values validated, ITSELF placement checked,nor/notmapped to negated groups.interpretPlan(plan, interpreter)is the single support-enforcement point: optional handler presence (mod/size/elemMatch) and theitselfdeclaration are the support matrix; the typedfeatureUnsupportedthrow lives here and nowhere else.@rapiq/sql:visitor/filters.tsdropswhereIn/whereAnchored/isCaseInsensitiveand all 19 visit methods for one handler per plan node (350 → ~250 lines, pure rendering). The adapter surface is untouched —@rapiq/typeormneeded zero source changes.@rapiq/memory:compiler.tssame treatment (398 → ~270 lines);binding.ts(quantification) untouched.typeorm/test/unit/parity.spec.ts): 31 hand-computed cases asserting the same condition selects the same rows on typeorm-over-better-sqlite3 and@rapiq/memory; a coverage test forces at least one case per table operator, so a future operator cannot ship without parity coverage. Declared-unsupported conditions assert the typed error.Deliberate behavior changes
notContains& co. now render(not (<regexp positive>) or <field> is null)instead of a negative-lookahead pattern. Semantically identical; lookaheads are gone entirely (one snapshot assertion updated).@rapiq/memorygainsnor/notcompound support (plain boolean negation, sql parity) — it previously threwoperatorUnsupportedwhile sql supported them.modvalue renders a never-match constant on sql (was: invalid SQL) exactly as memory always did; a non-arrayin/ninvalue lowers to a constant (sql previously crashed with a TypeError); an invalidelemMatchinterior throws the typed error on sql too (previously a crash);eq/ne(undefined)is unified tonulleverywhere.IFilterVisitormethods are kept — removing them in favor of the plan is a later, separately-deliberate step once downstream consumers migrate.Not in scope
IFilterVisitor/ removingcreateFilterRegex's negation flags from the public surface.sizesupport (JSON array length) — the plan node + support matrix make that a per-dialect follow-up.guide/filters.md's support statements (size/ITSELF) remain accurate as written.Testing
filters-plan.spec.ts, 32 tests), cross-backend parity suite (31 cases + coverage guard).nornow supported; unknown operators still throw typed).nx run-manybuild passes.Summary by CodeRabbit
New Features
Bug Fixes
Tests