Scan the X-Forwarded-For chain without splitting it (closes #133)
All checks were successful
check / check (push) Successful in 3m20s

The 64-hop cap bounded the walk but not the allocation: the chain was
split before it was capped, so a padded header cost a []string
proportional to its whole length on every request to the
unauthenticated receiver.

Cut hops off the right end of each header value in place with
strings.LastIndexByte instead, and walk multiple header values in
reverse rather than joining them. Bucket assignment is unchanged: the
walk still counts every comma-separated entry against the cap, skips
empty ones, stops at the first hop that is not a trusted proxy, and
falls back to the peer on an unreadable hop or an exhausted cap.

Measured over a 1 MB chain: 1,606,043 bytes allocated per call before,
16 bytes after.
This commit is contained in:
2026-08-12 10:04:55 +00:00
parent fd6397154a
commit b1eb2466f6
3 changed files with 99 additions and 17 deletions

View File

@@ -25,6 +25,11 @@ func IPFromHostPort(hp string) string {
return ipFromHostPort(hp) return ipFromHostPort(hp)
} }
// ClientKeyForTest exposes clientKey for testing.
func ClientKeyForTest(m *Middleware, r *http.Request) string {
return m.clientKey(r)
}
// IsClientTLS exposes isClientTLS for testing. // IsClientTLS exposes isClientTLS for testing.
func IsClientTLS(r *http.Request) bool { func IsClientTLS(r *http.Request) bool {
return isClientTLS(r) return isClientTLS(r)

View File

@@ -81,17 +81,32 @@ func (m *Middleware) isTrustedProxy(addr netip.Addr) bool {
// Only the last maxForwardedHops entries are examined. A longer chain // Only the last maxForwardedHops entries are examined. A longer chain
// is padding, and running out of hops falls back to the peer address // is padding, and running out of hops falls back to the peer address
// the same way an unreadable hop does. // the same way an unreadable hop does.
//
// The entries are cut off the right end of each header value in place
// rather than split out of it: the receiver is unauthenticated and a
// client can pad the header up to MaxHeaderBytes, so splitting would
// allocate in proportion to the padding (about 8 MB for a 1 MB
// header) before the cap could discard any of it. Multiple header
// values are walked in reverse for the same reason, since joining
// them copies the whole chain.
func (m *Middleware) forwardedClientAddr( func (m *Middleware) forwardedClientAddr(
r *http.Request, r *http.Request,
) (netip.Addr, bool) { ) (netip.Addr, bool) {
hops := strings.Split( seen := 0
strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",",
) for _, value := range slices.Backward(
if len(hops) > maxForwardedHops { r.Header.Values("X-Forwarded-For"),
hops = hops[len(hops)-maxForwardedHops:] ) {
for last := false; !last && seen < maxForwardedHops; seen++ {
hop := value
comma := strings.LastIndexByte(value, ',')
if comma < 0 {
last = true
} else {
hop, value = value[comma+1:], value[:comma]
} }
for _, hop := range slices.Backward(hops) {
hop = strings.TrimSpace(hop) hop = strings.TrimSpace(hop)
if hop == "" { if hop == "" {
continue continue
@@ -106,6 +121,7 @@ func (m *Middleware) forwardedClientAddr(
return addr, true return addr, true
} }
} }
}
return netip.Addr{}, false return netip.Addr{}, false
} }

View File

@@ -8,6 +8,7 @@ import (
"net/http/httptest" "net/http/httptest"
"net/netip" "net/netip"
"os" "os"
"runtime"
"strings" "strings"
"testing" "testing"
"time" "time"
@@ -609,6 +610,66 @@ func TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer(
) )
} }
// TestRateLimitKey_LongChainAllocationIsBounded is the allocation
// half of the hop cap. Capping the walk still left every request
// paying for the whole header the client sent, because the chain was
// split before it was capped: about 8 MB of []string for the 1 MB a
// default MaxHeaderBytes allows, on the unauthenticated receiver.
//
// Bytes are the measurement, not allocation count: strings.Split of a
// 1 MB chain is a single allocation, so testing.AllocsPerRun scores
// it as cheap. The test is deliberately sequential — it reads
// process-wide counters, and Go runs this package's parallel tests
// only after the sequential ones finish.
//
//nolint:paralleltest // reads process-wide allocation counters
func TestRateLimitKey_LongChainAllocationIsBounded(t *testing.T) {
// 100k hops of ", 10.0.0.2" is roughly 1 MB.
const (
hops = 100000
iterations = 50
maxBytesPerCall = 4096
)
m := rateLimitMiddleware(t, &config.Config{
TrustedProxies: trustedProxies("10.0.0.0/8"),
})
req := httptest.NewRequestWithContext(
context.Background(), http.MethodPost, loginPath, nil,
)
req.RemoteAddr = "10.0.0.1:44444"
req.Header.Set(
headerXFF, "9.9.9.9"+strings.Repeat(", 10.0.0.2", hops),
)
var before, after runtime.MemStats
var key string
runtime.ReadMemStats(&before)
for range iterations {
key = middleware.ClientKeyForTest(m, req)
}
runtime.ReadMemStats(&after)
perCall := (after.TotalAlloc - before.TotalAlloc) / iterations
assert.Less(
t, perCall, uint64(maxBytesPerCall),
"a %d-byte X-Forwarded-For must not allocate in proportion "+
"to its length, but cost %d bytes per call",
len(req.Header.Get(headerXFF)), perCall,
)
assert.Equal(
t, "10.0.0.1", key,
"the padded chain must still fall back to the peer address",
)
}
// TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves // TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves
// the receiver limiter uses the same gated key function as the // the receiver limiter uses the same gated key function as the
// POST limiters. // POST limiters.