Multiple image transform query strings in asset url #336

Activity

1. ch-turnstyle

   added

   bug

   on Dec 10, 2024
2. brimby commented on Dec 11, 2024

   

   brimby

   on Dec 11, 2024

   Last edited by brimby

   More actions

   I'm also getting this where the query string is appended twice. (For example https://mywebsite/images/image.jpg?w=500&h=500&auto=compress%2Cformat&fit=crop&dm=1728060295&s=134d1fad5d99eea8beebf6e0e6c6beaf?w=500&h=500&q=90&auto=format&fit=clip&dm=1728060295&s=98a5db076ee1b9b6f95fbd316963e2c3)

   I do use Servd as well, but I see the issue even on my local so I don't think they're to blame, although I will admit that I import DBs from staging sometimes, so I can't say for certain that it's impossible something is coming over from that side.

   Something interesting is that I use the image alignment configs (see below) and if I save the entry but then go back and change the alignment and save again, the bug goes away.

     "image": {
         "toolbar": [
           "imageStyle:alignLeft",
           "imageStyle:alignRight"
         ]
       }
   

   I think I'll write some js that will remove the second query param, but I must say this is a pretty gnarly bug and I don't even know where to start looking for the issue in the vendor code.

   Edit: Oh also I'm still on Craft 4, so it's not just a C5 thing
3. brimby commented on Dec 11, 2024

   

   brimby

   on Dec 11, 2024

   Last edited by brimby

   More actions

   Here's that js if it's helpful to you at all @ch-turnstyle . Might want to wrap it in either a document.ready function if you have jquery, or for vanilla, wrap it with a 'DOMContentLoaded' event listener. I use that one in the code below.

   // remove any second query string from img src
   document.addEventListener('DOMContentLoaded', () => {
     const imgs = [...document.getElementsByTagName('img')]
     imgs.forEach(img => {
       const urlSplit = img.src ? img.src.split('?') : []
       if (urlSplit.length > 2) img.src = urlSplit[0] + '?' + urlSplit[1]
     })
   })
   

   👍React with 👍1Reacted by Christine
4. i-just commented on Dec 11, 2024

   

   i-just

   on Dec 11, 2024

   Contributor

   More actions

   Hi, thanks for getting in touch!

   Those query strings come from the “Servd Assets and Helpers” plugin. It looks like this issue was raised in that plugin’s repo: servdhost/craft-asset-storage#80. Can you please follow up there?

   👍React with 👍1Reacted by Christine
5. mattgrayisok commented on Dec 20, 2024

   

   mattgrayisok

   on Dec 20, 2024

   More actions

   Hey @i-just

   I've just been looking into this within the Servd plugin and I think I've tracked down the root cause to the way that the asset URLs are being stored in the database when saved from a CKEditor field. I am having trouble finding the code within the CKEditor plugin which actually formats the way image URLs are stored so I'm hoping you can help me to debug further.

   Servd generates image URLs using the Imgix query param format, E.G:

   https://mywebsite/images/image.jpg?w=500&h=500&auto=compress%2Cformat&fit=crop&dm=1728060295&s=134d1fad5d99eea8beebf6e0e6c6beaf

   When these are rendered within the CKEditor field in the Craft control panel, they are showing up fine, and I can see that uses a pretty basic controller function to output that correct URL.

   However when the images are saved to the database, they are being stored in the elements_sites json blob like this:

   <figure class="image">
     <img style="aspect-ratio:500/375;" src="{asset:2631:transform:medium||https://mywebsite/images/image.jpg}?w=500&h=500&auto=compress%2Cformat&fit=crop&dm=1728060295&s=134d1fad5d99eea8beebf6e0e6c6beaf" width="500" height="500" alt="" />
   </figure>
   

   Notice that the url query params sit outside the {asset:} segment.

   When this is rendered on the front end, the {asset:} segment is expanded back out to a full URL with the transform applied, but the extra query params following the {asset:} segment are added onto the end again, leading to a doubling up of query params.

   So I think the CKEditor plugin might need adjusting to bring the query params for an asset URL inside the {asset:} segment. And I assume there's some code somewhere which is parsing the asset URL which is currently ignoring the query params causing this.

   I believe this would impact any images which use query params, including Imgix urls etc, so not exclusive to Servd.

   I also believe this only impacts Craft 5 as I haven't been able to replicate on any Craft 4 projects.

   Any thoughts very welcome!

   Matt
6. i-just commented on Jan 3, 2025

   

   i-just

   on Jan 3, 2025

   Contributor

   More actions

   Thanks for sharing your findings!

   The refs are parsed in the HtmlField package, which is required by the CKEditor plugin.

   I’ve set up a basic plugin (for Craft 5) that adds some hard-coded query params to the asset’s URL (similar to what I can see happening here) and in my case, the query param stays inside the curly braces <img style="aspect-ratio:200/200;" src="{asset:55:transform:w200||https://5.ddev.site/assets/local/150C.png?w=200&h=200&q=80}" alt="" width="200" height="200" />.
   The query string can be stored outside if this condition is not met.
7. mattgrayisok commented on Jan 3, 2025

   

   mattgrayisok

   on Jan 3, 2025

   More actions

   If I'm understanding that correctly, I think that condition would fail if the URL contains any character sequences that are changed by Html::decode(), which under the hood runs htmlspecialchars_decode over the query string, but that should only react to & " which shouldn't be present in the URL strings (unless something else is changing the ampersand characters to & before we get to this point).

   So I think we'll need to debug this function using a URL which is triggering the bug, to determine how the URL is being parsed as it's possible the nearby regex might also be parsing incorrectly.

   I'll see if I can get some more info.

   👍React with 👍1Reacted by Iwona Just
8. mattgrayisok commented on Jan 10, 2025

   

   mattgrayisok

   on Jan 10, 2025

   More actions

   Debugging has led me to a probable cause for this. CKEditor generates its embedded image URLs by called the generate-transform action directly using an ajax request. This does not seem to honour any pre and post image url generation hooks, so the way the Servd plugin is currently inserting transform URLs (using event hooks) is either ignored or doesn't work as expected. In CKEditor this results in different versions of the URL being generated via different means and then compared, which don't match, causing the bug.

   I think that replacing the image URL event hooks we currently use, with a full ImageTransformer implementation should fix the issue, however as there's no docs on ImageTransformers I'm not sure how they are supposed to be set up. Implementing the class itself is likely straight forward, however it looks as though ImageTransformers are chosen based on the transformer associated with a provided ImageTransform. However, I don't see any way to actually set a custom transformer on a transform other than specifying a transformer param on a dynamically created transform object.

   Also, I can see Craft cloud gets around this by simply forcing the use of Craft's transformer for everything:

   https://github.com/craftcms/cloud-extension-yii2/blob/04af6766aa395879d54cdbbdc676f718096fce55/src/Module.php#L75

   So it looks like:

   • CKEditor image URLs ignore image url generation hooks
   • The only way to work around this is with a custom ImageTransformer
   • ImageTransformers can only be set as a global or via dynamically created transform objects
   • This prevents us from being able to use Volume/Filesystem specific image transformers, as would be required to mix Servd asset platform stored images and images stored elsewhere (Craft Cloud just forces the use of Craft Cloud storage for everything to work around this problem).

   Given all of that, I don't think there's currently a way for us to fix this. Do you agree with my findings @i-just ? Let me know if I've missed anything that might help.
9. i-just commented on Jan 13, 2025

   

   i-just

   on Jan 13, 2025

   Contributor

   More actions

   Thanks for this, much appreciated! We’ll have a chat about this and see what we can do. I’ll post an update once we know more!
10. timkelty commented on Jan 21, 2025

    

    timkelty

    on Jan 21, 2025

    Last edited by timkelty

    Contributor

    More actions

    Thanks to everyone for the very useful debugging!

    @mattgrayisok @i-just I think I got to the bottom of this: craftcms/cms#16464

    craft-asset-storage is affected because it is falling back to a default transform when it is passed an empty transform – and that transform may not match the transform it should be passed (e.g. if the named transform has only width or height). Once the query strings don't match, the ref parsing from HtmlField has a bad day.

    @mattgrayisok You're also right that a custom ImageTransformer is a more straightforward way to handle this, but as you've discovered, they can't yet be applied to a particular FS type. Making this easier to deal with is planned for Craft 6, but I'm going to see if I can get something non-breaking into 5 so that an FS can define a default transformer.
11. timkelty commented on Jan 21, 2025

    

    timkelty

    on Jan 21, 2025

    Contributor

    More actions

    craftcms/cms:4.14.0 and craftcms/cms:5.6.0 are now released with this fix.

    Please let me know if anyone else is able to confirm the fix!
12. ch-turnstyle commented on Jan 31, 2025

    

    ch-turnstyle

    on Jan 31, 2025

    Author

    More actions

    Finally been able to test this, and transforms are now displaying as expected.

    Thank you @timkelty, @mattgrayisok and @i-just for all your efforts here!
13. angrybrad

    added

    repo:ckeditor

    on Oct 2, 2026