Merge branch 'master' into diagnostics

# Conflicts:
#	plugins.go
#	vendor/manifest

caddyhttp/httpserver/https.go

@@ -27,7 +27,7 @@ func activateHTTPS(cctx caddy.Context) error {
 	operatorPresent := !caddy.Started()
 
 	if !caddy.Quiet && operatorPresent {
-		fmt.Print("Activating privacy features...")
+		fmt.Print("Activating privacy features... ")
 	}
 
 	ctx := cctx.(*httpContext)
@@ -69,7 +69,7 @@ func activateHTTPS(cctx caddy.Context) error {
 	}
 
 	if !caddy.Quiet && operatorPresent {
-		fmt.Println(" done.")
+		fmt.Println("done.")
 	}
 
 	return nil
@@ -160,23 +160,37 @@ func hostHasOtherPort(allConfigs []*SiteConfig, thisConfigIdx int, otherPort str
 // to listen on HTTPPort. The TLS field of cfg must not be nil.
 func redirPlaintextHost(cfg *SiteConfig) *SiteConfig {
 	redirPort := cfg.Addr.Port
-	if redirPort == DefaultHTTPSPort {
-		redirPort = "" // default port is redundant
+	if redirPort == HTTPSPort {
+		// By default, HTTPSPort should be DefaultHTTPSPort,
+		// which of course doesn't need to be explicitly stated
+		// in the Location header. Even if HTTPSPort is changed
+		// so that it is no longer DefaultHTTPSPort, we shouldn't
+		// append it to the URL in the Location because changing
+		// the HTTPS port is assumed to be an internal-only change
+		// (in other words, we assume port forwarding is going on);
+		// but redirects go back to a presumably-external client.
+		// (If redirect clients are also internal, that is more
+		// advanced, and the user should configure HTTP->HTTPS
+		// redirects themselves.)
+		redirPort = ""
 	}
+
 	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.
+			// Construct the URL to which to redirect. Note that the Host in a
+			// request might contain a port, but we just need the hostname from
+			// it; and 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
+				requestHost = r.Host // Host did not contain a port, so use the whole value
 			}
 			if redirPort == "" {
 				toURL += requestHost
 			} else {
 				toURL += net.JoinHostPort(requestHost, redirPort)
 			}
+
 			toURL += r.URL.RequestURI()
 
 			w.Header().Set("Connection", "close")
@@ -184,9 +198,11 @@ func redirPlaintextHost(cfg *SiteConfig) *SiteConfig {
 			return 0, nil
 		})
 	}
+
 	host := cfg.Addr.Host
 	port := HTTPPort
 	addr := net.JoinHostPort(host, port)
+
 	return &SiteConfig{
 		Addr:       Address{Original: addr, Host: host, Port: port},
 		ListenHost: cfg.ListenHost,

caddyhttp/httpserver/https_test.go

@@ -53,7 +53,7 @@ func TestRedirPlaintextHost(t *testing.T) {
 		},
 		{
 			Host: "foohost",
-			Port: "443", // since this is the default HTTPS port, should not be included in Location value
+			Port: HTTPSPort, // since this is the 'default' HTTPS port, should not be included in Location value
 		},
 		{
 			Host:        "*.example.com",

caddyhttp/httpserver/plugin.go

@@ -91,11 +91,13 @@ func hideCaddyfile(cctx caddy.Context) error {
 	return nil
 }
 
-func newContext() caddy.Context {
-	return &httpContext{keysToSiteConfigs: make(map[string]*SiteConfig)}
+func newContext(inst *caddy.Instance) caddy.Context {
+	return &httpContext{instance: inst, keysToSiteConfigs: make(map[string]*SiteConfig)}
 }
 
 type httpContext struct {
+	instance *caddy.Instance
+
 	// keysToSiteConfigs maps an address at the top of a
 	// server block (a "key") to its SiteConfig. Not all
 	// SiteConfigs will be represented here, only ones
@@ -115,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 {
@@ -136,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
@@ -146,15 +167,19 @@ func (h *httpContext) InspectServerBlocks(sourceFile string, serverBlocks []cadd
 				altTLSSNIPort = HTTPSPort
 			}
 
+			// Make our caddytls.Config, which has a pointer to the
+			// instance's certificate cache and enough information
+			// to use automatic HTTPS when the time comes
+			caddytlsConfig := caddytls.NewConfig(h.instance)
+			caddytlsConfig.Hostname = addr.Host
+			caddytlsConfig.AltHTTPPort = altHTTPPort
+			caddytlsConfig.AltTLSSNIPort = altTLSSNIPort
+
 			// Save the config to our master list, and key it for lookups
 			cfg := &SiteConfig{
-				Addr: addr,
-				Root: Root,
-				TLS: &caddytls.Config{
-					Hostname:      addr.Host,
-					AltHTTPPort:   altHTTPPort,
-					AltTLSSNIPort: altTLSSNIPort,
-				},
+				Addr:            addr,
+				Root:            Root,
+				TLS:             caddytlsConfig,
 				originCaddyfile: sourceFile,
 				IndexPages:      staticfiles.DefaultIndexPages,
 			}

caddyhttp/httpserver/plugin_test.go

@@ -137,7 +137,7 @@ func TestAddressString(t *testing.T) {
 func TestInspectServerBlocksWithCustomDefaultPort(t *testing.T) {
 	Port = "9999"
 	filename := "Testfile"
-	ctx := newContext().(*httpContext)
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
 	input := strings.NewReader(`localhost`)
 	sblocks, err := caddyfile.Parse(filename, input, nil)
 	if err != nil {
@@ -153,9 +153,26 @@ 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().(*httpContext)
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
 	input := strings.NewReader("localhost {\n}\nLOCALHOST {\n}")
 	sblocks, err := caddyfile.Parse(filename, input, nil)
 	if err != nil {
@@ -207,7 +224,7 @@ func TestDirectivesList(t *testing.T) {
 }
 
 func TestContextSaveConfig(t *testing.T) {
-	ctx := newContext().(*httpContext)
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
 	ctx.saveConfig("foo", new(SiteConfig))
 	if _, ok := ctx.keysToSiteConfigs["foo"]; !ok {
 		t.Error("Expected config to be saved, but it wasn't")
@@ -226,7 +243,7 @@ func TestContextSaveConfig(t *testing.T) {
 
 // Test to make sure we are correctly hiding the Caddyfile
 func TestHideCaddyfile(t *testing.T) {
-	ctx := newContext().(*httpContext)
+	ctx := newContext(&caddy.Instance{Storage: make(map[interface{}]interface{})}).(*httpContext)
 	ctx.saveConfig("test", &SiteConfig{
 		Root:            Root,
 		originCaddyfile: "Testfile",

caddyhttp/httpserver/server.go

@@ -392,7 +392,7 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) (int, error)
 	if vhost == nil {
 		// check for ACME challenge even if vhost is nil;
 		// could be a new host coming online soon
-		if caddytls.HTTPChallengeHandler(w, r, "localhost", caddytls.DefaultHTTPAlternatePort) {
+		if caddytls.HTTPChallengeHandler(w, r, "localhost") {
 			return 0, nil
 		}
 		// otherwise, log the error and write a message to the client
@@ -408,7 +408,7 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) (int, error)
 
 	// we still check for ACME challenge if the vhost exists,
 	// because we must apply its HTTP challenge config settings
-	if s.proxyHTTPChallenge(vhost, w, r) {
+	if caddytls.HTTPChallengeHandler(w, r, vhost.ListenHost) {
 		return 0, nil
 	}
 
@@ -416,31 +416,25 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) (int, error)
 	// the URL path, so a request to example.com/foo/blog on the site
 	// defined as example.com/foo appears as /blog instead of /foo/blog.
 	if pathPrefix != "/" {
-		r.URL.Path = strings.TrimPrefix(r.URL.Path, pathPrefix)
-		if !strings.HasPrefix(r.URL.Path, "/") {
-			r.URL.Path = "/" + r.URL.Path
-		}
+		r.URL = trimPathPrefix(r.URL, pathPrefix)
 	}
 
 	return vhost.middlewareChain.ServeHTTP(w, r)
 }
 
-// proxyHTTPChallenge solves the ACME HTTP challenge if r is the HTTP
-// request for the challenge. If it is, and if the request has been
-// fulfilled (response written), true is returned; false otherwise.
-// If you don't have a vhost, just call the challenge handler directly.
-func (s *Server) proxyHTTPChallenge(vhost *SiteConfig, w http.ResponseWriter, r *http.Request) bool {
-	if vhost.Addr.Port != caddytls.HTTPChallengePort {
-		return false
+func trimPathPrefix(u *url.URL, prefix string) *url.URL {
+	// We need to use URL.EscapedPath() when trimming the pathPrefix as
+	// URL.Path is ambiguous about / or %2f - see docs. See #1927
+	trimmed := strings.TrimPrefix(u.EscapedPath(), prefix)
+	if !strings.HasPrefix(trimmed, "/") {
+		trimmed = "/" + trimmed
 	}
-	if vhost.TLS != nil && vhost.TLS.Manual {
-		return false
+	trimmedURL, err := url.Parse(trimmed)
+	if err != nil {
+		log.Printf("[ERROR] Unable to parse trimmed URL %s: %v", trimmed, err)
+		return u
 	}
-	altPort := caddytls.DefaultHTTPAlternatePort
-	if vhost.TLS != nil && vhost.TLS.AltHTTPPort != "" {
-		altPort = vhost.TLS.AltHTTPPort
-	}
-	return caddytls.HTTPChallengeHandler(w, r, vhost.ListenHost, altPort)
+	return trimmedURL
 }
 
 // Address returns the address s was assigned to listen on.

caddyhttp/httpserver/server_test.go

@@ -16,6 +16,7 @@ package httpserver
 
 import (
 	"net/http"
+	"net/url"
 	"testing"
 	"time"
 )
@@ -126,6 +127,94 @@ func TestMakeHTTPServerWithTimeouts(t *testing.T) {
 	}
 }
 
+func TestTrimPathPrefix(t *testing.T) {
+	for i, pt := range []struct {
+		path       string
+		prefix     string
+		expected   string
+		shouldFail bool
+	}{
+		{
+			path:       "/my/path",
+			prefix:     "/my",
+			expected:   "/path",
+			shouldFail: false,
+		},
+		{
+			path:       "/my/%2f/path",
+			prefix:     "/my",
+			expected:   "/%2f/path",
+			shouldFail: false,
+		},
+		{
+			path:       "/my/path",
+			prefix:     "/my/",
+			expected:   "/path",
+			shouldFail: false,
+		},
+		{
+			path:       "/my///path",
+			prefix:     "/my",
+			expected:   "/path",
+			shouldFail: true,
+		},
+		{
+			path:       "/my///path",
+			prefix:     "/my",
+			expected:   "///path",
+			shouldFail: false,
+		},
+		{
+			path:       "/my/path///slash",
+			prefix:     "/my",
+			expected:   "/path///slash",
+			shouldFail: false,
+		},
+		{
+			path:       "/my/%2f/path/%2f",
+			prefix:     "/my",
+			expected:   "/%2f/path/%2f",
+			shouldFail: false,
+		}, {
+			path:       "/my/%20/path",
+			prefix:     "/my",
+			expected:   "/%20/path",
+			shouldFail: false,
+		}, {
+			path:       "/path",
+			prefix:     "",
+			expected:   "/path",
+			shouldFail: false,
+		}, {
+			path:       "/path/my/",
+			prefix:     "/my",
+			expected:   "/path/my/",
+			shouldFail: false,
+		}, {
+			path:       "",
+			prefix:     "/my",
+			expected:   "/",
+			shouldFail: false,
+		}, {
+			path:       "/apath",
+			prefix:     "",
+			expected:   "/apath",
+			shouldFail: false,
+		},
+	} {
+
+		u, _ := url.Parse(pt.path)
+		if got, want := trimPathPrefix(u, pt.prefix), pt.expected; got.EscapedPath() != want {
+			if !pt.shouldFail {
+
+				t.Errorf("Test %d: Expected='%s', but was '%s' ", i, want, got.EscapedPath())
+			}
+		} else if pt.shouldFail {
+			t.Errorf("SHOULDFAIL Test %d: Expected='%s', and was '%s' but should fail", i, want, got.EscapedPath())
+		}
+	}
+}
+
 func TestMakeHTTPServerWithHeaderLimit(t *testing.T) {
 	for name, c := range map[string]struct {
 		group  []*SiteConfig