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 content embedding & sanitization to theme support w/ filterable component scripts and custom styles #865

Merged
merged 8 commits into from Jan 17, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Copy link
Member

westonruter commented Jan 16, 2018

  • Creates new helper functions for amp_get_content_embed_handlers() and amp_get_content_sanitizers() which are used now in both the plugin's paired mode post templates as well as when theme support is present. The $post argument is not passed to the amp_content_embed_handlers and amp_content_sanitizers filters when theme support is present since the content being sanitized/embedded may not be a post at all, e.g. a Text widget.
  • It is intended that soon the \AMP_Theme_Support::filter_the_content() method could be expanded to also apply to widget output.
  • Scripts and styles that are gathered during embedding and sanitizing are now injected into the head once output buffering is finalized.
  • Scripts and styles which are injected into the head can be filtered via amp_component_scripts and amp_custom_styles. The former should normally not be needed because the plugin should be able to detect all component scripts required. The styles filter will be useful when a theme, plugin, widget, etc wants to add additional styles, such as for the Recent Comments widget (cf. xwp/ampnews#28).
  • When WP_DEBUG is on, validation is performed on the amp theme support args, warning the user if any unrecognized args were provided.
  • The \AMP_DOM_Utils::get_dom_from_content() method has been updated to parse HTML with the actual blog_charset. This is important for eventually supporting non-UTF8 sites. See #855.

@westonruter westonruter added this to the v0.7 milestone Jan 16, 2018

@westonruter westonruter requested a review from ThierryA Jan 16, 2018

@ThierryA
Copy link
Collaborator

ThierryA left a comment

Great work here. Find the CR below.

// @todo Grab source of all enqueued styles and concatenate here?
// @todo Print contents of get_locale_stylesheet_uri()?
// @todo Allow this to be filtered after output buffering is complete so additional styles can be added by widgets and other components just-in-time?
$path = get_template_directory() . '/style.css'; // @todo Honor filter in get_stylesheet_directory_uri()? Style must be local.
$css = file_get_contents( $path ); // phpcs:ignore WordPress.WP.AlternativeFunctions -- It's not a remote file.
echo wp_strip_all_tags( $css ); // WPCS: XSS OK.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 16, 2018

Collaborator

I believe this should be removed.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 16, 2018

Author Member

The reason for it is that is that tag stripping is included in Custom CSS in core to protect against malicious CSS: https://github.com/WordPress/wordpress-develop/blob/9dd8b5a2102a197e236fc05de2e47e85c03a8bd9/src/wp-includes/theme.php#L1687

At the very least we should wrap the call to wp_get_custom_css() with wp_strip_all_tags().

);
}
foreach ( $amp_components as $component => $props ) {
if ( preg_match( '#<(form|input)\b#i', $html ) ) {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 16, 2018

Collaborator

Any specific reason to call this in the loop, this could wrap around the loop which would save memory?

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 16, 2018

Author Member

Oops, my logic here is bad. Fixed in 745a3db.

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Jan 16, 2018

Restarting builds…

@ThierryA

This comment has been minimized.

Copy link
Collaborator

ThierryA commented Jan 16, 2018

I restarted the builds a few times but they seem to be stuck. Pushed a blank commit to try triggering the builds again but that doesn't seem to work neither. It seems like there is a backlog on Travis side https://www.traviscistatus.com/
I subscribed to the incident to get notified when it is resolved.

@westonruter westonruter force-pushed the add/valid-canonical-content branch from a4e17b9 to 745a3db Jan 16, 2018

@ThierryA ThierryA merged commit e60cb15 into develop Jan 17, 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

@ThierryA ThierryA deleted the add/valid-canonical-content branch Jan 17, 2018

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.