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.
Plan. The items are from August, before the routes became /hooks and /hook/ID; check each against next first, fix only what is still true, and say in the PR which were already settled.
The middleware test comment names the thing it documents.
The README says the body cap runs per route group after the global middleware, and before CSRF and so before any form parsing.
The route-ordering table in internal/server/routes_test.go covers a POST route in every page route group that has one.
The MaxBodySize doc comment says it caps only POST, PUT and PATCH.
NewRouterForTest builds the server through the real constructor, or a comment says why the hand-built one is enough.
Model: opus-5-5
Plan. The items are from August, before the routes became `/hooks` and `/hook/ID`; check each against `next` first, fix only what is still true, and say in the PR which were already settled.
1. The middleware test comment names the thing it documents.
2. The README says the body cap runs per route group after the global middleware, and before CSRF and so before any form parsing.
3. The route-ordering table in `internal/server/routes_test.go` covers a POST route in every page route group that has one.
4. The `MaxBodySize` doc comment says it caps only POST, PUT and PATCH.
5. `NewRouterForTest` builds the server through the real constructor, or a comment says why the hand-built one is enough.
Model: opus-5-5
Item 3: one route test now requires an oversized POST, with no session and no CSRF token, to be refused with 413 before CSRF runs in every page route group with a POST route, the two webhook groups included.
Items 1, 4 and 5: the middleware test comment names runMaxBodySize, the MaxBodySize doc comment says other methods pass uncapped on purpose, and NewRouterForTest says why its hand-built Server is enough.
Item 2 was already settled on next: the README places the cap correctly.
Judgement call: item 5 got the explanatory comment, not a switch to New.
Model: opus-5-5
Done in https://git.eeqj.de/sneak/webhooker/pulls/449.
- Item 3: one route test now requires an oversized POST, with no session and no CSRF token, to be refused with `413` before CSRF runs in every page route group with a POST route, the two webhook groups included.
- Items 1, 4 and 5: the middleware test comment names `runMaxBodySize`, the `MaxBodySize` doc comment says other methods pass uncapped on purpose, and `NewRouterForTest` says why its hand-built `Server` is enough.
- Item 2 was already settled on `next`: the README places the cap correctly.
Judgement call: item 5 got the explanatory comment, not a switch to `New`.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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 saysmaxBodySizeHandler, but the declaration it sits above istype maxBodySizeResult struct, and nomaxBodySizeHandleridentifier exists anywhere. The prose actually describesrunMaxBodySize, 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, optionalMetrics,CORS,Timeout, optional Sentry) all run first. The middleware's own doc comment ininternal/middleware/middleware.gois 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
/pagesandPOST /passwordunder/user/{username}(exactly what the spec asked for)./sourcesand/source/{sourceID}have the correct order but nothing pins it, so a future reorder there would regress silently. Extend the table ininternal/server/routes_test.goto cover a POST route in each of the remaining two groups.Cap is POST/PUT/PATCH-only.
MaxBodySizeonly 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.NewRouterForTesthand-builds the server.internal/server/export_test.go:27-32constructs&Server{...}field by field, so any field added toServerlater 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
MaxBodySizedoc comment.make checkgreen via the repo's own entrypoints;.golangci.ymluntouched.clawbot referenced this issue2026-08-17 22:44:00 +02:00
Plan. The items are from August, before the routes became
/hooksand/hook/ID; check each againstnextfirst, fix only what is still true, and say in the PR which were already settled.internal/server/routes_test.gocovers a POST route in every page route group that has one.MaxBodySizedoc comment says it caps only POST, PUT and PATCH.NewRouterForTestbuilds the server through the real constructor, or a comment says why the hand-built one is enough.Model: opus-5-5
Done in #449.
413before CSRF runs in every page route group with a POST route, the two webhook groups included.runMaxBodySize, theMaxBodySizedoc comment says other methods pass uncapped on purpose, andNewRouterForTestsays why its hand-builtServeris enough.next: the README places the cap correctly.Judgement call: item 5 got the explanatory comment, not a switch to
New.Model: opus-5-5