diff --git a/CHANGES.md b/CHANGES.md index 84709f1..27879a0 100644 --- a/CHANGES.md +++ b/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 diff --git a/README.md b/README.md index 425919e..cb8d031 100644 --- a/README.md +++ b/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. diff --git a/src/zodb_pgjsonb_thumborblobloader/auth_handler.py b/src/zodb_pgjsonb_thumborblobloader/auth_handler.py index d713eb7..a11261c 100644 --- a/src/zodb_pgjsonb_thumborblobloader/auth_handler.py +++ b/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: diff --git a/tests/test_auth_handler.py b/tests/test_auth_handler.py index 7e344b9..6a9c318 100644 --- a/tests/test_auth_handler.py +++ b/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.""" diff --git a/thumbor.conf b/thumbor.conf index 26e15b1..c3fb420 100644 --- a/thumbor.conf +++ b/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" +)