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.

Sign up for GitHub

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

Jump to bottom

Add full document sanitization in theme support #929

Merged
merged 17 commits into from Feb 5, 2018
Merged

Add full document sanitization in theme support #929

merged 17 commits into from Feb 5, 2018

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Feb 4, 2018 •
edited

  • 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
Stop excluding children of HEAD
0d01dbb
@westonruter
Include extension_spec in exported tag_spec
c6e68b7
@westonruter
Rework output buffering sanitization to apply to entire doc
62cbd6c
@westonruter
Add ability to use html document element as sanitization root
372412b 
* Sanitize the elements in the HEAD.
* Deprecate get_body_node in favor of getting html or body element once and storing in var.
* Add support for alternative attribute names in check for mandatory attributes.
* Expand use of NAME_VALUE_DISPATCH for other value matching results.
@westonruter
Prevent lower-casing regex values from spec
8c39957
@westonruter
Exclude amp4ads rules from spec; omit html_format since unused
cd53fce
@westonruter
Add tests for sanitizing the html document element as a whole
d8c4054
@westonruter
Allow enqueued font URLs to be printed normally as stylesheet links
f6ccbe9
@westonruter
Enforce html element to have amp attribute if not present
af0da8c
@westonruter
Re-write amphtml-update.py to use PHP for serializing
7adb70f 
This reduces a lot of complexity and as well as it fixes errors in the PHP-generation
@westonruter
Just-in-time ensure that meta charset, meta viewport, and rel=canonic…
d4db6d5 
…al link are present
@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.

kienstra
kienstra approved these changes Feb 4, 2018
View reviewed changes
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.

includes/class-amp-theme-support.php
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.

@westonruter
Eliminate needless use of dispatch_key and looping over attr spec lis…
67b6de6 
…ts once failure found
@westonruter
Add support for value_properties, validating meta viewport content an…
9797778 
…d X-UA-Compatible content
@westonruter
Update use of constants in AMP_Rule_Spec
b8036f7 
* Remove newly-unused dispatch key constant.
* Use integers for flags.
* Use constants instead of string literals.
@westonruter westonruter mentioned this pull request Feb 5, 2018
Merged
@westonruter westonruter merged commit 574b253 into develop Feb 5, 2018
@westonruter westonruter deleted the add/full-document-sanitization branch February 5, 2018 05:37
@westonruter westonruter mentioned this pull request Feb 5, 2018
Closed
@ThierryA
Copy link
Collaborator

ThierryA commented Feb 5, 2018 •
edited

I absolutely love these changes ❤️

This was referenced Feb 6, 2018
Closed
Closed
@westonruter westonruter mentioned this pull request Jul 6, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kienstra kienstra kienstra approved these changes

@ThierryA ThierryA Awaiting requested review from ThierryA

Assignees

@westonruter westonruter

Labels
None yet
Projects
None yet
Milestone
v0.7
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@westonruter @kienstra @ThierryA