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

Sanitize HTML content #205

Closed
tnhu opened this issue Oct 12, 2022 · 8 comments
Closed

Sanitize HTML content #205

tnhu opened this issue Oct 12, 2022 · 8 comments

Comments

@tnhu
Copy link
Contributor

tnhu commented Oct 12, 2022

react-markdown-preview does not sanitize HTML content before rendering. Paste below code into https://uiwjs.github.io/react-markdown-preview and you'll see an alert showing up.

<div onmouseover="alert('alpha')">
  <a href="jAva script:alert('bravo')">delta</a>
  <img src="x" onerror="alert('charlie')">
  <iframe src="javascript:alert('delta')"></iframe>
  <math>
    <mi xlink:href="data:x,<script>alert('echo')</script>"></mi>
  </math>
</div>
<script>
require('child_process').spawn('echo', ['hack!']);
</script>

Maybe https://github.com/rehypejs/rehype-sanitize should be included?

@jaywcjlove
Copy link
Member

@tnhu

  • skipHtml (boolean, default: false) Ignore HTML in Markdown completely

@jaywcjlove
Copy link
Member

@tnhu Using rehype-sanitize is also ignorable.

@tnhu
Copy link
Contributor Author

tnhu commented Oct 17, 2022

@jaywcjlove skipHtml does not work. The html content is still injected.

import ReactMarkdownPreview from '@uiw/react-markdown-preview'

const source = `<div onmouseover="alert('alpha')">
  <a href="jAva script:alert('bravo')">delta</a>
  <img src="x" onerror="alert('charlie')">
  <iframe src="javascript:alert('delta')"></iframe>
  <math>
    <mi xlink:href="data:x,<script>alert('echo')</script>"></mi>
  </math>
</div>
<script>
require('child_process').spawn('echo', ['hack!']);
</script>`

...

<ReactMarkdownPreview source={source} skipHtml />  

--> delta is shown in an alert

@tnhu
Copy link
Contributor Author

tnhu commented Oct 17, 2022

jaywcjlove added a commit that referenced this issue Oct 18, 2022
jaywcjlove added a commit that referenced this issue Oct 18, 2022
github-actions bot added a commit that referenced this issue Oct 18, 2022
jaywcjlove added a commit that referenced this issue Oct 18, 2022
jaywcjlove added a commit that referenced this issue Oct 18, 2022
github-actions bot added a commit that referenced this issue Oct 18, 2022
@jaywcjlove
Copy link
Member

@tnhu https://codesandbox.io/embed/uiwjs-react-markdown-preview-issues-205-kl1xdq?fontsize=14&hidenavigation=1&theme=dark

import MarkdownPreview from "@uiw/react-markdown-preview";

const source = `<div onmouseover="alert('alpha')">
<a href="jAva script:alert('bravo')">delta</a>
<img src="x" onerror1="alert('charlie')">
<iframe src="javascript:alert('delta')"></iframe>
<math>
  <mi xlink:href="data:x,<script>alert('echo')</script>"></mi>
</math>
</div>
<script>
require('child_process').spawn('echo', ['hack!']);
</script>`;

export default function App() {
  return (
    <div className="App">
      <MarkdownPreview
        source={source}
        skipHtml={false}
      />
    </div>
  );
}

@jaywcjlove
Copy link
Member

@tnhu Upgrade v4.1.2

@tnhu
Copy link
Contributor Author

tnhu commented Oct 20, 2022

@jaywcjlove it works. Thank you very much! skipHtml={false} removed support for raw HTML, as a result, it prevents security issue like above.

Just to add a little more:

  1. Should it be skipHtml={true} to remove raw HTML instead of skipHtml={false}? skip something means ignore it. In this context, I think skip equals to false but actually removing support for it is confusing.
  2. Raw HTML should be ignored by default. Don't you think?
  3. When HTML is ignored (currently skipHtml={false}), all HTML tags in the markdown source are ignored. It's good for security purposes but it's harsh as we can't have some small nice things like <quote>, etc... It'd be best if somehow we can filter out the malicious ones, but still keep simple tags that we support.

@jaywcjlove
Copy link
Member

const customProps: MarkdownPreviewProps = {
allowElement: (element, index, parent) => {
if (other.allowElement) {
return other.allowElement(element, index, parent);
}
return /^[A-Za-z0-9]+$/.test(element.tagName);
},
};

@tnhu Parse html by default This is because I have many projects using this package, Keep the original features.

skipHtml={false} does not parse HTML by default, which is a feature of the dependent react-markdown package.

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

No branches or pull requests

2 participants