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

New Issue

clawbot · 2026-08-09T04:03:32+02:00

clawbot commented

2026-08-09 04:03:32 +02:00

Tracking issue for the five non-blocking nits raised in the independent review of PR #91 (#90), so they do not become untracked. None of these blocked that merge; they are recorded here rather than round-tripping the PR.

Items

Stale doc comment. internal/middleware/middleware_test.go (~line 435): the doc comment says maxBodySizeHandler, but the declaration it sits above is type maxBodySizeResult struct, and no maxBodySizeHandler identifier exists anywhere. The prose actually describes runMaxBodySize, declared below it. Fix the comment to name the thing it documents.
Inaccurate README sentence. README.md (~line 875) says the body cap runs "before any other middleware or handler runs". That is wrong: it is registered per route group, so the eight global middlewares (Recoverer, RequestID, SecurityHeaders, Logging, optional Metrics, CORS, Timeout, optional Sentry) all run first. The middleware's own doc comment in internal/middleware/middleware.go is accurate — align the README to it. The claim that matters and is true is that the cap runs before CSRF and therefore before any form parsing.
Ordering is test-guarded on only two of four groups. PR #91 added route-level ordering tests for /pages and POST /password under /user/{username} (exactly what the spec asked for). /sources and /source/{sourceID} have the correct order but nothing pins it, so a future reorder there would regress silently. Extend the table in internal/server/routes_test.go to cover a POST route in each of the remaining two groups.
Cap is POST/PUT/PATCH-only. MaxBodySize only wraps those three methods, and after #91 removed the handler-local readers it is the single enforcement point. A GET or DELETE carrying a large body on a form route is therefore uncapped. Not exploitable today (no form handler reads a body on those methods) and not a regression, but the method restriction should be stated in the middleware doc comment so the limitation is deliberate rather than incidental.
NewRouterForTest hand-builds the server. internal/server/export_test.go:27-32 constructs &Server{...} field by field, so any field added to Server later silently defaults to zero in these tests, and route tests could drift from the real wiring they exist to guard. Consider routing it through the real constructor, or add a comment explaining why the hand-built struct is sufficient.

Definition of done

Items 1 and 2 corrected.
Item 3: ordering assertions cover all four form route groups.
Item 4: method scope documented in the MaxBodySize doc comment.
Item 5: either fixed or a deliberate comment explaining the tradeoff.
make check green via the repo's own entrypoints; .golangci.yml untouched.

Tracking issue for the five non-blocking nits raised in the independent review of PR #91 (#90), so they do not become untracked. None of these blocked that merge; they are recorded here rather than round-tripping the PR. ## Items 1. **Stale doc comment.** `internal/middleware/middleware_test.go` (~line 435): the doc comment says `maxBodySizeHandler`, but the declaration it sits above is `type maxBodySizeResult struct`, and no `maxBodySizeHandler` identifier exists anywhere. The prose actually describes `runMaxBodySize`, declared below it. Fix the comment to name the thing it documents. 2. **Inaccurate README sentence.** `README.md` (~line 875) says the body cap runs "before any other middleware or handler runs". That is wrong: it is registered per route group, so the eight global middlewares (`Recoverer`, `RequestID`, `SecurityHeaders`, `Logging`, optional `Metrics`, `CORS`, `Timeout`, optional Sentry) all run first. The middleware's own doc comment in `internal/middleware/middleware.go` is accurate — align the README to it. The claim that matters and is true is that the cap runs before CSRF and therefore before any form parsing. 3. **Ordering is test-guarded on only two of four groups.** PR #91 added route-level ordering tests for `/pages` and `POST /password` under `/user/{username}` (exactly what the spec asked for). `/sources` and `/source/{sourceID}` have the correct order but nothing pins it, so a future reorder there would regress silently. Extend the table in `internal/server/routes_test.go` to cover a POST route in each of the remaining two groups. 4. **Cap is POST/PUT/PATCH-only.** `MaxBodySize` only wraps those three methods, and after #91 removed the handler-local readers it is the single enforcement point. A GET or DELETE carrying a large body on a form route is therefore uncapped. Not exploitable today (no form handler reads a body on those methods) and not a regression, but the method restriction should be stated in the middleware doc comment so the limitation is deliberate rather than incidental. 5. **`NewRouterForTest` hand-builds the server.** `internal/server/export_test.go:27-32` constructs `&Server{...}` field by field, so any field added to `Server` later silently defaults to zero in these tests, and route tests could drift from the real wiring they exist to guard. Consider routing it through the real constructor, or add a comment explaining why the hand-built struct is sufficient. ## Definition of done - Items 1 and 2 corrected. - Item 3: ordering assertions cover all four form route groups. - Item 4: method scope documented in the `MaxBodySize` doc comment. - Item 5: either fixed or a deliberate comment explaining the tradeoff. - `make check` green via the repo's own entrypoints; `.golangci.yml` untouched.

clawbot referenced this issue

2026-08-09 04:03:46 +02:00

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#93