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.

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

Add sanitization reporting #912

Merged
merged 43 commits into from Feb 9, 2018
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
0a44284
Add initial callback for listening for element/attribute removal in w…
westonruter Jan 27, 2018
e9f394a
Issue #843: Tracking for removed nodes and attributes.
Jan 28, 2018
d7104b8
Issue #843: Correct a failed Travis build by excluding a PHPCS rule.
Jan 28, 2018
616262f
Issue #843: Add a method to process markup for AMP validtity.
Jan 28, 2018
4714062
Issue #843: Track removed iframes in a helper method.
Jan 28, 2018
e209005
Issue #843: Initial registration of the REST endpoint for validation.
Jan 29, 2018
aed76c6
Issue #864: Support <amp-carousel> in 'Gallery' widget.
Jan 30, 2018
30c666f
Issue #843: Report removed attributes and nodes in a histogram.
Jan 30, 2018
76b0f17
Revert "Issue #864: Support <amp-carousel> in 'Gallery' widget."
Jan 30, 2018
0b7c3fc
Issue #843: Align equals signs vertically.
Jan 30, 2018
71744e5
Issue #843: Prepare to add headers to frontend GET requests.
Jan 30, 2018
da408bf
Issue #843: Align an equals sign to correct the failed Travis build.
Jan 30, 2018
ab3909a
Issue #864: Validation data in the response header.
Feb 1, 2018
90090b0
Issue #864: Remove an extra conditional, nest the 'mutation_callback.'
Feb 1, 2018
83dac87
Issue #864: Rename function to finish_output_buffering().
Feb 1, 2018
70521ef
Issue #843: Remove the extra variabl in the @return tag.
Feb 1, 2018
cc4fe85
Issue #843: Add processed markup to REST API response.
Feb 1, 2018
0b6a4ee
Issue #843: Output the 'processed_markup' at the bottom of the response.
Feb 1, 2018
db49a33
Issue #843: Merge in 'develop' branch and resolve conflicts.
Feb 1, 2018
c92bee4
Issue #843: Apply validation to post update on wp-admin/post.php.
Feb 1, 2018
6b5593b
Issue #843: Verify the nonce before validating on 'save_post'.
Feb 1, 2018
a168a5c
Issue #843: Sanitize $_GET value in addition to the 'ignore' comment.
Feb 1, 2018
a52ae05
Issue #843: Correct Travis error by changing error text.
Feb 1, 2018
0472a33
Issue #843: Report node removal in the rest of the sanitizers.
Feb 4, 2018
6d2350f
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter Feb 5, 2018
3de38b8
Issue #843: Call the 'mutation_callback' for more attribute removals.
Feb 5, 2018
46e7084
Issue #843: Merge in develop, resolve conflicts.
Feb 5, 2018
972ac2c
Issue #843: Merge in develop again, resolve conflicts.
Feb 7, 2018
d2ab9ee
Issue #843: Rename test to 'get_buffer' instead of 'prepare_response'.
Feb 7, 2018
cb0cfc9
Issue #843: Fix an issue in the error message.
Feb 7, 2018
084acaa
Issue #843: Revert renaming of methods, adjust unit tests.
Feb 8, 2018
13ab280
Issue #843: Remove special characters, update documentation.
Feb 8, 2018
f996673
Issue #843: Change @const to @var for constants.
Feb 8, 2018
f8aeca8
Issue #843: Rename class to 'AMP_Validation_Utils'
Feb 8, 2018
69317b8
Issue #843; Use constants instead of string literals.
Feb 8, 2018
583a6bc
Issue #843: Add nonce verification for the editor message.
Feb 8, 2018
cd910ff
Issue #843: Align comments in addition to variable names.
Feb 8, 2018
a6d071a
List the AMP-invalid elements and attributes that have been saved
westonruter Feb 8, 2018
b40729b
Only report mutations when node/attribute is removed due to invalidity
westonruter Feb 9, 2018
9954c11
Improve naming of callback and removal methods
westonruter Feb 9, 2018
15d9186
Remove add_header until use case is confirmed
westonruter Feb 9, 2018
bb7a175
Skip reporting removal of iframe's former parent node
westonruter Feb 9, 2018
29caa8a
Skip sanitization reporting when saving post that doesn't support AMP
westonruter Feb 9, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions amp.php
Expand Up @@ -95,6 +95,7 @@ function amp_after_setup_theme() {
add_action( 'wp_loaded', 'amp_add_options_menu' );
add_action( 'parse_query', 'amp_correct_query_when_is_front_page' );
AMP_Post_Type_Support::add_post_type_support();
AMP_Mutation_Utils::init();
}
add_action( 'after_setup_theme', 'amp_after_setup_theme', 5 );

Expand Down
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
Expand Up @@ -83,6 +83,7 @@ class AMP_Autoloader {
'AMP_DOM_Utils' => 'includes/utils/class-amp-dom-utils',
'AMP_HTML_Utils' => 'includes/utils/class-amp-html-utils',
'AMP_Image_Dimension_Extractor' => 'includes/utils/class-amp-image-dimension-extractor',
'AMP_Mutation_Utils' => 'includes/utils/class-amp-mutation-utils',
'AMP_String_Utils' => 'includes/utils/class-amp-string-utils',
'AMP_WP_Utils' => 'includes/utils/class-amp-wp-utils',
'AMP_Widget_Archives' => 'includes/widgets/class-amp-widget-archives',
Expand Down
7 changes: 6 additions & 1 deletion includes/class-amp-theme-support.php
Expand Up @@ -752,7 +752,9 @@ public static function start_output_buffering() {
* @see AMP_Theme_Support::start_output_buffering()
*/
public static function finish_output_buffering() {
echo self::prepare_response( ob_get_clean() ); // WPCS: xss ok.
$output = self::prepare_response( ob_get_clean() );
AMP_Mutation_Utils::add_header();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to limit this to only be added when a user specifically requests this additional information. Like there should be a nonce that must be present to authorize the reporting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @westonruter. That's a good idea to add the header only for users with a nonce.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied with a nonce, details to come shortly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AMP_Mutation_Utils::add_header() now has a check for authorization via self::is_authorized(). That verifies the nonce and capability.

echo $output; // WPCS: xss ok.
}

/**
Expand Down Expand Up @@ -785,6 +787,9 @@ public static function prepare_response( $response ) {
'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat.
'use_document_element' => true,
);
if ( AMP_Mutation_Utils::is_authorized() ) {
$args['mutation_callback'] = 'AMP_Mutation_Utils::track_removed';
}

// First ensure the mandatory amp attribute is present on the html element, as otherwise it will be stripped entirely.
if ( ! $dom->documentElement->hasAttribute( 'amp' ) && ! $dom->documentElement->hasAttribute( '⚡️' ) ) {
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-audio-sanitizer.php
Expand Up @@ -81,7 +81,7 @@ public function sanitize() {
* @see: https://github.com/ampproject/amphtml/issues/2261
*/
if ( 0 === $new_node->childNodes->length && empty( $new_attributes['src'] ) ) {
$node->parentNode->removeChild( $node );
$this->remove_child( $node );
} else {
$node->parentNode->replaceChild( $new_node, $node );
}
Expand Down
42 changes: 42 additions & 0 deletions includes/sanitizers/class-amp-base-sanitizer.php
Expand Up @@ -304,4 +304,46 @@ public function maybe_enforce_https_src( $src, $force_https = false ) {

return $src;
}

/**
* Removes a child of a node.
*
* Also, calls the mutation callback for it.
* This tracks all the nodes that were removed.
*
* @since 0.7
*
* @param DOMNode $child The node to remove.
* @return void.
*/
public function remove_child( $child ) {
if ( method_exists( $child->parentNode, 'removeChild' ) ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
$child->parentNode->removeChild( $child ); // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar.
if ( isset( $this->args['mutation_callback'] ) ) {
call_user_func( $this->args['mutation_callback'], $child, 'removed' );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe AMP_Mutation_Util::NODE_REMOVED could be used here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit uses that constant instead of the string.

}
}
}

/**
* Removes an attribute of a node.
*
* Also, calls the mutation callback for it.
* This tracks all the attributes that were removed.
*
* @since 0.7
*
* @param DOMNode $node The node for which to remove the attribute.
* @param string $attribute The attribute to remove from the node.
* @return void.
*/
public function remove_attribute( $node, $attribute ) {
if ( method_exists( $node, 'removeAttribute' ) ) {
$node->removeAttribute( $attribute );
if ( isset( $this->args['mutation_callback'] ) ) {
call_user_func( $this->args['mutation_callback'], $node, 'removed_attr', $attribute );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe AMP_Mutation_Util::ATTRIBUTE_REMOVED could be used here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit from above substitutes the constant.

}
}
}

}
16 changes: 8 additions & 8 deletions includes/sanitizers/class-amp-blacklist-sanitizer.php
Expand Up @@ -73,13 +73,13 @@ private function strip_attributes_recursive( $node, $bad_attributes, $bad_protoc
$attribute = $node->attributes->item( $i );
$attribute_name = strtolower( $attribute->name );
if ( in_array( $attribute_name, $bad_attributes, true ) ) {
$node->removeAttribute( $attribute_name );
$this->remove_attribute( $node, $attribute_name );
continue;
}

// The on* attributes (like onclick) are a special case.
if ( 0 === stripos( $attribute_name, 'on' ) && 'on' !== $attribute_name ) {
$node->removeAttribute( $attribute_name );
$this->remove_attribute( $node, $attribute_name );
continue;
} elseif ( 'a' === $node_name ) {
$this->sanitize_a_attribute( $node, $attribute );
Expand Down Expand Up @@ -112,10 +112,10 @@ private function strip_tags( $node, $tag_names ) {
for ( $i = $length - 1; $i >= 0; $i-- ) {
$element = $elements->item( $i );
$parent_node = $element->parentNode;
$parent_node->removeChild( $element );
$this->remove_child( $element );

if ( 'body' !== $parent_node->nodeName && AMP_DOM_Utils::is_node_empty( $parent_node ) ) {
$parent_node->parentNode->removeChild( $parent_node );
$this->remove_child( $parent_node );
}
}
}
Expand All @@ -134,13 +134,13 @@ private function sanitize_a_attribute( $node, $attribute ) {
$old_value = $attribute->value;
$new_value = trim( preg_replace( self::PATTERN_REL_WP_ATTACHMENT, '', $old_value ) );
if ( empty( $new_value ) ) {
$node->removeAttribute( $attribute_name );
$this->remove_attribute( $node, $attribute_name );
} elseif ( $old_value !== $new_value ) {
$node->setAttribute( $attribute_name, $new_value );
}
} elseif ( 'rev' === $attribute_name ) {
// rev removed from HTML5 spec, which was used by Jetpack Markdown.
$node->removeAttribute( $attribute_name );
$this->remove_attribute( $node, $attribute_name );
} elseif ( 'target' === $attribute_name ) {
// _blank is the only allowed value and it must be lowercase.
// replace _new with _blank and others should simply be removed.
Expand All @@ -150,7 +150,7 @@ private function sanitize_a_attribute( $node, $attribute ) {
$node->setAttribute( $attribute_name, '_blank' );
} else {
// Only _blank is allowed.
$node->removeAttribute( $attribute_name );
$this->remove_attribute( $node, $attribute_name );
}
}
}
Expand Down Expand Up @@ -219,7 +219,7 @@ private function replace_node_with_children( $node, $bad_attributes, $bad_protoc

// Remove the node from the parent, if defined.
if ( $node->parentNode ) {
$node->parentNode->removeChild( $node );
$this->remove_child( $node );
}
}

Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-form-sanitizer.php
Expand Up @@ -85,7 +85,7 @@ public function sanitize() {
$node->setAttribute( 'action', $action_url );
}
} elseif ( 'post' === $method ) {
$node->removeAttribute( 'action' );
$this->remove_attribute( $node, 'action' );
if ( ! $xhr_action ) {
// record that action was converted tp action-xhr.
$action_url = add_query_arg( '_wp_amp_action_xhr_converted', 1, $action_url );
Expand Down
7 changes: 4 additions & 3 deletions includes/sanitizers/class-amp-iframe-sanitizer.php
Expand Up @@ -74,7 +74,7 @@ public function sanitize() {
* @see: https://github.com/ampproject/amphtml/issues/2261
*/
if ( empty( $new_attributes['src'] ) ) {
$node->parentNode->removeChild( $node );
$this->remove_child( $node );
continue;
}

Expand All @@ -95,11 +95,11 @@ public function sanitize() {
$parent_node->replaceChild( $new_node, $node );
} else {
// AMP does not like iframes in <p> tags.
$parent_node->removeChild( $node );
$this->remove_child( $node );
$parent_node->parentNode->insertBefore( $new_node, $parent_node->nextSibling );

if ( AMP_DOM_Utils::is_node_empty( $parent_node ) ) {
$parent_node->parentNode->removeChild( $parent_node );
$this->remove_child( $parent_node );
}
}
}
Expand Down Expand Up @@ -193,4 +193,5 @@ private function build_placeholder( $parent_attributes ) {

return $placeholder_node;
}

}
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-img-sanitizer.php
Expand Up @@ -74,7 +74,7 @@ public function sanitize() {
}

if ( ! $node->hasAttribute( 'src' ) || '' === $node->getAttribute( 'src' ) ) {
$node->parentNode->removeChild( $node );
$this->remove_child( $node );
continue;
}

Expand Down
9 changes: 6 additions & 3 deletions includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
Expand Up @@ -704,6 +704,9 @@ private function sanitize_disallowed_attributes_in_node( $node, $attr_spec_list

foreach ( $attrs_to_remove as $attr ) {
$node->removeAttributeNode( $attr );
if ( isset( $this->args['mutation_callback'], $attr->name ) ) {
call_user_func( $this->args['mutation_callback'], $node, 'removed_attr', $attr->name );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe AMP_Mutation_Util::ATTRIBUTE_REMOVED could be used here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit from above substitutes the constant.

}
}
}

Expand Down Expand Up @@ -809,7 +812,7 @@ private function delegated_sanitize_disallowed_attribute_values_in_node( $node,
( true === $attr_spec_list[ $attr_name ][ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_EMPTY ] ) ) {
$node->setAttribute( $attr_name, '' );
} else {
$node->removeAttribute( $attr_name );
$this->remove_attribute( $node, $attr_name );
}
}
}
Expand Down Expand Up @@ -1448,13 +1451,13 @@ private function remove_node( $node ) {
*/
$parent = $node->parentNode;
if ( $node && $parent ) {
$parent->removeChild( $node );
$this->remove_child( $node );
}
while ( $parent && ! $parent->hasChildNodes() && $this->root_element !== $parent ) {
$node = $parent;
$parent = $parent->parentNode;
if ( $parent ) {
$parent->removeChild( $node );
$this->remove_child( $node );
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-video-sanitizer.php
Expand Up @@ -93,7 +93,7 @@ public function sanitize() {
* See: https://github.com/ampproject/amphtml/issues/2261
*/
if ( 0 === $new_node->childNodes->length && empty( $new_attributes['src'] ) ) {
$node->parentNode->removeChild( $node );
$this->remove_child( $node );
} else {
$node->parentNode->replaceChild( $new_node, $node );
}
Expand Down