Merge pull request #1119 from Automattic/fix/convert-amp-bind-attributes

Fix handling of amp-bind attributes to ensure that “>” can appear inside attribute values
ampproject · May 5, 2018 · 03f12f2 · 03f12f2
2 parents 2328f71 + c79af80
commit 03f12f2
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 12 deletions.
diff --git a/includes/utils/class-amp-dom-utils.php b/includes/utils/class-amp-dom-utils.php
@@ -207,7 +207,7 @@ public static function convert_amp_bind_attributes( $html ) {
 		$amp_bind_attr_prefix = self::get_amp_bind_placeholder_prefix();
 
 		// Pattern for HTML attribute accounting for binding attr name, boolean attribute, single/double-quoted attribute value, and unquoted attribute values.
-		$attr_regex = '#^\s+(?P<name>\[?[a-zA-Z0-9_\-]+\]?)(?P<value>=(?:"[^"]*"|\'[^\']*\'|[^\'"\s]+))?#';
+		$attr_regex = '#^\s+(?P<name>\[?[a-zA-Z0-9_\-]+\]?)(?P<value>=(?:"[^"]*+"|\'[^\']*+\'|[^\'"\s]+))?#';
 
 		/**
 		 * Replace callback.
@@ -240,14 +240,18 @@ public static function convert_amp_bind_attributes( $html ) {
 			return '<' . $tag_matches['name'] . $new_attrs . '>';
 		};
 
-		/*
-		 * Match all start tags that probably contain a binding attribute.
-		 * @todo Warning: The following pattern is brittle since it truncates HTML tags that have attributes that contain ">" or "=>".
-		 * For example, if there exists: `<body class="foo" [class]="bodyClasses.concat( isBar ? 'bar' : '' ).filter( className => '' != className )">`
-		 * Then this results in the following being output as the first child fo the body: `'' != className )">`
-		 */
+		// Match all start tags that contain a binding attribute.
+		$pattern   = join( '', array(
+			'#<',
+			'(?P<name>[a-zA-Z0-9_\-]+)',               // Tag name.
+			'(?P<attrs>\s',                            // Attributes.
+			'(?:[^>"\'\[\]]+|"[^"]*+"|\'[^\']*+\')*+', // Non-binding attributes tokens.
+			'\[[a-zA-Z0-9_\-]+\]',                     // One binding attribute key.
+			'(?:[^>"\']+|"[^"]*+"|\'[^\']*+\')*+',     // Any attribute tokens, including binding ones.
+			')>#s',
+		) );
 		$converted = preg_replace_callback(
-			'#<(?P<name>[a-zA-Z0-9_\-]+)(?P<attrs>\s[^>]+\]=[^>]+)>#',
+			$pattern,
 			$replace_callback,
 			$html
 		);

diff --git a/tests/test-class-amp-dom-utils.php b/tests/test-class-amp-dom-utils.php
@@ -110,12 +110,12 @@ public function test_amp_bind_conversion() {
 		// Test malformed.
 		$malformed_html = array(
 			'<amp-img width="123" [text]="..."</amp-img>',
-			'<amp-img width="123" [text] data-test="asd"></amp-img>',
+			'<amp-img width="123" [text="..."]></amp-img>',
 			'<amp-img width="123" [text]="..." *bad*></amp-img>',
 		);
 		foreach ( $malformed_html as $html ) {
 			$converted = AMP_DOM_Utils::convert_amp_bind_attributes( $html );
-			$this->assertNotContains( AMP_DOM_Utils::get_amp_bind_placeholder_prefix(), $converted );
+			$this->assertNotContains( AMP_DOM_Utils::get_amp_bind_placeholder_prefix(), $converted, "Source: $html" );
 		}
 	}
 
@@ -242,6 +242,8 @@ public function get_table_row_iterations() {
 			array( 10 ),
 			array( 100 ),
 			array( 1000 ),
+			array( 10000 ),
+			array( 100000 ),
 		);
 	}
 

diff --git a/tests/test-tag-and-attribute-sanitizer.php b/tests/test-tag-and-attribute-sanitizer.php
@@ -709,9 +709,15 @@ public function get_body_data() {
 				array( 'amp-bind' ),
 			),
 
+			'amp_bind_with_greater_than_symbol'                         => array(
+				'<div class="home page-template-default page page-id-7 logged-in wp-custom-logo group-blog" [class]="minnow.bodyClasses.concat( minnow.navMenuExpanded ? \'sidebar-open\' : \'\' ).filter( className => \'\' != className )">hello</div>',
+				'<div class="home page-template-default page page-id-7 logged-in wp-custom-logo group-blog" [class]="minnow.bodyClasses.concat( minnow.navMenuExpanded ? \'sidebar-open\' : \'\' ).filter( className =&gt; \'\' != className )">hello</div>',
+				array( 'amp-bind' ),
+			),
+
 			'amp_bad_bind_attr'                                         => array(
-				'<a [unrecognized] [href]="/">test</a><p [text]="\'Hello \' + name">Hello World</p>',
-				'<a [href]="/">test</a><p [text]="\'Hello \' + name">Hello World</p>',
+				'<a [href]=\'/\' [hidden]>test</a><p [text]="\'Hello \' + name" [unrecognized] title="Foo"><button [disabled]="" [type]=\'\'>Hello World</button></p>',
+				'<a [href]="/" [hidden]>test</a><p [text]="\'Hello \' + name" title="Foo"><button [disabled]="" [type]="">Hello World</button></p>',
 				array( 'amp-bind' ),
 			),