From 60fb347dc9645a60694baa5b87d59ded76294b28 Mon Sep 17 00:00:00 2001
From: Alain Schlesser <alain.schlesser@gmail.com>
Date: Wed, 21 Aug 2019 20:43:33 +0200
Subject: [PATCH 1/3] Normalize frameborder=no|yes to frameborder=0|1 in iframe
 sanitizer

---
 .../sanitizers/class-amp-base-sanitizer.php   | 28 +++++++++++++++
 .../sanitizers/class-amp-iframe-sanitizer.php | 11 +++---
 tests/php/test-amp-iframe-sanitizer.php       | 34 ++++++++++++++++++-
 3 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php
index 62b915512d2..ca360f1a394 100644
--- a/includes/sanitizers/class-amp-base-sanitizer.php
+++ b/includes/sanitizers/class-amp-base-sanitizer.php
@@ -254,6 +254,34 @@ public function sanitize_dimension( $value, $dimension ) {
 		return '';
 	}
 
+	/**
+	 * Sanitizes a boolean character (or string) into a '0' or '1' character.
+	 *
+	 * @param string $value A boolean character to sanitize. If a string containing more than a single
+	 *                      character is provided, only the first character is taken into account.
+	 *
+	 * @return string Returns either '0' or '1'.
+	 */
+	public function sanitize_boolean_digit( $value ) {
+
+		if ( empty( $value ) ) {
+			return '0';
+		}
+
+		if ( ! is_string( $value ) && ! is_numeric( $value ) ) {
+			return '0';
+		}
+
+		switch ( substr( (string) $value, 0, 1 ) ) {
+			case '1':
+			case 'y':
+			case 'Y':
+				return '1';
+		}
+
+		return '0';
+	}
+
 	/**
 	 * Sets the layout, and possibly the 'height' and 'width' attributes.
 	 *
diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php
index 70d1376cfbc..a9c4d2b6def 100644
--- a/includes/sanitizers/class-amp-iframe-sanitizer.php
+++ b/includes/sanitizers/class-amp-iframe-sanitizer.php
@@ -131,7 +131,9 @@ public function sanitize() {
 
 			if ( $this->args['add_noscript_fallback'] ) {
 				$node->setAttribute( 'src', $normalized_attributes['src'] );
-
+				if ( $node->hasAttribute( 'frameborder' ) ) {
+					$node->setAttribute( 'frameborder', $normalized_attributes['frameborder'] );
+				}
 				// Preserve original node in noscript for no-JS environments.
 				$this->append_old_node_noscript( $new_node, $node, $this->dom );
 			}
@@ -155,7 +157,7 @@ public function sanitize() {
 	 *      @type bool $allowfullscreen <iframe> `allowfullscreen` attribute - Convert 'false' to empty string ''
 	 *      @type bool $allowtransparency <iframe> `allowtransparency` attribute - Convert 'false' to empty string ''
 	 * }
-	 * @return array Returns HTML attributes; normalizes src, dimensions, frameborder, sandox, allowtransparency and allowfullscreen
+	 * @return array Returns HTML attributes; normalizes src, dimensions, frameborder, sandbox, allowtransparency and allowfullscreen
 	 */
 	private function normalize_attributes( $attributes ) {
 		$out = [];
@@ -189,10 +191,7 @@ private function normalize_attributes( $attributes ) {
 					break;
 
 				case 'frameborder':
-					if ( '0' !== $value && '1' !== $value ) {
-						$value = '0';
-					}
-					$out[ $name ] = $value;
+					$out[ $name ] = $this->sanitize_boolean_digit( $value );
 					break;
 
 				case 'allowfullscreen':
diff --git a/tests/php/test-amp-iframe-sanitizer.php b/tests/php/test-amp-iframe-sanitizer.php
index 61aea858745..3b9b1b4853a 100644
--- a/tests/php/test-amp-iframe-sanitizer.php
+++ b/tests/php/test-amp-iframe-sanitizer.php
@@ -111,7 +111,7 @@ public function get_data() {
 				'
 					<amp-iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="0" sandbox="allow-scripts allow-same-origin" layout="intrinsic" class="amp-wp-enforced-sizes">
 						<noscript>
-							<iframe src="https://example.com/embed/132886713" width="500" height="281"></iframe>
+							<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="0"></iframe>
 						</noscript>					
 					</amp-iframe>
 				',
@@ -342,6 +342,38 @@ public function get_data() {
 					'alias_origin'          => 'https://alt.example.org',
 				],
 			],
+
+			'iframe_with_frameborder_no'                => [
+				'<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="no" class="iframe-class" allowtransparency="false" allowfullscreen></iframe>',
+				'
+					<amp-iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="0" class="iframe-class amp-wp-enforced-sizes" allowfullscreen="" sandbox="allow-scripts allow-same-origin" layout="intrinsic">
+						<span placeholder="" class="amp-wp-iframe-placeholder"></span>
+						<noscript>
+							<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="0" class="iframe-class"></iframe>
+						</noscript>
+					</amp-iframe>
+				',
+				[
+					'add_noscript_fallback' => true,
+					'add_placeholder'       => true,
+				],
+			],
+
+			'iframe_with_frameborder_yes'               => [
+				'<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="yes" class="iframe-class" allowtransparency="false" allowfullscreen></iframe>',
+				'
+					<amp-iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="1" class="iframe-class amp-wp-enforced-sizes" allowfullscreen="" sandbox="allow-scripts allow-same-origin" layout="intrinsic">
+						<span placeholder="" class="amp-wp-iframe-placeholder"></span>
+						<noscript>
+							<iframe src="https://example.com/embed/132886713" width="500" height="281" frameborder="1" class="iframe-class"></iframe>
+						</noscript>
+					</amp-iframe>
+				',
+				[
+					'add_noscript_fallback' => true,
+					'add_placeholder'       => true,
+				],
+			],
 		];
 	}
 

From 7e8f290e121faa3b6ae564f097ec5f8435f378fd Mon Sep 17 00:00:00 2001
From: Alain Schlesser <alain.schlesser@gmail.com>
Date: Wed, 21 Aug 2019 20:57:07 +0200
Subject: [PATCH 2/3] Add inline comments

---
 includes/sanitizers/class-amp-base-sanitizer.php   | 3 +++
 includes/sanitizers/class-amp-iframe-sanitizer.php | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php
index ca360f1a394..3231643bdcf 100644
--- a/includes/sanitizers/class-amp-base-sanitizer.php
+++ b/includes/sanitizers/class-amp-base-sanitizer.php
@@ -264,14 +264,17 @@ public function sanitize_dimension( $value, $dimension ) {
 	 */
 	public function sanitize_boolean_digit( $value ) {
 
+		// Default to false if the value was forgotten.
 		if ( empty( $value ) ) {
 			return '0';
 		}
 
+		// Default to false if the value has an unexpected type.
 		if ( ! is_string( $value ) && ! is_numeric( $value ) ) {
 			return '0';
 		}
 
+		// See: https://github.com/ampproject/amp-wp/issues/2335#issuecomment-493209861
 		switch ( substr( (string) $value, 0, 1 ) ) {
 			case '1':
 			case 'y':
diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php
index a9c4d2b6def..9d315f4f529 100644
--- a/includes/sanitizers/class-amp-iframe-sanitizer.php
+++ b/includes/sanitizers/class-amp-iframe-sanitizer.php
@@ -131,9 +131,12 @@ public function sanitize() {
 
 			if ( $this->args['add_noscript_fallback'] ) {
 				$node->setAttribute( 'src', $normalized_attributes['src'] );
+
+				// AMP is stricter than HTML5 for this attribute, so make sure we use a normalized value.
 				if ( $node->hasAttribute( 'frameborder' ) ) {
 					$node->setAttribute( 'frameborder', $normalized_attributes['frameborder'] );
 				}
+
 				// Preserve original node in noscript for no-JS environments.
 				$this->append_old_node_noscript( $new_node, $node, $this->dom );
 			}

From 6418bf0c87cae957763f8b4908ccb19acad4323b Mon Sep 17 00:00:00 2001
From: Alain Schlesser <alain.schlesser@gmail.com>
Date: Thu, 22 Aug 2019 09:24:12 +0200
Subject: [PATCH 3/3] Move sanitize_boolean_digit() into iframe sanitizer for
 now

---
 .../sanitizers/class-amp-base-sanitizer.php   | 31 -------------------
 .../sanitizers/class-amp-iframe-sanitizer.php | 30 ++++++++++++++++++
 2 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/includes/sanitizers/class-amp-base-sanitizer.php b/includes/sanitizers/class-amp-base-sanitizer.php
index 3231643bdcf..62b915512d2 100644
--- a/includes/sanitizers/class-amp-base-sanitizer.php
+++ b/includes/sanitizers/class-amp-base-sanitizer.php
@@ -254,37 +254,6 @@ public function sanitize_dimension( $value, $dimension ) {
 		return '';
 	}
 
-	/**
-	 * Sanitizes a boolean character (or string) into a '0' or '1' character.
-	 *
-	 * @param string $value A boolean character to sanitize. If a string containing more than a single
-	 *                      character is provided, only the first character is taken into account.
-	 *
-	 * @return string Returns either '0' or '1'.
-	 */
-	public function sanitize_boolean_digit( $value ) {
-
-		// Default to false if the value was forgotten.
-		if ( empty( $value ) ) {
-			return '0';
-		}
-
-		// Default to false if the value has an unexpected type.
-		if ( ! is_string( $value ) && ! is_numeric( $value ) ) {
-			return '0';
-		}
-
-		// See: https://github.com/ampproject/amp-wp/issues/2335#issuecomment-493209861
-		switch ( substr( (string) $value, 0, 1 ) ) {
-			case '1':
-			case 'y':
-			case 'Y':
-				return '1';
-		}
-
-		return '0';
-	}
-
 	/**
 	 * Sets the layout, and possibly the 'height' and 'width' attributes.
 	 *
diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php
index 9d315f4f529..7b6c6ada5d6 100644
--- a/includes/sanitizers/class-amp-iframe-sanitizer.php
+++ b/includes/sanitizers/class-amp-iframe-sanitizer.php
@@ -273,4 +273,34 @@ private function build_placeholder( $parent_attributes ) {
 		return $placeholder_node;
 	}
 
+	/**
+	 * Sanitizes a boolean character (or string) into a '0' or '1' character.
+	 *
+	 * @param string $value A boolean character to sanitize. If a string containing more than a single
+	 *                      character is provided, only the first character is taken into account.
+	 *
+	 * @return string Returns either '0' or '1'.
+	 */
+	private function sanitize_boolean_digit( $value ) {
+
+		// Default to false if the value was forgotten.
+		if ( empty( $value ) ) {
+			return '0';
+		}
+
+		// Default to false if the value has an unexpected type.
+		if ( ! is_string( $value ) && ! is_numeric( $value ) ) {
+			return '0';
+		}
+
+		// See: https://github.com/ampproject/amp-wp/issues/2335#issuecomment-493209861.
+		switch ( substr( (string) $value, 0, 1 ) ) {
+			case '1':
+			case 'y':
+			case 'Y':
+				return '1';
+		}
+
+		return '0';
+	}
 }