diff --git a/STATUS.md b/STATUS.md new file mode 100644 index 0000000000000..0d7f17d152652 --- /dev/null +++ b/STATUS.md @@ -0,0 +1,118 @@ +## Critical Analysis: WP_HTML_Template vs. Ticket #60229 Requirements + +I've examined ticket #60229, the current branch implementation, and the test suite. Here's my analysis: + +--- + +### What's Working Well + +**1. Core Safety Goal Achieved** +The implementation delivers on the primary promise: context-aware auto-escaping. The code correctly escapes `&`, `<`, `>`, `"`, `'` in both text and attribute contexts. Tests confirm placeholders can't be injected via replacement values (non-recursive replacement). + +**2. Funky Comment Syntax** +The `` syntax is a smart choice—it's valid HTML (parsed as a funky comment), can't be nested by construction, and is visually distinctive. The parsing handles whitespace around names correctly. + +**3. Nested Templates** +Template composition (`WP_HTML_Template` as replacement value) works for text context with proper escaping propagation. The rejection of templates in attribute context is the right safety call. + +**4. Compile-once Design** +Lazy compilation with cached edits (`$edits` array) is efficient for template reuse. + +**5. Boolean Attribute Support** +Supply `true` to create a boolean attribute (`disabled=""` + `true` → `disabled`), or `false`/`null` to remove an attribute entirely. Only works for whole-attribute placeholders—partial placeholders reject boolean values. + +--- + +### What's Missing from the Ticket Requirements + +**1. No URL Escaping (Ticket TODO)** +The ticket explicitly says "does not escape URLs differently than other attributes." The XSS test shows `javascript:alert("xss")` only escapes quotes—no `esc_url()` equivalent. This is a security gap for `href`/`src` attributes. + +**2. No Attribute Spread** +Ticket comment 9 discusses "spread" attributes for making tags placeholders. Not implemented. + +**3. Missing Output Format Methods** +Ticket TODO lists `->final_output_to_browser()`, `->final_output_to_plaintext()`, `->final_output_to_markdown()`, etc. None exist. + +**4. Embed Replacement in Tag Processor (Ticket TODO)** +The ticket wants replacement embedded in the Tag Processor. Current implementation uses a separate class with its own parsing pass. + +--- + +### Overlooked Issues in the Ticket + +**1. RAWTEXT/RCDATA Element Handling Is Half-Baked** +Tests show placeholders inside `', + ), + <<<'HTML' + … Read more: <script>alert("xss")</script> + HTML, + ); + + // src/wp-includes/theme.php:978-979 - Theme error with name + yield 'theme.php:978 - theme error message' => array( + <<<'HTML' + Error: Current WordPress and PHP versions do not meet minimum requirements for . + HTML, + array( + 'theme_name' => 'Twenty Twenty-Five', + ), + <<<'HTML' + Error: Current WordPress and PHP versions do not meet minimum requirements for Twenty Twenty-Five. + HTML, + ); + + // src/wp-admin/includes/privacy-tools.php:404 - Code tag in error + yield 'privacy-tools.php:404 - code in error message' => array( + 'The post meta must be an array.', + array( + 'meta_key' => '_export_data_grouped', + ), + 'The _export_data_grouped post meta must be an array.', + ); + + /* + * Group 3: Edge cases. + */ + + // Placeholder reuse (same placeholder multiple times) + yield 'placeholder reuse' => array( + ' ', + array( + 'id' => 'user_name', + ), + ' ', + ); + + // Named placeholders + yield 'named placeholders' => array( + ' by ', + array( + 'postUrl' => 'https://example.com/post/', + 'postTitle' => 'Post Title', + 'authorUrl' => 'https://example.com/author/', + 'authorName' => 'Author Name', + ), + 'Post Title by Author Name', + ); + + // Nested template (pre-escaped HTML) + yield 'nested template for complex structure' => array( + '
', + array( + 'icon' => WP_HTML_Template::from( '' ), + 'message' => 'Something went wrong.', + ), + '
Something went wrong.
', + ); + + // Empty replacement + yield 'empty replacement value' => array( + '

Hello

', + array( + 'suffix' => '', + ), + '

Hello

', + ); + + // HTML entities in template (should be preserved) + yield 'HTML entities in template' => array( + '

', + array( + 'quote' => 'Hello World', + ), + '

“Hello World”

', + ); + + // Multiple attributes on same element + yield 'multiple attributes on element' => array( + '', + array( + 'type' => 'text', + 'name' => 'user_email', + 'value' => 'test@example.com', + 'placeholder' => 'Enter your email', + ), + '', + ); + + // Attribute value with quotes and special characters + yield 'attribute with quotes and ampersands' => array( + 'Link', + array( + 'url' => 'https://example.com/?a=1&b=2', + 'title' => <<<'TEXT' + Click "here" for Tom & Jerry + TEXT, + ), + <<<'HTML' + Link + HTML, + ); + + // Self-closing void element + yield 'self-closing meta tag' => array( + '', + array( + 'name' => 'description', + 'content' => <<<'TEXT' + A page about "cats" & dogs + TEXT, + ), + <<<'HTML' + + HTML, + ); + + // src/wp-includes/blocks/avatar.php:68 - Complex link with aria-label + yield 'blocks/avatar.php:68 - avatar link' => array( + <<<'HTML' + + HTML, + array( + 'url' => 'https://example.com/author/johndoe/', + 'target' => '_blank', + 'aria_label' => '(John Doe author archive, opens in a new tab)', + 'inner' => WP_HTML_Template::from( 'John Doe' ), + ), + <<<'HTML' + John Doe + HTML, + ); + } + + /** + * Verifies nested templates work correctly in a definition list. + * + * @ticket 60229 + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_nested_templates_in_definition_list() { + $row_template = WP_HTML_Template::from( "
\n
" ); + + $row_replacements = array(); + for ( $i = 1; $i <= 3; $i++ ) { + $row_replacements[ "row-{$i}" ] = $row_template->bind( + array( + 'term' => "Term \"{$i}\"", + 'definition' => WP_HTML_Template::from( 'IYKYK: ' ) + ->bind( + array( + 'i' => (string) $i, + 'expansion' => '"If You Know You Know"', + ) + ), + ) + ); + } + + $result = WP_HTML_Template::from( + <<<'HTML' +
+ + + +
+ HTML + )->bind( $row_replacements )->render(); + + $expected = + <<<'HTML' +
+
Term "1"
+
IYKYK: 1
+
Term "2"
+
IYKYK: 2
+
Term "3"
+
IYKYK: 3
+
+ HTML; + + $this->assertEqualHTML( $expected, $result ); + } + + /** + * Verifies table templates are not yet supported. + * + * @ticket 60229 + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_table_templates_not_yet_supported() { + $this->markTestSkipped( 'IN TABLE templates are not supported yet.' ); + $header_tpl = WP_HTML_Template::from( '' ) + ->bind( + array( + 'ID' => 'ID', + 'name' => 'Name', + 'value' => 'Value', + 'link' => 'Link', + ) + ); + $row_tpl = WP_HTML_Template::from( '' ); + + $row_gen = ( function () { + static $i = 1; + yield array( + 'ID' => $i, + 'name' => 'Name {$i}', + 'value' => WP_HTML_Template::from( 'Value {$i}' )->bind( array( 'i' => $i ) ), + 'link' => WP_HTML_Template::from( '' ) + ->bind( + array( + 'url' => '/example/1', + 'link-name' => 'Click here', + ) + ), + ); + } )(); + + $result = WP_HTML_Template::from( + <<<'HTML' + + + + + + + HTML + )->bind( + array( + 'header' => $header_tpl, + 'row-1' => $row_tpl->bind( $row_gen->next() ), + 'row-2' => $row_tpl->bind( $row_gen->next() ), + 'row-3' => $row_tpl->bind( $row_gen->next() ), + ) + )->render(); + + $expected = + <<<'HTML' + HTML; + $this->assertEqualHTML( $expected, $result ); + } + + /** + * Verifies that attributes are replaced in atomic elements (SCRIPT, STYLE, TITLE). + * + * These elements have special parsing rules that skip their content, + * but attributes should still be processed normally. + * + * @ticket 60229 + * + * @dataProvider data_atomic_element_attributes + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_atomic_element_attributes_are_replaced( string $template_string, array $replacements, string $expected ) { + $result = T::from( $template_string )->bind( $replacements )->render(); + $this->assertEqualHTML( $expected, $result ); + } + + public static function data_atomic_element_attributes() { + return array( + 'SCRIPT element attributes' => array( + '', + array( 'src' => '/js/app.js' ), + '', + ), + + 'STYLE element attributes' => array( + '', + array( 'media' => 'screen' ), + '', + ), + + 'TITLE element attributes' => array( + 'Page Title', + array( 'lang' => 'en' ), + 'Page Title', + ), + + 'TEXTAREA element attributes' => array( + '', + array( 'name' => 'my-textarea' ), + '', + ), + ); + } + + /** + * Verifies content placeholder behavior in elements with special parsing. + * + * - RAWTEXT elements (SCRIPT, STYLE): Content is skipped, placeholders preserved literally. + * - RCDATA elements (TITLE, TEXTAREA): Content is processed but placeholders are not + * recognized - they're treated as literal text and HTML-escaped. + * + * With strict validation, providing a replacement for a placeholder that won't be + * processed (inside SCRIPT/STYLE/TITLE/TEXTAREA) is an unused key error. + * + * @ticket 60229 + * + * @dataProvider data_atomic_element_content_placeholders + * + * @todo Implement correct handling of atomic elements. + * + * @covers ::from + * @covers ::render + */ + public function test_special_element_content_placeholder_behavior( string $template_string, string $expected ) { + $result = T::from( $template_string )->render(); + $this->assertEqualHTML( $expected, $result ); + } + + public static function data_atomic_element_content_placeholders() { + return array( + // RAWTEXT elements (SCRIPT, STYLE): Content is truly skipped, placeholders preserved literally. + 'SCRIPT content placeholder preserved' => array( + '', + '', + ), + + 'STYLE content placeholder preserved' => array( + '', + '', + ), + + // RCDATA elements (TITLE, TEXTAREA): Content is processed but placeholder + // patterns are not recognized - they're treated as literal text and escaped. + 'TITLE content placeholder escaped' => array( + 'Hello </%name>', + 'Hello </%name>', + ), + + 'TEXTAREA content placeholder escaped' => array( + '', + '', + ), + ); + } + + /** + * Verifies leading newline behavior in PRE elements. + * + * HTML5 specifies that a single leading newline immediately after the + *
 start tag is ignored. This test documents the template behavior.
+	 *
+	 * @ticket 60229
+	 *
+	 * @dataProvider data_pre_element_leading_newline
+	 *
+	 * @covers ::from
+	 * @covers ::bind
+	 * @covers ::render
+	 */
+	public function test_pre_element_leading_newline_behavior( string $template_string, array $replacements, string $expected ) {
+		$this->markTestSkipped( 'PRE newline handling is not yet correct.' );
+
+		$result = T::from( $template_string )->bind( $replacements )->render();
+		$this->assertEqualHTML( $expected, $result );
+	}
+
+	public static function data_pre_element_leading_newline() {
+		return array(
+			'PRE without newline'                         => array(
+				'
', + array( 'code' => "line1\nline2" ), + "
line1\nline2
", + ), + + 'PRE with newline' => array( + "
\n
", + array( 'code' => "line1\nline2" ), + "
line1\nline2
", + ), + + 'PRE with newline in replacement' => array( + "
\n
", + array( 'code' => "line1\nline2" ), + "
line1\nline2
", + ), + + 'PRE with newline and newline in replacement' => array( + "
\n
", + array( 'code' => "\nline1\nline2" ), + "
\n\nline1\nline2
", + ), + + 'PRE with newline, newline replacement, and additional contents' => array( + "
\n
", + array( 'code' => "\nline1" ), + "
\n\nline1
", + ), + ); + } + + /** + * Verifies bind() warns on missing replacement key. + * + * @ticket 60229 + * + * @covers ::bind + * + * @expectedIncorrectUsage WP_HTML_Template::bind + */ + public function test_bind_warns_on_missing_key() { + $template = T::from( '

' ); + $template->bind( array( 'name' => 'Alice' ) ); + } + + /** + * Verifies bind() warns on unused replacement key. + * + * @ticket 60229 + * + * @covers ::bind + * + * @expectedIncorrectUsage WP_HTML_Template::bind + */ + public function test_bind_warns_on_unused_key() { + $template = T::from( '

' ); + $template->bind( + array( + 'name' => 'Alice', + 'extra' => 'ignored', + ) + ); + } + + /** + * Verifies bind() warns when template used in attribute context. + * + * @ticket 60229 + * + * @covers ::bind + * + * @expectedIncorrectUsage WP_HTML_Template::bind + */ + public function test_bind_warns_on_template_in_attribute_context() { + $template = T::from( '' ); + $template->bind( array( 'html' => T::from( 'nested' ) ) ); + } + + /** + * Verifies that static text around placeholders in attributes is escaped. + * + * @ticket 60229 + * + * @dataProvider data_escapes_static_text_around_placeholder_in_attribute + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_escapes_static_text_around_placeholder_in_attribute( string $template_string, array $replacements, string $expected ) { + $result = T::from( $template_string )->bind( $replacements )->render(); + $this->assertEqualHTML( $expected, $result ); + } + + public static function data_escapes_static_text_around_placeholder_in_attribute() { + return array( + 'leading static text (prefix before placeholder)' => array( + 'Link', + array( 'slug' => 'hello' ), + 'Link', + ), + + 'trailing static text (suffix after placeholder)' => array( + 'Link', + array( 'slug' => 'hello' ), + 'Link', + ), + + 'ampersand in trailing static text must be escaped' => array( + 'Link', + array( 'base' => '/search?q=test' ), + 'Link', + ), + + 'ampersand entity in leading static text not double-escaped' => array( + 'Link', + array( 'val' => '2' ), + 'Link', + ), + + 'character reference in trailing static text preserved' => array( + '', + array( 'placeholder' => '' ), + '', + ), + + 'two placeholders in href' => array( + 'link', + array( + 'base' => '/posts', + 'slug' => 'hello-world', + ), + 'link', + ), + + 'three placeholders building URL' => array( + 'link', + array( + 'scheme' => 'https', + 'host' => 'example.com', + 'path' => 'page', + ), + 'link', + ), + + 'adjacent placeholders (no separator)' => array( + '', + array( + 'a' => 'Hello', + 'b' => 'World', + ), + '', + ), + + 'placeholders with static text between' => array( + 'link', + array( + 'base' => '/search', + 'page' => '2', + 'sort' => 'date', + ), + 'link', + ), + + 'same placeholder repeated in attribute' => array( + '', + array( 'val' => 'test' ), + '', + ), + + 'escaping in multiple placeholders' => array( + 'link', + array( + 'base' => '/search', + 'query' => 'a&b"d', + ), + 'link', + ), + + 'multiple placeholders across multiple attributes' => array( + 'link', + array( + 'url' => '/page', + 'a' => 'Alice', + 'b' => 'Bob', + ), + 'link', + ), + ); + } + + /** + * @ticket 60229 + */ + public function test_warns_on_unrecognized_replacements() { + $this->setExpectedIncorrectUsage( 'WP_HTML_Template::bind' ); + $template = T::from( '' ); + $template->bind( array( 'extra' => 'oops' ) ); + } + + /** + * @ticket 60229 + */ + public function test_warns_on_omit_replacement() { + $this->setExpectedIncorrectUsage( 'WP_HTML_Template::bind' ); + $template = T::from( '' ); + $template->bind( array() ); + } + + /** + * Verifies that boolean true creates a boolean attribute. + * + * @ticket 60229 + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_boolean_true_creates_boolean_attribute() { + $result = T::from( '' ) + ->bind( array( 'disabled' => true ) ) + ->render(); + $this->assertEqualHTML( '', $result ); + } + + /** + * Verifies that boolean false removes the attribute. + * + * @ticket 60229 + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_boolean_false_removes_attribute() { + $result = T::from( '' ) + ->bind( array( 'disabled' => false ) ) + ->render(); + $this->assertEqualHTML( '', $result ); + } + + /** + * Verifies that null removes the attribute (same as false). + * + * @ticket 60229 + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_null_removes_attribute() { + $result = T::from( '' ) + ->bind( array( 'class' => null ) ) + ->render(); + $this->assertEqualHTML( '', $result ); + } + + /** + * Verifies that boolean with partial placeholder returns false. + * + * @ticket 60229 + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_partial_placeholder_rejects_boolean() { + $result = T::from( '' ) + ->bind( array( 'suffix' => true ) ) + ->render(); + $this->assertFalse( $result ); + } + + /** + * @ticket 60229 + * + * @dataProvider data_boolean_attribute_handling + * + * @covers ::from + * @covers ::bind + * @covers ::render + */ + public function test_boolean_attribute_handling( string $template, array $replacements, string $expected ) { + $result = T::from( $template )->bind( $replacements )->render(); + $this->assertEqualHTML( $expected, $result ); + } + + public static function data_boolean_attribute_handling() { + return array( + 'true creates boolean attribute' => array( + '', + array( 'disabled' => true ), + '', + ), + + 'false removes attribute' => array( + '', + array( 'disabled' => false ), + '', + ), + + 'null removes attribute' => array( + '', + array( 'class' => null ), + '', + ), + + 'empty string keeps attribute with empty value' => array( + '', + array( 'value' => '' ), + '', + ), + + 'mixed boolean and string replacements' => array( + '', + array( + 'd' => true, + 'v' => 'test', + ), + '', + ), + + 'multiple attributes, one removed' => array( + '', + array( + 'c' => false, + 'i' => 'my-id', + ), + '', + ), + + 'single-quoted attribute with boolean' => array( + "", + array( 'disabled' => true ), + '', + ), + ); + } + + /** + * Verifies render() returns false for integer replacement value. + * + * @ticket 60229 + * + * @covers ::render + */ + public function test_render_returns_false_for_integer_replacement() { + $result = T::from( '

' )->bind( array( 'val' => 123 ) )->render(); + $this->assertFalse( $result ); + } + + /** + * Verifies render() returns false for array replacement value. + * + * @ticket 60229 + * + * @covers ::render + */ + public function test_render_returns_false_for_array_replacement() { + $result = T::from( '

' )->bind( array( 'val' => array( 'a', 'b' ) ) )->render(); + $this->assertFalse( $result ); + } + + /** + * Verifies render() returns false for object replacement value without __toString. + * + * @ticket 60229 + * + * @covers ::render + */ + public function test_render_returns_false_for_object_replacement() { + $result = T::from( '

' )->bind( array( 'val' => new stdClass() ) )->render(); + $this->assertFalse( $result ); + } + + /** + * Verifies render() returns false for null replacement value. + * + * @ticket 60229 + * + * @covers ::render + */ + public function test_render_returns_false_for_null_replacement() { + $result = T::from( '

' )->bind( array( 'val' => null ) )->render(); + $this->assertFalse( $result ); + } + + /** + * Verifies render() returns false for boolean replacement value. + * + * @ticket 60229 + * + * @covers ::render + */ + public function test_render_returns_false_for_boolean_replacement() { + $result = T::from( '

' )->bind( array( 'val' => true ) )->render(); + $this->assertFalse( $result ); + } +}