diff --git a/caddyhttp/httpserver/https.go b/caddyhttp/httpserver/https.go index 75ff8c6c..912e9f93 100644 --- a/caddyhttp/httpserver/https.go +++ b/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 diff --git a/caddyhttp/httpserver/https_test.go b/caddyhttp/httpserver/https_test.go index 04dcbaea..82a12700 100644 --- a/caddyhttp/httpserver/https_test.go +++ b/caddyhttp/httpserver/https_test.go @@ -1,6 +1,8 @@ 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), - }) + { + 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, "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) - } + // 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) + } - // 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) - } + // 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) + } - handler := cfg.middleware[0](nil) + handler := cfg.middleware[0](nil) - // 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) - } + // 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) + } - // 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) - - 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) + } } }