[airween]

what

• @validateHash: on a compile error of the macro-expanded pattern and on exceeded PCRE limits, return rule->op_negated instead of 0, so a negated rule doesn't match on a regex error. A compile error is now logged (level 3) and sets TX:MSC_PCRE_ERROR; other execution errors (rc < -1) set TX:MSC_PCRE_ERROR too
• @rx and @validateHash with legacy PCRE (--with-pcre): handle PCRE_ERROR_JITSTACKLIMIT like the match limit (sets TX:MSC_PCRE_LIMITS_EXCEEDED)
• replace the three TX flag blocks in re_operators.c with one helper, msre_op_set_tx_flag(), which takes the flag name
• two new cases in tests/regression/misc/10-pcre-negated.t for a negated @validateHash with an invalid expanded pattern and with exceeded PCRE limits

why

These are the follow-ups promised in the review of #3709:

• @validateHash had the same problem as @rx before fix: negated @rx must not match when the regex fails #3709: the error paths returned a fixed 0, which the negation in re.c turned into a match. A compile error was also neither logged nor flagged, so a non-negated rule skipped the hash check without any trace, and rule 200005 (TX:/^MSC_/) of the recommended configuration couldn't catch it.
• With PCRE2, msc_regexec_ex() reruns the match without JIT on PCRE2_ERROR_JIT_STACKLIMIT. With legacy PCRE, pcre_exec() returns PCRE_ERROR_JITSTACKLIMIT instead, so the rule took the rc < -1 path and skipped its remaining targets. It is now treated like an exceeded match limit. This path isn't covered by a test, since it needs a legacy PCRE build with JIT.

ModSecurity v3 is not affected: its @validateHash is a stub without a regex, and the hash engine directives are rejected by the parser.

references

• Regular Expression Failure Triggers !@rx #3123
• fix: negated @rx must not match when the regex fails #3709
• v2: !@validateHash matches when the regex fails (same as #3123) #3710
• fix: flag regex errors of @rx/@rxGlobal in TX:MSC_PCRE_ERROR #3711 (v3)

AI disclosure

Made with help of Claude Code, Opus 5.5

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

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: bd408df8-ea18-4419-ac6a-507f047ae0ff

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• 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, this covers the follow-ups from #3709 and fixes the @validateHash part of #3710. LGTM.

I checked:

• Compile error path: it now logs at level 3, sets TX:MSC_PCRE_ERROR and returns rule->op_negated, the same as @rx after #3709.
• Limits path: it now returns rule->op_negated instead of 0. With legacy PCRE, PCRE_ERROR_JITSTACKLIMIT is guarded by #ifdef, so older PCRE builds still compile. With PCRE2, msc_regexec_ex() already retries without JIT on PCRE2_ERROR_JIT_STACKLIMIT (msc_pcre.c:257), so the asymmetry is correct.
• Helper: msre_op_set_tx_flag() is a straight merge of the three copies; behaviour is unchanged.
• Tests: CI ran both new cases on the PCRE2/JIT regression job (10-pcre-negated.t, 7/7 passed). Both would fail without the fix: the compile error and the limit case would return 0, the negation would turn that into a match, and the request would get a 403. Both assert the debug line, so they can't pass by skipping the path. SecHashEngine On sets both hash_is_enabled and hash_enforcement, so the early return doesn't hide anything.

A few non-blocking comments:

1. Link #3710. Please add Refs #3710 to the description. I'd rather not auto-close it yet because of point 2.
2. The SecHashEngine Off early return is still open. msre_op_validateHash_execute() still does return 0; when the hash engine is disabled, so !@validateHash matches every request in that configuration. This isn't a regex error, so leaving it out of this PR makes sense. Either we decide on #3710 that a disabled operator should return rule->op_negated too (a no-op in both directions), or we document that !@validateHash needs SecHashEngine On. I can take it from there once this merges.
3. Sonar quality gate. It fails on duplication (63.6% of new code). Most of that is the identical limits block, with its #ifdef and error message, that now lives in both @rx and @validateHash. A small helper, e.g. static int msre_op_pcre_limits_exceeded(int rc) for the condition, would clear it. Optional; I'm fine merging as is.
4. Legacy PCRE + JIT path. It isn't covered by a test, as you noted. The only regression job runs PCRE2, so this path won't be exercised in CI. Not something to fix here, but worth keeping in mind if we ever add a --with-pcre regression job.

Once this is merged, I'll open the docs PR for MSC_PCRE_ERROR on v2 (3-3-variables.md still says it's v3-only).

[sonarqubecloud]

Quality Gate failed

Failed conditions
10.3% Duplication on New Code (required ≤ 3%)
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[airween]

Thanks @fzipi for the review. I pushed a second commit with the change we agreed on in #3711, plus your Sonar point:

• TX:MSC_PCRE_ERROR on exceeded limits. @rx and @validateHash now set both flags on exceeded limits, MSC_PCRE_LIMITS_EXCEEDED first. Since tx_vars is iterated in insertion order, rule 200005 names the more specific flag. With this, both TX flags mean the same in v2 and v3. Two new cases in 10-pcre-negated.t cover it: case 8 checks that TX:MSC_PCRE_ERROR is set on a limit error, and fails without the change. Case 9 checks that a TX:/^MSC_/ rule blocks and names TX:MSC_PCRE_LIMITS_EXCEEDED.

On your points:

1. Refs #3710. Added to the description, without auto-closing.
2. The SecHashEngine Off early return. Agreed, that belongs to v2: !@validateHash matches when the regex fails (same as #3123) #3710. I lean towards rule->op_negated there too, so that a disabled operator matches in neither direction. Note that the same early return is also taken when ctl:hashEnforcement=Off disables enforcement for a single transaction. It's yours, thanks for taking it.
3. Sonar duplication. I moved the whole limit handling into helpers, not only the condition: msre_op_pcre_limits_exceeded() for the condition, with the PCRE2/PCRE #ifdef, and msre_op_pcre_limits_error() for the flags and the log message. Each call site is down to the check and return rule->op_negated.
4. Legacy PCRE + JIT. Noted, that's for a --with-pcre regression job if we ever add one.

10-pcre-negated.t passes 9/9, and so does the full regression suite.

[fzipi]

LGTM