Closed chris-tuncap closed 2 years ago
Here is my answer to the PR, I'm going to close this issue for that reason:
1) We already use toastr in a non vulnerable way ( https://github.com/voidlabs/mosaico/commit/36f8ad8ed5b97434a353cfd9f6204b5ec07a933f ), and, we didn't pass user-submitted content to the notifications, so I guess the XSS was not applicable even before this patch, unless you load master templates or language packs from untrusted sources (but in that case I'd be warried XSS issues already exists in main mosaico) 2) You can replace toastr by using a viewModel plugin, with no changes at all to the mosaico codebase or any other existing extension that uses the viewModel.notifier error|success|info methods.
Just create a new viewModel plugin ( https://github.com/voidlabs/mosaico/wiki/Mosaico-Plugins#viewmodel-plugins ) And replace viewModel.notifier object with your own calling sweetalert.
e.g:
viewModel.notifier = {
'error': function(msg) { sweetAlert.fire({ icon: 'error', title: msg }); },
'success': function(msg) { sweetAlert.fire({ icon: 'success', title: msg }); },
'info': function(msg) { sweetAlert.fire({ icon: 'info', title: msg }); },
};
PS: I'll think better about this but I think toastr is a better UX solution for mosaico (e.g: I gave a very fast read to sweetalert2 docs and I guess one of the missing options from sweetalert is being able to stack multiple notifications.)
Toastr is not actively being maintained and Snyk has reported a vulnerability that has no fix: Issues with no direct upgrade or patch:
✗ Cross-site Scripting (XSS) [High Severity][https://snyk.io/vuln/SNYK-JS-TOASTR-2396430] in toastr@2.1.4 introduced by toastr@2.1.4 No upgrade or patch available
PR: https://github.com/voidlabs/mosaico/pull/658