Skip to content

Commit

Permalink
Merge pull request #1181 from Automattic/update/1091-fetch_import_rules
Browse files Browse the repository at this point in the history 
Fetch (local) stylesheets with @import instead of removing
  • Loading branch information
@westonruter
westonruter authored May 30, 2018
2 parents 888ac10 + ffe3c09 commit 6ec7e1b
Show file tree
Hide file tree
Showing 5 changed files with 205 additions and 18 deletions.
2 changes: 1 addition & 1 deletion contributing.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ TO create a build of the plugin as it will be deployed to WordPress.org, run:
npm run build-release
```

Note that this will currently take much longer than a regular build because it generates the files required for translation. You also must have WP-CLI installed with the `i18n` package.
Note that this will currently take much longer than a regular build because it generates the files required for translation. You also must have WP-CLI installed with the [`i18n-command` package](https://github.com/wp-cli/i18n-command).

## Updating Allowed Tags And Attributes

Expand Down
23 changes: 19 additions & 4 deletions includes/sanitizers/class-amp-base-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ abstract class AMP_Base_Sanitizer {
*/
protected $root_element;

/**
* Keep track of nodes that should not be removed to prevent duplicated validation errors since sanitization is rejected.
*
* @var array
*/
private $should_not_removed_nodes = array();

/**
* AMP_Base_Sanitizer constructor.
*
Expand Down Expand Up @@ -295,9 +302,17 @@ public function maybe_enforce_https_src( $src, $force_https = false ) {
* @return bool Whether the node should have been removed, that is, that the node was sanitized for validity.
*/
public function remove_invalid_child( $node, $validation_error = array() ) {

// Prevent double-reporting nodes that are rejected for sanitization.
if ( isset( $this->should_not_removed_nodes[ $node->nodeName ] ) && in_array( $node, $this->should_not_removed_nodes[ $node->nodeName ], true ) ) {
return false;
}

$should_remove = $this->should_sanitize_validation_error( $validation_error, compact( 'node' ) );
if ( $should_remove ) {
$node->parentNode->removeChild( $node );
} else {
$this->should_not_removed_nodes[ $node->nodeName ][] = $node;
}
return $should_remove;
}
Expand Down Expand Up @@ -404,7 +419,7 @@ public function prepare_validation_error( array $error = array(), array $data =
/**
* Get data-amp-* values from the parent node 'figure' added by editor block.
*
* @param DOMNode $node Base node.
* @param DOMElement $node Base node.
* @return array AMP data array.
*/
public function get_data_amp_attributes( $node ) {
Expand Down Expand Up @@ -445,9 +460,9 @@ public function filter_data_amp_attributes( $attributes, $amp_data ) {
/**
* Set attributes to node's parent element according to layout.
*
* @param DOMNode $node Node.
* @param array $new_attributes Attributes array.
* @param string $layout Layout.
* @param DOMElement $node Node.
* @param array $new_attributes Attributes array.
* @param string $layout Layout.
* @return array New attributes.
*/
public function filter_attachment_layout_attributes( $node, $new_attributes, $layout ) {
Expand Down
119 changes: 109 additions & 10 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,13 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
*/
private $calc_placeholders = array();

/**
* Log of the stylesheet URLs that have been imported to guard against infinite loops.
*
* @var array
*/
private $processed_imported_stylesheet_urls = array();

/**
* Get error codes that can be raised during parsing of CSS.
*
Expand All @@ -201,6 +208,9 @@ public static function get_css_parser_validation_error_codes() {
'illegal_css_property',
'removed_unused_css_rules',
'unrecognized_css',
'disallowed_external_file_url',
'disallowed_file_extension',
'file_path_not_found',
);
}

Expand Down Expand Up @@ -600,8 +610,8 @@ private function process_link_element( DOMElement $element ) {
*
* @type string[] $property_whitelist Exclusively-allowed properties.
* @type string[] $property_blacklist Disallowed properties.
* @type string $stylesheet_url Original URL for stylesheet when originating via link (or @import?).
* @type string $stylesheet_path Original filesystem path for stylesheet when originating via link (or @import?).
* @type string $stylesheet_url Original URL for stylesheet when originating via link or @import.
* @type string $stylesheet_path Original filesystem path for stylesheet when originating via link or @import.
* @type array $allowed_at_rules Allowed @-rules.
* @type bool $validate_keyframes Whether keyframes should be validated.
* }
Expand All @@ -610,7 +620,7 @@ private function process_link_element( DOMElement $element ) {
private function process_stylesheet( $stylesheet, $options = array() ) {
$parsed = null;
$cache_key = null;
$cache_group = 'amp-parsed-stylesheet-v5';
$cache_group = 'amp-parsed-stylesheet-v6';

$cache_impacting_options = array_merge(
wp_array_slice_assoc(
Expand Down Expand Up @@ -659,6 +669,82 @@ private function process_stylesheet( $stylesheet, $options = array() ) {
return $parsed['stylesheet'];
}

/**
* Parse imported stylesheet.
*
* @param Import $item Import object.
* @param CSSList $css_list CSS List.
* @param array $options {
* Options.
*
* @type string $stylesheet_url Original URL for stylesheet when originating via link or @import.
* }
* @return array Validation results.
*/
private function parse_import_stylesheet( Import $item, CSSList $css_list, $options ) {
$results = array();
$at_rule_args = $item->atRuleArgs();
$location = array_shift( $at_rule_args );
$media_query = array_shift( $at_rule_args );

if ( isset( $options['stylesheet_url'] ) ) {
$this->real_path_urls( array( $location ), $options['stylesheet_url'] );
}

$import_stylesheet_url = $location->getURL()->getString();

// Prevent importing something that has already been imported, and avoid infinite recursion.
if ( isset( $this->processed_imported_stylesheet_urls[ $import_stylesheet_url ] ) ) {
$css_list->remove( $item );
return array();
}
$this->processed_imported_stylesheet_urls[ $import_stylesheet_url ] = true;

// @todo Handle external per <https://github.com/Automattic/amp-wp/issues/1083>.
$css_file_path = $this->get_validated_url_file_path( $import_stylesheet_url, array( 'css', 'less', 'scss', 'sass' ) );

if ( is_wp_error( $css_file_path ) ) {
$error = array(
'code' => $css_file_path->get_error_code(),
'message' => $css_file_path->get_error_message(),
);
$sanitized = $this->should_sanitize_validation_error( $error );
if ( $sanitized ) {
$css_list->remove( $item );
}
$results[] = compact( 'error', 'sanitized' );
return $results;
}

// Load the CSS from the filesystem.
$stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore -- It's a local filesystem path not a remote request.

if ( $media_query ) {
$stylesheet = sprintf( '@media %s { %s }', $media_query, $stylesheet );
}

$stylesheet = $this->process_stylesheet( $stylesheet, array(
'allowed_at_rules' => $this->style_custom_cdata_spec['css_spec']['allowed_at_rules'],
'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['allowed_declarations'],
'stylesheet_url' => $import_stylesheet_url,
'stylesheet_path' => $css_file_path,
) );

$pending_stylesheet = array(
'keyframes' => false,
'stylesheet' => $stylesheet,
'node' => $this->current_node,
'sources' => $this->current_sources,
'imported' => $import_stylesheet_url,
);

// Stylesheet will now be inlined, so @import can be removed.
$css_list->remove( $item );

$this->pending_stylesheets[] = $pending_stylesheet;
return $results;
}

/**
* Parse stylesheet.
*
Expand Down Expand Up @@ -971,12 +1057,10 @@ private function process_css_list( CSSList $css_list, $options ) {
);
}
} elseif ( $css_item instanceof Import ) {
$error = array(
'code' => 'illegal_css_at_rule',
'at_rule' => 'import',
$results = array_merge(
$results,
$this->parse_import_stylesheet( $css_item, $css_list, $options )
);
$sanitized = $this->should_sanitize_validation_error( $error );
$results[] = compact( 'error', 'sanitized' );
} elseif ( $css_item instanceof AtRuleSet ) {
if ( ! in_array( $css_item->atRuleName(), $options['allowed_at_rules'], true ) ) {
$error = array(
Expand Down Expand Up @@ -1468,21 +1552,32 @@ private function finalize_styles() {
),
);

/*
* On Native AMP themes when there are new/rejected validation errors present, a parsed stylesheet may include
* @import rules. These must be moved to the beginning to be honored.
*/
$imports = array();

// Divide pending stylesheet between custom and keyframes, and calculate size of each.
while ( ! empty( $this->pending_stylesheets ) ) {
$pending_stylesheet = array_shift( $this->pending_stylesheets );

$set_name = ! empty( $pending_stylesheet['keyframes'] ) ? 'keyframes' : 'custom';
$size = 0;
foreach ( $pending_stylesheet['stylesheet'] as $part ) {
foreach ( $pending_stylesheet['stylesheet'] as $i => $part ) {
if ( is_string( $part ) ) {
$size += strlen( $part );
if ( '@import' === substr( $part, 0, 7 ) ) {
$imports[] = $part;
unset( $pending_stylesheet['stylesheet'][ $i ] );
}
} elseif ( is_array( $part ) ) {
$size += strlen( implode( ',', array_keys( $part[0] ) ) ); // Selectors.
$size += strlen( $part[1] ); // Declaration block.
}
}
$stylesheet_sets[ $set_name ]['total_size'] += $size;
$stylesheet_sets[ $set_name ]['imports'] = $imports;
$stylesheet_sets[ $set_name ]['pending_stylesheets'][] = $pending_stylesheet;
}

Expand Down Expand Up @@ -1513,7 +1608,8 @@ private function finalize_styles() {
$head->appendChild( $this->amp_custom_style_element );
}

$css = implode( '', $stylesheet_sets['custom']['final_stylesheets'] );
$css = implode( '', $stylesheet_sets['custom']['imports'] ); // For native dirty AMP.
$css .= implode( '', $stylesheet_sets['custom']['final_stylesheets'] );

/*
* Let the style[amp-custom] be populated with the concatenated CSS.
Expand Down Expand Up @@ -1544,6 +1640,9 @@ private function finalize_styles() {
$message .= sprintf( '[%s=%s]', $attribute->nodeName, $attribute->nodeValue );
}
}
if ( ! empty( $pending_stylesheet['imported'] ) ) {
$message .= sprintf( ' @import url(%s)', $pending_stylesheet['imported'] );
}
$message .= sprintf(
/* translators: %d is number of bytes */
_n( ' (%d byte)', ' (%d bytes)', $pending_stylesheet['size'], 'amp' ),
Expand Down
17 changes: 14 additions & 3 deletions includes/validation/class-amp-invalid-url-post-type.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,12 @@ public static function handle_validation_error_status_update() {
return;
}
$updated_count = 0;

$has_pre_term_description_filter = has_filter( 'pre_term_description', 'wp_filter_kses' );
if ( false !== $has_pre_term_description_filter ) {
remove_filter( 'pre_term_description', 'wp_filter_kses', $has_pre_term_description_filter );
}

foreach ( $_POST[ AMP_Validation_Manager::VALIDATION_ERROR_TERM_STATUS_QUERY_VAR ] as $term_slug => $status ) {
$term_slug = sanitize_key( $term_slug );
$term = get_term_by( 'slug', $term_slug, AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG );
Expand All @@ -805,6 +811,11 @@ public static function handle_validation_error_status_update() {
wp_update_term( $term->term_id, AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG, compact( 'term_group' ) );
}
}

if ( false !== $has_pre_term_description_filter ) {
add_filter( 'pre_term_description', 'wp_filter_kses', $has_pre_term_description_filter );
}

$args = array(
'amp_taxonomy_terms_updated' => $updated_count,
);
Expand Down Expand Up @@ -943,14 +954,14 @@ public static function print_validation_errors_meta_box( $post ) {
validateUrl = <?php echo wp_json_encode( add_query_arg( AMP_Validation_Manager::VALIDATE_QUERY_VAR, AMP_Validation_Manager::get_amp_validate_nonce(), $post->post_title ) ); ?>;
postId = <?php echo wp_json_encode( $post->ID ); ?>;
$( '#preview_validation_errors' ).on( 'click', function() {
var params = {};
var params = {}, validatePreviewUrl = validateUrl;
$( '.amp-validation-error-status' ).each( function() {
if ( this.value && ! this.options[ this.selectedIndex ].defaultSelected ) {
params[ this.name ] = this.value;
}
} );
validateUrl += '&' + $.param( params );
window.open( validateUrl, 'amp-validation-error-term-status-preview-' + String( postId ) );
validatePreviewUrl += '&' + $.param( params );
window.open( validatePreviewUrl, 'amp-validation-error-term-status-preview-' + String( postId ) );
} );
} );
</script>
Expand Down
62 changes: 62 additions & 0 deletions tests/test-amp-style-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -801,4 +801,66 @@ public function test_font_urls( $url, $error_codes ) {
$this->assertEmpty( $link );
}
}

/**
* Get data for CSS @import test.
*
* @return array
*/
public function get_css_import_data() {
$css_url = admin_url( 'css/login.css' );
$html = sprintf( '<html><head><link rel="stylesheet" href="%s"></head><style>@import url("https://something.example.com/style.css"); body { color:red; }</style></html>', $css_url ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
$stylesheet_patterns = array(
'/' . preg_quote( 'input[type="checkbox"]:disabled' ) . '/',
'/' . preg_quote( 'body.rtl' ) . '/',
'/' . preg_quote( '.login .message' ) . '/',
'/^' . preg_quote( 'body{color:red}' ) . '$/',
);
return array(
'sanitized' => array(
$html,
$stylesheet_patterns,
'/^(?!@import)/',
true,
),
'unsanitized' => array(
$html,
$stylesheet_patterns,
'/^@import/',
false,
),
);
}

/**
* Test CSS imports.
*
* @dataProvider get_css_import_data
* @covers AMP_Style_Sanitizer::parse_import_stylesheet()
*
* @param string $markup Markup.
* @param array $stylesheet_patterns Stylesheet patterns.
* @param string $style_pattern Style pattern for resulting style element.
* @param bool $sanitized Whether validation errors should be sanitized.
*/
public function test_css_import( $markup, $stylesheet_patterns, $style_pattern, $sanitized ) {
$dom = AMP_DOM_Utils::get_dom( $markup );
$sanitizer = new AMP_Style_Sanitizer( $dom, array(
'use_document_element' => true,
'remove_unused_rules' => 'never',
'validation_error_callback' => function() use ( $sanitized ) {
return $sanitized;
},
) );
$sanitizer->sanitize();
$stylesheets = array_values( $sanitizer->get_stylesheets() );
$this->assertCount( count( $stylesheet_patterns ), $stylesheets );
do {
$this->assertRegExp( array_shift( $stylesheet_patterns ), array_shift( $stylesheets ) );
} while ( ! empty( $stylesheet_patterns ) );

$dom_xpath = new DOMXPath( $dom );
$amp_custom = $dom_xpath->query( '//style[@amp-custom]' )->item( 0 );
$this->assertRegExp( $style_pattern, $amp_custom->textContent );
}
}

0 comments on commit 6ec7e1b

Please sign in to comment.