-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rendering issues with output #5
Comments
Looking at the svg file I can't see any issues. The file is delivered with a wrong Content-Type header, though:
|
I created a PR on 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. |
The SVG version was created using https://github.com/marionebl/svg-term-cli with the following command: svg-term --cast=rfS1M5ynKm1hGaBqJYJj0mGCi --frame --at=43250 --out=screenshot.svg For now, RawGit.com is used to work around GitHub setting an incorrect `Content-Type` header for SVG files. See marionebl/svg-term-cli#5. Closes #15.
The SVG version was created using https://github.com/marionebl/svg-term-cli with the following command: svg-term --cast=rfS1M5ynKm1hGaBqJYJj0mGCi --frame --at=43250 --out=screenshot.svg For now, RawGit.com is used to work around GitHub setting an incorrect `Content-Type` header for SVG files. See marionebl/svg-term-cli#5. Closes #15.
It’s not just the
It’s a little silly that GitHub forces sanitization in this case, as |
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. |
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. |
/cc @kivikakk for the content-type question. |
Is that Content-Type being served on the |
I thought the unsanitized would be |
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 |
The 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 |
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 |
Boiling this down to the use case of 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? |
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 |
Here is an example where I replaced the CSS with SVG attributes. Seems to work. |
Using rawgit.com only works for public projects, but not for private ones... |
I also tried to automatically replace the CSS with attributes using SVGO, but it only support this for inline styles 😞 |
@adius Using attributes also is not possible for animated renderings. |
@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 😂) |
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? ❤️ |
@kivikakk Looks perfect! I can’t wait to remove the dependency on rawgit.com. Thank you! What changes did you make exactly? |
@mathiasbynens We permitted the |
@kivikakk Cool! 😁, but Relaxed CSS? Can you point to a documentation what exactly that is? |
@adius Yes! We use |
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? |
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. |
Yeah, sounds reasonable. Thanks for the insights! |
* Might not require rawgit as per marionebl/svg-term-cli#5 (comment)
* Might not require rawgit as per marionebl/svg-term-cli#5 (comment)
After changing a number of casts to relative paths I have found no further issues. Thanks everyone for sorting this out ❤️ |
See https://github.com/GoogleChromeLabs/jsvu/blob/be49b12f5393cb2681a4cdac73ad48d9245f3dac/README.md#jsvu- — any idea what’s going wrong here?
The relevant commit is GoogleChromeLabs/jsvu@be49b12 and the command used to create the SVG is:
The text was updated successfully, but these errors were encountered: