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

Report removing unrecognized elements. #1287

Merged
merged 2 commits into from
Jul 28, 2018
Merged

Report removing unrecognized elements. #1287

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8476111
Remove unrecognized elements and report.
Jul 25, 2018
65c9cde
Test replace_node_with_children validation errors.
Jul 27, 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
57 changes: 38 additions & 19 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 @@ -85,6 +85,13 @@ class AMP_Tag_And_Attribute_Sanitizer extends AMP_Base_Sanitizer {
*/
protected $script_components = array();

/**
* Keep track of nodes that should not be replaced to prevent duplicated validation errors since sanitization is rejected.
*
* @var array
*/
protected $should_not_replace_nodes = array();

/**
* AMP_Tag_And_Attribute_Sanitizer constructor.
*
Expand Down Expand Up @@ -412,7 +419,7 @@ private function process_node( $node ) {
$attr_spec_list = $first_spec[ AMP_Rule_Spec::ATTR_SPEC_LIST ];
}
}
} // End if().
}

if ( ! empty( $attr_spec_list ) && $this->is_missing_mandatory_attribute( $attr_spec_list, $node ) ) {
$this->remove_node( $node );
Expand Down Expand Up @@ -1656,34 +1663,46 @@ private function get_ancestor_with_matching_spec_name( $node, $ancestor_spec_nam
* Also adds them to the stack for processing by the sanitize() function.
*
* @since 0.3.3
* @since 1.0 Fix silently removing unrecognized elements.
* @see https://github.com/Automattic/amp-wp/issues/1100
*
* @param DOMNode $node Node.
*/
private function replace_node_with_children( $node ) {

// If node has no children or no parent, just remove the node.
if ( ! $node->hasChildNodes() || ! $node->parentNode ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should not there be a $this->should_sanitize_validation_error( array(), compact( 'node' ) ) call in here as well? Otherwise the removal won't be reported as a validation error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@westonruter the if code block flows through to remove_node() where it’s handled. I did confirm it there. When I write the error unit tests, I’ll validate that too to ensure we have reporting for both paths.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're right. \AMP_Tag_And_Attribute_Sanitizer::remove_node() will call \AMP_Base_Sanitizer::remove_invalid_child() which reports the validation error. 👍

// If node has no children or no parent, just remove the node.
$this->remove_node( $node );

} else {
/*
* If node has children, replace it with them and push children onto stack
*
* Create a DOM fragment to hold the children
*/
$fragment = $this->dom->createDocumentFragment();

// Add all children to fragment/stack.
$child = $node->firstChild;
while ( $child ) {
$fragment->appendChild( $child );
$this->stack[] = $child;
$child = $node->firstChild;
}
return;
}

// Replace node with fragment.
$node->parentNode->replaceChild( $fragment, $node );
/*
* If node has children, replace it with them and push children onto stack
*
* Create a DOM fragment to hold the children
*/
$fragment = $this->dom->createDocumentFragment();

// Add all children to fragment/stack.
$child = $node->firstChild;
while ( $child ) {
$fragment->appendChild( $child );
$this->stack[] = $child;
$child = $node->firstChild;
}

// Prevent double-reporting nodes that are rejected for sanitization.
if ( isset( $this->should_not_replace_nodes[ $node->nodeName ] ) && in_array( $node, $this->should_not_replace_nodes[ $node->nodeName ], true ) ) {
return;
}

// Replace node with fragment.
$should_replace = $this->should_sanitize_validation_error( array(), compact( 'node' ) );
if ( $should_replace ) {
$node->parentNode->replaceChild( $fragment, $node );
} else {
$this->should_not_replace_nodes[ $node->nodeName ][] = $node;
}
}

Expand Down