feat: allow selectors on `*_NAMES` collections by blotus · Pull Request #1143 · corazawaf/coraza

blotus · 2024-09-04T12:32:26Z

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 explicit panic call.

There are 3 main changes:

Check if a collection supports selectors during parsing time, instead of throwing an error at runtime.
Make collections.NamedCollectionNames implements collection.Keyed: this allows the use of a selector for the collections created with .Names()
Remove runtime panics calls: as Coraza is designed to be embedded in other software, calling panic is never a good idea.

Parser changes

I've embedded information about whether a collection can be selected or not in the internal/variables/variables.go file, as a comment for each collection that does support it (hopefully, I did not miss any), and added a CanBeSelected method 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.NamedCollectionNames` implements `collection.Keyed`

This one is straightforward, NamedCollectionNames now implements Get, FindString and FindRegex.
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 `panic`

The first two were removed as part of making namedCollectionNames implements Keyed.

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).

… non-selectable collection

codecov · 2024-09-04T12:35:37Z

Codecov Report

Attention: Patch coverage is 59.37500% with 39 lines in your changes missing coverage. Please review.

Project coverage is 83.84%. Comparing base (7f11024) to head (7c1b8fb).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/variables/variablesmap.gen.go	50.00%	26 Missing ⚠️
internal/corazawaf/transaction.go	8.33%	11 Missing ⚠️
internal/collections/named.go	93.10%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1143      +/-   ##
==========================================
- Coverage   83.98%   83.84%   -0.15%     
==========================================
  Files         170      170              
  Lines        9824     9909      +85     
==========================================
+ Hits         8251     8308      +57     
- Misses       1329     1357      +28     
  Partials      244      244

Flag	Coverage Δ
coraza.rule.case_sensitive_args_keys	`83.80% <59.37%> (-0.15%)`	⬇️
coraza.rule.multiphase_evaluation	`83.49% <59.37%> (-0.15%)`	⬇️
coraza.rule.no_regex_multiline	`83.78% <59.37%> (-0.15%)`	⬇️
default	`83.84% <59.37%> (-0.15%)`	⬇️
examples+	`16.31% <2.08%> (-0.21%)`	⬇️
examples+coraza.rule.case_sensitive_args_keys	`83.80% <59.37%> (-0.15%)`	⬇️
examples+coraza.rule.multiphase_evaluation	`83.34% <59.37%> (-0.15%)`	⬇️
examples+coraza.rule.no_regex_multiline	`83.70% <59.37%> (-0.15%)`	⬇️
examples+memoize_builders	`83.81% <59.37%> (-0.15%)`	⬇️
examples+no_fs_access	`81.09% <59.37%> (-0.13%)`	⬇️
ftw	`83.84% <59.37%> (-0.15%)`	⬇️
memoize_builders	`83.94% <59.37%> (-0.15%)`	⬇️
no_fs_access	`83.34% <59.37%> (-0.15%)`	⬇️
tinygo	`83.81% <59.37%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jptosso · 2024-09-04T13:51:19Z

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

fzipi · 2024-09-08T22:22:45Z

+// CanBeSelected returns true if the variable supports selection (ie, `:foobar`)
+func (v RuleVariable) CanBeSelected() bool {
+	switch v {


I don't think adding everything here makes sense. Just return true on those who can, and use the default otherwise.

Although for performance it makes sense, I believe this is easier to maintain and more readable

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?

I agree that readability does not really matter here.
I've updated the code generation to only generate the cases where true is returned.

fzipi · 2024-09-08T22:26:08Z

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

Definitely not for this PR. We should create an issue to refactor generation then.

jptosso · 2024-09-18T16:26:21Z

LGTM in general, but I believe this lacks negative tests and its decreasing the general project coverage

jcchavezs · 2024-09-23T09:51:59Z


 func (c *NamedCollectionNames) FindRegex(key *regexp.Regexp) []types.MatchData {
-	panic("selection operator not supported")
+	var res []types.MatchData


Is there any chance data is empty? if so I would handle the empty case before this allocation.

Yes, there are probably situations where data can be empty (I haven't tested, but I'd expect a collection like XML to have an empty data on a non-XML request )

AFAIK, declaring a slice like this does not perform any actual allocation (other than the header of the slice, which will be all zero), and the actual allocation will be performed the first time we append to it, but I can add a check on data if you are worried about it.

Also, from what I see, this check is not performed in the existing code (here for example)

jcchavezs · 2024-09-23T09:53:34Z

+	for k, data := range c.collection.Map.data {
+		if key.MatchString(k) {
+			for _, d := range data {
+				res = append(res, &corazarules.MatchData{


I wonder if MatchData is mutable, if so we probably want to reuse the pointer?

jptosso · 2024-11-13T09:14:35Z

 		} else {
-			panic("attempted to use regex with non-selectable collection: " + rv.Variable.Name())
+			// This should probably never happen, selectability is checked at parsing time
+			tx.debugLogger.Error().Str("collection", rv.Variable.Name()).Msg("attempted to use regex with non-selectable collection")


We discussed time ago that panic is ok, as this is a low level issue and coraza should not run here

I'm not sure I agree with this: coraza is designed as a library, and in my point of view, this means that explicit panics must be avoided at all costs (with very little exceptions, if you can call panic, you can return an error), and not doing anything is almost always better than bringing down a production website.

If a function call can lead to a panic, it should be made very clear to the caller (either with an explicit function name (Must....) or, at the very least, with some documentation): I don't mind wrapping every call to coraza with a recover, but I need to be aware it's required.

For this specific case, it can only (AFAIK) be triggered by a configuration error, so this means it should be detected when parsing the configuration (and is now thanks to this PR), so the panic has become redundant.

jcchavezs · 2025-05-30T06:50:43Z

Thanks a lot @blotus !

blotus added 7 commits September 3, 2024 13:25

Emit a parsing time error when attempting to perform a selection on a…

a5b714e

… non-selectable collection

ENV / RESPONSE_ARGS can be selected

73a2508

make collections.NamedCollectionNames implement collection.Keyed

dd38d5f

remove panics in Transaction.GetField()

0e4f805

change type of *Names collections to collection.Keyed

8d4e521

comment

21e6aaa

add middleware test case on count of args names

e84a320

blotus requested a review from a team as a code owner September 4, 2024 12:32

blotus added 4 commits September 4, 2024 14:45

reenable tests disabled for debugging

f26a5ea

fix linting error

c6a4048

fix linting error

77f4159

remove debug spew call

280af44

fzipi reviewed Sep 8, 2024

View reviewed changes

fzipi changed the title ~~Allow selectors on *_NAMES collections~~ feat: allow selectors on *_NAMES collections Sep 8, 2024

Merge branch 'main' into no-panic-on-non-selectable-col

8dbefad

jcchavezs reviewed Sep 23, 2024

View reviewed changes

Comment thread internal/collections/named.go Outdated

jcchavezs reviewed Sep 23, 2024

View reviewed changes

Comment thread internal/variables/generator/variablesmap.go.tmpl Outdated

LaurenceJJones reviewed Oct 1, 2024

View reviewed changes

Comment thread internal/collections/named.go Outdated

jcchavezs and others added 6 commits October 2, 2024 15:11

Merge branch 'main' into no-panic-on-non-selectable-col

5ead3ff

tag more collections as being selectable

279c462

only generate code for selectable collections

5c8a2e2

early termination in FindRegex/FindString

e1d83b2

Merge branch 'main' into no-panic-on-non-selectable-col

1a8f57c

add test for getting non-existing item from named collection

f8e778f

jptosso reviewed Nov 13, 2024

View reviewed changes

jptosso and others added 2 commits December 13, 2024 09:41

Merge branch 'main' into no-panic-on-non-selectable-col

47be8bb

Merge branch 'main' into no-panic-on-non-selectable-col

41ca211

LaurenceJJones mentioned this pull request Feb 4, 2025

[appsec] seclang rules that makes crowdsec crash crowdsecurity/crowdsec#3441

Closed

fix conflict

7c1b8fb

jcchavezs approved these changes May 30, 2025

View reviewed changes

jcchavezs merged commit 1faa41d into corazawaf:main May 30, 2025
70 of 72 checks passed

blotus mentioned this pull request Jun 5, 2025

fix: regenerate variables map to allow selection on all supported collections #1371

Merged

M4tteoP mentioned this pull request Dec 1, 2025

REQUEST_COOKIES_NAMES crashes(?) coraza #1450

Closed

Uh oh!

Conversation

blotus commented Sep 4, 2024

Parser changes

collections.NamedCollectionNames implements collection.Keyed

Remove runtime panic

Uh oh!

codecov bot commented Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jptosso commented Sep 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fzipi commented Sep 8, 2024

Uh oh!

jptosso commented Sep 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcchavezs commented May 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

`collections.NamedCollectionNames` implements `collection.Keyed`

Remove runtime `panic`

codecov bot commented Sep 4, 2024 •

edited

Loading