fix: resolve client IP from the rightmost untrusted X-Forwarded-For entry (#26646)

coderd/httpmw/realip.go

@@ -70,7 +70,17 @@ func ExtractRealIPAddress(config *RealIPConfig, req *http.Request) (net.IP, erro
 	}
 
 	for _, trustedHeader := range config.TrustedHeaders {
-		addr := getRemoteAddress(req.Header.Get(trustedHeader))
+		// X-Forwarded-For is a list-valued header. Per RFC 7230, multiple
+		// field lines with the same name are equivalent to a single
+		// comma-separated value. Join them so a client cannot hide a spoofed
+		// address in the first field line, which Header.Get would return on
+		// its own. Other forwarding headers carry a single edge-proxy value,
+		// so use Header.Get to preserve their first-value semantics.
+		value := req.Header.Get(trustedHeader)
+		if http.CanonicalHeaderKey(trustedHeader) == headerXForwardedFor {
+			value = strings.Join(req.Header.Values(trustedHeader), ",")
+		}
+		addr := extractForwardedAddress(config, value)
 		if addr != nil {
 			return addr, nil
 		}
@@ -101,6 +111,14 @@ func FilterUntrustedOriginHeaders(config *RealIPConfig, req *http.Request) {
 	}
 
 	for _, header := range config.TrustedHeaders {
+		// X-Forwarded-For is a list-valued header whose field lines are
+		// equivalent to a single comma-separated value (RFC 7230 section
+		// 3.2.2). Join them so later hops are not dropped when collapsing to a
+		// single line. Other forwarding headers carry a single value.
+		if http.CanonicalHeaderKey(header) == headerXForwardedFor {
+			req.Header.Set(header, strings.Join(req.Header.Values(header), ","))
+			continue
+		}
 		req.Header.Set(header, req.Header.Get(header))
 	}
 }
@@ -185,12 +203,15 @@ func EnsureXForwardedForHeader(req *http.Request) error {
 	return nil
 }
 
-// getRemoteAddress extracts the IP address from the given string. If
-// the string contains commas, it assumes that the first part is the
-// original address.
+// getRemoteAddress extracts a single IP address from the given string,
+// stripping a port if present. If the string contains commas, only the
+// portion before the first comma is parsed. This helper does not select the
+// real client from a multi-hop X-Forwarded-For chain; use
+// extractForwardedAddress for that, which accounts for client-supplied values.
 func getRemoteAddress(address string) net.IP {
-	// X-Forwarded-For may contain multiple addresses, in case the
-	// proxies are chained; the first value is the client address
+	// A value may contain a port and, for a raw X-Forwarded-For value, more
+	// than one comma-separated address. Parse only the part before the first
+	// comma.
 	i := strings.IndexByte(address, ',')
 	if i == -1 {
 		i = len(address)
@@ -206,6 +227,32 @@ func getRemoteAddress(address string) net.IP {
 	return net.ParseIP(host)
 }
 
+// extractForwardedAddress parses a comma-separated forwarding header value and
+// returns the rightmost address that is not a trusted origin. Reverse proxies
+// append the peer that connected to them, so when every trusted proxy hop is
+// listed in TrustedOrigins, the rightmost untrusted address is the real client;
+// any values a client prepends to spoof its address sit to the left of the
+// addresses inserted by trusted proxies and are ignored. If every parsed address
+// is a trusted origin, the leftmost address is returned. It returns nil when no
+// address can be parsed.
+func extractForwardedAddress(config *RealIPConfig, value string) net.IP {
+	parts := strings.Split(value, ",")
+	var leftmost net.IP
+	for i := len(parts) - 1; i >= 0; i-- {
+		ip := getRemoteAddress(strings.TrimSpace(parts[i]))
+		if ip == nil {
+			continue
+		}
+		// Iterating right-to-left, so the last assignment is the leftmost
+		// valid address, used as the fallback when all hops are trusted.
+		leftmost = ip
+		if !isContainedIn(config.TrustedOrigins, ip) {
+			return ip
+		}
+	}
+	return leftmost
+}
+
 // isContainedIn checks that the given address is contained in the given
 // network.
 func isContainedIn(networks []*net.IPNet, address net.IP) bool {

coderd/httpmw/realip_test.go

@@ -82,6 +82,72 @@ func TestExtractAddress(t *testing.T) {
 			},
 			ExpectedRemoteAddr: "10.24.1.1",
 		},
+		{
+			// A chain of trusted proxies appends each hop. The rightmost
+			// untrusted address (the real client) wins, skipping the trusted
+			// inner-proxy hop.
+			Name: "picks-rightmost-untrusted",
+			Config: &httpmw.RealIPConfig{
+				TrustedOrigins: []*net.IPNet{
+					{
+						IP:   net.ParseIP("10.0.0.0"),
+						Mask: net.CIDRMask(8, 32),
+					},
+				},
+				TrustedHeaders: []string{
+					"X-Forwarded-For",
+				},
+			},
+			RemoteAddr: "10.0.0.1",
+			Header: http.Header{
+				"X-Forwarded-For": []string{"1.2.3.4, 203.0.113.5, 10.0.0.2"},
+			},
+			ExpectedRemoteAddr: "203.0.113.5",
+		},
+		{
+			// When every parsed hop is a trusted origin, there is no untrusted
+			// client to select, so the leftmost address is used.
+			Name: "all-trusted-falls-back-to-leftmost",
+			Config: &httpmw.RealIPConfig{
+				TrustedOrigins: []*net.IPNet{
+					{
+						IP:   net.ParseIP("10.0.0.0"),
+						Mask: net.CIDRMask(8, 32),
+					},
+				},
+				TrustedHeaders: []string{
+					"X-Forwarded-For",
+				},
+			},
+			RemoteAddr: "10.0.0.1",
+			Header: http.Header{
+				"X-Forwarded-For": []string{"10.0.0.1, 10.0.0.2"},
+			},
+			ExpectedRemoteAddr: "10.0.0.1",
+		},
+		{
+			// A proxy may append its hop as a separate header line. Per
+			// RFC 7230 section 3.2.2 these are equivalent to a single
+			// comma-joined value, so the spoofed first line must not be
+			// trusted on its own.
+			Name: "x-forwarded-for-set-multiple-times",
+			Config: &httpmw.RealIPConfig{
+				TrustedOrigins: []*net.IPNet{
+					{
+						IP:   net.ParseIP("10.0.0.0"),
+						Mask: net.CIDRMask(8, 32),
+					},
+				},
+				TrustedHeaders: []string{
+					"X-Forwarded-For",
+				},
+			},
+			RemoteAddr: "10.0.0.1",
+			Header: http.Header{
+				"X-Forwarded-For": []string{"1.2.3.4", "203.0.113.5, 10.0.0.2"},
+			},
+			ExpectedRemoteAddr: "203.0.113.5",
+		},
 		{
 			Name: "single-real-ip",
 			Config: &httpmw.RealIPConfig{
@@ -456,6 +522,31 @@ func TestFilterUntrusted(t *testing.T) {
 			},
 			ExpectedRemoteAddr: "1.2.3.4",
 		},
+		{
+			// For a trusted origin, multiple X-Forwarded-For field lines are
+			// joined into one comma-separated value rather than collapsed to
+			// the first line, so later hops are preserved.
+			Name: "trusted-origin-joins-multiple-x-forwarded-for",
+			Config: &httpmw.RealIPConfig{
+				TrustedOrigins: []*net.IPNet{
+					{
+						IP:   net.ParseIP("10.0.0.0"),
+						Mask: net.CIDRMask(8, 32),
+					},
+				},
+				TrustedHeaders: []string{
+					"X-Forwarded-For",
+				},
+			},
+			Header: http.Header{
+				"X-Forwarded-For": []string{"1.2.3.4", "203.0.113.5, 10.0.0.2"},
+			},
+			RemoteAddr: "10.0.0.1",
+			ExpectedHeader: http.Header{
+				"X-Forwarded-For": []string{"1.2.3.4,203.0.113.5, 10.0.0.2"},
+			},
+			ExpectedRemoteAddr: "10.0.0.1",
+		},
 	}
 
 	for _, test := range tests {