From c92e241527c0b59cf4c08b3be9f9f2d37baff56b Mon Sep 17 00:00:00 2001 From: Clemens Wolff Date: Mon, 30 Sep 2024 11:37:46 +0200 Subject: [PATCH 1/9] Preserve query string on redirect --- CHANGELOG.md | 1 + docs/src/django-settings.md | 10 ++++++++++ src/servestatic/base.py | 8 +++++++- src/servestatic/middleware.py | 6 ++++++ src/servestatic/responders.py | 10 +++++++++- tests/test_servestatic.py | 14 +++++++++++++- 6 files changed, 46 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index edabdecb..90fdcd2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Using the following categories, list your changes in this order: ### Added - Support Python 3.13. +- Django `settings.py:SERVESTATIC_PRESERVE_QUERY_STRING_ON_REDIRECT` will forward the request query string when issuing redirects. ## [2.0.1] - 2024-09-13 diff --git a/docs/src/django-settings.md b/docs/src/django-settings.md index 054f7d4b..e9e67ecf 100644 --- a/docs/src/django-settings.md +++ b/docs/src/django-settings.md @@ -204,6 +204,16 @@ This setting is only effective if the `ServeStatic` storage backend is being use --- +## `SERVESTATIC_PRESERVE_QUERY_STRING_ON_REDIRECT` + +**Default:** `False` + +Set to `True` to preserve the request query string when issuing redirects. + +By default, ServeStatic will strip the query string when issuing a redirect. If this setting is enabled, the query string will instead be forwarded to the redirect location. + +--- + ## `SERVESTATIC_MANIFEST_STRICT` **Default:** `True` diff --git a/src/servestatic/base.py b/src/servestatic/base.py index 2c16e665..f48be2c4 100644 --- a/src/servestatic/base.py +++ b/src/servestatic/base.py @@ -48,6 +48,7 @@ def __init__( add_headers_function: Callable[[Headers, str, str], None] | None = None, index_file: str | bool | None = None, immutable_file_test: Callable | str | None = None, + preserve_query_string_on_redirect: bool | None = None, ): self.autorefresh = autorefresh self.max_age = max_age @@ -60,6 +61,7 @@ def __init__( self.application = application self.files = {} self.directories = [] + self.preserve_query_string_on_redirect = preserve_query_string_on_redirect if index_file is True: self.index_file: str | None = "index.html" @@ -236,4 +238,8 @@ def redirect(self, from_url, to_url): msg = f"Cannot handle redirect: {from_url} > {to_url}" raise ValueError(msg) headers = {"Cache-Control": f"max-age={self.max_age}, public"} if self.max_age is not None else {} - return Redirect(relative_url, headers=headers) + return Redirect( + relative_url, + headers=headers, + preserve_query_string=self.preserve_query_string_on_redirect, + ) diff --git a/src/servestatic/middleware.py b/src/servestatic/middleware.py index b513d630..f5fbcfdb 100644 --- a/src/servestatic/middleware.py +++ b/src/servestatic/middleware.py @@ -73,6 +73,11 @@ def __init__(self, get_response=None, settings=django_settings): force_script_name = getattr(settings, "FORCE_SCRIPT_NAME", None) static_url = getattr(settings, "STATIC_URL", None) root = getattr(settings, "SERVESTATIC_ROOT", None) + preserve_query_string_on_redirect = getattr( + django_settings, + "SERVESTATIC_PRESERVE_QUERY_STRING_ON_REDIRECT", + False, + ) super().__init__( application=None, @@ -84,6 +89,7 @@ def __init__(self, get_response=None, settings=django_settings): add_headers_function=add_headers_function, index_file=self.index_file, immutable_file_test=immutable_file_test, + preserve_query_string_on_redirect=preserve_query_string_on_redirect, ) if self.static_prefix is None: diff --git a/src/servestatic/responders.py b/src/servestatic/responders.py index 7b4e8a02..f1b8c803 100644 --- a/src/servestatic/responders.py +++ b/src/servestatic/responders.py @@ -315,12 +315,20 @@ def get_path_and_headers(self, request_headers): class Redirect: - def __init__(self, location, headers=None): + def __init__(self, location, headers=None, preserve_query_string=False): headers = list(headers.items()) if headers else [] headers.append(("Location", quote(location.encode("utf8")))) self.response = Response(HTTPStatus.FOUND, headers, None) + self.preserve_query_string = preserve_query_string def get_response(self, method, request_headers): + query_string = request_headers.get("QUERY_STRING") + if query_string and self.preserve_query_string: + headers = list(self.response.headers) + name, value = headers[-1] + value = "{}?{}".format(value, query_string) + headers[-1] = (name, value) + return Response(self.response.status, headers, None) return self.response async def aget_response(self, method, request_headers): diff --git a/tests/test_servestatic.py b/tests/test_servestatic.py index dd9c3119..992dbb3f 100644 --- a/tests/test_servestatic.py +++ b/tests/test_servestatic.py @@ -16,7 +16,7 @@ import pytest from servestatic import ServeStatic -from servestatic.responders import StaticFile +from servestatic.responders import Redirect, StaticFile from .utils import AppServer, Files @@ -376,3 +376,15 @@ def test_chunked_file_size_matches_range_with_range_header(): while response.file.read(1): file_size += 1 assert file_size == 14 + + +def test_redirect_strips_query_string_by_default(): + responder = Redirect("/redirect/to/here/") + response = responder.get_response("GET", {"QUERY_STRING": "foo=1&bar=2"}) + assert response.headers[0] == ("Location", "/redirect/to/here/") + + +def test_redirect_preserves_query_string_if_configured(): + responder = Redirect("/redirect/to/here/", preserve_query_string=True) + response = responder.get_response("GET", {"QUERY_STRING": "foo=1&bar=2"}) + assert response.headers[0] == ("Location", "/redirect/to/here/?foo=1&bar=2") From e0ab1437bf54bedf1079f1e23f3cde602df72d53 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 30 Sep 2024 10:02:38 +0000 Subject: [PATCH 2/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/servestatic/responders.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/servestatic/responders.py b/src/servestatic/responders.py index f1b8c803..ff91b5fc 100644 --- a/src/servestatic/responders.py +++ b/src/servestatic/responders.py @@ -326,7 +326,7 @@ def get_response(self, method, request_headers): if query_string and self.preserve_query_string: headers = list(self.response.headers) name, value = headers[-1] - value = "{}?{}".format(value, query_string) + value = f"{value}?{query_string}" headers[-1] = (name, value) return Response(self.response.status, headers, None) return self.response From fdfbf1da1698603a84679a37887ebfb98563406c Mon Sep 17 00:00:00 2001 From: Clemens Wolff Date: Wed, 2 Oct 2024 09:49:16 +0200 Subject: [PATCH 3/9] Remove feature flag --- CHANGELOG.md | 2 +- docs/src/django-settings.md | 10 ---------- src/servestatic/base.py | 3 --- src/servestatic/middleware.py | 6 ------ src/servestatic/responders.py | 5 ++--- tests/test_servestatic.py | 8 +------- 6 files changed, 4 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90fdcd2e..04a553c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ Using the following categories, list your changes in this order: ### Added - Support Python 3.13. -- Django `settings.py:SERVESTATIC_PRESERVE_QUERY_STRING_ON_REDIRECT` will forward the request query string when issuing redirects. +- Query strings are now preserved and forwarded to the location when redirecting. ## [2.0.1] - 2024-09-13 diff --git a/docs/src/django-settings.md b/docs/src/django-settings.md index e9e67ecf..054f7d4b 100644 --- a/docs/src/django-settings.md +++ b/docs/src/django-settings.md @@ -204,16 +204,6 @@ This setting is only effective if the `ServeStatic` storage backend is being use --- -## `SERVESTATIC_PRESERVE_QUERY_STRING_ON_REDIRECT` - -**Default:** `False` - -Set to `True` to preserve the request query string when issuing redirects. - -By default, ServeStatic will strip the query string when issuing a redirect. If this setting is enabled, the query string will instead be forwarded to the redirect location. - ---- - ## `SERVESTATIC_MANIFEST_STRICT` **Default:** `True` diff --git a/src/servestatic/base.py b/src/servestatic/base.py index f48be2c4..8dc4a4c5 100644 --- a/src/servestatic/base.py +++ b/src/servestatic/base.py @@ -48,7 +48,6 @@ def __init__( add_headers_function: Callable[[Headers, str, str], None] | None = None, index_file: str | bool | None = None, immutable_file_test: Callable | str | None = None, - preserve_query_string_on_redirect: bool | None = None, ): self.autorefresh = autorefresh self.max_age = max_age @@ -61,7 +60,6 @@ def __init__( self.application = application self.files = {} self.directories = [] - self.preserve_query_string_on_redirect = preserve_query_string_on_redirect if index_file is True: self.index_file: str | None = "index.html" @@ -241,5 +239,4 @@ def redirect(self, from_url, to_url): return Redirect( relative_url, headers=headers, - preserve_query_string=self.preserve_query_string_on_redirect, ) diff --git a/src/servestatic/middleware.py b/src/servestatic/middleware.py index f5fbcfdb..b513d630 100644 --- a/src/servestatic/middleware.py +++ b/src/servestatic/middleware.py @@ -73,11 +73,6 @@ def __init__(self, get_response=None, settings=django_settings): force_script_name = getattr(settings, "FORCE_SCRIPT_NAME", None) static_url = getattr(settings, "STATIC_URL", None) root = getattr(settings, "SERVESTATIC_ROOT", None) - preserve_query_string_on_redirect = getattr( - django_settings, - "SERVESTATIC_PRESERVE_QUERY_STRING_ON_REDIRECT", - False, - ) super().__init__( application=None, @@ -89,7 +84,6 @@ def __init__(self, get_response=None, settings=django_settings): add_headers_function=add_headers_function, index_file=self.index_file, immutable_file_test=immutable_file_test, - preserve_query_string_on_redirect=preserve_query_string_on_redirect, ) if self.static_prefix is None: diff --git a/src/servestatic/responders.py b/src/servestatic/responders.py index ff91b5fc..9ae992d5 100644 --- a/src/servestatic/responders.py +++ b/src/servestatic/responders.py @@ -315,15 +315,14 @@ def get_path_and_headers(self, request_headers): class Redirect: - def __init__(self, location, headers=None, preserve_query_string=False): + def __init__(self, location, headers=None): headers = list(headers.items()) if headers else [] headers.append(("Location", quote(location.encode("utf8")))) self.response = Response(HTTPStatus.FOUND, headers, None) - self.preserve_query_string = preserve_query_string def get_response(self, method, request_headers): query_string = request_headers.get("QUERY_STRING") - if query_string and self.preserve_query_string: + if query_string: headers = list(self.response.headers) name, value = headers[-1] value = f"{value}?{query_string}" diff --git a/tests/test_servestatic.py b/tests/test_servestatic.py index 992dbb3f..b8523fa5 100644 --- a/tests/test_servestatic.py +++ b/tests/test_servestatic.py @@ -378,13 +378,7 @@ def test_chunked_file_size_matches_range_with_range_header(): assert file_size == 14 -def test_redirect_strips_query_string_by_default(): +def test_redirect_preserves_query_string(): responder = Redirect("/redirect/to/here/") response = responder.get_response("GET", {"QUERY_STRING": "foo=1&bar=2"}) - assert response.headers[0] == ("Location", "/redirect/to/here/") - - -def test_redirect_preserves_query_string_if_configured(): - responder = Redirect("/redirect/to/here/", preserve_query_string=True) - response = responder.get_response("GET", {"QUERY_STRING": "foo=1&bar=2"}) assert response.headers[0] == ("Location", "/redirect/to/here/?foo=1&bar=2") From 0db086d161bfc9d59fe4fc86ef2a7965147395a7 Mon Sep 17 00:00:00 2001 From: Clemens Wolff Date: Wed, 2 Oct 2024 09:52:58 +0200 Subject: [PATCH 4/9] Remove assumption that location is the last header --- src/servestatic/responders.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/servestatic/responders.py b/src/servestatic/responders.py index 9ae992d5..eee5bda4 100644 --- a/src/servestatic/responders.py +++ b/src/servestatic/responders.py @@ -315,18 +315,20 @@ def get_path_and_headers(self, request_headers): class Redirect: + location = "Location" + def __init__(self, location, headers=None): headers = list(headers.items()) if headers else [] - headers.append(("Location", quote(location.encode("utf8")))) + headers.append((self.location, quote(location.encode("utf8")))) self.response = Response(HTTPStatus.FOUND, headers, None) def get_response(self, method, request_headers): query_string = request_headers.get("QUERY_STRING") if query_string: headers = list(self.response.headers) - name, value = headers[-1] + i, value = next((i, value) for (i, (name, value)) in enumerate(headers) if name == self.location) value = f"{value}?{query_string}" - headers[-1] = (name, value) + headers[i] = (self.location, value) return Response(self.response.status, headers, None) return self.response From fc0db6d1ea1ffec280277091ec4c67c400ca6d71 Mon Sep 17 00:00:00 2001 From: Clemens Wolff Date: Wed, 2 Oct 2024 10:08:53 +0200 Subject: [PATCH 5/9] Add more tests --- src/servestatic/asgi.py | 1 + src/servestatic/responders.py | 2 +- tests/test_asgi.py | 16 +++++++++++++++- tests/test_servestatic.py | 9 +++++++++ 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/servestatic/asgi.py b/src/servestatic/asgi.py index 91e88b37..b2c2d944 100644 --- a/src/servestatic/asgi.py +++ b/src/servestatic/asgi.py @@ -47,6 +47,7 @@ async def __call__(self, scope, receive, send): wsgi_headers = { "HTTP_" + key.decode().upper().replace("-", "_"): value.decode() for key, value in scope["headers"] } + wsgi_headers["QUERY_STRING"] = scope["query_string"].decode() # Get the ServeStatic file response response = await self.static_file.aget_response(scope["method"], wsgi_headers) diff --git a/src/servestatic/responders.py b/src/servestatic/responders.py index eee5bda4..e83d191e 100644 --- a/src/servestatic/responders.py +++ b/src/servestatic/responders.py @@ -333,7 +333,7 @@ def get_response(self, method, request_headers): return self.response async def aget_response(self, method, request_headers): - return self.response + return self.get_response(method, request_headers) class NotARegularFileError(Exception): diff --git a/tests/test_asgi.py b/tests/test_asgi.py index 6489e407..67b0362d 100644 --- a/tests/test_asgi.py +++ b/tests/test_asgi.py @@ -15,6 +15,7 @@ def test_files(): return Files( js=str(Path("static") / "app.js"), + index=str(Path("static") / "with-index" / "index.html"), ) @@ -34,7 +35,12 @@ async def asgi_app(scope, receive, send): }) await send({"type": "http.response.body", "body": b"Not Found"}) - return ServeStaticASGI(asgi_app, root=test_files.directory, autorefresh=request.param) + return ServeStaticASGI( + asgi_app, + root=test_files.directory, + autorefresh=request.param, + index_file=True, + ) def test_get_js_static_file(application, test_files): @@ -47,6 +53,14 @@ def test_get_js_static_file(application, test_files): assert send.headers[b"content-length"] == str(len(test_files.js_content)).encode() +def test_redirect_preserves_query_string(application, test_files): + scope = AsgiScopeEmulator({"path": "/static/with-index", "query_string": b"v=1"}) + receive = AsgiReceiveEmulator() + send = AsgiSendEmulator() + asyncio.run(application(scope, receive, send)) + assert send.headers[b"location"] == b"with-index/?v=1" + + def test_user_app(application): scope = AsgiScopeEmulator({"path": "/"}) receive = AsgiReceiveEmulator() diff --git a/tests/test_servestatic.py b/tests/test_servestatic.py index b8523fa5..84002b77 100644 --- a/tests/test_servestatic.py +++ b/tests/test_servestatic.py @@ -245,6 +245,15 @@ def test_index_file_path_redirected(server, files): assert location == directory_url +def test_index_file_path_redirected_with_query_string(server, files): + directory_url = files.index_url.rpartition("/")[0] + "/" + query_string = "v=1" + response = server.get(f"{files.index_url}?{query_string}", allow_redirects=False) + location = urljoin(files.index_url, response.headers["Location"]) + assert response.status_code == 302 + assert location == f"{directory_url}?{query_string}" + + def test_directory_path_without_trailing_slash_redirected(server, files): directory_url = files.index_url.rpartition("/")[0] + "/" no_slash_url = directory_url.rstrip("/") From 01da2a9e1ec87b52d376d45de57967d054bcd9bd Mon Sep 17 00:00:00 2001 From: Clemens Wolff Date: Wed, 2 Oct 2024 11:11:20 +0200 Subject: [PATCH 6/9] Update CHANGELOG.md Co-authored-by: Mark Bakhit --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04a553c7..6dcf6f92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ Using the following categories, list your changes in this order: ### Added - Support Python 3.13. -- Query strings are now preserved and forwarded to the location when redirecting. +- Query strings are now preserved during HTTP redirections. ## [2.0.1] - 2024-09-13 From bf644905bb85a8e5c679ec6c58cc29e4c68bd36a Mon Sep 17 00:00:00 2001 From: Clemens Wolff Date: Wed, 2 Oct 2024 11:11:33 +0200 Subject: [PATCH 7/9] Update src/servestatic/base.py Co-authored-by: Mark Bakhit --- src/servestatic/base.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/servestatic/base.py b/src/servestatic/base.py index 8dc4a4c5..2c16e665 100644 --- a/src/servestatic/base.py +++ b/src/servestatic/base.py @@ -236,7 +236,4 @@ def redirect(self, from_url, to_url): msg = f"Cannot handle redirect: {from_url} > {to_url}" raise ValueError(msg) headers = {"Cache-Control": f"max-age={self.max_age}, public"} if self.max_age is not None else {} - return Redirect( - relative_url, - headers=headers, - ) + return Redirect(relative_url, headers=headers) From 29ec6fa4f2fb3e0be9b1c54324ff482bc670159f Mon Sep 17 00:00:00 2001 From: Clemens Wolff Date: Wed, 2 Oct 2024 11:11:48 +0200 Subject: [PATCH 8/9] Update tests/test_asgi.py Co-authored-by: Mark Bakhit --- tests/test_asgi.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_asgi.py b/tests/test_asgi.py index 67b0362d..94ee8d6d 100644 --- a/tests/test_asgi.py +++ b/tests/test_asgi.py @@ -54,11 +54,11 @@ def test_get_js_static_file(application, test_files): def test_redirect_preserves_query_string(application, test_files): - scope = AsgiScopeEmulator({"path": "/static/with-index", "query_string": b"v=1"}) + scope = AsgiScopeEmulator({"path": "/static/with-index", "query_string": b"v=1&x=2"}) receive = AsgiReceiveEmulator() send = AsgiSendEmulator() asyncio.run(application(scope, receive, send)) - assert send.headers[b"location"] == b"with-index/?v=1" + assert send.headers[b"location"] == b"with-index/?v=1&x=2" def test_user_app(application): From d465832d0da25bbd36606657d63e14ba85a4f942 Mon Sep 17 00:00:00 2001 From: Mark Bakhit Date: Wed, 2 Oct 2024 02:17:40 -0700 Subject: [PATCH 9/9] Apply suggestions from code review --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dcf6f92..87c9b329 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ Using the following categories, list your changes in this order: ### Added - Support Python 3.13. -- Query strings are now preserved during HTTP redirections. +- Query strings are now preserved during HTTP redirection. ## [2.0.1] - 2024-09-13