Consume duplicate forwarding headers in ProxyHeadersMiddleware (#2971)

Author: Kludex

docs/deployment/index.md

@@ -270,6 +270,8 @@ or Literals (e.g. `/path/to/socket.sock`). When running from CLI these are confi
 !!! Warning "Only trust clients you can actually trust!"
     Incorrectly trusting other clients can lead to malicious actors spoofing their apparent client address to your application.
 
+A proxy chain may send a header once per hop rather than as a single comma-separated value. Repeated `X-Forwarded-For` fields are combined in order (as the equivalent comma-separated list, [RFC 9110, 5.3](https://www.rfc-editor.org/rfc/rfc9110#section-5.3)), while for `X-Forwarded-Proto` the last field is used.
+
 For more information, check [`ProxyHeadersMiddleware`](https://github.com/Kludex/uvicorn/blob/main/uvicorn/middleware/proxy_headers.py).
 
 ### Client Port

tests/middleware/test_proxy_headers.py

@@ -582,7 +582,7 @@ async def _noop_send(message: ASGISendEvent) -> None:  # pragma: no cover
 
 
 @pytest.mark.anyio
-async def test_proxy_headers_duplicate_x_forwarded_for_is_ignored() -> None:
+async def test_proxy_headers_duplicate_x_forwarded_for_is_combined() -> None:
     captured: dict[str, tuple[str, int] | None] = {}
 
     async def app(scope: Scope, receive: ASGIReceiveCallable, send: ASGISendCallable) -> None:
@@ -592,29 +592,57 @@ async def app(scope: Scope, receive: ASGIReceiveCallable, send: ASGISendCallable
     middleware = ProxyHeadersMiddleware(app, trusted_hosts="127.0.0.1")
     scope = _make_http_scope(
         [
-            (b"x-forwarded-for", b"198.51.100.23, 127.0.0.1"),
-            (b"x-forwarded-for", b"203.0.113.66"),
+            (b"x-forwarded-for", b"203.0.113.66, 198.51.100.23"),
+            (b"x-forwarded-for", b"127.0.0.1"),
         ]
     )
     await middleware(scope, _noop_receive, _noop_send)
-    assert captured["client"] == ("127.0.0.1", 12345)
+    assert captured["client"] == ("198.51.100.23", 0)
 
 
 @pytest.mark.anyio
-async def test_proxy_headers_duplicate_x_forwarded_proto_is_ignored() -> None:
+@pytest.mark.parametrize(
+    ("proto_headers", "expected"),
+    [
+        ([(b"x-forwarded-proto", b"https"), (b"x-forwarded-proto", b"http")], "http"),
+        ([(b"x-forwarded-proto", b"http"), (b"x-forwarded-proto", b"https")], "https"),
+    ],
+)
+async def test_proxy_headers_duplicate_x_forwarded_proto_uses_last(
+    proto_headers: list[tuple[bytes, bytes]],
+    expected: str,
+) -> None:
     captured: dict[str, str] = {}
 
     async def app(scope: Scope, receive: ASGIReceiveCallable, send: ASGISendCallable) -> None:
         assert scope["type"] == "http"
         captured["scheme"] = scope["scheme"]
 
     middleware = ProxyHeadersMiddleware(app, trusted_hosts="127.0.0.1")
+    scope = _make_http_scope(proto_headers, scheme="http")
+    await middleware(scope, _noop_receive, _noop_send)
+    assert captured["scheme"] == expected
+
+
+@pytest.mark.anyio
+async def test_proxy_headers_haproxy_behind_alb() -> None:
+    captured: dict[str, object] = {}
+
+    async def app(scope: Scope, receive: ASGIReceiveCallable, send: ASGISendCallable) -> None:
+        assert scope["type"] == "http"
+        captured["client"] = scope["client"]
+        captured["scheme"] = scope["scheme"]
+
+    middleware = ProxyHeadersMiddleware(app, trusted_hosts=["127.0.0.1", "10.0.0.1"])
     scope = _make_http_scope(
         [
-            (b"x-forwarded-proto", b"http"),
+            (b"x-forwarded-for", b"1.2.3.4"),
+            (b"x-forwarded-for", b"10.0.0.1"),
+            (b"x-forwarded-proto", b"https"),
             (b"x-forwarded-proto", b"https"),
         ],
         scheme="http",
     )
     await middleware(scope, _noop_receive, _noop_send)
-    assert captured["scheme"] == "http"
+    assert captured["client"] == ("1.2.3.4", 0)
+    assert captured["scheme"] == "https"

uvicorn/middleware/proxy_headers.py

@@ -32,26 +32,25 @@ async def __call__(self, scope: Scope, receive: ASGIReceiveCallable, send: ASGIS
         client_host = client_addr[0] if client_addr else None
 
         if client_host in self.trusted_hosts:
-            x_forwarded_proto_values: list[bytes] = []
+            x_forwarded_proto_value: bytes | None = None
             x_forwarded_for_values: list[bytes] = []
             for name, value in scope["headers"]:
                 if name == b"x-forwarded-proto":
-                    x_forwarded_proto_values.append(value)
+                    x_forwarded_proto_value = value
                 elif name == b"x-forwarded-for":
                     x_forwarded_for_values.append(value)
 
-            # Only consume the header when exactly one copy is present to avoid spoofing issues.
-            if len(x_forwarded_proto_values) == 1:
-                x_forwarded_proto = x_forwarded_proto_values[0].decode("latin1").strip()
+            if x_forwarded_proto_value is not None:
+                x_forwarded_proto = x_forwarded_proto_value.decode("latin1").strip()
 
                 if x_forwarded_proto in {"http", "https", "ws", "wss"}:
                     if scope["type"] == "websocket":
                         scope["scheme"] = x_forwarded_proto.replace("http", "ws")
                     else:
                         scope["scheme"] = x_forwarded_proto
 
-            if len(x_forwarded_for_values) == 1:
-                x_forwarded_for = x_forwarded_for_values[0].decode("latin1")
+            if x_forwarded_for_values:
+                x_forwarded_for = b", ".join(x_forwarded_for_values).decode("latin1")
                 host, port = self.trusted_hosts.get_trusted_client_address(x_forwarded_for)
 
                 if host: