Enforce the body size limit before CSRF parses the form (closes #90)

chi runs Use middleware in registration order, and every form route
group registered CSRF() before MaxBodySize(). gorilla/csrf calls
r.PostFormValue, so the form was parsed under net/http's 10 MB default
and the intended 1 MB cap never applied to form fields. The
/user/{username} group, which carries POST /password, had no
MaxBodySize registration at all.

- Register MaxBodySize ahead of CSRF in /pages, /sources, and
  /source/{sourceID}, and add it to /user/{username}.
- Reject a declared-oversize body up front with 413. Reordering alone
  cannot produce one: http.MaxBytesReader surfaces its error on Read,
  so the form parse fails and gorilla/csrf answers 403 "no token" for
  what is really an oversized body. MaxBytesReader is still installed
  afterwards so chunked or length-lying clients stay hard-capped.
- Drop the handler-local MaxBytesReader calls in auth.go, profile.go,
  and source_management.go now that the middleware is the single
  enforcement point. maxBodyShift stays; webhook.go still uses it.

The /webhook/{uuid} receiver is untouched: it bounds itself with
io.LimitReader in readWebhookBody and is neither CSRF-protected nor
form-parsed.

Tests cover the middleware in isolation (declared oversize is rejected
without reaching a sentinel handler; at-limit and under-limit bodies
pass through intact; GET is unaffected; an undeclared oversize body is
truncated at the cap) and the real router built by SetupRoutes, so the
registration order itself is guarded: an oversized POST to
/pages/login returns 413 with no gorilla/csrf cookie issued, an
oversized POST /password with a valid session and CSRF token returns
413 and leaves the stored hash unchanged, and under-limit requests
still complete through the normal CSRF path.

internal/server/routes_test.go

@@ -0,0 +1,375 @@
+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) {}
+
+// 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{} },
+			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",
+	)
+}