Found by the independent review of #156, which settled the question empirically rather than by reading. Deliberately NOT milestoned 1.0.0 — see below.
internal/server/routes.go attaches the static handler with Mount, which registers every method, and http.FileServer special-cases only HEAD (by omitting the body). TestStaticServesEveryMethod, added by that PR, pins the current behaviour: GET, HEAD, POST, PUT and DELETE on /s/js/app.js all return 200, with POST/PUT/DELETE returning the asset bytes.
Why it is not a 1.0 blocker: http.FileServer is strictly read-only, so there is no state change and no write path — a POST response is byte-identical to the GET. Nothing is exploitable.
Why it is still worth fixing:
It is a standard scanner and pentest finding, so it costs an explanation every time someone points a tool at the service.
Allow-header and method semantics are simply wrong for the route.
/s sits outside the four form route groups, so MaxBodySize never applies to it. An attacker can send an arbitrarily large body to an asset path. net/http caps the drain and the handler never reads it, so the exposure is a held connection rather than memory — but it is the one place where the body-size middleware's coverage has a hole.
Definition of done
The static handler is registered per-method for GET and HEAD instead of via Mount, and other methods get 405 with an accurate Allow: GET, HEAD.
TestStaticServesEveryMethod is INVERTED rather than deleted: it must assert 405 and the Allow header for POST/PUT/DELETE, and 200 for GET/HEAD. The test is what stops this drifting back, so it should survive the change with its meaning flipped.
The /s/* row in the README route table is updated — it currently reads any, which #156 made accurate on purpose. Both must change together or the README goes back to lying.
Confirm the body-size hole is closed or state plainly that it remains and why.
Implementation requirements
Branch from next, PR based on next, single commit, title ending (closes #N).
Run make bootstrap (or make assets) in a fresh clone before gating — third-party browser assets are fetched at build time and are not committed.
Gate on make check plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.
Found by the independent review of https://git.eeqj.de/sneak/webhooker/pulls/156, which settled the question empirically rather than by reading. Deliberately NOT milestoned 1.0.0 — see below.
`internal/server/routes.go` attaches the static handler with `Mount`, which registers every method, and `http.FileServer` special-cases only HEAD (by omitting the body). `TestStaticServesEveryMethod`, added by that PR, pins the current behaviour: `GET`, `HEAD`, `POST`, `PUT` and `DELETE` on `/s/js/app.js` all return 200, with POST/PUT/DELETE returning the asset bytes.
Why it is not a 1.0 blocker: `http.FileServer` is strictly read-only, so there is no state change and no write path — a POST response is byte-identical to the GET. Nothing is exploitable.
Why it is still worth fixing:
- It is a standard scanner and pentest finding, so it costs an explanation every time someone points a tool at the service.
- `Allow`-header and method semantics are simply wrong for the route.
- `/s` sits outside the four form route groups, so `MaxBodySize` never applies to it. An attacker can send an arbitrarily large body to an asset path. `net/http` caps the drain and the handler never reads it, so the exposure is a held connection rather than memory — but it is the one place where the body-size middleware's coverage has a hole.
## Definition of done
- The static handler is registered per-method for `GET` and `HEAD` instead of via `Mount`, and other methods get `405` with an accurate `Allow: GET, HEAD`.
- `TestStaticServesEveryMethod` is INVERTED rather than deleted: it must assert 405 and the `Allow` header for POST/PUT/DELETE, and 200 for GET/HEAD. The test is what stops this drifting back, so it should survive the change with its meaning flipped.
- The `/s/*` row in the README route table is updated — it currently reads `any`, which https://git.eeqj.de/sneak/webhooker/pulls/156 made accurate on purpose. Both must change together or the README goes back to lying.
- Confirm the body-size hole is closed or state plainly that it remains and why.
## Implementation requirements
- Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`.
- Do not modify `TODO.md` (see https://git.eeqj.de/sneak/webhooker/issues/112).
- Run `make bootstrap` (or `make assets`) in a fresh clone before gating — third-party browser assets are fetched at build time and are not committed.
- Gate on `make check` plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.
clawbot
self-assigned this 2026-08-17 23:45:16 +02:00
Plan. The code the issue names is unchanged on next (4a72413): internal/server/routes.go mounts /s for every method, and TestStaticServesEveryMethod (internal/server/routes_test.go) pins that.
Register the static handler for GET and HEAD only. Other methods get 405 with Allow: GET, HEAD, through chi's method-not-allowed handling if it gives exactly that, otherwise a small handler.
Invert the test as the issue says: 405 and the Allow header for POST, PUT and DELETE; 200 for GET and HEAD.
Update the /s/* row in the README route table.
Say on the PR, in one line, whether the body-size gap is closed: a 405 returned before any read closes it.
Model: opus-5-5
Plan. The code the issue names is unchanged on `next` (`4a72413`): `internal/server/routes.go` mounts `/s` for every method, and `TestStaticServesEveryMethod` (`internal/server/routes_test.go`) pins that.
- Register the static handler for `GET` and `HEAD` only. Other methods get `405` with `Allow: GET, HEAD`, through chi's method-not-allowed handling if it gives exactly that, otherwise a small handler.
- Invert the test as the issue says: `405` and the `Allow` header for `POST`, `PUT` and `DELETE`; `200` for `GET` and `HEAD`.
- Update the `/s/*` row in the README route table.
- Say on the PR, in one line, whether the body-size gap is closed: a `405` returned before any read closes it.
Model: opus-5-5
Implemented in #335: /s/* now answers GET and HEAD only, and any other method gets 405 with Allow: GET, HEAD. The body-size gap is closed, because POST, PUT and PATCH are refused before the body is read.
Model: opus-5-5
Implemented in https://git.eeqj.de/sneak/webhooker/pulls/335: `/s/*` now answers `GET` and `HEAD` only, and any other method gets `405` with `Allow: GET, HEAD`. The body-size gap is closed, because `POST`, `PUT` and `PATCH` are refused before the body is read.
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.
Found by the independent review of #156, which settled the question empirically rather than by reading. Deliberately NOT milestoned 1.0.0 — see below.
internal/server/routes.goattaches the static handler withMount, which registers every method, andhttp.FileServerspecial-cases only HEAD (by omitting the body).TestStaticServesEveryMethod, added by that PR, pins the current behaviour:GET,HEAD,POST,PUTandDELETEon/s/js/app.jsall return 200, with POST/PUT/DELETE returning the asset bytes.Why it is not a 1.0 blocker:
http.FileServeris strictly read-only, so there is no state change and no write path — a POST response is byte-identical to the GET. Nothing is exploitable.Why it is still worth fixing:
Allow-header and method semantics are simply wrong for the route./ssits outside the four form route groups, soMaxBodySizenever applies to it. An attacker can send an arbitrarily large body to an asset path.net/httpcaps the drain and the handler never reads it, so the exposure is a held connection rather than memory — but it is the one place where the body-size middleware's coverage has a hole.Definition of done
GETandHEADinstead of viaMount, and other methods get405with an accurateAllow: GET, HEAD.TestStaticServesEveryMethodis INVERTED rather than deleted: it must assert 405 and theAllowheader for POST/PUT/DELETE, and 200 for GET/HEAD. The test is what stops this drifting back, so it should survive the change with its meaning flipped./s/*row in the README route table is updated — it currently readsany, which #156 made accurate on purpose. Both must change together or the README goes back to lying.Implementation requirements
next, PR based onnext, single commit, title ending(closes #N).TODO.md(see #112).make bootstrap(ormake assets) in a fresh clone before gating — third-party browser assets are fetched at build time and are not committed.make checkplus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.Plan. The code the issue names is unchanged on
next(4a72413):internal/server/routes.gomounts/sfor every method, andTestStaticServesEveryMethod(internal/server/routes_test.go) pins that.GETandHEADonly. Other methods get405withAllow: GET, HEAD, through chi's method-not-allowed handling if it gives exactly that, otherwise a small handler.405and theAllowheader forPOST,PUTandDELETE;200forGETandHEAD./s/*row in the README route table.405returned before any read closes it.Model: opus-5-5
Implemented in #335:
/s/*now answersGETandHEADonly, and any other method gets405withAllow: GET, HEAD. The body-size gap is closed, becausePOST,PUTandPATCHare refused before the body is read.Model: opus-5-5