diff --git a/caddyconfig/httpcaddyfile/serveroptions.go b/caddyconfig/httpcaddyfile/serveroptions.go index 005cf81d3..be0bc2b51 100644 --- a/caddyconfig/httpcaddyfile/serveroptions.go +++ b/caddyconfig/httpcaddyfile/serveroptions.go @@ -49,6 +49,7 @@ type serverOptions struct { MaxHeaderBytes int EnableFullDuplex bool ExpectedUnderscoreHeaders []string + ExpectedDotHeaders []string Protocols []string StrictSNIHost *bool TrustedProxiesRaw json.RawMessage @@ -226,6 +227,13 @@ func unmarshalCaddyfileServerOptions(d *caddyfile.Dispenser) (any, error) { } serverOpts.ExpectedUnderscoreHeaders = args + case "expected_dot_headers": + args := d.RemainingArgs() + if len(args) == 0 { + return nil, d.ArgErr() + } + serverOpts.ExpectedDotHeaders = args + case "log_credentials": if d.NextArg() { return nil, d.ArgErr() @@ -389,6 +397,7 @@ func applyServerOptions( server.MaxHeaderBytes = opts.MaxHeaderBytes server.EnableFullDuplex = opts.EnableFullDuplex server.ExpectedUnderscoreHeaders = opts.ExpectedUnderscoreHeaders + server.ExpectedDotHeaders = opts.ExpectedDotHeaders server.Protocols = opts.Protocols server.StrictSNIHost = opts.StrictSNIHost server.TrustedProxiesRaw = opts.TrustedProxiesRaw diff --git a/caddytest/integration/forwardauth_test.go b/caddytest/integration/forwardauth_test.go index 5f703bdfa..cf5005580 100644 --- a/caddytest/integration/forwardauth_test.go +++ b/caddytest/integration/forwardauth_test.go @@ -207,12 +207,12 @@ func TestForwardAuthCopyHeadersAuthResponseWins(t *testing.T) { } } -// 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. +// TestForwardAuthCopyHeadersUnderscoreAlias guards GHSA-f59h-q822-g45g and +// GHSA-49wc-4hcv-v58q: client-supplied `Remote_user`/`Remote.user` aliases +// of the copy_headers target `Remote-User` must be stripped before the +// auth route runs, otherwise a downstream CGI/FastCGI/PHP backend would +// fold all three names into the same HTTP_REMOTE_USER variable and the +// attacker would override the trusted identity. func TestForwardAuthCopyHeadersUnderscoreAlias(t *testing.T) { const wantRemoteUser = "alice" @@ -223,7 +223,7 @@ func TestForwardAuthCopyHeadersUnderscoreAlias(t *testing.T) { t.Cleanup(authSrv.Close) type received struct { - remoteUserHyphen, remoteUserUnderscore string + remoteUserHyphen, remoteUserUnderscore, remoteUserDot string } var ( mu sync.Mutex @@ -234,6 +234,7 @@ func TestForwardAuthCopyHeadersUnderscoreAlias(t *testing.T) { last = received{ remoteUserHyphen: r.Header.Get("Remote-User"), remoteUserUnderscore: strings.Join(r.Header["Remote_user"], ","), + remoteUserDot: strings.Join(r.Header["Remote.user"], ","), } mu.Unlock() fmt.Fprint(w, "ok") @@ -259,13 +260,15 @@ func TestForwardAuthCopyHeadersUnderscoreAlias(t *testing.T) { `, 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. + // Set the underscore and dot aliases via raw map access to bypass + // http.Header canonicalization, as an attacker would on the wire. req.Header["Remote_user"] = []string{"attacker"} + 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") + assert.Empty(t, last.remoteUserDot, "dot alias must be dropped") } diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 096619498..04ee7b18b 100644 --- a/modules/caddyhttp/app.go +++ b/modules/caddyhttp/app.go @@ -49,9 +49,9 @@ func init() { // defined by adding a good, default TLS connection policy. // // Similar to how other popular web servers work, incoming request header fields -// with underscores are ignored/dropped implicitly to mitigate security risks. -// Specific headers to allow can be explicitly configured using -// `expected_underscore_headers`. +// with underscores or dots are ignored/dropped implicitly to mitigate security +// risks. Specific headers to allow can be explicitly configured using +// `expected_underscore_headers` and `expected_dot_headers`. // // ### Placeholders // @@ -302,10 +302,13 @@ func (app *App) Provision(ctx caddy.Context) error { srv.ClientIPHeaders = []string{"X-Forwarded-For"} } - // precompute underscore header allowlist rules + // precompute underscore and dot header allowlist rules if err := srv.provisionUnderscoreHeaders(); err != nil { return fmt.Errorf("server %s: %v", srvName, err) } + if err := srv.provisionDotHeaders(); err != nil { + return fmt.Errorf("server %s: %v", srvName, err) + } // process each listener address for i := range srv.Listen { diff --git a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go index 4173df0d9..b292dd8ad 100644 --- a/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go +++ b/modules/caddyhttp/reverseproxy/fastcgi/fastcgi_test.go @@ -310,10 +310,11 @@ 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). +// spaces and dots) 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/dot +// filter in caddyhttp.Server.serveHTTP (see GHSA-f59h-q822-g45g, +// GHSA-49wc-4hcv-v58q). func TestHeaderNameReplacer(t *testing.T) { tests := []struct { in, want string @@ -323,6 +324,9 @@ func TestHeaderNameReplacer(t *testing.T) { // Underscores are preserved (the server has already dropped any // underscore-named headers when the filter is on). {"Remote_User", "Remote_User"}, + // Dots are preserved by this replacer too; the server-layer filter is + // what prevents a dotted alias (e.g. Remote.User) from reaching here. + {"Remote.User", "Remote.User"}, // Spaces are not rewritten because Go's HTTP parser rejects whitespace in // header field names. {"Foo Bar", "Foo Bar"}, diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go index 2dc7506c6..47c09ee12 100644 --- a/modules/caddyhttp/server.go +++ b/modules/caddyhttp/server.go @@ -137,9 +137,52 @@ type Server struct { // multiple values (repeated field), all values are dropped // as a safeguard against header injection. // + // If the same logical header name is also allowlisted (in dot form) + // in ExpectedDotHeaders, both spellings are kept independently. Only + // do this if you know your backend doesn't fold both forms onto the + // same variable name; PHP/CGI-style backends do (see + // ExpectedDotHeaders), so allowlisting both there reintroduces the + // ambiguity this filter exists to remove. + // + // A header name containing both an underscore and a dot (e.g. + // "webhook_user.id") is never matched by a prefix glob here, even + // one whose prefix matches, because the free-form suffix a glob + // allows can't be vetted for an embedded dot; only an exact (non-glob) + // entry for that literal spelling is honored. + // // TODO: This is an EXPERIMENTAL feature. Subject to change or removal. ExpectedUnderscoreHeaders []string `json:"expected_underscore_headers,omitempty"` + // A list of header field names containing dots that should be + // preserved instead of being dropped. By default, Caddy drops ALL + // headers with dots to prevent ambiguity with backends that fold + // dots to underscores when registering CGI-style variables, e.g. + // PHP's $_SERVER (GHSA-49wc-4hcv-v58q). When this list is + // configured, only the specified headers are kept; their + // hyphenated variants are actively dropped to prevent confusion. + // Entries are case-insensitive. A trailing "*" acts as a prefix + // glob (e.g., "webhook.*" matches any header starting with + // "webhook."). If an allowlisted header arrives with multiple + // values (repeated field), all values are dropped as a safeguard + // against header injection. + // + // Dotted headers are legal HTTP tokens and some non-CGI backends + // (e.g. Node.js, Go) use them as ordinary, unrelated header names. + // If your backend is one of those, this poses no risk. Only + // PHP/CGI/FastCGI-style backends fold '.', '_', and '-' onto the + // same variable name; avoid allowlisting both the dot and + // underscore form of the same logical name if such a backend is in + // the request path, since Caddy will not stop you and the backend + // may then see either value depending on map iteration order. + // + // A header name containing both a dot and an underscore is never + // matched by a prefix glob here, for the same reason described on + // ExpectedUnderscoreHeaders; only an exact (non-glob) entry for + // that literal spelling is honored. + // + // TODO: This is an EXPERIMENTAL feature. Subject to change or removal. + ExpectedDotHeaders []string `json:"expected_dot_headers,omitempty"` + // Routes describes how this server will handle requests. // Routes are executed sequentially. First a route's matchers // are evaluated, then its grouping. If it matches and has @@ -312,7 +355,12 @@ type Server struct { // precomputed underscore header allowlist (built during provisioning) underscoreExactAllow map[string]struct{} underscoreExactDrop map[string]struct{} - underscorePrefixRules []underscoreRule + underscorePrefixRules []aliasPrefixRule + + // precomputed dot header allowlist (built during provisioning) + dotExactAllow map[string]struct{} + dotExactDrop map[string]struct{} + dotPrefixRules []aliasPrefixRule // registered callback functions connStateFuncs []func(net.Conn, http.ConnState) @@ -321,30 +369,29 @@ type Server struct { onStopFuncs []func(context.Context) error // TODO: Experimental (Nov. 2023) } -// underscoreRule pairs a canonical underscore prefix with its hyphenated -// counterpart. Used for prefix-glob matching in the allowlist. -type underscoreRule struct { - allow string // canonical underscore form, e.g. "Webhook_" +// aliasPrefixRule pairs a canonical allowed prefix (underscore- or +// dot-named) with its hyphenated counterpart. Used for prefix-glob +// matching in the underscore/dot allowlists. +type aliasPrefixRule struct { + allow string // canonical allowed form, e.g. "Webhook_" or "Webhook." drop string // canonical hyphenated form, e.g. "Webhook-" } -// provisionUnderscoreHeaders validates the ExpectedUnderscoreHeaders -// entries and builds the precomputed maps and prefix rules used by -// the hot-path filter in serveHTTP. -func (s *Server) provisionUnderscoreHeaders() error { - if len(s.ExpectedUnderscoreHeaders) == 0 { - return nil - } +// provisionHeaderAliasAllowlist validates entries for a header-alias +// allowlist (ExpectedUnderscoreHeaders or ExpectedDotHeaders) and builds +// the precomputed maps and prefix rules used by the hot-path filter in +// serveHTTP. sep is the separator the entries must contain ('_' or '.'); +// directive is the Caddyfile/JSON name used in error messages. +func provisionHeaderAliasAllowlist(entries []string, sep rune, directive string) (exactAllow, exactDrop map[string]struct{}, prefixRules []aliasPrefixRule, err error) { + exactAllow = make(map[string]struct{}, len(entries)) + exactDrop = make(map[string]struct{}, len(entries)) - s.underscoreExactAllow = make(map[string]struct{}, len(s.ExpectedUnderscoreHeaders)) - s.underscoreExactDrop = make(map[string]struct{}, len(s.ExpectedUnderscoreHeaders)) - - for _, entry := range s.ExpectedUnderscoreHeaders { + for _, entry := range entries { // Reject non-ASCII bytes: Go's HTTP parser returns 400 for // non-ASCII header names, so such entries can never match. for i := 0; i < len(entry); i++ { if entry[i] >= 0x80 { - return fmt.Errorf("expected_underscore_headers: entry %q contains non-ASCII characters", entry) + return nil, nil, nil, fmt.Errorf("%s: entry %q contains non-ASCII characters", directive, entry) } } @@ -356,29 +403,55 @@ func (s *Server) provisionUnderscoreHeaders() error { // Reject entries with '*' not at the trailing position. if strings.ContainsRune(name, '*') { - return fmt.Errorf("expected_underscore_headers: entry %q has '*' in an invalid position (only a trailing '*' is allowed)", entry) + return nil, nil, nil, fmt.Errorf("%s: entry %q has '*' in an invalid position (only a trailing '*' is allowed)", directive, entry) } - // The name (without trailing '*') must contain at least one underscore. - if !strings.ContainsRune(name, '_') { - return fmt.Errorf("expected_underscore_headers: entry %q does not contain an underscore", entry) + // The name (without trailing '*') must contain at least one separator. + if !strings.ContainsRune(name, sep) { + return nil, nil, nil, fmt.Errorf("%s: entry %q does not contain a %q", directive, entry, sep) } canonAllow := http.CanonicalHeaderKey(name) - canonDrop := http.CanonicalHeaderKey(strings.ReplaceAll(name, "_", "-")) + canonDrop := http.CanonicalHeaderKey(strings.ReplaceAll(name, string(sep), "-")) if isGlob { - s.underscorePrefixRules = append(s.underscorePrefixRules, underscoreRule{ + prefixRules = append(prefixRules, aliasPrefixRule{ allow: canonAllow, drop: canonDrop, }) } else { - s.underscoreExactAllow[canonAllow] = struct{}{} - s.underscoreExactDrop[canonDrop] = struct{}{} + exactAllow[canonAllow] = struct{}{} + exactDrop[canonDrop] = struct{}{} } } - return nil + return exactAllow, exactDrop, prefixRules, nil +} + +// provisionUnderscoreHeaders validates the ExpectedUnderscoreHeaders +// entries and builds the precomputed maps and prefix rules used by +// the hot-path filter in serveHTTP. +func (s *Server) provisionUnderscoreHeaders() error { + if len(s.ExpectedUnderscoreHeaders) == 0 { + return nil + } + var err error + s.underscoreExactAllow, s.underscoreExactDrop, s.underscorePrefixRules, err = + provisionHeaderAliasAllowlist(s.ExpectedUnderscoreHeaders, '_', "expected_underscore_headers") + return err +} + +// provisionDotHeaders validates the ExpectedDotHeaders entries and +// builds the precomputed maps and prefix rules used by the hot-path +// filter in serveHTTP. +func (s *Server) provisionDotHeaders() error { + if len(s.ExpectedDotHeaders) == 0 { + return nil + } + var err error + s.dotExactAllow, s.dotExactDrop, s.dotPrefixRules, err = + provisionHeaderAliasAllowlist(s.ExpectedDotHeaders, '.', "expected_dot_headers") + return err } // isAllowedUnderscoreHeader reports whether key (a canonical header @@ -395,18 +468,41 @@ func (s *Server) isAllowedUnderscoreHeader(key string) bool { return false } +// isAllowedDotHeader reports whether key (a canonical header name +// containing a dot) is permitted by the allowlist. +func (s *Server) isAllowedDotHeader(key string) bool { + if _, ok := s.dotExactAllow[key]; ok { + return true + } + for _, rule := range s.dotPrefixRules { + if strings.HasPrefix(key, rule.allow) { + return true + } + } + return false +} + // isHyphenatedVariant reports whether key (a canonical header name -// without underscores) is the hyphenated variant of an allowlisted -// underscore header and should therefore be dropped. +// containing neither an underscore nor a dot) is the hyphenated variant +// of an allowlisted underscore or dot header and should therefore be +// dropped. func (s *Server) isHyphenatedVariant(key string) bool { if _, ok := s.underscoreExactDrop[key]; ok { return true } + if _, ok := s.dotExactDrop[key]; ok { + return true + } for _, rule := range s.underscorePrefixRules { if strings.HasPrefix(key, rule.drop) { return true } } + for _, rule := range s.dotPrefixRules { + if strings.HasPrefix(key, rule.drop) { + return true + } + } return false } @@ -604,44 +700,79 @@ 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). + // Drop headers whose names contain `_` or `.`: once FastCGI/CGI/FrankenPHP etc. + // rewrites `-` to `_` (and PHP additionally folds `.` to `_` when registering + // $_SERVER keys), an underscore or dot alias collides with the legitimate + // hyphenated header and can bypass `forward_auth copy_headers` + // (GHSA-f59h-q822-g45g, GHSA-49wc-4hcv-v58q). // - // When an allowlist is configured, only the listed headers are kept and - // their hyphenated variants are actively dropped to prevent ambiguity. - if len(s.ExpectedUnderscoreHeaders) == 0 { - for k := range r.Header { - if strings.ContainsRune(k, '_') { + // The two allowlists (ExpectedUnderscoreHeaders, ExpectedDotHeaders) are + // otherwise independent: each only keeps headers spelled with its own + // character, and either allowlist actively drops the plain-hyphenated + // variant of its entries. + for k := range r.Header { + hasUnderscore := strings.ContainsRune(k, '_') + hasDot := strings.ContainsRune(k, '.') + + switch { + case hasUnderscore && hasDot: + // A name containing both separators still collides with the + // hyphenated, underscore-only, and dot-only spellings once a + // CGI/FastCGI/PHP backend folds them onto the same variable + // name. A prefix glob can't vet its free-form suffix for an + // embedded "other" separator, so only an exact allowlist entry + // for this literal spelling is honored here. + _, underscoreOK := s.underscoreExactAllow[k] + _, dotOK := s.dotExactAllow[k] + if !underscoreOK && !dotOK { + delete(r.Header, k) + + if c := s.logger.Check(zapcore.DebugLevel, "dropping header containing both underscore and dot"); c != nil { + c.Write(zap.String("header", k)) + } + } else if n := len(r.Header[k]); n > 1 { + delete(r.Header, k) + + if c := s.logger.Check(zapcore.WarnLevel, "dropping allowlisted underscore/dot header with repeated values (possible spoofing)"); c != nil { + c.Write(zap.String("header", k), zap.Int("count", n)) + } + } + + case hasUnderscore: + if len(s.ExpectedUnderscoreHeaders) == 0 || !s.isAllowedUnderscoreHeader(k) { delete(r.Header, k) if c := s.logger.Check(zapcore.DebugLevel, "dropping header containing underscore"); c != nil { c.Write(zap.String("header", k)) } - } - } - } else { - for k := range r.Header { - if strings.ContainsRune(k, '_') { - if !s.isAllowedUnderscoreHeader(k) { - delete(r.Header, k) - - if c := s.logger.Check(zapcore.DebugLevel, "dropping header containing underscore"); c != nil { - c.Write(zap.String("header", k)) - } - } else if n := len(r.Header[k]); n > 1 { - delete(r.Header, k) - - if c := s.logger.Check(zapcore.WarnLevel, "dropping allowlisted underscore header with repeated values (possible spoofing)"); c != nil { - c.Write(zap.String("header", k), zap.Int("count", n)) - } - } - } else if s.isHyphenatedVariant(k) { + } else if n := len(r.Header[k]); n > 1 { delete(r.Header, k) - if c := s.logger.Check(zapcore.DebugLevel, "dropping hyphenated variant of expected underscore header"); c != nil { + if c := s.logger.Check(zapcore.WarnLevel, "dropping allowlisted underscore header with repeated values (possible spoofing)"); c != nil { + c.Write(zap.String("header", k), zap.Int("count", n)) + } + } + + case hasDot: + if len(s.ExpectedDotHeaders) == 0 || !s.isAllowedDotHeader(k) { + delete(r.Header, k) + + if c := s.logger.Check(zapcore.DebugLevel, "dropping header containing dot"); c != nil { c.Write(zap.String("header", k)) } + } else if n := len(r.Header[k]); n > 1 { + delete(r.Header, k) + + if c := s.logger.Check(zapcore.WarnLevel, "dropping allowlisted dot header with repeated values (possible spoofing)"); c != nil { + c.Write(zap.String("header", k), zap.Int("count", n)) + } + } + + case s.isHyphenatedVariant(k): + delete(r.Header, k) + + if c := s.logger.Check(zapcore.DebugLevel, "dropping hyphenated variant of expected underscore/dot header"); c != nil { + c.Write(zap.String("header", k)) } } } diff --git a/modules/caddyhttp/server_test.go b/modules/caddyhttp/server_test.go index 454794a84..08b621557 100644 --- a/modules/caddyhttp/server_test.go +++ b/modules/caddyhttp/server_test.go @@ -527,6 +527,51 @@ func TestServer_serveHTTP_LogsDroppedUnderscoreHeader(t *testing.T) { assert.Contains(t, buf.String(), `"header":"Remote_user"`) } +// TestServer_serveHTTP_DropsDotHeader guards GHSA-49wc-4hcv-v58q: a +// dot-named alias (e.g. `Remote.user`) of a hyphenated header must be +// dropped too, since PHP's $_SERVER registration folds `.` to `_` just +// like CGI/FastCGI folds `-` to `_`. +func TestServer_serveHTTP_DropsDotHeader(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_LogsDroppedDotHeader verifies each dropped dotted +// header is emitted at debug level, same as the underscore case. +func TestServer_serveHTTP_LogsDroppedDotHeader(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 dot"`) + assert.Contains(t, buf.String(), `"header":"Remote.user"`) +} + // --- Allowlist: exact match --- func TestServer_serveHTTP_AllowlistKeepsExactMatch(t *testing.T) { @@ -777,6 +822,277 @@ func TestServer_serveHTTP_LiteralAsteriskInHeader(t *testing.T) { assert.Equal(t, "val", got.Get("Webhook_*")) } +// --- Allowlist: dot headers (ExpectedDotHeaders), independent of underscore --- + +// TestServer_serveHTTP_DotAllowlistKeepsExactMatch verifies a dot-named +// header explicitly allowlisted via ExpectedDotHeaders survives, mirroring +// the underscore allowlist behavior. +func TestServer_serveHTTP_DotAllowlistKeepsExactMatch(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedDotHeaders: []string{"user.id"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionDotHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["User.id"] = []string{"zeus"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Equal(t, "zeus", got.Get("User.id")) +} + +// TestServer_serveHTTP_DotAllowlistDropsHyphenatedVariant verifies the +// hyphenated variant of an allowlisted dot header is dropped, same as for +// the underscore allowlist. +func TestServer_serveHTTP_DotAllowlistDropsHyphenatedVariant(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedDotHeaders: []string{"user.id"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionDotHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["User.id"] = []string{"zeus"} + req.Header.Set("User-Id", "attacker") + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Equal(t, "zeus", got.Get("User.id")) + assert.NotContains(t, *got, "User-Id") +} + +// TestServer_serveHTTP_DotAllowlistDropsUnderscoreVariant verifies that when +// only the dot form is allowlisted, an underscore-form header of the same +// logical name is NOT independently kept: ExpectedDotHeaders does not grant +// any allowance to underscore-named headers, so it falls through to the +// default underscore behavior (dropped, since ExpectedUnderscoreHeaders is +// unset). +func TestServer_serveHTTP_DotAllowlistDropsUnderscoreVariant(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedDotHeaders: []string{"user.id"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionDotHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["User.id"] = []string{"zeus"} + req.Header["User_id"] = []string{"attacker"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Equal(t, "zeus", got.Get("User.id")) + assert.NotContains(t, *got, "User_id") +} + +// TestServer_serveHTTP_DotAllowlistDropsUnlisted verifies an unlisted +// dot-form header is dropped just like its underscore-form counterpart. +func TestServer_serveHTTP_DotAllowlistDropsUnlisted(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedDotHeaders: []string{"user.id"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionDotHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["User.id"] = []string{"zeus"} + req.Header["Remote.user"] = []string{"attacker"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Equal(t, "zeus", got.Get("User.id")) + assert.NotContains(t, *got, "Remote.user") +} + +// TestServer_serveHTTP_DotPrefixGlobKeepsMatch verifies prefix-glob +// matching works for dot headers the same way it does for underscore +// headers. +func TestServer_serveHTTP_DotPrefixGlobKeepsMatch(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedDotHeaders: []string{"webhook.*"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionDotHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["Webhook.event"] = []string{"push"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Equal(t, "push", got.Get("Webhook.event")) +} + +// --- Both allowlists configured together --- + +// TestServer_serveHTTP_BothAllowlistsCoexistForDifferentNames verifies +// unrelated underscore and dot allowlist entries can coexist without +// interfering with each other. +func TestServer_serveHTTP_BothAllowlistsCoexistForDifferentNames(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedUnderscoreHeaders: []string{"user_id"}, + ExpectedDotHeaders: []string{"webhook.event"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionUnderscoreHeaders()) + require.NoError(t, s.provisionDotHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["User_id"] = []string{"zeus"} + req.Header["Webhook.event"] = []string{"push"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Equal(t, "zeus", got.Get("User_id")) + assert.Equal(t, "push", got.Get("Webhook.event")) +} + +// TestServer_serveHTTP_BothAllowlistsPermitSameLogicalName verifies Caddy +// does not enforce mutual exclusion between the two allowlists: an +// operator whose backend doesn't fold '.' and '_' onto the same variable +// (e.g. a Node.js or Go app behind reverse_proxy, not a CGI/FastCGI/PHP +// backend) may legitimately want both spellings of the same name kept as +// distinct headers. It's their responsibility to know whether that's safe +// for their backend; see the doc comments on ExpectedUnderscoreHeaders and +// ExpectedDotHeaders. +func TestServer_serveHTTP_BothAllowlistsPermitSameLogicalName(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedUnderscoreHeaders: []string{"user_id"}, + ExpectedDotHeaders: []string{"user.id"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionUnderscoreHeaders()) + require.NoError(t, s.provisionDotHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["User_id"] = []string{"alice"} + req.Header["User.id"] = []string{"bob"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Equal(t, "alice", got.Get("User_id")) + assert.Equal(t, "bob", got.Get("User.id")) +} + +// --- Headers with both underscore and dot in the same name --- + +// TestServer_serveHTTP_DropsMixedUnderscoreDotHeaderByDefault verifies a +// header spelled with both an underscore and a dot (e.g. "Webhook_user.id") +// is dropped by default, same as either separator alone. +func TestServer_serveHTTP_DropsMixedUnderscoreDotHeaderByDefault(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["Webhook_user.id"] = []string{"attacker"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.NotContains(t, *got, "Webhook_user.id") +} + +// TestServer_serveHTTP_PrefixGlobDoesNotLeakMixedHeader guards against a +// prefix glob unintentionally admitting a mixed-separator header: an +// underscore prefix rule like "webhook_*" matches "Webhook_user.id" via +// plain string prefix, but the embedded dot still collides at a +// CGI/FastCGI/PHP backend the same way a bare dot-named header would, so +// it must not slip through just because it also happens to start with an +// allowlisted underscore prefix. +func TestServer_serveHTTP_PrefixGlobDoesNotLeakMixedHeader(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedUnderscoreHeaders: []string{"webhook_*"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionUnderscoreHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["Webhook_user.id"] = []string{"attacker"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.NotContains(t, *got, "Webhook_user.id") +} + +// TestServer_serveHTTP_ExactAllowlistPermitsMixedHeader verifies that an +// operator who explicitly vetted the exact mixed-separator spelling (not a +// glob) can still allowlist it. +func TestServer_serveHTTP_ExactAllowlistPermitsMixedHeader(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedUnderscoreHeaders: []string{"webhook_user.id"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionUnderscoreHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["Webhook_user.id"] = []string{"ok"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.Equal(t, "ok", got.Get("Webhook_user.id")) +} + +// TestServer_serveHTTP_ExactAllowlistDropsMixedHeaderRepeatedValues verifies +// the repeated-value guard also applies to an exactly-allowlisted mixed +// header. +func TestServer_serveHTTP_ExactAllowlistDropsMixedHeaderRepeatedValues(t *testing.T) { + got := &http.Header{} + s := &Server{ + logger: zap.NewNop(), + ExpectedUnderscoreHeaders: []string{"webhook_user.id"}, + primaryHandlerChain: HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + *got = r.Header.Clone() + return nil + }), + } + require.NoError(t, s.provisionUnderscoreHeaders()) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req.Header["Webhook_user.id"] = []string{"ok", "injected"} + + require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) + assert.NotContains(t, *got, "Webhook_user.id") +} + // --- Combined allowlist --- func TestServer_serveHTTP_ExactAndPrefixCoexist(t *testing.T) { @@ -895,7 +1211,7 @@ func TestServer_serveHTTP_LogsHyphenatedVariantDrop(t *testing.T) { require.NoError(t, s.serveHTTP(httptest.NewRecorder(), req)) assert.Contains(t, buf.String(), `"level":"debug"`) - assert.Contains(t, buf.String(), `"msg":"dropping hyphenated variant of expected underscore header"`) + assert.Contains(t, buf.String(), `"msg":"dropping hyphenated variant of expected underscore/dot header"`) assert.Contains(t, buf.String(), `"header":"User-Id"`) } @@ -954,6 +1270,52 @@ func TestServer_provisionUnderscoreHeaders_DeduplicatesSilently(t *testing.T) { assert.Len(t, s.underscoreExactAllow, 1) } +func TestServer_provisionDotHeaders_EmptyListIsNoOp(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{}} + assert.NoError(t, s.provisionDotHeaders()) +} + +func TestServer_provisionDotHeaders_RejectsNoDot(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{"content-type"}} + assert.Error(t, s.provisionDotHeaders()) +} + +func TestServer_provisionDotHeaders_RejectsBareWildcard(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{"*"}} + assert.Error(t, s.provisionDotHeaders()) +} + +func TestServer_provisionDotHeaders_RejectsMidGlob(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{"f*oo.bar"}} + assert.Error(t, s.provisionDotHeaders()) +} + +func TestServer_provisionDotHeaders_RejectsLeadingGlob(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{"*.foo"}} + assert.Error(t, s.provisionDotHeaders()) +} + +func TestServer_provisionDotHeaders_RejectsNonASCII(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{"uşer.id"}} + assert.Error(t, s.provisionDotHeaders()) +} + +func TestServer_provisionDotHeaders_ValidExact(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{"user.id"}} + assert.NoError(t, s.provisionDotHeaders()) +} + +func TestServer_provisionDotHeaders_ValidGlob(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{"webhook.*"}} + assert.NoError(t, s.provisionDotHeaders()) +} + +func TestServer_provisionDotHeaders_DeduplicatesSilently(t *testing.T) { + s := &Server{ExpectedDotHeaders: []string{"user.id", "user.id"}} + require.NoError(t, s.provisionDotHeaders()) + assert.Len(t, s.dotExactAllow, 1) +} + // 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