From d51cd0fd29b95d772c3e0f43d4ed10a7f68275e2 Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 11 Aug 2026 14:37:38 +0200 Subject: [PATCH] Enforce the body size limit before CSRF parses the form (closes #90) CSRF ran before MaxBodySize, so the CSRF middleware parsed the form body before any cap applied and an oversized request was read in full before being rejected. MaxBodySize is now the first middleware in all four route groups that parse forms, ahead of CSRF and RequireAuth. An oversize request therefore gets 413 without the handler running and without state changing, including the password-change route. Note the ordering trade: an unauthenticated client now receives 413 rather than an auth redirect on /user/{username}/password. --- README.md | 17 +- internal/handlers/auth.go | 6 +- internal/handlers/profile.go | 5 +- internal/handlers/source_management.go | 36 +-- internal/middleware/middleware.go | 63 +++- internal/middleware/middleware_test.go | 149 ++++++++++ internal/server/export_test.go | 36 +++ internal/server/routes.go | 15 +- internal/server/routes_test.go | 383 +++++++++++++++++++++++++ 9 files changed, 662 insertions(+), 48 deletions(-) create mode 100644 internal/server/export_test.go create mode 100644 internal/server/routes_test.go diff --git a/README.md b/README.md index 12900f5..dd26e9b 100644 --- a/README.md +++ b/README.md @@ -963,9 +963,17 @@ Applied to all routes in this order: 8. **Sentry** — Error reporting to Sentry (if `SENTRY_DSN` is set; configured with `Repanic: true` so panics still reach Recoverer) -Additionally, form endpoints (`/pages`, `/sources`, `/source/*`) apply a -**MaxBodySize** middleware that limits POST/PUT/PATCH request bodies to -1 MB using `http.MaxBytesReader`, preventing oversized form submissions. +Additionally, form endpoints (`/pages`, `/user/*`, `/sources`, +`/source/*`) apply a **MaxBodySize** middleware that limits +POST/PUT/PATCH request bodies to 1 MB. It is registered ahead of the +CSRF middleware in every one of those route groups, because +gorilla/csrf parses the form; if the cap were installed after it, form +parsing would run under net/http's 10 MB default and the 1 MB limit +would never apply. A request that declares a `Content-Length` over the +limit is answered with `413 Request Entity Too Large` before any other +middleware or handler runs; a chunked request, or one that lies about +its length, is hard-capped by `http.MaxBytesReader` and fails +downstream at form-parse time. ### Authentication @@ -987,7 +995,8 @@ Additionally, form endpoints (`/pages`, `/sources`, `/source/*`) apply a - Production security headers on all responses: HSTS, X-Content-Type-Options (`nosniff`), X-Frame-Options (`DENY`), Content-Security-Policy, Referrer-Policy, and Permissions-Policy -- Request body size limits (1 MB) on all form POST endpoints +- Request body size limits (1 MB) on all form POST endpoints, enforced + by middleware that runs before CSRF parses the form - **CSRF protection** via [gorilla/csrf](https://github.com/gorilla/csrf) on all state-changing forms (cookie-based double-submit tokens with HMAC authentication). Applied to `/pages`, `/sources`, `/source`, and diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index b934280..e79d7f5 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -29,10 +29,8 @@ func (h *Handlers) HandleLoginPage() http.HandlerFunc { // HandleLoginSubmit handles the login form submission (POST) func (h *Handlers) HandleLoginSubmit() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - // Limit request body to prevent memory exhaustion - r.Body = http.MaxBytesReader(w, r.Body, 1< maxBytes { + s.log.Warn( + "request body exceeds limit", + "method", r.Method, + "path", r.URL.Path, + "content_length", r.ContentLength, + "limit", maxBytes, + ) + http.Error( + w, + "Request Entity Too Large", + http.StatusRequestEntityTooLarge, + ) + + return + } + + r.Body = http.MaxBytesReader(w, r.Body, maxBytes) + next.ServeHTTP(w, r) }) } diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index 71e49ad..fa18f11 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -3,10 +3,12 @@ package middleware_test import ( "context" "encoding/base64" + "io" "log/slog" "net/http" "net/http/httptest" "os" + "strings" "testing" "time" @@ -648,6 +650,153 @@ func TestNoCache_SetsHeaders(t *testing.T) { ) } +// --- MaxBodySize Middleware Tests --- + +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". +type maxBodySizeResult struct { + called bool + read int + readErr error + response *httptest.ResponseRecorder +} + +func runMaxBodySize( + t *testing.T, + req *http.Request, +) *maxBodySizeResult { + t.Helper() + + m, _ := testMiddleware(t, config.EnvironmentDev) + res := &maxBodySizeResult{response: httptest.NewRecorder()} + + handler := m.MaxBodySize(testBodyLimit)(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + res.called = true + + body, err := io.ReadAll(r.Body) + res.read = len(body) + res.readErr = err + + w.WriteHeader(http.StatusOK) + }, + )) + + handler.ServeHTTP(res.response, req) + + return res +} + +// postWithBody builds a POST request whose Content-Length is +// accurate for the given payload size. +func postWithBody(size int) *http.Request { + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPost, "/pages/login", + strings.NewReader(strings.Repeat("a", size)), + ) + req.Header.Set( + "Content-Type", "application/x-www-form-urlencoded", + ) + + return req +} + +func TestMaxBodySize_DeclaredOversize_413AndHandlerNotReached( + t *testing.T, +) { + t.Parallel() + + res := runMaxBodySize(t, postWithBody(int(testBodyLimit)+1)) + + assert.False( + t, res.called, + "handler must not be reached for an oversized body", + ) + assert.Equal( + t, http.StatusRequestEntityTooLarge, res.response.Code, + ) +} + +func TestMaxBodySize_AtLimit_PassesThrough(t *testing.T) { + t.Parallel() + + res := runMaxBodySize(t, postWithBody(int(testBodyLimit))) + + assert.True( + t, res.called, + "handler should be reached for a body at the limit", + ) + require.NoError(t, res.readErr) + assert.Equal(t, int(testBodyLimit), res.read) + assert.Equal(t, http.StatusOK, res.response.Code) +} + +func TestMaxBodySize_UnderLimit_PassesThrough(t *testing.T) { + t.Parallel() + + res := runMaxBodySize(t, postWithBody(1)) + + assert.True(t, res.called) + require.NoError(t, res.readErr) + assert.Equal(t, 1, res.read) + assert.Equal(t, http.StatusOK, res.response.Code) +} + +func TestMaxBodySize_GetWithOversizeBody_NotCapped(t *testing.T) { + t.Parallel() + + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodGet, "/pages/login", + strings.NewReader( + strings.Repeat("a", int(testBodyLimit)+1), + ), + ) + + res := runMaxBodySize(t, req) + + assert.True( + t, res.called, + "GET requests are not subject to the POST body cap", + ) + require.NoError(t, res.readErr) + assert.Equal(t, int(testBodyLimit)+1, res.read) +} + +// TestMaxBodySize_UndeclaredOversize_TruncatedAtCap covers the +// chunked / lying-Content-Length case: there is nothing to check up +// front, so the request reaches the handler but MaxBytesReader +// hard-caps the body and the read fails at the limit. +func TestMaxBodySize_UndeclaredOversize_TruncatedAtCap( + t *testing.T, +) { + t.Parallel() + + req := postWithBody(int(testBodyLimit) + 1) + // Simulate a chunked request: no declared length. + req.ContentLength = -1 + + res := runMaxBodySize(t, req) + + assert.True( + t, res.called, + "an undeclared oversize body cannot be rejected up front", + ) + require.Error( + t, res.readErr, + "reading past the cap must fail", + ) + assert.Equal( + t, int(testBodyLimit), res.read, + "the handler must not see more than the cap", + ) +} + // --- Helper Tests --- func TestIpFromHostPort(t *testing.T) { diff --git a/internal/server/export_test.go b/internal/server/export_test.go new file mode 100644 index 0000000..a4ed0ca --- /dev/null +++ b/internal/server/export_test.go @@ -0,0 +1,36 @@ +package server + +import ( + "log/slog" + "net/http" + + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/handlers" + "sneak.berlin/go/webhooker/internal/middleware" +) + +// MaxFormBodySizeForTest exposes the form body cap so tests can +// build requests that sit exactly at, below, and above it. +const MaxFormBodySizeForTest = maxFormBodySize + +// 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. +func NewRouterForTest( + log *slog.Logger, + cfg *config.Config, + mw *middleware.Middleware, + h *handlers.Handlers, +) http.Handler { + s := &Server{ + log: log, + mw: mw, + h: h, + params: ServerParams{Config: cfg}, + } + s.SetupRoutes() + + return s.router +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 71e7a32..75a68b4 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -90,9 +90,11 @@ func (s *Server) setupRoutes() { func (s *Server) setupPageRoutes() { s.router.Route("/pages", func(r chi.Router) { + // MaxBodySize must precede CSRF: gorilla/csrf parses the + // form, so the cap has to be installed before it runs. + r.Use(s.mw.MaxBodySize(maxFormBodySize)) r.Use(s.mw.CSRF()) r.Use(s.mw.NoCache()) - r.Use(s.mw.MaxBodySize(maxFormBodySize)) r.Group(func(r chi.Router) { r.Use(s.mw.LoginRateLimit()) @@ -106,6 +108,9 @@ func (s *Server) setupPageRoutes() { func (s *Server) setupUserRoutes() { s.router.Route("/user/{username}", func(r chi.Router) { + // MaxBodySize must precede CSRF: gorilla/csrf parses the + // form, so the cap has to be installed before it runs. + r.Use(s.mw.MaxBodySize(maxFormBodySize)) r.Use(s.mw.CSRF()) r.Use(s.mw.NoCache()) r.Use(s.mw.RequireAuth()) @@ -118,20 +123,24 @@ func (s *Server) setupUserRoutes() { func (s *Server) setupSourceRoutes() { s.router.Route("/sources", func(r chi.Router) { + // MaxBodySize must precede CSRF: gorilla/csrf parses the + // form, so the cap has to be installed before it runs. + r.Use(s.mw.MaxBodySize(maxFormBodySize)) r.Use(s.mw.CSRF()) r.Use(s.mw.NoCache()) r.Use(s.mw.RequireAuth()) - r.Use(s.mw.MaxBodySize(maxFormBodySize)) r.Get("/", s.h.HandleSourceList()) r.Get("/new", s.h.HandleSourceCreate()) r.Post("/new", s.h.HandleSourceCreateSubmit()) }) s.router.Route("/source/{sourceID}", func(r chi.Router) { + // MaxBodySize must precede CSRF: gorilla/csrf parses the + // form, so the cap has to be installed before it runs. + r.Use(s.mw.MaxBodySize(maxFormBodySize)) r.Use(s.mw.CSRF()) r.Use(s.mw.NoCache()) r.Use(s.mw.RequireAuth()) - r.Use(s.mw.MaxBodySize(maxFormBodySize)) r.Get("/", s.h.HandleSourceDetail()) r.Get("/edit", s.h.HandleSourceEdit()) r.Post("/edit", s.h.HandleSourceEditSubmit()) diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go new file mode 100644 index 0000000..1c5f72d --- /dev/null +++ b/internal/server/routes_test.go @@ -0,0 +1,383 @@ +package server_test + +import ( + "context" + "html" + "net/http" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "go.uber.org/fx/fxtest" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" + "sneak.berlin/go/webhooker/internal/globals" + "sneak.berlin/go/webhooker/internal/handlers" + "sneak.berlin/go/webhooker/internal/healthcheck" + "sneak.berlin/go/webhooker/internal/logger" + "sneak.berlin/go/webhooker/internal/middleware" + "sneak.berlin/go/webhooker/internal/server" + "sneak.berlin/go/webhooker/internal/session" +) + +// csrfCookieName is the cookie gorilla/csrf issues when it runs. Its +// presence or absence on a response is how these tests tell whether +// the CSRF middleware executed. +const csrfCookieName = "_gorilla_csrf" + +type noopNotifier struct{} + +func (n *noopNotifier) Notify([]delivery.Task) {} + +// noopEvictor satisfies handlers.New's delivery.WebhookEvictor +// dependency. These tests never delete a webhook, so there is +// nothing to record. +type noopEvictor struct{} + +func (e *noopEvictor) EvictWebhook(string) {} + +// testEnv is the real router from routes.go plus the collaborators +// tests need to seed users and forge sessions. +type testEnv struct { + router http.Handler + sess *session.Session + db *database.Database +} + +// newTestEnv wires the dependency graph with fx and builds the +// production route tree, so middleware registration order is +// exercised exactly as it ships. +func newTestEnv(t *testing.T) *testEnv { + t.Helper() + + var ( + log *logger.Logger + cfg *config.Config + mw *middleware.Middleware + hnd *handlers.Handlers + sess *session.Session + db *database.Database + ) + + app := fxtest.New( + t, + fx.Provide( + globals.New, + logger.New, + func() *config.Config { + return &config.Config{ + DataDir: t.TempDir(), + Environment: config.EnvironmentDev, + } + }, + database.New, + database.NewWebhookDBManager, + healthcheck.New, + session.New, + func() delivery.Notifier { return &noopNotifier{} }, + func() delivery.WebhookEvictor { return &noopEvictor{} }, + middleware.New, + handlers.New, + ), + fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db), + ) + app.RequireStart() + t.Cleanup(app.RequireStop) + + return &testEnv{ + router: server.NewRouterForTest(log.Get(), cfg, mw, hnd), + sess: sess, + db: db, + } +} + +// oversizeValue returns a form value one byte past the route-group +// body cap, so an encoded form containing it is guaranteed oversize. +func oversizeValue() string { + return strings.Repeat("a", int(server.MaxFormBodySizeForTest)+1) +} + +// csrfCookieSet reports whether the response issued a gorilla/csrf +// cookie, which only happens if the CSRF middleware ran. +func csrfCookieSet(w *httptest.ResponseRecorder) bool { + for _, c := range w.Result().Cookies() { + if c.Name == csrfCookieName { + return true + } + } + + return false +} + +// get issues a GET through the router with the supplied cookies. +func (e *testEnv) get( + path string, + cookies []*http.Cookie, +) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, path, nil, + ) + + for _, c := range cookies { + req.AddCookie(c) + } + + w := httptest.NewRecorder() + e.router.ServeHTTP(w, req) + + return w +} + +// post issues a urlencoded form POST through the router. The body is +// a strings.Reader, so the request carries an accurate +// Content-Length — the signal MaxBodySize checks up front. +func (e *testEnv) post( + path string, + form url.Values, + cookies []*http.Cookie, +) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, path, + strings.NewReader(form.Encode()), + ) + req.Header.Set( + "Content-Type", "application/x-www-form-urlencoded", + ) + + for _, c := range cookies { + req.AddCookie(c) + } + + w := httptest.NewRecorder() + e.router.ServeHTTP(w, req) + + return w +} + +// csrfFrom renders the page at path and returns the CSRF token from +// its form together with every cookie needed for the follow-up POST. +func (e *testEnv) csrfFrom( + t *testing.T, + path string, + cookies []*http.Cookie, +) (string, []*http.Cookie) { + t.Helper() + + w := e.get(path, cookies) + require.Equal(t, http.StatusOK, w.Code) + + pattern := regexp.MustCompile( + `name="csrf_token" value="([^"]+)"`, + ) + + match := pattern.FindStringSubmatch(w.Body.String()) + require.Len(t, match, 2, "form must embed a CSRF token") + + // html/template escapes "+" and "=" in attribute values, and + // gorilla/csrf tokens are standard base64, so the value read + // out of the markup has to be unescaped before it is submitted. + token := html.UnescapeString(match[1]) + + combined := make([]*http.Cookie, 0, len(cookies)) + combined = append(combined, cookies...) + combined = append(combined, w.Result().Cookies()...) + + return token, combined +} + +// authCookies forges an authenticated session for the given user. +func (e *testEnv) authCookies( + t *testing.T, + userID, username string, +) []*http.Cookie { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/setup", nil, + ) + w := httptest.NewRecorder() + + s, err := e.sess.Get(req) + require.NoError(t, err) + + e.sess.SetUser(s, userID, username) + require.NoError(t, e.sess.Save(req, w, s)) + + cookies := w.Result().Cookies() + require.NotEmpty(t, cookies, "session cookie should be set") + + return cookies +} + +// seedUser creates a user with the given password and returns the +// stored hash so tests can assert whether it later changed. +func (e *testEnv) seedUser( + t *testing.T, + username, password string, +) (string, string) { + t.Helper() + + hash, err := database.HashPassword(password) + require.NoError(t, err) + + user := &database.User{Username: username, Password: hash} + require.NoError(t, e.db.DB().Create(user).Error) + + return user.ID, hash +} + +// storedHash reads the current password hash for a username. +func (e *testEnv) storedHash(t *testing.T, username string) string { + t.Helper() + + var user database.User + + require.NoError(t, + e.db.DB().Where("username = ?", username). + First(&user).Error, + ) + + return user.Password +} + +// --- /pages group --- + +// TestPagesLogin_OversizeBody_RejectedBeforeCSRF proves the cap runs +// ahead of gorilla/csrf: the response is a clean 413 and no CSRF +// cookie was issued, so neither the CSRF middleware nor the login +// handler ran. +func TestPagesLogin_OversizeBody_RejectedBeforeCSRF(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + form := url.Values{} + form.Set("username", oversizeValue()) + form.Set("password", "irrelevant") + + w := env.post("/pages/login", form, nil) + + assert.Equal( + t, http.StatusRequestEntityTooLarge, w.Code, + ) + assert.False( + t, csrfCookieSet(w), + "CSRF middleware must not run for an oversized body", + ) +} + +// TestPagesLogin_UnderLimit_NoToken_CSRFRejects is the control for +// the test above: an identically shaped but under-limit POST does +// reach gorilla/csrf, which rejects it and issues its cookie. Without +// this, the missing-cookie assertion above would prove nothing. +func TestPagesLogin_UnderLimit_NoToken_CSRFRejects(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + form := url.Values{} + form.Set("username", "someone") + form.Set("password", "irrelevant") + + w := env.post("/pages/login", form, nil) + + assert.Equal(t, http.StatusForbidden, w.Code) + assert.True( + t, csrfCookieSet(w), + "CSRF middleware should run for an under-limit body", + ) +} + +// TestPagesLogin_UnderLimit_ValidToken_ReachesHandler proves the +// reorder did not break CSRF token handling: a token harvested from +// the rendered login form is still accepted and the request lands in +// the handler. +func TestPagesLogin_UnderLimit_ValidToken_ReachesHandler( + t *testing.T, +) { + t.Parallel() + + env := newTestEnv(t) + + token, cookies := env.csrfFrom(t, "/pages/login", nil) + + form := url.Values{} + form.Set("csrf_token", token) + form.Set("username", "nosuchuser") + form.Set("password", "wrongpassword") + + w := env.post("/pages/login", form, cookies) + + assert.Equal(t, http.StatusUnauthorized, w.Code) + assert.Contains( + t, w.Body.String(), "Invalid username or password", + "request should reach the login handler", + ) +} + +// --- /user/{username} group --- + +// TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged +// covers the route that previously had no middleware body cap at +// all. The request carries a valid session and a valid CSRF token, +// so the only thing that can stop it is the size cap; the unchanged +// password hash is the observable proof the handler never ran. +func TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged( + t *testing.T, +) { + t.Parallel() + + env := newTestEnv(t) + + userID, originalHash := env.seedUser(t, "pwuser", "oldpassword") + cookies := env.authCookies(t, userID, "pwuser") + token, cookies := env.csrfFrom(t, "/user/pwuser/", cookies) + + form := url.Values{} + form.Set("csrf_token", token) + form.Set("current_password", "oldpassword") + form.Set("new_password", oversizeValue()) + form.Set("confirm_password", oversizeValue()) + + w := env.post("/user/pwuser/password", form, cookies) + + assert.Equal( + t, http.StatusRequestEntityTooLarge, w.Code, + ) + assert.Equal( + t, originalHash, env.storedHash(t, "pwuser"), + "handler must not run, so the password must be unchanged", + ) +} + +// TestPasswordChange_UnderLimit_Succeeds proves that adding the cap +// to the /user/{username} group did not break the route it guards. +func TestPasswordChange_UnderLimit_Succeeds(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + userID, originalHash := env.seedUser(t, "okuser", "oldpassword") + cookies := env.authCookies(t, userID, "okuser") + token, cookies := env.csrfFrom(t, "/user/okuser/", cookies) + + form := url.Values{} + form.Set("csrf_token", token) + form.Set("current_password", "oldpassword") + form.Set("new_password", "brandnewpassword") + form.Set("confirm_password", "brandnewpassword") + + w := env.post("/user/okuser/password", form, cookies) + + assert.Equal(t, http.StatusOK, w.Code) + assert.NotEqual( + t, originalHash, env.storedHash(t, "okuser"), + "an under-limit password change should still apply", + ) +}