From 63bda6a0dc97e02d32865c31b5e46d2ead86ac7b Mon Sep 17 00:00:00 2001
From: Matthew Holt <mholt@users.noreply.github.com>
Date: Thu, 10 Dec 2020 14:36:46 -0700
Subject: caddyhttp: Clean up internal auto-HTTPS redirect code

Refactor redirect route creation into own function.

Improve condition for appending port.
Fixes a bug manifested through new test case:
TestAutoHTTPRedirectsWithHTTPListenerFirstInAddresses
---
 modules/caddyhttp/autohttps.go | 76 ++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 40 deletions(-)

(limited to 'modules/caddyhttp/autohttps.go')

diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
index fff4c46..9d6612f 100644
--- a/modules/caddyhttp/autohttps.go
+++ b/modules/caddyhttp/autohttps.go
@@ -303,31 +303,11 @@ uniqueDomainsLoop:
 			matcherSet = append(matcherSet, MatchHost(domains))
 		}
 
-		// build the address to which to redirect
 		addr, err := caddy.ParseNetworkAddress(addrStr)
 		if err != nil {
 			return err
 		}
-		redirTo := "https://{http.request.host}"
-		if addr.StartPort != uint(app.httpsPort()) {
-			redirTo += ":" + strconv.Itoa(int(addr.StartPort))
-		}
-		redirTo += "{http.request.uri}"
-
-		// build the route
-		redirRoute := Route{
-			MatcherSets: []MatcherSet{matcherSet},
-			Handlers: []MiddlewareHandler{
-				StaticResponse{
-					StatusCode: WeakString(strconv.Itoa(http.StatusPermanentRedirect)),
-					Headers: http.Header{
-						"Location":   []string{redirTo},
-						"Connection": []string{"close"},
-					},
-					Close: true,
-				},
-			},
-		}
+		redirRoute := app.makeRedirRoute(addr.StartPort, matcherSet)
 
 		// use the network/host information from the address,
 		// but change the port to the HTTP port then rebuild
@@ -355,25 +335,7 @@ uniqueDomainsLoop:
 	// it's not something that should be relied on. We can change this
 	// if we want to.
 	appendCatchAll := func(routes []Route) []Route {
-		redirTo := "https://{http.request.host}"
-		if app.httpsPort() != DefaultHTTPSPort {
-			redirTo += ":" + strconv.Itoa(app.httpsPort())
-		}
-		redirTo += "{http.request.uri}"
-		routes = append(routes, Route{
-			MatcherSets: []MatcherSet{{MatchProtocol("http")}},
-			Handlers: []MiddlewareHandler{
-				StaticResponse{
-					StatusCode: WeakString(strconv.Itoa(http.StatusPermanentRedirect)),
-					Headers: http.Header{
-						"Location":   []string{redirTo},
-						"Connection": []string{"close"},
-					},
-					Close: true,
-				},
-			},
-		})
-		return routes
+		return append(routes, app.makeRedirRoute(uint(app.httpsPort()), MatcherSet{MatchProtocol("http")}))
 	}
 
 redirServersLoop:
@@ -422,6 +384,40 @@ redirServersLoop:
 	return nil
 }
 
+func (app *App) makeRedirRoute(redirToPort uint, matcherSet MatcherSet) Route {
+	redirTo := "https://{http.request.host}"
+
+	// since this is an external redirect, we should only append an explicit
+	// port if we know it is not the officially standardized HTTPS port, and,
+	// notably, also not the port that Caddy thinks is the HTTPS port (the
+	// configurable HTTPSPort parameter) - we can't change the standard HTTPS
+	// port externally, so that config parameter is for internal use only;
+	// we also do not append the port if it happens to be the HTTP port as
+	// well, obviously (for example, user defines the HTTP port explicitly
+	// in the list of listen addresses for a server)
+	if redirToPort != uint(app.httpPort()) &&
+		redirToPort != uint(app.httpsPort()) &&
+		redirToPort != DefaultHTTPPort &&
+		redirToPort != DefaultHTTPSPort {
+		redirTo += ":" + strconv.Itoa(int(redirToPort))
+	}
+
+	redirTo += "{http.request.uri}"
+	return Route{
+		MatcherSets: []MatcherSet{matcherSet},
+		Handlers: []MiddlewareHandler{
+			StaticResponse{
+				StatusCode: WeakString(strconv.Itoa(http.StatusPermanentRedirect)),
+				Headers: http.Header{
+					"Location":   []string{redirTo},
+					"Connection": []string{"close"},
+				},
+				Close: true,
+			},
+		},
+	}
+}
+
 // createAutomationPolicy ensures that automated certificates for this
 // app are managed properly. This adds up to two automation policies:
 // one for the public names, and one for the internal names. If a catch-all
-- 
cgit v1.2.3