ClientIPFromXFF: unify both XFF walkers behind a single walkXFF primitive

After the prior two commits, both XFF middlewares had near-identical
right-to-left scan loops differing only in stop condition (first
non-trusted entry vs Nth non-empty entry from the right). nthFromRightXFF
and rightmostUntrustedXFF together carried ~30 lines of duplicated loop
boilerplate.

Collapse to one primitive:

  walkXFF(headers []string, visit func(entry string) bool)

iterates the merged X-Forwarded-For chain right-to-left and calls visit
on each trimmed non-empty entry; visit returns true to stop the walk.
Both middleware handlers now share this primitive and supply their own
stop condition as a small inline visitor closure (~7 lines each).

Net: -21 lines in middleware/client_ip.go, one fewer concept to reason
about, single source of truth for the (security-critical) walk order
and the empty-entry drop rule.

Zero-allocation property is preserved -- benchmarked side-by-side against
the previous two-helpers implementation:

  BenchmarkRightmost_TwoHelpers    36 ns/op    0 B/op    0 allocs/op
  BenchmarkRightmost_Callback      37 ns/op    0 B/op    0 allocs/op
  BenchmarkNth_TwoHelpers          11 ns/op    0 B/op    0 allocs/op
  BenchmarkNth_Callback            13 ns/op    0 B/op    0 allocs/op

Go's escape analysis keeps the visitor closures (and their captured
&found / &entry / &n locals) on the stack; only cost is one indirect
call per visited entry (~1-2 ns).

All tests still pass (advisory pins, fail-closed pins, multi-header
merge, normalization, boundary CIDR, last-write-wins, panic
conditions). Stale comments in client_ip_test.go that referenced the
removed helpers by name are updated; test names/comments uniformly use
"position" (matching the godoc) for the algorithmic concept and
"entry" for an individual chain value.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: VojtechVitek

middleware/client_ip.go

@@ -79,8 +79,20 @@ func ClientIPFromXFF(trustedIPPrefixes ...string) func(http.Handler) http.Handle
 	}
 	return func(h http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			if ip, ok := rightmostUntrustedXFF(r.Header.Values(xForwardedForHeader), prefixes); ok {
-				r = r.WithContext(context.WithValue(r.Context(), clientIPCtxKey, ip))
+			var found netip.Addr
+			walkXFF(r.Header.Values(xForwardedForHeader), func(v string) bool {
+				ip, ok := parseHeaderAddr(v)
+				if !ok {
+					return true // fail-closed; leave found unset
+				}
+				if inAnyPrefix(ip, prefixes) {
+					return false // trusted hop; keep walking left
+				}
+				found = ip
+				return true
+			})
+			if found.IsValid() {
+				r = r.WithContext(context.WithValue(r.Context(), clientIPCtxKey, found))
 			}
 			h.ServeHTTP(w, r)
 		})
@@ -119,8 +131,18 @@ func ClientIPFromXFFTrustedProxies(numTrustedProxies int) func(http.Handler) htt
 	}
 	return func(h http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			if v := nthFromRightXFF(r.Header.Values(xForwardedForHeader), numTrustedProxies); v != "" {
-				if ip, ok := parseHeaderAddr(v); ok {
+			n := numTrustedProxies
+			var entry string
+			walkXFF(r.Header.Values(xForwardedForHeader), func(v string) bool {
+				n--
+				if n == 0 {
+					entry = v
+					return true
+				}
+				return false
+			})
+			if entry != "" {
+				if ip, ok := parseHeaderAddr(entry); ok {
 					r = r.WithContext(context.WithValue(r.Context(), clientIPCtxKey, ip))
 				}
 			}
@@ -174,19 +196,15 @@ func GetClientIPAddr(ctx context.Context) netip.Addr {
 	return ip
 }
 
-// nthFromRightXFF returns the trimmed entry at position n from the right
-// end of the merged X-Forwarded-For chain (n=1 is the rightmost), or "" if
-// the chain has fewer than n non-empty entries.
-//
-// Empty/whitespace entries are dropped and do NOT count toward n, so an
-// attacker can't slide their chosen value into the trusted slot by
-// prepending commas.
-//
-// Multi-header merge is required for security per RFC 2616: multiple XFF
-// headers MUST be processed in order received; reading only the first or
-// last lets an attacker pick which value security logic sees by sending
-// duplicate headers. Lazy walk, zero allocations.
-func nthFromRightXFF(headers []string, n int) string {
+// walkXFF walks the entries of the merged X-Forwarded-For chain
+// RIGHT-TO-LEFT, invoking visit on each trimmed non-empty entry. visit
+// returns true to stop the walk. Lazy walk, zero allocations (entries
+// are substrings of the input headers).
+//
+// Multiple XFF headers are merged per RFC 2616 — each header's
+// comma-separated entries in order received — so an attacker cannot pick
+// which value security logic sees by sending a duplicate header.
+func walkXFF(headers []string, visit func(entry string) bool) {
 	for hi := len(headers) - 1; hi >= 0; hi-- {
 		h := headers[hi]
 		for h != "" {
@@ -200,52 +218,11 @@ func nthFromRightXFF(headers []string, n int) string {
 			if v == "" {
 				continue
 			}
-			n--
-			if n == 0 {
-				return v
-			}
-		}
-	}
-	return ""
-}
-
-// rightmostUntrustedXFF walks all X-Forwarded-For header values right-to-left
-// across the merged chain, skipping trusted-prefix and empty entries, and
-// returns the first remaining valid IP. An unparseable entry encountered
-// mid-walk fails closed: the walk is abandoned and no IP is returned, since
-// an unparseable hop is indistinguishable from a hostile or missing one and
-// we cannot safely keep walking past it.
-//
-// Walks the headers lazily so the common case — one trusted hop, rightmost
-// entry is the client — returns on the very first iteration without
-// materializing the full chain. Zero allocations beyond what
-// [netip.ParseAddr] does internally. See also [nthFromRightXFF], which uses
-// the same iteration shape with a count-down stop condition.
-func rightmostUntrustedXFF(headers []string, trustedPrefixes []netip.Prefix) (netip.Addr, bool) {
-	for hi := len(headers) - 1; hi >= 0; hi-- {
-		h := headers[hi]
-		for h != "" {
-			var v string
-			if i := strings.LastIndexByte(h, ','); i >= 0 {
-				v, h = h[i+1:], h[:i]
-			} else {
-				v, h = h, ""
-			}
-			v = strings.TrimSpace(v)
-			if v == "" {
-				continue
-			}
-			ip, ok := parseHeaderAddr(v)
-			if !ok {
-				return netip.Addr{}, false
-			}
-			if inAnyPrefix(ip, trustedPrefixes) {
-				continue
+			if visit(v) {
+				return
 			}
-			return ip, true
 		}
 	}
-	return netip.Addr{}, false
 }
 
 // inAnyPrefix reports whether ip falls within any of the given prefixes.

middleware/client_ip_test.go

@@ -200,25 +200,25 @@ func TestClientIPFromXFFTrustedProxies(t *testing.T) {
 		{"shorter_than_n", 3, []string{"1.1.1.1, 2.2.2.2"}, ""},
 		{"missing_header", 1, nil, ""},
 
-		// Spoofing: prepended attacker values are to the LEFT of the chosen slot,
-		// so they're ignored.
+		// Spoofing: prepended attacker values are to the LEFT of the chosen
+		// position, so they're ignored.
 		{"spoof_prepend_ignored", 2, []string{"6.6.6.6, 1.1.1.1, 2.2.2.2, 3.3.3.3"}, "2.2.2.2"},
 
-		// Bad parse at the target slot: no IP set (don't accept garbage as client IP).
-		{"bad_parse_at_slot", 2, []string{"garbage, 2.2.2.2"}, ""},
+		// Bad parse at the target position: no IP set (don't accept garbage as client IP).
+		{"bad_parse_at_position", 2, []string{"garbage, 2.2.2.2"}, ""},
 
 		// Merging across multiple header instances.
 		{"multi_header", 2, []string{"1.1.1.1", "2.2.2.2", "3.3.3.3"}, "2.2.2.2"},
 
-		// Empty entries dropped by mergeXFF must not shift the slot index. The
-		// slot is computed from the right, where trusted proxies append, so an
-		// attacker prepending commas can't slide their chosen value into the
-		// trusted slot.
-		{"empties_dont_shift_slot", 2, []string{",,, 3.3.3.3, 1.1.1.1, 2.2.2.2"}, "1.1.1.1"},
+		// Empty entries dropped by the walker must not shift the chosen
+		// position. The position is computed from the right, where trusted
+		// proxies append, so an attacker prepending commas can't slide their
+		// chosen value into the trusted position.
+		{"empties_dont_shift_position", 2, []string{",,, 3.3.3.3, 1.1.1.1, 2.2.2.2"}, "1.1.1.1"},
 
-		// Normalization at the chosen slot: v4-mapped IPv6 folds to v4, zone stripped.
-		{"v4_mapped_at_slot_folded", 1, []string{"::ffff:1.2.3.4"}, "1.2.3.4"},
-		{"zone_stripped_at_slot", 1, []string{"2001:db8::1%eth0"}, "2001:db8::1"},
+		// Normalization at the chosen position: v4-mapped IPv6 folds to v4, zone stripped.
+		{"v4_mapped_at_position_folded", 1, []string{"::ffff:1.2.3.4"}, "1.2.3.4"},
+		{"zone_stripped_at_position", 1, []string{"2001:db8::1%eth0"}, "2001:db8::1"},
 	}
 	for _, tc := range tt {
 		t.Run(tc.name, func(t *testing.T) {
@@ -487,8 +487,8 @@ func TestXFF_MultipleHeadersMerged(t *testing.T) {
 	}
 }
 
-// TestXFF_V4MappedIPv6BypassesTrustedV4Prefix demonstrates a gap in
-// rightmostUntrustedXFF: netip.Prefix.Contains returns false when an
+// TestXFF_V4MappedIPv6BypassesTrustedV4Prefix demonstrates a gap in the
+// rightmost-untrusted walker: netip.Prefix.Contains returns false when an
 // IPv4-mapped IPv6 address (::ffff:a.b.c.d) is checked against a plain v4
 // prefix. So an attacker who can inject the v4-mapped form of an internal
 // address into XFF — and whose own connecting IP happens to be in the
@@ -596,7 +596,7 @@ func TestClientIPFromRemoteAddr_V4MappedIsUnmapped(t *testing.T) {
 // by trim before parsing, and do not trip fail-closed.
 //
 // These tests are EXPECTED TO FAIL until the fail-closed change lands in
-// rightmostUntrustedXFF.
+// the ClientIPFromXFF walker.
 func TestXFF_FailClosedOnUnparseable(t *testing.T) {
 	tt := []struct {
 		name     string