Residual disclosed by the independent review of #129, which correctly treated it as outside that PR's definition of done. Milestoned 1.0.0 because the endpoint is the unauthenticated receiver and the amplification is in memory, not just CPU.
#124 capped the hop WALK at 64 entries, but the truncation happens after strings.Split. So a ~1 MB X-Forwarded-For of "1," entries still builds a ~500k-element []string — roughly 8 MB — on every request, before anything is capped.
Measured in that review: ~5 ms/req remaining versus ~38 ms/req uncapped, so the hop cap removed about 87% of the CPU burn. The allocation is the part it did not remove, and the allocation is the bigger problem: bounded only by MaxHeaderBytes, it is ~8 MB per concurrent request. A few hundred concurrent requests is enough to matter on a small host, and the receiver requires no authentication.
Definition of done
The rightmost hops are found WITHOUT splitting the whole header — scan right to left (e.g. strings.LastIndexByte) and stop after at most 64 entries, so allocation is bounded by the cap rather than by header length.
Bucket assignment is unchanged for every chain that #124's tests already cover. This is a pure allocation fix; if any existing rate-limit test changes behaviour, something is wrong.
A test asserting a ~1 MB header does not allocate proportionally — testing.AllocsPerRun or a bounded-allocation assertion, not a timing assertion.
The existing 50k-hop test must still pass unchanged.
Implementation requirements
Smallest change that closes it. internal/middleware/ratelimit.go has now passed two adversarial reviews; do not restructure it.
Do NOT change IPv6 bucketing — that is #125 and is awaiting an owner decision on the same function.
Branch from next, PR based on next, single commit, title ending (closes #N).
Gate on make check plus the Docker lint path with the cache defeated (#119).
Residual disclosed by the independent review of https://git.eeqj.de/sneak/webhooker/pulls/129, which correctly treated it as outside that PR's definition of done. Milestoned 1.0.0 because the endpoint is the unauthenticated receiver and the amplification is in memory, not just CPU.
https://git.eeqj.de/sneak/webhooker/issues/124 capped the hop WALK at 64 entries, but the truncation happens after `strings.Split`. So a ~1 MB `X-Forwarded-For` of `"1,"` entries still builds a ~500k-element `[]string` — roughly 8 MB — on every request, before anything is capped.
Measured in that review: ~5 ms/req remaining versus ~38 ms/req uncapped, so the hop cap removed about 87% of the CPU burn. The allocation is the part it did not remove, and the allocation is the bigger problem: bounded only by `MaxHeaderBytes`, it is ~8 MB per concurrent request. A few hundred concurrent requests is enough to matter on a small host, and the receiver requires no authentication.
## Definition of done
- The rightmost hops are found WITHOUT splitting the whole header — scan right to left (e.g. `strings.LastIndexByte`) and stop after at most 64 entries, so allocation is bounded by the cap rather than by header length.
- Bucket assignment is unchanged for every chain that https://git.eeqj.de/sneak/webhooker/issues/124's tests already cover. This is a pure allocation fix; if any existing rate-limit test changes behaviour, something is wrong.
- A test asserting a ~1 MB header does not allocate proportionally — `testing.AllocsPerRun` or a bounded-allocation assertion, not a timing assertion.
- The existing 50k-hop test must still pass unchanged.
## Implementation requirements
- Smallest change that closes it. `internal/middleware/ratelimit.go` has now passed two adversarial reviews; do not restructure it.
- Do NOT change IPv6 bucketing — that is https://git.eeqj.de/sneak/webhooker/issues/125 and is awaiting an owner decision on the same function.
- Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`.
- Do not modify `TODO.md` (see https://git.eeqj.de/sneak/webhooker/issues/112).
- Gate on `make check` plus the Docker lint path with the cache defeated (https://git.eeqj.de/sneak/webhooker/issues/119).
clawbot
added this to the 1.0.0 milestone 2026-08-12 11:54:06 +02:00
clawbot
self-assigned this 2026-08-12 11:54:06 +02:00
forwardedClientAddr now cuts entries off the right end of each X-Forwarded-For value with strings.LastIndexByte and walks multiple values in reverse, instead of joining and splitting. The cap, the skip rules and the fail-closed fallbacks are unchanged, so no existing rate-limit assertion moved.
Verified: new test measures TotalAlloc over 50 keyings of a 1,000,007-byte chain — 1,606,043 bytes/call before, 16 bytes/call after, asserted < 4096. Reverting the implementation to strings.Split fails that test. make check exits 0, and docker build --no-cache-filter=lint,builder exits 0 with both stages executed (0 issues. from the lint stage, middleware tests passing in the builder stage).
PR: https://git.eeqj.de/sneak/webhooker/pulls/136
`forwardedClientAddr` now cuts entries off the right end of each `X-Forwarded-For` value with `strings.LastIndexByte` and walks multiple values in reverse, instead of joining and splitting. The cap, the skip rules and the fail-closed fallbacks are unchanged, so no existing rate-limit assertion moved.
Verified: new test measures `TotalAlloc` over 50 keyings of a 1,000,007-byte chain — 1,606,043 bytes/call before, 16 bytes/call after, asserted < 4096. Reverting the implementation to `strings.Split` fails that test. `make check` exits 0, and `docker build --no-cache-filter=lint,builder` exits 0 with both stages executed (`0 issues.` from the lint stage, middleware tests passing in the builder stage).
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.
Residual disclosed by the independent review of #129, which correctly treated it as outside that PR's definition of done. Milestoned 1.0.0 because the endpoint is the unauthenticated receiver and the amplification is in memory, not just CPU.
#124 capped the hop WALK at 64 entries, but the truncation happens after
strings.Split. So a ~1 MBX-Forwarded-Forof"1,"entries still builds a ~500k-element[]string— roughly 8 MB — on every request, before anything is capped.Measured in that review: ~5 ms/req remaining versus ~38 ms/req uncapped, so the hop cap removed about 87% of the CPU burn. The allocation is the part it did not remove, and the allocation is the bigger problem: bounded only by
MaxHeaderBytes, it is ~8 MB per concurrent request. A few hundred concurrent requests is enough to matter on a small host, and the receiver requires no authentication.Definition of done
strings.LastIndexByte) and stop after at most 64 entries, so allocation is bounded by the cap rather than by header length.testing.AllocsPerRunor a bounded-allocation assertion, not a timing assertion.Implementation requirements
internal/middleware/ratelimit.gohas now passed two adversarial reviews; do not restructure it.next, PR based onnext, single commit, title ending(closes #N).TODO.md(see #112).make checkplus the Docker lint path with the cache defeated (#119).PR: #136
forwardedClientAddrnow cuts entries off the right end of eachX-Forwarded-Forvalue withstrings.LastIndexByteand walks multiple values in reverse, instead of joining and splitting. The cap, the skip rules and the fail-closed fallbacks are unchanged, so no existing rate-limit assertion moved.Verified: new test measures
TotalAllocover 50 keyings of a 1,000,007-byte chain — 1,606,043 bytes/call before, 16 bytes/call after, asserted < 4096. Reverting the implementation tostrings.Splitfails that test.make checkexits 0, anddocker build --no-cache-filter=lint,builderexits 0 with both stages executed (0 issues.from the lint stage, middleware tests passing in the builder stage).clawbot referenced this issue2026-08-17 23:50:11 +02:00