Repository: corazawaf/coraza
Pull request #700 (2023-03-08 06:47:30)

Replace action/operator tokenization from regex to code
Regexes are pretty slow and use memory, forever in the case of package variables. While not terrible, it's good to avoid them where we can I think. In this case there was a combination of regex and cut being used, and an incorrect `3` instead of `2` passed to the regex making the logic even more confusing. Manually writing the code results in much more, but it seems worth it

codecov[bot]: # [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/700?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Patch coverage: **`60.41`**% and project coverage change: **`-0.12`** :warning:
> Comparison is base [(`504fdde`)](https://codecov.io/gh/corazawaf/coraza/commit/504fddef55e740ee8dad4fd324dd1f56b89878ae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 80.91% compared to head [(`7fd4142`)](https://codecov.io/gh/corazawaf/coraza/pull/700?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 80.79%.

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


```diff
@@            Coverage Diff             @@
##           v3/dev     #700      +/-   ##
==========================================
- Coverage   80.91%   80.79%   -0.12%     
==========================================
  Files         146      146              
  Lines        8021     8056      +35     
==========================================
+ Hits         6490     6509      +19     
- Misses       1305     1316      +11     
- Partials      226      231       +5     
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `76.81% <60.41%> (-0.07%)` | :arrow_down: |
| examples | `27.26% <43.75%> (+0.04%)` | :arrow_up: |
| ftw | `50.67% <56.25%> (+0.01%)` | :arrow_up: |
| ftw-multiphase | `50.76% <56.25%> (+0.01%)` | :arrow_up: |
| tinygo | `75.82% <60.41%> (-0.11%)` | :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/700?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/700?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9ydWxlX3BhcnNlci5nbw==) | `80.62% <60.41%> (-2.41%)` | :arrow_down: |

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/700?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: LGTM besides a couple of comments

Patch:
diff --git a/internal/corazawaf/rule.go b/internal/corazawaf/rule.go
index f2fc310..27f898a 100644
--- a/internal/corazawaf/rule.go
+++ b/internal/corazawaf/rule.go
@@ -25,9 +25,6 @@ type ruleActionParams struct {
 	// The name of the action, used for logging
 	Name string
 
-	// Parameters used by the action
-	Param string
-
 	// The action to be executed
 	Function rules.Action
 }
@@ -59,9 +56,6 @@ type ruleVariableException struct {
 // to get values from the transaction's variables
 // It supports xml, regex, exceptions and many more features
 type ruleVariableParams struct {
-	// We store the name for performance
-	Name string
-
 	// If true, the count of results will be returned
 	Count bool
 
@@ -81,9 +75,6 @@ type ruleVariableParams struct {
 }
 
 type ruleTransformationParams struct {
-	// The transformation to be used, used for logging
-	Name string
-
 	// The transformation function to be used
 	Function rules.Transformation
 }
@@ -399,7 +390,6 @@ func (r *Rule) AddVariable(v variables.RuleVariable, key string, iscount bool) e
 	}
 
 	r.variables = append(r.variables, ruleVariableParams{
-		Name:       v.Name(),
 		Count:      iscount,
 		Variable:   v,
 		KeyStr:     strings.ToLower(key),
@@ -462,7 +452,7 @@ func (r *Rule) AddTransformation(name string, t rules.Transformation) error {
 	if t == nil || name == "" {
 		return fmt.Errorf("invalid transformation %q not found", name)
 	}
-	r.transformations = append(r.transformations, ruleTransformationParams{name, t})
+	r.transformations = append(r.transformations, ruleTransformationParams{Function: t})
 	r.transformationsID = transformationID(r.transformationsID, name)
 	return nil
 }
diff --git a/internal/seclang/directives.go b/internal/seclang/directives.go
index fd062df..6b6c9b4 100644
--- a/internal/seclang/directives.go
+++ b/internal/seclang/directives.go
@@ -922,7 +922,7 @@ func directiveSecRuleUpdateTargetByID(options *DirectiveOptions) error {
 		return err
 	}
 	rule := options.WAF.Rules.FindByID(id)
-	rp := &RuleParser{
+	rp := RuleParser{
 		rule:           rule,
 		options:        RuleOptions{},
 		defaultActions: map[types.RulePhase][]ruleAction{},
diff --git a/internal/seclang/rule_parser.go b/internal/seclang/rule_parser.go
index ac0e84f..f02f463 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"
@@ -172,9 +171,13 @@ func (p *RuleParser) ParseOperator(operator string) error {
 		operator = "!@rx " + operator[1:]
 	}
 
+	// We clone strings to ensure a slice into larger rule definition isn't kept in
+	// memory just to store operator information.
 	opRaw, opdataRaw, _ := strings.Cut(operator, " ")
+	opRaw = strings.Clone(opRaw)
 	op := strings.TrimSpace(opRaw)
 	opdata := strings.TrimSpace(opdataRaw)
+	opdata = strings.Clone(opdata)
 
 	if op[0] == '@' {
 		// we trim @
@@ -299,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
@@ -313,7 +312,7 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 	}
 
 	var err error
-	rp := &RuleParser{
+	rp := RuleParser{
 		options:        options,
 		rule:           corazawaf.NewRule(),
 		defaultActions: map[types.RulePhase][]ruleAction{},
@@ -333,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
 			}
 		}
@@ -381,7 +374,61 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 		lastChain.Chain = rule
 		return nil, nil
 	}
-	return rp.rule, nil
+	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 {


Test patch:
diff --git a/internal/corazawaf/transaction_test.go b/internal/corazawaf/transaction_test.go
index a5e8dce..18921af 100644
--- a/internal/corazawaf/transaction_test.go
+++ b/internal/corazawaf/transaction_test.go
@@ -823,7 +823,6 @@ func TestProcessBodiesSkippedIfHeadersPhasesNotReached(t *testing.T) {
 func TestTxVariables(t *testing.T) {
 	tx := makeTransaction(t)
 	rv := ruleVariableParams{
-		Name:     "REQUEST_HEADERS",
 		Variable: variables.RequestHeaders,
 		KeyStr:   "ho.*",
 		KeyRx:    regexp.MustCompile("ho.*"),
@@ -860,7 +859,6 @@ func TestTxVariables(t *testing.T) {
 func TestTxVariablesExceptions(t *testing.T) {
 	tx := makeTransaction(t)
 	rv := ruleVariableParams{
-		Name:     "REQUEST_HEADERS",
 		Variable: variables.RequestHeaders,
 		KeyStr:   "ho.*",
 		KeyRx:    regexp.MustCompile("ho.*"),
@@ -1050,7 +1048,6 @@ func TestTxAddArgument(t *testing.T) {
 func TestTxGetField(t *testing.T) {
 	tx := makeTransaction(t)
 	rvp := ruleVariableParams{
-		Name:     "args",
 		Variable: variables.Args,
 	}
 	if f := tx.GetField(rvp); len(f) != 3 {
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++ {