Merge pull request #5388 from ampproject/fix/keeping-component-scripts-for-kept-invalid-markup

Gather required script components even for kept invalid markup

Author: westonruter

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

@@ -573,34 +573,34 @@ static function ( $validation_error ) {
 			}
 		}
 
+		$attr_spec_list = array_merge(
+			$this->globally_allowed_attributes,
+			$attr_spec_list
+		);
+
 		// Remove element if it has illegal CDATA.
 		if ( ! empty( $cdata ) && $node instanceof DOMElement ) {
 			$validity = $this->validate_cdata_for_node( $node, $cdata );
 			if ( true !== $validity ) {
-				$this->remove_invalid_child(
+				$sanitized = $this->remove_invalid_child(
 					$node,
 					array_merge(
 						$validity,
 						[ 'spec_name' => $this->get_spec_name( $node, $tag_spec ) ]
 					)
 				);
-				return null;
+				return $sanitized ? null : $this->get_required_script_components( $node, $tag_spec, $attr_spec_list );
 			}
 		}
 
-		$merged_attr_spec_list = array_merge(
-			$this->globally_allowed_attributes,
-			$attr_spec_list
-		);
-
 		// Amend spec list with layout.
 		if ( isset( $tag_spec['amp_layout'] ) ) {
-			$merged_attr_spec_list = array_merge( $merged_attr_spec_list, $this->layout_allowed_attributes );
+			$attr_spec_list = array_merge( $attr_spec_list, $this->layout_allowed_attributes );
 
 			if ( isset( $tag_spec['amp_layout']['supported_layouts'] ) ) {
 				$layouts = wp_array_slice_assoc( Layout::FROM_SPEC, $tag_spec['amp_layout']['supported_layouts'] );
 
-				$merged_attr_spec_list['layout'][ AMP_Rule_Spec::VALUE_REGEX_CASEI ] = '(' . implode( '|', $layouts ) . ')';
+				$attr_spec_list['layout'][ AMP_Rule_Spec::VALUE_REGEX_CASEI ] = '(' . implode( '|', $layouts ) . ')';
 			}
 		}
 
@@ -626,12 +626,12 @@ static function ( $validation_error ) {
 		// Remove the element if it is has an invalid layout.
 		$layout_validity = $this->is_valid_layout( $tag_spec, $node );
 		if ( true !== $layout_validity ) {
-			$this->remove_invalid_child( $node, $layout_validity );
-			return null;
+			$sanitized = $this->remove_invalid_child( $node, $layout_validity );
+			return $sanitized ? null : $this->get_required_script_components( $node, $tag_spec, $attr_spec_list );
 		}
 
 		// Identify attribute values that don't conform to the attr_spec.
-		$disallowed_attributes = $this->sanitize_disallowed_attribute_values_in_node( $node, $merged_attr_spec_list );
+		$disallowed_attributes = $this->sanitize_disallowed_attribute_values_in_node( $node, $attr_spec_list );
 
 		// Remove all invalid attributes.
 		if ( ! empty( $disallowed_attributes ) ) {
@@ -679,7 +679,7 @@ static function ( $validation_error ) {
 
 					if ( $this->should_sanitize_validation_error( $validation_error, [ 'node' => $attr_node ] ) ) {
 						$properties = $this->parse_properties_attribute( $attr_node->nodeValue );
-						if ( ! empty( $merged_attr_spec_list[ $attr_node->nodeName ]['value_properties'][ $error_data['name'] ]['mandatory'] ) ) {
+						if ( ! empty( $attr_spec_list[ $attr_node->nodeName ]['value_properties'][ $error_data['name'] ]['mandatory'] ) ) {
 							$properties[ $error_data['name'] ] = $error_data['required_value'];
 						} else {
 							unset( $properties[ $error_data['name'] ] );
@@ -697,7 +697,7 @@ static function ( $validation_error ) {
 						$node->setAttribute( $attr_node->nodeName, $this->serialize_properties_attribute( $properties ) );
 					}
 				} else {
-					$attr_spec = isset( $merged_attr_spec_list[ $attr_node->nodeName ] ) ? $merged_attr_spec_list[ $attr_node->nodeName ] : [];
+					$attr_spec = isset( $attr_spec_list[ $attr_node->nodeName ] ) ? $attr_spec_list[ $attr_node->nodeName ] : [];
 					if ( $this->remove_invalid_attribute( $node, $attr_node, $validation_error, $attr_spec ) ) {
 						$removed_attributes[] = $attr_node;
 					}
@@ -726,71 +726,82 @@ static function ( $validation_error ) {
 		}
 
 		// After attributes have been sanitized (and potentially removed), if mandatory attribute(s) are missing, remove the element.
-		$missing_mandatory_attributes = $this->get_missing_mandatory_attributes( $merged_attr_spec_list, $node );
+		$missing_mandatory_attributes = $this->get_missing_mandatory_attributes( $attr_spec_list, $node );
 		if ( ! empty( $missing_mandatory_attributes ) ) {
-			$this->remove_invalid_child(
+			$sanitized = $this->remove_invalid_child(
 				$node,
 				[
 					'code'       => self::ATTR_REQUIRED_BUT_MISSING,
 					'attributes' => $missing_mandatory_attributes,
 					'spec_name'  => $this->get_spec_name( $node, $tag_spec ),
 				]
 			);
-			return null;
+			return $sanitized ? null : $this->get_required_script_components( $node, $tag_spec, $attr_spec_list );
 		}
 
 		if ( ! empty( $tag_spec[ AMP_Rule_Spec::MANDATORY_ANYOF ] ) ) {
 			$anyof_attributes = $this->get_element_attribute_intersection( $node, $tag_spec[ AMP_Rule_Spec::MANDATORY_ANYOF ] );
 			if ( 0 === count( $anyof_attributes ) ) {
-				$this->remove_invalid_child(
+				$sanitized = $this->remove_invalid_child(
 					$node,
 					[
 						'code'                  => self::MANDATORY_ANYOF_ATTR_MISSING,
 						'mandatory_anyof_attrs' => $tag_spec[ AMP_Rule_Spec::MANDATORY_ANYOF ], // @todo Temporary as value can be looked up via spec name. See https://github.com/ampproject/amp-wp/pull/3817.
 						'spec_name'             => $this->get_spec_name( $node, $tag_spec ),
 					]
 				);
-				return null;
+				return $sanitized ? null : $this->get_required_script_components( $node, $tag_spec, $attr_spec_list );
 			}
 		}
 
 		if ( ! empty( $tag_spec[ AMP_Rule_Spec::MANDATORY_ONEOF ] ) ) {
 			$oneof_attributes = $this->get_element_attribute_intersection( $node, $tag_spec[ AMP_Rule_Spec::MANDATORY_ONEOF ] );
 			if ( 0 === count( $oneof_attributes ) ) {
-				$this->remove_invalid_child(
+				$sanitized = $this->remove_invalid_child(
 					$node,
 					[
 						'code'                  => self::MANDATORY_ONEOF_ATTR_MISSING,
 						'mandatory_oneof_attrs' => $tag_spec[ AMP_Rule_Spec::MANDATORY_ONEOF ], // @todo Temporary as value can be looked up via spec name. See https://github.com/ampproject/amp-wp/pull/3817.
 						'spec_name'             => $this->get_spec_name( $node, $tag_spec ),
 					]
 				);
-				return null;
+				return $sanitized ? null : $this->get_required_script_components( $node, $tag_spec, $attr_spec_list );
 			} elseif ( count( $oneof_attributes ) > 1 ) {
-				$this->remove_invalid_child(
+				$sanitized = $this->remove_invalid_child(
 					$node,
 					[
 						'code'                  => self::DUPLICATE_ONEOF_ATTRS,
 						'duplicate_oneof_attrs' => $oneof_attributes,
 						'spec_name'             => $this->get_spec_name( $node, $tag_spec ),
 					]
 				);
-				return null;
+				return $sanitized ? null : $this->get_required_script_components( $node, $tag_spec, $attr_spec_list );
 			}
 		}
 
-		// Add required AMP component scripts.
+		return $this->get_required_script_components( $node, $tag_spec, $attr_spec_list );
+	}
+
+	/**
+	 * Get required AMP component scripts.
+	 *
+	 * @param DOMElement $node           Element.
+	 * @param array      $tag_spec       Tag spec.
+	 * @param array      $attr_spec_list Attribute spec list.
+	 * @return string[] Script component handles.
+	 */
+	private function get_required_script_components( DOMElement $node, $tag_spec, $attr_spec_list ) {
 		$script_components = [];
 		if ( ! empty( $tag_spec['requires_extension'] ) ) {
 			$script_components = array_merge( $script_components, $tag_spec['requires_extension'] );
 		}
 
 		// Add required AMP components for attributes.
 		foreach ( $node->attributes as $attribute ) {
-			if ( isset( $merged_attr_spec_list[ $attribute->nodeName ]['requires_extension'] ) ) {
+			if ( isset( $attr_spec_list[ $attribute->nodeName ]['requires_extension'] ) ) {
 				$script_components = array_merge(
 					$script_components,
-					$merged_attr_spec_list[ $attribute->nodeName ]['requires_extension']
+					$attr_spec_list[ $attribute->nodeName ]['requires_extension']
 				);
 			}
 		}
@@ -809,7 +820,6 @@ static function ( $validation_error ) {
 				}
 			}
 		}
-
 		return $script_components;
 	}