I’m not impressed with what I see of the three that there are, in some cases to do with how they came about and in some how they’ve been fixed.
The most recent one, https://security.snyk.io/vuln/SNYK-JS-MERMAID-2328372: I would be very concerned about trusting code that could be in any way adjacent to security written by whoever wrote (and whoever reviewed or committed) this original sanitizeUrl function <https://github.com/mermaid-js/mermaid/commit/066b7a0d0bda274...>. .replace(/javascript:/g, '') is obviously catastrophically wrong, breaking valid (though uncommon) URLs and completely failing to guard against javascript: URLs.
Yes, it’s fixed now, but the existence of the bug in the first place is highly alarming. No protection I can understand—for first-party use where you can trust the inputs it’s reasonable—but bad protection suggests someone tried but didn’t know what they were doing, and didn’t know that if you don’t know what you’re doing in security stuff you need to seek help, because there’s a surprisingly high chance that your bandaid will be worse than doing nothing (either that it actively makes things worse, or that it’s insufficient but gives an impression of safety). It’s dangerous cluelessness.
The middle one, https://security.snyk.io/vuln/SNYK-JS-MERMAID-1314738: the patch provided is utterly misguided and does not fix the alleged security vulnerability in the slightest—it barely even puts a bandaid over it, and it definitely breaks legitimate and reasonable stuff. See <https://github.com/mermaid-js/mermaid/pull/2123/commits/3d22...>: this is trivially insufficient and catastrophically wrong in its approach, so that if anything is actually depending on this code for security, it’s certainly broken. I haven’t immediately got an XSS in https://mermaid.live (something else is evidently providing the actual protection—so I think the advisory was either never valid, or it’s still unfixed), but it ruins reasonable labels like “Contrast with javascript: ahead-of-time compilation makes it faster” or “Do you strip javascript: URLs?” by removing the “javascript:” (eww!), but I can easily sneak a javascript: in there because of the sequential replacement done, with the likes of `java<iframescript:`. This is perhaps even more bogglingly incompetent than the /javascript:/g deletion. (Note that I’m using the word “incompetent” in its strict meaning, in no way as a slur. We all start out incompetent; but we need to develop enough of a feel for basically everything that we can identify situations where we’re not competent.) And even apart from all that, URL schemes are case-insensitive, as seen in data:text/html,<img%20src=x%20onerror=JavaScript:alert(1)>, so /javascript/ without the /i (case-insensitive) flag is insufficient anyway.
I’ve looked at two of them, might as well look at the third, https://security.snyk.io/vuln/SNYK-JS-MERMAID-174698. Oh wow. The first patch <https://github.com/mermaid-js/mermaid/commit/c33533082c598a0...> introduced /javascript:.*/g removal, which is both insufficient and excessive as already mostly discussed, implemented separately for flowchart and gantt (that’s a terrible idea that will consistently lead to divergent changes and missed places; this is library functionality that needs to be maintained in one place). Then the second patch <https://github.com/mermaid-js/mermaid/commit/f11d1a6fa1a5350...> switches to a real sanitiser, but leaves the terrible first approach around in a comment in one instance. And removes a bunch of console.log() calls that should never have been there. And starts escaping = as = for no reason (if you need this, something is badly wrong). And changes some conf to getConfig().flowchart for some reason. All in the one commit, with a very weak commit message that doesn’t address the why at all, and ignores most of the changes. This is not a clean code base or repository.
From what I’ve seen so far, I’m fairly confident that an audit of the code base would reveal multiple fairly severe security vulnerabilities. Also that if I started actually reviewing it I’d be crying out to drastically refactor large parts of it. I’m going to tip-toe away before I start poking this 20,000 line code base (excluding tests).
Someone can report that the second one hasn’t actually been fixed, and that the patch was actively harmful and worse than useless, if they’d like to. I don’t want to engage, lest I get sucked in. :-)
The most recent one, https://security.snyk.io/vuln/SNYK-JS-MERMAID-2328372: I would be very concerned about trusting code that could be in any way adjacent to security written by whoever wrote (and whoever reviewed or committed) this original sanitizeUrl function <https://github.com/mermaid-js/mermaid/commit/066b7a0d0bda274...>. .replace(/javascript:/g, '') is obviously catastrophically wrong, breaking valid (though uncommon) URLs and completely failing to guard against javascript: URLs.
Yes, it’s fixed now, but the existence of the bug in the first place is highly alarming. No protection I can understand—for first-party use where you can trust the inputs it’s reasonable—but bad protection suggests someone tried but didn’t know what they were doing, and didn’t know that if you don’t know what you’re doing in security stuff you need to seek help, because there’s a surprisingly high chance that your bandaid will be worse than doing nothing (either that it actively makes things worse, or that it’s insufficient but gives an impression of safety). It’s dangerous cluelessness.
The middle one, https://security.snyk.io/vuln/SNYK-JS-MERMAID-1314738: the patch provided is utterly misguided and does not fix the alleged security vulnerability in the slightest—it barely even puts a bandaid over it, and it definitely breaks legitimate and reasonable stuff. See <https://github.com/mermaid-js/mermaid/pull/2123/commits/3d22...>: this is trivially insufficient and catastrophically wrong in its approach, so that if anything is actually depending on this code for security, it’s certainly broken. I haven’t immediately got an XSS in https://mermaid.live (something else is evidently providing the actual protection—so I think the advisory was either never valid, or it’s still unfixed), but it ruins reasonable labels like “Contrast with javascript: ahead-of-time compilation makes it faster” or “Do you strip javascript: URLs?” by removing the “javascript:” (eww!), but I can easily sneak a javascript: in there because of the sequential replacement done, with the likes of `java<iframescript:`. This is perhaps even more bogglingly incompetent than the /javascript:/g deletion. (Note that I’m using the word “incompetent” in its strict meaning, in no way as a slur. We all start out incompetent; but we need to develop enough of a feel for basically everything that we can identify situations where we’re not competent.) And even apart from all that, URL schemes are case-insensitive, as seen in data:text/html,<img%20src=x%20onerror=JavaScript:alert(1)>, so /javascript/ without the /i (case-insensitive) flag is insufficient anyway.
I’ve looked at two of them, might as well look at the third, https://security.snyk.io/vuln/SNYK-JS-MERMAID-174698. Oh wow. The first patch <https://github.com/mermaid-js/mermaid/commit/c33533082c598a0...> introduced /javascript:.*/g removal, which is both insufficient and excessive as already mostly discussed, implemented separately for flowchart and gantt (that’s a terrible idea that will consistently lead to divergent changes and missed places; this is library functionality that needs to be maintained in one place). Then the second patch <https://github.com/mermaid-js/mermaid/commit/f11d1a6fa1a5350...> switches to a real sanitiser, but leaves the terrible first approach around in a comment in one instance. And removes a bunch of console.log() calls that should never have been there. And starts escaping = as = for no reason (if you need this, something is badly wrong). And changes some conf to getConfig().flowchart for some reason. All in the one commit, with a very weak commit message that doesn’t address the why at all, and ignores most of the changes. This is not a clean code base or repository.
From what I’ve seen so far, I’m fairly confident that an audit of the code base would reveal multiple fairly severe security vulnerabilities. Also that if I started actually reviewing it I’d be crying out to drastically refactor large parts of it. I’m going to tip-toe away before I start poking this 20,000 line code base (excluding tests).
Someone can report that the second one hasn’t actually been fixed, and that the patch was actively harmful and worse than useless, if they’d like to. I don’t want to engage, lest I get sucked in. :-)