Skip to content

Keep query params from leaking between embeds of the same image (#3567) - #4333

Merged
rhukster merged 2 commits into
getgrav:developfrom
wakqasahmed:fix/issue-3567-media-querystring-chaining
Sep 27, 2026
Merged

rhukster merged 2 commits into
getgrav:developfrom
wakqasahmed:fix/issue-3567-media-querystring-chaining

Conversation

@wakqasahmed

Copy link
Copy Markdown
Contributor

Fixes #3567.

When the same image is embedded more than once in Markdown, every embed goes through the page's single shared medium object. Query params that aren't media actions (like ?foobar=asdasd or ?preset=foo) are appended to that medium's querystring and never cleared, so the second and third embeds pick up everything the earlier ones added.

I first tried clearing the querystring in reset(), but url() deliberately resets before adding the querystring, and there are existing tests (plus the older "Medium objects losing query string attributes" fix) that depend on it surviving a reset. So I left that alone. Instead, the Markdown excerpt now works on $medium->copy(), so each embed starts from the untouched original. For that copy to really be independent, Medium::__clone() now also clones the @2x alternatives (the querystring gets pushed onto them too, so otherwise the srcset still leaked), and it drops any cached thumbnail, which would otherwise still point at the original medium as its parent and bring the leak back for lightbox/link embeds once the thumbnail had been built from Twig.

One visible output change: an embed without image actions now always serves the original file, even if an earlier embed of the same image was cropped or resized. Before, it happened to be served from the images/ cache path, only because the shared medium still had the earlier embed's image open. I updated the second assertion in testProcessImageHtml, which relied on that behavior.

This only covers the Markdown excerpt path. The public static Helpers\Excerpts::processMediaActions() still operates on whatever medium its caller passes in, so callers of that helper aren't changed here.

I added four tests to ExcerptsTest: the exact case from the issue (same jpg embedded three times plus once with no query), the same with a @2x alternative to cover the srcset, the lightbox case with a thumbnail already cached from Twig, and an SVG to cover the non-ImageMedium path. They all fail on develop and pass with this change. I ran the Excerpts, Page/Medium, Page/Markdown, Markdown and Twig tests, plus the full unit suite, on PHP 8.4 with GD, and everything passes.

@rhukster
rhukster merged commit cd78d66 into getgrav:develop Sep 27, 2026
4 checks passed
@rhukster

Copy link
Copy Markdown
Member

Thanks @wakqasahmed, this is a really thorough one. Merged as is. On top I added a few tests for other things that leaked between repeated embeds of the same image and that your change fixes too: a #fragment or ?style= from an earlier embed, and a retina srcset getting dropped after an earlier embed was cropped. I also added the CHANGELOG entry. It will be in 2.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reusing images chains parameters

2 participants