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

Core: Support passing nonce through jQuery.globalEval #4280

Merged
merged 1 commit into from Jan 21, 2019

Conversation

Projects
None yet
3 participants
@mgol
Copy link
Member

mgol commented Jan 17, 2019

Summary

Support passing nonce through jQuery.globalEval:

jQuery.globalEval( code, nonceValue );

Fixes gh-4278
Ref gh-3541
Ref gh-4269

Checklist

Docs issue: jquery/api.jquery.com#1123

@mgol mgol added the Core label Jan 17, 2019

@mgol mgol added this to the 3.4.0 milestone Jan 17, 2019

@mgol mgol self-assigned this Jan 17, 2019

@mgol

This comment has been minimized.

Copy link
Member Author

mgol commented Jan 17, 2019

I'll create docs issue if this gets accepted.

@mgol

This comment has been minimized.

Copy link
Member Author

mgol commented Jan 17, 2019

This is -1 +1 byte but that's a clever lie. It turns out you can set nonce via setAttribute, you just can't trust getAttribute( "nonce" ). Thus, I simplified the previous code which allowed to achieve the size decrease. Just applying the refactor without adding support for #4278 would give -11 bytes. So, effectively, this is +10 +12 bytes.

@mgol mgol requested review from dmethvin and timmywil Jan 17, 2019

@mgol mgol force-pushed the mgol:csp-nonce-globaleval branch from 1c422fd to 0d92506 Jan 17, 2019

@mgol

This comment has been minimized.

Copy link
Member Author

mgol commented Jan 17, 2019

The biggest question is: is this API OK? Or would you rather see something else?

src/core.js Outdated
@@ -238,8 +238,8 @@ jQuery.extend( {
},

// Evaluates a script in a global context
globalEval: function( code ) {
DOMEval( code );
globalEval: function( code, nonce ) {

This comment has been minimized.

@gibson042

gibson042 Jan 17, 2019

Member

I don't like the signature, one could easily imagine a realm parameter occupying that position. And I don't really feel great about an options object here, but maybe that is the best approach. :\

This comment has been minimized.

@mgol

mgol Jan 18, 2019

Author Member

Makes sense. I was also not quite happy about the signature but it's not a boolean so it's not that bad and I wasn't sure if an object isn't too much (we don't have a lot of APIs with one-prop object params, I think).

The object syntax const is just additional 2 bytes, not bad. Changes here: mgol@e49e3e8.

This comment has been minimized.

@mgol

mgol Jan 18, 2019

Author Member

Another version: mgol@8d9cf5e. Gzipped size is the same as mgol@e49e3e8, minified non-gzipped size is smaller by 7 bytes. And we're losing the getAttribute hacky check so perhaps that's better.

EDIT: This version is broken as we still need the getAttribute fallback for non-nonce params. But mgol@e49e3e8 is fine.

This comment has been minimized.

@timmywil

timmywil Jan 18, 2019

Member

I prefer the options object, even if there's only one option.

This comment has been minimized.

@mgol

mgol Jan 18, 2019

Author Member

@timmywil @gibson042 Cool. I updated the PR to use the options object. The old version is at mgol@0d92506 if anyone wanted to have a look.

// via an object.
val = node[ i ] || node.getAttribute && node.getAttribute( i );
if ( val ) {
script.setAttribute( i, val );

This comment has been minimized.

@gibson042

@mgol mgol force-pushed the mgol:csp-nonce-globaleval branch from 0d92506 to 1ce08f0 Jan 18, 2019

@timmywil
Copy link
Member

timmywil left a comment

LGTM

@mgol

This comment has been minimized.

Copy link
Member Author

mgol commented Jan 21, 2019

@mgol mgol removed the Needs review label Jan 21, 2019

@mgol mgol merged commit 5bdc85b into jquery:master Jan 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mgol mgol deleted the mgol:csp-nonce-globaleval branch Jan 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment