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

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

Merged
merged 13 commits into from
Mar 10, 2020
Merged

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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
826a430
Use Weston's snippet to correct ensure the amp-mustache template's ou…
kienstra Feb 25, 2020
5ef39b7
Add a test for script[amp-mustache] with 3 child nodes
kienstra Feb 25, 2020
83d3dd6
Change regex to accept single or no quote, add test
kienstra Feb 25, 2020
52d49e9
Improve regex, based on Alain's suggestions
kienstra Mar 2, 2020
65a16ef
Merge branch 'develop' into fix/mustache-template-workaround
kienstra Mar 2, 2020
a1d8469
Merge in develop, resolve conflicts
kienstra Mar 9, 2020
f86eab1
Revert "Merge in develop, resolve conflicts"
kienstra Mar 9, 2020
387bd10
Merge in develop, resolve conflicts
kienstra Mar 9, 2020
19af10d
Correct code styling, mainly removing spaces
kienstra Mar 9, 2020
2d7bb8a
Address failed unit tests and PSR-12 coding style
kienstra Mar 9, 2020
673e5a9
Improve naming of methods to preserve mustache script templates
westonruter Mar 10, 2020
73700b9
Fix order of operations for vendor vendor removal
westonruter Mar 10, 2020
b74d853
Improve phpdoc descriptions
westonruter Mar 10, 2020
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
View file
Open in desktop
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
View file
Open in desktop
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"] be renaming element to tmp-script, as a workaround to a libxml parsing issue.
westonruter marked this conversation as resolved.
Show resolved Hide resolved
*
* 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 of the mustache templates in.
westonruter marked this conversation as resolved.
Show resolved Hide resolved
* @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
View file
Open in desktop
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
View file
Open in desktop
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