Repository: corazawaf/coraza
Pull request #524 (2022-12-05 15:12:59)

Skips Evaluate during ProcessLogging if EngineOff
Closes https://github.com/corazawaf/coraza/issues/520.

When the rule engine is off, `tx.WAF.Rules.Eval` is still called during `ProcessLogging()`. It may end up with errors because of variables not initialized by previous rules (see https://github.com/corazawaf/coraza/issues/520).
This PR proposes to skill the rule evaluation but still process audit logs. I see value in this example scenario:
1. RuleEngine is `On`
2. A whitelist rule is triggered with `ctl:ruleEngine=Off`
3. I still want to have details about this transaction up to when the white list rule has been triggered

@jptosso, do you remember other considerations behind this comment?
https://github.com/corazawaf/coraza/blob/63a49c8b17230f81a1a812298cbc0c336e59b15f/internal/corazawaf/transaction.go#L905-L908


jptosso: It was some difference between modsec 2 and modsec 3 evaluation, but looks good to me.
codecov-commenter: # [Codecov](https://codecov.io/gh/corazawaf/coraza/pull/524?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) Report
Base: **78.08**% // Head: **78.21**% // Increases project coverage by **`+0.13%`** :tada:
> Coverage data is based on head [(`379bbef`)](https://codecov.io/gh/corazawaf/coraza/pull/524?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) compared to base [(`80c5f90`)](https://codecov.io/gh/corazawaf/coraza/commit/80c5f90cef4c42daf5abf95c093554ffb2f90371?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf).
> Patch coverage: 100.00% of modified lines in pull request are covered.

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


```diff
@@            Coverage Diff             @@
##           v3/dev     #524      +/-   ##
==========================================
+ Coverage   78.08%   78.21%   +0.13%     
==========================================
  Files         145      145              
  Lines        6876     6876              
==========================================
+ Hits         5369     5378       +9     
+ Misses       1270     1264       -6     
+ Partials      237      234       -3     
```

| Flag | Coverage Δ | |
|---|---|---|
| default | `73.48% <100.00%> (+0.13%)` | :arrow_up: |
| examples | `27.19% <75.00%> (ø)` | |
| ftw | `55.44% <75.00%> (ø)` | |
| tinygo | `71.53% <100.00%> (+0.23%)` | :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/524?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/524/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aW50ZXJuYWwvY29yYXphd2FmL3RyYW5zYWN0aW9uLmdv) | `79.94% <100.00%> (+0.75%)` | :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/524?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).

M4tteoP: Done! Added them for all the EngineOff checks

Patch:
diff --git a/go.work.sum b/go.work.sum
index 8c4428f..45c810a 100644
--- a/go.work.sum
+++ b/go.work.sum
@@ -24,8 +24,6 @@ github.com/client9/misspell v0.3.4 h1:ta993UF76GwbvJcIo3Y68y/M3WxlpEHPWIGDkJYwzJ
 github.com/cncf/udpa/go v0.0.0-20201120205902-5459f2c99403 h1:cqQfy1jclcSy/FwLjemeg3SR1yaINm74aQyupQ0Bl8M=
 github.com/coreos/go-semver v0.3.0 h1:wkHLiw0WNATZnSG7epLsujiMCgPAc9xhjJ4tgnAxmfM=
 github.com/coreos/go-systemd/v22 v22.3.3-0.20220203105225-a9a7ef127534 h1:rtAn27wIbmOGUs7RIbVgPEjb31ehTVniDwPGXyMxm5U=
-github.com/coreruleset/go-ftw v0.4.5 h1:qdT7Z547zBaT7G1czd+sX61WZDHRxz8GeNVtFp+GTsA=
-github.com/coreruleset/go-ftw v0.4.5/go.mod h1:wE4FMwBYg0Cse5Uj9BxZhgE7NQ1BxB4cog1iiboi+8I=
 github.com/cpuguy83/go-md2man/v2 v2.0.2 h1:p1EgwI/C7NhT0JmVkwCD2ZBK8j4aeHQX2pMHHBfMQ6w=
 github.com/dustin/go-humanize v1.0.0 h1:VSnTsYCnlFHaM2/igO1h6X3HA71jcobQuxemgkq4zYo=
 github.com/envoyproxy/go-control-plane v0.9.9-0.20210217033140-668b12f5399d h1:QyzYnTnPE15SQyUeqU6qLbWxMkwyAyu+vGksa0b7j00=
@@ -73,7 +71,6 @@ github.com/hashicorp/serf v0.9.6 h1:uuEX1kLR6aoda1TBttmJQKDLZE1Ob7KN0NPdE7EtCDc=
 github.com/hashicorp/vault/api v1.0.4 h1:j08Or/wryXT4AcHj1oCbMd7IijXcKzYUGw59LGu9onU=
 github.com/hashicorp/vault/sdk v0.1.13 h1:mOEPeOhT7jl0J4AMl1E705+BcmeRs1VmKNb9F0sMLy8=
 github.com/hashicorp/yamux v0.0.0-20181012175058-2f1d1f20f75d h1:kJCB4vdITiW1eC1vq2e6IsrXKrZit1bv/TDYFGMp4BQ=
-github.com/hjson/hjson-go/v4 v4.0.0 h1:wlm6IYYqHjOdXH1gHev4VoXCaW20HdQAGCxdOEEg2cs=
 github.com/jmespath/go-jmespath v0.4.0 h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg=
 github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8=
 github.com/jpillora/backoff v1.0.0 h1:uvFg412JmmHBHw7iwprIxkPMI+sGQ4kzOWsMeHnm2EA=
@@ -83,9 +80,7 @@ github.com/kisielk/errcheck v1.5.0 h1:e8esj/e4R+SAOwFwN+n3zr0nYeCyeweozKfO23MvHz
 github.com/kisielk/gotool v1.0.0 h1:AV2c/EiW3KqPNT9ZKl07ehoAGi4C5/01Cfbblndcapg=
 github.com/konsorten/go-windows-terminal-sequences v1.0.3 h1:CE8S1cTafDpPvMhIxNJKvHsGVBgn1xWYf1NbHQhywc8=
 github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515 h1:T+h1c/A9Gawja4Y9mFVWj2vyii2bbUNDw3kt9VxK2EY=
-github.com/kr/pretty v0.2.0 h1:s5hAObm+yFO5uHYt5dYjxi2rXrsnmRpJx4OYvIWUaQs=
 github.com/kr/pty v1.1.1 h1:VkoXIwSboBpnk99O/KFauAEILuNHv5DVFKZMBN/gUgw=
-github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0jegS5sx/RkqARlsWZ6pIwiU=
 github.com/mitchellh/cli v1.1.0 h1:tEElEatulEHDeedTxwckzyYMA5c86fbmNIUL1hBIiTg=
 github.com/mitchellh/go-homedir v1.1.0 h1:lukF9ziXFxDFPkA1vsr5zpc1XuPDn/wFntq5mG+4E0Y=
@@ -113,7 +108,6 @@ github.com/ryanuber/go-glob v1.0.0 h1:iQh3xXAumdQ+4Ufa5b25cRpC5TYKlno6hsv6Cb3pkB
 github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529 h1:nn5Wsu0esKSJiIVhscUtVbo7ada43DJhG55ua/hjS5I=
 github.com/sirupsen/logrus v1.6.0 h1:UBcNElsrwanuuMsnGSlYmtmgbb23qDR5dG+6X6Oo89I=
 github.com/stretchr/objx v0.1.1 h1:2vfRuCMp5sSVIDSqO8oNnWJq7mPa6KVP3iPIwFBuy8A=
-github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
 github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
 go.etcd.io/etcd/api/v3 v3.5.4 h1:OHVyt3TopwtUQ2GKdd5wu3PmmipR4FTwCqoEjSyRdIc=
@@ -146,7 +140,7 @@ google.golang.org/grpc v1.38.0 h1:/9BgsAsa5nWe26HqOlvlgJnqBuktYOLCgjCPqsa56W0=
 google.golang.org/protobuf v1.26.0 h1:bxAC2xTBsZGibn2RTntX0oH50xLsqy1OxA9tTL3p/lk=
 gopkg.in/alecthomas/kingpin.v2 v2.2.6 h1:jMFz6MfLP0/4fUyZle81rXUoxOBFi19VUFKVDOQfozc=
 gopkg.in/asn1-ber.v1 v1.0.0-20181015200546-f715ec2f112d h1:TxyelI5cVkbREznMhfzycHdkp5cLA7DpE+GKjSslYhM=
-gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogRM/Nc3DYOhEAlW+xobZo=
 gopkg.in/square/go-jose.v2 v2.3.1 h1:SK5KegNXmKmqE342YYN2qPHEnUYeoMiXXl1poUlI+o4=
+gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY=
 honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc h1:/hemPrYIhOhy8zYrNj+069zDB68us2sMGsfkFJO0iZs=
 sigs.k8s.io/yaml v1.2.0 h1:kr/MCeFWJWTwyaHoR9c8EjH9OumOmoF9YGiZd7lFm/Q=
diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go
index a49d148..4191023 100644
--- a/internal/corazawaf/transaction.go
+++ b/internal/corazawaf/transaction.go
@@ -734,7 +734,7 @@ func (tx *Transaction) ProcessURI(uri string, method string, httpVersion string)
 // note: Remember to check for a possible intervention.
 func (tx *Transaction) ProcessRequestHeaders() *types.Interruption {
 	if tx.RuleEngine == types.RuleEngineOff {
-		// RUle engine is disabled
+		// Rule engine is disabled
 		return nil
 	}
 	tx.WAF.Rules.Eval(types.PhaseRequestHeaders, tx)
@@ -902,11 +902,12 @@ func (tx *Transaction) ProcessResponseBody() (*types.Interruption, error) {
 // At this point there is not need to hold the connection, the response can be
 // delivered prior to the execution of this method.
 func (tx *Transaction) ProcessLogging() {
-	// I'm not sure why but modsecurity won't log if RuleEngine is disabled
-	// if tx.RuleEngine == RULE_ENGINE_OFF {
-	// 	return
-	// }
-	tx.WAF.Rules.Eval(types.PhaseLogging, tx)
+	// If Rule engine is disabled, Log phase rules are not going to be evaluated.
+	// This avoids trying to rely on variables not set by previous rules that
+	// have not been executed
+	if tx.RuleEngine != types.RuleEngineOff {
+		tx.WAF.Rules.Eval(types.PhaseLogging, tx)
+	}
 
 	if tx.AuditEngine == types.AuditEngineOff {
 		// Audit engine disabled
@@ -932,7 +933,7 @@ func (tx *Transaction) ProcessLogging() {
 
 	tx.WAF.Logger.Debug("[%s] Transaction marked for audit logging", tx.id)
 	if writer := tx.WAF.AuditLogWriter; writer != nil {
-		// we don't log if there is an empty audit logger
+		// We don't log if there is an empty audit logger
 		if err := writer.Write(tx.AuditLog()); err != nil {
 			tx.WAF.Logger.Error(err.Error())
 		}


Test patch:
diff --git a/internal/corazawaf/transaction_test.go b/internal/corazawaf/transaction_test.go
index 31e12b9..0fa158e 100644
--- a/internal/corazawaf/transaction_test.go
+++ b/internal/corazawaf/transaction_test.go
@@ -206,7 +206,61 @@ func TestResponseHeader(t *testing.T) {
 
 	interruption := tx.ProcessResponseHeaders(200, "OK")
 	if interruption != nil {
-		t.Error("expected interruption")
+		t.Error("unexpected interruption")
+	}
+}
+
+func TestProcessRequestHeadersEngineOff(t *testing.T) {
+	tx := wafi.NewTransaction()
+	tx.RuleEngine = types.RuleEngineOff
+
+	if !tx.IsRuleEngineOff() {
+		t.Error("expected Engine off")
+	}
+
+	_ = tx.ProcessRequestHeaders()
+	if tx.LastPhase != 0 { // 0 means no phases have been evaluated
+		t.Error("unexpected rule evaluation")
+	}
+}
+
+func TestProcessRequestBodyEngineOff(t *testing.T) {
+	tx := wafi.NewTransaction()
+	tx.RuleEngine = types.RuleEngineOff
+	if _, err := tx.ProcessRequestBody(); err != nil {
+		t.Error("failed to process request body")
+	}
+	if tx.LastPhase != 0 {
+		t.Error("unexpected rule evaluation")
+	}
+}
+
+func TestProcessResponseHeadersEngineOff(t *testing.T) {
+	tx := wafi.NewTransaction()
+	tx.RuleEngine = types.RuleEngineOff
+	_ = tx.ProcessResponseHeaders(200, "OK")
+	if tx.LastPhase != 0 {
+		t.Error("unexpected rule evaluation")
+	}
+}
+
+func TestProcessResponseBodyEngineOff(t *testing.T) {
+	tx := wafi.NewTransaction()
+	tx.RuleEngine = types.RuleEngineOff
+	if _, err := tx.ProcessResponseBody(); err != nil {
+		t.Error("Failed to process response body")
+	}
+	if tx.LastPhase != 0 {
+		t.Error("unexpected rule evaluation")
+	}
+}
+
+func TestProcessLoggingEngineOff(t *testing.T) {
+	tx := wafi.NewTransaction()
+	tx.RuleEngine = types.RuleEngineOff
+	tx.ProcessLogging()
+	if tx.LastPhase != 0 {
+		t.Error("unexpected rule evaluation")
 	}
 }