caddyhttp: Implement better logic for inserting the HTTP->HTTPS redirs (#4033)

* caddyhttp: Implement better logic for inserting the HTTP->HTTPS redirs

* caddyhttp: Add integration test

3 files changed, 69 insertions, 14 deletions

diff --git a/caddytest/integration/autohttps_test.go b/caddytest/integration/autohttps_test.go
index db6329a..72968e9 100644
--- a/caddytest/integration/autohttps_test.go
+++ b/caddytest/integration/autohttps_test.go
@@ -80,3 +80,26 @@ func TestAutoHTTPRedirectsWithHTTPListenerFirstInAddresses(t *testing.T) {
 `, "json")
 	tester.AssertRedirect("http://localhost:9080/", "https://localhost/", http.StatusPermanentRedirect)
 }
+
+func TestAutoHTTPRedirectsInsertedBeforeUserDefinedCatchAll(t *testing.T) {
+	tester := caddytest.NewTester(t)
+	tester.InitServer(`
+	{
+		http_port     9080
+		https_port    9443
+		local_certs
+	}
+	http://:9080 {
+		respond "Foo"
+	}
+	http://baz.localhost:9080 {
+		respond "Baz"
+	}
+	bar.localhost {
+		respond "Bar"
+	}
+  `, "caddyfile")
+	tester.AssertRedirect("http://bar.localhost:9080/", "https://bar.localhost/", http.StatusPermanentRedirect)
+	tester.AssertGetResponse("http://foo.localhost:9080/", 200, "Foo")
+	tester.AssertGetResponse("http://baz.localhost:9080/", 200, "Baz")
+}
diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
index 5c83d8f..da4428d 100644
--- a/modules/caddyhttp/autohttps.go
+++ b/modules/caddyhttp/autohttps.go
@@ -342,21 +342,22 @@ redirServersLoop:
 	for redirServerAddr, routes := range redirServers {
 		// for each redirect listener, see if there's already a
 		// server configured to listen on that exact address; if so,
-		// simply add the redirect route to the end of its route
-		// list; otherwise, we'll create a new server for all the
-		// listener addresses that are unused and serve the
-		// remaining redirects from it
-		for srvName, srv := range app.Servers {
+		// insert the redirect route to the end of its route list
+		// after any other routes with host matchers; otherwise,
+		// we'll create a new server for all the listener addresses
+		// that are unused and serve the remaining redirects from it
+		for _, srv := range app.Servers {
 			if srv.hasListenerAddress(redirServerAddr) {
-				// user has configured a server for the same address
-				// that the redirect runs from; simply append our
-				// redirect route to the existing routes, with a
-				// caveat that their config might override ours
-				app.logger.Warn("user server is listening on same interface as automatic HTTP->HTTPS redirects; user-configured routes might override these redirects",
-					zap.String("server_name", srvName),
-					zap.String("interface", redirServerAddr),
-				)
-				srv.Routes = append(srv.Routes, appendCatchAll(routes)...)
+				// find the index of the route after the last route with a host
+				// matcher, then insert the redirects there, but before any
+				// user-defined catch-all routes
+				// see https://github.com/caddyserver/caddy/issues/3212
+				insertIndex := srv.findLastRouteWithHostMatcher()
+				srv.Routes = append(srv.Routes[:insertIndex], append(routes, srv.Routes[insertIndex:]...)...)
+
+				// append our catch-all route in case the user didn't define their own
+				srv.Routes = appendCatchAll(srv.Routes)
+
 				continue redirServersLoop
 			}
 		}
diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go
index 27a1ca8..294ee6a 100644
--- a/modules/caddyhttp/server.go
+++ b/modules/caddyhttp/server.go
@@ -373,6 +373,37 @@ func (s *Server) hasTLSClientAuth() bool {
 	return false
 }
 
+// findLastRouteWithHostMatcher returns the index of the last route
+// in the server which has a host matcher. Used during Automatic HTTPS
+// to determine where to insert the HTTP->HTTPS redirect route, such
+// that it is after any other host matcher but before any "catch-all"
+// route without a host matcher.
+func (s *Server) findLastRouteWithHostMatcher() int {
+	lastIndex := len(s.Routes)
+	for i, route := range s.Routes {
+		// since we want to break out of an inner loop, use a closure
+		// to allow us to use 'return' when we found a host matcher
+		found := (func() bool {
+			for _, sets := range route.MatcherSets {
+				for _, matcher := range sets {
+					switch matcher.(type) {
+					case *MatchHost:
+						return true
+					}
+				}
+			}
+			return false
+		})()
+
+		// if we found the host matcher, change the lastIndex to
+		// just after the current route
+		if found {
+			lastIndex = i + 1
+		}
+	}
+	return lastIndex
+}
+
 // HTTPErrorConfig determines how to handle errors
 // from the HTTP handlers.
 type HTTPErrorConfig struct {