MaxBodySize logs the full client-chosen path ahead of RequireAuth, sidestepping the access-log line budget #176

New Issue

clawbot · 2026-08-18T01:09:18+02:00

clawbot commented

2026-08-18 01:09:18 +02:00

Found by the independent review of #174 while sweeping for siblings of the credential leak. Pre-existing, and outside that PR's scope.

internal/middleware/middleware.go:616-622 — the MaxBodySize rejection path logs r.URL.Path untruncated, at Warn level, so it is on by default. And internal/server/routes.go:128-131 places MaxBodySize ahead of RequireAuth.

So an unauthenticated POST /source/<8 KB of client-chosen text>/edit carrying an oversize declared Content-Length writes an arbitrary-length attacker-chosen path into the operator's log — bypassing the 2,560-byte per-line budget that #146 just established, because that budget lives in the access-log field capping and this is a separate log call.

Milestoned 1.0.0 for consistency: this is exactly the defect class #146 was moved into the milestone for — an unauthenticated client on the public internet writing unbounded attacker-chosen text into the operator's logs. That fix is incomplete while this path exists, and leaving it would mean the stated per-line bound is true of one log call and false of another, which is worse than not stating it.

Note it does NOT reopen #160: there is no query string involved, only the path.

Definition of done

The path in this log line is bounded, using the same field-capping mechanism #146 introduced rather than a second ad-hoc truncation — one budget, one implementation.
Audit every other log call for the same shape. This one was found by accident; the access-log sweep did not cover it because it is not the access log. Any slog call reaching a client-controlled value — path, header, form field, URL — before or independently of the access-log capping needs the same treatment or an explicit reason. List what you checked.
Consider whether MaxBodySize should sit behind RequireAuth instead. It probably should not — an oversize body ought to be rejected before any session work — so if you leave the ordering, say why in a comment, since the ordering is what makes this reachable unauthenticated.
A test that a flood of oversize requests with long client-chosen paths does not produce log lines that grow with the input.

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 in a fresh clone before gating — browser assets are fetched at build time, and make lint now needs Docker.
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/174 while sweeping for siblings of the credential leak. Pre-existing, and outside that PR's scope. `internal/middleware/middleware.go:616-622` — the `MaxBodySize` rejection path logs `r.URL.Path` **untruncated**, at `Warn` level, so it is on by default. And `internal/server/routes.go:128-131` places `MaxBodySize` **ahead of** `RequireAuth`. So an unauthenticated `POST /source/<8 KB of client-chosen text>/edit` carrying an oversize declared `Content-Length` writes an arbitrary-length attacker-chosen path into the operator's log — bypassing the 2,560-byte per-line budget that https://git.eeqj.de/sneak/webhooker/issues/146 just established, because that budget lives in the access-log field capping and this is a separate log call. Milestoned `1.0.0` for consistency: this is exactly the defect class https://git.eeqj.de/sneak/webhooker/issues/146 was moved into the milestone for — an unauthenticated client on the public internet writing unbounded attacker-chosen text into the operator's logs. That fix is incomplete while this path exists, and leaving it would mean the stated per-line bound is true of one log call and false of another, which is worse than not stating it. Note it does NOT reopen https://git.eeqj.de/sneak/webhooker/issues/160: there is no query string involved, only the path. ## Definition of done - The path in this log line is bounded, using the same field-capping mechanism https://git.eeqj.de/sneak/webhooker/issues/146 introduced rather than a second ad-hoc truncation — one budget, one implementation. - **Audit every other log call for the same shape.** This one was found by accident; the access-log sweep did not cover it because it is not the access log. Any `slog` call reaching a client-controlled value — path, header, form field, URL — before or independently of the access-log capping needs the same treatment or an explicit reason. List what you checked. - Consider whether `MaxBodySize` should sit behind `RequireAuth` instead. It probably should not — an oversize body ought to be rejected before any session work — so if you leave the ordering, say why in a comment, since the ordering is what makes this reachable unauthenticated. - A test that a flood of oversize requests with long client-chosen paths does not produce log lines that grow with the input. ## 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` in a fresh clone before gating — browser assets are fetched at build time, and `make lint` now needs Docker. - Gate on `make check` plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.

clawbot added this to the 1.0.0 milestone 2026-08-18 01:09:18 +02:00

clawbot self-assigned this 2026-08-18 01:09:18 +02:00

clawbot referenced this issue

2026-08-18 01:40:55 +02:00

GORM's default logger prints the full interpolated SQL, including the client-chosen path and username, on every record-not-found #178

clawbot referenced this issue from a commit

2026-08-18 01:47:06 +02:00

Bound every slog line against client-chosen text (closes #176)

clawbot referenced a pull request that will close this issue

2026-08-18 01:50:50 +02:00

Bound every slog line against client-chosen text (closes #176) #180

clawbot referenced this issue from a commit