Manipulation: Avoid concatenating strings in buildFragment

mgol · mgol · commit 872affd3bfe7 · 2020-06-05T18:59:41.000+02:00
Concatenating HTML strings in buildFragment is a possible security risk as it creates an opportunity of escaping the concatenated wrapper. It also makes it impossible to support secure HTML wrappers like [trusted types](https://web.dev/trusted-types/). It's safer to create wrapper elements using `document.createElement` & `appendChild`. The previous way was needed in jQuery <4 because IE <10 doesn't accept table parts set via `innerHTML`, even if the element which contents are set is a proper table element, e.g.: ```js tr.innerHTML = "<td></td>"; ``` The whole structure needs to be passed in one HTML string. jQuery 4 drops support for IE <11 so this is no longer an issue; in older version we'd have to duplicate the code paths. IE <10 needed to have `<option>` elements wrapped in `<select multiple="multiple">` but we no longer need that on master which makes the `document.createElement` way shorter as we don't have to call `setAttribute`. jQuery 1.x sometimes needed to have more than one element in the wrapper that would precede parts wrapping HTML input so descending needed to use `lastChild`. Since all wrappers are single-element now, we can use `firstChild` which compresses better as it's used in other places in the code as well. All these improvements, apart from making logic more secure, decrease the gzipped size by 55 bytes. Ref jquerygh-4409 Ref angular/angular.js#17028
diff --git a/src/manipulation/buildFragment.js b/src/manipulation/buildFragment.js
@@ -1,6 +1,8 @@
 import jQuery from "../core.js";
 import toType from "../core/toType.js";
 import isAttached from "../core/isAttached.js";
+import arr from "../var/arr.js";
+import document from "../var/document.js";
 import rtagName from "./var/rtagName.js";
 import rscriptType from "./var/rscriptType.js";
 import wrapMap from "./wrapMap.js";
@@ -35,15 +37,17 @@ function buildFragment( elems, context, scripts, selection, ignored ) {
 
 				// Deserialize a standard representation
 				tag = ( rtagName.exec( elem ) || [ "", "" ] )[ 1 ].toLowerCase();
-				wrap = wrapMap[ tag ] || wrapMap._default;
-				tmp.innerHTML = wrap[ 1 ] + jQuery.htmlPrefilter( elem ) + wrap[ 2 ];
+				wrap = wrapMap[ tag ] || arr;
 
-				// Descend through wrappers to the right content
-				j = wrap[ 0 ];
-				while ( j-- ) {
-					tmp = tmp.lastChild;
+				// Create wrappers & descend into them.
+				j = wrap.length;
+				while ( --j > -1 ) {
+					tmp.appendChild( document.createElement( wrap[ j ] ) );
+					tmp = tmp.firstChild;
 				}
 
+				tmp.innerHTML = jQuery.htmlPrefilter( elem );
+
 				jQuery.merge( nodes, tmp.childNodes );
 
 				// Remember the top-level container
diff --git a/src/manipulation/wrapMap.js b/src/manipulation/wrapMap.js
@@ -1,17 +1,14 @@
-// We have to close these tags to support XHTML (#13200)
 var wrapMap = {
 
 	// Table parts need to be wrapped with `<table>` or they're
 	// stripped to their contents when put in a div.
 	// XHTML parsers do not magically insert elements in the
 	// same way that tag soup parsers do, so we cannot shorten
 	// this by omitting <tbody> or other required elements.
-	thead: [ 1, "<table>", "</table>" ],
-	col: [ 2, "<table><colgroup>", "</colgroup></table>" ],
-	tr: [ 2, "<table><tbody>", "</tbody></table>" ],
-	td: [ 3, "<table><tbody><tr>", "</tr></tbody></table>" ],
-
-	_default: [ 0, "", "" ]
+	thead: [ "table" ],
+	col: [ "colgroup", "table" ],
+	tr: [ "tbody", "table" ],
+	td: [ "tr", "tbody", "table" ]
 };
 
 wrapMap.tbody = wrapMap.tfoot = wrapMap.colgroup = wrapMap.caption = wrapMap.thead;
diff --git a/test/unit/manipulation.js b/test/unit/manipulation.js
@@ -2969,3 +2969,14 @@ QUnit.test( "Sanitized HTML doesn't get unsanitized", function( assert ) {
 		test( "<noembed><noembed/><img src=url404 onerror=xss(12)>" );
 	}
 } );
+
+QUnit.test( "Works with invalid attempts to close the table wrapper", function( assert ) {
+	assert.expect( 3 );
+
+	// This test case attempts to close the tags which wrap input
+	// based on matching done in wrapMap which should be ignored.
+	var elem = jQuery( "<td></td></tr></tbody></table><td></td>" );
+	assert.strictEqual( elem.length, 2, "Two elements created" );
+	assert.strictEqual( elem[ 0 ].nodeName.toLowerCase(), "td", "First element is td" );
+	assert.strictEqual( elem[ 1 ].nodeName.toLowerCase(), "td", "Second element is td" );
+} );
