Raise validation errors for properties in attributes by pierlon · Pull Request #4137 · ampproject/amp-wp

pierlon · 2020-01-19T21:18:18Z

Summary

This PR allows for separate validation errors to be raised when an attribute has invalid properties, missing mandatory properties and missing required property values. Also, suitable titles for their error codes have been added, along with the AMP_Tag_And_Attribute_Sanitizer::ATTR_REQUIRED_BUT_MISSING error code.

Fixes #4070.

Invalid properties

If there are invalid properties within an attribute, each occurrence will be sanitized. For example, given the meta tag:

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

The following validation errors will be raised:

Missing mandatory properties

If a mandatory property within an attribute is missing, the attribute will be removed. For example, given the meta tag:

<meta name="viewport" content="initial-scale=1, user-scalable=no,">

A validation error will be raised for the missing width property, as well as for the content attribute being removed:

Note: This scenario can only be replicated by disabling AMP_Meta_Sanitizer so that it will not set the correct width property value. Currently, only the width property requires for a specific value to be set.

Missing required property values

When a property does not have the required value, the attribute will be removed. For example, given the meta tag:

<meta http-equiv="X-UA-Compatible" content="IE=9,chrome=1">

A validation error will be raised as the IE property does not have the required value edge, and the content attribute will be removed:

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).
Add unit tests for validation error properties.
Ensure tests are accounting for the various scenarios.
Ensure validation error messages are fully populated for the various error scenarios.

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

westonruter · 2020-01-22T19:59:26Z

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

+						$validation_error['attr_property_name'] = $error_data['property'];
+					} elseif ( self::MISSING_REQUIRED_PROPERTY_VALUE === $error_code ) {
+						$validation_error['attr_property_name']  = $error_data['property'];
+						$validation_error['attr_property_value'] = $error_data['value'];


Instead of attr_property_* what about just property_*? I don't feel strongly about this. If you have a reason for the attr_ prefix I'm happy with that.

I chose to prefix attr_ because property_name and property_value were already in use in the style sanitizer. I suppose renaming those instances to css_property_* can be done to resolve any ambiguity.

Renamed property_* to css_property_* and attr_property_* to property_* in 73d04ff.

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

pierlon · 2020-01-26T04:31:40Z

While reviewing the postmag theme, the following validation error can be seen in the AMP validator:

The property 'width' in attribute 'content' in tag 'meta name=viewport' is set to ' device-width', which is invalid.

Note the space prepended to the value device-width.

The plugin does not show this validation error because the property value is trimmed before comparison to the required value. This can be solved in two ways:

Compare the untrimmed value, which would cause the MISSING_REQUIRED_PROPERTY_VALUE validation error to occur
Save the trimmed value to the attribute to resolve the error

Thoughts @westonruter?

westonruter · 2020-01-26T23:55:39Z

The meta viewport in question in postmag is:

<meta name = "viewport" content = "width = device-width, initial-scale = 1.0" >

I can see this is indeed a problem for the AMP Validator:

Nevertheless, both Chrome and Firefox seem to read the viewport properties just fine with the extra whitespace. So this appears to perhaps be a bug with the AMP Validator.

Therefore, perhaps the right solution now is to “save the trimmed value to the attribute to resolve the error”. I'll open an issue on the AMP project to see if this is a bug. If so, the saved-trimming could then be removed. (See ampproject/amphtml#26496.)

westonruter · 2020-01-29T23:31:40Z

includes/validation/class-amp-validation-error-taxonomy.php

 				return __( 'Type', 'amp' );
 			case 'sources':
 				return __( 'Sources', 'amp' );
+			case 'property_name':


Should this be renamed to meta_property_name then?

Alternatively, the $validation_error['code'] can be used to differentiate which property type is in view here. And that could avoid the need to use css_property_name. Thoughts?

Personally, I prefer renaming it to meta_property_name to be more descriptive.

Done via fa1299e.

westonruter · 2020-01-29T23:34:18Z

includes/sanitizers/class-amp-meta-sanitizer.php


-		// @todo The invalid/missing properties should raise validation errors. See <https://github.com/ampproject/amp-wp/pull/3758/files#r348074703>.
 		foreach ( $viewport_settings as $index => $viewport_setting ) {
 			list( $property, $value ) = array_map( 'trim', explode( '=', $viewport_setting ) );


A missing change is to put the trimmed $property and $value into the $viewport_settings, correct? This would then address #4137 (comment)

I made the change to AMP_Tag_And_Attribute_Sanitizer in c4a5581 before seeing this. It definitely would be suited for AMP_Meta_Sanitizer.

Done via b257819.

This reverts commit c4a5581

westonruter · 2020-01-30T07:40:08Z

Ready for re-review?

pierlon · 2020-01-30T15:33:19Z

Yep!

…nt/4070-validate-props * 'develop' of github.com:ampproject/amp-wp: (108 commits) Update dependency semver to v7.1.2 (#4206) Update dependency @babel/core to v7.8.4 (#4199) Prevent errors in admin bar filters from non-array arguments (#4207) Discontinue preserving original style[amp-custom] Update dependency terser-webpack-plugin to v2.3.4 (#4189) Update dependency @babel/cli to v7.8.4 (#4198) Update dependency browserslist to v4.8.6 (#4196) Update dependency dealerdirect/phpcodesniffer-composer-installer to v0.6.2 (#4186) Split Reader mode style template part stylesheet from action-supplied styles Remove needless $xmlns_value_to_strip variable Fix PHPCS error Commit Weston's suggestion to use documentElement Use $html->hasAttributes to possibly exit early. If the html[lang] is "", overwrite it with the xml:lang Conditionally remove html[xmlns], and convert html[xml:lang] to html[lang] If head[profile=], strip profile, but don't create a new link Revert 7f60c5c (If there's already a link[rel=profile], don't create another Fix phpcs issues in failed Travis build If there's already a link[rel=profile], don't create another Convert head[profile] attribute to a link[rel=profile] ...

westonruter

@pierlon I'm seeing some discrepancies with what you indicate is expected.

Invalid properties (❓)

I'm seeing:

Sanitized as:

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

Nevertheless if I try:

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

There is no validation error for width=BAD:

Missing mandatory properties (❌)

I see no validation errors reported. Why?

Sanitized as:

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

Are validation errors actually even expected here? The AMP_Meta_Sanitizer ensures that a width property is supplied.

Missing required property values (✅)

This is matching the above screenshot.

Sanitized as:

-<meta http-equiv="X-UA-Compatible" content="IE=9,chrome=1">
+null

pierlon · 2020-02-03T04:03:08Z

Invalid properties

There is no validation error for width=BAD

In that case AMP_Meta_Sanitizer ensures the proper width property value is set before being validated.

Missing mandatory properties

I see no validation errors reported. Why?

I forgot to mentioned I had disabled AMP_Meta_Sanitizer to produce that scenario. There isn't any property value in the spec that has a required mandatory value, with width being the exception.

Edit: I've updated the description with a note explaining how the the error can be replicated.

westonruter · 2020-02-04T02:12:43Z

There's one more thing that isn't expected, and that is if an invalid property value is provided for viewport width. For example, if I add:

<meta name="viewport" content="width=600, initial-scale=1.0" >

Then this gets sanitized as:

<meta name="viewport" content="width=device-width,initial-scale=1.0">

And no validation error is raised. This is is an issue with the AMP_Meta_Sanitizer where it is silently replacing the value with something that is allowed.

westonruter · 2020-02-04T17:42:28Z

I'm not sure what the best resolution is for this. If the AMP_Meta_Sanitizer raises a validation error due to width=600, the behavior of the sanitizer should logically replace this with width=device-width when the status of the error is “removed”. However, if the user decides to make the status “kept” then the AMP_Tag_And_Attribute_Sanitizer will come along later and remove invalid width=600 property without replacing it with the required width=device-width. And the result would be a meta viewport that lacks the required property. It's a similar situation we have for the style sanitizer for when in Standard mode and the style sanitizer allows for the total CSS to go over 50KB. In that case we special-case the condition in the AMP_Tag_And_Attribute_Sanitizer:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Lines 807 to 817 in 19d54bd

    
           if ( 
        
           	isset( $cdata_spec['max_bytes'] ) && strlen( $element->textContent ) > $cdata_spec['max_bytes'] 
        
           	&& 
        
           	// Skip the <style amp-custom> tag, as we want to display it even with an excessive size if it passed the style sanitizer. 
        
           	// This would mean that AMP was disabled to not break the styling. 
        
           	! ( 'style' === $element->nodeName && $element->hasAttribute( 'amp-custom' ) ) 
        
           ) { 
        
           	return [ 
        
           		'code' => self::CDATA_TOO_LONG, 
        
           	]; 
        
           }

Which isn't great, but I can't think of a better solution.

pierlon · 2020-02-05T05:20:04Z

Would there be a case where a user would want to use a specific width? The page can't be AMP compatible unless it being set to device-width.

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

includes/validation/class-amp-validation-error-taxonomy.php

schlessera · 2020-02-14T17:23:13Z

I noticed that the source is not filled in my case. I edited the header.php file in the twentytwenty theme to change the viewport.

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

…alidate-props # Conflicts: # includes/validation/class-amp-validation-error-taxonomy.php

westonruter · 2020-02-15T22:21:30Z

I noticed that the source is not filled in my case. I edited the header.php file in the twentytwenty theme to change the viewport.

@schlessera This would be expected. If markup is not generated by a hook callback then no source information would be available. In the past the source stack info would show the theme as a fallback, followed by "(?)". That wasn't very helpful either, and it wasn't reliable as I recall.

pierlon · 2020-02-15T23:00:31Z

If markup is not generated by a hook callback then no source information would be available.

That was my understanding as well, thanks for clarifying.

Use array_map() instead of array_reduce()

…ag and attribute sanitizer

westonruter · 2020-02-17T00:03:26Z

includes/sanitizers/class-amp-meta-sanitizer.php

-			return;
-		}
-
-		// Ensure we have the 'width=device-width' setting included.
-		/**
-		 * Viewport tag.
-		 *
-		 * @var DOMElement $viewport_tag
-		 */
-		$viewport_tag      = $this->meta_tags[ self::TAG_VIEWPORT ][0];
-		$viewport_content  = $viewport_tag->getAttribute( 'content' );
-		$viewport_settings = array_map( 'trim', explode( ',', $viewport_content ) );
-		$width_found       = false;
-
-		// @todo The invalid/missing properties should raise validation errors. See <https://github.com/ampproject/amp-wp/pull/3758/files#r348074703>.
-		foreach ( $viewport_settings as $index => $viewport_setting ) {
-			list( $property, $value ) = array_map( 'trim', explode( '=', $viewport_setting ) );
-			if ( 'width' === $property ) {
-				if ( 'device-width' !== $value ) {
-					$viewport_settings[ $index ] = 'width=device-width';
-				}
-				$width_found = true;
-				break;
-			}
 		}
-
-		if ( ! $width_found ) {
-			array_unshift( $viewport_settings, 'width=device-width' );
-		}
-
-		$viewport_tag->setAttribute( 'content', implode( ',', $viewport_settings ) );


@schlessera I found that this code was no longer necessary now that required property values for all meta tags (not just meta[name=viewport]) in \AMP_Tag_And_Attribute_Sanitizer::process_node().

westonruter · 2020-02-17T06:38:57Z

Now given a head that contains:

<meta name="viewport" content="initial-scale=1.0" >
<meta http-equiv="X-UA-Compatible" content="IE=6,chrome=1,netscape=4">

Three validation errors are raised:

And with the errors being sanitized, the result is valid AMP:

<meta name="viewport" content="initial-scale=1.0,width=device-width">
<meta http-equiv="X-UA-Compatible" content="chrome=1">

westonruter · 2020-02-17T06:40:15Z

@schlessera @pierlon Please verify my additional changes and we'll merge.

pierlon · 2020-02-17T09:07:15Z

Added a more specific label for the "required property value" validation error:

Before:

After:

Otherwise from that, the prior changes made by @westonruter work as described, so ✔️ for me.

schlessera

I'm leaving my remaining code reviews issues as a mere comment here, as none of them are really obligatory.

includes/validation/class-amp-validation-error-taxonomy.php

schlessera · 2020-02-17T15:02:30Z

includes/validation/class-amp-validation-error-taxonomy.php

 							printf( '<code>%s</code>', esc_html( $value ) );
 						}
 						?>
+					<?php elseif ( 'meta_property_name' === $key ) : ?>


As we're using these array keys in multiple places, I would suggest using constants for them.

However, given that the long-term plan is to use objects for validation errors, this might just be a waste of effort at this point.

schlessera · 2020-02-17T15:04:14Z

tests/php/test-class-amp-meta-sanitizer.php

 				'<!DOCTYPE html><html><head><meta charset="utf-8"><meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"></head><body></body></html>',
 			],

+			// Trim whitespace between the properties and their values.


This file should be changed so that these single-line comments for each test case are converted into array keys. This way, PHPUnit would display the exact test case that is failing.

This reverts commit 953165b.

pierlon added 5 commits January 19, 2020 14:17

Raise separate validation errors for properties in an attribute

50c6c24

Format validation error for missing required properties

ec6ae77

Format validation error for required property value

5f6eaba

Format validation error for missing required attribute

439c863

Fix code formatting issues

9e840a6

googlebot added the cla: yes Signed the Google CLA label Jan 19, 2020

westonruter reviewed Jan 19, 2020

View reviewed changes

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved

westonruter reviewed Jan 22, 2020

View reviewed changes

pierlon added 2 commits January 24, 2020 03:00

Display invalid property value

014e272

Rename validation error keys to resolve any ambiguity that may arise

73d04ff

pierlon marked this pull request as ready for review January 24, 2020 08:12

pierlon requested a review from westonruter January 24, 2020 18:20

westonruter mentioned this pull request Jan 27, 2020

Meta viewport properties cause validation error when extra whitespace is present ampproject/amphtml#26496

Closed

westonruter reviewed Jan 29, 2020

View reviewed changes

pierlon added 4 commits January 30, 2020 01:07

Trim required property value if necessary

c4a5581

Revert "Trim required property value if necessary"

939394a

This reverts commit c4a5581

Set required property value from the meta sanitizer

b257819

Rename 'property_name' to 'meta_property_name'

fa1299e

pierlon requested a review from westonruter January 30, 2020 15:32

westonruter requested changes Feb 1, 2020

View reviewed changes

pierlon requested a review from westonruter February 3, 2020 04:15

pierlon requested a review from westonruter February 8, 2020 05:30

westonruter added this to the v1.5 milestone Feb 9, 2020

westonruter mentioned this pull request Feb 12, 2020

Add AMP Optimizer library #4019

Merged

9 tasks

schlessera requested changes Feb 14, 2020

View reviewed changes

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved

includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved

includes/validation/class-amp-validation-error-taxonomy.php Outdated Show resolved Hide resolved

pierlon and others added 3 commits February 15, 2020 15:25

Rename 'property_value' to 'meta_property_value'

bf8e7b5

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Make phpcs happy

b94b769

Merge remote-tracking branch 'origin/develop' into enhancement/4070-v…

75fc196

…alidate-props # Conflicts: # includes/validation/class-amp-validation-error-taxonomy.php

pierlon requested a review from schlessera February 15, 2020 23:00

westonruter added 3 commits February 16, 2020 09:09

Only update attribute if properties changed

4fe41f3

Use array_map() instead of array_reduce()

Handle case where multiple meta properties are invalid

c1c7c44

Remove required-property logic from meta sanitizer and flesh out in t…

8cf95cd

…ag and attribute sanitizer

westonruter reviewed Feb 17, 2020

View reviewed changes

Improve validation error details for invalid properties; improve tests

b0ae412

westonruter approved these changes Feb 17, 2020

View reviewed changes

pierlon added 2 commits February 17, 2020 03:21

Fix phpdoc

5f34302

Use more specific labels for MISSING_REQUIRED_PROPERTY_VALUE error

c7d84e3

schlessera reviewed Feb 17, 2020

View reviewed changes

schlessera approved these changes Feb 17, 2020

View reviewed changes

westonruter added 2 commits February 17, 2020 09:05

Use esc_html_e() rather than echoing esc_html()

953165b

Revert "Use esc_html_e() rather than echoing esc_html()"

67ad354

This reverts commit 953165b.

westonruter merged commit cd1e2bf into develop Feb 17, 2020

westonruter deleted the enhancement/4070-validate-props branch February 17, 2020 17:32

westonruter added the Changelogged label Mar 23, 2020

westonruter mentioned this pull request May 18, 2020

Meta viewport properties no longer need extraneous whitespace removed #4742

Closed

Conversation

pierlon commented Jan 19, 2020 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Invalid properties

Missing mandatory properties

Missing required property values

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierlon Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pierlon commented Jan 26, 2020

Uh oh!

westonruter commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jan 30, 2020

Uh oh!

pierlon commented Jan 30, 2020

Uh oh!

westonruter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Invalid properties (❓)

Missing mandatory properties (❌)

Missing required property values (✅)

Uh oh!

pierlon commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Invalid properties

Missing mandatory properties

Uh oh!

westonruter commented Feb 4, 2020

Uh oh!

westonruter commented Feb 4, 2020

Uh oh!

pierlon commented Feb 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schlessera commented Feb 14, 2020

Uh oh!

westonruter commented Feb 15, 2020

Uh oh!

pierlon commented Feb 15, 2020

Uh oh!

westonruter Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Feb 17, 2020

Uh oh!

westonruter commented Feb 17, 2020

Uh oh!

pierlon commented Feb 17, 2020

pierlon commented Jan 19, 2020 •

edited by westonruter

Loading

pierlon Jan 24, 2020 •

edited

Loading

westonruter commented Jan 26, 2020 •

edited

Loading

westonruter left a comment •

edited

Loading

pierlon commented Feb 3, 2020 •

edited

Loading

westonruter Feb 17, 2020 •

edited

Loading