From d65f5cb4751651c7c43ac3744c92cbd4b23eec04 Mon Sep 17 00:00:00 2001 From: Alan Tse Date: Wed, 25 Sep 2019 01:21:45 -0700 Subject: [PATCH] Fix handling of expired cookies so they are not stored in CookieJar (#4066) Co-Authored-By: Martijn Pieters --- CHANGES/4063.bugfix | 1 + CONTRIBUTORS.txt | 1 + aiohttp/cookiejar.py | 38 ++++++++++------ aiohttp/helpers.py | 10 ++++ requirements/ci.txt | 1 + tests/test_client_functional.py | 81 +++++++++++++++++++++++++++++++++ tests/test_cookiejar.py | 52 ++++++++++++++++++++- 7 files changed, 168 insertions(+), 16 deletions(-) create mode 100644 CHANGES/4063.bugfix diff --git a/CHANGES/4063.bugfix b/CHANGES/4063.bugfix new file mode 100644 index 00000000000..4a7e73b51b6 --- /dev/null +++ b/CHANGES/4063.bugfix @@ -0,0 +1 @@ +Fix handling of expired cookies so they are not stored in CookieJar. diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 0f474de6b04..8b8594bd774 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -4,6 +4,7 @@ A. Jesse Jiryu Davis Adam Cooper Adam Mills Adrián Chaves +Alan Tse Alec Hanefeld Alejandro Gómez Aleksandr Danshyn diff --git a/aiohttp/cookiejar.py b/aiohttp/cookiejar.py index 4b874fd4fe2..ac102bfc487 100644 --- a/aiohttp/cookiejar.py +++ b/aiohttp/cookiejar.py @@ -6,7 +6,6 @@ import warnings from collections import defaultdict from http.cookies import BaseCookie, Morsel, SimpleCookie # noqa -from math import ceil from typing import ( # noqa DefaultDict, Dict, @@ -23,7 +22,7 @@ from yarl import URL from .abc import AbstractCookieJar -from .helpers import get_running_loop, is_ip_address +from .helpers import get_running_loop, is_ip_address, next_whole_second from .typedefs import LooseCookies, PathLike __all__ = ('CookieJar', 'DummyCookieJar') @@ -48,15 +47,16 @@ class CookieJar(AbstractCookieJar): DATE_YEAR_RE = re.compile(r"(\d{2,4})") - MAX_TIME = 2051215261.0 # so far in future (2035-01-01) + MAX_TIME = datetime.datetime.max.replace( + tzinfo=datetime.timezone.utc) def __init__(self, *, unsafe: bool=False) -> None: self._loop = get_running_loop() self._cookies = defaultdict(SimpleCookie) #type: DefaultDict[str, SimpleCookie] # noqa self._host_only_cookies = set() # type: Set[Tuple[str, str]] self._unsafe = unsafe - self._next_expiration = ceil(self._loop.time()) - self._expirations = {} # type: Dict[Tuple[str, str], int] + self._next_expiration = next_whole_second() + self._expirations = {} # type: Dict[Tuple[str, str], datetime.datetime] # noqa: E501 def save(self, file_path: PathLike) -> None: file_path = pathlib.Path(file_path) @@ -71,7 +71,7 @@ def load(self, file_path: PathLike) -> None: def clear(self) -> None: self._cookies.clear() self._host_only_cookies.clear() - self._next_expiration = ceil(self._loop.time()) + self._next_expiration = next_whole_second() self._expirations.clear() def __iter__(self) -> 'Iterator[Morsel[str]]': @@ -83,7 +83,7 @@ def __len__(self) -> int: return sum(1 for i in self) def _do_expiration(self) -> None: - now = self._loop.time() + now = datetime.datetime.now(datetime.timezone.utc) if self._next_expiration > now: return if not self._expirations: @@ -102,12 +102,16 @@ def _do_expiration(self) -> None: for key in to_del: del expirations[key] - self._next_expiration = ceil(next_expiration) + try: + self._next_expiration = (next_expiration.replace(microsecond=0) + + datetime.timedelta(seconds=1)) + except OverflowError: + self._next_expiration = self.MAX_TIME - def _expire_cookie(self, when: float, domain: str, name: str) -> None: - iwhen = int(when) - self._next_expiration = min(self._next_expiration, iwhen) - self._expirations[(domain, name)] = iwhen + def _expire_cookie(self, when: datetime.datetime, domain: str, name: str + ) -> None: + self._next_expiration = min(self._next_expiration, when) + self._expirations[(domain, name)] = when def update_cookies(self, cookies: LooseCookies, @@ -165,7 +169,13 @@ def update_cookies(self, if max_age: try: delta_seconds = int(max_age) - self._expire_cookie(self._loop.time() + delta_seconds, + try: + max_age_expiration = ( + datetime.datetime.now(datetime.timezone.utc) + + datetime.timedelta(seconds=delta_seconds)) + except OverflowError: + max_age_expiration = self.MAX_TIME + self._expire_cookie(max_age_expiration, domain, name) except ValueError: cookie["max-age"] = "" @@ -175,7 +185,7 @@ def update_cookies(self, if expires: expire_time = self._parse_date(expires) if expire_time: - self._expire_cookie(expire_time.timestamp(), + self._expire_cookie(expire_time, domain, name) else: cookie["expires"] = "" diff --git a/aiohttp/helpers.py b/aiohttp/helpers.py index a38a3bb01c9..c9c84038c2d 100644 --- a/aiohttp/helpers.py +++ b/aiohttp/helpers.py @@ -4,6 +4,7 @@ import base64 import binascii import cgi +import datetime import functools import inspect import netrc @@ -457,6 +458,15 @@ def is_ip_address( return is_ipv4_address(host) or is_ipv6_address(host) +def next_whole_second() -> datetime.datetime: + """Return current time rounded up to the next whole second.""" + return ( + datetime.datetime.now( + datetime.timezone.utc).replace(microsecond=0) + + datetime.timedelta(seconds=0) + ) + + _cached_current_datetime = None # type: Optional[int] _cached_formatted_datetime = "" diff --git a/requirements/ci.txt b/requirements/ci.txt index a79b9923abf..fd292e30e4d 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -1,6 +1,7 @@ setuptools-git==1.2 mypy==0.720; implementation_name=="cpython" mypy-extensions==0.4.1; implementation_name=="cpython" +freezegun==0.3.12 -r ci-wheel.txt -r doc.txt diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index 374d01809d0..ea380a0eb6f 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -1,6 +1,7 @@ """HTTP client functional tests against aiohttp.web server""" import asyncio +import datetime import http.cookies import io import json @@ -1983,6 +1984,86 @@ async def handler(request): mock.ANY) +async def test_set_cookies_expired(aiohttp_client) -> None: + + async def handler(request): + ret = web.Response() + ret.set_cookie('c1', 'cookie1') + ret.set_cookie('c2', 'cookie2') + ret.headers.add('Set-Cookie', + 'c3=cookie3; ' + 'HttpOnly; Path=/' + " Expires=Tue, 1 Jan 1980 12:00:00 GMT; ") + return ret + + app = web.Application() + app.router.add_get('/', handler) + client = await aiohttp_client(app) + + resp = await client.get('/') + assert 200 == resp.status + cookie_names = {c.key for c in client.session.cookie_jar} + assert cookie_names == {'c1', 'c2'} + resp.close() + + +async def test_set_cookies_max_age(aiohttp_client) -> None: + + async def handler(request): + ret = web.Response() + ret.set_cookie('c1', 'cookie1') + ret.set_cookie('c2', 'cookie2') + ret.headers.add('Set-Cookie', + 'c3=cookie3; ' + 'HttpOnly; Path=/' + " Max-Age=1; ") + return ret + + app = web.Application() + app.router.add_get('/', handler) + client = await aiohttp_client(app) + + resp = await client.get('/') + assert 200 == resp.status + cookie_names = {c.key for c in client.session.cookie_jar} + assert cookie_names == {'c1', 'c2', 'c3'} + await asyncio.sleep(2) + cookie_names = {c.key for c in client.session.cookie_jar} + assert cookie_names == {'c1', 'c2'} + resp.close() + + +async def test_set_cookies_max_age_overflow(aiohttp_client) -> None: + + async def handler(request): + ret = web.Response() + ret.headers.add('Set-Cookie', + 'overflow=overflow; ' + 'HttpOnly; Path=/' + " Max-Age=" + str(overflow) + "; ") + return ret + + overflow = int(datetime.datetime.max.replace( + tzinfo=datetime.timezone.utc).timestamp()) + empty = None + try: + empty = (datetime.datetime.now(datetime.timezone.utc) + + datetime.timedelta(seconds=overflow)) + except OverflowError as ex: + assert isinstance(ex, OverflowError) + assert not isinstance(empty, datetime.datetime) + app = web.Application() + app.router.add_get('/', handler) + client = await aiohttp_client(app) + + resp = await client.get('/') + assert 200 == resp.status + for cookie in client.session.cookie_jar: + if cookie.key == 'overflow': + assert int(cookie['max-age']) == int(overflow) + resp.close() + + async def test_request_conn_error() -> None: client = aiohttp.ClientSession() with pytest.raises(aiohttp.ClientConnectionError): diff --git a/tests/test_cookiejar.py b/tests/test_cookiejar.py index 80352934ad9..4c4b961e487 100644 --- a/tests/test_cookiejar.py +++ b/tests/test_cookiejar.py @@ -7,6 +7,7 @@ from unittest import mock import pytest +from freezegun import freeze_time from yarl import URL from aiohttp import CookieJar, DummyCookieJar @@ -14,6 +15,32 @@ @pytest.fixture def cookies_to_send(): + return SimpleCookie( + "shared-cookie=first; " + "domain-cookie=second; Domain=example.com; " + "subdomain1-cookie=third; Domain=test1.example.com; " + "subdomain2-cookie=fourth; Domain=test2.example.com; " + "dotted-domain-cookie=fifth; Domain=.example.com; " + "different-domain-cookie=sixth; Domain=different.org; " + "secure-cookie=seventh; Domain=secure.com; Secure; " + "no-path-cookie=eighth; Domain=pathtest.com; " + "path1-cookie=nineth; Domain=pathtest.com; Path=/; " + "path2-cookie=tenth; Domain=pathtest.com; Path=/one; " + "path3-cookie=eleventh; Domain=pathtest.com; Path=/one/two; " + "path4-cookie=twelfth; Domain=pathtest.com; Path=/one/two/; " + "expires-cookie=thirteenth; Domain=expirestest.com; Path=/;" + " Expires=Tue, 1 Jan 2039 12:00:00 GMT; " + "max-age-cookie=fourteenth; Domain=maxagetest.com; Path=/;" + " Max-Age=60; " + "invalid-max-age-cookie=fifteenth; Domain=invalid-values.com; " + " Max-Age=string; " + "invalid-expires-cookie=sixteenth; Domain=invalid-values.com; " + " Expires=string;" + ) + + +@pytest.fixture +def cookies_to_send_with_expired(): return SimpleCookie( "shared-cookie=first; " "domain-cookie=second; Domain=example.com; " @@ -143,6 +170,18 @@ async def test_constructor(loop, cookies_to_send, cookies_to_receive) -> None: assert jar._loop is loop +async def test_constructor_with_expired(loop, cookies_to_send_with_expired, + cookies_to_receive) -> None: + jar = CookieJar() + jar.update_cookies(cookies_to_send_with_expired) + jar_cookies = SimpleCookie() + for cookie in jar: + dict.__setitem__(jar_cookies, cookie.key, cookie) + expected_cookies = cookies_to_send_with_expired + assert jar_cookies != expected_cookies + assert jar._loop is loop + + async def test_save_load( tmp_path, loop, cookies_to_send, cookies_to_receive ) -> None: @@ -340,10 +379,19 @@ async def make_jar(): self.jar = self.loop.run_until_complete(make_jar()) def timed_request(self, url, update_time, send_time): - with mock.patch.object(self.loop, 'time', return_value=update_time): + if isinstance(update_time, int): + update_time = datetime.timedelta(seconds=update_time) + elif isinstance(update_time, float): + update_time = datetime.datetime.fromtimestamp(update_time) + if isinstance(send_time, int): + send_time = datetime.timedelta(seconds=send_time) + elif isinstance(send_time, float): + send_time = datetime.datetime.fromtimestamp(send_time) + + with freeze_time(update_time): self.jar.update_cookies(self.cookies_to_send) - with mock.patch.object(self.loop, 'time', return_value=send_time): + with freeze_time(send_time): cookies_sent = self.jar.filter_cookies(URL(url)) self.jar.clear()