HTML API: Fix invalid comment syntax cases by dmsnell · Pull Request #4256 · WordPress/wordpress-develop

dmsnell · 2023-03-21T20:10:58Z

Status

Satisfy linter requirements
Create Trac ticket #58007-trac
Expand comments, docblocks, add version and Trac ticket refereces
Create backport in Gutenberg

What

Adds support for a few invalid HTML comment forms.

comments created by means of a tag closer with an invalid tag name, e.g. </3>
comments closed with the invalid --!> closer (comments should be closed by --> but if the ! appears it will also close it, in error).
empty tag name elements, which are technically skipped over and aren't comments, e.g. </>

Why

We want to continue to find and handle edge cases in HTML so that the Tag Processor remains reliable in the face of unexpected inputs.

How

Captures the patterns described in the HTML specification for parse errors relating to invalid comment construction. Since these are comments it's appropriate to proceed consuming input until the end, and there's no need to look for other tags or attributes in the process.

cc: @adamziel @gziolo

This is just a safety precaution to mitigate an accidental removal of the pointer increment. No actual problems motivated this change.

costdev

Thanks for the PR @dmsnell!

I've added some docs-related feedback and just one question regarding whether it's possible that an array offset may not exist.

costdev · 2023-03-29T14:07:52Z

src/wp-includes/html-api/class-wp-html-tag-processor.php

+					/*
+					 * Abruptly-closed empty comments are a sequence of dashes followed by `>`.
+					 */


Suggested change

/*

* Abruptly-closed empty comments are a sequence of dashes followed by `>`.

*/

// Abruptly-closed empty comments are a sequence of dashes followed by `>`.

This should use the single line comment format.

inlined in ddcb554

costdev · 2023-03-29T14:11:07Z

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php

+	 * @ticket 58007
+	 * @dataProvider data_next_tag_ignores_invalid_first_character_of_tag_name_comments


Suggested change

* @ticket 58007

* @dataProvider data_next_tag_ignores_invalid_first_character_of_tag_name_comments

* @ticket 58007

*

* @dataProvider data_next_tag_ignores_invalid_first_character_of_tag_name_comments

@ticket and @dataProvider should be separated by an empty line

split in ddcb554

costdev · 2023-03-29T14:11:57Z

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php

+	 * @ticket 58007
+	 * @dataProvider data_next_tag_ignores_invalid_first_character_of_tag_name_comments
+	 *
+	 * @param string $html_with_markers HTML containing an invalid tag closer whose element before and element after contain the "start" and "end" CSS classes.


Suggested change

* @param string $html_with_markers HTML containing an invalid tag closer whose element before and element after contain the "start" and "end" CSS classes.

* @param string $html_with_markers HTML containing an invalid tag closer whose element before

and element after contain the "start" and "end" CSS classes.

Let's break this line to limit the need for horizontal scrolling.

broken in ddcb554.

costdev · 2023-03-29T14:13:39Z

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php

+	 *
+	 * @ticket 58007


Suggested change

*

* @ticket 58007

Data providers don't need the @ticket annotation

removed in ddcb554

costdev · 2023-03-29T14:17:48Z

src/wp-includes/html-api/class-wp-html-tag-processor.php

+							return false;
+						}
+
+						if ( '>' === $html[ $closer_at + 2 ] ) {


Is it possible that $closer_at + 2 may not exist as an offset?

yes it was, and thanks for noticing! I've added new tests and fixes in ae59e1a and 926d491 which validate this particular case of unchecked indices

Thanks @dmsnell! I love the dataset names! 😆

@costdev

Props to @costdev for noting the unchecked indices.

costdev · 2023-04-12T17:22:06Z

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php

+	 *
+	 * @dataProvider data_next_tag_ignores_invalid_first_character_of_tag_name_comments


Suggested change

*

* @dataProvider data_next_tag_ignores_invalid_first_character_of_tag_name_comments

*

* @covers WP_HTML_Tag_Processor::next_tag

*

* @dataProvider data_next_tag_ignores_invalid_first_character_of_tag_name_comments

See this ticket as a reference for the test docblock order/spacing of annotations.

spaced out in 8813327

costdev · 2023-04-12T17:22:25Z

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php

+	 * @ticket 58007
+	 * @covers WP_HTML_Tag_Processor::next_tag


Suggested change

* @ticket 58007

* @covers WP_HTML_Tag_Processor::next_tag

* @ticket 58007

*

* @covers WP_HTML_Tag_Processor::next_tag

spaced out in 8813327

costdev · 2023-04-12T17:22:39Z

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php

+	 * @covers WP_HTML_Tag_Processor::next_tag
+	 * @dataProvider data_html_with_unclosed_comments


Suggested change

* @covers WP_HTML_Tag_Processor::next_tag

* @dataProvider data_html_with_unclosed_comments

* @covers WP_HTML_Tag_Processor::next_tag

*

* @dataProvider data_html_with_unclosed_comments

spaced out in 8813327

costdev · 2023-04-12T17:22:47Z

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php

+	 * @covers WP_HTML_Tag_Processor::next_tag
+	 * @dataProvider data_abruptly_closed_empty_comments


Suggested change

* @covers WP_HTML_Tag_Processor::next_tag

* @dataProvider data_abruptly_closed_empty_comments

* @covers WP_HTML_Tag_Processor::next_tag

*

* @dataProvider data_abruptly_closed_empty_comments

spaced out in 8813327

dmsnell · 2023-04-12T17:33:16Z

@costdev I think I've addressed all your comments now, thanks. must say I tried to get the docblock ordering and formatting right, but for some reason that's consistently hard to do

costdev

Thanks for the updates @dmsnell! Looks like there's just a PHPCS issue to resolve, but other than that this looks good to me 👍

dmsnell · 2023-04-12T19:10:27Z

So there were @costdev - sorry for missing that. I'll double-check once the tests finish again to make sure no remain.

ockham · 2023-04-19T12:23:41Z

src/wp-includes/html-api/class-wp-html-tag-processor.php

+			 * If a non-alpha starts the tag name in a tag closer it's a comment.
+			 * Find the first `>`, which closes the comment.
+			 *
+			 * See https://github.com/WordPress/wordpress-develop/pull/4256


Would it make sense to link to the spec as a somewhat more canonical source instead? While it's fair that this PR states the intent behind this change, I've been itching to see why we can actually do this 😅

How about e.g.

Suggested change

* See https://github.com/WordPress/wordpress-develop/pull/4256

* See https://html.spec.whatwg.org/#parse-error-invalid-first-character-of-tag-name

(which is the reference we're also using in the tests)

good catch; this was probably me pasting the wrong thing and not noticing it

updated in 663055b

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php

…ocument.

ockham

LGTM, thank you!

ockham · 2023-04-20T17:11:28Z

Committed to Core trunk in r55667.

ockham · 2023-04-20T17:17:42Z

Backported to Core's 6.2 branch in r55668.

- Support a few extra invalid comment syntaxes. WordPress/wordpress-develop#4256 - Invalidate bookmarks which have been eliminated by enqueued changes. WordPress/wordpress-develop#4116 - Expose whether the currently-matched tag has the self-closing flag. WordPress/wordpress-develop#4266 - Avoid double-writing an attribute value if given case-variations of the name. WordPress/wordpress-develop#4337

- Support a few extra invalid comment syntaxes. WordPress/wordpress-develop#4256 - Invalidate bookmarks which have been eliminated by enqueued changes. WordPress/wordpress-develop#4116 - Expose whether the currently-matched tag has the self-closing flag. WordPress/wordpress-develop#4266 - Avoid double-writing an attribute value if given case-variations of the name. WordPress/wordpress-develop#4337 - Linting updates from WordPress/wordpress-develop#4360.

- Support a few extra invalid comment syntaxes. WordPress/wordpress-develop#4256 - Invalidate bookmarks which have been eliminated by enqueued changes. WordPress/wordpress-develop#4116 - Expose whether the currently-matched tag has the self-closing flag. WordPress/wordpress-develop#4266 - Avoid double-writing an attribute value if given case-variations of the name. WordPress/wordpress-develop#4337 - Linting updates from WordPress/wordpress-develop#4360. - Avoid losing previous updates in certain cases when seeking earlier in a document. WordPress/wordpress-develop#4345

dmsnell force-pushed the html-api/support-invalid-first-character-in-tag-name-comments branch 2 times, most recently from f841c78 to 25df194 Compare March 21, 2023 23:10

dmsnell added 4 commits March 28, 2023 16:41

HTML API: Support extra non-normative comment constructions

9cc29bf

Fix newly introduced and broken tests

0d8b4f0

Fix implementation detail, move by one not two dashes

af699c0

Move pointer increment into loop condition to avoid infinite loops

2f22fda

This is just a safety precaution to mitigate an accidental removal of the pointer increment. No actual problems motivated this change.

dmsnell force-pushed the html-api/support-invalid-first-character-in-tag-name-comments branch from f6cfa42 to 2f22fda Compare March 28, 2023 23:42

Add references to Trac ticket, annotate tests

23a058a

dmsnell marked this pull request as ready for review March 29, 2023 04:00

dmsnell changed the title ~~WIP: HTML API: Support comments created by invalid tag name in tag closers~~ HTML API: Support comments created by invalid tag name in tag closers Mar 29, 2023

costdev requested changes Mar 29, 2023

View reviewed changes

dmsnell added 3 commits March 29, 2023 08:38

Linting issues

ddcb554

Correct unchecked indexing, feedback

ae59e1a

Props to @costdev for noting the unchecked indices.

Catch another indexing operation

926d491

costdev reviewed Apr 12, 2023

View reviewed changes

Format docblocks

8813327

costdev approved these changes Apr 12, 2023

View reviewed changes

Linting issues

6eaab8b

ockham reviewed Apr 19, 2023

View reviewed changes

tests/phpunit/tests/html-api/wpHtmlTagProcessor.php Show resolved Hide resolved

dmsnell changed the title ~~HTML API: Support comments created by invalid tag name in tag closers~~ HTML API: Fix two invalid comment syntax cases Apr 20, 2023

Update link referring to HTML spec instead of GitHub PR

47d4b4f

dmsnell force-pushed the html-api/support-invalid-first-character-in-tag-name-comments branch from 663055b to 47d4b4f Compare April 20, 2023 12:32

dmsnell added 2 commits April 20, 2023 14:52

Avoid regression where comment opening is next-to-last character in d…

c404124

…ocument.

Update docblock to mentionm abruptly-closed comment support.

bfdb290

Update docblock to indicate support for empty tags

abdd4a2

dmsnell changed the title ~~HTML API: Fix two invalid comment syntax cases~~ HTML API: Fix invalid comment syntax cases Apr 20, 2023

ockham approved these changes Apr 20, 2023

View reviewed changes

dmsnell closed this Apr 20, 2023

dmsnell deleted the html-api/support-invalid-first-character-in-tag-name-comments branch April 20, 2023 17:10

dmsnell mentioned this pull request Apr 20, 2023

HTML API: Backports from Core WordPress/gutenberg#49966

Merged

		* @ticket 58007
		* @dataProvider data_next_tag_ignores_invalid_first_character_of_tag_name_comments

	* @param string $html_with_markers HTML containing an invalid tag closer whose element before and element after contain the "start" and "end" CSS classes.
	* @param string $html_with_markers HTML containing an invalid tag closer whose element before
	and element after contain the "start" and "end" CSS classes.

		* @covers WP_HTML_Tag_Processor::next_tag
		* @dataProvider data_html_with_unclosed_comments

		* @covers WP_HTML_Tag_Processor::next_tag
		* @dataProvider data_abruptly_closed_empty_comments

	* See https://github.com/WordPress/wordpress-develop/pull/4256
	* See https://html.spec.whatwg.org/#parse-error-invalid-first-character-of-tag-name

Conversation

dmsnell commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status

What

Why

How

Uh oh!

costdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmsnell commented Apr 12, 2023

Uh oh!

costdev left a comment

Choose a reason for hiding this comment

Uh oh!

dmsnell commented Apr 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ockham left a comment

Choose a reason for hiding this comment

Uh oh!

ockham commented Apr 20, 2023

Uh oh!

ockham commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmsnell commented Mar 21, 2023 •

edited

Loading