fix(redis): Don't SETEX with a non-positive TTL

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)

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

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.