diff --git a/docs/news.rst b/docs/news.rst index 4e8ded529..ee603d162 100644 --- a/docs/news.rst +++ b/docs/news.rst @@ -6,6 +6,38 @@ Release notes .. note:: Scrapy 1.x will be the last series supporting Python 2. Scrapy 2.0, planned for Q4 2019 or Q1 2020, will support **Python 3 only**. +.. _release-1.8.2: + +Scrapy 1.8.2 (2022-03-01) +------------------------- + +**Security bug fixes:** + +- When a :class:`~scrapy.http.Request` object with cookies defined gets a + redirect response causing a new :class:`~scrapy.http.Request` object to be + scheduled, the cookies defined in the original + :class:`~scrapy.http.Request` object are no longer copied into the new + :class:`~scrapy.http.Request` object. + + If you manually set the ``Cookie`` header on a + :class:`~scrapy.http.Request` object and the domain name of the redirect + URL is not an exact match for the domain of the URL of the original + :class:`~scrapy.http.Request` object, your ``Cookie`` header is now dropped + from the new :class:`~scrapy.http.Request` object. + + The old behavior could be exploited by an attacker to gain access to your + cookies. Please, see the `cjvr-mfj7-j4j8 security advisory`_ for more + information. + + .. _cjvr-mfj7-j4j8 security advisory: https://github.com/scrapy/scrapy/security/advisories/GHSA-cjvr-mfj7-j4j8 + + .. note:: It is still possible to enable the sharing of cookies between + different domains with a shared domain suffix (e.g. + ``example.com`` and any subdomain) by defining the shared domain + suffix (e.g. ``example.com``) as the cookie domain when defining + your cookies. See the documentation of the + :class:`~scrapy.http.Request` class for more information. + .. _release-1.8.1: Scrapy 1.8.1 (2021-10-05) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index 49468a2e4..823b4b939 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -4,12 +4,28 @@ from six.moves.urllib.parse import urljoin from w3lib.url import safe_url_string from scrapy.http import HtmlResponse +from scrapy.utils.httpobj import urlparse_cached from scrapy.utils.response import get_meta_refresh from scrapy.exceptions import IgnoreRequest, NotConfigured logger = logging.getLogger(__name__) +def _build_redirect_request(source_request, *, url, method=None, body=None): + redirect_request = source_request.replace( + url=url, + method=method, + body=body, + cookies=None, + ) + if 'Cookie' in redirect_request.headers: + source_request_netloc = urlparse_cached(source_request).netloc + redirect_request_netloc = urlparse_cached(redirect_request).netloc + if source_request_netloc != redirect_request_netloc: + del redirect_request.headers['Cookie'] + return redirect_request + + class BaseRedirectMiddleware(object): enabled_setting = 'REDIRECT_ENABLED' @@ -48,10 +64,15 @@ class BaseRedirectMiddleware(object): raise IgnoreRequest("max redirections reached") def _redirect_request_using_get(self, request, redirect_url): - redirected = request.replace(url=redirect_url, method='GET', body='') - redirected.headers.pop('Content-Type', None) - redirected.headers.pop('Content-Length', None) - return redirected + redirect_request = _build_redirect_request( + request, + url=redirect_url, + method='GET', + body='', + ) + redirect_request.headers.pop('Content-Type', None) + redirect_request.headers.pop('Content-Length', None) + return redirect_request class RedirectMiddleware(BaseRedirectMiddleware): @@ -75,7 +96,7 @@ class RedirectMiddleware(BaseRedirectMiddleware): redirected_url = urljoin(request.url, location) if response.status in (301, 307, 308) or request.method == 'HEAD': - redirected = request.replace(url=redirected_url) + redirected = _build_redirect_request(request, url=redirected_url) return self._redirect(redirected, request, spider, response.status) redirected = self._redirect_request_using_get(request, redirected_url) diff --git a/tests/test_downloadermiddleware_cookies.py b/tests/test_downloadermiddleware_cookies.py index 04884fb78..f9fb14e9c 100644 --- a/tests/test_downloadermiddleware_cookies.py +++ b/tests/test_downloadermiddleware_cookies.py @@ -3,7 +3,9 @@ import logging from unittest import TestCase from testfixtures import LogCapture +from scrapy.downloadermiddlewares.redirect import RedirectMiddleware from scrapy.http import Response, Request +from scrapy.settings import Settings from scrapy.spiders import Spider from scrapy.utils.test import get_crawler from scrapy.exceptions import NotConfigured @@ -21,9 +23,11 @@ class CookiesMiddlewareTest(TestCase): def setUp(self): self.spider = Spider('foo') self.mw = CookiesMiddleware() + self.redirect_middleware = RedirectMiddleware(settings=Settings()) def tearDown(self): del self.mw + del self.redirect_middleware def test_basic(self): req = Request('http://scrapytest.org/') @@ -229,3 +233,154 @@ class CookiesMiddlewareTest(TestCase): assert self.mw.process_request(request, self.spider) is None self.assertIn('Cookie', request.headers) self.assertEqual(b'currencyCookie=USD', request.headers['Cookie']) + + def _test_cookie_redirect( + self, + source, + target, + *, + cookies1, + cookies2, + ): + input_cookies = {'a': 'b'} + + if not isinstance(source, dict): + source = {'url': source} + if not isinstance(target, dict): + target = {'url': target} + target.setdefault('status', 301) + + request1 = Request(cookies=input_cookies, **source) + self.mw.process_request(request1, self.spider) + cookies = request1.headers.get('Cookie') + self.assertEqual(cookies, b"a=b" if cookies1 else None) + + response = Response( + headers={ + 'Location': target['url'], + }, + **target, + ) + self.assertEqual( + self.mw.process_response(request1, response, self.spider), + response, + ) + + request2 = self.redirect_middleware.process_response( + request1, + response, + self.spider, + ) + self.assertIsInstance(request2, Request) + + self.mw.process_request(request2, self.spider) + cookies = request2.headers.get('Cookie') + self.assertEqual(cookies, b"a=b" if cookies2 else None) + + def test_cookie_redirect_same_domain(self): + self._test_cookie_redirect( + 'https://toscrape.com', + 'https://toscrape.com', + cookies1=True, + cookies2=True, + ) + + def test_cookie_redirect_same_domain_forcing_get(self): + self._test_cookie_redirect( + 'https://toscrape.com', + {'url': 'https://toscrape.com', 'status': 302}, + cookies1=True, + cookies2=True, + ) + + def test_cookie_redirect_different_domain(self): + self._test_cookie_redirect( + 'https://toscrape.com', + 'https://example.com', + cookies1=True, + cookies2=False, + ) + + def test_cookie_redirect_different_domain_forcing_get(self): + self._test_cookie_redirect( + 'https://toscrape.com', + {'url': 'https://example.com', 'status': 302}, + cookies1=True, + cookies2=False, + ) + + def _test_cookie_header_redirect( + self, + source, + target, + *, + cookies2, + ): + """Test the handling of a user-defined Cookie header when building a + redirect follow-up request. + + We follow RFC 6265 for cookie handling. The Cookie header can only + contain a list of key-value pairs (i.e. no additional cookie + parameters like Domain or Path). Because of that, we follow the same + rules that we would follow for the handling of the Set-Cookie response + header when the Domain is not set: the cookies must be limited to the + target URL domain (not even subdomains can receive those cookies). + + .. note:: This method tests the scenario where the cookie middleware is + disabled. Because of known issue #1992, when the cookies + middleware is enabled we do not need to be concerned about + the Cookie header getting leaked to unintended domains, + because the middleware empties the header from every request. + """ + if not isinstance(source, dict): + source = {'url': source} + if not isinstance(target, dict): + target = {'url': target} + target.setdefault('status', 301) + + request1 = Request(headers={'Cookie': b'a=b'}, **source) + + response = Response( + headers={ + 'Location': target['url'], + }, + **target, + ) + + request2 = self.redirect_middleware.process_response( + request1, + response, + self.spider, + ) + self.assertIsInstance(request2, Request) + + cookies = request2.headers.get('Cookie') + self.assertEqual(cookies, b"a=b" if cookies2 else None) + + def test_cookie_header_redirect_same_domain(self): + self._test_cookie_header_redirect( + 'https://toscrape.com', + 'https://toscrape.com', + cookies2=True, + ) + + def test_cookie_header_redirect_same_domain_forcing_get(self): + self._test_cookie_header_redirect( + 'https://toscrape.com', + {'url': 'https://toscrape.com', 'status': 302}, + cookies2=True, + ) + + def test_cookie_header_redirect_different_domain(self): + self._test_cookie_header_redirect( + 'https://toscrape.com', + 'https://example.com', + cookies2=False, + ) + + def test_cookie_header_redirect_different_domain_forcing_get(self): + self._test_cookie_header_redirect( + 'https://toscrape.com', + {'url': 'https://example.com', 'status': 302}, + cookies2=False, + )