When a handle_response status code did not evaluate to an integer, the
handler returned a 500 without closing the upstream response body,
leaking the connection.
(cherry picked from commit dccce45660)
* 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>
(cherry picked from commit 857d5731f3)
When a forward proxy is configured (network_proxy, forward_proxy_url or
HTTP_PROXY), net/http wraps dial errors in a *net.OpError with Op
"proxyconnect". tryAgain used a type assertion to detect DialError, so it
missed the wrapped error and treated it as a failure after the request
was sent. Non-GET requests were then not retried, even though no
connection to the proxy was ever established.
Use errors.As so a DialError is recognized anywhere in the error chain.
Errors that do not contain a DialError are unaffected, so requests that
may have reached the proxy or upstream are still not retried by default.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit f355b8687d)
With request_buffers or response_buffers set to unlimited, the error
from reading the body was discarded and whatever had been read was
treated as the complete body. A request cut short by request_body
max_size was forwarded truncated with a 200, and a truncated chunked
upstream response was sent to the client as complete.
Handle a read error the same way the limited case already does.
(cherry picked from commit fc3d83400b)
* reverseproxy: Only retry responses for expressions that use response data
Since #7569 every lb_retry_match set with an expression is evaluated against
upstream responses. An expression that only tests the request, such as
{http.request.method} == "POST", then matched every successful response:
the request was sent again to the next upstreams and the client got a 502
once retries ran out. Only treat expressions referencing {rp.*} placeholders
as response conditions.
* reverseproxy: Only count placeholders set from the response as response data
* reverseproxy: Read response placeholders from the checked expression
The expression matcher records the placeholder names its checked CEL
expression reads, so retry_match finds direct ph() calls and ignores
escaped placeholder text in string literals. A placeholder name only
known at evaluation time counts as response data.
* reverseproxy: Match response placeholder names exactly
Only the header placeholders are matched by prefix now, and both
upstream latency placeholders are listed. MatchExpression.Placeholders
returns a copy, so callers cannot change the provisioned matcher.
(cherry picked from commit 7c7456353e)
* reverseproxy: Keep own headers after 1xx responses
Since #6656 the 1xx handler clears the whole header map after writing the
informational response, which also drops headers set before proxying (for
example by the header handler). Restore those after clearing, as #5091 did.
* reverseproxy: Use maps.Copy to restore own headers
* reverseproxy: Check the 103 reaches the client in the 1xx headers test
(cherry picked from commit 519be12306)
Wrap backend dial failures as reverseproxy.DialError so non-GET requests can be retried when no connection was made.
AI-assisted implementation and test; reviewed by zemamba.
Signed-off-by: zemamba <zemamba@gmail.com>
(cherry picked from commit 221ebc4503)
[backport: test adapted to the 2.11 finalizeResponse signature, which
lacks the upstream address argument added by #7649]
(cherry picked from commit f7eeb1d65c)
* Move websocket header normalization later
A regression since #6621, these headers can be rewritten by cloneHeaders
and by .Add from user and transport ops. Moving this closer to where
it's round-tripped feels like the nicest solution.
I've put the harness I used to reproduce the issue at
https://codeberg.org/nfreya/caddy-websocket-headers-repro
* Integration test for normalized websocket headers
---------
Co-authored-by: Zen Dodd <mail@steadytao.com>
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
Build the Via request and response headers with strconv-based
concatenation instead of fmt.Sprintf, avoiding fmt's reflection and
formatting overhead on every proxied request/response. Same single
allocation, ~40% faster for that construction in isolation.
| before (fmt) | after (strconv) |
| sec/op | sec/op vs base |
PrepareRequest-8 | 6.136µ ± 11% | 4.491µ ± 33% -26.80% (p=0.007 n=10)|
B/op: 1008 -> 1008 (unchanged)
allocs/op: 12 -> 12 (unchanged)
No behavior change. Adds BenchmarkPrepareRequest, as per policy, exercising the
request preparation path.
* reverseproxy: Add ability to clear dynamic upstreams cache during retries
This is an optional interface for dynamic upstream modules to implement if they cache results.
TODO: More documentation; this is an experiment.
* Add some godoc
* Export interface; update godoc
This refactors the initial approach in PR #7281, replacing the UsagePool
with a dedicated package-level sync.Map and atomic.Int64 to track
in-flight requests without global lock contention.
It also introduces a lookup map in the admin API to fix a potential
O(n^2) iteration over upstreams, ensuring that draining upstreams
are correctly exposed across config reloads without leaking memory.
Co-authored-by: Y.Horie <u5.horie@gmail.com>
reverseproxy: optimize in-flight tracking and admin API
- Replaced sync.RWMutex with sync.Map and atomic.Int64 to avoid lock contention under high RPS.
- Introduced a lookup map in the admin API to fix a potential O(n^2) iteration over upstreams.
When a request arrives via a Unix domain socket (RemoteAddr == "@"),
net.SplitHostPort fails, causing addForwardedHeaders to strip all
X-Forwarded-* headers even when the connection is trusted via
trusted_proxies_unix.
Handle Unix socket connections before parsing RemoteAddr: if untrusted,
strip headers for security; if trusted, let clientIP remain empty (no
peer IP for a Unix socket hop) and fall through to the shared header
logic, preserving the existing XFF chain without appending a spurious
entry.
Amp-Thread-ID: https://ampcode.com/threads/T-019c4225-a0ad-7283-ac56-e2c01eae1103
Co-authored-by: Amp <amp@ampcode.com>
* capture the buffered body once, then reset clonedReq.Body before each retry
* no copy
* keep receiver name
* set the buf to nil after extraction and only return it to pool if not nil
---------
Co-authored-by: WeidiDeng <weidi_deng@icloud.com>
* chore: upgrade .golangci.yml and workflow to v2
run `golangci-lint fmt`
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* run `golangci-lint run --fix`
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* more lint fixes
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* bring back comments to .golangci.yml
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* appease the linter some more
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* oops
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* use embedded structs
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* use embedded structs where they were used before
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* disable rule `-QF1006`
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* missed a spot
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
---------
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Currently if we extract the DialInfo from a Request Context during an active health check, then the Upstream in the DialInfo is nil.
This PR attempts to set the Upstream to a sensible value, based on wether or not the Upstream has been overriden in the active health check's config.
* reverse_proxy: re-add healthy upstreams metric
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* lint
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
---------
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* buffer requests for fastcgi by default
* fix import cycle
* fix the return value of bufferedBody
* more comments about fastcgi buffering
---------
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
* reverse proxy: rewrite requests and responses for websocket over http2
* delete protocol pseudo-header
* modify cloned requests
* set request variable to track if it's a h2 websocket
* use request bodu
* rewrite request body
* use WebSocket instead of Websocket in the headers
* use logger check for zap loggers
* fix lint
i.e. Revert commit f5dce84a70
Two years ago, the patch in #4952 was a seemingly necessary way to fix an issue (sort of an edge case), but it broke other more common use cases (see #6666).
Now, as of #6669, it seems like the original issue can no longer be replicated, so we are reverting that patch, because it was incorrect anyway.
If it turns out the original issue returns, a more proper patch may be in #6669 (even if used as a baseline for a future fix). A potential future fix could be an opt-in setting.
* caddyhttp: Add `MatchWithError` to replace SetVar hack
* Error in IP matchers on TLS handshake not complete
* Use MatchWithError everywhere possible
* Move implementations to MatchWithError versions
* Looser interface checking to allow fallback
* CEL factories can return RequestMatcherWithError
* Clarifying comment since it's subtle that an err is returned
* Return 425 Too Early status in IP matchers
* Keep AnyMatch signature the same for now
* Apparently Deprecated can't be all-uppercase to get IDE linting
* Linter
D'Ali Ben Macha
Kévin Dunglas
Adarsh Kumar
Mohamed A. Abdel Fattah
Krish Singh Hura
Jay Hemnani
zemamba
Saleh
WeidiDeng
freya
Julien Voisin
Saleh
Dionis Ramadani
James Hartig
Matt Holt
Daniil Sivak
Zen Dodd
Tom Paulus
Francis Lavoie
Paulo Henrique
XYenon
Petr
Anthony Biondo
joemicky
Mohammed Al Sahaf
Jesper Brix Rosenkilde
bt90