Skip to content
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

Update sanitization filter to allow data: URIs in img tags #227

Closed
wants to merge 2 commits into from

Conversation

polybuildr
Copy link

Allowing data: URIs as values for the src attribute
adds great convenience without introducing any
security issues.

Allowing data: URIs as values for the src attribute
adds great convenience without introducing any
security issues.
@polybuildr
Copy link
Author

As per a discussion at github/markup#270, this should be safe enough.

@kbrock
Copy link
Contributor

kbrock commented Oct 7, 2015

nice 👍

@jch
Copy link
Contributor

jch commented Oct 7, 2015

@mastahyeti I see you were referenced in the other thread. Any thoughts on
the security implications of this change?
On Wed, Oct 7, 2015 at 5:34 AM Keenan Brock notifications@github.com
wrote:

nice [image: 👍]


Reply to this email directly or view it on GitHub
#227 (comment).

@btoews
Copy link

btoews commented Oct 7, 2015

There were a number of concerns brought up in github/markup#270. I think we'd need to suss those out before making this change for GitHub.com. Specifically:

  • We don't want links using the data: scheme and we add links to image tags by default.
  • "What happens if a data: img goes out in an email? Do most clients handle it correctly?"
  • "The only thing about base:64 that makes me worried, is the length of issues and logs, once it might get bigger(specially if it is a screenshot or something like this)."

@mhenry
Copy link

mhenry commented Nov 10, 2015

If supporting data:* is scary, is it possible to only support data:image/png, data:image/gif, and data:image/gif? Also, are there any mitigations for the concerns @mastahyeti copied over from github/markup#270? The spotty email client support for inline data URIs seems like the biggest doozy to me.

@flavianh
Copy link

flavianh commented Dec 5, 2015

Any news on this and how soon it can be integrated? How do you enforce data:image/png instead of data:*? Only one line was modified in the PR, should be a piece of cake... right?

@zbeekman
Copy link

zbeekman commented Feb 2, 2016

does this mean that I can't use base64 encoded svg images in README.md files? (yet)

@mchalapuk
Copy link

I urge you, good and courageous people, to click "Merge" with all haste.

@corneadoug
Copy link

Would love that feature too, so that we don't have to host images

@JamieMcDonnell
Copy link

We are looking for support of this feature too - using the following markdown format is still not displaying the base64 image as it should:
![an example red dot](data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==)

Please get this live ASAP as it will assist development no end to get a visual when a user reports an issue with countless apps that rely on GIT.

Thanks all.

@alex-the-man
Copy link

@jch Could you kindly merge this PR or explain your concern? Thanks :)

@ghost
Copy link

ghost commented Jan 12, 2017

+1

@@ -81,7 +81,7 @@ class SanitizationFilter < Filter
'ins' => {'cite' => ['http', 'https', :relative]},
'q' => {'cite' => ['http', 'https', :relative]},
'img' => {
'src' => ['http', 'https', :relative],
'src' => ['http', 'https', 'data', :relative],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(My data)

@JamieMcDonnell
Copy link

Hi all, any idea of an ETA when this will actually be live? Thanks in advance.

@CareyJWilliams
Copy link

Any further thoughts/discussion here?

@daetal-us
Copy link

👍

1 similar comment
@jeanfw
Copy link

jeanfw commented Feb 4, 2018

👍

@RichardBronosky
Copy link

I think it's time to reopen discussion on why this is not being accepted.

@CH-RhyMoore
Copy link

CH-RhyMoore commented Apr 6, 2018

Me too.

I was playing around with SVGs in Github Flavored Markdown today and ran into this limitation (again; I'd forgotten I'd run into several years ago). I suppose there could be an additional security angle, but...I wish this worked.

Here's my use case. I am working on a design system and there's a component for our set of icons. With something visual, it's often essential to have...a visual. But we're not committing the SVG files of our icons as individual assets. With our setup, this would be both unnecessary and noisy; individual files are created during builds and published, but not tracked as source code in the repo.

Wouldn't it be neat, I thought, if you could just see the icons in the README? No need for me to deploy a site. And if I could inline them, I wouldn't have to add the noise of committing assets just to display them, the noise of the README itself would be made up for because we wouldn't have to worry about files hosted elsewhere going poof & breaking our docs (especially previous versions), etc. This approach also happens to work very nicely with the tools I'm using to build and plans I've got for the future.

So, I tried URL encoding an SVG and using a data URI. The idea is that I could just write the icons directly to my README and, unsurprisingly, it does not work.

upload

![upload](data:image/svg+xml,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20width%3D%2224%22%20height%3D%2224%22%20display%3D%22block%22%20pointer-events%3D%22none%22%20viewBox%3D%220%200%2024%2024%22%3E%3Cpath%20fill%3D%22none%22%20stroke%3D%22currentColor%22%20stroke-linejoin%3D%22round%22%20stroke-width%3D%221.5%22%20d%3D%22M22.25%2016v4.25c0%201.176-.895%202-2%202H3.75c-1.104%200-2-.824-2-2V16M12%202v16m5.75-10.527L12.001%201.75%206.25%207.473%22%2F%3E%3C%2Fsvg%3E)

For a little more info and examples of how this works outside of GitHub, see https://css-tricks.com/probably-dont-base64-svg.

PS: There are all kinds of things that are awful, unreadable messes in the e-mail notifications I get from GitHub, so I really don't think that, at least, should be considered a legitimate blocker.

@CH-RhyMoore
Copy link

CH-RhyMoore commented Apr 6, 2018

Here's a demo with a script inside the SVG. https://codepen.io/morewry/pen/rdQevJ?editors=1001

I checked this in IE 10, IE 11, Yandex 14.12, Opera 52, Opera 51, Safari 8, Safari 9.1, Safari 10.1, Safari 11, Firefox 58, Firefox 59, Chrome 64, Chrome 65, Edge 15, and Edge 16. No unexpected console output appears.

@SephReed
Copy link

SephReed commented Jul 3, 2018

Sure would be nice if it wasn't impossible to show all the icons of my icons repo in the README. What's worse is that if this weren't a paid for private repo, it'd be a lot easier.

@akosthekiss
Copy link

I hope to give this PR a push with a ping. (The branch is now in a conflicting state but it's easy to resolve. Happy to help, if that helps. May it be the only thing that blocks this from merging.)

@polybuildr
Copy link
Author

My impression was that this was blocked because of other reasons - not just because of the stale PR, but I've merged master now and resolved the conflict just in case.

Repository owners, please let me know if you'd like me to squash and rebase the commit - happy to do that too.

@vearutop
Copy link

Let the merge of this PR be a True Christmas Miracle!

@kivikakk
Copy link
Contributor

I'm not sure who has write access and who doesn't (I don't), but as has been discussed in several other places: this gem is no longer the basis of HTML processing for github.com, and so changes made here will not be seen on GitHub itself.

@vearutop
Copy link

@kivikakk then why github/markup#270 is closed as off-topic? What/where should be changed to allow embedding images in markdown media?

@kivikakk
Copy link
Contributor

kivikakk commented Dec 2, 2018

My last comment on the issue is exactly what explains why it's closed as off-topic, and tells you exactly where to reach out to. Thanks.

@miyconst
Copy link

miyconst commented Jul 2, 2019

It's mid of 2019 and the feature is not yet there.. quite disappointed, since embedding tiny Base64 images into markdown documents is a very convenient feature.

@kcak11
Copy link

kcak11 commented Sep 20, 2019

Any updates regarding when this will get merged. Its been a long time and everyone seems to be waiting . . .

@SerialDev
Copy link

I'm not sure who has write access and who doesn't (I don't), but as has been discussed in several other places: this gem is no longer the basis of HTML processing for github.com, and so changes made here will not be seen on GitHub itself.

I do have to say closing it as off topic and suggesting a reach-out to github.com/contact just completely removes any ability for people to track down whether this feature will eventually be implemented, and an issue tracker is also a great way to have some transparency. its been 5 years since this support has been requested, so I don't feel writing to ask what is going on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.