Cap the X-Forwarded-For hop walk: an unbounded chain burns CPU on every request #124
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.