From 0839b54ebb8106916690119eeb224ffbf84c07e8 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Thu, 16 Mar 2023 17:17:12 -0400 Subject: [PATCH] rewrite: Add option to force modifying the query --- .../reverse_proxy_handle_response.txt | 10 +++++++--- modules/caddyhttp/rewrite/caddyfile.go | 14 ++++++++++++- modules/caddyhttp/rewrite/rewrite.go | 20 +++++++++++++++++-- modules/caddyhttp/rewrite/rewrite_test.go | 20 +++++++++++++++++++ 4 files changed, 58 insertions(+), 6 deletions(-) diff --git a/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.txt b/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.txt index f6a260900..657214ca8 100644 --- a/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.txt +++ b/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.txt @@ -11,7 +11,9 @@ reverse_proxy 127.0.0.1:65535 { @accel header X-Accel-Redirect * handle_response @accel { - respond "Header X-Accel-Redirect!" + rewrite * {rp.header.X-Accel-Redirect} { + modify_query + } } @another { @@ -104,10 +106,12 @@ reverse_proxy 127.0.0.1:65535 { }, "routes": [ { + "group": "group0", "handle": [ { - "body": "Header X-Accel-Redirect!", - "handler": "static_response" + "handler": "rewrite", + "modify_query": true, + "uri": "{http.reverse_proxy.header.X-Accel-Redirect}" } ] } diff --git a/modules/caddyhttp/rewrite/caddyfile.go b/modules/caddyhttp/rewrite/caddyfile.go index a34c1bb08..2d8cd44cb 100644 --- a/modules/caddyhttp/rewrite/caddyfile.go +++ b/modules/caddyhttp/rewrite/caddyfile.go @@ -34,7 +34,9 @@ func init() { // parseCaddyfileRewrite sets up a basic rewrite handler from Caddyfile tokens. Syntax: // -// rewrite [] +// rewrite [] { +// modify_query +// } // // Only URI components which are given in will be set in the resulting URI. // See the docs for the rewrite handler for more information. @@ -48,6 +50,16 @@ func parseCaddyfileRewrite(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, if h.NextArg() { return nil, h.ArgErr() } + + for nesting := h.Nesting(); h.NextBlock(nesting); { + switch h.Val() { + case "modify_query": + rewr.ModifyQuery = true + + default: + return nil, h.Errf("unknown subdirective: %s", h.Val()) + } + } } return rewr, nil } diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go index 31c977886..6782567d8 100644 --- a/modules/caddyhttp/rewrite/rewrite.go +++ b/modules/caddyhttp/rewrite/rewrite.go @@ -88,6 +88,17 @@ type Rewrite struct { // Performs regular expression replacements on the URI path. PathRegexp []*regexReplacer `json:"path_regexp,omitempty"` + // If true, the rewrite will be forced to also apply to the + // query part of the URL. This is only needed if the configured + // URI does not include a '?' character which is normally used + // to determine whether the query should be modified. In other + // words, this allows rewriting both the path and query when + // using a placeholder as the replacement value, whereas otherwise + // only the path would be rewritten because the placeholder itself + // does not contain a '?' character. Only use this if the placeholder + // is trusted to not be vulnerable to query injections. + ModifyQuery bool `json:"modify_query,omitempty"` + logger *zap.Logger } @@ -216,10 +227,15 @@ func (rewr Rewrite) Rewrite(r *http.Request, repl *caddy.Replacer) bool { // recompute; new path contains a query string var injectedQuery string newPath, injectedQuery = before, after - // don't overwrite explicitly-configured query string - if query == "" { + + // don't overwrite explicitly-configured query string, + // unless configured explicitly to do so + if query == "" || rewr.ModifyQuery { query = injectedQuery } + if rewr.ModifyQuery { + qsStart = 0 + } } if query != "" { diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go index 5875983b6..4e12ac94d 100644 --- a/modules/caddyhttp/rewrite/rewrite_test.go +++ b/modules/caddyhttp/rewrite/rewrite_test.go @@ -214,6 +214,23 @@ func TestRewrite(t *testing.T) { input: newRequest(t, "GET", "/foo#fragFirst?c=d"), expect: newRequest(t, "GET", "/bar#fragFirst?c=d"), }, + { + rule: Rewrite{URI: "{test.path_and_query}"}, + input: newRequest(t, "GET", "/"), + expect: newRequest(t, "GET", "/foo"), + }, + { + // TODO: This might be an incorrect result, since it also replaces + // the path with empty string when that might not be the intent. + rule: Rewrite{URI: "{test.query}", ModifyQuery: true}, + input: newRequest(t, "GET", "/foo"), + expect: newRequest(t, "GET", "?bar=1"), + }, + { + rule: Rewrite{URI: "{test.path_and_query}", ModifyQuery: true}, + input: newRequest(t, "GET", "/"), + expect: newRequest(t, "GET", "/foo?bar=1"), + }, { rule: Rewrite{StripPathPrefix: "/prefix"}, @@ -343,6 +360,9 @@ func TestRewrite(t *testing.T) { repl.Set("http.request.uri", tc.input.RequestURI) repl.Set("http.request.uri.path", tc.input.URL.Path) repl.Set("http.request.uri.query", tc.input.URL.RawQuery) + repl.Set("test.path", "/foo") + repl.Set("test.query", "?bar=1") + repl.Set("test.path_and_query", "/foo?bar=1") // we can't directly call Provision() without a valid caddy.Context // (TODO: fix that) so here we ad-hoc compile the regex