Skip to content

Commit

Permalink
Harmonize logic for getting stylesheet by URL
Browse files Browse the repository at this point in the history 
Remove STYLESHEET_INVALID_FILE_PATH code and always fall-back to HTTP request when file not on file system
  • Loading branch information
@westonruter
westonruter committed Dec 6, 2019
1 parent b96e4e7 commit 827659a
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 63 deletions.
97 changes: 37 additions & 60 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
const CSS_SYNTAX_PARSE_ERROR = 'CSS_SYNTAX_PARSE_ERROR';
const STYLESHEET_TOO_LONG = 'STYLESHEET_TOO_LONG';
const STYLESHEET_INVALID_FILE_URL = 'STYLESHEET_INVALID_FILE_URL';
const STYLESHEET_INVALID_FILE_PATH = 'STYLESHEET_INVALID_FILE_PATH';

// These are internal to the sanitizer and not exposed as validation error codes.
const STYLESHEET_DISALLOWED_FILE_EXT = 'STYLESHEET_DISALLOWED_FILE_EXT';
Expand Down Expand Up @@ -350,7 +349,6 @@ public static function get_css_parser_validation_error_codes() {
self::CSS_SYNTAX_INVALID_PROPERTY,
self::CSS_SYNTAX_INVALID_PROPERTY_NOLIST,
self::CSS_SYNTAX_PARSE_ERROR,
self::STYLESHEET_INVALID_FILE_PATH,
self::STYLESHEET_INVALID_FILE_URL,
self::STYLESHEET_TOO_LONG,
];
Expand Down Expand Up @@ -1290,37 +1288,18 @@ private function process_link_element( DOMElement $element ) {
return;
}

$css_file_path = $this->get_validated_url_file_path( $href, [ 'css', 'less', 'scss', 'sass' ] );
if ( ! is_wp_error( $css_file_path ) ) {
$stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents -- It's a local filesystem path not a remote request.
if ( false === $stylesheet ) {
$this->remove_invalid_child(
$element,
[
// @todo Also include details about the error.
'code' => self::STYLESHEET_INVALID_FILE_PATH,
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
]
);
return;
}
} else {
// Fall back to doing an HTTP request for the stylesheet is not accessible directly from the filesystem.
$contents = $this->fetch_external_stylesheet( $normalized_url );
if ( ! is_wp_error( $contents ) ) {
$stylesheet = $contents;
} else {
$this->remove_invalid_child(
$element,
[
// @todo Also include details about the error.
'code' => self::STYLESHEET_INVALID_FILE_URL,
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
'url' => $normalized_url,
]
);
return;
}
$stylesheet = $this->get_stylesheet_from_url( $href );
if ( $stylesheet instanceof WP_Error ) {
$this->remove_invalid_child(
$element,
[
// @todo Also include details about the error.
'code' => self::STYLESHEET_INVALID_FILE_URL,
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
'url' => $normalized_url,
]
);
return;
}

// Honor the link's media attribute.
Expand All @@ -1337,7 +1316,6 @@ private function process_link_element( DOMElement $element ) {
'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,
'spec_name' => self::STYLE_AMP_CUSTOM_SPEC_NAME,
]
);
Expand All @@ -1358,6 +1336,28 @@ private function process_link_element( DOMElement $element ) {
$this->set_current_node( null );
}

/**
* Get stylesheet from URL.
*
* @since 1.5.0
*
* @param string $stylesheet_url Stylesheet URL.
* @return string|WP_Error Stylesheet string on success, or WP_Error on failure.
*/
private function get_stylesheet_from_url( $stylesheet_url ) {
$stylesheet = false;
$css_file_path = $this->get_validated_url_file_path( $stylesheet_url, [ 'css', 'less', 'scss', 'sass' ] );
if ( ! is_wp_error( $css_file_path ) ) {
$stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents -- It's a local filesystem path not a remote request.
}
if ( is_string( $stylesheet ) ) {
return $stylesheet;
}

// Fall back to doing an HTTP request for the stylesheet is not accessible directly from the filesystem.
return $this->fetch_external_stylesheet( $stylesheet_url );
}

/**
* Fetch external stylesheet.
*
Expand Down Expand Up @@ -1412,7 +1412,6 @@ private function fetch_external_stylesheet( $url ) {
* @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 array $allowed_at_rules Allowed @-rules.
* @type bool $validate_keyframes Whether keyframes should be validated.
* @type string $spec_name Spec name.
Expand Down Expand Up @@ -1538,30 +1537,11 @@ private function parse_import_stylesheet( Import $item, CSSList $css_list, $opti
return [];
}

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

if ( is_wp_error( $css_file_path ) && ( self::STYLESHEET_DISALLOWED_FILE_EXT === $css_file_path->get_error_code() || self::STYLESHEET_EXTERNAL_FILE_URL === $css_file_path->get_error_code() ) ) {
$contents = $this->fetch_external_stylesheet( $import_stylesheet_url );
if ( is_wp_error( $contents ) ) {
$error = [
// @todo Also include details about the error.
'code' => self::STYLESHEET_INVALID_FILE_URL,
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
'url' => $import_stylesheet_url,
];
$sanitized = $this->should_sanitize_validation_error( $error );
if ( $sanitized ) {
$css_list->remove( $item );
}
$results[] = compact( 'error', 'sanitized' );
return $results;
}

$stylesheet = $contents;
} elseif ( is_wp_error( $css_file_path ) ) {
$stylesheet = $this->get_stylesheet_from_url( $import_stylesheet_url );
if ( $stylesheet instanceof WP_Error ) {
$error = [
// @todo Also include details about the error.
'code' => self::STYLESHEET_INVALID_FILE_PATH,
'code' => self::STYLESHEET_INVALID_FILE_URL,
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
'url' => $import_stylesheet_url,
];
Expand All @@ -1571,8 +1551,6 @@ private function parse_import_stylesheet( Import $item, CSSList $css_list, $opti
}
$results[] = compact( 'error', 'sanitized' );
return $results;
} else {
$stylesheet = file_get_contents( $css_file_path ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents -- It's a local filesystem path not a remote request.
}

if ( $media_query ) {
Expand Down Expand Up @@ -1705,7 +1683,6 @@ private function prepare_stylesheet( $stylesheet_string, $options = [] ) {
'property_whitelist' => [],
'validate_keyframes' => false,
'stylesheet_url' => null,
'stylesheet_path' => null,
'spec_name' => null,
],
$options
Expand Down
1 change: 0 additions & 1 deletion includes/validation/class-amp-validation-error-taxonomy.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2912,7 +2912,6 @@ public static function get_error_title_from_code( $validation_error ) {
case AMP_Style_Sanitizer::CSS_SYNTAX_PARSE_ERROR:
return esc_html__( 'CSS parse error', 'amp' );
case AMP_Style_Sanitizer::STYLESHEET_INVALID_FILE_URL:
case AMP_Style_Sanitizer::STYLESHEET_INVALID_FILE_PATH:
return esc_html__( 'Missing stylesheet file', 'amp' );
case AMP_Style_Sanitizer::CSS_SYNTAX_INVALID_PROPERTY:
case AMP_Style_Sanitizer::CSS_SYNTAX_INVALID_PROPERTY_NOLIST:
Expand Down
4 changes: 2 additions & 2 deletions tests/php/test-amp-style-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1959,7 +1959,7 @@ static function ( WP_UnitTestCase $test, $stylesheet ) {
'https://bogus.example.com/remote-also-does-not-exist.css',
],
'<style>div::after{content:"End"}</style><style>@import url("https://bogus.example.com/remote-finally-does-not-exist.css");</style><body class="locale-he-il"><div class="login message"></div><table class="form-table"><td></td></table></body>', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
3, // Three HTTP requests (to bogus.example.com). The local-does-not-exist.css checks filesystem directly.
4, // All four result in HTTP requests, even the local one because it doesn't exist on the filesystem.
static function ( $requested_url ) {
if ( false !== strpos( $requested_url, 'does-not-exist' ) ) {
return new WP_Error( 'does_not_exist' );
Expand Down Expand Up @@ -1999,7 +1999,7 @@ static function ( WP_UnitTestCase $test, $stylesheet ) {
'https://bogus.example.com/remote-also-does-not-exist.css',
],
'<style>div::after{content:"End"}</style><style>@import url("https://bogus.example.com/remote-finally-does-not-exist.css");</style><body class="locale-he-il"><div class="login message"></div><table class="form-table"><td></td></table></body>', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
3, // Three HTTP requests (to bogus.example.com). The local-does-not-exist.css checks filesystem directly.
4, // All four result in HTTP requests, even the local one because it doesn't exist on the filesystem.
static function ( $requested_url ) {
if ( false !== strpos( $requested_url, 'does-not-exist' ) ) {
return new WP_Error( 'does_not_exist' );
Expand Down

0 comments on commit 827659a

Please sign in to comment.