-
-
Notifications
You must be signed in to change notification settings - Fork 219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: removes multiline from default regex modifiers [breaking] #876
base: main
Are you sure you want to change the base?
Conversation
// The so called DOLLAR_ENDONLY modifier in PCRE2 is meant to tweak the meaning of dollar '$' | ||
// so that it matches only at the very end of the string (see: https://www.pcre.org/current/doc/html/pcre2pattern.html#SEC6) | ||
// It seems that re2 already behaves like that by default. | ||
pattern: `123$`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to find the DOLLAR_ENDONLY
modifier for RE2, but it seems that re2 by default behaves like DOLLAR_ENDONLY enabled.
See also https://regex101.com/r/mUcy69/1 (Mind the regex option D (dollar end only) enabled).
Am I missing something?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #876 +/- ##
==========================================
+ Coverage 82.72% 83.53% +0.81%
==========================================
Files 162 166 +4
Lines 9080 9556 +476
==========================================
+ Hits 7511 7983 +472
- Misses 1319 1323 +4
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Good to see ftw passing with this change. I wouldn't merge it until seeing the PR to ModSec v3 doing the same, otherwise someone will just ask in the future why we aren't aligned with ModSec v3. |
Totally 💯 |
Apart from aligning the behavior with ModSec, are we going to accept this breaking change and release it into a |
This PR evaluates the needed changes to align Coraza to Modsec v2 behavior rather than Modsec v3 in terms of default modifiers when compiling regexes. This necessity has been raised by coreruleset/coreruleset#3277.
We are currently running with two default modifiers:
While Modsec v2 (the reference implementation for the CRS) has:
Running the CRS rules with multiline may lead to false positives and lower performance.
Opening the PR to further discuss this, and whether we accept a breaking change for
v3.*
or not.