From 017601354be38cb027ff3ffb01aed79bd5d12610 Mon Sep 17 00:00:00 2001 From: lc6464 Date: Wed, 10 Jun 2026 12:30:00 +0800 Subject: [PATCH] fix(web): harden trusted proxy client IP parsing --- web/README.md | 1 + web/backend/middleware/access_control.go | 34 ++++++--- web/backend/middleware/access_control_test.go | 71 ++++++++++++++++++- 3 files changed, 95 insertions(+), 11 deletions(-) diff --git a/web/README.md b/web/README.md index b0463de01..5dc88b03d 100644 --- a/web/README.md +++ b/web/README.md @@ -156,6 +156,7 @@ When public access is enabled: - optional `allowed_cidrs` can restrict which client IP ranges may connect - `allow_localhost_bypass` defaults to `true`; set it to `false` when same-host proxies or tunnels should not bypass `allowed_cidrs` - optional `trusted_proxy_cidrs` can trust specific reverse proxies to supply the original client IP through headers such as `X-Forwarded-For` +- trusted proxy deployments should overwrite or sanitize forwarding headers such as `X-Forwarded-For` and `X-Real-IP` instead of passing through user-supplied values - the gateway host is overridden so remote clients can still use the launcher-managed proxy paths ## Build And Run diff --git a/web/backend/middleware/access_control.go b/web/backend/middleware/access_control.go index 86bd0ca06..13f68f0da 100644 --- a/web/backend/middleware/access_control.go +++ b/web/backend/middleware/access_control.go @@ -41,9 +41,7 @@ func IPAllowlist(cfg IPAllowlistConfig, next http.Handler) (http.Handler, error) ip := peerIP if containsIP(trustedProxyNets, peerIP) { - if forwardedIP := clientIPFromXForwardedFor(r.Header.Get("X-Forwarded-For")); forwardedIP != nil { - ip = forwardedIP - } + ip = clientIPFromXForwardedFor(r.Header.Get("X-Forwarded-For"), trustedProxyNets, peerIP) } if cfg.AllowLocalhostBypass && ip.IsLoopback() { @@ -88,16 +86,34 @@ func clientIPFromRemoteAddr(remoteAddr string) net.IP { return net.ParseIP(host) } -func clientIPFromXForwardedFor(header string) net.IP { - first, _, _ := strings.Cut(header, ",") - first = strings.Trim(strings.TrimSpace(first), `"`) - if first == "" { +func clientIPFromXForwardedFor(header string, trustedProxyNets []*net.IPNet, fallback net.IP) net.IP { + parts := strings.Split(header, ",") + ips := make([]net.IP, 0, len(parts)) + for _, part := range parts { + if ip := parseIPToken(part); ip != nil { + ips = append(ips, ip) + } + } + if len(ips) == 0 { + return fallback + } + for i := len(ips) - 1; i >= 0; i-- { + if !containsIP(trustedProxyNets, ips[i]) { + return ips[i] + } + } + return ips[0] +} + +func parseIPToken(raw string) net.IP { + token := strings.Trim(strings.TrimSpace(raw), `"`) + if token == "" { return nil } - if ip := net.ParseIP(first); ip != nil { + if ip := net.ParseIP(token); ip != nil { return ip } - if host, _, err := net.SplitHostPort(first); err == nil { + if host, _, err := net.SplitHostPort(token); err == nil { return net.ParseIP(host) } return nil diff --git a/web/backend/middleware/access_control_test.go b/web/backend/middleware/access_control_test.go index c0364e489..f046c783f 100644 --- a/web/backend/middleware/access_control_test.go +++ b/web/backend/middleware/access_control_test.go @@ -127,7 +127,7 @@ func TestIPAllowlist_IgnoresXForwardedForFromUntrustedPeer(t *testing.T) { } } -func TestIPAllowlist_UsesXForwardedForFromTrustedPeer(t *testing.T) { +func TestIPAllowlist_UsesRightmostUntrustedXForwardedForIPFromTrustedPeer(t *testing.T) { h, err := IPAllowlist(IPAllowlistConfig{ AllowedCIDRs: []string{"192.168.1.0/24"}, TrustedProxyCIDRs: []string{"10.0.0.0/8"}, @@ -141,7 +141,7 @@ func TestIPAllowlist_UsesXForwardedForFromTrustedPeer(t *testing.T) { rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodGet, "/", nil) req.RemoteAddr = "10.0.0.8:1234" - req.Header.Set("X-Forwarded-For", "192.168.1.88, 203.0.113.5") + req.Header.Set("X-Forwarded-For", "203.0.113.5, 192.168.1.88, 10.0.0.9") h.ServeHTTP(rec, req) if rec.Code != http.StatusOK { @@ -149,6 +149,73 @@ func TestIPAllowlist_UsesXForwardedForFromTrustedPeer(t *testing.T) { } } +func TestIPAllowlist_UsesRightmostUntrustedIPFromTrustedProxyChain(t *testing.T) { + h, err := IPAllowlist(IPAllowlistConfig{ + AllowedCIDRs: []string{"192.168.1.0/24"}, + TrustedProxyCIDRs: []string{"10.0.0.0/8"}, + }, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + if err != nil { + t.Fatalf("IPAllowlist() error = %v", err) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.8:1234" + req.Header.Set("X-Forwarded-For", "192.168.1.88, 10.0.0.9, 10.0.0.10") + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK) + } +} + +func TestIPAllowlist_DoesNotTrustSpoofedLeftmostLoopbackInXForwardedFor(t *testing.T) { + h, err := IPAllowlist(IPAllowlistConfig{ + AllowedCIDRs: []string{"192.168.1.0/24"}, + AllowLocalhostBypass: true, + TrustedProxyCIDRs: []string{"10.0.0.0/8"}, + }, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + if err != nil { + t.Fatalf("IPAllowlist() error = %v", err) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.8:1234" + req.Header.Set("X-Forwarded-For", "127.0.0.1, 203.0.113.5") + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusForbidden) + } +} + +func TestIPAllowlist_AllTrustedProxyChainFallsBackToLeftmostIP(t *testing.T) { + h, err := IPAllowlist(IPAllowlistConfig{ + AllowedCIDRs: []string{"192.168.1.0/24"}, + TrustedProxyCIDRs: []string{"10.0.0.0/8"}, + }, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + if err != nil { + t.Fatalf("IPAllowlist() error = %v", err) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.8:1234" + req.Header.Set("X-Forwarded-For", "10.0.0.9, 10.0.0.10") + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusForbidden) + } +} + func TestIPAllowlist_InvalidCIDR(t *testing.T) { _, err := IPAllowlist(IPAllowlistConfig{ AllowedCIDRs: []string{"bad-cidr"},