🐛 Do not consider body unreadable when it's http.NoBody (#352)

* Do not consider body unreadable when it's http.NoBody

Fix #351

* 🐛 appsec: only drop unreadable bodies for methods that carry one

Gate the crowdsecAppsecUnreadableBodyBlock drop on the request method,
mirroring the reference lua-cs-bouncer METHODS_WITH_BODY list
(POST/PUT/PATCH/DELETE). Browser GETs over HTTP/3 never carry a
Content-Length (quic-go always wraps the stream in a non-nil body with
ContentLength -1), so they were all wrongly dropped; they are now
forwarded to appsec headers-only, while gRPC streams (always POST) stay
droppable.

Fix #351

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* 🍱 add test and remove useless comment

* 🍱 add test for GET with unreadable body

* 🍱 fix log

* 🍱 fix test

---------

Co-authored-by: mhx <mathieu@hanotaux.fr>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: maxlerebourg <maxlerebourg@gmail.com>

bouncer.go

@@ -732,7 +732,17 @@ func crowdsecQuery(bouncer *Bouncer, stringURL string, data []byte) ([]byte, err
 // a 403. This mirrors the reference lua-cs-bouncer behavior, which refuses to
 // read the body of an HTTP/2+ request that has no Content-Length.
 func isBodyUnreadable(httpReq *http.Request) bool {
-	return httpReq.Body != nil && httpReq.ProtoMajor >= 2 && httpReq.ContentLength < 0
+	return httpReq.Body != nil && httpReq.Body != http.NoBody && httpReq.ProtoMajor >= 2 && httpReq.ContentLength < 0
+}
+
+// isMethodWithBody used only when isBodyUnreadable returns true but the request method can't have body.
+func isMethodWithBody(method string) bool {
+	switch method {
+	case http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete:
+		return true
+	default:
+		return false
+	}
 }
 
 func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error {
@@ -744,8 +754,7 @@ func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error {
 	var req *http.Request
 	switch {
 	case isBodyUnreadable(httpReq):
-		if bouncer.appsecUnreadableBodyBlock {
+		if bouncer.appsecUnreadableBodyBlock && isMethodWithBody(httpReq.Method) {
-			// The caller (handleNextServeHTTP) logs this returned error with the IP.
 			return errors.New("appsecQuery:unreadableBody dropped")
 		}
 		req, _ = http.NewRequest(http.MethodGet, routeURL.String(), nil)

bouncer_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"reflect"
+	"strings"
 	"testing"
 	"text/template"
 	"time"
@@ -396,29 +397,27 @@ func (b blockingBody) Read(_ []byte) (int, error) {
 func (blockingBody) Close() error { return nil }
 
 func Test_isBodyUnreadable(t *testing.T) {
+	realBody := func() io.ReadCloser { return io.NopCloser(strings.NewReader("data")) }
 	tests := []struct {
 		name          string
 		protoMajor    int
 		contentLength int64
-		hasBody       bool
+		body          io.ReadCloser
 		want          bool
 	}{
-		{name: "http2 grpc stream without content-length", protoMajor: 2, contentLength: -1, hasBody: true, want: true},
+		{name: "http2 grpc stream without content-length", protoMajor: 2, contentLength: -1, body: realBody(), want: true},
-		{name: "http3 stream without content-length", protoMajor: 3, contentLength: -1, hasBody: true, want: true},
+		{name: "http3 stream without content-length", protoMajor: 3, contentLength: -1, body: realBody(), want: true},
-		{name: "http2 with content-length", protoMajor: 2, contentLength: 42, hasBody: true, want: false},
+		{name: "http2 with content-length", protoMajor: 2, contentLength: 42, body: realBody(), want: false},
-		{name: "http1.1 chunked without content-length", protoMajor: 1, contentLength: -1, hasBody: true, want: false},
+		{name: "http1.1 chunked without content-length", protoMajor: 1, contentLength: -1, body: realBody(), want: false},
-		{name: "http2 without body", protoMajor: 2, contentLength: -1, hasBody: false, want: false},
+		{name: "http2 without body", protoMajor: 2, contentLength: -1, body: nil, want: false},
+		{name: "http2 with http.NoBody", protoMajor: 2, contentLength: -1, body: http.NoBody, want: false},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			req, _ := http.NewRequest(http.MethodPost, "http://localhost", nil)
 			req.ProtoMajor = tt.protoMajor
 			req.ContentLength = tt.contentLength
-			if tt.hasBody {
+			req.Body = tt.body
-				req.Body = http.NoBody
-			} else {
-				req.Body = nil
-			}
 			if got := isBodyUnreadable(req); got != tt.want {
 				t.Errorf("isBodyUnreadable() = %v, want %v", got, tt.want)
 			}
@@ -513,3 +512,46 @@ func Test_appsecQuery_dropUnreadableBody(t *testing.T) {
 		t.Fatal("appsecQuery() blocked on a streaming request body (issue #323 regression)")
 	}
 }
+
+func newUnreadableGetRequest(done <-chan struct{}) *http.Request {
+	req, _ := http.NewRequest(http.MethodGet, "http://localhost/", blockingBody{done: done})
+	req.ProtoMajor = 3
+	req.ContentLength = -1
+	return req
+}
+
+// Test_appsecQuery_unreadableBodyGetNotDropped is a regression test for issue #351
+func Test_appsecQuery_unreadableBodyGetNotDropped(t *testing.T) {
+	appsecServer := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
+		rw.WriteHeader(http.StatusOK)
+	}))
+	defer appsecServer.Close()
+
+	appsecURL, _ := url.Parse(appsecServer.URL)
+	bouncer := &Bouncer{
+		appsecScheme:              appsecURL.Scheme,
+		appsecHost:                appsecURL.Host,
+		appsecPath:                "/",
+		appsecBodyLimit:           10485760,
+		appsecUnreadableBodyBlock: true,
+		httpAppsecClient:          appsecServer.Client(),
+		log:                       logger.New("INFO", ""),
+	}
+
+	done := make(chan struct{})
+	defer close(done)
+
+	finished := make(chan error, 1)
+	go func() {
+		finished <- appsecQuery(bouncer, "1.2.3.4", newUnreadableGetRequest(done))
+	}()
+
+	select {
+	case err := <-finished:
+		if err != nil {
+			t.Errorf("appsecQuery() on an HTTP/3 GET without content-length returned error: %v", err)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("appsecQuery() blocked on an HTTP/3 GET request body (issue #351 regression)")
+	}
+}

tests/e2e/mock/scenarios/appsec/dynamic.yml

@@ -18,7 +18,7 @@ http:
         bouncer:
           enabled: "true"
           # IP bouncing disabled — this scenario exercises AppSec only.
-          crowdsecMode: none
+          crowdsecMode: appsec
           crowdsecLapiScheme: http
           crowdsecLapiHost: "@@LAPI_HOST@@"
           crowdsecLapiKey: "@@APIKEY@@"

tests/e2e/mock/scenarios/appsec/run.sh

@@ -30,6 +30,15 @@ body() {
 
   echo "[$SCENARIO] request that send bad body before crowdsecAppsecBodyLimit must pass (AppSec 403)"
   assert_status "http://127.0.0.1:${WEB_PORT}/foo" 403 -H "X-Forwarded-For: 1.2.3.4" -X POST -d "a=0&______"
+
+  echo "[$SCENARIO] request http2 that send no body GET (AppSec 200)"
+  assert_status "http://127.0.0.1:${WEB_PORT}/foo" 200 -H "X-Forwarded-For: 1.2.3.4" --http2-prior-knowledge -H "Content-Length:"
+
+  echo "[$SCENARIO] request http2 that send unreadable body GET (AppSec 403)"
+  assert_status "http://127.0.0.1:${WEB_PORT}/foo" 403 -H "X-Forwarded-For: 1.2.3.4" --http2-prior-knowledge -H "Content-Length:" -d "test"
+
+  echo "[$SCENARIO] request http2 that send unreadable body POST (AppSec 403)"
+  assert_status "http://127.0.0.1:${WEB_PORT}/foo" 403 -H "X-Forwarded-For: 1.2.3.4" --http2-prior-knowledge -H "Content-Length:" -X POST -d "test"
 }
 
 run_scenario "$SCENARIO" "$HERE" body