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

[WIP] Add dynamic handling of validation errors #1063

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
1 participant
@westonruter
Copy link
Member

westonruter commented Apr 6, 2018

Follow up on #1048 (comment)

I want to follow up later with some more improvements including the added ability to knowingly bypass removal of elements, attributes, and styles that are invalid but which a given site cannot afford to be removed. I'm thinking we'd want CSS over the 50KB limit to default to not be removed, even if that means it is invalid AMP. In terms of implementation here, I think this can be implemented by allowing the validation_error_callback to return false as a way to prevent removal from happening. That would allow a theme/plugin to decide on a case-by-case basis which things should get removed. This is in regards to #1048 (review) and #1048 (comment)

There are a few validation errors that should specifically be not sanitized by default since they could seriously impact a site:

  • Keyframe properties that aren't hardware accelerated.
  • CSS exceeding 50KB.
  • External stylesheets that can't be inlined.

In addition to allowing the user to manually acknowledge whether a given validation error should be considered critical or be ignored (#1003), there also needs to be a way to dynamically make this decision based on a user-supplied validation callback, a capability which would also be used in #1087 to check whether a validation error is a deal breaker for serving AMP for the current page.

Todo

  • When a deal-breaker validation error occurs, prevent AMP from being served from a given URL.
  • Make sure that error codes are distinct enough to be differentiated.
  • Make sure that validation errors still display when editing a post.
  • Make sure that sources are located when not serving validation errors from cache (pass $node to methods).
  • 🚫 Decide on which validation errors should not result in sanitization by default (if any).
  • 🚫 Consider having a AMP admin setting for whether to allow select validation errors when they occur.
  • 🚫 Look at factoring in sanitization-skipping to paired mode, to serve canonical dirty AMP documents but valid non-canonical (amphtml) ones. This is similar to incorporating prerendering into the postprocessing flow (#958).

Closes #956

@westonruter westonruter added this to the v1.0 milestone Apr 6, 2018

Allow the validation_error_callback to return false to prevent saniti…
…zation where relevant

* Move debug code into AMP_Validation_Utils class.
* Remove disable_invalid_removal sanitizer arg in favor of validation_error_callback

@westonruter westonruter force-pushed the add/conditional-validation branch from 615ff1f to 73f4373 Apr 6, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Apr 6, 2018

@pbakaus Are there other such validation errors that come to mind which would leave a site worse off than sanitizing them from the response?

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Apr 6, 2018

As of the current state of this PR the following is how a theme could on its own opt-in to allow illegal CSS properties (such as in keyframe animations):

add_filter( 'amp_content_sanitizers', function( $sanitizer_classes ) {
	foreach ( array_keys( $sanitizer_classes ) as $sanitizer_class ) {
		$original_callback = null;
		if ( isset( $sanitizer_classes[ $sanitizer_class ]['validation_error_callback'] ) ) {
			$original_callback = $sanitizer_classes[ $sanitizer_class ]['validation_error_callback'];
		}

		$sanitizer_classes[ $sanitizer_class ]['validation_error_callback'] = function( $validation_error ) use ( $original_callback ) {
			$retval = call_user_func( $original_callback, $validation_error );
			if ( isset( $validation_error['code'] ) && 'illegal_css_property' === $validation_error['code'] ) {
				return false;
			}
			return $retval;
		};
	}
	return $sanitizer_classes;
}, 20 );

Naturally this could be made more user friendly, such as with a filter.


UPDATE: Now it looks like this:

add_filter( 'amp_content_sanitizers', function( $sanitizer_classes ) {
	foreach ( $sanitizer_classes as &$args ) {
		$args['validation_error_callback'] = function( $validation_error ) {
			if ( isset( $validation_error['code'] ) && 'illegal_css_property' === $validation_error['code'] ) {
				return false;
			}
			return true;
		};
	}
	return $sanitizer_classes;
} );

@westonruter westonruter changed the title [WIP] Allow sanitization for validation errors to be skipped [WIP] Allow sanitization for select validation errors to be skipped Apr 6, 2018

Improve handling of validation errors in style sanitizer
* Run amp_content_sanitizers filter late to apply after sanitizers have been added.
* Wrap any existing validation_error_callback functions which have been supplied to do the handling of the validation error.
* Pass flag to style sanitizer to locate_sources to ensure sources are available for reporting when they'll be needed.
* Add handle_validation_error and set_current_node methods to reduce duplication.
* Fix inclusion of property_value in validation_error.
* Pass debug flag to AMP_Validation_Utils upon init.
@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Apr 20, 2018

👉 Update: On second thought, the ability for a validation callback to prevent the removal of something that is invalid should not be allowed. Rather, when validation errors occur and these validation errors are not acknowledged as being ignorable, then AMP should be prevented from being served altogether. See #1003 (comment)

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Apr 20, 2018

Ah, rather than letting the validation callback suppress sanitization of the error, the callback could instead be used to determine whether or not the validation error is “critical”, and when the first such error is encountered, switch to serving non-AMP.

@westonruter westonruter changed the title [WIP] Allow sanitization for select validation errors to be skipped [WIP] Add dynamic handling of validation errors Apr 20, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Apr 25, 2018

Commits cherry-picked into #1093.

@westonruter westonruter deleted the add/conditional-validation branch Apr 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.