From 31874263f2720a84a995abe094538d227ebf80c8 Mon Sep 17 00:00:00 2001 From: Daniel Berteaud Date: Fri, 24 Jul 2026 20:49:43 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Do=20not=20consider=20body=20unr?= =?UTF-8?q?eadable=20when=20it's=20http.NoBody=20(#352)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * :bento: add test and remove useless comment * :bento: add test for GET with unreadable body * :bento: fix log * :bento: fix test --------- Co-authored-by: mhx Co-authored-by: Claude Fable 5 Co-authored-by: maxlerebourg --- bouncer.go | 15 ++++- bouncer_test.go | 64 +++++++++++++++++---- tests/e2e/mock/scenarios/appsec/dynamic.yml | 2 +- tests/e2e/mock/scenarios/appsec/run.sh | 9 +++ 4 files changed, 75 insertions(+), 15 deletions(-) diff --git a/bouncer.go b/bouncer.go index a98cf18..a1746e8 100644 --- a/bouncer.go +++ b/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 { - // The caller (handleNextServeHTTP) logs this returned error with the IP. + if bouncer.appsecUnreadableBodyBlock && isMethodWithBody(httpReq.Method) { return errors.New("appsecQuery:unreadableBody dropped") } req, _ = http.NewRequest(http.MethodGet, routeURL.String(), nil) diff --git a/bouncer_test.go b/bouncer_test.go index 483d640..8eb03ed 100644 --- a/bouncer_test.go +++ b/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: "http3 stream without content-length", protoMajor: 3, contentLength: -1, hasBody: true, want: true}, - {name: "http2 with content-length", protoMajor: 2, contentLength: 42, hasBody: true, want: false}, - {name: "http1.1 chunked without content-length", protoMajor: 1, contentLength: -1, hasBody: true, want: false}, - {name: "http2 without body", protoMajor: 2, contentLength: -1, hasBody: false, want: false}, + {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, body: realBody(), want: true}, + {name: "http2 with content-length", protoMajor: 2, contentLength: 42, body: realBody(), 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, 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 = http.NoBody - } else { - req.Body = nil - } + req.Body = tt.body 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)") + } +} diff --git a/tests/e2e/mock/scenarios/appsec/dynamic.yml b/tests/e2e/mock/scenarios/appsec/dynamic.yml index 0bb38de..f02b286 100644 --- a/tests/e2e/mock/scenarios/appsec/dynamic.yml +++ b/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@@" diff --git a/tests/e2e/mock/scenarios/appsec/run.sh b/tests/e2e/mock/scenarios/appsec/run.sh index 7b92113..5427e5c 100755 --- a/tests/e2e/mock/scenarios/appsec/run.sh +++ b/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