Rendering issues with output

[mathiasbynens]

See https://github.com/GoogleChromeLabs/jsvu/blob/be49b12f5393cb2681a4cdac73ad48d9245f3dac/README.md#jsvu- — any idea what’s going wrong here?

The relevant commit is https://github.com/GoogleChromeLabs/jsvu/commit/be49b12f5393cb2681a4cdac73ad48d9245f3dac and the command used to create the SVG is:

svg-term --cast=rfS1M5ynKm1hGaBqJYJj0mGCi --frame --at=43250 --out=screenshot.svg

[marionebl]

Looking at the svg file I can't see any issues. The file is delivered with a wrong Content-Type header, though:

Server: GitHub.com
Content-Type: text/html; charset=utf-8
Location: https://raw.githubusercontent.com/GoogleChromeLabs/jsvu/be49b12f5393cb2681a4cdac73ad48d9245f3dac/screenshot.svg?sanitize=true
X-Runtime: 0.056509

[marionebl]

I created a PR on jsvu adressing this by using rawgit.com:

https://github.com/marionebl/jsvu/blob/32353c45a135526bd81b3dce31a5426ab77648c7/README.md

We should probably mention the Content-Type issue somewhere, thinking about the best place to document it.

[mathiasbynens]

It’s not just the Content-Type header. The ?sanitize=true part of GitHub’s auto-generated URL hints that it sanitizes the SVG, stripping font styles, among others. Compare:

• https://raw.githubusercontent.com/GoogleChromeLabs/jsvu/master/screenshot.svg
• https://raw.githubusercontent.com/GoogleChromeLabs/jsvu/master/screenshot.svg?sanitize=true

It’s a little silly that GitHub forces sanitization in this case, as <img src=svg> has no security issues. +cc @mastahyeti

[btoews]

I think the concern was that people can browse directly to the SVG in addition to having it included in a page via an image tag. Javascript from that SVG could then access other browser tabs on the raw.githubusercontent.com domain.

/cc @ptoomey3 since I think he knows more about our SVG sanitization than me.

[ptoomey3]

Yeah..that is basically a summary of the issue. If content in one raw tab has a handle to another raw tab, it could access raw content that it should not be able to. So, despite the raw subdomain being a sandbox domain, we still treat it as a domain where untrusted user content must be considered.

[ptoomey3]

/cc @kivikakk for the content-type question.

[kivikakk]

Is that Content-Type being served on the ?sanitize=true URL? It should only appear on the un-sanitized version.

[ptoomey3]

I thought the unsanitized would be text/plain.

[kivikakk]

True, and it is:

$ curl -Is https://raw.githubusercontent.com/GoogleChromeLabs/jsvu/be49b12f5393cb2681a4cdac73ad48d9245f3dac/screenshot.svg | grep -i ^content-type
Content-Type: text/plain; charset=utf-8

I'm not sure what URL was fetched in @marionebl's example. It looks like a redirect, given Location:?

[mathiasbynens]

I think the concern was that people can browse directly to the SVG in addition to having it included in a page via an image tag. Javascript from that SVG could then access other browser tabs on the raw.githubusercontent.com domain.

+

Yeah..that is basically a summary of the issue. If content in one raw tab has a handle to another raw tab, it could access raw content that it should not be able to. So, despite the raw subdomain being a sandbox domain, we still treat it as a domain where untrusted user content must be considered.

The Accept header for such requests (accessing the SVG URL directly) would look different, though. If it doesn’t contain text/html, application/html, application/xhtml+xml, nor application/xml (you could just check for the substring xml to cover those last three), it’s safe to send back the SVG even if it contains scripts. Pseudo-code:

if (accept.includes('text/html') || accept.includes('xml')) {
  sanitize();
} else {
  // No need to sanitize.
}

Or, to go with a safelist approach, you could check if all entries in the Accept header that are not */* start with image/. In that case, there’s no need to sanitize.

[btoews]

That sounds like it would work, but I don't think we're willing to rely on the assumption that all versions of all browsers always have and always will send sensible Accept headers. So long as we're serving all raw user content off the same domain, I think we'll want to remain conservative with what content we'll serve with correct content-types.

[marionebl]

Boiling this down to the use case of svg-term I wonder if not stripping any CSS from SVG for sanitized URLs could be a way foward? @mastahyeti @kivikakk @mathiasbynens

If I understand the entire context correctly this should allow CSS animations in SVG to work and create no new security issues with SVGs viewed as document, right?

[marionebl]

@adius has hit the ?sanitize=true issue in #25, too

[adius]

Not stripping the CSS sounds like a good idea.

If this is not going to happen you could also make svg-term-cli only use native SVG attributes

[adius]

Here is an example where I replaced the CSS with SVG attributes. Seems to work. https://github.com/adius/svg-term-bug

[adius]

Using rawgit.com only works for public projects, but not for private ones...

[adius]

I also tried to automatically replace the CSS with attributes using SVGO, but it only support this for inline styles 😞

[marionebl]

@adius Using attributes also is not possible for animated renderings.

[adius]

@marionebl Yes it is! You can use SMIL (e.g. https://adriansieber.com/waity/) to to so, but they want to remove support for it in future SVG versions (by the pace of SVG development this can be decades away 😂)

[kivikakk]

https://github.com/GoogleChromeLabs/jsvu/blob/be49b12f5393cb2681a4cdac73ad48d9245f3dac/README.md#jsvu- — this is now looking good with some recent changes we've made. Can someone with better knowledge of what it's meant to look like confirm for me? :heart:

[mathiasbynens]

@kivikakk Looks perfect! I can’t wait to remove the dependency on rawgit.com. Thank you! What changes did you make exactly?

[kivikakk]

@mathiasbynens We permitted the style ~tag~ attribute (and the default "relaxed" set of CSS within) in the sanitizer!

[adius]

@kivikakk Cool! 😁, but Relaxed CSS? Can you point to a documentation what exactly that is?

[kivikakk]

@adius Yes! We use sanitize — here's the relaxed CSS config: https://github.com/rgrove/sanitize/blob/c1b5ff2e50241743bff6129e47a862b7a5e86f8c/lib/sanitize/config/relaxed.rb#L39-L721

[adius]

Thanks!

And why is this necessary for CSS? Are there any security implications I'm not aware of, or is it more to shield you from future changes of the specification which might introduce security related features?

[kivikakk]

More the latter; in general we'll use a whitelist strategy for user-generated content that's being served from our domains. If it turns out it does actually break real content, then we'll have a look at that situation.

[adius]

Yeah, sounds reasonable. Thanks for the insights!

[marionebl]

After changing a number of casts to relative paths I have found no further issues.

Thanks everyone for sorting this out ❤️