Validation errors are not reported for “i-amphtml” appearing in CSS

[anand-ammathil]

The text (CDATA) inside tag 'style amp-custom' contains 'CSS i-amphtml- name prefix', which is disallowed.

[anand-ammathil]

it's my mistake. i added .i-amphtml in custom css

[Milor123]

@anand-a6 Thank you very much, it solved my problem. with:

• Remove all the .i-amphtml in the CSS of AMP

[JavedKhanUI]

@anand-a6 Thanks solved my problem.

[westonruter]

This is something that the AMP plugin could actually catch and remove automatically, while reporting a validation error. It hasn't come up frequently, however, so I don't know if it is something that we should explicitly account for.

[westonruter]

Note that the CSS tree shaker will be pre-emptively removing such i-amphtml-* class names because they are added dynamically with JS by the AMP framework. So this is not something that sites would normally encounter. Nevertheless, if a site is configured to prevent such class names from being removed, then this issue could occur.

For example, if someone pasted some HTML copied from an AMP page via DevTools, this could include elements with the i-amphtml-* class names, and if they do that, then this would prevent the class names from being tree-shaken. (Note that implementing Transformed AMP in #958 will cause WordPress to generate element with these class names up-front, but they will be added after the tree shaker runs in the style sanitizer, so it wouldn't come into play here.)

Here is some example code to cause the validation error which is not currently being caught by the AMP plugin:

add_action(
	'wp_footer',
	static function () {
		?>
		<style type="text/css" class="illegal-amphtml-classes">.i-amphtml-layout { outline: solid 1px red; }</style>
		<span class="i-amphtml-layout">i-amphtml-layout</span>
		<?php
	}
);

With this plugin code, WordPress is successfully removing the invalid class attribute because of the i-amphtml-layout:

The class is removed after the tree-shaker runs, so the style rule including .i-amphtml-layout is not removed. But what is confusing me is that the style[amp-custom] element is not getting marked as a validation error because of the presence of .i-amphtml-layout, and this leaks a validation error to the frontend:

I was expecting this to get flagged as an error in AMP_Tag_And_Attribute_Sanitizer::validate_cdata_for_node() via blacklisted_cdata_regex.

See the validator spec for style[amp-custom]:

    # These regex blacklists are temporary hacks to validate essential CSS
    # rules. They will be replaced later with more principled solutions.
    blacklisted_cdata_regex: {
      regex: "(^|\\W)i-amphtml-"
      error_message: "CSS i-amphtml- name prefix"
    }
    blacklisted_cdata_regex: {
      regex: "!\\s*important"
      error_message: "CSS !important"
    }

Nevertheless, the generated spec data only includes the second blacklisted_cdata_regex:

amp-wp/includes/sanitizers/class-amp-allowed-tags-generated.php

Lines 15327 to 15333 in 35ca34f

	'cdata' => array(
	'blacklisted_cdata_regex' => array(
	'error_message' => 'CSS !important',
	'regex' => '!\\s*important',
	),
	'css_spec' => array(
	'allowed_at_rules' => array(

So there is also a bug with amphtml-update.py here. See #4319.

[westonruter]

So validation errors that should be getting raised which aren't at present:

1. The tag-and-attribute sanitizer should be removing the style[amp-custom] when it contains i-amphtml.
2. Even better, the style sanitizer should raise validation errors proactively to remove each CSS selector up front that contain i-amphtml, even before the tag-and-attribute sanitizer encounters the serialized CSS in the style[amp-custom].

[westonruter]

Note: If an i-amphtml-* attributes appear in HTML class attribute, then a validation error should be raised for each instance and if the status of the invalid markup is “removed” then just the i-amphtml-* classes should be removed, not the entire class attribute.

[westonruter]

Note: In the same way that the tag-and-attribute-stanizer is allowing Custom CSS that is larger than 75KB (since the style sanitizer is handling it) we should also update the tag-and-attribute sanitizer to allow i-amphtml- prefixes. In other words, we can ignore this constraint when we know that the style sanitizer has run:

https://github.com/ampproject/amphtml/blob/01ba647457f8e12f9f7e0a5a23fb72d28c6e63f4/validator/validator-css.protoascii#L580-L585

Otherwise, if the invalid markup (the style containing the i-amphtml- prefix) were marked as kept from the style sanitizer, then they would get removed by the tag-and-attribute-sanitizer (along with all the custom CSS).

[westonruter]

QA Passed. Results here: #5356 (comment)