Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rewrite: Add option to force modifying the query #5438

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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}"
}
]
}
Expand Down
14 changes: 13 additions & 1 deletion modules/caddyhttp/rewrite/caddyfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ func init() {

// parseCaddyfileRewrite sets up a basic rewrite handler from Caddyfile tokens. Syntax:
//
// rewrite [<matcher>] <to>
// rewrite [<matcher>] <to> {
// modify_query
// }
//
// Only URI components which are given in <to> will be set in the resulting URI.
// See the docs for the rewrite handler for more information.
Expand All @@ -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
}
Expand Down
20 changes: 18 additions & 2 deletions modules/caddyhttp/rewrite/rewrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we call this SpanComponents or something a little more precise? It looks like what this change really does is make a single placeholder able to span URI components.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes me think of a <span> JS component. That name doesn't bring any mental association to what it does to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I mean, obviously this is a backend context (a rewrite middleware) where we're talking about the URI... but I can see why you'd have that correlation.

What about AllowExpansion or something?


logger *zap.Logger
}

Expand Down Expand Up @@ -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 != "" {
Expand Down
20 changes: 20 additions & 0 deletions modules/caddyhttp/rewrite/rewrite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down Expand Up @@ -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
Expand Down