New maxForwardedHops = 64 constant. forwardedClientAddr now examines only the last 64 entries of the chain; longer chains are truncated from the left before the walk. Running out of hops returns (Addr{}, false), so clientKey falls back to the peer address — the same fail-closed direction an unreadable hop already took. Bucket assignment for real chains (1-3 hops) is unchanged.
Corrected the comment on the unparseable-RemoteAddr fallback. It claimed keying on the raw value avoids collapsing those peers into one bucket; on a Unix-socket listener every peer carries the same RemoteAddr and does share one bucket. Behaviour unchanged, comment only.
internal/middleware/ratelimit_test.go: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer drives the login limiter from a trusted peer with a 50k-hop chain (~0.9 MB) whose head entry rotates per request and whose remaining hops are all inside the trusted-proxy prefix. Every request must land in the peer's bucket, so the last one is rejected; it also asserts the run completes promptly.
No change to IPv6 bucketing (#125 is a separate, undecided question on the same function). TODO.md untouched.
Mutation evidence
With the truncation removed and everything else identical, make test:
--- FAIL: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer (0.22s)
Messages: a padded X-Forwarded-For chain must fall back to the peer
address, not reach the client-controlled entry at the head of the chain
FAIL sneak.berlin/go/webhooker/internal/middleware 0.293s
The uncapped walk reaches the client-controlled head of the chain, so each rotated value mints a fresh bucket and nothing is rejected. With the cap restored the same test passes in 0.06s — 3.7x faster on the same input.
Gate evidence
make check exit 0, lint 0 issues.
Containerized path with the cache defeated, docker build --no-cache-filter=lint,builder --progress=plain ., exit 0. Both stages executed rather than replaying a cached log:
#21 [lint 8/8] RUN make lint
#21 93.54 0 issues.
#21 DONE 94.2s
#33 [builder 8/10] RUN make test
#33 98.90 --- PASS: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer (0.32s)
#33 98.90 ok sneak.berlin/go/webhooker/internal/middleware 1.467s
#33 DONE 102.4s
The only CACHED layers were dependency-fetch steps ahead of those two RUNs.
Closes https://git.eeqj.de/sneak/webhooker/issues/124.
## What changed
`internal/middleware/ratelimit.go`:
- New `maxForwardedHops = 64` constant. `forwardedClientAddr` now examines only the last 64 entries of the chain; longer chains are truncated from the left before the walk. Running out of hops returns `(Addr{}, false)`, so `clientKey` falls back to the peer address — the same fail-closed direction an unreadable hop already took. Bucket assignment for real chains (1-3 hops) is unchanged.
- Corrected the comment on the unparseable-`RemoteAddr` fallback. It claimed keying on the raw value avoids collapsing those peers into one bucket; on a Unix-socket listener every peer carries the same `RemoteAddr` and does share one bucket. Behaviour unchanged, comment only.
`internal/middleware/ratelimit_test.go`: `TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer` drives the login limiter from a trusted peer with a 50k-hop chain (~0.9 MB) whose head entry rotates per request and whose remaining hops are all inside the trusted-proxy prefix. Every request must land in the peer's bucket, so the last one is rejected; it also asserts the run completes promptly.
No change to IPv6 bucketing (https://git.eeqj.de/sneak/webhooker/issues/125 is a separate, undecided question on the same function). `TODO.md` untouched.
## Mutation evidence
With the truncation removed and everything else identical, `make test`:
```
--- FAIL: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer (0.22s)
Messages: a padded X-Forwarded-For chain must fall back to the peer
address, not reach the client-controlled entry at the head of the chain
FAIL sneak.berlin/go/webhooker/internal/middleware 0.293s
```
The uncapped walk reaches the client-controlled head of the chain, so each rotated value mints a fresh bucket and nothing is rejected. With the cap restored the same test passes in 0.06s — 3.7x faster on the same input.
## Gate evidence
`make check` exit 0, lint `0 issues.`
Containerized path with the cache defeated, `docker build --no-cache-filter=lint,builder --progress=plain .`, exit 0. Both stages executed rather than replaying a cached log:
```
#21 [lint 8/8] RUN make lint
#21 93.54 0 issues.
#21 DONE 94.2s
#33 [builder 8/10] RUN make test
#33 98.90 --- PASS: TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer (0.32s)
#33 98.90 ok sneak.berlin/go/webhooker/internal/middleware 1.467s
#33 DONE 102.4s
```
The only `CACHED` layers were dependency-fetch steps ahead of those two `RUN`s.
The chain walk in the rate-limit key function had no bound on hop
count. It runs whenever the direct peer is a trusted proxy, which is
the normal production deployment, so any client could pad
X-Forwarded-For to MaxHeaderBytes (~50k hops, ~0.9 MB) and make the
key function walk all of it on the unauthenticated receiver endpoint
before the request was rate-limited.
Only the last 64 entries are examined now; real chains are one to
three hops. A chain longer than the cap runs out of hops and falls
back to the peer address, the same fail-closed direction an
unreadable hop already took. Bucket assignment for real chains is
unchanged.
Also corrects the comment on the unparseable-RemoteAddr fallback: it
claimed keying on the raw value avoids collapsing those peers into
one bucket, but on a Unix-socket listener every peer carries the same
RemoteAddr and does share one bucket. The behaviour is fail-closed
and unchanged; only the comment was wrong.
clawbot
self-assigned this 2026-08-12 11:46:17 +02:00
PASS — no findings. Independently mutation-checked in a fresh clone: deleting only the truncation fails TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer at internal/middleware/ratelimit_test.go:405 with the stated message, and reversing it to hops[:maxForwardedHops] fails it too, so the test pins the direction as well as the cap; truncating the left end can only degrade a result to the peer fallback, never select a different address.
Gate on a60b96d: make check exit 0; docker build --no-cache-filter=lint,builder exit 0 with #21 [lint 8/8] RUN make lint -> 0 issues. (DONE 71.9s) and #33 [builder 8/10] RUN make test -> ok sneak.berlin/go/webhooker/internal/middleware 1.103s (DONE 68.7s), zero (cached) markers and no CACHED on either RUN. The new test ran at 0.04s against its 2s bound, so that assertion is not flaky-by-construction.
Disclosure: the Gitea check on this commit is still pending / "Waiting to run" (run 141) and the Actions API returns 403 for me, so the containerized run above is the only executed evidence I could obtain.
PASS — no findings. Independently mutation-checked in a fresh clone: deleting only the truncation fails `TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer` at `internal/middleware/ratelimit_test.go:405` with the stated message, and reversing it to `hops[:maxForwardedHops]` fails it too, so the test pins the direction as well as the cap; truncating the left end can only degrade a result to the peer fallback, never select a different address.
Gate on `a60b96d`: `make check` exit 0; `docker build --no-cache-filter=lint,builder` exit 0 with `#21 [lint 8/8] RUN make lint` -> `0 issues.` (DONE 71.9s) and `#33 [builder 8/10] RUN make test` -> `ok sneak.berlin/go/webhooker/internal/middleware 1.103s` (DONE 68.7s), zero `(cached)` markers and no `CACHED` on either RUN. The new test ran at 0.04s against its 2s bound, so that assertion is not flaky-by-construction.
Disclosure: the Gitea check on this commit is still `pending` / "Waiting to run" (run 141) and the Actions API returns 403 for me, so the containerized run above is the only executed evidence I could obtain.
clawbot
merged commit fd6397154a into next2026-08-12 11:53:48 +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 #124.
What changed
internal/middleware/ratelimit.go:maxForwardedHops = 64constant.forwardedClientAddrnow examines only the last 64 entries of the chain; longer chains are truncated from the left before the walk. Running out of hops returns(Addr{}, false), soclientKeyfalls back to the peer address — the same fail-closed direction an unreadable hop already took. Bucket assignment for real chains (1-3 hops) is unchanged.RemoteAddrfallback. It claimed keying on the raw value avoids collapsing those peers into one bucket; on a Unix-socket listener every peer carries the sameRemoteAddrand does share one bucket. Behaviour unchanged, comment only.internal/middleware/ratelimit_test.go:TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeerdrives the login limiter from a trusted peer with a 50k-hop chain (~0.9 MB) whose head entry rotates per request and whose remaining hops are all inside the trusted-proxy prefix. Every request must land in the peer's bucket, so the last one is rejected; it also asserts the run completes promptly.No change to IPv6 bucketing (#125 is a separate, undecided question on the same function).
TODO.mduntouched.Mutation evidence
With the truncation removed and everything else identical,
make test:The uncapped walk reaches the client-controlled head of the chain, so each rotated value mints a fresh bucket and nothing is rejected. With the cap restored the same test passes in 0.06s — 3.7x faster on the same input.
Gate evidence
make checkexit 0, lint0 issues.Containerized path with the cache defeated,
docker build --no-cache-filter=lint,builder --progress=plain ., exit 0. Both stages executed rather than replaying a cached log:The only
CACHEDlayers were dependency-fetch steps ahead of those twoRUNs.PASS — no findings. Independently mutation-checked in a fresh clone: deleting only the truncation fails
TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeeratinternal/middleware/ratelimit_test.go:405with the stated message, and reversing it tohops[:maxForwardedHops]fails it too, so the test pins the direction as well as the cap; truncating the left end can only degrade a result to the peer fallback, never select a different address.Gate on
a60b96d:make checkexit 0;docker build --no-cache-filter=lint,builderexit 0 with#21 [lint 8/8] RUN make lint->0 issues.(DONE 71.9s) and#33 [builder 8/10] RUN make test->ok sneak.berlin/go/webhooker/internal/middleware 1.103s(DONE 68.7s), zero(cached)markers and noCACHEDon either RUN. The new test ran at 0.04s against its 2s bound, so that assertion is not flaky-by-construction.Disclosure: the Gitea check on this commit is still
pending/ "Waiting to run" (run 141) and the Actions API returns 403 for me, so the containerized run above is the only executed evidence I could obtain.