-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
security: replace vulnerable regex with parser #1223
Changes from 3 commits
4717be9
1ad9ca0
fbf93a8
ba2fc13
47f4388
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -554,9 +554,51 @@ inline.normal = merge({}, inline); | |
inline.pedantic = merge({}, inline.normal, { | ||
strong: /^__(?=\S)([\s\S]*?\S)__(?!_)|^\*\*(?=\S)([\s\S]*?\S)\*\*(?!\*)/, | ||
em: /^_(?=\S)([\s\S]*?\S)_(?!_)|^\*(?=\S)([\s\S]*?\S)\*(?!\*)/, | ||
link: edit(/^!?\[(label)\]\(\s*<?([\s\S]*?)>?(?:\s+(['"][\s\S]*?['"]))?\s*\)/) | ||
.replace('label', inline._label) | ||
.getRegex(), | ||
/* Original link re: /^!?\[(label)\]\(\s*<?([\s\S]*?)>?(?:\s+(['"][\s\S]*?['"]))?\s*\)/ | ||
* This captures the spec reasonably well but is vulnerable to REDOS. | ||
* Instead we use a custom parser that follows the RegExp.exec semantics. */ | ||
link: { | ||
exec: function (s) { | ||
// [TEXT](DESTINATION) | ||
var generalLinkRe = edit(/^!?\[(label)\]\((.*?)\)/) | ||
.replace('label', inline._label) | ||
.getRegex(); | ||
|
||
function unwrapAngleBrackets (str) { | ||
if (str.match(/^<.*>$/)) { | ||
str = str.slice(1, -1); | ||
} | ||
return str; | ||
} | ||
|
||
var fullMatch = generalLinkRe.exec(s); | ||
if (fullMatch) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you flip this so that the if statement is smaller:
|
||
var text = fullMatch[1]; | ||
var destination = fullMatch[2]; | ||
|
||
var m; | ||
|
||
var destinationAndTitleRe = /^([^'"(]*[^\s])\s+(['"(].*['")])/; | ||
if (m = destinationAndTitleRe.exec(destination)) { | ||
// <destination> -> destination | ||
var dest1 = m[1].trim(); | ||
dest1 = unwrapAngleBrackets(dest1); | ||
var title1 = m[2]; | ||
return [fullMatch[0], text, dest1, title1]; | ||
} | ||
|
||
var destinationRe = /^(<?[\s\S]*>?)/; | ||
if (m = destinationRe.exec(destination)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The two function getMatch(r, fullMatch) {
var m = r.exec(fullMatch[2]);
if (m) {
var dest = unwrapAngleBrackets(m[1].trim());
var title = m[2];
return [fullMatch[0], fullMatch[1], dest, title];
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, but not a deal breaker for my review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// <destination> -> destination | ||
var dest2 = m[1].trim(); | ||
dest2 = unwrapAngleBrackets(dest2); | ||
var title2 = ''; | ||
return [fullMatch[0], text, dest2, title2]; | ||
} | ||
} | ||
return null; | ||
} | ||
}, | ||
reflink: edit(/^!?\[(label)\]\s*\[([^\]]*)\]/) | ||
.replace('label', inline._label) | ||
.getRegex() | ||
|
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.
Might be worth adding some doc blocks to introduce the why behind some of this...nothing too major, just to help those new to the code.