chore: setvar minor fix, tests, added warning when missing variable, …

…deprecates usage of tx.LogData (#892) * chore: setvar minor fix, tests, added warning when missing variable * adds tests with numerical operation starting with a negative number * deprecating Logdata field in transaction * updates test name
corazawaf · Nov 3, 2023 · 99da3ed · 99da3ed
1 parent a0a65dd
commit 99da3ed
Show file tree

Hide file tree

Showing 12 changed files with 156 additions and 38 deletions.
diff --git a/examples/http-server/go.mod b/examples/http-server/go.mod
@@ -1,6 +1,6 @@
 module github.com/corazawaf/coraza/v3/examples/http-server
 
-go 1.18
+go 1.19
 
 require github.com/corazawaf/coraza/v3 v3.0.0-20220914101451-05d352c89b24
 

diff --git a/experimental/plugins/macro/macro.go b/experimental/plugins/macro/macro.go
@@ -82,6 +82,8 @@ func expandToken(tx plugintypes.TransactionState, token macroToken) string {
 		}
 	}
 
+	// If the variable is known (e.g. TX) but the key is not found, we return the original text
+	tx.DebugLogger().Warn().Str("variable", token.variable.Name()).Str("key", token.key).Msg("key not found in collection, returning the original text")
 	return token.text
 }
 

diff --git a/go.work.sum b/go.work.sum
@@ -1,6 +1,5 @@
 golang.org/x/crypto v0.14.0 h1:wBqGXzWJW6m1XrIKlAH0Hs1JJ7+9KBwnIO8v66Q9cHc=
 golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf4=
 golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg=
-golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y=
 golang.org/x/term v0.13.0/go.mod h1:LTmsnFJwVN6bCy1rVCoS+qHT1HhALEFxKncY3WNNh4U=
 golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
diff --git a/internal/actions/logdata.go b/internal/actions/logdata.go
@@ -37,7 +37,7 @@ func (a *logdataFn) Init(r plugintypes.RuleMetadata, data string) error {
 }
 
 func (a *logdataFn) Evaluate(r plugintypes.RuleMetadata, tx plugintypes.TransactionState) {
-	tx.(*corazawaf.Transaction).Logdata = r.(*corazawaf.Rule).LogData.Expand(tx)
+	// logdata macro expansion is performed after all other actions have been evaluated (and potentially all the needed variables have been set)
 }
 
 func (a *logdataFn) Type() plugintypes.ActionType {

diff --git a/internal/actions/setvar.go b/internal/actions/setvar.go
@@ -136,20 +136,19 @@ func (a *setvarFn) evaluateTxCollection(r plugintypes.RuleMetadata, tx plugintyp
 		col.Remove(key)
 		return
 	}
-	res := ""
+	currentVal := ""
 	if r := col.Get(key); len(r) > 0 {
-		res = r[0]
+		currentVal = r[0]
 	}
 	var err error
 	switch {
 	case len(value) == 0:
 		// if nothing to input
 		col.Set(key, []string{""})
-	case value[0] == '+':
-		// if we want to sum
-		sum := 0
+	case value[0] == '+', value[0] == '-': // Increment or decrement, arithmetical operations
+		val := 0
 		if len(value) > 1 {
-			sum, err = strconv.Atoi(value[1:])
+			val, err = strconv.Atoi(value[1:])
 			if err != nil {
 				tx.DebugLogger().Error().
 					Str("var_value", value).
@@ -159,26 +158,23 @@ func (a *setvarFn) evaluateTxCollection(r plugintypes.RuleMetadata, tx plugintyp
 				return
 			}
 		}
-		val := 0
-		if res != "" {
-			val, err = strconv.Atoi(res)
+		currentValInt := 0
+		if currentVal != "" {
+			currentValInt, err = strconv.Atoi(currentVal)
 			if err != nil {
 				tx.DebugLogger().Error().
-					Str("var_key", res).
+					Str("var_key", currentVal).
 					Int("rule_id", r.ID()).
 					Err(err).
 					Msg("Invalid value")
 				return
 			}
 		}
-		col.Set(key, []string{strconv.Itoa(sum + val)})
-	case value[0] == '-':
-		me, _ := strconv.Atoi(value[1:])
-		txv, err := strconv.Atoi(res)
-		if err != nil {
-			return
+		if value[0] == '+' {
+			col.Set(key, []string{strconv.Itoa(currentValInt + val)})
+		} else {
+			col.Set(key, []string{strconv.Itoa(currentValInt - val)})
 		}
-		col.Set(key, []string{strconv.Itoa(txv - me)})
 	default:
 		col.Set(key, []string{value})
 	}

diff --git a/internal/actions/setvar_test.go b/internal/actions/setvar_test.go
@@ -4,19 +4,26 @@
 package actions
 
 import (
+	"bytes"
+	"strings"
 	"testing"
+
+	"github.com/corazawaf/coraza/v3/collection"
+	"github.com/corazawaf/coraza/v3/debuglog"
+	"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
+	"github.com/corazawaf/coraza/v3/internal/corazawaf"
 )
 
 type md struct {
 }
 
-func (_ md) ID() int {
+func (md) ID() int {
 	return 0
 }
-func (_ md) ParentID() int {
+func (md) ParentID() int {
 	return 0
 }
-func (_ md) Status() int {
+func (md) Status() int {
 	return 0
 }
 
@@ -46,3 +53,115 @@ func TestSetvarInit(t *testing.T) {
 		}
 	})
 }
+
+var invalidSyntaxAtoiError = "invalid syntax"
+var warningKeyNotFoundInCollection = "key not found in collection"
+
+func TestSetvarEvaluate(t *testing.T) {
+	tests := []struct {
+		name                     string
+		init                     string
+		init2                    string
+		expectInvalidSyntaxError bool
+		expectNewVarValue        string
+	}{
+		{
+			name:                     "Numerical operation + with existing variable",
+			init:                     "TX.var=5",
+			init2:                    "TX.newvar=+%{tx.var}",
+			expectInvalidSyntaxError: false,
+			expectNewVarValue:        "5",
+		},
+		{
+			name:                     "Numerical operation - with existing variable",
+			init:                     "TX.var=5",
+			init2:                    "TX.newvar=-%{tx.var}",
+			expectInvalidSyntaxError: false,
+			expectNewVarValue:        "-5",
+		},
+		{
+			name:                     "Numerical operation - with existing negative variable",
+			init:                     "TX.newvar=-5",
+			init2:                    "TX.newvar=+5",
+			expectInvalidSyntaxError: false,
+			expectNewVarValue:        "0",
+		},
+		{
+			name:                     "Numerical operation + with missing (or non-numerical) variable",
+			init:                     "TX.newvar=+%{tx.missingvar}",
+			expectInvalidSyntaxError: true,
+		},
+		{
+			name:                     "Numerical operation - with missing (or non-numerical) variable",
+			init:                     "TX.newvar=-%{tx.missingvar}",
+			expectInvalidSyntaxError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		logsBuf := &bytes.Buffer{}
+
+		logger := debuglog.Default().WithLevel(debuglog.LevelWarn).WithOutput(logsBuf)
+
+		t.Run(tt.name, func(t *testing.T) {
+			defer logsBuf.Reset()
+			a := setvar()
+			metadata := &md{}
+			if err := a.Init(metadata, tt.init); err != nil {
+				t.Error("unexpected error during setvar init")
+			}
+			waf := corazawaf.NewWAF()
+			waf.Logger = logger
+			tx := waf.NewTransaction()
+			a.Evaluate(metadata, tx)
+			if tt.expectInvalidSyntaxError {
+				if logsBuf.Len() == 0 {
+					t.Fatal("expected error")
+				}
+				if !strings.Contains(logsBuf.String(), invalidSyntaxAtoiError) {
+					t.Errorf("expected error containing %q, got %q", invalidSyntaxAtoiError, logsBuf.String())
+				}
+				if !strings.Contains(logsBuf.String(), warningKeyNotFoundInCollection) {
+					t.Errorf("expected error containing %q, got %q", warningKeyNotFoundInCollection, logsBuf.String())
+				}
+			}
+			if logsBuf.Len() != 0 && !tt.expectInvalidSyntaxError {
+				t.Fatalf("unexpected error: %s", logsBuf.String())
+			}
+
+			if tt.init2 != "" {
+				if err := a.Init(metadata, tt.init2); err != nil {
+					t.Fatal("unexpected error during setvar init")
+				}
+				a.Evaluate(metadata, tx)
+				if logsBuf.Len() != 0 && !tt.expectInvalidSyntaxError {
+					t.Fatalf("unexpected error: %s", logsBuf.String())
+				}
+			}
+			if tt.expectNewVarValue != "" {
+				checkCollectionValue(t, a.(*setvarFn), tx, "newvar", tt.expectNewVarValue)
+			}
+		})
+	}
+}
+
+func checkCollectionValue(t *testing.T, a *setvarFn, tx plugintypes.TransactionState, key string, expected string) {
+	t.Helper()
+	var col collection.Map
+	if c, ok := tx.Collection(a.collection).(collection.Map); !ok {
+		t.Fatal("collection in setvar is not a map")
+		return
+	} else {
+		col = c
+	}
+	if col == nil {
+		t.Fatal("collection in setvar is nil")
+		return
+	}
+	if col == nil {
+		t.Fatal("collection is nil")
+	}
+	if col.Get(key)[0] != expected {
+		t.Errorf("key %q: expected %q, got %q", key, expected, col.Get(key))
+	}
+}
diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go
@@ -342,7 +342,7 @@ func (r *Rule) doEvaluate(phase types.RulePhase, tx *Transaction, collectiveMatc
 		}
 
 		// Expansion of Msg and LogData is postponed here. It allows to run it only if the whole rule/chain
-		// matches and to rely on MATCHED_* variables updated by the chain, not just by the fist rule.
+		// matches and to rely on MATCHED_* variables updated by the chain, not just by the first rule.
 		if !r.MultiMatch {
 			if r.Msg != nil {
 				matchedValues[0].(*corazarules.MatchData).Message_ = r.Msg.Expand(tx)

diff --git a/internal/corazawaf/rule_test.go b/internal/corazawaf/rule_test.go
@@ -95,7 +95,8 @@ func (*dummyFlowAction) Init(_ plugintypes.RuleMetadata, _ string) error {
 }
 
 func (*dummyFlowAction) Evaluate(_ plugintypes.RuleMetadata, tx plugintypes.TransactionState) {
-	tx.(*Transaction).Logdata = "flow action triggered"
+	// SkipAfter is used in a improper way, just for testing purposes ensuring that the action has been enforced
+	tx.(*Transaction).SkipAfter = "flow action triggered"
 }
 
 func (*dummyFlowAction) Type() plugintypes.ActionType {
@@ -116,7 +117,7 @@ func TestFlowActionIfDetectionOnlyEngine(t *testing.T) {
 	if len(matchdata) != 1 {
 		t.Errorf("Expected 1 matchdata, got %d", len(matchdata))
 	}
-	if tx.Logdata != "flow action triggered" {
+	if tx.SkipAfter != "flow action triggered" {
 		t.Errorf("Expected flow action triggered with DetectionOnly engine")
 	}
 }
@@ -128,7 +129,7 @@ func (*dummyNonDisruptiveAction) Init(_ plugintypes.RuleMetadata, _ string) erro
 }
 
 func (*dummyNonDisruptiveAction) Evaluate(_ plugintypes.RuleMetadata, tx plugintypes.TransactionState) {
-	tx.(*Transaction).Logdata = "action enforced"
+	tx.(*Transaction).SkipAfter = "action enforced"
 }
 
 func (*dummyNonDisruptiveAction) Type() plugintypes.ActionType {
@@ -142,7 +143,7 @@ func TestMatchVariableRunsActionTypeNondisruptive(t *testing.T) {
 	action := &dummyNonDisruptiveAction{}
 	_ = rule.AddAction("dummyNonDisruptiveAction", action)
 	rule.matchVariable(tx, md)
-	if tx.Logdata != "action enforced" {
+	if tx.SkipAfter != "action enforced" {
 		t.Errorf("Expected non disruptive action to be enforced during matchVariable")
 	}
 }

diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go
@@ -48,6 +48,7 @@ type Transaction struct {
 	interruption *types.Interruption
 
 	// This is used to store log messages
+	// Deprecated since Coraza 3.0.5: this variable is not used, logdata values are stored in the matched rules
 	Logdata string
 
 	// Rules will be skipped after a rule with this SecMarker is found

diff --git a/internal/corazawaf/waf.go b/internal/corazawaf/waf.go
@@ -153,7 +153,7 @@ func (w *WAF) newTransactionWithID(id string) *Transaction {
 	tx.id = id
 	tx.matchedRules = []types.MatchedRule{}
 	tx.interruption = nil
-	tx.Logdata = ""
+	tx.Logdata = "" // Deprecated, this variable is not used. Logdata for each matched rule is stored in the MatchData field.
 	tx.SkipAfter = ""
 	tx.AuditEngine = w.AuditEngine
 	tx.AuditLogParts = w.AuditLogParts

diff --git a/testing/coreruleset/go.mod b/testing/coreruleset/go.mod
@@ -35,9 +35,9 @@ require (
 	github.com/tidwall/match v1.1.1 // indirect
 	github.com/tidwall/pretty v1.2.1 // indirect
 	github.com/yargevad/filepathx v1.0.0 // indirect
-	golang.org/x/crypto v0.12.0 // indirect
-	golang.org/x/net v0.14.0 // indirect
-	golang.org/x/sys v0.11.0 // indirect
+	golang.org/x/crypto v0.14.0 // indirect
+	golang.org/x/net v0.17.0 // indirect
+	golang.org/x/sys v0.13.0 // indirect
 	golang.org/x/tools v0.6.0 // indirect
 	golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect

diff --git a/testing/coreruleset/go.sum b/testing/coreruleset/go.sum
@@ -314,8 +314,8 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk
 golang.org/x/crypto v0.0.0-20190923035154-9ee001bba392/go.mod h1:/lpIB1dKB+9EgE3H3cr1v9wB50oz8l4C4h62xy7jSTY=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
-golang.org/x/crypto v0.12.0 h1:tFM/ta59kqch6LlvYnPa0yx5a83cL2nHflFhYKvv9Yk=
-golang.org/x/crypto v0.12.0/go.mod h1:NF0Gs7EO5K4qLn+Ylc+fih8BSTeIjAP05siRnAh98yw=
+golang.org/x/crypto v0.14.0 h1:wBqGXzWJW6m1XrIKlAH0Hs1JJ7+9KBwnIO8v66Q9cHc=
+golang.org/x/crypto v0.14.0/go.mod h1:MVFd36DqK4CsrnJYDkBA3VC4m2GkXAM0PvzMCn4JQf4=
 golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
 golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
 golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU=
@@ -343,8 +343,8 @@ golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwY
 golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg=
 golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
 golang.org/x/net v0.0.0-20210410081132-afb366fc7cd1/go.mod h1:9tjilg8BloeKEkVJvy7fQ90B1CfIiPueXVOjqfkSzI8=
-golang.org/x/net v0.14.0 h1:BONx9s002vGdD9umnlX1Po8vOZmrgH34qlHcD1MfK14=
-golang.org/x/net v0.14.0/go.mod h1:PpSgVXXLK0OxS0F31C1/tv6XNguvCrnXIDrFMspZIUI=
+golang.org/x/net v0.17.0 h1:pVaXccu2ozPjCXewfr1S7xza/zcXTity9cCdXQYSjIM=
+golang.org/x/net v0.17.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE=
 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
 golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
@@ -391,8 +391,8 @@ golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.11.0 h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM=
-golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE=
+golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.1-0.20181227161524-e6919f6577db/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
-Original file line number
+Diff line change
@@ Expand Up @@
     		}
     	}
+    	// If the variable is known (e.g. TX) but the key is not found, we return the original text
+    	tx.DebugLogger().Warn().Str("variable", token.variable.Name()).Str("key", token.key).Msg("key not found in collection, returning the original text")
     	return token.text
     }
@@ Expand Down @@