Enable multiline mode for rx #732
Conversation
operators/rx_test.go
Outdated
| @@ -38,6 +38,12 @@ func TestRx(t *testing.T) { | |||
| input: "グッバイワールド", | |||
| want: false, | |||
| }, | |||
| // Requires dotall | |||
There was a problem hiding this comment.
I couldn't come up with a simplified unit test for requiring multiline. The failing one is
I tried using ^ and \b together but it didn't seem to fail without the m option. Any other regex gurus able to suggest a simple unit test that fails without m?
There was a problem hiding this comment.
This test seems not working without m:
// Requires multiline
{
pattern: `^hello.*world`,
input: "test\nhello\nworld",
want: true,
},
--- FAIL: TestRx (0.00s)
--- FAIL: TestRx/^hello.*world/test_hello_world (0.00s)
/Users/matteopace/Repo/coraza/operators/rx_test.go:71: want true, got false
operators/rx.go
Outdated
| @@ -19,6 +20,7 @@ var _ rules.Operator = (*rx)(nil) | |||
|
|
|||
| func newRX(options rules.OperatorOptions) (rules.Operator, error) { | |||
| data := options.Arguments | |||
| data = fmt.Sprintf("(?sm)%s", data) | |||
There was a problem hiding this comment.
There isn't a programattic way to do it with Go but this seems like the way. Note I verified that multiple instances of (?sm) etc work so even if the input expression from CRS were to change to include options in the future, it wouldn't break.
There was a problem hiding this comment.
Thanks Rag, now it's clear how 942522 was supposed to match against 942522-7!
I would Maybe just add a comment about what this line tweak is for?
There was a problem hiding this comment.
While I am not a big fan of mutating user input I believe there is no other way to achieve this as even Go people recommends this https://groups.google.com/g/golang-nuts/c/jiVdamGFU9E
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## v3/dev #732 +/- ##
=======================================
Coverage 81.82% 81.82%
=======================================
Files 149 149
Lines 8280 8282 +2
=======================================
+ Hits 6775 6777 +2
Misses 1286 1286
Partials 219 219
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
What about https://pkg.go.dev/regexp/syntax#Parse regexp.Parse("regex", flags...) |
operators/rx.go
Outdated
| @@ -19,6 +20,7 @@ var _ rules.Operator = (*rx)(nil) | |||
|
|
|||
| func newRX(options rules.OperatorOptions) (rules.Operator, error) { | |||
| data := options.Arguments | |||
| data = fmt.Sprintf("(?sm)%s", data) | |||
There was a problem hiding this comment.
| data = fmt.Sprintf("(?sm)%s", data) | |
| // (?sm) enables multiline mode, see https://stackoverflow.com/a/27680233 | |
| // for more info | |
| data = fmt.Sprintf("(?sm)%s", data) |
|
Relevant rsc/binaryregexp#3
…On Thu, 23 Mar 2023, 14:38 Juan Pablo Tosso, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In operators/rx.go
<#732 (comment)>:
> @@ -19,6 +20,7 @@ var _ rules.Operator = (*rx)(nil)
func newRX(options rules.OperatorOptions) (rules.Operator, error) {
data := options.Arguments
+ data = fmt.Sprintf("(?sm)%s", data)
Also, can we concatenate multiple flags? Like (?sm)(?i).... Some CRS
regexes uses the case insensitive flag
—
Reply to this email directly, view it on GitHub
<#732 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAUF3VBQGRWGFCQIJZLW5RG65ANCNFSM6AAAAAAWEVKM5Y>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
@jptosso I added a test case to verify concatenating flags works fine https://github.com/corazawaf/coraza/compare/v3/dev...anuraaga:test-insensitive?expand=1
|
* Enable dotall/multiline mode for rx --------- Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>
I noticed ModSec uses dotall / multiline for regex
https://github.com/SpiderLabs/ModSecurity/blob/1feaa7d24b3ae19e6d9d2a4c055d64cd8a305aa1/src/utils/regex.cc#L68
Currently one CRS test fails because of the lack of multiline. I added dotall as well to match modsec though no CRS tests fail without it.