-
Notifications
You must be signed in to change notification settings - Fork 382
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
Changes from 31 commits
0a44284
e9f394a
d7104b8
616262f
4714062
e209005
aed76c6
30c666f
76b0f17
0b7c3fc
71744e5
da408bf
ab3909a
90090b0
83dac87
70521ef
cc4fe85
0b6a4ee
db49a33
c92bee4
6b5593b
a168a5c
a52ae05
0472a33
6d2350f
3de38b8
46e7084
972ac2c
d2ab9ee
cb0cfc9
084acaa
13ab280
f996673
f8aeca8
69317b8
583a6bc
cd910ff
a6d071a
b40729b
9954c11
15d9186
bb7a175
29caa8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, this commit from above substitutes the constant. |
||
} | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, this commit from above substitutes the constant. |
||
} | ||
} | ||
} | ||
|
||
|
@@ -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 ); | ||
} | ||
} | ||
} | ||
|
@@ -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 ); | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 viaself::is_authorized()
. That verifies the nonce and capability.