Skip to content

Fix X-Forwarded-For not properly handling ports#2823

Closed
worksbyfriday wants to merge 1 commit intoKludex:mainfrom
worksbyfriday:fix-x-forwarded-for-port
Closed

Fix X-Forwarded-For not properly handling ports#2823
worksbyfriday wants to merge 1 commit intoKludex:mainfrom
worksbyfriday:fix-x-forwarded-for-port

Conversation

@worksbyfriday
Copy link
Copy Markdown

Summary

  • When X-Forwarded-For entries include a port (e.g. 1.2.3.4:1024), the port was included in the host field of scope["client"], producing malformed tuples like ("1.2.3.4:1024", 0)
  • The trust check also failed because ipaddress.ip_address() cannot parse 1.2.3.4:1024
  • Added _parse_host_and_port() helper to extract host and port separately, handling IPv4 (host:port), bracketed IPv6 ([::1]:port), and bare addresses

Fixes #2789

Test plan

  • Added test_proxy_headers_x_forwarded_for_with_port — IPv4 with port, IPv4 without port, bracketed IPv6 with port, bare IPv6
  • Added test_proxy_headers_x_forwarded_for_port_with_trusted_proxy — verifies trust check works when proxy sends host:port
  • All 253 proxy header tests pass

When X-Forwarded-For entries include a port (e.g. "1.2.3.4:1024"),
the port was being included in the host field of scope["client"],
producing malformed tuples like ("1.2.3.4:1024", 0). Additionally,
the trust check was failing because ipaddress.ip_address() cannot
parse "1.2.3.4:1024".

Add _parse_host_and_port() to extract host and port from forwarded
entries, handling IPv4 (host:port), bracketed IPv6 ([::1]:port),
and bare addresses without ports.

Fixes Kludex#2789

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@hrv-dys hrv-dys left a comment

Choose a reason for hiding this comment

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

This tackles a real gap — X-Forwarded-For values can include ports (e.g., from AWS ALB or HAProxy), and the current code just passes the whole string as the host, which breaks the client tuple semantics.

On the implementation:

  1. _parse_host_and_port() — the function handles three cases: bracketed IPv6 ([::1]:8080), IPv4 with exactly one colon (1.2.3.4:8080), and bare hosts. The host.count(":") == 1 check for IPv4 is a good heuristic to distinguish 1.2.3.4:8080 from bare IPv6 like ::1. Nice.

  2. Trust checking — the change at line 166 (addr, _ = _parse_host_and_port(host)) strips the port before checking against trusted hosts. This is critical — without it, "10.0.0.1:8080" would fail the __contains__ check against "10.0.0.1" in the trusted set, and trusted proxies with ports would be treated as untrusted. The test test_proxy_headers_x_forwarded_for_port_with_trusted_proxy validates exactly this.

  3. The return value preserves the full host:port string as the client host — wait, actually looking again, you're returning (addr, port) from _parse_host_and_port but in get_trusted_client_host you still return the raw host string (which includes the port). So the scope["client"] tuple becomes ("1.2.3.4:8080", port_int) — hmm, actually no, looking at the middleware code: addr, port = _parse_host_and_port(host) and then scope["client"] = (addr, port). That's correct — the addr is just the IP, port is the integer. Good.

  4. Comparison with PR #2822 — that PR takes a regex-based approach with _IPV6_HOST_PORT and _IPV4_HOST_PORT compiled patterns and renames get_trusted_client_host to get_trusted_client_host_port to return the tuple directly. Both approaches pass all 245 tests. This PR's approach is arguably more readable — the sequential if/elif in _parse_host_and_port is easier to follow than regex matching. On the other hand, #2822's approach avoids parsing twice (once for trust checking, once for the client tuple).

All 245 proxy header tests pass (8 skipped — wsproto not installed, pre-existing).

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.

X-Forwarded-For doesn't properly handle ports

2 participants