Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent sanitization of Customizer preview styles #4977

Merged
merged 5 commits into from
Jul 4, 2020
Merged

Prevent sanitization of Customizer preview styles #4977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8598133
Prevent sanitization of Customizer preview styles
pierlon Jul 2, 2020
927ed5f
Ensure all dependencies of the `customizer-preview` style handle are …
pierlon Jul 3, 2020
8084a3f
Remove unused sanitizer args
pierlon Jul 3, 2020
57b9c9e
Reduce indentation
westonruter Jul 4, 2020
9dd7976
Remove last references to allow_dirty_styles and allow_dirty_scripts
westonruter Jul 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1460,11 +1460,19 @@ function amp_get_content_sanitizers( $post = null ) {
* @param string[] $element_xpaths XPath element queries. Context is the root element.
*/
$dev_mode_xpaths = (array) apply_filters( 'amp_dev_mode_element_xpaths', [] );

if ( is_admin_bar_showing() ) {
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]';
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]//*';
$dev_mode_xpaths[] = '//style[ @id = "admin-bar-inline-css" ]';
}

if ( is_customize_preview() ) {
// Scripts are always needed to inject changeset UUID.
$dev_mode_xpaths[] = '//script[ @src ]';
$dev_mode_xpaths[] = '//script[ not( @type ) or @type = "text/javascript" ]';
}

$sanitizers = array_merge(
[
'AMP_Dev_Mode_Sanitizer' => [
Expand Down
30 changes: 25 additions & 5 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,6 +1072,9 @@ static function( $html ) {
add_action( 'wp_enqueue_scripts', [ __CLASS__, 'enqueue_assets' ], 0 ); // Enqueue before theme's styles.
add_action( 'wp_enqueue_scripts', [ __CLASS__, 'dequeue_customize_preview_scripts' ], 1000 );
add_filter( 'customize_partial_render', [ __CLASS__, 'filter_customize_partial_render' ] );
if ( is_customize_preview() ) {
add_filter( 'style_loader_tag', [ __CLASS__, 'filter_customize_preview_style_loader_tag' ], 10, 2 );
}

add_action( 'wp_footer', 'amp_print_analytics' );

Expand Down Expand Up @@ -1560,6 +1563,28 @@ public static function filter_admin_bar_style_loader_tag( $tag, $handle ) {
return $tag;
}

/**
* Add data-ampdevmode attribute to any enqueued style that depends on the `customizer-preview` handle.
*
* @since 1.6
*
* @param string $tag The link tag for the enqueued style.
* @param string $handle The style's registered handle.
* @return string Tag.
*/
public static function filter_customize_preview_style_loader_tag( $tag, $handle ) {
$customize_preview = 'customize-preview';
if (
is_array( wp_styles()->registered[ $customize_preview ]->deps ) && in_array( $handle, wp_styles()->registered[ $customize_preview ]->deps, true )
? self::is_exclusively_dependent( wp_styles(), $handle, $customize_preview )
: self::has_dependency( wp_styles(), $handle, $customize_preview )
) {
$tag = preg_replace( '/(?<=<link)(?=\s|>)/i', ' ' . AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, $tag );
}

return $tag;
}

/**
* Add data-ampdevmode attribute to any enqueued script that depends on the admin-bar.
*
Expand Down Expand Up @@ -1895,8 +1920,6 @@ public static function filter_customize_partial_render( $partial ) {
$args = [
'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat.
'use_document_element' => false,
'allow_dirty_styles' => true,
'allow_dirty_scripts' => false,
];
AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args ); // @todo Include script assets in response?
$partial = AMP_DOM_Utils::get_content_from_dom( $dom );
Expand Down Expand Up @@ -1966,13 +1989,10 @@ public static function prepare_response( $response, $args = [] ) {
header( 'Content-Type: text/html; charset=utf-8' );
}

// @todo Both allow_dirty_styles and allow_dirty_scripts should eventually use AMP dev mode instead.
$args = array_merge(
[
'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 shortcuts).
'allow_dirty_scripts' => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
'user_can_validate' => AMP_Validation_Manager::has_cap(),
],
$args
Expand Down
2 changes: 0 additions & 2 deletions includes/sanitizers/class-amp-base-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ abstract class AMP_Base_Sanitizer {
* @type array $amp_globally_allowed_attributes
* @type array $amp_layout_allowed_attributes
* @type array $amp_bind_placeholder_prefix
* @type bool $allow_dirty_styles
* @type bool $allow_dirty_scripts
* @type bool $should_locate_sources
* @type callable $validation_error_callback
* }
Expand Down
11 changes: 2 additions & 9 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
* @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered.
* @type bool $use_document_element Whether the root of the document should be used rather than the body.
* @type bool $require_https_src Require HTTPS URLs.
* @type bool $allow_dirty_styles Allow dirty styles. This short-circuits the sanitize logic; it is used primarily in Customizer preview.
* @type callable $validation_error_callback Function to call when a validation error is encountered.
* @type bool $should_locate_sources Whether to locate the sources when reporting validation errors.
* @type string $parsed_cache_variant Additional value by which to vary parsed cache.
Expand Down Expand Up @@ -844,12 +843,6 @@ public function init( $sanitizers ) {
public function sanitize() {
$elements = [];

// @todo Instead of short-circuiting, this actually needs to turn off tree-shaking.
// Do nothing if inline styles are allowed. Note, a better alternative to this is AMP dev mode.
if ( ! empty( $this->args['allow_dirty_styles'] ) ) {
return;
}

$this->focus_class_name_selector_pattern = (
! empty( $this->args['focus_within_classes'] ) ?
self::get_class_name_selector_pattern( $this->args['focus_within_classes'] ) :
Expand Down Expand Up @@ -2759,9 +2752,9 @@ private function finalize_styles() {
$stylesheet_groups[ $group ]['included_count'] = $this->finalize_stylesheet_group( $group, $stylesheet_groups[ $group ] );
}

// If we're not working with the document element (e.g. for legacy post templates) then there is nothing left to do.
// If we're not working with the document element (e.g. for Customizer rendered partials) then there is nothing left to do.
if ( empty( $this->args['use_document_element'] ) ) {
return; // @todo This would no longer be true with <https://github.com/ampproject/amp-wp/issues/2202>.
return;
}

// Add style[amp-custom] to document.
Expand Down
66 changes: 0 additions & 66 deletions includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,72 +180,6 @@ public function __construct( $dom, $args = [] ) {

parent::__construct( $dom, $args );

// @todo AMP dev mode should eventually be used instead of allow_dirty_styles.
if ( ! empty( $this->args['allow_dirty_styles'] ) ) {
pierlon marked this conversation as resolved.
Show resolved Hide resolved

// Allow style attribute on all elements.
$this->args['amp_globally_allowed_attributes']['style'] = [];

// Remove restrictions on use of !important.
foreach ( $this->args['amp_allowed_tags']['style'] as &$style ) {
$style['cdata'] = [];
}

// Allow style elements.
$this->args['amp_allowed_tags']['style'][] = [
'attr_spec_list' => [
'type' => [
'value_casei' => 'text/css',
],
],
'cdata' => [],
'tag_spec' => [
'spec_name' => 'style for Customizer preview',
],
];

// Allow stylesheet links.
$this->args['amp_allowed_tags']['link'][] = [
'attr_spec_list' => [
'async' => [],
'crossorigin' => [],
'href' => [
'mandatory' => true,
],
'integrity' => [],
'media' => [],
'rel' => [
'dispatch_key' => 2,
'mandatory' => true,
'value_casei' => 'stylesheet',
],
'type' => [
'value_casei' => 'text/css',
],
],
'tag_spec' => [
'spec_name' => 'link rel=stylesheet for Customizer preview', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
],
];
}

// @todo AMP dev mode should eventually be used instead of allow_dirty_scripts.
// Allow scripts if requested.
if ( ! empty( $this->args['allow_dirty_scripts'] ) ) {
$this->args['amp_allowed_tags']['script'][] = [
'attr_spec_list' => [
'type' => [],
'src' => [],
'async' => [],
'defer' => [],
],
'cdata' => [],
'tag_spec' => [
'spec_name' => 'scripts for Customizer preview',
],
];
}

// Prepare allowlists.
$this->allowed_tags = $this->args['amp_allowed_tags'];
foreach ( AMP_Rule_Spec::$additional_allowed_tags as $tag_name => $tag_rule_spec ) {
Expand Down