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

Add full document sanitization in theme support #929

Merged
merged 17 commits into from Feb 5, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

commented Feb 4, 2018

  • Add sanitization of the entire html document not just the `body.
  • Add support for enqueueing font stylesheets from spec-allowed sources.
  • Rewrite amphtml-update.py to use PHP for generation of array literals as opposed to using the previous buggy Python functions.
  • Remove useless amp4ads from generated PHP.
  • Enforce that the <html> element has the amp attribute.
  • Enforce that there is a meta charset and meta viewport, after page is rendered and sanitization is complete. Wait to set rel=canonical link until page is rendered.
  • De-duplicate boilerplate CSS; print boilerplate CSS in same routing that does custom CSS.
  • Remove just-added dispatch_key logic from #926 since turns out to not be necessary.
  • Eliminate hard-coding of keyframes max_size.
  • Ensure styles in head get sanitized like styles output in body.

Todo:

  • Add some more unit tests.
  • Add validation of meta viewport..
  • Make tweaks for review on #926

Fixes #875.

@westonruter westonruter added this to the v0.7 milestone Feb 4, 2018

@westonruter westonruter requested review from ThierryA and kienstra Feb 4, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Feb 4, 2018

Review In Progress

Hi @westonruter,
I'm reviewing this now.

@kienstra
Copy link
Collaborator

left a comment

Approved
Made Minor Point

Hi @westonruter,
This pull request looks good. Phpize seems to have simplified creating the PHP in amphtml-update.py.

I made a minor point, but this is approved.

echo self::CUSTOM_STYLES_PLACEHOLDER; // WPCS: XSS OK.
echo '</style>';
public static function add_amp_styles_placeholder() {
echo self::STYLES_PLACEHOLDER; // WPCS: XSS OK.

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 4, 2018

Collaborator

This looks to have the same output:

echo wp_kses_post( self::STYLES_PLACEHOLDER );

Of course, this is a minor point, and not a blocker.

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 4, 2018

Author Member

Thanks. Since this is a constant it doesn't need escaping.

westonruter added some commits Feb 5, 2018

Update use of constants in AMP_Rule_Spec
* Remove newly-unused dispatch key constant.
* Use integers for flags.
* Use constants instead of string literals.

@westonruter westonruter merged commit 574b253 into develop Feb 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/full-document-sanitization branch Feb 5, 2018

@ThierryA

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2018

I absolutely love these changes ❤️

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.