Repository: corazawaf/coraza
Pull request #571 (2023-01-13 09:18:20)

chore: implements idempotency for phase calling.
Currently if there is an interruption in a previous phase and one call a ulterior phase, it will do the entire analysis, possible not returning the former interruption. While this is a programatic error, given the nature of the library we should let the user know this is happening.

> Thank you for contributing to Coraza WAF, your effort is greatly appreciated
> Before submitting check if what you want to add to `coraza` list meets [quality standards](https://github.com/corazawaf/coraza/blob/master/CONTRIBUTING.md#quality-standards) before sending pull request. Thanks!

**Make sure that you've checked the boxes below before you submit PR:**

- [ ] My code includes positive and negative tests.
- [ ] I have an appropriate description with correct grammar.
- [ ] I have read [Contribution guidelines](https://github.com/corazawaf/coraza/blob/master/CONTRIBUTING.md#contribution-guidelines), [maintainers note](https://github.com/corazawaf/coraza/blob/master/CONTRIBUTING.md#maintainers) and [Quality standard](https://github.com/github.com/corazawaf/coraza/v3sso/coraza-waf/blob/master/CONTRIBUTING.md#quality-standards).
- [ ] My code is properly linted and passes pre-commit tests.

Thanks for your contribution :heart:

sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=coraza&pullRequest=571)

[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=BUG)  
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=VULNERABILITY)  
[![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=coraza&pullRequest=571&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=coraza&pullRequest=571&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=coraza&pullRequest=571&resolved=false&types=SECURITY_HOTSPOT)  
[![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=CODE_SMELL) [92 Code Smells](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=CODE_SMELL)

[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=coraza&pullRequest=571&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=coraza&pullRequest=571&metric=new_coverage&view=list)  
[![0.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.1%')](https://sonarcloud.io/component_measures?id=coraza&pullRequest=571&metric=new_duplicated_lines_density&view=list) [0.1% Duplication](https://sonarcloud.io/component_measures?id=coraza&pullRequest=571&metric=new_duplicated_lines_density&view=list)


sonarcloud[bot]: Please retry analysis of this Pull-Request directly on [SonarCloud](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571).
sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=coraza&pullRequest=571)

[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=BUG)  
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=VULNERABILITY)  
[![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=coraza&pullRequest=571&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=coraza&pullRequest=571&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=coraza&pullRequest=571&resolved=false&types=SECURITY_HOTSPOT)  
[![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=CODE_SMELL) [92 Code Smells](https://sonarcloud.io/project/issues?id=coraza&pullRequest=571&resolved=false&types=CODE_SMELL)

[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=coraza&pullRequest=571&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=coraza&pullRequest=571&metric=new_coverage&view=list)  
[![0.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.1%')](https://sonarcloud.io/component_measures?id=coraza&pullRequest=571&metric=new_duplicated_lines_density&view=list) [0.1% Duplication](https://sonarcloud.io/component_measures?id=coraza&pullRequest=571&metric=new_duplicated_lines_density&view=list)


codecov-commenter: # [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/571?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Base: **78.72**% // Head: **78.79**% // Increases project coverage by **`+0.07%`** :tada:
> Coverage data is based on head [(`2ad3b36`)](https://codecov.io/gh/corazawaf/coraza/pull/571?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) compared to base [(`e9e29a5`)](https://codecov.io/gh/corazawaf/coraza/commit/e9e29a55b8b06355e48e6e50edb26086125db697?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf).
> Patch coverage: 95.23% of modified lines in pull request are covered.

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


```diff
@@            Coverage Diff             @@
##           v3/dev     #571      +/-   ##
==========================================
+ Coverage   78.72%   78.79%   +0.07%     
==========================================
  Files         145      145              
  Lines        6993     7009      +16     
==========================================
+ Hits         5505     5523      +18     
+ Misses       1257     1255       -2     
  Partials      231      231              
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `74.20% <95.23%> (+0.08%)` | :arrow_up: |
| examples | `27.19% <19.04%> (-0.07%)` | :arrow_down: |
| ftw | `56.53% <19.04%> (-0.14%)` | :arrow_down: |
| tinygo | `72.24% <95.23%> (+0.10%)` | :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/571?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | |
|---|---|---|
| [internal/corazawaf/transaction.go](https://codecov.io/gh/corazawaf/coraza/pull/571?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3RyYW5zYWN0aW9uLmdv) | `80.11% <95.23%> (+0.26%)` | :arrow_up: |
| [loggers/debug.go](https://codecov.io/gh/corazawaf/coraza/pull/571?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-bG9nZ2Vycy9kZWJ1Zy5nbw==) | `80.00% <0.00%> (+13.33%)` | :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/571?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/transaction.go b/internal/corazawaf/transaction.go
index 71aed0a..6412274 100644
--- a/internal/corazawaf/transaction.go
+++ b/internal/corazawaf/transaction.go
@@ -5,6 +5,7 @@ package corazawaf
 
 import (
 	"bufio"
+	"errors"
 	"fmt"
 	"io"
 	"mime"
@@ -744,6 +745,12 @@ func (tx *Transaction) ProcessRequestHeaders() *types.Interruption {
 		// Rule engine is disabled
 		return nil
 	}
+
+	if tx.interruption != nil {
+		tx.WAF.Logger.Error("Calling ProcessRequestHeaders but there is a preexisting interruption")
+		return tx.interruption
+	}
+
 	tx.WAF.Rules.Eval(types.PhaseRequestHeaders, tx)
 	return tx.interruption
 }
@@ -763,6 +770,12 @@ func (tx *Transaction) ProcessRequestBody() (*types.Interruption, error) {
 	if tx.RuleEngine == types.RuleEngineOff {
 		return nil, nil
 	}
+
+	if tx.interruption != nil {
+		tx.WAF.Logger.Error("Calling ProcessRequestBody but there is a preexisting interruption")
+		return tx.interruption, nil
+	}
+
 	// we won't process empty request bodies or disabled RequestBodyAccess
 	if !tx.RequestBodyAccess || tx.RequestBodyBuffer.Size() == 0 {
 		tx.WAF.Rules.Eval(types.PhaseRequestBody, tx)
@@ -815,7 +828,7 @@ func (tx *Transaction) ProcessRequestBody() (*types.Interruption, error) {
 	}
 	bodyprocessor, err := bodyprocessors.Get(rbp)
 	if err != nil {
-		tx.generateReqbodyError(fmt.Errorf("Invalid body processor"))
+		tx.generateReqbodyError(errors.New("invalid body processor"))
 		tx.WAF.Rules.Eval(types.PhaseRequestBody, tx)
 		return tx.interruption, nil
 	}
@@ -839,14 +852,19 @@ func (tx *Transaction) ProcessRequestBody() (*types.Interruption, error) {
 //
 // note: Remember to check for a possible intervention.
 func (tx *Transaction) ProcessResponseHeaders(code int, proto string) *types.Interruption {
-	c := strconv.Itoa(code)
-	tx.variables.responseStatus.Set(c)
-	tx.variables.responseProtocol.Set(proto)
-
 	if tx.RuleEngine == types.RuleEngineOff {
 		return nil
 	}
 
+	if tx.interruption != nil {
+		tx.WAF.Logger.Error("Calling ProcessResponseHeaders but there is a preexisting interruption")
+		return tx.interruption
+	}
+
+	c := strconv.Itoa(code)
+	tx.variables.responseStatus.Set(c)
+	tx.variables.responseProtocol.Set(proto)
+
 	tx.WAF.Rules.Eval(types.PhaseResponseHeaders, tx)
 	return tx.interruption
 }
@@ -875,8 +893,14 @@ func (tx *Transaction) ResponseBodyWriter() io.Writer {
 // note Remember to check for a possible intervention.
 func (tx *Transaction) ProcessResponseBody() (*types.Interruption, error) {
 	if tx.RuleEngine == types.RuleEngineOff {
+		return nil, nil
+	}
+
+	if tx.interruption != nil {
+		tx.WAF.Logger.Error("Calling ProcessResponseBody but there is a preexisting interruption")
 		return tx.interruption, nil
 	}
+
 	if !tx.ResponseBodyAccess || !tx.IsResponseBodyProcessable() {
 		tx.WAF.Logger.Debug("[%s] Skipping response body processing (Access: %t)", tx.id, tx.ResponseBodyAccess)
 		tx.WAF.Rules.Eval(types.PhaseResponseBody, tx)


Test patch:
diff --git a/internal/corazawaf/transaction_test.go b/internal/corazawaf/transaction_test.go
index ab95f20..94c7d28 100644
--- a/internal/corazawaf/transaction_test.go
+++ b/internal/corazawaf/transaction_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/corazawaf/coraza/v3/collection"
 	"github.com/corazawaf/coraza/v3/internal/corazarules"
 	utils "github.com/corazawaf/coraza/v3/internal/strings"
+	"github.com/corazawaf/coraza/v3/loggers"
 	"github.com/corazawaf/coraza/v3/macro"
 	"github.com/corazawaf/coraza/v3/types"
 	"github.com/corazawaf/coraza/v3/types/variables"
@@ -492,10 +493,6 @@ func TestTxVariablesExceptions(t *testing.T) {
 	}
 }
 
-func TestAuditLogMessages(t *testing.T) {
-
-}
-
 func TestTransactionSyncPool(t *testing.T) {
 	waf := NewWAF()
 	tx := waf.NewTransaction()
@@ -793,3 +790,81 @@ func BenchmarkMacro(b *testing.B) {
 		})
 	}
 }
+
+type inspectableLogger struct {
+	entries []string
+}
+
+func (l *inspectableLogger) Write(p []byte) (n int, err error) {
+	l.entries = append(l.entries, string(p))
+	return len(p), nil
+}
+
+func (l *inspectableLogger) Close() error {
+	l.entries = nil
+	return nil
+}
+
+func TestProcessorsIdempotency(t *testing.T) {
+	l := &inspectableLogger{}
+
+	waf := NewWAF()
+	waf.Logger.SetOutput(l)
+	waf.Logger.SetLevel(loggers.LogLevelError)
+
+	expectedInterruption := &types.Interruption{
+		RuleID: 123,
+	}
+
+	tx := waf.NewTransaction()
+	tx.interruption = expectedInterruption
+
+	testCases := map[string]func(tx *Transaction) *types.Interruption{
+		"ProcessRequestHeaders": func(tx *Transaction) *types.Interruption {
+			return tx.ProcessRequestHeaders()
+		},
+		"ProcessRequestBody": func(tx *Transaction) *types.Interruption {
+			it, err := tx.ProcessRequestBody()
+			if err != nil {
+				t.Fatal("unexpected error when processing request body")
+			}
+			return it
+		},
+		"ProcessResponseHeaders": func(tx *Transaction) *types.Interruption {
+			return tx.ProcessResponseHeaders(200, "HTTP/1")
+		},
+		"ProcessResponseBody": func(tx *Transaction) *types.Interruption {
+			it, err := tx.ProcessResponseBody()
+			if err != nil {
+				t.Fatal("unexpected error when processing response body")
+			}
+			return it
+		},
+	}
+
+	for processor, tCase := range testCases {
+		t.Run(processor, func(t *testing.T) {
+			it := tCase(tx)
+			if it == nil {
+				t.Fatal("expected interruption")
+			}
+
+			if it != expectedInterruption {
+				t.Fatal("unexpected interruption")
+			}
+
+			if want, have := 1, len(l.entries); want != have {
+				t.Fatalf("unexpected number of log entries, want %d, have %d", want, have)
+			}
+
+			expectedMessage := fmt.Sprintf("[ERROR] Calling %s but there is a preexisting interruption\n", processor)
+
+			if want, have := expectedMessage, l.entries[0]; want != have {
+				t.Fatalf("unexpected message, want %q, have %q", want, have)
+			}
+
+			l.Close()
+		})
+	}
+
+}