Repository navigation
Local image cache making it GDPR friendly - #3
Merged
Merged
Conversation
Reviews cached the Google URL of each reviewer's avatar and every layout
rendered it as a remote <img>, so every visitor hit googleusercontent.com on
every page view. That leaks visitor IPs to Google with no consent step, adds a
third-party handshake per avatar, and leaves broken images behind whenever
Google rotates an avatar URL.
Photos are now downloaded into media/mod_prettyreviews/images/{moduleId}/ and
served from there. A new ImageCacheHelper stores each photo under a hash of its
normalised source URL, so a shared avatar is stored once and a rotated URL
yields a new file whose predecessor is pruned on the next refresh. Reviewers
whose photo is not stored yet, or cannot be downloaded, get an avatar generated
from their initials, so nothing is ever fetched from Google at render time.
Photos are collected during refreshFromGoogle -- covering both the admin button
and the task scheduler plugin -- and, for caches written before this existed,
a few at a time while a page renders. The render path is capped at three
downloads on a three second timeout, backs off for six hours after a failure,
and never prunes, since it only sees the reviews being displayed. An explicit
refresh ignores the back-off, because somebody asked for it.
Downloads are restricted to Google photo hosts over https, capped at 2 MiB, and
accepted only when the bytes themselves decode as a JPEG, PNG, GIF or WebP --
the extension comes from that check, never from the URL, and a remote SVG is
always refused. Filenames read back from the cache file are re-validated before
they reach a page. Nothing here throws: a single unusable avatar must not break
a refresh or a page render.
The dispatcher swaps in the local URL in place of profile_photo_url, so all
eight layouts, their $safeUrl guards, and any template override keep working
unchanged. The stored Google URL is left alone -- it is the cache key and the
source to re-fetch from. New reviews are now stored as arrays to match those
read back from disk; the encoded JSON is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KaGC9xN356R5YKsdKWh4Yq
The module had no tests, and the photo cache added logic that is easy to get quietly wrong: which hosts a download is accepted from, what the stored file is named, which files a prune is allowed to delete, and which keys a page render may write back into the cache. Those are exactly the mistakes that stay invisible until a site leaks visitor data to Google or loses its photos. There is no Composer manifest here and no build step, so rather than pulling in a framework the tests stand up the handful of Joomla classes the helpers touch -- the filesystem wrappers, the HTTP client, the URI root and the logger. The File stub takes its write buffer by reference exactly as the real one does, so a caller that passes a literal fails here too. Each file runs in its own process against its own temporary site root, so one suite's fixtures cannot reach another. 107 assertions across three suites: storing, naming, de-duplicating, refusing and pruning photos; the initials avatars, including a name made of punctuation and one made of markup; the render-time backfill, its download cap, its back-off and its refusal to write display data back into the cache; and a refresh cycle over a rotated Google URL. The suites were checked by breaking the module in sixteen ways -- widening the host list, taking the file extension from the URL, dropping the size cap, skipping filename validation, pruning on a partial run, writing whole reviews back to the cache -- and confirming each one turns them red. The tests carry an export-ignore, so they stay out of the release archive that users download. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KaGC9xN356R5YKsdKWh4Yq
An update downloads at most 25 photos so the button stays responsive, which means a large cache needs several presses to fill up -- and nothing said so. The admin saw "Reviews have been updated" and had no way to know most photos were still showing initials avatars. The sync now counts the reviews it had to leave purely because the budget ran out, the AJAX response carries that number alongside the update result, and the admin message becomes a warning naming the count and asking for another press. Photos whose download was attempted and failed are deliberately not counted: pressing again straight away would not help those, and they are logged instead. The count is phrased trailing the sentence in every language, so it reads correctly for one photo and for fifty. Purge still answers with a plain true, so the admin script accepts both response shapes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KaGC9xN356R5YKsdKWh4Yq
Twenty-five photos per update was the wrong measure: it held a fast connection to twenty clicks for a five-hundred-review cache while doing nothing to bound a slow one, whose twenty-five downloads could still take as long as they pleased. What the cap was actually protecting -- the wall time of the request -- is now what is budgeted: an update starts downloads for at most about fifteen seconds, a page render for three, and a download already under way is allowed to finish. A fast connection fills a large cache in one press; a slow one leaves the remainder for the next, and the message reporting how many carries over unchanged. Pruning now keys off that report instead of a spent counter: a run that left photos waiting has not seen every file it would keep, so it does not prune. The deadline is read through a small now() seam so the tests can supply a clock they control -- each simulated download charges it a fixed cost -- instead of sleeping their way into flakiness. The mutation battery was updated alongside and still turns the suites red eighteen ways, including ignoring the deadline and pruning on a partial run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KaGC9xN356R5YKsdKWh4Yq
Downloading missing photos while a page renders was the feature's most involved piece: an outbound HTTP path inside anonymous requests, a render mode threaded through the sync, a six-hour back-off so dead URLs were not retried per view, and a write-back that had to merge two keys carefully to avoid clobbering the cache with display data. All of it served one case -- a cache written before photos were stored locally, on a site nobody refreshes -- and that case has two better answers already: the Update Reviews button and the task plugin. So page views no longer write or download anything. Photos are collected only when the reviews are refreshed; a cache from before this feature shows no reviewer photos until its next refresh, which the changelog and README now say plainly. With no render path left, the back-off bookkeeping and the attempt marker go too -- an explicit refresh always retried failures anyway -- and the sync sheds its mode flag, since every caller now hands it the whole cache. The seams added for the removed path (public budget constants, the overridable cache accessor) are private again. The backfill suite is replaced by a smaller one covering what remains at the seam: an old cache renders with photos hidden and downloads nothing, the first refresh fills it in, tampered filenames yield no URL, and purging still removes photos with or without the data file. The mutation battery, trimmed of the mutations whose targets no longer exist, still turns the suites red thirteen ways. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KaGC9xN356R5YKsdKWh4Yq
The fifteen-second budget only gated the moment a download started, so a photo begun just under the deadline could still run two sequential ten-second requests and push the refresh to roughly thirty-five seconds. The merged reviews are written only after the photo sync, so a default max_execution_time of thirty killed the request and threw them away. Each request is now given what the budget has left, capped at HTTP_TIMEOUT, so a run overshoots by at most MIN_TIMEOUT. That floor is not cosmetic: Joomla passes the value straight to the transport, where zero means no timeout at all, and the last fraction of a second would otherwise round down to exactly that. The retry against the original URL now only fires when normalising actually changed it. A URL already ending in =s256-c was being requested twice identically, spending the budget twice for nothing. Pruning no longer waits for a run that finished. Running out of time skips a download, not a loop iteration, so every review is still visited and $keep already names every stored file that is referenced; a photo the run never reached is not on disk yet, so there is nothing to lose. On a site that never completes a run inside the budget, orphans previously accumulated forever. Module image folders are created while the site runs, so they carry no index.html from the installer. One is written on creation, so a server with directory listing left on has nothing to show.
…ically Joomla's HTTP transports follow redirects themselves by default (libraries/vendor/joomla/http/src/Transport/Curl.php:141, and the manual fallback in the CMS transport at CurlTransport.php:199), so the host allowlist was only ever applied to the first url of a chain. The class treats photo urls as untrusted because they reach it through a writable file on disk, and that has to hold for every hop, not just the first. follow_location is now off and the chain is walked here, so each hop goes through isAllowedSource() before it is requested, up to three of them. Location headers are resolved for the absolute and host-relative forms; a relative path is refused rather than guessed at. Reading the response now uses the PSR-7 accessors. The ->code and ->body properties the module reached for come from a shim on Joomla\CMS\Http\Response that is deprecated in 6.0 and removed in 8.0. The same two calls in PrettyreviewsHelper::fetchFromGoogle() go with them, so the module does not leave one half converted. The test double modelled the old shape and so would not have caught it; it is now shaped like a PSR-7 response. A photo that cannot be downloaded leaves a marker counting the attempts. The first failure is always retried on the next run, so a refresh that could not reach Google at all still heals the moment someone presses the button again; a photo that fails twice in a row is left alone for an hour. Without this an avatar that is gone for good spends part of every refresh's budget, crowding out the ones that would have worked. Markers are pruned with everything else, and are excluded from the pattern that decides what may be served -- they are bookkeeping, not something to hand to a visitor. Photos and initials avatars are written beside their target and renamed into place. Both are read back by name and served, so a write interrupted half way would leave a broken image that is_file() is perfectly happy with. Stored photo urls are root-relative rather than absolute, so they keep working whichever of a site's domains, and whichever protocol, the page is served from.
The photos were being downloaded, stored and recorded correctly, and then thrown away at the last step: publicPhotoUrl() handed back a root-relative url, and every layout runs a photo through a guard that accepts nothing but http(s). So a module refreshed under 2.2.0 showed no avatars at all, with the files sitting on disk and every helper test green -- the suites assert on the url the helper returns and never render a layout, and one of them had the root-relative shape written down as correct. The url is absolute now. That is also what keeps template overrides already in the wild working: they carry their own copy of the http(s) guard, so an absolute url is the only shape they will render. Second, a cache written by 2.1.0 names no stored photo, so upgrading a site that had not been refreshed yet dropped its avatars too -- the case the render-time backfill used to cover. Such a review now falls back to an initials avatar built inline from the reviewer's name, so a module shows a complete row of avatars from the moment it is upgraded, without a page render downloading or writing anything. The real photos take over on the next refresh. A review carrying no photo url at all still renders nothing, which is what it got before and what it gets after a refresh. The svg builder is shared with the file writeInitials() already produced. The layouts get a photo-specific guard rather than a looser one: external links still go through the unchanged https-only $safeUrl. Covered by a new suite that renders all eight layouts through the real helper, which fails eight ways on the root-relative url and two more without the fallback, and holds the guard to refusing javascript:, data:text/html, an unencoded svg and a data uri with a quote in it. README and the changelog note both promised photos would stay hidden until the next refresh, which is no longer what happens.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.