Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and improve image validation when manipulating images #7666

Merged
merged 26 commits into from Mar 17, 2023

Conversation

jesseleite
Copy link
Member

@jesseleite jesseleite commented Mar 7, 2023

People are running into problems with glide server trying to process incompatible file types (see #7541), so we need whitelist which image formats get processed on upload, by glide tag, etc.

By default we are supporting these file formats...

CleanShot 2023-03-07 at 18 01 19

...But as @jacksleight mentioned here, imagick supports over 100 additional image formats with the proper dependencies installed on the server, so we're also making this configurable...

    /*
    |--------------------------------------------------------------------------
    | Additional Image Extensions
    |--------------------------------------------------------------------------
    |
    | Define any additional image file extensions you would like Statamic to
    | process. You should ensure that both your server and the selected
    | image manipulation driver properly supports these extensions.
    |
    */

    'additional_extensions' => [
        // 'heic',
    ],

This PR also expands mime type validation using Symfony's MimeTypes helper class, so that we can properly validate mime types for custom extensions configured as shown above. Before we were only allowing image/* mimetypes, but imagick can be set up to process PDF's (for example), so the mimetype could now be something like application/pdf.

Fixes #7541
Closes #7652

@jesseleite jesseleite added the bug label Mar 7, 2023
@what-the-diff
Copy link

what-the-diff bot commented Mar 7, 2023

  • Added ImageValidator class to validate image extensions and mimetypes.
  • Updated the uploader to use this new validator when processing source images on upload, instead of relying on Glide's exception handling (which is not very reliable).
  • Updated glide tag so that it doesn't try to process non-glideable files with a configured preset (eg: .md or .svg) - these will now be uploaded as normal assets without any errors/exceptions thrown by Glide during manipulation attempts. This also means we don't need an extra check in the tags for whether or not a file can be resized before trying to resize it via glide... which was unreliable anyway because some formats are supported by imagick but only if ghostscript is installed server side, etc.. Now all those edge cases should just work out of the box! :)
  • Also updated tests accordingly where necessary... eg: added test case for uploading eps files into container with presets defined; removed unnecessary checks from other tests since they're no longer needed due to changes above; fixed failing test caused by change First #1 above; etc..
  • Added ImageValidator facade and tests.
  • Updated ImageGenerator to use the new validators, which will prevent invalid images from being manipulated (and potentially crashing PHP).
  • Fixed a bug where Image::make() was called with an empty string or null value for some image types that don't have mimetypes defined in Laravel's MimeTypes class... this would cause Imagick/GD to throw errors when trying to manipulate those files as if they were images, so we now check against them before attempting manipulation at all! This should also help protect against malicious file uploads by preventing non-image files from getting through our validation checks and causing problems later on down the line during manipulations...

@jesseleite jesseleite changed the title Fix processing of svg's and pdf's on upload when using imagick Ensure only processable images are run through glide server Mar 7, 2023
@jacksleight
Copy link
Contributor

jacksleight commented Mar 8, 2023

Quick question on this: Will adding a format to additional_formats both allow it through the glide tag and cause it to be processed on upload?

Reason I ask is that could make sense for some formats (eg. HEIC), but not others (eg. PDF, where you might want to generate thumbnails but also keep the original file). Which additional formats you want processed on upload and which you want to be able to process through the tag might not be the same.

Appreciate that complicates things, guess the simplest solution is just to not use the process on upload feature in that situation.

@jasonvarga
Copy link
Member

Will adding a format to additional_formats both allow it through the glide tag and cause it to be processed on upload?

Yes. The solution I suppose would be to put PDFs in a separate container.

@jesseleite
Copy link
Member Author

jesseleite commented Mar 8, 2023

Will adding a format to additional_formats both allow it through the glide tag and cause it to be processed on upload?

To be clear, the only thing causing any images to be processed on upload (with regards to the original uploaded file), is your asset container settings here:

CleanShot 2023-03-08 at 10 22 06

If you set a "Process Source Images" preset there, then @jasonvarga is correct; Otherwise the additional_formats config will only affect the cache generated by the glide tag.

@jesseleite jesseleite removed the bug label Mar 9, 2023
@jesseleite jesseleite changed the title Ensure only processable images are run through glide server Fix and improve image extension and mimetype validation when manipulating images Mar 9, 2023
@jesseleite jesseleite marked this pull request as ready for review March 9, 2023 21:33
@jesseleite jesseleite changed the title Fix and improve image extension and mimetype validation when manipulating images Fix and improve image validation when manipulating images Mar 9, 2023
@jasonvarga jasonvarga force-pushed the fix/processing-source-images-with-svgs branch from 0ce1755 to b93343c Compare March 17, 2023 15:50
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.

Processing source images with imagick breaks SVG's and PDF's when using Imagick
4 participants