Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content Security Policy (CSP) compliance #1926

Closed
september28 opened this issue Mar 23, 2020 · 8 comments · Fixed by #1930 or #2116
Closed

Content Security Policy (CSP) compliance #1926

september28 opened this issue Mar 23, 2020 · 8 comments · Fixed by #1930 or #2116
Labels
released severity4: security No one wants a CVE on SweetAlert2

Comments

@september28
Copy link

september28 commented Mar 23, 2020

New feature motivation

Using SweetAlert2 for web extensions (built using Webpack, but relevant whatever the build tool) currently causes the web-ext linter (Mozilla Firefox) to highlight a number of (21 from sweetalert2.all[.min].js to be precise) "UNSAFE_VAR_ASSIGNMENT" warnings (using default web extension csp which I understand to be script-src 'self'; object-src 'self'). This is due mostly to the use of element.innerHTML = blah within SweetAlert's codebase.

I understand why this is, and this is probably one of the main advantages to SweetAlert over plain js message boxes (ability to make nice-looking html versions), however it would be useful to have a solution that avoids the unsafe assignment warnings in web-ext's linter.

One thing to note is that the linter does not make distinction between code that is actually used, and code that is there but will not ever be "used" (for instance fallback "if" statements for last resort assignment). Therefore the solution must ideally avoid element.innerHTML = altogether.

There may be an alternative, listed below. However I would be interested for anyone's thoughts on this.

New feature description

Remove all direct assignment via element.innerHTML which is considered unsafe by the linter

New feature implementation

Idea 1
There is some guidance here from Mozilla about HTML sanitization: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Safely_inserting_external_content_into_a_page#HTML_sanitization
Although I haven't tested whether the linter is intelligent enough to know that the content has been sanitized before assignment to innerHTML.

Idea 2
Otherwise I came across the following solution, which is a little more onerous unfortunately, but removes the need for innerHTML assignment altogether by using the DOMParser library...
https://devtidbits.com/2017/12/06/quick-fix-the-unsafe_var_assignment-warning-in-javascript/

Grateful for any comments about this...

@september28
Copy link
Author

Related to "Idea 1" in my original post... I have tried the DOMPurify library, however it doesn't seem to satisfy the linter. By the looks of it, the linter is not inteliigent enough to "understand" how innerHTML is being used.

As an example, DOMPurify actually causes many warnings in the linter because of the code that it uses to sanitise the input which uses innerHTML, outerHTML, insertAdjacentHTML.

@limonte
Copy link
Member

limonte commented Mar 26, 2020

The report is great, thank you @september28

Unfortunately, I don't think we can do anything here. Web-ext linter warnings is definitely not a reason for us to complicate the codebase and increase the bundle-size.

I believe there should be a way to suppress these warnings in the web-ext linter.

@limonte limonte closed this as completed Mar 26, 2020
@september28
Copy link
Author

Thanks @limonte I appreciate you don't want to overcomplicate the code base. For the rest of my js I actually wrote a quite lightweight solution (domWrap) using DOMParser which I believe is pretty neat (if I do say so myself). Included below if of interest:

function domWrap(str, tag="span") {
    if(str && typeof str === "string") {
        if(str[0] !== "<") {
            str = `<${tag}>${str}</${tag}>`;
        }
        str = `<span id="dw">${str}</span>`;
        let parser = new DOMParser();
        let dom = parser.parseFromString(str, "text/html");
        let ret = dom.getElementById("dw");
        return ret.firstChild;
    }
    return str;
}

The function returns a DOM element, so this can be used via appendChild, rather than innerHTML.

@limonte
Copy link
Member

limonte commented Mar 27, 2020

Hm, I might change my mind here. Thanks again @september28 for your input!

@limonte limonte reopened this Mar 27, 2020
@limonte limonte added the severity4: security No one wants a CVE on SweetAlert2 label Mar 27, 2020
@limonte
Copy link
Member

limonte commented Mar 28, 2020

🎉 This issue has been resolved in version 9.10.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@limonte
Copy link
Member

limonte commented Mar 28, 2020

element.innerHTML = is not used anymore 🎉

https://github.com/sweetalert2/sweetalert2/search?q=innerHTML&unscoped_q=innerHTML

@zenflow
Copy link
Member

zenflow commented Nov 26, 2020

@limonte This issue has popped up again with PR #2092 which used a element.innerHTML =

// Loader
loader.innerHTML = params.loaderHtml
dom.applyCustomClass(loader, params, 'loader')

@limonte
Copy link
Member

limonte commented Nov 26, 2020

🎉 This issue has been resolved in version 10.10.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released severity4: security No one wants a CVE on SweetAlert2
Projects
None yet
3 participants