search

craftcms / cms

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

EXIF rotation data is stripped even when `preserveExifData` is true #6947

Open mattgrayisok opened 3 years ago

mattgrayisok commented 3 years ago

Description

When an image is uploaded with EXIF rotation meta it is always set to \Imagick::ORIENTATION_UNDEFINED even when preserveExifData is true

Steps to reproduce

  1. Set preserveExifData to true
  2. Upload an image containing EXIF rotation meta
  3. Download the image from its asset volume and check the EXIF data. Rotation will be set to Unknown (0)

Additional info

Offending line is here:

https://github.com/craftcms/cms/blob/14f0fd652b7a99820f804ba6c0a0db762bf26206/src/services/Images.php#L394

Which sets to unknown, but the check to see whether the EXIF data should be preserved is inside ImageHelper::cleanExifDataFromImagickImage on the following line. The image is then resaved with the unknown rotation regardless of any settings.

This is also true if the image has been rotated by rotateImagesOnUploadByExifData although in that scenario the broken rotation meta doesn't cause any issues.

mattgrayisok commented 3 years ago

To save you having to hunt for an image to test with:

https://mcc.id.au/2020/image-orientation/arrow-with-orientation.jpg

And this works pretty well for analysing EXIF data:

http://exif.regex.info/exif.cgi

andris-sevcenko commented 3 years ago

The point of that behavior is to avoid confusion. It's much simpler to just always strip out EXIF orientation data than to deal with the unpredictability of image being sideways after transforming (which for sure will strip EXIF data). I would blame the browsers for this predicament since they silently will always rotate an image according to its EXIF data.

Not sure what the correct solution is here.

mattgrayisok commented 3 years ago

Thanks for the explanation. I would lobby for the rotation EXIF to only be removed if Craft has actually performed a correction rotation.

So if rotateImagesOnUploadByExifData is false and preserveExifData is true the rotation info remains intact. This would lead to the following scenarios:

Defaults (rotateImagesOnUploadByExifData: true, preserveExifData: false):

Craft performs any applicable rotation. All EXIF data including rotation is removed. Nothing unexpected but image transforms are performed on-server (CPU usage + php worker locking).

rotateImagesOnUploadByExifData: false, preserveExifData: false:

Image is not rotated by Craft. All EXIF data is stripped (I'm fine with this default BTW in order to better avoid PII leaks from EXIF). Images now have no way to be rotated appropriately but the developer has opted into this and they should have a good idea where to start researching because they've explicitly disabled rotateImagesOnUploadByExifData

rotateImagesOnUploadByExifData: true, preserveExifData: true:

Craft rotates the image and then sets EXIF rotation to 0. All other EXIF data is maintained. This is probably the least likely scenario, but could be useful for anyone who needs to grab EXIF data using a downstream service; E.G images go to S3, lambda is triggered to parse and push EXIF into a data processing pipeline.

rotateImagesOnUploadByExifData: false, preserveExifData: true:

Craft does nothing. All EXIF is left intact. In this scenario some browsers will try to rotate the original image, others won't, but to get into this situation the dev needs to have changed two separate config options so I'd hope they know what they're doing. Importantly, this is the only option which allows external services to handle the rotation of images (Imgix, serverless-sharp, Servd's Assets Platform) and therefore free up CPU and PHP workers on the PHP servers.

A specific use case

This issue was identified when debugging a public marketplace hosted on Servd. Users are able to upload images of products for sale and, as you can imagine, during busy periods all of the image rotations can soak up a lot of CPU and PHP workers. The images themselves are subsequently served from Servd's Asset Platform, which is capable of performing the EXIF rotation off-server, but the data is always stripped by Craft so currently it cannot.

Let me know if any further details would be beneficial 👍

andris-sevcenko commented 3 years ago

Sorry for the delay on this.

Another thing to take into account here is if you don't re-save the image either by rotating or simply removing the EXIF data, then the image cannot be considered "safe", so Craft has to resort to auto-guessing the image quality and re-save the image (so that any malicious code gets cleaned).

So what is the desired solution for you here?