diff --git a/cachecontrol/caches/redis_cache.py b/cachecontrol/caches/redis_cache.py index f859e719..55a56d0d 100644 --- a/cachecontrol/caches/redis_cache.py +++ b/cachecontrol/caches/redis_cache.py @@ -23,16 +23,21 @@ def get(self, key: str) -> bytes | None: def set( self, key: str, value: bytes, expires: int | datetime | None = None ) -> None: + """Store ``value``, optionally expiring it after ``expires``. + """ if not expires: self.conn.set(key, value) - elif isinstance(expires, datetime): + return + + if isinstance(expires, datetime): now_utc = datetime.now(timezone.utc) if expires.tzinfo is None: now_utc = now_utc.replace(tzinfo=None) - delta = expires - now_utc - self.conn.setex(key, int(delta.total_seconds()), value) + ttl = int((expires - now_utc).total_seconds()) else: - self.conn.setex(key, expires, value) + ttl = expires + + self.conn.setex(key, ttl, value) def delete(self, key: str) -> None: self.conn.delete(key) diff --git a/cachecontrol/controller.py b/cachecontrol/controller.py index 03b22185..d60cf7f2 100644 --- a/cachecontrol/controller.py +++ b/cachecontrol/controller.py @@ -301,9 +301,14 @@ def _cache_set( body: bytes | None = None, expires_time: int | None = None, ) -> None: + """Store the data in the cache. """ - Store the data in the cache. - """ + if expires_time is not None and expires_time <= 0: + # Already stale on arrival + logger.debug("Purging cached response: expires in the past") + self.cache.delete(cache_url) + return + if isinstance(self.cache, SeparateBodyBaseCache): # We pass in the body separately; just put a placeholder empty # string in the metadata. @@ -452,11 +457,15 @@ def cache_response( elif "expires" in response_headers: if response_headers["expires"]: expires = parsedate_tz(response_headers["expires"]) - if expires is not None: - expires_time = calendar.timegm(expires[:6]) - date + if expires is None: + # https://tools.ietf.org/html/rfc9111#section-5.3: an + # invalid Expires must be read as a time in the past. + expires_time = 0 else: - expires_time = None + expires_time = calendar.timegm(expires[:6]) - date + # A non-positive lifetime here means the response arrived + # stale logger.debug( "Caching b/c of expires header. expires in {} seconds".format( expires_time diff --git a/tests/test_cache_control.py b/tests/test_cache_control.py index 9d3d4d8e..ae282a75 100644 --- a/tests/test_cache_control.py +++ b/tests/test_cache_control.py @@ -119,6 +119,67 @@ def test_cache_response_no_store_with_etag(self, cc): assert not cc.cache.set.called + def test_cache_response_expires_in_future(self, cc): + now = time.time() + resp = self.resp( + { + "date": time.strftime(TIME_FMT, time.gmtime(now)), + "expires": time.strftime(TIME_FMT, time.gmtime(now + 3600)), + } + ) + cc.cache_response(self.req(), resp) + + cc.cache.set.assert_called_with(self.url, ANY, expires=3600) + + @pytest.mark.parametrize( + "expires", + [ + # RFC 9111 4.2.1: freshness lifetime is Expires - Date, which the + # origin is free to make negative to force revalidation. + "past", + # RFC 9111 5.3: an invalid Expires means "already expired". + "0", + "garbage", + ], + ) + def test_cache_response_expires_in_past_not_cached(self, cc, expires): + now = time.time() + if expires == "past": + expires = time.strftime(TIME_FMT, time.gmtime(now - 3600)) + resp = self.resp( + {"date": time.strftime(TIME_FMT, time.gmtime(now)), "expires": expires} + ) + cc.cache_response(self.req(), resp) + + assert not cc.cache.set.called + + def test_cache_response_expires_in_past_purges_existing_entry(self): + now = time.time() + cache = DictCache({self.url: b"stale"}) + cc = CacheController(cache, serializer=Mock()) + + resp = self.resp( + { + "date": time.strftime(TIME_FMT, time.gmtime(now)), + "expires": time.strftime(TIME_FMT, time.gmtime(now - 3600)), + } + ) + cc.cache_response(self.req(), resp) + + assert cc.cache.get(self.url) is None + + @pytest.mark.parametrize("expires_time", [0, -1, -3600]) + def test_cache_set_never_passes_non_positive_expires(self, cc, expires_time): + """``_cache_set`` is the only chokepoint into ``BaseCache.set``. + + Backends may therefore assume ``expires`` is either None or strictly + positive; a non-positive deadline must purge instead of store. + """ + cc._cache_set(self.url, self.req(), self.resp(), b"testing", expires_time) + + assert not cc.cache.set.called + cc.cache.delete.assert_called_with(self.url) + def test_no_cache_with_vary_star(self, cc): # Vary: * indicates that the response can never be served # from the cache, so storing it can be avoided.