cmd/alertmanager: reject invalid external URLs (#1960)

* cmd/alertmanager: reject invalid external URLs

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* Address Brian's comments

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* Simplify the code according to Max's feedback

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

cmd/alertmanager/main.go

@@ -30,6 +30,7 @@ import (
 
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
+	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"github.com/prometheus/common/model"
@@ -323,15 +324,16 @@ func run() int {
 	})
 
 	if err != nil {
-		level.Error(logger).Log("err", fmt.Errorf("failed to create API: %v", err.Error()))
+		level.Error(logger).Log("err", errors.Wrap(err, "failed to create API"))
 		return 1
 	}
 
-	amURL, err := extURL(*listenAddress, *externalURL)
+	amURL, err := extURL(logger, os.Hostname, *listenAddress, *externalURL)
 	if err != nil {
-		level.Error(logger).Log("err", err)
+		level.Error(logger).Log("msg", "failed to determine external URL", "err", err)
 		return 1
 	}
+	level.Debug(logger).Log("externalURL", amURL.String())
 
 	waitFunc := func() time.Duration { return 0 }
 	if peer != nil {
@@ -357,7 +359,7 @@ func run() int {
 	configCoordinator.Subscribe(func(conf *config.Config) error {
 		tmpl, err = template.FromGlobs(conf.Templates...)
 		if err != nil {
-			return fmt.Errorf("failed to parse templates: %v", err.Error())
+			return errors.Wrap(err, "failed to parse templates")
 		}
 		tmpl.ExternalURL = amURL
 
@@ -404,14 +406,13 @@ func run() int {
 	}
 
 	// Make routePrefix default to externalURL path if empty string.
-	if routePrefix == nil || *routePrefix == "" {
+	if *routePrefix == "" {
 		*routePrefix = amURL.Path
 	}
-
 	*routePrefix = "/" + strings.Trim(*routePrefix, "/")
+	level.Debug(logger).Log("routePrefix", *routePrefix)
 
 	router := route.New().WithInstrumentation(instrumentHandler)
-
 	if *routePrefix != "/" {
 		router = router.WithPrefix(*routePrefix)
 	}
@@ -481,9 +482,9 @@ func clusterWait(p *cluster.Peer, timeout time.Duration) func() time.Duration {
 	}
 }
 
-func extURL(listen, external string) (*url.URL, error) {
+func extURL(logger log.Logger, hostnamef func() (string, error), listen, external string) (*url.URL, error) {
 	if external == "" {
-		hostname, err := os.Hostname()
+		hostname, err := hostnamef()
 		if err != nil {
 			return nil, err
 		}
@@ -491,6 +492,9 @@ func extURL(listen, external string) (*url.URL, error) {
 		if err != nil {
 			return nil, err
 		}
+		if port == "" {
+			level.Warn(logger).Log("msg", "no port found for listen address", "address", listen)
+		}
 
 		external = fmt.Sprintf("http://%s:%s/", hostname, port)
 	}
@@ -499,6 +503,9 @@ func extURL(listen, external string) (*url.URL, error) {
 	if err != nil {
 		return nil, err
 	}
+	if u.Scheme != "http" && u.Scheme != "https" {
+		return nil, errors.Errorf("%q: invalid %q scheme, only 'http' and 'https' are supported", u.String(), u.Scheme)
+	}
 
 	ppref := strings.TrimRight(u.Path, "/")
 	if ppref != "" && !strings.HasPrefix(ppref, "/") {

cmd/alertmanager/main_test.go

@@ -14,8 +14,10 @@
 package main
 
 import (
+	"fmt"
 	"testing"
 
+	"github.com/go-kit/kit/log"
 	commoncfg "github.com/prometheus/common/config"
 	"github.com/stretchr/testify/require"
 
@@ -86,3 +88,79 @@ func TestBuildReceiverIntegrations(t *testing.T) {
 		})
 	}
 }
+
+func TestExternalURL(t *testing.T) {
+	hostname := "foo"
+	for _, tc := range []struct {
+		hostnameResolver func() (string, error)
+		external         string
+		listen           string
+
+		expURL string
+		err    bool
+	}{
+		{
+			listen: ":9093",
+			expURL: "http://" + hostname + ":9093",
+		},
+		{
+			listen: "localhost:9093",
+			expURL: "http://" + hostname + ":9093",
+		},
+		{
+			listen: "localhost:",
+			expURL: "http://" + hostname + ":",
+		},
+		{
+			external: "https://host.example.com",
+			expURL:   "https://host.example.com",
+		},
+		{
+			external: "https://host.example.com/",
+			expURL:   "https://host.example.com",
+		},
+		{
+			external: "http://host.example.com/alertmanager",
+			expURL:   "http://host.example.com/alertmanager",
+		},
+		{
+			external: "http://host.example.com/alertmanager/",
+			expURL:   "http://host.example.com/alertmanager",
+		},
+		{
+			external: "http://host.example.com/////alertmanager//",
+			expURL:   "http://host.example.com/////alertmanager",
+		},
+		{
+			err: true,
+		},
+		{
+			hostnameResolver: func() (string, error) { return "", fmt.Errorf("some error") },
+			err:              true,
+		},
+		{
+			external: "://broken url string",
+			err:      true,
+		},
+		{
+			external: "host.example.com:8080",
+			err:      true,
+		},
+	} {
+		tc := tc
+		if tc.hostnameResolver == nil {
+			tc.hostnameResolver = func() (string, error) {
+				return hostname, nil
+			}
+		}
+		t.Run(fmt.Sprintf("external=%q,listen=%q", tc.external, tc.listen), func(t *testing.T) {
+			u, err := extURL(log.NewNopLogger(), tc.hostnameResolver, tc.listen, tc.external)
+			if tc.err {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, tc.expURL, u.String())
+		})
+	}
+}