feat: allow selectors on *_NAMES collections
#1143
Conversation
… non-selectable collection
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1143 +/- ##
==========================================
- Coverage 82.72% 82.27% -0.45%
==========================================
Files 162 166 +4
Lines 9080 8109 -971
==========================================
- Hits 7511 6672 -839
+ Misses 1319 1187 -132
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. |
|
Interesting, thank you very much for your contribution Im a bit worried about how the complexity of variables is growing. Maybe not for this PR, but we need to improve generation of code, even for this "selectable" feature |
| // CanBeSelected returns true if the variable supports selection (ie, `:foobar`) | ||
| func (v RuleVariable) CanBeSelected() bool { | ||
| switch v { |
There was a problem hiding this comment.
I don't think adding everything here makes sense. Just return true on those who can, and use the default otherwise.
There was a problem hiding this comment.
Although for performance it makes sense, I believe this is easier to maintain and more readable
There was a problem hiding this comment.
This is auto generated so I would not be concern about readability. @blotus could you do a quick benchmark on this matter i.e. adding everything or just true and all the rest on a default?
Definitely not for this PR. We should create an issue to refactor generation then. |
fzipi
changed the title
*_NAMES collections*_NAMES collections
|
LGTM in general, but I believe this lacks negative tests and its decreasing the general project coverage |
| @@ -101,11 +101,41 @@ type NamedCollectionNames struct { | |||
| } | |||
|
|
|||
| func (c *NamedCollectionNames) FindRegex(key *regexp.Regexp) []types.MatchData { | |||
| panic("selection operator not supported") | |||
| var res []types.MatchData | |||
There was a problem hiding this comment.
Is there any chance data is empty? if so I would handle the empty case before this allocation.
| var res []types.MatchData | ||
|
|
||
| for k, data := range c.collection.Map.data { | ||
| if key.MatchString(k) { |
There was a problem hiding this comment.
Please use early termination to reduce the indentation.
| for k, data := range c.collection.Map.data { | ||
| if key.MatchString(k) { | ||
| for _, d := range data { | ||
| res = append(res, &corazarules.MatchData{ |
There was a problem hiding this comment.
I wonder if MatchData is mutable, if so we probably want to reuse the pointer?
| @@ -22,6 +22,17 @@ func (v RuleVariable) Name() string { | |||
| } | |||
| } | |||
|
|
|||
| //CanBeSelected returns true if the variable supports selection (ie, `:foobar`) | |||
There was a problem hiding this comment.
| //CanBeSelected returns true if the variable supports selection (ie, `:foobar`) | |
| // CanBeSelected returns true if the variable supports selection (e.g. `:foobar`) |
Hello,
This PR aims to allow the use of rules such as
SecRule &REQUEST_COOKIES_NAMES:JSESSIONID "@eq 0" "id:45"(supported by ModSecurity and also present as an example in the documentation of Coraza), which currently causes Coraza to crash due to an explicitpaniccall.There are 3 main changes:
collections.NamedCollectionNamesimplementscollection.Keyed: this allows the use of a selector for the collections created with.Names()panicscalls: as Coraza is designed to be embedded in other software, callingpanicis never a good idea.Parser changes
I've embedded information about whether a collection can be selected or not in the
internal/variables/variables.gofile, as a comment for each collection that does support it (hopefully, I did not miss any), and added aCanBeSelectedmethod that is called during parsing to check if the selector is allowed or not.I don't know if I'm really happy with embedding information in comments, but it was the least intrusive way I found to handle this.
collections.NamedCollectionNamesimplementscollection.KeyedThis one is straightforward,
NamedCollectionNamesnow implementsGet,FindStringandFindRegex.Because it's a named collection, the key and the value in the returned results will be the same: the name of the key.
Remove runtime
panicThe first two were removed as part of making
namedCollectionNamesimplementsKeyed.The other two (which are the ones that caused the crash mentioned at the beginning of this PR) have been replaced by an error log.
In theory, this log should never occur because selectability is now checked during parsing (in practice, it could happen if a collection is marked as selectable but does not implement
Keyed).