fix: resolve client IP from the rightmost untrusted X-Forwarded-For entry#26646
fix: resolve client IP from the rightmost untrusted X-Forwarded-For entry#26646sreya wants to merge 6 commits into
Conversation
The X-Forwarded-For header was parsed left-to-right, so the leftmost (client-controlled) value was accepted as the real client IP when the request arrived from a trusted proxy. That spoofable IP feeds per-IP login rate limiting and audit log source addresses. This test reproduces the spoof with a realistic trusted-proxy CIDR and currently fails: it asserts the rightmost non-trusted address (the real client) is used instead of the forged leftmost value. The fix follows in a subsequent commit.
Forwarding headers were parsed left-to-right, returning the leftmost comma-separated value. Reverse proxies append the peer that connected to them, so the leftmost value is client-controlled and could be forged to spoof the IP used for per-IP login rate limiting and audit log source addresses. Parse forwarding header chains right-to-left and return the first address that is not a trusted origin (the real client). When every hop is a trusted origin, fall back to the leftmost address to preserve behavior for broad-trust configurations. Whitespace around each hop is trimmed. This makes the recovered IP non-forgeable while leaving existing behavior unchanged when the trusted-origin set covers the whole chain.
A deep review found a residual spoofing vector: Header.Get returns only the first X-Forwarded-For field line, so a proxy that appends its hop as a separate header line left the client-controlled first line as the selected address. Per RFC 7230, multiple field lines with the same name are equivalent to one comma-separated value. Join all X-Forwarded-For field lines before selecting the rightmost untrusted address. Single-value forwarding headers (X-Real-Ip, Cf-Connecting-Ip, True-Client-Ip) keep first-value semantics via Header.Get. Adds a multiple-header-lines regression case and refines the helper comment to note the rightmost-untrusted guarantee holds when all trusted proxy hops are listed in TrustedOrigins.
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 6 findings (3 P3, 3 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel (14 reviewers). CRF-1 from Netero. CRF-2 convergent across 5 reviewers. CRF-3 convergent across 3 reviewers (verified mitigated by current call sequence). 3 P3, 3 Nit posted. 2 P4 dropped. Reviewed against 51591e3..3283dd7. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Solid security fix. The rightmost-untrusted algorithm is the correct defense against XFF spoofing, the multi-line header join closes a second vector, and the test-first approach (commit c685865 writes the failing test before the fix) shows genuine rigor. 14 reviewers examined this; Pariston's verdict: "I tried to build a case against this and could not."
3 P3, 3 Nit. No blocking issues.
CRF-2 and CRF-3 are on code outside this diff. CRF-2 is the stale getRemoteAddress comment that teaches the exact vulnerability pattern this PR fixes; five reviewers independently flagged it. CRF-3 is a pre-existing sibling pattern in FilterUntrustedOriginHeaders that is currently mitigated by the call sequence in proxyWorkspaceApp (line 563-566) where EnsureXForwardedForHeader rebuilds XFF from the already-resolved RemoteAddr. Worth fixing for structural safety, but not blocking.
Notable: extractForwardedAddress is called for all trusted headers, not just XFF. Five reviewers analyzed this and all confirmed it's a no-op for realistic single-value headers. If a non-XFF header ever carries commas, the new behavior (rightmost untrusted) is strictly more secure than the old (leftmost).
coderd/httpmw/realip.go:198
P3 [CRF-2] getRemoteAddress doc comment and inline comment still teach the exact security assumption this PR fixes.
"the first value is the client address"
No it isn't. That's the whole point of this PR. After this change, extractForwardedAddress handles XFF parsing with right-to-left trust evaluation. But the comment still tells the next reader that "the first value is the client address." If someone reads this and uses getRemoteAddress directly for a new forwarding header integration, the same spoofing vulnerability comes back.
The doc should describe what the function does (extracts the first comma-delimited IP from a string, stripping ports) without claiming it yields "the client address" or "the original address." The inline XFF comment should be removed or redirected to extractForwardedAddress.
(Leorio P3, Ryosuke Nit, Robin Nit, Meruem Nit, Gon Note)
🤖
coderd/httpmw/realip.go:113
P3 [CRF-3] FilterUntrustedOriginHeaders collapses multi-line XFF to the first (client-controlled) header line, the same class of bug this PR fixes in ExtractRealIPAddress.
req.Header.Set(header, req.Header.Get(header)) drops all but the first field line. In a multi-line spoofing scenario (X-Forwarded-For: spoofed\r\nX-Forwarded-For: real, 10.0.0.2), only spoofed survives.
Currently mitigated: the sole caller in proxyWorkspaceApp (proxy.go:563-566) immediately rebuilds XFF via EnsureXForwardedForHeader from the already-resolved RemoteAddr. But the mitigation is incidental, not structural; the two functions are not documented as a required pair. If FilterUntrustedOriginHeaders is ever called without EnsureXForwardedForHeader following, the multi-line bypass re-emerges.
Pre-existing, not introduced by this PR.
(Meruem P3, Kurapika Note)
🤖
🤖 This review was automatically generated with Coder Agents.
| // arbitrary values, while trusted proxies append the real client to the right. | ||
| // The resolved address must therefore be the rightmost entry that is not a | ||
| // trusted origin, not the leftmost (client-supplied) entry. | ||
| func TestExtractRealIPSpoofedXForwardedFor(t *testing.T) { |
There was a problem hiding this comment.
P3 [CRF-1] Missing test for the all-trusted-origins fallback path in extractForwardedAddress.
The doc comment (realip.go:225) states: "If every parsed address is a trusted origin, the leftmost address is returned." All three test cases include at least one untrusted address (realClient), so the leftmost fallback at line 242 is never exercised.
A case like XFF 10.0.0.1, 10.0.0.2 with 10.0.0.0/8 trusted would cover the fallback and lock down the contract.
(Netero)
🤖
| require.NoError(t, err) | ||
| require.Equal(t, realClient, addr.String(), | ||
| "must use the real client IP, not the spoofed leftmost value") | ||
| require.NotEqual(t, spoofedAddr, addr.String(), |
There was a problem hiding this comment.
Nit [CRF-4] require.NotEqual(t, spoofedAddr, addr.String()) is redundant after require.Equal(t, realClient, addr.String()) on line 822. Since realClient != spoofedAddr, the NotEqual can never fail independently. Reads as intent documentation rather than a test; removing it loses no coverage.
(Bisky)
🤖
| t.Parallel() | ||
|
|
||
| const ( | ||
| spoofedAddr = "1.2.3.4" |
There was a problem hiding this comment.
Nit [CRF-5] spoofedAddr names the data type (an address); realClient names the role (a client). Both are IP address strings. Using one naming axis would be clearer: spoofedAddr/realClientAddr or spoofedIP/realClientIP.
(Gon)
🤖
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Direct call to ExtractRealIPAddress. |
There was a problem hiding this comment.
Nit [CRF-6] "Direct call to ExtractRealIPAddress." The next line is httpmw.ExtractRealIPAddress(newConfig(), req). Pure restatement; the comment adds nothing the code doesn't show. Delete or replace with what makes this section distinct from the middleware section below (e.g., tests the return value vs. the RemoteAddr rewrite).
(Gon)
🤖
…tAddress The spoofing scenarios were in a separate TestExtractRealIPSpoofedXForwardedFor function whose direct ExtractRealIPAddress assertions duplicated the existing TestExtractAddress table. Move the single-proxy, chained-proxy, and multiple-header-line cases into that table, which also adds the previously missing narrow-CIDR (10.0.0.0/8) coverage alongside the existing 0.0.0.0/0 cases. The middleware-level RemoteAddr rewrite remains covered by TestTrustedOrigins.
Rename the spoofing cases to describe the behavior each verifies, and repurpose the redundant single-proxy case (which duplicated the chained case) to cover the no-trusted-origins scenario where X-Forwarded-For is ignored: - no-trusted-origins: forwarding headers ignored without trusted origins - picks-rightmost-untrusted: rightmost non-trusted entry wins - x-forwarded-for-set-multiple-times: multiple header lines are joined
The no-trusted-origins behavior is already covered by default-empty-config.
Resolve
X-Forwarded-Forby walking the chain right-to-left and selecting the rightmost entry that is not a trusted origin (the real client), falling back to the leftmost entry only when every parsed hop is a trusted origin.