Skip to content

Commit

Permalink
Issue #843: Report removed attributes and nodes in a histogram.
Browse files Browse the repository at this point in the history 
This is only one approach.
But for now, the response has counts for:
'removed_nodes' and 'removed_attributes'.
If a <script> is removed, 'removed_nodes' will be:
{"script":1}.
The count will increment every time the same node type is removed.
There is a similar histogram for 'removed_attributes'.
  • Loading branch information
Ryan Kienstra committed Jan 30, 2018
1 parent aed76c6 commit 30c666f
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 23 deletions.
27 changes: 20 additions & 7 deletions includes/utils/class-amp-mutation-utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,26 @@ class AMP_Mutation_Utils {
* @return void.
*/
public static function track_removed( $node, $removal_type, $attr_name = null ) {
if ( ( self::ATTRIBUTE_REMOVED === $removal_type ) && isset( $node->nodeName, $attr_name ) ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar
self::$removed_attributes[] = array(
$node->nodeName => $attr_name, // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar
);
} elseif ( self::NODE_REMOVED === $removal_type ) {
self::$removed_nodes[] = $node;
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;
}

/**
* Gets whether a node was removed in a sanitizer.
*
Expand Down Expand Up @@ -134,7 +145,9 @@ public static function validate_markup( WP_REST_Request $request ) {

self::process_markup( $json[ self::MARKUP_KEY ] );
$response = array(
'is_error' => self::was_node_removed(),
'has_error' => self::was_node_removed(),
'removed_nodes' => self::$removed_nodes,
'removed_attributes' => self::$removed_attributes,
);
self::reset_removed();

Expand Down
3 changes: 1 addition & 2 deletions tests/test-amp-iframe-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,8 @@ public function test_remove_child() {
'mutation_callback' => 'AMP_Mutation_Utils::track_removed',
) );
$sanitizer->remove_child( $child );
$expected_removed = reset( AMP_Mutation_Utils::$removed_nodes );
$this->assertEquals( null, $parent->firstChild );
$this->assertEquals( $child_tag_name, $expected_removed->tagName );
$this->assertEquals( 1, AMP_Mutation_Utils::$removed_nodes[ $child_tag_name ] );

$parent->appendChild( $child );
$this->assertEquals( $child, $parent->firstChild );
Expand Down
52 changes: 38 additions & 14 deletions tests/test-class-amp-mutation-utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,35 @@ public function setUp() {
*/
public function test_track_removed() {
$attr_name = 'invalid-attr';
$expected_removed_attr = array(
array(
$this->node->nodeName => $attr_name, // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar
),
$expected_removed_attrs = array(
$attr_name => 1,
);
$expected_removed_nodes = array(
'img' => 1,
);
$this->assertEmpty( AMP_Mutation_Utils::$removed_attributes );
$this->assertEmpty( AMP_Mutation_Utils::$removed_nodes );
AMP_Mutation_Utils::track_removed( $this->node, AMP_Mutation_Utils::ATTRIBUTE_REMOVED, $attr_name );
$this->assertEquals( $expected_removed_attr, AMP_Mutation_Utils::$removed_attributes );
$this->assertEquals( $expected_removed_attrs, AMP_Mutation_Utils::$removed_attributes );
AMP_Mutation_Utils::track_removed( $this->node, AMP_Mutation_Utils::NODE_REMOVED );
$this->assertEquals( array( $this->node ), AMP_Mutation_Utils::$removed_nodes );
$this->assertEquals( $expected_removed_nodes, AMP_Mutation_Utils::$removed_nodes );
}

/**
* Test increment_count.
*
* @see AMP_Mutation_Utils::increment_count()
*/
public function test_increment_count() {
$attribute = 'script';
$one_attribute = array(
$attribute => 1,
);
$expected = array(
$attribute => 2,
);
$this->assertEquals( $one_attribute, AMP_Mutation_Utils::increment_count( array(), $attribute ) );
$this->assertEquals( $expected, AMP_Mutation_Utils::increment_count( $one_attribute, $attribute ) );
}

/**
Expand Down Expand Up @@ -112,25 +130,24 @@ public function test_process_markup() {
AMP_Mutation_Utils::reset_removed();

AMP_Mutation_Utils::process_markup( $this->disallowed_tag );
$removed_node = reset( AMP_Mutation_Utils::$removed_nodes );
$this->assertEquals( 'script', $removed_node->nodeName );
$this->assertEquals( 1, AMP_Mutation_Utils::$removed_nodes['script'] );
$this->assertEquals( null, AMP_Mutation_Utils::$removed_attributes );

AMP_Mutation_Utils::reset_removed();
$disallowed_style = '<div style="display:none"></div>';
AMP_Mutation_Utils::process_markup( $disallowed_style );
$removed_attribute = reset( AMP_Mutation_Utils::$removed_attributes );
$removed_attribute = AMP_Mutation_Utils::$removed_attributes;
$expected_removed_attributes = array(
'div' => 'style',
'style' => 1,
);
$this->assertEquals( null, AMP_Mutation_Utils::$removed_nodes );
$this->assertEquals( $expected_removed_attributes, $removed_attribute );

AMP_Mutation_Utils::reset_removed();
$invalid_video = '<video width="200" height="100"></video>';
AMP_Mutation_Utils::process_markup( $invalid_video );
$removed_node = reset( AMP_Mutation_Utils::$removed_nodes );
$this->assertEquals( 'video', $removed_node->nodeName ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
$removed_node = AMP_Mutation_Utils::$removed_nodes;
$this->assertEquals( 1, AMP_Mutation_Utils::$removed_nodes['video'] ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
$this->assertEquals( null, AMP_Mutation_Utils::$removed_attributes );
}

Expand Down Expand Up @@ -180,14 +197,19 @@ public function test_permission() {
* @see AMP_Mutation_Utils::validate_markup()
*/
public function test_validate_markup() {
$error_key = 'has_error';
$request = new WP_REST_Request( 'POST', $this->expected_route );
$request->set_header( 'content-type', 'application/json' );
$request->set_body( wp_json_encode( array(
AMP_Mutation_Utils::MARKUP_KEY => $this->disallowed_tag,
) ) );
$response = AMP_Mutation_Utils::validate_markup( $request );
$expected_response = array(
'is_error' => true,
$error_key => true,
'removed_nodes' => array(
'script' => 1,
),
'removed_attributes' => null,
);
$this->assertEquals( $expected_response, $response );

Expand All @@ -196,7 +218,9 @@ public function test_validate_markup() {
) ) );
$response = AMP_Mutation_Utils::validate_markup( $request );
$expected_response = array(
'is_error' => false,
$error_key => false,
'removed_nodes' => null,
'removed_attributes' => null,
);
$this->assertEquals( $expected_response, $response );
}
Expand Down

0 comments on commit 30c666f

Please sign in to comment.