Files
Kévin DunglasandKağan Küçük 857d5731f3 caddyhttp: optimise header access and fix encoding negotiation (#8025)
* perf: bypass header key normalization for canonical literals

http.Header's Get, Set, Add and Del run textproto.CanonicalMIMEHeaderKey
on every call. When the key is a canonical literal the result is the key
itself, so the scan is pure overhead. Access the map directly instead.

Only hot, allocation-free paths are converted, and only where the key is
a canonical literal. Call sites taking a key from config, a template or
the request itself still go through the normalizing methods, since they
need it for correctness.

allHeaderValues and lastHeaderValue dropped their http.CanonicalHeaderKey
call for the same reason: all callers pass canonical literals.

benchstat, Apple M5 Pro, go1.27.1:

    PrepareRequest-15      659.4n ± 2%  ->  457.7n ± 2%  -30.58% (p=0.000 n=12)
    AcceptedEncodings-15   330.8n ± 5%  ->  309.4n ± 7%   -6.44% (p=0.019 n=10)

    ResponseWriterWriteHeader/event-stream-15    1007.5n ± 3%  ->  732.5n ± 1%  -27.30%
    ResponseWriterWriteHeader/informational-15     60.0n ± 3%  ->   48.0n ± 2%  -20.01%
    ResponseWriterWriteHeader/not-modified-15     214.3n ± 6%  ->  175.3n ± 4%  -18.20%
    ResponseWriterWriteHeader/connect-15           60.2n ± 3%  ->   51.9n ± 4%  -13.85%
    ResponseWriterWriteHeader/html-15             206.5n ± 4%  ->  197.9n ± 2%   -4.19%
    geomean                                                                    -14.38%

Allocations are unchanged.

* tests: verify http.Header map keys are canonical

Indexing http.Header directly skips the canonicalization Get/Set/Add/Del
perform, which also puts the key outside what CI checks. canonicalheader
only inspects method calls. staticcheck's SA1008 covers reads, but stops
descending as soon as an assignment writes to a header, so it misses the
write, the delete builtin and the read-modify-write append. A key that is
not canonical in those positions is a silent lookup miss, not a compile
error.

Walk the loaded packages and verify every string constant used as a key in
an http.Header map operation matches http.CanonicalHeaderKey. A helper that
takes the key from its caller would hide the literal again, so a parameter
named with a canonical prefix opts its arguments into the same check.

The WebSocket headers Caddy deliberately writes with RFC 6455 casing are
allowed explicitly; they are only ever written out, never looked up against
a parsed header map.

Covers 64 operations, runs in under a second, and promotes golang.org/x/tools
from an indirect to a direct dependency at the version already in the module
graph.

* headercheck: resolve aliases of http.Header

* headercheck: check http.Header composite literal keys

* headercheck: note overlap with canonicalheader

* headercheck: drop parameter-name check

Helpers now take the indexed values, so every key sits in an index expression the analyzer and the linter already check.

* headercheck: scope RFC 6455 exceptions to their call sites

A key-based allow-list also accepted reads such as h["Sec-WebSocket-Key"], which miss because parsed headers are canonical. Honor //nolint:canonicalheader on the line instead, which the linter will use too.

* encode: read every Accept-Encoding field line

AcceptedEncodings read only the first field line, so a client that split
the list across several lines had the rest ignored. RFC 9110 section 5.3
treats repeated list-field lines as one comma-joined list. Iterate over
the items of every line instead of joining them, and skip empty items,
which used to yield an empty encoding. file_server's precompressed
selection uses the same function and is fixed with it.

Fixes https://github.com/caddyserver/caddy/issues/8272

Co-authored-by: Kağan Küçük <kaankck@gmail.com>

* encode: read every Cache-Control and Content-Encoding field line

Only the first field line was read, but RFC 9110 section 5.3 makes
multiple lines of a list-based field equivalent to one comma-joined
value:

- Cache-Control: no-transform on a later line was ignored, so the
  request or response was compressed anyway.
- Content-Encoding: an empty first line followed by a real coding let
  the handler encode the body a second time.

* encode: parse Cache-Control no-transform per RFC 9111

The substring match missed No-Transform, though RFC 9111 section 5.2 compares directive names case-insensitively, and fired on text that is not the directive: longer tokens such as x-no-transform-ext, quoted arguments such as community="no-transform", and semicolon-separated values, which are not valid Cache-Control syntax. Split the field on commas outside quoted strings and match only a well-formed no-transform element.

* encode: keep accepted encoding preferences on the stack

AcceptedEncodings counted commas to size the preference slice, and the variable capacity put it on the heap. A constant capacity covers typical Accept-Encoding headers and lets the compiler keep the slice on the stack; append still moves it to the heap for longer ones. BenchmarkAcceptedEncodings: 151.8ns -> 120.5ns (-21%), 192 -> 64 B/op, 2 -> 1 allocs/op (n=8).

* encode: build the encoded ETag without fmt

fmt.Sprintf boxed both arguments and parsed the format on every encoded response with a strong ETag. Concatenation: 47ns -> 16.5ns, 56 -> 24 B, 3 -> 1 allocs.

* reverseproxy: access response Server and Via headers directly

finalizeResponse runs on every proxied response; skip the key normalization as prepareRequest already does on the request side.

* encode: access headers directly in Write, WriteHeader and Close

These run once per response; convert the remaining literal-key Get, Set and Add calls like the rest of the handler.

* encode: match Cache-Control no-transform with httpguts

httpguts.HeaderValuesContainsToken already compares list elements case-insensitively across field lines, so the hand-written list parser is unnecessary. A no-transform inside a comma-separated quoted argument now also disables encoding, which only errs toward not transforming.

* encode: inline the no-transform check

isEncodeAllowed only wrapped one httpguts call. Its unit test now drives the response directive through ServeHTTP instead.

---------

Co-authored-by: Kağan Küçük <kaankck@gmail.com>
2026-10-29 23:19:50 +10:00

180 lines
5.4 KiB
Go

// Copyright 2015 Matthew Holt and The Caddy Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// Package headercheck verifies that every http.Header key written directly
// into the map uses net/http's canonical form.
//
// Hot paths index http.Header directly to skip the canonicalization that
// Get/Set/Add/Del perform on every call. That trades a linted call for an
// unlinted map operation: the canonicalheader linter only inspects method
// calls, and staticcheck's SA1008 stops descending as soon as an assignment
// writes to a header, so neither covers a direct write or a delete. A key
// that is not canonical there is a silent lookup miss, not a compile error.
//
// This covers four shapes the enabled linters miss:
//
// h["x-key"] = []string{"v"} // direct write
// http.Header{"x-key": {"v"}} // composite literal
// delete(h, "x-key") // delete builtin
// h["x-key"] = append(h["x-key"], "v) // read-modify-write
package headercheck
import (
"go/ast"
"go/constant"
"go/token"
"go/types"
"net/http"
"strings"
"testing"
"golang.org/x/tools/go/packages"
)
// headerType is the type whose map operations must use canonical keys.
const headerType = "net/http.Header"
// nolintDirective marks a deliberate non-canonical key on its line, such as
// the RFC 6455 spellings written for case-sensitive upstreams.
const nolintDirective = "//nolint:canonicalheader"
// TODO: delete this package once golangci-lint ships https://github.com/lasiar/canonicalheader/pull/90;
// the linter honors the same //nolint:canonicalheader directives.
func TestHeaderKeysAreCanonical(t *testing.T) {
cfg := &packages.Config{
Mode: packages.NeedName | packages.NeedSyntax | packages.NeedTypes |
packages.NeedTypesInfo | packages.NeedDeps | packages.NeedImports,
Dir: "../..",
Tests: false,
}
pkgs, err := packages.Load(cfg, "./...")
if err != nil {
t.Fatalf("loading packages: %v", err)
}
if packages.PrintErrors(pkgs) > 0 {
t.Fatal("packages failed to load; see errors above")
}
var checked int
for _, pkg := range pkgs {
for _, file := range pkg.Syntax {
checked += checkFile(t, pkg, file)
}
}
// Guard against the walk silently covering nothing, e.g. if package
// loading is reconfigured and stops yielding type information.
if checked == 0 {
t.Fatal("no http.Header map operations found; the check is not running")
}
t.Logf("checked %d direct http.Header map operations", checked)
}
// checkFile reports the number of direct http.Header map operations examined.
func checkFile(t *testing.T, pkg *packages.Package, file *ast.File) int {
var checked int
exempt := make(map[int]bool)
for _, group := range file.Comments {
for _, c := range group.List {
if strings.HasPrefix(c.Text, nolintDirective) {
exempt[pkg.Fset.Position(c.Slash).Line] = true
}
}
}
report := func(pos token.Pos, key, form string) {
canonical := http.CanonicalHeaderKey(key)
if key == canonical || exempt[pkg.Fset.Position(pos).Line] {
return
}
t.Errorf("%s: %s uses non-canonical key %q; want %q",
pkg.Fset.Position(pos), form, key, canonical)
}
// keyOf returns the constant string value of an index or argument
// expression, so both literals and named constants are covered.
keyOf := func(expr ast.Expr) (string, bool) {
tv, ok := pkg.TypesInfo.Types[expr]
if !ok || tv.Value == nil || tv.Value.Kind() != constant.String {
return "", false
}
return constant.StringVal(tv.Value), true
}
isHeader := func(expr ast.Expr) bool {
tv, ok := pkg.TypesInfo.Types[expr]
if !ok || tv.Type == nil {
return false
}
named, ok := types.Unalias(tv.Type).(*types.Named)
if !ok {
return false
}
obj := named.Obj()
return obj != nil && obj.Pkg() != nil &&
obj.Pkg().Path()+"."+obj.Name() == headerType
}
ast.Inspect(file, func(n ast.Node) bool {
switch node := n.(type) {
// h["Key"], including the assignment form h["Key"] = ...
// that SA1008 skips.
case *ast.IndexExpr:
if !isHeader(node.X) {
return true
}
key, ok := keyOf(node.Index)
if !ok {
return true
}
checked++
report(node.Index.Pos(), key, "http.Header index")
// http.Header{"Key": ...}, which stores the key verbatim.
case *ast.CompositeLit:
if !isHeader(node) {
return true
}
for _, elt := range node.Elts {
kv, ok := elt.(*ast.KeyValueExpr)
if !ok {
continue
}
key, ok := keyOf(kv.Key)
if !ok {
continue
}
checked++
report(kv.Key.Pos(), key, "http.Header literal")
}
// delete(h, "Key"), which no enabled linter inspects.
case *ast.CallExpr:
if ident, ok := node.Fun.(*ast.Ident); ok && ident.Name == "delete" && len(node.Args) == 2 {
if _, ok := pkg.TypesInfo.Uses[ident].(*types.Builtin); ok && isHeader(node.Args[0]) {
if key, ok := keyOf(node.Args[1]); ok {
checked++
report(node.Args[1].Pos(), key, "delete on http.Header")
}
}
}
}
return true
})
return checked
}