diff --git a/docs/news.rst b/docs/news.rst index aef12d9db..9590fb1c4 100644 --- a/docs/news.rst +++ b/docs/news.rst @@ -52,6 +52,18 @@ Security bug fixes your cookies. See the documentation of the :class:`~scrapy.http.Request` class for more information. +- When the domain of a cookie, either received in the ``Set-Cookie`` header + of a response or defined in a :class:`~scrapy.http.Request` object, is set + to a `public suffix `_, the cookie is now + ignored unless the cookie domain is the same as the request domain. + + The old behavior could be exploited by an attacker to inject cookies from a + controlled domain into your cookiejar that could be sent to other domains + not controlled by the attacker. Please, see the `mfjm-vh54-3f96 security + advisory`_ for more information. + + .. _mfjm-vh54-3f96 security advisory: https://github.com/scrapy/scrapy/security/advisories/GHSA-mfjm-vh54-3f96 + Modified requirements ~~~~~~~~~~~~~~~~~~~~~ @@ -1875,6 +1887,7 @@ affect subclasses: (:issue:`3884`) + .. _release-1.8.2: Scrapy 1.8.2 (2022-03-01) @@ -1907,6 +1920,17 @@ Scrapy 1.8.2 (2022-03-01) your cookies. See the documentation of the :class:`~scrapy.http.Request` class for more information. +- When the domain of a cookie, either received in the ``Set-Cookie`` header + of a response or defined in a :class:`~scrapy.http.Request` object, is set + to a `public suffix `_, the cookie is now + ignored unless the cookie domain is the same as the request domain. + + The old behavior could be exploited by an attacker to inject cookies into + your requests to some other domains. Please, see the `mfjm-vh54-3f96 + security advisory`_ for more information. + + .. _mfjm-vh54-3f96 security advisory: https://github.com/scrapy/scrapy/security/advisories/GHSA-mfjm-vh54-3f96 + .. _release-1.8.1: diff --git a/scrapy/downloadermiddlewares/cookies.py b/scrapy/downloadermiddlewares/cookies.py index 0eee8d758..3afa06077 100644 --- a/scrapy/downloadermiddlewares/cookies.py +++ b/scrapy/downloadermiddlewares/cookies.py @@ -1,15 +1,26 @@ import logging from collections import defaultdict +from tldextract import TLDExtract + from scrapy.exceptions import NotConfigured from scrapy.http import Response from scrapy.http.cookies import CookieJar +from scrapy.utils.httpobj import urlparse_cached from scrapy.utils.python import to_unicode logger = logging.getLogger(__name__) +_split_domain = TLDExtract(include_psl_private_domains=True) + + +def _is_public_domain(domain): + parts = _split_domain(domain) + return not parts.domain + + class CookiesMiddleware: """This middleware enables working with sites that need cookies""" @@ -23,14 +34,29 @@ class CookiesMiddleware: raise NotConfigured return cls(crawler.settings.getbool('COOKIES_DEBUG')) + def _process_cookies(self, cookies, *, jar, request): + for cookie in cookies: + cookie_domain = cookie.domain + if cookie_domain.startswith('.'): + cookie_domain = cookie_domain[1:] + + request_domain = urlparse_cached(request).hostname.lower() + + if cookie_domain and _is_public_domain(cookie_domain): + if cookie_domain != request_domain: + continue + cookie.domain = request_domain + + jar.set_cookie_if_ok(cookie, request) + def process_request(self, request, spider): if request.meta.get('dont_merge_cookies', False): return cookiejarkey = request.meta.get("cookiejar") jar = self.jars[cookiejarkey] - for cookie in self._get_request_cookies(jar, request): - jar.set_cookie_if_ok(cookie, request) + cookies = self._get_request_cookies(jar, request) + self._process_cookies(cookies, jar=jar, request=request) # set Cookie header request.headers.pop('Cookie', None) @@ -44,7 +70,9 @@ class CookiesMiddleware: # extract cookies from Set-Cookie and drop invalid/expired cookies cookiejarkey = request.meta.get("cookiejar") jar = self.jars[cookiejarkey] - jar.extract_cookies(response, request) + cookies = jar.make_cookies(response, request) + self._process_cookies(cookies, jar=jar, request=request) + self._debug_set_cookie(response, spider) return response diff --git a/setup.py b/setup.py index 3a6ff2836..d86c0f285 100644 --- a/setup.py +++ b/setup.py @@ -32,6 +32,7 @@ install_requires = [ 'protego>=0.1.15', 'itemadapter>=0.1.0', 'setuptools', + 'tldextract', ] extras_require = {} cpython_dependencies = [ diff --git a/tests/test_downloadermiddleware_cookies.py b/tests/test_downloadermiddleware_cookies.py index 1747f3b94..ba7453255 100644 --- a/tests/test_downloadermiddleware_cookies.py +++ b/tests/test_downloadermiddleware_cookies.py @@ -15,6 +15,48 @@ from scrapy.utils.python import to_bytes from scrapy.utils.test import get_crawler +def _cookie_to_set_cookie_value(cookie): + """Given a cookie defined as a dictionary with name and value keys, and + optional path and domain keys, return the equivalent string that can be + associated to a ``Set-Cookie`` header.""" + decoded = {} + for key in ("name", "value", "path", "domain"): + if cookie.get(key) is None: + if key in ("name", "value"): + return + continue + if isinstance(cookie[key], (bool, float, int, str)): + decoded[key] = str(cookie[key]) + else: + try: + decoded[key] = cookie[key].decode("utf8") + except UnicodeDecodeError: + decoded[key] = cookie[key].decode("latin1", errors="replace") + + cookie_str = f"{decoded.pop('name')}={decoded.pop('value')}" + for key, value in decoded.items(): # path, domain + cookie_str += f"; {key.capitalize()}={value}" + return cookie_str + + +def _cookies_to_set_cookie_list(cookies): + """Given a group of cookie defined either as a dictionary or as a list of + dictionaries (i.e. in a format supported by the cookies parameter of + Request), return the equivalen list of strings that can be associated to a + ``Set-Cookie`` header.""" + if not cookies: + return [] + if isinstance(cookies, dict): + cookies = ({"name": k, "value": v} for k, v in cookies.items()) + return filter( + None, + ( + _cookie_to_set_cookie_value(cookie) + for cookie in cookies + ) + ) + + class CookiesMiddlewareTest(TestCase): def assertCookieValEqual(self, first, second, msg=None): @@ -523,3 +565,131 @@ class CookiesMiddlewareTest(TestCase): {'url': 'https://example.com', 'status': 302}, cookies2=False, ) + + def _test_user_set_cookie_domain_followup( + self, + url1, + url2, + domain, + *, + cookies1, + cookies2, + ): + input_cookies = [ + { + 'name': 'a', + 'value': 'b', + 'domain': domain, + } + ] + + request1 = Request(url1, cookies=input_cookies) + self.mw.process_request(request1, self.spider) + cookies = request1.headers.get('Cookie') + self.assertEqual(cookies, b"a=b" if cookies1 else None) + + request2 = Request(url2) + self.mw.process_request(request2, self.spider) + cookies = request2.headers.get('Cookie') + self.assertEqual(cookies, b"a=b" if cookies2 else None) + + def test_user_set_cookie_domain_suffix_private(self): + self._test_user_set_cookie_domain_followup( + 'https://books.toscrape.com', + 'https://quotes.toscrape.com', + 'toscrape.com', + cookies1=True, + cookies2=True, + ) + + def test_user_set_cookie_domain_suffix_public_period(self): + self._test_user_set_cookie_domain_followup( + 'https://foo.co.uk', + 'https://bar.co.uk', + 'co.uk', + cookies1=False, + cookies2=False, + ) + + def test_user_set_cookie_domain_suffix_public_private(self): + self._test_user_set_cookie_domain_followup( + 'https://foo.blogspot.com', + 'https://bar.blogspot.com', + 'blogspot.com', + cookies1=False, + cookies2=False, + ) + + def test_user_set_cookie_domain_public_period(self): + self._test_user_set_cookie_domain_followup( + 'https://co.uk', + 'https://co.uk', + 'co.uk', + cookies1=True, + cookies2=True, + ) + + def _test_server_set_cookie_domain_followup( + self, + url1, + url2, + domain, + *, + cookies, + ): + request1 = Request(url1) + self.mw.process_request(request1, self.spider) + + input_cookies = [ + { + 'name': 'a', + 'value': 'b', + 'domain': domain, + } + ] + + headers = { + 'Set-Cookie': _cookies_to_set_cookie_list(input_cookies), + } + response = Response(url1, status=200, headers=headers) + self.assertEqual( + self.mw.process_response(request1, response, self.spider), + response, + ) + + request2 = Request(url2) + self.mw.process_request(request2, self.spider) + actual_cookies = request2.headers.get('Cookie') + self.assertEqual(actual_cookies, b"a=b" if cookies else None) + + def test_server_set_cookie_domain_suffix_private(self): + self._test_server_set_cookie_domain_followup( + 'https://books.toscrape.com', + 'https://quotes.toscrape.com', + 'toscrape.com', + cookies=True, + ) + + def test_server_set_cookie_domain_suffix_public_period(self): + self._test_server_set_cookie_domain_followup( + 'https://foo.co.uk', + 'https://bar.co.uk', + 'co.uk', + cookies=False, + ) + + def test_server_set_cookie_domain_suffix_public_private(self): + self._test_server_set_cookie_domain_followup( + 'https://foo.blogspot.com', + 'https://bar.blogspot.com', + 'blogspot.com', + cookies=False, + ) + + def test_server_set_cookie_domain_public_period(self): + self._test_server_set_cookie_domain_followup( + 'https://co.uk', + 'https://co.uk', + 'co.uk', + cookies=True, + )