[airween]

what

• @rx / @rxGlobal: on a regex error (compile error or a failed search), also set TX:MSC_PCRE_ERROR=1, next to the existing standalone MSC_PCRE_ERROR variable
• modsecurity.conf-recommended: list MSC_PCRE_ERROR among the flags checked by rule 200005
• new regression cases in operator-rx.json and operator-rxGlobal.json: a negated rule with an invalid macro-expanded pattern is blocked by a TX:/^MSC_/ rule

why

After #3700, a negated @rx/@rxGlobal rule no longer matches on a regex error. For exceeded match limits, rule 200005 (TX:/^MSC_/) still blocks, because TX:MSC_PCRE_LIMITS_EXCEEDED is set. Compile errors and other regex errors only set the standalone MSC_PCRE_ERROR variable, which TX:/^MSC_/ does not see, so these requests passed. Setting the flag in TX too makes rule 200005 fail closed on every regex error, without any configuration change, and matches the behavior of ModSecurity v2.

references

• Regular Expression Failure Triggers !@rx #3123
• fix: do not let !@rx and !@rxGlobal match on a regex error #3700
• fix: negated @rx must not match when the regex fails #3709 (v2)

AI disclosure: made with help of Claude Code, Opus 5.5

Summary by CodeRabbit

• Bug Fixes
  • Regular-expression compilation and search errors are now consistently recorded in transaction data, including errors from negated regex operators. Match-limit failures are also identified separately.
• Tests
  • Added regression coverage for invalid macro-expanded patterns in regular-expression operators, verifying error reporting and the resulting HTTP response.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 23b07121-ca4b-465a-bb66-899e12d61b8d

Commits

Reviewing files that changed from the base of the PR and between 4096c56 and 286f87b.

Files selected for processing: 5

• src/operators/operator.cc
• src/operators/operator.h
• src/operators/rx.cc
• src/operators/rx_global.cc
• test/cppcheck_suppressions.txt

Files with no reviewable changes: 1

• test/cppcheck_suppressions.txt

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

Walkthrough

The @rx and @rxGlobal operators now use shared helpers to report regex compile and search errors. The helpers set transaction error variables and log errors. Regression tests cover invalid macro-expanded patterns with negated operators.

Changes: Regex error reporting

Layer / File(s)	Summary
Shared regex error reporting src/operators/operator.h, src/operators/operator.cc	Operator adds helpers that report regex compile and search errors. They set MSC_PCRE_ERROR in the transaction variable and TX collection. Search errors also set MSC_PCRE_LIMITS_EXCEEDED for match-limit errors and log the error category.
Operator integration and regression tests src/operators/rx.cc, src/operators/rx_global.cc, test/test-cases/regression/operator-rx.json, test/test-cases/regression/operator-rxGlobal.json, modsecurity.conf-recommended, test/cppcheck_suppressions.txt	Both operators call the shared helpers and retain their existing error return behavior. Regression tests check error flags for invalid macro-expanded patterns with negated operators. The recommended configuration documents MSC_PCRE_ERROR; cppcheck suppressions for the operators are removed.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

• owasp-modsecurity/ModSecurity#3700: Changes error returns for negated @rx and @rxGlobal; this PR adds shared error reporting to those paths.
• owasp-modsecurity/ModSecurity#3647: Covers invalid macro-expanded patterns in the same operators; this PR extends error reporting to the TX collection.

Suggested reviewers: fzipi, martinhsv

Merge Risk: Minimal · up to 286f8

The new regex error flags appear to reach the intended blocking rule. No issue identified here prevents merging after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: flagging regex errors from @rx and @rxGlobal in TX:MSC_PCRE_ERROR.
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: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 4 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[fzipi]

Thanks @airween, LGTM. This closes the gap left after #3700: a negated @rx/@rxGlobal with an invalid macro-expanded pattern no longer matches, but until now rule 200005 couldn't see the error either. Setting the TX flag next to the standalone variable makes the recommended config fail closed on every regex error with no configuration change, the same as v2 after #3709.

I checked:

• Both error paths in Rx and RxGlobal set the flag: the invalid pattern case (re->hasError()) and a failed search (regex_result != Ok). storeOrUpdateFirst is the same call already used for TX:MSC_PCRE_LIMITS_EXCEEDED.
• The new cases test what matters: a real rule 200005 (TX:/^MSC_/ "!@streq 0") blocks the request with a 409. Without the change, !@rx doesn't match after #3700, TX has no MSC_ entry, and the request gets a 200. So both cases fail without the fix.
• CI passes on every Linux, macOS and Windows job.

Some non-blocking comments:

1. Sonar duplication (100% of new code). The same four lines now sit in four places, next to the existing // FIXME: DRY regex error reporting comment. A small helper shared by both operators would clear the gate and resolve the FIXME. For example, a static function in operator.cc or a Transaction method like flagRegexError(RegexResult) that sets the standalone variable and the TX flag, plus MSC_PCRE_LIMITS_EXCEEDED on a match limit. Fine as a follow-up if you'd rather keep this PR minimal.

2. Match limit differs slightly from v2. On a match-limit error v3 now sets both TX:MSC_PCRE_ERROR and TX:MSC_PCRE_LIMITS_EXCEEDED. That mirrors the standalone variables, which v3 already sets together. v2 (#3709) sets only TX:MSC_PCRE_LIMITS_EXCEEDED for limits and keeps TX:MSC_PCRE_ERROR for other errors. Rule 200005 blocks either way. The visible difference is that its %{MATCHED_VAR_NAME} message may name TX:MSC_PCRE_ERROR instead of the more useful TX:MSC_PCRE_LIMITS_EXCEEDED, depending on TX iteration order. I'm fine either way; just mentioning it so we decide on purpose. I'll document whichever behavior we keep.

3. CHANGES. Rule 200005 now blocks requests it used to let through (an invalid pattern after macro expansion). That's the point of the change, but a CHANGES line that says so would help anyone upgrading with the recommended config.

I'll update the docs once this merges. owasp-modsecurity/documentation#4 currently says that v3 doesn't set TX:MSC_PCRE_ERROR.

[airween]

Thanks @fzipi for the review. On your points:

1. Shared helper. Done in the second commit (286f87b). The error handling now lives in two static helpers in Operator: reportRegexCompileError() for an invalid pattern and reportRegexSearchError() for a failed search. Both operators call them, and the FIXME is gone. I kept them out of Transaction, since that is part of the public API, and out of Utils::Regex, which doesn't know about transactions and is also used outside of operators. The debug log messages are unchanged, so the existing tests still apply. The log message is now built without a temporary variable. That variable was the only reason for the variableScope/unreadVariable cppcheck suppressions on rx.cc and rx_global.cc (it was unread in NO_LOGS builds), so I removed those suppressions as well.

2. Match limit vs. v2. I'd rather align v2 with v3 than the other way around. In v3, the standalone MSC_PCRE_ERROR already means "a regex failed for any reason", and MSC_PCRE_LIMITS_EXCEEDED names one of the causes. If the TX flag meant something narrower than the standalone variable with the same name, that would be confusing, and a rule that checks only TX:MSC_PCRE_ERROR would miss limit errors. So v3 stays as it is, and I'll change v2 in fix: handle regex errors of @validateHash like @rx #3712 to set TX:MSC_PCRE_ERROR on exceeded limits too. Then both flags have the same meaning in both engines, so please document it that way.

On %{MATCHED_VAR_NAME}: in v2, tx_vars is iterated in insertion order. Setting MSC_PCRE_LIMITS_EXCEEDED first means that rule 200005 names it there. In v3, the order isn't guaranteed. Rule 200005 is meant to fail closed, not to tell the cause apart, and the debug log already reports regex error 'MATCH_LIMIT'. Anyone who needs the cause in the audit log can add a separate rule on TX:MSC_PCRE_LIMITS_EXCEEDED.

3. CHANGES. I collect the entries for both branches right before the release, so I'll add it then, together with fix: do not let !@rx and !@rxGlobal match on a regex error #3700. The two are one change from a user's point of view.

Thanks for taking the docs, I will review that and may be will ask some change.

[sonarqubecloud]

Quality Gate failed

Failed conditions
3.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[fzipi]

Thanks @airween, the refactor looks good. Still LGTM.

What I checked on 286f87b:

• The helpers do the same thing as the old code. reportRegexCompileError() and reportRegexSearchError() set the same standalone variables and TX flags, in the same order and on the same paths. The debug lines are unchanged apart from the operator name, which is now passed in. Both helpers do nothing when there is no transaction, as the old if (transaction) blocks did. The return values in rx.cc/rx_global.cc and their comments are untouched.
• The cppcheck suppressions are no longer needed. With the temporary regex_error_str gone, cppcheck 2.22 reports nothing for rx.cc, rx_global.cc or operator.cc without the four variableScope/unreadVariable entries, and the CI cppcheck job is green.
• Forward declarations. enum class RegexResult; in operator.h matches the definition in regex.h, which has no explicit underlying type, so they agree.
• Tests. I built this locally (macOS, PCRE2). operator-rx.json and operator-rxGlobal.json pass, including the rule 200005 cases. The full make check gives 5137 total, 5117 pass, 20 skip, 0 fail.

On your answers:

1. Match limit. Agreed: TX:MSC_PCRE_ERROR should mean the same as the standalone variable, i.e. any regex failure, with MSC_PCRE_LIMITS_EXCEEDED naming the cause. I'll re-review #3712 once you push the v2 change, and update owasp-modsecurity/documentation#4 so both engines are described that way.
2. CHANGES at release time, together with #3700. Fine with me.

One optional nit: Sonar still fails at 3.6% duplication. That is probably the three lines that set MSC_PCRE_ERROR, now in both helpers. Having reportRegexCompileError() call a small shared flagRegexError(transaction), also used by the search helper, would likely clear the gate. Not a blocker for me.

[airween]

Thanks @fzipi for checking the refactor.

On the Sonar duplication: it isn't the MSC_PCRE_ERROR lines in the helpers. According to the PR analysis, operator.cc has no duplicated lines. The 2 lines it counts (2 of 56 new lines, hence 3.6%) are the two helper calls:

• rx.cc:87: reportRegexSearchError(transaction, *re, regex_result, "rx");
• rx_global.cc:86: reportRegexSearchError(transaction, *re, regex_result, "rxGlobal");

Both sit inside a block of 26 to 27 lines (rx.cc:84, rx_global.cc:83) that the two files already shared before this PR: the end of the error path and the whole capture handling. Sonar counts a new line inside an existing duplicated block as new duplicated code. A shared flagRegexError() wouldn't change that. Only sharing the capture handling would, and that's out of scope here, so I'd merge it as it is. If we want that, it can be a small separate refactor.

I'm going to merge this one - thank you for the help.