Merge pull request #4340 from ampproject/add/json-validation-error

In scripts expected to have JSON, ensure the JSON is valid
ampproject · Mar 9, 2020 · 4ba19bf · 4ba19bf
2 parents 60ad49b + 585c225
commit 4ba19bf
Show file tree

Hide file tree

Showing 9 changed files with 206 additions and 29 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -29,10 +29,6 @@ branches:
     - develop
     - /^\d+\.\d+$/
 
-env:
-  global:
-    - PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=true
-
 install:
   - nvm install
   - composer install
@@ -94,7 +90,7 @@ jobs:
 
     - name: E2E tests
       php: "7.3"
-      env: WP_VERSION=latest DEV_LIB_SKIP=phpcs,eslint,xmllint,phpsyntax,phpunit PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=
+      env: WP_VERSION=latest DEV_LIB_SKIP=phpcs,eslint,xmllint,phpsyntax,phpunit
       install:
         - nvm install
         - composer install

diff --git a/composer.json b/composer.json
@@ -10,6 +10,7 @@
     "ext-date": "*",
     "ext-dom": "*",
     "ext-iconv": "*",
+    "ext-json": "*",
     "ext-libxml": "*",
     "ext-spl": "*",
     "ampproject/common": "^1",

diff --git a/composer.lock b/composer.lock
diff --git a/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php b/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
@@ -45,6 +45,12 @@ class AMP_Tag_And_Attribute_Sanitizer extends AMP_Base_Sanitizer {
 	const INVALID_CDATA_CSS_IMPORTANT          = 'INVALID_CDATA_CSS_IMPORTANT';
 	const INVALID_CDATA_CONTENTS               = 'INVALID_CDATA_CONTENTS';
 	const INVALID_CDATA_HTML_COMMENTS          = 'INVALID_CDATA_HTML_COMMENTS';
+	const JSON_ERROR_CTRL_CHAR                 = 'JSON_ERROR_CTRL_CHAR';
+	const JSON_ERROR_DEPTH                     = 'JSON_ERROR_DEPTH';
+	const JSON_ERROR_EMPTY                     = 'JSON_ERROR_EMPTY';
+	const JSON_ERROR_STATE_MISMATCH            = 'JSON_ERROR_STATE_MISMATCH';
+	const JSON_ERROR_SYNTAX                    = 'JSON_ERROR_SYNTAX';
+	const JSON_ERROR_UTF8                      = 'JSON_ERROR_UTF8';
 	const INVALID_ATTR_VALUE                   = 'INVALID_ATTR_VALUE';
 	const INVALID_ATTR_VALUE_CASEI             = 'INVALID_ATTR_VALUE_CASEI';
 	const INVALID_ATTR_VALUE_REGEX             = 'INVALID_ATTR_VALUE_REGEX';
@@ -923,9 +929,50 @@ private function validate_cdata_for_node( DOMElement $element, $cdata_spec ) {
 				return [ 'code' => self::MANDATORY_CDATA_MISSING_OR_INCORRECT ];
 			}
 		}
+
+		// When the CDATA is expected to be JSON, ensure it's valid JSON.
+		if ( 'script' === $element->nodeName && in_array( $element->getAttribute( 'type' ), [ 'application/json', 'application/ld+json' ], true ) ) {
+			if ( '' === trim( $element->textContent ) ) {
+				return [ 'code' => self::JSON_ERROR_EMPTY ];
+			}
+
+			json_decode( $element->textContent );
+			$json_last_error = json_last_error();
+
+			if ( JSON_ERROR_NONE !== $json_last_error ) {
+				return [ 'code' => $this->get_json_error_code( $json_last_error ) ];
+			}
+		}
+
 		return true;
 	}
 
+	/**
+	 * Gets the JSON error code for the last error.
+	 *
+	 * @link https://www.php.net/manual/en/function.json-last-error.php#refsect1-function.json-last-error-returnvalues
+	 *
+	 * @param int $json_last_error The last JSON error code.
+	 * @return string The error code for the last JSON error.
+	 */
+	private function get_json_error_code( $json_last_error ) {
+		static $possible_json_errors = [
+			'JSON_ERROR_CTRL_CHAR',
+			'JSON_ERROR_DEPTH',
+			'JSON_ERROR_STATE_MISMATCH',
+			'JSON_ERROR_SYNTAX',
+			'JSON_ERROR_UTF8',
+		];
+
+		foreach ( $possible_json_errors as $possible_error ) {
+			if ( constant( $possible_error ) === $json_last_error ) {
+				return $possible_error;
+			}
+		}
+
+		return 'JSON_ERROR_SYNTAX';
+	}
+
 	/**
 	 * Determines is a node is currently valid per its tag specification.
 	 *

diff --git a/includes/validation/class-amp-validation-error-taxonomy.php b/includes/validation/class-amp-validation-error-taxonomy.php
@@ -1833,8 +1833,35 @@ public static function filter_manage_custom_columns( $content, $column_name, $te
 					$content .= '</p>';
 				}
 
-				if ( isset( $validation_error['message'] ) ) {
-					$content .= sprintf( '<p>%s</p>', esc_html( $validation_error['message'] ) );
+				$message = null;
+				switch ( $validation_error['code'] ) {
+					case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_EMPTY:
+						$message = __( 'Expected JSON, got an empty value', 'amp' );
+						break;
+					case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_DEPTH:
+						$message = __( 'The maximum stack depth has been exceeded', 'amp' );
+						break;
+					case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_STATE_MISMATCH:
+						$message = __( 'Invalid or malformed JSON', 'amp' );
+						break;
+					case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_CTRL_CHAR:
+						$message = __( 'Control character error, possibly incorrectly encoded', 'amp' );
+						break;
+					case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_SYNTAX:
+						$message = __( 'Syntax error', 'amp' );
+						break;
+					case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_UTF8:
+						/* translators: %s: UTF-8, a charset */
+						$message = sprintf( __( 'Malformed %s characters, possibly incorrectly encoded', 'amp' ), 'UTF-8' );
+						break;
+					default:
+						if ( isset( $validation_error['message'] ) ) {
+							$message = $validation_error['message'];
+						}
+				}
+
+				if ( $message ) {
+					$content .= sprintf( '<p>%s</p>', esc_html( $message ) );
 				}
 
 				break;
@@ -3024,6 +3051,13 @@ public static function get_error_title_from_code( $validation_error ) {
 			case AMP_Tag_And_Attribute_Sanitizer::INVALID_CDATA_CSS_IMPORTANT:
 			case AMP_Tag_And_Attribute_Sanitizer::CDATA_VIOLATES_BLACKLIST:
 				return esc_html__( 'Illegal text content', 'amp' );
+			case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_CTRL_CHAR:
+			case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_DEPTH:
+			case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_EMPTY:
+			case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_STATE_MISMATCH:
+			case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_SYNTAX:
+			case AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_UTF8:
+				return esc_html__( 'Invalid JSON', 'amp' );
 			case AMP_Style_Sanitizer::CSS_SYNTAX_INVALID_IMPORTANT:
 				$title = esc_html__( 'Illegal CSS !important property', 'amp' );
 				if ( isset( $validation_error['css_property_name'] ) ) {

diff --git a/lib/common/composer.json b/lib/common/composer.json
@@ -27,8 +27,7 @@
   "extra": {
     "branch-alias": {
       "dev-master": "1.0.x-dev",
-      "dev-develop": "1.0.x-dev",
-      "dev-add/958-amp-optimizer": "1.0.x-dev"
+      "dev-develop": "1.0.x-dev"
     }
   },
   "autoload": {

diff --git a/lib/optimizer/composer.json b/lib/optimizer/composer.json
@@ -28,8 +28,7 @@
   "extra": {
     "branch-alias": {
       "dev-master": "1.0.x-dev",
-      "dev-develop": "1.0.x-dev",
-      "dev-add/958-amp-optimizer": "1.0.x-dev"
+      "dev-develop": "1.0.x-dev"
     }
   },
   "autoload": {

diff --git a/tests/php/test-tag-and-attribute-sanitizer.php b/tests/php/test-tag-and-attribute-sanitizer.php
@@ -7,6 +7,8 @@
 
 use AmpProject\Dom\Document;
 
+// phpcs:disable WordPress.WP.EnqueuedResources
+
 /**
  * Test AMP_Tag_And_Attribute_Sanitizer
  *
@@ -974,7 +976,7 @@ static function() {
 			],
 
 			'remove_script_with_async_attribute'           => [
-				'<script async src="//cdn.someecards.com/assets/embed/embed-v1.07.min.js" charset="utf-8"></script>', // phpcs:ignore
+				'<script async src="//cdn.someecards.com/assets/embed/embed-v1.07.min.js" charset="utf-8"></script>',
 				'',
 				[],
 				[ AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_TAG ],
@@ -988,7 +990,7 @@ static function() {
 			],
 
 			'allow_node_with_valid_mandatory_attribute'    => [
-				'<amp-analytics><script type="application/json"></script></amp-analytics>',
+				'<amp-analytics><script type="application/json">{"vars": {"apid": "XXXX"}}</script></amp-analytics>',
 				null, // No change.
 				[ 'amp-analytics' ],
 			],
@@ -2268,10 +2270,10 @@ static function() {
 	public function get_html_data() {
 		$data = [
 			'meta_charset_and_viewport_and_canonical' => [
-				'<html amp lang="ar" dir="rtl"><head><meta charset="utf-8"><meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1"><meta name="viewport" content="width=device-width, minimum-scale=1"><base target="_blank"><link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Tangerine"><link rel="canonical" href="self.html"><title>marhabaan bialealim!</title></head><body></body></html>', // phpcs:ignore
+				'<html amp lang="ar" dir="rtl"><head><meta charset="utf-8"><meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1"><meta name="viewport" content="width=device-width, minimum-scale=1"><base target="_blank"><link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Tangerine"><link rel="canonical" href="self.html"><title>marhabaan bialealim!</title></head><body></body></html>',
 			],
 			'script_tag_externals'                    => [
-				'<html amp><head><meta charset="utf-8"><script async type="text/javascript" src="illegal.js"></script><script async src="illegal.js"></script><script src="illegal.js"></script><script type="text/javascript" src="illegal.js"></script></head><body></body></html>', // phpcs:ignore
+				'<html amp><head><meta charset="utf-8"><script async type="text/javascript" src="illegal.js"></script><script async src="illegal.js"></script><script src="illegal.js"></script><script type="text/javascript" src="illegal.js"></script></head><body></body></html>',
 				'<html amp><head><meta charset="utf-8"></head><body></body></html>',
 				[],
 				array_fill(
@@ -2295,7 +2297,7 @@ public function get_html_data() {
 				],
 			],
 			'style_external'                          => [
-				'<html amp><head><meta charset="utf-8"><link rel="stylesheet" href="https://example.com/test.css"></head><body></body></html>', // phpcs:ignore
+				'<html amp><head><meta charset="utf-8"><link rel="stylesheet" href="https://example.com/test.css"></head><body></body></html>',
 				'<html amp><head><meta charset="utf-8"></head><body></body></html>',
 				[],
 				[
@@ -2323,7 +2325,7 @@ public function get_html_data() {
 				),
 			],
 			'bad_external_font'                       => [
-				'<html amp><head><meta charset="utf-8"><link rel="stylesheet" href="https://fonts.example.com/css?family=Bad"></head><body></body></html>', // phpcs:ignore
+				'<html amp><head><meta charset="utf-8"><link rel="stylesheet" href="https://fonts.example.com/css?family=Bad"></head><body></body></html>',
 				'<html amp><head><meta charset="utf-8"></head><body></body></html>',
 				[],
 				[ AMP_Tag_And_Attribute_Sanitizer::INVALID_ATTR_VALUE_REGEX, AMP_Tag_And_Attribute_Sanitizer::ATTR_REQUIRED_BUT_MISSING ],
@@ -2463,8 +2465,44 @@ public function get_html_data() {
 				[],
 				[ AMP_Tag_And_Attribute_Sanitizer::INVALID_CDATA_HTML_COMMENTS ],
 			],
+			'cdata_malformed_json'                    => [
+				'<html><head><meta charset="utf-8"><script type="application/ld+json">{"example": </script></head><body></body></html>',
+				'<html><head><meta charset="utf-8"></head><body></body></html>',
+				[],
+				[ AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_SYNTAX ],
+			],
+			'cdata_malformed_json_with_emojis'        => [
+				'<html><head><meta charset="utf-8"><script type="application/ld+json">{"wrong": "' . wp_staticize_emoji( '🚧 🚧' ) . '"}</script></head><body></body></html>',
+				'<html><head><meta charset="utf-8"></head><body></body></html>',
+				[],
+				[ AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_SYNTAX ],
+			],
+			'cdata_malformed_utf8_json'               => [
+				sprintf( '<html><head><meta charset="utf-8"><script type="application/ld+json">{"wrong": "%s"}</script></head><body></body></html>', "\xFF" ),
+				'<html><head><meta charset="utf-8"></head><body></body></html>',
+				[],
+				[ AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_UTF8 ],
+			],
+			'cdata_empty_json_considered_invalid'     => [
+				'<html><head><meta charset="utf-8"><script type="application/ld+json"></script></head><body></body></html>',
+				'<html><head><meta charset="utf-8"></head><body></body></html>',
+				[],
+				[ AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_EMPTY ],
+			],
+			'analytics_empty_json_considered_invalid' => [
+				'<html><head><meta charset="utf-8"></head><body><amp-analytics><script type="application/json"> </script></amp-analytics></body></html>',
+				'<html><head><meta charset="utf-8"></head><body><amp-analytics></amp-analytics></body></html>',
+				[ 'amp-analytics' ],
+				[ AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_EMPTY ],
+			],
+			'analytics_state_mismatch_json_error'     => [
+				'<html><head><meta charset="utf-8"></head><body><amp-analytics><script type="application/json">{"foo": 1 ] }</script></amp-analytics></body></html>',
+				'<html><head><meta charset="utf-8"></head><body><amp-analytics></amp-analytics></body></html>',
+				[ 'amp-analytics' ],
+				[ AMP_Tag_And_Attribute_Sanitizer::JSON_ERROR_STATE_MISMATCH ],
+			],
 			'script_cdata_contents_bad'               => [
-				'<html><head><meta charset="utf-8"><script async src="https://cdn.ampproject.org/v0.js">document.write("bad");</script></head><body></body></html>', // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript
+				'<html><head><meta charset="utf-8"><script async src="https://cdn.ampproject.org/v0.js">document.write("bad");</script></head><body></body></html>',
 				'<html><head><meta charset="utf-8"></head><body></body></html>',
 				[],
 				[ AMP_Tag_And_Attribute_Sanitizer::INVALID_CDATA_CONTENTS ],