diff --git a/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.caddyfiletest b/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.caddyfiletest index f6a260900..86c9a1973 100644 --- a/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.caddyfiletest +++ b/caddytest/integration/caddyfile_adapt/reverse_proxy_handle_response.caddyfiletest @@ -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} { + force_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" + "force_modify_query": true, + "handler": "rewrite", + "uri": "{http.reverse_proxy.header.X-Accel-Redirect}" } ] } diff --git a/modules/caddyhttp/rewrite/caddyfile.go b/modules/caddyhttp/rewrite/caddyfile.go index 0f406161a..ee2352d9e 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 [] { +// force_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. @@ -50,12 +52,30 @@ func parseCaddyfileRewrite(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, return nil, h.Errf("too many arguments; should only be a matcher and a URI") } + parseBlock := func(rewr *Rewrite) error { + for nesting := h.Nesting(); h.NextBlock(nesting); { + switch h.Val() { + case "force_modify_query": + rewr.ForceModifyQuery = true + + default: + return h.Errf("unknown subdirective: %s", h.Val()) + } + } + return nil + } + // with only one arg, assume it's a rewrite URI with no matcher token if argsCount == 1 { if !h.NextArg() { return nil, h.ArgErr() } - return h.NewRoute(nil, Rewrite{URI: h.Val()}), nil + rewr := Rewrite{URI: h.Val()} + err := parseBlock(&rewr) + if err != nil { + return nil, err + } + return h.NewRoute(nil, rewr), nil } // parse the matcher token into a matcher set @@ -66,7 +86,12 @@ func parseCaddyfileRewrite(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, h.Next() // consume directive name again, matcher parsing does a reset h.Next() // advance to the rewrite URI - return h.NewRoute(userMatcherSet, Rewrite{URI: h.Val()}), nil + rewr := Rewrite{URI: h.Val()} + err = parseBlock(&rewr) + if err != nil { + return nil, err + } + return h.NewRoute(userMatcherSet, rewr), nil } // parseCaddyfileMethod sets up a basic method rewrite handler from Caddyfile tokens. Syntax: diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go index 2b18744db..6c232f8fc 100644 --- a/modules/caddyhttp/rewrite/rewrite.go +++ b/modules/caddyhttp/rewrite/rewrite.go @@ -92,6 +92,17 @@ type Rewrite struct { // Mutates the query string of the URI. Query *queryOps `json:"query,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. + ForceModifyQuery bool `json:"force_modify_query,omitempty"` + logger *zap.Logger } @@ -226,10 +237,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.ForceModifyQuery { query = injectedQuery } + if rewr.ForceModifyQuery { + qsStart = 0 + } } if query != "" { diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go index 81360baee..1f1a6df8c 100644 --- a/modules/caddyhttp/rewrite/rewrite_test.go +++ b/modules/caddyhttp/rewrite/rewrite_test.go @@ -224,6 +224,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}", ForceModifyQuery: true}, + input: newRequest(t, "GET", "/foo"), + expect: newRequest(t, "GET", "?bar=1"), + }, + { + rule: Rewrite{URI: "{test.path_and_query}", ForceModifyQuery: true}, + input: newRequest(t, "GET", "/"), + expect: newRequest(t, "GET", "/foo?bar=1"), + }, { rule: Rewrite{StripPathPrefix: "/prefix"}, @@ -358,6 +375,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