Skip to content

Commit b7761f7

Browse files
alukachalukach
authored
Fix default port handling in ProxyHeaderMiddleware for forwarded headers (#873)
1 parent 3b24f86 commit b7761f7

2 files changed

Lines changed: 123 additions & 25 deletions

File tree

‎stac_fastapi/api/stac_fastapi/api/middleware.py‎

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -82,36 +82,55 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
8282

8383
await self.app(scope, receive, send)
8484

85-
def _get_forwarded_url_parts(self, scope: Scope) -> Tuple[str, str, str]:
85+
def _get_forwarded_url_parts(self, scope: Scope) -> Tuple[str, str, int]:
8686
proto = scope.get("scheme", "http")
87-
header_host = self._get_header_value_by_name(scope, "host")
88-
if header_host is None:
89-
domain, port = scope["server"]
90-
else:
87+
# Assume default port based on protocol, can be overridden later
88+
port = 443 if proto == "https" else 80
89+
90+
if header_host := self._get_header_value_by_name(scope, "host"):
9191
header_host_parts = header_host.split(":")
92+
domain = header_host_parts[0]
9293
if len(header_host_parts) == 2:
93-
domain, port = header_host_parts
94-
else:
95-
domain = header_host_parts[0]
96-
port = None
97-
98-
port_str = None # make sure it is defined in all paths since we access it later
94+
with contextlib.suppress(ValueError):
95+
port = int(header_host_parts[1])
96+
else:
97+
# Not sure when we would not have a host header, but fallback to server info
98+
domain, port = scope["server"]
99+
port = int(port)
100+
101+
forwarded_port: Optional[str] = None
102+
forwarding_occurred = any(
103+
key
104+
in [
105+
b"forwarded",
106+
b"x-forwarded-proto",
107+
b"x-forwarded-host",
108+
b"x-forwarded-port",
109+
]
110+
for key, _ in scope["headers"]
111+
)
99112

100113
if forwarded := self._get_header_value_by_name(scope, "forwarded"):
101114
for proxy in forwarded.split(","):
102115
if proto_expr := _PROTO_HEADER_REGEX.search(proxy):
103116
proto = proto_expr.group("proto")
104117
if host_expr := _HOST_HEADER_REGEX.search(proxy):
105118
domain = host_expr.group("host")
106-
port_str = host_expr.group("port") # None if not present in the match
119+
forwarded_port = host_expr.group("port") # None if not present
107120

108121
else:
109-
domain = self._get_header_value_by_name(scope, "x-forwarded-host", domain)
110-
proto = self._get_header_value_by_name(scope, "x-forwarded-proto", proto)
111-
port_str = self._get_header_value_by_name(scope, "x-forwarded-port", port)
112-
113-
with contextlib.suppress(ValueError): # ignore ports that are not valid integers
114-
port = int(port_str) if port_str is not None else port
122+
domain = self._get_header_value_by_name(scope, "x-forwarded-host") or domain
123+
proto = self._get_header_value_by_name(scope, "x-forwarded-proto") or proto
124+
forwarded_port = self._get_header_value_by_name(scope, "x-forwarded-port")
125+
126+
if forwarding_occurred and not forwarded_port:
127+
# If forwarding occurred but no port was specified, use protocol default
128+
forwarded_port = "443" if proto == "https" else "80"
129+
130+
if forwarded_port:
131+
# ignore ports that are not valid integers
132+
with contextlib.suppress(ValueError):
133+
port = int(forwarded_port)
115134

116135
return (proto, domain, port)
117136

‎stac_fastapi/api/tests/test_middleware.py‎

Lines changed: 86 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ def test_replace_header_value_by_name(
7575
"scope,expected",
7676
[
7777
(
78-
{"scheme": "https", "server": ["testserver", 80], "headers": []},
79-
("https", "testserver", 80),
78+
{"scheme": "https", "server": ["testserver", 8000], "headers": []},
79+
("https", "testserver", 8000),
8080
),
8181
(
8282
{
@@ -92,7 +92,7 @@ def test_replace_header_value_by_name(
9292
"server": ["testserver", 80],
9393
"headers": [(b"host", b"testserver")],
9494
},
95-
("http", "testserver", None),
95+
("http", "testserver", 80),
9696
),
9797
(
9898
{
@@ -108,7 +108,7 @@ def test_replace_header_value_by_name(
108108
"server": ["testserver", 80],
109109
"headers": [(b"forwarded", b"proto=https;host=test:not-an-integer")],
110110
},
111-
("https", "test", 80),
111+
("https", "test", 443),
112112
),
113113
(
114114
{
@@ -124,7 +124,7 @@ def test_replace_header_value_by_name(
124124
"server": ["testserver", 80],
125125
"headers": [(b"x-forwarded-proto", b"https")],
126126
},
127-
("https", "testserver", 80),
127+
("https", "testserver", 443),
128128
),
129129
(
130130
{
@@ -191,11 +191,90 @@ def test_replace_header_value_by_name(
191191
(
192192
b"forwarded",
193193
# proto is set, but no host
194-
b'for="85.193.181.55";proto=https,for="85.193.181.55";proto=https',
194+
b'for="85.193.181.55";proto=https',
195195
)
196196
],
197197
},
198-
("https", "testserver", 80),
198+
("https", "testserver", 443),
199+
),
200+
(
201+
{
202+
"scheme": "http",
203+
"server": ["testserver", 8080],
204+
"headers": [
205+
(b"host", b"example.com:8080"),
206+
(
207+
b"forwarded",
208+
# Forwarded header with proto and host but no port
209+
# Should use default port for https (443), not the original host
210+
# port (8080)
211+
b"proto=https;host=myproxy.com",
212+
),
213+
],
214+
},
215+
("https", "myproxy.com", 443),
216+
),
217+
(
218+
{
219+
"scheme": "http",
220+
"server": ["testserver", 8080],
221+
"headers": [
222+
(b"host", b"example.com:8080"),
223+
(
224+
b"forwarded",
225+
# Forwarded header with proto and host but no port
226+
# Should use default port for http (80), not the original host
227+
# port (8080)
228+
b"proto=http;host=myproxy.com",
229+
),
230+
],
231+
},
232+
("http", "myproxy.com", 80),
233+
),
234+
(
235+
{
236+
"scheme": "http",
237+
"server": ["testserver", 8080],
238+
"headers": [
239+
(b"host", b"example.com:8080"),
240+
(b"x-forwarded-proto", b"https"),
241+
(b"x-forwarded-host", b"myproxy.com"),
242+
# No x-forwarded-port header
243+
# Should use default port for https (443), not the original host
244+
# port (8080)
245+
],
246+
},
247+
("https", "myproxy.com", 443),
248+
),
249+
(
250+
{
251+
"scheme": "http",
252+
"server": ["testserver", 8080],
253+
"headers": [
254+
(b"host", b"example.com:8080"),
255+
(b"x-forwarded-proto", b"http"),
256+
(b"x-forwarded-host", b"myproxy.com"),
257+
# No x-forwarded-port header
258+
# Should use default port for http (80), not the original host port
259+
# (8080)
260+
],
261+
},
262+
("http", "myproxy.com", 80),
263+
),
264+
(
265+
{
266+
"scheme": "http",
267+
"server": ["testserver", 80],
268+
"headers": [
269+
(b"host", b"testserver"),
270+
(b"x-forwarded-proto", b"https"),
271+
# No x-forwarded-host (domain stays the same)
272+
# No x-forwarded-port header
273+
# Should use default port for https (443), not the original port (80)
274+
# because the protocol changed
275+
],
276+
},
277+
("https", "testserver", 443),
199278
),
200279
],
201280
)

0 commit comments

Comments
 (0)