switch to alternate markdown library

[katanacrimson]

npm package marked contains unaddressed XSS vulns, looks like. at least three GH issues on their own repo, plus NodeBB/NodeBB#2273 just cropped up.

ref: markedjs/marked#492 markedjs/marked#203 markedjs/marked#229

This issue should be considered critical.

[julianlam]

So this has been reported upstream I see...

[katanacrimson]

Several times, even. Marked doesn't look well maintained, which is a bit surprising but eh.

Might want to look at this, imo: https://github.com/jgm/stmd/tree/master/js

That's commonmark, the attempt to superscede markdown's shitty specification.

[a5mith]

Judging by the amount of issues and outstanding PRs, i can't see this being fixed anytime soon. Maybe time to jump 🚢 and find another markdown alternative.

[julianlam]

I've been looking into markdown-js as a replacement. It's worth noting that Discourse uses markdown-js as well.

A nice-to-have feature would be the ability to break the markdown-to-html process into at least one intermediate step so we can have more fine-grained control over the parsing process (and so plugins can be less regex-heavy)

[katanacrimson]

Ehhh, I'm not fond of trying to hijack the parsing steps for markdown, considering it'll fragment the pseudo-spec even further when it comes to md

[julianlam]

The alternative is leaving the system as-is, and restricting plugin authors into using only regular expressions to parse HTML after it has been converted from Markdown. It's been working well so far, but we're running into issues as evidenced in NodeBB/NodeBB#2263

---

I didn't really explain the problem in that issue, but in a nutshell:

• At least 2 (probably more) plugins were relying on the anchor syntax to be exactly: <a href=, and a recent update to the Markdown plugin caused that syntax to become <a rel="nofollow" href=, breaking all of those plugins
• There's the possibility that this could mean plugins are incompatible with each other, if they both happen to add stuff between a and href
• See 0ca9b78

[akhoury]

+1 more ref: markedjs/marked#497

[julianlam]

Will play around with the markdown-js library later today....

[julianlam]

To be honest the markdown-js library seems a little immature at the moment. There's not much in the way of options, such as automatically turning url into HTML anchors, and parsing a single line break as a new paragraph.

I could be wrong about it, but fixing the bugs in marked might end up being less work than working around markdown-js' rules.

Will continue to investigate.

[akhoury]

this guy seems to have forked marked into this
https://github.com/jonschlinkert/remarked
then created a new repo
https://github.com/jonschlinkert/remarkable/

seems pretty active, like hours ago active.

[akhoury]

test your edge cases here:
http://jonschlinkert.github.io/remarkable/demo/

I tested mine :) from this issue: markedjs/marked#497

seems fast, needs a benchmark

[katanacrimson]

commonmark, i see stuff for plugins, some sane options...looks well thought out. should hit it with a battery of xss tests though, to be sure.

[akhoury]

the 3 issues OP mentioned also pass the XSS test on this page
http://jonschlinkert.github.io/remarkable/demo/

markedjs/marked#492
markedjs/marked#203
markedjs/marked#229

[julianlam]

Good work guys, will look into remarkable.

Should reach out to the author as well, see if we can get some dialogue going.

[julianlam]

@damianb Any further testing you can do against remarkable would be much appreciated, for peace of mind 😄

[akhoury]

moar of this:

According to this article from Github, they forked a C markdown parser called UpSkirt, into a new C Markdown parser called sundown, then created a Ruby wrapper called RedCarpet.

All of the above are not directly usable in Node, however:

• There is NodeJS port of UpSkirt, called robotskirt, might be worth looking at as well, 2 years since last activity, meh
• There is NodeJS port of Github's highly maintained RedCarpet's in NodeJS, called Docter - this one spawns a child process for each conversion .. meh., 1 year since last activity.

Also note that in some places you might see the old redcarpet link
https://github.com/tanoku/redcarpet which 404's now. Use this https://github.com/vmg/redcarpet

Still, so far, remarkable is my favorite.

[akhoury]

there are some nice tests here, all according to spec
https://github.com/jonschlinkert/remarkable/tree/master/test (see fixtures folder)

we should just add one that used Marked and compare.

[katanacrimson]

@julianlam unfortunately, i can't say i will have time anytime soon. hours at work are picking up as holidays approacheth.

it looks kinda like it might be well insulated, but i can't say for certain because there's a lot of code to step through.

@akhoury one thing to note is that those tests don't seem to have any vigorous attempts to push xss through. probably needs an xss cheat sheet shoved in there for kicks and giggles.

[julianlam]

NodeBB's package.json will install 0.7.0 (and up) only for v0.5.2, to be released today.

@a5mith I've reversed the rel="nofollow" bit, as it was too much of a breaking change. Will play around with making it a Remarkable plugin or something... so as far as I know you're the only one who's modified a plugin to account for rel="nofollow" 😄

[a5mith]

a plugin

Four plugins. 😆 On the plus side, I did get my duplicate npmjs accounts merged. So there's that.

[julianlam]

😅 If it helps, your efforts weren't in vain. They needed to be updated anyways, and they'll probably break again after 0.5.2 lands.

[a5mith]

Some are broken already. So I'll look into them, working on my next NodeBB forum at the minute.

[katanacrimson]

good turnaround time - ~26hrs. i do hope you'll be marking 0.5.2 as a security update, though, considering this context.