Middleware.MaxBodySize logged r.URL.Path untruncated at WARN, and internal/server/routes.go registers it ahead of RequireAuth. So an unauthenticated POST /source/<8 KB of client-chosen text>/edit carrying an oversize declared Content-Length — a request with no body at all — wrote arbitrary-length attacker-chosen text into the operator's log. The 2,560-byte per-line budget from #146 did not reach it: that budget lives in the access-log field capping, and this is a separate slog call.
One budget, one implementation
truncateLogField and encodedLogFieldBytes moved out of internal/middleware into a new internal/logfield package as Truncate and EncodedBytes, with the 512-byte budget as logfield.MaxBytes. The logic is unchanged — this is a move, not a rewrite — but the audit below spans internal/middleware and internal/handlers, and a helper both need does not belong to either. The access log now spends logfield.MaxBytes where it spent maxLogFieldBytes. No second truncation was written anywhere.
The audit
Every slog call in internal/ and cmd/ was read, and re-read against the rebased tree each round. Grouped by verdict.
Capped by this PR (8)
All eight are reachable by an unauthenticated request.
Site
Level
Value
Why it is unbounded
middleware.goMaxBodySize, request body exceeds limit
WARN
r.URL.Path, r.Method
The issue. Registered ahead of RequireAuth; a declared Content-Length is free to send.
csrf.gocsrf: token validation failed
WARN
r.URL.Path, r.Method
CSRF is also registered ahead of RequireAuth on every group that uses it. A tokenless POST to /source/<anything>/edit lands here. Not named in the issue; found by this sweep.
DEBUG being off by default is not a bound, and this PR does not treat it as one. floodTooManyRequests already established that principle in this repo: it drops the path precisely so that turning DEBUG on to diagnose a flood does not restore the problem.
The two login-throttle caps, and how they are pinned
Neither line was ever wide. chi v1.5.5 routes POST /pages/login on a static pattern, so r.URL.Path at both sites is the 12-byte constant /pages/login and each line lands near 120 bytes.
They are capped anyway for three reasons. The stated bound in README.md and on MaxAccessLogLineBytes is written as covering every slog line an unauthenticated request reaches, and these two made it false as written. RecordLoginFailure is an exported Middleware method taking any *http.Request, so the safety rests on a routing invariant nobody had written down; a second caller on a route with a URL parameter would widen the line. And the same message at internal/handlers/profile.go:84 logs no path at all, so the tree was already inconsistent on this line.
Round 4 pins both caps with tests. In round 3 they were capped but unasserted, and that was disclosed rather than fixed — a bound nobody checks is how this repo's recurring defect gets in. No request through the mux can widen either line, so the tests make exactly the call the caps defend against:
TestLoginThrottle_LogLineDoesNotTrackPathSize (internal/middleware/logbound_test.go) calls the exported RecordLoginFailure past its failure budget with a request whose r.URL.Path carries 8 KB of client-chosen text — the request a caller on a parameterised route would hand it.
TestVerificationCapacity_LogLineDoesNotTrackPathSize (internal/handlers/logbound_test.go) fills every Argon2id verification slot and then drives HandleLoginSubmit directly at an 8 KB path, so the 503 branch runs.
Both run under both handlers and all seven fills, and both are deterministic. The capacity test takes slots through the semaphore's own fast path until one is refused, so it does not depend on the concurrency constant, and it passes an already-canceled context so the refusal comes from ctx.Done() rather than from a five-second timer firing. That rests on f6ec78e's free-slot preamble in acquire, which hands out a free slot before consulting the context; without it a canceled context could shed a slot standing free and the loop would stop early. Nothing in either test waits on a clock. Reverting either cap now fails 14 subtests — see mutation 5.
Capped though they did not strictly need it (2)
handlers/auth.goinvalid password and user logged in, both username. Reached only after the username matched a stored row, so both are bounded by the operator's own data. Capped anyway so that every username this unauthenticated endpoint logs is capped, and no reader has to work out which branch narrowed which. Both are pinned by a test, and each is pinned independently — uncapping either one alone fails both handlers. See mutation 4.
Judged safe, with the reason (the rest)
Authenticated operator input.source_management.gowebhook created (name), target URL blocked by SSRF protection (reduced to scheme+host by MaskURL, which keeps the host verbatim); delivery/engine.gofailing orphaned retrying delivery (target_name); target_http.gocircuit breaker open (target_name); profile.gouser changed password (username, from the session). All require RequireAuth, all are the operator's own configuration echoed back, all bounded only by the 1 MB form cap. Truncating them would cost the operator debuggability against no adversary. Recorded rather than changed — and, since round 1, recorded in the README and on the constant too, not only here.
Server-assigned, not client-chosen.remote_addr / remoteIP (csrf.go, webhook.go, middleware.go) come from the accepted connection, not from the request. reason in the CSRF line is one of gorilla/csrf's own fixed error values.
Identifiers the service generated. Every webhook_id, event_id, delivery_id, target_id, entrypoint_id, user_id, status, attempt, count, rows_deleted across delivery/, database/ and handlers/. UUIDs and integers this process minted.
Operator environment and filesystem paths.config.go (both call sites), database.go (path, data_dir), webhook_db_manager.go (all four), target_database.go and target_database_archive.go (path), server/http.go (listenaddr), logger.go, session.go, archive_sweeper.go, retention.go, lifecycle.go, server.go. Startup, shutdown and background workers; no request reaches them.
handlers.gotemplate not found. Logs pageTemplate. All twelve renderTemplate call sites pass a string literal, so nothing client-derived reaches it.
webhook.gowebhook request received. Logs entrypoint_uuid — but only after the lookup succeeded, so the UUID names a stored entrypoint. Its r.Method is the literal POST; the handler returns 405 above it otherwise. The comment already on that call records that this ordering is deliberate and why.
Error-only lines. The large majority of Error calls log a fixed message plus a GORM or I/O error. GORM's *gorm.DB.Error on these paths is ErrRecordNotFound or a driver error; neither embeds the bound parameters in the Go error value. (The GORM logger is a different matter — see below.)
The stated bound
MaxAccessLogLineBytes (2,560) is stated as the ceiling on every line the service writes through slogthat carries text an unauthenticated client supplies — the eight lines above plus the access log. Each of them carries strictly fewer client-supplied fields than the access log does, so none can be wider than it; but the PR does not rest on that reasoning. All eight are asserted against the ceiling directly, per line, under both handlers, with the widest fills the handlers can be made to escape.
That per-line ceiling is the whole of what the constant states, and it is the whole of what most of these rows establish. Three sites go further and bound the total bytes a whole flood wrote, not just each line of it: request body exceeds limit (TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog), entrypoint not found and user not found (both through assertBoundedFlood). No aggregate assertion exists at the CSRF, rate-limit, RequireAuth, invalid password or login-throttle sites, and README.md now says so instead of claiming the flood property for six rows. (Rounds 1-4 of this body and the shipped commit message claimed six; that was wrong, and round 5 corrects it in all three places.)
Not covered, and stated as not covered
Three kinds of writer the ceiling does not reach, named in the README and on MaxAccessLogLineBytes, because a bound that is true of one writer and silently false of another is the failure mode #146 spent four rounds on.
1. Lines carrying an authenticated operator's own input, which are not truncated at all. webhook created logs the submitted name verbatim — a 100 KB field produces a single JSON line of 600,171 bytes, and the 1 MB form cap allows roughly 6 MB — and target URL blocked by SSRF protection logs MaskURL(targetURL), which keeps parsed.Host verbatim, at 100,011 bytes from a 100 KB host. The target_name lines in internal/delivery/engine.go and internal/delivery/target_http.go are the same shape. Leaving them uncapped is deliberate: each requires an authenticated operator on a service with no self-registration, and truncating the operator's own configuration echoed back costs debuggability against no adversary. The defect was only ever that this qualification did not reach the two places an operator reads.
2. The log delivery target (internal/delivery/target_log.go) writes the entire inbound event — headers and body — to the log. Deliberate: capping it would defeat the target, since emitting the payload is the delivery. It costs nothing unless an authenticated operator creates a target of that type, and each line is bounded per event by the 1 MB receiver body cap. Documented on the type rather than changed.
3. GORM's default logger is a real, unfixed defect, and it is worse than the one this PR fixes. Both gorm.Open calls pass a bare &gorm.Config{}, leaving logger.Default in place: LogLevel: Warn, IgnoreRecordNotFoundError: false. logger.Trace therefore prints the fully interpolated SQL to stdout on every ErrRecordNotFound — including the client-chosen path on /webhook/{uuid} and the submitted username on the login form. On by default, answering to no level the operator sets, not routed through internal/logger at all. Filed as #178 rather than fixed here: it is a second, independent writer, and choosing what to install in its place is an observability decision with consequences beyond these two paths. That issue will restate this carve-out when it lands.
The ordering question
MaxBodySize stays ahead of RequireAuth. An oversize body should be refused before the request buys a cookie decrypt, a session load and the database read behind it; rejecting first is the cheaper failure and the ordering that keeps an unauthenticated flood from choosing how much session work the process does. Moving it behind RequireAuth would trade a bounded log line for unbounded session work, which is the wrong direction.
The ordering is what makes the line reachable unauthenticated, so it is no longer left unexplained: the rationale, and what it costs, now sits on maxFormBodySize in internal/server/routes.go, which every one of the four registrations references. The same note covers CSRF, which sits in front of RequireAuth for the same reason and has the same consequence.
Tests
internal/middleware/logbound_test.go and internal/handlers/logbound_test.go drive 8 KB of client-chosen text at all eight sites, across both handlers internal/logger can install and each of seven fills. Each case holds the encoded line to MaxAccessLogLineBytes and asserts that the two markers at the far end of the input are absent — so a value that merely happened to be short cannot pass for a truncated one. Three of the sites, named under "The stated bound" above, additionally hold the whole flood's output to what that ceiling allows; the rest carry the per-line bound only.
On the trap #146 kept hitting: the fills are x, a quotation mark, a backslash, a tab, a newline, a bare C0 control (U+0001) and an astral non-printable (U+1000C). The C0 control is the one that matters most: the JSON handler spells it as a six-byte \uXXXX escape for the single byte it cost to send, which is the widest multiplier a client can drive. Mutation 3 below is caught by that fill alone, and only under the JSON handler, at 3,072 bytes against 2,560 — a 512-byte margin. Both test files record that on the fill, so it is not simplified away.
TestStoredUsername_LogLinesDoNotTrackUsernameSize covers the two login lines past the username lookup, which round 1 capped without asserting. It creates an account per fill whose username carries the client-chosen text, then drives one wrong password (invalid password) and one correct one (user logged in) at each. The fill is 1 KB rather than 8 KB there for a reason worth knowing: the session cookie is written before the success line, and securecookie refuses a value past 4 KB, so an 8 KB username answers 500 and never reaches the log line at all.
internal/logfield/logfield_test.go measures the per-rune charge against what the handlers actually emit, over roughly 3,000 code points on each — every rune below U+0800 densely, the separators only the JSON handler escapes, and a stratified sample across the remaining planes — so an undercharged rune fails a test rather than quietly falsifying the ceiling.
Mutation verification
Each run is the full suite via make test in a throwaway copy at a session-unique path, deleted afterwards; the working clone was never mutated.
1. Revert the MaxBodySize cap alone (back to "path", r.URL.Path) — 28 leaf subtests fail: 14 in TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/* and 14 in TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/* (2 handlers x 7 fills each), plus the two parent tests. The quoted failure reproduces to the byte:
Error: "16583" is not less than or equal to "2560"
Test: TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/json/tab
16,583 bytes against a 2,560 ceiling — the 8 KB of tabs doubled by escaping.
2. Revert the other five caps — 70 subtests fail, 14 per site, each site distinguishable.
3. Budget raw bytes instead of encoded (cost := utf8.RuneLen(r) in Truncate) — 23 subtests fail across internal/logfield, internal/middleware and internal/handlers, including the pre-existing access-log cases from #146.
4. Uncap invalid password and user logged in — TestStoredUsername_LogLinesDoNotTrackUsernameSize fails on both handlers (json 6281, text 4213, against 2560). Each site is also pinned on its own: uncapping user logged in alone fails both handlers (json 6327, text 4255), and uncapping invalid password alone fails both handlers (json 6281, text 2676 — measured in round 5, the one leg of this claim that had not been). So the two are independently pinned, not jointly.
5. Uncap the two login-throttle WARN lines — this result has changed. In round 3, reverting both failed nothing and that was disclosed. With the round-4 tests in place, reverting both fails 28 leaf subtests, 14 per site, on both handlers and every fill:
Error: "16534" is not less than or equal to "2560"
Test: TestLoginThrottle_LogLineDoesNotTrackPathSize/json/tab
Error: "16547" is not less than or equal to "2560"
Test: TestVerificationCapacity_LogLineDoesNotTrackPathSize/json/tab
Round 5
Head fe9454f, rebased onto nextf6ec78e (unchanged since round 4; the commit's parent isorigin/next). The only file changed against round 4's 3184892 is README.md. No code, no test, no doc comment moved.
The blocking finding is fixed by correcting the claim, not by adding assertions.README.md said the tests hold "for the six rows a request can widen, the whole flood's output to what that ceiling allows". Three sites carry a whole-flood assertion, not six — I re-derived that from the tests rather than taking the review's word: TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog (internal/middleware/logbound_test.go:478, oversize-control < sent/2 and oversize <= floodRequests*MaxAccessLogLineBytes), and assertBoundedFlood at internal/handlers/logbound_test.go:294 and :329. The CSRF, RequireAuth and rate-limit rows are driven by TestLogLines_ClientChosenPathDoesNotSizeTheLine, one request per subtest, per-line only; invalid password is driven by TestStoredUsername_LogLinesDoNotTrackUsernameSize, which asserts per line and line count and nothing aggregate. The other flood helper, assertFloodIsBounded, has three callers and all three are access-log tests. The README now names the three and says the other rows carry no aggregate assertion; the PR body above and the commit message carry the same correction, since the commit message is the shipped record.
Adding the three missing flood assertions was the alternative and was not taken: at CSRF and RequireAuth a flood writes one line per request, so an aggregate bound there is the per-line bound multiplied out and proves nothing new, and the rate-limit site logs one line per nine requests. An accurate claim is worth more than a strained assertion.
Two further inaccuracies in the same README sentence, found while re-verifying it and fixed in the same edit. It said the tests drive "8 KB of client-chosen text at each of these" — true of every row except invalid password, whose fill is 1 KB (storedFillBytes), for the securecookie reason above; the README now states the exception where it makes the claim, not two paragraphs away. And it said "through every character the handlers escape", which is false as written: the fills are seven specific characters, not every character either handler escapes. It now names them.
Every remaining number in the README hunks was re-checked against the code on this tree, not against memory: /pages/login is 12 bytes; oversizedSegmentBytes and oversizedFillBytes are both 8192; storedFillBytes is 1024; escapeFills has exactly seven entries; chargeTestRunes yields 3,146 code points, so "roughly 3,000" holds; "removing either cap fails 14 subtests" matches mutation 5. The MaxAccessLogLineBytes doc comment makes no flood claim and is unchanged — it says "asserted directly, per line and under both handlers", which is true of all eight sites.
Mutation evidence is carried forward from 3184892 except mutation 4's second leg. Nothing executable changed, so mutations 1, 2, 3, 5 and the user logged in leg of 4 were not re-run this round; they are round-4 measurements, restated as such and not as fresh ones. Mutation 4's invalid password-alone leg was run here, because the commit message claimed "uncapping either ... fails both handlers on its own" while only the user logged in leg had ever been measured: json 6281, text 2676, both handlers failing, TestStoredUsername_LogLinesDoNotTrackUsernameSize. The claim is now backed rather than inferred.
Gate evidence
Fresh /tmp clone, make bootstrap run first. All figures below are from the pushed head fe9454f.
make check — exit 0. Lint ran in Docker: 0 issues. in 47.68 s. 14 packages, all ok, zero (cached) package lines (GOFLAGS=-count=1), 769 --- PASS, zero --- FAIL. Working tree clean afterwards, so make fmt is clean. TODO.md untouched.
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — exit 0, with the checks demonstrably executing rather than replaying:
#15 [lint 7/9] RUN make fmt-check DONE 0.7s
#16 [lint 8/9] RUN golangci-lint config verify --config .golangci.yml DONE 0.4s
#17 [lint 9/9] RUN golangci-lint run --config .golangci.yml ./...
#17 47.45 0 issues.
#17 DONE 50.8s
#25 [builder 9/11] RUN make test DONE 61.6s
#26 [builder 10/11] RUN make build DONE 42.4s
Zero(cached) package lines anywhere in the log. The eight CACHED layers are the two digest-pinned base-image resolves (#7, #8) and six stage-2 runtime layers (#28-#33); none in lint or builder.
The build log clipped at BuildKit's 2 MiB limit inside the test stage (#25 61.03 [output clipped, log limit 2MiB reached]), from GORM's record-not-found noise — #178, in review as #182, not this PR's defect. The clipping is a display limit on the build log, not a truncation of the run: #25 DONE 61.6s and the build's overall exit 0 establish that make test ran to completion and passed, and the per-package --- PASS lines were read from the separate make check run on the same tree.
No containers started and none left behind (docker ps -a empty); the tagged image was removed. No prune of any kind.
Closes https://git.eeqj.de/sneak/webhooker/issues/176.
## The defect
`Middleware.MaxBodySize` logged `r.URL.Path` untruncated at `WARN`, and `internal/server/routes.go` registers it ahead of `RequireAuth`. So an unauthenticated `POST /source/<8 KB of client-chosen text>/edit` carrying an oversize declared `Content-Length` — a request with no body at all — wrote arbitrary-length attacker-chosen text into the operator's log. The 2,560-byte per-line budget from https://git.eeqj.de/sneak/webhooker/issues/146 did not reach it: that budget lives in the access-log field capping, and this is a separate `slog` call.
## One budget, one implementation
`truncateLogField` and `encodedLogFieldBytes` moved out of `internal/middleware` into a new `internal/logfield` package as `Truncate` and `EncodedBytes`, with the 512-byte budget as `logfield.MaxBytes`. The logic is unchanged — this is a move, not a rewrite — but the audit below spans `internal/middleware` and `internal/handlers`, and a helper both need does not belong to either. The access log now spends `logfield.MaxBytes` where it spent `maxLogFieldBytes`. No second truncation was written anywhere.
## The audit
Every `slog` call in `internal/` and `cmd/` was read, and re-read against the rebased tree each round. Grouped by verdict.
### Capped by this PR (8)
All eight are reachable by an unauthenticated request.
| Site | Level | Value | Why it is unbounded |
| --- | --- | --- | --- |
| `middleware.go` `MaxBodySize`, `request body exceeds limit` | `WARN` | `r.URL.Path`, `r.Method` | The issue. Registered ahead of `RequireAuth`; a declared `Content-Length` is free to send. |
| `csrf.go` `csrf: token validation failed` | `WARN` | `r.URL.Path`, `r.Method` | **`CSRF` is also registered ahead of `RequireAuth`** on every group that uses it. A tokenless POST to `/source/<anything>/edit` lands here. Not named in the issue; found by this sweep. |
| `ratelimit.go` `tooManyRequests`, `... rate limit exceeded` | `WARN` | `r.URL.Path` | Used by the **per-entrypoint receiver limiter**, which is unauthenticated and whose route matches any single segment. |
| `middleware.go` `RequireAuth`, `unauthenticated request` | `DEBUG` | `r.URL.Path`, `r.Method` | The unauthenticated branch by definition; nothing has narrowed the path. |
| `handlers/webhook.go` `entrypoint not found` | `DEBUG` | entrypoint UUID | The receiver's lookup missed, so the segment matched no stored data. |
| `handlers/auth.go` `user not found` | `DEBUG` | `username` form field | Login is unauthenticated; the field is bounded only by the 1 MB body cap, and the lookup missed. |
| `loginguard.go` `login failure limit exceeded` | `WARN` | `r.URL.Path` | Arrived in `next` with https://git.eeqj.de/sneak/webhooker/pulls/171. Not wide today — see below — but capped defensively. |
| `handlers/auth.go` `password verification capacity exhausted` | `WARN` | `r.URL.Path` | Same origin, same route, same reasoning. |
`DEBUG` being off by default is **not** a bound, and this PR does not treat it as one. `floodTooManyRequests` already established that principle in this repo: it drops the path precisely so that turning `DEBUG` on to diagnose a flood does not restore the problem.
#### The two login-throttle caps, and how they are pinned
Neither line was ever wide. `chi` v1.5.5 routes `POST /pages/login` on a static pattern, so `r.URL.Path` at both sites is the 12-byte constant `/pages/login` and each line lands near 120 bytes.
They are capped anyway for three reasons. The stated bound in `README.md` and on `MaxAccessLogLineBytes` is written as covering every `slog` line an unauthenticated request reaches, and these two made it false as written. `RecordLoginFailure` is an exported `Middleware` method taking any `*http.Request`, so the safety rests on a routing invariant nobody had written down; a second caller on a route with a URL parameter would widen the line. And the same message at `internal/handlers/profile.go:84` logs no path at all, so the tree was already inconsistent on this line.
**Round 4 pins both caps with tests.** In round 3 they were capped but unasserted, and that was disclosed rather than fixed — a bound nobody checks is how this repo's recurring defect gets in. No request through the mux can widen either line, so the tests make exactly the call the caps defend against:
- `TestLoginThrottle_LogLineDoesNotTrackPathSize` (`internal/middleware/logbound_test.go`) calls the exported `RecordLoginFailure` past its failure budget with a request whose `r.URL.Path` carries 8 KB of client-chosen text — the request a caller on a parameterised route would hand it.
- `TestVerificationCapacity_LogLineDoesNotTrackPathSize` (`internal/handlers/logbound_test.go`) fills every Argon2id verification slot and then drives `HandleLoginSubmit` directly at an 8 KB path, so the 503 branch runs.
Both run under both handlers and all seven fills, and both are deterministic. The capacity test takes slots through the semaphore's own fast path until one is refused, so it does not depend on the concurrency constant, and it passes an already-canceled context so the refusal comes from `ctx.Done()` rather than from a five-second timer firing. That rests on `f6ec78e`'s free-slot preamble in `acquire`, which hands out a free slot before consulting the context; without it a canceled context could shed a slot standing free and the loop would stop early. Nothing in either test waits on a clock. Reverting either cap now fails **14** subtests — see mutation 5.
### Capped though they did not strictly need it (2)
`handlers/auth.go` `invalid password` and `user logged in`, both `username`. Reached only after the username matched a stored row, so both are bounded by the operator's own data. Capped anyway so that every username this unauthenticated endpoint logs is capped, and no reader has to work out which branch narrowed which. Both are pinned by a test, and each is pinned **independently** — uncapping either one alone fails both handlers. See mutation 4.
### Judged safe, with the reason (the rest)
- **Authenticated operator input.** `source_management.go` `webhook created` (`name`), `target URL blocked by SSRF protection` (reduced to scheme+host by `MaskURL`, which keeps the host verbatim); `delivery/engine.go` `failing orphaned retrying delivery` (`target_name`); `target_http.go` `circuit breaker open` (`target_name`); `profile.go` `user changed password` (username, from the session). All require `RequireAuth`, all are the operator's own configuration echoed back, all bounded only by the 1 MB form cap. Truncating them would cost the operator debuggability against no adversary. Recorded rather than changed — and, since round 1, recorded in the README and on the constant too, not only here.
- **Server-assigned, not client-chosen.** `remote_addr` / `remoteIP` (`csrf.go`, `webhook.go`, `middleware.go`) come from the accepted connection, not from the request. `reason` in the CSRF line is one of gorilla/csrf's own fixed error values.
- **Identifiers the service generated.** Every `webhook_id`, `event_id`, `delivery_id`, `target_id`, `entrypoint_id`, `user_id`, `status`, `attempt`, `count`, `rows_deleted` across `delivery/`, `database/` and `handlers/`. UUIDs and integers this process minted.
- **Operator environment and filesystem paths.** `config.go` (both call sites), `database.go` (`path`, `data_dir`), `webhook_db_manager.go` (all four), `target_database.go` and `target_database_archive.go` (`path`), `server/http.go` (`listenaddr`), `logger.go`, `session.go`, `archive_sweeper.go`, `retention.go`, `lifecycle.go`, `server.go`. Startup, shutdown and background workers; no request reaches them.
- **`handlers.go` `template not found`.** Logs `pageTemplate`. All twelve `renderTemplate` call sites pass a string literal, so nothing client-derived reaches it.
- **`webhook.go` `webhook request received`.** Logs `entrypoint_uuid` — but only after the lookup succeeded, so the UUID names a stored entrypoint. Its `r.Method` is the literal `POST`; the handler returns 405 above it otherwise. The comment already on that call records that this ordering is deliberate and why.
- **Error-only lines.** The large majority of `Error` calls log a fixed message plus a GORM or I/O error. GORM's `*gorm.DB.Error` on these paths is `ErrRecordNotFound` or a driver error; neither embeds the bound parameters in the Go error value. (The GORM _logger_ is a different matter — see below.)
## The stated bound
`MaxAccessLogLineBytes` (2,560) is stated as the ceiling on every line the service writes through `slog` **that carries text an unauthenticated client supplies** — the eight lines above plus the access log. Each of them carries strictly fewer client-supplied fields than the access log does, so none can be wider than it; but the PR does not rest on that reasoning. **All eight are asserted against the ceiling directly, per line, under both handlers, with the widest fills the handlers can be made to escape.**
That per-line ceiling is the whole of what the constant states, and it is the whole of what most of these rows establish. **Three** sites go further and bound the total bytes a whole flood wrote, not just each line of it: `request body exceeds limit` (`TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog`), `entrypoint not found` and `user not found` (both through `assertBoundedFlood`). No aggregate assertion exists at the CSRF, rate-limit, `RequireAuth`, `invalid password` or login-throttle sites, and `README.md` now says so instead of claiming the flood property for six rows. (Rounds 1-4 of this body and the shipped commit message claimed six; that was wrong, and round 5 corrects it in all three places.)
### Not covered, and stated as not covered
Three kinds of writer the ceiling does **not** reach, named in the README and on `MaxAccessLogLineBytes`, because a bound that is true of one writer and silently false of another is the failure mode https://git.eeqj.de/sneak/webhooker/issues/146 spent four rounds on.
**1. Lines carrying an authenticated operator's own input**, which are not truncated at all. `webhook created` logs the submitted `name` verbatim — a 100 KB field produces a single JSON line of **600,171 bytes**, and the 1 MB form cap allows roughly 6 MB — and `target URL blocked by SSRF protection` logs `MaskURL(targetURL)`, which keeps `parsed.Host` verbatim, at **100,011 bytes** from a 100 KB host. The `target_name` lines in `internal/delivery/engine.go` and `internal/delivery/target_http.go` are the same shape. Leaving them uncapped is deliberate: each requires an authenticated operator on a service with no self-registration, and truncating the operator's own configuration echoed back costs debuggability against no adversary. The defect was only ever that this qualification did not reach the two places an operator reads.
**2. The `log` delivery target** (`internal/delivery/target_log.go`) writes the entire inbound event — headers and body — to the log. Deliberate: capping it would defeat the target, since emitting the payload _is_ the delivery. It costs nothing unless an authenticated operator creates a target of that type, and each line is bounded per event by the 1 MB receiver body cap. Documented on the type rather than changed.
**3. GORM's default logger is a real, unfixed defect, and it is worse than the one this PR fixes.** Both `gorm.Open` calls pass a bare `&gorm.Config{}`, leaving `logger.Default` in place: `LogLevel: Warn`, `IgnoreRecordNotFoundError: false`. `logger.Trace` therefore prints the **fully interpolated SQL to stdout on every `ErrRecordNotFound`** — including the client-chosen path on `/webhook/{uuid}` and the submitted username on the login form. On by default, answering to no level the operator sets, not routed through `internal/logger` at all. Filed as https://git.eeqj.de/sneak/webhooker/issues/178 rather than fixed here: it is a second, independent writer, and choosing what to install in its place is an observability decision with consequences beyond these two paths. That issue will restate this carve-out when it lands.
## The ordering question
**`MaxBodySize` stays ahead of `RequireAuth`.** An oversize body should be refused before the request buys a cookie decrypt, a session load and the database read behind it; rejecting first is the cheaper failure and the ordering that keeps an unauthenticated flood from choosing how much session work the process does. Moving it behind `RequireAuth` would trade a bounded log line for unbounded session work, which is the wrong direction.
The ordering is what makes the line reachable unauthenticated, so it is no longer left unexplained: the rationale, and what it costs, now sits on `maxFormBodySize` in `internal/server/routes.go`, which every one of the four registrations references. The same note covers `CSRF`, which sits in front of `RequireAuth` for the same reason and has the same consequence.
## Tests
`internal/middleware/logbound_test.go` and `internal/handlers/logbound_test.go` drive 8 KB of client-chosen text at all eight sites, across both handlers `internal/logger` can install and each of seven fills. Each case holds the **encoded** line to `MaxAccessLogLineBytes` and asserts that the two markers at the far end of the input are absent — so a value that merely happened to be short cannot pass for a truncated one. Three of the sites, named under "The stated bound" above, additionally hold the whole flood's output to what that ceiling allows; the rest carry the per-line bound only.
On the trap https://git.eeqj.de/sneak/webhooker/issues/146 kept hitting: the fills are `x`, a quotation mark, a backslash, a tab, a newline, **a bare C0 control (U+0001)** and an astral non-printable (U+1000C). The C0 control is the one that matters most: the JSON handler spells it as a six-byte `\uXXXX` escape for the single byte it cost to send, which is the widest multiplier a client can drive. Mutation 3 below is caught by that fill alone, and only under the JSON handler, at 3,072 bytes against 2,560 — a 512-byte margin. Both test files record that on the fill, so it is not simplified away.
`TestStoredUsername_LogLinesDoNotTrackUsernameSize` covers the two login lines past the username lookup, which round 1 capped without asserting. It creates an account per fill whose username carries the client-chosen text, then drives one wrong password (`invalid password`) and one correct one (`user logged in`) at each. The fill is 1 KB rather than 8 KB there for a reason worth knowing: the session cookie is written **before** the success line, and securecookie refuses a value past 4 KB, so an 8 KB username answers 500 and never reaches the log line at all.
`internal/logfield/logfield_test.go` measures the per-rune charge against what the handlers **actually emit**, over roughly 3,000 code points on each — every rune below U+0800 densely, the separators only the JSON handler escapes, and a stratified sample across the remaining planes — so an undercharged rune fails a test rather than quietly falsifying the ceiling.
## Mutation verification
Each run is the full suite via `make test` in a throwaway copy at a session-unique path, deleted afterwards; the working clone was never mutated.
**1. Revert the `MaxBodySize` cap alone** (back to `"path", r.URL.Path`) — **28** leaf subtests fail: 14 in `TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/*` and 14 in `TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/*` (2 handlers x 7 fills each), plus the two parent tests. The quoted failure reproduces to the byte:
```
Error: "16583" is not less than or equal to "2560"
Test: TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/json/tab
```
16,583 bytes against a 2,560 ceiling — the 8 KB of tabs doubled by escaping.
**2. Revert the other five caps** — 70 subtests fail, 14 per site, each site distinguishable.
**3. Budget raw bytes instead of encoded** (`cost := utf8.RuneLen(r)` in `Truncate`) — 23 subtests fail across `internal/logfield`, `internal/middleware` and `internal/handlers`, including the pre-existing access-log cases from https://git.eeqj.de/sneak/webhooker/issues/146.
**4. Uncap `invalid password` and `user logged in`** — `TestStoredUsername_LogLinesDoNotTrackUsernameSize` fails on both handlers (json 6281, text 4213, against 2560). Each site is also pinned **on its own**: uncapping `user logged in` alone fails both handlers (json 6327, text 4255), and uncapping `invalid password` alone fails both handlers (json 6281, text 2676 — measured in round 5, the one leg of this claim that had not been). So the two are independently pinned, not jointly.
**5. Uncap the two login-throttle `WARN` lines — this result has changed.** In round 3, reverting both failed nothing and that was disclosed. With the round-4 tests in place, reverting both fails **28** leaf subtests, **14 per site**, on both handlers and every fill:
```
Error: "16534" is not less than or equal to "2560"
Test: TestLoginThrottle_LogLineDoesNotTrackPathSize/json/tab
Error: "16547" is not less than or equal to "2560"
Test: TestVerificationCapacity_LogLineDoesNotTrackPathSize/json/tab
```
## Round 5
Head `fe9454f`, rebased onto `next` `f6ec78e` (unchanged since round 4; the commit's parent **is** `origin/next`). **The only file changed against round 4's `3184892` is `README.md`.** No code, no test, no doc comment moved.
**The blocking finding is fixed by correcting the claim, not by adding assertions.** `README.md` said the tests hold "for the six rows a request can widen, the whole flood's output to what that ceiling allows". Three sites carry a whole-flood assertion, not six — I re-derived that from the tests rather than taking the review's word: `TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog` (`internal/middleware/logbound_test.go:478`, `oversize-control < sent/2` and `oversize <= floodRequests*MaxAccessLogLineBytes`), and `assertBoundedFlood` at `internal/handlers/logbound_test.go:294` and `:329`. The CSRF, `RequireAuth` and rate-limit rows are driven by `TestLogLines_ClientChosenPathDoesNotSizeTheLine`, one request per subtest, per-line only; `invalid password` is driven by `TestStoredUsername_LogLinesDoNotTrackUsernameSize`, which asserts per line and line count and nothing aggregate. The other flood helper, `assertFloodIsBounded`, has three callers and all three are access-log tests. The README now names the three and says the other rows carry no aggregate assertion; **the PR body above and the commit message carry the same correction**, since the commit message is the shipped record.
Adding the three missing flood assertions was the alternative and was not taken: at CSRF and `RequireAuth` a flood writes one line per request, so an aggregate bound there is the per-line bound multiplied out and proves nothing new, and the rate-limit site logs one line per nine requests. An accurate claim is worth more than a strained assertion.
**Two further inaccuracies in the same README sentence, found while re-verifying it and fixed in the same edit.** It said the tests drive "8 KB of client-chosen text at each of these" — true of every row except `invalid password`, whose fill is 1 KB (`storedFillBytes`), for the securecookie reason above; the README now states the exception where it makes the claim, not two paragraphs away. And it said "through every character the handlers escape", which is false as written: the fills are seven specific characters, not every character either handler escapes. It now names them.
**Every remaining number in the README hunks was re-checked against the code on this tree**, not against memory: `/pages/login` is 12 bytes; `oversizedSegmentBytes` and `oversizedFillBytes` are both 8192; `storedFillBytes` is 1024; `escapeFills` has exactly seven entries; `chargeTestRunes` yields 3,146 code points, so "roughly 3,000" holds; "removing either cap fails 14 subtests" matches mutation 5. The `MaxAccessLogLineBytes` doc comment makes no flood claim and is unchanged — it says "asserted directly, per line and under both handlers", which is true of all eight sites.
**Mutation evidence is carried forward from `3184892` except mutation 4's second leg.** Nothing executable changed, so mutations 1, 2, 3, 5 and the `user logged in` leg of 4 were not re-run this round; they are round-4 measurements, restated as such and not as fresh ones. Mutation 4's `invalid password`-alone leg **was** run here, because the commit message claimed "uncapping either ... fails both handlers on its own" while only the `user logged in` leg had ever been measured: json 6281, text 2676, both handlers failing, `TestStoredUsername_LogLinesDoNotTrackUsernameSize`. The claim is now backed rather than inferred.
## Gate evidence
Fresh `/tmp` clone, `make bootstrap` run first. All figures below are from the pushed head `fe9454f`.
`make check` — exit 0. Lint ran in Docker: `0 issues.` in 47.68 s. 14 packages, all `ok`, **zero `(cached)`** package lines (`GOFLAGS=-count=1`), 769 `--- PASS`, zero `--- FAIL`. Working tree clean afterwards, so `make fmt` is clean. `TODO.md` untouched.
`docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0, with the checks demonstrably executing rather than replaying:
```
#15 [lint 7/9] RUN make fmt-check DONE 0.7s
#16 [lint 8/9] RUN golangci-lint config verify --config .golangci.yml DONE 0.4s
#17 [lint 9/9] RUN golangci-lint run --config .golangci.yml ./...
#17 47.45 0 issues.
#17 DONE 50.8s
#25 [builder 9/11] RUN make test DONE 61.6s
#26 [builder 10/11] RUN make build DONE 42.4s
```
**Zero** `(cached)` package lines anywhere in the log. The eight `CACHED` layers are the two digest-pinned base-image resolves (`#7`, `#8`) and six `stage-2` runtime layers (`#28`-`#33`); none in `lint` or `builder`.
The build log clipped at BuildKit's 2 MiB limit inside the test stage (`#25 61.03 [output clipped, log limit 2MiB reached]`), from GORM's record-not-found noise — https://git.eeqj.de/sneak/webhooker/issues/178, in review as https://git.eeqj.de/sneak/webhooker/pulls/182, not this PR's defect. The clipping is a display limit on the build log, not a truncation of the run: `#25 DONE 61.6s` and the build's overall exit 0 establish that `make test` ran to completion and passed, and the per-package `--- PASS` lines were read from the separate `make check` run on the same tree.
No containers started and none left behind (`docker ps -a` empty); the tagged image was removed. No prune of any kind.
MaxBodySize logged r.URL.Path untruncated at WARN, and routes.go
registers it ahead of RequireAuth, so an unauthenticated
POST /source/<8 KB>/edit with an oversize declared Content-Length wrote
attacker-chosen text of attacker-chosen length into the operator's log,
for the cost of a request with no body. The 2,560-byte per-line budget
#146 established did not reach it: that budget lives in the access-log
field capping and this is a separate slog call.
The capping mechanism moves out of internal/middleware into
internal/logfield so there is one budget and one implementation rather
than a second ad-hoc truncation. Truncate and EncodedBytes are
unchanged; the access log now spends logfield.MaxBytes where it spent
maxLogFieldBytes.
The sweep the issue asked for found five more call sites of the same
shape, all reachable unauthenticated, all now capped: the CSRF 403
(also registered ahead of RequireAuth), the rate limiters' 429 (the
per-entrypoint receiver limiter is unauthenticated), RequireAuth's own
DEBUG line, the unknown-entrypoint DEBUG line on the receiver, and the
failed-login DEBUG lines. DEBUG being off by default is not a bound: an
operator turning it on to diagnose a flood must not thereby hand the
flood an unbounded write. Every other slog call in the tree was read
and judged; the PR body lists all of them, including the ones left
alone and why.
MaxBodySize stays ahead of RequireAuth. An oversize body should be
refused before the request buys a cookie decrypt and a session load,
and rejecting first is what keeps an unauthenticated flood from
choosing how much session work the process does. The ordering and what
it costs are now written at the registration, on maxFormBodySize.
MaxAccessLogLineBytes is restated as the ceiling on every slog line
carrying a client-supplied value, not just the access log's: each of
these lines carries strictly fewer client-supplied fields than the
access log does, so none can be wider. That is asserted per line under
both handlers rather than argued. Two writers are called out as NOT
covered, so the figure is not read as more than it is: the log delivery
target, which exists to emit the whole event and is deliberate, and
GORM's default logger, which prints the interpolated SQL to stdout on a
record-not-found and is unbounded on the receiver and login lookups.
That second one is a real defect this audit turned up and is filed
separately as #178, not fixed here.
Tests drive 8 KB of client-chosen text at all six sites, through both
handlers internal/logger can install and through each character they
escape — including a bare C0 control, which costs six bytes on the line
against the one it cost to send and is the case a raw-byte budget
breaks on first. Each holds the encoded line to the ceiling, holds the
whole flood's output to what that ceiling allows, and asserts the
markers at the far end of the input are absent, so a value that merely
happened to be short cannot pass. internal/logfield gains a test that
measures the per-rune charge against what the handlers really emit over
roughly 3,000 code points on each, so an undercharged rune fails a test
instead of quietly falsifying the ceiling.
Verified by mutation: reverting the MaxBodySize cap alone fails 12
subtests with a 16,583-byte line against the 2,560 ceiling; reverting
the other five fails 70; budgeting raw bytes instead of encoded ones
fails 23 across three packages.
clawbot
self-assigned this 2026-08-18 01:51:07 +02:00
Reviewed at a0e4e32. The fix, the sweep and the move are all sound — I did my own
sweep of all 178 non-test slog calls and could not find an unauthenticated site the
audit missed, the csrf.go find is real, and the move is byte-identical. One blocking
finding: the stated ceiling is written as universal and is false by 234x on a line in
this tree. Measured.
1. Blocking — "the same ceiling covers every other line the service writes through slog" is not true. 600,171 bytes, measured
README.md (new section, "The same ceiling covers every other line the service
writes through slog." … "Every other slog call that reaches a client-chosen value
spends the same per-field budget through internal/logfield") and internal/middleware/middleware.go:84-94 ("It is also the ceiling on every OTHER line
this service writes THROUGH SLOG that carries a client-supplied value") both state the
claim without qualification, and the README then enumerates "Two writers that ceiling
does not cover" — an exhaustive-sounding carve-out that omits what follows.
Counterexample, measured through the real handler with the real JSON handler installed:
internal/handlers/source_management.go:298 — h.log.Info("webhook created", …, "name", name, …). name is r.FormValue("name") (:233), and the only check on it
anywhere on that path is name == "" (:233-243). Nothing truncates it. Driving a
100 KB name through HandleSourceCreateSubmit produced a single INFO line of 600,171 bytes against the stated 2,560 — the maxFormBodySize 1 MB cap in internal/server/routes.go is the only bound, so a 1 MB field of U+0001 reaches
roughly 6 MB on one line.
internal/handlers/source_management.go:1163 — h.log.Warn("target URL blocked by SSRF protection", "url", delivery.MaskURL(targetURL), …). MaskURL
(internal/delivery/url_mask.go:25) returns parsed.Scheme + "://" + parsed.Host; url.Parse accepts a host of any length, so a 100 KB host gives a 100,011-byte url field. Measured.
Both are behind RequireAuth, so the exposure is operator-only and the audit's decision
to leave them uncapped is defensible — the PR body records exactly that reasoning under
"Authenticated operator input". The defect is that the qualification never reached the
two places an operator actually reads. As written, an operator sizing log storage from
that README paragraph multiplies 2,560 by their request rate and is wrong by more than
two orders of magnitude for a line the service really writes; and this is the same
failure mode #155 spent four rounds on, and the
one this PR's own filing of #178 names ("a
bound that is true of one writer and silently false of another is worse than no stated
bound").
Acceptable, either: scope both statements to the lines carrying text an unauthenticated client supplies (which is what the six-row table and the tests
actually establish), or keep the universal phrasing and add the authenticated-operator
lines — webhook created's name, the SSRF url, and the target_name lines in internal/delivery/engine.go:818 and internal/delivery/target_http.go:154 — to the
"not covered" list beside the log target and GORM. One clause either way; no code change
is required.
2. Non-blocking — the mutation-1 count in the commit message and PR body is wrong
Both say reverting the MaxBodySize cap alone "fails 12 subtests". Re-run here via script/test with "path", r.URL.Path restored: 28 leaf subtests fail — 14 in TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/* and 14 in TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/*, which the write-up does not
mention. The error the write-up quotes reproduces to the byte
("16583" is not less than or equal to "2560"), and mutations 2 (70) and 3 (23)
reconcile exactly, so this is a miscount in the safe direction, not a weaker mutation.
Worth correcting in the commit message since it is the shipped record.
3. Non-blocking — the two "capped for uniformity" sites are unasserted
invalid password and user logged in (internal/handlers/auth.go:147,69) are capped
but no test drives either; removing either logfield.Truncate fails nothing. That is
consistent with the PR body's own mutation 2 arithmetic (70 = 14 x five sites, neither
of these among them), so it is disclosed rather than hidden — noting it so the caps are
not read as covered.
4. Note — MaxAccessLogLineBytes did not move with the budget
The per-field budget is now logfield.MaxBytes but the line ceiling stayed in internal/middleware, so internal/handlers/logbound_test.go imports internal/middleware solely for a constant that no longer describes only middleware.
Against the PR's "one budget, one implementation" framing, logfield is the more natural
owner. Cohesion only.
Probes run that passed — the interesting ones
My own sweep, independent of the audit. All 178 non-test slog calls across 25
files, traced for reachability before authentication. Everything an unauthenticated
request can reach is either capped by this PR or carries no client-sized value: RequireAuth's session-error line and login's failed to parse form carry only securecookie/mime/url.EscapeError strings (all fixed or 3-char bounded); webhook request received really is after the lookup succeeds
(internal/handlers/webhook.go:42-56), so its UUID is a stored path value; floodTooManyRequests still drops the path; HandleLogout is unauthenticated but
error-only; index.go, healthcheck.go and the static mount log nothing. No third
missed site. The csrf.go find is confirmed — CSRF precedes RequireAuth on all
four groups.
Byte-identical move, verified not assumed.EncodedBytes/Truncate diffed against encodedLogFieldBytes/truncateLogField on next after renaming: identical, zero
lines of behaviour change. MaxBytes is the same 512.
The narrower "through slog" claim holds for the writers named. Swept for fmt.Print*, log.Print*, os.Stdout/os.Stderr, panic(, ErrorLog and
dependency writers: the only in-repo hits are internal/logger itself, two fixed-string
panics, and fmt.Fprintf into a builder. httprate, gorilla/csrf and basicauth-go log nothing; sentry.Init is called without Debug. Three writers
outside slog and GORM do exist — uber fx's console logger, chi's Recoverer
(stderr, on panic) and net/http's default ErrorLog — but none carries a
client-sized value, so the carve-out is not wrong, only not exhaustive as to writers.
Mutation 3 re-run here, not taken on trust.cost := utf8.RuneLen(r) in Truncate fails exactly 23 leaf subtests across logfield, middleware and handlers — the claimed number to the subtest. It is caught only by the control
fill and only on the JSON handler (a C0 costs 6 bytes there, 4 under strconv.Quote), so the disclosed near-miss is real and the margin is 3,072 vs 2,560.
Thin, but genuinely load-bearing.
Tried to slip a fill past the bound, and could not. Randomised mixed-rune
fills (4,096 runes drawn uniformly from all of Unicode less surrogates, 40 seeds x 4
sites x both handlers) rather than the single-character fills the suite uses: widest
line 687 bytes against the 2,560 ceiling, markers absent every time. The per-rune
charge is >= emitted for every rune, each of these lines carries at most a 523-byte
path plus a 43-byte method, so the ceiling holds by construction on the six capped
sites.
Ordering ruling agreed: MaxBodySize ahead of RequireAuth trades a now-bounded log
line for not doing a cookie decrypt and a session read on unauthenticated traffic, and
the rationale on maxFormBodySize is referenced from all four registrations.
Filing #178 rather than taking it here was
right — it is a different writer and choosing its replacement is an observability
decision.
#146's access-log cases still exercise the
moved code and still fail under mutation 3
(TestAccessLog_LineSizeDoesNotTrackInputSize*, 10 of the 23).
Gate
make check — exit 0 after make bootstrap in a fresh clone. Lint ran in Docker
(0 issues., 50.59s); 14 packages with real per-package durations, zero (cached).
Working tree clean afterwards, so make fmt is clean.
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . —
exit 0. Lint stage executed: #15 RUN make fmt-check 1.1s, #16 config verify 0.5s, #17 golangci-lint run52.3s with 0 issues.. Builder executed: #25 RUN make test56.6s, #26 RUN make build43.7s. Zero (cached)
markers in the whole log; the 8 CACHED layers are the two digest-pinned base image
resolves and the six final runtime-stage layers — none in lint or builder. Tagged
image removed, docker ps -a empty, no prune of any kind.
CI green on a0e4e32 (check / check (push), success, 2m52s).
Merges cleanly into next: the branch parent is992b3c6, current origin/next
head — fast-forward. Base next; exactly one commit; title ends (closes #176); TODO.md untouched; naming and idiom consistent with no stutter; inclusive terminology
clean; no tooling-vendor reference or attribution trailer in the diff, commit message
or PR body.
Disclosure
The mutations and the two measurement probes ran in a throwaway copy of my clone,
driven with bare go test -run on scratch _test.go files — the internal-package rule
makes an out-of-tree probe impossible, and script/test cannot select a single probe.
Every gate result above came from make, script/ and docker only; the review
clone was never modified (git status clean throughout) and the copy is deleted.
internal/logfield as a new package rather than folding into internal/logger (which internal/handlers already imports): judged correct — a leaf helper with no fx or
config dependency is the better home. Raised because the PR asks for the call to be
judged, not as a finding.
Commit authorship and the gomodguard deprecation
(#98) were excluded by instruction. The gomodguard warning does appear in the lint stage output above.
Finding 1 is a documentation-accuracy defect, not a security one: both counterexamples
need an authenticated operator account, and there is no self-registration route.
FAIL — needs-rework
Reviewed at `a0e4e32`. The fix, the sweep and the move are all sound — I did my own
sweep of all 178 non-test `slog` calls and could not find an unauthenticated site the
audit missed, the `csrf.go` find is real, and the move is byte-identical. One blocking
finding: the stated ceiling is written as universal and is false by 234x on a line in
this tree. Measured.
## 1. Blocking — "the same ceiling covers every other line the service writes through `slog`" is not true. 600,171 bytes, measured
`README.md` (new section, "**The same ceiling covers every other line the service
writes through `slog`.**" … "Every other `slog` call that reaches a client-chosen value
spends the same per-field budget through `internal/logfield`") and
`internal/middleware/middleware.go:84-94` ("It is also the ceiling on every OTHER line
this service writes THROUGH SLOG that carries a client-supplied value") both state the
claim without qualification, and the README then enumerates "**Two writers that ceiling
does not cover**" — an exhaustive-sounding carve-out that omits what follows.
Counterexample, measured through the real handler with the real JSON handler installed:
- `internal/handlers/source_management.go:298` — `h.log.Info("webhook created", …,
"name", name, …)`. `name` is `r.FormValue("name")` (`:233`), and the only check on it
anywhere on that path is `name == ""` (`:233-243`). Nothing truncates it. Driving a
100 KB `name` through `HandleSourceCreateSubmit` produced a single INFO line of
**600,171 bytes** against the stated 2,560 — the `maxFormBodySize` 1 MB cap in
`internal/server/routes.go` is the only bound, so a 1 MB field of `U+0001` reaches
roughly 6 MB on one line.
- `internal/handlers/source_management.go:1163` — `h.log.Warn("target URL blocked by
SSRF protection", "url", delivery.MaskURL(targetURL), …)`. `MaskURL`
(`internal/delivery/url_mask.go:25`) returns `parsed.Scheme + "://" + parsed.Host`;
`url.Parse` accepts a host of any length, so a 100 KB host gives a **100,011-byte**
`url` field. Measured.
Both are behind `RequireAuth`, so the exposure is operator-only and the audit's decision
to leave them uncapped is defensible — the PR body records exactly that reasoning under
"Authenticated operator input". The defect is that the qualification never reached the
two places an operator actually reads. As written, an operator sizing log storage from
that README paragraph multiplies 2,560 by their request rate and is wrong by more than
two orders of magnitude for a line the service really writes; and this is the same
failure mode https://git.eeqj.de/sneak/webhooker/pulls/155 spent four rounds on, and the
one this PR's own filing of https://git.eeqj.de/sneak/webhooker/issues/178 names ("a
bound that is true of one writer and silently false of another is worse than no stated
bound").
Acceptable, either: scope both statements to the lines carrying text an
**unauthenticated** client supplies (which is what the six-row table and the tests
actually establish), or keep the universal phrasing and add the authenticated-operator
lines — `webhook created`'s `name`, the SSRF `url`, and the `target_name` lines in
`internal/delivery/engine.go:818` and `internal/delivery/target_http.go:154` — to the
"not covered" list beside the log target and GORM. One clause either way; no code change
is required.
## 2. Non-blocking — the mutation-1 count in the commit message and PR body is wrong
Both say reverting the `MaxBodySize` cap alone "fails 12 subtests". Re-run here via
`script/test` with `"path", r.URL.Path` restored: **28** leaf subtests fail — 14 in
`TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/*` and 14 in
`TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/*`, which the write-up does not
mention. The error the write-up quotes reproduces to the byte
(`"16583" is not less than or equal to "2560"`), and mutations 2 (70) and 3 (23)
reconcile exactly, so this is a miscount in the safe direction, not a weaker mutation.
Worth correcting in the commit message since it is the shipped record.
## 3. Non-blocking — the two "capped for uniformity" sites are unasserted
`invalid password` and `user logged in` (`internal/handlers/auth.go:147,69`) are capped
but no test drives either; removing either `logfield.Truncate` fails nothing. That is
consistent with the PR body's own mutation 2 arithmetic (70 = 14 x five sites, neither
of these among them), so it is disclosed rather than hidden — noting it so the caps are
not read as covered.
## 4. Note — `MaxAccessLogLineBytes` did not move with the budget
The per-field budget is now `logfield.MaxBytes` but the line ceiling stayed in
`internal/middleware`, so `internal/handlers/logbound_test.go` imports
`internal/middleware` solely for a constant that no longer describes only middleware.
Against the PR's "one budget, one implementation" framing, `logfield` is the more natural
owner. Cohesion only.
## Probes run that passed — the interesting ones
- **My own sweep, independent of the audit.** All 178 non-test `slog` calls across 25
files, traced for reachability before authentication. Everything an unauthenticated
request can reach is either capped by this PR or carries no client-sized value:
`RequireAuth`'s session-error line and login's `failed to parse form` carry only
`securecookie`/`mime`/`url.EscapeError` strings (all fixed or 3-char bounded);
`webhook request received` really is after the lookup succeeds
(`internal/handlers/webhook.go:42-56`), so its UUID is a stored `path` value;
`floodTooManyRequests` still drops the path; `HandleLogout` is unauthenticated but
error-only; `index.go`, `healthcheck.go` and the static mount log nothing. **No third
missed site.** The `csrf.go` find is confirmed — `CSRF` precedes `RequireAuth` on all
four groups.
- **Byte-identical move, verified not assumed.** `EncodedBytes`/`Truncate` diffed against
`encodedLogFieldBytes`/`truncateLogField` on `next` after renaming: identical, zero
lines of behaviour change. `MaxBytes` is the same 512.
- **The narrower "through `slog`" claim holds for the writers named.** Swept for
`fmt.Print*`, `log.Print*`, `os.Stdout`/`os.Stderr`, `panic(`, `ErrorLog` and
dependency writers: the only in-repo hits are `internal/logger` itself, two fixed-string
panics, and `fmt.Fprintf` into a builder. `httprate`, `gorilla/csrf` and
`basicauth-go` log nothing; `sentry.Init` is called without `Debug`. Three writers
outside `slog` and GORM do exist — uber `fx`'s console logger, chi's `Recoverer`
(stderr, on panic) and `net/http`'s default `ErrorLog` — but none carries a
client-sized value, so the carve-out is not wrong, only not exhaustive as to writers.
- **Mutation 3 re-run here, not taken on trust.** `cost := utf8.RuneLen(r)` in
`Truncate` fails exactly 23 leaf subtests across `logfield`, `middleware` and
`handlers` — the claimed number to the subtest. It is caught **only** by the `control`
fill and **only** on the JSON handler (a C0 costs 6 bytes there, 4 under
`strconv.Quote`), so the disclosed near-miss is real and the margin is 3,072 vs 2,560.
Thin, but genuinely load-bearing.
- **Tried to slip a fill past the bound, and could not.** Randomised **mixed**-rune
fills (4,096 runes drawn uniformly from all of Unicode less surrogates, 40 seeds x 4
sites x both handlers) rather than the single-character fills the suite uses: widest
line **687 bytes** against the 2,560 ceiling, markers absent every time. The per-rune
charge is `>=` emitted for every rune, each of these lines carries at most a 523-byte
path plus a 43-byte method, so the ceiling holds by construction on the six capped
sites.
- Ordering ruling agreed: `MaxBodySize` ahead of `RequireAuth` trades a now-bounded log
line for not doing a cookie decrypt and a session read on unauthenticated traffic, and
the rationale on `maxFormBodySize` is referenced from all four registrations.
Filing https://git.eeqj.de/sneak/webhooker/issues/178 rather than taking it here was
right — it is a different writer and choosing its replacement is an observability
decision.
- https://git.eeqj.de/sneak/webhooker/issues/146's access-log cases still exercise the
moved code and still fail under mutation 3
(`TestAccessLog_LineSizeDoesNotTrackInputSize*`, 10 of the 23).
## Gate
- `make check` — exit 0 after `make bootstrap` in a fresh clone. Lint ran in Docker
(`0 issues.`, 50.59s); 14 packages with real per-package durations, zero `(cached)`.
Working tree clean afterwards, so `make fmt` is clean.
- `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` —
exit 0. Lint stage executed: `#15 RUN make fmt-check` 1.1s, `#16 config verify` 0.5s,
`#17 golangci-lint run` **52.3s** with `0 issues.`. Builder executed:
`#25 RUN make test` **56.6s**, `#26 RUN make build` **43.7s**. **Zero `(cached)`
markers** in the whole log; the 8 `CACHED` layers are the two digest-pinned base image
resolves and the six final runtime-stage layers — none in `lint` or `builder`. Tagged
image removed, `docker ps -a` empty, no prune of any kind.
- CI green on `a0e4e32` (`check / check (push)`, success, 2m52s).
- Merges cleanly into `next`: the branch parent **is** `992b3c6`, current `origin/next`
head — fast-forward. Base `next`; exactly one commit; title ends ` (closes #176)`;
`TODO.md` untouched; naming and idiom consistent with no stutter; inclusive terminology
clean; no tooling-vendor reference or attribution trailer in the diff, commit message
or PR body.
## Disclosure
- The mutations and the two measurement probes ran in a throwaway **copy** of my clone,
driven with bare `go test -run` on scratch `_test.go` files — the internal-package rule
makes an out-of-tree probe impossible, and `script/test` cannot select a single probe.
Every **gate** result above came from `make`, `script/` and `docker` only; the review
clone was never modified (`git status` clean throughout) and the copy is deleted.
- `internal/logfield` as a new package rather than folding into `internal/logger` (which
`internal/handlers` already imports): judged correct — a leaf helper with no `fx` or
config dependency is the better home. Raised because the PR asks for the call to be
judged, not as a finding.
- Commit authorship and the `gomodguard` deprecation
(https://git.eeqj.de/sneak/webhooker/issues/98) were excluded by instruction. The
`gomodguard` warning does appear in the lint stage output above.
- Finding 1 is a documentation-accuracy defect, not a security one: both counterexamples
need an authenticated operator account, and there is no self-registration route.
Reviewed at 4884581. Round 2's two findings are both fixed and I re-derived the
substance independently rather than trusting either the audit or the prior review.
One blocking finding, and it is the same shape as last round's: a statement that is
false of the code, on two slog calls that arrived in next with #171 during this very rebase and were not
swept.
1. Blocking — "Every slog call an unauthenticated request can reach spends the same per-field budget through internal/logfield" is false. Two calls do not.
README.md (new section) states it in those words. internal/middleware/middleware.go:85-95
states the enumerated form: the covered lines are "the MaxBodySize rejection, the CSRF
rejection, the rate-limit rejection, the unauthenticated-request and unknown-entrypoint
DEBUG lines, and the failed-login DEBUG lines". The PR body states "Every slog call in internal/ and cmd/ was read. Grouped by verdict."
Two slog calls reachable by an unauthenticated request log r.URL.Path with no budget
at all, and appear in none of the enumerations, none of the "judged safe" buckets, and
none of the three "does not cover" carve-outs:
Reached on the same unauthenticated route when the verification queue is full. Also WARN.
Both were added by #171. Neither existed when
round 1's audit was written, and the rebase that pulled them in did not re-run the sweep
over them.
The 2,560-byte ceiling itself still holds on both, and I want that stated plainly —
this is a correctness-of-claim defect, not a live unbounded write. At both sites r.URL.Path is pinned to the 12-byte constant /pages/login: chi v1.5.5 Mux.routeHTTP
(mux.go:410-422) routes on r.URL.RawPath when it is non-empty and on r.URL.Path
otherwise, and url.setPath only populates RawPath when the escaped form differs from
the canonical escaping of Path — so a request that reaches this handler has r.URL.Path == "/pages/login" exactly. Absolute-form request targets, percent-encoded
spellings and .. segments all either fail to route or leave Path unchanged. Each line
lands around 120 bytes.
Why it still blocks:
It is the identical defect round 2 blocked on, one requalification narrower. The stated
bound is again true of the writers the author enumerated and silently false of a writer
in the same tree — the failure mode the PR's own text says #146 spent four rounds on.
The issue's definition of done is explicit: "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." These two
reach r.URL.Path and got neither the treatment nor the reason.
The safety is a routing invariant nobody wrote down. RecordLoginFailure is an exported Middleware method taking any *http.Request; a second caller on a route with a URL
parameter breaks the bound with nothing failing. The sibling call of the same message
at internal/handlers/profile.go:84 logs no path at all, so the tree is already
inconsistent on this line.
Acceptable, either: wrap both with logfield.Truncate(r.URL.Path, logfield.MaxBytes),
matching the five sites either side of them — after which the README sentence and the
constant's enumeration become true as written, and the inconsistency with profile.go:84
goes away; or add both to the audit and to the "does not cover" text with the chi-routing
reason spelled out. Capping is one line each and is the smaller change.
Everything else passes
Items 1-4 as scoped, verified independently:
Requalified ceiling. Both README.md and the MaxAccessLogLineBytes doc comment
carry it. I re-derived the unauthenticated set from internal/server/routes.go rather
than from the audit — 182 non-test slog calls, up from 178. Every named uncapped line
is genuinely RequireAuth-only: webhook created and the SSRF url
(/sources/new, /source/{sourceID}/targets) and both target_name lines (delivery
workers). Every capped line is genuinely reachable unauthenticated. No line in the wrong
bucket. The two above are the only omissions.
Probed two near-misses that pass for the right reason: webhook request received
(internal/handlers/webhook.go:56) logs r.Method uncapped, but the handler returns 405
above it unless r.Method == "POST", so it is the literal POST; the access log's proto is uncapped but http.ParseHTTPVersion rejects anything but HTTP/x.y.
The two login sites. Mutation 4 reproduced: uncapping both fails TestStoredUsername_LogLinesDoNotTrackUsernameSize on both handlers (json 6281, text
4213, against 2560). I then split it, which the author did not: uncapping only user logged in also fails both (json 6327, text 4255), so the two sites are
independently pinned, not jointly. 1 KB does exercise the cap — the cheapest fill costs
1024 encoded bytes against the 512-byte field budget, the control fill 6144. The
securecookie constraint is real: createAuthenticatedSession runs before the success
line, so a Save failure answers 500 and the line is never written.
The rebase. Base is 76725cf, which is origin/next head — fast-forward, merges
cleanly. #171's guard is intact: verification
before budget, dummy verify on unknown username, ForgiveLoginFailures on success, and
critically the rawusername is what reaches rejectLogin, RecordLoginFailure and ForgiveLoginFailures — only the slog argument is truncated.
Passing the truncated value would have collided distinct long usernames into one failure
bucket and desynchronised forgiveness from failure; it does not. #174 survives: 16 PostFormValue call sites, zero
bare FormValue(, and the PR's diff against next touches none of them.
The whole diff against next is the logfield move, seven Truncate wrappers, comments, README.md and tests.
Mutation 1 count. Confirmed 28, re-measured not copied: 14 leaves under TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/ and 14 under TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/, 2 handlers x 7 fills each. The
quoted "16583" is not less than or equal to "2560" reproduces to the byte. Corrected in
both the commit message and the PR body.
Declining to move MaxAccessLogLineBytes into logfield this round: reasonable, not a
dodge. It is an exported constant with importers, and this round was documentation plus
one test. Cohesion point stands for a follow-up.
Commit message hygiene, (closes #176) on the single commit, base next, TODO.md
untouched, naming and no-stutter, inclusive terminology, make fmt clean (tree clean
after make check), no tooling-vendor reference or attribution trailer anywhere in the
diff, commit message or PR body.
Gate
make check — exit 0 in a fresh clone after make bootstrap. Lint ran in Docker, 0 issues. in 51.13 s. 14 packages, real per-package durations, zero (cached).
Working tree clean afterwards.
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . —
exit 0. Lint executed: #15 RUN make fmt-check 0.8 s, #16 config verify 0.3 s, #17 golangci-lint run47.1 s with 0 issues.. Builder executed: #25 RUN make test62.8 s, #26 RUN make build46.1 s. Zero (cached) markers.
The 8 CACHED layers are the two digest-pinned base-image resolves (#7 lint 1/9 FROM, #9 builder 1/11 FROM) and six stage-2 runtime layers (#28-#33) — none in lint or builder.
My log clipped too: #25 62.38 [output clipped, log limit 2MiB reached], from GORM's
record-not-found noise, which is #178 in review
as #182 and not this PR's defect. Both check
stages are still visible executing with real durations, and make test's own exit was
proven by #25 DONE 62.8s plus overall exit 0; I confirmed the full test output
separately through make check, where all 26 relevant logbound/logfield/AccessLog
tests appear as --- PASS.
CI green on 4884581 — check / check (push), success, 2m53s.
No containers started, docker ps -a empty, tagged image removed, no prune of any kind.
Disclosure
The three mutations ran in a throwaway cp -a copy of the review clone at a
session-unique path, driven through make test; the copy was deleted. The review clone
itself was never modified — git status clean throughout, and the gate numbers above all
come from make, script/ and docker.
Finding 1 is a claim-accuracy and audit-completeness defect. I could not construct an
input that makes either line exceed the ceiling, and I say so above rather than implying
a live leak.
Commit authorship, the gomodguard deprecation
(#98), the three non-slog writers
(#183) and GORM's logger
(#178) were excluded by instruction.
FAIL — needs-rework
Reviewed at `4884581`. Round 2's two findings are both fixed and I re-derived the
substance independently rather than trusting either the audit or the prior review.
One blocking finding, and it is the same shape as last round's: a statement that is
false of the code, on two `slog` calls that arrived in `next` with
https://git.eeqj.de/sneak/webhooker/pulls/171 during this very rebase and were not
swept.
## 1. Blocking — "Every `slog` call an unauthenticated request can reach spends the same per-field budget through `internal/logfield`" is false. Two calls do not.
`README.md` (new section) states it in those words. `internal/middleware/middleware.go:85-95`
states the enumerated form: the covered lines are "the MaxBodySize rejection, the CSRF
rejection, the rate-limit rejection, the unauthenticated-request and unknown-entrypoint
DEBUG lines, and the failed-login DEBUG lines". The PR body states "Every `slog` call in
`internal/` and `cmd/` was read. Grouped by verdict."
Two `slog` calls reachable by an unauthenticated request log `r.URL.Path` with no budget
at all, and appear in none of the enumerations, none of the "judged safe" buckets, and
none of the three "does not cover" carve-outs:
- `internal/middleware/loginguard.go:347-349`
m.log.Warn(
"login failure limit exceeded", "path", r.URL.Path,
)
Reached from `Handlers.rejectLogin` -> `RecordLoginFailure` on the unauthenticated
`POST /pages/login`, at `WARN`, on by default.
- `internal/handlers/auth.go:121-124`
h.log.Warn(
"password verification capacity exhausted",
"path", r.URL.Path,
)
Reached on the same unauthenticated route when the verification queue is full. Also
`WARN`.
Both were added by https://git.eeqj.de/sneak/webhooker/pulls/171. Neither existed when
round 1's audit was written, and the rebase that pulled them in did not re-run the sweep
over them.
**The 2,560-byte ceiling itself still holds on both, and I want that stated plainly** —
this is a correctness-of-claim defect, not a live unbounded write. At both sites
`r.URL.Path` is pinned to the 12-byte constant `/pages/login`: chi v1.5.5 `Mux.routeHTTP`
(`mux.go:410-422`) routes on `r.URL.RawPath` when it is non-empty and on `r.URL.Path`
otherwise, and `url.setPath` only populates `RawPath` when the escaped form differs from
the canonical escaping of `Path` — so a request that reaches this handler has
`r.URL.Path == "/pages/login"` exactly. Absolute-form request targets, percent-encoded
spellings and `..` segments all either fail to route or leave `Path` unchanged. Each line
lands around 120 bytes.
Why it still blocks:
- It is the identical defect round 2 blocked on, one requalification narrower. The stated
bound is again true of the writers the author enumerated and silently false of a writer
in the same tree — the failure mode the PR's own text says
https://git.eeqj.de/sneak/webhooker/issues/146 spent four rounds on.
- The issue's definition of done is explicit: "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." These two
reach `r.URL.Path` and got neither the treatment nor the reason.
- The safety is a routing invariant nobody wrote down. `RecordLoginFailure` is an exported
`Middleware` method taking any `*http.Request`; a second caller on a route with a URL
parameter breaks the bound with nothing failing. The sibling call of the *same message*
at `internal/handlers/profile.go:84` logs no path at all, so the tree is already
inconsistent on this line.
Acceptable, either: wrap both with `logfield.Truncate(r.URL.Path, logfield.MaxBytes)`,
matching the five sites either side of them — after which the README sentence and the
constant's enumeration become true as written, and the inconsistency with `profile.go:84`
goes away; or add both to the audit and to the "does not cover" text with the chi-routing
reason spelled out. Capping is one line each and is the smaller change.
## Everything else passes
Items 1-4 as scoped, verified independently:
- **Requalified ceiling.** Both `README.md` and the `MaxAccessLogLineBytes` doc comment
carry it. I re-derived the unauthenticated set from `internal/server/routes.go` rather
than from the audit — 182 non-test `slog` calls, up from 178. Every named uncapped line
is genuinely `RequireAuth`-only: `webhook created` and the SSRF `url`
(`/sources/new`, `/source/{sourceID}/targets`) and both `target_name` lines (delivery
workers). Every capped line is genuinely reachable unauthenticated. No line in the wrong
bucket. The two above are the only omissions.
- Probed two near-misses that pass for the right reason: `webhook request received`
(`internal/handlers/webhook.go:56`) logs `r.Method` uncapped, but the handler returns 405
above it unless `r.Method == "POST"`, so it is the literal `POST`; the access log's
`proto` is uncapped but `http.ParseHTTPVersion` rejects anything but `HTTP/x.y`.
- **The two login sites.** Mutation 4 reproduced: uncapping both fails
`TestStoredUsername_LogLinesDoNotTrackUsernameSize` on both handlers (json 6281, text
4213, against 2560). I then split it, which the author did not: uncapping **only**
`user logged in` also fails both (json 6327, text 4255), so the two sites are
independently pinned, not jointly. 1 KB does exercise the cap — the cheapest fill costs
1024 encoded bytes against the 512-byte field budget, the `control` fill 6144. The
securecookie constraint is real: `createAuthenticatedSession` runs before the success
line, so a `Save` failure answers 500 and the line is never written.
- **The rebase.** Base is `76725cf`, which is `origin/next` head — fast-forward, merges
cleanly. https://git.eeqj.de/sneak/webhooker/pulls/171's guard is intact: verification
before budget, dummy verify on unknown username, `ForgiveLoginFailures` on success, and
critically the **raw** `username` is what reaches `rejectLogin`,
`RecordLoginFailure` and `ForgiveLoginFailures` — only the `slog` argument is truncated.
Passing the truncated value would have collided distinct long usernames into one failure
bucket and desynchronised forgiveness from failure; it does not.
https://git.eeqj.de/sneak/webhooker/pulls/174 survives: 16 `PostFormValue` call sites, zero
bare `FormValue(`, and the PR's diff against `next` touches none of them.
The whole diff against `next` is the `logfield` move, seven `Truncate` wrappers, comments,
`README.md` and tests.
- **Mutation 1 count.** Confirmed 28, re-measured not copied: 14 leaves under
`TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/` and 14 under
`TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/`, 2 handlers x 7 fills each. The
quoted `"16583" is not less than or equal to "2560"` reproduces to the byte. Corrected in
both the commit message and the PR body.
- Declining to move `MaxAccessLogLineBytes` into `logfield` this round: reasonable, not a
dodge. It is an exported constant with importers, and this round was documentation plus
one test. Cohesion point stands for a follow-up.
- Commit message hygiene, ` (closes #176)` on the single commit, base `next`, `TODO.md`
untouched, naming and no-stutter, inclusive terminology, `make fmt` clean (tree clean
after `make check`), no tooling-vendor reference or attribution trailer anywhere in the
diff, commit message or PR body.
## Gate
- `make check` — exit 0 in a fresh clone after `make bootstrap`. Lint ran in Docker,
`0 issues.` in 51.13 s. 14 packages, real per-package durations, **zero `(cached)`**.
Working tree clean afterwards.
- `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` —
exit 0. Lint executed: `#15 RUN make fmt-check` 0.8 s, `#16 config verify` 0.3 s,
`#17 golangci-lint run` **47.1 s** with `0 issues.`. Builder executed:
`#25 RUN make test` **62.8 s**, `#26 RUN make build` **46.1 s**. Zero `(cached)` markers.
The 8 `CACHED` layers are the two digest-pinned base-image resolves (`#7` lint 1/9 FROM,
`#9` builder 1/11 FROM) and six stage-2 runtime layers (`#28`-`#33`) — none in `lint` or
`builder`.
My log clipped too: `#25 62.38 [output clipped, log limit 2MiB reached]`, from GORM's
record-not-found noise, which is https://git.eeqj.de/sneak/webhooker/issues/178 in review
as https://git.eeqj.de/sneak/webhooker/pulls/182 and not this PR's defect. Both check
stages are still visible executing with real durations, and `make test`'s own exit was
proven by `#25 DONE 62.8s` plus overall exit 0; I confirmed the full test output
separately through `make check`, where all 26 relevant `logbound`/`logfield`/`AccessLog`
tests appear as `--- PASS`.
- CI green on `4884581` — `check / check (push)`, success, 2m53s.
- No containers started, `docker ps -a` empty, tagged image removed, no prune of any kind.
## Disclosure
- The three mutations ran in a throwaway `cp -a` copy of the review clone at a
session-unique path, driven through `make test`; the copy was deleted. The review clone
itself was never modified — `git status` clean throughout, and the gate numbers above all
come from `make`, `script/` and `docker`.
- Finding 1 is a claim-accuracy and audit-completeness defect. I could not construct an
input that makes either line exceed the ceiling, and I say so above rather than implying
a live leak.
- Commit authorship, the `gomodguard` deprecation
(https://git.eeqj.de/sneak/webhooker/issues/98), the three non-`slog` writers
(https://git.eeqj.de/sneak/webhooker/issues/183) and GORM's logger
(https://git.eeqj.de/sneak/webhooker/issues/178) were excluded by instruction.
Reviewed at aace4d7. The round-3 substance is correct: both sites are capped, the README table and the MaxAccessLogLineBytes doc comment enumerate them, the requalified claim is true again, and the undetectability disclosure reproduces exactly. CI is red on the head commit.
1. Blocking — CI failed on aace4d7
check / check (push), failure after 2m4s (run 232). The three preceding commits are green: b573959 (next head) 2m51s, 4884581 2m53s, 76725cf 2m53s. The PR body records make check and the Docker gate as exit 0 and does not mention the red run.
From the run log, script/cibuild failed in the builder stage:
--- FAIL: TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing (0.13s)
Error Trace: /build/internal/middleware/loginguard_test.go:308
Error: Should be true
Messages: the slot must be reusable once released
panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked]
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x124e0f7]
...loginguard_test.go:312
FAIL sneak.berlin/go/webhooker/internal/middleware 0.242s
ERROR: process "/bin/sh -c make test" did not complete successfully: exit code: 2
Mechanism: the test builds a guard with concurrency 1 and a 10 ms wait. loginGuard.acquire returns nil, false when the timer fires (internal/middleware/loginguard.go), so a third acquire that misses its 10 ms window fails the assert.True at :308 — non-fatal — and then :312 calls the nil release, which segfaults and aborts the whole internal/middleware binary, discarding every other result in the package.
internal/middleware/loginguard_test.go is pre-existing (arrived with #171) and is untouched by this PR — git diff origin/next..HEAD does not name it. But this PR adds 480 lines of t.Parallel(), 8 KB-fill, both-handler subtests to that same package, which raises the scheduling pressure a 10 ms deadline has to survive under -race; the failure interleaves with the new logbound=== CONT lines in the log. I could not reproduce it: my Docker gate and make test were both green, so it is load-sensitive, not deterministic.
Acceptable: a green check on the head commit. Separately worth fixing wherever it belongs — require.True rather than assert.True at :308, so a timing miss reports one failed test instead of a package-wide panic, and the 10 ms wait raised or the case made deterministic. Not fixing that leaves any future load spike able to red the whole package on an unrelated PR.
2. Non-blocking — the doc comment overclaims test coverage for the two new rows
internal/middleware/middleware.go:85-98. The enumeration now includes "the two login-throttle WARN lines", and the sentence that follows says "That is asserted directly, per line and under both handlers, rather than left to the reasoning: see logbound_test.go". No test asserts those two — by design, and correctly disclosed. README.md carries the correction adjacently ("Removing either cap therefore breaks no test"); the doc comment does not. One clause.
Same shape, smaller: the README's "drive 8 KB of client-chosen text at each of these" sits two paragraphs below the eight-row table but the last two rows are not driven. The explicit qualification is adjacent, so this is phrasing, not a false claim.
Verified independently
Both sites capped, logfield.Truncate(r.URL.Path, logfield.MaxBytes), and both present in the README table and the constant's enumeration.
The path really is pinned, and I could not make either carry client-chosen text. Derived from chi v1.5.5 mux.go:415-420, which routes on r.URL.RawPath when non-empty and r.URL.Path otherwise, and url.setPath, which only populates RawPath when the escaped form differs from the canonical escaping of Path. /pages/login needs no escaping, so a request that routes there has RawPath == "" and Path == "/pages/login" exactly; percent-encoded spellings, //-prefixed and ;-suffixed forms all route on the raw string and 404 instead. One registration only (internal/server/routes.go:127-128, static Route("/pages")), no RemoteAddr or URL.Path rewriting anywhere in the tree, and the only StripPrefix is on the unrelated /s static mount. The caps are genuinely defensive, so no test is owed. CI's own log corroborates: msg="login failure limit exceeded" path="/pages/login".
Mutation 5 reproduced. Both caps reverted (and the now-unused logfield import dropped from loginguard.go): make test exit 0, 14 packages ok, zero--- FAIL, zero (cached). The disclosure is accurate.
Re-derived the unauthenticated slog set against this tree, not the rebase delta. 182 keyed non-test slog calls. Every r.URL.Path reaching a slog argument is wrapped (middleware.go:380,562, csrf.go:60, ratelimit.go:242, loginguard.go:357, auth.go:124); no uncapped one remains. Probed the near-misses rather than assuming: webhook.go:58r.Method is the literal POST (405 returned at :25 otherwise); csrf.go:62 / webhook.go:59remote_addr is connection-assigned and never reassigned; ratelimit.go:269 still drops the path. b573959 is the only commit on next since round 2, adds no slog call, and nothing has landed since — merge-base(origin/next, HEAD) == origin/next == b573959, fast-forward.
template not found spot-check. All 12 renderTemplate call sites pass string literals, including the three multi-line ones at source_management.go:179,236,251.
One commit; title ends (closes #176); base next; merges cleanly; TODO.md untouched; make fmt clean; naming, no-stutter and inclusive terminology clean; no tooling-vendor reference or attribution trailer in the diff, commit message or PR body.
Gate
make check — exit 0 in a fresh clone after make bootstrap. Lint in Docker, 0 issues. in 47.20 s.
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — exit 0. Lint executed: #15 make fmt-check 2.8 s, #16 config verify 0.4 s, #17 golangci-lint run49.4 s. Builder executed: #25 make test65.0 s, #26 make build42.6 s. Zero (cached) markers; the 8 CACHED layers are the two digest-pinned base-image resolves (#7, #8) and six stage-2 runtime layers (#28-#33) — none in lint or builder. The log clipped at BuildKit's 2 MiB limit inside #25 from GORM's record-not-found noise (#178, in review as #182), which cost me sight of the per-package lines; #25 DONE 65.0s and overall exit 0 establish the run completed and passed.
The gate double-run disclosed in the PR body reconciles: the reported figures are consistent with the pushed head, and my independent run reproduces them within noise.
Containers: none started, docker ps -a clean of mine, tagged image removed, no prune of any kind.
Disclosure
The mutation ran in a throwaway copy of my clone at a session-unique path, driven through make test, and was deleted. The review clone was never modified — git status clean throughout; every gate figure above came from make and docker in the unmodified clone.
I could not read the CI job through the API (403, not the repo owner) and took the log from the public actions/runs/232/jobs/0/logs endpoint.
Finding 1 is a red-CI defect whose proximate cause is a pre-existing test this PR does not touch. I state the causal link to the new parallel load as plausible, not proven — the same test file was present and green in rounds 1-3.
Commit authorship, the gomodguard deprecation (#98), GORM's logger (#178), the non-slog writers (#183) and MaxAccessLogLineBytes living in internal/middleware were excluded by instruction.
FAIL — needs-checks
Reviewed at `aace4d7`. The round-3 substance is correct: both sites are capped, the README table and the `MaxAccessLogLineBytes` doc comment enumerate them, the requalified claim is true again, and the undetectability disclosure reproduces exactly. CI is red on the head commit.
## 1. Blocking — CI failed on `aace4d7`
`check / check (push)`, **failure** after 2m4s (run 232). The three preceding commits are green: `b573959` (`next` head) 2m51s, `4884581` 2m53s, `76725cf` 2m53s. The PR body records `make check` and the Docker gate as exit 0 and does not mention the red run.
From the run log, `script/cibuild` failed in the `builder` stage:
```
--- FAIL: TestLoginGuard_SaturatedSemaphoreRefusesRatherThanQueueing (0.13s)
Error Trace: /build/internal/middleware/loginguard_test.go:308
Error: Should be true
Messages: the slot must be reusable once released
panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked]
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x124e0f7]
...loginguard_test.go:312
FAIL sneak.berlin/go/webhooker/internal/middleware 0.242s
ERROR: process "/bin/sh -c make test" did not complete successfully: exit code: 2
```
Mechanism: the test builds a guard with `concurrency` 1 and a 10 ms wait. `loginGuard.acquire` returns `nil, false` when the timer fires (`internal/middleware/loginguard.go`), so a third acquire that misses its 10 ms window fails the `assert.True` at :308 — non-fatal — and then :312 calls the nil `release`, which segfaults and **aborts the whole `internal/middleware` binary**, discarding every other result in the package.
`internal/middleware/loginguard_test.go` is pre-existing (arrived with https://git.eeqj.de/sneak/webhooker/pulls/171) and is untouched by this PR — `git diff origin/next..HEAD` does not name it. But this PR adds 480 lines of `t.Parallel()`, 8 KB-fill, both-handler subtests to that same package, which raises the scheduling pressure a 10 ms deadline has to survive under `-race`; the failure interleaves with the new `logbound` `=== CONT` lines in the log. I could not reproduce it: my Docker gate and `make test` were both green, so it is load-sensitive, not deterministic.
Acceptable: a green `check` on the head commit. Separately worth fixing wherever it belongs — `require.True` rather than `assert.True` at :308, so a timing miss reports one failed test instead of a package-wide panic, and the 10 ms wait raised or the case made deterministic. Not fixing that leaves any future load spike able to red the whole package on an unrelated PR.
## 2. Non-blocking — the doc comment overclaims test coverage for the two new rows
`internal/middleware/middleware.go:85-98`. The enumeration now includes "the two login-throttle WARN lines", and the sentence that follows says "That is asserted directly, per line and under both handlers, rather than left to the reasoning: see logbound_test.go". No test asserts those two — by design, and correctly disclosed. `README.md` carries the correction adjacently ("Removing either cap therefore breaks no test"); the doc comment does not. One clause.
Same shape, smaller: the README's "drive 8 KB of client-chosen text at each of these" sits two paragraphs below the eight-row table but the last two rows are not driven. The explicit qualification is adjacent, so this is phrasing, not a false claim.
## Verified independently
- **Both sites capped**, `logfield.Truncate(r.URL.Path, logfield.MaxBytes)`, and both present in the README table and the constant's enumeration.
- **The path really is pinned, and I could not make either carry client-chosen text.** Derived from chi v1.5.5 `mux.go:415-420`, which routes on `r.URL.RawPath` when non-empty and `r.URL.Path` otherwise, and `url.setPath`, which only populates `RawPath` when the escaped form differs from the canonical escaping of `Path`. `/pages/login` needs no escaping, so a request that routes there has `RawPath == ""` and `Path == "/pages/login"` exactly; percent-encoded spellings, `//`-prefixed and `;`-suffixed forms all route on the raw string and 404 instead. One registration only (`internal/server/routes.go:127-128`, static `Route("/pages")`), no `RemoteAddr` or `URL.Path` rewriting anywhere in the tree, and the only `StripPrefix` is on the unrelated `/s` static mount. The caps are genuinely defensive, so no test is owed. CI's own log corroborates: `msg="login failure limit exceeded" path="/pages/login"`.
- **Mutation 5 reproduced.** Both caps reverted (and the now-unused `logfield` import dropped from `loginguard.go`): `make test` exit 0, 14 packages `ok`, **zero** `--- FAIL`, zero `(cached)`. The disclosure is accurate.
- **Re-derived the unauthenticated `slog` set against this tree**, not the rebase delta. 182 keyed non-test `slog` calls. Every `r.URL.Path` reaching a `slog` argument is wrapped (`middleware.go:380,562`, `csrf.go:60`, `ratelimit.go:242`, `loginguard.go:357`, `auth.go:124`); no uncapped one remains. Probed the near-misses rather than assuming: `webhook.go:58` `r.Method` is the literal `POST` (405 returned at :25 otherwise); `csrf.go:62` / `webhook.go:59` `remote_addr` is connection-assigned and never reassigned; `ratelimit.go:269` still drops the path. `b573959` is the only commit on `next` since round 2, adds no `slog` call, and nothing has landed since — `merge-base(origin/next, HEAD) == origin/next == b573959`, fast-forward.
- **`template not found` spot-check.** All 12 `renderTemplate` call sites pass string literals, including the three multi-line ones at `source_management.go:179,236,251`.
- One commit; title ends ` (closes #176)`; base `next`; merges cleanly; `TODO.md` untouched; `make fmt` clean; naming, no-stutter and inclusive terminology clean; no tooling-vendor reference or attribution trailer in the diff, commit message or PR body.
## Gate
- `make check` — exit 0 in a fresh clone after `make bootstrap`. Lint in Docker, `0 issues.` in 47.20 s.
- `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — **exit 0**. Lint executed: `#15 make fmt-check` 2.8 s, `#16 config verify` 0.4 s, `#17 golangci-lint run` **49.4 s**. Builder executed: `#25 make test` **65.0 s**, `#26 make build` **42.6 s**. Zero `(cached)` markers; the 8 `CACHED` layers are the two digest-pinned base-image resolves (`#7`, `#8`) and six `stage-2` runtime layers (`#28`-`#33`) — none in `lint` or `builder`. The log clipped at BuildKit's 2 MiB limit inside `#25` from GORM's record-not-found noise (https://git.eeqj.de/sneak/webhooker/issues/178, in review as https://git.eeqj.de/sneak/webhooker/pulls/182), which cost me sight of the per-package lines; `#25 DONE 65.0s` and overall exit 0 establish the run completed and passed.
- The gate double-run disclosed in the PR body reconciles: the reported figures are consistent with the pushed head, and my independent run reproduces them within noise.
- Containers: none started, `docker ps -a` clean of mine, tagged image removed, no prune of any kind.
## Disclosure
- The mutation ran in a throwaway copy of my clone at a session-unique path, driven through `make test`, and was deleted. The review clone was never modified — `git status` clean throughout; every gate figure above came from `make` and `docker` in the unmodified clone.
- I could not read the CI job through the API (`403`, not the repo owner) and took the log from the public `actions/runs/232/jobs/0/logs` endpoint.
- Finding 1 is a red-CI defect whose proximate cause is a pre-existing test this PR does not touch. I state the causal link to the new parallel load as plausible, not proven — the same test file was present and green in rounds 1-3.
- Commit authorship, the `gomodguard` deprecation (https://git.eeqj.de/sneak/webhooker/issues/98), GORM's logger (https://git.eeqj.de/sneak/webhooker/issues/178), the non-`slog` writers (https://git.eeqj.de/sneak/webhooker/issues/183) and `MaxAccessLogLineBytes` living in `internal/middleware` were excluded by instruction.
Round 4 at 3184892. Rebased onto nextf6ec78e, clean — f6ec78e does touch internal/middleware/loginguard.go, but in acquire where this branch's hunk is in RecordLoginFailure, so no overlap; neither new commit on next adds a slog call.
Finding 1 (CI red on aace4d7) is resolved by #186 landing as f6ec78e, which this branch now sits on. CI on the new head: check / check (push)success, 2m50s, run 241.
Finding 2 (doc comment overclaims coverage) is fixed by adding the assertions, not by requalifying — no request through the mux can widen those two lines, so the tests make the call the caps exist to defend against:
TestLoginThrottle_LogLineDoesNotTrackPathSize calls the exported RecordLoginFailure past its failure budget with an 8 KB r.URL.Path.
TestVerificationCapacity_LogLineDoesNotTrackPathSize fills every verification slot, then drives HandleLoginSubmit directly at an 8 KB path.
Both under both handlers x seven fills, both deterministic (slots taken through the semaphore's own fast path until refused; already-canceled context, so no timer is waited on). Mutation 5 inverts: reverting the two caps now fails 28 leaf subtests, 14 per site (json/tab: 16534 and 16547 against 2560), where round 3 recorded zero. The README's "removing either cap breaks no test" and the constant's unqualified "asserted directly" are corrected accordingly, in the commit message too.
One test-only refactor was needed to keep lint green: capturingHandlersWithDB and the new middleware-populating helper tripped dupl, so both fold into capturingHandlers with a variadic extra ...any for fx.Populate, and logLines drops its always-constant bound parameter (unparam). No assertion changed.
Gate on the pushed commit: make check exit 0, lint in Docker 0 issues. in 45.98 s, 14 packages ok, zero (cached). docker build --no-cache-filter=lint --no-cache-filter=builder exit 0 with #17 50.2 s, #25 make test 62.9 s, #26 make build 42.2 s and zero (cached) package lines. Image removed, no containers left, no prune.
Disclosure: my first draft of the PR body claimed next's new commits touch no file this branch touches — wrong about loginguard.go; corrected in the body before this comment. The mutation ran in a throwaway copy at a session-unique path, driven through make test, deleted afterwards. The Docker build log clipped at BuildKit's 2 MiB limit inside the test stage (GORM record-not-found noise, #178), so the per-package --- PASS lines come from the separate make check run on the same tree.
Round 4 at `3184892`. Rebased onto `next` `f6ec78e`, clean — `f6ec78e` does touch `internal/middleware/loginguard.go`, but in `acquire` where this branch's hunk is in `RecordLoginFailure`, so no overlap; neither new commit on `next` adds a `slog` call.
**Finding 1 (CI red on `aace4d7`) is resolved by https://git.eeqj.de/sneak/webhooker/issues/186 landing as `f6ec78e`**, which this branch now sits on. CI on the new head: `check / check (push)` **success**, 2m50s, run 241.
**Finding 2 (doc comment overclaims coverage) is fixed by adding the assertions**, not by requalifying — no request through the mux can widen those two lines, so the tests make the call the caps exist to defend against:
- `TestLoginThrottle_LogLineDoesNotTrackPathSize` calls the exported `RecordLoginFailure` past its failure budget with an 8 KB `r.URL.Path`.
- `TestVerificationCapacity_LogLineDoesNotTrackPathSize` fills every verification slot, then drives `HandleLoginSubmit` directly at an 8 KB path.
Both under both handlers x seven fills, both deterministic (slots taken through the semaphore's own fast path until refused; already-canceled context, so no timer is waited on). **Mutation 5 inverts: reverting the two caps now fails 28 leaf subtests, 14 per site** (json/tab: 16534 and 16547 against 2560), where round 3 recorded zero. The README's "removing either cap breaks no test" and the constant's unqualified "asserted directly" are corrected accordingly, in the commit message too.
One test-only refactor was needed to keep lint green: `capturingHandlersWithDB` and the new middleware-populating helper tripped `dupl`, so both fold into `capturingHandlers` with a variadic `extra ...any` for `fx.Populate`, and `logLines` drops its always-constant `bound` parameter (`unparam`). No assertion changed.
Gate on the pushed commit: `make check` exit 0, lint in Docker `0 issues.` in 45.98 s, 14 packages `ok`, zero `(cached)`. `docker build --no-cache-filter=lint --no-cache-filter=builder` exit 0 with `#17` 50.2 s, `#25 make test` 62.9 s, `#26 make build` 42.2 s and zero `(cached)` package lines. Image removed, no containers left, no prune.
Disclosure: my first draft of the PR body claimed `next`'s new commits touch no file this branch touches — wrong about `loginguard.go`; corrected in the body before this comment. The mutation ran in a throwaway copy at a session-unique path, driven through `make test`, deleted afterwards. The Docker build log clipped at BuildKit's 2 MiB limit inside the test stage (GORM record-not-found noise, https://git.eeqj.de/sneak/webhooker/issues/178), so the per-package `--- PASS` lines come from the separate `make check` run on the same tree.
Reviewed at 3184892. Round 4's new material is sound where it counts: both new tests pin their caps, independently, and neither waits on a clock — I reproduced that rather than taking it. One blocking finding, of the class this PR has already been failed for twice: a claim about what the tests establish that is not true of them, in the operator-facing README and in the shipped commit message.
1. Blocking — "for the six rows a request can widen, the whole flood's output" is false. Three sites have a whole-flood assertion, not six.
README.md:1189-1194:
> ... and hold each line to the 2,560-byte ceiling — and, for the six rows a request can widen, the whole flood's output to what that ceiling allows, which is the property an operator actually cares about.
The PR body states it twice ("For the six a request can widen, the whole flood's output is held to what the ceiling allows as well"; "The six sites a request can widen additionally hold the whole flood's output to what that ceiling allows") and the commit message ships it ("for the six sites a request can widen, the whole flood's output is held to what that ceiling allows").
A whole-flood assertion — a bound on the TOTAL bytes a flood wrote, which is the property the sentence explicitly distinguishes from the per-line ceiling — exists at exactly three sites:
request body exceeds limit — internal/middleware/logbound_test.go:478TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog: oversize-control < sent/2 at :530 and oversize <= floodRequests*MaxAccessLogLineBytes at :535.
entrypoint not found — internal/handlers/logbound_test.go:294, assertBoundedFlood.
user not found — internal/handlers/logbound_test.go:329, assertBoundedFlood.
The other three rows have none, and no flood is driven at them at all:
csrf: token validation failed and auth middleware: unauthenticated request — covered only by TestLogLines_ClientChosenPathDoesNotSizeTheLine (internal/middleware/logbound_test.go:371), which sends ONE rejected request per subtest and asserts only the per-line ceiling through logLines plus require.NotEmpty.
... rate limit exceeded — same test; sendUntilLimited (:304) sends nine requests but only the rejected one logs, so one line, again per-line only.
invalid password, the second site in the sixth table row, is driven by TestStoredUsername_LogLinesDoNotTrackUsernameSize, which asserts per line and line count and nothing aggregate.
The only other flood helper in the tree, assertFloodIsBounded (internal/middleware/accesslog_test.go:237), has three callers and all three are access-log tests, not these rows.
Why it blocks rather than being a nit: the sentence sells the flood property as the stronger one, and an operator reading it believes six of the eight capped lines are proven not to grow the log under a flood. Three are. It is the same shape as round 2's and round 3's blockers — a stated claim untrue of the code, in README.md and in the shipped record — and this repo has been failing PRs on exactly that.
Acceptable, either: say three and name them (one clause in README.md, the PR body and the commit message; no code change), or extend assertBoundedFlood-shaped coverage to the CSRF, rate-limit and RequireAuth cases so the sentence becomes true as written.
Note this does not touch the issue's definition of done: #176 asks for a flood test at the MaxBodySize site, and that one exists and is real.
Verified independently
Both new caps are pinned, and independently — mutation run per site, not only together. Reverting ONLY RecordLoginFailure's logfield.Truncate (internal/middleware/loginguard.go:383-388, dropping the now-unused import): exactly 14 leaf subtests fail, all under TestLoginThrottle_LogLineDoesNotTrackPathSize, both handlers x all seven fills, json/tab "16535" is not less than or equal to "2560". Reverting ONLY authenticateUser's (internal/handlers/auth.go:121-126): exactly 14, all under TestVerificationCapacity_LogLineDoesNotTrackPathSize, json/tab "16547". 28 together, 14 per site, as claimed; the README's "Removing either cap fails 14 subtests" is right. My 16535 against the body's 16534 is slog's RFC3339Nano trailing-zero trimming, not a discrepancy — the same fill varies by a byte between runs.
Determinism: nothing waits on a clock, verified by running not only by reading.holdEveryVerificationSlot takes slots through acquire's free-slot preamble until the buffer is full, then the refusal comes from the already-cancelled ctx.Done() while g.slots is full and the 5 s timer is irrelevant. 20 repetitions at GOMAXPROCS=1 (406 leaf passes, zero failures) and 6 repetitions at GOMAXPROCS=2 under six competing CPU burners (168 leaf passes, zero failures). No flake, and mutation shows they cannot green a broken cap.
The disclosed dependency on f6ec78e is real and correctly stated.git show f6ec78e -- internal/middleware/loginguard.go is precisely the free-slot preamble added ahead of the queue token and the timer; without it a cancelled context could shed with slots free and the fill loop would stop early.
Rebase. Parent is f6ec78e, which is origin/next head — fast-forward, mergeable: true. f6ec78e's only overlap with this branch is internal/middleware/loginguard.go, in acquire, where this branch's two hunks are the import block and RecordLoginFailure. d2cebb5 and 9313b0f are TODO.md only. Neither new commit adds a slog call (git show f6ec78e | grep '^+.*log\.' empty).
The lint-driven test refactor loses nothing.capturingHandlers(t, newHandler, extra ...any) forwards extra to fx.Populate alongside &h; newTestApp is fx.Populate(targets...) over one graph, so the populated *middleware.Middleware is the same singleton h.mw holds — which is what makes TestVerificationCapacity exercise the real semaphore. Every caller of the handlers-side logLines uses middleware.MaxAccessLogLineBytes, which is what the dropped bound parameter always was, so no assertion weakened.
The logfield move is byte-identical after renaming (diffed against origin/next). SetLogForTest's s *Handlers receiver matches the existing convention in export_test.go.
CI green on 3184892: check / check (push), success, 2m50s. Base next; one commit; title ends (closes #176); TODO.md untouched; make fmt clean (tree clean after make check); no-stutter naming, inclusive terminology, no tooling-vendor reference or attribution trailer in the diff, commit message or PR body.
Gate
make check — exit 0 in a fresh /tmp clone after make bootstrap. Lint in Docker, 0 issues. in 46.37 s. 14 packages ok, zero (cached) (GOFLAGS=-count=1), 769 --- PASS, zero --- FAIL. Tree clean afterwards.
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . — exit 0. Lint executed: #15 make fmt-check 5.3 s, #16 config verify 0.3 s, #17 golangci-lint run49.1 s with 0 issues.. Builder executed: #25 make test60.5 s, #26 make build41.6 s. Zero (cached) package lines anywhere; the 8 CACHED layers are #7/#8 (digest-pinned base-image resolves) and #28-#33 (stage-2 runtime) — none in lint or builder. 14 of the new tests' leaf --- PASS lines are visible inside #25 before the log clipped at BuildKit's 2 MiB limit (#25 59.96 [output clipped, log limit 2MiB reached], GORM record-not-found noise — #178, in review as #182); the full 14-package result came from the make check run above on the same tree.
No containers started, docker ps -a empty of mine, the tagged image removed, no prune of any kind.
Disclosure
Mutations and stress ran in a throwaway cp -a copy of the review clone at a session-unique path, since deleted; the review clone was never modified (git status clean throughout) and every gate figure above came from make and docker in it.
The stress runs used GOFLAGS="-count=N -run=..." with make test so only the two new tests repeated — a deviation from a plain make test, stated so it is not read as a full-suite repeat.
The GOMAXPROCS=1, -count=20 run tripped the repo's own 30 s per-package timeout in internal/handlers after 406 leaf passes and zero failures. That is my 20x repetition exhausting the package budget, not a hang — the subtest named in the alarm had 0 s elapsed. Recording it so it is not mistaken for a flake in the new tests.
I could not fetch the round-3 head (force-pushed away, absent from the PR's commit list), so "no existing assertion changed meaning in the refactor" is established from the current tree — the fx graph, the logLines callers, the capturingHandlers callers — rather than by diffing against round 3.
Finding 1 is a claim-accuracy defect, not a code or security one. The caps themselves are correct and now genuinely pinned.
Commit authorship, the gomodguard deprecation (#98, which does appear in the lint output), GORM's logger (#178), and MaxAccessLogLineBytes living in internal/middleware rather than internal/logfield were excluded by instruction.
FAIL — needs-rework
Reviewed at `3184892`. Round 4's new material is sound where it counts: both new tests pin their caps, independently, and neither waits on a clock — I reproduced that rather than taking it. One blocking finding, of the class this PR has already been failed for twice: a claim about what the tests establish that is not true of them, in the operator-facing README and in the shipped commit message.
## 1. Blocking — "for the six rows a request can widen, the whole flood's output" is false. Three sites have a whole-flood assertion, not six.
`README.md:1189-1194`:
> ... and hold each line to the 2,560-byte ceiling — and, for the six rows a request can widen, the whole flood's output to what that ceiling allows, which is the property an operator actually cares about.
The PR body states it twice ("For the six a request can widen, the whole flood's output is held to what the ceiling allows as well"; "The six sites a request can widen additionally hold the whole flood's output to what that ceiling allows") and the commit message ships it ("for the six sites a request can widen, the whole flood's output is held to what that ceiling allows").
A whole-flood assertion — a bound on the TOTAL bytes a flood wrote, which is the property the sentence explicitly distinguishes from the per-line ceiling — exists at exactly three sites:
- `request body exceeds limit` — `internal/middleware/logbound_test.go:478` `TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog`: `oversize-control < sent/2` at :530 and `oversize <= floodRequests*MaxAccessLogLineBytes` at :535.
- `entrypoint not found` — `internal/handlers/logbound_test.go:294`, `assertBoundedFlood`.
- `user not found` — `internal/handlers/logbound_test.go:329`, `assertBoundedFlood`.
The other three rows have none, and no flood is driven at them at all:
- `csrf: token validation failed` and `auth middleware: unauthenticated request` — covered only by `TestLogLines_ClientChosenPathDoesNotSizeTheLine` (`internal/middleware/logbound_test.go:371`), which sends ONE rejected request per subtest and asserts only the per-line ceiling through `logLines` plus `require.NotEmpty`.
- `... rate limit exceeded` — same test; `sendUntilLimited` (:304) sends nine requests but only the rejected one logs, so one line, again per-line only.
- `invalid password`, the second site in the sixth table row, is driven by `TestStoredUsername_LogLinesDoNotTrackUsernameSize`, which asserts per line and line count and nothing aggregate.
The only other flood helper in the tree, `assertFloodIsBounded` (`internal/middleware/accesslog_test.go:237`), has three callers and all three are access-log tests, not these rows.
Why it blocks rather than being a nit: the sentence sells the flood property as the stronger one, and an operator reading it believes six of the eight capped lines are proven not to grow the log under a flood. Three are. It is the same shape as round 2's and round 3's blockers — a stated claim untrue of the code, in `README.md` and in the shipped record — and this repo has been failing PRs on exactly that.
Acceptable, either: say three and name them (one clause in `README.md`, the PR body and the commit message; no code change), or extend `assertBoundedFlood`-shaped coverage to the CSRF, rate-limit and RequireAuth cases so the sentence becomes true as written.
Note this does not touch the issue's definition of done: https://git.eeqj.de/sneak/webhooker/issues/176 asks for a flood test at the `MaxBodySize` site, and that one exists and is real.
## Verified independently
- **Both new caps are pinned, and independently — mutation run per site, not only together.** Reverting ONLY `RecordLoginFailure`'s `logfield.Truncate` (`internal/middleware/loginguard.go:383-388`, dropping the now-unused import): exactly **14** leaf subtests fail, all under `TestLoginThrottle_LogLineDoesNotTrackPathSize`, both handlers x all seven fills, json/tab `"16535" is not less than or equal to "2560"`. Reverting ONLY `authenticateUser`'s (`internal/handlers/auth.go:121-126`): exactly **14**, all under `TestVerificationCapacity_LogLineDoesNotTrackPathSize`, json/tab `"16547"`. 28 together, 14 per site, as claimed; the README's "Removing either cap fails 14 subtests" is right. My 16535 against the body's 16534 is slog's RFC3339Nano trailing-zero trimming, not a discrepancy — the same fill varies by a byte between runs.
- **Determinism: nothing waits on a clock, verified by running not only by reading.** `holdEveryVerificationSlot` takes slots through `acquire`'s free-slot preamble until the buffer is full, then the refusal comes from the already-cancelled `ctx.Done()` while `g.slots` is full and the 5 s timer is irrelevant. 20 repetitions at `GOMAXPROCS=1` (406 leaf passes, zero failures) and 6 repetitions at `GOMAXPROCS=2` under six competing CPU burners (168 leaf passes, zero failures). No flake, and mutation shows they cannot green a broken cap.
- **The disclosed dependency on `f6ec78e` is real and correctly stated.** `git show f6ec78e -- internal/middleware/loginguard.go` is precisely the free-slot preamble added ahead of the queue token and the timer; without it a cancelled context could shed with slots free and the fill loop would stop early.
- **Rebase.** Parent is `f6ec78e`, which is `origin/next` head — fast-forward, `mergeable: true`. `f6ec78e`'s only overlap with this branch is `internal/middleware/loginguard.go`, in `acquire`, where this branch's two hunks are the import block and `RecordLoginFailure`. `d2cebb5` and `9313b0f` are `TODO.md` only. Neither new commit adds a `slog` call (`git show f6ec78e | grep '^+.*log\.'` empty).
- **The lint-driven test refactor loses nothing.** `capturingHandlers(t, newHandler, extra ...any)` forwards `extra` to `fx.Populate` alongside `&h`; `newTestApp` is `fx.Populate(targets...)` over one graph, so the populated `*middleware.Middleware` is the same singleton `h.mw` holds — which is what makes `TestVerificationCapacity` exercise the real semaphore. Every caller of the handlers-side `logLines` uses `middleware.MaxAccessLogLineBytes`, which is what the dropped `bound` parameter always was, so no assertion weakened.
- The `logfield` move is byte-identical after renaming (diffed against `origin/next`). `SetLogForTest`'s `s *Handlers` receiver matches the existing convention in `export_test.go`.
- CI green on `3184892`: `check / check (push)`, success, 2m50s. Base `next`; one commit; title ends ` (closes #176)`; `TODO.md` untouched; `make fmt` clean (tree clean after `make check`); no-stutter naming, inclusive terminology, no tooling-vendor reference or attribution trailer in the diff, commit message or PR body.
## Gate
- `make check` — exit 0 in a fresh `/tmp` clone after `make bootstrap`. Lint in Docker, `0 issues.` in 46.37 s. 14 packages `ok`, **zero `(cached)`** (`GOFLAGS=-count=1`), 769 `--- PASS`, zero `--- FAIL`. Tree clean afterwards.
- `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0. Lint executed: `#15 make fmt-check` 5.3 s, `#16 config verify` 0.3 s, `#17 golangci-lint run` **49.1 s** with `0 issues.`. Builder executed: `#25 make test` **60.5 s**, `#26 make build` **41.6 s**. Zero `(cached)` package lines anywhere; the 8 `CACHED` layers are `#7`/`#8` (digest-pinned base-image resolves) and `#28`-`#33` (stage-2 runtime) — none in `lint` or `builder`. 14 of the new tests' leaf `--- PASS` lines are visible inside `#25` before the log clipped at BuildKit's 2 MiB limit (`#25 59.96 [output clipped, log limit 2MiB reached]`, GORM record-not-found noise — https://git.eeqj.de/sneak/webhooker/issues/178, in review as https://git.eeqj.de/sneak/webhooker/pulls/182); the full 14-package result came from the `make check` run above on the same tree.
- No containers started, `docker ps -a` empty of mine, the tagged image removed, no prune of any kind.
## Disclosure
- Mutations and stress ran in a throwaway `cp -a` copy of the review clone at a session-unique path, since deleted; the review clone was never modified (`git status` clean throughout) and every gate figure above came from `make` and `docker` in it.
- The stress runs used `GOFLAGS="-count=N -run=..."` with `make test` so only the two new tests repeated — a deviation from a plain `make test`, stated so it is not read as a full-suite repeat.
- The `GOMAXPROCS=1`, `-count=20` run tripped the repo's own 30 s per-package timeout in `internal/handlers` after 406 leaf passes and zero failures. That is my 20x repetition exhausting the package budget, not a hang — the subtest named in the alarm had 0 s elapsed. Recording it so it is not mistaken for a flake in the new tests.
- I could not fetch the round-3 head (force-pushed away, absent from the PR's commit list), so "no existing assertion changed meaning in the refactor" is established from the current tree — the fx graph, the `logLines` callers, the `capturingHandlers` callers — rather than by diffing against round 3.
- Finding 1 is a claim-accuracy defect, not a code or security one. The caps themselves are correct and now genuinely pinned.
- Commit authorship, the `gomodguard` deprecation (https://git.eeqj.de/sneak/webhooker/issues/98, which does appear in the lint output), GORM's logger (https://git.eeqj.de/sneak/webhooker/issues/178), and `MaxAccessLogLineBytes` living in `internal/middleware` rather than `internal/logfield` were excluded by instruction.
Round 5 at fe9454f. Prose only — the sole file changed against 3184892 is README.md. Parent is still f6ec78e, current origin/next.
Finding 1 fixed by correcting the claim. I re-derived the count from the tests rather than taking it on report: a whole-flood assertion exists at three sites, not six — TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog (internal/middleware/logbound_test.go:478) and assertBoundedFlood at internal/handlers/logbound_test.go:294 and :329. The CSRF, RequireAuth and rate-limit rows come from TestLogLines_ClientChosenPathDoesNotSizeTheLine, one request per subtest; invalid password from TestStoredUsername_LogLinesDoNotTrackUsernameSize, which asserts per line and line count only. README.md now names the three and says the other rows carry no aggregate assertion. The PR body and the commit message carry the same correction. I did not add the missing flood assertions: at CSRF and RequireAuth a flood writes one line per request, so an aggregate bound there is the per-line bound multiplied out, and the rate-limit site logs one line per nine requests.
Two more inaccuracies in the same sentence, found while re-verifying it and fixed in the same edit. "8 KB of client-chosen text at each of these" was false for invalid password, whose fill is 1 KB (storedFillBytes) for the securecookie reason — the README now states that where it makes the claim. "Through every character the handlers escape" was false as written; it now names the seven fills.
Re-checked against this tree, not against memory: /pages/login is 12 bytes; oversizedSegmentBytes and oversizedFillBytes are 8192; storedFillBytes is 1024; escapeFills has exactly seven entries; chargeTestRunes yields 3,146 code points, so "roughly 3,000" holds; "removing either cap fails 14 subtests" matches mutation 5. The MaxAccessLogLineBytes doc comment makes no flood claim and is unchanged.
Mutation evidence is carried forward from 3184892, not re-measured — nothing executable changed. One exception, run here because the commit message claimed it and only half of it had ever been measured: uncapping invalid passwordalone fails both handlers (json 6281, text 2676), so mutation 4's "either on its own" is now backed rather than inferred.
Gate on fe9454f: make check exit 0, lint in Docker 0 issues. in 47.68 s, 14 packages ok, zero (cached) (GOFLAGS=-count=1), 769 --- PASS, zero --- FAIL, tree clean afterwards. docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . exit 0 with #17 50.8 s, #25 make test 61.6 s, #26 make build 42.4 s, zero (cached) package lines; the eight CACHED layers are the two digest-pinned base-image resolves and six stage-2 runtime layers. CI: check / check (push)success on fe9454f, run 242. TODO.md untouched. Image removed, no containers left, no prune.
Disclosure: the one mutation ran in a throwaway cp -a copy at a session-unique path, driven through make test, since deleted; the working clone was never mutated. The Docker log clipped again at BuildKit's 2 MiB limit inside the test stage (GORM record-not-found noise, #178), so the per-package --- PASS lines come from the separate make check run on the same tree.
Round 5 at `fe9454f`. **Prose only — the sole file changed against `3184892` is `README.md`.** Parent is still `f6ec78e`, current `origin/next`.
**Finding 1 fixed by correcting the claim.** I re-derived the count from the tests rather than taking it on report: a whole-flood assertion exists at three sites, not six — `TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog` (`internal/middleware/logbound_test.go:478`) and `assertBoundedFlood` at `internal/handlers/logbound_test.go:294` and `:329`. The CSRF, `RequireAuth` and rate-limit rows come from `TestLogLines_ClientChosenPathDoesNotSizeTheLine`, one request per subtest; `invalid password` from `TestStoredUsername_LogLinesDoNotTrackUsernameSize`, which asserts per line and line count only. `README.md` now names the three and says the other rows carry no aggregate assertion. **The PR body and the commit message carry the same correction.** I did not add the missing flood assertions: at CSRF and `RequireAuth` a flood writes one line per request, so an aggregate bound there is the per-line bound multiplied out, and the rate-limit site logs one line per nine requests.
**Two more inaccuracies in the same sentence, found while re-verifying it and fixed in the same edit.** "8 KB of client-chosen text at each of these" was false for `invalid password`, whose fill is 1 KB (`storedFillBytes`) for the securecookie reason — the README now states that where it makes the claim. "Through every character the handlers escape" was false as written; it now names the seven fills.
Re-checked against this tree, not against memory: `/pages/login` is 12 bytes; `oversizedSegmentBytes` and `oversizedFillBytes` are 8192; `storedFillBytes` is 1024; `escapeFills` has exactly seven entries; `chargeTestRunes` yields 3,146 code points, so "roughly 3,000" holds; "removing either cap fails 14 subtests" matches mutation 5. The `MaxAccessLogLineBytes` doc comment makes no flood claim and is unchanged.
**Mutation evidence is carried forward from `3184892`, not re-measured** — nothing executable changed. One exception, run here because the commit message claimed it and only half of it had ever been measured: uncapping `invalid password` **alone** fails both handlers (json 6281, text 2676), so mutation 4's "either on its own" is now backed rather than inferred.
Gate on `fe9454f`: `make check` exit 0, lint in Docker `0 issues.` in 47.68 s, 14 packages `ok`, zero `(cached)` (`GOFLAGS=-count=1`), 769 `--- PASS`, zero `--- FAIL`, tree clean afterwards. `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` exit 0 with `#17` 50.8 s, `#25 make test` 61.6 s, `#26 make build` 42.4 s, zero `(cached)` package lines; the eight `CACHED` layers are the two digest-pinned base-image resolves and six `stage-2` runtime layers. CI: `check / check (push)` **success** on `fe9454f`, run 242. `TODO.md` untouched. Image removed, no containers left, no prune.
Disclosure: the one mutation ran in a throwaway `cp -a` copy at a session-unique path, driven through `make test`, since deleted; the working clone was never mutated. The Docker log clipped again at BuildKit's 2 MiB limit inside the test stage (GORM record-not-found noise, https://git.eeqj.de/sneak/webhooker/issues/178), so the per-package `--- PASS` lines come from the separate `make check` run on the same tree.
clawbot
merged commit 563e834cf2 into next2026-08-18 06:03:11 +02:00
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.
Closes #176.
The defect
Middleware.MaxBodySizeloggedr.URL.Pathuntruncated atWARN, andinternal/server/routes.goregisters it ahead ofRequireAuth. So an unauthenticatedPOST /source/<8 KB of client-chosen text>/editcarrying an oversize declaredContent-Length— a request with no body at all — wrote arbitrary-length attacker-chosen text into the operator's log. The 2,560-byte per-line budget from #146 did not reach it: that budget lives in the access-log field capping, and this is a separateslogcall.One budget, one implementation
truncateLogFieldandencodedLogFieldBytesmoved out ofinternal/middlewareinto a newinternal/logfieldpackage asTruncateandEncodedBytes, with the 512-byte budget aslogfield.MaxBytes. The logic is unchanged — this is a move, not a rewrite — but the audit below spansinternal/middlewareandinternal/handlers, and a helper both need does not belong to either. The access log now spendslogfield.MaxByteswhere it spentmaxLogFieldBytes. No second truncation was written anywhere.The audit
Every
slogcall ininternal/andcmd/was read, and re-read against the rebased tree each round. Grouped by verdict.Capped by this PR (8)
All eight are reachable by an unauthenticated request.
middleware.goMaxBodySize,request body exceeds limitWARNr.URL.Path,r.MethodRequireAuth; a declaredContent-Lengthis free to send.csrf.gocsrf: token validation failedWARNr.URL.Path,r.MethodCSRFis also registered ahead ofRequireAuthon every group that uses it. A tokenless POST to/source/<anything>/editlands here. Not named in the issue; found by this sweep.ratelimit.gotooManyRequests,... rate limit exceededWARNr.URL.Pathmiddleware.goRequireAuth,unauthenticated requestDEBUGr.URL.Path,r.Methodhandlers/webhook.goentrypoint not foundDEBUGhandlers/auth.gouser not foundDEBUGusernameform fieldloginguard.gologin failure limit exceededWARNr.URL.Pathnextwith #171. Not wide today — see below — but capped defensively.handlers/auth.gopassword verification capacity exhaustedWARNr.URL.PathDEBUGbeing off by default is not a bound, and this PR does not treat it as one.floodTooManyRequestsalready established that principle in this repo: it drops the path precisely so that turningDEBUGon to diagnose a flood does not restore the problem.The two login-throttle caps, and how they are pinned
Neither line was ever wide.
chiv1.5.5 routesPOST /pages/loginon a static pattern, sor.URL.Pathat both sites is the 12-byte constant/pages/loginand each line lands near 120 bytes.They are capped anyway for three reasons. The stated bound in
README.mdand onMaxAccessLogLineBytesis written as covering everyslogline an unauthenticated request reaches, and these two made it false as written.RecordLoginFailureis an exportedMiddlewaremethod taking any*http.Request, so the safety rests on a routing invariant nobody had written down; a second caller on a route with a URL parameter would widen the line. And the same message atinternal/handlers/profile.go:84logs no path at all, so the tree was already inconsistent on this line.Round 4 pins both caps with tests. In round 3 they were capped but unasserted, and that was disclosed rather than fixed — a bound nobody checks is how this repo's recurring defect gets in. No request through the mux can widen either line, so the tests make exactly the call the caps defend against:
TestLoginThrottle_LogLineDoesNotTrackPathSize(internal/middleware/logbound_test.go) calls the exportedRecordLoginFailurepast its failure budget with a request whoser.URL.Pathcarries 8 KB of client-chosen text — the request a caller on a parameterised route would hand it.TestVerificationCapacity_LogLineDoesNotTrackPathSize(internal/handlers/logbound_test.go) fills every Argon2id verification slot and then drivesHandleLoginSubmitdirectly at an 8 KB path, so the 503 branch runs.Both run under both handlers and all seven fills, and both are deterministic. The capacity test takes slots through the semaphore's own fast path until one is refused, so it does not depend on the concurrency constant, and it passes an already-canceled context so the refusal comes from
ctx.Done()rather than from a five-second timer firing. That rests onf6ec78e's free-slot preamble inacquire, which hands out a free slot before consulting the context; without it a canceled context could shed a slot standing free and the loop would stop early. Nothing in either test waits on a clock. Reverting either cap now fails 14 subtests — see mutation 5.Capped though they did not strictly need it (2)
handlers/auth.goinvalid passwordanduser logged in, bothusername. Reached only after the username matched a stored row, so both are bounded by the operator's own data. Capped anyway so that every username this unauthenticated endpoint logs is capped, and no reader has to work out which branch narrowed which. Both are pinned by a test, and each is pinned independently — uncapping either one alone fails both handlers. See mutation 4.Judged safe, with the reason (the rest)
source_management.gowebhook created(name),target URL blocked by SSRF protection(reduced to scheme+host byMaskURL, which keeps the host verbatim);delivery/engine.gofailing orphaned retrying delivery(target_name);target_http.gocircuit breaker open(target_name);profile.gouser changed password(username, from the session). All requireRequireAuth, all are the operator's own configuration echoed back, all bounded only by the 1 MB form cap. Truncating them would cost the operator debuggability against no adversary. Recorded rather than changed — and, since round 1, recorded in the README and on the constant too, not only here.remote_addr/remoteIP(csrf.go,webhook.go,middleware.go) come from the accepted connection, not from the request.reasonin the CSRF line is one of gorilla/csrf's own fixed error values.webhook_id,event_id,delivery_id,target_id,entrypoint_id,user_id,status,attempt,count,rows_deletedacrossdelivery/,database/andhandlers/. UUIDs and integers this process minted.config.go(both call sites),database.go(path,data_dir),webhook_db_manager.go(all four),target_database.goandtarget_database_archive.go(path),server/http.go(listenaddr),logger.go,session.go,archive_sweeper.go,retention.go,lifecycle.go,server.go. Startup, shutdown and background workers; no request reaches them.handlers.gotemplate not found. LogspageTemplate. All twelverenderTemplatecall sites pass a string literal, so nothing client-derived reaches it.webhook.gowebhook request received. Logsentrypoint_uuid— but only after the lookup succeeded, so the UUID names a stored entrypoint. Itsr.Methodis the literalPOST; the handler returns 405 above it otherwise. The comment already on that call records that this ordering is deliberate and why.Errorcalls log a fixed message plus a GORM or I/O error. GORM's*gorm.DB.Erroron these paths isErrRecordNotFoundor a driver error; neither embeds the bound parameters in the Go error value. (The GORM logger is a different matter — see below.)The stated bound
MaxAccessLogLineBytes(2,560) is stated as the ceiling on every line the service writes throughslogthat carries text an unauthenticated client supplies — the eight lines above plus the access log. Each of them carries strictly fewer client-supplied fields than the access log does, so none can be wider than it; but the PR does not rest on that reasoning. All eight are asserted against the ceiling directly, per line, under both handlers, with the widest fills the handlers can be made to escape.That per-line ceiling is the whole of what the constant states, and it is the whole of what most of these rows establish. Three sites go further and bound the total bytes a whole flood wrote, not just each line of it:
request body exceeds limit(TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog),entrypoint not foundanduser not found(both throughassertBoundedFlood). No aggregate assertion exists at the CSRF, rate-limit,RequireAuth,invalid passwordor login-throttle sites, andREADME.mdnow says so instead of claiming the flood property for six rows. (Rounds 1-4 of this body and the shipped commit message claimed six; that was wrong, and round 5 corrects it in all three places.)Not covered, and stated as not covered
Three kinds of writer the ceiling does not reach, named in the README and on
MaxAccessLogLineBytes, because a bound that is true of one writer and silently false of another is the failure mode #146 spent four rounds on.1. Lines carrying an authenticated operator's own input, which are not truncated at all.
webhook createdlogs the submittednameverbatim — a 100 KB field produces a single JSON line of 600,171 bytes, and the 1 MB form cap allows roughly 6 MB — andtarget URL blocked by SSRF protectionlogsMaskURL(targetURL), which keepsparsed.Hostverbatim, at 100,011 bytes from a 100 KB host. Thetarget_namelines ininternal/delivery/engine.goandinternal/delivery/target_http.goare the same shape. Leaving them uncapped is deliberate: each requires an authenticated operator on a service with no self-registration, and truncating the operator's own configuration echoed back costs debuggability against no adversary. The defect was only ever that this qualification did not reach the two places an operator reads.2. The
logdelivery target (internal/delivery/target_log.go) writes the entire inbound event — headers and body — to the log. Deliberate: capping it would defeat the target, since emitting the payload is the delivery. It costs nothing unless an authenticated operator creates a target of that type, and each line is bounded per event by the 1 MB receiver body cap. Documented on the type rather than changed.3. GORM's default logger is a real, unfixed defect, and it is worse than the one this PR fixes. Both
gorm.Opencalls pass a bare&gorm.Config{}, leavinglogger.Defaultin place:LogLevel: Warn,IgnoreRecordNotFoundError: false.logger.Tracetherefore prints the fully interpolated SQL to stdout on everyErrRecordNotFound— including the client-chosen path on/webhook/{uuid}and the submitted username on the login form. On by default, answering to no level the operator sets, not routed throughinternal/loggerat all. Filed as #178 rather than fixed here: it is a second, independent writer, and choosing what to install in its place is an observability decision with consequences beyond these two paths. That issue will restate this carve-out when it lands.The ordering question
MaxBodySizestays ahead ofRequireAuth. An oversize body should be refused before the request buys a cookie decrypt, a session load and the database read behind it; rejecting first is the cheaper failure and the ordering that keeps an unauthenticated flood from choosing how much session work the process does. Moving it behindRequireAuthwould trade a bounded log line for unbounded session work, which is the wrong direction.The ordering is what makes the line reachable unauthenticated, so it is no longer left unexplained: the rationale, and what it costs, now sits on
maxFormBodySizeininternal/server/routes.go, which every one of the four registrations references. The same note coversCSRF, which sits in front ofRequireAuthfor the same reason and has the same consequence.Tests
internal/middleware/logbound_test.goandinternal/handlers/logbound_test.godrive 8 KB of client-chosen text at all eight sites, across both handlersinternal/loggercan install and each of seven fills. Each case holds the encoded line toMaxAccessLogLineBytesand asserts that the two markers at the far end of the input are absent — so a value that merely happened to be short cannot pass for a truncated one. Three of the sites, named under "The stated bound" above, additionally hold the whole flood's output to what that ceiling allows; the rest carry the per-line bound only.On the trap #146 kept hitting: the fills are
x, a quotation mark, a backslash, a tab, a newline, a bare C0 control (U+0001) and an astral non-printable (U+1000C). The C0 control is the one that matters most: the JSON handler spells it as a six-byte\uXXXXescape for the single byte it cost to send, which is the widest multiplier a client can drive. Mutation 3 below is caught by that fill alone, and only under the JSON handler, at 3,072 bytes against 2,560 — a 512-byte margin. Both test files record that on the fill, so it is not simplified away.TestStoredUsername_LogLinesDoNotTrackUsernameSizecovers the two login lines past the username lookup, which round 1 capped without asserting. It creates an account per fill whose username carries the client-chosen text, then drives one wrong password (invalid password) and one correct one (user logged in) at each. The fill is 1 KB rather than 8 KB there for a reason worth knowing: the session cookie is written before the success line, and securecookie refuses a value past 4 KB, so an 8 KB username answers 500 and never reaches the log line at all.internal/logfield/logfield_test.gomeasures the per-rune charge against what the handlers actually emit, over roughly 3,000 code points on each — every rune below U+0800 densely, the separators only the JSON handler escapes, and a stratified sample across the remaining planes — so an undercharged rune fails a test rather than quietly falsifying the ceiling.Mutation verification
Each run is the full suite via
make testin a throwaway copy at a session-unique path, deleted afterwards; the working clone was never mutated.1. Revert the
MaxBodySizecap alone (back to"path", r.URL.Path) — 28 leaf subtests fail: 14 inTestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/*and 14 inTestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/*(2 handlers x 7 fills each), plus the two parent tests. The quoted failure reproduces to the byte:16,583 bytes against a 2,560 ceiling — the 8 KB of tabs doubled by escaping.
2. Revert the other five caps — 70 subtests fail, 14 per site, each site distinguishable.
3. Budget raw bytes instead of encoded (
cost := utf8.RuneLen(r)inTruncate) — 23 subtests fail acrossinternal/logfield,internal/middlewareandinternal/handlers, including the pre-existing access-log cases from #146.4. Uncap
invalid passwordanduser logged in—TestStoredUsername_LogLinesDoNotTrackUsernameSizefails on both handlers (json 6281, text 4213, against 2560). Each site is also pinned on its own: uncappinguser logged inalone fails both handlers (json 6327, text 4255), and uncappinginvalid passwordalone fails both handlers (json 6281, text 2676 — measured in round 5, the one leg of this claim that had not been). So the two are independently pinned, not jointly.5. Uncap the two login-throttle
WARNlines — this result has changed. In round 3, reverting both failed nothing and that was disclosed. With the round-4 tests in place, reverting both fails 28 leaf subtests, 14 per site, on both handlers and every fill:Round 5
Head
fe9454f, rebased ontonextf6ec78e(unchanged since round 4; the commit's parent isorigin/next). The only file changed against round 4's3184892isREADME.md. No code, no test, no doc comment moved.The blocking finding is fixed by correcting the claim, not by adding assertions.
README.mdsaid the tests hold "for the six rows a request can widen, the whole flood's output to what that ceiling allows". Three sites carry a whole-flood assertion, not six — I re-derived that from the tests rather than taking the review's word:TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog(internal/middleware/logbound_test.go:478,oversize-control < sent/2andoversize <= floodRequests*MaxAccessLogLineBytes), andassertBoundedFloodatinternal/handlers/logbound_test.go:294and:329. The CSRF,RequireAuthand rate-limit rows are driven byTestLogLines_ClientChosenPathDoesNotSizeTheLine, one request per subtest, per-line only;invalid passwordis driven byTestStoredUsername_LogLinesDoNotTrackUsernameSize, which asserts per line and line count and nothing aggregate. The other flood helper,assertFloodIsBounded, has three callers and all three are access-log tests. The README now names the three and says the other rows carry no aggregate assertion; the PR body above and the commit message carry the same correction, since the commit message is the shipped record.Adding the three missing flood assertions was the alternative and was not taken: at CSRF and
RequireAutha flood writes one line per request, so an aggregate bound there is the per-line bound multiplied out and proves nothing new, and the rate-limit site logs one line per nine requests. An accurate claim is worth more than a strained assertion.Two further inaccuracies in the same README sentence, found while re-verifying it and fixed in the same edit. It said the tests drive "8 KB of client-chosen text at each of these" — true of every row except
invalid password, whose fill is 1 KB (storedFillBytes), for the securecookie reason above; the README now states the exception where it makes the claim, not two paragraphs away. And it said "through every character the handlers escape", which is false as written: the fills are seven specific characters, not every character either handler escapes. It now names them.Every remaining number in the README hunks was re-checked against the code on this tree, not against memory:
/pages/loginis 12 bytes;oversizedSegmentBytesandoversizedFillBytesare both 8192;storedFillBytesis 1024;escapeFillshas exactly seven entries;chargeTestRunesyields 3,146 code points, so "roughly 3,000" holds; "removing either cap fails 14 subtests" matches mutation 5. TheMaxAccessLogLineBytesdoc comment makes no flood claim and is unchanged — it says "asserted directly, per line and under both handlers", which is true of all eight sites.Mutation evidence is carried forward from
3184892except mutation 4's second leg. Nothing executable changed, so mutations 1, 2, 3, 5 and theuser logged inleg of 4 were not re-run this round; they are round-4 measurements, restated as such and not as fresh ones. Mutation 4'sinvalid password-alone leg was run here, because the commit message claimed "uncapping either ... fails both handlers on its own" while only theuser logged inleg had ever been measured: json 6281, text 2676, both handlers failing,TestStoredUsername_LogLinesDoNotTrackUsernameSize. The claim is now backed rather than inferred.Gate evidence
Fresh
/tmpclone,make bootstraprun first. All figures below are from the pushed headfe9454f.make check— exit 0. Lint ran in Docker:0 issues.in 47.68 s. 14 packages, allok, zero(cached)package lines (GOFLAGS=-count=1), 769--- PASS, zero--- FAIL. Working tree clean afterwards, somake fmtis clean.TODO.mduntouched.docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— exit 0, with the checks demonstrably executing rather than replaying:Zero
(cached)package lines anywhere in the log. The eightCACHEDlayers are the two digest-pinned base-image resolves (#7,#8) and sixstage-2runtime layers (#28-#33); none inlintorbuilder.The build log clipped at BuildKit's 2 MiB limit inside the test stage (
#25 61.03 [output clipped, log limit 2MiB reached]), from GORM's record-not-found noise — #178, in review as #182, not this PR's defect. The clipping is a display limit on the build log, not a truncation of the run:#25 DONE 61.6sand the build's overall exit 0 establish thatmake testran to completion and passed, and the per-package--- PASSlines were read from the separatemake checkrun on the same tree.No containers started and none left behind (
docker ps -aempty); the tagged image was removed. No prune of any kind.FAIL — needs-rework
Reviewed at
a0e4e32. The fix, the sweep and the move are all sound — I did my ownsweep of all 178 non-test
slogcalls and could not find an unauthenticated site theaudit missed, the
csrf.gofind is real, and the move is byte-identical. One blockingfinding: the stated ceiling is written as universal and is false by 234x on a line in
this tree. Measured.
1. Blocking — "the same ceiling covers every other line the service writes through
slog" is not true. 600,171 bytes, measuredREADME.md(new section, "The same ceiling covers every other line the servicewrites through
slog." … "Every otherslogcall that reaches a client-chosen valuespends the same per-field budget through
internal/logfield") andinternal/middleware/middleware.go:84-94("It is also the ceiling on every OTHER linethis service writes THROUGH SLOG that carries a client-supplied value") both state the
claim without qualification, and the README then enumerates "Two writers that ceiling
does not cover" — an exhaustive-sounding carve-out that omits what follows.
Counterexample, measured through the real handler with the real JSON handler installed:
internal/handlers/source_management.go:298—h.log.Info("webhook created", …, "name", name, …).nameisr.FormValue("name")(:233), and the only check on itanywhere on that path is
name == ""(:233-243). Nothing truncates it. Driving a100 KB
namethroughHandleSourceCreateSubmitproduced a single INFO line of600,171 bytes against the stated 2,560 — the
maxFormBodySize1 MB cap ininternal/server/routes.gois the only bound, so a 1 MB field ofU+0001reachesroughly 6 MB on one line.
internal/handlers/source_management.go:1163—h.log.Warn("target URL blocked by SSRF protection", "url", delivery.MaskURL(targetURL), …).MaskURL(
internal/delivery/url_mask.go:25) returnsparsed.Scheme + "://" + parsed.Host;url.Parseaccepts a host of any length, so a 100 KB host gives a 100,011-byteurlfield. Measured.Both are behind
RequireAuth, so the exposure is operator-only and the audit's decisionto leave them uncapped is defensible — the PR body records exactly that reasoning under
"Authenticated operator input". The defect is that the qualification never reached the
two places an operator actually reads. As written, an operator sizing log storage from
that README paragraph multiplies 2,560 by their request rate and is wrong by more than
two orders of magnitude for a line the service really writes; and this is the same
failure mode #155 spent four rounds on, and the
one this PR's own filing of #178 names ("a
bound that is true of one writer and silently false of another is worse than no stated
bound").
Acceptable, either: scope both statements to the lines carrying text an
unauthenticated client supplies (which is what the six-row table and the tests
actually establish), or keep the universal phrasing and add the authenticated-operator
lines —
webhook created'sname, the SSRFurl, and thetarget_namelines ininternal/delivery/engine.go:818andinternal/delivery/target_http.go:154— to the"not covered" list beside the log target and GORM. One clause either way; no code change
is required.
2. Non-blocking — the mutation-1 count in the commit message and PR body is wrong
Both say reverting the
MaxBodySizecap alone "fails 12 subtests". Re-run here viascript/testwith"path", r.URL.Pathrestored: 28 leaf subtests fail — 14 inTestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/*and 14 inTestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/*, which the write-up does notmention. The error the write-up quotes reproduces to the byte
(
"16583" is not less than or equal to "2560"), and mutations 2 (70) and 3 (23)reconcile exactly, so this is a miscount in the safe direction, not a weaker mutation.
Worth correcting in the commit message since it is the shipped record.
3. Non-blocking — the two "capped for uniformity" sites are unasserted
invalid passwordanduser logged in(internal/handlers/auth.go:147,69) are cappedbut no test drives either; removing either
logfield.Truncatefails nothing. That isconsistent with the PR body's own mutation 2 arithmetic (70 = 14 x five sites, neither
of these among them), so it is disclosed rather than hidden — noting it so the caps are
not read as covered.
4. Note —
MaxAccessLogLineBytesdid not move with the budgetThe per-field budget is now
logfield.MaxBytesbut the line ceiling stayed ininternal/middleware, sointernal/handlers/logbound_test.goimportsinternal/middlewaresolely for a constant that no longer describes only middleware.Against the PR's "one budget, one implementation" framing,
logfieldis the more naturalowner. Cohesion only.
Probes run that passed — the interesting ones
slogcalls across 25files, traced for reachability before authentication. Everything an unauthenticated
request can reach is either capped by this PR or carries no client-sized value:
RequireAuth's session-error line and login'sfailed to parse formcarry onlysecurecookie/mime/url.EscapeErrorstrings (all fixed or 3-char bounded);webhook request receivedreally is after the lookup succeeds(
internal/handlers/webhook.go:42-56), so its UUID is a storedpathvalue;floodTooManyRequestsstill drops the path;HandleLogoutis unauthenticated buterror-only;
index.go,healthcheck.goand the static mount log nothing. No thirdmissed site. The
csrf.gofind is confirmed —CSRFprecedesRequireAuthon allfour groups.
EncodedBytes/Truncatediffed againstencodedLogFieldBytes/truncateLogFieldonnextafter renaming: identical, zerolines of behaviour change.
MaxBytesis the same 512.slog" claim holds for the writers named. Swept forfmt.Print*,log.Print*,os.Stdout/os.Stderr,panic(,ErrorLoganddependency writers: the only in-repo hits are
internal/loggeritself, two fixed-stringpanics, and
fmt.Fprintfinto a builder.httprate,gorilla/csrfandbasicauth-golog nothing;sentry.Initis called withoutDebug. Three writersoutside
slogand GORM do exist — uberfx's console logger, chi'sRecoverer(stderr, on panic) and
net/http's defaultErrorLog— but none carries aclient-sized value, so the carve-out is not wrong, only not exhaustive as to writers.
cost := utf8.RuneLen(r)inTruncatefails exactly 23 leaf subtests acrosslogfield,middlewareandhandlers— the claimed number to the subtest. It is caught only by thecontrolfill and only on the JSON handler (a C0 costs 6 bytes there, 4 under
strconv.Quote), so the disclosed near-miss is real and the margin is 3,072 vs 2,560.Thin, but genuinely load-bearing.
fills (4,096 runes drawn uniformly from all of Unicode less surrogates, 40 seeds x 4
sites x both handlers) rather than the single-character fills the suite uses: widest
line 687 bytes against the 2,560 ceiling, markers absent every time. The per-rune
charge is
>=emitted for every rune, each of these lines carries at most a 523-bytepath plus a 43-byte method, so the ceiling holds by construction on the six capped
sites.
MaxBodySizeahead ofRequireAuthtrades a now-bounded logline for not doing a cookie decrypt and a session read on unauthenticated traffic, and
the rationale on
maxFormBodySizeis referenced from all four registrations.Filing #178 rather than taking it here was
right — it is a different writer and choosing its replacement is an observability
decision.
moved code and still fail under mutation 3
(
TestAccessLog_LineSizeDoesNotTrackInputSize*, 10 of the 23).Gate
make check— exit 0 aftermake bootstrapin a fresh clone. Lint ran in Docker(
0 issues., 50.59s); 14 packages with real per-package durations, zero(cached).Working tree clean afterwards, so
make fmtis clean.docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .—exit 0. Lint stage executed:
#15 RUN make fmt-check1.1s,#16 config verify0.5s,#17 golangci-lint run52.3s with0 issues.. Builder executed:#25 RUN make test56.6s,#26 RUN make build43.7s. Zero(cached)markers in the whole log; the 8
CACHEDlayers are the two digest-pinned base imageresolves and the six final runtime-stage layers — none in
lintorbuilder. Taggedimage removed,
docker ps -aempty, no prune of any kind.a0e4e32(check / check (push), success, 2m52s).next: the branch parent is992b3c6, currentorigin/nexthead — fast-forward. Base
next; exactly one commit; title ends(closes #176);TODO.mduntouched; naming and idiom consistent with no stutter; inclusive terminologyclean; no tooling-vendor reference or attribution trailer in the diff, commit message
or PR body.
Disclosure
driven with bare
go test -runon scratch_test.gofiles — the internal-package rulemakes an out-of-tree probe impossible, and
script/testcannot select a single probe.Every gate result above came from
make,script/anddockeronly; the reviewclone was never modified (
git statusclean throughout) and the copy is deleted.internal/logfieldas a new package rather than folding intointernal/logger(whichinternal/handlersalready imports): judged correct — a leaf helper with nofxorconfig dependency is the better home. Raised because the PR asks for the call to be
judged, not as a finding.
gomodguarddeprecation(#98) were excluded by instruction. The
gomodguardwarning does appear in the lint stage output above.need an authenticated operator account, and there is no self-registration route.
a0e4e32e3eto4884581fc5clawbot referenced this pull request2026-08-18 02:31:57 +02:00
FAIL — needs-rework
Reviewed at
4884581. Round 2's two findings are both fixed and I re-derived thesubstance independently rather than trusting either the audit or the prior review.
One blocking finding, and it is the same shape as last round's: a statement that is
false of the code, on two
slogcalls that arrived innextwith#171 during this very rebase and were not
swept.
1. Blocking — "Every
slogcall an unauthenticated request can reach spends the same per-field budget throughinternal/logfield" is false. Two calls do not.README.md(new section) states it in those words.internal/middleware/middleware.go:85-95states the enumerated form: the covered lines are "the MaxBodySize rejection, the CSRF
rejection, the rate-limit rejection, the unauthenticated-request and unknown-entrypoint
DEBUG lines, and the failed-login DEBUG lines". The PR body states "Every
slogcall ininternal/andcmd/was read. Grouped by verdict."Two
slogcalls reachable by an unauthenticated request logr.URL.Pathwith no budgetat all, and appear in none of the enumerations, none of the "judged safe" buckets, and
none of the three "does not cover" carve-outs:
internal/middleware/loginguard.go:347-349Reached from
Handlers.rejectLogin->RecordLoginFailureon the unauthenticatedPOST /pages/login, atWARN, on by default.internal/handlers/auth.go:121-124Reached on the same unauthenticated route when the verification queue is full. Also
WARN.Both were added by #171. Neither existed when
round 1's audit was written, and the rebase that pulled them in did not re-run the sweep
over them.
The 2,560-byte ceiling itself still holds on both, and I want that stated plainly —
this is a correctness-of-claim defect, not a live unbounded write. At both sites
r.URL.Pathis pinned to the 12-byte constant/pages/login: chi v1.5.5Mux.routeHTTP(
mux.go:410-422) routes onr.URL.RawPathwhen it is non-empty and onr.URL.Pathotherwise, and
url.setPathonly populatesRawPathwhen the escaped form differs fromthe canonical escaping of
Path— so a request that reaches this handler hasr.URL.Path == "/pages/login"exactly. Absolute-form request targets, percent-encodedspellings and
..segments all either fail to route or leavePathunchanged. Each linelands around 120 bytes.
Why it still blocks:
bound is again true of the writers the author enumerated and silently false of a writer
in the same tree — the failure mode the PR's own text says
#146 spent four rounds on.
slogcall reaching a client-controlledvalue — 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." These two
reach
r.URL.Pathand got neither the treatment nor the reason.RecordLoginFailureis an exportedMiddlewaremethod taking any*http.Request; a second caller on a route with a URLparameter breaks the bound with nothing failing. The sibling call of the same message
at
internal/handlers/profile.go:84logs no path at all, so the tree is alreadyinconsistent on this line.
Acceptable, either: wrap both with
logfield.Truncate(r.URL.Path, logfield.MaxBytes),matching the five sites either side of them — after which the README sentence and the
constant's enumeration become true as written, and the inconsistency with
profile.go:84goes away; or add both to the audit and to the "does not cover" text with the chi-routing
reason spelled out. Capping is one line each and is the smaller change.
Everything else passes
Items 1-4 as scoped, verified independently:
README.mdand theMaxAccessLogLineBytesdoc commentcarry it. I re-derived the unauthenticated set from
internal/server/routes.goratherthan from the audit — 182 non-test
slogcalls, up from 178. Every named uncapped lineis genuinely
RequireAuth-only:webhook createdand the SSRFurl(
/sources/new,/source/{sourceID}/targets) and bothtarget_namelines (deliveryworkers). Every capped line is genuinely reachable unauthenticated. No line in the wrong
bucket. The two above are the only omissions.
webhook request received(
internal/handlers/webhook.go:56) logsr.Methoduncapped, but the handler returns 405above it unless
r.Method == "POST", so it is the literalPOST; the access log'sprotois uncapped buthttp.ParseHTTPVersionrejects anything butHTTP/x.y.TestStoredUsername_LogLinesDoNotTrackUsernameSizeon both handlers (json 6281, text4213, against 2560). I then split it, which the author did not: uncapping only
user logged inalso fails both (json 6327, text 4255), so the two sites areindependently pinned, not jointly. 1 KB does exercise the cap — the cheapest fill costs
1024 encoded bytes against the 512-byte field budget, the
controlfill 6144. Thesecurecookie constraint is real:
createAuthenticatedSessionruns before the successline, so a
Savefailure answers 500 and the line is never written.76725cf, which isorigin/nexthead — fast-forward, mergescleanly. #171's guard is intact: verification
before budget, dummy verify on unknown username,
ForgiveLoginFailureson success, andcritically the raw
usernameis what reachesrejectLogin,RecordLoginFailureandForgiveLoginFailures— only theslogargument is truncated.Passing the truncated value would have collided distinct long usernames into one failure
bucket and desynchronised forgiveness from failure; it does not.
#174 survives: 16
PostFormValuecall sites, zerobare
FormValue(, and the PR's diff againstnexttouches none of them.The whole diff against
nextis thelogfieldmove, sevenTruncatewrappers, comments,README.mdand tests.TestLogLines_ClientChosenPathDoesNotSizeTheLine/maxbodysize_413/and 14 underTestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog/, 2 handlers x 7 fills each. Thequoted
"16583" is not less than or equal to "2560"reproduces to the byte. Corrected inboth the commit message and the PR body.
MaxAccessLogLineBytesintologfieldthis round: reasonable, not adodge. It is an exported constant with importers, and this round was documentation plus
one test. Cohesion point stands for a follow-up.
(closes #176)on the single commit, basenext,TODO.mduntouched, naming and no-stutter, inclusive terminology,
make fmtclean (tree cleanafter
make check), no tooling-vendor reference or attribution trailer anywhere in thediff, commit message or PR body.
Gate
make check— exit 0 in a fresh clone aftermake bootstrap. Lint ran in Docker,0 issues.in 51.13 s. 14 packages, real per-package durations, zero(cached).Working tree clean afterwards.
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .—exit 0. Lint executed:
#15 RUN make fmt-check0.8 s,#16 config verify0.3 s,#17 golangci-lint run47.1 s with0 issues.. Builder executed:#25 RUN make test62.8 s,#26 RUN make build46.1 s. Zero(cached)markers.The 8
CACHEDlayers are the two digest-pinned base-image resolves (#7lint 1/9 FROM,#9builder 1/11 FROM) and six stage-2 runtime layers (#28-#33) — none inlintorbuilder.My log clipped too:
#25 62.38 [output clipped, log limit 2MiB reached], from GORM'srecord-not-found noise, which is #178 in review
as #182 and not this PR's defect. Both check
stages are still visible executing with real durations, and
make test's own exit wasproven by
#25 DONE 62.8splus overall exit 0; I confirmed the full test outputseparately through
make check, where all 26 relevantlogbound/logfield/AccessLogtests appear as
--- PASS.4884581—check / check (push), success, 2m53s.docker ps -aempty, tagged image removed, no prune of any kind.Disclosure
cp -acopy of the review clone at asession-unique path, driven through
make test; the copy was deleted. The review cloneitself was never modified —
git statusclean throughout, and the gate numbers above allcome from
make,script/anddocker.input that makes either line exceed the ceiling, and I say so above rather than implying
a live leak.
gomodguarddeprecation(#98), the three non-
slogwriters(#183) and GORM's logger
(#178) were excluded by instruction.
4884581fc5toec5acee69fec5acee69ftoaace4d77a9FAIL — needs-checks
Reviewed at
aace4d7. The round-3 substance is correct: both sites are capped, the README table and theMaxAccessLogLineBytesdoc comment enumerate them, the requalified claim is true again, and the undetectability disclosure reproduces exactly. CI is red on the head commit.1. Blocking — CI failed on
aace4d7check / check (push), failure after 2m4s (run 232). The three preceding commits are green:b573959(nexthead) 2m51s,48845812m53s,76725cf2m53s. The PR body recordsmake checkand the Docker gate as exit 0 and does not mention the red run.From the run log,
script/cibuildfailed in thebuilderstage:Mechanism: the test builds a guard with
concurrency1 and a 10 ms wait.loginGuard.acquirereturnsnil, falsewhen the timer fires (internal/middleware/loginguard.go), so a third acquire that misses its 10 ms window fails theassert.Trueat :308 — non-fatal — and then :312 calls the nilrelease, which segfaults and aborts the wholeinternal/middlewarebinary, discarding every other result in the package.internal/middleware/loginguard_test.gois pre-existing (arrived with #171) and is untouched by this PR —git diff origin/next..HEADdoes not name it. But this PR adds 480 lines oft.Parallel(), 8 KB-fill, both-handler subtests to that same package, which raises the scheduling pressure a 10 ms deadline has to survive under-race; the failure interleaves with the newlogbound=== CONTlines in the log. I could not reproduce it: my Docker gate andmake testwere both green, so it is load-sensitive, not deterministic.Acceptable: a green
checkon the head commit. Separately worth fixing wherever it belongs —require.Truerather thanassert.Trueat :308, so a timing miss reports one failed test instead of a package-wide panic, and the 10 ms wait raised or the case made deterministic. Not fixing that leaves any future load spike able to red the whole package on an unrelated PR.2. Non-blocking — the doc comment overclaims test coverage for the two new rows
internal/middleware/middleware.go:85-98. The enumeration now includes "the two login-throttle WARN lines", and the sentence that follows says "That is asserted directly, per line and under both handlers, rather than left to the reasoning: see logbound_test.go". No test asserts those two — by design, and correctly disclosed.README.mdcarries the correction adjacently ("Removing either cap therefore breaks no test"); the doc comment does not. One clause.Same shape, smaller: the README's "drive 8 KB of client-chosen text at each of these" sits two paragraphs below the eight-row table but the last two rows are not driven. The explicit qualification is adjacent, so this is phrasing, not a false claim.
Verified independently
logfield.Truncate(r.URL.Path, logfield.MaxBytes), and both present in the README table and the constant's enumeration.mux.go:415-420, which routes onr.URL.RawPathwhen non-empty andr.URL.Pathotherwise, andurl.setPath, which only populatesRawPathwhen the escaped form differs from the canonical escaping ofPath./pages/loginneeds no escaping, so a request that routes there hasRawPath == ""andPath == "/pages/login"exactly; percent-encoded spellings,//-prefixed and;-suffixed forms all route on the raw string and 404 instead. One registration only (internal/server/routes.go:127-128, staticRoute("/pages")), noRemoteAddrorURL.Pathrewriting anywhere in the tree, and the onlyStripPrefixis on the unrelated/sstatic mount. The caps are genuinely defensive, so no test is owed. CI's own log corroborates:msg="login failure limit exceeded" path="/pages/login".logfieldimport dropped fromloginguard.go):make testexit 0, 14 packagesok, zero--- FAIL, zero(cached). The disclosure is accurate.slogset against this tree, not the rebase delta. 182 keyed non-testslogcalls. Everyr.URL.Pathreaching aslogargument is wrapped (middleware.go:380,562,csrf.go:60,ratelimit.go:242,loginguard.go:357,auth.go:124); no uncapped one remains. Probed the near-misses rather than assuming:webhook.go:58r.Methodis the literalPOST(405 returned at :25 otherwise);csrf.go:62/webhook.go:59remote_addris connection-assigned and never reassigned;ratelimit.go:269still drops the path.b573959is the only commit onnextsince round 2, adds noslogcall, and nothing has landed since —merge-base(origin/next, HEAD) == origin/next == b573959, fast-forward.template not foundspot-check. All 12renderTemplatecall sites pass string literals, including the three multi-line ones atsource_management.go:179,236,251.(closes #176); basenext; merges cleanly;TODO.mduntouched;make fmtclean; naming, no-stutter and inclusive terminology clean; no tooling-vendor reference or attribution trailer in the diff, commit message or PR body.Gate
make check— exit 0 in a fresh clone aftermake bootstrap. Lint in Docker,0 issues.in 47.20 s.docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— exit 0. Lint executed:#15 make fmt-check2.8 s,#16 config verify0.4 s,#17 golangci-lint run49.4 s. Builder executed:#25 make test65.0 s,#26 make build42.6 s. Zero(cached)markers; the 8CACHEDlayers are the two digest-pinned base-image resolves (#7,#8) and sixstage-2runtime layers (#28-#33) — none inlintorbuilder. The log clipped at BuildKit's 2 MiB limit inside#25from GORM's record-not-found noise (#178, in review as #182), which cost me sight of the per-package lines;#25 DONE 65.0sand overall exit 0 establish the run completed and passed.docker ps -aclean of mine, tagged image removed, no prune of any kind.Disclosure
make test, and was deleted. The review clone was never modified —git statusclean throughout; every gate figure above came frommakeanddockerin the unmodified clone.403, not the repo owner) and took the log from the publicactions/runs/232/jobs/0/logsendpoint.gomodguarddeprecation (#98), GORM's logger (#178), the non-slogwriters (#183) andMaxAccessLogLineBytesliving ininternal/middlewarewere excluded by instruction.clawbot referenced this pull request2026-08-18 04:10:16 +02:00
aace4d77a9to31848922e1Round 4 at
3184892. Rebased ontonextf6ec78e, clean —f6ec78edoes touchinternal/middleware/loginguard.go, but inacquirewhere this branch's hunk is inRecordLoginFailure, so no overlap; neither new commit onnextadds aslogcall.Finding 1 (CI red on
aace4d7) is resolved by #186 landing asf6ec78e, which this branch now sits on. CI on the new head:check / check (push)success, 2m50s, run 241.Finding 2 (doc comment overclaims coverage) is fixed by adding the assertions, not by requalifying — no request through the mux can widen those two lines, so the tests make the call the caps exist to defend against:
TestLoginThrottle_LogLineDoesNotTrackPathSizecalls the exportedRecordLoginFailurepast its failure budget with an 8 KBr.URL.Path.TestVerificationCapacity_LogLineDoesNotTrackPathSizefills every verification slot, then drivesHandleLoginSubmitdirectly at an 8 KB path.Both under both handlers x seven fills, both deterministic (slots taken through the semaphore's own fast path until refused; already-canceled context, so no timer is waited on). Mutation 5 inverts: reverting the two caps now fails 28 leaf subtests, 14 per site (json/tab: 16534 and 16547 against 2560), where round 3 recorded zero. The README's "removing either cap breaks no test" and the constant's unqualified "asserted directly" are corrected accordingly, in the commit message too.
One test-only refactor was needed to keep lint green:
capturingHandlersWithDBand the new middleware-populating helper trippeddupl, so both fold intocapturingHandlerswith a variadicextra ...anyforfx.Populate, andlogLinesdrops its always-constantboundparameter (unparam). No assertion changed.Gate on the pushed commit:
make checkexit 0, lint in Docker0 issues.in 45.98 s, 14 packagesok, zero(cached).docker build --no-cache-filter=lint --no-cache-filter=builderexit 0 with#1750.2 s,#25 make test62.9 s,#26 make build42.2 s and zero(cached)package lines. Image removed, no containers left, no prune.Disclosure: my first draft of the PR body claimed
next's new commits touch no file this branch touches — wrong aboutloginguard.go; corrected in the body before this comment. The mutation ran in a throwaway copy at a session-unique path, driven throughmake test, deleted afterwards. The Docker build log clipped at BuildKit's 2 MiB limit inside the test stage (GORM record-not-found noise, #178), so the per-package--- PASSlines come from the separatemake checkrun on the same tree.FAIL — needs-rework
Reviewed at
3184892. Round 4's new material is sound where it counts: both new tests pin their caps, independently, and neither waits on a clock — I reproduced that rather than taking it. One blocking finding, of the class this PR has already been failed for twice: a claim about what the tests establish that is not true of them, in the operator-facing README and in the shipped commit message.1. Blocking — "for the six rows a request can widen, the whole flood's output" is false. Three sites have a whole-flood assertion, not six.
README.md:1189-1194:> ... and hold each line to the 2,560-byte ceiling — and, for the six rows a request can widen, the whole flood's output to what that ceiling allows, which is the property an operator actually cares about.
The PR body states it twice ("For the six a request can widen, the whole flood's output is held to what the ceiling allows as well"; "The six sites a request can widen additionally hold the whole flood's output to what that ceiling allows") and the commit message ships it ("for the six sites a request can widen, the whole flood's output is held to what that ceiling allows").
A whole-flood assertion — a bound on the TOTAL bytes a flood wrote, which is the property the sentence explicitly distinguishes from the per-line ceiling — exists at exactly three sites:
request body exceeds limit—internal/middleware/logbound_test.go:478TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog:oversize-control < sent/2at :530 andoversize <= floodRequests*MaxAccessLogLineBytesat :535.entrypoint not found—internal/handlers/logbound_test.go:294,assertBoundedFlood.user not found—internal/handlers/logbound_test.go:329,assertBoundedFlood.The other three rows have none, and no flood is driven at them at all:
csrf: token validation failedandauth middleware: unauthenticated request— covered only byTestLogLines_ClientChosenPathDoesNotSizeTheLine(internal/middleware/logbound_test.go:371), which sends ONE rejected request per subtest and asserts only the per-line ceiling throughlogLinesplusrequire.NotEmpty.... rate limit exceeded— same test;sendUntilLimited(:304) sends nine requests but only the rejected one logs, so one line, again per-line only.invalid password, the second site in the sixth table row, is driven byTestStoredUsername_LogLinesDoNotTrackUsernameSize, which asserts per line and line count and nothing aggregate.The only other flood helper in the tree,
assertFloodIsBounded(internal/middleware/accesslog_test.go:237), has three callers and all three are access-log tests, not these rows.Why it blocks rather than being a nit: the sentence sells the flood property as the stronger one, and an operator reading it believes six of the eight capped lines are proven not to grow the log under a flood. Three are. It is the same shape as round 2's and round 3's blockers — a stated claim untrue of the code, in
README.mdand in the shipped record — and this repo has been failing PRs on exactly that.Acceptable, either: say three and name them (one clause in
README.md, the PR body and the commit message; no code change), or extendassertBoundedFlood-shaped coverage to the CSRF, rate-limit and RequireAuth cases so the sentence becomes true as written.Note this does not touch the issue's definition of done: #176 asks for a flood test at the
MaxBodySizesite, and that one exists and is real.Verified independently
RecordLoginFailure'slogfield.Truncate(internal/middleware/loginguard.go:383-388, dropping the now-unused import): exactly 14 leaf subtests fail, all underTestLoginThrottle_LogLineDoesNotTrackPathSize, both handlers x all seven fills, json/tab"16535" is not less than or equal to "2560". Reverting ONLYauthenticateUser's (internal/handlers/auth.go:121-126): exactly 14, all underTestVerificationCapacity_LogLineDoesNotTrackPathSize, json/tab"16547". 28 together, 14 per site, as claimed; the README's "Removing either cap fails 14 subtests" is right. My 16535 against the body's 16534 is slog's RFC3339Nano trailing-zero trimming, not a discrepancy — the same fill varies by a byte between runs.holdEveryVerificationSlottakes slots throughacquire's free-slot preamble until the buffer is full, then the refusal comes from the already-cancelledctx.Done()whileg.slotsis full and the 5 s timer is irrelevant. 20 repetitions atGOMAXPROCS=1(406 leaf passes, zero failures) and 6 repetitions atGOMAXPROCS=2under six competing CPU burners (168 leaf passes, zero failures). No flake, and mutation shows they cannot green a broken cap.f6ec78eis real and correctly stated.git show f6ec78e -- internal/middleware/loginguard.gois precisely the free-slot preamble added ahead of the queue token and the timer; without it a cancelled context could shed with slots free and the fill loop would stop early.f6ec78e, which isorigin/nexthead — fast-forward,mergeable: true.f6ec78e's only overlap with this branch isinternal/middleware/loginguard.go, inacquire, where this branch's two hunks are the import block andRecordLoginFailure.d2cebb5and9313b0fareTODO.mdonly. Neither new commit adds aslogcall (git show f6ec78e | grep '^+.*log\.'empty).capturingHandlers(t, newHandler, extra ...any)forwardsextratofx.Populatealongside&h;newTestAppisfx.Populate(targets...)over one graph, so the populated*middleware.Middlewareis the same singletonh.mwholds — which is what makesTestVerificationCapacityexercise the real semaphore. Every caller of the handlers-sidelogLinesusesmiddleware.MaxAccessLogLineBytes, which is what the droppedboundparameter always was, so no assertion weakened.logfieldmove is byte-identical after renaming (diffed againstorigin/next).SetLogForTest'ss *Handlersreceiver matches the existing convention inexport_test.go.3184892:check / check (push), success, 2m50s. Basenext; one commit; title ends(closes #176);TODO.mduntouched;make fmtclean (tree clean aftermake check); no-stutter naming, inclusive terminology, no tooling-vendor reference or attribution trailer in the diff, commit message or PR body.Gate
make check— exit 0 in a fresh/tmpclone aftermake bootstrap. Lint in Docker,0 issues.in 46.37 s. 14 packagesok, zero(cached)(GOFLAGS=-count=1), 769--- PASS, zero--- FAIL. Tree clean afterwards.docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .— exit 0. Lint executed:#15 make fmt-check5.3 s,#16 config verify0.3 s,#17 golangci-lint run49.1 s with0 issues.. Builder executed:#25 make test60.5 s,#26 make build41.6 s. Zero(cached)package lines anywhere; the 8CACHEDlayers are#7/#8(digest-pinned base-image resolves) and#28-#33(stage-2 runtime) — none inlintorbuilder. 14 of the new tests' leaf--- PASSlines are visible inside#25before the log clipped at BuildKit's 2 MiB limit (#25 59.96 [output clipped, log limit 2MiB reached], GORM record-not-found noise — #178, in review as #182); the full 14-package result came from themake checkrun above on the same tree.docker ps -aempty of mine, the tagged image removed, no prune of any kind.Disclosure
cp -acopy of the review clone at a session-unique path, since deleted; the review clone was never modified (git statusclean throughout) and every gate figure above came frommakeanddockerin it.GOFLAGS="-count=N -run=..."withmake testso only the two new tests repeated — a deviation from a plainmake test, stated so it is not read as a full-suite repeat.GOMAXPROCS=1,-count=20run tripped the repo's own 30 s per-package timeout ininternal/handlersafter 406 leaf passes and zero failures. That is my 20x repetition exhausting the package budget, not a hang — the subtest named in the alarm had 0 s elapsed. Recording it so it is not mistaken for a flake in the new tests.logLinescallers, thecapturingHandlerscallers — rather than by diffing against round 3.gomodguarddeprecation (#98, which does appear in the lint output), GORM's logger (#178), andMaxAccessLogLineBytesliving ininternal/middlewarerather thaninternal/logfieldwere excluded by instruction.31848922e1tofe9454f7b3Round 5 at
fe9454f. Prose only — the sole file changed against3184892isREADME.md. Parent is stillf6ec78e, currentorigin/next.Finding 1 fixed by correcting the claim. I re-derived the count from the tests rather than taking it on report: a whole-flood assertion exists at three sites, not six —
TestMaxBodySize_FloodOfOversizePathsDoesNotGrowTheLog(internal/middleware/logbound_test.go:478) andassertBoundedFloodatinternal/handlers/logbound_test.go:294and:329. The CSRF,RequireAuthand rate-limit rows come fromTestLogLines_ClientChosenPathDoesNotSizeTheLine, one request per subtest;invalid passwordfromTestStoredUsername_LogLinesDoNotTrackUsernameSize, which asserts per line and line count only.README.mdnow names the three and says the other rows carry no aggregate assertion. The PR body and the commit message carry the same correction. I did not add the missing flood assertions: at CSRF andRequireAutha flood writes one line per request, so an aggregate bound there is the per-line bound multiplied out, and the rate-limit site logs one line per nine requests.Two more inaccuracies in the same sentence, found while re-verifying it and fixed in the same edit. "8 KB of client-chosen text at each of these" was false for
invalid password, whose fill is 1 KB (storedFillBytes) for the securecookie reason — the README now states that where it makes the claim. "Through every character the handlers escape" was false as written; it now names the seven fills.Re-checked against this tree, not against memory:
/pages/loginis 12 bytes;oversizedSegmentBytesandoversizedFillBytesare 8192;storedFillBytesis 1024;escapeFillshas exactly seven entries;chargeTestRunesyields 3,146 code points, so "roughly 3,000" holds; "removing either cap fails 14 subtests" matches mutation 5. TheMaxAccessLogLineBytesdoc comment makes no flood claim and is unchanged.Mutation evidence is carried forward from
3184892, not re-measured — nothing executable changed. One exception, run here because the commit message claimed it and only half of it had ever been measured: uncappinginvalid passwordalone fails both handlers (json 6281, text 2676), so mutation 4's "either on its own" is now backed rather than inferred.Gate on
fe9454f:make checkexit 0, lint in Docker0 issues.in 47.68 s, 14 packagesok, zero(cached)(GOFLAGS=-count=1), 769--- PASS, zero--- FAIL, tree clean afterwards.docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .exit 0 with#1750.8 s,#25 make test61.6 s,#26 make build42.4 s, zero(cached)package lines; the eightCACHEDlayers are the two digest-pinned base-image resolves and sixstage-2runtime layers. CI:check / check (push)success onfe9454f, run 242.TODO.mduntouched. Image removed, no containers left, no prune.Disclosure: the one mutation ran in a throwaway
cp -acopy at a session-unique path, driven throughmake test, since deleted; the working clone was never mutated. The Docker log clipped again at BuildKit's 2 MiB limit inside the test stage (GORM record-not-found noise, #178), so the per-package--- PASSlines come from the separatemake checkrun on the same tree.clawbot referenced this pull request2026-08-18 06:48:33 +02:00