Fix handling of expired cookies so they are not stored in CookieJar (#…

…4066)


Co-Authored-By: Martijn Pieters <[email protected]>

CHANGES/4063.bugfix

@@ -0,0 +1 @@
+Fix handling of expired cookies so they are not stored in CookieJar.

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

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"] = ""

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 = ""
 

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

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):

tests/test_cookiejar.py

@@ -7,13 +7,40 @@
 from unittest import mock
 
 import pytest
+from freezegun import freeze_time
 from yarl import URL
 
 from aiohttp import CookieJar, DummyCookieJar
 
 
 @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()