feat: add support for case sensitive args #1059
Conversation
Signed-off-by: Felipe Zipitria <[email protected]>
fzipi
force-pushed
the
change-args-names-to-be-case-sensitive
branch
from
6c89ae1
to
20c4076
Compare
| @@ -119,7 +119,7 @@ SecRule ARGS_GET "@rx prepayloadpost" "id:200, phase:2, log, msg:'Rule Parent 2 | |||
| SecRule MATCHED_VAR "@rx pre" "chain" | |||
| SecRule MATCHED_VAR "@rx post" | |||
|
|
|||
| SecRule ARGS_GET:var3 "@rx pre3payloadpost" "id:300, phase:2, log, msg:'Rule Parent 300', \ | |||
| SecRule ARGS_GET:Var3 "@rx pre3payloadpost" "id:300, phase:2, log, msg:'Rule Parent 300', \ | |||
There was a problem hiding this comment.
Isn't this a breaking change in the behavior of the engine? Up to now users might have written rules in lowercase that would not work anymore once this PR gets merged. Are we okay with this change in a minor release?
There was a problem hiding this comment.
IMHO yes, because this is the common behavior in other engines.
There was a problem hiding this comment.
We had some controvercy about what we consider a breaking change at different points in the project. I think if in general we can avoid BC we should do it. Maybe we could leverage a build tag for this and introduce the BC in 4.0? WDYT?
There was a problem hiding this comment.
Just checking, have there been any test cases added to CRS test suite verifying the insensitive behavior? If this is to bring parity to other engines, I guess we should verify it with the suite.
But agree if rules deployed that worked stop working and possibly crash users, it's best to be behind a flag of some sort, that can be made the default after a major. With go's promise of compatibility, it's common to auto-update on minors and expect no change. Just for reference, the recent blog about rand was quite profound for me on how far it's necessary to go to... be go.
There was a problem hiding this comment.
What do you mean? Does CRS need to change and add tests about this when the expected behavior and rfc says this is the right thing?
There was a problem hiding this comment.
IMHO yes, because this is the common behavior in other engines.
I think this is key to me, rules that use to work won't work anymore and with no notice. That is the definition of breaking change (yes, we are breaking something that was broken). I think it could easily considered a bugfix if this was fixing an incorrect behaviour and or making work something that wasn't working bur from my understanding this is going to make something that use to work to not to work anymore?
While this is completely our fault, I think it is too late to do the right thing in v3 and I would be more inclined to use a build tag and make this part of normal engine on v4. The person who complained about this can use the build tag and all the rest can live without having to know about this, that is to me the definition of compatibility.
BONUS: the change for supporting a build tag seems very easy as in this PR we support both.
There was a problem hiding this comment.
Can someone then, with more knownledge than me, add that magnificent build tag and we can get over this?
There was a problem hiding this comment.
@M4tteoP in order to be facts driven, could you please show a rule that would break across versions so we are 100% sure this aren't hypoteticall concerns?
There was a problem hiding this comment.
Uhmm, the line we are commenting on should indeed be the case:
With ARGS not case-sensitive, we might have in place a rule like:
SecRule ARGS_GET:var3 "@rx pre3payloadpost" "id:300, phase:2, log, msg:'Rule Parent 300'...
We are expecting that it will check ARGS_GET:var3, ARGS_GET:Var3, ARGS_GET:VAR3...
Enabling case sensitivity, it would suddenly just check ARGS_GET:var3. If a user wrote that rule with the intent of matching for example ARGS_GET:Var3, the rule would never trigger anymore (The test is calling /testcase3.php?Var3=pre3payloadpost)
* feat: adds build tag for case sensitive args keys. * chore: updates license year.
| // Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //go:build coraza.rule.case_sensitive_args_keys |
There was a problem hiding this comment.
I think it deserves a line in the README under the Build tags section
There was a problem hiding this comment.
Something like the following?
* `case_sensitive_args_keys` - enables case-sensitive matching for ARGS keys, aligning Coraza behavior with RFC 3986 specification. It will be enabled by default in the next major version.
if 👍 I would push the line and merge it
what
why
notes
I don't like too much that it is the rule that tells if the collection is case sensitive. Ideally, we should be able to ask the collection (by calling collection.IsCaseSensitive() or similar) so that responsibility is delegated properly. But didn't knew how to do it first hand.