diff --git a/internal/middleware/export_test.go b/internal/middleware/export_test.go index 2752f1c..222ca64 100644 --- a/internal/middleware/export_test.go +++ b/internal/middleware/export_test.go @@ -25,6 +25,11 @@ func IPFromHostPort(hp string) string { 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. func IsClientTLS(r *http.Request) bool { return isClientTLS(r) diff --git a/internal/middleware/ratelimit.go b/internal/middleware/ratelimit.go index 05ad0a9..e32ae52 100644 --- a/internal/middleware/ratelimit.go +++ b/internal/middleware/ratelimit.go @@ -81,29 +81,45 @@ func (m *Middleware) isTrustedProxy(addr netip.Addr) bool { // 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. +// +// 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( 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:] - } + seen := 0 - for _, hop := range slices.Backward(hops) { - hop = strings.TrimSpace(hop) - if hop == "" { - continue - } + for _, value := range slices.Backward( + r.Header.Values("X-Forwarded-For"), + ) { + for last := false; !last && seen < maxForwardedHops; seen++ { + hop := value - addr, err := netip.ParseAddr(hop) - if err != nil { - return netip.Addr{}, false - } + comma := strings.LastIndexByte(value, ',') + if comma < 0 { + last = true + } else { + hop, value = value[comma+1:], value[:comma] + } - if addr = normalizeAddr(addr); !m.isTrustedProxy(addr) { - return addr, true + hop = strings.TrimSpace(hop) + if hop == "" { + continue + } + + addr, err := netip.ParseAddr(hop) + if err != nil { + return netip.Addr{}, false + } + + if addr = normalizeAddr(addr); !m.isTrustedProxy(addr) { + return addr, true + } } } diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index 64362be..9740f4c 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "net/netip" "os" + "runtime" "strings" "testing" "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 // the receiver limiter uses the same gated key function as the // POST limiters.