Skip to content

HTML API: Fix possible infinite loop parsing incomplete script tags #7128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed

HTML API: Fix possible infinite loop parsing incomplete script tags #7128

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c98f64d
Add test case for infinite loop in skip_script_data
sirreal Aug 2, 2024
8da0bd1
Fix infinite loop in skip_script_data
sirreal Aug 1, 2024
8c3fe3a
Use prefix increment consistent with the rest of the method
sirreal Aug 2, 2024
b8f0027
Fix the infinite loop by ensuring advance is reached
sirreal Aug 2, 2024
663b94a
Update tests with ticket and another case
sirreal Aug 2, 2024
53b2e64
Merge branch 'trunk' into html-api/fix-script-tag-infinite-loop
dmsnell Aug 2, 2024
aa92098
Return early instead of breaking to the end of the function.
dmsnell Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/wp-includes/html-api/class-wp-html-tag-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1431,8 +1431,15 @@ private function skip_script_data(): bool {
continue;
}

// Everything of interest past here starts with "<".
if ( $at + 1 >= $doc_length || '<' !== $html[ $at++ ] ) {
if ( $at + 1 >= $doc_length ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've chosen to break here because there's no way to find the script closer. The document is incomplete, so breaking and returning false below enters the correct code path. This could also return false directly instead of breaking out of the loop.

This condition could likely be increased. We need to match at least <!-- so we could apply this with at least $at + 3.

Suggested change
if ( $at + 1 >= $doc_length ) {
// The parser needs to match at least `<!--` from here.
if ( $at + 3 >= $doc_length ) {

I also think this could apply more aggressive length checks earlier, then skip all of the subsequent length checks. Ultimately, we need to find a script closer in order to exit true. It doesn't matter how much of the script contents are parsed if it can't be closed the parser will stop on incomplete input. After this line:

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

Line 1416 in a799101

$at += strcspn( $html, '-<', $at );

We could include this to eagerly reach incomplete input if we know a closer cannot be found.

if ( $at + 8 > $doc_length ) { return false; }

Here's my thinking:

/* Things could happen in the loop, but they're irrelevant if a `</script>` cannot be found  */

// $at:  V
<script> ----->␃
// $at+8         ^

/* A script closer could fit here and is ultimately found */

// $at:          V
<script> bla bla </script>␃
// $at+8                 ^

/* A script closer would fit so processing continues */
// $at:          V
<script> bla bla -            more document…␃
// $at+8                 ^

Maybe the proposed bugfix could be landed and included in WordPress 6.6, with more optimizations included landed in another change to trunk.

@dmsnell curious to hear your thoughts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are valid points. what I like about advancing one character at a time is that in some cases, I leaned on that to ensure we didn't count wrong. a targeting fix sounds nice now, and then a follow-up can be tested in isolation, particularly for performance, for skipping ahead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I also like returning false directly - no need to further confuse control flow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've started work on this in #9230

break;
}

/*
* Everything of interest past here starts with "<".
* Check this character and advance position regardless.
*/
if ( '<' !== $html[ $at++ ] ) {
continue;
}

Expand Down
28 changes: 28 additions & 0 deletions tests/phpunit/tests/html-api/wpHtmlTagProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2875,4 +2875,32 @@ public function insert_after( $new_html ) {
'Should have properly applied the update from in front of the cursor.'
);
}

/**
* Test an infinite loop bugfix in incomplete script tag parsing.
*
* @small
*
* @ticket 61810
*/
public function test_script_tag_processing_no_infinite_loop_final_dash() {
$processor = new WP_HTML_Tag_Processor( '<script>-' );

$this->assertFalse( $processor->next_tag() );
$this->assertTrue( $processor->paused_at_incomplete_token() );
}

/**
* Test an infinite loop bugfix in incomplete script tag parsing.
*
* @small
*
* @ticket 61810
*/
public function test_script_tag_processing_no_infinite_loop_final_left_angle_bracket() {
$processor = new WP_HTML_Tag_Processor( '<script><' );

$this->assertFalse( $processor->next_tag() );
$this->assertTrue( $processor->paused_at_incomplete_token() );
}
}
Loading