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

westonruter
Copy link
Member

@westonruter westonruter 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 Fix dispatch_key handling for NAME_VALUE_DISPATCH; add cdata validation #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:

Fixes #875.

@westonruter westonruter added this to the v0.7 milestone Feb 4, 2018
@kienstra
Copy link
Contributor

kienstra commented Feb 4, 2018

Review In Progress

Hi @westonruter,
I'm reviewing this now.

Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* Remove newly-unused dispatch key constant.
* Use integers for flags.
* Use constants instead of string literals.
@ThierryA
Copy link
Collaborator

ThierryA 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants