Repository: corazawaf/coraza
Pull request #712 (2023-03-10 20:45:20)

chore: Adds checks for SecDefaultActions and related tests
This PR:
- Enforces validation checks for SecDefaultActions following @fzipi comments: https://github.com/corazawaf/coraza/issues/494#issuecomment-1407402538 and directive documentation
- Adds related tests
- Fixes defaultActionsPhase2 overridden by any SecDefaultAction defined (details in the code)

codecov[bot]: ## [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/712?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Patch coverage: **`85.00`**% and project coverage change: **`+0.13`** :tada:
> Comparison is base [(`1e241f0`)](https://codecov.io/gh/corazawaf/coraza/commit/1e241f023fe9f98797c52a077a6d8c162705961c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.42% compared to head [(`c03dde4`)](https://codecov.io/gh/corazawaf/coraza/pull/712?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.55%.

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


```diff
@@            Coverage Diff             @@
##           v3/dev     #712      +/-   ##
==========================================
+ Coverage   81.42%   81.55%   +0.13%     
==========================================
  Files         146      146              
  Lines        8235     8247      +12     
==========================================
+ Hits         6705     6726      +21     
+ Misses       1303     1297       -6     
+ Partials      227      224       -3     
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `77.56% <85.00%> (+0.14%)` | :arrow_up: |
| examples | `26.87% <35.00%> (-0.07%)` | :arrow_down: |
| ftw | `49.86% <40.00%> (-0.08%)` | :arrow_down: |
| ftw-multiphase | `49.94% <40.00%> (-0.08%)` | :arrow_down: |
| tinygo | `76.83% <85.00%> (+0.17%)` | :arrow_up: |

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/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [internal/corazawaf/rule.go](https://codecov.io/gh/corazawaf/coraza/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3J1bGUuZ28=) | `77.38% <ø> (ø)` | |
| [internal/seclang/rule\_parser.go](https://codecov.io/gh/corazawaf/coraza/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9ydWxlX3BhcnNlci5nbw==) | `85.90% <85.00%> (+1.79%)` | :arrow_up: |

... and [1 file with indirect coverage changes](https://codecov.io/gh/corazawaf/coraza/pull/712/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf)

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/712?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).

fzipi: Thanks @M4tteoP for addressing this!
jcchavezs: @fzipi are you happy with this?

Patch:
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/seclang/rule_parser.go b/internal/seclang/rule_parser.go
index 08a290e..d5c4ced 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 {
@@ -513,8 +533,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/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'"
 `,
 })