Skip to content

Fetch CSS over HTTP when URL lacks extension; convert font CDN stylesheets @imports to convert to links instead of fetching #1357

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

Merged
westonruter merged 3 commits into from
Aug 27, 2018
Merged

Fetch CSS over HTTP when URL lacks extension; convert font CDN stylesheets @imports to convert to links instead of fetching #1357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2023e3
Prevent fetching imported font CDN stylesheets; convert to links
westonruter Aug 25, 2018
c68afcd
Fetch stylesheet over HTTP when URL lacks recognized file extension
westonruter Aug 25, 2018
0a082d9
Trigger _doing_it_wrong() if stylesheet is imported from font CDN ins…
westonruter Aug 25, 2018
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
120 changes: 94 additions & 26 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 @@ -205,6 +205,15 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
*/
private $processed_imported_stylesheet_urls = array();

/**
* List of font stylesheets that were @import'ed which should have been <link>'ed to instead.
*
* These font URLs will be cached with the parsed CSS and then converted into stylesheet links.
*
* @var array
*/
private $imported_font_urls = array();

/**
* Mapping of HTML element selectors to AMP selector elements.
*
Expand Down Expand Up @@ -465,7 +474,7 @@ public function sanitize() {
* @see WP_Styles::_css_href()
*
* @param string $url The file URL.
* @param string[] $allowed_extensions Allowed file extensions.
* @param string[] $allowed_extensions Allowed file extensions for local files.
* @return string|WP_Error Style's absolute validated filesystem path, or WP_Error when error.
*/
public function get_validated_url_file_path( $url, $allowed_extensions = array() ) {
Expand Down Expand Up @@ -504,7 +513,7 @@ public function get_validated_url_file_path( $url, $allowed_extensions = array()
$pattern = sprintf( '/\.(%s)$/i', implode( '|', $allowed_extensions ) );
if ( ! preg_match( $pattern, $url ) ) {
/* translators: %s: the file URL. */
return new WP_Error( 'disallowed_file_extension', sprintf( __( 'Skipped file which does not have an allowed file extension (%s).', 'amp' ), $url ) );
return new WP_Error( 'disallowed_file_extension', sprintf( __( 'File does not have an allowed file extension for filesystem access (%s).', 'amp' ), $url ) );
}
}

Expand Down Expand Up @@ -576,17 +585,19 @@ private function process_style_element( DOMElement $element ) {
$stylesheet = sprintf( '@media %s { %s }', $media, $stylesheet );
}

$stylesheet = $this->process_stylesheet( $stylesheet, array(
$processed = $this->process_stylesheet( $stylesheet, array(
'allowed_at_rules' => $cdata_spec['css_spec']['allowed_at_rules'],
'property_whitelist' => $cdata_spec['css_spec']['declaration'],
'validate_keyframes' => $cdata_spec['css_spec']['validate_keyframes'],
) );

$this->pending_stylesheets[] = array(
'keyframes' => $is_keyframes,
'stylesheet' => $stylesheet,
'node' => $element,
'sources' => $this->current_sources,
$this->pending_stylesheets[] = array_merge(
array(
'keyframes' => $is_keyframes,
'node' => $element,
'sources' => $this->current_sources,
),
wp_array_slice_assoc( $processed, array( 'stylesheet', 'imported_font_urls' ) )
);

if ( $element->hasAttribute( 'amp-custom' ) ) {
Expand Down Expand Up @@ -656,7 +667,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 ) && 'external_file_url' === $css_file_path->get_error_code() ) {
if ( is_wp_error( $css_file_path ) && ( 'disallowed_file_extension' === $css_file_path->get_error_code() || 'external_file_url' === $css_file_path->get_error_code() ) ) {
$contents = $this->fetch_external_stylesheet( $normalized_url );
if ( is_wp_error( $contents ) ) {
$this->remove_invalid_child( $element, array(
Expand Down Expand Up @@ -692,18 +703,20 @@ private function process_link_element( DOMElement $element ) {

$this->set_current_node( $element ); // And sources when needing to be located.

$stylesheet = $this->process_stylesheet( $stylesheet, array(
$processed = $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']['declaration'],
'stylesheet_url' => $href,
'stylesheet_path' => $css_file_path,
) );

$this->pending_stylesheets[] = array(
'keyframes' => false,
'stylesheet' => $stylesheet,
'node' => $element,
'sources' => $this->current_sources, // Needed because node is removed below.
$this->pending_stylesheets[] = array_merge(
array(
'keyframes' => false,
'node' => $element,
'sources' => $this->current_sources, // Needed because node is removed below.
),
wp_array_slice_assoc( $processed, array( 'stylesheet', 'imported_font_urls' ) )
);

// Remove now that styles have been processed.
Expand Down Expand Up @@ -755,12 +768,18 @@ private function fetch_external_stylesheet( $url ) {
* @type array $allowed_at_rules Allowed @-rules.
* @type bool $validate_keyframes Whether keyframes should be validated.
* }
* @return array Processed stylesheet parts.
* @return array {
* Processed stylesheet.
*
* @type array $stylesheet Stylesheet parts, where arrays are tuples for declaration blocks.
* @type array $validation_results Validation results, array containing arrays with error and sanitized keys.
* @type array $imported_font_urls Imported font stylesheet URLs.
* }
*/
private function process_stylesheet( $stylesheet, $options = array() ) {
$parsed = null;
$cache_key = null;
$cache_group = 'amp-parsed-stylesheet-v10'; // This should be bumped whenever the PHP-CSS-Parser is updated.
$cache_group = 'amp-parsed-stylesheet-v11'; // This should be bumped whenever the PHP-CSS-Parser is updated.

$cache_impacting_options = array_merge(
wp_array_slice_assoc(
Expand Down Expand Up @@ -809,7 +828,7 @@ private function process_stylesheet( $stylesheet, $options = array() ) {
}
}

return $parsed['stylesheet'];
return $parsed;
}

/**
Expand Down Expand Up @@ -843,9 +862,26 @@ private function parse_import_stylesheet( Import $item, CSSList $css_list, $opti
}
$this->processed_imported_stylesheet_urls[ $import_stylesheet_url ] = true;

// Prevent importing font stylesheets from allowed font CDNs. These will get added to the document as links instead.
$https_import_stylesheet_url = preg_replace( '#^(http:)?(?=//)#', 'https:', $import_stylesheet_url );
if ( $this->allowed_font_src_regex && preg_match( $this->allowed_font_src_regex, $https_import_stylesheet_url ) ) {
$this->imported_font_urls[] = $https_import_stylesheet_url;
$css_list->remove( $item );
_doing_it_wrong(
'wp_enqueue_style',
esc_html( sprintf(
/* translators: %s is URL to font CDN */
__( 'It is not a best practice to use @import to load font CDN stylesheets. Please use wp_enqueue_style() to enqueue %s as its own separate script.', 'amp' ),
$import_stylesheet_url
) ),
'1.0'
);
return array();
}

$css_file_path = $this->get_validated_url_file_path( $import_stylesheet_url, array( 'css', 'less', 'scss', 'sass' ) );

if ( is_wp_error( $css_file_path ) && 'external_file_url' === $css_file_path->get_error_code() ) {
if ( is_wp_error( $css_file_path ) && ( 'disallowed_file_extension' === $css_file_path->get_error_code() || 'external_file_url' === $css_file_path->get_error_code() ) ) {
$contents = $this->fetch_external_stylesheet( $import_stylesheet_url );
if ( is_wp_error( $contents ) ) {
$error = array(
Expand Down Expand Up @@ -922,6 +958,8 @@ private function parse_import_stylesheet( Import $item, CSSList $css_list, $opti
private function parse_stylesheet( $stylesheet_string, $options ) {
$validation_results = array();
$css_document = null;

$this->imported_font_urls = array();
try {
// Remove spaces from data URLs, which cause errors and PHP-CSS-Parser can't handle them.
$stylesheet_string = $this->remove_spaces_from_data_urls( $stylesheet_string );
Expand Down Expand Up @@ -960,7 +998,12 @@ function ( $value ) {

$validation_results[] = compact( 'error', 'sanitized' );
}
return compact( 'validation_results', 'css_document' );
return array_merge(
compact( 'validation_results', 'css_document' ),
array(
'imported_font_urls' => $this->imported_font_urls,
)
);
}

/**
Expand All @@ -975,6 +1018,7 @@ function ( $value ) {
*
* @type array $stylesheet Stylesheet parts, where arrays are tuples for declaration blocks.
* @type array $validation_results Validation results, array containing arrays with error and sanitized keys.
* @type array $imported_font_urls Imported font stylesheet URLs.
* }
*/
private function prepare_stylesheet( $stylesheet_string, $options = array() ) {
Expand Down Expand Up @@ -1098,7 +1142,12 @@ function( $matches ) use ( $selector, &$selectors_parsed ) {

$this->parse_css_duration += ( microtime( true ) - $start_time );

return compact( 'stylesheet', 'validation_results' );
return array_merge(
compact( 'stylesheet', 'validation_results' ),
array(
'imported_font_urls' => $parsed_stylesheet['imported_font_urls'],
)
);
}

/**
Expand Down Expand Up @@ -1475,8 +1524,8 @@ private function process_font_face_at_rule( AtRuleSet $ruleset ) {
continue;
}

// Ensure file exists.
$path = $this->get_validated_url_file_path( $guessed_url );
// Ensure font file exists.
$path = $this->get_validated_url_file_path( $guessed_url, array( 'woff', 'woff2', 'ttf', 'otf', 'svg' ) );
if ( is_wp_error( $path ) ) {
continue;
}
Expand Down Expand Up @@ -1658,16 +1707,16 @@ private function collect_inline_styles( $element ) {

$this->set_current_node( $element ); // And sources when needing to be located.

$stylesheet = $this->process_stylesheet( $rule, array(
$processed = $this->process_stylesheet( $rule, array(
'allowed_at_rules' => array(),
'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['declaration'],
) );

$element->removeAttribute( 'style' );

if ( $stylesheet ) {
if ( $processed['stylesheet'] ) {
$this->pending_stylesheets[] = array(
'stylesheet' => $stylesheet,
'stylesheet' => $processed['stylesheet'],
'node' => $element,
'sources' => $this->current_sources,
);
Expand Down Expand Up @@ -1710,6 +1759,8 @@ private function finalize_styles() {
),
);

$imported_font_urls = array();

/*
* 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.
Expand Down Expand Up @@ -1737,6 +1788,10 @@ private function finalize_styles() {
$stylesheet_sets[ $set_name ]['total_size'] += $size;
$stylesheet_sets[ $set_name ]['imports'] = $imports;
$stylesheet_sets[ $set_name ]['pending_stylesheets'][] = $pending_stylesheet;

if ( ! empty( $pending_stylesheet['imported_font_urls'] ) ) {
$imported_font_urls = array_merge( $imported_font_urls, $pending_stylesheet['imported_font_urls'] );
}
}

// Process the pending stylesheets.
Expand Down Expand Up @@ -1827,6 +1882,19 @@ private function finalize_styles() {
}
}

/*
* Add font stylesheets from CDNs which were extracted from @import rules.
* We can't add crossorigin=anonymous to these since such a CORS request would not be made in the non-AMP version,
* and so if the service worker cached the opaque response on the non-AMP version then it wouldn't be usable in
* the AMP version if it was requested with CORS.
*/
foreach ( array_unique( $imported_font_urls ) as $imported_font_url ) {
$link = $this->dom->createElement( 'link' );
$link->setAttribute( 'rel', 'stylesheet' );
$link->setAttribute( 'href', $imported_font_url );
$this->head->appendChild( $link );
}

// Add style[amp-keyframes] to document.
if ( ! empty( $stylesheet_sets['keyframes']['final_stylesheets'] ) ) {
$body = $this->dom->getElementsByTagName( 'body' )->item( 0 );
Expand Down
54 changes: 49 additions & 5 deletions tests/test-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,13 @@ public function get_link_and_style_test_data() {
),
array(),
),
'external_link_without_css_file_extension' => array(
'<html amp><head><meta charset="utf-8"><link rel="stylesheet" href="https://example.com/_static/??-eJx9kN1SAyEMhV9Iip3aOl44Pgs"></head><body><span>externally-styled</span></body></html>', // phpcs:ignore
array(
'span:before{content:"Returned from: https://example.com/_static/??-eJx9kN1SAyEMhV9Iip3aOl44Pgs"}',
),
array(),
),
);
}

Expand All @@ -373,6 +380,16 @@ public function test_link_and_style_elements( $source, $expected_stylesheets, $e
add_filter( 'locale', function() {
return 'en_US';
} );
add_filter( 'pre_http_request', function( $preempt, $request, $url ) {
unset( $request, $preempt );
$preempt = array(
'response' => array(
'code' => 200,
),
'body' => sprintf( 'span:before { content: "Returned from: %s"; }', $url ),
);
return $preempt;
}, 10, 3 );
$dom = AMP_DOM_Utils::get_dom( $source );

$error_codes = array();
Expand Down Expand Up @@ -1243,14 +1260,23 @@ public function test_cors_enabled_stylesheet_url() {
/**
* Test CSS imports.
*
* @expectedIncorrectUsage wp_enqueue_style
* @covers AMP_Style_Sanitizer::parse_import_stylesheet()
*/
public function test_css_import() {
$local_css_url = admin_url( 'css/login.css' );
$import_css_url = 'https://stylesheets.example.com/style.css';
$markup = sprintf( '<html><head><link rel="stylesheet" href="%s"><style>@import url("%s"); body { color:red; }</style></head><body>hello</body></html>', $local_css_url, $import_css_url ); // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
$local_css_url = admin_url( 'css/login.css' );
$import_font_url = 'https://fonts.googleapis.com/css?family=Merriweather:300|PT+Serif:400i|Open+Sans:800|Zilla+Slab:300,400,500|Montserrat:800|Muli:400&subset=cyrillic-ext,latin-ext,cyrillic,greek,greek-ext,vietnamese';
$import_css_url = 'https://stylesheets.example.com/style.css';
$import_css_url2 = 'https://stylesheets.example.com/dynamic-css/';
$markup = sprintf(
'<html><head><link rel="stylesheet" href="%s"><style>@import url("%s"); body { color:red; }</style><style>@import "%s";</style><style>@import "%s";</style></head><body>hello</body></html>', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
$local_css_url,
$import_css_url,
$import_font_url,
$import_css_url2
);

add_filter( 'pre_http_request', function( $preempt, $request, $url ) use ( $import_css_url ) {
add_filter( 'pre_http_request', function( $preempt, $request, $url ) use ( $import_css_url, $import_css_url2 ) {
unset( $request );
if ( $url === $import_css_url ) {
$preempt = array(
Expand All @@ -1259,6 +1285,13 @@ public function test_css_import() {
),
'body' => 'html { background-color:lightblue; }',
);
} elseif ( $url === $import_css_url2 ) {
$preempt = array(
'response' => array(
'code' => 200,
),
'body' => 'strong { background-color:red; }',
);
}
return $preempt;
}, 10, 3 );
Expand All @@ -1270,7 +1303,7 @@ public function test_css_import() {
) );
$sanitizer->sanitize();
$stylesheets = array_values( $sanitizer->get_stylesheets() );
$this->assertCount( 2, $stylesheets );
$this->assertCount( 4, $stylesheets );
$this->assertRegExp(
'/' . implode( '.*', array(
preg_quote( 'input[type="checkbox"]:disabled' ),
Expand All @@ -1286,6 +1319,17 @@ public function test_css_import() {
) ) . '/s',
$stylesheets[1]
);

$this->assertEmpty( $stylesheets[2] ); // Since it was importing a font CDN URL.
$this->assertEquals( 'strong{background-color:red}', $stylesheets[3] );

$this->assertNotContains( '@import', $dom->getElementsByTagName( 'style' )->item( 0 )->textContent );

$links = $dom->getElementsByTagName( 'link' );
$this->assertEquals( 1, $links->length );
$link = $links->item( 0 );
$this->assertEquals( $import_font_url, $link->getAttribute( 'href' ) );
$this->assertEquals( 'stylesheet', $link->getAttribute( 'rel' ) );
}

/**
Expand Down