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

Prevent removal of closing table and td tags in script[template="amp-mustache"] #4333

Merged
merged 13 commits into from
Mar 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@ module.exports = function( grunt ) {
command: [
'if [ ! -e build ]; then echo "Run grunt build first."; exit 1; fi',
'cd build',
'if [ -d vendor/ampproject/common/vendor ]; then rm -r vendor/ampproject/common/vendor; fi',
'if [ -d vendor/ampproject/optimizer/vendor ]; then rm -r vendor/ampproject/optimizer/vendor; fi',
'composer install --no-dev -o',
'for symlinksource in $(find vendor/ampproject -type l); do symlinktarget=$(readlink "$symlinksource") && rm "$symlinksource" && cp -r "vendor/ampproject/$symlinktarget" "$symlinksource"; done',
'composer remove cweagans/composer-patches --update-no-dev -o',
'rm -r ' + productionVendorExcludedFilePatterns.join( ' ' )
'rm -r ' + productionVendorExcludedFilePatterns.join( ' ' ),
'if [ -d vendor/ampproject/common/vendor ]; then rm -r vendor/ampproject/common/vendor; fi',
'if [ -d vendor/ampproject/optimizer/vendor ]; then rm -r vendor/ampproject/optimizer/vendor; fi'
].join( ' && ' ),
},
create_build_zip: {
Expand Down
43 changes: 43 additions & 0 deletions lib/common/src/Dom/Document.php
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ public function loadHTMLFragment($source, $options = 0)
$source = $this->convertAmpBindAttributes($source);
$source = $this->replaceSelfClosingTags($source);
$source = $this->maybeReplaceNoscriptElements($source);
$source = $this->secureMustacheScriptTemplates($source);
$source = $this->secureDoctypeNode($source);
$source = $this->convertAmpEmojiAttribute($source);

Expand All @@ -384,6 +385,7 @@ public function loadHTMLFragment($source, $options = 0)

if ($success) {
$this->normalizeHtmlAttributes();
$this->restoreMustacheScriptTemplates();

// Remove http-equiv charset again.
$meta = $this->head->firstChild;
Expand Down Expand Up @@ -850,6 +852,47 @@ private function maybeRestoreNoscriptElements($html)
);
}

/**
* Secures instances of script[template="amp-mustache"] by renaming element to tmp-script, as a workaround to a libxml parsing issue.
*
* This script can have closing tags of its children table and td stripped.
* So this changes its name from script to tmp-script to avoid this.
*
* @link https://github.com/ampproject/amp-wp/issues/4254
* @see restoreMustacheScriptTemplates() Reciprocal function.
*
* @param string $html To replace the tag name that contains the mustache templates.
* @return string The HTML, with the tag name of the mustache templates replaced.
*/
private function secureMustacheScriptTemplates($html)
{
return preg_replace(
'#<script(\s[^>]*?template=(["\']?)amp-mustache\2[^>]*)>(.*?)</script\s*?>#i',
'<tmp-script$1>$3</tmp-script>',
$html
);
}

/**
* Restores the tag names of script[template="amp-mustache"] elements that were replaced earlier.
*
* @see secureMustacheScriptTemplates() Reciprocal function.
*/
private function restoreMustacheScriptTemplates()
{
$tmp_script_elements = iterator_to_array($this->getElementsByTagName('tmp-script'));
foreach ($tmp_script_elements as $tmp_script_element) {
$script = $this->createElement(Tag::SCRIPT);
foreach ($tmp_script_element->attributes as $attr) {
$script->setAttribute($attr->nodeName, $attr->nodeValue);
}
while ($tmp_script_element->firstChild) {
$script->appendChild($tmp_script_element->firstChild);
}
$tmp_script_element->parentNode->replaceChild($script, $tmp_script_element);
}
}

/**
* Replace AMP binding attributes with something that libxml can parse (as HTML5 data-* attributes).
*
Expand Down
35 changes: 35 additions & 0 deletions lib/common/tests/Dom/DocumentTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,41 @@ public function dataDomDocument()
'<!DOCTYPE html><html><head profile=""></head><body></body></html>',
'<!DOCTYPE html><html><head><meta charset="utf-8"></head><body></body></html>',
],
'amp_mustache_template_with_table' => [
'utf-8',
'<!DOCTYPE html><html>' . $head . '<body><template type="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></template></body></html>',
'<!DOCTYPE html><html>' . $head . '<body><template type="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></template></body></html>',
],
'amp_mustache_template_script_with_table' => [
'utf-8',
'<!DOCTYPE html><html>' . $head . '<body><script id="baz" type="text/plain" template="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
'<!DOCTYPE html><html>' . $head . '<body><script id="baz" type="text/plain" template="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
],
'amp_mustache_spaces_in_closing_script' => [
'utf-8',
'<!DOCTYPE html><html>' . $head . '<body><script id="baz" type="text/plain" template="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script ></body></html>',
'<!DOCTYPE html><html>' . $head . '<body><script id="baz" type="text/plain" template="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
],
'amp_mustache_template_single_quotes' => [
'utf-8',
'<!DOCTYPE html><html>' . $head . '<body><script type=\'text/plain\' template=\'amp-mustache\'><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
'<!DOCTYPE html><html>' . $head . '<body><script type="text/plain" template="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
],
'amp_mustache_no_quote' => [
'utf-8',
'<!DOCTYPE html><html>' . $head . '<body><script type=\'text/plain\' template=amp-mustache><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
'<!DOCTYPE html><html>' . $head . '<body><script type="text/plain" template="amp-mustache"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
],
'amp_mustache_script_multiple_children' => [
'utf-8',
'<!DOCTYPE html><html>' . $head . '<body><script type="text/plain" template="amp-mustache"><h1>{{heading}}</h1><p>{{content}}</p><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
'<!DOCTYPE html><html>' . $head . '<body><script type="text/plain" template="amp-mustache"><h1>{{heading}}</h1><p>{{content}}</p><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script></body></html>',
],
'multiple_mustache_templates_still_appear' => [
'utf-8',
'<!DOCTYPE html><html>' . $head . '<body><script template="amp-mustache" type="text/plain" id="foo"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script><script type="text/plain" template="amp-mustache" id="example"><p>{{#baz}}This is inside a template{{/baz}}</p></script></body></html>',
'<!DOCTYPE html><html>' . $head . '<body><script template="amp-mustache" type="text/plain" id="foo"><table><tr>{{#example}}<td></td>{{/example}}</tr></table></script><script type="text/plain" template="amp-mustache" id="example"><p>{{#baz}}This is inside a template{{/baz}}</p></script></body></html>',
],
];
}

Expand Down
2 changes: 1 addition & 1 deletion lib/optimizer/tests/src/TestMarkup.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ final class TestMarkup
const SCRIPT_AMPMRAID = '<script async host-service="amp-mraid" src="https://cdn.ampproject.org/v0/amp-mraid-0.1.js"></script>';

// ScriptAMPMustache is the script for amp-mustache.
const SCRIPT_AMPMUSTACHE = '<script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.1.js"></script>';
const SCRIPT_AMPMUSTACHE = '<script async="" custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.1.js"></script>';

// ScriptAMPRuntime is the AMP script tag.
const SCRIPT_AMPRUNTIME = '<script async src="https://cdn.ampproject.org/v0.js"></script>';
Expand Down