Repository navigation
perf: drop redundant exclusion check in rule evaluation - #3692
chweidling wants to merge 2 commits into
Conversation
RuleWithOperator::evaluate() checked exclusion.contains(v) twice for every value that is not excluded. The second check can never be true, because the first one already skips excluded values, but it repeats the scan over all exclusions, including their regex searches. Variables::contains() now uses Regex::search() instead of searchAll(), as it only needs to know whether the regex matches. With CRS v4 and four regex exclusions via SecRuleUpdateTargetByTag, this increases performance of the benchmark test by about 6%. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Exclusion checks src/rule_with_operator.cc, src/variables/variable.h |
RuleWithOperator::evaluate checks variable exclusions separately from tag-based target removals. Variables::contains uses the result of search to check regex-backed variables. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Possibly related PRs
- owasp-modsecurity/ModSecurity#3526: Both PRs change regex-backed target exclusion behavior.
Suggested reviewers: airween
Merge Risk: 
95f6d
-
The PR appears mergeable; no concrete correctness, availability, or integration risk was found.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title is a short, specific conventional-commit style summary that matches the changeset. The diff removes a redundant exclusion.contains(v) check in RuleWithOperator::evaluate, which the PR object… Docstring Coverage Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title is a short, specific conventional-commit style summary that matches the changeset. The diff removes a redundant exclusion.contains(v) check in RuleWithOperator::evaluate, which the PR objectives describe as made redundant by the preceding check, and changes Variables::contains() to use Regex::search() instead of searchAll(). The title names the primary change (dropping a redundant exclusion check for performance) and does not describe anything absent from the changeset. It omits the Regex::search() change, which the title does not need to cover.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
fzipi
left a comment
fzipi
left a comment
There was a problem hiding this comment.
Thanks @chweidling, nice catch.
I checked both hunks:
rule_with_operator.cc:exclusionandvdon't change between the firstif(which alreadycontinues whenexclusion.contains(v)is true) and the second one. So the removed term was always false, on every exclusion path (by id, tag or msg, key or regex). No behavior change.variable.h:Regex::search(const std::string&)returns 1/0 (match or no match), not an offset. It uses the same JIT and no-JIT fallback assearchAll(), sosearch(...) > 0is equivalent to!searchAll(...).empty(), including for empty matches like/^/. To be clear for anyone reading the bot summary above: matches at position 0 are still counted. The existing "anchored at beginning of Subject" cases inconfig-update-target-by-tag.jsoncover that and pass. I also checked!ARGS:'/^utm_/',!ARGS:'/utm_/'(match in the middle of the key) and!ARGS:'/^/'locally.
Tiny optional nit: != 0, or just using the result as a bool, would read less like an offset comparison. That's probably what tripped up CodeRabbit.
I built this locally on macOS (PCRE2 with JIT, together with #3691, which touches disjoint files). The exclusion and ctl regression files pass: config-update-target-by-tag/-by-id, issue-2296, issue-2099, action-ctl_rule_remove_by_tag, action-ctl_rule_remove_target_by_id/by_tag. The full make check passes as well (5128 total, 5110 pass, 18 skip, 0 fail). I didn't re-run the benchmark, but the reasoning for the gain with regex exclusions makes sense.
LGTM.
search() returns 1 for a match and 0 otherwise, not an offset. > 0 read like a position check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
what
RuleWithOperator::evaluate()no longer checksexclusion.contains(v)a second time. At that point it is always false: the check right before it already skips excluded values.Variables::contains()usesRegex::search()instead ofsearchAll(), as it only needs to know whether a regex exclusion matches.why
Every value that is not excluded paid for the scan over all exclusions twice, including the regex searches of exclusions like
!REQUEST_COOKIES:'/^_ga/'. An exclusion added by tag, e.g.OWASP_CRS, applies to every rule.With CRS v4 (
download-owasp-v4-rules.sh) plus these exclusions, this change increases performance of thebenchmarktest by about 6%:Without regex exclusions there is no difference.
🤖 Generated with Claude Code
Summary by CodeRabbit