Proper image type checking (instead of extensions) for ImageSizer

[teppo]

We had a slight problem with ImageSizer recently; everything seemed to be fine but creating thumbnails still failed. The reason for this turned out to be that those images were of unsupported type (windows bitmaps) yet their extension was ".jpg" and thus ImageSizer considered them proper JPEG's and acted accordingly.

Anyway, sorry for the spam but this is the solution I ended up implementing. I'd be grateful if Ryan (or someone else who knows the core well) would take a look and tell me if this is a valid solution -- and if it is, I'd like to suggest changing current extension-based "filtering" to something more like this

I also took a look at the Upload class / InputfieldFile, but seems to me that implementing proper imagetype filtering there would require a lot of work.. and doesn't necessarily even make sense in that context.

--- wire/core/ImageSizer.php	(revision 1717)
+++ wire/core/ImageSizer.php	(working copy)
@@ -46,6 +46,12 @@
	);

+	   /**
+		* Type of image
+		*
+		*/
+	   protected $imagetype;
+
	/**
	 * Allow images to be upscaled / enlarged?
	 *
	 */
@@ -67,11 +73,10 @@
	 * File extensions that are supported for resizing
	 *
	 */
-	   protected $supportedExtensions = array(
-			   'gif',
-			   'jpg',
-			   'jpeg',
-			   'png',
+	   protected $supportedImagetypes = array(
+			   IMAGETYPE_GIF,
+			   IMAGETYPE_JPEG,
+			   IMAGETYPE_PNG,
			);

	/**
@@ -83,9 +88,10 @@
			$this->filename = $filename;
			$p = pathinfo($filename);
			$this->extension = strtolower($p['extension']);
+			   $this->imagetype = exif_imagetype($filename);
			$basename = $p['basename'];

-			   if(!in_array($this->extension, $this->supportedExtensions))
+			   if(!in_array($this->imagetype, $this->supportedImagetypes))
					throw new WireException("$basename is an unsupported image type");

			if(!$this->loadImageInfo())
@@ -130,12 +136,9 @@
			$source = $this->filename;
			$dest = str_replace("." . $this->extension, "_tmp." . $this->extension, $source);

-			   switch($this->extension) {
-					   case 'gif': $image = @imagecreatefromgif($source); break;
-					   case 'png': $image = @imagecreatefrompng($source); break;
-					   case 'jpeg':
-					   case 'jpg': $image = @imagecreatefromjpeg($source); break;
-			   }
+			   if($this->imagetype == IMAGETYPE_GIF) $image = @imagecreatefromgif($source);
+			   if($this->imagetype == IMAGETYPE_PNG) $image = @imagecreatefrompng($source);
+			   if($this->imagetype == IMAGETYPE_JPEG) $image = @imagecreatefromjpeg($source);

			if(!$image) return false;

@@ -143,7 +146,7 @@

			$thumb = imagecreatetruecolor($gdWidth, $gdHeight);

-			   if($this->extension == 'png') { // Adam's PNG transparency fix
+			   if($this->imagetype == IMAGETYPE_PNG) { // Adam's PNG transparency fix
					imagealphablending($thumb, false);
					imagesavealpha($thumb, true);
			} else {
@@ -155,7 +158,7 @@
			imagecopyresampled($thumb, $image, 0, 0, 0, 0, $gdWidth, $gdHeight, $this->image['width'], $this->image['height']);
			$thumb2 = imagecreatetruecolor($targetWidth, $targetHeight);

-			   if($this->extension == 'png') {
+			   if($this->imagetype == IMAGETYPE_PNG) {
					imagealphablending($thumb2, false);
					imagesavealpha($thumb2, true);
			} else {
@@ -170,20 +173,13 @@
			imagecopyresampled($thumb2, $thumb, 0, 0, $w1, $h1, $targetWidth, $targetHeight, $targetWidth, $targetHeight);

			// write to file
-			   switch($this->extension) {
-					   case 'gif':
-							   imagegif($thumb2, $dest);
-							   break;
-					   case 'png':
-							   // convert 1-100 (worst-best) scale to 0-9 (best-worst) scale for PNG
-							   $quality = round(abs(($this->quality - 100) / 11.111111));
-							   imagepng($thumb2, $dest, $quality);
-							   break;
-					   case 'jpeg':
-					   case 'jpg':
-							   imagejpeg($thumb2, $dest, $this->quality);
-							   break;
+			   if($this->imagetype == IMAGETYPE_GIF) imagegif($thumb2, $dest);
+			   if($this->imagetype == IMAGETYPE_PNG) {
+				   // convert 1-100 (worst-best) scale to 0-9 (best-worst) scale for PNG
+				   $quality = round(abs(($this->quality - 100) / 11.111111));
+				   imagepng($thumb2, $dest, $quality);
			}
+			   if($this->imagetype == IMAGETYPE_JPEG) imagejpeg($thumb2, $dest, $this->quality);

			unlink($source);
			rename($dest, $source);

... and yes, that's output from SVN diff. I hope you guys can cope with that / understand what's going on in there. I'm not very familiar with Git and how it handles stuff like this. Sorry.

[ryan]

Thanks for posting this! The SVN diff looks exactly like the Git ones. This seems like a good method, perhaps even in addition to extension filtering. But one thing I'm wondering is, does it change anything about the result? ImageSizer fails at it's own check rather than at GD's check, but seems like it would still fail. I'm just wondering how this solved the issue of BMP images renamed to JPGs?

[teppo]

Ryan, there are actually certain rather important things that this changes. Don't know if I can explain this properly without a huge pile of screenshots, but I'll try anyway

First of all, if only extension is checked at ImageSizer and image cannot be resized, currently it fails silently and Pageimage thinks it has a valid thumbnail. What it actually has is just a copy of the original image and when asked for thumb, it outputs that, resulting in rather awful situations layout-wise.

By applying proper image type filtering, instead of failing silently ImageSizer throws an exception and thus Pageimage won't think it has a valid thumbnail. IMHO this is a much better solution -- and makes it very clear to the developer what's going on there. This is actually how this would've solved our problem; current behavior made us somewhat confused because there was absolutely no error, everything seemingly went smoothly and only when we started testing those oddly behaving images with file-command did we realize that they were actually bitmaps.

Same thing, by the way, happens at the admin UI if the image field has "Display thumbnails in page editor?" checked. Without image type filtering it outputs full copy of the original image, but when image type is checked properly it fails ("bitmap.0x100.bmp is an unsupported image type.")

I do realize that this is rather rare scenario really, but what I'm saying here is that current filtering isn't (in my opinion) valid. Extension is just something someone has decided to write there and we have no way to confirm if it's actually useful in any way. Also, currently ImageSizer can easily be fooled to send invalid data to GD -- I'm not sure if there's any real problem related to this, but I'd still be a bit careful, 'cause it might in some rare cases interpret that data in strange ways..

[ryan]

This makes sense, thanks for the great explanation. I will pull this in to the ImageSizer class to have it check both extension and type.

[ryan]

Teppo, the dev branch now now has switched from extension detection to type detection per your suggestion and code.

https://github.com/ryancramerdesign/ProcessWire/blob/dev/wire/core/ImageSizer.php

Thanks,

Ryan

[teppo]

That's good to hear, thanks Ryan!

[horst]

Hi, there is a post here describing some strange behave. I don't really know whats going on there and if the ImageSizer is invoked in this, but when looking into ImageSizer->resize (Line 284 and following) there is not really checked the result of write-to-file functions.

Maybe it would be better to do something like:

 $result = imagejpeg($thumb2, $dest, $this->quality);
...
if($result===false) return false;

or at least do a check at the end and return the result of $this->loadImageInfo() instead of always return true.

[ryan]

Horst, that's a good suggestion. I have gone ahead and added this additional check. I guess the thought was that if there was going to be a fail, it would have happened before it reached this point. But that's not a good assumption to make, so I think your suggestion definitely makes sense.