From 33ef5450f988963096ee618e41342184af3e8e44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Tue, 20 Feb 2024 13:36:59 +0100 Subject: [PATCH 1/3] Fix RedirectMiddleware --- scrapy/downloadermiddlewares/redirect.py | 2 + tests/test_downloadermiddleware_redirect.py | 43 +++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index 3176ed930..d26c1b12b 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -110,6 +110,8 @@ class RedirectMiddleware(BaseRedirectMiddleware): location = request_scheme + "://" + location.lstrip("/") redirected_url = urljoin(request.url, location) + if urlparse(redirected_url).scheme not in {"http", "https"}: + return response if response.status in (301, 307, 308) or request.method == "HEAD": redirected = _build_redirect_request(request, url=redirected_url) diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index 10b8ca9af..60bff8fac 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -1,4 +1,7 @@ import unittest +from itertools import product + +import pytest from scrapy.downloadermiddlewares.redirect import ( MetaRefreshMiddleware, @@ -279,6 +282,46 @@ class RedirectMiddlewareTest(unittest.TestCase): ) +@pytest.mark.parametrize( + ("url", "location", "target"), + ( + # http/https → http/https redirects + *( + ( + f"{input_scheme}://example.com/a", + f"{output_scheme}://example.com/b", + f"{output_scheme}://example.com/b", + ) + for input_scheme, output_scheme in product(("http", "https"), repeat=2) + ), + # http/https → data/file/ftp/s3/foo does not redirect + *( + ( + f"{input_scheme}://example.com/a", + f"{output_scheme}://example.com/b", + None, + ) + for input_scheme in ("http", "https") + for output_scheme in ("data", "file", "ftp", "s3", "foo") + ), + # Note: We do not test data/file/ftp/s3 schemes for the initial URL + # because their download handlers cannot return a status code of 3xx. + ), +) +def test_redirect_schemes(url, location, target): + crawler = get_crawler(Spider) + spider = crawler._create_spider("foo") + mw = RedirectMiddleware.from_crawler(crawler) + request = Request(url) + response = Response(url, headers={"Location": location}, status=301) + redirect = mw.process_response(request, response, spider) + if target is None: + assert redirect == response + else: + assert isinstance(redirect, Request) + assert redirect.url == target + + class MetaRefreshMiddlewareTest(unittest.TestCase): def setUp(self): crawler = get_crawler(Spider) From 685cf5940f707e3ea03c92cf35b504b74fe9ff59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Tue, 20 Feb 2024 13:57:06 +0100 Subject: [PATCH 2/3] Fix MetaRefreshMiddleware --- scrapy/downloadermiddlewares/redirect.py | 8 +- tests/test_downloadermiddleware_redirect.py | 111 +++++++++++++++----- 2 files changed, 90 insertions(+), 29 deletions(-) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index d26c1b12b..d95aa44c0 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -134,12 +134,16 @@ class MetaRefreshMiddleware(BaseRedirectMiddleware): request.meta.get("dont_redirect", False) or request.method == "HEAD" or not isinstance(response, HtmlResponse) + or urlparse_cached(request).scheme not in {"http", "https"} ): return response interval, url = get_meta_refresh(response, ignore_tags=self._ignore_tags) - if url and interval < self._maxdelay: + if not url: + return response + if urlparse(url).scheme not in {"http", "https"}: + return response + if interval < self._maxdelay: redirected = self._redirect_request_using_get(request, url) return self._redirect(redirected, request, spider, "meta refresh") - return response diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index 60bff8fac..01f23c7e5 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -1,5 +1,5 @@ import unittest -from itertools import product +from itertools import chain, product import pytest @@ -282,32 +282,45 @@ class RedirectMiddlewareTest(unittest.TestCase): ) -@pytest.mark.parametrize( - ("url", "location", "target"), - ( - # http/https → http/https redirects - *( - ( - f"{input_scheme}://example.com/a", - f"{output_scheme}://example.com/b", - f"{output_scheme}://example.com/b", - ) - for input_scheme, output_scheme in product(("http", "https"), repeat=2) - ), - # http/https → data/file/ftp/s3/foo does not redirect - *( - ( - f"{input_scheme}://example.com/a", - f"{output_scheme}://example.com/b", - None, - ) - for input_scheme in ("http", "https") - for output_scheme in ("data", "file", "ftp", "s3", "foo") - ), - # Note: We do not test data/file/ftp/s3 schemes for the initial URL - # because their download handlers cannot return a status code of 3xx. +SCHEME_PARAMS = ("url", "location", "target") +HTTP_SCHEMES = ("http", "https") +NON_HTTP_SCHEMES = ("data", "file", "ftp", "s3", "foo") +REDIRECT_SCHEME_CASES = ( + # http/https → http/https redirects + *( + ( + f"{input_scheme}://example.com/a", + f"{output_scheme}://example.com/b", + f"{output_scheme}://example.com/b", + ) + for input_scheme, output_scheme in product(HTTP_SCHEMES, repeat=2) ), + # http/https → data/file/ftp/s3/foo does not redirect + *( + ( + f"{input_scheme}://example.com/a", + f"{output_scheme}://example.com/b", + None, + ) + for input_scheme in HTTP_SCHEMES + for output_scheme in NON_HTTP_SCHEMES + ), + # http/https → relative redirects + *( + ( + f"{scheme}://example.com/a", + location, + f"{scheme}://example.com/b", + ) + for scheme in HTTP_SCHEMES + for location in ("//example.com/b", "/b") + ), + # Note: We do not test data/file/ftp/s3 schemes for the initial URL + # because their download handlers cannot return a status code of 3xx. ) + + +@pytest.mark.parametrize(SCHEME_PARAMS, REDIRECT_SCHEME_CASES) def test_redirect_schemes(url, location, target): crawler = get_crawler(Spider) spider = crawler._create_spider("foo") @@ -322,6 +335,11 @@ def test_redirect_schemes(url, location, target): assert redirect.url == target +def meta_refresh_body(url, interval=5): + html = f"""""" + return html.encode("utf-8") + + class MetaRefreshMiddlewareTest(unittest.TestCase): def setUp(self): crawler = get_crawler(Spider) @@ -329,8 +347,7 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): self.mw = MetaRefreshMiddleware.from_crawler(crawler) def _body(self, interval=5, url="http://example.org/newpage"): - html = f"""""" - return html.encode("utf-8") + return meta_refresh_body(url, interval) def test_priority_adjust(self): req = Request("http://a.com") @@ -457,5 +474,45 @@ class MetaRefreshMiddlewareTest(unittest.TestCase): assert isinstance(response, Response) +@pytest.mark.parametrize( + SCHEME_PARAMS, + ( + *REDIRECT_SCHEME_CASES, + # data/file/ftp/s3/foo → * does not redirect + *( + ( + f"{input_scheme}://example.com/a", + f"{output_scheme}://example.com/b", + None, + ) + for input_scheme in NON_HTTP_SCHEMES + for output_scheme in chain(HTTP_SCHEMES, NON_HTTP_SCHEMES) + ), + # data/file/ftp/s3/foo → relative does not redirect + *( + ( + f"{scheme}://example.com/a", + location, + None, + ) + for scheme in NON_HTTP_SCHEMES + for location in ("//example.com/b", "/b") + ), + ), +) +def test_meta_refresh_schemes(url, location, target): + crawler = get_crawler(Spider) + spider = crawler._create_spider("foo") + mw = MetaRefreshMiddleware.from_crawler(crawler) + request = Request(url) + response = HtmlResponse(url, body=meta_refresh_body(location)) + redirect = mw.process_response(request, response, spider) + if target is None: + assert redirect == response + else: + assert isinstance(redirect, Request) + assert redirect.url == target + + if __name__ == "__main__": unittest.main() From c04bba9e0a6132f3d877ec087762d88c9523d41f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Tue, 20 Feb 2024 15:31:10 +0100 Subject: [PATCH 3/3] Force werkzeug < 3 when using mitmproxy --- tox.ini | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tox.ini b/tox.ini index 381da9773..850068d1b 100644 --- a/tox.ini +++ b/tox.ini @@ -16,6 +16,9 @@ deps = #mitmproxy >= 5.3.0; python_version >= '3.9' and implementation_name != 'pypy' # The tests hang with mitmproxy 8.0.0: https://github.com/scrapy/scrapy/issues/5454 mitmproxy >= 4.0.4, < 8; python_version < '3.9' and implementation_name != 'pypy' + # https://github.com/pallets/werkzeug/pull/2768 breaks flask, required by + # mitmproxy. + werkzeug < 3; python_version < '3.9' and implementation_name != 'pypy' passenv = S3_TEST_FILE_URI AWS_ACCESS_KEY_ID