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

Preserve CSP nonce on scripts with src attribute in DOM manipulation. #4323

Closed
buddh4 opened this Issue Mar 16, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@buddh4
Copy link
Contributor

commented Mar 16, 2019

When appending html content containing script tags with src attribute, I get a CSP script-src violation error. Tested with current master branch.

Since domManip uses jQuery_evalUrl for script tags with src attribute it violates CSP's with a nonce script rule. I'am aware of #3969 (comment) but I can't use crossDomain since I require sync script loading. The following fix seems only to work for scripts without src attribute https://github.com/jquery/jquery/pull/4269/files.

The problematic line:

jQuery._evalUrl( node.src );

Hacky fix:

manipulation/_evalUrl.js:

Adding a node parameter to _evalUrl which will be passed to jQuery.globalEval.

jQuery._evalUrl = function( url, node ) {
    return jQuery.ajax( {
        url: url,

        // Make this explicit, since user can override this through ajaxSetup (#11264)
        type: "GET",
        dataType: "script",
        cache: true,
        async: false,
        global: false,

        // Only evaluate the response if it is successful (gh-4126)
        // dataFilter is not invoked for failure responses, so using it instead
        // of the default converter is kludgy but it works.
        converters: {
            "text script": function() {}
        },
        dataFilter: function( response ) {
            jQuery.globalEval( response, node );
        }
    } );
};

/manipulation.js:159

In my tests I noticed that the nonce attribute is not accessible anymore after the script nodes were added by appendChild() (if CSP is active). A dirty workaround was to backup the nonce attributes before inserting the dom like this:

for ( ; i < l; i++ ) {
    node = fragment;

    if ( i !== iNoClone ) {
        node = jQuery.clone( node, true, true );

        // Keep references to cloned scripts for later restoration
        if ( hasScripts ) {

            // Support: Android <=4.0 only, PhantomJS 1 only
            // push.apply(_, arraylike) throws on ancient WebKit
            jQuery.merge( scripts, getAll( node, "script" ) );
        }
    }

    /**
     * Backup nonce attributes, since they won't be accessible after dom insertion
     */
    if ( hasScripts ) {
        scripts.forEach( function( scr ) {
            scr.nonceValue = scr.getAttribute( "nonce" );
        } );
    }


    callback.call( collection[ i ], node, i );
}

if ( hasScripts ) {
    doc = scripts[ scripts.length - 1 ].ownerDocument;

    // Reenable scripts
    jQuery.map( scripts, restoreScript );

    // Evaluate executable scripts on first document insertion
    for ( i = 0; i < hasScripts; i++ ) {
        node = scripts[ i ];
        if ( rscriptType.test( node.type || "" ) &&
            !dataPriv.access( node, "globalEval" ) &&
            jQuery.contains( doc, node ) ) {

            if ( node.src && ( node.type || "" ).toLowerCase()  !== "module" ) {

                // Optional AJAX dependency, but won't run scripts if not present
                if ( jQuery._evalUrl && !node.noModule ) {

                    /**
                     * Check for nonce backup value
                     */
                    if ( node.nonceValue ) {
                        node.setAttribute( "nonce", node.nonceValue );
                    }

                    jQuery._evalUrl( node.src,  node );
                }
            } else {
                DOMEval( node.textContent.replace( rcleanScript, "" ), node, doc );
            }
        }
    }
}
@mgol

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

Thanks for the report! jQuery.globalEval now supports passing nonce as the second attribute so that's kind-of taken (it's not released yet but please look on master). Passing the whole node wouldn't be a nice API IMO - jQuery.globalEval is a public API so we need to care it's as useful as possible and that internal parameters are not exposed.

The nonce attribute is not available to read after insertion due to the reasons outlined in this comment in DOMEval.js. We need to use the property if supported and - only if not - fallback to an attribute; no need to cache anything.

Since you found the issue and the possible solution, would you want to take a stab at solving this issue (I can help)? We plan to release jQuery 3.4.0 soon so if this doesn't land soon it'll have to wait until the next release.

@buddh4

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Thanks for the clarification, I'll have a look as soon as I can.

buddh4 added a commit to buddh4/jquery that referenced this issue Mar 19, 2019

mgol added a commit to buddh4/jquery that referenced this issue Mar 20, 2019

mgol added a commit to buddh4/jquery that referenced this issue Mar 20, 2019

@mgol mgol added the Manipulation label Mar 20, 2019

@mgol mgol added this to the 3.4.0 milestone Mar 20, 2019

@mgol

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

PR: #4328

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.