sec: parse X-Forwarded-For from trusted proxies via WIREFAN_TRUSTED_PROXIES

internal/server/upgrade.go

@@ -4,6 +4,8 @@ import (
 	"errors"
 	"log/slog"
 	"net/http"
+	"net/netip"
+	"os"
 	"strings"
 	"sync"
 
@@ -33,6 +35,7 @@ type UpgradeHandler struct {
 	rateLimit      *ratelimit.Limiter
 	policy         conn.Policy
 	hub            *hub.Hub
+	trustedProxies []netip.Prefix
 
 	ipMu    sync.Mutex
 	ipCount map[string]int
@@ -49,6 +52,7 @@ func NewUpgradeHandler(st store.Store, origins []string, reg registry.Registry,
 		rateLimit:      rl,
 		policy:         pol,
 		hub:            h,
+		trustedProxies: parseTrustedProxies(os.Getenv("WIREFAN_TRUSTED_PROXIES")),
 		ipCount:        map[string]int{},
 		ipCap:          defaultIPCap,
 	}
@@ -87,23 +91,107 @@ func sanitizeLogValue(s string) string {
 	return string(out)
 }
 
-// clientIP extracts a best-effort source IP. Behind a trusted proxy the caller
-// would want X-Forwarded-For, but for the direct-connect demo path RemoteAddr
-// is fine. Strips the port suffix; handles IPv6 (e.g. "[::1]:1234") by
-// preserving the bracketed host as the key.
-func clientIP(r *http.Request) string {
-	addr := r.RemoteAddr
-	// IPv6: "[::1]:1234"
-	if strings.HasPrefix(addr, "[") {
-		if end := strings.LastIndex(addr, "]"); end > 0 {
-			return addr[:end+1]
+// parseTrustedProxies turns a comma-separated CIDR / single-IP list into
+// netip.Prefix values. Bare IPs (e.g. "127.0.0.1") become /32 or /128.
+// Malformed entries are silently dropped — operators should verify with a
+// smoke test, and a hard failure here would block boot on a typo without
+// a config-validation tool to backstop it.
+func parseTrustedProxies(raw string) []netip.Prefix {
+	if raw == "" {
+		return nil
+	}
+	var prefixes []netip.Prefix
+	for _, s := range strings.Split(raw, ",") {
+		s = strings.TrimSpace(s)
+		if s == "" {
+			continue
+		}
+		if p, err := netip.ParsePrefix(s); err == nil {
+			prefixes = append(prefixes, p)
+			continue
+		}
+		if a, err := netip.ParseAddr(s); err == nil {
+			bits := 32
+			if a.Is6() {
+				bits = 128
+			}
+			if p, err := a.Prefix(bits); err == nil {
+				prefixes = append(prefixes, p)
+			}
+		}
+	}
+	return prefixes
+}
+
+func addrInPrefixes(a netip.Addr, prefixes []netip.Prefix) bool {
+	if !a.IsValid() {
+		return false
+	}
+	for _, p := range prefixes {
+		if p.Contains(a) {
+			return true
+		}
+	}
+	return false
+}
+
+// stripHostPort handles "1.2.3.4:5678", "[::1]:5678", "[::1]", and "::1".
+// Returns the address part as a netip.Addr, plus the original string form.
+func stripHostPort(s string) (netip.Addr, string, bool) {
+	s = strings.TrimSpace(s)
+	if s == "" {
+		return netip.Addr{}, "", false
+	}
+	host := s
+	if strings.HasPrefix(s, "[") {
+		if end := strings.LastIndex(s, "]"); end > 0 {
+			host = s[1:end]
+		}
+	} else if i := strings.LastIndex(s, ":"); i > 0 && strings.Count(s, ":") == 1 {
+		// IPv4 with port. Bare IPv6 has multiple colons; leave it alone.
+		host = s[:i]
+	}
+	a, err := netip.ParseAddr(host)
+	if err != nil {
+		return netip.Addr{}, host, false
+	}
+	return a, host, true
+}
+
+// clientIP returns a best-effort source IP. When the request's RemoteAddr is
+// in trustedProxies, the X-Forwarded-For header is consulted using the
+// rightmost-untrusted-hop algorithm: walk the XFF list right to left and
+// return the first hop whose IP is not in trustedProxies. This is the
+// algorithm called for in CWE-348 mitigations (do NOT use the leftmost
+// entry — clients can pick that themselves).
+func clientIP(r *http.Request, trustedProxies []netip.Prefix) string {
+	raAddr, raStr, raOk := stripHostPort(r.RemoteAddr)
+	if !raOk {
+		return r.RemoteAddr
+	}
+	if len(trustedProxies) == 0 || !addrInPrefixes(raAddr, trustedProxies) {
+		return raStr
+	}
+	xff := r.Header.Get("X-Forwarded-For")
+	if xff == "" {
+		return raStr
+	}
+	hops := strings.Split(xff, ",")
+	for i := len(hops) - 1; i >= 0; i-- {
+		hopAddr, hopStr, ok := stripHostPort(hops[i])
+		if !ok {
+			continue
+		}
+		if !addrInPrefixes(hopAddr, trustedProxies) {
+			return hopStr
 		}
-		return addr
 	}
-	if i := strings.LastIndex(addr, ":"); i > 0 {
-		return addr[:i]
+	// Every XFF hop was trusted; treat the leftmost as the originator.
+	if leftAddr, leftStr, ok := stripHostPort(hops[0]); ok {
+		_ = leftAddr
+		return leftStr
 	}
-	return addr
+	return raStr
 }
 
 func (h *UpgradeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
@@ -124,7 +212,7 @@ func (h *UpgradeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// model; we count ALL conns on the IP, not just phantom ones (a real
 	// browser will only ever have a couple of tabs open at once, so the
 	// distinction doesn't matter in practice).
-	ip := clientIP(r)
+	ip := clientIP(r, h.trustedProxies)
 	h.ipMu.Lock()
 	if h.ipCount[ip] >= h.ipCap {
 		h.ipMu.Unlock()

internal/server/upgrade_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"net/netip"
 	"strings"
 	"testing"
 	"time"
@@ -53,3 +54,92 @@ func newTestUpgrader(t *testing.T) http.Handler {
 	t.Cleanup(rl.Close)
 	return NewUpgradeHandler(store.NewMemory(), []string{"*"}, registry.NewSyncMap(), "test-signing-secret", fanout.NewPerConn(), rl, conn.PolicyDisconnect{}, hub.New())
 }
+
+func TestParseTrustedProxies(t *testing.T) {
+	tests := []struct {
+		in   string
+		want int
+	}{
+		{"", 0},
+		{"127.0.0.1", 1},
+		{"127.0.0.1/32", 1},
+		{"10.0.0.0/8,192.168.0.0/16", 2},
+		{"127.0.0.1, ::1", 2},
+		{"  not-a-cidr  ", 0},
+		{"10.0.0.0/8,bogus,192.168.0.0/16", 2}, // bad entry dropped
+	}
+	for _, tt := range tests {
+		got := parseTrustedProxies(tt.in)
+		if len(got) != tt.want {
+			t.Errorf("parseTrustedProxies(%q) = %d prefixes, want %d", tt.in, len(got), tt.want)
+		}
+	}
+}
+
+func TestClientIPNoTrust(t *testing.T) {
+	r := httptest.NewRequest("GET", "/", nil)
+	r.RemoteAddr = "203.0.113.5:1234"
+	r.Header.Set("X-Forwarded-For", "8.8.8.8")
+	if got := clientIP(r, nil); got != "203.0.113.5" {
+		t.Errorf("untrusted: got %q, want %q (XFF must be ignored without trust)", got, "203.0.113.5")
+	}
+}
+
+func TestClientIPTrustedProxyPicksRightmostUntrusted(t *testing.T) {
+	trusted := parseTrustedProxies("127.0.0.1/32,10.0.0.0/8")
+	cases := []struct {
+		name       string
+		remoteAddr string
+		xff        string
+		want       string
+	}{
+		{
+			"loopback proxy + single client",
+			"127.0.0.1:8000", "203.0.113.5", "203.0.113.5",
+		},
+		{
+			"chain of trusted proxies — first untrusted is the client",
+			"127.0.0.1:8000", "203.0.113.5, 10.0.0.5, 127.0.0.1", "203.0.113.5",
+		},
+		{
+			"client tries to forge — first untrusted hop wins",
+			"127.0.0.1:8000", "1.2.3.4, 9.9.9.9, 10.0.0.5", "9.9.9.9",
+		},
+		{
+			"all hops trusted — fall back to leftmost",
+			"127.0.0.1:8000", "10.0.0.1, 127.0.0.1, 10.0.0.2", "10.0.0.1",
+		},
+		{
+			"untrusted source ignores XFF",
+			"203.0.113.5:1234", "1.2.3.4", "203.0.113.5",
+		},
+		{
+			"IPv6 loopback proxy",
+			"[::1]:8000", "203.0.113.5", "203.0.113.5",
+		},
+	}
+	// Add ::1 to trusted for the IPv6 case.
+	trusted = append(trusted, mustPrefix(t, "::1/128"))
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			r := httptest.NewRequest("GET", "/", nil)
+			r.RemoteAddr = c.remoteAddr
+			if c.xff != "" {
+				r.Header.Set("X-Forwarded-For", c.xff)
+			}
+			got := clientIP(r, trusted)
+			if got != c.want {
+				t.Errorf("clientIP = %q, want %q", got, c.want)
+			}
+		})
+	}
+}
+
+func mustPrefix(t *testing.T, s string) netip.Prefix {
+	t.Helper()
+	p, err := netip.ParsePrefix(s)
+	if err != nil {
+		t.Fatalf("ParsePrefix(%q): %v", s, err)
+	}
+	return p
+}