diff --git a/CHANGES.md b/CHANGES.md index da69d97..df70a5f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,8 +2,32 @@ ## 0.6.5 (unreleased) +- Fix: SVG (skip-Thumbor) images no longer emit uid-based scale URLs that + permanently 404. Root cause was not `purge_scales` but the volatile + `ThumborScaleStorage` introduced in 0.6.x: `get_or_generate` reads a + fresh empty per-instance dict on every traversal, so *no* uid scale URL + could ever resolve — the `plone.scale` annotation is never consulted. + Fixed on both ends: skip-types now emit the original field URL with a + modification-time cache buster (both plone.namedfile code paths, the + legacy `__init__` for 7.x and `_scale_url` for >= 8.0.0a2), the HiDPI + `srcset` attribute and the `srcset()` method emit Thumbor URLs, and + `get_or_generate` heals legacy uid URLs (cached HTML, stale + `image_scales` catalog metadata) by parsing the deterministic + `{fieldname}-{width}-{md5}` uid and regenerating the info on the fly — + restricted to widths registered in `plone.allowed_sizes`. Review + hardening on top: srcset() mirrors the parent's edge-case guards + (zero-size original, original-size back-fill, unresolvable src scale), + and the HiDPI srcset path threads crop info through for scale infos + that carry a scale name. + Closes [#17](https://github.com/bluedynamics/plone-pgthumbor/issues/17). + - Add `cdk8s-plone` to the ecosystem navigation dropdown in the docs. +- Chore: apply ruff 0.16 markdown code-fence formatting to four docs files + (pre-existing drift; the QA workflow runs the latest ruff via uvx over + the whole repo). Mark up `zope2.Public` as inline code in a security-doc + heading so vale's Microsoft.Spacing rule no longer trips on it. + ## 0.6.4 (2026-04-20) - Fix: `_needs_auth_url()` no longer issues a PostgreSQL query per image. diff --git a/docs/sources/explanation/security.md b/docs/sources/explanation/security.md index 691a36e..d8e35be 100644 --- a/docs/sources/explanation/security.md +++ b/docs/sources/explanation/security.md @@ -140,7 +140,7 @@ method: The auth handler is registered via Thumbor's `HANDLER_LISTS` configuration: ```python -HANDLER_LISTS = ['zodb_pgjsonb_thumborblobloader.auth_handler'] +HANDLER_LISTS = ["zodb_pgjsonb_thumborblobloader.auth_handler"] ``` `get_handlers()` returns a list of `(url_regex, handler_class, context)` tuples @@ -206,7 +206,7 @@ Invalid or | Object not in catalog | 404 | Unknown object | | Database error | 503 | Service unavailable | -### Why zope2.Public permission +### Why `zope2.Public` permission The `@thumbor-auth` service is registered with `permission="zope2.Public"` -- it is accessible without authentication. diff --git a/docs/sources/how-to/configure-thumbor.md b/docs/sources/how-to/configure-thumbor.md index e20493b..d391fa9 100644 --- a/docs/sources/how-to/configure-thumbor.md +++ b/docs/sources/how-to/configure-thumbor.md @@ -52,7 +52,6 @@ HANDLER_LISTS = [ "thumbor.handler_lists.healthcheck", "zodb_pgjsonb_thumborblobloader.auth_handler", ] - ``` The healthcheck handler must come first so `/healthcheck` is matched before @@ -80,6 +79,7 @@ Can also be set via the `THUMBOR_SECURITY_KEY` environment variable: ```python import os + SECURITY_KEY = os.environ.get("THUMBOR_SECURITY_KEY", "") ``` @@ -137,6 +137,7 @@ Both settings can be configured via environment variables: ```python import os + AUTO_WEBP = os.environ.get("THUMBOR_AUTO_WEBP", "true").lower() in ("true", "1", "yes") AUTO_AVIF = os.environ.get("THUMBOR_AUTO_AVIF", "false").lower() in ("true", "1", "yes") ``` @@ -179,6 +180,7 @@ Can be configured via environment variable: ```python import os + _detectors = os.environ.get("THUMBOR_DETECTORS", "") if _detectors: DETECTORS = [d.strip() for d in _detectors.split(",") if d.strip()] @@ -227,6 +229,7 @@ Can be set via environment variable: ```python import os + PGTHUMBOR_DSN = os.environ.get("PGTHUMBOR_DSN", "") ``` @@ -271,6 +274,7 @@ Can be set via environment variable: ```python import os + PGTHUMBOR_PLONE_AUTH_URL = os.environ.get("PGTHUMBOR_PLONE_AUTH_URL", "") ``` diff --git a/docs/sources/how-to/write-crop-provider.md b/docs/sources/how-to/write-crop-provider.md index 3c9158b..b59fa8a 100644 --- a/docs/sources/how-to/write-crop-provider.md +++ b/docs/sources/how-to/write-crop-provider.md @@ -21,7 +21,6 @@ from zope.interface import implementer @implementer(ICropProvider) class MyCropProvider: - def __init__(self, context): self.context = context diff --git a/docs/superpowers/plans/2026-04-13-hosting-aware-thumbor-urls.md b/docs/superpowers/plans/2026-04-13-hosting-aware-thumbor-urls.md index 392d51e..e576159 100644 --- a/docs/superpowers/plans/2026-04-13-hosting-aware-thumbor-urls.md +++ b/docs/superpowers/plans/2026-04-13-hosting-aware-thumbor-urls.md @@ -108,9 +108,7 @@ class TestRootRelativeForm: def test_nested_path(self): assert ( - resolve_thumbor_prefix( - "/img/thumbor", "https://site-a.example/subsite" - ) + resolve_thumbor_prefix("/img/thumbor", "https://site-a.example/subsite") == "https://site-a.example/img/thumbor" ) @@ -180,9 +178,9 @@ class TestOutputShape: ["https://cdn.example/thumbor", "/thumbor", "thumbor"], ) def test_no_trailing_slash(self, server_url): - assert not resolve_thumbor_prefix( - server_url, "https://plone.example" - ).endswith("/") + assert not resolve_thumbor_prefix(server_url, "https://plone.example").endswith( + "/" + ) @pytest.mark.parametrize( "server_url,expected_prefix", @@ -253,9 +251,7 @@ def resolve_thumbor_prefix(server_url: str, nav_root_url: str) -> str: has a query / fragment, or is otherwise malformed. """ if not server_url or server_url.strip() != server_url: - raise ValueError( - "PGTHUMBOR_SERVER_URL is empty or has surrounding whitespace" - ) + raise ValueError("PGTHUMBOR_SERVER_URL is empty or has surrounding whitespace") if "?" in server_url or "#" in server_url: raise ValueError( "PGTHUMBOR_SERVER_URL must not contain a query string or fragment" @@ -410,30 +406,26 @@ Expected: the three `*_rejected` tests FAIL (no `ValueError` raised), the accept Modify [`src/plone/pgthumbor/config.py`](../../../src/plone/pgthumbor/config.py) — replace `__post_init__` with: ```python - def __post_init__(self): - from plone.pgthumbor.prefix import _ALLOWED_SCHEMES +def __post_init__(self): + from plone.pgthumbor.prefix import _ALLOWED_SCHEMES - server_url = self.server_url - if not server_url or server_url.strip() != server_url: - raise ValueError( - "PGTHUMBOR_SERVER_URL is empty or has surrounding whitespace" - ) - if "?" in server_url or "#" in server_url: + server_url = self.server_url + if not server_url or server_url.strip() != server_url: + raise ValueError("PGTHUMBOR_SERVER_URL is empty or has surrounding whitespace") + if "?" in server_url or "#" in server_url: + raise ValueError( + "PGTHUMBOR_SERVER_URL must not contain a query string or fragment" + ) + if server_url.startswith(_ALLOWED_SCHEMES): + from urllib.parse import urlsplit + + parts = urlsplit(server_url) + if not parts.netloc: raise ValueError( - "PGTHUMBOR_SERVER_URL must not contain a query string or " - "fragment" + f"PGTHUMBOR_SERVER_URL {server_url!r} has a scheme but no host" ) - if server_url.startswith(_ALLOWED_SCHEMES): - from urllib.parse import urlsplit - - parts = urlsplit(server_url) - if not parts.netloc: - raise ValueError( - f"PGTHUMBOR_SERVER_URL {server_url!r} has a scheme " - "but no host" - ) - # Normalise: strip trailing slash - object.__setattr__(self, "server_url", server_url.rstrip("/")) + # Normalise: strip trailing slash + object.__setattr__(self, "server_url", server_url.rstrip("/")) ``` - [ ] **Step 5: Run tests — all pass now** @@ -696,17 +688,16 @@ import pytest @pytest.mark.parametrize( "server_url, nav_root, expected_prefix", [ - ("https://cdn.example/thumbor", "https://site-a.example/2019", - "https://cdn.example/thumbor"), - ("/thumbor", "https://site-a.example/2019", - "https://site-a.example/thumbor"), - ("thumbor", "https://arch.example/2019", - "https://arch.example/2019/thumbor"), + ( + "https://cdn.example/thumbor", + "https://site-a.example/2019", + "https://cdn.example/thumbor", + ), + ("/thumbor", "https://site-a.example/2019", "https://site-a.example/thumbor"), + ("thumbor", "https://arch.example/2019", "https://arch.example/2019/thumbor"), ], ) -def test_url_uses_configured_prefix( - monkeypatch, server_url, nav_root, expected_prefix -): +def test_url_uses_configured_prefix(monkeypatch, server_url, nav_root, expected_prefix): from plone.pgthumbor import scaling as scaling_mod from plone.pgthumbor.scaling import ThumborImageScale @@ -717,9 +708,7 @@ def test_url_uses_configured_prefix( ) nav_root_mock = MagicMock() nav_root_mock.absolute_url.return_value = nav_root - monkeypatch.setattr( - scaling_mod, "get_navigation_root", lambda c: nav_root_mock - ) + monkeypatch.setattr(scaling_mod, "get_navigation_root", lambda c: nav_root_mock) ctx = MagicMock() ctx.absolute_url.return_value = f"{nav_root}/doc" @@ -794,9 +783,7 @@ def _make_adapter(monkeypatch, server_url, nav_root): ) nav_root_mock = MagicMock() nav_root_mock.absolute_url.return_value = nav_root - monkeypatch.setattr( - fa_mod, "get_navigation_root", lambda c: nav_root_mock - ) + monkeypatch.setattr(fa_mod, "get_navigation_root", lambda c: nav_root_mock) field = MagicMock() field.__name__ = "image" @@ -810,9 +797,7 @@ def _make_adapter(monkeypatch, server_url, nav_root): class TestScaleViewFromUrl: """The download value stored in image_scales metadata.""" - def test_absolute_thumbor_url_stored_as_signed_path_marker( - self, monkeypatch - ): + def test_absolute_thumbor_url_stored_as_signed_path_marker(self, monkeypatch): adapter = _make_adapter( monkeypatch, "https://cdn.example/thumbor", "https://plone.example" ) @@ -821,12 +806,8 @@ class TestScaleViewFromUrl: ) assert stored == "thumbor:/abc/300x200/fit-in/42/ff" - def test_root_relative_thumbor_url_stored_as_signed_path_marker( - self, monkeypatch - ): - adapter = _make_adapter( - monkeypatch, "/thumbor", "https://plone.example" - ) + def test_root_relative_thumbor_url_stored_as_signed_path_marker(self, monkeypatch): + adapter = _make_adapter(monkeypatch, "/thumbor", "https://plone.example") stored = adapter._scale_view_from_url( "https://plone.example/thumbor/abc/300x200/fit-in/42/ff" ) @@ -835,9 +816,7 @@ class TestScaleViewFromUrl: def test_nav_root_relative_thumbor_url_stored_as_signed_path_marker( self, monkeypatch ): - adapter = _make_adapter( - monkeypatch, "thumbor", "https://arch.example/2019" - ) + adapter = _make_adapter(monkeypatch, "thumbor", "https://arch.example/2019") stored = adapter._scale_view_from_url( "https://arch.example/2019/thumbor/abc/300x200/fit-in/42/ff" ) @@ -921,11 +900,9 @@ class ThumborImageScalesFieldAdapter(ImageFieldScales): cfg = get_thumbor_config() if cfg is not None: nav_root = get_navigation_root(self.context) - prefix = resolve_thumbor_prefix( - cfg.server_url, nav_root.absolute_url() - ) + prefix = resolve_thumbor_prefix(cfg.server_url, nav_root.absolute_url()) if url.startswith(prefix + "/") or url == prefix: - signed_path = url[len(prefix):] or "/" + signed_path = url[len(prefix) :] or "/" return f"{THUMBOR_MARKER}{signed_path}" return super()._scale_view_from_url(url) ``` @@ -973,11 +950,17 @@ KEY = "test-secret-key" def _make_brain(download, field="image", width=300, height=200): brain = MagicMock() brain.image_scales = { - field: [{"scales": {"preview": { - "download": download, - "width": width, - "height": height, - }}}] + field: [ + { + "scales": { + "preview": { + "download": download, + "width": width, + "height": height, + } + } + } + ] } brain.getURL.return_value = "https://plone.example/folder/doc" brain.Title = "An Image" @@ -1014,12 +997,21 @@ class TestThumborMarkerRendering: @pytest.mark.parametrize( "server_url, nav_root, expected_prefix", [ - ("https://cdn.example/thumbor", "https://plone.example", - "https://cdn.example/thumbor"), - ("/thumbor", "https://site-a.example/2019", - "https://site-a.example/thumbor"), - ("thumbor", "https://arch.example/2019", - "https://arch.example/2019/thumbor"), + ( + "https://cdn.example/thumbor", + "https://plone.example", + "https://cdn.example/thumbor", + ), + ( + "/thumbor", + "https://site-a.example/2019", + "https://site-a.example/thumbor", + ), + ( + "thumbor", + "https://arch.example/2019", + "https://arch.example/2019/thumbor", + ), ], ) def test_marker_gets_resolved_with_current_prefix( @@ -1028,9 +1020,7 @@ class TestThumborMarkerRendering: view = _make_view(monkeypatch, nav_root, server_url) brain = _make_brain("thumbor:/abc/300x200/fit-in/42/ff") - tag = view._tag_from_brain_image_scales( - brain, "image", scale="preview" - ) + tag = view._tag_from_brain_image_scales(brain, "image", scale="preview") assert tag is not None assert f'src="{expected_prefix}/abc/300x200/fit-in/42/ff"' in tag @@ -1044,26 +1034,17 @@ class TestLegacyDownloadRendering: ) brain = _make_brain("https://legacy.example/foo/bar.jpg") - tag = view._tag_from_brain_image_scales( - brain, "image", scale="preview" - ) + tag = view._tag_from_brain_image_scales(brain, "image", scale="preview") assert 'src="https://legacy.example/foo/bar.jpg"' in tag - def test_legacy_relative_download_concatenated_to_brain_url( - self, monkeypatch - ): + def test_legacy_relative_download_concatenated_to_brain_url(self, monkeypatch): view = _make_view( monkeypatch, "https://plone.example", "https://cdn.example/thumbor" ) brain = _make_brain("@@images/uid-xyz.jpeg") - tag = view._tag_from_brain_image_scales( - brain, "image", scale="preview" - ) - assert ( - 'src="https://plone.example/folder/doc/@@images/uid-xyz.jpeg"' - in tag - ) + tag = view._tag_from_brain_image_scales(brain, "image", scale="preview") + assert 'src="https://plone.example/folder/doc/@@images/uid-xyz.jpeg"' in tag class TestMissingOrInvalidMetadata: @@ -1074,8 +1055,7 @@ class TestMissingOrInvalidMetadata: brain = MagicMock() brain.image_scales = None assert ( - view._tag_from_brain_image_scales(brain, "image", scale="preview") - is None + view._tag_from_brain_image_scales(brain, "image", scale="preview") is None ) def test_thumbor_marker_without_config_falls_back(self, monkeypatch): @@ -1085,8 +1065,7 @@ class TestMissingOrInvalidMetadata: view = _make_view(monkeypatch, "https://plone.example", server_url=None) brain = _make_brain("thumbor:/abc/300x200/42/ff") assert ( - view._tag_from_brain_image_scales(brain, "image", scale="preview") - is None + view._tag_from_brain_image_scales(brain, "image", scale="preview") is None ) ``` @@ -1107,9 +1086,7 @@ from plone.pgthumbor.field_adapter import THUMBOR_MARKER class ThumborNavigationRootScaling(NavigationRootScaling): """@@image_scale view that resolves Thumbor markers at render time.""" - def _tag_from_brain_image_scales( - self, brain, fieldname, scale=None, **kwargs - ): + def _tag_from_brain_image_scales(self, brain, fieldname, scale=None, **kwargs): # Reuse the parent's validation / hidpi / lookup logic by # pulling the relevant stored dict ourselves — but we need to # handle our custom "thumbor:" marker before the parent's @@ -1134,10 +1111,8 @@ class ThumborNavigationRootScaling(NavigationRootScaling): # Thumbor not configured anymore; caller falls back # to object-based rendering. return None - signed_path = download[len(THUMBOR_MARKER):] - prefix = resolve_thumbor_prefix( - cfg.server_url, self.context.absolute_url() - ) + signed_path = download[len(THUMBOR_MARKER) :] + prefix = resolve_thumbor_prefix(cfg.server_url, self.context.absolute_url()) src = f"{prefix}{signed_path}" return self._render_image_tag(brain, data, src, **kwargs) @@ -1146,8 +1121,9 @@ class ThumborNavigationRootScaling(NavigationRootScaling): brain, fieldname, scale=scale, **kwargs ) - def _render_image_tag(self, brain, data, src, alt=_marker, - css_class=None, title=_marker, **kwargs): + def _render_image_tag( + self, brain, data, src, alt=_marker, css_class=None, title=_marker, **kwargs + ): from plone.namedfile.scaling import _image_tag_from_values, _marker if title is _marker: diff --git a/src/plone/pgthumbor/scaling.py b/src/plone/pgthumbor/scaling.py index ca8632e..979ef2c 100644 --- a/src/plone/pgthumbor/scaling.py +++ b/src/plone/pgthumbor/scaling.py @@ -7,6 +7,8 @@ from __future__ import annotations from AccessControl.PermissionRole import rolesForPermissionOn +from plone.namedfile.scaling import _image_tag_from_values +from plone.namedfile.scaling import _marker from plone.namedfile.scaling import ImageScale from plone.namedfile.scaling import ImageScaling from plone.pgthumbor.blob import get_blob_ids @@ -14,6 +16,7 @@ from plone.pgthumbor.interfaces import ICropProvider from plone.pgthumbor.url import scale_mode_to_thumbor from plone.pgthumbor.url import thumbor_url +from plone.rfc822.interfaces import IPrimaryFieldInfo from ZODB.utils import u64 from zope.component import queryAdapter @@ -142,6 +145,27 @@ def _default_scale_url(context, uid, extension, base_url=None): return f"{base_url}/@@images/{uid}.{extension}" +def _skip_type_fallback_url(context, data, fieldname, base_url=None): + """Original-field URL for types Thumbor cannot process (SVG). + + Browsers scale vector images themselves; with the volatile + ThumborScaleStorage a uid-based scale URL can never resolve + (issue #17), so the field URL is the only stable target. The + ``?v=`` modification-time cache buster compensates for the weaker + HTTP caching of non-unique URLs. + """ + if base_url is None: + base_url = context.absolute_url() + url = f"{base_url}/@@images/{fieldname}" + modified = getattr(data, "modified", None) + if modified is None: + modified = getattr(context, "_p_mtime", None) + try: + return f"{url}?v={int(float(modified) * 1000)}" + except (TypeError, ValueError): + return url + + # True if installed plone.namedfile has _scale_url (>= 8.0.0a2) _HAS_SCALE_URL = hasattr(ImageScale, "_scale_url") @@ -174,6 +198,10 @@ def __init__(self, context, request, **info): if url: self._thumbor_url = url self.url = url + elif getattr(self.data, "contentType", "") in _SKIP_THUMBOR_TYPES: + fieldname = info.get("fieldname") or getattr(self, "fieldname", None) + if fieldname: + self.url = _skip_type_fallback_url(context, self.data, fieldname) def _scale_url(self, uid, extension, base_url=None, scale_info=None): """Generate Thumbor URL if possible, otherwise fall back to default.""" @@ -190,10 +218,45 @@ def _scale_url(self, uid, extension, base_url=None, scale_info=None): if url: self._thumbor_url = url return url + if scale_info and getattr(self.data, "contentType", "") in _SKIP_THUMBOR_TYPES: + fieldname = scale_info.get("fieldname") or getattr(self, "fieldname", None) + if fieldname: + return _skip_type_fallback_url( + self.context, self.data, fieldname, base_url + ) if _HAS_SCALE_URL: return super()._scale_url(uid, extension, base_url, scale_info=scale_info) return _default_scale_url(self.context, uid, extension, base_url) + def srcset_attribute(self): + """HiDPI srcset with Thumbor URLs — uid URLs never resolve here. + + Skip-types get no srcset (vector scales itself); entries where no + Thumbor URL can be built are dropped rather than emitted dead. + """ + if not self.srcset: + return "" + if getattr(self.data, "contentType", "") in _SKIP_THUMBOR_TYPES: + return "" + fieldname = getattr(self, "fieldname", None) + parts = [] + for entry in self.srcset: + factor = entry.get("scale") + if not factor: + continue + crop = _get_crop(self.context, fieldname, entry) + url = _build_thumbor_url( + self.context, + self.data, + entry.get("width", 0) or 0, + entry.get("height", 0) or 0, + entry.get("mode", "scale"), + crop=crop, + ) + if url: + parts.append(f"{url} {factor}x") + return ", ".join(parts) + def index_html(self): """302 redirect to Thumbor URL instead of streaming ZODB data.""" if self._thumbor_url: @@ -223,6 +286,113 @@ def _scale_url(self, uid, extension, base_url=None, scale_info=None): ) if url: return url + if getattr(data, "contentType", "") in _SKIP_THUMBOR_TYPES: + return _skip_type_fallback_url( + self.context, data, scale_info["fieldname"], base_url + ) if _HAS_SCALE_URL: return super()._scale_url(uid, extension, base_url, scale_info=scale_info) return _default_scale_url(self.context, uid, extension, base_url) + + def srcset( + self, + fieldname=None, + scale_in_src="huge", + sizes="", + alt=_marker, + css_class=None, + title=_marker, + **kwargs, + ): + """Reimplementation of plone.namedfile's srcset(). + + The parent builds ``@@images/{uid}`` URLs straight from + ``storage.pre_scale`` — dead under the volatile storage + (issue #17). Build every URL from a scale view instead, whose + ``.url`` is a Thumbor URL (raster) or field URL (skip-types). + """ + if fieldname is None: + try: + primary = IPrimaryFieldInfo(self.context, None) + except TypeError: + return + if primary is None: + return + fieldname = primary.fieldname + + data = getattr(self.context, fieldname, None) + if getattr(data, "contentType", "") in _SKIP_THUMBOR_TYPES: + # Vector: one URL fits all widths — plain img tag suffices. + return self.tag( + fieldname=fieldname, + alt=alt, + css_class=css_class, + title=title, + **kwargs, + ) + + original_width, original_height = self.getImageSize(fieldname) + if not original_width or not original_height: + return None + + srcset_urls = [] + + # Back-fill an original-size entry when no configured scale + # already covers it — mirrors the parent's guard so an + # undersized original still yields a non-empty srcset. The URL + # always comes from a scale view, never a bare uid string. + available_widths = [width for (width, _height) in self.available_sizes.values()] + if original_width not in available_widths: + scale_view = self.scale( + fieldname=fieldname, + width=original_width, + height=original_height, + pre=True, + include_srcset=False, + ) + if scale_view is not None: + srcset_urls.append(f"{scale_view.url} {scale_view.width}w") + + for _name, (width, height) in self.available_sizes.items(): + if width <= original_width: + scale_view = self.scale( + fieldname=fieldname, + width=width, + height=height, + pre=True, + include_srcset=False, + ) + if scale_view is not None: + srcset_urls.append(f"{scale_view.url} {scale_view.width}w") + + attributes = {} + if title is _marker: + attributes["title"] = self.context.Title() + elif title: + attributes["title"] = title + if alt is _marker: + attributes["alt"] = self.context.Title() + else: + attributes["alt"] = alt + if css_class is not None: + attributes["class"] = css_class + attributes.update(**kwargs) + attributes["sizes"] = sizes + attributes["srcset"] = ", ".join(srcset_urls) + + if scale_in_src not in self.available_sizes: + for key, (width, _height) in self.available_sizes.items(): + if width <= original_width: + scale_in_src = key + break + + scale_view = self.scale(fieldname=fieldname, scale=scale_in_src, pre=True) + if scale_view is None: + return None + attributes["src"] = scale_view.url + if "width" not in attributes: + attributes["width"] = scale_view.width + if "height" not in attributes: + attributes["height"] = scale_view.height + + return _image_tag_from_values(*attributes.items()) diff --git a/src/plone/pgthumbor/storage.py b/src/plone/pgthumbor/storage.py index 97b804d..939f79b 100644 --- a/src/plone/pgthumbor/storage.py +++ b/src/plone/pgthumbor/storage.py @@ -13,14 +13,38 @@ from __future__ import annotations from plone.pgthumbor.interfaces import IPlonePgthumborLayer +from plone.registry.interfaces import IRegistry from plone.scale.storage import AnnotationStorage +from zope.component import queryUtility from zope.globalrequest import getRequest import logging +import re logger = logging.getLogger(__name__) +# plone.scale >= 4 deterministic uid: {fieldname}-{width}-{md5hex} +_LEGACY_UID_RE = re.compile( + r"^(?P.+)-(?P\d{1,9})-(?P[0-9a-f]{32})$" +) + + +def _allowed_scale_sizes(): + """Map width -> (width, height) for all registered plone.allowed_sizes.""" + registry = queryUtility(IRegistry) + if registry is None: + return {} + sizes = {} + for line in registry.get("plone.allowed_sizes") or (): + try: + _name, dims = line.split() + width, height = dims.split(":") + sizes.setdefault(int(width), (int(width), int(height))) + except ValueError: + continue + return sizes + def thumbor_scale_storage_factory(context, modified=None): """Adapter factory that returns ThumborScaleStorage only when active. @@ -67,12 +91,43 @@ def scale(self, **parameters): return self.pre_scale(**parameters) def get_or_generate(self, uid): - """Return stored info without generating image data. + """Return stored info, or heal a legacy uid without stored state. - Unlike the parent, never calls generate_scale() even if - data is None — in Thumbor mode, data is always None. + The volatile storage is empty on every fresh adapter instance, so + uid URLs from cached HTML or stale image_scales catalog metadata + would always 404 (issue #17). The uid format is deterministic + and parseable — regenerate the info on the fly instead. + """ + info = self.get(uid) + if info is not None: + return info + return self._heal_legacy_uid(uid) + + def _heal_legacy_uid(self, uid): + """Rebuild scale info from a ``{fieldname}-{width}-{md5hex}`` uid. + + Only widths registered in ``plone.allowed_sizes`` are accepted + (width 0 = original dimensions), so this cannot be abused to get + arbitrary dimensions signed on demand. ``pre_scale`` itself + returns None for unknown fields or empty values. """ - return self.get(uid) + match = _LEGACY_UID_RE.match(uid) + if match is None: + return None + fieldname = match.group("fieldname") + width = int(match.group("width")) + if width == 0: + dims = (None, None) + else: + dims = _allowed_scale_sizes().get(width) + if dims is None: + return None + info = self.pre_scale( + fieldname=fieldname, width=dims[0], height=dims[1], mode="scale" + ) + if info is not None: + info.setdefault("fieldname", fieldname) + return info def generate_scale(self, uid=None, **parameters): """Override to prevent Pillow invocation. diff --git a/tests/test_scaling.py b/tests/test_scaling.py index ff840d9..25d6533 100644 --- a/tests/test_scaling.py +++ b/tests/test_scaling.py @@ -114,7 +114,8 @@ def test_tag_has_thumbor_src(self, monkeypatch): assert f'src="{scale.url}"' in tag def test_svg_fallback(self, monkeypatch): - """SVG images should use standard Plone URLs, not Thumbor.""" + """SVG images must use the original field URL — uid URLs never + resolve under the volatile ThumborScaleStorage (issue #17).""" from plone.pgthumbor.scaling import ThumborImageScale _setup_env(monkeypatch) @@ -122,6 +123,8 @@ def test_svg_fallback(self, monkeypatch): ctx.absolute_url.return_value = "http://plone:8080/doc" request = MagicMock() data = _mock_image_data(content_type="image/svg+xml") + data.modified = None + ctx._p_mtime = None scale = ThumborImageScale( ctx, @@ -134,9 +137,94 @@ def test_svg_fallback(self, monkeypatch): mimetype="image/svg+xml", ) - # SVG should use standard Plone URL assert SERVER not in scale.url - assert "@@images" in scale.url + assert scale.url == "http://plone:8080/doc/@@images/image" + assert "abc123" not in scale.url + + def test_svg_fallback_cache_buster_from_field_modified(self, monkeypatch): + """When the field carries a modification time, append ?v=.""" + from plone.pgthumbor.scaling import ThumborImageScale + + _setup_env(monkeypatch) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + data = _mock_image_data(content_type="image/svg+xml") + data.modified = 1700000000.0 + + scale = ThumborImageScale( + ctx, + MagicMock(), + data=data, + fieldname="image", + width=400, + height=300, + uid="image-400-abc123", + mimetype="image/svg+xml", + ) + + assert scale.url == "http://plone:8080/doc/@@images/image?v=1700000000000" + + def test_svg_fallback_cache_buster_from_p_mtime(self, monkeypatch): + """Without field.modified, fall back to context._p_mtime.""" + from plone.pgthumbor.scaling import ThumborImageScale + + _setup_env(monkeypatch) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + ctx._p_mtime = 1600000000.0 + data = _mock_image_data(content_type="image/svg+xml") + data.modified = None + + scale = ThumborImageScale( + ctx, + MagicMock(), + data=data, + fieldname="image", + width=400, + height=300, + uid="image-400-abc123", + mimetype="image/svg+xml", + ) + + assert scale.url == "http://plone:8080/doc/@@images/image?v=1600000000000" + + def test_svg_fallback_legacy_namedfile_path(self, monkeypatch): + """The legacy (<8.0.0a2) __init__ branch must also emit the field + URL — production runs plone.namedfile 7.x.""" + from plone.pgthumbor.scaling import ThumborImageScale + + import plone.pgthumbor.scaling as scaling_mod + + _setup_env(monkeypatch) + monkeypatch.setattr(scaling_mod, "_HAS_SCALE_URL", False) + + from plone.pgthumbor.scaling import _default_scale_url + + monkeypatch.setattr( + ThumborImageScale, + "_scale_url", + lambda self, uid, ext, base_url=None, scale_info=None: _default_scale_url( + self.context, uid, ext, base_url + ), + ) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + ctx._p_mtime = None + data = _mock_image_data(content_type="image/svg+xml") + data.modified = None + + scale = ThumborImageScale( + ctx, + MagicMock(), + data=data, + fieldname="image", + width=400, + height=300, + uid="image-400-abc123", + mimetype="image/svg+xml", + ) + + assert scale.url == "http://plone:8080/doc/@@images/image" def test_not_configured_falls_back(self, monkeypatch): """When Thumbor not configured, use standard Plone URL.""" @@ -184,6 +272,109 @@ def test_original_image_no_thumbor(self, monkeypatch): assert "@@images" in scale.url assert SERVER not in scale.url + def test_srcset_attribute_uses_thumbor_urls(self, monkeypatch): + """HiDPI srcset entries must be Thumbor URLs, not dead uid URLs.""" + from plone.pgthumbor.scaling import ThumborImageScale + + _setup_env(monkeypatch) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + data = _mock_image_data() + + scale = ThumborImageScale( + ctx, + MagicMock(), + data=data, + fieldname="image", + width=400, + height=300, + uid="image-400-abc123", + mimetype="image/jpeg", + srcset=[ + {"uid": "image-800-def456", "width": 800, "height": 600, "scale": 2}, + ], + ) + + attr = scale.srcset_attribute() + assert attr.endswith(" 2x") + assert attr.startswith(SERVER) + assert "def456" not in attr + + def test_srcset_attribute_empty_for_svg(self, monkeypatch): + """Vector images need no srcset — one URL fits all densities.""" + from plone.pgthumbor.scaling import ThumborImageScale + + _setup_env(monkeypatch) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + ctx._p_mtime = None + data = _mock_image_data(content_type="image/svg+xml") + data.modified = None + + scale = ThumborImageScale( + ctx, + MagicMock(), + data=data, + fieldname="image", + width=400, + height=300, + uid="image-400-abc123", + mimetype="image/svg+xml", + srcset=[ + {"uid": "image-800-def456", "width": 800, "height": 600, "scale": 2}, + ], + ) + + assert scale.srcset_attribute() == "" + + def test_srcset_attribute_drops_unresolvable_entries(self, monkeypatch): + """No Thumbor config → entries are dropped, never emitted as uid URLs.""" + from plone.pgthumbor.scaling import ThumborImageScale + + env_override(monkeypatch) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + data = _mock_image_data() + + scale = ThumborImageScale( + ctx, + MagicMock(), + data=data, + fieldname="image", + width=400, + height=300, + uid="image-400-abc123", + mimetype="image/jpeg", + srcset=[ + {"uid": "image-800-def456", "width": 800, "height": 600, "scale": 2}, + ], + ) + + assert scale.srcset_attribute() == "" + + def test_srcset_attribute_skips_entry_without_scale_factor(self, monkeypatch): + """A srcset entry lacking the 'scale' factor is skipped, not a KeyError.""" + from plone.pgthumbor.scaling import ThumborImageScale + + _setup_env(monkeypatch) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + data = _mock_image_data() + + scale = ThumborImageScale( + ctx, + MagicMock(), + data=data, + fieldname="image", + width=400, + height=300, + uid="image-400-abc123", + mimetype="image/jpeg", + srcset=[{"uid": "image-800-def456", "width": 800, "height": 600}], + ) + + assert scale.srcset_attribute() == "" + class TestThumborImageScaling: """Test ThumborImageScaling (@@images view override).""" @@ -636,6 +827,28 @@ def test_scale_url_fallback_no_fieldname(self, monkeypatch): result = scaling._scale_url("uid123", "jpeg") assert "@@images/uid123.jpeg" in result + def test_scale_url_svg_returns_field_url(self, monkeypatch): + """ThumborImageScaling._scale_url must not emit uid URLs for SVG.""" + from plone.pgthumbor.scaling import ThumborImageScaling + + _setup_env(monkeypatch) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + ctx._p_mtime = None + svg = _mock_image_data(content_type="image/svg+xml") + svg.modified = None + ctx.logo = svg + view = ThumborImageScaling(ctx, MagicMock()) + + uid = "logo-200-" + "a" * 32 + url = view._scale_url( + uid, + "svg", + scale_info={"fieldname": "logo", "width": 200, "height": 200, "uid": uid}, + ) + + assert url == "http://plone:8080/doc/@@images/logo" + class TestGetCrop: """Test _get_crop() helper.""" @@ -824,3 +1037,106 @@ def test_scaling_scale_url_with_crop(self, monkeypatch): ) assert result.startswith(SERVER) assert "10x20:300x400" in result + + +class TestThumborImageScalingSrcset: + """srcset() must never emit uid URLs (issue #17).""" + + def _make_view(self, monkeypatch, content_type="image/jpeg"): + from plone.namedfile.scaling import ImageScaling + from plone.pgthumbor.scaling import ThumborImageScaling + + _setup_env(monkeypatch) + ctx = MagicMock() + ctx.absolute_url.return_value = "http://plone:8080/doc" + ctx.Title.return_value = "Doc" + ctx.image = _mock_image_data(content_type=content_type) + view = ThumborImageScaling(ctx, MagicMock()) + monkeypatch.setattr( + ImageScaling, + "available_sizes", + {"preview": (400, 400), "large": (800, 800)}, + raising=False, + ) + view.getImageSize = lambda fieldname: (800, 600) + return view + + def test_srcset_uses_scale_view_urls(self, monkeypatch): + """Each configured size must be requested — and rendered — via its + own scale view, not a single mocked return_value that would mask + per-size behavior.""" + view = self._make_view(monkeypatch) + calls = [] + + def fake_scale(fieldname=None, width=None, height=None, scale=None, **kw): + calls.append( + { + "fieldname": fieldname, + "width": width, + "height": height, + "scale": scale, + } + ) + fake = MagicMock() + resolved_width = width or {"preview": 400, "large": 800}.get(scale, 400) + fake.url = f"{SERVER}/signed/{resolved_width}x0/42/ff" + fake.width = resolved_width + fake.height = resolved_width * 3 // 4 + return fake + + view.scale = MagicMock(side_effect=fake_scale) + + tag = view.srcset(fieldname="image", scale_in_src="preview") + + assert SERVER in tag + assert "@@images/image-" not in tag + assert "400w" in tag + assert "800w" in tag + requested_widths = {c["width"] for c in calls if c["width"] is not None} + assert requested_widths == {400, 800} + assert any(c["scale"] == "preview" for c in calls) + + def test_srcset_svg_delegates_to_tag(self, monkeypatch): + from plone.namedfile.scaling import _marker + + view = self._make_view(monkeypatch, content_type="image/svg+xml") + view.tag = MagicMock(return_value="") + + result = view.srcset(fieldname="image") + + assert result == "" + view.tag.assert_called_once_with( + fieldname="image", alt=_marker, css_class=None, title=_marker + ) + + def test_srcset_unresolvable_src_scale_returns_none(self, monkeypatch): + """An unresolvable scale_in_src must not raise AttributeError on + ``None.url`` — mirrors the parent's ``if scale is None: return + None`` guard (issue #17 fix round 1, Finding 1).""" + view = self._make_view(monkeypatch) + view.scale = MagicMock(return_value=None) + + assert view.srcset(fieldname="image", scale_in_src="nonexistent") is None + + def test_srcset_undersized_original_backfills_original_entry(self, monkeypatch): + """An original smaller than every configured scale must still get a + non-empty srcset, back-filled with an original-size scale-view URL + (issue #17 fix round 1, Finding 2).""" + view = self._make_view(monkeypatch) + view.getImageSize = lambda fieldname: (100, 80) + calls = [] + + def fake_scale(fieldname=None, scale=None, height=None, width=None, **kw): + calls.append({"scale": scale, "width": width, "height": height}) + fake = MagicMock() + fake.url = f"{SERVER}/signed/{width or scale}/42/ff" + fake.width = width or 100 + fake.height = height or 80 + return fake + + view.scale = MagicMock(side_effect=fake_scale) + + tag = view.srcset(fieldname="image", scale_in_src="preview") + + assert "srcset=" in tag + assert f"{SERVER}/signed/100/42/ff 100w" in tag diff --git a/tests/test_storage.py b/tests/test_storage.py index 955a8d9..b28f056 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -101,6 +101,164 @@ def test_separate_instances_separate_storage(self): s1.storage["key"] = "value" assert "key" not in s2.storage + def test_get_or_generate_heals_legacy_uid(self): + """A uid-shaped miss regenerates scale info via pre_scale (issue #17): + cached HTML and stale image_scales metadata keep working.""" + storage = _make_storage() + healed = { + "uid": "image-400-" + "a" * 32, + "data": None, + "width": 400, + "height": 400, + } + + with ( + patch.object(storage, "pre_scale", return_value=dict(healed)) as mock_pre, + patch( + "plone.pgthumbor.storage._allowed_scale_sizes", + return_value={400: (400, 400)}, + ), + ): + result = storage.get_or_generate("image-400-" + "b" * 32) + + mock_pre.assert_called_once_with( + fieldname="image", width=400, height=400, mode="scale" + ) + assert result["fieldname"] == "image" + + def test_get_or_generate_heals_fieldname_with_dashes(self): + """fieldname may contain dashes — parse from the right.""" + storage = _make_storage() + + with ( + patch.object(storage, "pre_scale", return_value={"uid": "x"}) as mock_pre, + patch( + "plone.pgthumbor.storage._allowed_scale_sizes", + return_value={200: (200, 200)}, + ), + ): + storage.get_or_generate("my-logo-field-200-" + "c" * 32) + + mock_pre.assert_called_once_with( + fieldname="my-logo-field", width=200, height=200, mode="scale" + ) + + def test_get_or_generate_rejects_unregistered_width(self): + """Unknown widths 404 — prevents on-demand signing of arbitrary + dimensions (cache-filling amplification).""" + storage = _make_storage() + + with ( + patch.object(storage, "pre_scale") as mock_pre, + patch( + "plone.pgthumbor.storage._allowed_scale_sizes", + return_value={400: (400, 400)}, + ), + ): + assert storage.get_or_generate("image-999-" + "b" * 32) is None + + mock_pre.assert_not_called() + + def test_get_or_generate_rejects_malformed_uid(self): + """Hash part must look like md5-hex; anything else stays a 404.""" + storage = _make_storage() + + with patch.object(storage, "pre_scale") as mock_pre: + assert storage.get_or_generate("image-400-nothex") is None + assert storage.get_or_generate("image-400") is None + + mock_pre.assert_not_called() + + def test_get_or_generate_width_zero_means_original(self): + """uid 'image-0-' comes from bare tag()/pre_scale without a + width — regenerate with original dimensions.""" + storage = _make_storage() + + with ( + patch.object(storage, "pre_scale", return_value={"uid": "x"}) as mock_pre, + patch( + "plone.pgthumbor.storage._allowed_scale_sizes", + return_value={}, + ), + ): + storage.get_or_generate("image-0-" + "d" * 32) + + mock_pre.assert_called_once_with( + fieldname="image", width=None, height=None, mode="scale" + ) + + def test_get_or_generate_pre_scale_none_returns_none(self): + """pre_scale returning None (missing field/value) stays a 404.""" + storage = _make_storage() + + with ( + patch.object(storage, "pre_scale", return_value=None), + patch( + "plone.pgthumbor.storage._allowed_scale_sizes", + return_value={400: (400, 400)}, + ), + ): + assert storage.get_or_generate("gone-400-" + "e" * 32) is None + + def test_get_or_generate_rejects_oversized_width(self): + """A multi-thousand-digit width must not raise — reject with None.""" + storage = _make_storage() + + with patch.object(storage, "pre_scale") as mock_pre: + assert ( + storage.get_or_generate("image-" + "9" * 5000 + "-" + "a" * 32) is None + ) + + mock_pre.assert_not_called() + + +class TestAllowedScaleSizes: + """Direct tests for the registry-parsing DoS gate.""" + + def _registry_with(self, lines): + registry = MagicMock() + registry.get.return_value = lines + return registry + + def test_parses_registered_sizes(self): + from plone.pgthumbor.storage import _allowed_scale_sizes + + registry = self._registry_with(["preview 400:400", "large 800:65536"]) + with patch("plone.pgthumbor.storage.queryUtility", return_value=registry): + sizes = _allowed_scale_sizes() + + assert sizes == {400: (400, 400), 800: (800, 65536)} + registry.get.assert_called_once_with("plone.allowed_sizes") + + def test_first_width_wins_on_duplicates(self): + from plone.pgthumbor.storage import _allowed_scale_sizes + + registry = self._registry_with(["a 400:400", "b 400:300"]) + with patch("plone.pgthumbor.storage.queryUtility", return_value=registry): + assert _allowed_scale_sizes() == {400: (400, 400)} + + def test_skips_malformed_lines(self): + from plone.pgthumbor.storage import _allowed_scale_sizes + + registry = self._registry_with( + ["broken", "no-dims 400", "preview 400:400", "bad 400x300"] + ) + with patch("plone.pgthumbor.storage.queryUtility", return_value=registry): + assert _allowed_scale_sizes() == {400: (400, 400)} + + def test_no_registry_returns_empty(self): + from plone.pgthumbor.storage import _allowed_scale_sizes + + with patch("plone.pgthumbor.storage.queryUtility", return_value=None): + assert _allowed_scale_sizes() == {} + + def test_none_record_returns_empty(self): + from plone.pgthumbor.storage import _allowed_scale_sizes + + registry = self._registry_with(None) + with patch("plone.pgthumbor.storage.queryUtility", return_value=registry): + assert _allowed_scale_sizes() == {} + class TestThumborScaleStorageFactory: """Test that the factory respects the browser layer."""