Prevent a ReDoS vulnerability

[dominykas]

Closes #331.

Ported from markdown-it/markdown-it@89c8620. The author of the modified file and that commit is the same, so I assume they knew what they were doing.

I will remove the merge of #333 once the build is green. Not sure what happened with older nodes, but I won't bother about it right now. Greenish build: https://travis-ci.org/jonschlinkert/remarkable/builds/532349463

[jonschlinkert]

Thank you! I’ll merge ASAP, but I’m traveling until Saturday so it might be a couple of days. Sorry for the late reply.

[fernandopasik]

@jonschlinkert is there any updates when we could expect this merged?

Also, would it be possible a newly published version after?

[vladikoff]

Should probably fix the eslint error as part of this PR?

[dominykas]

Should probably fix the eslint error as part of this PR?

I have a separate PR for that: #333.

[DanCech]

As noted in #331 the same issue impact the comment regex, the same fix would likely work there.

[Vadorequest]

What's the status on this? I'm not familiar with Remarkable but it's my most important security red flag as GitHub shows it as a critical security issue.

[coveralls]

Coverage remained the same at 98.537% when pulling 7361070 on dominykas:fix-redos into 4ad13a0 on jonschlinkert:master.

[coveralls]

Coverage remained the same at 98.537% when pulling 0c9d32b on dominykas:fix-redos into 4ad13a0 on jonschlinkert:master.

[DanCech]

LGTM

[TrySound]

Great, thanks! Will be a part of v2

[Vadorequest]

Since this is a security issue, I suggest it is released as a patch version, so that NPM packages that use ^/~ will use automatically the patched version.

[TrySound]

I agree. Though it's hard since we already have breaking changes in master branch.

[Vadorequest]

That's not a real issue, it's often the case in software. The recommended way to handle that is often to cherry-pick the changes to apply the security fix in older version.

I believe the code that was changed in this version is quite old, but applying this PR (cherry-pick) to a branch created from an old tag or commit shouldn't conflict.

I'm not sure if you see what I mean by that, but basically going back to the state the git tree was (tag/commit), then cherry picking this PR commit and releasing a new patch version on NPM should do the trick! :)

[dominykas]

I'm happy to cherrypick all changes into the last known version and open a PR - just need someone to hit merge and publish afterwards?

[feruzm]

@dominykas Have you done it? Please do... so they can do minor release before v2.

[dominykas]

Would just like to hear back from @TrySound (or someone else) that they're interested in that.

[TrySound]

I cherry picked your commits here. Is it ok to publish now?
https://github.com/jonschlinkert/remarkable/commits/1.7.2

[TrySound]

Is it ok to have a release in a branch?

[dominykas]

Seems OK, and yes - releasing from a branch is perfectly fine from npm perspective (fwiw, you don't even have to have a git repo to release...), although not sure what the release process is for the repo - it also seems the tag names for releases here don't have the v prefix, so the branch name may collide with the tag name. I usually have something like a 1.x branch for backported releases.

[TrySound]

Ok. Started https://github.com/jonschlinkert/remarkable/tree/v1.x branch. Published 1.7.2. Thanks everybody!

[dominykas]

Thank you!