Fix X-Forwarded-Proto for TLS remote upstreams

[aron-muon]

Summary

Behind a TLS-terminating load balancer (e.g. an AWS ALB), the proxy's pod-local inbound connection is plain HTTP. httputil.SetXForwarded() derives X-Forwarded-Proto from that inbound hop, so remote upstreams always received X-Forwarded-Proto: http. Upstreams that enforce HTTPS based on this header respond with 301 redirects, and because every forwarded request keeps reporting http, the proxy loops until it hits the redirect limit — preventing MCP session initialization.

• Rewrite X-Forwarded-Proto for remote upstreams in setXForwardedHeaders. The value is now either:
  • the trusted inbound X-Forwarded-Proto (e.g. set by the ALB) when trustProxyHeaders is enabled, or
  • the actual upstream connection scheme parsed from the target URI (fallback).
• Local backends are unchanged (still derive from the inbound hop).
• Add a new upstreamForwardedProto helper and expand the table-driven test coverage.

Fixes #5567

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

TestSetXForwardedHeaders was expanded into a table covering: local backend (unchanged http), remote https/http upstreams (derive from upstream scheme), trusted inbound proto honored (and winning over upstream scheme), trust enabled without an inbound header (falls back to upstream scheme), and untrusted inbound proto ignored.

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Does this introduce a user-facing change?

Yes. MCPRemoteProxy (and any remote HTTP/SSE proxy) deployed behind a TLS-terminating load balancer now forwards a correct X-Forwarded-Proto to TLS upstreams, fixing infinite HTTP→HTTPS redirect loops during MCP session initialization. When trustProxyHeaders is enabled, a trusted inbound X-Forwarded-Proto (e.g. from an ALB) is honored; otherwise the proxy uses the upstream connection scheme.

Implementation plan

Approved implementation plan

Root cause: setXForwardedHeaders calls httputil.ProxyRequest.SetXForwarded(), which sets X-Forwarded-Proto from the pod-local inbound connection (pr.In.TLS == nil → http). Client-supplied forwarded headers are stripped by httputil before Rewrite runs, so the original HTTPS scheme is lost.

Fix (in pkg/transport/proxy/transparent/transparent_proxy.go):

1. Capture the inbound X-Forwarded-Proto before SetXForwarded() overwrites it.
2. For remote upstreams only, set X-Forwarded-Proto to:
   • the captured inbound value when trustProxyHeaders is enabled and present, else
   • the scheme parsed from the configured target URI (validated to http/https).
3. Leave local-backend behavior untouched.
4. Expand xforwarded_test.go into a table-driven test for the matrix above.

Special notes for reviewers

• The fix is scoped to remote upstreams (p.isRemote), matching the issue (MCPRemoteProxy). Local pod-to-pod backends keep the existing inbound-derived scheme.
• The fallback derives the scheme from the configured target URI, which for remote upstreams always reflects the protocol the upstream actually receives (the session backend_url override keeps the original scheme for HTTPS targets).
• Reusing the existing trustProxyHeaders knob means no new config surface is introduced; the documented option now also governs X-Forwarded-Proto passthrough, consistent with the SSE response processor's existing use of it.

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.29%. Comparing base (95f3460) to head (d6f779a).
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5594      +/-   ##
==========================================
+ Coverage   70.05%   70.29%   +0.23%     
==========================================
  Files         659      648      -11     
  Lines       66884    66023     -861     
==========================================
- Hits        46857    46411     -446     
+ Misses      16647    16269     -378     
+ Partials     3380     3343      -37     

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

[JAORMX]

Got the panel together on this one (spec, standards, security, architecture, MCP protocol, devex) with the higher reasoning model, since this is trust-boundary code and you asked for extra care. Good news first: the fix is sound and well-scoped. It kills the redirect loop, preserves the X-Forwarded-Host removal control, leaves local backends untouched, and the test coverage is solid. No Critical or High. But there's one trust-boundary gap I'd want closed before merge, plus a stale comment that's actively misleading.

The one to fix before merge: unvalidated X-Forwarded-Proto on the trust path

upstreamForwardedProto (transparent_proxy.go:1106) has an asymmetry. The fallback path validates the scheme against {http, https} (the switch at line 1114). The trust path does not... when trustProxyHeaders is on, inboundProto is returned verbatim with no check:

if p.trustProxyHeaders && inboundProto != "" {
    return inboundProto   // any string passes through
}

And inboundProto comes from pr.In.Header.Get("X-Forwarded-Proto") (line 1083), which is client-controllable. The Go stdlib ReverseProxy strips client X-Forwarded-* from the outbound request before Rewrite runs, but pr.In retains whatever the client sent. So when trustProxyHeaders is enabled, an attacker who can reach the proxy directly (port-forward, misconfigured ingress, internal cluster access... the listener is plain HTTP) can set X-Forwarded-Proto: http against an HTTPS upstream and force the very redirect loop this PR exists to fix. Or send arbitrary tokens to a third-party upstream. CWE-601 / OWASP A05:2021.

Now, the caveats: this is gated behind an opt-in flag whose documented purpose is exactly "trust these headers from a reverse proxy," so exploitation requires operator misconfiguration. And the same unvalidated-trust pattern already exists on the SSE response-processor path (sse_response_processor.go:159-166), so this PR doesn't introduce a new class of trust, it adds a second consumer. CRLF injection is NOT possible here... Go's HTTP parser rejects control chars in header values at parse time. So this is Medium, not Higher.

But this is security code, and the fix is one line. Validate inboundProto against {http, https} on the trust path, same as the fallback:

func (p *TransparentProxy) upstreamForwardedProto(inboundProto string) string {
    if p.trustProxyHeaders && isHTTPScheme(inboundProto) {
        return inboundProto
    }
    // ... fallback unchanged
}

func isHTTPScheme(s string) bool { return s == "http" || s == "https" }

Makes the trust boundary explicit and bounds the reflected value regardless of proxy placement. Add a test row for inboundProto: "httpx" with trustProxyHeaders: true asserting the fallback wins.

Stale // Deprecated comment (cross-confirmed by four reviewers)

The trustProxyHeaders field at transparent_proxy.go:135 reads:

// Deprecated: trustProxyHeaders indicates whether to trust X-Forwarded-* headers (moved to SSEResponseProcessor)
trustProxyHeaders bool

This PR adds a second live reader at line 1107 on the outbound path. The field is no longer "moved to SSEResponseProcessor"... it's actively load-bearing in two directions now (SSE response rewriting, and outbound X-Forwarded-Proto trust). The Deprecated: marker is actively harmful here: a developer doing the cleanup the marker suggests ("this was moved, I can drop the field") would silently regress the fix this PR adds. Per go-style "Keep Comments Synchronized With Code" ("a comment that contradicts the code is worse than no comment"), this is a hard violation. Drop Deprecated: and document both read sites.

Judgement calls

backend_url override and the targetURI source of truth. setXForwardedHeaders runs at line 1144, before the backend_url session-metadata override at lines 1148-1165 that can rewrite pr.Out.URL.Scheme/Host. So the header is derived from the static targetURI, not the actual request destination. I traced this carefully: podBackendURL (line 819) always preserves the targetURI scheme (it bails to targetURI for HTTPS targets, and for HTTP it only swaps the host), so the mismatch is inert today. But it's a latent trap... if backend_url ever carries an independent scheme, the header will silently lie about the wire protocol while pr.Out.URL.Scheme diverges. Two options: cheapest is a one-line comment at line 1110 noting the scheme-equivalence assumption so the next maintainer doesn't "fix" it by reading pr.Out.URL.Scheme and introducing an ordering bug. Cleaner is to move setXForwardedHeaders to after the backend_url block and derive from pr.Out.URL.Scheme. I'd take the comment now and the refactor only if scheme divergence is on the roadmap.

Redirect chains don't re-run Rewrite. followRedirects re-issues via base.RoundTrip, not through the Rewrite closure, so X-Forwarded-Proto stays at the value computed on the first hop across the whole redirect chain. For same-scheme redirects that's fine. The edge case is an HTTP→HTTPS upgrade redirect within a chain... the stale http would carry to the HTTPS target, re-introducing the original bug for that hop. Unlikely in practice (most HTTPS-enforcing servers don't redirect from HTTP to HTTP-then-HTTPS on the same host within a chain the proxy initiates), but it's the same root cause. Worth a one-line note in the upstreamForwardedProto doc, or a recompute inside followRedirects if you want defense-in-depth.

trustProxyHeaders dual-purpose. Re-using the flag for outbound trust is defensible... both uses are "do we trust reverse-proxy-supplied X-Forwarded-*?"... but the two directions (inbound response rewriting vs outbound request-header trust) are independent trust decisions over different headers. Acceptable to collapse them behind one boolean today, but only if the comment is fixed (see above). Don't split the field unless there's a concrete near-term reason to... that's a wide blast radius through the constructor chain for a hypothetical.

Polish

• upstreamForwardedProto re-parses p.targetURI per request (line 1110) when the Rewrite closure already has targetURL parsed once at line 1134. Pass the parsed URL (or just the scheme string) in instead.
• The capture comment at line 1081 ("before SetXForwarded() overwrites it") is slightly misleading... SetXForwarded() writes to pr.Out, not pr.In, so the capture ordering isn't strictly required. The real subtlety is that pr.In (not pr.Out) is the source because httputil strips X-Forwarded-* from the outbound request before Rewrite runs. Worth tightening the comment to say that.
• Test table is missing the default: branch of the scheme switch (e.g. targetURI: "ftp://..." asserting wantProto: "") and a junk trusted proto ("ftp" with trustProxyHeaders: true). One row each closes the gap.

Summary

The fix is correct for the reported bug and the security posture against the usual forwarded-header attacks is solid (no CRLF injection, host removal preserved, local path unchanged, fallback validates). The one thing I'd block on is the unvalidated inboundProto on the trust path... one line to close a real trust-boundary gap in security code. The stale // Deprecated comment is a hard standards violation and should go in the same change. The backend_url ordering and redirect-chain edge cases are judgement calls that can land as follow-ups if you want to keep this tight.

[JAORMX]

Trust-boundary gap: the fallback path validates the scheme against {http, https} (the switch at line 1114), but the trust path returns inboundProto verbatim with no check. inboundProto comes from pr.In.Header.Get("X-Forwarded-Proto") (line 1083), which is client-controllable... the stdlib ReverseProxy strips client X-Forwarded-* from pr.Out before Rewrite, but pr.In retains whatever the client sent.

When trustProxyHeaders is on, an attacker who can reach the proxy directly (port-forward, misconfigured ingress... the listener is plain HTTP) can set X-Forwarded-Proto: http against an HTTPS upstream and force the redirect loop this PR exists to fix, or send arbitrary tokens to a third-party upstream. CWE-601 / OWASP A05:2021.

Gated behind an opt-in flag, so exploitation requires operator misconfiguration, and the same unvalidated-trust pattern already exists on the SSE response-processor path (sse_response_processor.go:159-166). CRLF injection is NOT possible (Go's parser rejects control chars in header values). So this is Medium, not higher. But this is security code, and the fix is one line:

if p.trustProxyHeaders && isHTTPScheme(inboundProto) {
    return inboundProto
}

with func isHTTPScheme(s string) bool { return s == "http" || s == "https" }. Add a test row for inboundProto: "httpx" with trustProxyHeaders: true asserting the fallback wins.

[aron-muon]

Fixed in d6f779a. Both paths now validate against {http, https} via a shared isHTTPScheme helper:

func (p *TransparentProxy) upstreamForwardedProto(inboundProto, upstreamScheme string) string {
    if p.trustProxyHeaders && isHTTPScheme(inboundProto) {
        return inboundProto
    }
    if isHTTPScheme(upstreamScheme) {
        return upstreamScheme
    }
    return ""
}

func isHTTPScheme(s string) bool { return s == "http" || s == "https" }

A junk trusted value now falls through to the upstream scheme rather than being reflected. Added the requested test row (inboundProto: "httpx" with trustProxyHeaders: true asserting the fallback wins), plus a non-http upstream scheme row covering the fallback's default branch. Good catch — agreed this belonged in the same change given it's security code.

[JAORMX]

Latent trap: setXForwardedHeaders runs at line 1144, before the backend_url override at lines 1148-1165 that can rewrite pr.Out.URL.Scheme/Host. So the header is derived from the static targetURI, not the actual request destination.

I traced this... podBackendURL (line 819) always preserves the targetURI scheme (bails to targetURI for HTTPS, only swaps host for HTTP), so the mismatch is inert today. But if backend_url ever carries an independent scheme, the header will silently lie about the wire protocol. Cheapest fix: a one-line comment here noting the scheme-equivalence assumption so the next maintainer doesn't "fix" it by reading pr.Out.URL.Scheme and introducing an ordering bug. Cleaner: move setXForwardedHeaders to after the backend_url block and derive from pr.Out.URL.Scheme. I'd take the comment now.

[aron-muon]

Took the comment option in d6f779a. setXForwardedHeaders now receives the upstream scheme already parsed in Start (so no per-request re-parse), and the doc on upstreamForwardedProto spells out the assumption and warns the next maintainer off the trap:

upstreamScheme is the scheme of the statically-configured target URL [...]. It is used deliberately instead of pr.Out.URL.Scheme: the per-session backend_url override that can rewrite pr.Out.URL runs after this function [...]. The override only ever swaps the host for HTTP targets and preserves the scheme for HTTPS targets (see podBackendURL), so upstreamScheme always matches the wire protocol. Do not "simplify" this to read pr.Out.URL.Scheme.

Left the move-after-backend_url refactor as a potential follow-up if a scheme-divergent backend_url ever lands.

[JAORMX]

The comment "before SetXForwarded() overwrites it" is slightly misleading... SetXForwarded() writes to pr.Out, not pr.In, so the capture ordering isn't strictly required. The real subtlety is that pr.In (not pr.Out) is the source because httputil strips X-Forwarded-* from the outbound request before Rewrite runs. Worth tightening the comment to say that, since that's the non-obvious fact a future maintainer needs.

[aron-muon]

Tightened in d6f779a. The comment now explains the real subtlety — that pr.In is the source because httputil strips the client headers from the outbound request:

// Read the client-supplied X-Forwarded-Proto from pr.In. We must read it
// from the inbound request (not pr.Out) because httputil.ReverseProxy strips
// all client-supplied X-Forwarded-* headers from the outbound request before
// Rewrite runs, so pr.Out never carries the original client value.