-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Prevent a ReDoS vulnerability #335
Conversation
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. |
@jonschlinkert is there any updates when we could expect this merged? Also, would it be possible a newly published version after? |
Should probably fix the |
I have a separate PR for that: #333. |
As noted in #331 the same issue impact the comment regex, the same fix would likely work there. |
Once again - prior art: markdown-it/markdown-it@6ab7cc3 Hat tip @DanCech jonschlinkert#335 (comment) for pointing out jonschlinkert#331 (comment), which I missed initially
Once again - prior art: markdown-it/markdown-it@6ab7cc3 Hat tip @DanCech jonschlinkert#335 (comment) for pointing out jonschlinkert#331 (comment), which I missed initially
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Great, thanks! Will be a part of v2 |
Since this is a security issue, I suggest it is released as a patch version, so that NPM packages that use |
I agree. Though it's hard since we already have breaking changes in master branch. |
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! :) |
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? |
@dominykas Have you done it? Please do... so they can do minor release before v2. |
Would just like to hear back from @TrySound (or someone else) that they're interested in that. |
fix: prevent a ReDoS vulnerability Ported from markdown-it/markdown-it@89c8620. fix: prevent ReDoS with comments Once again - prior art: markdown-it/markdown-it@6ab7cc3 Hat tip @DanCech #335 (comment) for pointing out #331 (comment), which I missed initially
I cherry picked your commits here. Is it ok to publish now? |
Is it ok to have a release in a branch? |
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 |
Ok. Started https://github.com/jonschlinkert/remarkable/tree/v1.x branch. Published 1.7.2. Thanks everybody! |
Thank you! |
fix: prevent a ReDoS vulnerability Ported from markdown-it/markdown-it@89c8620. fix: prevent ReDoS with comments Once again - prior art: markdown-it/markdown-it@6ab7cc3 Hat tip @DanCech jonschlinkert/remarkable#335 (comment) for pointing out jonschlinkert/remarkable#331 (comment), which I missed initially
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