Repository: corazawaf/coraza
Pull request #721 (2023-03-14 02:55:35)

Replace remaining usage of generic config map with internal struct

codecov[bot]: ## [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/721?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: **`+0.05`** :tada:
> Comparison is base [(`1febd65`)](https://codecov.io/gh/corazawaf/coraza/commit/1febd65d2f160e87e7b7955ba2e82a06333cf1d6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.49% compared to head [(`c47f0e4`)](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.54%.

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


```diff
@@            Coverage Diff             @@
##           v3/dev     #721      +/-   ##
==========================================
+ Coverage   81.49%   81.54%   +0.05%     
==========================================
  Files         145      148       +3     
  Lines        8261     8257       -4     
==========================================
+ Hits         6732     6733       +1     
+ Misses       1302     1298       -4     
+ Partials      227      226       -1     
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `77.55% <100.00%> (+0.05%)` | :arrow_up: |
| examples | `26.90% <54.54%> (-0.08%)` | :arrow_down: |
| ftw | `49.77% <78.78%> (-0.04%)` | :arrow_down: |
| ftw-multiphase | `49.86% <78.78%> (-0.04%)` | :arrow_down: |
| tinygo | `76.79% <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/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [actions/ctl.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-YWN0aW9ucy9jdGwuZ28=) | `95.84% <ø> (+1.50%)` | :arrow_up: |
| [types/waf.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-dHlwZXMvd2FmLmdv) | `96.29% <ø> (-0.77%)` | :arrow_down: |
| [internal/seclang/directives.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9kaXJlY3RpdmVzLmdv) | `70.85% <100.00%> (-0.07%)` | :arrow_down: |
| [internal/seclang/parser.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9wYXJzZXIuZ28=) | `92.36% <100.00%> (-0.06%)` | :arrow_down: |
| [internal/seclang/rule\_parser.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9ydWxlX3BhcnNlci5nbw==) | `84.14% <100.00%> (+0.03%)` | :arrow_up: |
| [transformations/transformations.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-dHJhbnNmb3JtYXRpb25zL3RyYW5zZm9ybWF0aW9ucy5nbw==) | `100.00% <100.00%> (ø)` | |
| [transformations/trim.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-dHJhbnNmb3JtYXRpb25zL3RyaW0uZ28=) | `100.00% <100.00%> (ø)` | |
| [transformations/trim\_left.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-dHJhbnNmb3JtYXRpb25zL3RyaW1fbGVmdC5nbw==) | `100.00% <100.00%> (ø)` | |
| [transformations/trim\_right.go](https://codecov.io/gh/corazawaf/coraza/pull/721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-dHJhbnNmb3JtYXRpb25zL3RyaW1fcmlnaHQuZ28=) | `100.00% <100.00%> (ø)` | |

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/721?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/bodyprocessors/json.go b/bodyprocessors/json.go
index b084e4c..f4326f4 100644
--- a/bodyprocessors/json.go
+++ b/bodyprocessors/json.go
@@ -23,10 +23,7 @@ func (js *jsonBodyProcessor) ProcessRequest(reader io.Reader, v rules.Transactio
 	if err != nil {
 		return err
 	}
-	argsGetCol := v.ArgsGet()
 	for key, value := range data {
-		// TODO: This hack prevent GET variables from overriding POST variables
-		argsGetCol.Remove(key)
 		col.SetIndex(key, 0, value)
 	}
 	return nil
diff --git a/internal/seclang/directives.go b/internal/seclang/directives.go
index 2ba18d9..50d3fca 100644
--- a/internal/seclang/directives.go
+++ b/internal/seclang/directives.go
@@ -25,7 +25,6 @@ import (
 // TODO(anuraaga): Propagation of config probably should be separated from a directive's options.
 type DirectiveOptions struct {
 	WAF      *corazawaf.WAF
-	Config   types.Config
 	Opts     string
 	Path     []string
 	Datasets map[string][]string
@@ -33,6 +32,10 @@ type DirectiveOptions struct {
 	// AuditLog is configuration of audit logging, populated by multiple directives and consumed by
 	// SecAuditLog.
 	AuditLog auditlog.Config
+
+	// Parser is configuration of the parser, populated by multiple directives and consumed by
+	// directives that parse.
+	Parser ParserConfig
 }
 
 type directive = func(options *DirectiveOptions) error
@@ -109,8 +112,8 @@ func directiveSecMarker(options *DirectiveOptions) error {
 	rule.SecMark_ = options.Opts
 	rule.ID_ = 0
 	rule.Phase_ = 0
-	rule.Line_ = options.Config.Get("parser_last_line", 0).(int)
-	rule.File_ = options.Config.Get("parser_config_file", "").(string)
+	rule.Line_ = options.Parser.LastLine
+	rule.File_ = options.Parser.ConfigFile
 	if err := options.WAF.Rules.Add(rule); err != nil {
 		return err
 	}
@@ -136,7 +139,7 @@ func directiveSecAction(options *DirectiveOptions) error {
 	rule, err := ParseRule(RuleOptions{
 		WithOperator: false,
 		WAF:          options.WAF,
-		Config:       options.Config,
+		ParserConfig: options.Parser,
 		Directive:    "SecAction",
 		Data:         options.Opts,
 	})
@@ -172,11 +175,11 @@ func directiveSecRule(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	ignoreErrors := options.Config.Get("ignore_rule_compilation_errors", false).(bool)
+	ignoreErrors := options.Parser.IgnoreRuleCompilationErrors
 	rule, err := ParseRule(RuleOptions{
 		WithOperator: true,
 		WAF:          options.WAF,
-		Config:       options.Config,
+		ParserConfig: options.Parser,
 		Directive:    "SecRule",
 		Data:         options.Opts,
 	})
@@ -532,9 +535,8 @@ func directiveSecDefaultAction(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	da, _ := options.Config.Get("rule_default_actions", []string{}).([]string)
-	da = append(da, options.Opts)
-	options.Config.Set("rule_default_actions", da)
+	options.Parser.RuleDefaultActions = append(options.Parser.RuleDefaultActions, options.Opts)
+	options.Parser.HasRuleDefaultActions = true
 	return nil
 }
 
@@ -946,7 +948,7 @@ func directiveSecIgnoreRuleCompilationErrors(options *DirectiveOptions) error {
 			Msg(`Running in Compatibility Mode (SecIgnoreRuleCompilationErrors On), 
 			which may cause unexpected behavior on faulty rules.`)
 	}
-	options.Config.Set("ignore_rule_compilation_errors", b)
+	options.Parser.IgnoreRuleCompilationErrors = b
 	return nil
 }
 
diff --git a/internal/seclang/parser.go b/internal/seclang/parser.go
index 4e83064..79f91e4 100644
--- a/internal/seclang/parser.go
+++ b/internal/seclang/parser.go
@@ -16,7 +16,6 @@ import (
 	"github.com/corazawaf/coraza/v3/internal/corazawaf"
 	"github.com/corazawaf/coraza/v3/internal/environment"
 	"github.com/corazawaf/coraza/v3/internal/io"
-	"github.com/corazawaf/coraza/v3/types"
 )
 
 // maxIncludeRecursion is used to avoid DDOS by including files that include
@@ -165,16 +164,16 @@ func (p *Parser) evaluateLine(l string) error {
 	}
 
 	p.options.Opts = opts
-	p.options.Config.Set("last_profile_line", p.currentLine)
-	p.options.Config.Set("parser_config_file", p.currentFile)
-	p.options.Config.Set("parser_config_dir", p.currentDir)
-	p.options.Config.Set("parser_root", p.root)
+	p.options.Parser.LastLine = p.currentLine
+	p.options.Parser.ConfigFile = p.currentFile
+	p.options.Parser.ConfigDir = p.currentDir
+	p.options.Parser.Root = p.root
 	if environment.HasAccessToFS {
 		wd, err := os.Getwd()
 		if err != nil {
 			return err
 		}
-		p.options.Config.Set("working_dir", wd)
+		p.options.Parser.WorkingDir = wd
 	}
 
 	if err := d(p.options); err != nil {
@@ -206,7 +205,6 @@ func NewParser(waf *corazawaf.WAF) *Parser {
 	p := &Parser{
 		options: &DirectiveOptions{
 			WAF:      waf,
-			Config:   make(types.Config),
 			Datasets: make(map[string][]string),
 			AuditLog: auditlog.NewConfig(),
 		},
@@ -214,3 +212,16 @@ func NewParser(waf *corazawaf.WAF) *Parser {
 	}
 	return p
 }
+
+type ParserConfig struct {
+	DisabledRuleActions         []string
+	DisabledRuleOperators       []string
+	RuleDefaultActions          []string
+	HasRuleDefaultActions       bool
+	IgnoreRuleCompilationErrors bool
+	LastLine                    int
+	ConfigFile                  string
+	ConfigDir                   string
+	Root                        fs.FS
+	WorkingDir                  string
+}
diff --git a/internal/seclang/rule_parser.go b/internal/seclang/rule_parser.go
index bf98dc4..6fb483c 100644
--- a/internal/seclang/rule_parser.go
+++ b/internal/seclang/rule_parser.go
@@ -6,12 +6,10 @@ package seclang
 import (
 	"errors"
 	"fmt"
-	"io/fs"
 	"strings"
 
 	actionsmod "github.com/corazawaf/coraza/v3/actions"
 	"github.com/corazawaf/coraza/v3/internal/corazawaf"
-	"github.com/corazawaf/coraza/v3/internal/io"
 	utils "github.com/corazawaf/coraza/v3/internal/strings"
 	operators "github.com/corazawaf/coraza/v3/operators"
 	"github.com/corazawaf/coraza/v3/rules"
@@ -19,7 +17,7 @@ import (
 	"github.com/corazawaf/coraza/v3/types/variables"
 )
 
-const defaultActionsPhase2 = "phase:2,log,auditlog,pass"
+var defaultActionsPhase2 = []string{"phase:2,log,auditlog,pass"}
 
 type ruleAction struct {
 	Key   string
@@ -190,12 +188,12 @@ func (p *RuleParser) ParseOperator(operator string) error {
 	opts := rules.OperatorOptions{
 		Arguments: opdata,
 		Path: []string{
-			p.options.Config.Get("parser_config_dir", "").(string),
+			p.options.ParserConfig.ConfigDir,
 		},
-		Root: p.options.Config.Get("parser_root", io.OSFS{}).(fs.FS),
+		Root: p.options.ParserConfig.Root,
 	}
 
-	if wd := p.options.Config.Get("working_dir", "").(string); wd != "" {
+	if wd := p.options.ParserConfig.WorkingDir; wd != "" {
 		opts.Path = append(opts.Path, wd)
 	}
 
@@ -246,7 +244,7 @@ func (p *RuleParser) ParseDefaultActions(actions string) error {
 // ParseActions parses a comma separated list of actions:arguments
 // Arguments can be wrapper inside quotes
 func (p *RuleParser) ParseActions(actions string) error {
-	disabledActions := p.options.Config.Get("disabled_rule_actions", []string{}).([]string)
+	disabledActions := p.options.ParserConfig.DisabledRuleActions
 	act, err := parseActions(actions)
 	if err != nil {
 		return err
@@ -297,7 +295,7 @@ func (p *RuleParser) Rule() *corazawaf.Rule {
 type RuleOptions struct {
 	WithOperator bool
 	WAF          *corazawaf.WAF
-	Config       types.Config
+	ParserConfig ParserConfig
 	Directive    string
 	Data         string
 }
@@ -318,10 +316,11 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 		defaultActions: map[types.RulePhase][]ruleAction{},
 	}
 
-	defaultActions := options.Config.Get("rule_default_actions", []string{
-		defaultActionsPhase2,
-	}).([]string)
-	disabledRuleOperators := options.Config.Get("disabled_rule_operators", []string{}).([]string)
+	defaultActions := defaultActionsPhase2
+	if options.ParserConfig.HasRuleDefaultActions {
+		defaultActions = options.ParserConfig.RuleDefaultActions
+	}
+	disabledRuleOperators := options.ParserConfig.DisabledRuleOperators
 
 	for _, da := range defaultActions {
 		err = rp.ParseDefaultActions(da)
@@ -360,8 +359,8 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 	}
 	rule := rp.Rule()
 	rule.Raw_ = fmt.Sprintf("%s %s", options.Directive, options.Data)
-	rule.File_ = options.Config.Get("parser_config_file", "").(string)
-	rule.Line_ = options.Config.Get("parser_last_line", 0).(int)
+	rule.File_ = options.ParserConfig.ConfigFile
+	rule.Line_ = options.ParserConfig.LastLine
 
 	if parent := getLastRuleExpectingChain(options.WAF); parent != nil {
 		rule.ParentID_ = parent.ID_
diff --git a/types/waf.go b/types/waf.go
index dffc0dc..b54e76e 100644
--- a/types/waf.go
+++ b/types/waf.go
@@ -8,27 +8,6 @@ import (
 	"strings"
 )
 
-// Config is used to store the configuration of the WAF
-// Internal configurations are not exported.
-// This is used to connect directives with audit auditlog.
-// A WAF instance will share it's Config object with the
-// audit auditlog.
-type Config map[string]interface{}
-
-// Get returns the configuration value for the given key
-// If the key is not found, it returns the default value
-func (w Config) Get(key string, defaultValue interface{}) interface{} {
-	if value, ok := w[key]; ok {
-		return value
-	}
-	return defaultValue
-}
-
-// Set sets the configuration value for the given key
-func (w Config) Set(key string, value interface{}) {
-	w[key] = value
-}
-
 // AuditEngineStatus represents the functionality
 // of the audit engine.
 type AuditEngineStatus int


Test patch:
diff --git a/testing/engine/json.go b/testing/engine/json.go
index 0848261..a1c554c 100644
--- a/testing/engine/json.go
+++ b/testing/engine/json.go
@@ -34,13 +34,13 @@ var _ = profile.RegisterProfile(profile.Profile{
 								101,
 								1100,
 								1101,
+								1102,
 								1010,
 								1000,
 								1011,
 							},
 							NonTriggeredRules: []int{
 								1111,
-								1102,
 								103,
 							},
 							Headers: map[string]string{
@@ -67,7 +67,7 @@ SecRule REQUEST_BODY "456" "id:103, phase:2, log"
 SecRule ARGS:json.test "@eq 123" "id:1100, phase:2, log, block"
 SecRule ARGS:json.test3.2 "@eq 55" "id:1101, phase:2, log, block"
 
-# We test for some vulnerability
+# Both GET and POST can be matched for the same key
 SecRule ARGS:json.test "@eq 456" "id:1102, phase:2, log, block"
 
 SecRule ARGS:json.test3 "@eq 3" "id: 1010, phase:2, log, block"