Fail startup on half-set metrics credentials (closes #205)

METRICS_USERNAME alone mounted /metrics behind a credential map
whose only password was the empty string, so `curl -u 'metrics:'`
returned 200 while the startup log reported hasMetricsAuth:false.
The route mount tested the username and the log tested both, so the
two could disagree about whether the endpoint existed.

Config.MetricsAuthEnabled is now the single value behind both: the
/metrics mount, the Prometheus recording middleware and the startup
log's hasMetricsAuth field all read it, and it requires both
credentials. loadFromEnv rejects a half-set pair outright with an
error naming both variables in either direction, so a set-but-invalid
configuration aborts startup rather than degrading into an endpoint
the operator did not ask for. Both unset stays valid and leaves
/metrics unmounted.

Tests cover all four combinations at the config layer, counting
"set to the empty string" and "not set at all" as separate inputs,
plus the route tree's behaviour in each state.

internal/server/routes.go

@@ -55,8 +55,11 @@ func (s *Server) setupGlobalMiddleware() {
 	s.router.Use(s.mw.SecurityHeaders())
 	s.router.Use(s.mw.Logging())
 
-	// Metrics middleware (only if credentials are configured)
-	if s.params.Config.MetricsUsername != "" {
+	// Metrics recording middleware, registered only when the
+	// endpoint that exposes what it records is served. The
+	// condition is the same MetricsAuthEnabled the /metrics mount
+	// in setupRoutes reads.
+	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Use(s.mw.Metrics())
 	}
 
@@ -103,8 +106,14 @@ func (s *Server) setupRoutes() {
 		s.h.HandleHealthCheck(),
 	)
 
-	// set up authenticated /metrics route:
-	if s.params.Config.MetricsUsername != "" {
+	// Authenticated /metrics route. The condition is
+	// Config.MetricsAuthEnabled and never the username alone: a
+	// username with an empty password would otherwise mount the
+	// endpoint behind a credential map that accepts an empty
+	// password. Config rejects that combination at startup, and
+	// this reads the same value the startup log reports, so the
+	// two cannot disagree about whether the route exists.
+	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Group(func(r chi.Router) {
 			r.Use(s.mw.MetricsAuth())
 			r.Get(

internal/server/routes_test.go

@@ -34,6 +34,13 @@ import (
 // the CSRF middleware executed.
 const csrfCookieName = "_gorilla_csrf"
 
+const (
+	// metricsUser and metricsAuthValue are the /metrics basic-auth
+	// credentials the metrics routing tests below configure.
+	metricsUser      = "metrics"
+	metricsAuthValue = "s3cret"
+)
+
 type noopNotifier struct{}
 
 func (n *noopNotifier) Notify([]delivery.Task) {}
@@ -69,9 +76,23 @@ type testEnv struct {
 func newTestEnv(t *testing.T) *testEnv {
 	t.Helper()
 
+	return newTestEnvWithConfig(t, &config.Config{
+		DataDir:     t.TempDir(),
+		Environment: config.EnvironmentDev,
+	})
+}
+
+// newTestEnvWithConfig is newTestEnv over a caller-supplied Config,
+// for the routes whose existence the configuration decides. The same
+// pointer reaches the router and every middleware, so a test cannot
+// accidentally configure one and not the other.
+func newTestEnvWithConfig(
+	t *testing.T, cfg *config.Config,
+) *testEnv {
+	t.Helper()
+
 	var (
 		log   *logger.Logger
-		cfg   *config.Config
 		mw    *middleware.Middleware
 		hnd   *handlers.Handlers
 		sess  *session.Session
@@ -84,12 +105,7 @@ func newTestEnv(t *testing.T) *testEnv {
 		fx.Provide(
 			globals.New,
 			logger.New,
-			func() *config.Config {
-				return &config.Config{
-					DataDir:     t.TempDir(),
-					Environment: config.EnvironmentDev,
-				}
-			},
+			func() *config.Config { return cfg },
 			database.New,
 			database.NewWebhookDBManager,
 			healthcheck.New,
@@ -99,7 +115,7 @@ func newTestEnv(t *testing.T) *testEnv {
 			middleware.New,
 			handlers.New,
 		),
-		fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db, &dbMgr),
+		fx.Populate(&log, &mw, &hnd, &sess, &db, &dbMgr),
 	)
 	app.RequireStart()
 	t.Cleanup(app.RequireStop)
@@ -657,3 +673,119 @@ func TestSourceLogsBody_OtherUser404s(t *testing.T) {
 	assert.Equal(t, http.StatusSeeOther, anon.Code)
 	assert.Equal(t, "/pages/login", anon.Header().Get("Location"))
 }
+
+// metricsConfig is a Config differing from the routing default only
+// in the two /metrics credentials.
+func metricsConfig(
+	t *testing.T, username, password string,
+) *config.Config {
+	t.Helper()
+
+	return &config.Config{
+		DataDir:         t.TempDir(),
+		Environment:     config.EnvironmentDev,
+		MetricsUsername: username,
+		MetricsPassword: password,
+	}
+}
+
+// metricsRequest asks the real router for /metrics with the given
+// basic-auth credentials, or with no Authorization header when
+// username is empty.
+func (e *testEnv) metricsRequest(
+	username, password string,
+) *httptest.ResponseRecorder {
+	req := httptest.NewRequestWithContext(
+		context.Background(), http.MethodGet, "/metrics", nil,
+	)
+
+	if username != "" {
+		req.SetBasicAuth(username, password)
+	}
+
+	w := httptest.NewRecorder()
+	e.router.ServeHTTP(w, req)
+
+	return w
+}
+
+// TestMetricsRouteUnmountedWithoutCredentials pins that with neither
+// credential configured the route does not exist, which is the
+// documented behaviour and the only valid way for /metrics to be
+// absent.
+func TestMetricsRouteUnmountedWithoutCredentials(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnvWithConfig(t, metricsConfig(t, "", ""))
+
+	assert.Equal(
+		t, http.StatusNotFound,
+		env.metricsRequest("", "").Code,
+	)
+}
+
+// TestMetricsRouteRequiresCredentials pins that with both credentials
+// configured the route exists and every request that does not carry
+// the configured pair is refused — including the empty password that
+// a half-set configuration used to make sufficient.
+func TestMetricsRouteRequiresCredentials(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnvWithConfig(
+		t, metricsConfig(t, metricsUser, metricsAuthValue),
+	)
+
+	assert.Equal(
+		t, http.StatusUnauthorized,
+		env.metricsRequest("", "").Code,
+		"no credentials must not reach the metrics handler",
+	)
+	assert.Equal(
+		t, http.StatusUnauthorized,
+		env.metricsRequest(metricsUser, "").Code,
+		"an empty password must not reach the metrics handler",
+	)
+	assert.Equal(
+		t, http.StatusUnauthorized,
+		env.metricsRequest(metricsUser, "wrong").Code,
+	)
+
+	ok := env.metricsRequest(metricsUser, metricsAuthValue)
+	assert.Equal(t, http.StatusOK, ok.Code)
+	assert.Contains(t, ok.Body.String(), "go_goroutines")
+}
+
+// TestMetricsRouteUnmountedOnHalfSetConfig pins the defect from
+// https://git.eeqj.de/sneak/webhooker/issues/205 at the routing
+// layer. Config rejects a half-set pair at startup, so this Config
+// cannot be reached from the environment; the assertion is that the
+// route tree does not publish an endpoint accepting an empty
+// password even when handed one anyway, because the mount and the
+// startup log's hasMetricsAuth read the same value.
+func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name     string
+		username string
+		password string
+	}{
+		{name: "username only", username: metricsUser},
+		{name: "password only", password: metricsAuthValue},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			cfg := metricsConfig(t, tc.username, tc.password)
+			env := newTestEnvWithConfig(t, cfg)
+
+			assert.False(t, cfg.MetricsAuthEnabled())
+			assert.Equal(
+				t, http.StatusNotFound,
+				env.metricsRequest(
+					tc.username, tc.password,
+				).Code,
+			)
+		})
+	}
+}