internal/middleware/middleware.go

@@ -600,7 +600,10 @@ func bodyLimitedMethod(method string) bool {
 }
 
 // MaxBodySize returns middleware that limits the size of
-// POST/PUT/PATCH request bodies to maxBytes. It must be registered
+// POST/PUT/PATCH request bodies to maxBytes. A request with any other
+// method passes through uncapped, deliberately: no route behind it
+// reads a body on GET, HEAD or DELETE. A handler that starts to needs
+// its method added to bodyLimitedMethod first. It must be registered
 // before any middleware that parses the body — notably CSRF, which
 // calls r.PostFormValue — so that form parsing happens under this
 // cap rather than net/http's 10 MB default.

internal/middleware/middleware_test.go

@@ -730,10 +730,8 @@ func TestNoCache_SetsHeaders(t *testing.T) {
 
 const testBodyLimit int64 = 64
 
-// maxBodySizeHandler wraps a sentinel handler in MaxBodySize with
+// maxBodySizeResult is what runMaxBodySize's sentinel handler saw,
-// testBodyLimit. The sentinel records whether it ran and how much of
+// together with the response.
-// the body it managed to read, so tests can distinguish "never
-// reached" from "reached but truncated".
 type maxBodySizeResult struct {
 	called   bool
 	read     int
@@ -741,6 +739,10 @@ type maxBodySizeResult struct {
 	response *httptest.ResponseRecorder
 }
 
+// runMaxBodySize wraps a sentinel handler in MaxBodySize with
+// testBodyLimit and serves req through it. The sentinel records
+// whether it ran and how much of the body it managed to read, so
+// tests can distinguish "never reached" from "reached but truncated".
 func runMaxBodySize(
 	t *testing.T,
 	req *http.Request,

internal/server/error_page_test.go

@@ -191,7 +191,7 @@ func TestErrorPage_PanicOnAdminPage(t *testing.T) {
 
 	w := serve(
 		server.NewRouterWithPageProbeForTest(
-			env.log.Get(), env.cfg, env.mw, env.hnd,
+			t, env.log, env.cfg, env.mw, env.hnd,
 			true, panicProbeHandler,
 		),
 		server.PageProbePattern,
@@ -200,7 +200,7 @@ func TestErrorPage_PanicOnAdminPage(t *testing.T) {
 
 	w = serve(
 		server.NewRouterWithProbeForTest(
-			env.log.Get(), env.cfg, env.mw, env.hnd,
+			t, env.log, env.cfg, env.mw, env.hnd,
 			true, panicProbeHandler,
 		),
 		server.ProbePattern,

internal/server/export_test.go

@@ -1,13 +1,16 @@
 package server
 
 import (
-	"log/slog"
 	"net/http"
+	"testing"
 
 	"github.com/getsentry/sentry-go"
 	"github.com/go-chi/chi"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/fx/fxtest"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/handlers"
+	"sneak.berlin/go/webhooker/internal/logger"
 	"sneak.berlin/go/webhooker/internal/middleware"
 )
 
@@ -34,23 +37,45 @@ func SentryClientOptionsForTest(
 	return sentryClientOptions(dsn, release)
 }
 
+// newServerForTest builds a Server through New, as the application
+// does, on a lifecycle that is never started: the hooks New adds to
+// it never run, so nothing listens.
+func newServerForTest(
+	t *testing.T,
+	log *logger.Logger,
+	cfg *config.Config,
+	mw *middleware.Middleware,
+	h *handlers.Handlers,
+) *Server {
+	t.Helper()
+
+	s, err := New(fxtest.NewLifecycle(t), ServerParams{
+		Logger:     log,
+		Config:     cfg,
+		Middleware: mw,
+		Handlers:   h,
+	})
+	require.NoError(t, err)
+
+	return s
+}
+
 // NewRouterForTest builds the real route tree via SetupRoutes with
-// the supplied middleware and handlers, bypassing the fx lifecycle
+// the supplied middleware and handlers, on a Server from New whose
-// and the HTTP listener. Tests use it so that route-group middleware
+// lifecycle is never started, so no HTTP listener runs. Tests use it
-// registration order is exercised exactly as it ships, rather than
+// so that route-group middleware registration order is exercised
-// against a hand-rebuilt chain that could drift from routes.go.
+// exactly as it ships, rather than against a hand-rebuilt chain that
+// could drift from routes.go.
 func NewRouterForTest(
-	log *slog.Logger,
+	t *testing.T,
+	log *logger.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 ) http.Handler {
-	s := &Server{
+	t.Helper()
-		log:    log,
+
-		mw:     mw,
+	s := newServerForTest(t, log, cfg, mw, h)
-		h:      h,
-		params: ServerParams{Config: cfg},
-	}
 	s.SetupRoutes()
 
 	return s.router
@@ -83,19 +108,17 @@ const ProbePattern = "/probe"
 // option and the recoverer registered outside it is the thing a test
 // has to be able to pin.
 func NewRouterWithProbeForTest(
-	log *slog.Logger,
+	t *testing.T,
+	log *logger.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 	sentryEnabled bool,
 	probe http.HandlerFunc,
 ) http.Handler {
-	s := &Server{
+	t.Helper()
-		log:    log,
+
-		mw:     mw,
+	s := newServerForTest(t, log, cfg, mw, h)
-		h:      h,
-		params: ServerParams{Config: cfg},
-	}
 	s.sentryEnabled.Store(sentryEnabled)
 	s.SetupRoutes()
 	s.router.Handle(ProbePattern, probe)
@@ -113,19 +136,17 @@ const PageProbePattern = "/pages/probe"
 // it, so the probe runs behind that group's own middleware exactly as
 // the group's real routes do.
 func NewRouterWithPageProbeForTest(
-	log *slog.Logger,
+	t *testing.T,
+	log *logger.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 	sentryEnabled bool,
 	probe http.HandlerFunc,
 ) http.Handler {
-	s := &Server{
+	t.Helper()
-		log:    log,
+
-		mw:     mw,
+	s := newServerForTest(t, log, cfg, mw, h)
-		h:      h,
-		params: ServerParams{Config: cfg},
-	}
 	s.sentryEnabled.Store(sentryEnabled)
 	s.SetupRoutes()
 

internal/server/recoverer_test.go

@@ -199,7 +199,7 @@ func TestPanicProbeChild(t *testing.T) {
 	env := newTestEnv(t)
 
 	router := server.NewRouterWithProbeForTest(
-		env.log.Get(), env.cfg, env.mw, env.hnd,
+		t, env.log, env.cfg, env.mw, env.hnd,
 		false, panicProbeHandler,
 	)
 
@@ -253,7 +253,7 @@ func TestSentryStillSeesAPanic(t *testing.T) {
 	require.NoError(t, err)
 
 	router := server.NewRouterWithProbeForTest(
-		env.log.Get(), env.cfg, env.mw, env.hnd,
+		t, env.log, env.cfg, env.mw, env.hnd,
 		true, panicProbeHandler,
 	)
 

internal/server/response_controller_test.go

@@ -67,11 +67,11 @@ func TestResponseControllerThroughProductionRouter(t *testing.T) {
 
 			routers := map[string]http.Handler{
 				server.ProbePattern: server.NewRouterWithProbeForTest(
-					env.log.Get(), env.cfg, env.mw, env.hnd,
+					t, env.log, env.cfg, env.mw, env.hnd,
 					tc.sentryEnabled, probe,
 				),
 				server.PageProbePattern: server.NewRouterWithPageProbeForTest(
-					env.log.Get(), env.cfg, env.mw, env.hnd,
+					t, env.log, env.cfg, env.mw, env.hnd,
 					tc.sentryEnabled, probe,
 				),
 			}

internal/server/routes_test.go

@@ -136,7 +136,7 @@ func newTestEnvWithConfig(
 	t.Cleanup(app.RequireStop)
 
 	return &testEnv{
-		router: server.NewRouterForTest(log.Get(), cfg, mw, hnd),
+		router: server.NewRouterForTest(t, log, cfg, mw, hnd),
 		sess:   sess,
 		db:     db,
 		dbMgr:  dbMgr,
@@ -531,6 +531,48 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 	}
 }
 
+// --- every page route group ---
+
+// TestPageRouteGroups_OversizeBody_RejectedBeforeCSRF pins the body
+// cap ahead of CSRF and RequireAuth in every page route group. The
+// requests carry no session and no CSRF token, so if either ran first
+// the answer would be a 403 or a redirect to the login page rather
+// than 413, and CSRF would issue its cookie (see
+// TestPagesLogin_UnderLimit_NoToken_CSRFRejects). /settings has no
+// POST route, but its group's middleware runs before the method is
+// matched, so a POST there still reaches CSRF's form parsing if the
+// cap moves after it. The user and webhook in the paths need not
+// exist: nothing after the cap runs.
+func TestPageRouteGroups_OversizeBody_RejectedBeforeCSRF(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+
+	form := url.Values{}
+	form.Set("name", oversizeValue())
+
+	for _, path := range []string{
+		"/pages/login",
+		"/user/nobody/password",
+		"/settings/",
+		"/hooks/new",
+		"/hook/nonexistent/edit",
+	} {
+		w := env.post(path, form, nil)
+
+		assert.Equal(
+			t, http.StatusRequestEntityTooLarge, w.Code, path,
+		)
+		assert.False(
+			t, csrfCookieSet(w),
+			"CSRF middleware must not run for an oversized body to %s",
+			path,
+		)
+	}
+}
+
 // --- /pages group ---
 
 // TestPagesLogin_OversizeBody_RejectedBeforeCSRF proves the cap runs
@@ -1610,7 +1652,7 @@ func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
 	)
 	third := &testEnv{
 		router: server.NewRouterForTest(
-			first.log.Get(), first.cfg, first.mw, first.hnd,
+			t, first.log, first.cfg, first.mw, first.hnd,
 		),
 	}