From 0afd1a6d9588c1b7777c159d6b94dc03495a6ab4 Mon Sep 17 00:00:00 2001 From: Alexis Date: Sun, 2 Aug 2026 15:18:20 +0200 Subject: [PATCH 1/2] fix(redis): Don't SETEX with a non-positive TTL --- cachecontrol/caches/redis_cache.py | 14 +++++-- tests/test_storage_redis.py | 63 ++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/cachecontrol/caches/redis_cache.py b/cachecontrol/caches/redis_cache.py index f859e719..61f9174a 100644 --- a/cachecontrol/caches/redis_cache.py +++ b/cachecontrol/caches/redis_cache.py @@ -25,14 +25,20 @@ def set( ) -> None: 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: + ttl = expires + + if ttl <= 0: + self.conn.delete(key) else: - self.conn.setex(key, expires, value) + self.conn.setex(key, ttl, value) def delete(self, key: str) -> None: self.conn.delete(key) diff --git a/tests/test_storage_redis.py b/tests/test_storage_redis.py index d1c64b45..0e937e6c 100644 --- a/tests/test_storage_redis.py +++ b/tests/test_storage_redis.py @@ -2,25 +2,64 @@ # # SPDX-License-Identifier: Apache-2.0 -from datetime import datetime, timezone -from unittest.mock import Mock +from datetime import datetime, timedelta, timezone + +import pytest +from redis.exceptions import ResponseError from cachecontrol.caches import RedisCache +class FakeRedis: + def __init__(self) -> None: + self.values: dict[str, bytes] = {} + self.ttls: dict[str, int] = {} + + def set(self, key: str, value: bytes) -> None: + self.values[key] = value + self.ttls.pop(key, None) + + def setex(self, key: str, seconds: int, value: bytes) -> None: + if seconds <= 0: + raise ResponseError("invalid expire time in 'setex' command") + self.values[key] = value + self.ttls[key] = seconds + + def get(self, key: str) -> bytes | None: + return self.values.get(key) + + def delete(self, key: str) -> None: + self.values.pop(key, None) + self.ttls.pop(key, None) + + +ONE_HOUR = timedelta(hours=1) + + class TestRedisCache: def setup_method(self): - self.conn = Mock() + self.conn = FakeRedis() self.cache = RedisCache(self.conn) - def test_set_expiration_datetime(self): - self.cache.set("foo", "bar", expires=datetime(2014, 2, 2)) - assert self.conn.setex.called + @pytest.mark.parametrize("tzinfo", [None, timezone.utc], ids=["naive", "aware"]) + @pytest.mark.parametrize( + "offset, expected", + [(ONE_HOUR, b"bar"), (-ONE_HOUR, None)], + ids=["future", "past"], + ) + def test_set_expiration_datetime(self, tzinfo, offset, expected): + """A deadline already in the past must not reach SETEX.""" + expires = datetime.now(timezone.utc).replace(tzinfo=tzinfo) + offset + + self.cache.set("foo", b"bar", expires=expires) + + assert self.conn.get("foo") == expected - def test_set_expiration_datetime_aware(self): - self.cache.set("foo", "bar", expires=datetime(2014, 2, 2, tzinfo=timezone.utc)) - assert self.conn.setex.called + @pytest.mark.parametrize( + "expires, expected", [(600, b"bar"), (-600, None)], ids=["positive", "negative"] + ) + def test_set_expiration_int(self, expires, expected): + """controller.py computes ``Expires - Date``, which can go negative.""" + self.cache.set("foo", b"bar", expires=expires) - def test_set_expiration_int(self): - self.cache.set("foo", "bar", expires=600) - assert self.conn.setex.called + assert self.conn.get("foo") == expected From 55d730c0cc0006c57b8a4c2ad0a992c5bc257fb1 Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 4 Aug 2026 15:36:11 +0200 Subject: [PATCH 2/2] Move limit to controller --- cachecontrol/caches/redis_cache.py | 7 ++-- cachecontrol/controller.py | 19 ++++++--- tests/test_cache_control.py | 61 +++++++++++++++++++++++++++++ tests/test_storage_redis.py | 63 ++++++------------------------ 4 files changed, 90 insertions(+), 60 deletions(-) diff --git a/cachecontrol/caches/redis_cache.py b/cachecontrol/caches/redis_cache.py index 61f9174a..55a56d0d 100644 --- a/cachecontrol/caches/redis_cache.py +++ b/cachecontrol/caches/redis_cache.py @@ -23,6 +23,8 @@ 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) return @@ -35,10 +37,7 @@ def set( else: ttl = expires - if ttl <= 0: - self.conn.delete(key) - else: - self.conn.setex(key, ttl, value) + 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. diff --git a/tests/test_storage_redis.py b/tests/test_storage_redis.py index 0e937e6c..d1c64b45 100644 --- a/tests/test_storage_redis.py +++ b/tests/test_storage_redis.py @@ -2,64 +2,25 @@ # # SPDX-License-Identifier: Apache-2.0 -from datetime import datetime, timedelta, timezone - -import pytest -from redis.exceptions import ResponseError +from datetime import datetime, timezone +from unittest.mock import Mock from cachecontrol.caches import RedisCache -class FakeRedis: - def __init__(self) -> None: - self.values: dict[str, bytes] = {} - self.ttls: dict[str, int] = {} - - def set(self, key: str, value: bytes) -> None: - self.values[key] = value - self.ttls.pop(key, None) - - def setex(self, key: str, seconds: int, value: bytes) -> None: - if seconds <= 0: - raise ResponseError("invalid expire time in 'setex' command") - self.values[key] = value - self.ttls[key] = seconds - - def get(self, key: str) -> bytes | None: - return self.values.get(key) - - def delete(self, key: str) -> None: - self.values.pop(key, None) - self.ttls.pop(key, None) - - -ONE_HOUR = timedelta(hours=1) - - class TestRedisCache: def setup_method(self): - self.conn = FakeRedis() + self.conn = Mock() self.cache = RedisCache(self.conn) - @pytest.mark.parametrize("tzinfo", [None, timezone.utc], ids=["naive", "aware"]) - @pytest.mark.parametrize( - "offset, expected", - [(ONE_HOUR, b"bar"), (-ONE_HOUR, None)], - ids=["future", "past"], - ) - def test_set_expiration_datetime(self, tzinfo, offset, expected): - """A deadline already in the past must not reach SETEX.""" - expires = datetime.now(timezone.utc).replace(tzinfo=tzinfo) + offset - - self.cache.set("foo", b"bar", expires=expires) - - assert self.conn.get("foo") == expected + def test_set_expiration_datetime(self): + self.cache.set("foo", "bar", expires=datetime(2014, 2, 2)) + assert self.conn.setex.called - @pytest.mark.parametrize( - "expires, expected", [(600, b"bar"), (-600, None)], ids=["positive", "negative"] - ) - def test_set_expiration_int(self, expires, expected): - """controller.py computes ``Expires - Date``, which can go negative.""" - self.cache.set("foo", b"bar", expires=expires) + def test_set_expiration_datetime_aware(self): + self.cache.set("foo", "bar", expires=datetime(2014, 2, 2, tzinfo=timezone.utc)) + assert self.conn.setex.called - assert self.conn.get("foo") == expected + def test_set_expiration_int(self): + self.cache.set("foo", "bar", expires=600) + assert self.conn.setex.called