httpserver: Proper HTTP->HTTPS for wildcard sites (fixes #1625)

caddyhttp/httpserver/https.go

@@ -146,13 +146,20 @@ func redirPlaintextHost(cfg *SiteConfig) *SiteConfig {
 	}
 	redirMiddleware := func(next Handler) Handler {
 		return HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) {
+			// Construct the URL to which to redirect. Note that the Host in a request might
+			// contain a port, but we just need the hostname; we'll set the port if needed.
 			toURL := "https://"
+			requestHost, _, err := net.SplitHostPort(r.Host)
+			if err != nil {
+				requestHost = r.Host // Host did not contain a port; great
+			}
 			if redirPort == "" {
-				toURL += cfg.Addr.Host // don't use r.Host as it may have a port included
+				toURL += requestHost
 			} else {
-				toURL += net.JoinHostPort(cfg.Addr.Host, redirPort)
+				toURL += net.JoinHostPort(requestHost, redirPort)
 			}
 			toURL += r.URL.RequestURI()
+
 			w.Header().Set("Connection", "close")
 			http.Redirect(w, r, toURL, http.StatusMovedPermanently)
 			return 0, nil

caddyhttp/httpserver/https_test.go

 package httpserver
 
 import (
+	"fmt"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -9,75 +11,107 @@ import (
 )
 
 func TestRedirPlaintextHost(t *testing.T) {
-	cfg := redirPlaintextHost(&SiteConfig{
-		Addr: Address{
+	for i, testcase := range []struct {
+		Host        string // used for the site config
+		Port        string
+		ListenHost  string
+		RequestHost string // if different from Host
+	}{
+		{
+			Host: "foohost",
+		},
+		{
+			Host: "foohost",
+			Port: "80",
+		},
+		{
 			Host: "foohost",
 			Port: "1234",
 		},
-		ListenHost: "93.184.216.34",
-		TLS:        new(caddytls.Config),
-	})
-
-	// Check host and port
-	if actual, expected := cfg.Addr.Host, "foohost"; actual != expected {
-		t.Errorf("Expected redir config to have host %s but got %s", expected, actual)
-	}
-	if actual, expected := cfg.ListenHost, "93.184.216.34"; actual != expected {
-		t.Errorf("Expected redir config to have bindhost %s but got %s", expected, actual)
-	}
-	if actual, expected := cfg.Addr.Port, "80"; actual != expected {
-		t.Errorf("Expected redir config to have port '%s' but got '%s'", expected, actual)
-	}
-
-	// Make sure redirect handler is set up properly
-	if cfg.middleware == nil || len(cfg.middleware) != 1 {
-		t.Fatalf("Redir config middleware not set up properly; got: %#v", cfg.middleware)
-	}
+		{
+			Host:       "foohost",
+			ListenHost: "93.184.216.34",
+		},
+		{
+			Host:       "foohost",
+			Port:       "1234",
+			ListenHost: "93.184.216.34",
+		},
+		{
+			Host: "foohost",
+			Port: "443", // since this is the default HTTPS port, should not be included in Location value
+		},
+		{
+			Host:        "*.example.com",
+			RequestHost: "foo.example.com",
+		},
+		{
+			Host:        "*.example.com",
+			Port:        "1234",
+			RequestHost: "foo.example.com:1234",
+		},
+	} {
+		cfg := redirPlaintextHost(&SiteConfig{
+			Addr: Address{
+				Host: testcase.Host,
+				Port: testcase.Port,
+			},
+			ListenHost: testcase.ListenHost,
+			TLS:        new(caddytls.Config),
+		})
+
+		// Check host and port
+		if actual, expected := cfg.Addr.Host, testcase.Host; actual != expected {
+			t.Errorf("Test %d: Expected redir config to have host %s but got %s", i, expected, actual)
+		}
+		if actual, expected := cfg.ListenHost, testcase.ListenHost; actual != expected {
+			t.Errorf("Test %d: Expected redir config to have bindhost %s but got %s", i, expected, actual)
+		}
+		if actual, expected := cfg.Addr.Port, HTTPPort; actual != expected {
+			t.Errorf("Test %d: Expected redir config to have port '%s' but got '%s'", i, expected, actual)
+		}
 
-	handler := cfg.middleware[0](nil)
+		// Make sure redirect handler is set up properly
+		if cfg.middleware == nil || len(cfg.middleware) != 1 {
+			t.Fatalf("Test %d: Redir config middleware not set up properly; got: %#v", i, cfg.middleware)
+		}
 
-	// Check redirect for correctness
-	rec := httptest.NewRecorder()
-	req, err := http.NewRequest("GET", "http://foohost/bar?q=1", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-	status, err := handler.ServeHTTP(rec, req)
-	if status != 0 {
-		t.Errorf("Expected status return to be 0, but was %d", status)
-	}
-	if err != nil {
-		t.Errorf("Expected returned error to be nil, but was %v", err)
-	}
-	if rec.Code != http.StatusMovedPermanently {
-		t.Errorf("Expected status %d but got %d", http.StatusMovedPermanently, rec.Code)
-	}
-	if got, want := rec.Header().Get("Location"), "https://foohost:1234/bar?q=1"; got != want {
-		t.Errorf("Expected Location: '%s' but got '%s'", want, got)
-	}
+		handler := cfg.middleware[0](nil)
 
-	// browsers can infer a default port from scheme, so make sure the port
-	// doesn't get added in explicitly for default ports like 443 for https.
-	cfg = redirPlaintextHost(&SiteConfig{Addr: Address{Host: "foohost", Port: "443"}, TLS: new(caddytls.Config)})
-	handler = cfg.middleware[0](nil)
+		// Check redirect for correctness, first by inspecting error and status code
+		requestHost := testcase.Host // hostname of request might be different than in config (e.g. wildcards)
+		if testcase.RequestHost != "" {
+			requestHost = testcase.RequestHost
+		}
+		rec := httptest.NewRecorder()
+		req, err := http.NewRequest("GET", "http://"+requestHost+"/bar?q=1", nil)
+		if err != nil {
+			t.Fatalf("Test %d: %v", i, err)
+		}
+		status, err := handler.ServeHTTP(rec, req)
+		if status != 0 {
+			t.Errorf("Test %d: Expected status return to be 0, but was %d", i, status)
+		}
+		if err != nil {
+			t.Errorf("Test %d: Expected returned error to be nil, but was %v", i, err)
+		}
+		if rec.Code != http.StatusMovedPermanently {
+			t.Errorf("Test %d: Expected status %d but got %d", http.StatusMovedPermanently, i, rec.Code)
+		}
 
-	rec = httptest.NewRecorder()
-	req, err = http.NewRequest("GET", "http://foohost/bar?q=1", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-	status, err = handler.ServeHTTP(rec, req)
-	if status != 0 {
-		t.Errorf("Expected status return to be 0, but was %d", status)
-	}
-	if err != nil {
-		t.Errorf("Expected returned error to be nil, but was %v", err)
-	}
-	if rec.Code != http.StatusMovedPermanently {
-		t.Errorf("Expected status %d but got %d", http.StatusMovedPermanently, rec.Code)
-	}
-	if got, want := rec.Header().Get("Location"), "https://foohost/bar?q=1"; got != want {
-		t.Errorf("Expected Location: '%s' but got '%s'", want, got)
+		// Now check the Location value. It should mirror the hostname and port of the request
+		// unless the port is redundant, in which case it should be dropped.
+		locationHost, _, err := net.SplitHostPort(requestHost)
+		if err != nil {
+			locationHost = requestHost
+		}
+		expectedLoc := fmt.Sprintf("https://%s/bar?q=1", locationHost)
+		if testcase.Port != "" && testcase.Port != DefaultHTTPSPort {
+			expectedLoc = fmt.Sprintf("https://%s:%s/bar?q=1", locationHost, testcase.Port)
+		}
+		if got, want := rec.Header().Get("Location"), expectedLoc; got != want {
+			t.Errorf("Test %d: Expected Location: '%s' but got '%s'", i, want, got)
+		}
 	}
 }