http: A little more polish on rewrite handler and try_files directive

5 files changed, 51 insertions, 48 deletions

diff --git a/modules/caddyhttp/fileserver/caddyfile.go b/modules/caddyhttp/fileserver/caddyfile.go
index aa11ba0..d26b435 100644
--- a/modules/caddyhttp/fileserver/caddyfile.go
+++ b/modules/caddyhttp/fileserver/caddyfile.go
@@ -107,11 +107,17 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 //            try_files <files...>
 //        }
 //    }
-//    rewrite @try_files {http.matchers.file.relative}{http.request.uri.query_string}
+//    rewrite @try_files {http.matchers.file.relative}
 //
-// If any of the files in the list have a query string, the query string will
-// be ignored when checking for file existence, but will be augmented into
-// the request's URI when rewriting the request.
+// This directive rewrites request paths only, preserving any other part
+// of the URI, unless the part is explicitly given in the file list. For
+// example, if any of the files in the list have a query string:
+//
+//    try_files {path} index.php?{query}&p={path}
+//
+// then the query string will not be treated as part of the file name; and
+// if that file matches, the given query string will replace any query string
+// that already exists on the request URI.
 func parseTryFiles(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) {
 	if !h.Next() {
 		return nil, h.ArgErr()
@@ -123,16 +129,14 @@ func parseTryFiles(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error)
 	}
 
 	// makeRoute returns a route that tries the files listed in try
-	// and then rewrites to the matched file, and appends writeURIAppend
-	// to the end of the query string.
-	makeRoute := func(try []string, writeURIAppend string) []httpcaddyfile.ConfigValue {
+	// and then rewrites to the matched file; userQueryString is
+	// appended to the rewrite rule.
+	makeRoute := func(try []string, userQueryString string) []httpcaddyfile.ConfigValue {
 		handler := rewrite.Rewrite{
-			URI: "{http.matchers.file.relative}{http.request.uri.query_string}" + writeURIAppend,
+			URI: "{http.matchers.file.relative}" + userQueryString,
 		}
 		matcherSet := caddy.ModuleMap{
-			"file": h.JSON(MatchFile{
-				TryFiles: try,
-			}),
+			"file": h.JSON(MatchFile{TryFiles: try}),
 		}
 		return h.NewRoute(matcherSet, handler)
 	}
@@ -149,7 +153,7 @@ func parseTryFiles(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error)
 				result = append(result, makeRoute(try, "")...)
 				try = []string{}
 			}
-			result = append(result, makeRoute([]string{item[:idx]}, "&"+item[idx+1:])...)
+			result = append(result, makeRoute([]string{item[:idx]}, item[idx:])...)
 			continue
 		}
 		// accumulate consecutive non-query-string parameters
diff --git a/modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go b/modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go
index ca0d6cc..dee6eb5 100644
--- a/modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go
+++ b/modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go
@@ -149,7 +149,7 @@ func parsePHPFastCGI(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error
 		}),
 	}
 	rewriteHandler := rewrite.Rewrite{
-		URI: "{http.matchers.file.relative}?{http.request.uri.query}",
+		URI: "{http.matchers.file.relative}",
 	}
 	rewriteRoute := caddyhttp.Route{
 		MatcherSetsRaw: []caddy.ModuleMap{rewriteMatcherSet},
diff --git a/modules/caddyhttp/rewrite/caddyfile.go b/modules/caddyhttp/rewrite/caddyfile.go
index 368db15..6ec51c0 100644
--- a/modules/caddyhttp/rewrite/caddyfile.go
+++ b/modules/caddyhttp/rewrite/caddyfile.go
@@ -32,7 +32,8 @@ func init() {
 //
 //     rewrite [<matcher>] <to>
 //
-// The <to> parameter becomes the new URI.
+// 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.
 func parseCaddyfileRewrite(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) {
 	var rewr Rewrite
 	for h.Next() {
@@ -58,7 +59,7 @@ func parseCaddyfileStripPrefix(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHand
 		if !h.NextArg() {
 			return nil, h.ArgErr()
 		}
-		rewr.StripPrefix = h.Val()
+		rewr.StripPathPrefix = h.Val()
 		if h.NextArg() {
 			return nil, h.ArgErr()
 		}
@@ -77,7 +78,7 @@ func parseCaddyfileStripSuffix(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHand
 		if !h.NextArg() {
 			return nil, h.ArgErr()
 		}
-		rewr.StripSuffix = h.Val()
+		rewr.StripPathSuffix = h.Val()
 		if h.NextArg() {
 			return nil, h.ArgErr()
 		}
diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index c069db9..abcaa84 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -17,7 +17,6 @@ package rewrite
 import (
 	"net/http"
 	"net/url"
-	"strconv"
 	"strings"
 
 	"github.com/caddyserver/caddy/v2"
@@ -31,32 +30,40 @@ func init() {
 
 // Rewrite is a middleware which can rewrite HTTP requests.
 //
-// These rewrite properties are applied to a request in this order:
-// Method, URI, StripPrefix, StripSuffix, URISubstring.
-//
-// TODO: This module is still a WIP and may experience breaking changes.
+// The Method and URI properties are "setters": the request URI
+// will be set to the given values. Other properties are "modifiers":
+// they modify existing files but do not explicitly specify what the
+// result will be. It is atypical to combine the use of setters and
+// modifiers in a single rewrite.
 type Rewrite struct {
 	// Changes the request's HTTP verb.
 	Method string `json:"method,omitempty"`
 
-	// Changes the request's URI (path, query string, and fragment if present).
+	// Changes the request's URI, which consists of path and query string.
 	// Only components of the URI that are specified will be changed.
+	// For example, a value of "/foo.html" or "foo.html" will only change
+	// the path and will preserve any existing query string. Similarly, a
+	// value of "?a=b" will only change the query string and will not affect
+	// the path. Both can also be changed: "/foo?a=b" - this sets both the
+	// path and query string at the same time.
+	//
+	// You can also use placeholders. For example, to preserve the existing
+	// query string, you might use: "?{http.request.uri.query}&a=b". Any
+	// key-value pairs you add to the query string will not overwrite
+	// existing values.
+	//
+	// To clear the query string, explicitly set an empty one: "?"
 	URI string `json:"uri,omitempty"`
 
 	// Strips the given prefix from the beginning of the URI path.
-	StripPrefix string `json:"strip_prefix,omitempty"`
+	StripPathPrefix string `json:"strip_path_prefix,omitempty"`
 
 	// Strips the given suffix from the end of the URI path.
-	StripSuffix string `json:"strip_suffix,omitempty"`
+	StripPathSuffix string `json:"strip_path_suffix,omitempty"`
 
 	// Performs substring replacements on the URI.
 	URISubstring []replacer `json:"uri_substring,omitempty"`
 
-	// If set to a 3xx HTTP status code and if the URI was rewritten (changed),
-	// the handler will issue a simple HTTP redirect to the new URI using the
-	// given status code.
-	HTTPRedirect caddyhttp.WeakString `json:"http_redirect,omitempty"`
-
 	logger *zap.Logger
 }
 
@@ -88,15 +95,6 @@ func (rewr Rewrite) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddy
 			zap.String("method", r.Method),
 			zap.String("uri", r.RequestURI),
 		)
-		if rewr.HTTPRedirect != "" {
-			statusCode, err := strconv.Atoi(repl.ReplaceAll(rewr.HTTPRedirect.String(), ""))
-			if err != nil {
-				return caddyhttp.Error(http.StatusInternalServerError, err)
-			}
-			w.Header().Set("Location", r.RequestURI)
-			w.WriteHeader(statusCode)
-			return nil
-		}
 	}
 
 	return next.ServeHTTP(w, r)
@@ -148,12 +146,12 @@ func (rewr Rewrite) rewrite(r *http.Request, repl *caddy.Replacer, logger *zap.L
 	}
 
 	// strip path prefix or suffix
-	if rewr.StripPrefix != "" {
-		prefix := repl.ReplaceAll(rewr.StripPrefix, "")
+	if rewr.StripPathPrefix != "" {
+		prefix := repl.ReplaceAll(rewr.StripPathPrefix, "")
 		r.URL.Path = strings.TrimPrefix(r.URL.Path, prefix)
 	}
-	if rewr.StripSuffix != "" {
-		suffix := repl.ReplaceAll(rewr.StripSuffix, "")
+	if rewr.StripPathSuffix != "" {
+		suffix := repl.ReplaceAll(rewr.StripPathSuffix, "")
 		r.URL.Path = strings.TrimSuffix(r.URL.Path, suffix)
 	}
 
diff --git a/modules/caddyhttp/rewrite/rewrite_test.go b/modules/caddyhttp/rewrite/rewrite_test.go
index de82d8d..3dbc2d6 100644
--- a/modules/caddyhttp/rewrite/rewrite_test.go
+++ b/modules/caddyhttp/rewrite/rewrite_test.go
@@ -150,33 +150,33 @@ func TestRewrite(t *testing.T) {
 		},
 
 		{
-			rule:   Rewrite{StripPrefix: "/prefix"},
+			rule:   Rewrite{StripPathPrefix: "/prefix"},
 			input:  newRequest(t, "GET", "/foo/bar"),
 			expect: newRequest(t, "GET", "/foo/bar"),
 		},
 		{
-			rule:   Rewrite{StripPrefix: "/prefix"},
+			rule:   Rewrite{StripPathPrefix: "/prefix"},
 			input:  newRequest(t, "GET", "/prefix/foo/bar"),
 			expect: newRequest(t, "GET", "/foo/bar"),
 		},
 		{
-			rule:   Rewrite{StripPrefix: "/prefix"},
+			rule:   Rewrite{StripPathPrefix: "/prefix"},
 			input:  newRequest(t, "GET", "/foo/prefix/bar"),
 			expect: newRequest(t, "GET", "/foo/prefix/bar"),
 		},
 
 		{
-			rule:   Rewrite{StripSuffix: "/suffix"},
+			rule:   Rewrite{StripPathSuffix: "/suffix"},
 			input:  newRequest(t, "GET", "/foo/bar"),
 			expect: newRequest(t, "GET", "/foo/bar"),
 		},
 		{
-			rule:   Rewrite{StripSuffix: "suffix"},
+			rule:   Rewrite{StripPathSuffix: "suffix"},
 			input:  newRequest(t, "GET", "/foo/bar/suffix"),
 			expect: newRequest(t, "GET", "/foo/bar/"),
 		},
 		{
-			rule:   Rewrite{StripSuffix: "/suffix"},
+			rule:   Rewrite{StripPathSuffix: "/suffix"},
 			input:  newRequest(t, "GET", "/foo/suffix/bar"),
 			expect: newRequest(t, "GET", "/foo/suffix/bar"),
 		},