Skip to content

Commit

Permalink
Fixes #9221. Wraps openings of html comments and CDATA blocks found a…
Browse files Browse the repository at this point in the history
…t the beginning of inserted script elements into a javascript block comment so that the new implementation of globalEval will not throw an exception in IE (execScript being less lenient than eval). Unit tests added.
  • Loading branch information
jaubourg committed May 11, 2011
1 parent 3a1c27b commit 391398c
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 3 deletions.
5 changes: 3 additions & 2 deletions src/manipulation.js
Expand Up @@ -10,6 +10,7 @@ var rinlinejQuery = / jQuery\d+="(?:\d+|null)"/g,
// checked="checked" or checked
rchecked = /checked\s*(?:[^=]|=\s*.checked.)/i,
rscriptType = /\/(java|ecma)script/i,
rcleanScript = /^\s*<!(?:\[CDATA\[|\-\-)/,
wrapMap = {
option: [ 1, "<select multiple='multiple'>", "</select>" ],
legend: [ 1, "<fieldset>", "</fieldset>" ],
Expand Down Expand Up @@ -500,7 +501,7 @@ jQuery.each({
function getAll( elem ) {
if ( "getElementsByTagName" in elem ) {
return elem.getElementsByTagName( "*" );

} else if ( "querySelectorAll" in elem ) {
return elem.querySelectorAll( "*" );

Expand Down Expand Up @@ -738,7 +739,7 @@ function evalScript( i, elem ) {
dataType: "script"
});
} else {
jQuery.globalEval( elem.text || elem.textContent || elem.innerHTML || "" );
jQuery.globalEval( ( elem.text || elem.textContent || elem.innerHTML || "" ).replace( rcleanScript, "/*$0*/" ) );
}

if ( elem.parentNode ) {
Expand Down
20 changes: 19 additions & 1 deletion test/unit/manipulation.js
Expand Up @@ -1044,7 +1044,7 @@ test("clone(form element) (Bug #3879, #6655)", function() {

equals( clone.is(":checked"), element.is(":checked"), "Checked input cloned correctly" );
equals( clone[0].defaultValue, "foo", "Checked input defaultValue cloned correctly" );

// defaultChecked also gets set now due to setAttribute in attr, is this check still valid?
// equals( clone[0].defaultChecked, !jQuery.support.noCloneChecked, "Checked input defaultChecked cloned correctly" );

Expand Down Expand Up @@ -1396,3 +1396,21 @@ test("jQuery.buildFragment - no plain-text caching (Bug #6779)", function() {
equals($f.text(), bad.join(""), "Cached strings that match Object properties");
$f.remove();
});

test( "jQuery.html - execute scripts escaped with html comment or CDATA (#9221)", function() {
expect( 2 );
jQuery( [
'<script type="text/javascript">',
'<!--',
'ok( true, "<!-- handled" );',
'//-->',
'</script>'
].join ( "\n" ) ).appendTo( "#qunit-fixture" );
jQuery( [
'<script type="text/javascript">',
'<![CDATA[',
'ok( true, "<![CDATA[ handled" );',
'//]]>',
'</script>'
].join ( "\n" ) ).appendTo( "#qunit-fixture" );
});

3 comments on commit 391398c

@jeresig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this commit. Why would someone comment out the last line of the comment but not the first one? What's the use case here? And is this really important enough to land post-RC?

@jaubourg
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, every browser we support do allow an opening html comment at the start of inline script tags, exemplified by the fact it worked in 1.5.2 when we were still using script tag injection. Sadly, this feature is popular enough to have a library like Drupal use it.

If you want to revert it, feel free to do it, but it's clearly a regression, it breaks a major PHP library, it's actually a bug since what we were doing with scripts in 1.6.0 wasn't symetric with what the browsers do. Thought it was perfectly into the boundaries between a RC and a release.

@jdalton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yea! That's another reason script injection was awesome, easy xbrowser support for both comment styles in the code. This continues to bite Prototype, it seems some security software would also wrap some ajax responses.

Please sign in to comment.