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.
Mohammed Al Sahaf
Zen Dodd
TowyTowy
David Carliez
Amariah Abishai
Francis Lavoie
0jaspahwa
Gautam R
Faiyaz Rahman
Kévin Dunglas
bzyy1024
yangshenghui