🐛 appsec: do not buffer unreadable (gRPC/HTTP2) request bodies (#332)

* 🐛 fix appsec silently 403-ing gRPC streams with unreadable body A bidirectional gRPC stream is an HTTP/2 request with no Content-Length whose body never reaches EOF. Since #321 removed the ContentLength guard, appsecQuery buffered it with io.ReadAll, which blocked until the request timed out and was turned into a 403 (issue #323). The backend was never reached (OriginStatus:0). Mirror the reference lua-cs-bouncer behaviour: detect an unreadable body (ProtoMajor >= 2 && ContentLength < 0) and, instead of buffering it, forward the request to Appsec with headers only. Add a new CrowdsecAppsecDropUnreadableBody option (default false) that mirrors the reference APPSEC_DROP_UNREADABLE_BODY: when true, such requests are blocked outright instead of forwarded without their body. Readable HTTP/1.1 bodies are still buffered and inspected, so the bypass closed by #321 stays closed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 🚨 appsec: satisfy linters (gocritic ifElseChain, misspell) Rewrite the body-handling if/else chain in appsecQuery as a switch (gocritic) and use US spelling "behavior" (misspell). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 🔇 appsec: drop redundant unreadable-body debug log Address review on #332: the caller (handleNextServeHTTP) already logs the returned error with the request IP, so the inner Debug line duplicated it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 🍱 increase gocyclo * 🍱 fix lint --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: maxlerebourg <maxlerebourg@gmail.com>
2026-07-21 11:38:59 +02:00 · 2026-07-01 12:44:58 +02:00
parent 1c1672c856
commit d32f271195
5 changed files with 280 additions and 116 deletions
@@ -2,15 +2,19 @@ package crowdsec_bouncer_traefik_plugin //nolint:revive,stylecheck

 import (
 	"context"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"reflect"
 	"testing"
 	"text/template"
+	"time"

 	cache "github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin/pkg/cache"
 	configuration "github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin/pkg/configuration"
 	ip "github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin/pkg/ip"
+	logger "github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin/pkg/logger"
 )

 func TestServeHTTP(t *testing.T) {
@@ -376,3 +380,136 @@ func TestCaptchaMethodBasedLogic(t *testing.T) {
 		})
 	}
 }
+
+// blockingBody simulates a request body that never reaches EOF, like a
+// bidirectional gRPC stream that keeps its body open for the whole life of
+// the connection. Reading from it blocks until the test is done.
+type blockingBody struct {
+	done <-chan struct{}
+}
+
+func (b blockingBody) Read(_ []byte) (int, error) {
+	<-b.done
+	return 0, io.EOF
+}
+
+func (blockingBody) Close() error { return nil }
+
+func Test_isBodyUnreadable(t *testing.T) {
+	tests := []struct {
+		name          string
+		protoMajor    int
+		contentLength int64
+		hasBody       bool
+		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},
+	}
+	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
+			}
+			if got := isBodyUnreadable(req); got != tt.want {
+				t.Errorf("isBodyUnreadable() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+// newStreamingRequest builds an HTTP/2 request whose body never reaches EOF,
+// like a bidirectional gRPC stream (issue #323).
+func newStreamingRequest(done <-chan struct{}) *http.Request {
+	req, _ := http.NewRequest(http.MethodPost, "http://localhost/signalexchange.SignalExchange/ConnectStream", blockingBody{done: done})
+	req.Header.Set("Content-Type", "application/grpc")
+	req.ProtoMajor = 2
+	req.ContentLength = -1
+	return req
+}
+
+// Test_appsecQuery_streamingDoesNotBlock is a regression test for issue #323:
+// a gRPC streaming request whose body never reaches EOF must not be buffered
+// (io.ReadAll would block until timeout and wrongly produce a 403). The appsec
+// query must complete promptly, inspecting headers only.
+func Test_appsecQuery_streamingDoesNotBlock(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,
+		appsecUnreachableBlock: true,
+		appsecFailureBlock:     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", newStreamingRequest(done))
+	}()
+
+	select {
+	case err := <-finished:
+		if err != nil {
+			t.Errorf("appsecQuery() on streaming request returned error: %v", err)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("appsecQuery() blocked on a streaming request body (issue #323 regression)")
+	}
+}
+
+// Test_appsecQuery_dropUnreadableBody verifies that, when configured to do so,
+// a request with an unreadable body is dropped (blocked) instead of forwarded
+// without its body, mirroring the reference APPSEC_DROP_UNREADABLE_BODY option.
+func Test_appsecQuery_dropUnreadableBody(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", newStreamingRequest(done))
+	}()
+
+	select {
+	case err := <-finished:
+		if err == nil {
+			t.Error("appsecQuery() expected an error to block the request, got nil")
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("appsecQuery() blocked on a streaming request body (issue #323 regression)")
+	}
+}