Found in the independent review of #122, which introduced the walk. Milestoned 1.0.0 because the milestone shipped the code and the affected endpoint is the unauthenticated receiver.
internal/middleware/ratelimit.go:76-94 walks X-Forwarded-For right to left with no bound on the number of hops.
The walk only runs when the direct peer is a trusted proxy — but that is the normal production deployment, where every client arrives via the proxy. So any client can send a ~0.9 MB X-Forwarded-For (50k hops) and make the key function burn roughly 29 ms of CPU before the request is even rate-limited. Measured under -race during the review: 173 ms for 6 requests.
Bounded by MaxHeaderBytes, and it changes no bucket assignment, so this is a CPU-burn amplification rather than a limiter bypass. It is cheap to close and sits in the request path of an unauthenticated endpoint.
Definition of done
The hop walk stops after a fixed cap (64 hops is ample; real chains are 1-3).
Beyond the cap, fall back to the peer address — the same fail-closed direction the rest of the function already takes. Never fall back to something more permissive.
A test asserting a 50k-hop chain from a trusted peer still keys on the peer address and returns promptly.
Also fold in (same file, same review)
internal/middleware/ratelimit.go:115-117: the comment overclaims. On a Unix-socket listener every RemoteAddr is identical, so those peers do all collapse into one bucket. The behaviour fails closed and is fine; the comment is wrong. Correct it.
Implementation requirements
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. A fully cached script/cibuild is not evidence (#119).
Found in the independent review of https://git.eeqj.de/sneak/webhooker/pulls/122, which introduced the walk. Milestoned 1.0.0 because the milestone shipped the code and the affected endpoint is the unauthenticated receiver.
`internal/middleware/ratelimit.go:76-94` walks `X-Forwarded-For` right to left with no bound on the number of hops.
The walk only runs when the direct peer is a trusted proxy — but that is the normal production deployment, where every client arrives via the proxy. So any client can send a ~0.9 MB `X-Forwarded-For` (50k hops) and make the key function burn roughly 29 ms of CPU before the request is even rate-limited. Measured under `-race` during the review: 173 ms for 6 requests.
Bounded by `MaxHeaderBytes`, and it changes no bucket assignment, so this is a CPU-burn amplification rather than a limiter bypass. It is cheap to close and sits in the request path of an unauthenticated endpoint.
## Definition of done
- The hop walk stops after a fixed cap (64 hops is ample; real chains are 1-3).
- Beyond the cap, fall back to the peer address — the same fail-closed direction the rest of the function already takes. Never fall back to something more permissive.
- A test asserting a 50k-hop chain from a trusted peer still keys on the peer address and returns promptly.
## Also fold in (same file, same review)
`internal/middleware/ratelimit.go:115-117`: the comment overclaims. On a Unix-socket listener every `RemoteAddr` is identical, so those peers do all collapse into one bucket. The behaviour fails closed and is fine; the comment is wrong. Correct it.
## Implementation requirements
- Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`.
- Do not modify `TODO.md` (see https://git.eeqj.de/sneak/webhooker/issues/112).
- Gate on `make check` plus the Docker lint path with the cache defeated. A fully cached `script/cibuild` is not evidence (https://git.eeqj.de/sneak/webhooker/issues/119).
clawbot
added this to the 1.0.0 milestone 2026-08-12 11:36:41 +02:00
clawbot
self-assigned this 2026-08-12 11:36:41 +02:00
forwardedClientAddr now examines only the last maxForwardedHops = 64 entries; a longer chain is truncated from the left before the walk, so running out of hops falls back to the peer address exactly as an unreadable hop already did. Real chains (1-3 hops) key identically to before. The RemoteAddr fallback comment is corrected in the same commit.
Verified: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer sends a 50k-hop chain (~0.9 MB) from a trusted peer with a rotating head entry and all-trusted padding; every request must share the peer's bucket. Mutation-checked — with the truncation removed the test fails (the uncapped walk reaches the client-controlled head and mints a fresh bucket per request, 0.22s), and passes at 0.06s with the cap.
Gate: make check exit 0, and docker build --no-cache-filter=lint,builder exit 0 with both RUN make lint (0 issues., 94.2s) and RUN make test (102.4s) executing rather than replaying a cached log.
IPv6 bucketing untouched, per #125. TODO.md untouched, per #112.
Implemented in https://git.eeqj.de/sneak/webhooker/pulls/129 (base `next`).
`forwardedClientAddr` now examines only the last `maxForwardedHops = 64` entries; a longer chain is truncated from the left before the walk, so running out of hops falls back to the peer address exactly as an unreadable hop already did. Real chains (1-3 hops) key identically to before. The `RemoteAddr` fallback comment is corrected in the same commit.
Verified: `TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer` sends a 50k-hop chain (~0.9 MB) from a trusted peer with a rotating head entry and all-trusted padding; every request must share the peer's bucket. Mutation-checked — with the truncation removed the test fails (the uncapped walk reaches the client-controlled head and mints a fresh bucket per request, 0.22s), and passes at 0.06s with the cap.
Gate: `make check` exit 0, and `docker build --no-cache-filter=lint,builder` exit 0 with both `RUN make lint` (`0 issues.`, 94.2s) and `RUN make test` (102.4s) executing rather than replaying a cached log.
IPv6 bucketing untouched, per https://git.eeqj.de/sneak/webhooker/issues/125. `TODO.md` untouched, per https://git.eeqj.de/sneak/webhooker/issues/112.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Found in the independent review of #122, which introduced the walk. Milestoned 1.0.0 because the milestone shipped the code and the affected endpoint is the unauthenticated receiver.
internal/middleware/ratelimit.go:76-94walksX-Forwarded-Forright to left with no bound on the number of hops.The walk only runs when the direct peer is a trusted proxy — but that is the normal production deployment, where every client arrives via the proxy. So any client can send a ~0.9 MB
X-Forwarded-For(50k hops) and make the key function burn roughly 29 ms of CPU before the request is even rate-limited. Measured under-raceduring the review: 173 ms for 6 requests.Bounded by
MaxHeaderBytes, and it changes no bucket assignment, so this is a CPU-burn amplification rather than a limiter bypass. It is cheap to close and sits in the request path of an unauthenticated endpoint.Definition of done
Also fold in (same file, same review)
internal/middleware/ratelimit.go:115-117: the comment overclaims. On a Unix-socket listener everyRemoteAddris identical, so those peers do all collapse into one bucket. The behaviour fails closed and is fine; the comment is wrong. Correct it.Implementation requirements
next, PR based onnext, single commit, title ending(closes #N).TODO.md(see #112).make checkplus the Docker lint path with the cache defeated. A fully cachedscript/cibuildis not evidence (#119).Implemented in #129 (base
next).forwardedClientAddrnow examines only the lastmaxForwardedHops = 64entries; a longer chain is truncated from the left before the walk, so running out of hops falls back to the peer address exactly as an unreadable hop already did. Real chains (1-3 hops) key identically to before. TheRemoteAddrfallback comment is corrected in the same commit.Verified:
TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeersends a 50k-hop chain (~0.9 MB) from a trusted peer with a rotating head entry and all-trusted padding; every request must share the peer's bucket. Mutation-checked — with the truncation removed the test fails (the uncapped walk reaches the client-controlled head and mints a fresh bucket per request, 0.22s), and passes at 0.06s with the cap.Gate:
make checkexit 0, anddocker build --no-cache-filter=lint,builderexit 0 with bothRUN make lint(0 issues., 94.2s) andRUN make test(102.4s) executing rather than replaying a cached log.IPv6 bucketing untouched, per #125.
TODO.mduntouched, per #112.clawbot referenced this issue2026-08-17 23:50:11 +02:00