Fix DOM exception "Not Found Error" when cleaning up after invalid at…

…tribute removal (#3682)

* Defer cleanup removal of invalid attributes until determining if they are actually all removed

* Improve phpdoc comments

Co-Authored-By: Alain Schlesser <[email protected]>

* Add explanation to check server error log when errors occur

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 the 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 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()
+			 * 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 );
 			}
 		}
 

includes/validation/class-amp-validation-manager.php

@@ -1909,21 +1909,22 @@ public static function validate_url( $url ) {
 	 * @return string Error message.
 	 */
 	public static function get_validate_url_error_message( $error_code ) {
+		$check_error_log = __( 'Please check your server\'s PHP error logs; to do this you may need to enable WP_DEBUG_LOG.', 'amp' );
 		switch ( $error_code ) {
 			case 'http_request_failed':
-				return __( 'Failed to fetch URL(s) to validate. This may be due to a request timeout.', 'amp' );
+				return __( 'Failed to fetch URL(s) to validate. This may be due to a request timeout.', 'amp' ) . ' ' . $check_error_log;
 			case 'white_screen_of_death':
-				return __( 'Unable to validate URL. Encountered a white screen of death likely due to a fatal error. Please check your server\'s PHP error logs.', 'amp' );
+				return __( 'Unable to validate URL. Encountered a white screen of death likely due to a fatal error.', 'amp' ) . ' ' . $check_error_log;
 			case '404':
 				return __( 'The fetched URL was not found. It may have been deleted. If so, you can trash this.', 'amp' );
 			case '500':
-				return __( 'An internal server error occurred when fetching the URL for validation.', 'amp' );
+				return __( 'An internal server error occurred when fetching the URL for validation.', 'amp' ) . ' ' . $check_error_log;
 			case 'response_comment_absent':
 				return sprintf(
 					/* translators: %s: AMP_VALIDATION */
-					__( 'URL validation failed to due to the absence of the expected JSON-containing %s comment after the body.', 'amp' ),
+					__( 'URL validation failed to due to the absence of the expected JSON-containing %s comment after the body. This is often due to a PHP fatal error occurring.', 'amp' ),
 					'AMP_VALIDATION'
-				);
+				) . ' ' . $check_error_log;
 			case 'malformed_json_validation_errors':
 				return sprintf(
 					/* translators: %s: AMP_VALIDATION */

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' ],
+			],
 		];
 	}