From a60b96d3f9c9405f1fb21c02b8afc80ff54b4a1e Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 12 Aug 2026 09:45:30 +0000 Subject: [PATCH] Cap the X-Forwarded-For hop walk at 64 entries (closes #124) 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. --- internal/middleware/ratelimit.go | 20 +++++++++++-- internal/middleware/ratelimit_test.go | 41 +++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/internal/middleware/ratelimit.go b/internal/middleware/ratelimit.go index 6a97bb7..05ad0a9 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -32,6 +32,13 @@ const ( // receiver rate limit. The configured limit is expressed in // requests per minute. receiverRateInterval = 1 * time.Minute + + // maxForwardedHops bounds how many X-Forwarded-For entries the + // chain walk examines. Real chains are one to three hops, but a + // client can pad the header up to MaxHeaderBytes, so without a + // bound every request pays a walk proportional to whatever the + // client sent. + maxForwardedHops = 64 ) // normalizeAddr strips the IPv4-in-IPv6 wrapper and any zone from @@ -70,12 +77,19 @@ func (m *Middleware) isTrustedProxy(addr netip.Addr) bool { // a trusted proxy is the client. A hop that cannot be read as a bare // address ends the walk: past it the chain is not the shape assumed // here, so the caller falls back to the peer address. +// +// Only the last maxForwardedHops entries are examined. A longer chain +// is padding, and running out of hops falls back to the peer address +// the same way an unreadable hop does. func (m *Middleware) forwardedClientAddr( r *http.Request, ) (netip.Addr, bool) { hops := strings.Split( strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",", ) + if len(hops) > maxForwardedHops { + hops = hops[len(hops)-maxForwardedHops:] + } for _, hop := range slices.Backward(hops) { hop = strings.TrimSpace(hop) @@ -113,8 +127,10 @@ func (m *Middleware) clientKey(r *http.Request) string { peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr)) if err != nil { // Not an address we can reason about; key on the raw - // value rather than collapsing such peers into one - // shared bucket. + // value, the most specific identity left. On a + // Unix-socket listener every peer carries the same + // RemoteAddr and so shares one bucket, which is the + // fail-closed direction. return r.RemoteAddr } diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index 69ce76a..64362be 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -8,7 +8,9 @@ import ( "net/http/httptest" "net/netip" "os" + "strings" "testing" + "time" "github.com/stretchr/testify/assert" "sneak.berlin/go/webhooker/internal/config" @@ -568,6 +570,45 @@ func TestRateLimitKey_ChainWalkSkipsClientPrepended(t *testing.T) { ) } +// TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer covers the +// hop-walk cap. A client behind the trusted proxy can pad +// X-Forwarded-For with tens of thousands of trusted-looking hops, +// which costs a walk proportional to the padding and, once the walk +// runs off the left end of the chain, reaches the entry the client +// put there. Capping the walk stops both: the key falls back to the +// peer address, so rotating the head of the chain mints no bucket, +// and the run does not scale with the chain length. +func TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer( + t *testing.T, +) { + t.Parallel() + + // 50k hops is roughly 0.9 MB, within the default + // MaxHeaderBytes. + const hops = 50000 + + padding := strings.Repeat(", 10.0.0.2", hops-1) + + start := time.Now() + + assertSharedBucket( + t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444", + func(i int) map[string]string { + return map[string]string{ + headerXFF: fmt.Sprintf("9.9.9.%d%s", i+1, padding), + } + }, + "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", + ) + + assert.Less( + t, time.Since(start), 2*time.Second, + "the capped walk must not scale with the chain length", + ) +} + // TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves // the receiver limiter uses the same gated key function as the // POST limiters. -- 2.49.1