Imageproxy breaks SVG image links

[Makoto]

Demonstration:

Original link:

http://slack.taiga.sh/badge.svg

 

The reason this fails is because \IPS\Image cannot validate SVG images. The current method you are using attempts to validate images using hex signatures,

		$signatures = array(
			'gif'	=> array(
				'47' . '49' . '46' . '38' . '37' . '61',
				'47' . '49' . '46' . '38' . '39' . '61'
			),
			'jpeg'	=> array(
				'ff' . 'd8' . 'ff'
			),
			'png'	=> array(
				'89' . '50' . '4e' . '47' . '0d' . '0a' . '1a' . '0a'
			)
		);	

However, considering SVG is just XML and.. as far as I can tell, really has no solid standard you can reference for validation, I'm not sure how you'd prefer to handle this.

But basically, to reproduce, just link an svg image (feel free to use mine above) in a post and submit it with image proxy enabled.

The image link will first throw a 502 error in imageproxy.php at line 80, then 404's after that since it's still stored in the core_image_proxy table with NULL as its location (and thus, the images remain broken on sites with Imageproxy enabled).

[Makoto]

The simplest solution would be to just not try and parse SVG images using image proxy, and if you wanted to go that route,

DontParseSvgImageProxy.patch

[bfarber]

Thanks for reporting, I will open an internal bug report.

 

[Makoto]

Bumping as this issue can still arise. See the badge's here as an example:

   

In this case the issue is Github badges do not have file extensions, so you'll have to check the mimetype instead of just relying on the file extension to prevent these from being parsed.

[Makoto]

Here's another simple patch for this that checks the mimetype instead of just relying on the extension.

Assume you only really need to do this for SVG's since those are the only image types the Image class can't handle that I know of.

 

svg_mimetype.patch

[Makoto]

Also, the way you're currently handling URL's to parse extensions does not account for query strings. E.g,

Does not work. But,

Does.

If you're just checking for SVG, you should be only grabbing the first three characters are the ., not everything. But really, just checking the headers/mimetype is overall a better and more reliable solution. Image links do not always have have valid extensions (and they're not required to, they only need valid mimetypes).

[bfarber]

This is resolved for 4.5 already

[Makoto]

Is it resolved already or did you resolve it after I reported it

Dubious. Who else actually would have run into this issue and reported it?

Either way great, thanks, looking forward to it.

[Stuart Silvester]

Just now, Makoto

Is it resolved already or did you resolve it after I reported it

Dubious. Who else actually would have run into this issue and reported it?

Either way great, thanks, looking forward to it.

It was already resolved 👍

[Makoto]

Cool. Also @Stuart Silvester / @bfarber, what is the current cache policy here on proxied images?

Badges like those above are dynamic and update routinely, so a cache time longer than 24-72 hours would be less than ideal.

[bfarber]

I don't know off hand, but it's unlikely we're going to change it here. 😛 

[Makoto]

Boo. Okay, fair enough! I'll just watch it and see. If it takes like, a month to update or so, I'll probably just remove the more dynamic badges. Or manually update them maybe.