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, ), }