-
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 4 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,72 @@ 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(); | ||
|
||
// destination: DESTINATION from generalLinkRe | ||
// returns [destination, title]: no angle-brackets on destination, no quotes on title | ||
function splitIntoDestinationAndTitle (destination) { | ||
function unwrapAngleBrackets (str) { | ||
if (str.match(/^<.*>$/)) { | ||
str = str.slice(1, -1); | ||
} | ||
return str; | ||
} | ||
|
||
// Valid DESTINATIONs, in decreasing specificity. | ||
var destinationAndTitleRe = /^([^'"(]*[^\s])\s+(['"(].*['")])/; | ||
var destinationRe = /^(<?[\s\S]*>?)/; | ||
var parsingRegexes = [destinationAndTitleRe, destinationRe]; | ||
|
||
var match = false; | ||
var dest = undefined; | ||
var title = undefined; | ||
for (var i = 0; i < parsingRegexes.length; i++) { | ||
match = parsingRegexes[i].exec(destination); | ||
if (match) { | ||
dest = match[1]; | ||
title = match[2]; | ||
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. You don't need to assign dest and title here. Simply use |
||
break; | ||
} | ||
} | ||
|
||
if (match) { | ||
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:
|
||
// title is optional. | ||
if (typeof title === 'undefined') { | ||
title = ''; | ||
} | ||
|
||
// Format dest. | ||
dest = dest.trim(); | ||
dest = unwrapAngleBrackets(dest); | ||
|
||
return [dest, title]; | ||
} | ||
return null; | ||
} | ||
|
||
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]; | ||
|
||
// Does 'destination' match spec? | ||
var destinationAndTitle = splitIntoDestinationAndTitle(destination); | ||
if (destinationAndTitle) { | ||
return [fullMatch[0], text, destinationAndTitle[0], destinationAndTitle[1]]; | ||
} | ||
} | ||
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.