Enforce request body size limit before CSRF middleware parses the form #90

New Issue

Closed

opened 2026-08-07 19:12:08 +02:00 by clawbot · 2 comments

clawbot commented 2026-08-07 19:12:08 +02:00

Collaborator

Copy Link

Tracking issue from the PR #83 review (non-blocking there — pre-existing idiom shared with the login handler).

Problem

Handlers install http.MaxBytesReader (1 MB) inside the handler, but gorilla/csrf (v1.7.3, helpers.go:113) has already parsed the request body via r.PostFormValue by the time the handler runs. The form is therefore parsed under net/http's default 10 MB cap, and the intended 1 MB limit is never effective for form fields. Affects at least the login and password-change handlers.

Definition of done

• The body size limit is applied as middleware BEFORE the CSRF middleware in the chain (route-group scoped), so form parsing happens under the intended cap.
• Handler-local MaxBytesReader calls that are now redundant are removed or kept consistent deliberately (documented either way).
• Test: an oversized POST body is rejected (413) and the handler is never reached.

Tracking issue from the PR #83 review (non-blocking there — pre-existing idiom shared with the login handler). ## Problem Handlers install `http.MaxBytesReader` (1 MB) inside the handler, but gorilla/csrf (v1.7.3, helpers.go:113) has already parsed the request body via `r.PostFormValue` by the time the handler runs. The form is therefore parsed under net/http's default 10 MB cap, and the intended 1 MB limit is never effective for form fields. Affects at least the login and password-change handlers. ## Definition of done - The body size limit is applied as middleware BEFORE the CSRF middleware in the chain (route-group scoped), so form parsing happens under the intended cap. - Handler-local `MaxBytesReader` calls that are now redundant are removed or kept consistent deliberately (documented either way). - Test: an oversized POST body is rejected (413) and the handler is never reached.

clawbot referenced this issue 2026-08-07 19:12:16 +02:00

Add admin password change flow (closes #65) #83

clawbot commented 2026-08-09 03:34:49 +02:00

Author

Collaborator

Copy Link

Implementation requirements

Baseline: main @ 4f5ecb1.

Confirmed root cause

internal/server/routes.go. chi runs Use middleware in registration order, and every group registers CSRF() before MaxBodySize(maxFormBodySize):

• /pages — CSRF() line 93, MaxBodySize line 95
• /sources — CSRF() line 118, MaxBodySize line 121
• /source/{sourceID} — CSRF() line 128, MaxBodySize line 131

gorilla/csrf calls r.PostFormValue (helpers.go:113), so the form is parsed under net/http's default 10 MB cap and the intended 1 MB limit never applies to form fields.

Worse, and not noted in the original report: the /user/{username} group (setupUserRoutes, lines 107-116) has no MaxBodySize at all, and that is where POST /password from #83 lives. So the password-change endpoint currently has no middleware-level body cap whatsoever — only the handler-local http.MaxBytesReader in profile.go:35, which runs after CSRF has already parsed the body.

1. Ordering

In every route group that accepts a POST, MaxBodySize(maxFormBodySize) must be registered before CSRF(). Groups to fix: /pages, /sources, /source/{sourceID}. Groups to fix and add the missing middleware to: /user/{username}.

2. Make 413 actually reachable

Reordering alone does not produce a 413. http.MaxBytesReader surfaces its error on Read, so CSRF's form parse just fails and gorilla/csrf answers 403 "no token" — the operator sees a misleading CSRF error for what is really an oversized body.

So MaxBodySize must also reject up front: if the request is a POST/PUT/PATCH and r.ContentLength is greater than the limit, write 413 and return without calling next. Keep installing http.MaxBytesReader as well, so a chunked or Content-Length-lying client is still hard-capped rather than buffered. Document that behaviour in the middleware doc comment: declared-oversize gets a clean 413, undeclared-oversize gets truncated at the cap and fails downstream.

3. Redundant handler-local limits

These now sit behind a middleware cap and are dead weight:

• internal/handlers/auth.go:33
• internal/handlers/profile.go:35
• internal/handlers/source_management.go lines 131, 390, 413, 729, 789, 812

Remove them and let the middleware be the single enforcement point — duplicated limits drift apart. If you keep any, the PR body must say which and why. maxBodyShift in handlers.go is still used by webhook.go, so do not delete the constant.

Out of scope: the /webhook/{uuid} receiver. It reads through its own io.LimitReader(r.Body, maxWebhookBodySize+1) in readWebhookBody, is not CSRF-protected, and is not form-parsed. Leave it alone.

4. Tests

• An oversized POST with an accurate Content-Length to a form route returns 413 and the handler is never reached (assert with a sentinel handler or an observable side effect, not just the status code).
• A request at or under the limit still succeeds through the normal CSRF path — proves the reorder did not break CSRF token handling.
• Cover at least one route from the /pages group and the POST /password route in the /user/{username} group, since the latter had no cap at all.

5. Docs

• TODO.md updated in the same commit as the code.
• README only if it documents request limits.

Definition of done

• Body cap is enforced before any form parsing on every form route.
• POST /password is covered.
• make check green, via the repo's own entrypoints only (make check / script/cibuild) — never raw go/golangci-lint.
• .golangci.yml untouched.
• Single commit, title ending in (closes #90).
• No attribution trailers.

## Implementation requirements Baseline: `main` @ `4f5ecb1`. ### Confirmed root cause `internal/server/routes.go`. chi runs `Use` middleware in registration order, and every group registers `CSRF()` **before** `MaxBodySize(maxFormBodySize)`: - `/pages` — `CSRF()` line 93, `MaxBodySize` line 95 - `/sources` — `CSRF()` line 118, `MaxBodySize` line 121 - `/source/{sourceID}` — `CSRF()` line 128, `MaxBodySize` line 131 gorilla/csrf calls `r.PostFormValue` (helpers.go:113), so the form is parsed under net/http's default 10 MB cap and the intended 1 MB limit never applies to form fields. Worse, and not noted in the original report: the `/user/{username}` group (`setupUserRoutes`, lines 107-116) has **no** `MaxBodySize` at all, and that is where `POST /password` from #83 lives. So the password-change endpoint currently has no middleware-level body cap whatsoever — only the handler-local `http.MaxBytesReader` in `profile.go:35`, which runs after CSRF has already parsed the body. ### 1. Ordering In every route group that accepts a POST, `MaxBodySize(maxFormBodySize)` must be registered before `CSRF()`. Groups to fix: `/pages`, `/sources`, `/source/{sourceID}`. Groups to fix **and** add the missing middleware to: `/user/{username}`. ### 2. Make 413 actually reachable Reordering alone does not produce a 413. `http.MaxBytesReader` surfaces its error on `Read`, so CSRF's form parse just fails and gorilla/csrf answers 403 "no token" — the operator sees a misleading CSRF error for what is really an oversized body. So `MaxBodySize` must also reject up front: if the request is a POST/PUT/PATCH and `r.ContentLength` is greater than the limit, write 413 and return without calling `next`. Keep installing `http.MaxBytesReader` as well, so a chunked or Content-Length-lying client is still hard-capped rather than buffered. Document that behaviour in the middleware doc comment: declared-oversize gets a clean 413, undeclared-oversize gets truncated at the cap and fails downstream. ### 3. Redundant handler-local limits These now sit behind a middleware cap and are dead weight: - `internal/handlers/auth.go:33` - `internal/handlers/profile.go:35` - `internal/handlers/source_management.go` lines 131, 390, 413, 729, 789, 812 Remove them and let the middleware be the single enforcement point — duplicated limits drift apart. If you keep any, the PR body must say which and why. `maxBodyShift` in `handlers.go` is still used by `webhook.go`, so do not delete the constant. **Out of scope:** the `/webhook/{uuid}` receiver. It reads through its own `io.LimitReader(r.Body, maxWebhookBodySize+1)` in `readWebhookBody`, is not CSRF-protected, and is not form-parsed. Leave it alone. ### 4. Tests - An oversized POST with an accurate `Content-Length` to a form route returns 413 and the handler is never reached (assert with a sentinel handler or an observable side effect, not just the status code). - A request at or under the limit still succeeds through the normal CSRF path — proves the reorder did not break CSRF token handling. - Cover at least one route from the `/pages` group and the `POST /password` route in the `/user/{username}` group, since the latter had no cap at all. ### 5. Docs - `TODO.md` updated in the **same commit** as the code. - README only if it documents request limits. ### Definition of done - Body cap is enforced before any form parsing on every form route. - `POST /password` is covered. - `make check` green, via the repo's own entrypoints only (`make check` / `script/cibuild`) — never raw `go`/`golangci-lint`. - `.golangci.yml` untouched. - Single commit, title ending in ` (closes #90)`. - No attribution trailers.

clawbot commented 2026-08-09 03:37:31 +02:00

Author

Collaborator

Copy Link

Implementation plan

Branch issue-90-body-limit-before-csrf off main @ 4f5ecb1, single commit titled ending in (closes #90).

1. internal/middleware/middleware.go — make MaxBodySize able to answer 413

Rewrite the middleware body so that, for POST/PUT/PATCH:

1. If r.ContentLength > maxBytes, log and write 413 Request Entity Too Large and return without calling next. This is the only way to get a real 413: http.MaxBytesReader surfaces its error on Read, at which point gorilla/csrf's r.PostFormValue has already swallowed it and answers 403 Forbidden - invalid CSRF token, which is a misleading diagnosis for an oversized body.
2. Still wrap r.Body in http.MaxBytesReader(w, r.Body, maxBytes) afterwards, so a chunked request (ContentLength == -1) or a client that lies about its Content-Length is hard-capped rather than buffered without bound.

The doc comment will state both paths explicitly: declared-oversize gets a clean 413 and never reaches downstream middleware or the handler; undeclared-oversize is truncated at the cap and fails downstream (form parse error / CSRF rejection), which is a correctness-preserving fallback rather than a clean diagnostic.

2. internal/server/routes.go — ordering per route group

MaxBodySize(maxFormBodySize) becomes the first Use in every group that accepts a POST, ahead of CSRF():

• /pages — move existing registration up
• /sources — move existing registration up
• /source/{sourceID} — move existing registration up
• /user/{username} — add it (currently has none at all, and POST /password from #83 lives here)

3. Remove the now-redundant handler-local caps

Delete the http.MaxBytesReader calls at internal/handlers/auth.go:33, internal/handlers/profile.go:35, and internal/handlers/source_management.go lines 130, 389, 412, 728, 788, 811. The middleware becomes the single enforcement point. I do not plan to keep any of them; if a compile constraint forces one to stay I will say so in the PR body. maxBodyShift in handlers.go stays — webhook.go still uses it for maxWebhookBodySize.

/webhook/{uuid} is untouched, per the out-of-scope note.

4. Tests

Unit tests in internal/middleware/middleware_test.go for MaxBodySize:

• declared-oversize POST returns 413 and a sentinel next handler records that it was never called
• at-limit and under-limit POSTs pass through and the body is fully readable by next
• oversized GET passes through untouched (the cap is POST/PUT/PATCH-scoped)
• chunked/undeclared oversize reaches next but the read fails at the cap

Route-level tests against the real router from routes.go (not a hand-rebuilt chain, so the test actually guards the registration order): a new internal/server/export_test.go exposes a helper that constructs a Server with a test Middleware/Handlers and returns the configured chi router, and a new internal/server/routes_test.go (external server_test package) builds the dependency graph with fxtest the same way internal/handlers/handlers_test.go does. Cases:

• POST /pages/login with an accurate oversized Content-Length returns 413, and no _gorilla_csrf cookie is set on the response — proving gorilla/csrf never ran, therefore the handler never ran
• POST /user/{username}/password with an authenticated session, a valid CSRF token harvested from the rendered profile page, and an oversized body returns 413 and the stored password hash is unchanged — an observable side effect proving the handler was not reached
• POST /pages/login under the limit with a CSRF token harvested from GET /pages/login still reaches the handler, proving the reorder did not break CSRF token handling
• POST /pages/login under the limit without a token still gets 403 from CSRF

5. Docs

• README.md does document request limits (the middleware list around line 872 and the Security bullet around line 894); both will be updated to include /user/* and the up-front 413 behaviour.
• TODO.md updated in the same commit per its Workflow section.

Verification

make fmt then make check only — no raw go/golangci-lint. .golangci.yml untouched.

## Implementation plan Branch `issue-90-body-limit-before-csrf` off `main` @ `4f5ecb1`, single commit titled ending in ` (closes #90)`. ### 1. `internal/middleware/middleware.go` — make `MaxBodySize` able to answer 413 Rewrite the middleware body so that, for `POST`/`PUT`/`PATCH`: 1. If `r.ContentLength > maxBytes`, log and write `413 Request Entity Too Large` and return **without** calling `next`. This is the only way to get a real 413: `http.MaxBytesReader` surfaces its error on `Read`, at which point gorilla/csrf's `r.PostFormValue` has already swallowed it and answers `403 Forbidden - invalid CSRF token`, which is a misleading diagnosis for an oversized body. 2. Still wrap `r.Body` in `http.MaxBytesReader(w, r.Body, maxBytes)` afterwards, so a chunked request (`ContentLength == -1`) or a client that lies about its `Content-Length` is hard-capped rather than buffered without bound. The doc comment will state both paths explicitly: declared-oversize gets a clean 413 and never reaches downstream middleware or the handler; undeclared-oversize is truncated at the cap and fails downstream (form parse error / CSRF rejection), which is a correctness-preserving fallback rather than a clean diagnostic. ### 2. `internal/server/routes.go` — ordering per route group `MaxBodySize(maxFormBodySize)` becomes the **first** `Use` in every group that accepts a POST, ahead of `CSRF()`: - `/pages` — move existing registration up - `/sources` — move existing registration up - `/source/{sourceID}` — move existing registration up - `/user/{username}` — **add** it (currently has none at all, and `POST /password` from #83 lives here) ### 3. Remove the now-redundant handler-local caps Delete the `http.MaxBytesReader` calls at `internal/handlers/auth.go:33`, `internal/handlers/profile.go:35`, and `internal/handlers/source_management.go` lines 130, 389, 412, 728, 788, 811. The middleware becomes the single enforcement point. I do not plan to keep any of them; if a compile constraint forces one to stay I will say so in the PR body. `maxBodyShift` in `handlers.go` stays — `webhook.go` still uses it for `maxWebhookBodySize`. `/webhook/{uuid}` is untouched, per the out-of-scope note. ### 4. Tests Unit tests in `internal/middleware/middleware_test.go` for `MaxBodySize`: - declared-oversize POST returns 413 and a sentinel `next` handler records that it was never called - at-limit and under-limit POSTs pass through and the body is fully readable by `next` - oversized `GET` passes through untouched (the cap is POST/PUT/PATCH-scoped) - chunked/undeclared oversize reaches `next` but the read fails at the cap Route-level tests against the **real** router from `routes.go` (not a hand-rebuilt chain, so the test actually guards the registration order): a new `internal/server/export_test.go` exposes a helper that constructs a `Server` with a test `Middleware`/`Handlers` and returns the configured `chi` router, and a new `internal/server/routes_test.go` (external `server_test` package) builds the dependency graph with `fxtest` the same way `internal/handlers/handlers_test.go` does. Cases: - `POST /pages/login` with an accurate oversized `Content-Length` returns 413, and no `_gorilla_csrf` cookie is set on the response — proving gorilla/csrf never ran, therefore the handler never ran - `POST /user/{username}/password` with an authenticated session, a valid CSRF token harvested from the rendered profile page, and an oversized body returns 413 **and the stored password hash is unchanged** — an observable side effect proving the handler was not reached - `POST /pages/login` under the limit with a CSRF token harvested from `GET /pages/login` still reaches the handler, proving the reorder did not break CSRF token handling - `POST /pages/login` under the limit without a token still gets 403 from CSRF ### 5. Docs - `README.md` does document request limits (the middleware list around line 872 and the Security bullet around line 894); both will be updated to include `/user/*` and the up-front 413 behaviour. - `TODO.md` updated in the same commit per its Workflow section. ### Verification `make fmt` then `make check` only — no raw `go`/`golangci-lint`. `.golangci.yml` untouched.

clawbot referenced this issue from a commit 2026-08-09 03:53:38 +02:00

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

clawbot referenced a pull request that will close this issue 2026-08-09 03:54:18 +02:00

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

clawbot referenced this issue 2026-08-09 04:02:30 +02:00

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

clawbot referenced this issue 2026-08-09 04:03:32 +02:00

Follow-ups from the PR #91 review: body-limit docs and route-ordering test coverage #93

clawbot referenced this issue 2026-08-09 04:08:25 +02:00

Fail loudly on set-but-unparseable env config values (closes #80) #92

clawbot referenced this issue from a commit 2026-08-11 14:17:59 +02:00

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

clawbot closed this issue 2026-08-11 14:37:39 +02:00

clawbot referenced this issue from a commit 2026-08-11 14:37:41 +02:00

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

clawbot referenced this issue 2026-08-11 14:48:08 +02:00

Milestone 1.0.0: internet-facing readiness #111

clawbot referenced this issue 2026-08-12 12:28:38 +02:00

Milestone 1.0.0: internet-facing readiness #111

clawbot referenced this issue 2026-08-12 12:30:13 +02:00

Release-readiness accuracy: docs contradict the code, TODO.md omits landed units, debug logging in the production asset #141

clawbot referenced this issue from a commit 2026-08-12 12:42:53 +02:00

Correct release-blocking documentation inaccuracies (closes #141)

clawbot referenced this issue 2026-08-12 12:43:08 +02:00

Correct release-blocking documentation inaccuracies (closes #141) #144

clawbot referenced this issue from a commit 2026-08-12 13:00:45 +02:00

Correct release-blocking documentation inaccuracies (closes #141)

clawbot referenced this issue from a commit 2026-08-12 13:05:33 +02:00

Correct release-blocking documentation inaccuracies (closes #141)

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

clawbot/docs-uuid-is-the-secret

next

feat/receiver-rate-limit

issue-70-log-target

No results found.

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#90