Improve validating sanitizer with context for why element/attribute is invalid #3780

westonruter · 2019-11-16T04:35:00Z

Summary

Introduce fine-grained error codes. Instead of just invalid_element when a required attribute is missing, the more specific error ATTR_REQUIRED_BUT_MISSING is raised which explains why it is invalid. Similarly, when an attribute value is incorrect, instead of invalid_attribute occurring when an attribute value violates a regex pattern, a more specific INVALID_ATTR_VALUE_REGEX pattern is raised. Methods which check for whether a node is valid now return an error code on failure rather than just false. These fine-grained error codes will be used in a subsequent PR to add detailed messages, though at the moment just codes are used.
Remove redundant checks inAMP_Tag_And_Attribute_Sanitizer::validate_attr_spec_list_for_node() which prevent subsequent ability to raise specific error codes while sanitizing attributes.
Discontinue premature removal of element which misses a mandatory attribute.
Align error codes with those used by the actual AMP validator, where possible.
The underlying spec_name is included in the validation error, allowing in a future PR to link a validation error directly to the spec which defines specifically why it is invalid.
Add assertions when generating the spec to ensure the expected spec names are present, that all spec names are unique (which will be needed later), and that the error_message for blacklisted_cdata_regex is as expected (since we map to an error code).
Update conversion sanitizers (image, video, audio, iframe) to emit ATTR_REQUIRED_BUT_MISSING validation errors when src is missing.
Make AMP_Tag_And_Attribute_Sanitizer::sanitize_disallowed_attribute_values_in_node read only, deferring removal or emptying of invalid attribute to AMP_Base_Sanitizer::remove_invalid_attribute.
Similarly, discontinue removing invalid descendants in AMP_Tag_And_Attribute_Sanitizer::validate_attr_spec_list_for_node() (which should be read-only) to a later point.
Merge redundant get_disallowed_attributes_in_node into AMP_Tag_And_Attribute_Sanitizer::sanitize_disallowed_attribute_values_in_node.
Eliminate missing_body_element validation error in Style sanitizer in favor of just adding the body.
Make AMP_Tag_And_Attribute_Sanitizer::validate_attr_spec_list_for_node() always return integer scores, rather than use a float for the special case.
Improve tests for AMP_Tag_And_Attribute_Sanitizer by eliminating duplicated assertions and adding error code checking to each test.
Fix handling of special case URL __amp_source_origin, preventing it from being interpreted as a relative URL.

See #1420.

For Subsequent PRs

Rework data model in class-amp-allowed-tags-generated.php to be more efficient to look up a tag spec by spec_name. See Refactor generated spec data to facilitate looking up by spec name #3817.
Add translatable error messages for each specific error code, removing the logic from \AMP_Validation_Error_Taxonomy::get_error_title_from_code() and into the respective sanitizer that originated the validation error. Improve structure of validation error to make it easier to construct messages. For example, harmonize node_attributes and element_attributes to just element_attributes. See Improve architecture of composing error messages for each validation error code #4071.
Send validation error messages back in the amp_validity REST API field to use in the block warnings, as opposed to duplicating the error message logic. See Improve displaying validation errors' invalid markup in Gutenberg block warning notice #3664.
Include error details in validation error when failing to fetch external stylesheets. See Not enough information given when fetch_external_stylesheet failure happened #2449.
Raise separate DISALLOWED_PROPERTY_IN_ATTR_VALUE validation errors for each invalid property, and update the value when sanitization is accepted. See Add AMP_DOM_Document & meta tag sanitizer #3758 (comment) and Add validation of individual properties in meta content attributes #4070.

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).

schlessera

I haven't looked at everything in detail yet, but added a few general comments and questions about the approach.

schlessera · 2019-11-26T15:04:38Z

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

@@ -537,13 +551,15 @@ public function prepare_validation_error( array $error = [], array $data = [] )

 		if ( $node instanceof DOMElement ) {
 			if ( ! isset( $error['code'] ) ) {
-				$error['code'] = AMP_Validation_Error_Taxonomy::INVALID_ELEMENT_CODE;
+				$error['code'] = AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_TAG;


The base class retrieves internals from one of its extending classes.

Can we extract all of the validation error constants into a separate class? I tend to create an interface with only constants in such a case. An interface with constants better communicates what is happening: two different pieces of code have a contract about how to refer to a specific problem. Contracts should be encoded in interface, not use internal logic of one another.

namespace Amp\AmpWP; interface ValidationError { const DISALLOWED_TAG = 'DISALLOWED_TAG'; const DISALLOWED_CHILD_TAG = 'DISALLOWED_CHILD_TAG'; const DISALLOWED_FIRST_CHILD_TAG = 'DISALLOWED_FIRST_CHILD_TAG'; // [...] }

Thi is separate of any inheritance chain, and provides one central place where you retrieve constants from. The interface name also makes the syntax nicer to read:

$error['code'] = ValidationError::DISALLOWED_TAG;

If we need to have more grouping, we can have multiple interfaces, like CssValidationError, HtmlValidationError, ...?

Good point. On one hand, I think this should just be removed entirely, and let the code be UNKNOWN_ERROR. Each sanitizer should be responsible for indicating the code, specifically.

Otherwise, I think this should be scoped with what you discussed in #3780 (comment). It's bound up with generating PHP objects from the spec.

So I think we should do it, but probably not in the scope of this PR.

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

schlessera · 2019-11-26T15:34:10Z

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

+				$error_code = self::INVALID_BLACKLISTED_VALUE_REGEX;
+			} 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?


If you have 100 properties, you'd love to know exactly which ones are invalid and which ones are valid.

Yes. Either the invalid properties could be communicated as a property of the one validation error, or there could be multiple validation errors each for a single property. That's the question here. I think the latter is better.

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

westonruter · 2019-11-26T18:57:52Z

Rebasing after large PR merged into develop…

…c_list_for_node

This reverts commits 8ae5c3b and 0b30e4a.

westonruter · 2019-11-29T22:35:06Z

@schlessera There are 3 error codes left which do not have test assertions:

CDATA_VIOLATES_BLACKLIST: The first one should never occur because it will only happen if the switch statement passes through all cases, which we currently catch when parsing the spec.
CSS_SYNTAX_PARSE_ERROR: I am not aware of this being a possible error code in practice because the PHP CSS Parser is configured with “LenientParsing” enabled: “When used (which is true by default), the parser will not choke on unexpected tokens but simply ignore them.”
STYLESHEET_INVALID_FILE_PATH: I couldn't find a way to cause this error scenario.

…id-markup-reason * 'develop' of github.com:ampproject/amp-wp: Pull the built `block-libray` package from Gutenberg SVN if it does not exists (#3847) Update dependency @babel/plugin-transform-react-jsx to v7.7.4 (#3688) Update dependency @babel/plugin-proposal-class-properties to v7… (#3687) Update dependency @babel/cli to v7.7.4 (#3685) Update dependency browserslist to v4.7.3 (#3792) Update dependency postcss to v7.0.23 (#3791) Update dependency autoprefixer to v9.7.2 (#3679) For the Gallery block, use the recommended amp-lightbox-gallery

schlessera · 2019-12-03T13:36:14Z

STYLESHEET_INVALID_FILE_PATH does not make much sense in our current context. Could it be that this only relates to Optimized AMP SSR?

schlessera · 2019-12-03T15:38:53Z

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

@@ -82,7 +82,7 @@ abstract class AMP_Base_Sanitizer {
 	 *
 	 * @var array
 	 */
-	private $should_not_removed_nodes = [];
+	private $nodes_to_keep = [];


Oh, maybe I should mention I searched for usage first:

https://wpdirectory.net/search/01DV66Q5TGAA54FBQJA46N9D6R

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

schlessera · 2019-12-03T16:43:20Z

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

@@ -1466,6 +1518,10 @@ private function check_attr_spec_rule_disallowed_relative( DOMElement $node, $at
 		if ( isset( $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_RELATIVE ] ) && ! $attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_RELATIVE ] ) {
 			if ( $node->hasAttribute( $attr_name ) ) {
 				foreach ( $this->extract_attribute_urls( $node->getAttributeNode( $attr_name ) ) as $url ) {
+					if ( '__amp_source_origin' === $url ) {


Maybe use a constant for this as well?

schlessera · 2019-12-03T16:43:31Z

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

@@ -1486,6 +1542,10 @@ private function check_attr_spec_rule_disallowed_relative( DOMElement $node, $at
 				foreach ( $attr_spec_rule[ AMP_Rule_Spec::ALTERNATIVE_NAMES ] as $alternative_name ) {
 					if ( $node->hasAttribute( $alternative_name ) ) {
 						foreach ( $this->extract_attribute_urls( $node->getAttributeNode( $alternative_name ), $attr_name ) as $url ) {
+							if ( '__amp_source_origin' === $url ) {


Same here, use of a constant makes sense.

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

tests/php/test-amp-tag-and-attribute-sanitizer-private-methods.php

tests/php/test-tag-and-attribute-sanitizer.php

schlessera · 2019-12-03T17:08:21Z

Did a quick search, and no one seems to be using the constants in AMP_Validation_Error_Taxonomy, so BC should be fine.

@param

…id-markup-reason * 'develop' of github.com:ampproject/amp-wp: (143 commits) Update dependency @wordpress/block-editor to v3.3.0 (#3691) Update dependency @wordpress/editor to v9.8.0 (#3693) Update dependency @wordpress/compose to v3.8.0 (#3736) Use comment as array key for data set to show when failure happens Remove unused AMP_YouTube_Embed_Handler::sanitize_v_arg method after 7a97571 Bump stylesheet cache group after #3866 (#3880) Delete AMP_YouTube_Embed_Handler::shortcode() and oembed() Delete AMP_Twitter_Embed_Handler::oembed() Prevent wrapping plugin names in code tags Update dependency @wordpress/blocks to v6.8.0 (#3734) Update dependency @wordpress/core-data to v2.8.0 (#3737) Update dependency @wordpress/edit-post to v3.9.0 (#3692) Update dependency @wordpress/components to v8.4.0 (#3735) Update dependency @wordpress/element to v2.9.0 (#3741) Align @param descriptions in test_video_override Replace a call to ->shortcode() with the logic from shortcode() Refactor AMP_Vimeo_Embed_Handler::shortcode() into video_override() Deprecate AMP_YouTube_Embed_Handler::shortcode() Restore AMP_YouTube_Embed_Handler::video_override() Improve theme inline CSS checks ...

Co-Authored-By: Alain Schlesser <[email protected]>

Remove STYLESHEET_INVALID_FILE_PATH code and always fall-back to HTTP request when file not on file system

…s as strings Co-Authored-By: Alain Schlesser <[email protected]>

westonruter · 2019-12-06T16:17:10Z

STYLESHEET_INVALID_FILE_PATH does not make much sense in our current context. Could it be that this only relates to Optimized AMP SSR?

Not really. It's an error that was returned (sometimes) when a file looked like it was on the filesystem but it could not be read. However, I've now eliminated this in 827659a by always falling back to an HTTP request when the file can't seem to be found on the filesystem.

kienstra · 2019-12-09T17:35:51Z

If it's alright, tomorrow might be the earliest I could review this.

schlessera · 2020-02-14T09:37:32Z

@westonruter You've moved this back into Ready for Review, but looking at it, it seems to be merged and there was no mention of this needing additional changes. Is this Done?

westonruter · 2020-02-14T18:27:18Z

I hadn't moved it to Done yet because #1420 isn't done yet and I wanted to make sure the other follow-ups had been filed as issues. They have now, so I'll move this to done.

googlebot added the cla: yes Signed the Google CLA label Nov 16, 2019

westonruter added this to the v1.5 milestone Nov 16, 2019

westonruter force-pushed the add/invalid-markup-reason branch from a909107 to aa129aa Compare November 22, 2019 23:22

westonruter mentioned this pull request Nov 22, 2019

Add AMP_DOM_Document & meta tag sanitizer #3758

Merged

13 tasks

westonruter force-pushed the add/invalid-markup-reason branch 2 times, most recently from a2ff9d1 to e5ef273 Compare November 26, 2019 13:47

westonruter marked this pull request as ready for review November 26, 2019 14:39

westonruter requested a review from schlessera November 26, 2019 14:39

westonruter mentioned this pull request Nov 26, 2019

Refactor generated spec data to facilitate looking up by spec name #3817

Closed

5 tasks

schlessera reviewed Nov 26, 2019

View reviewed changes

westonruter added 17 commits November 26, 2019 10:57

Eliminate premature/redundant attribute checks from validate_attr_spe…

bf28199

…c_list_for_node

Remove redundant is_missing_mandatory_attribute check

656ebb0

Consolidate removal of elements without mandatory attributes

a7f2750

Introduce illegal_cdata error code

6475b70

Fix return value docs for validate_attr_spec_list_for_node

ad440fa

Reuse check_attr_spec_rule_mandatory in is_missing_mandatory_attribute

3f2f50e

Introduce specific error codes for attribute value violations

3bb3d8e

Include list of missing mandatory attributes in validation error

ad66b8f

Extract AMP validator error codes and messages from spec

35638c5

Use constant in switch statement

a2e20d6

Fix removing emptyable-attributes and fix is_missing_mandatory_attribute

ef80051

Add constants for error codes; improve CDATA error reporting

2d6ec3d

Add fine-grained error codes for CDATA

00eb55c

Revert "Extract AMP validator error codes and messages from spec"

a6d0fc9

This reverts commits 8ae5c3b and 0b30e4a.

Fix validation of __amp_source_origin URL value

15fb6dd

Eliminate duplicated testing; add code checking

c55c58c

Add DISALLOWED_DESCENDANT_TAG error code

7f4bf4e

westonruter mentioned this pull request Dec 2, 2019

Jetpack Custom CSS error message broken in AMP #3868

Closed

schlessera mentioned this pull request Dec 3, 2019

Use objects for validation errors #3875

Closed

Bring sanity to the code

322035b

schlessera reviewed Dec 3, 2019

View reviewed changes

schlessera requested changes Dec 3, 2019

View reviewed changes

westonruter and others added 4 commits December 6, 2019 19:17

Fix typos in comments and code style

b96e4e7

Co-Authored-By: Alain Schlesser <[email protected]>

Harmonize logic for getting stylesheet by URL

827659a

Remove STYLESHEET_INVALID_FILE_PATH code and always fall-back to HTTP request when file not on file system

Use SORT_REGULAR flag for array_unique() instead of serializing array…

f7d00f9

…s as strings Co-Authored-By: Alain Schlesser <[email protected]>

westonruter requested review from schlessera, pierlon and kienstra December 6, 2019 16:17

schlessera approved these changes Dec 11, 2019

View reviewed changes

westonruter merged commit 199bd78 into develop Dec 12, 2019

westonruter mentioned this pull request Jan 12, 2020

Improve architecture of composing error messages for each validation error code #4071

Closed

schlessera self-assigned this Feb 13, 2020

schlessera deleted the add/invalid-markup-reason branch March 7, 2020 06:22

westonruter mentioned this pull request Mar 19, 2020

Add missing error titles for error codes #4405

Merged

4 tasks

westonruter added the Changelogged label Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve validating sanitizer with context for why element/attribute is invalid #3780

Improve validating sanitizer with context for why element/attribute is invalid #3780

westonruter commented Nov 16, 2019 •

edited

Loading

schlessera left a comment

schlessera Nov 26, 2019

westonruter Nov 26, 2019

schlessera Nov 26, 2019

westonruter Nov 26, 2019

westonruter commented Nov 26, 2019

westonruter commented Nov 29, 2019

schlessera commented Dec 3, 2019

schlessera Dec 3, 2019

schlessera Dec 3, 2019

schlessera Dec 3, 2019

schlessera Dec 3, 2019

schlessera commented Dec 3, 2019

westonruter commented Dec 6, 2019

kienstra commented Dec 9, 2019

schlessera commented Feb 14, 2020

westonruter commented Feb 14, 2020

Improve validating sanitizer with context for why element/attribute is invalid #3780

Improve validating sanitizer with context for why element/attribute is invalid #3780

Conversation

westonruter commented Nov 16, 2019 • edited Loading

Summary

For Subsequent PRs

Checklist

schlessera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Nov 26, 2019

westonruter commented Nov 29, 2019

schlessera commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlessera commented Dec 3, 2019

westonruter commented Dec 6, 2019

kienstra commented Dec 9, 2019

schlessera commented Feb 14, 2020

westonruter commented Feb 14, 2020

westonruter commented Nov 16, 2019 •

edited

Loading