diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 01207b645ca..2ab0127ced7 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -604,7 +604,7 @@ public static function amend_comment_form() { * @see get_query_template() * * @param array $templates Template hierarchy. - * @returns array Templates. + * @return array Templates. */ public static function filter_paired_template_hierarchy( $templates ) { $support = get_theme_support( 'amp' ); @@ -991,15 +991,12 @@ public static function prepare_response( $response, $args = array() ) { return $response; } - $is_validation_debug_mode = ! empty( $_REQUEST[ AMP_Validation_Utils::DEBUG_QUERY_VAR ] ); // WPCS: csrf ok. - $args = array_merge( array( - 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat. - 'use_document_element' => true, - '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' => $is_validation_debug_mode, + 'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat. + 'use_document_element' => true, + '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. ), $args ); @@ -1051,9 +1048,7 @@ public static function prepare_response( $response, $args = array() ) { } if ( AMP_Validation_Utils::should_validate_response() ) { - AMP_Validation_Utils::finalize_validation( $dom, array( - 'remove_source_comments' => ! $is_validation_debug_mode, - ) ); + AMP_Validation_Utils::finalize_validation( $dom ); } $response = "\n"; diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php index 1c2cb183be6..d9c9e5cae47 100644 --- a/includes/sanitizers/class-amp-base-sanitizer.php +++ b/includes/sanitizers/class-amp-base-sanitizer.php @@ -54,7 +54,6 @@ 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 $validation_error_callback * } */ @@ -290,18 +289,22 @@ public function maybe_enforce_https_src( $src, $force_https = false ) { * * @param DOMNode|DOMElement $node The node to remove. * @param array $args Additional args to pass to validation error callback. - * - * @return void + * @return bool Whether the node should have been removed, that is, that the node was sanitized for validity. */ public function remove_invalid_child( $node, $args = array() ) { + $should_remove = true; if ( isset( $this->args['validation_error_callback'] ) ) { - call_user_func( $this->args['validation_error_callback'], + $result = call_user_func( $this->args['validation_error_callback'], array_merge( compact( 'node' ), $args ) ); + if ( is_bool( $result ) ) { + $should_remove = $result; + } } - if ( empty( $this->args['disable_invalid_removal'] ) ) { + if ( $should_remove ) { $node->parentNode->removeChild( $node ); } + return $should_remove; } /** @@ -315,15 +318,16 @@ public function remove_invalid_child( $node, $args = array() ) { * @param DOMElement $element The node for which to remove the attribute. * @param DOMAttr|string $attribute The attribute to remove from the element. * @param array $args Additional args to pass to validation error callback. - * @return void + * @return bool Whether the node should have been removed, that is, that the node was sanitized for validity. */ public function remove_invalid_attribute( $element, $attribute, $args = array() ) { + $should_remove = true; if ( isset( $this->args['validation_error_callback'] ) ) { if ( is_string( $attribute ) ) { $attribute = $element->getAttributeNode( $attribute ); } if ( $attribute ) { - call_user_func( $this->args['validation_error_callback'], + $result = call_user_func( $this->args['validation_error_callback'], array_merge( array( 'node' => $attribute, @@ -331,16 +335,20 @@ public function remove_invalid_attribute( $element, $attribute, $args = array() $args ) ); - if ( empty( $this->args['disable_invalid_removal'] ) ) { + if ( is_bool( $result ) ) { + $should_remove = $result; + } + if ( $should_remove ) { $element->removeAttributeNode( $attribute ); } } - } elseif ( empty( $this->args['disable_invalid_removal'] ) ) { + } else { if ( is_string( $attribute ) ) { $element->removeAttribute( $attribute ); } else { $element->removeAttributeNode( $attribute ); } } + return $should_remove; } } diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 8440c097369..033c3180789 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -367,7 +367,7 @@ private function process_style_element( DOMElement $element ) { 'stylesheet' => $stylesheet, 'node' => $element, ); - if ( ! empty( $this->args['validation_error_callback'] ) ) { + if ( ! empty( $this->args['validation_error_callback'] ) ) { // @todo This needs to be capture_sources. $pending_stylesheet['sources'] = AMP_Validation_Utils::locate_sources( $element ); // Needed because node is removed below. } $this->pending_stylesheets[] = $pending_stylesheet; @@ -402,6 +402,7 @@ private function process_link_element( DOMElement $element ) { $css_file_path = $this->get_validated_url_file_path( $href, array( 'css', 'less', 'scss', 'sass' ) ); if ( is_wp_error( $css_file_path ) ) { $this->remove_invalid_child( $element, array( + 'code' => $css_file_path->get_error_code(), 'message' => $css_file_path->get_error_message(), ) ); return; @@ -411,6 +412,7 @@ private function process_link_element( DOMElement $element ) { $stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request. if ( false === $stylesheet ) { $this->remove_invalid_child( $element, array( + 'code' => 'stylesheet_file_missing', 'message' => __( 'Unable to load stylesheet from filesystem.', 'amp' ), ) ); return; @@ -434,7 +436,7 @@ private function process_link_element( DOMElement $element ) { 'stylesheet' => $stylesheet, 'node' => $element, ); - if ( ! empty( $this->args['validation_error_callback'] ) ) { + if ( ! empty( $this->args['validation_error_callback'] ) ) { // @todo This needs to be capture_sources. $pending_stylesheet['sources'] = AMP_Validation_Utils::locate_sources( $element ); // Needed because node is removed below. } $this->pending_stylesheets[] = $pending_stylesheet; @@ -470,6 +472,7 @@ private function process_link_element( DOMElement $element ) { private function process_stylesheet( $stylesheet, $node, $options = array() ) { $cache_impacting_options = wp_array_slice_assoc( $options, + // @todo Needs to be a debug flag here because stored validation_errors will vary based on whether sources are being captured. array( 'property_whitelist', 'property_blacklist', 'convert_width_to_max_width', 'stylesheet_url', 'allowed_at_rules' ) ); @@ -489,9 +492,9 @@ private function process_stylesheet( $stylesheet, $node, $options = array() ) { // The expiration is to ensure transient doesn't stick around forever since no LRU flushing like with external object cache. set_transient( $cache_key . $cache_group, $parsed, MONTH_IN_SECONDS ); } - } + } elseif ( ! empty( $this->args['validation_error_callback'] ) && ! empty( $parsed['validation_errors'] ) ) { - if ( ! empty( $this->args['validation_error_callback'] ) && ! empty( $parsed['validation_errors'] ) ) { + // Report cached validation errors. foreach ( $parsed['validation_errors'] as $validation_error ) { call_user_func( $this->args['validation_error_callback'], array_merge( $validation_error, compact( 'node' ) ) ); } @@ -609,10 +612,14 @@ function ( $value ) { } } } catch ( Exception $exception ) { - $validation_errors[] = array( + $validation_error = array( 'code' => 'css_parse_error', 'message' => $exception->getMessage(), ); + if ( ! empty( $this->args['validation_error_callback'] ) ) { + call_user_func( $this->args['validation_error_callback'], $validation_error ); + } + $validation_error[] = $validation_error; } $this->parse_css_duration += ( microtime( true ) - $start_time ); @@ -620,6 +627,21 @@ function ( $value ) { return compact( 'stylesheet', 'validation_errors' ); } + /** + * Check whether or not sanitization should occur in response to validation error. + * + * @since 1.0 + * + * @param array $validation_error Validation error. + * @return bool Whether to sanitize. + */ + private function should_sanitize( $validation_error ) { + if ( empty( $this->args['validation_error_callback'] ) ) { + return true; + } + return false !== call_user_func( $this->args['validation_error_callback'], $validation_error ); + } + /** * Process CSS list. * @@ -633,70 +655,87 @@ private function process_css_list( CSSList $css_list, $options ) { $validation_errors = array(); foreach ( $css_list->getContents() as $css_item ) { + $should_remove_item = false; if ( $css_item instanceof DeclarationBlock && empty( $options['validate_keyframes'] ) ) { $validation_errors = array_merge( $validation_errors, $this->process_css_declaration_block( $css_item, $css_list, $options ) ); } elseif ( $css_item instanceof AtRuleBlockList ) { - if ( in_array( $css_item->atRuleName(), $options['allowed_at_rules'], true ) ) { - $validation_errors = array_merge( - $validation_errors, - $this->process_css_list( $css_item, $options ) - ); - } else { - $validation_errors[] = array( + if ( ! in_array( $css_item->atRuleName(), $options['allowed_at_rules'], true ) ) { + $validation_error = array( 'code' => 'illegal_css_at_rule', /* translators: %s is the CSS at-rule name. */ 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), ); - $css_list->remove( $css_item ); + $validation_errors[] = $validation_error; + $should_remove_item = $this->should_sanitize( $validation_error ); + } + if ( ! $should_remove_item ) { + $validation_errors = array_merge( + $validation_errors, + $this->process_css_list( $css_item, $options ) + ); } } elseif ( $css_item instanceof Import ) { - $validation_errors[] = array( + $validation_error = array( 'code' => 'illegal_css_import_rule', 'message' => __( 'CSS @import is currently disallowed.', 'amp' ), ); - $css_list->remove( $css_item ); + $validation_errors[] = $validation_error; + $should_remove_item = $this->should_sanitize( $validation_error ); } elseif ( $css_item instanceof AtRuleSet ) { - if ( in_array( $css_item->atRuleName(), $options['allowed_at_rules'], true ) ) { + if ( ! in_array( $css_item->atRuleName(), $options['allowed_at_rules'], true ) ) { + $validation_error = array( + 'code' => 'illegal_css_at_rule', + /* translators: %s is the CSS at-rule name. */ + 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), + ); + $validation_errors[] = $validation_errors; + $should_remove_item = $this->should_sanitize( $validation_error ); + } + + if ( ! $should_remove_item ) { $validation_errors = array_merge( $validation_errors, $this->process_css_declaration_block( $css_item, $css_list, $options ) ); - } else { - $validation_errors[] = array( + } + } elseif ( $css_item instanceof KeyFrame ) { + if ( ! in_array( 'keyframes', $options['allowed_at_rules'], true ) ) { + $validation_error = array( 'code' => 'illegal_css_at_rule', /* translators: %s is the CSS at-rule name. */ 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), ); - $css_list->remove( $css_item ); + $validation_errors[] = $validation_errors; + $should_remove_item = $this->should_sanitize( $validation_error ); } - } elseif ( $css_item instanceof KeyFrame ) { - if ( in_array( 'keyframes', $options['allowed_at_rules'], true ) ) { + + if ( ! $should_remove_item ) { $validation_errors = array_merge( $validation_errors, $this->process_css_keyframes( $css_item, $options ) ); - } else { - $validation_errors[] = array( - 'code' => 'illegal_css_at_rule', - /* translators: %s is the CSS at-rule name. */ - 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), - ); } } elseif ( $css_item instanceof AtRule ) { - $validation_errors[] = array( + $validation_error = array( 'code' => 'illegal_css_at_rule', /* translators: %s is the CSS at-rule name. */ 'message' => sprintf( __( 'CSS @%s rules are currently disallowed.', 'amp' ), $css_item->atRuleName() ), ); - $css_list->remove( $css_item ); + $validation_errors[] = $validation_errors; + $should_remove_item = $this->should_sanitize( $validation_error ); } else { - $validation_errors[] = array( + $validation_error = array( 'code' => 'unrecognized_css', 'message' => __( 'Unrecognized CSS removed.', 'amp' ), ); + $validation_errors[] = $validation_errors; + $should_remove_item = $this->should_sanitize( $validation_error ); + } + + if ( $should_remove_item ) { $css_list->remove( $css_item ); } } @@ -758,24 +797,30 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l foreach ( $properties as $property ) { $vendorless_property_name = preg_replace( '/^-\w+-/', '', $property->getRule() ); if ( ! in_array( $vendorless_property_name, $options['property_whitelist'], true ) ) { - $validation_errors[] = array( + $validation_error = array( 'code' => 'illegal_css_property', 'property_name' => $property->getRule(), 'property_value' => $property->getValue(), ); - $ruleset->removeRule( $property->getRule() ); + if ( $this->should_sanitize( $validation_error ) ) { + $ruleset->removeRule( $property->getRule() ); + } + $validation_errors[] = $validation_error; } } } else { foreach ( $options['property_blacklist'] as $illegal_property_name ) { $properties = $ruleset->getRules( $illegal_property_name ); foreach ( $properties as $property ) { - $validation_errors[] = array( + $validation_error = array( 'code' => 'illegal_css_property', 'property_name' => $property->getRule(), 'property_value' => $property->getValue(), ); - $ruleset->removeRule( $property->getRule() ); + if ( $this->should_sanitize( $validation_error ) ) { + $ruleset->removeRule( $property->getRule() ); + } + $validation_errors[] = $validation_error; } } } @@ -937,11 +982,14 @@ private function process_css_keyframes( KeyFrame $css_list, $options ) { if ( ! empty( $options['property_whitelist'] ) ) { foreach ( $css_list->getContents() as $rules ) { if ( ! ( $rules instanceof DeclarationBlock ) ) { - $validation_errors[] = array( + $validation_error = array( 'code' => 'unrecognized_css', 'message' => __( 'Unrecognized CSS removed.', 'amp' ), ); - $css_list->remove( $rules ); + if ( $this->should_sanitize( $validation_error ) ) { + $css_list->remove( $rules ); + } + $validation_errors[] = $validation_error; continue; } @@ -954,12 +1002,15 @@ private function process_css_keyframes( KeyFrame $css_list, $options ) { foreach ( $properties as $property ) { $vendorless_property_name = preg_replace( '/^-\w+-/', '', $property->getRule() ); if ( ! in_array( $vendorless_property_name, $options['property_whitelist'], true ) ) { - $validation_errors[] = array( + $validation_error = array( 'code' => 'illegal_css_property', 'property_name' => $property->getRule(), 'property_value' => $property->getValue(), ); - $rules->removeRule( $property->getRule() ); + if ( $this->should_sanitize( $validation_error ) ) { + $rules->removeRule( $property->getRule() ); + } + $validation_errors[] = $validation_error; } } } @@ -979,7 +1030,9 @@ private function process_css_keyframes( KeyFrame $css_list, $options ) { * @return array Validation errors. */ private function transform_important_qualifiers( RuleSet $ruleset, CSSList $css_list ) { - $validation_errors = array(); + $validation_errors = array(); + + // An !important only makes sense for rulesets that have selectors. $allow_transformation = ( $ruleset instanceof DeclarationBlock && @@ -990,17 +1043,19 @@ private function transform_important_qualifiers( RuleSet $ruleset, CSSList $css_ $importants = array(); foreach ( $properties as $property ) { if ( $property->getIsImportant() ) { - $property->setIsImportant( false ); - - // An !important doesn't make sense for rulesets that don't have selectors. if ( $allow_transformation ) { $importants[] = $property; + $property->setIsImportant( false ); $ruleset->removeRule( $property->getRule() ); } else { - $validation_errors[] = array( + $validation_error = array( 'code' => 'illegal_css_important', 'message' => __( 'Illegal CSS !important qualifier.', 'amp' ), ); + if ( $this->should_sanitize( $validation_error ) ) { + $property->setIsImportant( false ); + } + $validation_errors[] = $validation_error; } } } @@ -1087,7 +1142,7 @@ private function collect_inline_styles( $element ) { 'node' => $element, 'keyframes' => false, ); - if ( ! empty( $this->args['validation_error_callback'] ) ) { + if ( ! empty( $this->args['validation_error_callback'] ) ) { // @todo Needs to be capture_sources or something, like debug. $pending_stylesheet['sources'] = AMP_Validation_Utils::locate_sources( $element ); // Needed because node is removed below. } @@ -1281,25 +1336,25 @@ function( $selector ) { // Report validation error if size is now too big. $sheet_size = strlen( $stylesheet ); if ( $final_size + $sheet_size > $stylesheet_set['cdata_spec']['max_bytes'] ) { - if ( ! empty( $this->args['validation_error_callback'] ) ) { - $validation_error = array( - 'code' => 'excessive_css', - 'message' => sprintf( - /* translators: %d is the number of bytes over the limit */ - __( 'Too much CSS output (by %d bytes).', 'amp' ), - ( $final_size + $sheet_size ) - $stylesheet_set['cdata_spec']['max_bytes'] - ), - ); - if ( isset( $pending_stylesheet['sources'] ) ) { - $validation_error['sources'] = $pending_stylesheet['sources']; - } - call_user_func( $this->args['validation_error_callback'], $validation_error ); + $validation_error = array( + 'code' => 'excessive_css', + 'message' => sprintf( + /* translators: %d is the number of bytes over the limit */ + __( 'Too much CSS output (by %d bytes).', 'amp' ), + ( $final_size + $sheet_size ) - $stylesheet_set['cdata_spec']['max_bytes'] + ), + ); + if ( isset( $pending_stylesheet['sources'] ) ) { + $validation_error['sources'] = $pending_stylesheet['sources']; + } + if ( $this->should_sanitize( $validation_error ) ) { + continue; } - } else { - $final_size += $sheet_size; - - $stylesheet_set['final_stylesheets'][ $hash ] = $stylesheet; } + + $final_size += $sheet_size; + + $stylesheet_set['final_stylesheets'][ $hash ] = $stylesheet; } return $stylesheet_set; diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php index 9982f4f3839..cd62ff16f62 100644 --- a/includes/utils/class-amp-validation-utils.php +++ b/includes/utils/class-amp-validation-utils.php @@ -193,12 +193,23 @@ class AMP_Validation_Utils { */ protected static $current_hook_source_stack = array(); + /** + * Whether in debug mode. + * + * This means that sanitization will not be applied for validation errors. + * + * @var bool + */ + public static $debug = false; + /** * Add the actions. * * @return void */ public static function init() { + self::$debug = isset( $_REQUEST[ self::DEBUG_QUERY_VAR ] ); // WPCS: csrf ok. + if ( current_theme_supports( 'amp' ) ) { add_action( 'init', array( __CLASS__, 'register_post_type' ) ); add_filter( 'dashboard_glance_items', array( __CLASS__, 'filter_dashboard_glance_items' ) ); @@ -403,6 +414,7 @@ public static function has_cap() { * @type string $code Error code. * @type DOMElement|DOMNode $node The removed node. * } + * @return bool Whether the validation error should result in sanitization. */ public static function add_validation_error( array $data ) { $node = null; @@ -455,6 +467,8 @@ public static function add_validation_error( array $data ) { } self::$validation_errors[] = $data; + + return ! self::$debug; } /** @@ -1051,7 +1065,7 @@ public static function finalize_validation( DOMDocument $dom, $args = array() ) $args = array_merge( array( 'send_validation_errors_header' => true, - 'remove_source_comments' => true, + 'remove_source_comments' => ! self::$debug, 'append_validation_status_comment' => true, ), $args