try_files, rewrite: allow query string in try_files (fix #2891)

Also some minor cleanup/improvements discovered along the way

4 files changed, 57 insertions, 23 deletions

diff --git a/caddyconfig/httpcaddyfile/directives.go b/caddyconfig/httpcaddyfile/directives.go
index 58aff98..5ec4993 100644
--- a/caddyconfig/httpcaddyfile/directives.go
+++ b/caddyconfig/httpcaddyfile/directives.go
@@ -53,6 +53,8 @@ func RegisterDirective(dir string, setupFunc UnmarshalFunc) {
 
 // RegisterHandlerDirective is like RegisterDirective, but for
 // directives which specifically output only an HTTP handler.
+// Directives registered with this function will always have
+// an optional matcher token as the first argument.
 func RegisterHandlerDirective(dir string, setupFunc UnmarshalHandlerFunc) {
 	RegisterDirective(dir, func(h Helper) ([]ConfigValue, error) {
 		if !h.Next() {
@@ -117,7 +119,7 @@ func (h Helper) Caddyfiles() []string {
 }
 
 // JSON converts val into JSON. Any errors are added to warnings.
-func (h Helper) JSON(val interface{}, warnings *[]caddyconfig.Warning) json.RawMessage {
+func (h Helper) JSON(val interface{}) json.RawMessage {
 	return caddyconfig.JSON(val, h.warnings)
 }
 
@@ -135,9 +137,14 @@ func (h Helper) MatcherToken() (caddy.ModuleMap, bool, error) {
 // NewRoute returns config values relevant to creating a new HTTP route.
 func (h Helper) NewRoute(matcherSet caddy.ModuleMap,
 	handler caddyhttp.MiddlewareHandler) []ConfigValue {
-	mod, err := caddy.GetModule(caddy.GetModuleName(handler))
+	mod, err := caddy.GetModule(caddy.GetModuleID(handler))
 	if err != nil {
-		// TODO: append to warnings
+		*h.warnings = append(*h.warnings, caddyconfig.Warning{
+			File:    h.File(),
+			Line:    h.Line(),
+			Message: err.Error(),
+		})
+		return nil
 	}
 	var matcherSetsRaw []caddy.ModuleMap
 	if matcherSet != nil {
diff --git a/modules/caddyhttp/fileserver/caddyfile.go b/modules/caddyhttp/fileserver/caddyfile.go
index fb931a1..8013fa2 100644
--- a/modules/caddyhttp/fileserver/caddyfile.go
+++ b/modules/caddyhttp/fileserver/caddyfile.go
@@ -15,6 +15,8 @@
 package fileserver
 
 import (
+	"strings"
+
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
@@ -98,7 +100,7 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 //
 //    try_files <files...>
 //
-// and is shorthand for:
+// and is basically shorthand for:
 //
 //    matcher:try_files {
 //        file {
@@ -107,25 +109,55 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 //    }
 //    rewrite match:try_files {http.matchers.file.relative}{http.request.uri.query_string}
 //
+// 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.
 func parseTryFiles(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) {
 	if !h.Next() {
 		return nil, h.ArgErr()
 	}
 
-	try := h.RemainingArgs()
-	if len(try) == 0 {
+	tryFiles := h.RemainingArgs()
+	if len(tryFiles) == 0 {
 		return nil, h.ArgErr()
 	}
 
-	handler := rewrite.Rewrite{
-		URI: "{http.matchers.file.relative}{http.request.uri.query_string}",
+	// 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 {
+		handler := rewrite.Rewrite{
+			URI: "{http.matchers.file.relative}{http.request.uri.query_string}" + writeURIAppend,
+		}
+		matcherSet := caddy.ModuleMap{
+			"file": h.JSON(MatchFile{
+				TryFiles: try,
+			}),
+		}
+		return h.NewRoute(matcherSet, handler)
 	}
 
-	matcherSet := caddy.ModuleMap{
-		"file": h.JSON(MatchFile{
-			TryFiles: try,
-		}, nil),
+	var result []httpcaddyfile.ConfigValue
+
+	// if there are query strings in the list, we have to split into
+	// a separate route for each item with a query string, because
+	// the rewrite is different for that item
+	var try []string
+	for _, item := range tryFiles {
+		if idx := strings.Index(item, "?"); idx >= 0 {
+			if len(try) > 0 {
+				result = append(result, makeRoute(try, "")...)
+				try = []string{}
+			}
+			result = append(result, makeRoute([]string{item[:idx]}, "&"+item[idx+1:])...)
+			continue
+		}
+		// accumulate consecutive non-query-string parameters
+		try = append(try, item)
+	}
+	if len(try) > 0 {
+		result = append(result, makeRoute(try, "")...)
 	}
 
-	return h.NewRoute(matcherSet, handler), nil
+	return result, nil
 }
diff --git a/modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go b/modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go
index 8e723b2..699f680 100644
--- a/modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go
+++ b/modules/caddyhttp/reverseproxy/fastcgi/caddyfile.go
@@ -125,12 +125,12 @@ func parsePHPFastCGI(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error
 	redirMatcherSet := caddy.ModuleMap{
 		"file": h.JSON(fileserver.MatchFile{
 			TryFiles: []string{"{http.request.uri.path}/index.php"},
-		}, nil),
+		}),
 		"not": h.JSON(caddyhttp.MatchNegate{
 			MatchersRaw: caddy.ModuleMap{
-				"path": h.JSON(caddyhttp.MatchPath{"*/"}, nil),
+				"path": h.JSON(caddyhttp.MatchPath{"*/"}),
 			},
-		}, nil),
+		}),
 	}
 	redirHandler := caddyhttp.StaticResponse{
 		StatusCode: caddyhttp.WeakString("308"),
@@ -145,7 +145,7 @@ func parsePHPFastCGI(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error
 	rewriteMatcherSet := caddy.ModuleMap{
 		"file": h.JSON(fileserver.MatchFile{
 			TryFiles: []string{"{http.request.uri.path}", "{http.request.uri.path}/index.php", "index.php"},
-		}, nil),
+		}),
 	}
 	rewriteHandler := rewrite.Rewrite{
 		URI:      "{http.matchers.file.relative}{http.request.uri.query_string}",
@@ -159,7 +159,7 @@ func parsePHPFastCGI(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error
 	// route to actually reverse proxy requests to PHP files;
 	// match only requests that are for PHP files
 	rpMatcherSet := caddy.ModuleMap{
-		"path": h.JSON([]string{"*.php"}, nil),
+		"path": h.JSON([]string{"*.php"}),
 	}
 
 	// if the user specified a matcher token, use that
diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index a142080..adb90f4 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -190,11 +190,6 @@ func (rep replacer) do(r *http.Request, repl caddy.Replacer) bool {
 	r.URL.Path = strings.Replace(oldPath, find, replace, lim)
 	r.URL.RawQuery = strings.Replace(oldQuery, find, replace, lim)
 
-	// changed := r.URL.Path != oldPath && r.URL.RawQuery != oldQuery
-	// if changed {
-	// 	r.RequestURI = r.URL.RequestURI()
-	// }
-
 	return r.URL.Path != oldPath && r.URL.RawQuery != oldQuery
 }