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

Fix YouTube embeds #383

Closed
DasSkelett opened this issue Aug 7, 2021 · 14 comments · Fixed by #398
Closed

Fix YouTube embeds #383

DasSkelett opened this issue Aug 7, 2021 · 14 comments · Fixed by #398
Labels
Area: Frontend Related to HTML, JS, CSS, or other browser things Priority: Normal Type: Bug

Comments

@DasSkelett
Copy link
Member

DasSkelett commented Aug 7, 2021

Description (What went wrong?):
In #336 we started using Bleach to fix that huge XSS vulnerability on SpaceDock. Bleach escapes all HTML tags and attributes that it considers unsafe (or better, that another library bleach-allowlist considers unsafe).
This also broke YouTube embeds, which are explicitly advertised on the MarkDown info page.

This mod adds a bunch of visual effects to KSP, most importantly atmospheric scattering effects and improved water shaders to KSP.

<iframe allowfullscreen="" frameborder="0" height="600" src="//www.youtube-nocookie.com/embed/XVvglDyM-Ok?rel=0" width="100%"></iframe> 

Reproduction Steps (What did you do?):
Go to https://spacedock.info/mod/141/scatterer, see the escaped iframe,
rendered from [[https://www.youtube.com/watch?v=XVvglDyM-Ok]]

Expected Behavior (What do you think should have happened instead?):
YouTube embeds should work as advertised.

Environment (OS/Browser/Plugins/etc):
All, Firefox on Linux & Windows here

Extra Information (Screenshots/Error Messages/Javascript Console Output):
Planning to work on this myself, opening the issue so I don't forget about it.

Initial idea: Allow iframes if the src is www.youtube-nocookie.com. Not sure if bleach is that configurable.

@DasSkelett DasSkelett added Type: Bug Priority: Normal Area: Frontend Related to HTML, JS, CSS, or other browser things labels Aug 7, 2021
@HebaruSan
Copy link
Contributor

It would be nice if we could somehow run Bleach only on what the user entered, and not on what embed_youtube returns. Maybe there's a way of rearranging the KerbDown extension to achieve that?

def embed_youtube(link: urllib.parse.ParseResult) -> etree.Element:
q = parse_qs(link.query)
v = q['v'][0]
el = etree.Element('iframe')
el.set('width', '100%')
el.set('height', '600')
el.set('frameborder', '0')
el.set('allowfullscreen', '')
el.set('src', '//www.youtube-nocookie.com/embed/' + v + '?rel=0')
return el

@DasSkelett
Copy link
Member Author

You mean rearranging the bleach and the markdown calls in the templates so that the MarkDown rendering happens after the sanitizing?
I.e. {{ mod.description | bleach | markdown }} instead of {{ mod.description | markdown | bleach }}?

I want to avoid that, as this could allow using MarkDown-specifics that Bleach doesn't detect or bugs in our rendering library in order to bypass sanitization and potentially insert unwanted HTML again.

@HebaruSan
Copy link
Contributor

That's the right general idea, but I was thinking more along the lines of replacing the bleach filter with a Markdown extension which we could then put before or after Kerbdown, or adding it as another step in KerbDown, or somesuch, so the two processes could operate in parallel and not interfere with one another.

@HebaruSan
Copy link
Contributor

I assume imgur embedding is broken as well? It's in the extension code but easy to miss on the markdown page: "and Imgur albums"

@HebaruSan
Copy link
Contributor

HebaruSan commented Aug 7, 2021

We are not the first to encounter this!

Python-Markdown/markdown#225 (comment)

  • If you use a plugin that outputs an special tag with classes or something (for example to embed a YouTube video), you don't want to user to be able to be able to put arbitrary iframes in markdown, and now you have to write an special callable filter for bleach to allow this.

@HebaruSan
Copy link
Contributor

replacing the bleach filter with a Markdown extension which we could then put before or after Kerbdown

This may already exist: https://pypi.org/project/mdx_bleach/

@DasSkelett
Copy link
Member Author

That's the right general idea, but I was thinking more along the lines of replacing the bleach filter with a Markdown extension which we could then put before or after Kerbdown, or adding it as another step in KerbDown, or somesuch, so the two processes could operate in parallel and not interfere with one another.

Ah I see. That doesn't sound bad. It would also fix the potential problem of forgetting to put the | bleach pipe in new templates / new Markdown text locations.

However we'd somehow need to tell the Markdown library to first run all rendering steps except our embed converter, then run Bleach, then run the embed conversion. I'll have to check the docs whether this is possible.

@HebaruSan

This comment has been minimized.

@DasSkelett
Copy link
Member Author

https://spacedock.info/mod/2202/test has a bunch of stuff that should be escaped, https://spacedock.info/mod/141/scatterer has a YouTube embed. I think I also have a mod in my local dev env that tries a bunch of other tags, like images via markdown and raw HTML, I can give you that in a minute.

@DasSkelett
Copy link
Member Author

Okay, pasted into the description of https://alpha.spacedock.info/mod/5/Awesome%20Mod

@HebaruSan
Copy link
Contributor

HebaruSan commented Aug 7, 2021

mdx_bleach throws an AttributeError when it tries to start up, see Wenzil/mdx_bleach#5. Looks non-viable. 😭

Side note, it works by adding to md.postprocessors, so I'm guessing it would run after all HTML generation and therefore still break KerbDown.

@HebaruSan
Copy link
Contributor

Okay, pasted into the description of https://alpha.spacedock.info/mod/5/Awesome%20Mod

Thanks, that will be useful. I added some embeds at the end for testing this issue.

@HebaruSan
Copy link
Contributor

HebaruSan commented Aug 7, 2021

Initial idea: Allow iframes if the src is www.youtube-nocookie.com. Not sure if bleach is that configurable.

It seems that bleach's attribute filter can accept a custom function, but tags can only be a list of strings. So we do not seem to have the hooks we would need to do a simple custom filter.

@DasSkelett
Copy link
Member Author

DasSkelett commented Aug 7, 2021

If we create our completely own filter, which we add additionally before the default filter, we could basically completely define what it does to each HTML "token".
Unfortunately this still suffers from the problem you mentioned in Discord, we can't mark something as safe, so the default filter after ours would still escape the iframe. We'd need to add iframe to the allowed list and find a way to call into Bleach' own sanitizing functions for single tokens if the src is not safe.

Another idea: basically keep the embed tags as is (in the [[ ]] form), and do the replacing in the frontend code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Related to HTML, JS, CSS, or other browser things Priority: Normal Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants