Skip to content

Commit 5dd2243

Browse files
ClientIPFromXFF: fail closed on unparseable XFF entries
PR #967 review issue #6 (adam-p): the rightmost-untrusted walk previously treated a netip.ParseAddr failure the same as a trusted-prefix match — silently skip and keep walking left. That is unsafe. Rightmost-untrusted only works because we trust ourselves to be able to read every hop to the right of the client. An unparseable hop is indistinguishable from a hostile or missing one. Walking past it can let us "find" a client IP that is actually any prepended value an attacker chose to put further left in the chain — exactly the spoofing shape this middleware was written to prevent. Flip the !ok branch in rightmostUntrustedXFF from continue to return (zero, false). Trusted-prefix still continues, empties/whitespace still skip via trim, fail-closed only fires on actual parse failures. ClientIPFromXFFTrustedProxies and ClientIPFromHeader are unaffected: they are single-value reads and already produce no IP on parse failure by construction. Godoc: - ClientIPFromXFF gets one concise line stating the fail-closed contract (visible to library users). - rightmostUntrustedXFF's internal comment is expanded to explain the security rationale (visible to chi maintainers). The four cases pinned in the previous commit's TestXFF_FailClosedOnUnparseable now pass; full middleware test suite stays green. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 717827a commit 5dd2243

1 file changed

Lines changed: 12 additions & 3 deletions

File tree

middleware/client_ip.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ func ClientIPFromHeader(trustedHeader string) func(http.Handler) http.Handler {
4848
// of the given trusted CIDR prefixes. The first IP that is not trusted is
4949
// the client. Read it with [GetClientIP].
5050
//
51+
// An unparseable entry mid-chain aborts the walk and leaves no client IP
52+
// set (fail-closed) — we can't safely trust anything left of garbage.
53+
//
5154
// Use this when you sit behind one or more reverse proxies whose IP ranges
5255
// you can enumerate as CIDRs:
5356
//
@@ -193,8 +196,11 @@ func mergeXFF(headers []string) []string {
193196
}
194197

195198
// rightmostUntrustedXFF walks all X-Forwarded-For header values right-to-left
196-
// across the merged chain, skipping IPs that match trustedPrefixes (and
197-
// unparseable / empty entries), and returns the first remaining valid IP.
199+
// across the merged chain, skipping trusted-prefix and empty entries, and
200+
// returns the first remaining valid IP. An unparseable entry encountered
201+
// mid-walk fails closed: the walk is abandoned and no IP is returned, since
202+
// an unparseable hop is indistinguishable from a hostile or missing one and
203+
// we cannot safely keep walking past it.
198204
//
199205
// Walks the headers lazily rather than calling [mergeXFF] so the common case
200206
// — one trusted hop, the rightmost entry is the client — returns on the very
@@ -215,7 +221,10 @@ func rightmostUntrustedXFF(headers []string, trustedPrefixes []netip.Prefix) (ne
215221
continue
216222
}
217223
ip, ok := parseHeaderAddr(v)
218-
if !ok || inAnyPrefix(ip, trustedPrefixes) {
224+
if !ok {
225+
return netip.Addr{}, false
226+
}
227+
if inAnyPrefix(ip, trustedPrefixes) {
219228
continue
220229
}
221230
return ip, true

0 commit comments

Comments
 (0)