Fix relative SVG rendering

[ioquatix]

As discussed in isaacs/github#316

Essentially embedding SVG images into a README.md file doesn't work but probably should. It's apparently broken due to the content type being incorrect for the SVG file being served.

[gustavomdsantos]

+1

[derekstavis]

👍

[FranklinYu]

👍

[klorenz]

+1

[ghost]

👍

[dangnelson]

Wow, has this really been a bug for over a year?

[FranklinYu]

@dangnelson No, I don't think they even regard it as an issue. This is probably a "won't fix".

[kivikakk]

This isn't an issue with the Markup gem itself, but it certainly is a valid bug. Looking into a resolution now.

[ioquatix]

@kivikakk any update? Thanks!!

[kivikakk]

@ioquatix: we're discussing it currently! The biggest issue we need to overcome is security due to JS execution in SVG — allowing user-controlled scripts on a GitHub domain could be dangerous — so we're considering strategies to work in out. It might take a bit to get it fully ironed out, but I'm pushing this forward internally, so I'll let you know when there's any updates.

[ioquatix]

@kivikakk I thought that might be a problem. I'm glad you are thinking about it. The content is served from a raw.github.io right? What about using something like https://content-security-policy.com to protect your main domain?

[ioquatix]

@kivikakk Any update on this?

[kivikakk]

Unfortunately not yet; there's been a lot going on. We are still pushing forward with the issue (lots of moving parts), but slowly.

[ioquatix]

@kivikakk Perhaps this is a stupid comment/suggestion, and I understand what your are saying, but then how come SVG previews already work in this context? https://github.com/ioquatix/build-dependency/blob/master/partial.svg

[kivikakk]

@ioquatix when rendering a full blob, we use our internal "Render" system, which does sandboxed content rendering. Because it can take some time, there's also AJAX and a bunch of other things involved. (e.g. here's the actual path to the SVG renderer) This would look super ugly if used inline (all those AJAX spinners ...), and because it depends on an <iframe> for sandboxing, getting the width/height to propagate would be super awkward.

So we're looking into a solution that doesn't require using Render while still providing adequate security. (Currently thinking of sanitising the SVG ourselves, or a solution involving randomised subdomain names for each request to prevent cross-tab JS access.)

[ioquatix]

Oh, thanks for the awesome explanation.

That makes sense.

Well, I'm glad you are working on it and I'm certainly looking forward to be able to use SVG in my documentation/README :)

[ghost]

Relates to #270 , because SVGs can be Base64-encoded.

[ioquatix]

@kivikakk Sorry, just my monthly check for updates - how are things progressing? I'm still looking forward to putting SVG into my README.md - what about simply sanitising all scripts from the SVG?

[mrbinky3000]

@myurkin Your test wiki images are all broken again. Same with you @ptoomey3 Github, how do we add svg images to README.md files?

[gucu112]

I was not having problem with adding SVG image using relative path lately, just like this:
![Image](Image.svg)

Then, after pushing my changes to the repo I have right-clicked on the image (which was visible of course) and was able to see that the ?sanitize=true was added at the end of the image path.

It seems that the issue is resolved and can be closed.

[myurkin]

@gucu112 , the limitations described in my comment above - #556 (comment) - are still present. So the issue is only half-resolved.

[gucu112]

@myurkin Sorry, I didn't know it is still issue for wiki pages (as I was tested for README.md file).

[mkormendy]

@kivikakk Well it appears that any style tags found inside of the defs gets stripped by the sanitizer as well. For example, a font-face from google open source fonts @import'ed with CSS inside the SVG get stripped completely from the SVG file. This breaks the rendering of the image text.

Broken here: https://github.com/mkormendy/MechanicalSoup

Should look like:

[kivikakk]

@myurkin this has now (finally) been fixed!

@mkormendy we're mulling the security implications of allowing @import (we currently sanitise CSS in the svg, and obviously the contents of an @import cannot be sanitised). I'll let you know where we end up on this.

[kivikakk]

@mkormendy we did some digging to determine the implications of allowing this and ended up discovering it wouldn't make a difference if we did -- it turns out that all external resources will be ignored by browsers when an SVG is being displayed via an <img> tag. MDN documents this, and while I can't find any official documentation re: e.g. Chrome, my own local testing confirms that the browser makes no attempt to load the external resources. The W3 wiki notes:

The SVG document is not allowed to fetch any resources. This also applies to scripts, stylesheets or images.

So I think converting the SVG to a png as you have is as good as it gets.

[mojavelinux]

@kivikakk That's absolutely correct (based on my own research into this). Referencing an SVG using an <img> tag rasterizes the image and prevents it from further loading anything. It's basically inert.

Here's the most relevant literature I know about what SVG features are available when using the <img> tag: https://www.smashingmagazine.com/2014/11/styling-and-animating-svgs-with-css/#embedding-svgs

This is the key clause:

Styles and animations applied to an SVG using an external CSS resource will not be preserved once the SVG is embedded.

Basically, the SVG can't make any external references.

[FranklinYu]

If the font is small then data: URI is also a solution.

[longsonr]

@mkormendy we did some digging to determine the implications of allowing this and ended up discovering it wouldn't make a difference if we did -- it turns out that all external resources will be ignored by browsers when an SVG is being displayed via an <img> tag. MDN documents this, and while I can't find any official documentation re: e.g. Chrome, my own local testing confirms that the browser makes no attempt to load the external resources. The W3 wiki notes:

The SVG document is not allowed to fetch any resources. This also applies to scripts, stylesheets or images.

So I think converting the SVG to a png as you have is as good as it gets.

It's documented here: https://www.w3.org/TR/SVG2/conform.html#processing-modes

SVG when loaded as an <img> or CSS background-image (or any other way that's an image rather than a document) cannot execute script and must be self contained i.e. can't load any other external resources. This is basically so that your thought process for "should I support images" does not need to depend on also asking yourself whether the image is a raster image or an SVG image.

FWIW I'm one of the authors of SVG support in Firefox.

[mkormendy]

@longsonr thanks for following up with this. I guess the implications of this weren't fully planned out even though the code has partial allowance for this and has operational success in some instances. I love the work that the W3C has done to document web convention, however they are inherently monolithic in their approach, and as such, are prone to common pitfalls like incomplete strategies and speed of correction (or lack of awareness to correct in the first place).

That being said, I have no basis at the time for requiring this any further, but in the future someone might. Researching the correct security handling for such a scenario could be extensive, but I do believe (beyond the W3C) that there are others who have done part of the leg work for figuring out this branch of the requirement and it's just a matter of putting the pieces together. This is my final word on this.

[anton-rudeshko]

Hi all.

It's still an issue if image is too big and you want to make it a link pointing to a full screen display of SVG source.

[![alt](./assets/img.svg)](./assets/img.svg)

It works okay while viewed through MD, but on click it'll show "Sorry, this file is invalid so it cannot be displayed":

I expected that this raw + sanitize workaround will work, but no:

[![alt](./assets/img.svg)](./assets/img.svg?raw=true&sanitize=true)

GitHub drops sanitize=true while resolving raw url, as @myurkin mentioned in #556 (comment):

It seems that the problem (inside the wiki repo) boils down to not adding ?sanitize=true when transforming the relative path to svg into the absolute path at raw.githubusercontent.com

[Mizux]

@anton-rudeshko You should try
![alt](https://rawgit.luolix.top/<user>/<repo>/<branch>/assets/img.svg)

[anton-rudeshko]

Thank you for your suggestion, I'll try it. And yet I'm not sure if GH Enterprise have anything like rawgithub subdomain.

[FranklinYu]

@Mizux Note that it's a third-party website. Even worse it has reached End-of-Live. See https://rawgit.com/ (RawGit and raw-GitHub are the same website; both domains point to the same IP address.)

[RankJay]

Hey there !
I want to know why does my repo does show the SVG in ReadME file but not on my public profile.

Basically, I had made this SVG that should be showcased on my public profile, but due to some error its only showing in repo and not on my public profile