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

use iframe to sandbox generated html #1295

Merged
merged 4 commits into from
Jul 9, 2018
Merged

use iframe to sandbox generated html #1295

merged 4 commits into from
Jul 9, 2018

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Jun 24, 2018

Description

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@UziTech UziTech requested review from styfle and joshbruce June 24, 2018 06:57
@styfle
Copy link
Member

styfle commented Jun 24, 2018

Isn’t there a sanitize option in marked to prevent XSS? Or did we already remove that?

@UziTech
Copy link
Member Author

UziTech commented Jun 24, 2018

Yes we do have a sanitize option. But I don't want the demo to show sanitize output if the user isn't looking for that.

Plus I think we should deprecate that option.

Descussion #1232

@styfle
Copy link
Member

styfle commented Jun 24, 2018

Ok if that's the case, should we add this iframe solution to the USING_ADVANCED.md docs to make it the preferred method of sanitizing rather than the sanitize option?

@UziTech
Copy link
Member Author

UziTech commented Jun 24, 2018

The iframe solution wouldn't work well in most situations.

I think we should recommend something like DOMPurify if they are displaying user generated content.

DOMPurify.sanitize(marked(...));

@styfle
Copy link
Member

styfle commented Jun 24, 2018

The iframe solution wouldn't work well in most situations.

Why use it here? Maybe the demo should use DOMPurify?

@UziTech
Copy link
Member Author

UziTech commented Jun 24, 2018

Because it's a demo. I don't want the user to see sanitized output if they don't specify the sanitize option

@styfle styfle requested a review from davisjam June 25, 2018 19:18
@styfle
Copy link
Member

styfle commented Jun 25, 2018

@UziTech That makes sense.


@joshbruce or @davisjam can you take a look at this?

@joshbruce
Copy link
Member

Hey! Sorry been kinda quiet - lots going on with the day job. Would like @davisjam to take a look just in there's any security weirdness though I don't think there should be (brain a little fried).

@lirantal
Copy link

Thanks for the security disclaimer notice, appreciate the awareness you set for users 👍

README.md Outdated
## Usage
## Usage

### Warning: 🚨 Marked does not sanitize the output HTML by default 🚨
Copy link
Contributor

Choose a reason for hiding this comment

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

Could link to the docs on sanitize here.

@davisjam
Copy link
Contributor

davisjam commented Jun 27, 2018

I don't see the sanitize option documented clearly anywhere. The USING_ADVANCED.md docs say:

|sanitize    |`boolean` |`false`  |???      |If true, sanitize the HTML passed into `markdownString` with the `sanitizer` function.|
|sanitizer   |`function`|`null`   |???      |A function to sanitize the HTML passed into `markdownString`.|

This sounds a bit circular to me.

Thoughts on:

  • Enhancing the description there (and explicitly mentioning the XSS possibility)?
  • Directing readers to the advanced docs from the regular docs for security considerations?

@UziTech
Copy link
Member Author

UziTech commented Jun 27, 2018

I still say we deprecate the sanitize and sanitizer options and recommend

DOMPurify.sanitize(marked(...));

like in #1232 (comment)

marked should be all about converting markdown to HTML and let other packages handle the sanitizing and displaying the HTML.

@styfle
Copy link
Member

styfle commented Jul 9, 2018

I'm going to merge this because it addresses sanitization of marked.js.org and we shouldn't leave this XSS vulnerability lingering any longer.

If anyone wants to update the docs to make this sanitation process clearer, feel free to submit a new PR.

Thanks!

This was referenced Apr 7, 2020
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
use iframe to sandbox generated html
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.

5 participants