Repository: corazawaf/coraza
Pull request #703 (2023-03-09 02:36:48)

Small cleanups to actions parsing
We don't do any unicode handling in the parse so we can loop on bytes without utf8 decoding.

Also don't call `Builder.String()` multiple times to simplify the code (no affect on allocations as that method doesn't allocate, just cleaner IMO)

After
```
goos: darwin
goarch: arm64
pkg: github.com/corazawaf/coraza/v3/internal/seclang
BenchmarkParseActions
BenchmarkParseActions-10    	 1255496	       932.3 ns/op
PASS
```

Before
```
goos: darwin
goarch: arm64
pkg: github.com/corazawaf/coraza/v3/internal/seclang
BenchmarkParseActions
BenchmarkParseActions-10    	 1042354	      1104 ns/op
PASS
```

codecov[bot]: # [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/703?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 no project coverage change
> Comparison is base [(`22b7402`)](https://codecov.io/gh/corazawaf/coraza/commit/22b740260d09867255cb975f8f5106243df7c740?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.24% compared to head [(`964f250`)](https://codecov.io/gh/corazawaf/coraza/pull/703?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.24%.

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


```diff
@@           Coverage Diff           @@
##           v3/dev     #703   +/-   ##
=======================================
  Coverage   81.24%   81.24%           
=======================================
  Files         146      146           
  Lines        8173     8174    +1     
=======================================
+ Hits         6640     6641    +1     
  Misses       1307     1307           
  Partials      226      226           
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `77.24% <100.00%> (+<0.01%)` | :arrow_up: |
| examples | `26.81% <100.00%> (+<0.01%)` | :arrow_up: |
| ftw | `49.85% <83.33%> (+<0.01%)` | :arrow_up: |
| ftw-multiphase | `49.94% <83.33%> (+<0.01%)` | :arrow_up: |
| tinygo | `76.41% <100.00%> (-0.04%)` | :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/703?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [internal/seclang/rule\_parser.go](https://codecov.io/gh/corazawaf/coraza/pull/703?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9ydWxlX3BhcnNlci5nbw==) | `83.16% <100.00%> (+0.04%)` | :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/703?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: cc @manojgop @ns-sundar

Patch:
diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go
index 27f898a..050ba45 100644
--- a/internal/corazawaf/rule.go
+++ b/internal/corazawaf/rule.go
@@ -614,8 +614,6 @@ func minPhase(v variables.RuleVariable) types.RulePhase {
 		return types.PhaseUnknown
 	case variables.StatusLine:
 		return types.PhaseResponseHeaders
-	case variables.InboundErrorData:
-		return types.PhaseRequestBody
 	case variables.Duration:
 		// If used in matching, would need to be defined for multiple inferredPhases to make sense
 		return types.PhaseUnknown
diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go
index a88f32e..00ab7cb 100644
--- a/internal/corazawaf/transaction.go
+++ b/internal/corazawaf/transaction.go
@@ -200,8 +200,6 @@ func (tx *Transaction) Collection(idx variables.RuleVariable) collection.Collect
 		return tx.variables.highestSeverity
 	case variables.StatusLine:
 		return tx.variables.statusLine
-	case variables.InboundErrorData:
-		return tx.variables.inboundErrorData
 	case variables.Duration:
 		return tx.variables.duration
 	case variables.ResponseHeadersNames:
@@ -790,11 +788,11 @@ func (tx *Transaction) WriteRequestBody(b []byte) (*types.Interruption, int, err
 	if tx.requestBodyBuffer.length >= (math.MaxInt64 - writingBytes) {
 		// Overflow, failing. MaxInt64 is not a realistic payload size. Furthermore, it has been tested that
 		// bytes.Buffer does not work with this kind of sizes. See comments in BodyBuffer Write(data []byte)
-		return nil, 0, errors.New("Overflow reached while writing request body")
+		return nil, 0, errors.New("overflow reached while writing request body")
 	}
 
 	if tx.requestBodyBuffer.length+writingBytes >= tx.RequestBodyLimit {
-		tx.variables.inboundErrorData.Set("1")
+		tx.variables.inboundDataError.Set("1")
 		if tx.WAF.RequestBodyLimitAction == types.BodyLimitActionReject {
 			// We interrupt this transaction in case RequestBodyLimitAction is Reject
 			return setAndReturnBodyLimitInterruption(tx)
@@ -860,7 +858,7 @@ func (tx *Transaction) ReadRequestBodyFrom(r io.Reader) (*types.Interruption, in
 			return nil, 0, errors.New("overflow reached while writing request body")
 		}
 		if tx.requestBodyBuffer.length+writingBytes >= tx.RequestBodyLimit {
-			tx.variables.inboundErrorData.Set("1")
+			tx.variables.inboundDataError.Set("1")
 			if tx.WAF.RequestBodyLimitAction == types.BodyLimitActionReject {
 				return setAndReturnBodyLimitInterruption(tx)
 			}
@@ -1476,7 +1474,6 @@ type TransactionVariables struct {
 	geo                      *collections.Map
 	highestSeverity          *collections.Single
 	inboundDataError         *collections.Single
-	inboundErrorData         *collections.Single
 	matchedVar               *collections.Single
 	matchedVarName           *collections.Single
 	matchedVars              *collections.NamedCollection
@@ -1574,7 +1571,6 @@ func NewTransactionVariables() *TransactionVariables {
 	v.serverPort = collections.NewSingle(variables.ServerPort)
 	v.highestSeverity = collections.NewSingle(variables.HighestSeverity)
 	v.statusLine = collections.NewSingle(variables.StatusLine)
-	v.inboundErrorData = collections.NewSingle(variables.InboundErrorData)
 	v.duration = collections.NewSingle(variables.Duration)
 	v.resBodyError = collections.NewSingle(variables.ResBodyError)
 	v.resBodyErrorMsg = collections.NewSingle(variables.ResBodyErrorMsg)
@@ -1786,10 +1782,6 @@ func (v *TransactionVariables) StatusLine() collection.Single {
 	return v.statusLine
 }
 
-func (v *TransactionVariables) InboundErrorData() collection.Single {
-	return v.inboundErrorData
-}
-
 func (v *TransactionVariables) Env() collection.Map {
 	return v.env
 }
@@ -1999,9 +1991,6 @@ func (v *TransactionVariables) All(f func(v variables.RuleVariable, col collecti
 	if !f(variables.InboundDataError, v.inboundDataError) {
 		return
 	}
-	if !f(variables.InboundErrorData, v.inboundErrorData) {
-		return
-	}
 	if !f(variables.MatchedVar, v.matchedVar) {
 		return
 	}
diff --git a/internal/seclang/rule_parser.go b/internal/seclang/rule_parser.go
index 7790cba..d956456 100644
--- a/internal/seclang/rule_parser.go
+++ b/internal/seclang/rule_parser.go
@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
-	"regexp"
 	"strings"
 
 	actionsmod "github.com/corazawaf/coraza/v3/actions"
@@ -303,10 +302,6 @@ type RuleOptions struct {
 	Data         string
 }
 
-// ruleTokenRegex splits the sections operator and actions.
-// e.g. &REQUEST_COOKIES_NAMES:'/^(?:phpMyAdminphp|MyAdmin_https)$/'|ARGS:test "id:3" => "id:3"
-var ruleTokenRegex = regexp.MustCompile(`"(?:[^"\\]|\\.)*"`)
-
 // ParseRule parses a rule from a string
 // The string must match the seclang format
 // In case WithOperator is false, the rule will be parsed without operator
@@ -337,27 +332,21 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 	actions := ""
 
 	if options.WithOperator {
-		matches := ruleTokenRegex.FindAllString(options.Data, 3) // we use at most second match
-		if len(matches) == 0 {
-			return nil, fmt.Errorf("invalid rule with no transformation matches: %q", options.Data)
+		vars, operator, acts, err := parseActionOperator(options.Data)
+		if err != nil {
+			return nil, err
 		}
-		operator := utils.MaybeRemoveQuotes(matches[0])
 		if utils.InSlice(operator, disabledRuleOperators) {
 			return nil, fmt.Errorf("%s rule operator is disabled", operator)
 		}
-		vars, _, _ := strings.Cut(options.Data, " ")
-		err = rp.ParseVariables(vars)
-		if err != nil {
+		if err := rp.ParseVariables(vars); err != nil {
 			return nil, err
 		}
-		err = rp.ParseOperator(operator)
-		if err != nil {
+		if err := rp.ParseOperator(operator); err != nil {
 			return nil, err
 		}
-		if len(matches) > 1 {
-			actions = utils.MaybeRemoveQuotes(matches[1])
-			err = rp.ParseActions(actions)
-			if err != nil {
+		if acts != "" {
+			if err := rp.ParseActions(acts); err != nil {
 				return nil, err
 			}
 		}
@@ -388,6 +377,60 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 	return rule, nil
 }
 
+func parseActionOperator(data string) (vars string, op string, actions string, err error) {
+	// So only need to TrimLeft below
+	data = strings.Trim(data, " ")
+	vars, rest, ok := strings.Cut(data, " ")
+	if !ok {
+		return "", "", "", fmt.Errorf("invalid format for rule with operator: %q", data)
+	}
+
+	rest = strings.TrimLeft(rest, " ")
+
+	if len(rest) == 0 || rest[0] != '"' {
+		return "", "", "", fmt.Errorf("invalid operator for rule with operator: %q", data)
+	}
+
+	op, rest, err = cutQuotedString(rest)
+	if err != nil {
+		return
+	}
+	op = utils.MaybeRemoveQuotes(op)
+
+	rest = strings.TrimLeft(rest, " ")
+	if len(rest) == 0 {
+		// No actions
+		return
+	}
+
+	if len(rest) < 2 || rest[0] != '"' || rest[len(rest)-1] != '"' {
+		return "", "", "", fmt.Errorf("invalid actions for rule with operator: %q", data)
+	}
+	actions = utils.MaybeRemoveQuotes(rest)
+
+	return
+}
+
+func cutQuotedString(s string) (string, string, error) {
+	if len(s) == 0 || s[0] != '"' {
+		return "", "", fmt.Errorf("expected quoted string: %q", s)
+	}
+
+	for i := 1; i < len(s); i++ {
+		// Search until first quote that isn't part of an escape sequence.
+		if s[i] != '"' {
+			continue
+		}
+		if s[i-1] == '\\' {
+			continue
+		}
+
+		return s[:i+1], s[i+1:], nil
+	}
+
+	return "", "", fmt.Errorf("expected terminating quote: %q", s)
+}
+
 func getLastRuleExpectingChain(w *corazawaf.WAF) *corazawaf.Rule {
 	rules := w.Rules.GetRules()
 	if len(rules) == 0 {
@@ -412,28 +455,29 @@ const unset = -1
 // Action arguments are allowed to wrap values between colons(”)
 func parseActions(actions string) ([]ruleAction, error) {
 	iskey := true
-	var ckey strings.Builder
-	var cval strings.Builder
-	ckey.Reset()
-	cval.Reset()
+	var key strings.Builder
+	var val strings.Builder
+	key.Reset()
+	val.Reset()
 
 	quoted := false
 	var res []ruleAction
 	var err error
 	disruptiveActionIndex := unset
 actionLoop:
-	for i, c := range actions {
+	for i := 0; i < len(actions); i++ {
+		c := actions[i]
 		switch {
 		case iskey && c == ' ':
 			// skip whitespaces in key
 			continue actionLoop
 		case !quoted && c == ',':
-			res, disruptiveActionIndex, err = appendRuleAction(res, &ckey, &cval, disruptiveActionIndex)
+			res, disruptiveActionIndex, err = appendRuleAction(res, key.String(), val.String(), disruptiveActionIndex)
 			if err != nil {
 				return nil, err
 			}
-			ckey.Reset()
-			cval.Reset()
+			key.Reset()
+			val.Reset()
 			iskey = true
 		case iskey && c == ':':
 			iskey = false
@@ -449,13 +493,13 @@ actionLoop:
 				// skip unquoted whitespaces
 				continue actionLoop
 			}
-			cval.WriteRune(c)
+			val.WriteByte(c)
 		case iskey:
-			ckey.WriteRune(c)
+			key.WriteByte(c)
 		}
 		if i+1 == len(actions) {
 			// last action, returned disruptiveActionIndex is not needed
-			res, _, err = appendRuleAction(res, &ckey, &cval, disruptiveActionIndex)
+			res, _, err = appendRuleAction(res, key.String(), val.String(), disruptiveActionIndex)
 			if err != nil {
 				return nil, err
 			}
@@ -464,8 +508,8 @@ actionLoop:
 	return res, nil
 }
 
-func appendRuleAction(res []ruleAction, ckey *strings.Builder, cval *strings.Builder, disruptiveActionIndex int) ([]ruleAction, int, error) {
-	f, err := actionsmod.Get(ckey.String())
+func appendRuleAction(res []ruleAction, key string, val string, disruptiveActionIndex int) ([]ruleAction, int, error) {
+	f, err := actionsmod.Get(key)
 	if err != nil {
 		return res, unset, err
 	}
@@ -474,8 +518,8 @@ func appendRuleAction(res []ruleAction, ckey *strings.Builder, cval *strings.Bui
 		// actions present, or inherited, only the last one will take effect).
 		// Therefore, if we encounter another disruptive action, we replace the previous one.
 		res[disruptiveActionIndex] = ruleAction{
-			Key:   ckey.String(),
-			Value: cval.String(),
+			Key:   key,
+			Value: val,
 			F:     f,
 			Atype: f.Type(),
 		}
@@ -484,8 +528,8 @@ func appendRuleAction(res []ruleAction, ckey *strings.Builder, cval *strings.Bui
 			disruptiveActionIndex = len(res)
 		}
 		res = append(res, ruleAction{
-			Key:   ckey.String(),
-			Value: cval.String(),
+			Key:   key,
+			Value: val,
 			F:     f,
 			Atype: f.Type(),
 		})
diff --git a/internal/variables/variables.go b/internal/variables/variables.go
index b737944..717b18f 100644
--- a/internal/variables/variables.go
+++ b/internal/variables/variables.go
@@ -28,7 +28,8 @@ const (
 	FilesCombinedSize
 	// FullRequestLength is the length of the full request
 	FullRequestLength
-	// InboundDataError represents errors for inbound data
+	// InboundDataError will be set to 1 when the request body size
+	// is above the setting configured by SecRequesteBodyLimit
 	InboundDataError
 	// MatchedVar is the value of the matched variable
 	MatchedVar
@@ -107,9 +108,6 @@ const (
 	// StatusLine is the status line of the response, including the request method
 	// and HTTP version information
 	StatusLine
-	// InboundErrorData will be set to 1 when the request body size
-	// is above the setting configured by SecRequesteBodyLimit
-	InboundErrorData
 	// Duration contains the time in miliseconds from
 	// the beginning of the transaction until this point
 	Duration
diff --git a/internal/variables/variablesmap.gen.go b/internal/variables/variablesmap.gen.go
index c7291b8..8b707ac 100644
--- a/internal/variables/variablesmap.gen.go
+++ b/internal/variables/variablesmap.gen.go
@@ -90,8 +90,6 @@ func (v RuleVariable) Name() string {
 		return "HIGHEST_SEVERITY"
 	case StatusLine:
 		return "STATUS_LINE"
-	case InboundErrorData:
-		return "INBOUND_ERROR_DATA"
 	case Duration:
 		return "DURATION"
 	case ResponseHeadersNames:
@@ -253,7 +251,6 @@ var rulemapRev = map[string]RuleVariable{
 	"SERVER_PORT":                      ServerPort,
 	"HIGHEST_SEVERITY":                 HighestSeverity,
 	"STATUS_LINE":                      StatusLine,
-	"INBOUND_ERROR_DATA":               InboundErrorData,
 	"DURATION":                         Duration,
 	"RESPONSE_HEADERS_NAMES":           ResponseHeadersNames,
 	"REQUEST_HEADERS_NAMES":            RequestHeadersNames,
diff --git a/rules/transaction.go b/rules/transaction.go
index 3b0c245..88b7029 100644
--- a/rules/transaction.go
+++ b/rules/transaction.go
@@ -94,7 +94,6 @@ type TransactionVariables interface {
 	ServerPort() collection.Single
 	HighestSeverity() collection.Single
 	StatusLine() collection.Single
-	InboundErrorData() collection.Single
 	Env() collection.Map
 	TX() collection.Map
 	Rule() collection.Map
diff --git a/types/variables/variables.go b/types/variables/variables.go
index 0678e2e..c8363fa 100644
--- a/types/variables/variables.go
+++ b/types/variables/variables.go
@@ -28,7 +28,8 @@ const (
 	FilesCombinedSize = variables.FilesCombinedSize
 	// FullRequestLength is the length of the full request
 	FullRequestLength = variables.FullRequestLength
-	// InboundDataError represents errors for inbound data
+	// InboundDataError will be set to 1 when the request body size
+	// is above the setting configured by SecRequesteBodyLimit
 	InboundDataError = variables.InboundDataError
 	// MatchedVar is the value of the matched variable
 	MatchedVar = variables.MatchedVar
@@ -109,9 +110,6 @@ const (
 	// StatusLine is the status line of the response, including the request method
 	// and HTTP version information
 	StatusLine = variables.StatusLine
-	// InboundErrorData will be set to 1 when the request body size
-	// is above the setting configured by SecRequesteBodyLimit
-	InboundErrorData = variables.InboundErrorData
 	// Duration contains the time in miliseconds from
 	// the beginning of the transaction until this point
 	Duration = variables.Duration


Test patch:
diff --git a/internal/seclang/rule_parser_test.go b/internal/seclang/rule_parser_test.go
index fcba3ee..4761fc2 100644
--- a/internal/seclang/rule_parser_test.go
+++ b/internal/seclang/rule_parser_test.go
@@ -146,6 +146,27 @@ func TestDefaultActionsForPhase2(t *testing.T) {
 		t.Error("phase 1 rules shouldn't have log set by default actions")
 	}
 }
+
+func TestInvalidOperatorRuleData(t *testing.T) {
+	tests := []string{
+		`ARGS`,
+		`ARGS `,
+		`ARGS notquoted "deny"`,
+		`Args "op`,
+		`ARGS "op" notquoted`,
+		`Args "op" "`,
+		`Args "op" "deny`,
+	}
+	for _, tc := range tests {
+		tt := tc
+		t.Run(tt, func(t *testing.T) {
+			if _, _, _, err := parseActionOperator(tt); err == nil {
+				t.Error("expected error")
+			}
+		})
+	}
+}
+
 func BenchmarkParseActions(b *testing.B) {
 	actionsToBeParsed := "id:980170,phase:5,pass,t:none,noauditlog,msg:'Anomaly Scores:Inbound Scores - Outbound Scores',tag:test"
 	for i := 0; i < b.N; i++ {