sneak/webhooker
1
0
Fork 0
Code Issues 24 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

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

Open
clawbot wants to merge 1 commits from issue-90-body-limit-before-csrf into main
pull from: issue-90-body-limit-before-csrf
merge into: sneak:main
Conversation 4 Commits 1 Files Changed 10 +661 -48
clawbot commented 2026-08-09 03:54:18 +02:00
Collaborator
Copy Link

Fixes the ordering bug from the #83 review: the 1 MB form body cap was registered after the CSRF middleware, so it never applied to form fields.

The bug

chi runs Use middleware in registration order. Every form route group in internal/server/routes.go registered CSRF() before MaxBodySize(maxFormBodySize). gorilla/csrf (v1.7.3, helpers.go:113) calls r.PostFormValue, which parses the body — so by the time MaxBodySize installed its reader, the form had already been parsed under net/http's default 10 MB cap. The intended 1 MB limit was dead code for form fields on every one of those routes.

Ordering fix, per route group

MaxBodySize(maxFormBodySize) is now the first Use in each group, ahead of CSRF():

Group Before After
/pages CSRF, NoCache, MaxBodySize MaxBodySize, CSRF, NoCache
/sources CSRF, NoCache, RequireAuth, MaxBodySize MaxBodySize, CSRF, NoCache, RequireAuth
/source/{sourceID} CSRF, NoCache, RequireAuth, MaxBodySize MaxBodySize, CSRF, NoCache, RequireAuth
/user/{username} CSRF, NoCache, RequireAuth — no cap at all MaxBodySize, CSRF, NoCache, RequireAuth

Each group carries a comment stating why the cap has to precede CSRF, so the order is not silently "tidied" back later.

The missing cap on /user/{username}

Worth calling out separately, because it is worse than the reported bug: setupUserRoutes had no MaxBodySize registration whatsoever. That is the group where POST /password from #83 lives, so the password-change endpoint had no middleware-level body cap at all — its only limit was the handler-local http.MaxBytesReader in profile.go:35, which ran after CSRF had already parsed the body and was therefore useless for form fields. This PR adds the middleware to that group.

Why MaxBytesReader alone could not produce a 413

Reordering by itself does not give you a 413. http.MaxBytesReader does not reject anything at wrap time — it reports the overflow as an error from Read. With the reader correctly installed ahead of CSRF, an oversized body makes gorilla/csrf's form parse fail, and gorilla/csrf converts that into its own 403 Forbidden - invalid CSRF token. The operator would see a bogus CSRF error for what is really an oversized request.

So MaxBodySize now also checks up front. For POST/PUT/PATCH, if r.ContentLength exceeds the limit it logs, writes 413 Request Entity Too Large, and returns without calling next — neither CSRF nor the endpoint handler runs. http.MaxBytesReader is still installed afterwards, so the two paths are:

  • Declared oversize — clean 413, nothing downstream executes.
  • Undeclared oversize (chunked, ContentLength == -1, or a client lying about its length) — nothing to check up front, so the reader hard-caps the body at the limit and the request fails downstream at form-parse time. Less precise as a diagnostic, but the body is still never buffered past the cap, which is the property that matters.

Both paths are spelled out in the middleware's doc comment.

Handler-local MaxBytesReader calls removed

All of them, so there is a single enforcement point and no duplicated limits to drift apart. Each site keeps a one-line comment pointing at the middleware:

  • internal/handlers/auth.go (login submit)
  • internal/handlers/profile.go (password change)
  • internal/handlers/source_management.go — six sites: source create submit, source edit submit, applyWebhookEdit, entrypoint create, target create submit, processTargetCreate

None were kept. The maxBodyShift constant in handlers.go stays — webhook.go still uses it for maxWebhookBodySize.

Out of scope, untouched: the /webhook/{uuid} receiver. It bounds itself via io.LimitReader(r.Body, maxWebhookBodySize+1) in readWebhookBody, is not CSRF-protected, and is not form-parsed.

Tests

Middleware unit tests (internal/middleware/middleware_test.go), all against a sentinel next handler that records whether it ran and how many bytes it read:

  • declared-oversize POST returns 413 and the sentinel is never called
  • at-limit and under-limit POSTs pass through with the body fully readable
  • an oversized GET is untouched (the cap is POST/PUT/PATCH-scoped)
  • an undeclared-oversize body (ContentLength == -1) reaches the sentinel but the read errors at exactly the cap — the fallback path, pinned so it cannot silently become unbounded

Route-level tests (internal/server/routes_test.go, new) run against the real router produced by SetupRoutes, not a hand-rebuilt chain, so they guard the registration order itself. A new internal/server/export_test.go exposes a helper that builds a Server with a test Middleware/Handlers and returns the configured router; the dependency graph is wired with fxtest the same way internal/handlers/handlers_test.go does.

  • POST /pages/login with an accurate oversized Content-Length returns 413 and no _gorilla_csrf cookie is set — gorilla/csrf issues its cookie whenever it runs, so the cookie's absence is positive evidence that CSRF, and therefore the handler, never executed.
  • POST /pages/login under the limit without a token returns 403 and does set the _gorilla_csrf cookie. This is the control for the test above: without it, the missing-cookie assertion would prove nothing.
  • POST /pages/login under the limit with a token harvested from the rendered login form still reaches the handler (401 plus "Invalid username or password"), proving the reorder did not break CSRF token handling.
  • POST /user/{username}/password with a valid session, a valid CSRF token from the rendered profile page, and an oversized body returns 413 and the stored password hash is unchanged — an observable side effect, not just a status code, proving the handler was not reached on the route that previously had no cap.
  • POST /user/{username}/password under the limit still succeeds and changes the hash, proving the newly added middleware did not break the route it guards.

I verified the route-level tests actually bite: temporarily restoring the old CSRF-before-MaxBodySize order in the /pages group makes TestPagesLogin_OversizeBody_RejectedBeforeCSRF fail on both assertions (403 instead of 413, and a CSRF cookie present), while the two under-limit tests keep passing.

One test-only wrinkle worth noting for the reviewer: html/template escapes + as + in attribute values and gorilla/csrf tokens are standard base64, so a token scraped out of the rendered markup must be run through html.UnescapeString before it is submitted. Without that the valid-token cases fail with "CSRF token invalid".

Docs

  • README.md: the middleware section now lists /user/* among the capped groups and explains the ordering requirement and the two enforcement paths; the Security bullet notes the cap runs before CSRF parses the form.
  • TODO.md: updated in the same commit as the code. The entry went to the top of Completed Steps; the Next Step (event retention cleanup) is unrelated to this issue and was left in place.

Verification

  • make fmt run; make fmt-check clean.
  • make test green across all packages.
  • script/cibuild (docker build .) green end to end — this is the authoritative gate, since it runs the Dockerfile lint stage with the hash-pinned golangci-lint v2.12.2 plus make test and make build.
  • Host make lint reports one finding, G704 (gosec) at internal/delivery/client_ssrf_test.go:78, in a file this PR does not touch. I confirmed it is pre-existing by stashing the branch and re-running lint on the clean baseline — identical single finding. The host linter is newer than the pinned CI one; the pinned linter in script/cibuild does not report it.
  • .golangci.yml untouched (sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb), golangci-lint pin in the Dockerfile untouched.
  • Single commit.
Fixes the ordering bug from the #83 review: the 1 MB form body cap was registered *after* the CSRF middleware, so it never applied to form fields. ## The bug chi runs `Use` middleware in registration order. Every form route group in `internal/server/routes.go` registered `CSRF()` before `MaxBodySize(maxFormBodySize)`. gorilla/csrf (v1.7.3, `helpers.go:113`) calls `r.PostFormValue`, which parses the body — so by the time `MaxBodySize` installed its reader, the form had already been parsed under net/http's default 10 MB cap. The intended 1 MB limit was dead code for form fields on every one of those routes. ## Ordering fix, per route group `MaxBodySize(maxFormBodySize)` is now the first `Use` in each group, ahead of `CSRF()`: | Group | Before | After | | --- | --- | --- | | `/pages` | `CSRF`, `NoCache`, `MaxBodySize` | `MaxBodySize`, `CSRF`, `NoCache` | | `/sources` | `CSRF`, `NoCache`, `RequireAuth`, `MaxBodySize` | `MaxBodySize`, `CSRF`, `NoCache`, `RequireAuth` | | `/source/{sourceID}` | `CSRF`, `NoCache`, `RequireAuth`, `MaxBodySize` | `MaxBodySize`, `CSRF`, `NoCache`, `RequireAuth` | | `/user/{username}` | `CSRF`, `NoCache`, `RequireAuth` — **no cap at all** | `MaxBodySize`, `CSRF`, `NoCache`, `RequireAuth` | Each group carries a comment stating why the cap has to precede CSRF, so the order is not silently "tidied" back later. ## The missing cap on `/user/{username}` Worth calling out separately, because it is worse than the reported bug: `setupUserRoutes` had **no** `MaxBodySize` registration whatsoever. That is the group where `POST /password` from #83 lives, so the password-change endpoint had no middleware-level body cap at all — its only limit was the handler-local `http.MaxBytesReader` in `profile.go:35`, which ran after CSRF had already parsed the body and was therefore useless for form fields. This PR adds the middleware to that group. ## Why `MaxBytesReader` alone could not produce a 413 Reordering by itself does not give you a 413. `http.MaxBytesReader` does not reject anything at wrap time — it reports the overflow as an error from `Read`. With the reader correctly installed ahead of CSRF, an oversized body makes gorilla/csrf's form parse fail, and gorilla/csrf converts that into its own `403 Forbidden - invalid CSRF token`. The operator would see a bogus CSRF error for what is really an oversized request. So `MaxBodySize` now also checks up front. For `POST`/`PUT`/`PATCH`, if `r.ContentLength` exceeds the limit it logs, writes `413 Request Entity Too Large`, and returns **without** calling `next` — neither CSRF nor the endpoint handler runs. `http.MaxBytesReader` is still installed afterwards, so the two paths are: - **Declared oversize** — clean 413, nothing downstream executes. - **Undeclared oversize** (chunked, `ContentLength == -1`, or a client lying about its length) — nothing to check up front, so the reader hard-caps the body at the limit and the request fails downstream at form-parse time. Less precise as a diagnostic, but the body is still never buffered past the cap, which is the property that matters. Both paths are spelled out in the middleware's doc comment. ## Handler-local `MaxBytesReader` calls removed All of them, so there is a single enforcement point and no duplicated limits to drift apart. Each site keeps a one-line comment pointing at the middleware: - `internal/handlers/auth.go` (login submit) - `internal/handlers/profile.go` (password change) - `internal/handlers/source_management.go` — six sites: source create submit, source edit submit, `applyWebhookEdit`, entrypoint create, target create submit, `processTargetCreate` **None were kept.** The `maxBodyShift` constant in `handlers.go` stays — `webhook.go` still uses it for `maxWebhookBodySize`. **Out of scope, untouched:** the `/webhook/{uuid}` receiver. It bounds itself via `io.LimitReader(r.Body, maxWebhookBodySize+1)` in `readWebhookBody`, is not CSRF-protected, and is not form-parsed. ## Tests **Middleware unit tests** (`internal/middleware/middleware_test.go`), all against a sentinel `next` handler that records whether it ran and how many bytes it read: - declared-oversize POST returns 413 and the sentinel is never called - at-limit and under-limit POSTs pass through with the body fully readable - an oversized `GET` is untouched (the cap is POST/PUT/PATCH-scoped) - an undeclared-oversize body (`ContentLength == -1`) reaches the sentinel but the read errors at exactly the cap — the fallback path, pinned so it cannot silently become unbounded **Route-level tests** (`internal/server/routes_test.go`, new) run against the **real** router produced by `SetupRoutes`, not a hand-rebuilt chain, so they guard the registration order itself. A new `internal/server/export_test.go` exposes a helper that builds a `Server` with a test `Middleware`/`Handlers` and returns the configured router; the dependency graph is wired with `fxtest` the same way `internal/handlers/handlers_test.go` does. - `POST /pages/login` with an accurate oversized `Content-Length` returns 413 **and no `_gorilla_csrf` cookie is set** — gorilla/csrf issues its cookie whenever it runs, so the cookie's absence is positive evidence that CSRF, and therefore the handler, never executed. - `POST /pages/login` under the limit without a token returns 403 **and does** set the `_gorilla_csrf` cookie. This is the control for the test above: without it, the missing-cookie assertion would prove nothing. - `POST /pages/login` under the limit with a token harvested from the rendered login form still reaches the handler (401 plus "Invalid username or password"), proving the reorder did not break CSRF token handling. - `POST /user/{username}/password` with a valid session, a valid CSRF token from the rendered profile page, and an oversized body returns 413 **and the stored password hash is unchanged** — an observable side effect, not just a status code, proving the handler was not reached on the route that previously had no cap. - `POST /user/{username}/password` under the limit still succeeds and changes the hash, proving the newly added middleware did not break the route it guards. I verified the route-level tests actually bite: temporarily restoring the old `CSRF`-before-`MaxBodySize` order in the `/pages` group makes `TestPagesLogin_OversizeBody_RejectedBeforeCSRF` fail on both assertions (403 instead of 413, and a CSRF cookie present), while the two under-limit tests keep passing. One test-only wrinkle worth noting for the reviewer: `html/template` escapes `+` as `+` in attribute values and gorilla/csrf tokens are standard base64, so a token scraped out of the rendered markup must be run through `html.UnescapeString` before it is submitted. Without that the valid-token cases fail with "CSRF token invalid". ## Docs - `README.md`: the middleware section now lists `/user/*` among the capped groups and explains the ordering requirement and the two enforcement paths; the Security bullet notes the cap runs before CSRF parses the form. - `TODO.md`: updated in the same commit as the code. The entry went to the top of Completed Steps; the Next Step (event retention cleanup) is unrelated to this issue and was left in place. ## Verification - `make fmt` run; `make fmt-check` clean. - `make test` green across all packages. - `script/cibuild` (`docker build .`) green end to end — this is the authoritative gate, since it runs the Dockerfile lint stage with the hash-pinned golangci-lint v2.12.2 plus `make test` and `make build`. - Host `make lint` reports one finding, `G704` (gosec) at `internal/delivery/client_ssrf_test.go:78`, in a file this PR does not touch. I confirmed it is pre-existing by stashing the branch and re-running lint on the clean baseline — identical single finding. The host linter is newer than the pinned CI one; the pinned linter in `script/cibuild` does not report it. - `.golangci.yml` untouched (sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`), golangci-lint pin in the `Dockerfile` untouched. - Single commit.
clawbot added 1 commit 2026-08-09 03:54:19 +02:00
Enforce the body size limit before CSRF parses the form (closes #90)
All checks were successful
check / check (push) Successful in 3m6s
Details
08c9c1a5d8 
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.
clawbot added the needs-review label 2026-08-09 03:54:28 +02:00
clawbot self-assigned this 2026-08-09 03:54:33 +02:00
clawbot commented 2026-08-09 03:54:53 +02:00
Author
Collaborator
Copy Link

What I built

One commit, 08c9c1a, on issue-90-body-limit-before-csrf off main @ 4f5ecb1.

Ordering. MaxBodySize(maxFormBodySize) is now registered ahead of CSRF() in all four form route groups — /pages, /sources, /source/{sourceID}, and /user/{username}. The last of those had no cap registered at all, which is why POST /password was completely unbounded at the middleware layer. Each group has a comment explaining the constraint.

Real 413s. MaxBodySize now rejects a POST/PUT/PATCH whose r.ContentLength exceeds the limit before calling next. Reordering alone could not do this: http.MaxBytesReader only errors on Read, so the oversized body would surface as gorilla/csrf's 403 "no token" instead. MaxBytesReader is still installed afterwards so chunked and length-lying clients remain hard-capped, and the doc comment documents both paths.

Cleanup. All eight handler-local http.MaxBytesReader calls removed (auth.go, profile.go, six in source_management.go) — none kept. maxBodyShift stays because webhook.go needs it. The /webhook/{uuid} receiver is untouched per the out-of-scope note.

How I verified it

  • script/cibuild green end to end. That is the real gate: the Dockerfile lint stage runs the hash-pinned golangci-lint v2.12.2, then make test and make build.
  • make fmt run, make fmt-check clean, make test green in all nine packages.
  • Host make lint shows one G704 gosec finding at internal/delivery/client_ssrf_test.go:78, a file this PR does not touch. I stashed the branch and re-ran lint against the clean baseline: identical single finding, so it is pre-existing host-linter noise, and the pinned CI linter does not report it.
  • .golangci.yml byte-identical (sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb).

Mutation check. I did not just assert that the new tests pass — I confirmed they fail on the bug. Temporarily restoring the old CSRF-before-MaxBodySize order in /pages makes TestPagesLogin_OversizeBody_RejectedBeforeCSRF fail on both of its assertions (403 rather than 413, and a _gorilla_csrf cookie present), while the under-limit tests continue to pass. Then I reverted the mutation.

The route-level tests run against the actual router from SetupRoutes rather than a hand-rebuilt middleware chain, so they guard the registration order itself and cannot drift from routes.go. The "handler never reached" claim is backed by observable side effects, not just status codes: the absence of a _gorilla_csrf cookie on the login route (with an under-limit control test proving the cookie does appear when CSRF runs), and an unchanged stored password hash on POST /password.

One thing a reviewer may trip over if they extend these tests: html/template escapes + as + in attribute values, and gorilla/csrf tokens are standard base64, so a token scraped from rendered markup has to go through html.UnescapeString before submission. Without it the valid-token cases fail with "CSRF token invalid" — that cost me a debugging cycle.

## What I built One commit, `08c9c1a`, on `issue-90-body-limit-before-csrf` off `main` @ `4f5ecb1`. **Ordering.** `MaxBodySize(maxFormBodySize)` is now registered ahead of `CSRF()` in all four form route groups — `/pages`, `/sources`, `/source/{sourceID}`, and `/user/{username}`. The last of those had no cap registered at all, which is why `POST /password` was completely unbounded at the middleware layer. Each group has a comment explaining the constraint. **Real 413s.** `MaxBodySize` now rejects a `POST`/`PUT`/`PATCH` whose `r.ContentLength` exceeds the limit before calling `next`. Reordering alone could not do this: `http.MaxBytesReader` only errors on `Read`, so the oversized body would surface as gorilla/csrf's 403 "no token" instead. `MaxBytesReader` is still installed afterwards so chunked and length-lying clients remain hard-capped, and the doc comment documents both paths. **Cleanup.** All eight handler-local `http.MaxBytesReader` calls removed (`auth.go`, `profile.go`, six in `source_management.go`) — none kept. `maxBodyShift` stays because `webhook.go` needs it. The `/webhook/{uuid}` receiver is untouched per the out-of-scope note. ## How I verified it - `script/cibuild` green end to end. That is the real gate: the Dockerfile lint stage runs the hash-pinned golangci-lint v2.12.2, then `make test` and `make build`. - `make fmt` run, `make fmt-check` clean, `make test` green in all nine packages. - Host `make lint` shows one `G704` gosec finding at `internal/delivery/client_ssrf_test.go:78`, a file this PR does not touch. I stashed the branch and re-ran lint against the clean baseline: identical single finding, so it is pre-existing host-linter noise, and the pinned CI linter does not report it. - `.golangci.yml` byte-identical (sha256 `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`). **Mutation check.** I did not just assert that the new tests pass — I confirmed they fail on the bug. Temporarily restoring the old `CSRF`-before-`MaxBodySize` order in `/pages` makes `TestPagesLogin_OversizeBody_RejectedBeforeCSRF` fail on both of its assertions (403 rather than 413, and a `_gorilla_csrf` cookie present), while the under-limit tests continue to pass. Then I reverted the mutation. The route-level tests run against the actual router from `SetupRoutes` rather than a hand-rebuilt middleware chain, so they guard the registration order itself and cannot drift from `routes.go`. The "handler never reached" claim is backed by observable side effects, not just status codes: the absence of a `_gorilla_csrf` cookie on the login route (with an under-limit control test proving the cookie does appear when CSRF runs), and an unchanged stored password hash on `POST /password`. One thing a reviewer may trip over if they extend these tests: `html/template` escapes `+` as `+` in attribute values, and gorilla/csrf tokens are standard base64, so a token scraped from rendered markup has to go through `html.UnescapeString` before submission. Without it the valid-token cases fail with "CSRF token invalid" — that cost me a debugging cycle.
clawbot commented 2026-08-09 04:02:30 +02:00
Author
Collaborator
Copy Link

Review: PR #91 — verdict PASS

Reviewed at head 08c9c1a against base 4f5ecb1 (still the tip of
main, so this merges cleanly with no rebase needed).

No blocking findings. Five non-blocking nits below.

Spec conformance (issue #90 "Implementation requirements")

1. Ordering — verified by reading internal/server/routes.go and by
execution. MaxBodySize(maxFormBodySize) is the first Use in all
four groups: /pages (line 95), /user/{username} (113), /sources
(128), /source/{sourceID} (140). I enumerated every POST-accepting
route in the file to check for bypasses:

  • The /pages login sub-group (r.Group at line 99, adding
    LoginRateLimit) inherits the parent group's Use stack, so
    MaxBodySize still runs first for POST /pages/login.
  • POST /pages/logout (105) sits directly on the parent group. Capped.
  • r.With(s.mw.PasswordChangeRateLimit()).Post("/password", ...)
    (118-120) — With appends to the group stack rather than replacing
    it, so the cap still precedes CSRF. Confirmed by execution, see
    mutation testing below.
  • The ten r.Post registrations in /source/{sourceID} (146-169) are
    all on the capped group.
  • The only POST-reachable route outside the four groups is
    /webhook/{uuid} (HandleFunc, 174), which is out of scope.

2. The 413 path — verified by reading
internal/middleware/middleware.go:318-354. r.ContentLength > maxBytes is an int64 > int64 comparison, so the unknown-length
sentinel -1 falls through correctly rather than tripping the
rejection, and a body exactly at the limit is admitted (matching
TestMaxBodySize_AtLimit_PassesThrough). The unknown-length case is
still hard-capped: http.MaxBytesReader(w, r.Body, maxBytes) is
installed on line 349 on the fall-through path, and
TestMaxBodySize_UndeclaredOversize_TruncatedAtCap pins that the
handler sees exactly maxBytes and gets a read error, not an
unbounded buffer. No sign or width mistakes.

3. Handler-local removals — verified by reading; each traced to a
covering group. All eight sites are gone and none is reachable from
an uncapped route: auth.go HandleLoginSubmit → POST /pages/login;
profile.go HandlePasswordChange → POST /user/{username}/password;
source_management.go HandleSourceCreateSubmit → POST /sources/new,
HandleSourceEditSubmit → POST /source/{sourceID}/edit,
HandleEntrypointCreate → POST /source/{sourceID}/entrypoints,
HandleTargetCreate → POST /source/{sourceID}/targets, plus
applyWebhookEdit and processTargetCreate, which are internal
helpers called only from the two submit handlers above (and were
already dead code, since the caller had run ParseForm before invoking
them). maxBodyShift is correctly retained in handlers.go:26 and
still consumed by webhook.go:17.

4. Tests — the "handler never reached" claim holds, verified by
execution. I did not take the PR body's mutation claim on trust; I
reproduced it in a throwaway worktree and reverted both mutations:

  • Removing r.Use(s.mw.MaxBodySize(...)) from setupUserRoutes
    (restoring main's state) fails
    TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged on
    both assertions — 413 vs. the actual response, and the stored
    Argon2id hash changes. So the side-effect assertion is genuinely
    load-bearing, not decorative. Worth noting because the obvious
    objection — that a password-hashing input length limit would leave
    the hash unchanged anyway and make the assertion vacuous — does not
    apply here: this repo uses Argon2id, which has no bcrypt-style
    72-byte input ceiling, so the oversized value really is persisted
    when the cap is absent.
  • Swapping /pages back to CSRF before MaxBodySize fails
    TestPagesLogin_OversizeBody_RejectedBeforeCSRF with expected: 413, actual: 403, while both under-limit tests keep passing.

The _gorilla_csrf-cookie-absence signal is properly controlled by
TestPagesLogin_UnderLimit_NoToken_CSRFRejects, which asserts the
cookie is issued on a 403 — without that control the absence
assertion would prove nothing. Running against the real router from
SetupRoutes rather than a hand-rebuilt chain is the right call; it is
what makes the ordering itself testable.

5. /webhook/{uuid} — verified by reading. Untouched by the diff.
readWebhookBody (internal/handlers/webhook.go:135-164) still reads
through io.LimitReader(r.Body, maxWebhookBodySize+1) and returns 413
when the result exceeds the limit. Correct out-of-scope handling.

Repo policy

  • .golangci.yml unmodified — sha256 on the head tree is
    021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb,
    and git diff origin/main..HEAD -- .golangci.yml Dockerfile script/bootstrap is empty, so the golangci-lint v2.12.2 pin is
    intact.
  • TODO.md updated in the same commit as the code. The Next Step
    (event retention cleanup) was correctly left in place, since this
    work was issue-driven rather than the Next Step.
  • Single commit; title Enforce the body size limit before CSRF parses the form (closes #90) ends with the required (closes #N). Body is
    wrapped and explains the why.
  • No attribution trailers; no AI/vendor references anywhere in the
    commit message or the tree.
  • No 4-byte characters in any changed file.
  • No non-inclusive terminology introduced.
  • No scope creep: every hunk maps to a numbered spec item.

Verification runs

  • script/cibuild — exit 0 (executed).
  • CI on 08c9c1a — check / check (push) success in 3m6s. It was
    still pending when I started; it has since gone green.
  • make check on the head tree — all packages pass, including the five
    new internal/server route tests and the five new
    internal/middleware MaxBodySize tests. It exits non-zero on a
    single G704 (gosec) finding at
    internal/delivery/client_ssrf_test.go:78. I did not take the PR
    body's word for this being pre-existing: I ran make lint in a
    separate worktree on a clean origin/main and got the identical
    single finding. It is host-linter-version skew (the host golangci is
    newer than the pinned CI v2.12.2), untouched by this PR, and not
    attributable to it.
  • make fmt-check — clean. make check left the tree unmodified
    (git status --porcelain empty).

Non-blocking nits

  1. internal/middleware/middleware_test.go:435 — the doc comment reads
    "maxBodySizeHandler wraps a sentinel handler..." but the
    declaration it sits on is type maxBodySizeResult struct. There is
    no maxBodySizeHandler identifier anywhere; the comment appears to
    describe runMaxBodySize, which is the function immediately below
    and is itself undocumented. Go doc comments must begin with the name
    of the thing they document. Acceptable: retitle the comment to
    maxBodySizeResult records what the sentinel handler observed...
    and move the wrapping description onto runMaxBodySize.

  2. README.md (the new MaxBodySize paragraph, ~line 875) — "is
    answered with 413 Request Entity Too Large before any other
    middleware or handler runs" is inaccurate as written. It sits
    immediately after the enumerated list of eight global middlewares
    (Recoverer, RequestID, SecurityHeaders, Logging, Metrics, CORS,
    Timeout, Sentry), all of which do run before it. The middleware's own
    doc comment gets this right ("neither CSRF nor the endpoint handler
    runs"). Acceptable: scope the README claim the same way — "before any
    other middleware in the route group, and before the handler".

  3. Route-level ordering is only test-guarded for /pages and
    /user/{username}. That satisfies the spec, which asked for exactly
    those two, but /sources and /source/{sourceID} are guarded by
    reading alone and can silently regress if someone reorders the Use
    calls. Acceptable: a table-driven case over all four groups, each
    asserting 413 plus no _gorilla_csrf cookie, which would cost only a
    few lines given the helpers already present in routes_test.go.

  4. MaxBodySize is scoped to POST/PUT/PATCH, and
    TestMaxBodySize_GetWithOversizeBody_NotCapped now pins that as
    deliberate. Since the handler-local readers are gone, the middleware
    is the single enforcement point, so a GET or DELETE carrying a large
    body to a form route has no cap at all. Not exploitable today — no
    handler on those routes reads r.Body on a non-POST method — and not
    a regression from main, where the handler-local readers were also
    only on POST paths. Acceptable: one sentence in the MaxBodySize doc
    comment recording that non-body methods are intentionally
    unrestricted, so the gap is a decision rather than an oversight.

  5. internal/server/export_test.go:27-32 — NewRouterForTest builds
    &Server{...} field by field instead of going through the
    constructor. If SetupRoutes later reads a Server field that this
    literal does not set (sentryEnabled is already in that category),
    tests will silently exercise the zero value. The tradeoff is
    documented in the function comment and is reasonable for avoiding the
    fx lifecycle; flagging only so it is a known cost.

## Review: PR #91 — verdict PASS Reviewed at head `08c9c1a` against base `4f5ecb1` (still the tip of `main`, so this merges cleanly with no rebase needed). No blocking findings. Five non-blocking nits below. ### Spec conformance (issue #90 "Implementation requirements") **1. Ordering — verified by reading `internal/server/routes.go` and by execution.** `MaxBodySize(maxFormBodySize)` is the first `Use` in all four groups: `/pages` (line 95), `/user/{username}` (113), `/sources` (128), `/source/{sourceID}` (140). I enumerated every POST-accepting route in the file to check for bypasses: - The `/pages` login sub-group (`r.Group` at line 99, adding `LoginRateLimit`) inherits the parent group's `Use` stack, so `MaxBodySize` still runs first for `POST /pages/login`. - `POST /pages/logout` (105) sits directly on the parent group. Capped. - `r.With(s.mw.PasswordChangeRateLimit()).Post("/password", ...)` (118-120) — `With` appends to the group stack rather than replacing it, so the cap still precedes CSRF. Confirmed by execution, see mutation testing below. - The ten `r.Post` registrations in `/source/{sourceID}` (146-169) are all on the capped group. - The only POST-reachable route outside the four groups is `/webhook/{uuid}` (`HandleFunc`, 174), which is out of scope. **2. The 413 path — verified by reading `internal/middleware/middleware.go:318-354`.** `r.ContentLength > maxBytes` is an `int64 > int64` comparison, so the unknown-length sentinel `-1` falls through correctly rather than tripping the rejection, and a body exactly at the limit is admitted (matching `TestMaxBodySize_AtLimit_PassesThrough`). The unknown-length case is still hard-capped: `http.MaxBytesReader(w, r.Body, maxBytes)` is installed on line 349 on the fall-through path, and `TestMaxBodySize_UndeclaredOversize_TruncatedAtCap` pins that the handler sees exactly `maxBytes` and gets a read error, not an unbounded buffer. No sign or width mistakes. **3. Handler-local removals — verified by reading; each traced to a covering group.** All eight sites are gone and none is reachable from an uncapped route: `auth.go` `HandleLoginSubmit` → `POST /pages/login`; `profile.go` `HandlePasswordChange` → `POST /user/{username}/password`; `source_management.go` `HandleSourceCreateSubmit` → `POST /sources/new`, `HandleSourceEditSubmit` → `POST /source/{sourceID}/edit`, `HandleEntrypointCreate` → `POST /source/{sourceID}/entrypoints`, `HandleTargetCreate` → `POST /source/{sourceID}/targets`, plus `applyWebhookEdit` and `processTargetCreate`, which are internal helpers called only from the two submit handlers above (and were already dead code, since the caller had run `ParseForm` before invoking them). `maxBodyShift` is correctly retained in `handlers.go:26` and still consumed by `webhook.go:17`. **4. Tests — the "handler never reached" claim holds, verified by execution.** I did not take the PR body's mutation claim on trust; I reproduced it in a throwaway worktree and reverted both mutations: - Removing `r.Use(s.mw.MaxBodySize(...))` from `setupUserRoutes` (restoring `main`'s state) fails `TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged` on **both** assertions — 413 vs. the actual response, and the stored Argon2id hash changes. So the side-effect assertion is genuinely load-bearing, not decorative. Worth noting because the obvious objection — that a password-hashing input length limit would leave the hash unchanged anyway and make the assertion vacuous — does not apply here: this repo uses Argon2id, which has no bcrypt-style 72-byte input ceiling, so the oversized value really is persisted when the cap is absent. - Swapping `/pages` back to `CSRF` before `MaxBodySize` fails `TestPagesLogin_OversizeBody_RejectedBeforeCSRF` with `expected: 413, actual: 403`, while both under-limit tests keep passing. The `_gorilla_csrf`-cookie-absence signal is properly controlled by `TestPagesLogin_UnderLimit_NoToken_CSRFRejects`, which asserts the cookie *is* issued on a 403 — without that control the absence assertion would prove nothing. Running against the real router from `SetupRoutes` rather than a hand-rebuilt chain is the right call; it is what makes the ordering itself testable. **5. `/webhook/{uuid}` — verified by reading.** Untouched by the diff. `readWebhookBody` (`internal/handlers/webhook.go:135-164`) still reads through `io.LimitReader(r.Body, maxWebhookBodySize+1)` and returns 413 when the result exceeds the limit. Correct out-of-scope handling. ### Repo policy - `.golangci.yml` unmodified — sha256 on the head tree is `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, and `git diff origin/main..HEAD -- .golangci.yml Dockerfile script/bootstrap` is empty, so the golangci-lint v2.12.2 pin is intact. - `TODO.md` updated in the same commit as the code. The Next Step (event retention cleanup) was correctly left in place, since this work was issue-driven rather than the Next Step. - Single commit; title `Enforce the body size limit before CSRF parses the form (closes #90)` ends with the required ` (closes #N)`. Body is wrapped and explains the why. - No attribution trailers; no AI/vendor references anywhere in the commit message or the tree. - No 4-byte characters in any changed file. - No non-inclusive terminology introduced. - No scope creep: every hunk maps to a numbered spec item. ### Verification runs - `script/cibuild` — exit 0 (executed). - CI on `08c9c1a` — `check / check (push)` **success** in 3m6s. It was still pending when I started; it has since gone green. - `make check` on the head tree — all packages pass, including the five new `internal/server` route tests and the five new `internal/middleware` `MaxBodySize` tests. It exits non-zero on a single `G704` (gosec) finding at `internal/delivery/client_ssrf_test.go:78`. I did not take the PR body's word for this being pre-existing: I ran `make lint` in a separate worktree on a clean `origin/main` and got the identical single finding. It is host-linter-version skew (the host golangci is newer than the pinned CI v2.12.2), untouched by this PR, and not attributable to it. - `make fmt-check` — clean. `make check` left the tree unmodified (`git status --porcelain` empty). ### Non-blocking nits 1. `internal/middleware/middleware_test.go:435` — the doc comment reads "`maxBodySizeHandler` wraps a sentinel handler..." but the declaration it sits on is `type maxBodySizeResult struct`. There is no `maxBodySizeHandler` identifier anywhere; the comment appears to describe `runMaxBodySize`, which is the function immediately below and is itself undocumented. Go doc comments must begin with the name of the thing they document. Acceptable: retitle the comment to `maxBodySizeResult records what the sentinel handler observed...` and move the wrapping description onto `runMaxBodySize`. 2. `README.md` (the new MaxBodySize paragraph, ~line 875) — "is answered with `413 Request Entity Too Large` before any other middleware or handler runs" is inaccurate as written. It sits immediately after the enumerated list of eight global middlewares (Recoverer, RequestID, SecurityHeaders, Logging, Metrics, CORS, Timeout, Sentry), all of which do run before it. The middleware's own doc comment gets this right ("neither CSRF nor the endpoint handler runs"). Acceptable: scope the README claim the same way — "before any other middleware in the route group, and before the handler". 3. Route-level ordering is only test-guarded for `/pages` and `/user/{username}`. That satisfies the spec, which asked for exactly those two, but `/sources` and `/source/{sourceID}` are guarded by reading alone and can silently regress if someone reorders the `Use` calls. Acceptable: a table-driven case over all four groups, each asserting 413 plus no `_gorilla_csrf` cookie, which would cost only a few lines given the helpers already present in `routes_test.go`. 4. `MaxBodySize` is scoped to POST/PUT/PATCH, and `TestMaxBodySize_GetWithOversizeBody_NotCapped` now pins that as deliberate. Since the handler-local readers are gone, the middleware is the single enforcement point, so a GET or DELETE carrying a large body to a form route has no cap at all. Not exploitable today — no handler on those routes reads `r.Body` on a non-POST method — and not a regression from `main`, where the handler-local readers were also only on POST paths. Acceptable: one sentence in the `MaxBodySize` doc comment recording that non-body methods are intentionally unrestricted, so the gap is a decision rather than an oversight. 5. `internal/server/export_test.go:27-32` — `NewRouterForTest` builds `&Server{...}` field by field instead of going through the constructor. If `SetupRoutes` later reads a `Server` field that this literal does not set (`sentryEnabled` is already in that category), tests will silently exercise the zero value. The tradeoff is documented in the function comment and is reasonable for avoiding the fx lifecycle; flagging only so it is a known cost.
clawbot commented 2026-08-09 04:03:46 +02:00
Author
Collaborator
Copy Link

Manager note

Independent review verdict: PASS, no blocking findings. The reviewer did not author this change.

Two things I want to highlight because they raise my confidence beyond a read-only review:

  • The reviewer mutation-tested the central claim instead of trusting the PR body. Reverting the MaxBodySize registration in setupUserRoutes fails TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged on both assertions, and reverting the /pages ordering fails TestPagesLogin_OversizeBody_RejectedBeforeCSRF with expected: 413, actual: 403. So the tests genuinely pin the fix rather than passing incidentally.
  • The reviewer specifically chased down the obvious objection to the "handler never reached" assertion — that a password-hashing input-length ceiling could make the unchanged-hash check vacuous. It does not apply here: the repo uses Argon2id, not bcrypt, so the oversized value really is persisted once the cap is removed. That was the weakest point in the test design and it held up.

Also independently confirmed: script/cibuild exit 0, Gitea CI green on 08c9c1a, .golangci.yml byte-identical, base 4f5ecb1 still the tip of main so this merges without a rebase. The one gosec G704 finding a host make lint reports in internal/delivery/client_ssrf_test.go reproduces on a clean origin/main and is host-linter version skew, not this change.

The five non-blocking nits are now tracked as #93 rather than round-tripping this PR — they are two documentation inaccuracies, two coverage/scope observations, and one test-harness tradeoff, none of which affect the correctness of the fix.

Labeled merge-ready and assigned to @sneak for merge. Note for merge ordering: PR #92 (#80) also touches TODO.md, so whichever of the two lands second will need a trivial TODO.md rebase.

## Manager note Independent review verdict: **PASS**, no blocking findings. The reviewer did not author this change. Two things I want to highlight because they raise my confidence beyond a read-only review: - The reviewer **mutation-tested** the central claim instead of trusting the PR body. Reverting the `MaxBodySize` registration in `setupUserRoutes` fails `TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged` on both assertions, and reverting the `/pages` ordering fails `TestPagesLogin_OversizeBody_RejectedBeforeCSRF` with `expected: 413, actual: 403`. So the tests genuinely pin the fix rather than passing incidentally. - The reviewer specifically chased down the obvious objection to the "handler never reached" assertion — that a password-hashing input-length ceiling could make the unchanged-hash check vacuous. It does not apply here: the repo uses Argon2id, not bcrypt, so the oversized value really is persisted once the cap is removed. That was the weakest point in the test design and it held up. Also independently confirmed: `script/cibuild` exit 0, Gitea CI green on `08c9c1a`, `.golangci.yml` byte-identical, base `4f5ecb1` still the tip of `main` so this merges without a rebase. The one `gosec` G704 finding a host `make lint` reports in `internal/delivery/client_ssrf_test.go` reproduces on a clean `origin/main` and is host-linter version skew, not this change. The five non-blocking nits are now tracked as #93 rather than round-tripping this PR — they are two documentation inaccuracies, two coverage/scope observations, and one test-harness tradeoff, none of which affect the correctness of the fix. Labeled `merge-ready` and assigned to @sneak for merge. Note for merge ordering: PR #92 (#80) also touches `TODO.md`, so whichever of the two lands second will need a trivial `TODO.md` rebase.
clawbot added merge-ready and removed needs-review labels 2026-08-09 04:03:50 +02:00
clawbot removed their assignment 2026-08-09 04:03:54 +02:00
sneak was assigned by clawbot 2026-08-09 04:03:55 +02:00
clawbot commented 2026-08-09 07:37:52 +02:00
Author
Collaborator
Copy Link

Verification re-check: the green is real

A fleet-wide warning came in that script/cibuild can report a green it did not earn — it is a plain docker build . with no cache control, and the Dockerfile does COPY . . then RUN make check, so on an unchanged tree Docker serves the check layer from cache and the build exits 0 without running anything. Observed elsewhere as a SUCCESS in 0.262 seconds with every layer CACHED.

This PR cites script/cibuild exit 0 as its authoritative lint evidence, and in this repo that matters: the host golangci-lint (v2.10.1) disagrees with the pinned v2.12.2, so a cached layer would leave the pinned-linter result unproven.

Re-checked against Gitea CI, which builds each commit on a runner independently of any local Docker cache:

  • 08c9c1a — check / check (push): success in 3m6s (run 97)

A cached build finishes in under a second. Three minutes is a genuine execution inside the pinned v2.12.2 image. The claim stands. No re-label, no pull-back.

All five currently merge-ready PRs were re-checked the same way and all have genuine multi-minute CI runs: #87 2m37s, #91 3m6s, #92 6m3s, #96 2m43s, #100 3m3s.

## Verification re-check: the green is real A fleet-wide warning came in that `script/cibuild` can report a green it did not earn — it is a plain `docker build .` with no cache control, and the Dockerfile does `COPY . .` then `RUN make check`, so on an unchanged tree Docker serves the check layer from cache and the build exits 0 without running anything. Observed elsewhere as a SUCCESS in 0.262 seconds with every layer `CACHED`. This PR cites `script/cibuild` exit 0 as its authoritative lint evidence, and in this repo that matters: the host golangci-lint (v2.10.1) disagrees with the pinned v2.12.2, so a cached layer would leave the pinned-linter result unproven. **Re-checked against Gitea CI, which builds each commit on a runner independently of any local Docker cache:** - `08c9c1a` — `check / check (push)`: **success in 3m6s** (run 97) A cached build finishes in under a second. Three minutes is a genuine execution inside the pinned v2.12.2 image. **The claim stands.** No re-label, no pull-back. All five currently merge-ready PRs were re-checked the same way and all have genuine multi-minute CI runs: #87 2m37s, #91 3m6s, #92 6m3s, #96 2m43s, #100 3m3s.
All checks were successful
check / check (push) Successful in 3m6s
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue-90-body-limit-before-csrf:issue-90-body-limit-before-csrf
git checkout issue-90-body-limit-before-csrf
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#91
Delete Branch "issue-90-body-limit-before-csrf"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?