Permalink
Browse files

Fixes #11402. domManip now also removes the closing part of HTML comm…

…ents or CDATA surrounding executed scripts. Unit tests added.
  • Loading branch information...
1 parent e3cf0e2 commit a743be19bd3622071c22e9874c92024bc3f5367a @jaubourg jaubourg committed May 5, 2012
Showing with 19 additions and 1 deletion.
  1. +1 −1 src/manipulation.js
  2. +10 −0 test/data/cleanScript.html
  3. +8 −0 test/unit/ajax.js
View
@@ -29,7 +29,7 @@ var nodeNames = "abbr|article|aside|audio|bdi|canvas|data|datalist|details|figca
// checked="checked" or checked
rchecked = /checked\s*(?:[^=]|=\s*.checked.)/i,
rscriptType = /\/(java|ecma)script/i,
- rcleanScript = /^\s*<!(?:\[CDATA\[|\-\-)/,
+ rcleanScript = /^\s*<!(?:\[CDATA\[|\-\-)|[\]\-]{2}>\s*$/g,
@gnarf

gnarf May 6, 2012

Owner

I don't think any of the - need escapng here:

/^\s*<!(?:\[CDATA\[|--)|[\]-]{2}>\s*$/g

The first two have no special meaning outside of [] and the third one, so long as the hyphen is the first or last character inside of a [] it doesn't need to be escaped either.

@gnarf

gnarf May 6, 2012

Owner

Also, this shouldn't matter if the (?:\[CDATA\[|--) is a non-capture group should it? Might be able to remove the ?: as well

@jaubourg

jaubourg May 6, 2012

Member

AFAIK, jslint used to be very pedantic about escaping in regexp. Dunno, if jshint saves the day here.

As for the non-capture group, I seem to remember it's faster not to capture but I may be wrong.

@dmethvin

dmethvin May 7, 2012

Owner

I've was bitten by jshint's insistence on escaping chars inside [ ] about a month ago so yeah. And I'd leave it non-capture unless you need to capture, I thought it was a bit faster that way too and it could be a BIG chunk o script captured.

wrapMap = {
option: [ 1, "<select multiple='multiple'>", "</select>" ],
legend: [ 1, "<fieldset>", "</fieldset>" ],
View
@@ -0,0 +1,10 @@
+<script>
+<!--
+ok( true, "script within html comments executed" );
@dmethvin

dmethvin May 7, 2012

Owner

I can't figure out why the test case in http://bugs.jquery.com/ticket/10285 fails, but this unit test doesn't. Oh well, there's always tomorrow.

+-->
+</script>
+<script>
+<![CDATA[
+ok( true, "script within CDATA executed" );
+]]>
+</script>
View
@@ -2467,6 +2467,14 @@ test( "jQuery.domManip - no side effect because of ajaxSetup or global events (#
});
});
+test( "jQuery.domManip - script in comments are properly evaluated (#11402)", function() {
+ expect( 2 );
+ stop();
+ jQuery( "#qunit-fixture" ).load( "data/cleanScript.html", function() {
+ start();
+ });
+});
+
test("jQuery.ajax - active counter", function() {
ok( jQuery.active == 0, "ajax active counter should be zero: " + jQuery.active );
});

0 comments on commit a743be1

Please sign in to comment.