Allow action generates logs in DetectionOnly mode

[MrWako]

Description

In Deny mode the WAF seems to correctly handle the allow action, and after triggering an allow rule other rules in the same and subsequent phases are ignored and not triggered. In contrast in DetectionOnly mode if the same allow rule is triggered the WAF seems to continue and evaluate subsequent rules. Whilst this doesn't block the request it does generate critical log messages which makes it hard to tune the WAF and be confident that it is correctly configured, before switching to Deny mode.

I guess it might be expected behaviour, although as mentioned it makes switching between DetectionOnly and Deny mode challenging.

Steps to reproduce

import (
    "fmt"
    "log"
    "net/http"
    "net/http/httptest"
    "testing"

    coreruleset "github.com/corazawaf/coraza-coreruleset"
    "github.com/corazawaf/coraza/v3"
    coraza_http "github.com/corazawaf/coraza/v3/http"
    "github.com/corazawaf/coraza/v3/types"
    "github.com/stretchr/testify/assert"
)

func Test_Allow(t *testing.T) {
    // In deny mode we exit the WAF transaction and don't trigger any subsequent rules
    fmt.Println("Testing Deny Mode")
    denyWAF := testPanicWAF(t, true)
    denyH := coraza_http.WrapHandler(denyWAF, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
        rw.WriteHeader(http.StatusCreated)
    }))
    rw := httptest.NewRecorder()
    r := httptest.NewRequest(http.MethodDelete, "/allowed-path", nil)
    denyH.ServeHTTP(rw, r)
    assert.Equal(t, http.StatusCreated, rw.Code)

    // In detect mode we still log all the rules we break even though we have allowed the request
    fmt.Println("Testing Detect Mode")
    detectWAF := testPanicWAF(t, false)
    detectH := coraza_http.WrapHandler(detectWAF, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
        rw.WriteHeader(http.StatusCreated)
    }))

    rw = httptest.NewRecorder()
    r = httptest.NewRequest(http.MethodDelete, "/allowed-path", nil)
    detectH.ServeHTTP(rw, r)
    assert.Equal(t, http.StatusCreated, rw.Code)
}

// testPanicWAF is a waf that will panic if it logs anything.
// The WAF is configured so that anything on the route /allowed-path should be allowed through
func testPanicWAF(t *testing.T, deny bool) coraza.WAF {
    errorCallBack := func(err types.MatchedRule) {
        msg := err.Message()
        id := err.Rule().ID()
        file := err.Rule().File()
        serverity := err.Rule().Severity().String()
        uri := err.URI()
        data := err.Data()
        log.Panicf("WAF [%s] %s [%s] ID: %d [%s] URI: %s\n", serverity, msg, data, id, file, uri)
    }

    config := coraza.NewWAFConfig().
        WithErrorCallback(errorCallBack).
        WithRootFS(coreruleset.FS).
        WithDirectives("Include @coraza.conf-recommended").
        WithDirectives("Include @crs-setup.conf.example").
        WithDirectives(`SecRule REQUEST_URI "^/allowed-path" "id:1,phase:1,allow,nolog"`)

    if deny {
        config = config.WithDirectives("SecRuleEngine On")
    } else {
        config = config.WithDirectives("SecRuleEngine DetectionOnly")
    }

    config = config.WithDirectives("Include @owasp_crs/*.conf")

    waf, err := coraza.NewWAF(config)
    assert.NoError(t, err)
    return waf
}

Expected result

No logs generated for either Deny or Detect.

Actual result

output from the test

Testing Deny Mode
Testing Detect Mode
2024/04/25 21:20:06 WAF [critical] Method is not allowed by policy [DELETE] ID: 911100 [@owasp_crs/REQUEST-911-METHOD-ENFORCEMENT.conf] URI: /allowed-path
--- FAIL: Test_Allow (0.14s)
panic: WAF [critical] Method is not allowed by policy [DELETE] ID: 911100 [@owasp_crs/REQUEST-911-METHOD-ENFORCEMENT.conf] URI: /allowed-path
 [recovered]
    panic: WAF [critical] Method is not allowed by policy [DELETE] ID: 911100 [@owasp_crs/REQUEST-911-METHOD-ENFORCEMENT.conf] URI: /allowed-path

[M4tteoP]

Hey, thanks for the report. It definitely deserves some more time to look at it. Most likely it comes into play the fact that allow action is part of the disruptive actions, therefore also that action is not performed running in DetectionOnly mode.

At first sight, I don't fully get the example: the DELETE method does not seem to be part of the allowed_methods, so I would expect to have 911100 logged in both Deny and Detect Mode 🤔

[jcchavezs]

Ping @fzipi as I helped him to implement allow action.

[agclark27]

Is there any reason not to set the ActionType of allow to ActionTypeFlow? Alternatively, one could create a special exception for the allow action in doEvaluate here. To the OP's comments, there is a lot of noise generated during detection-only testing that makes it hard to identify what rules may still need allowed exceptions before bringing the WAF into blocking mode.