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

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,
@@ -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 = 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: datetime.datetime = self._next_whole_second()
+        self._expirations: Dict[Tuple[str, str], datetime.datetime] = {}
 
     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 = self._next_whole_second()
         self._expirations.clear()
 
     def __iter__(self) -> 'Iterator[Morsel[str]]':
@@ -82,8 +82,16 @@ def __iter__(self) -> 'Iterator[Morsel[str]]':
     def __len__(self) -> int:
         return sum(1 for i in self)
 
+    def _next_whole_second(self) -> 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)
+        )
+
     def _do_expiration(self) -> None:
-        now = self._loop.time()
+        now: datetime.datetime = datetime.datetime.now(datetime.timezone.utc)
         if self._next_expiration > now:
             return
         if not self._expirations:
@@ -102,12 +110,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,8 +177,10 @@ def update_cookies(self,
             if max_age:
                 try:
                     delta_seconds = int(max_age)
-                    self._expire_cookie(self._loop.time() + delta_seconds,
-                                        domain, name)
+                    self._expire_cookie(datetime.datetime.now(
+                        datetime.timezone.utc) +
+                        datetime.timedelta(seconds=delta_seconds),
+                        domain, name)
                 except ValueError:
                     cookie["max-age"] = ""
 
@@ -175,8 +189,9 @@ def update_cookies(self,
                 if expires:
                     expire_time = self._parse_date(expires)
                     if expire_time:
-                        self._expire_cookie(expire_time.timestamp(),
-                                            domain, name)
+                        self._expire_cookie(datetime.datetime.fromtimestamp(
+                            expire_time.timestamp(), tz=datetime.timezone.utc),
+                            domain, name)
                     else:
                         cookie["expires"] = ""
 

requirements/dev.txt

@@ -1,3 +1,4 @@
 -r ci.txt
 -r towncrier.txt
 cherry_picker==1.3.2; python_version>="3.6"
+freezegun==0.3.12

tests/test_client_functional.py

@@ -1983,6 +1983,55 @@ 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_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()