From be96cc0e656de2cac5913508f1dfab79872bfd7e Mon Sep 17 00:00:00 2001
From: Matthew Holt <mholt@users.noreply.github.com>
Date: Thu, 15 Feb 2018 00:04:31 -0700
Subject: [PATCH] httpserver: Raise error when adjusted site addresses clash at
 startup

See discussion on #2015 for how this situation was discovered. For a
Caddyfile like this:

	localhost {
		...
	}
	:2015 {
		...
	}

Running Caddy like this:

	caddy -host localhost

Produces two sites both defined as `localhost:2015` because the flag
changes the default host value to be `localhost`. This should be an
error since the sites are not distinct and it is confusing. It can also
cause issues with TLS handshakes loading the wrong cert, as the linked
discussion shows.
---
 caddy.go                            |  2 +-
 caddyhttp/httpserver/plugin.go      | 21 ++++++++++++++++++++-
 caddyhttp/httpserver/plugin_test.go | 17 +++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/caddy.go b/caddy.go
index 628673a73..917a5d069 100644
--- a/caddy.go
+++ b/caddy.go
@@ -612,7 +612,7 @@ func ValidateAndExecuteDirectives(cdyfile Input, inst *Instance, justValidate bo
 
 	sblocks, err = inst.context.InspectServerBlocks(cdyfile.Path(), sblocks)
 	if err != nil {
-		return err
+		return fmt.Errorf("error inspecting server blocks: %v", err)
 	}
 
 	return executeDirectives(inst, cdyfile.Path(), stype.Directives(), sblocks, justValidate)
diff --git a/caddyhttp/httpserver/plugin.go b/caddyhttp/httpserver/plugin.go
index ea31a58d8..93811abcb 100644
--- a/caddyhttp/httpserver/plugin.go
+++ b/caddyhttp/httpserver/plugin.go
@@ -117,12 +117,14 @@ func (h *httpContext) saveConfig(key string, cfg *SiteConfig) {
 // executing directives and otherwise prepares the directives to
 // be parsed and executed.
 func (h *httpContext) InspectServerBlocks(sourceFile string, serverBlocks []caddyfile.ServerBlock) ([]caddyfile.ServerBlock, error) {
+	siteAddrs := make(map[string]string)
+
 	// For each address in each server block, make a new config
 	for _, sb := range serverBlocks {
 		for _, key := range sb.Keys {
 			key = strings.ToLower(key)
 			if _, dup := h.keysToSiteConfigs[key]; dup {
-				return serverBlocks, fmt.Errorf("duplicate site address: %s", key)
+				return serverBlocks, fmt.Errorf("duplicate site key: %s", key)
 			}
 			addr, err := standardizeAddress(key)
 			if err != nil {
@@ -138,6 +140,23 @@ func (h *httpContext) InspectServerBlocks(sourceFile string, serverBlocks []cadd
 				addr.Port = Port
 			}
 
+			// Make sure the adjusted site address is distinct
+			addrCopy := addr // make copy so we don't disturb the original, carefully-parsed address struct
+			if addrCopy.Port == "" && Port == DefaultPort {
+				addrCopy.Port = Port
+			}
+			addrStr := strings.ToLower(addrCopy.String())
+			if otherSiteKey, dup := siteAddrs[addrStr]; dup {
+				err := fmt.Errorf("duplicate site address: %s", addrStr)
+				if (addrCopy.Host == Host && Host != DefaultHost) ||
+					(addrCopy.Port == Port && Port != DefaultPort) {
+					err = fmt.Errorf("site defined as %s is a duplicate of %s because of modified "+
+						"default host and/or port values (usually via -host or -port flags)", key, otherSiteKey)
+				}
+				return serverBlocks, err
+			}
+			siteAddrs[addrStr] = key
+
 			// If default HTTP or HTTPS ports have been customized,
 			// make sure the ACME challenge ports match
 			var altHTTPPort, altTLSSNIPort string
diff --git a/caddyhttp/httpserver/plugin_test.go b/caddyhttp/httpserver/plugin_test.go
index 5a60f2e83..f7b9cfc00 100644
--- a/caddyhttp/httpserver/plugin_test.go
+++ b/caddyhttp/httpserver/plugin_test.go
@@ -153,6 +153,23 @@ func TestInspectServerBlocksWithCustomDefaultPort(t *testing.T) {
 	}
 }
 
+// See discussion on PR #2015
+func TestInspectServerBlocksWithAdjustedAddress(t *testing.T) {
+	Port = DefaultPort
+	Host = "example.com"
+	filename := "Testfile"
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
+	input := strings.NewReader("example.com {\n}\n:2015 {\n}")
+	sblocks, err := caddyfile.Parse(filename, input, nil)
+	if err != nil {
+		t.Fatalf("Expected no error setting up test, got: %v", err)
+	}
+	_, err = ctx.InspectServerBlocks(filename, sblocks)
+	if err == nil {
+		t.Fatalf("Expected an error because site definitions should overlap, got: %v", err)
+	}
+}
+
 func TestInspectServerBlocksCaseInsensitiveKey(t *testing.T) {
 	filename := "Testfile"
 	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)