diff --git a/docs/news.rst b/docs/news.rst index d8efa9ba0..db6749ae7 100644 --- a/docs/news.rst +++ b/docs/news.rst @@ -8,17 +8,55 @@ Release notes Scrapy 2.14.2 (unreleased) -------------------------- -Security bug fixes -~~~~~~~~~~~~~~~~~~ +Deprecations +~~~~~~~~~~~~ -- Cross-origin 307/308 redirects now remove the request body. See the - 7j88-353p-36rg_ security advisory for details. +- Using a response URL string as the first parameter on calls to + :meth:`scrapy.spidermiddlewares.referer.RefererMiddleware.policy` is + deprecated. Pass a :class:`~scrapy.http.Response` instead. - .. _7j88-353p-36rg: https://github.com/scrapy/scrapy/security/advisories/GHSA-7j88-353p-36rg + The parameter has also been renamed to ``response`` to reflect this change. + The old parameter name (``resp_or_url``) is deprecated. -- 301 redirects now force the request method to GET, in line with modern - browsers. The original request method is now only maintained on 307 and 308 - redirects. +Bug fixes +~~~~~~~~~ + +- Aligned redirect method conversions to ``GET`` with the `standard + `__: + + - 301 redirects of ``POST`` requests turn into ``GET`` requests. + + - Only ``POST`` 302 redirects turn into ``GET`` requests, other methods + are preserved. + + - ``HEAD`` 303 redirects do not turn into ``GET`` requests. + + - ``GET`` 303 redirects do not get their body or standard ``Content-*`` + headers removed. + + .. note:: Turning into a ``GET`` request implies not only a method change, + but also omitting the body and ``Content-*`` headers in the redirect + request. + +- Redirects where the original request body is dropped now also get their + ``Content-Encoding``, ``Content-Language`` and ``Content-Location`` headers + removed, in addition to ``Content-Type`` and ``Content-Length`` that were + already being removed. + +- Redirects now maintain the source URL fragment if the redirect URL doesn't + have one. This may be useful when using browser-based download handlers, + like those of `scrapy-playwright`_ or `scrapy-zyte-api`_, and letting + Scrapy handle redirects. + + .. _scrapy-playwright: https://github.com/scrapy-plugins/scrapy-playwright + .. _scrapy-zyte-api: https://scrapy-zyte-api.readthedocs.io/en/latest/ + +- The ``Referer`` header is now removed on redirect if + :class:`~scrapy.spidermiddlewares.referer.RefererMiddleware` is disabled. + +- The handling of the ``Referer`` header on redirects now accounts for the + ``Referer-Policy`` header of the response that triggers the redirect + request. .. _release-2.14.1: diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index 0ab2f58bc..9520a5cbb 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -2,14 +2,17 @@ from __future__ import annotations import logging from typing import TYPE_CHECKING, Any, cast -from urllib.parse import urljoin +from urllib.parse import urljoin, urlparse from w3lib.url import safe_url_string +from scrapy import signals from scrapy.exceptions import IgnoreRequest, NotConfigured from scrapy.http import HtmlResponse, Response +from scrapy.spidermiddlewares.referer import RefererMiddleware from scrapy.utils.decorators import _warn_spider_arg from scrapy.utils.httpobj import urlparse_cached +from scrapy.utils.python import global_object_name from scrapy.utils.response import get_meta_refresh if TYPE_CHECKING: @@ -24,67 +27,6 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) -def _build_redirect_request( - source_request: Request, *, url: str, **kwargs: Any -) -> Request: - redirect_request = source_request.replace( - url=url, - **kwargs, - cls=None, - cookies=None, - ) - if "_scheme_proxy" in redirect_request.meta: - source_request_scheme = urlparse_cached(source_request).scheme - redirect_request_scheme = urlparse_cached(redirect_request).scheme - if source_request_scheme != redirect_request_scheme: - redirect_request.meta.pop("_scheme_proxy") - redirect_request.meta.pop("proxy", None) - redirect_request.meta.pop("_auth_proxy", None) - redirect_request.headers.pop(b"Proxy-Authorization", None) - has_cookie_header = "Cookie" in redirect_request.headers - has_authorization_header = "Authorization" in redirect_request.headers - has_request_body = bool(getattr(redirect_request, "body", None)) - if has_cookie_header or has_authorization_header or has_request_body: - default_ports = {"http": 80, "https": 443} - - parsed_source_request = urlparse_cached(source_request) - source_scheme, source_host, source_port = ( - parsed_source_request.scheme, - parsed_source_request.hostname, - parsed_source_request.port - or default_ports.get(parsed_source_request.scheme), - ) - - parsed_redirect_request = urlparse_cached(redirect_request) - redirect_scheme, redirect_host, redirect_port = ( - parsed_redirect_request.scheme, - parsed_redirect_request.hostname, - parsed_redirect_request.port - or default_ports.get(parsed_redirect_request.scheme), - ) - - if has_cookie_header and ( - redirect_scheme not in {source_scheme, "https"} - or source_host != redirect_host - ): - del redirect_request.headers["Cookie"] - - # https://fetch.spec.whatwg.org/#ref-for-cors-non-wildcard-request-header-name - if has_authorization_header and ( - source_scheme != redirect_scheme - or source_host != redirect_host - or source_port != redirect_port - ): - del redirect_request.headers["Authorization"] - - if has_request_body and source_host != redirect_host: - redirect_request = redirect_request.replace(body=b"") - redirect_request.headers.pop("Content-Type", None) - redirect_request.headers.pop("Content-Length", None) - - return redirect_request - - class BaseRedirectMiddleware: crawler: Crawler enabled_setting: str = "REDIRECT_ENABLED" @@ -95,13 +37,59 @@ class BaseRedirectMiddleware: self.max_redirect_times: int = settings.getint("REDIRECT_MAX_TIMES") self.priority_adjust: int = settings.getint("REDIRECT_PRIORITY_ADJUST") + self._referer_spider_middleware: RefererMiddleware | None = None @classmethod def from_crawler(cls, crawler: Crawler) -> Self: o = cls(crawler.settings) o.crawler = crawler + crawler.signals.connect(o._engine_started, signal=signals.engine_started) return o + def handle_referer(self, request: Request, response: Response) -> None: + """Remove, modify or keep the Referer header of *request* based on the + *response* that triggered *request*. + + By default, this method finds a run-time instance of + scrapy.spidermiddlewares.referer.RefererMiddleware (or of a subclass) + and uses it to set the right Referer header. + + Override this method if you use a different Scrapy component to handle + Referer headers, of if you want to use a custom logic to set the + Referer header on redirects. + """ + request.headers.pop("Referer", None) + if not self._referer_spider_middleware: + return + self._referer_spider_middleware.get_processed_request( + request, response + ) + + def _engine_started(self) -> None: + self._referer_spider_middleware = self.crawler.get_spider_middleware(RefererMiddleware) + if self._referer_spider_middleware: + return + redirect_cls = global_object_name(self.__class__) + referer_cls = global_object_name(RefererMiddleware) + if self.__class__ in (RedirectMiddleware, MetaRefreshMiddleware): + replacement = ( + f"replace {redirect_cls} with a subclass that overrides the " + f"handle_referer() method" + ) + else: + replacement = ( + f"or edit {redirect_cls} (if defined in your code base) to " + f"override the handle_referer() method, or replace " + f"{redirect_cls} with a subclass that overrides the " + f"handle_referer() method." + ) + logger.warning( + f"{redirect_cls} found no {referer_cls} instance to handle " + f"Referer header handling, so the Referer header will be removed " + f"on redirects. To set a Referer header on redirects, enable " + f"{referer_cls} (or a subclass), or {replacement}.", + ) + def _redirect(self, redirected: Request, request: Request, reason: Any) -> Request: ttl = request.meta.setdefault("redirect_ttl", self.max_redirect_times) redirects = request.meta.get("redirect_times", 0) + 1 @@ -132,17 +120,79 @@ class BaseRedirectMiddleware: ) raise IgnoreRequest("max redirections reached") - def _redirect_request_using_get( - self, request: Request, redirect_url: str + + def _build_redirect_request( + self, source_request: Request, response: Response, *, url: str, **kwargs: Any ) -> Request: - redirect_request = _build_redirect_request( + redirect_request = source_request.replace( + url=url, + **kwargs, + cls=None, + cookies=None, + ) + if "_scheme_proxy" in redirect_request.meta: + source_request_scheme = urlparse_cached(source_request).scheme + redirect_request_scheme = urlparse_cached(redirect_request).scheme + if source_request_scheme != redirect_request_scheme: + redirect_request.meta.pop("_scheme_proxy") + redirect_request.meta.pop("proxy", None) + redirect_request.meta.pop("_auth_proxy", None) + redirect_request.headers.pop(b"Proxy-Authorization", None) + + has_cookie_header = "Cookie" in redirect_request.headers + has_authorization_header = "Authorization" in redirect_request.headers + if has_cookie_header or has_authorization_header: + default_ports = {"http": 80, "https": 443} + + parsed_source_request = urlparse_cached(source_request) + source_scheme, source_host, source_port = ( + parsed_source_request.scheme, + parsed_source_request.hostname, + parsed_source_request.port + or default_ports.get(parsed_source_request.scheme), + ) + + parsed_redirect_request = urlparse_cached(redirect_request) + redirect_scheme, redirect_host, redirect_port = ( + parsed_redirect_request.scheme, + parsed_redirect_request.hostname, + parsed_redirect_request.port + or default_ports.get(parsed_redirect_request.scheme), + ) + + if has_cookie_header and ( + redirect_scheme not in {source_scheme, "https"} + or source_host != redirect_host + ): + del redirect_request.headers["Cookie"] + + # https://fetch.spec.whatwg.org/#ref-for-cors-non-wildcard-request-header-name + if has_authorization_header and ( + source_scheme != redirect_scheme + or source_host != redirect_host + or source_port != redirect_port + ): + del redirect_request.headers["Authorization"] + + self.handle_referer(redirect_request, response) + + return redirect_request + + def _redirect_request_using_get( + self, request: Request, response: Response, redirect_url: str + ) -> Request: + redirect_request = self._build_redirect_request( request, + response, url=redirect_url, method="GET", body="", ) redirect_request.headers.pop("Content-Type", None) redirect_request.headers.pop("Content-Length", None) + redirect_request.headers.pop("Content-Encoding", None) + redirect_request.headers.pop("Content-Language", None) + redirect_request.headers.pop("Content-Location", None) return redirect_request @@ -176,14 +226,22 @@ class RedirectMiddleware(BaseRedirectMiddleware): location = request_scheme + "://" + location.lstrip("/") redirected_url = urljoin(request.url, location) - redirected = _build_redirect_request(request, url=redirected_url) + + if not urlparse(redirected_url).fragment: + fragment = urlparse_cached(request).fragment + if fragment: + redirected_url = urljoin(redirected_url, f"#{fragment}") + + redirected = self._build_redirect_request(request, response, url=redirected_url) if urlparse_cached(redirected).scheme not in {"http", "https"}: return response - if response.status in (307, 308) or request.method == "HEAD": - return self._redirect(redirected, request, response.status) + if ( + (response.status in (301, 302) and request.method == "POST") + or (response.status == 303 and request.method not in ("GET", "HEAD")) + ): + redirected = self._redirect_request_using_get(request, response, redirected_url) - redirected = self._redirect_request_using_get(request, redirected_url) return self._redirect(redirected, request, response.status) @@ -210,7 +268,7 @@ class MetaRefreshMiddleware(BaseRedirectMiddleware): interval, url = get_meta_refresh(response, ignore_tags=self._ignore_tags) if not url: return response - redirected = self._redirect_request_using_get(request, url) + redirected = self._redirect_request_using_get(request, response, url) if urlparse_cached(redirected).scheme not in {"http", "https"}: return response if cast("float", interval) < self._maxdelay: diff --git a/scrapy/spidermiddlewares/referer.py b/scrapy/spidermiddlewares/referer.py index e4d0f4014..5825f6467 100644 --- a/scrapy/spidermiddlewares/referer.py +++ b/scrapy/spidermiddlewares/referer.py @@ -9,6 +9,7 @@ import warnings from abc import ABC, abstractmethod from typing import TYPE_CHECKING, cast from urllib.parse import urlparse +from warnings import warn from w3lib.url import safe_url_string @@ -342,29 +343,47 @@ class RefererMiddleware(BaseSpiderMiddleware): def from_crawler(cls, crawler: Crawler) -> Self: if not crawler.settings.getbool("REFERER_ENABLED"): raise NotConfigured - mw = cls(crawler.settings) + return cls(crawler.settings) - # Note: this hook is a bit of a hack to intercept redirections - crawler.signals.connect(mw.request_scheduled, signal=signals.request_scheduled) - - return mw - - def policy(self, resp_or_url: Response | str, request: Request) -> ReferrerPolicy: - """ - Determine Referrer-Policy to use from a parent Response (or URL), - and a Request to be sent. + def policy( + self, + response: Response | str | None = None, + request: Request | None = None, + **kwargs, + ) -> ReferrerPolicy: + """Return the referrer policy to use for *request* based on *request* + meta, *response* and settings. - if a valid policy is set in Request meta, it is used. - - if the policy is set in meta but is wrong (e.g. a typo error), - the policy from settings is used - - if the policy is not set in Request meta, - but there is a Referrer-policy header in the parent response, - it is used if valid + - if the policy is set in meta but is wrong (e.g. a typo error), the + policy from settings is used + - if the policy is not set in Request meta, but there is a + Referrer-Policy header in the parent response, it is used if valid - otherwise, the policy from settings is used. """ + if "resp_or_url" in kwargs: + if response is not None: + raise TypeError("Cannot pass both 'response' and 'resp_or_url'") + response = kwargs.pop("resp_or_url") + warn( + "Passing 'resp_or_url' is deprecated, use 'response' instead.", + DeprecationWarning, + stacklevel=2, + ) + if response is None: + raise TypeError("Missing required argument: 'response'") + if request is None: + raise TypeError("Missing required argument: 'request'") + if isinstance(response, str): + warn( + "Passing a response URL to RefererMiddleware.policy() instead " + "of a Response object is deprecated.", + DeprecationWarning, + stacklevel=2, + ) policy_name = request.meta.get("referrer_policy") - if policy_name is None and isinstance(resp_or_url, Response): - policy_header = resp_or_url.headers.get("Referrer-Policy") + if policy_name is None and isinstance(response, Response): + policy_header = response.headers.get("Referrer-Policy") if policy_header is not None: policy_name = to_unicode(policy_header.decode("latin1")) if policy_name is None: @@ -383,25 +402,3 @@ class RefererMiddleware(BaseSpiderMiddleware): if referrer is not None: request.headers.setdefault("Referer", referrer) return request - - def request_scheduled(self, request: Request, spider: Spider) -> None: - # check redirected request to patch "Referer" header if necessary - redirected_urls = request.meta.get("redirect_urls", []) - if redirected_urls: - request_referrer = request.headers.get("Referer") - # we don't patch the referrer value if there is none - if request_referrer is not None: - # the request's referrer header value acts as a surrogate - # for the parent response URL - # - # Note: if the 3xx response contained a Referrer-Policy header, - # the information is not available using this hook - parent_url = safe_url_string(request_referrer) - policy_referrer = self.policy(parent_url, request).referrer( - parent_url, request.url - ) - if policy_referrer != request_referrer.decode("latin1"): - if policy_referrer is None: - request.headers.pop("Referer") - else: - request.headers["Referer"] = policy_referrer diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index 421b654a7..4197c7c86 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -1,4 +1,6 @@ +import logging from itertools import chain, product +from unittest.mock import MagicMock import pytest @@ -9,8 +11,14 @@ from scrapy.downloadermiddlewares.redirect import ( ) from scrapy.exceptions import IgnoreRequest from scrapy.http import HtmlResponse, Request, Response +from scrapy.spidermiddlewares.referer import ( + POLICY_NO_REFERRER, + POLICY_ORIGIN, + POLICY_UNSAFE_URL, + RefererMiddleware, +) from scrapy.spiders import Spider -from scrapy.utils.misc import set_environ +from scrapy.utils.misc import build_from_crawler, set_environ from scrapy.utils.spider import DefaultSpider from scrapy.utils.test import get_crawler @@ -18,8 +26,8 @@ from scrapy.utils.test import get_crawler class Base: class Test: def test_priority_adjust(self): - req = Request("http://a.com") - rsp = self.get_response(req, "http://a.com/redirected") + req = Request("http://a.example") + rsp = self.get_response(req, "http://a.example/redirected") req2 = self.mw.process_response(req, rsp) assert req2.priority > req.priority @@ -65,7 +73,7 @@ class Base: def test_max_redirect_times(self): self.mw.max_redirect_times = 1 - req = Request("http://scrapytest.org/302") + req = Request("http://a.example/302") rsp = self.get_response(req, "/redirected") req = self.mw.process_response(req, rsp) @@ -77,7 +85,7 @@ class Base: def test_ttl(self): self.mw.max_redirect_times = 100 - req = Request("http://scrapytest.org/302", meta={"redirect_ttl": 1}) + req = Request("http://a.example/302", meta={"redirect_ttl": 1}) rsp = self.get_response(req, "/a") req = self.mw.process_response(req, rsp) @@ -86,22 +94,22 @@ class Base: self.mw.process_response(req, rsp) def test_redirect_urls(self): - req1 = Request("http://scrapytest.org/first") + req1 = Request("http://a.example/first") rsp1 = self.get_response(req1, "/redirected") req2 = self.mw.process_response(req1, rsp1) rsp2 = self.get_response(req1, "/redirected2") req3 = self.mw.process_response(req2, rsp2) - assert req2.url == "http://scrapytest.org/redirected" - assert req2.meta["redirect_urls"] == ["http://scrapytest.org/first"] - assert req3.url == "http://scrapytest.org/redirected2" + assert req2.url == "http://a.example/redirected" + assert req2.meta["redirect_urls"] == ["http://a.example/first"] + assert req3.url == "http://a.example/redirected2" assert req3.meta["redirect_urls"] == [ - "http://scrapytest.org/first", - "http://scrapytest.org/redirected", + "http://a.example/first", + "http://a.example/redirected", ] def test_redirect_reasons(self): - req1 = Request("http://scrapytest.org/first") + req1 = Request("http://a.example/first") rsp1 = self.get_response(req1, "/redirected1") req2 = self.mw.process_response(req1, rsp1) rsp2 = self.get_response(req2, "/redirected2") @@ -1046,8 +1054,158 @@ class TestRedirectMiddleware(Base.Test): assert "Content-Length" not in redirect_request.headers assert not redirect_request.body + @pytest.mark.parametrize("status", [301, 302]) + @pytest.mark.parametrize("method", ["PUT", "DELETE"]) + def test_method_not_converted_on_301_302(self, status, method): + url = f"http://www.example.com/{status}" + url2 = "http://www.example.com/redirected" + body = b"test-body" + req = Request( + url, + method=method, + body=body, + headers={"Content-Type": "text/plain", "Content-Length": str(len(body))}, + ) + rsp = Response(url, headers={"Location": url2}, status=status) + + req2 = self.mw.process_response(req, rsp) + assert isinstance(req2, Request) + assert req2.url == url2 + assert req2.method == method + assert req2.body == body + assert req2.headers[b"Content-Type"] == b"text/plain" + assert req2.headers[b"Content-Length"] == str(len(body)).encode() + + @pytest.mark.parametrize("method", ["PUT", "DELETE"]) + def test_method_converted_on_303(self, method): + status = 303 + url = f"http://www.example.com/{status}" + url2 = "http://www.example.com/redirected" + req = Request(url, method=method) + rsp = Response(url, headers={"Location": url2}, status=status) + + req2 = self.mw.process_response(req, rsp) + assert isinstance(req2, Request) + assert req2.url == url2 + assert req2.method == "GET" + + def test_get_method_body_preserved_on_303(self): + status = 303 + url = f"http://www.example.com/{status}" + url2 = "http://www.example.com/redirected" + body = b"test-body" + req = Request( + url, + method="GET", + body=body, + headers={"Content-Type": "text/plain", "Content-Length": str(len(body))}, + ) + rsp = Response(url, headers={"Location": url2}, status=status) + + req2 = self.mw.process_response(req, rsp) + assert isinstance(req2, Request) + assert req2.url == url2 + assert req2.method == "GET" + assert req2.body == body + assert req2.headers[b"Content-Type"] == b"text/plain" + assert req2.headers[b"Content-Length"] == str(len(body)).encode() + + def test_redirect_strips_content_headers(self): + url = "http://www.example.com/303" + url2 = "http://www.example.com/redirected" + headers = { + "Content-Type": "application/json", + "Content-Length": "100", + "Content-Encoding": "gzip", + "Content-Language": "en", + "Content-Location": "http://www.example.com/original", + "X-Custom": "foo", + } + req = Request(url, method="POST", headers=headers, body=b"foo") + rsp = Response(url, headers={"Location": url2}, status=303) + + req2 = self.mw.process_response(req, rsp) + assert isinstance(req2, Request) + assert req2.url == url2 + assert req2.method == "GET" + assert req2.body == b"" + assert "Content-Type" not in req2.headers + assert "Content-Length" not in req2.headers + assert "Content-Encoding" not in req2.headers + assert "Content-Language" not in req2.headers + assert "Content-Location" not in req2.headers + assert req2.headers["X-Custom"] == b"foo" + + @pytest.mark.parametrize( + ( + "referer", + "source_url", + "target_url", + "policy_header", + "expected_referer", + ), + [ + ( + None, + "http://www.example.com/302", + "http://www.example.com/redirected", + None, + b"http://www.example.com/302", + ), + ( + "http://example.com/old", + "http://www.example.com/302", + "http://www.example.com/redirected", + None, + b"http://www.example.com/302", + ), + ( + "https://example.com/old", + "https://www.example.com/302", + "http://www.example.com/redirected", + None, + None, + ), + ( + "http://example.com/old", + "http://www.example.com/foo/bar", + "http://www.example.com/redirected", + "origin", + b"http://www.example.com/", + ), + ], + ) + def test_redirect_referer( + self, referer, source_url, target_url, policy_header, expected_referer + ): + headers = {"Referer": referer} if referer else {} + source_request = Request(source_url, headers=headers) + resp_headers = {"Location": target_url} + if policy_header: + resp_headers["Referrer-Policy"] = policy_header + response = Response(source_url, headers=resp_headers, status=302) + crawler = get_crawler() + referer_mw = build_from_crawler(RefererMiddleware, crawler) + redirect_mw = self.mwcls.from_crawler(crawler) + redirect_mw._referer_spider_middleware = referer_mw + redirect_request = redirect_mw.process_response(source_request, response) + if expected_referer: + assert redirect_request.headers.get("Referer") == expected_referer + else: + assert "Referer" not in redirect_request.headers + + def test_redirect_strips_referer_no_middleware(self): + source_url = "http://www.example.com/302" + redirect_url = "http://www.example.com/redirected" + source_request = Request(source_url, headers={"Referer": "http://example.com/old"}) + response = Response(source_url, headers={"Location": redirect_url}, status=302) + redirect_mw = self.mwcls.from_crawler(get_crawler()) + redirect_mw._referer_spider_middleware = None + redirect_request = redirect_mw.process_response(source_request, response) + assert "Referer" not in redirect_request.headers + @pytest.mark.parametrize("status", [307, 308]) - def test_cross_origin_strip_body(self, status): + def test_cross_origin_maintain_body(self, status): source_url = "https://example.com" target_url = "https://attacker.example" body = b"secret" @@ -1062,9 +1220,19 @@ class TestRedirectMiddleware(Base.Test): assert isinstance(redirect_request, Request) assert redirect_request.url == target_url assert redirect_request.method == "POST" - assert not getattr(redirect_request, "body", None) - assert b"Content-Type" not in redirect_request.headers - assert b"Content-Length" not in redirect_request.headers + assert redirect_request.body == body + assert redirect_request.headers[b"Content-Type"] == b"application/json" + assert redirect_request.headers[b"Content-Length"] == str(len(body)).encode() + + def test_redirect_keeps_fragment(self): + url = "http://www.example.com/301#frag" + url2 = "http://www.example.com/redirected" + req = Request(url) + rsp = Response(url, headers={"Location": url2}, status=302) + + req2 = self.mw.process_response(req, rsp) + assert isinstance(req2, Request) + assert req2.url == "http://www.example.com/redirected#frag" def test_redirect_302_head(self): url = "http://www.example.com/302" @@ -1311,4 +1479,114 @@ def test_meta_refresh_schemes(url, location, target): assert redirect == response else: assert isinstance(redirect, Request) - assert redirect.url == target + +@pytest.mark.parametrize( + ("policy", "source_url", "target_url", "expected_referrer"), + [ + # The policy header affects the outcome. + # (without it, the https → http switch would drop the referer) + ( + POLICY_UNSAFE_URL, + "https://a.example/1", + "http://a.example/2", + b"https://a.example/1", + ), + # The policy header can get the Referer header removed. + ( + POLICY_NO_REFERRER, + "http://a.example/1", + "http://a.example/2", + None, + ), + # The policy header can get the Referer header edited (path stripped). + ( + POLICY_ORIGIN, + "http://a.example/1", + "http://a.example/2", + b"http://a.example/", + ), + ], +) +def test_response_referrer_policy( + policy, source_url, target_url, expected_referrer +): + crawler = get_crawler() + referrer_mw = build_from_crawler(RefererMiddleware, crawler) + redirect_mw = build_from_crawler(RedirectMiddleware, crawler) + redirect_mw._referer_spider_middleware = referrer_mw + source_request = Request(source_url) + extra_headers = {} + if policy: + extra_headers["Referrer-Policy"] = policy + response_redirect = Response( + source_request.url, status=301, headers={"Location": target_url, **extra_headers} + ) + source_request = redirect_mw.process_response( + source_request, response_redirect + ) + assert isinstance(source_request, Request) + + assert source_request.headers.get("Referer") == expected_referrer + + +def test_no_warning_when_referer_middleware_present(caplog): + crawler = get_crawler() + crawler.get_spider_middleware = MagicMock(return_value=MagicMock()) + mw = build_from_crawler(RedirectMiddleware, crawler) + mw._engine_started() + assert not caplog.records + + +def test_warning_redirect_middleware(caplog): + crawler = get_crawler() + crawler.get_spider_middleware = MagicMock(return_value=None) + mw = build_from_crawler(RedirectMiddleware, crawler) + with caplog.at_level(logging.WARNING): + mw._engine_started() + assert ( + "scrapy.downloadermiddlewares.redirect.RedirectMiddleware found no " + "scrapy.spidermiddlewares.referer.RefererMiddleware" + ) in caplog.text + assert "enable scrapy.spidermiddlewares.referer.RefererMiddleware (or a subclass)" in caplog.text + assert ( + "replace scrapy.downloadermiddlewares.redirect.RedirectMiddleware " + "with a subclass that overrides the handle_referer() method" + ) in caplog.text + + +def test_warning_meta_refresh_middleware(caplog): + crawler = get_crawler() + crawler.get_spider_middleware = MagicMock(return_value=None) + mw = build_from_crawler(MetaRefreshMiddleware, crawler) + with caplog.at_level(logging.WARNING): + mw._engine_started() + assert ( + "scrapy.downloadermiddlewares.redirect.MetaRefreshMiddleware found no " + "scrapy.spidermiddlewares.referer.RefererMiddleware" + ) in caplog.text + assert "enable scrapy.spidermiddlewares.referer.RefererMiddleware (or a subclass)" in caplog.text + assert ( + "replace scrapy.downloadermiddlewares.redirect.MetaRefreshMiddleware " + "with a subclass that overrides the handle_referer() method" + ) in caplog.text + + +def test_warning_subclass(caplog): + class MyRedirectMiddleware(RedirectMiddleware): + pass + + crawler = get_crawler() + crawler.get_spider_middleware = MagicMock(return_value=None) + mw = build_from_crawler(MyRedirectMiddleware, crawler) + with caplog.at_level(logging.WARNING): + mw._engine_started() + assert ( + "test_warning_subclass..MyRedirectMiddleware found no " + "scrapy.spidermiddlewares.referer.RefererMiddleware" + ) in caplog.text + assert "enable scrapy.spidermiddlewares.referer.RefererMiddleware (or a subclass)" in caplog.text + assert "edit " in caplog.text + assert "test_warning_subclass..MyRedirectMiddleware" in caplog.text + assert ( + "(if defined in your code base) to override the handle_referer() method" + ) in caplog.text diff --git a/tests/test_spidermiddleware_referer.py b/tests/test_spidermiddleware_referer.py index 68fcad98c..b4090b78c 100644 --- a/tests/test_spidermiddleware_referer.py +++ b/tests/test_spidermiddleware_referer.py @@ -6,7 +6,6 @@ from urllib.parse import urlparse import pytest -from scrapy.downloadermiddlewares.redirect import RedirectMiddleware from scrapy.http import Request, Response from scrapy.settings import Settings from scrapy.spidermiddlewares.referer import ( @@ -31,6 +30,7 @@ from scrapy.spidermiddlewares.referer import ( StrictOriginWhenCrossOriginPolicy, UnsafeUrlPolicy, ) +from scrapy.utils.misc import build_from_crawler from scrapy.utils.spider import DefaultSpider from scrapy.utils.test import get_crawler @@ -970,391 +970,58 @@ class TestPolicyHeaderPrecedence004( resp_headers = {"Referrer-Policy": ""} -class TestReferrerOnRedirect(TestRefererMiddleware): - settings = {"REFERRER_POLICY": "scrapy.spidermiddlewares.referer.UnsafeUrlPolicy"} - scenarii: Sequence[ - tuple[str, str, tuple[tuple[int, str], ...], bytes | None, bytes | None] - ] = [ # type: ignore[assignment] - ( - "http://scrapytest.org/1", # parent - "http://scrapytest.org/2", # target - ( - # redirections: code, URL - (301, "http://scrapytest.org/3"), - (301, "http://scrapytest.org/4"), - ), - b"http://scrapytest.org/1", # expected initial referer - b"http://scrapytest.org/1", # expected referer for the redirection request - ), - ( - "https://scrapytest.org/1", - "https://scrapytest.org/2", - ( - # redirecting to non-secure URL - (301, "http://scrapytest.org/3"), - ), - b"https://scrapytest.org/1", - b"https://scrapytest.org/1", - ), - ( - "https://scrapytest.org/1", - "https://scrapytest.com/2", - ( - # redirecting to non-secure URL: different origin - (301, "http://scrapytest.com/3"), - ), - b"https://scrapytest.org/1", - b"https://scrapytest.org/1", - ), - ] +class TestPolicyMethodResponseParamRename: + def setup_method(self): + self.crawler = get_crawler() + self.mw = build_from_crawler(RefererMiddleware, self.crawler) + self.request = Request("http://www.example.com") + self.response = Response("http://www.example.com") - @pytest.fixture - def crawler(self) -> Crawler: - crawler = get_crawler(DefaultSpider, self.settings) - crawler.spider = crawler._create_spider() - return crawler + def test_pos_string(self): + with warnings.catch_warnings(record=True) as w: + self.mw.policy("http://old.com", self.request) + found = False + for warning in w: + if "Passing a response URL" in str(warning.message): + found = True + break + assert found - @pytest.fixture - def referrermw(self, crawler: Crawler) -> RefererMiddleware: - return RefererMiddleware.from_crawler(crawler) + def test_pos_response(self): + with warnings.catch_warnings(record=True) as w: + self.mw.policy(self.response, self.request) + for warning in w: + assert "resp_or_url" not in str(warning.message) - @pytest.fixture - def redirectmw(self, crawler: Crawler) -> RedirectMiddleware: - return RedirectMiddleware.from_crawler(crawler) + def test_key_resp_or_url(self): + with warnings.catch_warnings(record=True) as w: + self.mw.policy(resp_or_url=self.response, request=self.request) + found = False + for warning in w: + if "Passing 'resp_or_url' is deprecated, use 'response' instead" in str(warning.message): + found = True + break + assert found - def test( # type: ignore[override] - self, - crawler: Crawler, - referrermw: RefererMiddleware, - redirectmw: RedirectMiddleware, - ) -> None: - for ( - parent, - target, - redirections, - init_referrer, - final_referrer, - ) in self.scenarii: - response = self.get_response(parent) - request = self.get_request(target) + def test_key_response(self): + with warnings.catch_warnings(record=True) as w: + self.mw.policy(response=self.response, request=self.request) + for warning in w: + assert "resp_or_url" not in str(warning.message) - out = list(referrermw.process_spider_output(response, [request])) - assert out[0].headers.get("Referer") == init_referrer + def test_key_response_string(self): + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + self.mw.policy(response="http://old.com", request=self.request) + found = False + for warning in w: + if "Passing a response URL" in str(warning.message): + found = True + break + assert found - for status, url in redirections: - response = Response( - request.url, headers={"Location": url}, status=status - ) - request = cast( - "Request", redirectmw.process_response(request, response) - ) - assert crawler.spider - referrermw.request_scheduled(request, crawler.spider) - - assert isinstance(request, Request) - assert request.headers.get("Referer") == final_referrer + def test_both_resp_or_url_and_response(self): + with pytest.raises(TypeError, match="Cannot pass both 'response' and 'resp_or_url'"): + self.mw.policy(response=self.response, resp_or_url=self.response, request=self.request) -class TestReferrerOnRedirectNoReferrer(TestReferrerOnRedirect): - """ - No Referrer policy never sets the "Referer" header. - HTTP redirections should not change that. - """ - - settings = {"REFERRER_POLICY": "no-referrer"} - scenarii = [ - ( - "http://scrapytest.org/1", # parent - "http://scrapytest.org/2", # target - ( - # redirections: code, URL - (301, "http://scrapytest.org/3"), - (301, "http://scrapytest.org/4"), - ), - None, # expected initial "Referer" - None, # expected "Referer" for the redirection request - ), - ( - "https://scrapytest.org/1", - "https://scrapytest.org/2", - ((301, "http://scrapytest.org/3"),), - None, - None, - ), - ( - "https://scrapytest.org/1", - "https://example.com/2", # different origin - ((301, "http://scrapytest.com/3"),), - None, - None, - ), - ] - - -class TestReferrerOnRedirectSameOrigin(TestReferrerOnRedirect): - """ - Same Origin policy sends the full URL as "Referer" if the target origin - is the same as the parent response (same protocol, same domain, same port). - - HTTP redirections to a different domain or a lower secure level - should have the "Referer" removed. - """ - - settings = {"REFERRER_POLICY": "same-origin"} - scenarii = [ - ( - "http://scrapytest.org/101", # origin - "http://scrapytest.org/102", # target - ( - # redirections: code, URL - (301, "http://scrapytest.org/103"), - (301, "http://scrapytest.org/104"), - ), - b"http://scrapytest.org/101", # expected initial "Referer" - b"http://scrapytest.org/101", # expected referer for the redirection request - ), - ( - "https://scrapytest.org/201", - "https://scrapytest.org/202", - ( - # redirecting from secure to non-secure URL == different origin - (301, "http://scrapytest.org/203"), - ), - b"https://scrapytest.org/201", - None, - ), - ( - "https://scrapytest.org/301", - "https://scrapytest.org/302", - ( - # different domain == different origin - (301, "http://example.com/303"), - ), - b"https://scrapytest.org/301", - None, - ), - ] - - -class TestReferrerOnRedirectStrictOrigin(TestReferrerOnRedirect): - """ - Strict Origin policy will always send the "origin" as referrer - (think of it as the parent URL without the path part), - unless the security level is lower and no "Referer" is sent. - - Redirections from secure to non-secure URLs should have the - "Referrer" header removed if necessary. - """ - - settings = {"REFERRER_POLICY": POLICY_STRICT_ORIGIN} - scenarii = [ - ( - "http://scrapytest.org/101", - "http://scrapytest.org/102", - ( - (301, "http://scrapytest.org/103"), - (301, "http://scrapytest.org/104"), - ), - b"http://scrapytest.org/", # send origin - b"http://scrapytest.org/", # redirects to same origin: send origin - ), - ( - "https://scrapytest.org/201", - "https://scrapytest.org/202", - ( - # redirecting to non-secure URL: no referrer - (301, "http://scrapytest.org/203"), - ), - b"https://scrapytest.org/", - None, - ), - ( - "https://scrapytest.org/301", - "https://scrapytest.org/302", - ( - # redirecting to non-secure URL (different domain): no referrer - (301, "http://example.com/303"), - ), - b"https://scrapytest.org/", - None, - ), - ( - "http://scrapy.org/401", - "http://example.com/402", - ((301, "http://scrapytest.org/403"),), - b"http://scrapy.org/", - b"http://scrapy.org/", - ), - ( - "https://scrapy.org/501", - "https://example.com/502", - ( - # HTTPS all along, so origin referrer is kept as-is - (301, "https://google.com/503"), - (301, "https://facebook.com/504"), - ), - b"https://scrapy.org/", - b"https://scrapy.org/", - ), - ( - "https://scrapytest.org/601", - "http://scrapytest.org/602", # TLS to non-TLS: no referrer - ( - ( - 301, - "https://scrapytest.org/603", - ), # TLS URL again: (still) no referrer - ), - None, - None, - ), - ] - - -class TestReferrerOnRedirectOriginWhenCrossOrigin(TestReferrerOnRedirect): - """ - Origin When Cross-Origin policy sends the full URL as "Referer", - unless the target's origin is different (different domain, different protocol) - in which case only the origin is sent. - - Redirections to a different origin should strip the "Referer" - to the parent origin. - """ - - settings = {"REFERRER_POLICY": POLICY_ORIGIN_WHEN_CROSS_ORIGIN} - scenarii = [ - ( - "http://scrapytest.org/101", # origin - "http://scrapytest.org/102", # target + redirection - ( - # redirections: code, URL - (301, "http://scrapytest.org/103"), - (301, "http://scrapytest.org/104"), - ), - b"http://scrapytest.org/101", # expected initial referer - b"http://scrapytest.org/101", # expected referer for the redirection request - ), - ( - "https://scrapytest.org/201", - "https://scrapytest.org/202", - ( - # redirecting to non-secure URL: send origin - (301, "http://scrapytest.org/203"), - ), - b"https://scrapytest.org/201", - b"https://scrapytest.org/", - ), - ( - "https://scrapytest.org/301", - "https://scrapytest.org/302", - ( - # redirecting to non-secure URL (different domain): send origin - (301, "http://example.com/303"), - ), - b"https://scrapytest.org/301", - b"https://scrapytest.org/", - ), - ( - "http://scrapy.org/401", - "http://example.com/402", - ((301, "http://scrapytest.org/403"),), - b"http://scrapy.org/", - b"http://scrapy.org/", - ), - ( - "https://scrapy.org/501", - "https://example.com/502", - ( - # all different domains: send origin - (301, "https://google.com/503"), - (301, "https://facebook.com/504"), - ), - b"https://scrapy.org/", - b"https://scrapy.org/", - ), - ( - "https://scrapytest.org/301", - "http://scrapytest.org/302", # TLS to non-TLS: send origin - ((301, "https://scrapytest.org/303"),), # TLS URL again: send origin (also) - b"https://scrapytest.org/", - b"https://scrapytest.org/", - ), - ] - - -class TestReferrerOnRedirectStrictOriginWhenCrossOrigin(TestReferrerOnRedirect): - """ - Strict Origin When Cross-Origin policy sends the full URL as "Referer", - unless the target's origin is different (different domain, different protocol) - in which case only the origin is sent... - Unless there's also a downgrade in security and then the "Referer" header - is not sent. - - Redirections to a different origin should strip the "Referer" to the parent origin, - and from https:// to http:// will remove the "Referer" header. - """ - - settings = {"REFERRER_POLICY": POLICY_STRICT_ORIGIN_WHEN_CROSS_ORIGIN} - scenarii = [ - ( - "http://scrapytest.org/101", # origin - "http://scrapytest.org/102", # target + redirection - ( - # redirections: code, URL - (301, "http://scrapytest.org/103"), - (301, "http://scrapytest.org/104"), - ), - b"http://scrapytest.org/101", # expected initial referer - b"http://scrapytest.org/101", # expected referer for the redirection request - ), - ( - "https://scrapytest.org/201", - "https://scrapytest.org/202", - ( - # redirecting to non-secure URL: do not send the "Referer" header - (301, "http://scrapytest.org/203"), - ), - b"https://scrapytest.org/201", - None, - ), - ( - "https://scrapytest.org/301", - "https://scrapytest.org/302", - ( - # redirecting to non-secure URL (different domain): send origin - (301, "http://example.com/303"), - ), - b"https://scrapytest.org/301", - None, - ), - ( - "http://scrapy.org/401", - "http://example.com/402", - ((301, "http://scrapytest.org/403"),), - b"http://scrapy.org/", - b"http://scrapy.org/", - ), - ( - "https://scrapy.org/501", - "https://example.com/502", - ( - # all different domains: send origin - (301, "https://google.com/503"), - (301, "https://facebook.com/504"), - ), - b"https://scrapy.org/", - b"https://scrapy.org/", - ), - ( - "https://scrapytest.org/601", - "http://scrapytest.org/602", # TLS to non-TLS: do not send "Referer" - ( - ( - 301, - "https://scrapytest.org/603", - ), # TLS URL again: (still) send nothing - ), - None, - None, - ), - ]