Skip to content

Commit

Permalink
Introduce DISALLOWED_ATTR_CLASS_NAME and CSS_DISALLOWED_SELECTOR errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@westonruter
westonruter committed Sep 11, 2020
1 parent de3706d commit 0cd5776
Show file tree
Hide file tree
Showing 4 changed files with 154 additions and 16 deletions.
70 changes: 61 additions & 9 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,19 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
const STYLE_AMP_CUSTOM_GROUP_INDEX = 0;
const STYLE_AMP_KEYFRAMES_GROUP_INDEX = 1;

// Error codes raised during parsing CSS. See get_css_parser_validation_error_codes.
const CSS_SYNTAX_INVALID_AT_RULE = 'CSS_SYNTAX_INVALID_AT_RULE';
const CSS_SYNTAX_INVALID_DECLARATION = 'CSS_SYNTAX_INVALID_DECLARATION';
const CSS_SYNTAX_INVALID_PROPERTY = 'CSS_SYNTAX_INVALID_PROPERTY';
const CSS_SYNTAX_INVALID_PROPERTY_NOLIST = 'CSS_SYNTAX_INVALID_PROPERTY_NOLIST';
const CSS_SYNTAX_INVALID_IMPORTANT = 'CSS_SYNTAX_INVALID_IMPORTANT';
const CSS_SYNTAX_PARSE_ERROR = 'CSS_SYNTAX_PARSE_ERROR';
const STYLESHEET_TOO_LONG = 'STYLESHEET_TOO_LONG';
const CSS_DISALLOWED_SELECTOR = 'CSS_DISALLOWED_SELECTOR';
const STYLESHEET_FETCH_ERROR = 'STYLESHEET_FETCH_ERROR';
const STYLESHEET_TOO_LONG = 'STYLESHEET_TOO_LONG';

// Error code when encountering 'i-amphtml-' prefixing a class name in an HTML class attribute.
const DISALLOWED_ATTR_CLASS_NAME = 'DISALLOWED_ATTR_CLASS_NAME';

// 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 @@ -358,10 +363,11 @@ public static function get_css_parser_validation_error_codes() {
return [
self::CSS_SYNTAX_INVALID_AT_RULE,
self::CSS_SYNTAX_INVALID_DECLARATION,
self::CSS_SYNTAX_INVALID_IMPORTANT,
self::CSS_SYNTAX_INVALID_PROPERTY,
self::CSS_SYNTAX_INVALID_PROPERTY_NOLIST,
self::CSS_SYNTAX_INVALID_IMPORTANT,
self::CSS_SYNTAX_PARSE_ERROR,
self::CSS_DISALLOWED_SELECTOR,
self::STYLESHEET_FETCH_ERROR,
self::STYLESHEET_TOO_LONG,
];
Expand Down Expand Up @@ -500,21 +506,45 @@ private function get_used_class_names() {
'amp-mode-keyboard-active',
];

$classes = ' ';
$classes = [];

$i_amphtml_prefix = 'i-amphtml-';
$i_amphtml_prefix_length = 10;
foreach ( $this->dom->xpath->query( '//*/@class' ) as $class_attribute ) {
$classes .= ' ' . $class_attribute->nodeValue;
/** @var DOMAttr $class_attribute */
$mutated = false;
$element_classes = [];
foreach ( array_filter( preg_split( '/\s+/', trim( $class_attribute->nodeValue ) ) ) as $class_name ) {
if ( substr( $class_name, 0, $i_amphtml_prefix_length ) === $i_amphtml_prefix ) {
$error = [
'code' => self::DISALLOWED_ATTR_CLASS_NAME,
'type' => AMP_Validation_Error_Taxonomy::HTML_ATTRIBUTE_ERROR_TYPE,
'class_name' => $class_name,
];
$sanitized = $this->should_sanitize_validation_error( $error, [ 'node' => $class_attribute ] );
if ( $sanitized ) {
$mutated = true;
continue;
}
}
$element_classes[] = $class_name;
}
if ( $mutated ) {
$class_attribute->nodeValue = implode( ' ', $element_classes );
}
$classes = array_merge( $classes, $element_classes );
}

// Find all [class] attributes and capture the contents of any single- or double-quoted strings.
foreach ( $this->dom->xpath->query( '//*/@' . Document::AMP_BIND_DATA_ATTR_PREFIX . 'class' ) as $bound_class_attribute ) {
if ( preg_match_all( '/([\'"])([^\1]*?)\1/', $bound_class_attribute->nodeValue, $matches ) ) {
$classes .= ' ' . implode( ' ', $matches[2] );
$classes = array_merge( $classes, $matches[2] );
}
}

$class_names = array_merge(
$dynamic_class_names,
array_unique( array_filter( preg_split( '/\s+/', trim( $classes ) ) ) )
array_unique( array_filter( $classes ) )
);

// Find all instances of the toggleClass() action to prevent the class name from being tree-shaken.
Expand Down Expand Up @@ -2266,7 +2296,10 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l
$results = [];

if ( $ruleset instanceof DeclarationBlock ) {
$this->ampify_ruleset_selectors( $ruleset );
$results = array_merge(
$results,
$this->ampify_ruleset_selectors( $ruleset )
);
if ( 0 === count( $ruleset->getSelectors() ) ) {
$css_list->remove( $ruleset );
return $results;
Expand Down Expand Up @@ -2326,7 +2359,6 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l
if ( 0 === count( $ruleset->getRules() ) ) {
$css_list->remove( $ruleset );
}
// @todo Delete rules with selectors for -amphtml- class and i-amphtml- tags.
return $results;
}

Expand Down Expand Up @@ -3010,14 +3042,32 @@ public function add_css_budget_to_admin_bar() {
* Convert CSS selectors and remove obsolete selector hacks for IE.
*
* @param DeclarationBlock $ruleset Ruleset.
* @return array Validation results.
*/
private function ampify_ruleset_selectors( $ruleset ) {
$selectors = [];
$selectors = [];
$results = [];

$has_changed_selectors = false;
$language = strtolower( get_bloginfo( 'language' ) );
foreach ( $ruleset->getSelectors() as $old_selector ) {
$selector = $old_selector->getSelector();

// Strip out selectors that contain the disallowed prefix 'i-amphtml-'.
if ( preg_match( '/(^|\W)i-amphtml-/', $selector ) ) {
$error = [
'code' => self::CSS_DISALLOWED_SELECTOR,
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
'css_selector' => $selector,
];
$sanitized = $this->should_sanitize_validation_error( $error );
$results[] = compact( 'error', 'sanitized' );
if ( $sanitized ) {
$has_changed_selectors = true;
continue;
}
}

// Automatically tree-shake IE6/IE7 hacks for selectors with `* html` and `*+html`.
if ( preg_match( '/^\*\s*\+?\s*html/', $selector ) ) {
$has_changed_selectors = true;
Expand Down Expand Up @@ -3183,6 +3233,8 @@ static function ( $matches ) {
if ( $has_changed_selectors ) {
$ruleset->setSelectors( $selectors );
}

return $results;
}

/**
Expand Down
3 changes: 2 additions & 1 deletion includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -878,7 +878,8 @@ private function validate_cdata_for_node( DOMElement $element, $cdata_spec ) {
// There are only a few error messages, so map them to error codes.
switch ( $disallowed_cdata_regex['error_message'] ) {
case 'CSS i-amphtml- name prefix':
return [ 'code' => self::INVALID_CDATA_CSS_I_AMPHTML_NAME ]; // @todo This really should be done as part of the CSS sanitizer.
// The prefix used in selectors is handled by style sanitizer.
return [ 'code' => self::INVALID_CDATA_CSS_I_AMPHTML_NAME ];
case 'contents':
return [ 'code' => self::INVALID_CDATA_CONTENTS ];
case 'html comments':
Expand Down
14 changes: 13 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 @@ -2152,7 +2152,7 @@ public static function render_single_url_error_details( $validation_error, $term
?>
<dt><?php echo esc_html( self::get_source_key_label( $key, $validation_error ) ); ?></dt>
<dd class="detailed">
<?php if ( in_array( $key, [ 'node_name', 'parent_name', 'required_parent_name', 'required_attr_value' ], true ) ) : ?>
<?php if ( in_array( $key, [ 'node_name', 'parent_name', 'required_parent_name', 'required_attr_value', 'css_selector', 'class_name' ], true ) ) : ?>
<code><?php echo esc_html( $value ); ?></code>
<?php elseif ( 'css_property_name' === $key ) : ?>
<?php
Expand Down Expand Up @@ -2996,6 +2996,14 @@ public static function get_error_title_from_code( $validation_error ) {
$title .= sprintf( ': <code>%s</code>', esc_html( $validation_error['css_property_name'] ) );
}
return $title;
case AMP_Style_Sanitizer::CSS_DISALLOWED_SELECTOR:
return esc_html__( 'Illegal CSS selector', 'amp' );
case AMP_Style_Sanitizer::DISALLOWED_ATTR_CLASS_NAME:
$title = esc_html__( 'Disallowed class name', 'amp' );
if ( isset( $validation_error['class_name'] ) ) {
$title .= sprintf( ': <code>%s</code>', esc_html( $validation_error['class_name'] ) );
}
return $title;
case AMP_Tag_And_Attribute_Sanitizer::CDATA_TOO_LONG:
case AMP_Tag_And_Attribute_Sanitizer::MANDATORY_CDATA_MISSING_OR_INCORRECT:
case AMP_Tag_And_Attribute_Sanitizer::INVALID_CDATA_HTML_COMMENTS:
Expand Down Expand Up @@ -3277,6 +3285,10 @@ public static function get_source_key_label( $key, $validation_error ) {
return __( 'Parent element', 'amp' );
case 'css_property_name':
return __( 'CSS property', 'amp' );
case 'css_selector':
return __( 'CSS selector', 'amp' );
case 'class_name':
return __( 'Class name', 'amp' );
case 'duplicate_oneof_attrs':
return __( 'Mutually exclusive attributes', 'amp' );
case 'text':
Expand Down
83 changes: 78 additions & 5 deletions tests/php/test-amp-style-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,73 @@ public function get_body_style_attribute_data() {
':root:not(#_):not(#_):not(#_):not(#_):not(#_) .amp-wp-d4ea4c7{outline:solid 1px black}',
],
],
'with_internal_amp_selectors_and_class_names' => [
'
<html>
<head>
<style>
amp-img[layout=intrinsic],
amp-img.i-amphtml-layout-responsive,
amp-img:not(:not(.i-amphtml-layout-responsive)), /* Double :not() to prevent tree-shaker from masking validation error. */
amp-img.size-full {
outline: solid 1px red;
}
</style>
<style>
amp-img > *:first-child,
amp-img > *:first-child:not(:not(.i-amphtml-sizer)), /* Double :not() to prevent tree-shaker from masking validation error. */
i-amphtml-sizer.i-amphtml-sizer
{
outline: dotted 2px orange;
}
</style>
</head>
<body>
<amp-img class="size-full wp-image-904 alignright amp-wp-enforced-sizes i-amphtml-layout-intrinsic i-amphtml-layout-size-defined" title="Image Alignment 150x150" alt="Image Alignment 150x150" src="https://example.com/wp-content/uploads/2013/03/image-alignment-150x150-1.jpg" width="150" height="150" layout="intrinsic" i-amphtml-layout="intrinsic">
<i-amphtml-sizer class="i-amphtml-sizer">
<img alt="" aria-hidden="true" class="i-amphtml-intrinsic-sizer" role="presentation" src="data:image/svg+xml;base64,PHN2ZyBoZWlnaHQ9JzE1MCcgd2lkdGg9JzE1MCcgeG1sbnM9J2h0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnJyB2ZXJzaW9uPScxLjEnLz4=">
</i-amphtml-sizer>
<noscript>
<img loading="lazy" class="size-full wp-image-904 alignright" title="Image Alignment 150x150" alt="Image Alignment 150x150" src="https://example.com/wp-content/uploads/2013/03/image-alignment-150x150-1.jpg" width="150" height="150">
</noscript>
</amp-img>
</body>
</html>
',
'
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width">
</head>
<body>
<amp-img class="size-full wp-image-904 alignright amp-wp-enforced-sizes" title="Image Alignment 150x150" alt="Image Alignment 150x150" src="https://example.com/wp-content/uploads/2013/03/image-alignment-150x150-1.jpg" width="150" height="150" layout="intrinsic">
<noscript>
<img loading="lazy" class="size-full wp-image-904 alignright" title="Image Alignment 150x150" alt="Image Alignment 150x150" src="https://example.com/wp-content/uploads/2013/03/image-alignment-150x150-1.jpg" width="150" height="150">
</noscript>
</amp-img>
</body>
</html>
',
[
'amp-img[layout=intrinsic],amp-img.size-full{outline:solid 1px red}',
'amp-img > *:first-child{outline:dotted 2px orange}',
],
[
AMP_Style_Sanitizer::CSS_DISALLOWED_SELECTOR,
AMP_Style_Sanitizer::CSS_DISALLOWED_SELECTOR,
AMP_Style_Sanitizer::CSS_DISALLOWED_SELECTOR,
AMP_Style_Sanitizer::CSS_DISALLOWED_SELECTOR,
AMP_Style_Sanitizer::DISALLOWED_ATTR_CLASS_NAME,
AMP_Style_Sanitizer::DISALLOWED_ATTR_CLASS_NAME,
AMP_Style_Sanitizer::DISALLOWED_ATTR_CLASS_NAME,
AMP_Style_Sanitizer::DISALLOWED_ATTR_CLASS_NAME,
AMP_Tag_And_Attribute_Sanitizer::MANDATORY_TAG_ANCESTOR,
AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_TAG,
AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_ATTR,
],
],
];
}

Expand Down Expand Up @@ -393,12 +460,17 @@ public function test_body_style_attribute_sanitizer( $source, $expected_content,
$validating_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, $args );
$validating_sanitizer->sanitize();

if ( $use_document_element && count( $sanitizer->get_stylesheets() ) > 0 ) {
$this->assertEquals( 1, $dom->xpath->query( '//style[ @amp-custom ]' )->length, 'Expected stylesheet to be present in page. Failure means INVALID_CDATA_CSS_I_AMPHTML_NAME happened.' );
}

// Remove style elements since we will examine the underlying stylesheets instead.
foreach ( iterator_to_array( $dom->getElementsByTagName( 'style' ) ) as $element ) {
$element->parentNode->removeChild( $element );
}
foreach ( iterator_to_array( $dom->getElementsByTagName( 'noscript' ) ) as $element ) {
$element->parentNode->removeChild( $element );
if ( 'noscript' === $element->parentNode->nodeName ) {
$element->parentNode->parentNode->removeChild( $element->parentNode );
} else {
$element->parentNode->removeChild( $element );
}
}

// Test content.
Expand Down Expand Up @@ -3127,9 +3199,10 @@ public function test_get_css_parser_validation_error_codes() {
AMP_Style_Sanitizer::CSS_SYNTAX_PARSE_ERROR,
AMP_Style_Sanitizer::STYLESHEET_FETCH_ERROR,
AMP_Style_Sanitizer::STYLESHEET_TOO_LONG,
AMP_Style_Sanitizer::CSS_DISALLOWED_SELECTOR,
];

$this->assertEquals( $expected, AMP_Style_Sanitizer::get_css_parser_validation_error_codes() );
$this->assertEqualSets( $expected, AMP_Style_Sanitizer::get_css_parser_validation_error_codes() );
}

/**
Expand Down

0 comments on commit 0cd5776

Please sign in to comment.