Allow Imagemagick to work with remote streamwrappers (s3)

I wonder whether #1522328: Use Imagick php extension instead of convert binary would resolve this more elegantly, as one would expect that the PECL extension is able to natively handle PHP stream wrappers correctly?

The code looks good to me, but I'm not able to test.

Would be great to get test results and confirmations from other users. Make sure to test both remote and local URIs.

Works wonderfully for me!

Works perfectly!

The patch looks good.

+++ b/imagemagick.module
@@ -371,10 +371,20 @@ function _imagemagick_convert($source, $destination, $args) {
-  $source = drupal_realpath($source);
+  $source = file_create_url($source);

If the file is local, there is no need to user its URL. For good performances, an URL should not be used for local files.

The attached patch avoid using URLs for local files. It also add support for URLs without scheme (as currently produced by the amazons3 module).

The first patch submitted by @aasarava works great! However, @mongolito404 is onto something with performance which sounds great. That patch (#7) did however not apply cleanly. Perhaps you can make a new one with a "clean"/fresh module install?

Patch in #7 apply cleany on version 1.0 when used in a Drush make file with:

projects[imagemagick][version] = 1.0
projects[imagemagick][patch][1695068] = http://www.drupal.org/files/issues/imagemagick-support_remote_files-1695068-7.patch

I also tested to apply the patch manually to a clean git checkout on the 7.x-1.x branch (at e348bb6a17c799d48066c75c22e4b16ec7a329fa) using both the patch and git apply commands without any issue.

Patch works great.
However, it doesn't seem to be working when the source image is over https.

We are using this patch in production, with HTTPS (using Amazon S3). So it does work with Amazon S3 file over HTTPS. Could you try running image magick manually on your HTTPS image ? Is your HTTPS hosting using a self-signed certificate, or a certificate with a root CA not registered system wide. ImageMagick does the content retrieval, so it does the HTTPS handling.

Perhaps the feature could be enhanced with a option to always force local download instead of using HTTP(S) URL as sources. But IMHO, it should be handled in a separated issue.

This patch is working for me along with the 2.x branch of the AmazonS3 module. A few questions:

1. +++ b/imagemagick.module
   @@ -371,9 +371,27 @@ function _imagemagick_convert($source, $destination, $args) {
   +    $source = file_create_url($source_original);
   

   It wasn't clear to me until I dug into convert that it could handle http / https URLs. We should probably document that here.

   But, what about files that may not have an external URL? Is it safer to copy the image to temporary:// and use that instead?

2. +++ b/imagemagick.module
   @@ -371,9 +371,27 @@ function _imagemagick_convert($source, $destination, $args) {
   +    $base = drupal_basename($destination_original);
   

   Can we use drupal_tempnam here?

1. Indeed support for remote files without an external URL is doable, via a local copy.
2. Yes, drupal_tempnam should be used here.

This uses drupal_tempnam instead.

Using drupal_tempnam() that way doesn't seem to work, I get errors like this:

ImageMagick error 1: convert: unable to open image `temporary://imagemagick_d7BEpz': No such file or directory @ error/blob.c/OpenBlob/2638. convert: WriteBlob Failed `temporary://imagemagick_d7BEpz' @ error/png.c/MagickPNGErrorHandler/1728. in _imagemagick_convert_exec()

Patch from #7 works nicely.

I don't really see why #7 would work and #14 wouldn't, they do almost the exact same thing (see the interdiff), and #14 works great for us. Maybe you were using #7 and switched to #14, causing it to get confused or something?

I had issues with imagemagick on my prod server running Nginx, even if it worked fine on devserver (Apache) with #7 patch.
Neither patch #7 or #14 applied to a fresh download of last dev, worked for me on prod/nginx.

However patch on #2502999: Imagemagick doesn't handle PHP streams did work.

Maybe it's worth merging those two initiatives for a commit to a stable release, just saying.
Thanks.

I applied the patch mentioned in #14, but am still getting this warning - Warning: getimagesize(): Filename cannot be empty in image_imagemagick_get_info()

and it is unable to generate the image derivate with following exception -
Exception: Amazon S3 was unable to create an image style derivative. Check the temporary directory configuration and permissions.

you can ignore #18, It was issue with the site level module not having the patch.

Added d8 version. :)

Lets change the version to 8.x-1.x, we can go back to 7.x when it is committed.

Thanks @yongt9412

In fact, the hooks of the module have changed from version 7, see #2568619: Plan for ImageMagick 8.

My suggestion would be rather than this, develop the following:

1. in hook_imagemagick_pre_parse_file_alter(), copy the remote source file to a local temp file, and set the local source path to the realpath of the temp file (via $toolkit->setSourceLocalPath($source);). Actually I am not sure how the current patch would work since ImageMagick binaries do not take URLs in input? (the base implementation of the hook resolves public://, private:// and temporary:// to their realpaths)

2. in hook_imagemagick_arguments_alter(), set the local destination path to again a local temp file if the real destination is remote

3. move the logic to copy back the temp file to the final destination to an implementation of hook_imagemagick_post_save_alter() which is kicked after the convert process is finished. This is currently not implemented as hook_imagemagick_arguments_alter() already maps local stream wrappers to their realpath and no file movement is needed.

This way, we would have ImageMagick's 'convert' binaries get passed local files for input and output, and all the logic to move from/to remote in the hooks.

BTW this could also live in a separate module, but I am fine to get something like this in as long as we do not hardcode specific stream wrappers.

Also, this needs manual testing and confirmation that it actually works, with detailed use-case steps (most of the automated tests are skipped on DrupalCI since ImageMagick's binaries are not installed on bots...)

Yes, we just started with this. Thanks for the detailed feedback.

We're working on this in combination with https://www.drupal.org/project/s3fs and will test it manually on our side using the s3fs:// stream wrapper provided by that module.

We will provide steps to test using that module.

Thank you for such detailed feedback, I still need to test this manually but it would be good if I can get some feedback.

Well for sure nothing will happen if you put code in the api.php file :) that's purely documentation.

You will need to put the code either in imagemagick.module hook implementations (like you did in previous patch), or implement hooks in the module file of the s3fs module (but in that case, please open an issue on that module's issue queue).

In general:

a) in pre_parse_alter, I'd recommend to append the remote file extension to the temp file (e.g. jpg, gif, etc). That would help ImageMagick's identify command to recognise the image file format.

b) in arguments_alter, you need to check if getDestinationPath() refers to a remote file. If so, then create a temp file as well, and setDestinationLocalPath() to that temp file. You do that in your code, but you're not checking if the destination is remote upfront.

c) in post_save_alter, check again if getDestinationPath() refers to a remote file. If so, file_unmanaged_move($toolkit->getDestinationLocalPath(), $toolkit->getDestinationPath());

So sorry, that is totally true, I think should be better in this way. :)

I have no means to test this, sorry, but it looks like this is going in the right direction. So, purely code review:

1. +++ b/imagemagick.module
   @@ -27,6 +27,12 @@ function imagemagick_imagemagick_pre_parse_file_alter(ImagemagickToolkit $toolki
   +        $temp_path = tempnam('temporary://', 'imagemagick_');
   

   I think we should use \Drupal::service('file_system')->tempnam() instead of pure PHP tempnam, but I am not sure.

2. +++ b/imagemagick.module
   @@ -27,6 +27,12 @@ function imagemagick_imagemagick_pre_parse_file_alter(ImagemagickToolkit $toolki
   +        $temp_path .= $toolkit->getSourceFormat();
   

   getSourceFormat() will return the ImageMagick format which may not match the actual extension. Better just use pathinfo($source, PATHINFO_EXTENSION).

3. +++ b/imagemagick.module
   @@ -58,6 +64,12 @@ function imagemagick_imagemagick_arguments_alter(ImagemagickToolkit $toolkit, $c
   +          $temp_path = tempnam('temporary://', 'imagemagick_');
   

   same as above

4. +++ b/imagemagick.module
   @@ -58,6 +64,12 @@ function imagemagick_imagemagick_arguments_alter(ImagemagickToolkit $toolkit, $c
   +          $temp_path = tempnam('temporary://', 'imagemagick_');
   +          $path = file_unmanaged_move($toolkit->getSource(), $temp_path);
   +          $toolkit->setDestinationLocalPath(\Drupal::service('file_system')->realpath($path));
   +          $toolkit->setDestination($toolkit->getDestinationLocalPath());
   

   you should not need to move the file, just create the temp and set it to local destination path. Also, why setDestination()? That would override the final destination with the local one, and later save_alter will not be find the final target destination anymore.

Finally, just a general comment - I think this is fine but would somehow force the logic of copying from remote to local and viceversa for any non-local stream wrapper. Theoretically contrib may add non-local streams for which they would like to manage the process differently, and implementing this in this module will make that more difficult. How about moving this patch to s3fs module, and implement imagemagick's hooks there? That would have the benefit of keeping this module 'agnostic' of how files are made accessible to ImageMagick's binaries as local files.

Maybe I'm overkilling though ;) opinions really appreciated.

It makes sense to me to do this here as a default. There are many possible remote stream wrappers and it seems like a safe assumption that they will need something like this, because out of the box, they will definitely not work.

And if someone really wants to do something differently, can't they just implement the alter hook and let them be called after imagemagick and override what we did?

@Berdir yes, that makes sense. Probably imagemagick hooks should be last though, so that if no local paths are set by other modules already, then defaults kick in. But that order will have to be managed by hook_module_implements_alter in the modules, I guess.

So OK, when we have an RTBC patch here along #27 and #28 I will commit it.

Thank you so much for the feedback. Changes made based on comment #28 . We tested manually seems to be working fine. Here there are some steps to reproduce and test:
- Install s3fs (https://www.drupal.org/project/s3fs) module.
- Add your keys, bucket settings, region and root folder on settings.
- Create/Edit a content type and add a image field, in field settings select 'Amazon S3 files' on 'Upload destination'.
- Add a new node, upload an image, you will see the image source is taken from an external amazon link.

Re. #31:

1. some comments in the code would help :)

2. +++ b/imagemagick.module
   @@ -86,3 +97,14 @@ function imagemagick_imagemagick_arguments_alter(ImagemagickToolkit $toolkit, $c
   +    file_unmanaged_move($toolkit->getDestinationLocalPath(), $toolkit->getDestination());
   

   when moving to final destination post_save, shouldn't we add FILE_EXIST_REPLACE argument to file_unmanaged_move to avoid potential file renames?

Changes made based on comment #32.

An evolution of #33, I could find a way to add some automated tests too, that can only run locally, though.

Marking as RTBC based on #2713241-5: Division by Zero Error. Will wait a bit longer if any more feedback before committing it.

If the remote file has an URI, you want to use that URI instead of copying locally for better performance as done in #7 (ie. there is no need to copy a large file if only parts of it are needed, this is useful when dealing with video or large PDF). Not sure why this was removed (instead of just adding the separate handling of remote files without a public URL).

#36: in ::parseExifData, there is a call to exif_read_data(). This PHP function does neither support URLs nor stream wrappers (see #2279381-7: Does Not Work with ImageMagick, and https://bugs.php.net/bug.php?id=65187), so copying from remote to a local temp seems the only safe way to support all the functionalities.

... and again, per #29 and #30 this is a default implementation for remote files support via hooks. Other modules may override that implementation with an alternative one.

Committed #34 to 8.x-1.x. Thanks all!

Back to 7.x-1.x - not sure if for a backport or an alternative implementation.

#37: Does the ImageMagick module itself calls exif_read_data(). If not, then I don't see why it has to be it responsibility to the module to ensure local files.
#38: file_create_url() is a generic solution to retrieve the URL a file,it is a safe core defined API. It is not specific to any stream wrapper implementation. Adding support for it does not add much complexity and provide sane defaults behavior. I'm all for copying file locally when they don't have an URL, but ignoring a simple solution to an actual performance issue is really not nice toward the users.

If IM itself does not need the file to be local, it should not penalize everyone with poor performances because users of an additional module need local files. The default behavior should be to use URL when available and only failback to local copy when needed. Then if an additional modules requires local files, it should ensure itself by overriding the implementation or by adding a check for local files before processing them.

If IM itself need local files, it should ensure local files are used only when required. Many users of the IM module could use it without having the need for the features requiring EXIF metadata (I've several site running with the URL patch without issue). Again, penalizing their performances because other users require local file does not seems fair to me (given that the solution is relatively easy to implement).

#41:

Feedback below relates to D8 version only.

Does the ImageMagick module itself calls exif_read_data()

The D8 version, yes. It does so in ImagemagickToolkit::parseExifData which is used to get EXIF orientation in case the image parsing is done via PHP getimagesize() instead of ImageMagick's identify binary.

• if we were to use the URL, the file will need to be fetched twice from remote - once for parsing (via identify or via getimagesize) and once for convert. Not sure this is a performance gain vs copying to local once and then running both steps from local.
• if you believe you can read from URL direct, then the thing you can do is implement hook_imagemagick_pre_parse_file_alter in another module, use ImagemagickToolkit::setSourceLocalPath() to set the local path (which is the one used to be passed to the binaries via proc_open) to the file URL, and ensure that hook implementation is executed before ImageMagick module's one. With that, the local path will be already set when ImageMagick's will execute its hooks that in that case will just skip execution.

With that said, I am actually open. I can also revert the commit in #39 if there is consensus. I committed based on #31 and #35, but also looking at comment #12.

Setting back to RTBC for 8.x-1.x, opinions/comments welcome.

actually NR seems a better status

@pbuyle I'll leave commit in #39 in, there has been no further feedback. Please open a new issue for the D8 version if you want to continue latest discussion. Setting this back to the D7 version.

If IM itself does not need the file to be local, it should not penalize everyone with poor performances because users of an additional module need local files. The default behavior should be to use URL when available and only failback to local copy when needed.

This patch was applied just a day before "ImageTragick" was released onto the world: https://imagetragick.com/

This set of exploits allows arbitrary code execution on the server, so it's being taken pretty seriously by systems administrators.

The recommended fix for this vulnerability includes adding the following to the ImageMagick policy.xml file:

<policymap>
  <policy domain="coder" rights="none" pattern="EPHEMERAL" />
  <policy domain="coder" rights="none" pattern="URL" />
  <policy domain="coder" rights="none" pattern="HTTPS" />
  <policy domain="coder" rights="none" pattern="MVG" />
  <policy domain="coder" rights="none" pattern="MSL" />
  <policy domain="coder" rights="none" pattern="TEXT" />
  <policy domain="coder" rights="none" pattern="SHOW" />
  <policy domain="coder" rights="none" pattern="WIN" />
  <policy domain="coder" rights="none" pattern="PLT" />
</policymap>

The exclusion of "URL" and "HTTPS" means that remote URLs are entirely disabled.

So all that is to say, the D8 patch fixed this problem before it was actually required. For sites going forward, they almost certainly will need to copy files down locally prior to running ImageMagick on them.

For those users like myself that were using the D7 version of the patch in comment #7, that version did NOT copy files down locally first, and so when our system administrator disabled remote URLs in ImageMagick, our S3-hosted images stopped being resized.

So the D7 patch needs to be rerolled to match the D8 approach, which should avoid this problem.

Here's a D7 port of the D8 patch that copies down the remote file prior to attempting to run ImageMagick on it. After the operation is completed, the temporary file is deleted.

The policy change is not the mandatory fix, it's a recommended way to mitigate the issues. At some point actual fixes for the security issues may be issued and usage of remote URLs (and ) will safe again.

If you don't deal with UGC content and trust the few users who are allowed to upload IM processed files on your server, then you don't have to block remote URLs (or any other IM delegates)). In our case, forcing a local copy of the file is a big performance hit and using remote URL is a low security risk. So in our case, using remote file is a must and we could argue that the D8 patch is broken.

There is valid use cases for both approaches. Instead of removing one in favor of another, could we have both available and a configuration variable to switch between them?

The approach in #46 assumes that only stream wrappers for local files exist. If you're e.g. using remote_stream_wrapper (which adds http: / https: streams) the check via drupal_realpath() will succeed even though the file is remote.

Adds a whitelist for local streams.

The policy change is not the mandatory fix, it's a recommended way to mitigate the issues. At some point actual fixes for the security issues may be issued and usage of remote URLs (and ) will safe again.

It's not mandatory but it will be the way many sites deal with the problem. And you're right that eventually there will be real fix, but until then, copying the file locally is great because it not only works with the current mitigation recommendation, but it also makes currently vulnerable sites safe, even if they don't implement the current fix.

In our case, forcing a local copy of the file is a big performance hit and using remote URL is a low security risk. So in our case, using remote file is a must and we could argue that the D8 patch is broken.

I think you may be overstating the performance hit. The remote file is going to have to be transfered via HTTP between the two servers regardless of whether we transfer it manually or leave ImageMagick to transfer it for us. Saving it to disk will have an impact but only a fraction of the time it takes to transfer the file in the first place, or to do the manipulations on it.

To test the difference, I made up a bash script that uses ImageMagick directly vs. copying the remote file down first:

	Time 1 (remote)	Time 2 (curl + local)
	3183	3227
	3407	3951
	3219	2672
	2841	2840
	2939	2971
	2760	3257
	3656	3132
	2988	3435
	2839	2754
	3085	2981
Average	3091.7	3122
Std deviation	281.5114168	376.3405668

Times are all in milliseconds.

The wild variance between times is mostly due to differences in HTTP transfer speed. This was on a 40Mbit connection to a traditional HDD (not SSD). The difference in deviation makes the two statistically equivalent. Though if you calculate it out, the direct transfer in this sample set was 0.1% faster.

So IMO, making an option is unnecessary. If we find adding an option is worthwhile, then let's make it a follow-up, otherwise we'll have to wait for the option to be put into the D8 version before this is fixed in D7.

Oh, here's my bash script for anyone wanting to test on their own environments. The file transfered was this image from NASA: http://www.nasa.gov/sites/default/files/thumbnails/image/ngc1333.jpg, a 4.5 MB JPG. Resized down to 220px.

The approach in #46 assumes that only stream wrappers for local files exist. If you're e.g. using remote_stream_wrapper (which adds http: / https: streams) the check via drupal_realpath() will succeed even though the file is remote.

This is a good point too. And actually adding this hidden list of local streams would potentially solve the request for making an option to disable the copying down first. You could add "s3" or whatever other remote system you have to the list, and then the file wouldn't be copied down.

So the new patch is +1 from me.

EDIT: On further checking the updated patch would not accommodate the ability to disable downloading locally first.

+  if (!$source || !in_array($scheme, $local_streams)) {

$source will still be empty on any S3 installation, so the local copying will happen even if the "s3" stream wrapper were added to the new "imagemagick_local_streams" variable.

I think the patch in #49 is ready to go. It matches the D8 approach. Although there is no option to disable copying down the file, we should consider adding an option for it in a separate issue for both the D8 and D7 versions. Right now my experience is indicating there's a negligible performance impact copying files down locally and a large benefit (for both security and compatibility). And in any case I'd hate to see this issue held up if have to fix the D8 version first before finally making ImageMagick work with the AmazonS3 module for Drupal 7. As it is now, without at least some fix, ImageMagick doesn't work at all with S3.

The patch in #49 does not fix the call to drupal_realpath($image->source) in </code> in <code>image_imagemagick_get_info. Tha attached path remove this usage of drupal_realpath(). image_imagemagick_get_info() is wrapper around getimagesize() which already support remote streams.

+1 Thanks @pbuyle. That change is also in our production version, I didn't realize that it wasn't in the ImageMagick module already.

Should we set this as fixed then?

@yongt9412 I don't see any commit to the 7.x-1.x branch.

Yeah, #55 still needs to be committed. @pbuyle++

Thank you all! @pbuyle would you commit your changes to 7.x?

@muellm I can't commit, I'm not a maintainer of this module.

So if I am using the remote_stream_wrapper module (https://www.drupal.org/project/remote_stream_wrapper) do I need this patch?

@SocialNicheGuru Yes, the patch is needed if you want to manipulate image on any kind of remote file using Imagemagick.

Confirming the RTBC for the 7.x-1.x branch.

This patch has two critical bugs in the way it handles the temporary files.

1. The call to drupal_tempnam('temporary://', 'imagemagick_'); creates a file called e.g. /tmp/imagemagick_EshYd. This file is then dropped on the floor and forgotten about when $source_temp and $destination are appended with the image's file extension.

Two months of operation on a lightly visited website has left that site's /tmp folder with almost 220 thousand files in it. Fortunately, they're all 0-length files, but this is obviously still quite undesirable.

2. If the convert operation fails for any reason, the temporary source file is dropped on the floor and forgotten about. This leaves non-empty files littering the /tmp folder, filling up the disk.

The attached patch handles both of these issues. I'm sorry that it's not an upgraded version of the patch from #55, but I don't know how to do that. You need to apply both #55 and this patch to get the fix.

Fortunately, bug #1 was already solved in the commit on the 8.x branch from back in June. I don't think bug #2 was, though.

Thanks for noticing that @coredumperror, here's a patch that integrates #55 and #66 for 7.x

Something to note in the #67 patch (as this was an issue for me) is the final call to return (bool) file_unmanaged_move($destination, $destination_original); in _imagemagick_convert(). The default functionality (FILE_EXISTS_RENAME) renames the file which may be the desired functionality for some people. Others may need the file to be replaced (FILE_EXISTS_REPLACE). Just an observation to keep in mind in case anyone else is wondering why their filename has an _0 on it...

+1 for patch #67.

We used it to create image_styles over https, since the ImageMagick library doesn't support https uri's. (http://www.imagemagick.org/discourse-server/viewtopic.php?t=18321#p70654)

• source: external images over https (https://www.example.org/test.jpg)
• destination: styles in public:// (https://www.mysite.com/sites/default/files/styles/thumbnail/https/www.example.org/test.jpg)

+1 for patch #67.

As mentioned in #68, I would have thought that specifying the FILE_EXISTS_REPLACE should probably be used on the file_unmanaged_move(), rather than the default FILE_EXISTS_RENAME, at least in the context of using ImageMagick just for creating image style derivatives. Modules such as Javascript Imagecrop depend on this (which is what prompted me to dig deeper into the code).

Perhaps a check for /styles/ in the destination URI, and matching filenames in both source and destination would be sufficient checks to know whether to replace or rename?

Apart from that, the patch in #67 works great for me. Many thanks to those who made it work!

Patch #67 works good, thanks! As multiple people confirmed it works, I will change status to RTBC.

I was also seeing this error on Pantheon, the patch in #67 fixed it for me, thank you! +1 on RTBC.

+1 for patch in #67

+1. My derived images are now super happy with S3.