Erroneous XML processing instructions (e.g. <?php ?>) are not sanitized

[westonruter]

As reported on the support forums, upon upgrading to AMP 1.3 they they started getting the AMP validator complaining about validation errors for this markup:

<?php
$schema = get_post_meta(get_the_ID(), 'schema', true);
if(!empty($schema)) {
echo $schema;
}
?>

Naturally this is a bug since the PHP code is getting incorrectly included on the page. But still this <?php ... ?> processing instruction and others like it should be getting sanitized. It was actually getting removed in the previous version of the plugin, with an error code of unknown:

We should restore this behavior, or rather add a more specific error code like invalid_processing_instruction. The validator doesn't apparently allow any processing instructions, including:

• 🚫 <?xml version="1.0" encoding="UTF-8" ?>
• 🚫 <?xml-stylesheet type="text/xsl" href="style.xsl"?>
• 🚫 <?php /*...*/ ?>

This would be a regression likely introduced by #3243.

Testing Instructions

Try adding the above processing instructions to a Custom HTML block and verify that they get caught by the sanitizer and removed by default.

[westonruter]

Here is a workaround plugin to restore the sanitization of processing instructions: https://gist.github.com/westonruter/96ae3cb713051aae867b874196eef910

[westonruter]

Proposed patch to fix this in the plugin:

diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
index 7d8df775..0d2f3ab2 100644
--- a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
+++ b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
@@ -291,6 +291,8 @@ class AMP_Tag_And_Attribute_Sanitizer extends AMP_Base_Sanitizer {
 						$result
 					);
 				}
+			} elseif ( $this_child instanceof DOMProcessingInstruction ) {
+				$this->remove_invalid_child( $this_child, [ 'code' => 'invalid_processing_instruction' ] );
 			}
 			$this_child = $next_child;
 		}
diff --git a/tests/php/test-tag-and-attribute-sanitizer.php b/tests/php/test-tag-and-attribute-sanitizer.php
index bd59920e..2724dac6 100644
--- a/tests/php/test-tag-and-attribute-sanitizer.php
+++ b/tests/php/test-tag-and-attribute-sanitizer.php
@@ -1670,6 +1670,13 @@ class AMP_Tag_And_Attribute_Sanitizer_Test extends WP_UnitTestCase {
 				[],
 				[ 'invalid_element' ],
 			],
+
+			'invalid_php_pi'                               => [
+				'<?php $schema = get_post_meta(get_the_ID(), \'schema\', true); if(!empty($schema)) { echo $schema; } ?>',
+				'',
+				[],
+				[ 'invalid_processing_instruction' ],
+			],
 		];
 	}

Perhaps we should audit other DOM node types to make sure that nothing else is added which is illegal.

[csossi]

Verified in QA

[westonruter]

Verified in QA

Actually, this highlights some deficiencies. Opened #3660 to fix.