Canonicalize the path before capturing and substituting its components, not only afterward. Retain repeated slashes, query strings and valid alternate encodings; cover the supplied depth-changing traversal and encoded dot segments.
Addresses scan report 34013c71-0786-4179-8403-eacc143a1d89 (marked duplicate, but not fixed by the prior post-replacement safeguard).
Code written by GitHub Copilot (GPT-6.1 Sol, Claude Opus 5.5); reviewed by mholt.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Decoded path placeholders (orig_uri.path, path components and file
matcher values) were inserted raw into the rewritten URI, so an
encoded '%' or '?' could be decoded a second time. Escape all decoded
path placeholders in path context in a single replacement pass; escaped
and whole-URI placeholders keep their URI syntax.
Addresses scan report 9e3a082391f793d1.
Code written by GitHub Copilot (GPT-6.1 Sol, Claude Opus 5.5); reviewed by mholt.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Like prefix/suffix stripping, substring and regexp path replacement can
produce a relative path or expose dot segments that later path-scoped
handlers interpret differently. Apply the existing canonicalizePath
safeguard whenever these operations change the path.
Addresses scan report 65568b0d767b733c.
Code written by GitHub Copilot (GPT-6.1 Sol, Claude Opus 5.5); reviewed by mholt.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* 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>
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>
* 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>
changePath cleared RawPath whenever it equalled url.Path, which
discarded a valid non-canonical percent-encoding produced by a
replacement. This compare `RawPath` against the default escaping of Path
instead.
* rewrite: scope keyed query replace to its named key
* rewrite: cover keyed search_regexp query replace in test
* rewrite: provision query replace test via Provision path
When the rewrite URI template ends with a literal '?' and contains a placeholder that expands to client-controlled bytes (e.g. {http.request.header.X-Fwd}), those bytes flow into buildQueryString which runs a second Replacer pass. If the bytes contain placeholder syntax such as {env.SECRET}, that placeholder is evaluated, allowing disclosure of environment variables, files (via {file./path}), or internal request vars through the rewritten request URI.
Escape '{' and '}' in the injected query before assigning it to the query variable, so the second pass cannot find any placeholder syntax to evaluate. Operator-written placeholders in the rewrite template are already expanded by the first pass on the path component, so the only '{' or '}' surviving into the injected query must have come from replacement values.
Fixes GHSA-j8px-rmrx-76h9.
Includes three regression tests mirroring the 'is not re-expanded' tests in modules/caddyhttp/vars_test.go.
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
* chore: upgrade .golangci.yml and workflow to v2
run `golangci-lint fmt`
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* run `golangci-lint run --fix`
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* more lint fixes
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* bring back comments to .golangci.yml
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* appease the linter some more
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* oops
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* use embedded structs
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* use embedded structs where they were used before
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* disable rule `-QF1006`
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* missed a spot
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
---------
Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
* Implemented basic uri query operations
* Added support for query operations block
* Applied Replacer on all query keys and values
* Implemented rename query key opration
* Rewrite struct: Changed QueryOperations field to Query and comments cleanup
* Cleaned up comments, changed the order of operations and added more tests
* Changed order of fields in queryOps struct to match the operations order
* use gofmput to format code
* use gci to format imports
* reconfigure gci
* linter autofixes
* rearrange imports a little
* export GOOS=windows golangci-lint run ./... --fix
* reverseproxy: Mask the WS close message when we're the client
* weakrand
* Bump golangci-lint version so path ignores work on Windows
* gofmt
* ugh, gofmt everything, I guess
Ideally I'd just remove the parameter to caddy.Context.Logger(), but
this would break most Caddy plugins.
Instead, I'm making it variadic and marking it as partially deprecated.
In the future, I might completely remove the parameter once most
plugins have updated.
* caddyconfig: WIP implementation of handle_path
* caddyconfig: Complete the implementation - h.NewRoute was key
* caddyconfig: Add handle_path integration test
* caddyhttp: Use the path matcher as-is, strip the trailing *, update test
If a placeholder in the path component injects a query string such as
the {http.request.uri} placeholder is wont to do, we need to separate it
out from the path.
* rewrite: strip_prefix, strip_suffix, uri_replace -> uri (closes#3140)
* Add period, to satisfy @whitestrake :) and my own OCD
* Restore implied / prefix
Paths always begin with a slash, and omitting the leading slash could be
convenient to avoid confusion with a path matcher in the Caddyfile. I do
not think there would be any harm to implicitly add the leading slash.
The fix that was initially put forth in #2971 was good, but only for
up to one layer of nesting. The real problem was that we forgot to
increment nesting when already inside a block if we saw another open
curly brace that opens another block (dispenser.go L157-158).
The new 'handle' directive allows HTTP Caddyfiles to be designed more
like nginx location blocks if the user prefers. Inside a handle block,
directives are still ordered just like they are outside of them, but
handler blocks at a given level of nesting are mutually exclusive.
This work benefitted from some refactoring and cleanup.
Before, modifying the path might have affected how a new query string
was built if the query string relied on the path. Now, we build each
component in isolation and only change the URI on the request later.
Also, prevent trailing & in query string.
Matthew HoltandCopilot
Julien Voisin
Zen Dodd
Nguyen Thanh Dat
Kévin Dunglas
TowyTowyandClaude Fable 5
Francis LavoieandClaude Opus 5
Saleh
alhuda
LohitandMatt Holt
Rayan Salhabandcyphercodes
Mohammed Al Sahaf
mister-turtle
Alexander Stecher
Aziz Rmadi
a
Jacob Gadikian
Tran Phong
Chirag Maheshwari
go-d