From d084f4f912dfe0e18e6f3ce85b0988dbbd93702c Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Fri, 2 Oct 2026 17:53:21 +0200 Subject: [PATCH] Pin the body cap's order in every page route group (closes #93) Follow-ups from an August review of the body cap, each checked against the current tree. One route test now requires an oversized POST, with no session and no CSRF token, to be refused with 413 before CSRF runs, in every page route group with a POST route and in /settings/, so moving a group's body cap after CSRF fails it. The three test router helpers build the server through New with a lifecycle that is never started, so no field is set by hand. The middleware test comment names runMaxBodySize, and the MaxBodySize doc comment says methods other than POST, PUT and PATCH pass uncapped on purpose. The README item was already settled. Model: opus-5-5 --- internal/middleware/middleware.go | 5 +- internal/middleware/middleware_test.go | 10 +-- internal/server/error_page_test.go | 4 +- internal/server/export_test.go | 73 +++++++++++++-------- internal/server/recoverer_test.go | 4 +- internal/server/response_controller_test.go | 4 +- internal/server/routes_test.go | 46 ++++++++++++- 7 files changed, 107 insertions(+), 39 deletions(-) diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 51c8bbd..a1a312b 100644 --- a/internal/middleware/middleware.go +++ b/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. diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index e576659..4482232 100644 --- a/internal/middleware/middleware_test.go +++ b/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 -// testBodyLimit. 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". +// maxBodySizeResult is what runMaxBodySize's sentinel handler saw, +// together with the response. 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, diff --git a/internal/server/error_page_test.go b/internal/server/error_page_test.go index 06abf2e..7f18dba 100644 --- a/internal/server/error_page_test.go +++ b/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, diff --git a/internal/server/export_test.go b/internal/server/export_test.go index 4c4f645..36ce86a 100644 --- a/internal/server/export_test.go +++ b/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 -// and the HTTP listener. Tests use it so that route-group middleware -// registration order is exercised exactly as it ships, rather than -// against a hand-rebuilt chain that could drift from routes.go. +// the supplied middleware and handlers, on a Server from New whose +// lifecycle is never started, so no HTTP listener runs. Tests use it +// so that route-group middleware registration order is exercised +// 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{ - log: log, - mw: mw, - h: h, - params: ServerParams{Config: cfg}, - } + t.Helper() + + s := newServerForTest(t, log, cfg, mw, h) 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{ - log: log, - mw: mw, - h: h, - params: ServerParams{Config: cfg}, - } + t.Helper() + + s := newServerForTest(t, log, cfg, mw, h) 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{ - log: log, - mw: mw, - h: h, - params: ServerParams{Config: cfg}, - } + t.Helper() + + s := newServerForTest(t, log, cfg, mw, h) s.sentryEnabled.Store(sentryEnabled) s.SetupRoutes() diff --git a/internal/server/recoverer_test.go b/internal/server/recoverer_test.go index 25a90f0..df0805d 100644 --- a/internal/server/recoverer_test.go +++ b/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, ) diff --git a/internal/server/response_controller_test.go b/internal/server/response_controller_test.go index ee08889..debb103 100644 --- a/internal/server/response_controller_test.go +++ b/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, ), } diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index 1d12f11..6eb605c 100644 --- a/internal/server/routes_test.go +++ b/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, ), }