Jump to content

Imageproxy breaks SVG image links

Featured Replies

Posted

Demonstration:

badge.svg

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).

  • Author

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

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

badge.svg

 

  • 10 months later...
  • Author

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

GitHub GitHub tag (latest by date) GitHub issues GitHub last commit

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.

  • Author

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

  • Author

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

ips-dev-helper.svg?label=release

Does not work. But,

ips-dev-helper.svg

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).

This is resolved for 4.5 already

  • Author

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.

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 👍

  • Author

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.

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

  • Author

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.

Archived

This topic is now archived and is closed to further replies.

Recently Browsing 0

  • No registered users viewing this page.