Render admin page errors in the normal layout (closes #382)

Every 400, 403, 404 and 500 on an admin page now answers with an error page in the normal layout: the status, one fixed line explaining it, and a link back to the webhook list, or to sign-in when nobody is signed in. Unknown paths reach it through the router's not-found handler, a refused form token through the CSRF middleware, and a panic through a recoverer each admin page route group installs first. Status codes are unchanged, and the page always sends Cache-Control: no-store.

If the error page fails to render, the answer is the same status in plain text; if it panics, the answer is a 500. The receiver, the healthcheck and /metrics keep their plain answers.

Model: opus-5-5

internal/server/error_page_test.go

@@ -0,0 +1,222 @@
+package server_test
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strconv"
+	"testing"
+
+	"github.com/getsentry/sentry-go"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/server"
+)
+
+// The link back the error page offers: to the webhook list for a
+// signed-in user, to sign-in for anyone else.
+const (
+	backToWebhooks = `<a href="/hooks" class="btn-secondary">` +
+		`Back to webhooks</a>`
+	backToSignIn = `<a href="/pages/login" class="btn-primary">` +
+		`Sign in</a>`
+)
+
+// assertErrorPage checks that w is the error page for status, in the
+// normal layout, offering link.
+func assertErrorPage(
+	t *testing.T,
+	w *httptest.ResponseRecorder,
+	status int,
+	link string,
+) {
+	t.Helper()
+
+	body := w.Body.String()
+
+	assert.Equal(t, status, w.Code)
+	assert.Equal(
+		t, "text/html; charset=utf-8", w.Header().Get("Content-Type"),
+	)
+	assert.Equal(t, "no-store", w.Header().Get("Cache-Control"))
+	assert.Contains(t, body, `<nav class="app-bar"`)
+	assert.Contains(
+		t, body, strconv.Itoa(status)+" "+http.StatusText(status),
+	)
+	assert.Contains(t, body, link)
+}
+
+func TestErrorPage_DeletedWebhook(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	userID, _ := env.seedUser(t, "owner", "somepassword")
+	cookies := env.authCookies(t, userID, "owner")
+
+	wh := env.seedWebhook(t, userID)
+	require.NoError(t, env.db.DB().Delete(wh).Error)
+
+	w := env.get("/hook/"+wh.ID, cookies)
+
+	assertErrorPage(t, w, http.StatusNotFound, backToWebhooks)
+}
+
+func TestErrorPage_DeletedTarget(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	userID, _ := env.seedUser(t, "owner", "somepassword")
+	cookies := env.authCookies(t, userID, "owner")
+
+	wh := env.seedWebhook(t, userID)
+	tgt := env.seedTarget(t, wh.ID)
+	require.NoError(t, env.db.DB().Delete(tgt).Error)
+
+	w := env.get(
+		"/hook/"+wh.ID+"/targets/"+tgt.ID+"/edit", cookies,
+	)
+
+	assertErrorPage(t, w, http.StatusNotFound, backToWebhooks)
+}
+
+func TestErrorPage_UnknownPath(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	userID, _ := env.seedUser(t, "owner", "somepassword")
+	cookies := env.authCookies(t, userID, "owner")
+
+	assertErrorPage(
+		t, env.get("/no-such-page", nil),
+		http.StatusNotFound, backToSignIn,
+	)
+
+	// Outside every route group there is no form token, so the
+	// page leaves out the logout form rather than offer one that
+	// would be refused.
+	w := env.get("/no-such-page", cookies)
+	assertErrorPage(t, w, http.StatusNotFound, backToWebhooks)
+	assert.NotContains(t, w.Body.String(), `action="/pages/logout"`)
+
+	// Inside a route group the page has a token, and logout works.
+	wh := env.seedWebhook(t, userID)
+	w = env.get("/hook/"+wh.ID+"/no-such-page", cookies)
+	assertErrorPage(t, w, http.StatusNotFound, backToWebhooks)
+	assert.Contains(t, w.Body.String(), `action="/pages/logout"`)
+}
+
+func TestErrorPage_BadCSRFToken(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	form := url.Values{}
+	form.Set("username", "someone")
+	form.Set("password", "irrelevant")
+	form.Set("csrf_token", "not-a-token")
+
+	assertErrorPage(
+		t, env.post("/pages/login", form, nil),
+		http.StatusForbidden, backToSignIn,
+	)
+
+	userID, _ := env.seedUser(t, "owner", "somepassword")
+	cookies := env.authCookies(t, userID, "owner")
+	wh := env.seedWebhook(t, userID)
+
+	edit := url.Values{}
+	edit.Set("name", "renamed")
+
+	assertErrorPage(
+		t, env.post("/hook/"+wh.ID+"/edit", edit, cookies),
+		http.StatusForbidden, backToWebhooks,
+	)
+}
+
+// TestErrorPage_PanicOnAdminPage sends a panicking handler in an
+// admin page route group through the real router, with error
+// tracking on: the client gets the 500 error page, and the tracker
+// still gets the panic, once. The same panic outside the admin page
+// route groups keeps the plain 500.
+func TestErrorPage_PanicOnAdminPage(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	transport := &captureTransport{}
+
+	opts := server.SentryClientOptionsForTest(
+		"https://public@sentry.invalid/1", "webhooker-test",
+	)
+	opts.Transport = transport
+
+	client, err := sentry.NewClient(opts)
+	require.NoError(t, err)
+
+	serve := func(router http.Handler, path string) *httptest.ResponseRecorder {
+		req := httptest.NewRequestWithContext(
+			sentry.SetHubOnContext(
+				context.Background(),
+				sentry.NewHub(client, sentry.NewScope()),
+			),
+			http.MethodGet, path, nil,
+		)
+
+		w := httptest.NewRecorder()
+		router.ServeHTTP(w, req)
+
+		return w
+	}
+
+	w := serve(
+		server.NewRouterWithPageProbeForTest(
+			env.log.Get(), env.cfg, env.mw, env.hnd,
+			true, panicProbeHandler,
+		),
+		server.PageProbePattern,
+	)
+	assertErrorPage(t, w, http.StatusInternalServerError, backToSignIn)
+
+	w = serve(
+		server.NewRouterWithProbeForTest(
+			env.log.Get(), env.cfg, env.mw, env.hnd,
+			true, panicProbeHandler,
+		),
+		server.ProbePattern,
+	)
+	assert.Equal(t, http.StatusInternalServerError, w.Code)
+	assert.Equal(t, "Internal Server Error\n", w.Body.String())
+
+	require.Len(t, transport.events, 2)
+
+	for _, event := range transport.events {
+		assert.Contains(t, marshalEvent(t, event), panicProbeMarker)
+	}
+}
+
+// TestErrorPage_ReceiverStaysPlain pins that the error page is for
+// the web UI only: a sender posting to an entrypoint that does not
+// exist still gets the plain-text answer.
+func TestErrorPage_ReceiverStaysPlain(t *testing.T) {
+	t.Parallel()
+
+	// newTestEnv leaves the receiver rate limit at zero, which
+	// refuses every request before it reaches the receiver.
+	env := newTestEnvWithConfig(t, &config.Config{
+		DataDir:           t.TempDir(),
+		Environment:       config.EnvironmentDev,
+		ReceiverRateLimit: 10,
+	})
+
+	w := env.post(
+		"/h/0b8f3c1e-7d2a-4e6b-9f15-3a9c2d4e6f70", url.Values{}, nil,
+	)
+
+	assert.Equal(t, http.StatusNotFound, w.Code)
+	assert.Equal(t, "404 page not found\n", w.Body.String())
+}

internal/server/export_test.go

@@ -5,6 +5,7 @@ import (
 	"net/http"
 
 	"github.com/getsentry/sentry-go"
+	"github.com/go-chi/chi"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/middleware"
@@ -101,3 +102,39 @@ func NewRouterWithProbeForTest(
 
 	return s.router
 }
+
+// PageProbePattern is where NewRouterWithPageProbeForTest serves its
+// probe: inside the /pages route group, the admin page group a
+// request reaches without signing in.
+const PageProbePattern = "/pages/probe"
+
+// NewRouterWithPageProbeForTest is NewRouterWithProbeForTest with the
+// probe added to the /pages route group once SetupRoutes has built
+// it, so the probe runs behind that group's own middleware exactly as
+// the group's real routes do.
+func NewRouterWithPageProbeForTest(
+	log *slog.Logger,
+	cfg *config.Config,
+	mw *middleware.Middleware,
+	h *handlers.Handlers,
+	sentryEnabled bool,
+	probe http.HandlerFunc,
+) http.Handler {
+	s := &Server{
+		log:    log,
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
+	}
+	s.sentryEnabled.Store(sentryEnabled)
+	s.SetupRoutes()
+
+	for _, route := range s.router.Routes() {
+		pages, ok := route.SubRoutes.(chi.Router)
+		if ok && route.Pattern == "/pages/*" {
+			pages.Get("/probe", probe)
+		}
+	}
+
+	return s.router
+}

internal/server/routes.go

@@ -15,9 +15,10 @@ import (
 // bytes) for form POST endpoints. 1 MB is generous for any form
 // submission while preventing abuse from oversized payloads.
 //
-// Every route group below installs MaxBodySize(maxFormBodySize) as
-// its FIRST middleware, ahead of both CSRF and RequireAuth. Both
-// orderings are deliberate.
+// The four admin page route groups below (/pages, /user/{username},
+// /hooks and /hook/{sourceID}) install MaxBodySize(maxFormBodySize)
+// right after their recoverer and error reporting, ahead of both CSRF
+// and RequireAuth. Both orderings are deliberate.
 //
 // Ahead of CSRF because gorilla/csrf parses the form. The cap has to
 // be installed before anything reads the body, or the parse runs
@@ -46,6 +47,14 @@ const requestTimeout = 60 * time.Second
 // server's router.
 func (s *Server) SetupRoutes() {
 	s.router = chi.NewRouter()
+
+	// An unknown path gets the error page. Registered before the
+	// global middleware, because chi wraps a not-found handler in the
+	// middleware already on its router, which would then run twice.
+	// The route groups below wrap it in their own middleware the same
+	// way; running theirs twice is harmless.
+	s.router.NotFound(s.h.HandleErrorPage(http.StatusNotFound))
+
 	s.setupGlobalMiddleware()
 	s.setupRoutes()
 }
@@ -69,23 +78,33 @@ func (s *Server) setupGlobalMiddleware() {
 	// Panic recovery, deliberately here rather than first. It has to
 	// run inside every middleware that observes the response, so the
 	// 500 it writes is the status the access log records and the
-	// metrics count, and outside the sentryhttp handler below, whose
+	// metrics count, and outside the sentryhttp handler, whose
 	// Repanic option needs something further out to catch what it
 	// re-raises. chi's own middleware.Recoverer held the first slot
 	// until it was measured: on a current Go release it crashes
 	// inside its stack pretty-printer instead of recovering, so the
 	// connection dropped and the original panic was never reported.
 	// See https://git.eeqj.de/sneak/webhooker/issues/187.
-	s.router.Use(s.mw.Recoverer())
+	s.recoverPanics(s.router, nil)
+}
+
+// recoverPanics installs on r the recoverer, answering a panic with
+// page (a plain 500 when page is nil), and inside it the Sentry error
+// reporting (if SENTRY_DSN is set). Repanic is true so panics still
+// bubble up to the recoverer.
+//
+// Each admin page route group installs its own, with the error page,
+// as its first middleware. A panic there is logged, reported and
+// answered inside the group and never reaches the global recoverer,
+// which keeps the plain 500 for every other route.
+func (s *Server) recoverPanics(r chi.Router, page http.Handler) {
+	r.Use(s.mw.Recoverer(page))
 
-	// Sentry error reporting (if SENTRY_DSN is set). Repanic is
-	// true so panics still bubble up to the Recoverer middleware
-	// registered immediately above.
 	if s.sentryEnabled.Load() {
 		sentryHandler := sentryhttp.New(sentryhttp.Options{
 			Repanic: true,
 		})
-		s.router.Use(sentryHandler.Handle)
+		r.Use(sentryHandler.Handle)
 	}
 }
 
@@ -147,10 +166,13 @@ func (s *Server) setupRoutes() {
 
 func (s *Server) setupPageRoutes() {
 	s.router.Route("/pages", func(r chi.Router) {
+		s.recoverPanics(
+			r, s.h.HandleErrorPage(http.StatusInternalServerError),
+		)
 		// MaxBodySize precedes CSRF and RequireAuth deliberately;
 		// see maxFormBodySize for why, and for what it costs.
 		r.Use(s.mw.MaxBodySize(maxFormBodySize))
-		r.Use(s.mw.CSRF())
+		r.Use(s.mw.CSRF(s.h.HandleErrorPage(http.StatusForbidden)))
 		r.Use(s.mw.NoCache())
 
 		// The login POST carries no pre-emptive rate limiter. Behind
@@ -169,10 +191,13 @@ func (s *Server) setupPageRoutes() {
 
 func (s *Server) setupUserRoutes() {
 	s.router.Route("/user/{username}", func(r chi.Router) {
+		s.recoverPanics(
+			r, s.h.HandleErrorPage(http.StatusInternalServerError),
+		)
 		// MaxBodySize precedes CSRF and RequireAuth deliberately;
 		// see maxFormBodySize for why, and for what it costs.
 		r.Use(s.mw.MaxBodySize(maxFormBodySize))
-		r.Use(s.mw.CSRF())
+		r.Use(s.mw.CSRF(s.h.HandleErrorPage(http.StatusForbidden)))
 		r.Use(s.mw.NoCache())
 		r.Use(s.mw.RequireAuth())
 		r.Get("/", s.h.HandleProfile())
@@ -184,10 +209,13 @@ func (s *Server) setupUserRoutes() {
 
 func (s *Server) setupSourceRoutes() {
 	s.router.Route("/hooks", func(r chi.Router) {
+		s.recoverPanics(
+			r, s.h.HandleErrorPage(http.StatusInternalServerError),
+		)
 		// MaxBodySize precedes CSRF and RequireAuth deliberately;
 		// see maxFormBodySize for why, and for what it costs.
 		r.Use(s.mw.MaxBodySize(maxFormBodySize))
-		r.Use(s.mw.CSRF())
+		r.Use(s.mw.CSRF(s.h.HandleErrorPage(http.StatusForbidden)))
 		r.Use(s.mw.NoCache())
 		r.Use(s.mw.RequireAuth())
 		r.Get("/", s.h.HandleSourceList())
@@ -196,10 +224,13 @@ func (s *Server) setupSourceRoutes() {
 	})
 
 	s.router.Route("/hook/{sourceID}", func(r chi.Router) {
+		s.recoverPanics(
+			r, s.h.HandleErrorPage(http.StatusInternalServerError),
+		)
 		// MaxBodySize precedes CSRF and RequireAuth deliberately;
 		// see maxFormBodySize for why, and for what it costs.
 		r.Use(s.mw.MaxBodySize(maxFormBodySize))
-		r.Use(s.mw.CSRF())
+		r.Use(s.mw.CSRF(s.h.HandleErrorPage(http.StatusForbidden)))
 		r.Use(s.mw.NoCache())
 		r.Use(s.mw.RequireAuth())
 		r.Get("/", s.h.HandleSourceDetail())