Skip to content

Commit

Permalink
Merge pull request from GHSA-g4mx-q9vg-27p4
Browse files Browse the repository at this point in the history
  • Loading branch information
illia-v committed Oct 17, 2023
1 parent 944f0eb commit b594c5c
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 2 deletions.
7 changes: 7 additions & 0 deletions dummyserver/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ def redirect(self, request):
status = request.params.get("status", "303 See Other")
if len(status) == 3:
status = "%s Redirect" % status.decode("latin-1")
elif isinstance(status, bytes):
status = status.decode("latin-1")

headers = [("Location", target)]
return Response(status=status, headers=headers)
Expand Down Expand Up @@ -264,6 +266,11 @@ def encodingrequest(self, request):
def headers(self, request):
return Response(json.dumps(dict(request.headers)))

def headers_and_params(self, request):
return Response(
json.dumps({"headers": dict(request.headers), "params": request.params})
)

def successful_retry(self, request):
"""Handler which will return an error and then success
Expand Down
18 changes: 18 additions & 0 deletions src/urllib3/_collections.py
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,24 @@ def getlist(self, key, default=__marker):
else:
return vals[1:]

def _prepare_for_method_change(self):
"""
Remove content-specific header fields before changing the request
method to GET or HEAD according to RFC 9110, Section 15.4.
"""
content_specific_headers = [
"Content-Encoding",
"Content-Language",
"Content-Location",
"Content-Type",
"Content-Length",
"Digest",
"Last-Modified",
]
for header in content_specific_headers:
self.discard(header)
return self

# Backwards compatibility for httplib
getheaders = getlist
getallmatchingheaders = getlist
Expand Down
5 changes: 5 additions & 0 deletions src/urllib3/connectionpool.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from socket import error as SocketError
from socket import timeout as SocketTimeout

from ._collections import HTTPHeaderDict
from .connection import (
BaseSSLError,
BrokenPipeError,
Expand Down Expand Up @@ -843,7 +844,11 @@ def _is_ssl_error_message_from_http_proxy(ssl_error):
redirect_location = redirect and response.get_redirect_location()
if redirect_location:
if response.status == 303:
# Change the method according to RFC 9110, Section 15.4.4.
method = "GET"
# And lose the body not to transfer anything sensitive.
body = None
headers = HTTPHeaderDict(headers)._prepare_for_method_change()

try:
retries = retries.increment(method, url, response=response, _pool=self)
Expand Down
7 changes: 5 additions & 2 deletions src/urllib3/poolmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import functools
import logging

from ._collections import RecentlyUsedContainer
from ._collections import HTTPHeaderDict, RecentlyUsedContainer
from .connectionpool import HTTPConnectionPool, HTTPSConnectionPool, port_by_scheme
from .exceptions import (
LocationValueError,
Expand Down Expand Up @@ -382,9 +382,12 @@ def urlopen(self, method, url, redirect=True, **kw):
# Support relative URLs for redirecting.
redirect_location = urljoin(url, redirect_location)

# RFC 7231, Section 6.4.4
if response.status == 303:
# Change the method according to RFC 9110, Section 15.4.4.
method = "GET"
# And lose the body not to transfer anything sensitive.
kw["body"] = None
kw["headers"] = HTTPHeaderDict(kw["headers"])._prepare_for_method_change()

retries = kw.get("retries")
if not isinstance(retries, Retry):
Expand Down
11 changes: 11 additions & 0 deletions test/with_dummyserver/test_connectionpool.py
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,17 @@ def test_redirect(self):
assert r.status == 200
assert r.data == b"Dummy server!"

def test_303_redirect_makes_request_lose_body(self):
with HTTPConnectionPool(self.host, self.port) as pool:
response = pool.request(
"POST",
"/redirect",
fields={"target": "/headers_and_params", "status": "303 See Other"},
)
data = json.loads(response.data)
assert data["params"] == {}
assert "Content-Type" not in HTTPHeaderDict(data["headers"])

def test_bad_connect(self):
with HTTPConnectionPool("badhost.invalid", self.port) as pool:
with pytest.raises(MaxRetryError) as e:
Expand Down
15 changes: 15 additions & 0 deletions test/with_dummyserver/test_poolmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from dummyserver.server import HAS_IPV6
from dummyserver.testcase import HTTPDummyServerTestCase, IPv6HTTPDummyServerTestCase
from urllib3._collections import HTTPHeaderDict
from urllib3.connectionpool import port_by_scheme
from urllib3.exceptions import MaxRetryError, URLSchemeUnknown
from urllib3.poolmanager import PoolManager
Expand Down Expand Up @@ -236,6 +237,20 @@ def test_redirect_without_preload_releases_connection(self):
assert r._pool.num_connections == 1
assert len(http.pools) == 1

def test_303_redirect_makes_request_lose_body(self):
with PoolManager() as http:
response = http.request(
"POST",
"%s/redirect" % self.base_url,
fields={
"target": "%s/headers_and_params" % self.base_url,
"status": "303 See Other",
},
)
data = json.loads(response.data)
assert data["params"] == {}
assert "Content-Type" not in HTTPHeaderDict(data["headers"])

def test_unknown_scheme(self):
with PoolManager() as http:
unknown_scheme = "unknown"
Expand Down

0 comments on commit b594c5c

Please sign in to comment.