Package causes vulnerability

[lukasbicus]

Dependabot reported one vulnerability:

All versions of the package react-draft-wysiwyg are vulnerable to Cross-site Scripting (XSS) via the Embedded button which will then result in saving the payload in the <iframe> tag.

See a Snyk detail: https://security.snyk.io/package/npm/react-draft-wysiwyg

[mfernandes-alcumus]

Same here mend.io has highlighted it as a medium vulnerability. My organization is having some concerns about this lib and thinking of moving away from it. Can anybody get this sorted sooner as I really enjoy this library

[SpeedyCraftah]

Thought I'd share some insight here because I also use this package. I've had a look at the actual vulnerability, including the POC, and the vulnerability message makes the issue sound a lot worse than it actually is; the issue is actually if the user copy and pastes a link with a vulnerable payload into the embed link field (see below): There is no possibility of remote code execution from an arbitrary linked website, a user must be effectively tricked to paste a crafted link into the embed link field, with the attack vector mainly becoming a social engineering one which I argue is no more different than telling someone to ctrl+shift+I and paste in some dodgy code, and at that not a simple one since most users can recognise that javascript: around the beginning of a URL is a bit unusual, but nevertheless still a vulnerability. (in case you're curious, the actual embedded website is behind an iframe so there is little to no risk there).

Quite disappointing the advisory doesn't mention any mitigation when there are tons, bit of a lazy advisory that provides little to no context and mitigations.

I've come up with 2 ways you can mitigate this completely until we wait for a patch on the NPM package (library looks to be dead so a fork is probably needed with a fix):

Just remove the button

The library is quite flexible so you can remove the embed link button (it's a bit of an old web feature anyways, you don't need it for 99% of use cases) by passing in a custom react component with an empty render function:

<Editor toolbar={{ embedded: { component: class extends React.Component { render() {} } } }} />

If you really want to keep it

The library also lets you intercept the URL before it gets processed, so you can do the validation/sanitizing that the library fails to do yourself:

<Editor toolbar={{ embedded: { embedCallback: { try { return ["http:", "https:"].includes(new URL(url).protocol) ? url : alert("Unsupported URL detected!") } catch(_) { alert("Invalid URL detected!") } } } }} />

This will make sure that the URL passed is both valid and only contains supported protocols (http and https in this case).

Also

Funnily enough, in the future this vulnerability will probably be inadvertently patched in newer react versions:

Warning: A future version of React will block javascript: URLs as a security precaution. Use event handlers instead if you can. If you need to generate unsafe HTML try using dangerouslySetInnerHTML instead. React was passed "javascript:alert('this works xss')".

Never quite seen a vulnerability patch itself, but here we are.

[lukasbicus]

Thanks for the explanation and additional info, @SpeedyCraftah. Of course, we would like to have it fixed at least due to the broken windows effect (https://en.wikipedia.org/wiki/Broken_windows_theory).

[SpeedyCraftah]

Thanks for the explanation and additional info, @SpeedyCraftah. Of course, we would like to have it fixed at least due to the broken windows effect (https://en.wikipedia.org/wiki/Broken_windows_theory).

Yeah I do agree with you here, it's also nice to clear that vulnerability warning from npm.. tell you what I'll submit a PR with a fix in a few days or so or sooner and see if it gets merged, if not I'll start a fork and publish to npm which addresses security issues with some minor maintenance updates (maybe even merge some open PRs here too), if the library suddenly revives itself I'll deprecate mine. I do use it for a small project of mine so I'd be happy to keep it going.

[mfernandes-alcumus]

Thank you @SpeedyCraftah I will try that work around but if we have fork that'll be so good

[SpeedyCraftah]

Although I am not too happy with keeping it alive long-term, a lot of these rich text editors for react (including this one) rely on a lot of very specific dependencies which are no longer maintained, I reckon it won't take long for a major vulnerability to prop up in one of the dependencies that react-draft-wysiwyg uses, I am already having a big headache trying to port my project over to newer react and remix.js purely because the APIs the dependencies use are outdated.

Maybe what this package needs in the long term is a complete overhaul, get rid of the complex overengineered dependencies like draft-js and try to keep it as slim as possible, this should make it easier to maintain in the future, make it a lot simpler and make it last longer without any major issues, but that definitely will need a fork with a big breaking change (although I think the port over shouldn't be too difficult and it will probably just keep working without much attention as long as react stays around).

[mfernandes-alcumus]

Thank you @SpeedyCraftah

[SpeedyCraftah]

Fixed fork is almost ready! Going to give it a test, then publish it to NPM 🙂. I've had a look at current pull requests and all of them are going unanswered, so I think it will be quicker if I just fork it.

[SpeedyCraftah]

Fork is now published to npm! https://www.npmjs.com/package/react-draft-wysiwyg-next You can install it using npm i react-draft-wysiwyg-next (you may have to add --legacy-peer-deps on newer React versions, I will also work on fixing this so you don't have to). This fork addresses the aforementioned XSS vulnerability by providing a patch.

I will also work on merging some of the PRs in this repository over the next coming weeks, but if you have a PR which you want merged feel free to submit it in the new repo and I will get it merged.

[PavelRazumov]

@SpeedyCraftah Hi, I checked the updated version of the library (https://www.npmjs.com/package/react-draft-wysiwyg-next), unfortunately when importing I get an error, the project is written in TS and there are not enough @types for import. Can you tell me the solution?

[SpeedyCraftah]

@SpeedyCraftah Hi, I checked the updated version of the library (https://www.npmjs.com/package/react-draft-wysiwyg-next), unfortunately when importing I get an error, the project is written in TS and there are not enough @types for import. Can you tell me the solution?

Ah I see the problem, thanks for letting me know. It's because the original library had types provided by DefinitelyTyped, which is external hence it wouldn't transfer over to the fork. I will integrate the DefinitelyTyped typings directly in the project.

[SpeedyCraftah]

@PavelRazumov Got some time to add the types in 🙂 I've published it to NPM, please test to see if it's working fine now. These are the types from the DefinitelyTyped repository, it looks quite rushed so will probably improve it in the future, but they do have the fundamentals for decent typings so no issue at the moment.

[PavelRazumov]

@SpeedyCraftah Hi, i checked version 1.17.2 but have error

[SpeedyCraftah]

@SpeedyCraftah Hi, i checked version 1.17.2 but have error

I'm not able to reproduce your error, how did you try to install the package? It may just be a stale package lock, running npm uninstall react-draft-wysiwyg-next --legacy-peer-deps then npm install react-draft-wysiwyg-next --legacy-peer-deps should fix it, alternatively delete your node_modules and reinstall.

[PavelRazumov]

@SpeedyCraftah Yes, you are right, version is correct, package install success thank you !

[plamber]

@SpeedyCraftah: thank you. Appreciate your effort