craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.28k stars 635 forks source link

[4.3.8.2]: New asset versioning hash is added after asset URL generation is handled (Imgix) #12663

Closed croxton closed 1 year ago

croxton commented 1 year ago

What happened?

Description

This change broke transforms for image previews in the control panel, where they are handled by my plugin Imgixer, which uses Imgix. This happened because the additional version hash (v=xxx) is added after the plugin creates a signed Imgix URL. Imgix URLs can be signed for security - an MD5 hash combining the full image url with a secret token. This is quite a common approach with third party image services. Because Craft now appends a further version hash to this URL, the image request results in a 403.

I'd guess this might affect other plugins that use Imgix and similar services for transforms in the control panel.

Steps to reproduce

// Asset::EVENT_DEFINE_URL or Asset::EVENT_BEFORE_GENERATE_TRANSFORM
Event::on(
    Asset::class,
    Asset::EVENT_DEFINE_URL,
    function (DefineAssetUrlEvent $event) {
        // create a signed Imgix URL
        $event->url = $this->urlService->getUrl($event->sender, $event->transform);
        if ($event->url) {
            $event->handled = true;
        }
    }
);

Expected behavior

Either:

Craft CMS version

4.3.8.2

PHP version

8.1

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

brandonkelly commented 1 year ago

Just to clarify, is the issue that we are overriding an existing v param? Or that any modifications to the URL whatsoever will throw off the hash?

Is the previous change in 5f320453ea1855f2407e592f5a90b813f4641ffb problematic as well? If not, what makes thumbs different?

croxton commented 1 year ago

No, not overriding an existing 'v' param - that param is simply appended after the imgix url has been generated. Interestingly the 'v' isn't added to thumbs when returning a url from Assets::EVENT_GET_ASSET_THUMB_URL. I'm not sure why, but that might explain why I only just noticed the problem.

Thumbs still work, it's previews and the image editor where the 'v' is simply added onto the end of the url.

brandonkelly commented 1 year ago

Ah ok, so you use the event to completely bypass Craft’s involvement in the thumb URL generation.

I’m not entirely sure how to resolve this. Perhaps you could start adding a v param to your URLs before signing them? Then we can stop adding it if there already is one.

croxton commented 1 year ago

I’m not entirely sure how to resolve this. Perhaps you could start adding a v param to your URLs before signing them? Then we can stop adding it if there already is one.

That would work, yes please!

croxton commented 1 year ago

I went ahead and added the versioning v parameter to generated URLs - before they are signed - and the good news is Craft already takes that into account. This completely resolves the issue in Imgix. Thank you for the suggestion!

I did however run into another issue with another image service provider Imagekit, that is specific to images shown in the image preview modal and the image editor. The issue is that the image url is subsequently urlencoded after it has been generated by my plugin. The encoded URL presumably does not match the signed hash in Imagekit (because it was based on the unencoded URL), so it returns a 403. Imgix on the other hand will happily accept an encoded URL.

I'm not sure if that is easily fixable because I assume there must be good reason for encoding the URLs in those contexts?

brandonkelly commented 1 year ago

We just discussed this internally and came up with a better solution. Going forward Craft will stop adding the v param to control panel image URLs automatically unless the URL begins with one of the asset’s base filesystem URLs. Which will rule out URLs to Imgix, etc.

brandonkelly commented 1 year ago

Craft 4.3.9 is out with that fix!