Merge pull request #6021 from ampproject/fix/6011-catastrophic-backtr…

…ace-during-validation Optimize method of stripping validation response of affixed HTML comments
ampproject · Mar 31, 2021 · b9bf0d2 · b9bf0d2
2 parents 7450147 + b222c5d
commit b9bf0d2
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 10 deletions.
diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php
@@ -1848,7 +1848,22 @@ public static function validate_url( $url ) {
 		$response = ltrim( $response );
 
 		// Strip HTML comments that may have been injected at the end of the response (e.g. by a caching plugin).
-		$response = preg_replace( '/<!--.*?-->\s*$/s', '', $response );
+		while ( ! empty( $response ) ) {
+			$response = rtrim( $response );
+			$length   = strlen( $response );
+
+			if ( $length < 3 || '-' !== $response[ $length - 3 ] || '-' !== $response[ $length - 2 ] || '>' !== $response[ $length - 1 ] ) {
+				break;
+			}
+
+			$start = strrpos( $response, '<!--' );
+
+			if ( false === $start ) {
+				break;
+			}
+
+			$response = substr( $response, 0, $start );
+		}
 
 		if ( '' === $response ) {
 			return new WP_Error( 'white_screen_of_death' );

diff --git a/tests/php/validation/test-class-amp-validation-manager.php b/tests/php/validation/test-class-amp-validation-manager.php
@@ -2075,21 +2075,71 @@ public function test_validate_after_plugin_activation() {
 		$this->assertEquals( [ $validation_error ], get_transient( AMP_Validation_Manager::PLUGIN_ACTIVATION_VALIDATION_ERRORS_TRANSIENT_KEY ) );
 	}
 
+	/**
+	 * Get validation errors.
+	 *
+	 * @return array
+	 */
+	public function get_validation_errors() {
+		return [
+			'simple error'                         => [
+				[
+					[
+						'code' => 'example',
+					],
+				],
+				'',
+			],
+			'error containing a huge HTML comment' => [
+				[
+					[
+						'code' => 'example',
+						'text' => '<!-- ' . str_repeat( 'a', 1000000 ) . ' -->',
+					],
+				],
+				"\n<!-- Generated by greatness! -->\n",
+			],
+			'error with multiple comments at end'  => [
+				[
+					[
+						'code' => 'example',
+					],
+				],
+				'<!-- generated 2 seconds ago -->
+				<!-- generated in 1.134 seconds -->
+				<!-- served from batcache in 0.003 seconds -->
+				<!-- expires in 298 seconds -->',
+			],
+			'error with multi-line comment at end' => [
+				[
+					[
+						'code' => 'example',
+					],
+				],
+				'<!--
+					generated 2 seconds ago
+					generated in 1.134 seconds
+					served from batcache in 0.003 seconds
+					expires in 298 seconds
+				-->',
+			],
+		];
+	}
+
 	/**
 	 * Test for validate_url() and validate_url_and_store().
 	 *
+	 * @dataProvider get_validation_errors()
+	 *
 	 * @covers AMP_Validation_Manager::validate_url()
 	 * @covers AMP_Validation_Manager::validate_url_and_store()
+	 *
+	 * @param array  $validation_errors Validation errors.
+	 * @param string $after_matter      After matter that is appended to response body.
 	 */
-	public function test_validate_url() {
+	public function test_validate_url( $validation_errors, $after_matter ) {
 		AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG );
 
-		$validation_errors = [
-			[
-				'code' => 'example',
-			],
-		];
-
 		// Test headers absent.
 		self::factory()->post->create();
 		$filter = static function() {
@@ -2119,7 +2169,7 @@ public function test_validate_url() {
 			'id'   => 123,
 		];
 		$stylesheets = [ [ 'CSS!' ] ];
-		$filter        = function( $pre, $r, $url ) use ( $validation_errors, $php_error, $queried_object, $stylesheets ) {
+		$filter        = function( $pre, $r, $url ) use ( $validation_errors, $php_error, $queried_object, $stylesheets, $after_matter ) {
 			$this->assertStringContains( AMP_Validation_Manager::VALIDATE_QUERY_VAR . '=', $url );
 
 			$validation = [
@@ -2135,7 +2185,7 @@ public function test_validate_url() {
 
 			return [
 				// Prepend JSON with byte order mark, whitespace, and append with HTML comment to ensure stripped.
-				'body'    => "\xEF\xBB\xBF" . ' ' . wp_json_encode( $validation ) . "\n<!-- Generated by greatness! -->\n",
+				'body'    => "\xEF\xBB\xBF" . ' ' . wp_json_encode( $validation ) . $after_matter,
 				'headers' => [
 					'content-type' => 'application/json',
 				],