From 441d5eb0628c4a9fbe74c13eca6ab255056e3e57 Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Thu, 23 Apr 2026 01:29:03 -0600 Subject: [PATCH] caddyhttp: prefer port 443 in auto-HTTPS and add tests (#7666) --- caddytest/integration/autohttps_test.go | 22 +++++++++++++ modules/caddyhttp/autohttps.go | 38 +++++++++++++++++---- modules/caddyhttp/autohttps_test.go | 44 +++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 7 deletions(-) create mode 100644 modules/caddyhttp/autohttps_test.go diff --git a/caddytest/integration/autohttps_test.go b/caddytest/integration/autohttps_test.go index fdfb5a93e..88a0aee03 100644 --- a/caddytest/integration/autohttps_test.go +++ b/caddytest/integration/autohttps_test.go @@ -55,6 +55,28 @@ func TestAutoHTTPtoHTTPSRedirectsExplicitPortDifferentFromHTTPSPort(t *testing.T tester.AssertRedirect("http://localhost:9080/", "https://localhost:1234/", http.StatusPermanentRedirect) } +func TestAutoHTTPtoHTTPSRedirectsPreferHTTPSPortOverAlternatePort(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(` + { + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + local_certs + } + localhost { + respond "Canonical" + } + + localhost:10443 { + respond "Alternate" + } + `, "caddyfile") + + tester.AssertRedirect("http://localhost:9080/", "https://localhost/", http.StatusPermanentRedirect) +} + func TestAutoHTTPRedirectsWithHTTPListenerFirstInAddresses(t *testing.T) { tester := caddytest.NewTester(t) tester.InitServer(` diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go index 32e9f106d..4d9759000 100644 --- a/modules/caddyhttp/autohttps.go +++ b/modules/caddyhttp/autohttps.go @@ -258,18 +258,13 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er // an empty string to indicate a catch-all, which we have to // treat special later if len(serverDomainSet) == 0 { - redirDomains[""] = append(redirDomains[""], addr) + app.recordAutoHTTPSRedirectAddress(redirDomains, "", addr) continue } // ...and associate it with each domain in this server for d := range serverDomainSet { - // if this domain is used on more than one HTTPS-enabled - // port, we'll have to choose one, so prefer the HTTPS port - if _, ok := redirDomains[d]; !ok || - addr.StartPort == uint(app.httpsPort()) { - redirDomains[d] = append(redirDomains[d], addr) - } + app.recordAutoHTTPSRedirectAddress(redirDomains, d, addr) } } } @@ -517,6 +512,35 @@ redirServersLoop: return nil } +// recordAutoHTTPSRedirectAddress stores redirect destinations for one domain +// using a single winning port while keeping all bind addresses on that port. +// +// This is needed to avoid two opposite regressions in auto-HTTPS redirects: +// preserve all listener addresses when a site binds multiple addresses on the +// same HTTPS port, but do not mix in alternate HTTPS ports when the canonical +// app HTTPS port is also available. +func (app *App) recordAutoHTTPSRedirectAddress(redirDomains map[string][]caddy.NetworkAddress, domain string, addr caddy.NetworkAddress) { + existing := redirDomains[domain] + if len(existing) == 0 { + redirDomains[domain] = []caddy.NetworkAddress{addr} + return + } + + existingPort := existing[0].StartPort + if addr.StartPort != existingPort { + if addr.StartPort == uint(app.httpsPort()) && existingPort != uint(app.httpsPort()) { + redirDomains[domain] = []caddy.NetworkAddress{addr} + } + return + } + + if slices.Contains(existing, addr) { + return + } + + redirDomains[domain] = append(existing, addr) +} + func (app *App) makeRedirRoute(redirToPort uint, matcherSet MatcherSet) Route { redirTo := "https://{http.request.host}" diff --git a/modules/caddyhttp/autohttps_test.go b/modules/caddyhttp/autohttps_test.go new file mode 100644 index 000000000..b5cc64d94 --- /dev/null +++ b/modules/caddyhttp/autohttps_test.go @@ -0,0 +1,44 @@ +package caddyhttp + +import ( + "testing" + + "github.com/caddyserver/caddy/v2" +) + +func TestRecordAutoHTTPSRedirectAddressPrefersHTTPSPort(t *testing.T) { + app := &App{HTTPSPort: 443} + redirDomains := make(map[string][]caddy.NetworkAddress) + + app.recordAutoHTTPSRedirectAddress(redirDomains, "example.com", caddy.NetworkAddress{Network: "tcp", StartPort: 2345, EndPort: 2345}) + app.recordAutoHTTPSRedirectAddress(redirDomains, "example.com", caddy.NetworkAddress{Network: "tcp", StartPort: 443, EndPort: 443}) + app.recordAutoHTTPSRedirectAddress(redirDomains, "example.com", caddy.NetworkAddress{Network: "tcp", StartPort: 8443, EndPort: 8443}) + + got := redirDomains["example.com"] + if len(got) != 1 { + t.Fatalf("expected 1 redirect address, got %d: %#v", len(got), got) + } + if got[0].StartPort != 443 { + t.Fatalf("expected redirect to prefer HTTPS port 443, got %#v", got[0]) + } +} + +func TestRecordAutoHTTPSRedirectAddressKeepsAllBindAddressesOnWinningPort(t *testing.T) { + app := &App{HTTPSPort: 443} + redirDomains := make(map[string][]caddy.NetworkAddress) + + app.recordAutoHTTPSRedirectAddress(redirDomains, "example.com", caddy.NetworkAddress{Network: "tcp", Host: "10.0.0.189", StartPort: 8443, EndPort: 8443}) + app.recordAutoHTTPSRedirectAddress(redirDomains, "example.com", caddy.NetworkAddress{Network: "tcp", Host: "10.0.0.189", StartPort: 443, EndPort: 443}) + app.recordAutoHTTPSRedirectAddress(redirDomains, "example.com", caddy.NetworkAddress{Network: "tcp", Host: "2603:c024:8002:9500:9eb:e5d3:3975:d056", StartPort: 443, EndPort: 443}) + + got := redirDomains["example.com"] + if len(got) != 2 { + t.Fatalf("expected 2 redirect addresses for both bind addresses on the winning port, got %d: %#v", len(got), got) + } + if got[0].StartPort != 443 || got[1].StartPort != 443 { + t.Fatalf("expected both redirect addresses to stay on HTTPS port 443, got %#v", got) + } + if got[0].Host != "10.0.0.189" || got[1].Host != "2603:c024:8002:9500:9eb:e5d3:3975:d056" { + t.Fatalf("expected both bind addresses to be preserved, got %#v", got) + } +}