From 52751721766cc83230341be0330a573060fac0fd Mon Sep 17 00:00:00 2001 From: David Lord Date: Wed, 16 Mar 2022 16:27:36 -0700 Subject: [PATCH 1/4] use 308 for append_slash_redirect --- CHANGES.rst | 3 +++ src/werkzeug/utils.py | 6 +++++- tests/test_utils.py | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index ada72a7b0..ce7b69f85 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -90,6 +90,9 @@ Unreleased and ``CONTENT_TYPE``. :pr:`2348` - ``append_slash_redirect`` handles ``PATH_INFO`` with internal slashes. :issue:`1972`, :pr:`2338` +- The default status code for ``append_slash_redirect`` is 308 instead + of 301. This preserves the request body, and matches a previous + change to ``strict_slashes`` in routing. :issue:`2351` Version 2.0.3 diff --git a/src/werkzeug/utils.py b/src/werkzeug/utils.py index 3293ad04d..b92f0810e 100644 --- a/src/werkzeug/utils.py +++ b/src/werkzeug/utils.py @@ -287,7 +287,7 @@ def redirect( return response -def append_slash_redirect(environ: "WSGIEnvironment", code: int = 301) -> "Response": +def append_slash_redirect(environ: "WSGIEnvironment", code: int = 308) -> "Response": """Redirect to the current URL with a slash appended. If the current URL is ``/user/42``, the redirect URL will be @@ -304,6 +304,10 @@ def append_slash_redirect(environ: "WSGIEnvironment", code: int = 301) -> "Respo .. versionchanged:: 2.1 Produce a relative URL that only modifies the last segment. Relevant when the current path has multiple segments. + + .. versionchanged:: 2.1 + The default status code is 308 instead of 301. This preserves + the request method and body. """ tail = environ["PATH_INFO"].rpartition("/")[2] diff --git a/tests/test_utils.py b/tests/test_utils.py index 3f70f1a9b..fa370dfe0 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -256,7 +256,7 @@ def app(env, sr): client = Client(app) response = client.get(path, base_url=base_url) - assert response.status_code == 301 + assert response.status_code == 308 assert response.headers["Location"] == expected_location From 00401f7ea51ea57c80d94f26862bdf7845889f4e Mon Sep 17 00:00:00 2001 From: David Lord Date: Thu, 17 Mar 2022 06:55:07 -0700 Subject: [PATCH 2/4] close stream only after final redirect --- CHANGES.rst | 2 ++ src/werkzeug/test.py | 11 ++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index ce7b69f85..2bae7f00b 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -93,6 +93,8 @@ Unreleased - The default status code for ``append_slash_redirect`` is 308 instead of 301. This preserves the request body, and matches a previous change to ``strict_slashes`` in routing. :issue:`2351` +- Fix ``ValueError: I/O operation on closed file.`` with the test + client when following more than one redirect. :issue:`2353` Version 2.0.3 diff --git a/src/werkzeug/test.py b/src/werkzeug/test.py index aae2e52c5..b0a22b7ab 100644 --- a/src/werkzeug/test.py +++ b/src/werkzeug/test.py @@ -1091,7 +1091,10 @@ def open( redirects = set() history: t.List["TestResponse"] = [] - while follow_redirects and response.status_code in { + if not follow_redirects: + return response + + while response.status_code in { 301, 302, 303, @@ -1118,14 +1121,12 @@ def open( history.append(response) response = self.resolve_redirect(response, buffered=buffered) else: - # This is the final request after redirects, or not - # following redirects. + # This is the final request after redirects. response.history = tuple(history) # Close the input stream when closing the response, in case # the input is an open temporary file. response.call_on_close(request.input_stream.close) - - return response + return response def get(self, *args: t.Any, **kw: t.Any) -> "TestResponse": """Call :meth:`open` with ``method`` set to ``GET``.""" From b7616b2e4fd4674cd2964508e7ceb311382db503 Mon Sep 17 00:00:00 2001 From: David Lord Date: Thu, 17 Mar 2022 07:44:30 -0700 Subject: [PATCH 3/4] disable autocorrect_location_header --- CHANGES.rst | 3 +++ src/werkzeug/wrappers/response.py | 10 +++++++--- tests/middleware/test_proxy_fix.py | 8 ++++++-- tests/test_test.py | 4 ++-- tests/test_utils.py | 19 ++++++++++++++----- 5 files changed, 32 insertions(+), 12 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 2bae7f00b..95c9fa9cd 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -95,6 +95,9 @@ Unreleased change to ``strict_slashes`` in routing. :issue:`2351` - Fix ``ValueError: I/O operation on closed file.`` with the test client when following more than one redirect. :issue:`2353` +- ``Response.autocorrect_location_header`` is disabled by default. + The ``Location`` header URL will remain relative, and exclude the + scheme and domain, by default. :issue:`2352` Version 2.0.3 diff --git a/src/werkzeug/wrappers/response.py b/src/werkzeug/wrappers/response.py index dcb3d62b1..d648062e7 100644 --- a/src/werkzeug/wrappers/response.py +++ b/src/werkzeug/wrappers/response.py @@ -142,11 +142,15 @@ def application(environ, start_response): #: your code to the name change. implicit_sequence_conversion = True - #: Should this response object correct the location header to be RFC - #: conformant? This is true by default. + #: If a redirect ``Location`` header is a relative URL, make it an + #: absolute URL, including scheme and domain. + #: + #: .. versionchanged:: 2.1 + #: This is disabled by default, so responses will send relative + #: redirects. #: #: .. versionadded:: 0.8 - autocorrect_location_header = True + autocorrect_location_header = False #: Should this response object automatically set the content-length #: header if possible? This is true by default. diff --git a/tests/middleware/test_proxy_fix.py b/tests/middleware/test_proxy_fix.py index abbc05c33..6dc318141 100644 --- a/tests/middleware/test_proxy_fix.py +++ b/tests/middleware/test_proxy_fix.py @@ -7,6 +7,7 @@ from werkzeug.test import create_environ from werkzeug.utils import redirect from werkzeug.wrappers import Request +from werkzeug.wrappers import Response @pytest.mark.parametrize( @@ -158,7 +159,9 @@ ), ), ) -def test_proxy_fix(kwargs, base, url_root): +def test_proxy_fix(monkeypatch, kwargs, base, url_root): + monkeypatch.setattr(Response, "autocorrect_location_header", True) + @Request.application def app(request): # for header @@ -173,7 +176,8 @@ def app(request): # match doesn't include prefix assert urls.match("/parrot")[0] == "parrot" - # location header will start with url_root + # With autocorrect_location_header enabled, location header will + # start with url_root return redirect(parrot_url) url_map = Map([Rule("/parrot", endpoint="parrot")]) diff --git a/tests/test_test.py b/tests/test_test.py index b776eed14..7cb43e1fa 100644 --- a/tests/test_test.py +++ b/tests/test_test.py @@ -556,13 +556,13 @@ def app(request): assert response.history[-1].request.path == "/second" assert response.history[-1].status_code == 302 - assert response.history[-1].location == "http://localhost/third" + assert response.history[-1].location == "/third" assert len(response.history[-1].history) == 1 assert response.history[-1].history[-1] is response.history[-2] assert response.history[-2].request.path == "/first" assert response.history[-2].status_code == 302 - assert response.history[-2].location == "http://localhost/second" + assert response.history[-2].location == "/second" assert len(response.history[-2].history) == 0 diff --git a/tests/test_utils.py b/tests/test_utils.py index fa370dfe0..ce93989eb 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -3,6 +3,7 @@ import pytest +from werkzeug import Request from werkzeug import utils from werkzeug.datastructures import Headers from werkzeug.http import http_date @@ -237,7 +238,7 @@ def test_header_set_duplication_bug(): @pytest.mark.parametrize( - "path, base_url, expected_location", + ("path", "base_url", "absolute_location"), [ ("foo", "http://example.org/app", "http://example.org/app/foo/"), ("/foo", "http://example.org/app", "http://example.org/app/foo/"), @@ -250,14 +251,22 @@ def test_header_set_duplication_bug(): ("/", "http://example.org/app", "http://example.org/app/"), ], ) -def test_append_slash_redirect(path, base_url, expected_location): - def app(env, sr): - return utils.append_slash_redirect(env)(env, sr) +@pytest.mark.parametrize("autocorrect", [False, True]) +def test_append_slash_redirect(autocorrect, path, base_url, absolute_location): + @Request.application + def app(request): + rv = utils.append_slash_redirect(request.environ) + rv.autocorrect_location_header = autocorrect + return rv client = Client(app) response = client.get(path, base_url=base_url) assert response.status_code == 308 - assert response.headers["Location"] == expected_location + + if not autocorrect: + assert response.headers["Location"].count("/") == 1 + else: + assert response.headers["Location"] == absolute_location def test_cached_property_doc(): From dfb5d5cab83bcf39a82401a0c0653461282fa54b Mon Sep 17 00:00:00 2001 From: David Lord Date: Thu, 17 Mar 2022 09:03:09 -0700 Subject: [PATCH 4/4] add back fail-on-warning for tests --- setup.cfg | 2 ++ 1 file changed, 2 insertions(+) diff --git a/setup.cfg b/setup.cfg index bb47d64cd..247831128 100644 --- a/setup.cfg +++ b/setup.cfg @@ -43,6 +43,8 @@ where = src [tool:pytest] testpaths = tests +filterwarnings = + error [coverage:run] branch = True