/jquery

Permalink
Browse files

Fixes #13714. jQuery.globalEval gotcha w/ strings that contain valid,… 

… prologue position strict mode pragma

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
  • Loading branch information...
@rwaldron
rwaldron committed Apr 3, 2013
1 parent 05d55d0 commit feea9394b75a5dcb16fad013a402b388f97cefba
Unified Split
Showing with 27 additions and 4 deletions.
  1. +19 −4 src/core.js
  2. +8 −0 test/unit/core.js
View
23 src/core.js
@@ -515,10 +515,25 @@ jQuery.extend({
noop: function() {},
// Evaluates a script in a global context
globalEval: function( data ) {
var indirect = eval;
if ( jQuery.trim( data ) ) {
indirect( data + ";" );
globalEval: function( code ) {
var script,
indirect = eval;
code = jQuery.trim( code ) + ";";

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Apr 3, 2013

Member

I've see the ";" was there previously too. What's the reason it's required?
Copy URL
@jdalton

jdalton Apr 3, 2013

Member

I've see the ";" was there previously too. What's the reason it's required?

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Apr 3, 2013

Member

Ya know... I recall there was some error when I was first re-writing the core.js code for 2.0, but now I can't reproduce it... I'll pull this out. Thanks for making me re-look at that
Copy URL
@rwaldron

rwaldron Apr 3, 2013

Member

Ya know... I recall there was some error when I was first re-writing the core.js code for 2.0, but now I can't reproduce it... I'll pull this out. Thanks for making me re-look at that

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Apr 3, 2013

Member

I think I found why:

eval(Object('alert("s")'));// will not alert "a" and instead returns the string object

From http://es5.github.com/#x15.1.2.1

  1. If Type(x) is not String, return x.

So the ";" is used to coerce it to a string (though "" would do)
Copy URL
@jdalton

jdalton Apr 3, 2013

Member

I think I found why:

eval(Object('alert("s")'));// will not alert "a" and instead returns the string object

From http://es5.github.com/#x15.1.2.1

  1. If Type(x) is not String, return x.

So the ";" is used to coerce it to a string (though "" would do)

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 3, 2013

Member

Yeah that bit me long ago. Why the spec thinks that is right I'll never know.
Copy URL
@dmethvin

dmethvin Apr 3, 2013

Member

Yeah that bit me long ago. Why the spec thinks that is right I'll never know.

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Apr 3, 2013

Member

But now we're using the trimmed content, where before we weren't. So either this isn't necessary or the the coercion has to happen before the trim, right?
Copy URL
@scottgonzalez

scottgonzalez Apr 3, 2013

Member

But now we're using the trimmed content, where before we weren't. So either this isn't necessary or the the coercion has to happen before the trim, right?

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Apr 3, 2013

Member

The trim itself provides the coercion
Copy URL
@rwaldron

rwaldron Apr 3, 2013

Member

The trim itself provides the coercion

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Apr 3, 2013

Member

@scottgonzalez trrrriiiiimmm! That's the second time I've overlooked that ;P
Copy URL
@jdalton

jdalton Apr 3, 2013

Member

@scottgonzalez trrrriiiiimmm! That's the second time I've overlooked that ;P
if ( code ) {
// If the code includes a valid, prologue position
// strict mode pragma, execute code by injecting a
// script tag into the document.
if ( code.indexOf("use strict") === 1 ) {

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Apr 3, 2013

Member

There could be whitespace before the "use strict" directive.
Copy URL
@jdalton

jdalton Apr 3, 2013

Member

There could be whitespace before the "use strict" directive.

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Apr 3, 2013

Member

Line 522 calls jQuery.trim().
Copy URL
@scottgonzalez

scottgonzalez Apr 3, 2013

Member

Line 522 calls jQuery.trim().

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Apr 3, 2013

Member

@scottgonzalez oh word!
Copy URL
@jdalton

jdalton Apr 3, 2013

Member

@scottgonzalez oh word!
script = document.createElement("script");
script.text = code;
document.head.appendChild( script ).parentNode.removeChild( script );
} else {
// Otherwise, avoid the DOM node creation, insertion
// and removal by using an indirect global eval
indirect( code );
}
}

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Apr 3, 2013

Member

Why 2 code paths? Could we use PRs for this kind of stuff, please?
Copy URL
@jaubourg

jaubourg Apr 3, 2013

Member

Why 2 code paths? Could we use PRs for this kind of stuff, please?

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Apr 3, 2013

Member

Sorry, I thought my comments were sufficient:

If the code includes a valid, prologue position
strict mode pragma, execute code by injecting a
script tag into the document.
... Do the injection.


Otherwise, avoid the DOM node creation, insertion
and removal by using an indirect global eval

I'm confident that the path needed for "use strict" will be fairly uncommon with jQuery users, so I'd rather not impose any potential performance penalties of a DOM operation on the most common cases.
Copy URL
@rwaldron

rwaldron Apr 3, 2013

Member

Sorry, I thought my comments were sufficient:

If the code includes a valid, prologue position
strict mode pragma, execute code by injecting a
script tag into the document.
... Do the injection.


Otherwise, avoid the DOM node creation, insertion
and removal by using an indirect global eval

I'm confident that the path needed for "use strict" will be fairly uncommon with jQuery users, so I'd rather not impose any potential performance penalties of a DOM operation on the most common cases.
},
View
8 test/unit/core.js
@@ -215,6 +215,14 @@ test( "globalEval", function() {
equal( window.globalEvalTest, 3, "Test context (this) is the window object" );
});
test( "globalEval with 'use strict'", function() {
expect( 1 );
Globals.register("strictEvalTest");
jQuery.globalEval("'use strict'; var strictEvalTest = 1;");
equal( window.strictEvalTest, 1, "Test variable declarations are global (strict mode)" );
});
test("noConflict", function() {
expect(7);

1 comment on commit feea939

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Apr 3, 2013

Member

Related, cujojs/curl.
Member

jdalton commented on feea939 Apr 3, 2013

Related, cujojs/curl.
Please sign in to comment.