Repository: corazawaf/coraza
Pull request #724 (2023-03-15 03:07:50)

Propagate parsed line for raw rule storage
Raw rules are stored for audit log purposes. Currently we reconstruct the raw string, but this means that much content of the string is stored twice, in this new string, and as slices from the original parsed line that is often stored in operators such as rx or pm. If we propagate from the original line instead, the string is only present once.

codecov[bot]: ## [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/724?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Patch coverage: **`80.00`**% and project coverage change: **`+0.02`** :tada:
> Comparison is base [(`ac7bbf9`)](https://codecov.io/gh/corazawaf/coraza/commit/ac7bbf94d3a73712e38bc482a45ed52c60bc9dcd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.50% compared to head [(`0d2ec19`)](https://codecov.io/gh/corazawaf/coraza/pull/724?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) 81.52%.

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


```diff
@@            Coverage Diff             @@
##           v3/dev     #724      +/-   ##
==========================================
+ Coverage   81.50%   81.52%   +0.02%     
==========================================
  Files         148      148              
  Lines        8265     8264       -1     
==========================================
+ Hits         6736     6737       +1     
+ Misses       1302     1300       -2     
  Partials      227      227              
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `77.53% <80.00%> (+0.02%)` | :arrow_up: |
| examples | `25.54% <60.00%> (+<0.01%)` | :arrow_up: |
| ftw | `49.60% <80.00%> (+0.01%)` | :arrow_up: |
| ftw-multiphase | `49.69% <80.00%> (+0.01%)` | :arrow_up: |
| tinygo | `76.65% <80.00%> (+0.02%)` | :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/724?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [internal/seclang/parser.go](https://codecov.io/gh/corazawaf/coraza/pull/724?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9wYXJzZXIuZ28=) | `93.84% <50.00%> (+1.47%)` | :arrow_up: |
| [internal/seclang/directives.go](https://codecov.io/gh/corazawaf/coraza/pull/724?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9kaXJlY3RpdmVzLmdv) | `70.97% <100.00%> (+0.12%)` | :arrow_up: |
| [internal/seclang/rule\_parser.go](https://codecov.io/gh/corazawaf/coraza/pull/724?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvc2VjbGFuZy9ydWxlX3BhcnNlci5nbw==) | `84.07% <100.00%> (-0.08%)` | :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 in Codecov by Sentry](https://codecov.io/gh/corazawaf/coraza/pull/724?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/internal/corazawaf/rulegroup.go b/internal/corazawaf/rulegroup.go
index 2ea341e..96c9415 100644
--- a/internal/corazawaf/rulegroup.go
+++ b/internal/corazawaf/rulegroup.go
@@ -214,6 +214,8 @@ RulesLoop:
 	if tx.AllowType == corazatypes.AllowTypePhase {
 		tx.AllowType = corazatypes.AllowTypeUnset
 	}
+	// // Reset Skip counter at the end of each phase. Skip actions work only within the current processing phase
+	tx.Skip = 0
 
 	tx.stopWatches[phase] = time.Now().UnixNano() - ts
 	return tx.interruption != nil
diff --git a/internal/seclang/directives.go b/internal/seclang/directives.go
index 50d3fca..271c209 100644
--- a/internal/seclang/directives.go
+++ b/internal/seclang/directives.go
@@ -25,6 +25,7 @@ import (
 // TODO(anuraaga): Propagation of config probably should be separated from a directive's options.
 type DirectiveOptions struct {
 	WAF      *corazawaf.WAF
+	Raw      string
 	Opts     string
 	Path     []string
 	Datasets map[string][]string
@@ -140,6 +141,7 @@ func directiveSecAction(options *DirectiveOptions) error {
 		WithOperator: false,
 		WAF:          options.WAF,
 		ParserConfig: options.Parser,
+		Raw:          options.Raw,
 		Directive:    "SecAction",
 		Data:         options.Opts,
 	})
@@ -180,6 +182,7 @@ func directiveSecRule(options *DirectiveOptions) error {
 		WithOperator: true,
 		WAF:          options.WAF,
 		ParserConfig: options.Parser,
+		Raw:          options.Raw,
 		Directive:    "SecRule",
 		Data:         options.Opts,
 	})
diff --git a/internal/seclang/parser.go b/internal/seclang/parser.go
index 79f91e4..9672036 100644
--- a/internal/seclang/parser.go
+++ b/internal/seclang/parser.go
@@ -135,8 +135,7 @@ func (p *Parser) FromString(data string) error {
 
 func (p *Parser) evaluateLine(l string) error {
 	if l == "" || l[0] == '#' {
-		// at this point we should not receive an empty line or a commented line
-		return errors.New("invalid line")
+		panic("invalid line")
 	}
 	// first we get the directive
 	dir, opts, _ := strings.Cut(l, " ")
@@ -163,6 +162,7 @@ func (p *Parser) evaluateLine(l string) error {
 		return p.log(fmt.Sprintf("unknown directive %q", directive))
 	}
 
+	p.options.Raw = l
 	p.options.Opts = opts
 	p.options.Parser.LastLine = p.currentLine
 	p.options.Parser.ConfigFile = p.currentFile
diff --git a/internal/seclang/rule_parser.go b/internal/seclang/rule_parser.go
index 6fb483c..08a290e 100644
--- a/internal/seclang/rule_parser.go
+++ b/internal/seclang/rule_parser.go
@@ -172,10 +172,8 @@ func (p *RuleParser) ParseOperator(operator string) error {
 	// 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 @
@@ -296,6 +294,7 @@ type RuleOptions struct {
 	WithOperator bool
 	WAF          *corazawaf.WAF
 	ParserConfig ParserConfig
+	Raw          string
 	Directive    string
 	Data         string
 }
@@ -358,7 +357,7 @@ func ParseRule(options RuleOptions) (*corazawaf.Rule, error) {
 		}
 	}
 	rule := rp.Rule()
-	rule.Raw_ = fmt.Sprintf("%s %s", options.Directive, options.Data)
+	rule.Raw_ = options.Raw
 	rule.File_ = options.ParserConfig.ConfigFile
 	rule.Line_ = options.ParserConfig.LastLine
 


Test patch:
diff --git a/testing/engine/skip.go b/testing/engine/skip.go
new file mode 100644
index 0000000..20c9111
--- /dev/null
+++ b/testing/engine/skip.go
@@ -0,0 +1,212 @@
+// 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:      "M4tteoP",
+		Description: "Tests skip actions",
+		Enabled:     true,
+		Name:        "skip.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "skip actions",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI: "/firstskip",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules:    []int{1, 2},
+							NonTriggeredRules: []int{3},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 500,
+								Data:   "",
+								RuleID: 2,
+								Action: "deny",
+							},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+SecDebugLogLevel 5
+
+# Skip action of rule 1 will skip the next rule (if any) of the same phase. Therefore rule 3 will be skipped and rule 2 will be triggered.
+SecRule REQUEST_URI "/firstskip" "id:1, phase:1, skip:1,log"
+SecRule REQUEST_URI "/firstskip" "id:2, phase:2, t:none, log, deny, status:500"
+SecRule REQUEST_URI "/firstskip" "id:3, phase:1, t:none, log, deny, status:500"
+`,
+})
+
+var _ = profile.RegisterProfile(profile.Profile{
+	Meta: profile.Meta{
+		Author:      "M4tteoP",
+		Description: "Tests skip actions",
+		Enabled:     true,
+		Name:        "skip_2.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "skip actions",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI:     "/phase_skip",
+							Method:  "POST",
+							Headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"},
+							Data:    "phase_skip",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{20, 21},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 500,
+								Data:   "",
+								RuleID: 21,
+								Action: "deny",
+							},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+SecDebugLogLevel 5
+SecRequestBodyAccess On
+
+# Skip action works only within the current phase, rule 20 should not skip rule 21 
+SecRule REQUEST_URI "/phase_skip" "id:20, phase:1, skip:1,log"
+SecRule REQUEST_BODY "phase_skip" "id:21, phase:2, t:none, log, deny, status:500"
+`,
+})
+var _ = profile.RegisterProfile(profile.Profile{
+	Meta: profile.Meta{
+		Author:      "M4tteoP",
+		Description: "Tests skip actions",
+		Enabled:     true,
+		Name:        "skip_3.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "skip actions",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI:     "/phase_multi_skip",
+							Method:  "POST",
+							Headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"},
+							Data:    "phase_multi_skip",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules:    []int{30, 31},
+							NonTriggeredRules: []int{32},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+SecDebugLogLevel 5
+SecRequestBodyAccess On
+
+# Skip action works only within the current phase, rule 30 should skip rule 32, not rule 31
+SecRule REQUEST_URI "/phase_multi_skip" "id:30, phase:2, skip:1,log"
+SecRule REQUEST_URI "/phase_multi_skip" "id:31, phase:1,pass"
+SecRule REQUEST_BODY "phase_multi_skip" "id:32, phase:2, t:none, log, deny, status:500"
+`,
+})
+
+var _ = profile.RegisterProfile(profile.Profile{
+	Meta: profile.Meta{
+		Author:      "M4tteoP",
+		Description: "Tests skipafter action",
+		Enabled:     true,
+		Name:        "skipafter.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "skipafter action",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI:     "/single_skipafter_phase",
+							Method:  "POST",
+							Headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"},
+							Data:    "single_skipafter_phase",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules: []int{40, 41},
+							Interruption: &profile.ExpectedInterruption{
+								Status: 500,
+								Data:   "",
+								RuleID: 41,
+								Action: "deny",
+							},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+SecDebugLogLevel 5
+
+# SkipAfter action works only within the current phase
+SecRule REQUEST_URI "/single_skipafter_phase" "id:40, phase:1, skipAfter:LOCATION_ONE,log"
+SecRule REQUEST_URI|REQUEST_BODY "single_skipafter_phase" "id:41, phase:2, t:none, log, deny, status:500"
+SecMarker LOCATION_ONE
+`,
+})
+
+var _ = profile.RegisterProfile(profile.Profile{
+	Meta: profile.Meta{
+		Author:      "M4tteoP",
+		Description: "Tests skipafter action",
+		Enabled:     true,
+		Name:        "skipafter_2.yaml",
+	},
+	Tests: []profile.Test{
+		{
+			Title: "skipafter action",
+			Stages: []profile.Stage{
+				{
+					Stage: profile.SubStage{
+						Input: profile.StageInput{
+							URI:     "/multi_skipafter_phase",
+							Method:  "POST",
+							Headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"},
+							Data:    "multi_skipafter_phase",
+						},
+						Output: profile.ExpectedOutput{
+							TriggeredRules:    []int{50, 51},
+							NonTriggeredRules: []int{52},
+						},
+					},
+				},
+			},
+		},
+	},
+	Rules: `
+SecDebugLogLevel 5
+
+# skipAfter action works only within the current phase, rule 50 should skip rule 52, not rule 51
+SecRule REQUEST_URI "/multi_skipafter_phase" "id:50, phase:2, skipAfter:LOCATION_TWO,log"
+SecRule REQUEST_URI "/multi_skipafter_phase" "id:51, phase:1,pass"
+SecRule REQUEST_BODY "multi_skipafter_phase" "id:52, phase:2, t:none, log, deny, status:500"
+SecMarker LOCATION_TWO
+`,
+})