Skip to content

fix(middleware/proxy): append RealIP to X-Forwarded-For for WebSocket requests#2994

Open
kawaway wants to merge 1 commit into
labstack:masterfrom
kawaway:master
Open

fix(middleware/proxy): append RealIP to X-Forwarded-For for WebSocket requests#2994
kawaway wants to merge 1 commit into
labstack:masterfrom
kawaway:master

Conversation

@kawaway
Copy link
Copy Markdown

@kawaway kawaway commented Jun 6, 2026

Description

This PR fixes #2993

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) or via the explicit (*ProxyRequest).SetXForwarded
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.

Change

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).

Test

Added TestProxyWebSocketXForwardedFor exercising 4 cases:

  • no incoming X-Forwarded-For → only c.RealIP()
  • single-line single-entry → preserved + c.RealIP() at the tail
  • single-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.

Backwards compatibility

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.

Background

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.

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.

<Change>

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).

<Test>
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.

<Backwards compatibility>
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.

<Background>
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.
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.18%. Comparing base (01b45be) to head (24bf2df).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2994   +/-   ##
=======================================
  Coverage   93.17%   93.18%           
=======================================
  Files          43       43           
  Lines        4501     4503    +2     
=======================================
+ Hits         4194     4196    +2     
  Misses        189      189           
  Partials      118      118           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that mu and capturedHeader logic in test is excessive. If the LLM is trying to avoid running data races in parallel tests it could create server per test. but this locking makes it harder to understand - locking at nilling and then later at reading seems wrong

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

proxy middleware drops upstream X-Forwarded-For chain on WebSocket Upgrade requests

2 participants