From 3eb8e48ff052e1ad16d88c683672c306d2077a11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Fri, 29 May 2026 19:37:17 +0200 Subject: [PATCH] Merge commit from fork * feat: drop headers with underscore in their names * feat: Caddyfile binding and tests for underscore-in-header drop Add the `allow_underscore_in_headers` global server option, refine the doc comment, and cover the filter end-to-end: server-level unit tests (drop, opt-out, debug log, RFC-7230 space rejection), a fastcgi unit test for the trimmed header name replacer, and forward_auth integration tests for both the default-drop and opt-out paths. * remove allow_underscore_in_headers option for now --- caddytest/integration/forwardauth_test.go | 65 ++++++++++++++++ .../caddyhttp/reverseproxy/fastcgi/fastcgi.go | 2 +- .../reverseproxy/fastcgi/fastcgi_test.go | 24 ++++++ modules/caddyhttp/server.go | 13 ++++ modules/caddyhttp/server_test.go | 77 +++++++++++++++++++ 5 files changed, 180 insertions(+), 1 deletion(-) diff --git a/caddytest/integration/forwardauth_test.go b/caddytest/integration/forwardauth_test.go index 513c80906..5f703bdfa 100644 --- a/caddytest/integration/forwardauth_test.go +++ b/caddytest/integration/forwardauth_test.go @@ -22,6 +22,8 @@ import ( "sync" "testing" + "github.com/stretchr/testify/assert" + "github.com/caddyserver/caddy/v2/caddytest" ) @@ -204,3 +206,66 @@ func TestForwardAuthCopyHeadersAuthResponseWins(t *testing.T) { t.Errorf("X-User-Role: want %q, got %q", wantUserRole, gotRole) } } + +// TestForwardAuthCopyHeadersUnderscoreAlias guards GHSA-f59h-q822-g45g: +// a client-supplied `Remote_user` alias of the copy_headers target +// `Remote-User` must be stripped before the auth route runs, otherwise +// a downstream CGI/FastCGI backend would fold both names into the same +// HTTP_REMOTE_USER variable and the attacker would override the trusted +// identity. +func TestForwardAuthCopyHeadersUnderscoreAlias(t *testing.T) { + const wantRemoteUser = "alice" + + authSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Remote-User", wantRemoteUser) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(authSrv.Close) + + type received struct { + remoteUserHyphen, remoteUserUnderscore string + } + var ( + mu sync.Mutex + last received + ) + backendSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + last = received{ + remoteUserHyphen: r.Header.Get("Remote-User"), + remoteUserUnderscore: strings.Join(r.Header["Remote_user"], ","), + } + mu.Unlock() + fmt.Fprint(w, "ok") + })) + t.Cleanup(backendSrv.Close) + + tester := caddytest.NewTester(t) + tester.InitServer(fmt.Sprintf(` + { + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + grace_period 1ns + } + http://localhost:9080 { + forward_auth %s { + uri / + copy_headers Remote-User + } + reverse_proxy %s + } + `, strings.TrimPrefix(authSrv.URL, "http://"), strings.TrimPrefix(backendSrv.URL, "http://")), "caddyfile") + + req, _ := http.NewRequest(http.MethodGet, "http://localhost:9080/", nil) + // Set the underscore alias via raw map access to bypass http.Header + // canonicalization, as an attacker would on the wire. + req.Header["Remote_user"] = []string{"attacker"} + tester.AssertResponse(req, http.StatusOK, "ok") + + mu.Lock() + defer mu.Unlock() + assert.Equal(t, wantRemoteUser, last.remoteUserHyphen, "trusted Remote-User must reach the backend") + assert.Empty(t, last.remoteUserUnderscore, "underscore alias must be dropped") +} diff --git a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go index f91394e58..9b602ee5d 100644 --- a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go +++ b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go @@ -507,7 +507,7 @@ var tlsProtocolStrings = map[uint16]string{ tls.VersionTLS13: "TLSv1.3", } -var headerNameReplacer = strings.NewReplacer(" ", "_", "-", "_") +var headerNameReplacer = strings.NewReplacer("-", "_") // Interface guards var ( diff --git a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go index 4977ae998..2b22c813e 100644 --- a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go +++ b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go @@ -304,6 +304,30 @@ func TestSplitPosUnicodeSecurityRegression(t *testing.T) { } } +// TestHeaderNameReplacer asserts the CGI header-to-env normalization rule: +// hyphens are mapped to underscores while every other character (including +// spaces) is passed through. Spaces are not RFC 7230 tokens, so they cannot +// reach this function from the wire; the only header names that survive +// untouched at the server layer are sanitized by the underscore filter in +// caddyhttp.Server.serveHTTP (see GHSA-f59h-q822-g45g). +func TestHeaderNameReplacer(t *testing.T) { + tests := []struct { + in, want string + }{ + {"X-Forwarded-For", "X_Forwarded_For"}, + {"Remote-User", "Remote_User"}, + // Underscores are preserved (the server has already dropped any + // underscore-named headers when the filter is on). + {"Remote_User", "Remote_User"}, + // Spaces are not rewritten because Go's HTTP parser rejects whitespace in + // header field names. + {"Foo Bar", "Foo Bar"}, + } + for _, tt := range tests { + assert.Equal(t, tt.want, headerNameReplacer.Replace(tt.in), "input %q", tt.in) + } +} + // TestSplitPosSecurityRegressionUnicodeBypass guards against the FrankenPHP // advisories GHSA-3g8v-8r37-cgjm (uninitialized match flag on inner non-ASCII // byte) and GHSA-v4h7-cj44-8fc8 (Unicode equivalence via search.IgnoreCase diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go index 66f93989b..0479af83d 100644 --- a/modules/caddyhttp/server.go +++ b/modules/caddyhttp/server.go @@ -494,6 +494,19 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) error { } } + // Drop headers whose names contain `_`: once FastCGI/CGI/FrankenPHP etc. rewrites `-` to + // `_`, an underscore alias collides with the legitimate hyphenated header + // and can bypass `forward_auth copy_headers` (GHSA-f59h-q822-g45g). + for k := range r.Header { + if strings.ContainsRune(k, '_') { + delete(r.Header, k) + + if c := s.logger.Check(zapcore.DebugLevel, "dropping header containing underscore"); c != nil { + c.Write(zap.String("header", k)) + } + } + } + // execute the primary handler chain return s.primaryHandlerChain.ServeHTTP(w, r) } diff --git a/modules/caddyhttp/server_test.go b/modules/caddyhttp/server_test.go index eecb392e4..fb6b2d49c 100644 --- a/modules/caddyhttp/server_test.go +++ b/modules/caddyhttp/server_test.go @@ -1,16 +1,20 @@ package caddyhttp import ( + "bufio" "bytes" "context" "io" + "net" "net/http" "net/http/httptest" "net/netip" + "strings" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/zap" "go.uber.org/zap/zapcore" ) @@ -478,6 +482,79 @@ func TestServer_DetermineTrustedProxy_MatchRightMostUntrustedSkippingTrusted(t * assert.Equal(t, clientIP, "45.54.45.54") } +// TestServer_serveHTTP_DropsUnderscoreHeader covers GHSA-f59h-q822-g45g: an +// underscore-named alias (e.g. `Remote_user`) of a hyphenated header must be +// dropped before any handler runs. +func TestServer_serveHTTP_DropsUnderscoreHeader(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["X-Real-Header"] = []string{"ok"} + req.Header["Remote_user"] = []string{"attacker"} + req.Header["Remote_groups"] = []string{"admin"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.NotContains(t, *got, "Remote_user") + assert.NotContains(t, *got, "Remote_groups") + assert.Equal(t, "ok", got.Get("X-Real-Header")) +} + +// TestServer_serveHTTP_LogsDroppedUnderscoreHeader verifies each dropped +// header is emitted at debug level so operators can diagnose unexpectedly +// missing headers without spamming the log on adversarial traffic. +func TestServer_serveHTTP_LogsDroppedUnderscoreHeader(t *testing.T) { + var buf bytes.Buffer + s := &Server{ + logger: testLogger(buf.Write), + primaryHandlerChain: HandlerFunc(func(http.ResponseWriter, *http.Request) error { + return nil + }), + } + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["Remote_user"] = []string{"attacker"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Contains(t, buf.String(), `"level":"debug"`) + assert.Contains(t, buf.String(), `"msg":"dropping header containing underscore"`) + assert.Contains(t, buf.String(), `"header":"Remote_user"`) +} + +// TestServer_SpaceInHeaderNameReturnsBadRequest documents why the underscore +// filter does not also strip space-named headers: Go's HTTP parser rejects a +// space in a field name with 400 before any handler runs, so such a request +// can never reach Caddy's pipeline. +func TestServer_SpaceInHeaderNameReturnsBadRequest(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("handler must not be reached; got headers %v", r.Header) + })) + t.Cleanup(srv.Close) + + addr := strings.TrimPrefix(srv.URL, "http://") + conn, err := net.DialTimeout("tcp", addr, 5*time.Second) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + require.NoError(t, conn.SetDeadline(time.Now().Add(5*time.Second))) + + _, err = conn.Write([]byte("GET / HTTP/1.1\r\n" + + "Host: " + addr + "\r\n" + + "Remote User: attacker\r\n" + + "Connection: close\r\n\r\n")) + require.NoError(t, err) + + resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + require.NoError(t, err) + t.Cleanup(func() { _ = resp.Body.Close() }) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) +} + func TestServer_DetermineTrustedProxy_MatchRightMostUntrustedFirst(t *testing.T) { localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8")