fix(http): take the client IP from the right side of X-Forwarded-For

Proxies append the address they received a request from to
X-Forwarded-For, so with nginx's $proxy_add_x_forwarded_for, Fly.io,
and most load balancers the leftmost entry is whatever the client sent.
FindClientIP used that entry, which meant the client_ip in logs and the
IP stored on web sessions could be set to anything by the client.

Skip addresses in TRUSTED_REVERSE_PROXY_NETWORKS from the right and use
the first one outside them (or the leftmost when every hop is trusted).
Multiple X-Forwarded-For header lines are now combined, since
Header.Get only returns the first.

Setups that chain proxies but only list the innermost one (e.g.
Cloudflare in front of nginx, with only nginx trusted) will now see the
outer proxy's address and need to add its networks.

See https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/X-Forwarded-For#selecting_an_ip_address

internal/http/request/client_ip.go

@@ -35,15 +35,20 @@ func IsTrustedIP(remoteIP string, trustedNetworks []string) bool {
 }
 
 // FindClientIP returns the real client IP address using trusted reverse-proxy headers when allowed.
-func FindClientIP(r *http.Request, isTrustedProxyClient bool) string {
+func FindClientIP(r *http.Request, isTrustedProxyClient bool, trustedNetworks []string) string {
 	if isTrustedProxyClient {
 		headers := [...]string{"X-Forwarded-For", "X-Real-Ip"}
 		for _, header := range headers {
-			value := r.Header.Get(header)
+			value := strings.Join(r.Header.Values(header), ",")
 
 			if value != "" {
+				// Proxies append to the list, so skip trusted hops from the right.
 				addresses := strings.Split(value, ",")
-				address := strings.TrimSpace(addresses[0])
+				i := len(addresses) - 1
+				for i > 0 && IsTrustedIP(dropIPv6zone(strings.TrimSpace(addresses[i])), trustedNetworks) {
+					i--
+				}
+				address := strings.TrimSpace(addresses[i])
 				address = dropIPv6zone(address)
 
 				if net.ParseIP(address) != nil {

internal/http/request/client_ip_test.go

@@ -8,39 +8,41 @@ import (
 	"testing"
 )
 
+var trustedProxyNetworks = []string{"127.0.0.0/8", "192.168.0.0/16"}
+
 func TestFindClientIPWithoutHeaders(t *testing.T) {
 	r := &http.Request{RemoteAddr: "192.168.0.1:4242"}
-	if ip := FindClientIP(r, false); ip != "192.168.0.1" {
+	if ip := FindClientIP(r, false, nil); ip != "192.168.0.1" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
 	r = &http.Request{RemoteAddr: "192.168.0.1"}
-	if ip := FindClientIP(r, false); ip != "192.168.0.1" {
+	if ip := FindClientIP(r, false, nil); ip != "192.168.0.1" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
 	r = &http.Request{RemoteAddr: "fe80::14c2:f039:edc7:edc7"}
-	if ip := FindClientIP(r, false); ip != "fe80::14c2:f039:edc7:edc7" {
+	if ip := FindClientIP(r, false, nil); ip != "fe80::14c2:f039:edc7:edc7" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
 	r = &http.Request{RemoteAddr: "fe80::14c2:f039:edc7:edc7%eth0"}
-	if ip := FindClientIP(r, false); ip != "fe80::14c2:f039:edc7:edc7" {
+	if ip := FindClientIP(r, false, nil); ip != "fe80::14c2:f039:edc7:edc7" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
 	r = &http.Request{RemoteAddr: "[fe80::14c2:f039:edc7:edc7%eth0]:4242"}
-	if ip := FindClientIP(r, false); ip != "fe80::14c2:f039:edc7:edc7" {
+	if ip := FindClientIP(r, false, nil); ip != "fe80::14c2:f039:edc7:edc7" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
 	r = &http.Request{RemoteAddr: "@"}
-	if ip := FindClientIP(r, false); ip != "127.0.0.1" {
+	if ip := FindClientIP(r, false, nil); ip != "127.0.0.1" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
 	r = &http.Request{RemoteAddr: ""}
-	if ip := FindClientIP(r, false); ip != "127.0.0.1" {
+	if ip := FindClientIP(r, false, nil); ip != "127.0.0.1" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 }
@@ -51,7 +53,7 @@ func TestFindClientIPWithXFFHeader(t *testing.T) {
 	headers.Set("X-Forwarded-For", "203.0.113.195, 70.41.3.18, 150.172.238.178")
 	r := &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers}
 
-	if ip := FindClientIP(r, true); ip != "203.0.113.195" {
+	if ip := FindClientIP(r, true, trustedProxyNetworks); ip != "150.172.238.178" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
@@ -60,7 +62,7 @@ func TestFindClientIPWithXFFHeader(t *testing.T) {
 	headers.Set("X-Forwarded-For", "2001:db8:85a3:8d3:1319:8a2e:370:7348")
 	r = &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers}
 
-	if ip := FindClientIP(r, true); ip != "2001:db8:85a3:8d3:1319:8a2e:370:7348" {
+	if ip := FindClientIP(r, true, trustedProxyNetworks); ip != "2001:db8:85a3:8d3:1319:8a2e:370:7348" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
@@ -69,7 +71,7 @@ func TestFindClientIPWithXFFHeader(t *testing.T) {
 	headers.Set("X-Forwarded-For", "fe80::14c2:f039:edc7:edc7%eth0")
 	r = &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers}
 
-	if ip := FindClientIP(r, true); ip != "fe80::14c2:f039:edc7:edc7" {
+	if ip := FindClientIP(r, true, trustedProxyNetworks); ip != "fe80::14c2:f039:edc7:edc7" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
@@ -78,7 +80,7 @@ func TestFindClientIPWithXFFHeader(t *testing.T) {
 	headers.Set("X-Forwarded-For", "70.41.3.18")
 	r = &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers}
 
-	if ip := FindClientIP(r, true); ip != "70.41.3.18" {
+	if ip := FindClientIP(r, true, trustedProxyNetworks); ip != "70.41.3.18" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 
@@ -87,17 +89,42 @@ func TestFindClientIPWithXFFHeader(t *testing.T) {
 	headers.Set("X-Forwarded-For", "fake IP")
 	r = &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers}
 
-	if ip := FindClientIP(r, true); ip != "192.168.0.1" {
+	if ip := FindClientIP(r, true, trustedProxyNetworks); ip != "192.168.0.1" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 }
 
+func TestFindClientIPSkipsTrustedProxiesInXFFHeader(t *testing.T) {
+	scenarios := []struct {
+		values   []string
+		expected string
+	}{
+		{[]string{"203.0.113.195, 192.168.1.2"}, "203.0.113.195"},
+		{[]string{"1.2.3.4, 203.0.113.195, 192.168.1.2"}, "203.0.113.195"},
+		{[]string{"1.2.3.4", "203.0.113.195, 192.168.1.2"}, "203.0.113.195"},
+		{[]string{"192.168.1.3, 192.168.1.2"}, "192.168.1.3"},
+		{[]string{"fake IP, 192.168.1.2"}, "192.168.0.1"},
+	}
+
+	for _, scenario := range scenarios {
+		headers := http.Header{}
+		for _, value := range scenario.values {
+			headers.Add("X-Forwarded-For", value)
+		}
+		r := &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers}
+
+		if ip := FindClientIP(r, true, trustedProxyNetworks); ip != scenario.expected {
+			t.Errorf(`Expected %q for X-Forwarded-For %q, got %q`, scenario.expected, scenario.values, ip)
+		}
+	}
+}
+
 func TestClientIPWithXRealIPHeader(t *testing.T) {
 	headers := http.Header{}
 	headers.Set("X-Real-Ip", "192.168.122.1")
 	r := &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers}
 
-	if ip := FindClientIP(r, true); ip != "192.168.122.1" {
+	if ip := FindClientIP(r, true, trustedProxyNetworks); ip != "192.168.122.1" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 }
@@ -109,7 +136,7 @@ func TestClientIPWithBothHeaders(t *testing.T) {
 
 	r := &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers}
 
-	if ip := FindClientIP(r, true); ip != "203.0.113.195" {
+	if ip := FindClientIP(r, true, trustedProxyNetworks); ip != "150.172.238.178" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 }
@@ -121,7 +148,7 @@ func TestClientIPWithUnixSocketRemoteAddrAndBothHeaders(t *testing.T) {
 
 	r := &http.Request{RemoteAddr: "@", Header: headers}
 
-	if ip := FindClientIP(r, true); ip != "203.0.113.195" {
+	if ip := FindClientIP(r, true, trustedProxyNetworks); ip != "150.172.238.178" {
 		t.Fatalf(`Unexpected result, got: %q`, ip)
 	}
 }

internal/http/server/middleware.go

@@ -16,8 +16,9 @@ import (
 func middleware(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		remoteIP := request.FindRemoteIP(r)
-		isTrustedProxyClientIP := request.IsTrustedIP(remoteIP, config.Opts.TrustedReverseProxyNetworks())
-		clientIP := request.FindClientIP(r, isTrustedProxyClientIP)
+		trustedNetworks := config.Opts.TrustedReverseProxyNetworks()
+		isTrustedProxyClientIP := request.IsTrustedIP(remoteIP, trustedNetworks)
+		clientIP := request.FindClientIP(r, isTrustedProxyClientIP, trustedNetworks)
 		ctx := r.Context()
 		ctx = context.WithValue(ctx, request.ClientIPContextKey, clientIP)