Skip to content

Commit f05b132

Browse files
illia-vpquentinsethmlarson
authored
Merge commit from fork
* Apply Quentin's suggestion Co-authored-by: Quentin Pradet <[email protected]> * Add tests for disabled redirects in the pool manager * Add a possible fix for the issue with not raised `MaxRetryError` * Make urllib3 handle redirects instead of JS when JSPI is used * Fix info in the new comment * State that redirects with XHR are not controlled by urllib3 * Remove excessive params from new test requests * Add tests reaching max non-0 redirects * Test redirects with Emscripten * Fix `test_merge_pool_kwargs` * Add a changelog entry * Parametrize tests * Drop a fix for Emscripten * Apply Seth's suggestion to docs Co-authored-by: Seth Michael Larson <[email protected]> * Use a minor release instead of the patch one --------- Co-authored-by: Quentin Pradet <[email protected]> Co-authored-by: Seth Michael Larson <[email protected]>
1 parent d03fe32 commit f05b132

File tree

7 files changed

+148
-4
lines changed

7 files changed

+148
-4
lines changed

‎CHANGES.rst‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,12 @@
1+
2.5.0 (TBD)
2+
==================
3+
4+
- Fixed a security issue where restricting the maximum number of followed
5+
redirects at the ``urllib3.PoolManager`` level via the ``retries`` parameter
6+
did not work.
7+
- TODO: add other entries in the release PR.
8+
9+
110
2.4.0 (2025-04-10)
211
==================
312

‎docs/reference/contrib/emscripten.rst‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ Features which are usable with Emscripten support are:
6565
* Timeouts
6666
* Retries
6767
* Streaming (with Web Workers and Cross-Origin Isolation)
68-
* Redirects
68+
* Redirects (determined by browser/runtime, not restrictable with urllib3)
6969
* Decompressing response bodies
7070

7171
Features which don't work with Emscripten:

‎dummyserver/app.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ async def encodingrequest() -> ResponseReturnValue:
227227

228228

229229
@hypercorn_app.route("/redirect", methods=["GET", "POST", "PUT"])
230+
@pyodide_testing_app.route("/redirect", methods=["GET", "POST", "PUT"])
230231
async def redirect() -> ResponseReturnValue:
231232
"Perform a redirect to ``target``"
232233
values = await request.values

‎src/urllib3/poolmanager.py‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,22 @@ def __init__(
203203
**connection_pool_kw: typing.Any,
204204
) -> None:
205205
super().__init__(headers)
206+
if "retries" in connection_pool_kw:
207+
retries = connection_pool_kw["retries"]
208+
if not isinstance(retries, Retry):
209+
# When Retry is initialized, raise_on_redirect is based
210+
# on a redirect boolean value.
211+
# But requests made via a pool manager always set
212+
# redirect to False, and raise_on_redirect always ends
213+
# up being False consequently.
214+
# Here we fix the issue by setting raise_on_redirect to
215+
# a value needed by the pool manager without considering
216+
# the redirect boolean.
217+
raise_on_redirect = retries is not False
218+
retries = Retry.from_int(retries, redirect=False)
219+
retries.raise_on_redirect = raise_on_redirect
220+
connection_pool_kw = connection_pool_kw.copy()
221+
connection_pool_kw["retries"] = retries
206222
self.connection_pool_kw = connection_pool_kw
207223

208224
self.pools: RecentlyUsedContainer[PoolKey, HTTPConnectionPool]
@@ -456,7 +472,7 @@ def urlopen( # type: ignore[override]
456472
kw["body"] = None
457473
kw["headers"] = HTTPHeaderDict(kw["headers"])._prepare_for_method_change()
458474

459-
retries = kw.get("retries")
475+
retries = kw.get("retries", response.retries)
460476
if not isinstance(retries, Retry):
461477
retries = Retry.from_int(retries, redirect=redirect)
462478

‎test/contrib/emscripten/test_emscripten.py‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -944,6 +944,22 @@ def count_calls(self, *args, **argv): # type: ignore[no-untyped-def]
944944
pyodide_test(selenium_coverage, testserver_http.http_host, find_unused_port())
945945

946946

947+
def test_redirects(
948+
selenium_coverage: typing.Any, testserver_http: PyodideServerInfo
949+
) -> None:
950+
@run_in_pyodide # type: ignore[misc]
951+
def pyodide_test(selenium_coverage: typing.Any, host: str, port: int) -> None:
952+
from urllib3 import request
953+
954+
redirect_url = f"http://{host}:{port}/redirect"
955+
response = request("GET", redirect_url)
956+
assert response.status == 200
957+
958+
pyodide_test(
959+
selenium_coverage, testserver_http.http_host, testserver_http.http_port
960+
)
961+
962+
947963
def test_insecure_requests_warning(
948964
selenium_coverage: typing.Any, testserver_http: PyodideServerInfo
949965
) -> None:

‎test/test_poolmanager.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -379,9 +379,10 @@ def test_pool_kwargs_socket_options(self) -> None:
379379

380380
def test_merge_pool_kwargs(self) -> None:
381381
"""Assert _merge_pool_kwargs works in the happy case"""
382-
p = PoolManager(retries=100)
382+
retries = retry.Retry(total=100)
383+
p = PoolManager(retries=retries)
383384
merged = p._merge_pool_kwargs({"new_key": "value"})
384-
assert {"retries": 100, "new_key": "value"} == merged
385+
assert {"retries": retries, "new_key": "value"} == merged
385386

386387
def test_merge_pool_kwargs_none(self) -> None:
387388
"""Assert false-y values to _merge_pool_kwargs result in defaults"""

‎test/with_dummyserver/test_poolmanager.py‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,89 @@ def test_redirect_to_relative_url(self) -> None:
8484
assert r.status == 200
8585
assert r.data == b"Dummy server!"
8686

87+
@pytest.mark.parametrize(
88+
"retries",
89+
(0, Retry(total=0), Retry(redirect=0), Retry(total=0, redirect=0)),
90+
)
91+
def test_redirects_disabled_for_pool_manager_with_0(
92+
self, retries: typing.Literal[0] | Retry
93+
) -> None:
94+
"""
95+
Check handling redirects when retries is set to 0 on the pool
96+
manager.
97+
"""
98+
with PoolManager(retries=retries) as http:
99+
with pytest.raises(MaxRetryError):
100+
http.request("GET", f"{self.base_url}/redirect")
101+
102+
# Setting redirect=True should not change the behavior.
103+
with pytest.raises(MaxRetryError):
104+
http.request("GET", f"{self.base_url}/redirect", redirect=True)
105+
106+
# Setting redirect=False should not make it follow the redirect,
107+
# but MaxRetryError should not be raised.
108+
response = http.request("GET", f"{self.base_url}/redirect", redirect=False)
109+
assert response.status == 303
110+
111+
@pytest.mark.parametrize(
112+
"retries",
113+
(
114+
False,
115+
Retry(total=False),
116+
Retry(redirect=False),
117+
Retry(total=False, redirect=False),
118+
),
119+
)
120+
def test_redirects_disabled_for_pool_manager_with_false(
121+
self, retries: typing.Literal[False] | Retry
122+
) -> None:
123+
"""
124+
Check that setting retries set to False on the pool manager disables
125+
raising MaxRetryError and redirect=True does not change the
126+
behavior.
127+
"""
128+
with PoolManager(retries=retries) as http:
129+
response = http.request("GET", f"{self.base_url}/redirect")
130+
assert response.status == 303
131+
132+
response = http.request("GET", f"{self.base_url}/redirect", redirect=True)
133+
assert response.status == 303
134+
135+
response = http.request("GET", f"{self.base_url}/redirect", redirect=False)
136+
assert response.status == 303
137+
138+
def test_redirects_disabled_for_individual_request(self) -> None:
139+
"""
140+
Check handling redirects when they are meant to be disabled
141+
on the request level.
142+
"""
143+
with PoolManager() as http:
144+
# Check when redirect is not passed.
145+
with pytest.raises(MaxRetryError):
146+
http.request("GET", f"{self.base_url}/redirect", retries=0)
147+
response = http.request("GET", f"{self.base_url}/redirect", retries=False)
148+
assert response.status == 303
149+
150+
# Check when redirect=True.
151+
with pytest.raises(MaxRetryError):
152+
http.request(
153+
"GET", f"{self.base_url}/redirect", retries=0, redirect=True
154+
)
155+
response = http.request(
156+
"GET", f"{self.base_url}/redirect", retries=False, redirect=True
157+
)
158+
assert response.status == 303
159+
160+
# Check when redirect=False.
161+
response = http.request(
162+
"GET", f"{self.base_url}/redirect", retries=0, redirect=False
163+
)
164+
assert response.status == 303
165+
response = http.request(
166+
"GET", f"{self.base_url}/redirect", retries=False, redirect=False
167+
)
168+
assert response.status == 303
169+
87170
def test_cross_host_redirect(self) -> None:
88171
with PoolManager() as http:
89172
cross_host_location = f"{self.base_url_alt}/echo?a=b"
@@ -138,6 +221,24 @@ def test_too_many_redirects(self) -> None:
138221
pool = http.connection_from_host(self.host, self.port)
139222
assert pool.num_connections == 1
140223

224+
# Check when retries are configured for the pool manager.
225+
with PoolManager(retries=1) as http:
226+
with pytest.raises(MaxRetryError):
227+
http.request(
228+
"GET",
229+
f"{self.base_url}/redirect",
230+
fields={"target": f"/redirect?target={self.base_url}/"},
231+
)
232+
233+
# Here we allow more retries for the request.
234+
response = http.request(
235+
"GET",
236+
f"{self.base_url}/redirect",
237+
fields={"target": f"/redirect?target={self.base_url}/"},
238+
retries=2,
239+
)
240+
assert response.status == 200
241+
141242
def test_redirect_cross_host_remove_headers(self) -> None:
142243
with PoolManager() as http:
143244
r = http.request(

0 commit comments

Comments
 (0)