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

Sanitizer removes square-bracketed binding attributes #1076

Closed
laras126 opened this issue Apr 16, 2018 · 4 comments
Closed

Sanitizer removes square-bracketed binding attributes #1076

laras126 opened this issue Apr 16, 2018 · 4 comments

Comments

@laras126
Copy link

Expected behavior:

The square-bracketed binding attributes identified in $globally_allowed_attrs should not removed by the sanitizer.

Actual behavior:

Attributes surrounded with square-brackets e.g. [class] and [text] are removed by the sanitizer even though they are AMP compatible.

Steps to reproduce:

  1. Activate plugin version 0.6.2
  2. Include amp-bind in component scripts in functions.php:
add_filter( 'amp_post_template_data', 'my_amp_component_scripts' );
function my_amp_component_scripts( $data ) {
	$custom_component_scripts = array(
		'amp-bind'  => 'https://cdn.ampproject.org/v0/amp-bind-0.1.js'
	);
	$data['amp_component_scripts'] = array_merge( $data['amp_component_scripts'], $custom_component_scripts );
	return $data;
}
  1. Create a simple shortcode utilizing a binding attribute, also in functions.php:
add_shortcode( 'my_button', 'my_button_shortcode' );
function my_button_shortcode( $atts ) {
	if( is_amp_endpoint() ) {
		return '<p [text]="\'Hello \' + foo">Hello World</p><button on="tap:AMP.setState({foo: \'amp-bind\'})">Click</button>';
	}
}
  1. Add the shortcode [my_button] to the post editor.
  2. Visit the post's AMP version, and inspect the button to see the binding attribute has been removed.

Notes

Adding a print_r($attr_name); to ./includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php reveals that, in the above example, the name of the attribute to be removed is foo. A similar problem occurs when a different expression constitutes the attribute's value – e.g. [class]="foo ? 'foo' : 'bar'" – the $attr_name is printed as :.

If the sanitizer is disabled by commenting out line 118 in ./includes/templates/class-amp-content.php, the amp-bind attribute is rendered and functions as expected.

@westonruter
Copy link
Member

@laras126 Hi! You're absolutely right. This is a problem in v0.6 of the plugin. However, in 0.7 support for amp-bind was added via #895. Would you please try the 0.7 branch and confirm that the problem with amp-bind is fixed? Note also that you won't have to include amp-bind script manually either because it will be automatically added in:

https://github.com/Automattic/amp-wp/blob/b53de14a00e7302ecc967aa3a798e7fca3eddbf1/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php#L462-L475

@laras126
Copy link
Author

laras126 commented Apr 17, 2018

@westonruter Heyy, it works! How would you suggest implementing this update on a live site running the version on the plugin repository?

@westonruter
Copy link
Member

@laras126 with the plugin repo cloned, you can run these commands:

git checkout 0.7
npm install
npm run build

That will create an amp.zip in the directory which you can install on a site via the WP admin. You'll have to deactivate and uninstall any existing AMP plugin first before you do this. Otherwise, you can extract the ZIP and SFTP/rsync/git-push the files up.

@laras126
Copy link
Author

Okay great, thank you @westonruter! Works like a charm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants