From be1d4fb43c9e90e0e14a5a729dff6f0324b09e0b Mon Sep 17 00:00:00 2001 From: andrewstellman Date: Sat, 4 Apr 2026 14:51:48 -0400 Subject: [PATCH 1/2] Fix Accept-Encoding substring matching in compress middleware --- middleware/compress.go | 21 ++++++++++++-- middleware/compress_test.go | 55 +++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/middleware/compress.go b/middleware/compress.go index 4e46f70a..213cefff 100644 --- a/middleware/compress.go +++ b/middleware/compress.go @@ -239,9 +239,26 @@ func (c *Compressor) selectEncoder(h http.Header, w io.Writer) (io.Writer, strin func matchAcceptEncoding(accepted []string, encoding string) bool { for _, v := range accepted { - if strings.Contains(v, encoding) { - return true + // Split off any parameters (e.g. ";q=0.5") and trim whitespace + name, params, _ := strings.Cut(strings.TrimSpace(v), ";") + name = strings.TrimSpace(name) + + if !strings.EqualFold(name, encoding) { + continue } + + // Check for explicit q=0, which means the client refused this encoding + if params != "" { + params = strings.TrimSpace(params) + if strings.HasPrefix(params, "q=") { + qval := strings.TrimSpace(params[2:]) + if qval == "0" || qval == "0." || qval == "0.0" || qval == "0.00" || qval == "0.000" { + return false + } + } + } + + return true } return false } diff --git a/middleware/compress_test.go b/middleware/compress_test.go index 028343c8..4ebd03be 100644 --- a/middleware/compress_test.go +++ b/middleware/compress_test.go @@ -169,6 +169,61 @@ func TestCompressorWildcards(t *testing.T) { } } +// TestMatchAcceptEncoding verifies that Accept-Encoding negotiation uses +// proper token matching rather than substring matching. The current +// implementation uses strings.Contains, which incorrectly matches: +// - "gzip;q=0" as gzip (client explicitly refused gzip) +// - "xgzip" as gzip (not a real encoding, but contains "gzip" as a substring) +func TestMatchAcceptEncoding(t *testing.T) { + tests := []struct { + name string + accepted []string + encoding string + want bool + }{ + { + name: "exact match", + accepted: []string{"gzip"}, + encoding: "gzip", + want: true, + }, + { + name: "q=0 means refused", + accepted: []string{"gzip;q=0"}, + encoding: "gzip", + want: false, + }, + { + name: "substring should not match", + accepted: []string{"xgzip"}, + encoding: "gzip", + want: false, + }, + { + name: "encoding with positive q-value", + accepted: []string{"gzip;q=0.5"}, + encoding: "gzip", + want: true, + }, + { + name: "encoding with whitespace", + accepted: []string{" gzip "}, + encoding: "gzip", + want: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := matchAcceptEncoding(tc.accepted, tc.encoding) + if got != tc.want { + t.Errorf("matchAcceptEncoding(%v, %q) = %v, want %v", + tc.accepted, tc.encoding, got, tc.want) + } + }) + } +} + func testRequestWithAcceptedEncodings(t *testing.T, ts *httptest.Server, method, path string, encodings ...string) (*http.Response, string) { req, err := http.NewRequest(method, ts.URL+path, nil) if err != nil { From b3089d1c7d083501d634352dc7956e15d0fad197 Mon Sep 17 00:00:00 2001 From: andrewstellman Date: Sat, 4 Apr 2026 15:00:04 -0400 Subject: [PATCH 2/2] Fix Recoverer upgrade detection to handle case-insensitive tokenized Connection header --- middleware/recoverer.go | 15 ++++++++- middleware/recoverer_test.go | 63 ++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/middleware/recoverer.go b/middleware/recoverer.go index 81342dfa..70f677ef 100644 --- a/middleware/recoverer.go +++ b/middleware/recoverer.go @@ -36,7 +36,7 @@ func Recoverer(next http.Handler) http.Handler { PrintPrettyStack(rvr) } - if r.Header.Get("Connection") != "Upgrade" { + if !headerContainsToken(r.Header, "Connection", "Upgrade") { w.WriteHeader(http.StatusInternalServerError) } } @@ -201,3 +201,16 @@ func (s prettyStack) decorateSourceLine(line string, useColor bool, num int) (st return buf.String(), nil } + +// headerContainsToken checks whether a comma-separated, case-insensitive +// HTTP header contains a specific token (RFC 7230 §3.2.6). +func headerContainsToken(h http.Header, headerName, token string) bool { + for _, v := range h[http.CanonicalHeaderKey(headerName)] { + for _, s := range strings.Split(v, ",") { + if strings.EqualFold(strings.TrimSpace(s), token) { + return true + } + } + } + return false +} diff --git a/middleware/recoverer_test.go b/middleware/recoverer_test.go index e50f47cd..71145161 100644 --- a/middleware/recoverer_test.go +++ b/middleware/recoverer_test.go @@ -41,6 +41,69 @@ func TestRecoverer(t *testing.T) { t.Fatal("First func call line should start with ->.") } +// TestRecovererUpgradeConnectionDetection verifies that the Recoverer does not +// write a 500 status code when the Connection header indicates an upgrade. +// HTTP headers are case-insensitive (RFC 7230 §3.2) and the Connection header +// can contain multiple comma-separated tokens (e.g. "keep-alive, Upgrade"). +// The current implementation only matches the exact string "Upgrade". +func TestRecovererUpgradeConnectionDetection(t *testing.T) { + tests := []struct { + name string + connHeader string + expect500 bool + }{ + { + name: "exact Upgrade is not 500", + connHeader: "Upgrade", + expect500: false, + }, + { + name: "lowercase upgrade is not 500", + connHeader: "upgrade", + expect500: false, + }, + { + name: "Upgrade in token list is not 500", + connHeader: "keep-alive, Upgrade", + expect500: false, + }, + { + name: "no Connection header is 500", + connHeader: "", + expect500: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + oldRecovererErrorWriter := recovererErrorWriter + defer func() { recovererErrorWriter = oldRecovererErrorWriter }() + recovererErrorWriter = &bytes.Buffer{} + + r := chi.NewRouter() + r.Use(Recoverer) + r.Get("/", panickingHandler) + + w := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/", nil) + if err != nil { + t.Fatal(err) + } + if tc.connHeader != "" { + req.Header.Set("Connection", tc.connHeader) + } + + r.ServeHTTP(w, req) + + got500 := w.Code == http.StatusInternalServerError + if got500 != tc.expect500 { + t.Errorf("Connection: %q — got status %d, expected 500=%v", + tc.connHeader, w.Code, tc.expect500) + } + }) + } +} + func TestRecovererAbortHandler(t *testing.T) { defer func() { rcv := recover()