From 24bf2df1640aaaccfd26c0da006a26b52b728e0d Mon Sep 17 00:00:00 2001 From: kawaway Date: Sat, 6 Jun 2026 12:20:22 +0900 Subject: [PATCH 1/2] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The proxy middleware's WebSocket path currently sets `X-Forwarded-For` only when the header is empty, dropping the proxy's own peer IP from the chain whenever upstream proxies had already added entries. This breaks downstream services that rely on the "rightmost untrusted" rule to extract the real client IP, including echo's own `ExtractIPFromXFFHeader`. The HTTP path delegates to `net/http/httputil.ReverseProxy`, which appends `RemoteAddr` to the existing `X-Forwarded-For` chain — either inline in `ServeHTTP`'s default Director path ([reverseproxy.go#L519-L531](https://github.com/golang/go/blob/go1.26.3/src/net/http/httputil/reverseproxy.go#L519-L531)) or via the explicit [`(*ProxyRequest).SetXForwarded`](https://github.com/golang/go/blob/go1.26.3/src/net/http/httputil/reverseproxy.go#L82-L96) when a `Rewrite` callback is configured. The WebSocket path uses `proxyRaw`, which writes the request verbatim via `r.Write(out)`, so this middleware is the only place where the appending happens for WebSocket Upgrade requests. Replace the "set if empty" guard with always-append. Read values via map access to preserve multi-line `X-Forwarded-For` headers (RFC 9110 §5.3 allows combining them by joining values with commas). Added TestProxyWebSocketXForwardedFor exercising 4 cases: - no incoming X-Forwarded-For → only c.RealIP() - single-line single-entry → preserved + c.RealIP() at the tail - ingle-line comma-separated → preserved + c.RealIP() at the tail - multi-line headers (multiple X-Forwarded-For occurrences) → joined with , + c.RealIP() at the tail Each case captures the request header at WebSocket Upgrade time inside the upstream handler and asserts both the appended tail and the preserved prefix. The change is additive: existing entries are preserved and the proxy's own peer IP is added at the tail. Downstream readers using the standard "rightmost untrusted" rule (e.g. echo.ExtractIPFromXFFHeader) see no behavioral difference for chains where they already worked, and start returning the correct IP for chains where the proxy IP was previously dropped. We hit this in production with an Echo-based WebSocket reverse proxy fronting an internal service that uses echo.ExtractIPFromXFFHeader for IP-based authorization. Multi-hop deployments (customer proxy → our reverse proxy → backend) broke because the reverse proxy's egress IP was missing from the chain reaching the backend. --- middleware/proxy.go | 10 +++- middleware/proxy_test.go | 123 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 2 deletions(-) diff --git a/middleware/proxy.go b/middleware/proxy.go index a40d58130..497aefea4 100644 --- a/middleware/proxy.go +++ b/middleware/proxy.go @@ -355,8 +355,14 @@ func (config ProxyConfig) ToMiddleware() (echo.MiddlewareFunc, error) { if req.Header.Get(echo.HeaderXForwardedProto) == "" { req.Header.Set(echo.HeaderXForwardedProto, c.Scheme()) } - if c.IsWebSocket() && req.Header.Get(echo.HeaderXForwardedFor) == "" { // For HTTP, it is automatically set by Go HTTP reverse proxy. - req.Header.Set(echo.HeaderXForwardedFor, c.RealIP()) + if c.IsWebSocket() { // For HTTP, this is set by Go HTTP reverse proxy. + // Append, not set, to preserve the incoming chain from upstream proxies. + prior := req.Header[echo.HeaderXForwardedFor] + if len(prior) > 0 { + req.Header.Set(echo.HeaderXForwardedFor, strings.Join(prior, ", ")+", "+c.RealIP()) + } else { + req.Header.Set(echo.HeaderXForwardedFor, c.RealIP()) + } } retries := config.RetryCount diff --git a/middleware/proxy_test.go b/middleware/proxy_test.go index 5494b23ba..d77c35911 100644 --- a/middleware/proxy_test.go +++ b/middleware/proxy_test.go @@ -15,6 +15,7 @@ import ( "net/http/httptest" "net/url" "regexp" + "strings" "sync" "testing" "time" @@ -1164,3 +1165,125 @@ func TestProxyWithConfigWebSocketTLS2NonTLS(t *testing.T) { assert.NoError(t, err) assert.Equal(t, sendMsg, recvMsg) } + +// TestProxyWebSocketXForwardedFor verifies that for WebSocket Upgrade requests, +// the proxy middleware appends c.RealIP() to any existing X-Forwarded-For chain, +// mirroring net/http/httputil.(*ProxyRequest).SetXForwarded used by the HTTP path. +// +// Regression guard for the previous "set only if empty" behavior, which dropped +// the proxy's own peer IP from the chain whenever upstream proxies had already +// added entries. +func TestProxyWebSocketXForwardedFor(t *testing.T) { + var ( + mu sync.Mutex + capturedHeader http.Header + ) + + // Upstream that captures the request header at WS Upgrade time, then echoes. + upstreamHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + wsHandler := func(conn *websocket.Conn) { + mu.Lock() + capturedHeader = conn.Request().Header.Clone() + mu.Unlock() + + defer conn.Close() + + var msg string + if err := websocket.Message.Receive(conn, &msg); err == nil { + _ = websocket.Message.Send(conn, msg) + } + } + websocket.Server{Handler: wsHandler}.ServeHTTP(w, r) + }) + upstream := httptest.NewServer(upstreamHandler) + defer upstream.Close() + + tgtURL, _ := url.Parse(upstream.URL) + e := echo.New() + balancer := NewRandomBalancer([]*ProxyTarget{{URL: tgtURL}}) + e.Use(ProxyWithConfig(ProxyConfig{Balancer: balancer})) + proxySrv := httptest.NewServer(e) + defer proxySrv.Close() + + proxyWSURL, _ := url.Parse(proxySrv.URL) + proxyWSURL.Scheme = "ws" + + tests := []struct { + name string + incomingXFF []string // nil = no incoming X-Forwarded-For header at all + wantPrefix string // expected join of entries preceding the appended proxy RealIP + }{ + { + name: "no incoming XFF, only proxy RealIP is set", + incomingXFF: nil, + wantPrefix: "", + }, + { + name: "single-line single-entry XFF is preserved with proxy RealIP appended", + incomingXFF: []string{"203.0.113.1"}, + wantPrefix: "203.0.113.1", + }, + { + name: "single-line comma-separated XFF is preserved with proxy RealIP appended", + incomingXFF: []string{"203.0.113.1, 10.0.0.5"}, + wantPrefix: "203.0.113.1, 10.0.0.5", + }, + { + name: "multi-line XFF (multiple header occurrences) is joined with proxy RealIP appended", + incomingXFF: []string{"203.0.113.1", "10.0.0.5"}, + wantPrefix: "203.0.113.1, 10.0.0.5", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mu.Lock() + capturedHeader = nil + mu.Unlock() + + origin, _ := url.Parse(proxySrv.URL) + cfg := &websocket.Config{ + Location: proxyWSURL, + Origin: origin, + Version: websocket.ProtocolVersionHybi13, + Header: http.Header{}, + } + + for _, v := range tt.incomingXFF { + cfg.Header.Add(echo.HeaderXForwardedFor, v) + } + + wsConn, err := websocket.DialConfig(cfg) + assert.NoError(t, err) + + defer wsConn.Close() + + // Exchange one message to ensure the upstream handler has captured the header. + assert.NoError(t, websocket.Message.Send(wsConn, "ping")) + + var got string + assert.NoError(t, websocket.Message.Receive(wsConn, &got)) + + mu.Lock() + xff := capturedHeader.Get(echo.HeaderXForwardedFor) + mu.Unlock() + + // The middleware uses Header.Set, so the upstream sees exactly one + // X-Forwarded-For header line. Split it back into entries. + entries := strings.Split(xff, ", ") + assert.NotEmpty(t, entries, "X-Forwarded-For must be set by the proxy middleware") + + // The tail entry is the proxy's c.RealIP(). When the test client dials + // via httptest.NewServer the proxy sees 127.0.0.1. + tail := entries[len(entries)-1] + assert.Equal(t, "127.0.0.1", tail, + "proxy RealIP must be appended at the tail of X-Forwarded-For") + + // The remaining entries must equal the prior chain, preserving order + // and joining multi-line headers with ", ". + gotPrefix := strings.Join(entries[:len(entries)-1], ", ") + assert.Equal(t, tt.wantPrefix, gotPrefix, + "prior X-Forwarded-For entries must be preserved before the appended RealIP") + }) + } +} From a811a1eb94d5f19f42b36ff90d7a8766a1e35d61 Mon Sep 17 00:00:00 2001 From: kawaway Date: Sat, 6 Jun 2026 19:27:44 +0900 Subject: [PATCH 2/2] Set up upstream per test --- middleware/proxy_test.go | 71 +++++++++++++++------------------------- 1 file changed, 27 insertions(+), 44 deletions(-) diff --git a/middleware/proxy_test.go b/middleware/proxy_test.go index d77c35911..5053f7945 100644 --- a/middleware/proxy_test.go +++ b/middleware/proxy_test.go @@ -1174,40 +1174,6 @@ func TestProxyWithConfigWebSocketTLS2NonTLS(t *testing.T) { // the proxy's own peer IP from the chain whenever upstream proxies had already // added entries. func TestProxyWebSocketXForwardedFor(t *testing.T) { - var ( - mu sync.Mutex - capturedHeader http.Header - ) - - // Upstream that captures the request header at WS Upgrade time, then echoes. - upstreamHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - wsHandler := func(conn *websocket.Conn) { - mu.Lock() - capturedHeader = conn.Request().Header.Clone() - mu.Unlock() - - defer conn.Close() - - var msg string - if err := websocket.Message.Receive(conn, &msg); err == nil { - _ = websocket.Message.Send(conn, msg) - } - } - websocket.Server{Handler: wsHandler}.ServeHTTP(w, r) - }) - upstream := httptest.NewServer(upstreamHandler) - defer upstream.Close() - - tgtURL, _ := url.Parse(upstream.URL) - e := echo.New() - balancer := NewRandomBalancer([]*ProxyTarget{{URL: tgtURL}}) - e.Use(ProxyWithConfig(ProxyConfig{Balancer: balancer})) - proxySrv := httptest.NewServer(e) - defer proxySrv.Close() - - proxyWSURL, _ := url.Parse(proxySrv.URL) - proxyWSURL.Scheme = "ws" - tests := []struct { name string incomingXFF []string // nil = no incoming X-Forwarded-For header at all @@ -1237,9 +1203,30 @@ func TestProxyWebSocketXForwardedFor(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mu.Lock() - capturedHeader = nil - mu.Unlock() + // Buffered so the upstream handler never blocks before the client reads. + headerCh := make(chan http.Header, 1) + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + wsHandler := func(conn *websocket.Conn) { + headerCh <- conn.Request().Header.Clone() + defer conn.Close() + var msg string + if err := websocket.Message.Receive(conn, &msg); err == nil { + _ = websocket.Message.Send(conn, msg) + } + } + websocket.Server{Handler: wsHandler}.ServeHTTP(w, r) + })) + defer upstream.Close() + + tgtURL, _ := url.Parse(upstream.URL) + e := echo.New() + e.Use(ProxyWithConfig(ProxyConfig{Balancer: NewRandomBalancer([]*ProxyTarget{{URL: tgtURL}})})) + proxySrv := httptest.NewServer(e) + defer proxySrv.Close() + + proxyWSURL, _ := url.Parse(proxySrv.URL) + proxyWSURL.Scheme = "ws" origin, _ := url.Parse(proxySrv.URL) cfg := &websocket.Config{ @@ -1248,25 +1235,21 @@ func TestProxyWebSocketXForwardedFor(t *testing.T) { Version: websocket.ProtocolVersionHybi13, Header: http.Header{}, } - for _, v := range tt.incomingXFF { cfg.Header.Add(echo.HeaderXForwardedFor, v) } wsConn, err := websocket.DialConfig(cfg) assert.NoError(t, err) - defer wsConn.Close() - // Exchange one message to ensure the upstream handler has captured the header. assert.NoError(t, websocket.Message.Send(wsConn, "ping")) - var got string assert.NoError(t, websocket.Message.Receive(wsConn, &got)) - mu.Lock() - xff := capturedHeader.Get(echo.HeaderXForwardedFor) - mu.Unlock() + // The handler sends to headerCh before echoing, so it arrives before Receive returns. + captured := <-headerCh + xff := captured.Get(echo.HeaderXForwardedFor) // The middleware uses Header.Set, so the upstream sees exactly one // X-Forwarded-For header line. Split it back into entries.