Normalize frameborder=no|yes to frameborder=0|1 in iframe sanitizer

Cherry-picks #3064
ampproject · Aug 27, 2019 · 8a063e1 · 8a063e1
1 parent 32d2d70
commit 8a063e1
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 6 deletions.
diff --git a/includes/sanitizers/class-amp-iframe-sanitizer.php b/includes/sanitizers/class-amp-iframe-sanitizer.php
@@ -132,6 +132,11 @@ 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 );
 			}
@@ -155,7 +160,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 +194,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':
@@ -271,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';
+	}
 }
diff --git 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,
+				],
+			],
 		];
 	}