security: use rtrim, not unsafe /X+$/ #1260
Conversation
Problem: replace(/X+$/, '') is vulnerable to REDOS Solution: Replace all instances I could find with a custom rtrim
|
This is an uncontentious excerpt from #1226. It affects "clean-up" regexes, not "parsing" regexes. |
lib/marked.js
Outdated
| } else { | ||
| allButC = true; | ||
| } | ||
| var mustMatchC = !allButC; |
There was a problem hiding this comment.
If someone calls rtrim(cap, '\n', false) allButC will be true? seems like a potential source of many future errors.
I would just replace the mustMatchC's below with !allButC
It should be fine for allButC to be truthy or falsey
There was a problem hiding this comment.
I'll tweak the naming.
|
(I am on vacation until end of May so won't be acting on this until then @joshbruce @styfle @UziTech). |
|
@UziTech How's the new version look? I think it's more readable now, especially the clearer variable names in the final "step left" section. |
lib/marked.js
Outdated
| // /c*$/ is vulnerable to REDOS. | ||
| // invert: Remove suffix of non-c chars instead. Default false. | ||
| function rtrim(str, c, invert) { | ||
| if (typeof invert === 'undefined' || !invert) { |
There was a problem hiding this comment.
Is this if statement necessary?
It seems like this would be the same (falsly by default) without explicitly setting it to false.
There was a problem hiding this comment.
It appears not. Too many years of C have addled my brain.
styfle
changed the title
styfle
changed the title
security: use rtrim, not unsafe /X+$/
Problem:
replace(/X+$/, '')is vulnerable to REDOSSolution:
Replace these instances with a custom rtrim.