Repository: corazawaf/coraza
Pull request #707 (2023-03-09 21:27:55)

fix: allow:phase and allow:request actions, extends related tests
It seems that `allow:phase` and `allow:request` actions are not working as expected, skipping only one rule and not the whole phase (or all the expected phases). Proposing a fix and extending related engine tests.

M4tteoP: Because of https://github.com/corazawaf/coraza/pull/527, I would really appreciate @fzipi feedback, I hope to have not misinterpreted some of your logic 🙏
codecov[bot]: ## [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/707?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Patch coverage: **`100.00`**% and project coverage change: **`-3.47`** :warning:
> Comparison is base [(`9e486ba`)](https://codecov.io/gh/corazawaf/coraza/commit/9e486ba422b434e34f89c20609602bc26340a675?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.33% compared to head [(`5edac47`)](https://codecov.io/gh/corazawaf/coraza/pull/707?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 77.87%.

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


```diff
@@            Coverage Diff             @@
##           v3/dev     #707      +/-   ##
==========================================
- Coverage   81.33%   77.87%   -3.47%     
==========================================
  Files         146      145       -1     
  Lines        8197     8179      -18     
==========================================
- Hits         6667     6369     -298     
- Misses       1303     1593     +290     
+ Partials      227      217      -10     
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `?` | |
| examples | `?` | |
| ftw | `?` | |
| ftw-multiphase | `49.94% <13.04%> (-0.09%)` | :arrow_down: |
| tinygo | `76.61% <100.00%> (+0.05%)` | :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/707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [internal/corazawaf/rulegroup.go](https://codecov.io/gh/corazawaf/coraza/pull/707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3J1bGVncm91cC5nbw==) | `96.47% <100.00%> (+0.54%)` | :arrow_up: |
| [testing/engine.go](https://codecov.io/gh/corazawaf/coraza/pull/707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-dGVzdGluZy9lbmdpbmUuZ28=) | `58.73% <100.00%> (ø)` | |

... and [17 files with indirect coverage changes](https://codecov.io/gh/corazawaf/coraza/pull/707/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/707?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/internal/corazawaf/rulegroup.go b/internal/corazawaf/rulegroup.go
index fe6c183..2ea341e 100644
--- a/internal/corazawaf/rulegroup.go
+++ b/internal/corazawaf/rulegroup.go
@@ -171,11 +171,28 @@ RulesLoop:
 		case corazatypes.AllowTypeUnset:
 			break
 		case corazatypes.AllowTypePhase:
-			tx.AllowType = corazatypes.AllowTypeUnset
-			continue RulesLoop
-		case corazatypes.AllowTypeRequest:
-			tx.AllowType = corazatypes.AllowTypeUnset
+			// Allow phase requires skipping all rules of the current phase.
+			// It is done by breaking the loop and resetting AllowType for the next phase right after the loop.
+			tx.DebugLogger().Debug().
+				Int("phase", int(phase)).
+				Msg("Skipping phase because of allow phase action")
 			break RulesLoop
+		case corazatypes.AllowTypeRequest:
+			// Allow request requires skipping all rules of any request phase.
+			// It is done by breaking the loop only if in a request phase (1 or 2)
+			// and resetting AllowType once the request phases are over (after request body phase)
+			tx.DebugLogger().Debug().
+				Int("phase", int(phase)).
+				Msg("Skipping phase because of allow request action")
+			if phase == types.PhaseRequestHeaders {
+				// tx.AllowType is not resetted because another request phase might be called
+				break RulesLoop
+			}
+			if phase == types.PhaseRequestBody {
+				// // tx.AllowType is resetted, currently PhaseRequestBody is the last request phase
+				tx.AllowType = corazatypes.AllowTypeUnset
+				break RulesLoop
+			}
 		case corazatypes.AllowTypeAll:
 			break RulesLoop
 		}
@@ -191,6 +208,13 @@ RulesLoop:
 		Int("phase", int(phase)).
 		Msg("Finished phase")
 
+	// Reset AllowType if meant to allow only this specific phase. It is particuarly needed
+	// to reset it at this point, in case of an allow:phase action enforced by the last rule of the phase.
+	// In this case, allow:phase must not have any impact on the next phase.
+	if tx.AllowType == corazatypes.AllowTypePhase {
+		tx.AllowType = corazatypes.AllowTypeUnset
+	}
+
 	tx.stopWatches[phase] = time.Now().UnixNano() - ts
 	return tx.interruption != nil
 }


Test patch:
diff --git a/testing/engine.go b/testing/engine.go
index 9fe17aa..d471213 100644
--- a/testing/engine.go
+++ b/testing/engine.go
@@ -188,7 +188,7 @@ func (t *Test) RunPhases() error {
 func (t *Test) OutputInterruptionErrors() []string {
 	var errors []string
 
-	if t.ExpectedOutput.Interruption != nil {
+	if t.ExpectedOutput.Interruption != nil && t.transaction.IsInterrupted() {
 		if t.ExpectedOutput.Interruption.Action != t.transaction.Interruption().Action {
 			errors = append(errors, fmt.Sprintf("Interruption.Action: expected: '%s', got: '%s'",
 				t.ExpectedOutput.Interruption.Action, t.transaction.Interruption().Action))
diff --git a/testing/engine/allow.go b/testing/engine/allow.go
index 6992839..05e3f5d 100644
--- a/testing/engine/allow.go
+++ b/testing/engine/allow.go
@@ -45,6 +45,7 @@ var _ = profile.RegisterProfile(profile.Profile{
 							},
 							NonTriggeredRules: []int{
 								12,
+								13,
 							},
 							Interruption: &profile.ExpectedInterruption{
 								Status: 500,
@@ -67,6 +68,8 @@ var _ = profile.RegisterProfile(profile.Profile{
 							},
 							NonTriggeredRules: []int{
 								32,
+								33,
+								34,
 							},
 							Interruption: &profile.ExpectedInterruption{
 								Status: 500,
@@ -77,21 +80,89 @@ var _ = profile.RegisterProfile(profile.Profile{
 						},
 					},
 				},
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI: "/useless_request_allow",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								45,
+								46,
+							},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 500,
+								Data:   "",
+								RuleID: 46,
+								Action: "deny",
+							},
+						},
+					},
+				},
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI:    "/allow_only_response?key=allow_only_response",
+							Method: "POST",
+							Headers: map[string]string{
+								"Content-type": "application/x-www-form-urlencoded",
+							},
+							Data: "allow_only_response",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								50,
+								51,
+							},
+							NonTriggeredRules: []int{
+								61,
+								62,
+							},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 500,
+								Data:   "",
+								RuleID: 51,
+								Action: "deny",
+							},
+						},
+					},
+				},
 			},
 		},
 	},
 	Rules: `
 SecDebugLogLevel 5
+SecRequestBodyAccess On
 SecRule REQUEST_URI "/allow_me" "id:1,phase:1,allow,msg:'ALLOWED'"
 SecRule REQUEST_URI "/allow_me" "id:2,phase:1,deny,msg:'DENIED'"
 
+# Rule 11 allows phase 1, so rule 12 and 13, being both phase 1, are not triggered. Rule 22 is then triggered in phase 2.
 SecRule REQUEST_URI "/partial_allow" "id:11,phase:1,allow:phase,msg:'Allowed in this phase only'"
 SecRule REQUEST_URI "/partial_allow" "id:12,phase:1,deny,msg:'NOT DENIED'"
+SecRule REQUEST_URI "/partial_allow" "id:13,phase:1,deny,msg:'NOT DENIED'"
 SecRule REQUEST_URI "/partial_allow" "id:22,phase:2,deny,status:500,msg:'DENIED'"
 
-SecRule REQUEST_URI "/request_allow" "id:31,phase:1,allow:request,msg:'Allowed at the request.'"
-SecRule REQUEST_URI "/request_allow" "id:32,phase:2,deny,msg:'NOT DENIED'"
+# Rule 31 allows all the request phases (1 and 2), therefore rules 32, 33 and 34 should not be triggered. Rule 42 is
+# expected to be triggered at phase 3.
+SecRule REQUEST_URI "/request_allow" "id:31,phase:1,allow:request,msg:'Allowed at the request'"
+SecRule REQUEST_URI "/request_allow" "id:32,phase:1,deny,msg:'NOT DENIED'"
+SecRule REQUEST_URI "/request_allow" "id:33,phase:2,deny,msg:'NOT DENIED'"
+SecRule REQUEST_URI "/request_allow" "id:34,phase:2,deny,msg:'NOT DENIED'"
 SecRule REQUEST_URI "/request_allow" "id:42,phase:3,deny,status:500,msg:'DENIED'"
 
+# Rule 45 allows only request phases (1 and 2), it should not impact on phase 3.
+# Therefor, rule 46 is expected to be triggered.
+SecRule REQUEST_URI "/useless_request_allow" "id:45,phase:1,allow:request,msg:'Allowed at the request'"
+SecRule REQUEST_URI "/useless_request_allow" "id:46,phase:3,deny,status:500,msg:'DENIED'"
+
+# Rule 50 allows only the current phase (phase 1), it should not impact any other rule (being part of other phases).
+# Rule 61 is meant to allow only from phase 3, rule 51, at phase 2 should deny the request
+# before reaching phase 3. Therefore rule 61 and 62 should not be triggered.
+# Suitable for testing that allow:phase is not propagated to other phases and for testing
+# multiphase evaluation combined with with allow actions.
+SecRule REQUEST_URI "/allow_only_response" "id:50,phase:1,allow:phase,msg:'Allowed phase 1'"
+SecRule REQUEST_BODY "allow_only_response" "id:51,phase:2,deny,status:500,msg:'Denied request'"
+SecRule REQUEST_URI "/allow_only_response" "id:61,phase:3,allow,msg:'Allowed Response not triggered'"
+SecRule REQUEST_URI "/allow_only_response" "id:62,phase:4,deny,msg:'Deny response not triggered'"
 `,
 })