Restricts MatchedRule list to rules with log directive #840
Conversation
M4tteoP
changed the title
internal/seclang/rules_test.go
Outdated
| @@ -171,7 +171,8 @@ func TestRuleLogging(t *testing.T) { | |||
| tx.AddGetRequestArgument("test1", "123") | |||
| tx.AddGetRequestArgument("test2", "456") | |||
| tx.ProcessRequestHeaders() | |||
| if len(tx.MatchedRules()) != 3 { | |||
| // Only rules with log action are added to the matched rules, therefore we expect 2 occurrences | |||
| if len(tx.MatchedRules()) != 2 { | |||
There was a problem hiding this comment.
This test was testing that 3 rules were triggered (and added to matched rules, but only 2 logged something. With the new logic, also the intent of it changes a bit
internal/corazawaf/transaction.go
Outdated
| tx.WAF.ErrorLogCb(mr) | ||
| if r.Log { | ||
| // Only rules with log data will added to the matchedRules list | ||
| tx.matchedRules = append(tx.matchedRules, mr) |
There was a problem hiding this comment.
Should we not return from the top ... if r.Log is not there why to do even some processing (a debug log will help for those cases where a rule has an action but log -- possibly will be atypo only :)) -- cannot see anything obvious breaking here if we exit right from start
|
@jptosso That's why I opened it as a draft, I want to double-check that it is "just this" 😂 |
|
We should work on the concepts |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #840 +/- ##
==========================================
+ Coverage 81.51% 81.58% +0.06%
==========================================
Files 158 159 +1
Lines 8947 9007 +60
==========================================
+ Hits 7293 7348 +55
- Misses 1408 1412 +4
- Partials 246 247 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
I can not contribute with a code review, but if you can explain me what's up, I can at least comment from a conceptional perspective. Please start by explaining "matchdata list". |
M4tteoP
changed the title
|
Hey @dune73, thanks for chiming in. "matchdata list" in the title was actually not the right term (updated now), it rather is currently the |
|
Got you. I presume this list is coraza-specific and that's why I do not know it? Based on your explanation, I presume that a blocking rule with |
|
Giving to it a second thought, I don't think that adding an exception like this altering the concept of the Following an offline conversation with @jptosso, this PR is going to be superseded by #848. We aim to keep |
closes #839