Malformed CSS properties leak AMP validation error past the sanitizer

[westonruter]

Bug Description

When a theme or plugin adds CSS like the following:

body {
	transition: all .3s ease-in-out;
	-webkit-transition: all .3s ease-in-out;
	-moz-transition: all .3s ease-in-out;
	-0-transition: all .3s ease-in-out;
}

Notice the -0-transition. This causes an AMP validation error:

CSS syntax error in tag 'style amp-custom' - invalid declaration.

The same thing happens in a theme like organic-lite (on WordPress.org) which has instances of an invalid property 4z-index:1;.

Expected Behaviour

Such invalid CSS properties should be removed by the style sanitizer.

Steps to reproduce

1. Add a Custom HTML block with the above CSS in a <style>...</style> tag.
2. Publish the post.
3. View the AMP version.
4. See an AMP validation error.

Screenshots

Additional context

• WordPress version:
• Plugin version:
• Gutenberg plugin version (if applicable):
• AMP plugin template mode:
• PHP version:
• OS:
• Browser: [e.g. chrome, safari]
• Device: [e.g. iPhone6]

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• No AMP validation error should occur on the frontend. The invalid property should either be removed entirely by the PHP-CSS-Parser, or else the invalid property should raise an internal validation error.

Implementation brief

It is not clear to me whether this should be fixed in the AMP plugin or rather in PHP-CSS-Parser. I think it actually should be addressed in the latter because such CSS is actually a syntax error:

So I think the PHP-CSS-Parser should be updated to strip such invalid properties from the parsed rules.

QA testing instructions

1. Add a Custom HTML block with the following CSS:

body {
	transition: all .3s ease-in-out;
	-webkit-transition: all .3s ease-in-out;
	-moz-transition: all .3s ease-in-out;
	-0-transition: all .3s ease-in-out;
}

2. Publish the post.
3. View the AMP version.
4. Verify: The CSS does not generate a validation error.

Demo

Changelog entry

[pierlon]

I think a CSS validator would be needed here to ensure any form of malformed CSS is not leaked. Unfortunately there isn't any PHP CSS validator library (that I could find) that would fit the requirements here, and the one being used by W3C is in Java.

The easiest fix I think may be to get a list of valid CSS rule sets to validate against for now.

[westonruter]

The PHP-CSS-Parser should be handling this during parsing. It has a loose parsing mode that recovers from parsing errors, and I think this should fall into that category and be stripped.

The issue is just that the property doesn't match the required pattern.

[westonruter]

Here's the PHP-CSS-Parser code in question: https://github.com/sabberworm/PHP-CSS-Parser/blob/a80a97d3fdb2164d72cbf0fb3cfc40b1d630a877/lib/Sabberworm/CSS/Rule/Rule.php#L34-L38

I think the problem is that $oParserState->parseIdentifier() is too liberal in what it considers to be an identifier. See code here: https://github.com/sabberworm/PHP-CSS-Parser/blob/2875198419445d6d98b4e320996e0ab81e3e3e22/lib/Sabberworm/CSS/Parsing/ParserState.php#L51-L113

In particular, the parseCharacter method is problematic here when the $bIsForIdentifier arg is passed: https://github.com/sabberworm/PHP-CSS-Parser/blob/2875198419445d6d98b4e320996e0ab81e3e3e22/lib/Sabberworm/CSS/Parsing/ParserState.php#L98-L108

Note the ranges allowed:

// Ranges: a-z A-Z 0-9 - _

So this means that parseCharacter will return any character in that range. I think that parseIdentifier then needs to be updated to throw an exception of the sequence of parsed characters is illegal according to the CSS spec. A property is an identifier (ident) which is defined as being:

ident	= ?{nmstart}{nmchar}*
nmstart	= [_a-z]|{nonascii}|{escape}
nmchar	= [_a-z0-9-]|{nonascii}|{escape}

So PHP-CSS-Parser is not taking into account the restriction in nmstart.

[pierlon]

Ah I see. I'll review this to ensure it fixes the reported bug here and make a PR to sabberworm/PHP-CSS-Parser with the fix.

[pierlon]

I've opened MyIntervals/PHP-CSS-Parser/pull/185 to address this.

[csossi]

Verified in QA