Auto-reject excessive CSS validation errors #3346

schlessera · 2019-09-25T16:17:14Z

The AMP_Validation_Manager::is_sanitization_auto_accepted() now accepts an $error as argument and auto-rejects the specific case of an excessive CSS validation error.

In addition, the Tag & Attribute sanitizer specifically ignores the <style amp-custom> tag when it comes to the max_bytes rule in the spec (introduced in #3084).

Finally, to ensure that the resulting <style amp-custom> is not stripped itself for excessive size, the tag & attribute sanitizer treats this tag as a special case for enforcing the 'max_bytes' spec rule.

Fixes #2326

The `AMP_Validation_Manager::is_sanitization_auto_accepted()` now accepts an `$error` as argument and auto-rejects the specific case of an excessive CSS validation error. In addition, the Tag & Attribute sanitizer specifically ignores the `<style amp-custom>` tag when it comes to the `max_bytes` rule in the spec.

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

Co-Authored-By: Weston Ruter <westonruter@google.com>

schlessera · 2019-09-25T16:47:43Z

Lots of broken tests for this change, will have to go through them and see what exact Pandora's box I actually opened here... ;)

westonruter · 2019-09-25T16:52:38Z

Lots of broken tests for this change, will have to go through them and see what exact Pandora's box I actually opened here... ;)

It may be just a matter of replacing add_theme_support('amp') with add_theme_support('amp', ['paired' => true]), in the tests.

westonruter · 2019-09-25T16:53:49Z

Alternatively, if the return value of is_sanitization_auto_accepted can be filtered, then the tests could could be updated to just __return_true for that filter.

schlessera · 2019-09-25T16:58:10Z

if the return value of is_sanitization_auto_accepted can be filtered

Is that something we should add anyway?

westonruter · 2019-09-25T17:02:11Z

I think so. I think it makes sense to be able to give the ability to filter whether a given validation error is auto-accepted. This would be needed when removing the auto-accept checkbox, ultimately.

schlessera · 2019-09-26T02:02:16Z

There's only one failing test now: https://travis-ci.org/ampproject/amp-wp/jobs/589550895#L1035-L1044

The 'invalid_element' that is not being stripped anymore is very specifically is the <style amp-custom> in here:

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

Lines 1818 to 1836 in 5888398

    
           		$bad_dev_mode_document = sprintf( 
        
           			' 
        
           				<html amp data-ampdevmode> 
        
           					<head> 
        
           						<meta charset="utf-8"> 
        
           						<style amp-custom data-ampdevmode>%s</style> 
        
           					</head> 
        
           					<body> 
        
           						<amp-state id="something"> 
        
           							<script type="application/json" data-ampdevmode>%s</script> 
        
           						</amp-state> 
        
           						<button data-ampdevmode onclick="alert(\'Hello!\')"></button> 
        
           						<script data-ampdevmode>document.write("Hello World!")</script> 
        
           					</body> 
        
           				</html> 
        
           				', 
        
           			'button::before { content:"' . str_repeat( 'a', 50001 ) . '";"}', 
        
           			'{"foo":"' . str_repeat( 'b', 100001 ) . '"}' 
        
           		);

The current logic is ignoring 'max_bytes' for <style amp-custom> tags so they don't get removed: https://github.com/ampproject/amp-wp/pull/3346/files#diff-ee41b32e64c90445a71328ca93cfa799R676

I could of course change the test now, but I wonder if there's no way to make the edge case I'm overriding more specific so that the test wouldn't break here. I can't think of a way though.

@westonruter Thoughts?

schlessera · 2019-09-26T02:08:07Z

The only way I can think of here without modifying the tests is to check from within the Tag & Attribute sanitizer whether we have NEW_REJECTED validation errors in the current document. However:

I'm not yet sure this would reliably work in terms of timing.
It would tightly couple the Tag & Attribute sanitizer to the current WordPress data model, which we want to avoid.

So for now I'm going ahead and changing that test instead, but let's discuss further as needed.

…case handling

schlessera · 2019-09-26T03:28:22Z

I think so. I think it makes sense to be able to give the ability to filter whether a given validation error is auto-accepted. This would be needed when removing the auto-accept checkbox, ultimately.

As I don't actually need it here, I'll open a separate issue for that.

Link: #3350

schlessera · 2019-09-26T03:32:07Z

I added tests for:

asserting that excessive_style is rejected by default (validation manager)
<style amp-custom> is not removed if it exceeds 'max_bytes' (tag & attribute sanitizer)

I think this should cover the current behavior. The style sanitizer did not require any changes. However, right now, the CSS manifest comment does not contain any added information. I'm not sure if we should add something there or not.

kienstra · 2019-09-26T07:36:19Z

Looks Good

Hi @schlessera,
This looks good.

On testing this with Query Monitor to ensure the CSS is above 50 KB, the excessive_css errors were auto-rejected in Standard mode:

...and the <amp-custom> didn't have any styling removed:

Before this PR, the Query Monitor CSS was removed, as it put the CSS over the 50 KB limit.

kienstra · 2019-09-26T06:38:26Z

includes/validation/class-amp-validation-manager.php

 	 * @return bool Whether sanitization is forcibly accepted.
 	 */
-	public static function is_sanitization_auto_accepted() {
-		return amp_is_canonical() || AMP_Options_Manager::get_option( 'auto_accept_sanitization' );
+	public static function is_sanitization_auto_accepted( $error = null ) {


Good idea to accept an optional $error parameter.

schlessera added Sanitizers CSS labels Sep 25, 2019

googlebot added the cla: yes Signed the Google CLA label Sep 25, 2019

Make PHPCS happy

38a0a9d

westonruter reviewed Sep 25, 2019

View reviewed changes

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

Rearrange edge case conditionals

2c25146

westonruter reviewed Sep 25, 2019

View reviewed changes

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

Use hasAttribute( $node )

a1e6979

Co-Authored-By: Weston Ruter <westonruter@google.com>

Fix logic in is_sanitization_auto_accepted()

9d88367

schlessera added 3 commits September 26, 2019 04:10

Remove amp-custom from devmode max-bytes test to not hit the special …

beeeb07

…case handling

Add test to assert that excessive css in amp custom is not stripped

27472e2

Add test to assert that auto-sanitization is disabled for excessive-css

a0943ef

schlessera mentioned this pull request Sep 26, 2019

Remove validation handling options with the checkbox to disable auto-accepting sanitization #3350

Closed

schlessera marked this pull request as ready for review September 26, 2019 03:40

schlessera requested review from westonruter and kienstra September 26, 2019 03:40

kienstra approved these changes Sep 26, 2019

View reviewed changes

schlessera changed the title ~~[WIP] Auto-reject excessive CSS validation errors~~ Auto-reject excessive CSS validation errors Sep 26, 2019

schlessera mentioned this pull request Sep 26, 2019

Fix bug in style sanitizer not freeing up memory because of wrong index #3349

Merged

schlessera added 2 commits September 26, 2019 16:50

Make PHPCS happy

6029aee

Rectify tests

2b236e7

westonruter added this to the v1.3.1 milestone Oct 8, 2019

westonruter merged commit e564bb8 into develop Oct 8, 2019

westonruter deleted the fix/2326-discontinue-excluding-excessive-css-in-standard-mode branch October 8, 2019 02:20

schlessera mentioned this pull request Oct 9, 2019

AMP mode and plugin/theme versions need to be included in validated environment setting to determine whether stale #3474

Closed

swissspidy modified the milestones: v1.3.1, v1.4 Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-reject excessive CSS validation errors #3346

Auto-reject excessive CSS validation errors #3346

schlessera commented Sep 25, 2019 •

edited

Loading

schlessera commented Sep 25, 2019

westonruter commented Sep 25, 2019 •

edited

Loading

westonruter commented Sep 25, 2019

schlessera commented Sep 25, 2019

westonruter commented Sep 25, 2019

schlessera commented Sep 26, 2019

schlessera commented Sep 26, 2019

schlessera commented Sep 26, 2019 •

edited

Loading

schlessera commented Sep 26, 2019 •

edited

Loading

kienstra commented Sep 26, 2019

kienstra Sep 26, 2019

Auto-reject excessive CSS validation errors #3346

Auto-reject excessive CSS validation errors #3346

Conversation

schlessera commented Sep 25, 2019 • edited Loading

schlessera commented Sep 25, 2019

westonruter commented Sep 25, 2019 • edited Loading

westonruter commented Sep 25, 2019

schlessera commented Sep 25, 2019

westonruter commented Sep 25, 2019

schlessera commented Sep 26, 2019

schlessera commented Sep 26, 2019

schlessera commented Sep 26, 2019 • edited Loading

schlessera commented Sep 26, 2019 • edited Loading

kienstra commented Sep 26, 2019

kienstra Sep 26, 2019

Choose a reason for hiding this comment

schlessera commented Sep 25, 2019 •

edited

Loading

westonruter commented Sep 25, 2019 •

edited

Loading

schlessera commented Sep 26, 2019 •

edited

Loading

schlessera commented Sep 26, 2019 •

edited

Loading