Bound username length at creation (closes #184)

A login stores the username in the session cookie, which cannot carry
a value past about 4096 bytes, so a username of about 2 KB or more
could never log in and the login answered 500.

Usernames are now limited to 1024 bytes, about half of what the cookie
can carry. The User model rejects a longer one with
ErrUsernameTooLong, and a check constraint on the users table rejects
it for any path that bypasses the model. The comment on
MaxUsernameBytes gives the arithmetic.

Model: opus-5-5

README.md

@@ -1472,7 +1472,7 @@ A registered user of the webhooker service.
 | Field      | Type     | Description |
 | ---------- | -------- | ----------- |
 | `id`       | UUID     | Primary key |
-| `username` | string   | Unique login name |
+| `username` | string   | Unique login name, at most 1024 bytes so that it fits in the session cookie |
 | `password` | string   | Argon2id hash (never exposed via API) |
 
 **Relations:** Has many Webhooks. Has many APIKeys.
@@ -2412,14 +2412,14 @@ Removing either cap fails 14 subtests.
 
 `internal/middleware/logbound_test.go` and
 `internal/handlers/logbound_test.go` drive 8 KB of client-chosen text
-at each of these — 1 KB at `invalid password`, whose accounts are
-shared with the successful-login line, where a username past 4 KB
-overflows the session cookie and answers 500 before that line is
-written — through both handlers, and through seven fills: plain text
-as the baseline, and then the quotation mark, backslash, tab, newline,
-C0 control and astral non-printable, six characters the wider of the
-two handlers spends more on than the client spent sending them. Every
-case holds each line to the 2,560-byte ceiling. That per-line ceiling
+at each of these — just under 1 KB at `invalid password`, whose
+accounts are shared with the successful-login line and so must stay
+within the 1024-byte username limit — through both handlers, and
+through seven fills: plain text as the baseline, and then the
+quotation mark, backslash, tab, newline, C0 control and astral
+non-printable, six characters the wider of the two handlers spends
+more on than the client spent sending them. Every case holds each
+line to the 2,560-byte ceiling. That per-line ceiling
 is what the figure above states, and every row establishes it.
 
 Three of the sites go further and bound the whole flood's output — the
@@ -2721,7 +2721,7 @@ abuse limit later; they are tracked as future work.
 | ------ | --------------------------- | ----------- |
 | `GET`  | `/`                         | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
 | `GET`  | `/.well-known/healthcheck`  | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
-| `GET`, `HEAD` | `/s/*`              | Static file serving (embedded CSS, JS). `GET` and `HEAD` only — `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` and `CONNECT` are answered `405 Method Not Allowed` with `Allow: GET, HEAD`. Any other method (such as `PROPFIND`) is refused by chi before it reaches this route, and gets `405` without an `Allow` header. Pinned by `TestStaticServesOnlyGetAndHead` |
+| any    | `/s/*`                      | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` |
 | `POST` | `/webhook/{uuid}`           | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) |
 
 #### Authentication Endpoints

cmd/webhooker/main.go

@@ -16,7 +16,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/resetpw"
 	"sneak.berlin/go/webhooker/internal/server"
@@ -178,10 +177,6 @@ func newApp() *fx.App {
 			healthcheck.New,
 			session.New,
 			handlers.New,
-			// The registry /metrics serves, and the delivery
-			// collectors registered on it.
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			// The one SSRF guard both target-creation validation
 			// and the delivery dialer consult, so they cannot

internal/database/model_user.go

@@ -1,13 +1,57 @@
 package database
 
+import (
+	"errors"
+	"fmt"
+
+	"gorm.io/gorm"
+)
+
+// MaxUsernameBytes is the longest username, in bytes, that a user may
+// have. The same number appears in the check constraint on
+// User.Username, because a struct tag cannot reference a constant.
+//
+// A login stores the username in the session cookie, and both
+// securecookie and browsers refuse a cookie value past about 4096
+// bytes. That value is the session base64-encoded twice, so it holds
+// 4096 × 3/4 × 3/4 = 2304 bytes of session, and the signature,
+// timestamp and the session's other values take about 270 of those: a
+// username longer than about 2030 bytes can never log in. The limit is
+// about half that, so the session can carry more values later without
+// locking out an account whose username is already at the limit.
+const MaxUsernameBytes = 1024
+
+// ErrUsernameTooLong is returned when a user is saved with a username
+// longer than MaxUsernameBytes.
+var ErrUsernameTooLong = errors.New("username is too long")
+
 // User represents a user of the webhooker service
+//
+//nolint:lll // a struct tag cannot wrap
 type User struct {
 	BaseModel
 
-	Username string `gorm:"uniqueIndex;not null" json:"username"`
-	Password string `gorm:"not null"             json:"-"` // Argon2 hashed
+	Username string `gorm:"uniqueIndex;not null;check:length(CAST(username AS BLOB)) <= 1024" json:"username"`
+	Password string `gorm:"not null"                                                          json:"-"` // Argon2 hashed
 
 	// Relations
 	Webhooks []Webhook `json:"webhooks,omitempty"`
 	APIKeys  []APIKey  `json:"apiKeys,omitempty"`
 }
+
+// BeforeSave rejects a username longer than MaxUsernameBytes, so every
+// path that saves a user through GORM gets ErrUsernameTooLong rather
+// than the database's constraint error. The check constraint behind it
+// holds for any path that writes the table without this model.
+func (u *User) BeforeSave(_ *gorm.DB) error {
+	if len(u.Username) > MaxUsernameBytes {
+		return fmt.Errorf(
+			"%w: %d bytes, limit is %d",
+			ErrUsernameTooLong,
+			len(u.Username),
+			MaxUsernameBytes,
+		)
+	}
+
+	return nil
+}

internal/database/model_user_test.go

@@ -0,0 +1,65 @@
+package database_test
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// usernameAtLimit is exactly MaxUsernameBytes long, built from a
+// two-byte character. A check that counted characters rather than bytes
+// would see half the length and let the one-byte-longer name through.
+func usernameAtLimit() string {
+	return strings.Repeat("é", database.MaxUsernameBytes/2)
+}
+
+func TestUserCreate_RejectsOverlongUsername(t *testing.T) {
+	t.Parallel()
+
+	db := startedTestDB(t)
+
+	err := db.Create(&database.User{
+		Username: usernameAtLimit() + "x",
+		Password: "hash",
+	}).Error
+
+	require.ErrorIs(t, err, database.ErrUsernameTooLong)
+}
+
+func TestUserCreate_AcceptsUsernameAtLimit(t *testing.T) {
+	t.Parallel()
+
+	db := startedTestDB(t)
+
+	require.NoError(t, db.Create(&database.User{
+		Username: usernameAtLimit(),
+		Password: "hash",
+	}).Error)
+}
+
+// TestUsersTable_EnforcesUsernameLimitWithoutTheModel inserts with raw
+// SQL, as a path that bypassed User.BeforeSave would, so only the
+// table's check constraint stands between it and an over-long
+// username. Accepting the name at the limit and refusing the next byte
+// also pins the constraint's number to MaxUsernameBytes.
+func TestUsersTable_EnforcesUsernameLimitWithoutTheModel(t *testing.T) {
+	t.Parallel()
+
+	db := startedTestDB(t)
+
+	insert := "INSERT INTO users (id, username, password) VALUES (?, ?, ?)"
+
+	require.NoError(t, db.Exec(
+		insert, uuid.New().String(), usernameAtLimit(), "hash",
+	).Error)
+
+	err := db.Exec(
+		insert, uuid.New().String(), usernameAtLimit()+"x", "hash",
+	).Error
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "CHECK constraint failed")
+}

internal/delivery/engine.go

@@ -148,7 +148,6 @@ type EngineParams struct {
 	DBManager *database.WebhookDBManager
 	Logger    *logger.Logger
 	SSRFGuard *Guard
-	Metrics   *metrics.Set
 }
 
 // Engine processes queued deliveries in the background
@@ -168,10 +167,10 @@ type Engine struct {
 	retryCh    chan Task
 	workers    int
 
-	// mtr is the delivery metric set. Production wires the one
-	// registered on the registry /metrics serves; a test can
-	// substitute a set registered on a registry it holds, so it can
-	// gather what its own deliveries recorded.
+	// mtr is the delivery metric set. Production wires the
+	// process-wide one; a test can substitute a set registered on
+	// a private registry so its assertions are not disturbed by
+	// deliveries other tests are making at the same time.
 	mtr *metrics.Set
 
 	// targets maps each target type to its implementation.
@@ -205,7 +204,7 @@ func New(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    defaultWorkers,
-		mtr:        params.Metrics,
+		mtr:        metrics.Default(),
 	}
 
 	e.initTargets(&http.Client{

internal/delivery/export_test.go

@@ -9,7 +9,6 @@ import (
 	"net/url"
 	"time"
 
-	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
@@ -390,7 +389,7 @@ func NewTestEngine(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.New(prometheus.NewRegistry()),
+		mtr:        metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -405,7 +404,7 @@ func NewTestEngineSmallRetry(
 	e := &Engine{
 		log:     log,
 		retryCh: make(chan Task, 1),
-		mtr:     metrics.New(prometheus.NewRegistry()),
+		mtr:     metrics.Default(),
 	}
 	e.initTargets(nil)
 
@@ -428,7 +427,7 @@ func NewTestEngineWithDB(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.New(prometheus.NewRegistry()),
+		mtr:        metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -436,7 +435,8 @@ func NewTestEngineWithDB(
 }
 
 // ExportSetMetrics substitutes the engine's metric set, so a test can
-// assert on collectors registered on a registry it holds.
+// assert on collectors registered on a private registry instead of
+// the process-wide ones every other test is also moving.
 func (e *Engine) ExportSetMetrics(mtr *metrics.Set) {
 	e.mtr = mtr
 }

internal/delivery/metrics_test.go

@@ -35,8 +35,9 @@ const (
 )
 
 // mIsolate gives the setup's engine a metric set registered on a
-// registry this test holds, so its exact assertions can gather from
-// it.
+// private registry. The process-wide collectors are moved by every
+// other delivery test running in parallel, so exact assertions are
+// only possible against a registry this test owns.
 func mIsolate(
 	t *testing.T, s iSetup,
 ) *prometheus.Registry {

internal/handlers/auth_test.go

@@ -453,3 +453,33 @@ func TestLogin_SuccessCreatesSession(t *testing.T) {
 		"the issued cookie must carry an authenticated session",
 	)
 }
+
+// TestLogin_UsernameAtLimitCanLogIn shows that a username of exactly
+// database.MaxUsernameBytes still fits in the session cookie. Past
+// what the cookie can carry, a correct login answers 500.
+func TestLogin_UsernameAtLimitCanLogIn(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h  *handlers.Handlers
+		db *database.Database
+	)
+
+	app := newTestApp(t, &h, &db)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	username := strings.Repeat("a", database.MaxUsernameBytes)
+
+	hash, err := database.HashPassword(operatorPassword)
+	require.NoError(t, err)
+	require.NoError(t, db.DB().Create(&database.User{
+		Username: username,
+		Password: hash,
+	}).Error)
+
+	w := submitLogin(h, sharedProxyPeer, username, operatorPassword)
+
+	assert.Equal(t, http.StatusSeeOther, w.Code)
+}

internal/handlers/handlers.go

@@ -12,7 +12,6 @@ import (
 	"net/http"
 	"sync/atomic"
 
-	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
@@ -62,8 +61,6 @@ type HandlersParams struct {
 	Notifier     delivery.Notifier
 	Evictor      delivery.WebhookEvictor
 	SSRFGuard    *delivery.Guard
-	Metrics      *metrics.Set
-	Registry     *prometheus.Registry
 }
 
 // Handlers provides HTTP handler methods for all application
@@ -125,7 +122,7 @@ func New(
 	s.mw = params.Middleware
 	s.notifier = params.Notifier
 	s.evictor = params.Evictor
-	s.mtr = params.Metrics
+	s.mtr = metrics.Default()
 	s.ssrf = params.SSRFGuard
 
 	// Parse all page templates once at startup

internal/handlers/handlers_test.go

@@ -20,7 +20,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/session"
 )
@@ -110,8 +109,6 @@ func newTestApp(
 			func(r *recordingEvictor) delivery.WebhookEvictor {
 				return r
 			},
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/handlers/logbound_test.go

@@ -339,11 +339,9 @@ const storedUserPassword = "correct-horse-battery-staple"
 // storedFillBytes is the raw length of the client-chosen value in
 // those accounts' usernames. It is well past the 512-byte field
 // budget, so the line is still truncated, but short enough that the
-// session cookie a successful login writes stays inside
-// securecookie's 4 KB limit: the cookie is written BEFORE the
-// "user logged in" line, so an 8 KB username answers 500 and never
-// reaches it.
-const storedFillBytes = 1024
+// whole username, markers and fill name included, stays within
+// database.MaxUsernameBytes.
+const storedFillBytes = 960
 
 // storedFill builds a username fill of storedFillBytes raw bytes out
 // of repetitions of ch, with both markers at its far end.

internal/handlers/metrics.go

@@ -1,20 +0,0 @@
-package handlers
-
-import (
-	"net/http"
-
-	"github.com/prometheus/client_golang/prometheus/promhttp"
-)
-
-// HandleMetrics returns the Prometheus scrape handler for the
-// registry every collector in this process registers on. It is what
-// promhttp.Handler builds for the global default registry, including
-// the promhttp_metric_handler_* series that count scrapes, pointed at
-// that registry instead.
-func (s *Handlers) HandleMetrics() http.HandlerFunc {
-	reg := s.params.Registry
-
-	return promhttp.InstrumentMetricHandler(
-		reg, promhttp.HandlerFor(reg, promhttp.HandlerOpts{}),
-	).ServeHTTP
-}

internal/metrics/metrics.go

@@ -3,17 +3,17 @@
 // deliveries are attempted, how they end, how long they take, how
 // deep the queues are, and how many circuit breakers are open.
 //
-// It also builds the registry the authenticated /metrics route
-// serves. These collectors, the inbound HTTP metrics recorded in
-// internal/middleware, and the Go runtime and process collectors all
-// register on that one registry, never on Prometheus's global default.
+// The inbound HTTP metrics come from the go-http-metrics recorder in
+// internal/middleware and land on prometheus.DefaultRegisterer. These
+// collectors register there too, so both surfaces are gathered by the
+// one promhttp handler mounted on the authenticated /metrics route.
 package metrics
 
 import (
+	"sync"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"github.com/prometheus/client_golang/prometheus/collectors"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"sneak.berlin/go/webhooker/internal/database"
 )
@@ -57,31 +57,25 @@ var knownTargetTypes = []database.TargetType{
 	database.TargetTypeSlack,
 }
 
-// NewRegistry returns the registry /metrics serves, carrying the Go
-// runtime and process collectors that Prometheus's global default
-// registry carries, so the go_* and process_* series stay in the
-// scrape.
+// defaultSet is the process-wide metric set, registered on the same
+// registry the HTTP middleware and the /metrics handler already use.
+// It is built on first use rather than in an init so that a test
+// binary that never touches metrics never registers them.
 //
-// A registry of its own, rather than the global default, is what lets
-// two dependency graphs in one process — two tests, say — each
-// register their collectors without the second registration
-// panicking.
-func NewRegistry() *prometheus.Registry {
-	reg := prometheus.NewRegistry()
-	reg.MustRegister(
-		collectors.NewGoCollector(),
-		collectors.NewProcessCollector(
-			collectors.ProcessCollectorOpts{},
-		),
-	)
+//nolint:gochecknoglobals // one process-wide registration, by design
+var defaultSet = sync.OnceValue(func() *Set {
+	return New(prometheus.DefaultRegisterer)
+})
 
-	return reg
+// Default returns the process-wide metric set.
+func Default() *Set {
+	return defaultSet()
 }
 
 // Set is one registered group of webhooker's delivery collectors.
-// Production builds one on the registry /metrics serves; tests build
-// their own against a private registry so assertions are not
-// disturbed by deliveries other tests are making concurrently.
+// Production uses the single Default set; tests build their own
+// against a private registry so assertions are not disturbed by
+// deliveries other tests are making concurrently.
 type Set struct {
 	eventsReceived      prometheus.Counter
 	deliveryAttempts    *prometheus.CounterVec
@@ -99,7 +93,7 @@ type Set struct {
 // New registers a full set of delivery collectors on reg and returns
 // it. It panics if reg already holds them, which is the intended
 // behaviour for a duplicate registration.
-func New(reg *prometheus.Registry) *Set {
+func New(reg prometheus.Registerer) *Set {
 	factory := promauto.With(reg)
 
 	s := &Set{

internal/middleware/export_test.go

@@ -10,7 +10,8 @@ import (
 
 // MetricsMiddlewareForTest builds the metrics recording middleware
 // against a caller-supplied recorder, so a test can gather from its
-// own Prometheus registry without building a whole Middleware.
+// own Prometheus registry rather than the process-wide default one
+// that Middleware.Metrics uses.
 func MetricsMiddlewareForTest(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {

internal/middleware/metrics.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/go-chi/chi"
 	httpmetrics "github.com/slok/go-http-metrics/metrics"
+	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	ghmm "github.com/slok/go-http-metrics/middleware"
 	"github.com/slok/go-http-metrics/middleware/std"
 )
@@ -151,14 +152,16 @@ func (r boundedLabelRecorder) AddInflightRequests(
 var _ httpmetrics.Recorder = boundedLabelRecorder{}
 
 // Metrics returns middleware that records Prometheus HTTP metrics on
-// the registry the /metrics route serves. Every call shares the one
-// recorder New built, so any number of routers can install it.
+// the default registry, which is the one the /metrics route gathers.
 func (s *Middleware) Metrics() func(http.Handler) http.Handler {
-	return metricsMiddleware(s.metricsRecorder)
+	return metricsMiddleware(
+		prommetrics.NewRecorder(prommetrics.Config{}),
+	)
 }
 
 // metricsMiddleware builds the recording middleware against a given
-// recorder, so tests can gather from a registry of their own.
+// recorder, so tests can gather from a registry of their own instead
+// of the process-wide default.
 func metricsMiddleware(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {

internal/middleware/metrics_test.go

@@ -57,8 +57,9 @@ const (
 // Server.setupWebhookRoutes inside it. That ordering is the whole
 // defect, so a test that flattens it would prove nothing.
 //
-// The recorder writes to a registry of the test's own, so each test
-// observes only its own traffic.
+// The recorder writes to a registry of the test's own rather than the
+// process-wide default one, so each test observes only its own
+// traffic.
 func metricsTestRouter(
 	t *testing.T,
 	receiverLimit int,

internal/middleware/middleware.go

@@ -13,9 +13,6 @@ import (
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
 	"github.com/go-chi/cors"
-	"github.com/prometheus/client_golang/prometheus"
-	httpmetrics "github.com/slok/go-http-metrics/metrics"
-	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/globals"
@@ -151,11 +148,10 @@ const (
 type MiddlewareParams struct {
 	fx.In
 
-	Logger   *logger.Logger
-	Globals  *globals.Globals
-	Config   *config.Config
-	Session  *session.Session
-	Registry *prometheus.Registry
+	Logger  *logger.Logger
+	Globals *globals.Globals
+	Config  *config.Config
+	Session *session.Session
 }
 
 // Middleware provides HTTP middleware for logging, CORS, auth, and
@@ -165,12 +161,6 @@ type Middleware struct {
 	params  *MiddlewareParams
 	session *session.Session
 
-	// metricsRecorder records the inbound HTTP metrics on the
-	// registry /metrics serves. It is built once, in New, because
-	// building it registers its collectors, and a second
-	// registration on the same registry panics; see Metrics.
-	metricsRecorder httpmetrics.Recorder
-
 	// loginGuard counts failed credential verifications and bounds
 	// concurrent password hashing. It is built on first use so that
 	// every construction path gets one; see guard().
@@ -189,9 +179,6 @@ func New(
 	s.params = &params
 	s.log = params.Logger.Get()
 	s.session = params.Session
-	s.metricsRecorder = prommetrics.NewRecorder(
-		prommetrics.Config{Registry: params.Registry},
-	)
 
 	return s, nil
 }

internal/resetpw/resetpw_test.go

@@ -24,7 +24,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/resetpw"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -164,8 +163,6 @@ func newServerApp(
 			session.New,
 			func() delivery.Notifier { return &noopNotifier{} },
 			func() delivery.WebhookEvictor { return &noopEvictor{} },
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/server/routes.go

@@ -7,6 +7,7 @@ import (
 	sentryhttp "github.com/getsentry/sentry-go/http"
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
+	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"sneak.berlin/go/webhooker/static"
 )
 
@@ -91,25 +92,11 @@ func (s *Server) setupGlobalMiddleware() {
 func (s *Server) setupRoutes() {
 	s.router.Get("/", s.h.HandleIndex())
 
-	// Static assets answer GET and HEAD only. chi's default 405
-	// carries no Allow header, so this group supplies its own.
-	staticFiles := http.StripPrefix(
-		"/s", http.FileServer(http.FS(static.Static)),
+	s.router.Mount(
+		"/s",
+		http.StripPrefix("/s", http.FileServer(http.FS(static.Static))),
 	)
 
-	s.router.Route("/s", func(r chi.Router) {
-		r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Allow", "GET, HEAD")
-			http.Error(
-				w,
-				"Method Not Allowed",
-				http.StatusMethodNotAllowed,
-			)
-		})
-		r.Method(http.MethodGet, "/*", staticFiles)
-		r.Method(http.MethodHead, "/*", staticFiles)
-	})
-
 	s.router.Route("/api/v1", func(_ chi.Router) {
 		// API routes will be added here.
 	})
@@ -129,7 +116,12 @@ func (s *Server) setupRoutes() {
 	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Group(func(r chi.Router) {
 			r.Use(s.mw.MetricsAuth())
-			r.Get("/metrics", s.h.HandleMetrics())
+			r.Get(
+				"/metrics",
+				http.HandlerFunc(
+					promhttp.Handler().ServeHTTP,
+				),
+			)
 		})
 	}
 

internal/server/routes_test.go

@@ -23,7 +23,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/server"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -113,8 +112,6 @@ func newTestEnvWithConfig(
 			session.New,
 			func() delivery.Notifier { return &noopNotifier{} },
 			func() delivery.WebhookEvictor { return &noopEvictor{} },
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,
@@ -399,15 +396,13 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
 
 // --- /s static group ---
 
-// TestStaticServesOnlyGetAndHead pins the methods the static group
-// answers: GET and HEAD are served the asset, and the other methods
-// chi routes (POST, PUT, DELETE and the rest) are refused with 405
-// and an Allow header naming those two. A method chi does not route,
-// such as PROPFIND, is refused with 405 by the top-level router
-// before it reaches the static group, so it gets no Allow header.
-// The README documents this; the test is what keeps the two from
-// drifting.
-func TestStaticServesOnlyGetAndHead(t *testing.T) {
+// TestStaticServesEveryMethod pins what the static mount actually
+// answers. chi's Mount registers the handler for all methods and
+// http.FileServer only special-cases HEAD (by suppressing the body),
+// so a POST or a DELETE to an asset is served the file rather than
+// refused. The README documents this; the test is what keeps the two
+// from drifting.
+func TestStaticServesEveryMethod(t *testing.T) {
 	t.Parallel()
 
 	env := newTestEnv(t)
@@ -422,7 +417,6 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 		http.MethodPost,
 		http.MethodPut,
 		http.MethodDelete,
-		"PROPFIND",
 	} {
 		t.Run(method, func(t *testing.T) {
 			t.Parallel()
@@ -434,38 +428,18 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 			w := httptest.NewRecorder()
 			env.router.ServeHTTP(w, req)
 
-			switch method {
-			case http.MethodGet:
-				assert.Equal(t, http.StatusOK, w.Code)
-				assert.Equal(t, body, w.Body.Bytes(),
-					"the asset itself is returned")
-			case http.MethodHead:
-				assert.Equal(t, http.StatusOK, w.Code)
+			assert.Equal(t, http.StatusOK, w.Code,
+				"static mount answers every method")
+
+			if method == http.MethodHead {
 				assert.Empty(t, w.Body.Bytes(),
 					"HEAD must not carry a body")
-			case "PROPFIND":
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Empty(t, w.Header().Get("Allow"),
-					"chi refuses a method it does not route "+
-						"before the static group runs")
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
-			default:
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Equal(
-					t, "GET, HEAD", w.Header().Get("Allow"),
-				)
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
+
+				return
 			}
+
+			assert.Equal(t, body, w.Body.Bytes(),
+				"the asset itself is returned")
 		})
 	}
 }
@@ -964,43 +938,3 @@ func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
 		})
 	}
 }
-
-// TestTwoMetricsRoutersInOneProcess pins
-// https://git.eeqj.de/sneak/webhooker/issues/227: a second
-// metrics-enabled router in one process used to panic, because the
-// HTTP metrics registered on Prometheus's global default registry.
-// Two routers are built over separate dependency graphs and a third
-// over the first graph again, and each must still serve the HTTP,
-// delivery and Go runtime series.
-func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
-	t.Parallel()
-
-	first := newTestEnvWithConfig(
-		t, metricsConfig(t, metricsUser, metricsAuthValue),
-	)
-	second := newTestEnvWithConfig(
-		t, metricsConfig(t, metricsUser, metricsAuthValue),
-	)
-	third := &testEnv{
-		router: server.NewRouterForTest(
-			first.log.Get(), first.cfg, first.mw, first.hnd,
-		),
-	}
-
-	for _, env := range []*testEnv{first, second, third} {
-		env.get("/", nil)
-
-		scrape := env.metricsRequest(metricsUser, metricsAuthValue)
-		require.Equal(t, http.StatusOK, scrape.Code)
-
-		for _, series := range []string{
-			"http_request_duration_seconds",
-			"http_response_size_bytes",
-			"http_requests_inflight",
-			"webhooker_events_received_total",
-			"go_goroutines",
-		} {
-			assert.Contains(t, scrape.Body.String(), series)
-		}
-	}
-}