Defer cleanup removal of invalid attributes until determining if they…

… are actually all removed

includes/sanitizers/class-amp-base-sanitizer.php

@@ -487,7 +487,6 @@ public function remove_invalid_attribute( $element, $attribute, $validation_erro
 		$should_remove = $this->should_sanitize_validation_error( $validation_error, compact( 'node' ) );
 		if ( $should_remove ) {
 			$element->removeAttributeNode( $node );
-			$this->clean_up_after_attribute_removal( $element, $node, $validation_error );
 		}
 		return $should_remove;
 	}
@@ -591,12 +590,10 @@ public function prepare_validation_error( array $error = [], array $data = [] )
 	 *
 	 * @since 1.3
 	 *
-	 * @param DOMElement $element          The node for which he attribute was
-	 *                                     removed.
-	 * @param DOMAttr    $attribute        The attribute that was removed.
-	 * @param array      $validation_error Validation error details.
+	 * @param DOMElement $element   The node for which he attribute was removed.
+	 * @param DOMAttr    $attribute The attribute that was removed.
 	 */
-	protected function clean_up_after_attribute_removal( $element, $attribute, $validation_error ) {
+	protected function clean_up_after_attribute_removal( $element, $attribute ) {
 		static $attributes_tied_to_href = [ 'target', 'download', 'rel', 'rev', 'hreflang', 'type' ];
 
 		if ( 'href' === $attribute->nodeName ) {

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

@@ -583,8 +583,24 @@ private function process_node( DOMElement $node ) {
 			foreach ( $node->attributes as $attribute ) {
 				$validation_error['element_attributes'][ $attribute->nodeName ] = $attribute->nodeValue;
 			}
+			$removed_attributes = [];
 			foreach ( $disallowed_attributes as $disallowed_attribute ) {
-				$this->remove_invalid_attribute( $node, $disallowed_attribute, $validation_error );
+				if ( $this->remove_invalid_attribute( $node, $disallowed_attribute, $validation_error ) ) {
+					$removed_attributes[] = $disallowed_attribute;
+				}
+			}
+
+			/*
+			 * Only run cleanup after the fact to prevent a scenario where invalid markup is kept and so the attribute
+			 * is actually not removed. This prevents a "DOMException: Not Found Error" from happening in when calling
+			 * remove_invalid_attribute() since clean_up_after_attribute_removal() can end up removing invalid link
+			 * attributes (like 'target') when there is an invalid 'href' attribute, but if the 'target' attribute is
+			 * itself invalid, then if clean_up_after_attribute_removal is called inside of remove_invalid_attribute
+			 * then it can cause a subsequent invocation of remove_invalid_attribute to try to remove an invalid
+			 * attribute that has already been removed from the DOM.
+			 */
+			foreach ( $removed_attributes as $removed_attribute ) {
+				$this->clean_up_after_attribute_removal( $node, $removed_attribute );
 			}
 		}
 

tests/php/test-tag-and-attribute-sanitizer.php

@@ -1707,6 +1707,13 @@ static function() {
 				[],
 				[ 'invalid_processing_instruction' ],
 			],
+
+			'malformed_attribute_syntax_curly_quotes'      => [
+				'<a href=“%E2%80%9Chttps://example.com/path/to/post/%E2%80%9D“ target=“_blank“ rel=“noopener“>Whatever</a>',
+				'<a>Whatever</a>',
+				[],
+				[ 'invalid_attribute', 'invalid_attribute' ],
+			],
 		];
 	}