From fd6397154a393364e5991ac9b8f60d7e9244691e Mon Sep 17 00:00:00 2001
From: clawbot <clawbot@noreply.example.org>
Date: Wed, 12 Aug 2026 11:53:48 +0200
Subject: [PATCH] Cap the X-Forwarded-For hop walk at 64 entries (closes #124)

The walk now keeps only the rightmost 64 hops, so an attacker-supplied chain cannot burn unbounded CPU in the rate-limit key function. Running off the end of the truncated slice falls back to the peer address, the same fail-closed direction the rest of the function takes. Also corrects the unparseable-RemoteAddr comment, which overclaimed about Unix-socket peers.
---
 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.