summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMatthew Holt <mholt@users.noreply.github.com>2020-01-15 11:44:21 -0700
committerMatthew Holt <mholt@users.noreply.github.com>2020-01-15 11:44:21 -0700
commit07ad4655db5d37635edf36fa91d997d166d306b0 (patch)
tree650c80de9c3eab174c692142f58246c7a429003e
parent271b5af14894a8cca5fc6aa6f1c17823a1fb5ff3 (diff)
rewrite: Make URI modifications more transactional (#2891)
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.
-rw-r--r--modules/caddyhttp/rewrite/rewrite.go26
-rw-r--r--modules/caddyhttp/rewrite/rewrite_test.go10
2 files changed, 31 insertions, 5 deletions
diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index abcaa84..e2d57c4 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -112,7 +112,7 @@ func (rewr Rewrite) rewrite(r *http.Request, repl *caddy.Replacer, logger *zap.L
r.Method = strings.ToUpper(repl.ReplaceAll(rewr.Method, ""))
}
- // uri (path, query string, and fragment just because)
+ // uri (path, query string, and fragment... because why not)
if uri := rewr.URI; uri != "" {
// find the bounds of each part of the URI that exist
pathStart, qsStart, fragStart := -1, -1, -1
@@ -134,14 +134,30 @@ func (rewr Rewrite) rewrite(r *http.Request, repl *caddy.Replacer, logger *zap.L
qsEnd = len(uri)
}
+ // build components which are specified, and store them
+ // in a temporary variable so that they all read the
+ // same version of the URI
+ var newPath, newQuery, newFrag string
if pathStart >= 0 {
- r.URL.Path = repl.ReplaceAll(uri[pathStart:pathEnd], "")
+ newPath = repl.ReplaceAll(uri[pathStart:pathEnd], "")
}
if qsStart >= 0 {
- r.URL.RawQuery = buildQueryString(uri[qsStart:qsEnd], repl)
+ newQuery = buildQueryString(uri[qsStart:qsEnd], repl)
}
if fragStart >= 0 {
- r.URL.Fragment = repl.ReplaceAll(uri[fragStart:], "")
+ newFrag = repl.ReplaceAll(uri[fragStart:], "")
+ }
+
+ // update the URI with the new components
+ // only after building them
+ if pathStart >= 0 {
+ r.URL.Path = newPath
+ }
+ if qsStart >= 0 {
+ r.URL.RawQuery = newQuery
+ }
+ if fragStart >= 0 {
+ r.URL.Fragment = newFrag
}
}
@@ -206,7 +222,7 @@ func buildQueryString(qs string, repl *caddy.Replacer) string {
// if previous iteration wrote a value,
// that means we are writing a key
if wroteVal {
- if sb.Len() > 0 {
+ if sb.Len() > 0 && len(comp) > 0 {
sb.WriteRune('&')
}
} else {
diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go
index 3dbc2d6..34a0cdb 100644
--- a/modules/caddyhttp/rewrite/rewrite_test.go
+++ b/modules/caddyhttp/rewrite/rewrite_test.go
@@ -64,6 +64,16 @@ func TestRewrite(t *testing.T) {
expect: newRequest(t, "GET", "/foo/bar"),
},
{
+ rule: Rewrite{URI: "/index.php?p={http.request.uri.path}"},
+ input: newRequest(t, "GET", "/foo/bar"),
+ expect: newRequest(t, "GET", "/index.php?p=%2Ffoo%2Fbar"),
+ },
+ {
+ rule: Rewrite{URI: "?a=b&{http.request.uri.query}"},
+ input: newRequest(t, "GET", "/"),
+ expect: newRequest(t, "GET", "/?a=b"),
+ },
+ {
rule: Rewrite{URI: "/?c=d"},
input: newRequest(t, "GET", "/"),
expect: newRequest(t, "GET", "/?c=d"),