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

Jump to bottom

Update sanitization reporting #951

Merged
merged 5 commits into from
Feb 15, 2018
Merged

Update sanitization reporting #951

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c54a02d
Refactor remove_invalid_callback to take single DOMNode arg, whether …
westonruter Feb 9, 2018
fd56169
Indicate the counts for invalid elements and attributes
westonruter Feb 10, 2018
2305fcb
Issue #843: Move Sanitization reporting to 'edit_form_top.'
Feb 13, 2018
e7f896d
Issue #843: Prevent displayling extra errors for <p>.
Feb 13, 2018
18a877e
Issue #843: Exit early if ! has_cap().
Feb 13, 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
23 changes: 16 additions & 7 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 @@ -314,12 +314,12 @@ public function maybe_enforce_https_src( $src, $force_https = false ) {
* @since 0.7
*
* @param DOMElement $child The node to remove.
* @return void.
* @return void
*/
public function remove_invalid_child( $child ) {
$child->parentNode->removeChild( $child );
if ( isset( $this->args['remove_invalid_callback'] ) ) {
call_user_func( $this->args['remove_invalid_callback'], $child, AMP_Validation_Utils::NODE_REMOVED );
call_user_func( $this->args['remove_invalid_callback'], $child );
}
}

Expand All @@ -331,14 +331,23 @@ public function remove_invalid_child( $child ) {
*
* @since 0.7
*
* @param DOMElement $element The node for which to remove the attribute.
* @param string $attribute The attribute to remove from the node.
* @return void.
* @param DOMElement $element The node for which to remove the attribute.
* @param DOMAttr|string $attribute The attribute to remove from the element.
* @return void
*/
public function remove_invalid_attribute( $element, $attribute ) {
$element->removeAttribute( $attribute );
if ( isset( $this->args['remove_invalid_callback'] ) ) {
call_user_func( $this->args['remove_invalid_callback'], $element, AMP_Validation_Utils::ATTRIBUTE_REMOVED, $attribute );
if ( is_string( $attribute ) ) {
$attribute = $element->getAttributeNode( $attribute );
}
if ( $attribute ) {
$element->removeAttributeNode( $attribute );
call_user_func( $this->args['remove_invalid_callback'], $attribute );
}
} elseif ( is_string( $attribute ) ) {
$element->removeAttribute( $attribute );
} else {
$element->removeAttributeNode( $attribute );
}
}

Expand Down
5 changes: 1 addition & 4 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 @@ -729,10 +729,7 @@ private function sanitize_disallowed_attributes_in_node( $node, $attr_spec_list
}

foreach ( $attrs_to_remove as $attr ) {
$node->removeAttributeNode( $attr );
if ( isset( $this->args['remove_invalid_callback'], $attr->name ) ) {
call_user_func( $this->args['remove_invalid_callback'], $node, AMP_Validation_Utils::ATTRIBUTE_REMOVED, $attr->name );
}
$this->remove_invalid_attribute( $node, $attr );
}
}

Expand Down
126 changes: 60 additions & 66 deletions includes/utils/class-amp-validation-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,6 @@
*/
class AMP_Validation_Utils {

/**
* The argument if an attribute was removed.
*
* @var array.
*/
const ATTRIBUTE_REMOVED = 'removed_attr';

/**
* The argument if a node was removed.
*
* @var array.
*/
const NODE_REMOVED = 'removed';

/**
* Key for the markup value in the REST API endpoint.
*
Expand Down Expand Up @@ -68,24 +54,17 @@ class AMP_Validation_Utils {
*/
const ERROR_NONCE_ACTION = 'invalid_amp_message';

/**
* The attributes that the sanitizer removed.
*
* @var array.
*/
public static $removed_attributes;

/**
* The nodes that the sanitizer removed.
*
* @var array.
* @var DOMNode[]
*/
public static $removed_nodes;
public static $removed_nodes = array();

/**
* Add the actions.
*
* @return void.
* @return void
*/
public static function init() {
add_action( 'rest_api_init', array( __CLASS__, 'amp_rest_validation' ) );
Expand All @@ -94,32 +73,13 @@ public static function init() {
}

/**
* Tracks when a sanitizer removes an attribute or node.
* Tracks when a sanitizer removes an node (element or attribute).
*
* @param DOMNode|DOMElement $node The node in which there was a removal.
* @param string $removal_type The removal: 'removed_attr' for an attribute, or 'removed' for a node or element.
* @param string $attr_name The name of the attribute removed (optional).
* @return void.
* @param DOMNode $node The node which was removed.
* @return void
*/
public static function track_removed( $node, $removal_type, $attr_name = null ) {
if ( ( self::ATTRIBUTE_REMOVED === $removal_type ) && isset( $attr_name ) ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar
self::$removed_attributes = self::increment_count( self::$removed_attributes, $attr_name );
} elseif ( ( self::NODE_REMOVED === $removal_type ) && isset( $node->nodeName ) ) {
self::$removed_nodes = self::increment_count( self::$removed_nodes, $node->nodeName ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar
}
}

/**
* Tracks when a sanitizer removes an attribute or node.
*
* @param array $histogram The count of attributes or nodes removed.
* @param string $key The attribute or node name removed.
* @return array $histogram The incremented histogram.
*/
public static function increment_count( $histogram, $key ) {
$current_value = isset( $histogram[ $key ] ) ? $histogram[ $key ] : 0;
$histogram[ $key ] = $current_value + 1;
return $histogram;
public static function track_removed( $node ) {
self::$removed_nodes[] = $node;
}

/**
Expand Down Expand Up @@ -212,11 +172,34 @@ public static function get_response( $markup = null ) {
self::process_markup( $markup );
$response['processed_markup'] = $markup;
}
$response = array_merge( array(
self::ERROR_KEY => self::was_node_removed(),
'removed_nodes' => self::$removed_nodes,
'removed_attributes' => self::$removed_attributes,
), $response );

$removed_elements = array();
$removed_attributes = array();
foreach ( self::$removed_nodes as $node ) {
if ( $node instanceof DOMAttr ) {
if ( ! isset( $removed_attributes[ $node->nodeName ] ) ) {
$removed_attributes[ $node->nodeName ] = 1;
} else {
$removed_attributes[ $node->nodeName ]++;
}
} elseif ( $node instanceof DOMElement ) {
if ( ! isset( $removed_elements[ $node->nodeName ] ) ) {
$removed_elements[ $node->nodeName ] = 1;
} else {
$removed_elements[ $node->nodeName ]++;
}
}
}

$response = array_merge(
array(
self::ERROR_KEY => self::was_node_removed(),
),
compact(
'removed_elements',
'removed_attributes'
),
$response );
self::reset_removed();

return $response;
Expand All @@ -232,8 +215,7 @@ public static function get_response( $markup = null ) {
* @return void.
*/
public static function reset_removed() {
self::$removed_nodes = null;
self::$removed_attributes = null;
self::$removed_nodes = array();
}

/**
Expand Down Expand Up @@ -272,8 +254,8 @@ public static function validate_content( $post_id, $post ) {
$location = AMP_Validation_Utils::error_message( $location );
$location = add_query_arg(
array(
'removed_elements' => array_keys( $response['removed_nodes'] ),
'removed_attributes' => array_keys( $response['removed_attributes'] ),
'removed_elements' => $response['removed_elements'],
'removed_attributes' => $response['removed_attributes'],
),
$location
);
Expand Down Expand Up @@ -334,20 +316,32 @@ public static function display_error() {
if ( self::ERROR_QUERY_VALUE === $error ) {
echo '<div class="notice notice-warning">';
printf( '<p>%s</p>', esc_html__( 'Warning: There is content which fails AMP validation; it will be stripped when served as AMP.', 'amp' ) );
$removed_sets = array();
if ( ! empty( $_GET['removed_elements'] ) && is_array( $_GET['removed_elements'] ) ) {
printf(
'<p>%s %s</p>',
esc_html__( 'Invalid elements:', 'amp' ),
'<code>' . implode( '</code>, <code>', array_map( 'esc_html', $_GET['removed_elements'] ) ) . '</code>'
$removed_sets[] = array(
'label' => __( 'Invalid elements:', 'amp' ),
'names' => array_map( 'sanitize_key', $_GET['removed_elements'] ),
);
}
if ( ! empty( $_GET['removed_attributes'] ) ) {
printf(
'<p>%s %s</p>',
esc_html__( 'Invalid attributes:', 'amp' ),
'<code>' . implode( '</code>, <code>', array_map( 'esc_html', $_GET['removed_attributes'] ) ) . '</code>'
if ( ! empty( $_GET['removed_attributes'] ) && is_array( $_GET['removed_attributes'] ) ) {
$removed_sets[] = array(
'label' => __( 'Invalid attributes:', 'amp' ),
'names' => array_map( 'sanitize_key', $_GET['removed_attributes'] ),
);
}
foreach ( $removed_sets as $removed_set ) {
printf( '<p>%s ', esc_html( $removed_set['label'] ) );
$items = array();
foreach ( $removed_set['names'] as $name => $count ) {
if ( 1 === intval( $count ) ) {
$items[] = sprintf( '<code>%s</code>', esc_html( $name ) );
} else {
$items[] = sprintf( '<code>%s</code> (%d)', esc_html( $name ), $count );
}
}
echo implode( ', ', $items ); // WPCS: XSS OK.
echo '</p>';
}
echo '</div>';
}
}
Expand Down
23 changes: 7 additions & 16 deletions tests/test-class-amp-base-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,41 +245,37 @@ public function test_enforce_sizes_attribute( $source_params, $expected_value, $
/**
* Tests remove_child.
*
* @see AMP_Base_Sanitizer::remove_invalid_child()
* @covers AMP_Base_Sanitizer::remove_invalid_child()
*/
public function test_remove_child() {
$parent_tag_name = 'div';
$child_tag_name = 'h1';
$dom_document = new DOMDocument( '1.0', 'utf-8' );
$parent = $dom_document->createElement( $parent_tag_name );
$child = $dom_document->createElement( 'h1' );
$parent->appendChild( $child );

// To ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
// @codingStandardsIgnoreStart

$this->assertEquals( $child, $parent->firstChild );
$sanitizer = new AMP_Iframe_Sanitizer( $dom_document, array(
'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed',
) );
$sanitizer->remove_invalid_child( $child );
$this->assertEquals( null, $parent->firstChild );
$this->assertEquals( 1, AMP_Validation_Utils::$removed_nodes[ $child_tag_name ] );
$this->assertCount( 1, AMP_Validation_Utils::$removed_nodes );
$this->assertEquals( $child, AMP_Validation_Utils::$removed_nodes[0] );

$parent->appendChild( $child );
$this->assertEquals( $child, $parent->firstChild );
$sanitizer->remove_invalid_child( $child );

$this->assertEquals( null, $parent->firstChild );
$this->assertEquals( null, $child->parentNode );
// @codingStandardsIgnoreEnd
AMP_Validation_Utils::$removed_nodes = null;
}

/**
* Tests remove_child.
*
* @see AMP_Base_Sanitizer::remove_invalid_child()
* @covers AMP_Base_Sanitizer::remove_invalid_child()
*/
public function test_remove_attribute() {
AMP_Validation_Utils::reset_removed();
Expand All @@ -288,20 +284,15 @@ public function test_remove_attribute() {
$dom_document = new DOMDocument( '1.0', 'utf-8' );
$video = $dom_document->createElement( $video_name );
$video->setAttribute( $attribute, 'someFunction()' );
$attr_node = $video->getAttributeNode( $attribute );

// To ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
// @codingStandardsIgnoreStart
$args = array(
$args = array(
'remove_invalid_callback' => 'AMP_Validation_Utils::track_removed',
);
$expected_removed = array(
$attribute => 1,
);
$sanitizer = new AMP_Video_Sanitizer( $dom_document, $args );
$sanitizer->remove_invalid_attribute( $video, $attribute );
$this->assertEquals( null, $video->getAttribute( $attribute ) );
$this->assertEquals( $expected_removed, AMP_Validation_Utils::$removed_attributes );
// @codingStandardsIgnoreEnd
$this->assertEquals( array( $attr_node ), AMP_Validation_Utils::$removed_nodes );
AMP_Validation_Utils::reset_removed();
}

Expand Down
16 changes: 14 additions & 2 deletions tests/test-class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public function test_prepare_response() {
<html amp <?php language_attributes(); ?>>
<head>
<?php wp_head(); ?>
<script data-head>document.write('TODO: This needs to be sanitized as well once.');</script>
<script data-head>document.write('Illegal');</script>
</head>
<body>
<img width="100" height="100" src="https://example.com/test.png">
Expand All @@ -114,12 +114,19 @@ public function test_prepare_response() {
data-aax_src="302"></amp-ad>
<?php wp_footer(); ?>

<button onclick="alert('Illegal');">no-onclick</button>

<style>body { background: black; }</style>
</body>
</html>
<?php
$original_html = trim( ob_get_clean() );
$sanitized_html = AMP_Theme_Support::prepare_response( $original_html );
$removed_nodes = array();
$sanitized_html = AMP_Theme_Support::prepare_response( $original_html, array(
'remove_invalid_callback' => function( $node ) use ( &$removed_nodes ) {
$removed_nodes[ $node->nodeName ] = $node;
},
) );

$this->assertContains( '<meta charset="' . get_bloginfo( 'charset' ) . '">', $sanitized_html );
$this->assertContains( '<meta name="viewport" content="width=device-width,minimum-scale=1">', $sanitized_html );
Expand All @@ -136,6 +143,11 @@ public function test_prepare_response() {
$this->assertContains( '<script async custom-element="amp-audio"', $sanitized_html );

$this->assertContains( '<script async custom-element="amp-ad"', $sanitized_html );

$this->assertContains( '<button>no-onclick</button>', $sanitized_html );
$this->assertCount( 2, $removed_nodes );
$this->assertInstanceOf( 'DOMElement', $removed_nodes['script'] );
$this->assertInstanceOf( 'DOMAttr', $removed_nodes['onclick'] );
}

/**
Expand Down
Loading