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>
* caddyauth: isolate provider responses to prevent cross-provider clobbering
When multiple authentication providers are configured, each was handed the
real ResponseWriter, so a failing provider that wrote to the response (a
401 challenge or a login redirect) could clobber the response of another
provider or of the successful handler chain. Because provider map iteration
order is randomized, which provider's side effects won was nondeterministic.
A single provider now receives the real ResponseWriter unchanged — no
buffering, and Flusher/Hijacker/Pusher/ReaderFrom preserved exactly as
before. Only with multiple providers does each get its own buffered writer;
those writers embed caddyhttp.ResponseWriterWrapper so the underlying
capabilities remain type-assertable via http.ResponseController (Flush is
suppressed while buffering so a provider cannot prematurely commit the
response), and the buffered body is size-capped to avoid unbounded memory.
On success the winning provider's headers (e.g. a Set-Cookie) are copied to
the real writer and the chain proceeds. On total failure one provider's
challenge headers are applied (a redirect is sent as a full response),
otherwise the auth error is returned so handle_errors runs and a
header-only challenge (like basic auth) still returns 401.
Fixes#5190
* caddyauth: return total consumed bytes and drain error from ReadFrom
ReadFrom drained a source that exceeded the buffer cap but reported only
the bytes retained in the buffer and dropped any error from the drain,
violating the io.ReaderFrom contract: a caller such as io.Copy would see
fewer bytes than were actually consumed from the source, and a read
failure during the drain was silently swallowed. Return the
retained-plus-drained total and propagate the drain error.
* caddyauth: preserve Flusher and Hijacker on the buffered writer
The buffered writer used for multi-provider isolation embeds
caddyhttp.ResponseWriterWrapper, which promotes only Header, Write and
WriteHeader from the wrapped ResponseWriter and adds Push, ReadFrom and
Unwrap. Flush and Hijack were therefore reachable only through
http.ResponseController; a provider doing a plain w.(http.Flusher) or
w.(http.Hijacker) assertion silently lost them once a second provider was
configured.
Declare both on bufferedResponseWriter. Flush is a no-op so a provider
cannot prematurely commit a buffered response, and FlushError keeps the
same suppression for ResponseController, which prefers it over Flush.
Hijack delegates through the embedded wrapper, mirroring
responseRecorder.Hijack in the caddyhttp package.
The existing capability test only probed via http.ResponseController,
which is why this went unnoticed. It now asserts each capability both by
direct type assertion and through the controller, and uses two
non-authenticating providers so the probe is guaranteed to run — map
iteration order previously allowed a succeeding provider to break out of
the loop before the probe executed.
Also replace two header copy loops with maps.Copy, fixing the mapsloop
lint failures.
* encode: flush headers immediately for server-sent events responses
The encode middleware withholds the response header until the first body
write so it can sniff content-type and apply the minimum_length threshold.
For a text/event-stream response the upstream typically writes headers and
flushes to establish the event stream before any event body is available,
so the client never received the headers and the stream stalled; the same
buffering also delayed individual events.
When WriteHeader sees a text/event-stream content type, initialize encoding
and write the header through immediately. Forcing the header out also marks
the response as started, so subsequent event writes bypass the minimum_length
buffering and stream to the client as they arrive.
Fixes#6293
* encode: add WriteHeader benchmark covering SSE fast path
* encode: replace mime.ParseMediaType with bound-checked SSE check
WriteHeader runs an SSE Content-Type check on every call once headers
haven't been written yet. mime.ParseMediaType parses the full media
type, including parameters, even when nothing matches, which shows up
on the hot header-write path.
Replace it with a bound-checked manual prefix/boundary check (isSSE),
skipping parameter parsing for the common non-SSE case.
* encode: reject content types with junk after text/event-stream
isSSE accepted any suffix after a space, so a value like
"text/event-stream nonsense" was treated as an SSE response. After the
media type, skip optional whitespace and require either the end of the
value or a parameter separator. The check remains allocation-free, so
the hot-path motivation for the manual matcher is preserved.
---------
Co-authored-by: SillyZir <269283839+SillyZir@users.noreply.github.com>
Co-authored-by: Kévin Dunglas <kevin@les-tilleuls.coop>
AcceptedEncodings runs for every request when response compression is enabled,
so its allocations contribute directly to per-request garbage and GC pressure.
While the gains are modest, this cuts allocations by two-thirds and bytes
nearly in half on a hot path that executes on every compressed request. Fewer
allocations means less work for the garbage collector, which at high request
rates translates into lower GC frequency and steadier tail latency, not just
faster execution of this one function in isolation.
For each token in the Accept-Encoding header it called strings.Split(accepted,
";"), allocating a throwaway slice per encoding, and grew the prefs slice from
empty. This commit replaces the per-token Split with strings.Cut (zero
allocation) and presize prefs from the comma count.
As requested per the policy, this commit adds a benchmark to exercise the
change. With header "gzip, deflate, br;q=0.9, zstd;q=0.8":
AcceptedEncodings
- sec/op 1158.5n -> 771.8n (-33.38%)
- B/op 408 -> 216 (-47.06%)
- allocs/op 9 -> 3 (-66.67%)
Co-authored-by: @dunglas
Zen Dodd