Repository: corazawaf/coraza
Pull request #726 (2023-03-16 07:19:24)

Cleanup and optimize rulegroup
It is best to use a slice of structs rather than pointers wherever possible to reduce memory usage a little and improve iteration performance.

This also replaces some find+delete patterns with just delete since they are internal code anyways

codecov[bot]: ## [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/726?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Patch coverage: **`60.00`**% and project coverage change: **`-0.10`** :warning:
> Comparison is base [(`c10adce`)](https://codecov.io/gh/corazawaf/coraza/commit/c10adce59e7f902682837e38d9877399fe04e678?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.52% compared to head [(`593230b`)](https://codecov.io/gh/corazawaf/coraza/pull/726?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.43%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           v3/dev     #726      +/-   ##
==========================================
- Coverage   81.52%   81.43%   -0.10%     
==========================================
  Files         148      148              
  Lines        8266     8258       -8     
==========================================
- Hits         6739     6725      -14     
- Misses       1300     1304       +4     
- Partials      227      229       +2     
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `77.44% <60.00%> (-0.10%)` | :arrow_down: |
| examples | `25.60% <24.00%> (+0.03%)` | :arrow_up: |
| ftw | `49.64% <28.00%> (+0.02%)` | :arrow_up: |
| ftw-multiphase | `49.73% <28.00%> (+0.02%)` | :arrow_up: |
| tinygo | `76.55% <60.00%> (-0.11%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/corazawaf/coraza/pull/726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [internal/seclang/directives.go](https://codecov.io/gh/corazawaf/coraza/pull/726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9kaXJlY3RpdmVzLmdv) | `71.58% <0.00%> (+0.60%)` | :arrow_up: |
| [internal/corazawaf/rulegroup.go](https://codecov.io/gh/corazawaf/coraza/pull/726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3J1bGVncm91cC5nbw==) | `89.43% <61.90%> (-7.14%)` | :arrow_down: |
| [actions/ctl.go](https://codecov.io/gh/corazawaf/coraza/pull/726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-YWN0aW9ucy9jdGwuZ28=) | `95.84% <100.00%> (ø)` | |
| [internal/seclang/rule\_parser.go](https://codecov.io/gh/corazawaf/coraza/pull/726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9ydWxlX3BhcnNlci5nbw==) | `84.07% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf)

</details>

[:umbrella: View full report in Codecov by Sentry](https://codecov.io/gh/corazawaf/coraza/pull/726?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf).


Patch:
diff --git a/actions/ctl.go b/actions/ctl.go
index 42137f1..b50fc3a 100644
--- a/actions/ctl.go
+++ b/actions/ctl.go
@@ -355,7 +355,7 @@ func parseCtl(data string) (ctlFunctionType, string, variables.RuleVariable, str
 	return act, value, collection, strings.TrimSpace(colkey), nil
 }
 
-func rangeToInts(rules []*corazawaf.Rule, input string) ([]int, error) {
+func rangeToInts(rules []corazawaf.Rule, input string) ([]int, error) {
 	if len(input) == 0 {
 		return nil, errors.New("empty input")
 	}
diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go
index 050ba45..76af7d9 100644
--- a/internal/corazawaf/rule.go
+++ b/internal/corazawaf/rule.go
@@ -512,6 +512,7 @@ func (r *Rule) executeTransformations(value string) (string, []error) {
 }
 
 // NewRule returns a new initialized rule
+// By default, the rule is set to phase 2
 func NewRule() *Rule {
 	return &Rule{
 		RuleMetadata: corazarules.RuleMetadata{
diff --git a/internal/corazawaf/rulegroup.go b/internal/corazawaf/rulegroup.go
index 96c9415..214f22c 100644
--- a/internal/corazawaf/rulegroup.go
+++ b/internal/corazawaf/rulegroup.go
@@ -18,7 +18,7 @@ import (
 // It is not concurrent safe, so it's not recommended to use it
 // after compilation
 type RuleGroup struct {
-	rules []*Rule
+	rules []Rule
 }
 
 // Add a rule to the collection
@@ -29,7 +29,7 @@ func (rg *RuleGroup) Add(rule *Rule) error {
 		return nil
 	}
 
-	if rg.FindByID(rule.ID_) != nil && rule.ID_ != 0 {
+	if rule.ID_ != 0 && rg.FindByID(rule.ID_) != nil {
 		return fmt.Errorf("there is a another rule with id %d", rule.ID_)
 	}
 
@@ -45,20 +45,20 @@ func (rg *RuleGroup) Add(rule *Rule) error {
 		}
 	}
 
-	rg.rules = append(rg.rules, rule)
+	rg.rules = append(rg.rules, *rule)
 	return nil
 }
 
 // GetRules returns the slice of rules,
-func (rg *RuleGroup) GetRules() []*Rule {
+func (rg *RuleGroup) GetRules() []Rule {
 	return rg.rules
 }
 
 // FindByID return a Rule with the requested Id
 func (rg *RuleGroup) FindByID(id int) *Rule {
-	for _, r := range rg.rules {
+	for i, r := range rg.rules {
 		if r.ID_ == id {
-			return r
+			return &rg.rules[i]
 		}
 	}
 	return nil
@@ -67,34 +67,33 @@ func (rg *RuleGroup) FindByID(id int) *Rule {
 // DeleteByID removes a rule by it's Id
 func (rg *RuleGroup) DeleteByID(id int) {
 	for i, r := range rg.rules {
-		if r != nil && r.ID_ == id {
-			copy(rg.rules[i:], rg.rules[i+1:])
-			rg.rules[len(rg.rules)-1] = nil
-			rg.rules = rg.rules[:len(rg.rules)-1]
+		if r.ID_ == id {
+			rg.rules = append(rg.rules[:i], rg.rules[i+1:]...)
+			return
 		}
 	}
 }
 
-// FindByMsg returns a slice of rules that matches the msg
-func (rg *RuleGroup) FindByMsg(msg string) []*Rule {
-	var rules []*Rule
+// DeleteByMsg deletes rules with the given message.
+func (rg *RuleGroup) DeleteByMsg(msg string) {
+	var kept []Rule
 	for _, r := range rg.rules {
-		if r.Msg.String() == msg {
-			rules = append(rules, r)
+		if r.Msg.String() != msg {
+			kept = append(kept, r)
 		}
 	}
-	return rules
+	rg.rules = kept
 }
 
-// FindByTag returns a slice of rules that matches the tag
-func (rg *RuleGroup) FindByTag(tag string) []*Rule {
-	var rules []*Rule
+// DeleteByTag deletes rules with the given tag.
+func (rg *RuleGroup) DeleteByTag(tag string) {
+	var kept []Rule
 	for _, r := range rg.rules {
-		if strings.InSlice(tag, r.Tags_) {
-			rules = append(rules, r)
+		if !strings.InSlice(tag, r.Tags_) {
+			kept = append(kept, r)
 		}
 	}
-	return rules
+	rg.rules = kept
 }
 
 // Count returns the count of rules
@@ -102,11 +101,6 @@ func (rg *RuleGroup) Count() int {
 	return len(rg.rules)
 }
 
-// Clear will remove each and every rule stored
-func (rg *RuleGroup) Clear() {
-	rg.rules = []*Rule{}
-}
-
 // Eval rules for the specified phase, between 1 and 5
 // Rules are evaluated in syntactic order and the evaluation finishes
 // as soon as an interruption has been triggered.
@@ -124,7 +118,8 @@ func (rg *RuleGroup) Eval(phase types.RulePhase, tx *Transaction) bool {
 		delete(transformationCache, k)
 	}
 RulesLoop:
-	for _, r := range tx.WAF.Rules.GetRules() {
+	for i := range rg.rules {
+		r := &rg.rules[i]
 		// if there is already an interruption and the phase isn't logging
 		// we break the loop
 		if tx.interruption != nil && phase != types.PhaseLogging {
@@ -226,9 +221,7 @@ RulesLoop:
 // You might use this function to replace the rules
 // and "reload" the WAF
 func NewRuleGroup() RuleGroup {
-	return RuleGroup{
-		rules: []*Rule{},
-	}
+	return RuleGroup{}
 }
 
 type transformationKey struct {
diff --git a/internal/seclang/directives.go b/internal/seclang/directives.go
index 271c209..44b3575 100644
--- a/internal/seclang/directives.go
+++ b/internal/seclang/directives.go
@@ -313,9 +313,7 @@ func directiveSecRuleRemoveByTag(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	for _, r := range options.WAF.Rules.FindByTag(options.Opts) {
-		options.WAF.Rules.DeleteByID(r.ID_)
-	}
+	options.WAF.Rules.DeleteByTag(options.Opts)
 	return nil
 }
 
@@ -324,9 +322,7 @@ func directiveSecRuleRemoveByMsg(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	for _, r := range options.WAF.Rules.FindByMsg(options.Opts) {
-		options.WAF.Rules.DeleteByID(r.ID_)
-	}
+	options.WAF.Rules.DeleteByMsg(options.Opts)
 	return nil
 }
 
diff --git a/internal/seclang/rule_parser.go b/internal/seclang/rule_parser.go
index 08a290e..9e229af 100644
--- a/internal/seclang/rule_parser.go
+++ b/internal/seclang/rule_parser.go
@@ -17,7 +17,7 @@ import (
 	"github.com/corazawaf/coraza/v3/types/variables"
 )
 
-var defaultActionsPhase2 = []string{"phase:2,log,auditlog,pass"}
+var defaultActionsPhase2 = "phase:2,log,auditlog,pass"
 
 type ruleAction struct {
 	Key   string
@@ -228,12 +228,23 @@ func (p *RuleParser) ParseDefaultActions(actions string) error {
 		if action.Atype == rules.ActionTypeDisruptive {
 			defaultDisruptive = action.Key
 		}
+		// SecDefaultActions can not contain metadata actions
+		if action.Atype == rules.ActionTypeMetadata {
+			return fmt.Errorf("SecDefaultAction must not contain metadata actions: %s", actions)
+		}
+		// Transformations are not suitable to be part of the default actions defined by SecDefaultActions
+		if action.Key == "t" {
+			return fmt.Errorf("SecDefaultAction must not contain transformation actions: %s", actions)
+		}
 	}
 	if phase == 0 {
-		return errors.New("SecDefaultAction must contain a phase")
+		return fmt.Errorf("SecDefaultAction must contain a phase")
 	}
 	if defaultDisruptive == "" {
-		return errors.New("SecDefaultAction must contain a disruptive action: " + actions)
+		return fmt.Errorf("SecDefaultAction must contain a disruptive action: %s", actions)
+	}
+	if p.defaultActions[types.RulePhase(phase)] != nil {
+		return fmt.Errorf("SecDefaultAction already defined for this phase: %s", actions)
 	}
 	p.defaultActions[types.RulePhase(phase)] = act
 	return nil
@@ -262,6 +273,7 @@ func (p *RuleParser) ParseActions(actions string) error {
 		}
 	}
 
+	// if the rule is missing the phase, the default phase assigned is phase 2 (See NewRule())
 	phase := p.rule.Phase_
 
 	defaults := p.defaultActions[phase]
@@ -314,19 +326,27 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 		rule:           corazawaf.NewRule(),
 		defaultActions: map[types.RulePhase][]ruleAction{},
 	}
-
-	defaultActions := defaultActionsPhase2
+	var defaultActionsRaw []string
+	// Default actions are persisted only inside the ParserConfig, therefore they are parsed every time a rule is parsed
+	// and not just once when the SecDefaultAction is read.
 	if options.ParserConfig.HasRuleDefaultActions {
-		defaultActions = options.ParserConfig.RuleDefaultActions
+		defaultActionsRaw = options.ParserConfig.RuleDefaultActions
 	}
 	disabledRuleOperators := options.ParserConfig.DisabledRuleOperators
+	for _, da := range defaultActionsRaw {
 
-	for _, da := range defaultActions {
 		err = rp.ParseDefaultActions(da)
 		if err != nil {
 			return nil, err
 		}
 	}
+	// If no default actions for phase 2 are defined, defaultActionsPhase2 variable (hardcoded default actions for phase 2) is used.
+	if rp.defaultActions[types.PhaseRequestBody] == nil {
+		err = rp.ParseDefaultActions(defaultActionsPhase2)
+		if err != nil {
+			return nil, err
+		}
+	}
 	actions := ""
 
 	if options.WithOperator {
@@ -434,7 +454,8 @@ func getLastRuleExpectingChain(w *corazawaf.WAF) *corazawaf.Rule {
 	if len(rules) == 0 {
 		return nil
 	}
-	lastRule := rules[len(rules)-1]
+
+	lastRule := &rules[len(rules)-1]
 	parent := lastRule
 	for parent.Chain != nil {
 		parent = parent.Chain
@@ -443,6 +464,7 @@ func getLastRuleExpectingChain(w *corazawaf.WAF) *corazawaf.Rule {
 	if parent.HasChain && parent.Chain == nil {
 		return lastRule
 	}
+
 	return nil
 }
 
@@ -513,8 +535,8 @@ func parseActions(actions string) ([]ruleAction, error) {
 }
 
 func appendRuleAction(res []ruleAction, key string, val string, disruptiveActionIndex int) ([]ruleAction, int, error) {
-	key = strings.TrimSpace(key)
-	val = strings.TrimSpace(val)
+	key = strings.ToLower(strings.TrimSpace(key))
+	val = strings.TrimSpace(val) // We may want to keep case sensitive values (e.g. Messages)
 	val = utils.MaybeRemoveQuotes(val)
 	f, err := actionsmod.Get(key)
 	if err != nil {


Test patch:
diff --git a/actions/ctl_test.go b/actions/ctl_test.go
index fd10599..ca9a443 100644
--- a/actions/ctl_test.go
+++ b/actions/ctl_test.go
@@ -424,7 +424,7 @@ func TestParseCtl(t *testing.T) {
 
 }
 func TestCtlParseRange(t *testing.T) {
-	rules := []*corazawaf.Rule{
+	rules := []corazawaf.Rule{
 		{
 			RuleMetadata: corazarules.RuleMetadata{
 				ID_: 5,
diff --git a/internal/corazawaf/rulegroup_test.go b/internal/corazawaf/rulegroup_test.go
index d6f87f8..8bf13f4 100644
--- a/internal/corazawaf/rulegroup_test.go
+++ b/internal/corazawaf/rulegroup_test.go
@@ -27,15 +27,20 @@ func TestRG(t *testing.T) {
 		t.Error("Failed to add rule to rulegroup")
 	}
 
-	if len(rg.FindByMsg("test")) != 1 {
-		t.Error("Failed to find rules by msg")
+	rg.DeleteByTag("test")
+	if rg.Count() != 0 {
+		t.Error("Failed to remove rule from rulegroup")
+	}
+
+	if err := rg.Add(r); err != nil {
+		t.Error("Failed to add rule to rulegroup")
 	}
 
-	if len(rg.FindByTag("test")) != 1 {
-		t.Error("Failed to find rules by tag")
+	if rg.Count() != 1 {
+		t.Error("Failed to add rule to rulegroup")
 	}
 
-	rg.DeleteByID(1)
+	rg.DeleteByMsg("test")
 	if rg.Count() != 0 {
 		t.Error("Failed to remove rule from rulegroup")
 	}
diff --git a/internal/seclang/rule_parser_test.go b/internal/seclang/rule_parser_test.go
index 4761fc2..a9af315 100644
--- a/internal/seclang/rule_parser_test.go
+++ b/internal/seclang/rule_parser_test.go
@@ -126,10 +126,83 @@ func TestSecRuleUpdateTargetVariableNegation(t *testing.T) {
 	}
 }
 
+func TestDefaultActionsErrors(t *testing.T) {
+	testCases := map[string]struct {
+		rules string
+	}{
+		"SecDefaultAction with bad actions": {
+			rules: `SecDefaultAction "logauditlog,pass"`,
+		},
+		"Missing phase": {
+			rules: `SecDefaultAction "log,auditlog,pass"`,
+		},
+		"Bad phase": {
+			rules: `SecDefaultAction "phase:A,log,auditlog,pass"`,
+		},
+		"Missing disruptive action": {
+			rules: `SecDefaultAction "phase:1,log,auditlog"`,
+		},
+		"SecDefaultAction with metadata action": {
+			rules: `SecDefaultAction "phase:1,log,auditlog,pass,msg:'metadata action'"`,
+		},
+		"SecDefaultAction with a transformation": {
+			rules: `SecDefaultAction "phase:1,log,auditlog,pass,t:none"`,
+		},
+		"SecDefaultAction with a transformation uppercase": {
+			rules: `SecDefaultAction "phase:1,log,auditlog,pass,T:NoNe"`,
+		},
+		"Multiple SecDefaultAction for the same phase": {
+			rules: `SecDefaultAction "phase:1,log,auditlog,pass"
+			SecDefaultAction "phase:1,nolog,noauditlog,pass"`,
+		},
+	}
+	for name, tCase := range testCases {
+		t.Run(name, func(t *testing.T) {
+			dummySecAction := `
+			SecAction "id:1,phase:1" `
+			waf := corazawaf.NewWAF()
+			p := NewParser(waf)
+			// SecDefaultActions are parsed only when a rule is parsed, thus we add a dummy SecAction
+			err := p.FromString(tCase.rules + dummySecAction)
+			if err == nil {
+				t.Error("expected error")
+			}
+		})
+	}
+}
+
+func TestDefaultActionsForPhase2Overridable(t *testing.T) {
+	waf := corazawaf.NewWAF()
+	p := NewParser(waf)
+	// A SecDefaultAction at phase:2 defined by the user has to override the hardcoded defaultActionsPhase2
+	err := p.FromString(`
+	SecDefaultAction "phase:2,nolog,noauditlog,pass"
+	SecAction "id:1,noauditlog"
+	SecAction "id:2,phase:1"`)
+	if err != nil {
+		t.Errorf("unexpected error: %s", err.Error())
+	}
+	if waf.Rules.GetRules()[0].Log != false {
+		t.Error("failed to set log to false")
+	}
+	if waf.Rules.GetRules()[0].Audit != false {
+		t.Error("failed to set audit to false")
+	}
+	if waf.Rules.GetRules()[1].Log != false {
+		t.Error("phase 1 rules shouldn't have log set by default actions of different phases")
+	}
+	if waf.Rules.GetRules()[1].Audit != false {
+		t.Error("phase 1 rules shouldn't have log set by default actions of different phases")
+	}
+}
+
 func TestDefaultActionsForPhase2(t *testing.T) {
 	waf := corazawaf.NewWAF()
 	p := NewParser(waf)
+	// Via defaultActionsPhase2 variable, the default actions for phase 2 are hardcoded in Coraza.
+	// Only a SecDefaultAction of the same phase should override it.
 	err := p.FromString(`
+	SecDefaultAction "phase:3,log,auditlog,pass"
 	SecAction "id:1,phase:2"
 	SecAction "id:2,phase:1"`)
 	if err != nil {
@@ -143,7 +216,7 @@ func TestDefaultActionsForPhase2(t *testing.T) {
 	}
 
 	if waf.Rules.GetRules()[1].Log || waf.Rules.GetRules()[1].Audit {
-		t.Error("phase 1 rules shouldn't have log set by default actions")
+		t.Error("phase 1 rules shouldn't have log set by default actions of different phases")
 	}
 }
 
diff --git a/testing/engine/disruptive_actions.go b/testing/engine/disruptive_actions.go
index e662820..2f18887 100644
--- a/testing/engine/disruptive_actions.go
+++ b/testing/engine/disruptive_actions.go
@@ -304,7 +304,9 @@ SecRule REQUEST_URI "/redirect5$" "phase:5,id:51,log,status:302,redirect:https:/
 SecRule REQUEST_URI "/deny5$" "phase:5,id:52,log,status:500,deny"
 SecRule REQUEST_URI "/drop5$" "phase:5,id:53,log,drop"
 
-SecDefaultAction phase:2,deny,status:501,log,logdata:'WOOOP_BLOCKED_BY_CORAZA_TEST'
-SecRule REQUEST_URI "/default/block" "id:103,block"
+# Rule 103 is missing the phase, therefore phase:2 is implicitly applied with its related default actions
+# So we will expect a deny with 501 response for the blocking action.
+SecDefaultAction "phase:2,deny,status:501,log"
+SecRule REQUEST_URI "/default/block" "id:103,block,logdata:'WOOOP_BLOCKED_BY_CORAZA_TEST'"
 `,
 })