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

Render html in heading #1622

Merged
merged 3 commits into from
Mar 22, 2020
Merged

Render html in heading #1622

merged 3 commits into from
Mar 22, 2020

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Mar 20, 2020

Marked version: 0.8.1

Description

  • Add html to TextRenderer for html in headings.
  • Fix Slugger to remove html tags.

Fixes #1621

Contributor

  • Test(s) exist to ensure functionality and minimize regression

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

@vercel
Copy link

vercel bot commented Mar 20, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/markedjs/markedjs/6rqwcwe36
✅ Preview: https://markedjs-git-fork-uzitech-render-html.markedjs.now.sh

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@UziTech
Copy link
Member Author

UziTech commented Mar 20, 2020

@davisjam could you check that /<!?\/?[\w-]+(?: .*)?\/?>/g isn't vulnerable to redos?

@joshbruce
Copy link
Member

Would like to hold off on my approval until @davisjam (or someone) can review for REDOS.

@Gerrit0
Copy link

Gerrit0 commented Mar 21, 2020

According to http://redos-checker.surge.sh/, it is vulnerable, which makes sense since the lookahead can also match characters in [\w-]+. I suggest /<[!\/\w-].*?>/g, which provides mostly similar results without being vulnerable.

That said, both of these options break rather easily... regex isn't a good html parser. The <em> test added implies that the contents within a tag should be preserved... but what should be done here?

<a href="BREAK>" target="_blank">tag</a>

@UziTech
Copy link
Member Author

UziTech commented Mar 22, 2020

?: isn't a look ahead. It is a non-capturing group. And http://redos-checker.surge.sh says <!?\/?[\w-]+(?: .*\/?>|\/?>) is not vulnerable which is the same as <!?\/?[\w-]+(?: .*)?\/?>.

safe-regex which is used by http://redos-checker.surge.sh just checks the star height of the regex so it has a lot of false positives. It is designed so if it says it is NOT vulnerable than you know it is safe but if it says it is vulnerable it still might not be vulnerable.

as for the <a href="BREAK>" target="_blank">tag</a> example CommonMark/GFM spec says '>' must be percent encoded in valid markdown so it is ok to assume '>' is the end of a tag.

@UziTech
Copy link
Member Author

UziTech commented Mar 22, 2020

That being said your regex might be better since it is simpler. I would like to match GitHub but I don't know what regex GitHub uses for it's heading ids.

@UziTech
Copy link
Member Author

UziTech commented Mar 22, 2020

I simplified the regex to /<[!\/a-z].*?>/ig so it must start with a !, /, or a letter to be considered an html tag.

@joshbruce this regex is definitely not vulnerable.

@joshbruce
Copy link
Member

I’m finding myself being pulled to family and friends at the moment. Can we temporarily move to a single review model - as long as it doesn’t introduce a security vulnerability? Further, if it does, the only required review would be for the security piece?

@joshbruce
Copy link
Member

Not in a position to complete second approves flow. @styfle??

@UziTech
Copy link
Member Author

UziTech commented Mar 22, 2020

I got it.

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

Successfully merging this pull request may close these issues.

TypeError: this.renderer.html is not a function (Error in 0.8.1, OK in 0.8.0)
4 participants