Repository: corazawaf/coraza
Pull request #527 (2022-12-10 16:59:14)

feat: implement allow disruptive action
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>

Implements the `allow` disruptive action.

codecov-commenter: # [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/527?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Base: **78.21**% // Head: **78.43**% // Increases project coverage by **`+0.21%`** :tada:
> Coverage data is based on head [(`1e245e9`)](https://codecov.io/gh/corazawaf/coraza/pull/527?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) compared to base [(`fb52935`)](https://codecov.io/gh/corazawaf/coraza/commit/fb52935e8360bc64a2a8dbe64d8aa716d1b0cbda?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf).
> Patch coverage: 55.55% of modified lines in pull request are covered.

> :exclamation: Current head 1e245e9 differs from pull request most recent head c2d198c. Consider uploading reports for the commit c2d198c to get more accurate results

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


```diff
@@            Coverage Diff             @@
##           v3/dev     #527      +/-   ##
==========================================
+ Coverage   78.21%   78.43%   +0.21%     
==========================================
  Files         145      145              
  Lines        6876     6880       +4     
==========================================
+ Hits         5378     5396      +18     
+ Misses       1264     1250      -14     
  Partials      234      234              
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `73.70% <55.55%> (+0.22%)` | :arrow_up: |
| examples | `27.24% <22.22%> (+0.04%)` | :arrow_up: |
| ftw | `55.47% <22.22%> (+0.02%)` | :arrow_up: |
| tinygo | `71.77% <55.55%> (+0.23%)` | :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/527?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [internal/corazawaf/transaction.go](https://codecov.io/gh/corazawaf/coraza/pull/527/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3RyYW5zYWN0aW9uLmdv) | `79.94% <ø> (ø)` | |
| [internal/corazawaf/rulegroup.go](https://codecov.io/gh/corazawaf/coraza/pull/527/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3J1bGVncm91cC5nbw==) | `88.17% <45.45%> (-5.74%)` | :arrow_down: |
| [actions/allow.go](https://codecov.io/gh/corazawaf/coraza/pull/527/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-YWN0aW9ucy9hbGxvdy5nbw==) | `66.66% <66.66%> (+66.66%)` | :arrow_up: |
| [internal/corazawaf/waf.go](https://codecov.io/gh/corazawaf/coraza/pull/527/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3dhZi5nbw==) | `91.66% <100.00%> (+0.05%)` | :arrow_up: |

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 at Codecov](https://codecov.io/gh/corazawaf/coraza/pull/527?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).

jcchavezs: Any movement on this?
fzipi: I was going to piggyback on the internal movement that @jptosso was doing, and create some helpers around. But looks like that one never got pushed...
jptosso: moving to internal was canceled because wasm connector was using many things we were going to unexport. 
fzipi: Then I'll review where I was on this and push it again.
jcchavezs: Friendly ping @fzipi
fzipi: Thanks @jcchavezs ! More friendly pong!
jptosso: doesn't look good to me
fzipi: This one is ready to merge now.
jcchavezs: PTAL @anuraaga 
jptosso: PTAL @anuraaga 

Patch:
diff --git a/actions/allow.go b/actions/allow.go
index 9a6358f..de68dff 100644
--- a/actions/allow.go
+++ b/actions/allow.go
@@ -6,39 +6,44 @@ package actions
 import (
 	"fmt"
 
+	"github.com/corazawaf/coraza/v3/internal/corazatypes"
+	"github.com/corazawaf/coraza/v3/internal/corazawaf"
 	"github.com/corazawaf/coraza/v3/rules"
 )
 
 // 0 nothing, 1 phase, 2 request
 type allowFn struct {
-	allow int
+	allow corazatypes.AllowType
 }
 
 func (a *allowFn) Init(r rules.RuleMetadata, b1 string) error {
 	switch b1 {
 	case "phase":
-		a.allow = 2 // skip current phase
+		a.allow = corazatypes.AllowTypePhase // skip current phase
 	case "request":
-		a.allow = 3 // skip phases until RESPONSE_HEADERS
+		a.allow = corazatypes.AllowTypeRequest // skip phases until RESPONSE_HEADERS
 	case "":
-		a.allow = 1 // skip all phases
+		a.allow = corazatypes.AllowTypeAll // skip all phases
 	default:
 		return fmt.Errorf("invalid argument %s for allow", b1)
 	}
 	return nil
 }
 
-func (a *allowFn) Evaluate(r rules.RuleMetadata, tx rules.TransactionState) {
-	// TODO implement this:
-	/*
-		if a.allow == 1 {
-			tx.RuleEngine = coraza.RULE_ENGINE_OFF
-		} else if a.allow == 2 {
-			//tx.SkipToPhase = tx.LastPhase +1
-		} else if a.allow == 3 && tx.LastPhase < 3 {
-			//tx.SkipToPhase = 3
-		}
-	*/
+// Evaluate Allow has the following rules:
+//
+// Example: `SecRule REMOTE_ADDR "^192\.168\.1\.100$" "phase:1,id:95,nolog,allow"`
+//
+//   - If used one its own, like in the example above, allow will affect the entire transaction,
+//     stopping processing of the current phase but also skipping over all other phases apart from the logging phase.
+//     (The logging phase is special; it is designed to always execute.)
+//   - If used with parameter "phase", allow will cause the engine to stop processing the current phase.
+//     Other phases will continue as normal.
+//   - If used with parameter "request", allow will cause the engine to stop processing the current phase.
+//     The next phase to be processed will be phase types.PhaseResponseHeaders.
+func (a *allowFn) Evaluate(r rules.RuleMetadata, txS rules.TransactionState) {
+	tx := txS.(*corazawaf.Transaction)
+	tx.AllowType = a.allow
 }
 
 func (a *allowFn) Type() rules.ActionType {
diff --git a/internal/corazatypes/allow.go b/internal/corazatypes/allow.go
new file mode 100644
index 0000000..08978f0
--- /dev/null
+++ b/internal/corazatypes/allow.go
@@ -0,0 +1,14 @@
+// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors
+// SPDX-License-Identifier: Apache-2.0
+
+package corazatypes
+
+// AllowType represents the values that the allow disruptive action can take
+type AllowType int
+
+const (
+	AllowTypeUnset AllowType = iota
+	AllowTypeAll
+	AllowTypePhase
+	AllowTypeRequest
+)
diff --git a/internal/corazawaf/rulegroup.go b/internal/corazawaf/rulegroup.go
index 919fb49..9a3c2af 100644
--- a/internal/corazawaf/rulegroup.go
+++ b/internal/corazawaf/rulegroup.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/corazawaf/coraza/v3/internal/corazatypes"
 	"github.com/corazawaf/coraza/v3/internal/strings"
 	"github.com/corazawaf/coraza/v3/types"
 	"github.com/corazawaf/coraza/v3/types/variables"
@@ -107,6 +108,8 @@ func (rg *RuleGroup) Clear() {
 }
 
 // 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.
 // Returns true if transaction is disrupted
 func (rg *RuleGroup) Eval(phase types.RulePhase, tx *Transaction) bool {
 	tx.WAF.Logger.Debug("[%s] Evaluating phase %d", tx.id, int(phase))
@@ -119,6 +122,8 @@ func (rg *RuleGroup) Eval(phase types.RulePhase, tx *Transaction) bool {
 	}
 RulesLoop:
 	for _, r := range tx.WAF.Rules.GetRules() {
+		// if there is already an interruption and the phase isn't logging
+		// we break the loop
 		if tx.interruption != nil && phase != types.PhaseLogging {
 			break RulesLoop
 		}
@@ -152,7 +157,19 @@ RulesLoop:
 			// Skipping rule
 			continue
 		}
-		// TODO this lines are SUPER SLOW
+		switch tx.AllowType {
+		case corazatypes.AllowTypeUnset:
+			break
+		case corazatypes.AllowTypePhase:
+			tx.AllowType = corazatypes.AllowTypeUnset
+			continue RulesLoop
+		case corazatypes.AllowTypeRequest:
+			tx.AllowType = corazatypes.AllowTypeUnset
+			break RulesLoop
+		case corazatypes.AllowTypeAll:
+			break RulesLoop
+		}
+		// TODO these lines are SUPER SLOW
 		// we reset matched_vars, matched_vars_names, etc
 		tx.variables.matchedVars.Reset()
 		tx.variables.matchedVarsNames.Reset()
diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go
index e0180b3..c463475 100644
--- a/internal/corazawaf/transaction.go
+++ b/internal/corazawaf/transaction.go
@@ -20,6 +20,7 @@ import (
 	"github.com/corazawaf/coraza/v3/collection"
 	"github.com/corazawaf/coraza/v3/internal/collections"
 	"github.com/corazawaf/coraza/v3/internal/corazarules"
+	corazatypes "github.com/corazawaf/coraza/v3/internal/corazatypes"
 	stringsutil "github.com/corazawaf/coraza/v3/internal/strings"
 	urlutil "github.com/corazawaf/coraza/v3/internal/url"
 	"github.com/corazawaf/coraza/v3/loggers"
@@ -51,6 +52,9 @@ type Transaction struct {
 	// Rules will be skipped after a rule with this SecMarker is found
 	SkipAfter string
 
+	// AllowType is used by the allow disruptive action to skip evaluating rules after being allowed
+	AllowType corazatypes.AllowType
+
 	// Copies from the WAF instance that may be overwritten by the ctl action
 	AuditEngine              types.AuditEngineStatus
 	AuditLogParts            types.AuditLogParts
diff --git a/internal/corazawaf/waf.go b/internal/corazawaf/waf.go
index 1bade4d..f453321 100644
--- a/internal/corazawaf/waf.go
+++ b/internal/corazawaf/waf.go
@@ -161,6 +161,7 @@ func (w *WAF) newTransactionWithID(id string) *Transaction {
 	tx.ruleRemoveByID = nil
 	tx.ruleRemoveTargetByID = map[int][]ruleVariableParams{}
 	tx.Skip = 0
+	tx.AllowType = 0
 	tx.Capture = false
 	tx.stopWatches = map[types.RulePhase]int64{}
 	tx.WAF = w
diff --git a/types/transaction.go b/types/transaction.go
index 637e217..5b8510e 100644
--- a/types/transaction.go
+++ b/types/transaction.go
@@ -7,12 +7,12 @@ import (
 	"io"
 )
 
-// ArgumentType is used to define types of argument for transactions
-// There are three supported types: POST, GET and PATH
+// ArgumentType is used to define corazatypes of argument for transactions
+// There are three supported corazatypes: POST, GET and PATH
 type ArgumentType int
 
 const (
-	// ArgumentInvalid is used to define invalid argument types
+	// ArgumentInvalid is used to define invalid argument corazatypes
 	ArgumentInvalid ArgumentType = iota
 	// ArgumentGET is used to define GET arguments
 	ArgumentGET


Test patch:
diff --git a/http/middleware_test.go b/http/middleware_test.go
index 6f0c81d..03e6216 100644
--- a/http/middleware_test.go
+++ b/http/middleware_test.go
@@ -294,6 +294,16 @@ func TestHttpServer(t *testing.T) {
 			expectedStatus:   403,
 			expectedRespBody: "", // blocking at response body phase means returning it empty
 		},
+		"allow": {
+			reqURI:         "/allow_me",
+			expectedProto:  "HTTP/1.1",
+			expectedStatus: 201,
+		},
+		"deny passes over allow due to ordering": {
+			reqURI:         "/allow_me?id=0",
+			expectedProto:  "HTTP/1.1",
+			expectedStatus: 403,
+		},
 	}
 
 	// Perform tests
@@ -311,10 +321,11 @@ func TestHttpServer(t *testing.T) {
 	SecResponseBodyAccess On
 	SecResponseBodyMimeType text/plain
 	SecRequestBodyLimitAction ` + limitAction + `
-	SecRule ARGS:id "@eq 0" "id:1, phase:1,deny, status:403,msg:'Invalid id',log,auditlog"
+	SecRule ARGS:id "@eq 0" "id:10, phase:1,deny, status:403,msg:'Invalid id',log,auditlog"
 	SecRule REQUEST_BODY "@contains eval" "id:100, phase:2,deny, status:403,msg:'Invalid request body',log,auditlog"
 	SecRule RESPONSE_HEADERS:Foo "@pm bar" "id:199,phase:3,deny,t:lowercase,deny, status:401,msg:'Invalid response header',log,auditlog"
 	SecRule RESPONSE_BODY "@contains password" "id:200, phase:4,deny, status:403,msg:'Invalid response body',log,auditlog"
+	SecRule REQUEST_URI "/allow_me" "id:9,phase:1,allow,msg:'ALLOWED'"
 `).WithErrorCallback(errLogger(t)).WithDebugLogger(&debugLogger{t: t})
 			if l := tCase.reqBodyLimit; l > 0 {
 				conf = conf.WithRequestBodyAccess().WithRequestBodyLimit(l).WithRequestBodyInMemoryLimit(l)
diff --git a/testing/engine/allow.go b/testing/engine/allow.go
new file mode 100644
index 0000000..6992839
--- /dev/null
+++ b/testing/engine/allow.go
@@ -0,0 +1,97 @@
+// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors
+// SPDX-License-Identifier: Apache-2.0
+
+package engine
+
+import (
+	"github.com/corazawaf/coraza/v3/testing/profile"
+)
+
+var _ = profile.RegisterProfile(profile.Profile{
+	Meta: profile.Meta{
+		Author:      "fzipi",
+		Description: "Test if the allow action works",
+		Enabled:     true,
+		Name:        "allow.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "actions",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI: "/allow_me?key=value&key=other_value",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								1,
+							},
+							NonTriggeredRules: []int{
+								2,
+							},
+						},
+					},
+				},
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI: "/partial_allow?key=value&key=other_value",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								11,
+								22,
+							},
+							NonTriggeredRules: []int{
+								12,
+							},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 500,
+								Data:   "",
+								RuleID: 22,
+								Action: "deny",
+							},
+						},
+					},
+				},
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI: "/request_allow?key=value&key=other_value",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{
+								31,
+								42,
+							},
+							NonTriggeredRules: []int{
+								32,
+							},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 500,
+								Data:   "",
+								RuleID: 42,
+								Action: "deny",
+							},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+SecDebugLogLevel 5
+SecRule REQUEST_URI "/allow_me" "id:1,phase:1,allow,msg:'ALLOWED'"
+SecRule REQUEST_URI "/allow_me" "id:2,phase:1,deny,msg:'DENIED'"
+
+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: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'"
+SecRule REQUEST_URI "/request_allow" "id:42,phase:3,deny,status:500,msg:'DENIED'"
+
+`,
+})