Skip to content

Commit

Permalink
Refactor how sources are determined for validation
Browse files Browse the repository at this point in the history 
* Allow sources to be determined when calling remove_invalid_child() from any sanitizers (not just whitelist).
* Include more context in sources, including the shortcode tag and hook name.
* Run removal callback before node is removed from document.
* Add amp_disable_invalid_removal arg to prevent invalid elements and attributes from being removed (for debugging).
* Prevent calling reset_removed() inside get_response() so that multiple copies of response can be obtained. Reduce side effects.
* Remove use of constant for 'remove_invalid_callback' key since adds class dependency from AMP_Base_Sanitizer to AMP_Validation_Utils.
* Prevent wrapping action output if it does not contain HTML tags, to prevent comments from corrupting actions triggered in HTML attributes.
* Rename get_response() to get_validation_results() to prevent confusion with returning WP_REST_Response.
* Add comment to end of body that contains the validation results when validation is being performed.
* Check should_validate_front_end before calling store_validation_errors, not the other way around.
* Remove redundant calls to has_cap().
* Add reporting for removal of amp-keyframe element.
* Allow reporting of external stylesheet links.
* Prevent applying the_content filters twice when calling validate_content.
  • Loading branch information
@westonruter
westonruter committed Mar 2, 2018
1 parent 2715cf1 commit 76e2be5
Show file tree
Hide file tree
Showing 9 changed files with 306 additions and 238 deletions.
25 changes: 19 additions & 6 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,10 @@ public static function register_hooks() {
add_action( 'comment_form', array( __CLASS__, 'add_amp_comment_form_templates' ), 100 );
remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' );

if ( AMP_Validation_Utils::should_validate_front_end() ) {
AMP_Validation_Utils::add_validation_hooks();
}

// @todo Add character conversion.
}

Expand Down Expand Up @@ -955,11 +959,12 @@ public static function prepare_response( $response, $args = array() ) {

$args = array_merge(
array(
'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat.
'use_document_element' => true,
AMP_Validation_Utils::CALLBACK_KEY => null,
'allow_dirty_styles' => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
'allow_dirty_scripts' => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat.
'use_document_element' => true,
'remove_invalid_callback' => null,
'allow_dirty_styles' => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
'allow_dirty_scripts' => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
'disable_invalid_removal' => ! empty( $_REQUEST['amp_disable_invalid_removal'] ), // WPCS: csrf ok.
),
$args
);
Expand Down Expand Up @@ -994,7 +999,15 @@ public static function prepare_response( $response, $args = array() ) {
trigger_error( esc_html( sprintf( __( 'The database has the %s encoding when it needs to be utf-8 to work with AMP.', 'amp' ), get_bloginfo( 'charset' ) ) ), E_USER_WARNING ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
}

AMP_Validation_Utils::store_validation_errors();
if ( AMP_Validation_Utils::should_validate_front_end() ) {
AMP_Validation_Utils::store_validation_errors();
$comment = $dom->createComment( "\nValidation Status:\n" . wp_json_encode( AMP_Validation_Utils::get_validation_results() ) );
$body = $dom->getElementsByTagName( 'body' )->item( 0 );
if ( $body ) {
$body->appendChild( $comment );
}
}

$response = "<!DOCTYPE html>\n";
$response .= AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement );

Expand Down
65 changes: 21 additions & 44 deletions includes/sanitizers/class-amp-base-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ abstract class AMP_Base_Sanitizer {
* @type array $amp_bind_placeholder_prefix
* @type bool $allow_dirty_styles
* @type bool $allow_dirty_scripts
* @type bool $disable_invalid_removal
* @type callable $remove_invalid_callback
* }
*/
Expand All @@ -76,15 +77,6 @@ abstract class AMP_Base_Sanitizer {
*/
protected $root_element;

/**
* Stack of the plugin or theme that is outputting markup, if any.
*
* Each item in the stack is an array containing the 'type' (theme, plugin, mu-plugin) and the 'name' of the extension.
*
* @var array[][]
*/
public $current_sources = array();

/**
* AMP_Base_Sanitizer constructor.
*
Expand Down Expand Up @@ -334,16 +326,17 @@ public function maybe_enforce_https_src( $src, $force_https = false ) {
*/
public function remove_invalid_child( $child ) {
$parent = $child->parentNode;
$child->parentNode->removeChild( $child );
if ( isset( $this->args[ AMP_Validation_Utils::CALLBACK_KEY ] ) ) {
call_user_func( $this->args[ AMP_Validation_Utils::CALLBACK_KEY ],
if ( isset( $this->args['remove_invalid_callback'] ) ) {
call_user_func( $this->args['remove_invalid_callback'],
array(
'node' => $child,
'parent' => $parent,
'sources' => $this->current_sources,
'node' => $child,
'parent' => $parent,
)
);
}
if ( empty( $this->args['disable_invalid_removal'] ) ) {
$child->parentNode->removeChild( $child );
}
}

/**
Expand All @@ -359,43 +352,27 @@ public function remove_invalid_child( $child ) {
* @return void
*/
public function remove_invalid_attribute( $element, $attribute ) {
if ( isset( $this->args[ AMP_Validation_Utils::CALLBACK_KEY ] ) ) {
if ( isset( $this->args['remove_invalid_callback'] ) ) {
if ( is_string( $attribute ) ) {
$attribute = $element->getAttributeNode( $attribute );
}
if ( $attribute ) {
$element->removeAttributeNode( $attribute );
call_user_func( $this->args[ AMP_Validation_Utils::CALLBACK_KEY ],
call_user_func( $this->args['remove_invalid_callback'],
array(
'node' => $attribute,
'parent' => $element,
'sources' => $this->current_sources,
'node' => $attribute,
'parent' => $element,
)
);
if ( empty( $this->args['disable_invalid_removal'] ) ) {
$element->removeAttributeNode( $attribute );
}
}
} elseif ( empty( $this->args['disable_invalid_removal'] ) ) {
if ( is_string( $attribute ) ) {
$element->removeAttribute( $attribute );
} else {
$element->removeAttributeNode( $attribute );
}
} elseif ( is_string( $attribute ) ) {
$element->removeAttribute( $attribute );
} else {
$element->removeAttributeNode( $attribute );
}
}

/**
* Updates the captured current plugin that is outputting markup.
*
* @since 0.7
*
* @param DOMComment $node The node to check for the presence of a plugin in a comment.
* @return void
*/
public function capture_current_source( $node ) {
if ( ! preg_match( '#(?P<closing>/)?(?P<type>theme|plugin|mu-plugin):(?P<name>.*)#s', $node->nodeValue, $matches ) ) {
return;
}
if ( ! empty( $matches['closing'] ) ) {
$this->current_sources[] = wp_array_slice_assoc( $matches, array( 'type', 'name' ) );
} else {
array_pop( $this->current_sources );
}
}
}
6 changes: 3 additions & 3 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ public function sanitize() {
}
$this->amp_custom_style_element->appendChild( $this->dom->createTextNode( $css ) );

// @todo This would be a candidate for sanitization reporting.
// @todo This would be a candidate for sanitization reporting, when ! empty( $skipped ) || $total_size > $this->custom_max_size.
// Add comments to indicate which sheets were not included.
foreach ( array_reverse( $skipped ) as $skip ) {
$this->amp_custom_style_element->parentNode->insertBefore(
Expand Down Expand Up @@ -306,7 +306,7 @@ private function process_style_element( DOMElement $element ) {
if ( 'body' === $element->parentNode->nodeName && $element->hasAttribute( 'amp-keyframes' ) ) {
$validity = $this->validate_amp_keyframe( $element );
if ( true !== $validity ) {
$element->parentNode->removeChild( $element ); // @todo Add reporting.
$this->remove_invalid_child( $element );
}
return;
}
Expand Down Expand Up @@ -344,7 +344,7 @@ private function process_link_element( DOMElement $element ) {

$css_file_path = $this->get_validated_css_file_path( $href );
if ( is_wp_error( $css_file_path ) ) {
$element->parentNode->removeChild( $element ); // @todo Report removal. Show HTML comment?
$this->remove_invalid_child( $element );
return;
}

Expand Down
7 changes: 2 additions & 5 deletions includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,10 +283,7 @@ public function sanitize() {
private function process_node( $node ) {

// Don't process text or comment nodes.
if ( XML_COMMENT_NODE === $node->nodeType ) {
$this->capture_current_source( $node );
return;
} elseif ( XML_TEXT_NODE === $node->nodeType || XML_CDATA_SECTION_NODE === $node->nodeType ) {
if ( XML_TEXT_NODE === $node->nodeType || XML_COMMENT_NODE === $node->nodeType || XML_CDATA_SECTION_NODE === $node->nodeType ) {
return;
}

Expand Down Expand Up @@ -1562,7 +1559,7 @@ private function remove_node( $node ) {
$node = $parent;
$parent = $parent->parentNode;
if ( $parent ) {
$this->remove_invalid_child( $node );
$parent->removeChild( $node );
}
}
}
Expand Down
Loading

0 comments on commit 76e2be5

Please sign in to comment.