Script Injection possible

[mgrubinger]

Hi! Thanks for this nice toast plugin!

Just wanted to let you know that i is possible to inject malicious code due to the usage of innerHTML. It's easy to test on the demo site as well by entering a message like:
They see me rolling <img src=x onerror='alert("hi");' />
or
They see me rolling <img src=x onerror='alert(document.cookie)' />

Please consider escaping message in some way or replace innerHTML with textContent.

Thanks!

[caroso1222]

Hey Martin! Thanks for bringing this up. I was aware of this issue when I built support for custom HTML. I decided to offload the responsibility to sanitize the content to the developer. I don't want Notyf to be too smart, to be honest. I'd rather have it be a dumb component even if it comes at the cost of such liability.

Your point is valid, though. I'll add a warning in the docs to make sure devs don't miss this (hopefully their frameworks will also help in this).

Thanks!

[jagracey]

@caroso1222 I appreciate the work you put into developing and designing Notyf. That said, I hope you can reconsider @mgrubinger's proposal of essentially changing line 174 from message.innerHTML = ... to message.textContent = ....

As an open-source developer it's okay to de-scope features and responsibilities- but in this case it would make sense to apply the straightforward fix. Offloading the responsibility of sanitising to the developer is a poor methodology in general as the modern sanitisation approach is to leverage element.textContent anyway.

[caroso1222]

Hey @jagracey. Thanks for your input! I understand the concern. In my view, there are two things to consider here.

First, textContent is by no means an alternative to innerHTML. It's a very different feature. Changing that line would result in breaking changes as you'd no longer be able to render notifications like 'You scored <b>500 pts </b>!' which was a feature requested by the community (#17, #22). Supporting custom HTML is one of the upsides of Notyf these days.

Second, HTML sanitization is too complex for this plugin to handle. I wouldn't like to have my project running with several libraries, all of them having 200 LOC of HTML sanitation.

Once again, I appreciate the intention. I understand an injection attack is serious and that's why I put the warning in the readme. However, Introducing a breaking change or writing several LOC to sanitize is not in the scope of the project.