Eliminate X-AMP-Validation-Errors response header in favor of HTML af…

…ter body closing tag Fixes issue where large response header can cause internal server error
jritterbush · Apr 29, 2018 · 75c1ba1 · 75c1ba1
1 parent 18ade27
commit 75c1ba1
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 44 deletions.
diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php
@@ -124,13 +124,6 @@ class AMP_Validation_Utils {
 	 */
 	const NONCE_ACTION = 'amp_recheck_';
 
-	/**
-	 * HTTP response header name containing JSON-serialized validation errors.
-	 *
-	 * @var string
-	 */
-	const VALIDATION_ERRORS_RESPONSE_HEADER_NAME = 'X-AMP-Validation-Errors';
-
 	/**
 	 * Transient key to store validation errors when activating a plugin.
 	 *
@@ -1116,39 +1109,27 @@ public static function should_validate_response() {
 	 *     Args.
 	 *
 	 *     @type bool $remove_source_comments           Whether source comments should be removed. Defaults to true.
-	 *     @type bool $send_validation_errors_header    Whether the X-AMP-Validation-Errors header should be sent. Defaults to true.
 	 *     @type bool $append_validation_status_comment Whether the validation errors should be appended as an HTML comment. Defaults to true.
 	 * }
 	 */
 	public static function finalize_validation( DOMDocument $dom, $args = array() ) {
 		$args = array_merge(
 			array(
-				'send_validation_errors_header'    => true,
 				'remove_source_comments'           => true,
 				'append_validation_status_comment' => true,
 			),
 			$args
 		);
 
-		if ( $args['send_validation_errors_header'] && ! headers_sent() ) {
-			self::send_validation_errors_header();
-		}
-
 		if ( $args['remove_source_comments'] ) {
 			self::remove_source_comments( $dom );
 		}
 
 		if ( $args['append_validation_status_comment'] ) {
-			$report  = "\n# Validation Status\n";
-			$report .= "\n## Summary\n";
-			$report .= wp_json_encode( self::summarize_validation_errors( self::$validation_errors ), 128 /* JSON_PRETTY_PRINT */ ) . "\n";
-			$report .= "\n## Details\n";
-			$report .= wp_json_encode( self::$validation_errors, 128 /* JSON_PRETTY_PRINT */ ) . "\n";
-			$comment = $dom->createComment( $report );
-			$body    = $dom->getElementsByTagName( 'body' )->item( 0 );
-			if ( $body ) {
-				$body->appendChild( $comment );
-			}
+			$encoded = wp_json_encode( self::$validation_errors, 128 /* JSON_PRETTY_PRINT */ );
+			$encoded = str_replace( '--', '\u002d\u002d', $encoded ); // Prevent "--" in strings from breaking out of HTML comments.
+			$comment = $dom->createComment( 'AMP_VALIDATION_ERRORS:' . $encoded . "\n" );
+			$dom->documentElement->appendChild( $comment );
 		}
 	}
 
@@ -1198,13 +1179,6 @@ public static function register_post_type() {
 		$post_type->cap->create_posts = 'do_not_allow';
 	}
 
-	/**
-	 * Send validation errors back in response header.
-	 */
-	public static function send_validation_errors_header() {
-		header( self::VALIDATION_ERRORS_RESPONSE_HEADER_NAME . ': ' . wp_json_encode( self::$validation_errors ) );
-	}
-
 	/**
 	 * Stores the validation errors.
 	 *
@@ -1347,11 +1321,11 @@ public static function validate_url( $url ) {
 				wp_remote_retrieve_response_message( $r )
 			);
 		}
-		$json = wp_remote_retrieve_header( $r, self::VALIDATION_ERRORS_RESPONSE_HEADER_NAME );
-		if ( ! $json ) {
-			return new WP_Error( 'response_header_absent' );
+		$response = wp_remote_retrieve_body( $r );
+		if ( ! preg_match( '#</body>.*?<!--\s*AMP_VALIDATION_ERRORS\s*:\s*(\[.*?\])\s*-->#s', $response, $matches ) ) {
+			return new WP_Error( 'response_comment_absent' );
 		}
-		$validation_errors = json_decode( $json, true );
+		$validation_errors = json_decode( $matches[1], true );
 		if ( ! is_array( $validation_errors ) ) {
 			return new WP_Error( 'malformed_json_validation_errors' );
 		}

diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php
@@ -871,9 +871,9 @@ public function test_validate_after_plugin_activation() {
 		$this->factory()->post->create();
 		$filter = function() use ( $validation_errors ) {
 			return array(
-				'body'    => '',
-				'headers' => array(
-					AMP_Validation_Utils::VALIDATION_ERRORS_RESPONSE_HEADER_NAME => wp_json_encode( $validation_errors ),
+				'body' => sprintf(
+					'<html amp><head></head><body></body><!--%s--></html>',
+					'AMP_VALIDATION_ERRORS:' . wp_json_encode( $validation_errors )
 				),
 			);
 		};
@@ -907,7 +907,7 @@ public function test_validate_url() {
 		add_filter( 'pre_http_request', $filter );
 		$r = AMP_Validation_Utils::validate_url( home_url( '/' ) );
 		$this->assertInstanceOf( 'WP_Error', $r );
-		$this->assertEquals( 'response_header_absent', $r->get_error_code() );
+		$this->assertEquals( 'response_comment_absent', $r->get_error_code() );
 		remove_filter( 'pre_http_request', $filter );
 
 		// Test success.
@@ -924,9 +924,9 @@ public function test_validate_url() {
 				$url
 			);
 			return array(
-				'body'    => '',
-				'headers' => array(
-					AMP_Validation_Utils::VALIDATION_ERRORS_RESPONSE_HEADER_NAME => wp_json_encode( $validation_errors ),
+				'body' => sprintf(
+					'<html amp><head></head><body></body><!--%s--></html>',
+					'AMP_VALIDATION_ERRORS:' . wp_json_encode( $validation_errors )
 				),
 			);
 		};
@@ -1085,9 +1085,9 @@ public function test_handle_bulk_action() {
 		$that   = $this;
 		$filter = function() use ( $that ) {
 			return array(
-				'body'    => '',
-				'headers' => array(
-					AMP_Validation_Utils::VALIDATION_ERRORS_RESPONSE_HEADER_NAME => wp_json_encode( $that->get_mock_errors() ),
+				'body' => sprintf(
+					'<html amp><head></head><body></body><!--%s--></html>',
+					'AMP_VALIDATION_ERRORS:' . wp_json_encode( $that->get_mock_errors() )
 				),
 			);
 		};