* caddyhttp: presize buffers in hot paths to reduce allocations
Right-size three per-request allocations that previously grew from a
nil/empty backing array, causing repeated reallocation:
- CleanPath: pre-Grow the strings.Builder used when slash collapsing is
disabled, instead of letting it reallocate as runes are appended
- headers: presize the rewritten value slice in HeaderOps.ApplyTo's Set
loop instead of appending from nil
- reverseproxy: presize the weights slice in WeightedRoundRobinSelection
.Select
This commit also adds benchmarks exercising each path. Measured with benchstat
with count=20 and GOMAXPROCS=1, with p=0.000:
sec/op B/op allocs/op
CleanPathNoCollapse 1013.4n -> 828.6n (-18.23%) 120->64 (-46.67%) 4->1 (-75.00%)
HeaderOpsApplyToSet 1262.0n -> 955.8n (-24.27%) 255->152 (-40.39%) 9->6 (-33.33%)
WeightedRoundRobinSelect 215.8n -> 200.3n ( -7.20%) 112->96 (-14.29%) 2->2 (N/A)
* caddyhttp: presize strings.Builder in query rewrite and path matcher
Two builders whose output size is bounded by a known input length were
growing from empty, causing repeated reallocation. Seed each with a
capacity hint:
- rewrite.buildQueryString: pre-Grow to len(qs) (runs per request when
rewrite query manipulation is configured)
- MatchPath.matchPatternWithEscapeSequence: pre-Grow to len(escapedPath)
(reached only when a path pattern contains % escape sequences)
Adds a benchmark for buildQueryString, measured with benchstat:
BuildQueryString:
- sec/op 965.2n -> 743.3n (-22.99%)
- B/op 248->80 (-67.74%)
- allocs/op 5->1 (-80.00%)
The builder previously reallocated four times per call. The matcher path
is cold and not separately benchmarked.
* chore: add benchmark headers
---------
Signed-off-by: Julien Voisin <jvoisin@users.noreply.github.com>
(cherry picked from commit c8492563ee)
With fewer than two weights, WeightedRoundRobinSelection returned the
first upstream without checking that it is available.
(cherry picked from commit aeea18be86)
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)
* fastcgi: keep the php_fastcgi directory redirect on the same origin
* fastcgi: build the directory redirect from the escaped original segment
(cherry picked from commit 32bba15459)
* caddytls: validate curve and protocol names, compare protocol IDs
* caddytls: Add final newline to tests and drop duplicate protocol comment
(cherry picked from commit 71c5812cfe)
* fastcgi: Stop response bodies at the declared Content-Length
* fastcgi: Use a cancellable test context in proxy tests
(cherry picked from commit b058e3f3f6)
* reverseproxy: strip hop-by-hop headers when copying response headers
copy_response_headers copied the upstream header map before
finalizeResponse could strip hop-by-hop fields. Clone and strip
first, so include and exclude cannot forward them, and leave the
upstream response unchanged for later handlers in the route.
Co-authored-by: Grok 4.7 <noreply@x.ai>
(cherry picked from commit 258863f6f6)
* reverseproxy: close backend body and hijacked socket on upgrade setup failures
When an upstream response indicates an HTTP upgrade (101 Switching Protocols),
handleUpgradeResponse takes ownership of res.Body and subsequently hijacks
the client connection.
However, if any setup check fails prior to starting handleUpgradeTunnel
(such as mismatched upgrade headers, non-writable body assertion failure,
hijack unsupported, or handshake buffer flush errors), the function returned
early without closing res.Body or the hijacked client socket.
Because calling Hijack() permanently disables net/http's automatic connection
cleanup, and raw TCP sockets are not closed by the garbage collector, these
early failure exits leaked file descriptors into CLOSE_WAIT/ESTABLISHED.
Fix this by:
1. Adding a deferred cleanup guard that closes res.Body and any hijacked
front connection unless the tunnel successfully takes ownership.
2. Wrapping connections with onceCloseConn to ensure idempotent closure
while preserving optional CloseWrite (TCP half-close) support.
3. Adding TestUpgradeResponseCleanupOnFailure to verify cleanup.
* reverseproxy: test post-hijack cleanup and remove once-close wrappers
Cover handshake flush and buffered backend-write failures after successful hijacking, checking that both connections close. Keep setup-failure cleanup while using the original connections directly.
Code and tests were AI-assisted using an OpenAI coding assistant in Pi.
(cherry picked from commit 62bc606433)
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)
* fastcgi: derive SCRIPT_FILENAME from the normalized request path
Path matchers evaluate a normalized request path — MatchPath applies
normalizeWindowsPath and then CleanPath before comparing — but the FastCGI
transport split r.URL.Path verbatim. Caddy does not rewrite r.URL.Path itself,
so the two disagreed whenever the path contained dot segments after the split
token: /admin.php/../../other.php was matched as /other.php while
SCRIPT_FILENAME resolved to <root>/admin.php, and on Windows the same held for
/admin.php\..\other.php. A route gated by a path matcher could therefore execute
a script that matcher never evaluated.
php_fastcgi generates a rewrite route that normally normalizes the path first,
but that route can fail to match, so the transport establishes the invariant
itself rather than inheriting it. The reason it can fail to match is addressed
in the following commit; this change does not depend on it.
caddyhttp.NormalizedPath is added so a consumer acting on a path a matcher has
already judged derives it exactly as the matcher did, rather than reassembling
the two steps and risking drift.
Scope — two divergences of the same class are NOT addressed here:
* SCRIPT_NAME remains a prefix of the request path, so a matcher naming a
script exactly (path /admin.php) is still reachable via PATH_INFO at
/admin.php/anything. Matchers protecting a script need a trailing wildcard
regardless; that is inherent to CGI, not to this bug.
* On Windows, NTFS 8.3 short names are not rejected here. fileserver's
ServeHTTP rejects them (and ':' for ADS) precisely because matchers cannot
see through them, so GET /ADMIN~1.PHP is judged as /admin~1.php by a
path matcher while NTFS resolves SCRIPT_FILENAME to admin.php. Closing that
means sharing fileserver's hasWindowsShortName across packages, which is a
larger change than this fix; it is reported alongside this one and a patch
is available on request.
Side effects: PATH_INFO and DOCUMENT_URI are now derived from the normalized
path, so repeated slashes are merged and "." segments removed (/index.php//a//b
yields PATH_INFO=/a/b rather than //a//b), and on Windows per-component trailing
dots and spaces are folded. An application routing on a raw PATH_INFO will see
this. An empty URL.Path stays empty rather than becoming ".". REQUEST_URI is
unchanged and still carries the original target.
Tests: a buildEnv regression over literal and percent-encoded dot segments,
covering that legitimate PATH_INFO is preserved and an empty path stays empty; a
Windows-only counterpart whose cases each fail without the Windows
normalization; and an end-to-end test of a basic_auth-protected php_fastcgi
route against a FastCGI responder reporting the SCRIPT_FILENAME it received. The
end-to-end test uses `index off` so no rewrite route exists, ensuring it
exercises the transport rather than the rewrite.
* fileserver: make first_exist_fallback fall back when the file is absent
The try_policy first_exist_fallback is documented to match unconditionally,
falling back to the last try_files entry when no earlier candidate exists. It
did not: candidates are built with fs.Glob, which returns only paths that exist,
so an absent final entry left the list empty and the fallback branch — nested
inside the loop over that list — was unreachable.
php_fastcgi hard-codes this policy so its generated rewrite route always runs;
when the fallback failed, that rewrite was silently skipped.
Keep the pattern itself as a candidate when nothing globbed. This also aligns
Linux with Windows, where globResults is populated without an existence check
and the fallback already fired.
Behaviour change worth calling out: on a php_fastcgi site whose root has no
index.php (or no file matching the configured index), the rewrite now always
runs, so a request for a path that exists neither on the filesystem nor as a PHP
script is proxied to the backend as <root>/index.php instead of falling through
to file_server and returning 404. That is what the documented fallback promises,
but for a site of standalone .php scripts plus static assets it is a visible
change: unmatched requests now reach PHP-FPM.
The other try policies still test existence explicitly, so a candidate that does
not exist is skipped — but note this applies to ANY pattern that expands to
nothing, not only a malformed glob: a file whose literal name contains glob
metacharacters can now match under first_exist, largest_size and smallest_size
where previously globbing found it only via expansion.
* fileserver: synthesize the literal candidate only for the fallback entry
Address review: the previous commit appended the pattern itself as a
candidate inside makeCandidates whenever globbing found nothing. Since
makeCandidates is shared by every try_policy, first_exist, largest_size,
smallest_size and most_recently_modified could then stat the literal
pattern, so a file whose name contains glob metacharacters (for example
"lit[1].txt") matched where it did not before.
Restore makeCandidates to its original behaviour and build the literal
candidate only for the final entry of first_exist_fallback, the one
entry that is documented to match unconditionally. Placeholders in that
entry are expanded without glob escaping, since nothing is globbed, so
a request for /a[b].txt falls back to /a[b].txt rather than /a\[b].txt.
The path is still cleaned and joined with SanitizedPathJoin, so it
cannot escape the root.
This supersedes the last paragraph of the previous commit message: the
other policies now behave exactly as they did before that commit. The
php_fastcgi behaviour change described there still applies.
Add a regression test that creates a file literally named "lit[1].txt"
and checks that only the fallback entry of first_exist_fallback matches
it by its literal name, including the split_path remainder.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* fastcgi: add license headers to new test files
Address review: phpfastcgi_test.go and fastcgi_windows_test.go were
added without the Apache license header used across the codebase. On
the Windows test, the header goes above the build constraint, matching
replacer_fuzz.go.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* fastcgi: avoid hardcoded Unix paths in dot-segment tests
Address review: the new tests compared SCRIPT_FILENAME against strings
with forward slashes ("/srv/other.php", a "/admin.php" suffix), but the
transport makes the root absolute and joins it with filepath.Join, so
on Windows the value is C:\srv\other.php and the assertions would fail.
Use a temporary directory as the root and build the expected paths with
filepath instead.
Also lower the bcrypt cost of the integration test's basic_auth hash to
the minimum. At cost 14 the authenticated request exceeded the 5s test
client timeout under -race.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 4ec5a5c39a)
A CGI script that redirects by printing only a Location header got a
200 response with that header, which clients do not follow. RFC 3875
section 6.2.3 says the server sends 302 Found for this response, and
net/http/cgi, nginx and Apache do so.
(cherry picked from commit 1962b20401)
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>
A request whose body has no length of its own — Transfer-Encoding: chunked, or
HTTP/2 and HTTP/3 requests sent without the header — cannot be forwarded over
FastCGI until it has been buffered in full, because CGI/1.1 requires
CONTENT_LENGTH and php-fpm hangs when it is absent or wrong and the body is not
empty. When request_buffers is too small to hold the whole body, the length
stays unknown and the request is refused with 411.
The refusal said none of that. RoundTrip passes r.ContentLength to Post, which
is -1 for such a request, so the operator got a bare "411 Length Required" with
either no error at all or strconv's "invalid syntax" on a value they never
wrote. On #7386 that sent two people looking for the fault in their backend.
The 411 now carries the reason and the remedy, and the unknown-length case is
told apart from a genuinely malformed value. ParseUint becomes ParseInt plus an
explicit negative check, which is strictly tighter: values above MaxInt64 used
to be accepted and are unreachable anyway, since CONTENT_LENGTH is always
FormatInt of an int64 by the time Do sees it.
No status code changes. Every request that was refused before is still refused;
raising request_buffers is still what makes a large chunked body work.
Tests: the reachable path through Post, the unusable CONTENT_LENGTH values Do
itself guards against, an integration test driving a chunked body through a
fastcgi reverse_proxy over a unix socket at each side of the buffer boundary
(including request_buffers -1, which succeeds at any size and is the remedy),
and a boundary test recording that a body exactly the size of the buffer counts
as partial.
That last one is deliberately not changed here. Telling "exactly the limit"
apart from "more to come" costs a read that a paused stream may never answer,
and bufferedBody also backs response_buffers: measured against a body that
delivers exactly the limit and stops, the current code hands the buffered
prefix on immediately, while peeking one byte first delivers nothing at all.
Co-authored-by: Aditya <205600203+Rohilalala@users.noreply.github.com>
Co-authored-by: Zen Dodd <mail@steadytao.com>
* reverseproxy: build TLS client config once so HTTP/3 keeps tls_trust_pool
reuse the config built for the HTTP/1.1+2 transport and give the HTTP/3
transport a clone of it to honour the tls_trust_pool
Signed-off-by: Timo Stark <tippexs91@googlemail.com>
Co-authored-by: Faiyaz Rahman <faiyazrahman03@gmail.com>
* Adding issue reference
---------
Signed-off-by: Timo Stark <tippexs91@googlemail.com>
Co-authored-by: Faiyaz Rahman <faiyazrahman03@gmail.com>
* reverseproxy: propagate TCP half-close on upgraded streams
handleUpgradeResponse starts one copy goroutine per direction but returns as
soon as the first one reports a result, and returning runs two unconditional
deferred closes (the client connection and the backend connection). A client
that ends its send direction with a TCP half-close therefore causes the whole
tunnel to be torn down: the backend connection is reset mid-conversation and
whatever it was about to send is never delivered.
Treat a clean EOF as the end of one direction rather than the end of the
tunnel. When a copy finishes without error, propagate the half-close to the
destination with CloseWrite where the connection supports it, and keep waiting
for the other direction so pending bytes can still drain. Real copy errors, the
stream timeout and request cancellation/shutdown still tear down immediately.
If the destination does not support CloseWrite, the peer cannot be told that
the direction ended, so the previous behavior is kept and the tunnel is torn
down rather than held open until the stream timeout.
This is the same class that was fixed upstream in net/http/httputil
(https://go.dev/issue/35892); Caddy has its own upgraded-stream copier, so the
standard library fix does not apply here.
Fixes#8026
* reverseproxy: follow net/http/httputil shape for the half-close
Replace the copyResult struct and the closeWriter/closeWrite helpers with
upstream's plain error channel and errCopyDone sentinel, propagating
CloseWrite inside the copier, as requested in review.
Behaviour is unchanged. The wait is repeated twice inside the existing
select because handleUpgradeResponse also selects on the stream timeout
and sizes the channel at 2 so both goroutines can exit when it fires
(#7418), which net/http/httputil has no equivalent of.
* reverseproxy: explain the repeated wait next to it
Move the reasoning for why the wait is repeated inside the select, rather
than written as upstream's two-line form, into a comment beside the loop
so a future reader does not have to find the review discussion.
* reverseproxy: add an upgraded-stream half-close integration test
Drive a real upgraded stream through the handler and check that closing one
direction is propagated to the other side instead of tearing the tunnel down.
Mirrors net/http/httputil's TestReverseProxyWebSocketHalfTCP, which covers the
same class upstream (https://go.dev/issue/35892): a backend that hijacks and
hands back a *net.TCPConn, the handler in front of it, and a client that dials
it directly, then the four close-read / close-write combinations.
Against the unfixed handler the two half-close cases fail - the surviving
direction never delivers its pending bytes - while the two close-read cases
pass, so the test isolates exactly the reported behaviour.
* reverseproxy: give the half-close test reads a deadline
Without one, a regression blocks in ReadFull until the package timeout
instead of failing the test, which is the flakiness an end-to-end test of
this kind is usually accused of. Ten seconds is far above any real latency
here and turns a hang into a clear failure.
* reverseproxy: hijack the test backend via http.NewResponseController
Matches how the rest of the tree reaches Hijack (responsewriter.go,
caddyauth.go, streaming.go itself) and keeps working if the writer is
wrapped, where a direct http.Hijacker assertion would not.
The active health check body is user-supplied and commonly JSON, but it
was expanded with ReplaceAll, which blanks any `{...}` the replacer does
not recognize. Only globals are available here, so a JSON
health_request_body was sent as an empty body.
This is not limited to bodies without placeholders. A body that does use
one is mangled too: `{"token":"{env.SOME_VAR}"}` is sent as `"}`, since
the opening `{"token":"` is consumed as an unrecognized placeholder
before the real one is reached.
Use ReplaceKnown, matching the header values a few lines below, so
unrecognized braces are left intact and real placeholders still expand.
Only the body is changed. The Host header on the same path keeps
ReplaceAll deliberately: Go's HTTP server blanks a Host containing
braces, so leaving `{...}` intact there would send a Host the upstream
discards, where blanking it lets Go fall back to the URL host.
* Implement HTTPoxy mitigation in FastCGI
Added HTTPoxy mitigation to prevent trusting client-supplied Proxy header for HTTP_PROXY environment variable.
* Implement test for HTTPoxy vulnerability protection
Add test to ensure HTTPoxy vulnerability is mitigated by dropping client-supplied Proxy headers.
* gofmt: format code
* revert unrelated gofmt change to replacer_test.go
* reverseproxy: isolate active health-check state per distinct check config
Multiple reverse_proxy handlers configured with different active health
checks (health_uri, health_headers, ...) against the same upstream dial
address currently share a single Host in the global pool, so one
handler's failing probes mark the address unhealthy for every other
handler. Key the pool by dial address plus a stable fingerprint of the
active health-check config, so distinct checks get independent health
state.
The fingerprint is strictly internal to pool identity: the Prometheus
upstreams_healthy label and the /reverse_proxy/upstreams admin endpoint
continue to report the plain dial address, unchanged.
Dynamic upstreams are intentionally out of scope here: they resolve
through a separate per-lookup path (dynamicHosts) and collapsing there
has different lifetime semantics; noted for a follow-up.
Fixes#7870
* reverseproxy: use strings.Cut in hostKeyAddress
Satisfies the modernize linter; behaviour is unchanged, since Cut returns
the whole string when the separator is absent.
* reverseproxy: expose the health-check fingerprint as a public discriminator
Health state is now kept per (dial address, active health check config),
but both user-visible surfaces still reported address alone:
- caddy_reverse_proxy_upstreams_healthy was labeled only by upstream, so
every handler sharing an address wrote the same series concurrently and
the reported value was whichever updater ran last. The metric gains a
health_check label carrying the config fingerprint ("" when no active
checks), so each health target owns its series; aggregate across checks
with sum/min by (upstream).
- /reverse_proxy/upstreams reported one entry per pool key but with only
the plain address, so consumers indexing by address silently discarded
all but one entry. Entries now carry health_check (omitted when empty),
and the endpoint documents that (address, health_check) is the entry's
identity — one entry per health target, deliberately not aggregated,
since any aggregation here would be lossy and undocumented.
Tests: two handlers on one address with different checks must produce two
metric series reflecting their own state (fails if the fingerprint is
dropped from the label), and two admin entries distinguished by non-empty
fingerprints.
* reverseproxy: narrow the fix to per-Upstream active health counters
Move the consecutive active pass/fail counters from Host onto Upstream,
alongside the active unhealthy state that already lives there, instead of
re-keying the global host pool.
Host is keyed by dial address alone, but an active health check is
configured per handler, so two handlers dialing the same address with
different health_uri or health_headers share those counters and can push
each other over their own thresholds. Upstream is already per-handler and
already carries the active unhealthy flag, so the counters belong next to
it and the pool keeps its plain dial-address keys.
This drops the host key fingerprint and its exposure in the metric label
and the admin upstreams endpoint; the metric series identity is left for
separate consideration.
---------
Co-authored-by: SillyZir <269283839+SillyZir@users.noreply.github.com>
Co-authored-by: Zen Dodd <mail@steadytao.com>
Caddy's underscore header filter (GHSA-f59h-q822-g45g) only checked
for `_`. PHP folds `.` to `_` when registering $_SERVER keys the same
way CGI/FastCGI folds `-` to `_`, so a dotted alias (e.g. Remote.User)
survived the filter and collided with the legitimate hyphenated
header once it reached a PHP/FastCGI backend, bypassing forward_auth
copy_headers the same way the underscore alias did.
Extends the filter to drop `.` symmetrically, adds an
`expected_dot_headers` allowlist mirroring `expected_underscore_headers`,
and handles header names containing both separators (only an exact
allowlist entry is honored there, since a prefix glob's free-form
suffix can't be vetted for an embedded second separator).
Root cause identified by @iliaal in the FrankenPHP advisory
GHSA-49wc-4hcv-v58q.
Preallocate the sources slice in MultiUpstreams.Provision and the
netAddrs slice in UpstreamResolver.ParseAddresses now that their final
lengths are known, avoiding the intermediate append regrowth
allocations. For a 4-address resolver this reduces ParseAddresses from
11 to 9 allocs/op and 528 to 384 B/op; the benefit scales with the
number of sources/addresses.
| before | after |
| sec/op | sec/op vs base |
UpstreamResolverParseAddresses-8 | 2.289µ |1.946µ ~ (p=0.165) noisy |
B/op: 528 → 384 -27.27% (p=0.000)
allocs/op: 11 → 9 -18.18% (p=0.000)
Adds BenchmarkUpstreamResolverParseAddresses as per policy.
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.
* feat(fastcgi): populate SERVER_ADDR by default
Populate the SERVER_ADDR FastCGI environment variable using the local socket address of the incoming connection from the request context. This improves out-of-the-box compatibility for PHP applications that rely on $_SERVER['SERVER_ADDR'] (such as legacy frameworks and custom telemetry packages) when migrating from Nginx or Apache.
Co-developed-by: Gemini AI <renich+gemini@woralelandia.com>
Signed-off-by: Rénich Bon Ćirić <renich@woralelandia.com>
* fix(fastcgi): validate SERVER_ADDR IP format and add test coverage
Validate that SERVER_ADDR is only populated when the local address parses as a valid IP address, preventing Unix socket paths from being assigned if SplitHostPort fails. Add test cases covering IPv4, IPv6, missing context, Unix sockets, and explicit overrides.
Co-developed-by: Gemini AI <renich+gemini@woralelandia.com>
Signed-off-by: Rénich Bon Ćirić <renich@woralelandia.com>
The random_choose selection policy is meant to implement
power-of-d-choices: sample d available upstreams uniformly, then pick
the least-loaded of that sample. The sampling loop, however, was not a
correct reservoir sample (Algorithm R): it never filled the first k
reservoir slots unconditionally, instead writing every candidate to a
random slot j = rand(i+1), which can evict an earlier candidate while
leaving another slot nil. It also derived j from the upstream's index
in the pool rather than from the number of available upstreams seen,
skewing the sample whenever unavailable upstreams precede available
ones.
As a result the reservoir frequently held fewer than min(k, available)
upstreams, so the least-load comparison often never happened. Most
notably, with two upstreams and 'random_choose 2' (the canonical
power-of-two-choices setup), half of all requests were routed to the
more-loaded upstream even when it was saturated and the other idle --
identical behavior to plain 'random'. The nil slots this leaves behind
were also the cause of the panic reported in #3810, which was patched
by skipping nils in leastRequests rather than by fixing the sampling.
Replace the loop with a standard Algorithm R reservoir sample over the
available upstreams: fill the first k slots, then replace a random slot
with probability k/n. Every available upstream is now sampled uniformly
and the reservoir always holds min(k, available) candidates.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* reverseproxy: validate on weighted_round_robin policy
Validate that weighted_round_robin has a non-zero total weight.
This prevents configurations such as:
weighted_round_robin 0 0
from being accepted and causing a divide-by-zero panic during request handling.
* test: validation test on zero weight upstreams.
* test: provision called instead of totalweight setting
* reverseproxy: validate on negative upstream weights
* test: regression test on weighted_round_robin selection policy
Kévin Dunglas
Julien Voisin
D'Ali Ben Macha
sam-kota
Akinyemi Toluwalase
Sulaiman Dauda
tomholford
Rob Martin
Adarsh Kumar
Mohamed A. Abdel Fattah
Krish Singh Hura
pamnario
Jay Hemnani
zemamba
Saleh
Zen Dodd
WeidiDeng
Timo StarkandFaiyaz Rahman
Turhan ACAR
Islam Elsayed
bzyy1024
yangshenghui
Renich Bon Ciric
Christian Mehlmauer
TowyTowyandClaude Fable 5
alhuda
Saleh
Dionis Ramadani
Rhul