mirror of
https://github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin.git
synced 2026-09-04 21:18:52 +02:00
🐛 do not drop bodyless DELETE over HTTP/3
Fix #385. Over HTTP/3 a bodyless request cannot be told apart from one carrying an unreadable body. Go's HTTP/2 server can make that distinction -- it only sets ContentLength to -1 when the client left the stream open for DATA frames, so a request that ends at the headers keeps ContentLength 0 and isBodyUnreadable stays false (x/net/http2/server.go, `bodyOpen := !f.StreamEnded()`). quic-go has no equivalent: http3/server_conn.go assigns req.Body unconditionally and http3/headers.go defaults ContentLength to -1 whenever the Content-Length header is absent, so every bodyless HTTP/3 request looks unreadable regardless of method. With DELETE in isMethodWithBody, that turns an ordinary fetch(url, {method: "DELETE"}) into a 403 on HTTP/3 while the same call succeeds on HTTP/2. dani reported this on #352 before it was merged. A DELETE body is legal but has no defined semantics (RFC 9110 9.3.5), and the gRPC streams #323/#332 guard against are always POST, so dropping DELETE from the list costs no protection there. Note this diverges from lua-cs-bouncer's METHODS_WITH_BODY, which still lists DELETE; the same false positive likely applies there. Adds Test_appsecQuery_unreadableBodyMethods, which pins the behaviour for all seven methods and fails on the previous code for DELETE. isMethodWithBody had no direct coverage before.
This commit is contained in:
+1
-1
@@ -746,7 +746,7 @@ func isBodyUnreadable(httpReq *http.Request) bool {
|
|||||||
// isMethodWithBody used only when isBodyUnreadable returns true but the request method can't have body.
|
// isMethodWithBody used only when isBodyUnreadable returns true but the request method can't have body.
|
||||||
func isMethodWithBody(method string) bool {
|
func isMethodWithBody(method string) bool {
|
||||||
switch method {
|
switch method {
|
||||||
case http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete:
|
case http.MethodPost, http.MethodPut, http.MethodPatch:
|
||||||
return true
|
return true
|
||||||
default:
|
default:
|
||||||
return false
|
return false
|
||||||
|
|||||||
@@ -600,3 +600,66 @@ func Test_appsecQuery_reusesConnection(t *testing.T) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func newUnreadableRequest(method string, done <-chan struct{}) *http.Request {
|
||||||
|
req, _ := http.NewRequest(method, "http://localhost/api/admin/reservations/8fff14a2", blockingBody{done: done})
|
||||||
|
req.ProtoMajor = 3
|
||||||
|
req.ContentLength = -1
|
||||||
|
return req
|
||||||
|
}
|
||||||
|
|
||||||
|
func Test_appsecQuery_unreadableBodyMethods(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)
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
method string
|
||||||
|
wantDropped bool
|
||||||
|
}{
|
||||||
|
{method: http.MethodGet, wantDropped: false},
|
||||||
|
{method: http.MethodHead, wantDropped: false},
|
||||||
|
{method: http.MethodOptions, wantDropped: false},
|
||||||
|
{method: http.MethodDelete, wantDropped: false},
|
||||||
|
{method: http.MethodPost, wantDropped: true},
|
||||||
|
{method: http.MethodPut, wantDropped: true},
|
||||||
|
{method: http.MethodPatch, wantDropped: true},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.method, func(t *testing.T) {
|
||||||
|
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", newUnreadableRequest(tt.method, done))
|
||||||
|
}()
|
||||||
|
|
||||||
|
select {
|
||||||
|
case err := <-finished:
|
||||||
|
if tt.wantDropped && err == nil {
|
||||||
|
t.Errorf("appsecQuery() on an unreadable-body %s: expected the request to be dropped, got nil", tt.method)
|
||||||
|
}
|
||||||
|
if !tt.wantDropped && err != nil {
|
||||||
|
t.Errorf("appsecQuery() on a bodyless HTTP/3 %s returned error: %v", tt.method, err)
|
||||||
|
}
|
||||||
|
case <-time.After(2 * time.Second):
|
||||||
|
t.Fatalf("appsecQuery() blocked on an unreadable %s body", tt.method)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user