Tag Processor: Decode/Encode HTML attributs in get_attribute/set_attr…

…ibute to prevent XSS (#44447)

Co-authored-by: Dennis Snell <[email protected]>

lib/experimental/html/class-wp-html-tag-processor.php

@@ -908,7 +908,9 @@ public function get_attribute( $name ) {
  return true;
  }
 
- return substr( $this->html, $attribute->value_starts_at, $attribute->value_length );
+ $raw_value = substr( $this->html, $attribute->value_starts_at, $attribute->value_length );
+
+ return html_entity_decode( $raw_value );
  }
 
  /**
@@ -945,6 +947,8 @@ public function get_tag() {
  * - When `true` is passed as the value, then only the attribute name is added to the tag.
  * - When `false` is passed, the attribute gets removed if it existed before.
  *
+ * For string attributes, the value is escaped using the `esc_attr` function.
+ *
  * @since 6.2.0
  *
  * @param string $name The attribute name to target.
@@ -968,8 +972,7 @@ public function set_attribute( $name, $value ) {
  if ( true === $value ) {
  $updated_attribute = $name;
  } else {
- // @TODO: What escaping and sanitization do we need here?
- $escaped_new_value = str_replace( '"', '"', $value );
+ $escaped_new_value = esc_attr( $value );
  $updated_attribute = "{$name}=\"{$escaped_new_value}\"";
  }
 

phpunit/html/wp-html-tag-processor-test.php → phpunit/html/wp-html-tag-processor-standalone-test.php

@@ -1,14 +1,23 @@
 <?php
 /**
  * Unit tests covering WP_HTML_Tag_Processor functionality.
+ * This file takes about 100ms to run because it does not load
+ * any WordPress libraries:
+ *
+ * ```
+ * ./vendor/bin/phpunit --no-configuration ./phpunit/html/wp-html-tag-processor-test.php
+ * ```
+ *
+ * Put all new WP_HTML_Tag_Processor tests here, and only add new cases to
+ * wp-html-tag-processor-test-wp.php when they cannot run without WordPress.
  *
  * @package WordPress
  * @subpackage HTML
  */
 
 if ( ! function_exists( 'esc_attr' ) ) {
- function esc_attr( $s ) {
- return str_replace( '"', '&quot;', $s );
+ function esc_attr( $string ) {
+ return htmlspecialchars( $string, ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401, 'utf-8', false );
  }
 }
 
@@ -23,7 +32,7 @@ abstract class WP_UnitTestCase extends \PHPUnit\Framework\TestCase {}
  *
  * @coversDefaultClass WP_HTML_Tag_Processor
  */
-class WP_HTML_Tag_Processor_Test extends WP_UnitTestCase {
+class WP_HTML_Tag_Processor_Standalone_Test extends WP_UnitTestCase {
  const HTML_SIMPLE = '<div id="first"><span id="second">Text</span></div>';
  const HTML_WITH_CLASSES = '<div class="main with-border" id="first"><span class="not-main bold with-border" id="second">Text</span></div>';
  const HTML_MALFORMED = '<div><span class="d-md-none" Notifications</span><span class="d-none d-md-inline">Back to notifications</span></div>';
@@ -134,6 +143,17 @@ public function test_get_attribute_returns_string_for_truthy_attributes() {
  $this->assertSame( 'true', $p->get_attribute( 'hidden' ), 'Accessing a hidden="true" attribute value did not return "true"' );
  }
 
+ /**
+ * @ticket 56299
+ *
+ * @covers WP_HTML_Tag_Processor::get_attribute
+ */
+ public function test_get_attribute_decodes_html_character_references() {
+ $p = new WP_HTML_Tag_Processor( '<div id="the &quot;grande&quot; is &lt; &#x033;&#50;oz&dagger;"></div>' );
+ $p->next_tag();
+ $this->assertSame( 'the "grande" is < 32oz†', $p->get_attribute( 'id' ), 'HTML Attribute value was returned without decoding character references' );
+ }
+
  /**
  * @ticket 56299
  *
@@ -235,6 +255,68 @@ public function test_set_attribute_on_a_non_existing_tag_does_not_change_the_mar
  );
  }
 
+ /**
+ * Passing a double quote inside of an attribute values could lead to an XSS attack as follows:
+ *
+ * <code>
+ * $p = new WP_HTML_Tag_Processor( '<div class="header"></div>' );
+ * $p->next_tag();
+ * $p->set_attribute('class', '" onclick="alert');
+ * echo $p;
+ * // <div class="" onclick="alert"></div>
+ * </code>
+ *
+ * To prevent it, `set_attribute` calls `esc_attr()` on its given values.
+ *
+ * <code>
+ * <div class="&quot; onclick=&quot;alert"></div>
+ * </code>
+ *
+ * @ticket 56299
+ *
+ * @dataProvider data_set_attribute_escapable_values
+ * @covers set_attribute
+ */
+ public function test_set_attribute_prevents_xss( $attribute_value ) {
+ $p = new WP_HTML_Tag_Processor( '<div></div>' );
+ $p->next_tag();
+ $p->set_attribute( 'test', $attribute_value );
+
+ /*
+ * Testing the escaping is hard using tools that properly parse
+ * HTML because they might interpret the escaped values. It's hard
+ * with tools that don't understand HTML because they might get
+ * confused by improperly-escaped values.
+ *
+ * For this test, since we control the input HTML we're going to
+ * do what looks like the opposite of what we want to be doing with
+ * this library but are only doing so because we have full control
+ * over the content and because we want to look at the raw values.
+ */
+ $match = null;
+ preg_match( '~^<div test=(.*)></div>$~', (string) $p, $match );
+ list( , $actual_value ) = $match;
+
+ $this->assertEquals( $actual_value, '"' . esc_attr( $attribute_value ) . '"' );
+ }
+
+ /**
+ * Data provider with HTML attribute values that might need escaping.
+ */
+ public function data_set_attribute_escapable_values() {
+ return array(
+ array( '"' ),
+ array( '&quot;' ),
+ array( '&' ),
+ array( '&amp;' ),
+ array( '&euro;' ),
+ array( "'" ),
+ array( '<>' ),
+ array( '&quot";' ),
+ array( '" onclick="alert(\'1\');"><span onclick=""></span><script>alert("1")</script>' ),
+ );
+ }
+
  /**
  * @ticket 56299
  *
@@ -999,12 +1081,12 @@ public function data_malformed_tag() {
 
  $examples['HTML tag opening inside attribute value'] = array(
  '<pre id="<code" class="wp-block-code <code is poetry&gt;"><code>This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
- '<pre foo="bar" id="<code" class="wp-block-code <code is poetry&gt; firstTag"><code class="secondTag">This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
+ '<pre foo="bar" id="<code" class="wp-block-code &lt;code is poetry&gt; firstTag"><code class="secondTag">This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
  );
 
  $examples['HTML tag brackets in attribute values and data markup'] = array(
  '<pre id="<code-&gt;-block-&gt;" class="wp-block-code <code is poetry&gt;"><code>This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
- '<pre foo="bar" id="<code-&gt;-block-&gt;" class="wp-block-code <code is poetry&gt; firstTag"><code class="secondTag">This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
+ '<pre foo="bar" id="<code-&gt;-block-&gt;" class="wp-block-code &lt;code is poetry&gt; firstTag"><code class="secondTag">This &lt;is> a &lt;strong is="true">thing.</code></pre><span>test</span>',
  );
 
  $examples['Single and double quotes in attribute value'] = array(

phpunit/html/wp-html-tag-processor-wp-test.php

@@ -0,0 +1,91 @@
+<?php
+/**
+ * Unit tests covering WP_HTML_Tag_Processor functionality.
+ * This file is only for tests that require WordPress libraries.
+ * Put all other tests in wp-html-tag-processor-test-standalone.php.
+ *
+ * Run these tests locally as follows:
+ *
+ * ```
+ * npx wp-env run phpunit "phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --filter WP_HTML_Tag_Processor_Test_WP"
+ * ```
+ *
+ * @package WordPress
+ * @subpackage HTML
+ */
+
+require_once __DIR__ . '/../../lib/experimental/html/index.php';
+
+/**
+ * @group html
+ *
+ * @coversDefaultClass WP_HTML_Tag_Processor
+ */
+class WP_HTML_Tag_Processor_Test_WP extends WP_UnitTestCase {
+
+ /**
+ * Passing a double quote inside of an attribute values could lead to an XSS attack as follows:
+ *
+ * <code>
+ * $p = new WP_HTML_Tag_Processor( '<div class="header"></div>' );
+ * $p->next_tag();
+ * $p->set_attribute('class', '" onclick="alert');
+ * echo $p;
+ * // <div class="" onclick="alert"></div>
+ * </code>
+ *
+ * To prevent it, `set_attribute` calls `esc_attr()` on its given values.
+ *
+ * <code>
+ * <div class="&quot; onclick=&quot;alert"></div>
+ * </code>
+ *
+ * @ticket 56299
+ *
+ * @dataProvider data_set_attribute_escapable_values
+ * @covers set_attribute
+ */
+ public function test_set_attribute_prevents_xss( $value_to_set, $expected_result ) {
+ $p = new WP_HTML_Tag_Processor( '<div></div>' );
+ $p->next_tag();
+ $p->set_attribute( 'test', $value_to_set );
+
+ /*
+ * Testing the escaping is hard using tools that properly parse
+ * HTML because they might interpret the escaped values. It's hard
+ * with tools that don't understand HTML because they might get
+ * confused by improperly-escaped values.
+ *
+ * For this test, since we control the input HTML we're going to
+ * do what looks like the opposite of what we want to be doing with
+ * this library but are only doing so because we have full control
+ * over the content and because we want to look at the raw values.
+ */
+ $match = null;
+ preg_match( '~^<div test=(.*)></div>$~', (string) $p, $match );
+ list( , $actual_value ) = $match;
+
+ $this->assertEquals( $actual_value, '"' . $expected_result . '"' );
+ }
+
+ /**
+ * Data provider with HTML attribute values that might need escaping.
+ */
+ public function data_set_attribute_escapable_values() {
+ return array(
+ array( '"', '&quot;' ),
+ array( '&quot;', '&quot;' ),
+ array( '&', '&amp;' ),
+ array( '&amp;', '&amp;' ),
+ array( '&euro;', '&euro;' ),
+ array( "'", '&#039;' ),
+ array( '<>', '&lt;&gt;' ),
+ array( '&quot";', '&amp;quot&quot;;' ),
+ array(
+ '" onclick="alert(\'1\');"><span onclick=""></span><script>alert("1")</script>',
+ '&quot; onclick=&quot;alert(&#039;1&#039;);&quot;&gt;&lt;span onclick=&quot;&quot;&gt;&lt;/span&gt;&lt;script&gt;alert(&quot;1&quot;)&lt;/script&gt;',
+ ),
+ );
+ }
+
+}