winter icon indicating copy to clipboard operation
winter copied to clipboard

ImageResizer clobbers query string on URLs to resized images

Open lackneets opened this issue 3 years ago • 5 comments

Winter CMS Build

1.2

PHP Version

8.1

Database engine

SQLite

Plugins installed

No response

Issue description

My project is configured to store file on s3. And I set a image type column in backend list, but it cannot display correctly.

CleanShot 2022-12-22 at 18 09 41@2x

Steps to replicate

At this line, I figure out the access token (query string) which is after filename has been url-encoded as below:

example.jpg%3FX-Amz-Content-Sha256%3DUNSIGNED-PAYLOAD%26X-Amz-Algorithm%3DAWS4-HMAC-SHA256%26X-Amz-Credential%3Dxxxxxxxxxx%2F20221222%2Fap-northeast-1%2Fs3%2Faws4_request%26X-Amz-Date%3D20221222T093208Z%26X-Amz-SignedHeaders%3Dhost%26X-Amz-Expires%3D3600%26X-Amz-Signature%3Dxxxxxxxxxx

It should be:

example.jpg?X-Amz-Content-Sha256=UNSIGNED-PAYLOAD&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=xxxxxxxxxx/20221222/ap-northeast-1/s3/aws4_request&X-Amz-Date=20221222T093208Z&X-Amz-SignedHeaders=host&X-Amz-Expires=3600&X-Amz-Signature=xxxxxxxxxx

I understand a filename without url-encode is unsafe, however it should be optimized for AWS s3 file access. Perhaps to split out query string before encode it.

Workaround

No response

lackneets avatar Dec 22 '22 10:12 lackneets

What does your filesystem disk configuration look like along with the configuration for your CMS file folders?

LukeTowers avatar Dec 22 '22 17:12 LukeTowers

What does your filesystem disk configuration look like along with the configuration for your CMS file folders?

Did you checked the source code I mentioned ? I don't think there has any relevant to my config. Anything besides this list column is correct.

Here is my plugin code:

<?php namespace My\Plugin\Models;

class Comment extends Model
{

    public $attachOne = [
        'image' => [
            'System\Models\File',
            'public' => false,
            'disk' => 's3',
        ],
    ];
}
// Tested, all got correct URL!
url($comment->image->path); // ✅
url($comment->image->getThumb(400, 400)); // ✅

But this is bad: (source)

use System\Classes\ImageResizer;

// Got wrong encoded URL
ImageResizer::filterGetUrl($comment->image, 40, 40); // 👎🏼

Furthermore, I also tested upload files to media manager. Everything is OK.

And here is how my config looks like:

<?php
// config/cms.php
return [
    'storage' => [
        'uploads' => [
            'disk' => 's3',
            'folder' => 'dev/uploads',
            'path' => 'https://my-bucket.s3.amazonaws.com/dev/uploads',
            'temporaryUrlTTL' => 3600,
        ],
        'media' => [
            'disk' => 's3',
            'folder' => 'dev/media',
            'path' => 'https://my-bucket.s3.amazonaws.com/dev/media',
        ],
        'resized' => [
            'disk' => 's3',
            'folder' => 'dev/resized',
            'path' => 'https://my-bucket.s3.amazonaws.com/dev/resized',
        ],
    ],
];
<?php
// config/filesystems.php
return [
    'default' => 's3',
    'disks' => [
        's3' => [
            'bucket' => 'my-bucket',
            'driver' => 's3',
            'endpoint' => null,
            'key' => 'AKI..................2A',
            'region' => 'ap-northeast-1',
            'secret' => 'Fxt49.............8TE',
            'stream_uploads' => false,
            'url' => 'https://my-bucket.s3.amazonaws.com/',
            'use_path_style_endpoint' => false,
        ],
    ],
];

lackneets avatar Dec 23 '22 01:12 lackneets

Workaround

Currently I will solve this problem by adding custom list column to replace the image resize function.

CleanShot 2022-12-23 at 15 10 28@2x
class Plugin extends PluginBase
{
    public function registerListColumnTypes()
    {
        return [
            'thumbnail' => function ($value, $column, $record) {
                $image = null;
                $config = $column->config;

                // Get config options with defaults
                $width = isset($config['width']) ? $config['width'] : 50;
                $height = isset($config['height']) ? $config['height'] : 50;
                $options = isset($config['options']) ? $config['options'] : [];
                $fallback = isset($config['default']) ? $config['default'] : null;

                // Handle attachMany relationships
                if (isset($record->attachMany[$column->columnName])) {
                    $image = $value->first();

                    // Handle attachOne relationships
                } elseif (isset($record->attachOne[$column->columnName])) {
                    $image = $value;

                    // Handle absolute URLs
                } elseif (str_contains($value, '://')) {
                    $image = $value;

                    // Handle embedded data URLs
                } elseif (starts_with($value, 'data:image')) {
                    $image = $value;

                    // Assume all other values to be from the media library
                } elseif (!empty($value)) {
                    $image = MediaLibrary::url($value);
                }

                if (!$image && $fallback) {
                    $image = $fallback;
                }

                if ($image) {
                    // $imageUrl = ImageResizer::filterGetUrl($image, $width, $height, $options);
                    // 🔥 KEY POINT: replace to this:
                    $imageUrl = $value->getThumb($width, $height, $options);
                    return "<img src='$imageUrl' width='$width' height='$height' />";
                }
            },
        ];
    }
}

columns:
    # ...
    image:
        label: image
        type: thumbnail
        options:
            mode: crop

lackneets avatar Dec 23 '22 07:12 lackneets

@lackneets can you try replacing the problem lines in ImageResizer.php with the following:

// Ensure that a properly encoded URL is returned
$url = \Winter\Storm\Router\UrlGenerator::buildUrl($url);

LukeTowers avatar Feb 26 '23 14:02 LukeTowers

@lackneets are you able to test my proposed fix? If it works I can merge it. Otherwise it'll have to wait until I or someone else can reproduce the original issue and test the fix.

LukeTowers avatar Apr 25 '24 23:04 LukeTowers