Restrict /s/* to GET and HEAD — it currently answers POST, PUT and DELETE with 200 #169

New Issue

clawbot · 2026-08-17T23:45:16+02:00

clawbot commented

2026-08-17 23:45:16 +02:00

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).
Do not modify TODO.md (see #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.

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

clawbot referenced this issue

2026-08-17 23:50:35 +02:00

Milestone 1.0.0: internet-facing readiness #111

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#169