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 validation of individual properties in meta content attributes #4070

Closed
westonruter opened this issue Jan 12, 2020 · 4 comments · Fixed by #4137
Closed

Add validation of individual properties in meta content attributes #4070

westonruter opened this issue Jan 12, 2020 · 4 comments · Fixed by #4137
Labels
QA passed Has passed QA and is done Sanitizers
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Jan 12, 2020

Feature description

Split out from #3758 via the conversation with @schlessera in #3758 (comment):

More generally, would it be possible to refactor the tag & meta sanitizer to strip the offending properties only, and then check if the entire thing is still valid or not? This way, we wouldn't have to duplicate this logic all over the place...

This is in relation to this code:

} elseif ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_PROPERTIES ] ) &&
AMP_Rule_Spec::FAIL === $this->check_attr_spec_rule_value_properties( $node, $attr_name, $attr_spec_rule ) ) {
// @todo Should there be a separate validation error for each invalid property?
$error_code = self::DISALLOWED_PROPERTY_IN_ATTR_VALUE; // @todo Which property(s) in particular?
}


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Adding invalid properties to a meta[viewport] should not result in the entire attribute being removed, if there are other valid properties.
  • A separate validation error should be raised for each invalid property in a meta[content] attribute.

Implementation brief

Instead of raising one single DISALLOWED_PROPERTY_IN_ATTR_VALUE for the entire meta[content] attribute and then causing the entire content attribute to be removed, we instead need to:

  1. Raise a DISALLOWED_PROPERTY_IN_ATTR_VALUE validation error for each individual property in the content attribute.
  2. Remove the invalid property from the content attribute if the validation error if \AMP_Base_Sanitizer::should_sanitize_validation_error() returns true.

What this means is that the \AMP_Tag_And_Attribute_Sanitizer::sanitize_disallowed_attribute_values_in_node() method will need to be augmented to not simply return a list of [ $attr_node, $error_code ] tuples. I the case of invalid properties, it will also need to return [ $attr_node, self::DISALLOWED_PROPERTY_IN_ATTR_VALUE, $property_name, $property_value ].

This resulting list of $disallowed_attributes will then need to be iterated over in the foreach ( $disallowed_attributes as $disallowed_attribute ) {…} here:

// Remove all invalid attributes.
if ( ! empty( $disallowed_attributes ) ) {
/*
* Capture all element attributes up front so that differing validation errors result when
* one invalid attribute is accepted but the others are still rejected.
*/
$validation_error = [
'element_attributes' => [],
];
foreach ( $node->attributes as $attribute ) {
$validation_error['element_attributes'][ $attribute->nodeName ] = $attribute->nodeValue;
}
$removed_attributes = [];
foreach ( $disallowed_attributes as $disallowed_attribute ) {
/**
* Returned vars.
*
* @var DOMAttr $attr_node
* @var string $error_code
*/
list( $attr_node, $error_code ) = $disallowed_attribute;
$validation_error['code'] = $error_code;
$attr_spec = isset( $merged_attr_spec_list[ $attr_node->nodeName ] ) ? $merged_attr_spec_list[ $attr_node->nodeName ] : [];
if ( $this->remove_invalid_attribute( $node, $attr_node, $validation_error, $attr_spec ) ) {
$removed_attributes[] = $attr_node;
}
}

Where instead of calling the \AMP_Base_Sanitizer::remove_invalid_attribute(), there would need to be a direct call to AMP_Base_Sanitizer::should_sanitize_validation_error() and if it returns true, update the value of the attribute to omit that invalid property.

QA testing instructions

  • Install the "Brovy" theme
  • Visit an AMP URL
  • Verify: The validation errors should contain a validation error "Invalid property value: user-scalable"

Demo

Changelog entry

@schlessera
Copy link
Collaborator

This would be one more case where the move to error objects instead of error strings&arrays would make sense. Error objects could have an arbitrary number of additional properties and associated logic.

@westonruter
Copy link
Member Author

This would be one more case where the move to error objects instead of error strings&arrays would make sense. Error objects could have an arbitrary number of additional properties and associated logic.

Agreed. However, We can implement this using primitives in the immediate term, and then we can let the requirements help inform the refactor for the next version.

@westonruter
Copy link
Member Author

A good theme to test with this is brovy. The meta viewport added by the theme is:

<meta name="viewport" content="width=device-width, initial-scale=1, user-scalabe=no">

So the invalid properties are causing the meta tag to be removed. This is wrong. Only the invalid properties should be removed.

@csossi
Copy link

csossi commented Feb 26, 2020

Verified in QA
image

@csossi csossi added the QA passed Has passed QA and is done label Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA passed Has passed QA and is done Sanitizers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants