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

Migrate to pagedown for markdown parsing #514

Merged
merged 2 commits into from
Oct 27, 2017
Merged

Migrate to pagedown for markdown parsing #514

merged 2 commits into from
Oct 27, 2017

Conversation

balloob
Copy link
Member

@balloob balloob commented Oct 27, 2017

We have recently been notified by Marcin Teodorczyk from https://www.intive.com about a vulnerability in Home Assistant: we did not sanitize the Markdown output. This means that our users have been vulnerable to script injection attacks. The severity is low, as to be able to create a persistent notification an attacker would need access to the instance.

Then, when migrating to the new package format, I accidentally removed the markdown file so the vulnerability no longer existed, but neither did the persistent notifications work.

While adding back a sanitized version of the markdown parser, I realized that when the component was migrated to ES6 in #465 we forgot to call super.ready(), so the Polymer template never initialized. That too actually resolved the vulnerability 😉

This PR migrates to use Pagedown, the Markdown renderer used by StackExchange: https://github.com/StackExchange/pagedown . This renderer includes a sanitizer after which I was no longer able to reproduce the script injection attack supplied by Marcin.

As a bonus, when the script fails to load, we will render the message as a text content.

I think that we should take a closer look what we want to support in the persistent notification markdown. We might just want to limit it to links and maybe paragraphs? (That's for a future PR)

Copy link
Contributor

@andrey-git andrey-git left a comment

Choose a reason for hiding this comment

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

Are you deleting the previous md parser?

@balloob
Copy link
Member Author

balloob commented Oct 27, 2017

I accidentally already did. It was a file in www_static but I forgot to copy it over to this repo when consolidating

@balloob balloob merged commit ce2b4b6 into master Oct 27, 2017
@balloob balloob deleted the markdown-upgrade branch November 11, 2017 04:53
@robbiet480
Copy link
Member

CVE-2017-16782

@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants