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

Add missing error titles for error codes #4405

Merged
merged 14 commits into from
Mar 21, 2020
Merged

Add missing error titles for error codes #4405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4294c39
Generate PHP class with error codes from AMP specification
pierlon Mar 18, 2020
be03da4
Revert "Generate PHP class with error codes from AMP specification"
pierlon Mar 19, 2020
14dd042
Add error titles for error codes found in AMP specification
pierlon Mar 19, 2020
978f4e4
Add error titles for internal error codes
pierlon Mar 20, 2020
6e57c25
Make phpcs happy
pierlon Mar 20, 2020
29eea3c
Replace quotes with code tags
pierlon Mar 20, 2020
3c29860
Remove DISALLOWED_TAG_MULTIPLE_CHOICES validation error code
pierlon Mar 20, 2020
6a1dd8b
Make phpcs happy
pierlon Mar 20, 2020
4475925
Remove markup from translations
pierlon Mar 20, 2020
46b1849
Tighten up validation error titles
westonruter Mar 21, 2020
905ae4a
Fix MISSING_URL checks by inferring ALLOW_EMPTY from MANDATORY
westonruter Mar 21, 2020
feababc
Prevent case where validation error can be raised on already-removed …
westonruter Mar 21, 2020
3bb4f0f
Merge branch 'develop' of github.com:ampproject/amp-wp into enhanceme…
westonruter Mar 21, 2020
ceee80c
Fix passing of numbers to _n()
westonruter Mar 21, 2020
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
5 changes: 5 additions & 0 deletions includes/sanitizers/class-amp-base-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,11 @@ public function remove_invalid_child( $node, $validation_error = [] ) {

$should_remove = $this->should_sanitize_validation_error( $validation_error, compact( 'node' ) );
if ( $should_remove ) {
if ( null === $node->parentNode ) {
// Node no longer exists.
return $should_remove;
}

$node->parentNode->removeChild( $node );
} else {
$this->nodes_to_keep[ $node->nodeName ][] = $node;
Expand Down
93 changes: 66 additions & 27 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 @@ -27,7 +27,6 @@
class AMP_Tag_And_Attribute_Sanitizer extends AMP_Base_Sanitizer {

const DISALLOWED_TAG = 'DISALLOWED_TAG';
const DISALLOWED_TAG_MULTIPLE_CHOICES = 'DISALLOWED_TAG_MULTIPLE_CHOICES';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've went ahead and removed the DISALLOWED_TAG_MULTIPLE_CHOICES error code as I don't see a meaningful way of displaying the multiple validation errors on the page. Instead, each error will be sanitized separately which allows for each to be viewed.

const DISALLOWED_CHILD_TAG = 'DISALLOWED_CHILD_TAG';
const DISALLOWED_FIRST_CHILD_TAG = 'DISALLOWED_FIRST_CHILD_TAG';
const INCORRECT_NUM_CHILD_TAGS = 'INCORRECT_NUM_CHILD_TAGS';
Expand Down Expand Up @@ -58,7 +57,7 @@ class AMP_Tag_And_Attribute_Sanitizer extends AMP_Base_Sanitizer {
const INVALID_URL_PROTOCOL = 'INVALID_URL_PROTOCOL';
const INVALID_URL = 'INVALID_URL';
const DISALLOWED_RELATIVE_URL = 'DISALLOWED_RELATIVE_URL';
const DISALLOWED_EMPTY_URL = 'DISALLOWED_EMPTY_URL';
const MISSING_URL = 'MISSING_URL';
const INVALID_BLACKLISTED_VALUE_REGEX = 'INVALID_BLACKLISTED_VALUE_REGEX';
const DISALLOWED_PROPERTY_IN_ATTR_VALUE = 'DISALLOWED_PROPERTY_IN_ATTR_VALUE';
const MISSING_MANDATORY_PROPERTY = 'MISSING_MANDATORY_PROPERTY';
Expand Down Expand Up @@ -505,7 +504,15 @@ private function process_node( DOMElement $node ) {
array_unique(
array_map(
static function ( $validation_error ) {
unset( $validation_error['spec_name'] );
unset(
$validation_error['spec_name'],
// Remove other keys that may make the error unique.
$validation_error['required_parent_name'],
$validation_error['required_ancestor_name'],
$validation_error['required_child_count'],
$validation_error['required_min_child_count'],
$validation_error['required_attr_value']
);
return $validation_error;
},
$validation_errors
Expand All @@ -527,13 +534,11 @@ static function ( $validation_error ) {
);
} else {
// Otherwise, we have a rare condition where multiple tag specs fail for different reasons.
$this->remove_invalid_child(
$node,
[
'code' => self::DISALLOWED_TAG_MULTIPLE_CHOICES,
'errors' => $validation_errors,
]
);
foreach ( $validation_errors as $validation_error ) {
if ( true === $this->remove_invalid_child( $node, $validation_error ) ) {
break; // Once removed, ignore remaining errors.
}
}
Comment on lines -530 to +541
Copy link
Member

Choose a reason for hiding this comment

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

Given a Custom HTML block with the following markup:

<amp-accordion id="my-accordion" disable-session-states="">
  <section>
    <h2>Section 1</h2>
    <p>Content in section 12.</p>
  </section>
  <section>
    <h2>Section 2</h2>
    <div>Content in section 2.</div>
  </section>
  <section expanded="">
    <!--<h2>Section 3</h2>-->
    <amp-img src="/static/inline-examples/images/squirrel.jpg" width="320" height="256"></amp-img>
  </section>
</amp-accordion>

This is invalid because of the h2 being commented out. The amp-accordion component requires an h2 followed by a div or a p. The validation errors for this are not expected, however:

image

Notice the amp-img lacks the expected Custom HTML source.

I've fixed this here in feababc by breaking the loop of validation error reporting once a markup causing validation error is removed.

This will probably make this code no longer necessary:

amp-wp/includes/sanitizers/class-amp-base-sanitizer.php

Lines 477 to 481 in 3c29860

if ( null === $node->parentNode ) {
// Node no longer exists.
return $should_remove;
}

Though it doesn't hurt to have it in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice the amp-img lacks the expected Custom HTML source.

Good catch!

This will probably make this code no longer necessary:

In this context yes, but it could potentially prevent an error being thrown when attempting to remove a node multiple times.

}
}
return null;
Expand Down Expand Up @@ -993,14 +998,16 @@ private function validate_tag_spec_for_node( DOMElement $node, $tag_spec ) {

if ( ! empty( $tag_spec[ AMP_Rule_Spec::MANDATORY_PARENT ] ) && ! $this->has_parent( $node, $tag_spec[ AMP_Rule_Spec::MANDATORY_PARENT ] ) ) {
return [
'code' => self::WRONG_PARENT_TAG,
'code' => self::WRONG_PARENT_TAG,
'required_parent_name' => $tag_spec[ AMP_Rule_Spec::MANDATORY_PARENT ],
Copy link
Member

Choose a reason for hiding this comment

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

Note that at some point we will not need to capture the required_parent_name in the error. We could instead capture the spec_name alone. Then the spec_name could be looked up to obtain the required_parent_name (or other properties depending on the error). This will be made possible by #3817. However, that is not yet available, so what you are doing here is the perfect approach in the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I initially was retrieving it by looking up the tag spec in AMP_Allowed_Tags_Generated::class, but I realized it was available in the sanitizer so I took it from there.

This will be made possible by #3817.

Awesome, made a note of that.

];
}

// Extension scripts must be in the head. Note this currently never fails because all AMP scripts are moved to the head before sanitization.
if ( isset( $tag_spec['extension_spec'] ) && ! $this->has_parent( $node, 'head' ) ) {
return [
'code' => self::WRONG_PARENT_TAG,
'code' => self::WRONG_PARENT_TAG,
'required_parent_name' => 'head',
];
}

Expand All @@ -1017,7 +1024,8 @@ private function validate_tag_spec_for_node( DOMElement $node, $tag_spec ) {

if ( ! empty( $tag_spec[ AMP_Rule_Spec::MANDATORY_ANCESTOR ] ) && ! $this->has_ancestor( $node, $tag_spec[ AMP_Rule_Spec::MANDATORY_ANCESTOR ] ) ) {
return [
'code' => self::MANDATORY_TAG_ANCESTOR,
'code' => self::MANDATORY_TAG_ANCESTOR,
'required_ancestor_name' => $tag_spec[ AMP_Rule_Spec::MANDATORY_ANCESTOR ],
];
}

Expand Down Expand Up @@ -1275,9 +1283,9 @@ private function sanitize_disallowed_attribute_values_in_node( DOMElement $node,
} elseif ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ] ) &&
AMP_Rule_Spec::FAIL === $this->check_attr_spec_rule_valid_url( $node, $attr_name, $attr_spec_rule ) ) {
$attrs_to_remove[] = [ $attr_node, self::INVALID_URL, null ];
} elseif ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_EMPTY ] ) &&
} elseif ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ] ) &&
AMP_Rule_Spec::FAIL === $this->check_attr_spec_rule_disallowed_empty( $node, $attr_name, $attr_spec_rule ) ) {
$attrs_to_remove[] = [ $attr_node, self::DISALLOWED_EMPTY_URL, null ];
$attrs_to_remove[] = [ $attr_node, self::MISSING_URL, null ];
} elseif ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_RELATIVE ] ) &&
AMP_Rule_Spec::FAIL === $this->check_attr_spec_rule_disallowed_relative( $node, $attr_name, $attr_spec_rule ) ) {
$attrs_to_remove[] = [ $attr_node, self::DISALLOWED_RELATIVE_URL, null ];
Expand Down Expand Up @@ -1334,11 +1342,17 @@ private function is_valid_layout( $tag_spec, $node ) {
$input_height->validate( $allow_auto, $allow_fluid );

if ( ! $input_width->isValid() ) {
return [ 'code' => self::INVALID_LAYOUT_WIDTH ];
return [
'code' => self::INVALID_LAYOUT_WIDTH,
'attribute' => 'width',
];
}

if ( ! $input_height->isValid() ) {
return [ 'code' => self::INVALID_LAYOUT_HEIGHT ];
return [
'code' => self::INVALID_LAYOUT_HEIGHT,
'attribute' => 'height',
];
}

// No need to go further if there is no layout attribute.
Expand All @@ -1356,7 +1370,10 @@ private function is_valid_layout( $tag_spec, $node ) {

// Only FLEX_ITEM allows for height to be set to auto.
if ( $height->isAuto() && AMP_Rule_Spec::LAYOUT_FLEX_ITEM !== $layout ) {
return [ 'code' => self::INVALID_LAYOUT_AUTO_HEIGHT ];
return [
'code' => self::INVALID_LAYOUT_AUTO_HEIGHT,
'attribute' => 'height',
];
}

// FIXED, FIXED_HEIGHT, INTRINSIC, RESPONSIVE must have height set.
Expand All @@ -1369,12 +1386,19 @@ private function is_valid_layout( $tag_spec, $node ) {
) &&
! $height->isDefined()
) {
return [ 'code' => self::INVALID_LAYOUT_NO_HEIGHT ];
return [
'code' => self::INVALID_LAYOUT_NO_HEIGHT,
'attribute' => 'height',
];
}

// For FIXED_HEIGHT if width is set it must be auto.
if ( AMP_Rule_Spec::LAYOUT_FIXED_HEIGHT === $layout && $width->isDefined() && ! $width->isAuto() ) {
return [ 'code' => self::INVALID_LAYOUT_FIXED_HEIGHT ];
return [
'code' => self::INVALID_LAYOUT_FIXED_HEIGHT,
'attribute' => 'width',
'required_attr_value' => 'auto',
];
}

// FIXED, INTRINSIC, RESPONSIVE must have width set and not be auto.
Expand All @@ -1384,7 +1408,10 @@ private function is_valid_layout( $tag_spec, $node ) {
AMP_Rule_Spec::LAYOUT_RESPONSIVE === $layout
) {
if ( ! $width->isDefined() || $width->isAuto() ) {
return [ 'code' => self::INVALID_LAYOUT_AUTO_WIDTH ];
return [
'code' => self::INVALID_LAYOUT_AUTO_WIDTH,
'attribute' => 'width',
];
}
}

Expand All @@ -1401,7 +1428,11 @@ private function is_valid_layout( $tag_spec, $node ) {

// Heights attribute is only allowed for RESPONSIVE layout.
if ( ! $this->is_empty_attribute_value( $heights_attr ) && AMP_Rule_Spec::LAYOUT_RESPONSIVE !== $layout ) {
return [ 'code' => self::INVALID_LAYOUT_HEIGHTS ];
return [
'code' => self::INVALID_LAYOUT_HEIGHTS,
'attribute' => 'layout',
'required_attr_value' => 'responsive',
];
}

return true;
Expand Down Expand Up @@ -1973,7 +2004,12 @@ private function check_attr_spec_rule_disallowed_relative( DOMElement $node, $at
* is no rule for this attribute.
*/
private function check_attr_spec_rule_disallowed_empty( DOMElement $node, $attr_name, $attr_spec_rule ) {
if ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_EMPTY ] ) && ! $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_EMPTY ] && $node->hasAttribute( $attr_name ) ) {
if ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_EMPTY ] ) ) {
$allow_empty = $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_EMPTY ];
} else {
$allow_empty = empty( $attr_spec_rule[ AMP_Rule_Spec::MANDATORY ] );
}
if ( ! $allow_empty && $node->hasAttribute( $attr_name ) ) {
$attr_value = $node->getAttribute( $attr_name );
if ( empty( $attr_value ) ) {
return AMP_Rule_Spec::FAIL;
Expand Down Expand Up @@ -2336,6 +2372,7 @@ private function remove_disallowed_descendants( DOMElement $node, $allowed_desce
[
'code' => self::DISALLOWED_DESCENDANT_TAG,
'allowed_descendants' => $allowed_descendants,
'disallowed_ancestor' => $node->parentNode->nodeName,
'spec_name' => $spec_name,
]
);
Expand Down Expand Up @@ -2395,8 +2432,9 @@ private function check_valid_children( DOMElement $node, $child_tags ) {
return true;
} else {
return [
'code' => self::INCORRECT_NUM_CHILD_TAGS,
'children_count' => $child_element_count,
'code' => self::INCORRECT_NUM_CHILD_TAGS,
'children_count' => $child_element_count,
'required_child_count' => $child_tags['mandatory_num_child_tags'],
];
}
}
Expand All @@ -2408,8 +2446,9 @@ private function check_valid_children( DOMElement $node, $child_tags ) {
return true;
} else {
return [
'code' => self::INCORRECT_MIN_NUM_CHILD_TAGS,
'children_count' => $child_element_count,
'code' => self::INCORRECT_MIN_NUM_CHILD_TAGS,
'children_count' => $child_element_count,
'required_min_child_count' => $child_tags['mandatory_min_num_child_tags'],
];
}
}
Expand Down
Loading