* 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 respond directive expanded header field names and values with
repl.ReplaceAll, which blanks any {...} the replacer does not recognize
Header values are config data, often JSON or carrying literal braces, so a
value like {key:value} was sent empty and a-{b}-c came out a--c
Use ReplaceKnown, matching the header handler fix in #4880 (same class as
#4418) and the respond body expansion a few lines below, so unknown braces
survive and real placeholders still expand
Opening the bbolt database only reported timeout, which sounds like a
broken config. Catch bolt.ErrTimeout with errors.Is and say another
process holds the lock, and that caddy reload is the right command when
Caddy is already running. The error stays fatal.
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.
App.Emit does a fair amount of work before it can discover that no handler
is bound: it derives three loggers, one of which formats the event's UUID
even when debug logging is off, and registers a replacer callback. Only
then does it reach "shortcut if event not bound at all".
Some events are emitted on every TLS handshake -- CertMagic emits
tls_get_certificate as the first statement of GetCertificateWithContext --
so on a server with no events configuration that work runs per handshake
and is discarded every time.
Return early when neither the event's name nor the catch-all is bound and
debug logging is off, which are exactly the conditions under which nothing
can observe the event. caddy.NewEvent still runs, so the returned Event is
unchanged for callers.
Benchmarks are included; measurements are in the pull request.
* caddytls: synchronize storage cleaner with TLS.Stop via context and WaitGroup
* Unify the storage cleanup synchronization to both tls and ech.
* Cannot embed the sync.WaitGroup directly as the TLS struct is copied.
* caddytls: propagate cancellable context to ECH rotation and add sync tests
Pass cancellable context to ECH key rotation so in-flight storage locks
and operations unblock when TLS.Stop is invoked. Add comprehensive tests
verifying TLS.Stop cleanly unblocks and waits for storage cleaner and
ECH workers.
---------
Co-authored-by: Zen Dodd <mail@steadytao.com>
The admin host check compared r.Host against the allowed URL's Host
with byte-for-byte equality. url.Parse does not normalize host case,
so an allowed 'http://Example.com:2019' rejected a client Host of
'example.com:2019' with 'host not allowed', and the same happened for
an uppercase variant of a lowercase entry like localhost.
The Origin check got this treatment in 7973 already, the DNS rebinding
Host check right next to it did not. Both read the same allowedOrigins
list, so this applies the same strings.EqualFold treatment there.
Regression test covers both fold directions, the default localhost
entry and the negative case that must keep failing
- willCycle now reports a cycle when from == to, so addEdge rejects
self-loops (a self-importing file was previously accepted).
- removeNode now drops the removed node's outgoing edges and any
incoming edges pointing at it, keeping the adjacency map consistent.
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
The admin origin allow-list compared origin.Host against the allowed
URL's Host with byte-for-byte equality. url.Parse does not normalize
host case, so an allowed 'http://Example.com:8080' rejected a client
origin of 'http://example.com:8080' even though RFC 3986 §3.2.2 says
host names are case-insensitive. Use strings.EqualFold.
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Per https://go.dev/ref/mod#go-mod-file-ident, '@' is not a valid module
path character. The old implementation used strings.LastIndex to allow
inputs like github.com/@user/module@v1.0.0 to parse, but such inputs
are not valid module paths per the spec. Reject them explicitly.
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
A port range is inclusive on both ends, so a range from start to end
spans (end - start + 1) ports. The old (end - start) > maxPortSpan
check let 0-65535 through even though that spans 65536 ports, one
above the 65535 cap. Include the trailing port in the count.
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
v1.0.0 renamed SetGoMemLimitWithOpts to Set and dropped the other Set*
helpers. The options this call passes are unchanged, as are the 0.9
default ratio and the AUTOMEMLIMIT handling, so behavior is the same.
Only the already-deprecated AUTOMEMLIMIT_DEBUG and AUTOMEMLIMIT_EXPERIMENT
environment variables are gone, and neither is used here.
StripPathSuffix is documented to behave like StripPathPrefix: the suffix
is matched in normalized (unescaped) space except where the pattern uses
an escape sequence. But suffix stripping was implemented as
reverse(trimPathPrefix(reverse(escapedPath), reverse(suffix)))
Reversing the strings moves the '%' to the *end* of each "%xx" escape,
which defeats trimPathPrefix's escape detection (it expects '%' to
precede the two hex digits). As a result the escape-aware, normalized
comparison never happened for suffixes: a decoded pattern failed to
match a percent-encoded path.
Concretely, StripPathPrefix "/a/b/c" strips "/a%2Fb/c/d" to "/d", but the
mirror StripPathSuffix "/b/c" left "/a/b%2Fc" untouched instead of
producing "/a"; likewise StripPathSuffix "bc" did not strip "/a%62c".
This has been the behavior since #4948, which introduced both the
escape-aware trimPathPrefix and the reverse-based suffix trimming.
Replace the reverse trick with a dedicated trimPathSuffix that iterates
from the ends of both strings and applies the same escape-aware,
case-insensitive comparison as trimPathPrefix. An escape in the pattern
itself is still compared literally, so "%2fsuffix" continues to require
the path to contain that exact escape.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* admin: normalize request path in remote admin access-control check
Co-authored-by: atlarix-agent <agent@atlarix.dev>
* admin: fix empty allowedPath regression and dead code in path normalization
path.Clean("") returns ".", so cleaning allowedPath unconditionally
silently broke the allow-all behavior when Paths: [""] is configured.
Short-circuit the empty case before cleaning to preserve that behavior.
Also remove the dead strings.HasSuffix(allowedPath, "/") branch —
after path.Clean the path never has a trailing slash, so the unified
reqPath == allowedPath || HasPrefix(reqPath, allowedPath+"/") form
covers exact match, subpath boundary, and trailing-slash requests.
Co-authored-by: atlarix-agent <agent@atlarix.dev>
Co-authored-by: iabdullah215 <muhammadabdullah8040@gmail.com>
* admin: validate non-canonical configured paths at provisioning
path.Clean(allowedPath) silently broadens misconfigured values like
// or /.. into /, which grants unintended access to all endpoints.
Reject non-canonical paths during provisioning in
replaceRemoteAdminServer so misconfigurations fail fast with a
clear error. The path.Clean in adminPathAllowed remains as
defense-in-depth but is now a safe no-op on validated inputs.
Co-authored-by: atlarix-agent <agent@atlarix.dev>
* admin: validate non-canonical configured paths at provisioning
path.Clean(allowedPath) silently broadens misconfigured values like
// or /.. into /, which grants unintended access to all endpoints.
Reject non-canonical paths during provisioning in
replaceRemoteAdminServer so misconfigurations fail fast with a
clear error. The path.Clean in adminPathAllowed remains as
defense-in-depth but is now a safe no-op on validated inputs.
Co-authored-by: atlarix-agent <agent@atlarix.dev>
* admin: fix TrimRight → TrimSuffix in provisioning path validation
strings.TrimRight strips all trailing slashes, so a configured path
like /foo// passed validation (both slashes trimmed to /foo matching
path.Clean output) but was silently broadened to /foo at runtime.
Use strings.TrimSuffix instead, which removes exactly one trailing
slash — the only form the exemption was meant to allow (users write
/pki/ca/prod/ meaning the /pki/ca/prod scope).
Also update the // test case: with TrimSuffix, // is just / + one
trailing slash, which is valid under the exemption. Add a new test
for /foo// (double trailing slashes → wantErr: true).
Co-authored-by: atlarix-agent <agent@atlarix.dev>
* admin: reject non-canonical root permission path
* admin: preserve trailing-slash permission semantics
---------
Co-authored-by: atlarix-agent <agent@atlarix.dev>
Co-authored-by: iabdullah215 <muhammadabdullah8040@gmail.com>
Co-authored-by: Zen Dodd <mail@steadytao.com>
* rewrite: don't drop a trailing '=' from the query string
buildQueryString scanned for '=' unconditionally when looking for the
end of a component, but '=' only delimits a key from its value once;
any further '=' bytes are literal data. When the query ended with '=',
that byte was consumed as a delimiter with nothing following it to
re-emit, so it was silently lost:
?x=1&sig=YWJjZA== => ?x=1&sig=YWJjZA=
This corrupts base64 padding in the last query parameter, which is the
shape of an S3 presigned URL (X-Amz-Signature), turning a valid
signature into a 403. Only the final '=' of the query was affected;
?sig=YWJjZA==&x=1 came through intact.
Disable the '=' search while consuming a value so that only '&' ends it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* rewrite: honor a query string injected by a replacement value
Which URI components get written back was decided from the literal
config string, before placeholders were expanded, but an injected query
is only detected after expansion. The two were never reconciled: for
`rewrite * {rp.header.X-Accel-Redirect}` there is no literal '?', so
qsStart stayed -1, and the correctly-built query string was computed and
then discarded by the `if qsStart >= 0` guard.
Only half the split was applied. The path was still truncated at the
injected '?', so the query was not preserved either -- it was dropped,
and any query already on the request survived in its place:
GET /orig?keep=me, X-Accel-Redirect: /hello?some=param
=> /hello?keep=me
Track whether a query was actually injected and include that in the
write-back condition. Appending a literal '?' to the rewrite value was
the known workaround precisely because it set qsStart; that keeps
working and is now unnecessary.
The flag is only set where the injected query is adopted, so an
explicitly configured query still wins, and a value with no '?' still
leaves the query untouched -- which is what the implicit rewrites of
try_files and php_fastcgi rely on. Those stay safe regardless, since
escapePathPlaceholders already escapes the two placeholders they use, so
a client-supplied %3F cannot split the URI.
Fixes#5208
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* rewrite: drop a fragment injected by a replacement value
The scan that separates path, query and fragment runs on the literal
config string, so a '#' arriving later via a replacement value was never
treated as a delimiter. It leaked into whichever component it landed in:
X-Accel-Redirect: /hello?p=x#frag => RawQuery = "p=x#frag"
Everything after '#' is fragment (RFC 3986 section 4.2) and a fragment is
never sent to the server, so drop it before the path is split, mirroring
how the scan already handles a literal '#'. An escaped %23 is unaffected,
so a real '#' in a path or query is still expressible, and a configured
fragment still wins over an injected one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* caddyhttp: mitigate slowloris via idle read/write deadlines
ReadTimeout and WriteTimeout previously applied as a single hard
deadline over the whole body/response through http.Server, so any
non-zero value also killed large transfers from legitimately slow
clients. Reset the deadline on every successful read/write instead
(via http.ResponseController), and give both a sane 1m default now
that doing so no longer penalizes slow-but-progressing clients.
* caddyhttp: split idle read/write timeouts from the existing hard ones
Reworking ReadTimeout/WriteTimeout's own semantics was an unwanted
behavior change for existing configs relying on the hard deadline.
Leave them untouched and add ReadIdleTimeout/WriteIdleTimeout instead,
reset on every successful read/write; both default to 1m since,
being new, no existing config could have depended on a different
value. Combining an idle timeout with its hard counterpart now gives
the same base+ceiling shape as Apache's mod_reqtimeout, for free.
* caddyhttp: cap idle-reset deadlines at the hard timeout ceiling
Deadlines are a single absolute value on the connection, not a min of
several: ReadTimeout/WriteTimeout's own hard deadline, set once by
net/http before the handler runs, was silently getting overwritten by
the first idle-reset Read/Write, voiding it entirely. Clamp the
idle-reset deadline to the hard one when both are set, so combining
them actually behaves like the advertised base+ceiling.
* caddyhttp: add ReadMinRate/WriteMinRate, Apache MinRate equivalent
Pure idle-reset alone doesn't bound a trickle that sends just enough
to never go idle. ReadMinRate/WriteMinRate (bytes/second) grow the
allowed deadline from a fixed start based on bytes transferred so far
instead of resetting to a flat window on every call, so a transfer
that doesn't sustain the configured rate falls behind real time and
gets cut, matching Apache mod_reqtimeout's MinRate. Zero (default)
keeps the existing flat idle-reset behavior unchanged.
* caddyhttp: use named return and consistent blank lines in idleDeadline
Matches the named-return style already used by ResponseWriterWrapper.ReadFrom.
* caddyhttp: chunk idleTimeoutWriter's Write/ReadFrom, cap at 64 KiB
SetWriteDeadline bounds the whole call it precedes, not just a stall
within it. net.Conn.Write loops internally until a buffer is fully
sent (unlike Read, which returns after one syscall), and
ResponseWriter.ReadFrom hands the entire remaining source to the
connection in one call. A single large Write, or any body copied via
io.Copy triggering the ReadFrom fast path (http.ServeContent, static
file serving), had its whole transfer bounded by one deadline,
silently truncating a slow-but-healthy transfer exactly like a hard
WriteTimeout would - the same bug found and fixed the same way in
FrankenPHP's go_ub_write (php/frankenphp#2574).
Cap each underlying call at 64 KiB and reset the deadline between
chunks instead. net/sendfile.go special-cases *io.LimitedReader, so
chunking ReadFrom still uses the sendfile fast path per chunk.
* caddyhttp: export idle-timeout types, add configurable MaxWriteChunk
Export IdleTimeoutReader/IdleTimeoutWriter/IdleDeadline so other
packages (request_body next) can reuse the same idle-reset mechanism
instead of reimplementing it, and turn the hardcoded 64 KiB write
chunk size into a configurable MaxWriteChunk field defaulting to the
same value - nginx's sendfile_max_chunk exists for the identical
reason and is admin-tunable rather than fixed.
* requestbody: idle-reset ReadTimeout/WriteTimeout, add MinRate/MaxWriteChunk
ReadTimeout/WriteTimeout set a single deadline once, so any transfer
running longer than the timeout got cut regardless of whether it was
actually stalled - the same bug the server-wide timeouts had before
switching to idle-reset. Reuse caddyhttp.IdleTimeoutReader/Writer here
too, giving per-route granularity nginx/Apache have via location/
directory scoping and Caddy's server-wide timeouts don't: a route
matching this handler can now set its own idle window independently
from the rest of the server block.
* caddyhttp: fold read/write min_rate into the idle-timeout directive
Two directives per rate (read_body_idle + read_body_min_rate) for a
value that's meaningless without the other. Fold min_rate into the
idle-timeout directive as an optional second argument instead.
* caddyhttp: split write pacing out of request_body into new timeouts handler
request_body is a request-body concern (max_size, set); ReadTimeout/
WriteTimeout/MinRate/MaxWriteChunk pace both directions, and write
pacing has nothing to do with the request body. Move all of it to a
dedicated http.handlers.timeouts module instead, mirroring the
server-wide timeouts option one level down.
* 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
* caddyhttp: match url_pattern against decoded, cleaned path
The url_pattern matcher evaluated the raw, percent-encoded request URI
while path-consuming handlers resolve the decoded, cleaned r.URL.Path.
An encoded-slash payload such as "..%2f" stayed a single opaque segment
for the WHATWG URLPattern parser, so "/public/..%2fadmin/secret" matched
"/public/*" while handlers decoded it to "/admin/secret", bypassing any
route-level access control built with url_pattern.
Match the same path model handlers resolve: decode the path, normalize
and clean it (mirroring the path matcher and #4407), then re-encode
through url.URL to a canonical escaped form before running the pattern.
The go-urlpattern library is spec-correct; the fix is in the integration.
* build(deps): bump github.com/dunglas/go-urlpattern to v1.0.0
Moves off the pseudo-version to the first tagged release.
* 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>
* caddyhttp: shield specific hostnames from a covering wildcard's client auth (@sillyzir)
A connection policy for a wildcard hostname (e.g. *.example.com with
client_auth) is first-match by SNI, so it also applied client
authentication to more specific hostnames served by their own site
blocks (public.example.com) — sites that never asked for mTLS.
Two cases produce the shielding empty policy that fixes this:
- site blocks whose TLS config yields a connection policy with no
settings (previously discarded as having no effect);
- site blocks with no TLS connection policy at all — the reported
case — for which an empty policy is now synthesized.
Either way the empty policy is hoisted directly above the first
client-auth-bearing policy whose wildcard SNI covers the hostname, so
first-match shields it from the client-auth requirement.
Deliberately scoped to client authentication: other wildcard policy
settings, such as certificate selection, are ones a covered hostname
generally WANTS to inherit (see tls_automation_wildcard_shadowing);
sni matchers that fail to decode emit an adapt warning instead of
being silently skipped.
Fixes#7860
* caddyhttp: shield preserves the wildcard policy's other settings (@sillyzir)
The hoisted shield was an empty policy, and connection policies are
first-match: it lifted the client-auth requirement but also suppressed
every other setting the covering wildcard policy carried (certificate
selection, protocol bounds, ALPN). Hoist a copy of the covering policy
with only client_authentication removed instead, so the shielded
hostname keeps inheriting the rest.
The new adapt test gives the wildcard policy protocols and alpn in
addition to client_auth and asserts the shield carries both while
dropping only client authentication; the existing test (client_auth
only) is unchanged, which is exactly why it could not catch this.
* caddyhttp: shield each covering wildcard policy separately (@sillyzir)
The hoisting loop stopped at the first client-auth policy covering any of a
site block's hostnames and gave the shield that block's whole SNI matcher, so
hostnames covered by a *different* wildcard matched it too. Because connection
policies are first-match, those hostnames then took the wrong policy's
settings and lost their own client-authentication requirement entirely.
Map each hostname to the first covering policy individually and hoist one
shield per covering policy, matching only the hostnames it covers. The
existing policies' wildcard SNI names are decoded once up front, and the
shields are inserted back to front so an insertion cannot shift the index of a
covering policy still to be shielded.
* chore: fumpt (@steadytao)
* chore: fix master lint (@steadytao)
---------
Co-authored-by: SillyZir <269283839+SillyZir@users.noreply.github.com>
Co-authored-by: Zen Dodd <mail@steadytao.com>
* fileserver: add failing test for calculateEtag collision
calculateEtag concatenates base36(mtime) and base36(size) with no
separator, so distinct (mtime, size) pairs can yield identical digit
strings and thus identical ETags.
* fileserver: prevent ETag collisions by separating mtime and size components
calculateEtag concatenated base36(mtime) and base36(size) with no
separator, so distinct (mtime, size) pairs could decode to the same
digit string and produce identical ETags.
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.
addHTTPVarsToReplacer ran SetVar(ctx, "uuid", new(requestID)) for every
request during replacer setup, allocating a *requestID unconditionally even
though it is only ever read by the {http.request.uuid} placeholder. The UUID
value itself was already generated lazily (requestID.String caches on first
call), but the container was not.
Allocate the *requestID on first access instead: the uuid placeholder now
creates and stores it in the request's vars table when absent, so repeated
references within a request still share one instance (and thus one UUID),
while requests that never reference the UUID pay nothing.
Add a benchmark for the per-request replacer setup and a regression test for
the uuid placeholder, which previously had no coverage.
Benchmark (BenchmarkAddHTTPVarsToReplacer, linux/arm64, count=6), common path
where the UUID is never referenced:
before: ~700 ns/op 352 B/op 9 allocs/op
after: ~690 ns/op 336 B/op 8 allocs/op
One 16-byte allocation removed per request; a GC-pressure win rather than a
latency one.
* network_proxy: reject proxy URLs that resolve to a port with no host
The host check after placeholder replacement had its arguments swapped:
strings.Split("", pUrl.Host)[0] == ":"
This splits the empty string using pUrl.Host as the separator, so it
returns [""] and element 0 is always "", never ":". The comparison was
dead for every possible host value, which meant the "http://:80" case
named in the comment directly below it was never rejected. Such a URL
was handed back as the proxy, and the failure surfaced later as a
confusing dial error instead of the intended message.
Only the pUrl.Host == "" half of the condition ever did anything, so
"/some/path" was still caught.
Use url.URL.Hostname(), which returns the host with any port stripped
and is "" for exactly the two cases the comment describes -- ":80" and
"" -- while leaving IPv6 literals such as "[::1]:80" and userinfo forms
intact. That collapses both clauses into one expression.
Also adds tests for this function; the package previously had none.
* network_proxy: cover userinfo-only and IPv6 hosts in the tests
Differential-tested the old predicate against the new one across 36 URL
forms. Every behavioural change is in the same direction -- previously
accepted, now rejected -- and all of them are host-less. Nothing that was
rejected before is accepted now, and no legitimate host form changes.
Two of those forms were worth pinning down in the test:
- "http://user:pass@:8080" parses with Host ":8080", so it is just as
host-less as "http://:80". The comment in the source doesn't name
this variant, but it was accepted before and is rejected now.
- IPv6 literals are full of colons, so a repair that split Host on ":"
rather than using Hostname() could plausibly reject them. Added
"[::1]:8080" and "[2001:db8::1]" as regression guards; both are
accepted before and after, which is the point.
On unfixed master the port-only and userinfo cases both fail; the IPv6
cases pass on both sides.
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>
techknowlogick
XiaoleC05
dependabot[bot]
Turhan ACAR
Zen Dodd
Abdellatif Anaflous
wangjingshuiku
Faiyaz Rahman
Islam Elsayed
Y.Horie
Jens-Uwe Mager
Alexandre Daubois
Sash
Mohammed Al Sahaf
James Ko
Kévin Dunglas
TowyTowy
David Carliez
Amariah Abishai
Francis Lavoie
0jaspahwa
Gautam R
bzyy1024
yangshenghui
Steffen Busch
SillyZir
Kévin Dunglas
Julien Voisin
Ben Younes
Rohit Behera
Renich Bon Ciric
cui fliter