Merge pull request #8 from bluedynamics/fix/no-cache-error-responses

fix: microcache error responses instead of pinning long-TTL (closes #5)

Author: jensens

CHANGES.md

@@ -2,6 +2,20 @@
 
 ## 0.4.3 (unreleased)
 
+- Fix: `AuthImagingHandler.finish()` no longer applies the long-TTL
+  `Cache-Control` override to error responses (4xx/5xx).  Previously a
+  transient Thumbor 400 (e.g. a PIL decompression-bomb rejection) would
+  inherit `public, max-age=31536000, immutable` and get pinned in
+  downstream HTTP caches (Varnish, CDN) for a year — a single bad fetch
+  persistently hid the image on every shard that cached it.
+  Errors now get a short microcache (`PGTHUMBOR_CACHE_CONTROL_ERROR`,
+  default `public, max-age=10`) instead: decouples transient errors
+  from long-term cache poisoning AND lets downstream caches absorb
+  request floods for broken URLs (cheap DoS amplification defense —
+  a single bad URL won't fan out to one Thumbor hit per request).
+  3xx responses are left untouched (Thumbor default).
+  Fixes [#5](https://github.com/bluedynamics/zodb-pgjsonb-thumborblobloader/issues/5).
+
 - Fix: boto3 S3 client is now created with an explicit
   `max_pool_connections` via `botocore.Config` (default 50, overridable
   via `PGTHUMBOR_S3_MAX_POOL_CONNECTIONS`).  The boto3 default of 10

README.md

@@ -106,6 +106,7 @@ The image is automatically rebuilt weekly when a new Thumbor version appears on
 | `PGTHUMBOR_AUTH_CACHE_TTL` | `60` | Auth cache TTL in seconds |
 | `PGTHUMBOR_CACHE_CONTROL_AUTHENTICATED` | `private, max-age=86400` | Cache-Control for authenticated images (browser-only, no proxy caching) |
 | `PGTHUMBOR_CACHE_CONTROL_PUBLIC` | `""` | Cache-Control for public images (empty = Thumbor default) |
+| `PGTHUMBOR_CACHE_CONTROL_ERROR` | `public, max-age=10` | Cache-Control for 4xx/5xx responses — microcache decouples transient errors from long-term cache poisoning and absorbs request floods for broken URLs |
 
 The Plone auth handler (and Cache-Control overrides) is only loaded when `PGTHUMBOR_PLONE_AUTH_URL` is set.
 

src/zodb_pgjsonb_thumborblobloader/auth_handler.py

@@ -70,9 +70,28 @@ async def get(self, **kwargs):
         await super().get(**kwargs)
 
     def finish(self, *args, **kwargs):
+        # Only apply the long-TTL Cache-Control override on success.
+        # Otherwise a transient 4xx/5xx from Thumbor (e.g. a PIL
+        # decompression-bomb 400) would get pinned in downstream HTTP
+        # caches for the full max-age window, hiding the image for a year.
+        #
+        # Errors get a short "microcache" TTL instead of no-store: it
+        # decouples transient errors from long-term cache poisoning,
+        # *and* it absorbs request floods for the same broken URL —
+        # downstream caches serve the error themselves for the next
+        # few seconds instead of each request hitting Thumbor. Cheap
+        # DoS amplification defense on top of the primary fix.
+        status = self.get_status()
         cc = getattr(self, "_cache_control_override", "")
-        if cc:
-            self.set_header("Cache-Control", cc)
+        if 200 <= status < 300:
+            if cc:
+                self.set_header("Cache-Control", cc)
+        elif status >= 400:
+            error_cc = self.context.config.get(
+                "PGTHUMBOR_CACHE_CONTROL_ERROR",
+                "public, max-age=10",
+            )
+            self.set_header("Cache-Control", error_cc)
         super().finish(*args, **kwargs)
 
     def _extract_content_zoid(self) -> str | None:

tests/test_auth_handler.py

@@ -228,6 +228,8 @@ def _make_handler(
         path="/hmac/300x200/42/ff/1a",
         cc_auth="private, max-age=86400",
         cc_public="",
+        cc_error="public, max-age=10",
+        status=200,
     ):
         from zodb_pgjsonb_thumborblobloader.auth_handler import _auth_cache
         from zodb_pgjsonb_thumborblobloader.auth_handler import AuthImagingHandler
@@ -239,13 +241,17 @@ def _make_handler(
         handler.request.path = path
         handler.request.headers = {"Cookie": "auth=abc123"}
         handler.context = MagicMock()
-        handler.context.config.get = lambda key, default=None: {
+        config = {
             "PGTHUMBOR_PLONE_AUTH_URL": "http://plone:8080/Plone",
             "PGTHUMBOR_AUTH_CACHE_TTL": 60,
             "PGTHUMBOR_CACHE_CONTROL_AUTHENTICATED": cc_auth,
             "PGTHUMBOR_CACHE_CONTROL_PUBLIC": cc_public,
-        }.get(key, default)
+        }
+        if cc_error is not None:
+            config["PGTHUMBOR_CACHE_CONTROL_ERROR"] = cc_error
+        handler.context.config.get = lambda key, default=None: config.get(key, default)
         handler._headers = {}
+        handler.get_status = lambda: status
         return handler
 
     def test_authenticated_request_sets_private(self):
@@ -322,6 +328,78 @@ def test_finish_skips_header_when_empty_override(self):
 
         assert "Cache-Control" not in headers_set
 
+    def test_finish_error_status_gets_microcache(self):
+        """4xx response gets a short microcache, not the long-TTL override."""
+        handler = self._make_handler(status=400)
+        handler._cache_control_override = "public, max-age=31536000, immutable"
+        headers_set = {}
+        handler.set_header = lambda k, v: headers_set.update({k: v})
+
+        with patch.object(
+            type(handler).__mro__[1], "finish", lambda self, *a, **kw: None
+        ):
+            handler.finish()
+
+        # The long-TTL override MUST NOT leak into the error response.
+        assert headers_set["Cache-Control"] == "public, max-age=10"
+
+    def test_finish_error_without_override_still_gets_microcache(self):
+        """Error responses get a microcache even if no override was set."""
+        handler = self._make_handler(status=404)
+        # No _cache_control_override attribute set at all
+        headers_set = {}
+        handler.set_header = lambda k, v: headers_set.update({k: v})
+
+        with patch.object(
+            type(handler).__mro__[1], "finish", lambda self, *a, **kw: None
+        ):
+            handler.finish()
+
+        assert headers_set["Cache-Control"] == "public, max-age=10"
+
+    def test_finish_5xx_also_microcached(self):
+        """5xx responses get the same microcache treatment as 4xx."""
+        handler = self._make_handler(status=503)
+        handler._cache_control_override = "public, max-age=31536000, immutable"
+        headers_set = {}
+        handler.set_header = lambda k, v: headers_set.update({k: v})
+
+        with patch.object(
+            type(handler).__mro__[1], "finish", lambda self, *a, **kw: None
+        ):
+            handler.finish()
+
+        assert headers_set["Cache-Control"] == "public, max-age=10"
+
+    def test_finish_error_microcache_configurable(self):
+        """PGTHUMBOR_CACHE_CONTROL_ERROR overrides the default microcache."""
+        handler = self._make_handler(
+            status=400, cc_error="private, max-age=30, must-revalidate"
+        )
+        headers_set = {}
+        handler.set_header = lambda k, v: headers_set.update({k: v})
+
+        with patch.object(
+            type(handler).__mro__[1], "finish", lambda self, *a, **kw: None
+        ):
+            handler.finish()
+
+        assert headers_set["Cache-Control"] == "private, max-age=30, must-revalidate"
+
+    def test_finish_3xx_leaves_cache_control_alone(self):
+        """Redirects: no long-TTL override, no microcache — Thumbor default."""
+        handler = self._make_handler(status=304)
+        handler._cache_control_override = "public, max-age=31536000, immutable"
+        headers_set = {}
+        handler.set_header = lambda k, v: headers_set.update({k: v})
+
+        with patch.object(
+            type(handler).__mro__[1], "finish", lambda self, *a, **kw: None
+        ):
+            handler.finish()
+
+        assert "Cache-Control" not in headers_set
+
 
 class TestGetHandlers:
     """Test get_handlers() returns correct URL pattern."""

thumbor.conf

@@ -58,3 +58,9 @@ PGTHUMBOR_CACHE_CONTROL_AUTHENTICATED = os.environ.get(
 PGTHUMBOR_CACHE_CONTROL_PUBLIC = os.environ.get(
     "PGTHUMBOR_CACHE_CONTROL_PUBLIC", ""
 )
+# 4xx/5xx responses: microcache (default 10s) so a transient error is not
+# pinned in downstream HTTP caches for the full max-age of the success path,
+# and so downstream caches absorb request floods for broken URLs.
+PGTHUMBOR_CACHE_CONTROL_ERROR = os.environ.get(
+    "PGTHUMBOR_CACHE_CONTROL_ERROR", "public, max-age=10"
+)