Skip to content

Conversation

@sirreal
Copy link
Owner

@sirreal sirreal commented Jul 23, 2025

Trac ticket:


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@sirreal sirreal force-pushed the fix/62797-inline-script-tag-escape-alternate branch 2 times, most recently from 3052284 to 4b9aa42 Compare July 30, 2025 08:03
@sirreal sirreal requested a review from Copilot July 30, 2025 16:54

This comment was marked as outdated.

@sirreal sirreal requested a review from Copilot July 30, 2025 19:32

This comment was marked as outdated.

@sirreal sirreal requested a review from Copilot July 30, 2025 21:23

This comment was marked as outdated.

@sirreal sirreal requested a review from Copilot August 6, 2025 11:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes inline script tag escaping to prevent script injection vulnerabilities when embedding data in HTML script tags. The changes improve security by properly escaping dangerous patterns that could break out of script contexts.

  • Implements proper escaping for script tag content that could close or interfere with script elements
  • Adds comprehensive detection of JavaScript vs non-JavaScript script tags based on HTML specification
  • Updates inline script functions to use the new HTML Tag Processor for safe content handling

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/wp-includes/html-api/class-wp-html-tag-processor.php Adds JavaScript script tag detection and Unicode escaping for dangerous patterns
src/wp-includes/script-loader.php Updates inline script generation to use HTML Tag Processor for safe escaping
src/wp-includes/functions.wp-scripts.php Improves script tag detection and content extraction using HTML Tag Processor
src/wp-includes/block-editor.php Adds JSON_UNESCAPED_SLASHES flag to prevent double-escaping of slashes
tests/phpunit/tests/html-api/wpHtmlTagProcessorModifiableText.php Adds comprehensive test coverage for script tag escaping scenarios
tests/phpunit/tests/blocks/editor.php Updates test expectations and adds security test for script tag closure

*
* @see https://html.spec.whatwg.org/multipage/scripting.html#prepare-the-script-element
*
* @since {WP_VERSION}
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The @SInCE tag contains a placeholder '{WP_VERSION}' instead of an actual version number. This should be replaced with the specific WordPress version when this feature is released.

Suggested change
* @since {WP_VERSION}
* @since 6.5.0

Copilot uses AI. Check for mistakes.
* JavaScript can be safely escaped.
* Non-JavaScript script tags have unknown semantics.
*
* @todo consider applying to JSON and importmap script tags as well.
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

This TODO comment suggests unfinished work. Consider either implementing the suggested feature for JSON and importmap script tags or removing this comment if it's not planned for this release.

Suggested change
* @todo consider applying to JSON and importmap script tags as well.

Copilot uses AI. Check for mistakes.
'Tricky script open tag with \r' => array( '<script></script>', "<!-- <script\r>", "<script><!-- <\\u0073cript\r></script>" ),
'Tricky script open tag with \r\n' => array( '<script></script>', "<!-- <script\r\n>", "<script><!-- <\\u0073cript\r\n></script>" ),
'Tricky script close tag with \r' => array( '<script></script>', "// </script\r>", "<script>// </\\u0073cript\r></script>" ),
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\\u0073cript\r\n></script>" ),
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The expected output contains double backslashes '\u0073' which appears incorrect. It should be '\u0073' (single backslash) to represent the Unicode escape sequence properly.

Suggested change
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\\u0073cript\r\n></script>" ),
'Tricky script open tag with \r' => array( '<script></script>', "<!-- <script\r>", "<script><!-- <\u0073cript\r></script>" ),
'Tricky script open tag with \r\n' => array( '<script></script>', "<!-- <script\r\n>", "<script><!-- <\u0073cript\r\n></script>" ),
'Tricky script close tag with \r' => array( '<script></script>', "// </script\r>", "<script>// </\u0073cript\r></script>" ),
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\u0073cript\r\n></script>" ),

Copilot uses AI. Check for mistakes.
'Tricky script open tag with \r' => array( '<script></script>', "<!-- <script\r>", "<script><!-- <\\u0073cript\r></script>" ),
'Tricky script open tag with \r\n' => array( '<script></script>', "<!-- <script\r\n>", "<script><!-- <\\u0073cript\r\n></script>" ),
'Tricky script close tag with \r' => array( '<script></script>', "// </script\r>", "<script>// </\\u0073cript\r></script>" ),
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\\u0073cript\r\n></script>" ),
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The expected output contains double backslashes '\u0073' which appears incorrect. It should be '\u0073' (single backslash) to represent the Unicode escape sequence properly.

Suggested change
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\\u0073cript\r\n></script>" ),
'Tricky script open tag with \r' => array( '<script></script>', "<!-- <script\r>", "<script><!-- <\u0073cript\r></script>" ),
'Tricky script open tag with \r\n' => array( '<script></script>', "<!-- <script\r\n>", "<script><!-- <\u0073cript\r\n></script>" ),
'Tricky script close tag with \r' => array( '<script></script>', "// </script\r>", "<script>// </\u0073cript\r></script>" ),
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\u0073cript\r\n></script>" ),

Copilot uses AI. Check for mistakes.
'Tricky script open tag with \r' => array( '<script></script>', "<!-- <script\r>", "<script><!-- <\\u0073cript\r></script>" ),
'Tricky script open tag with \r\n' => array( '<script></script>', "<!-- <script\r\n>", "<script><!-- <\\u0073cript\r\n></script>" ),
'Tricky script close tag with \r' => array( '<script></script>', "// </script\r>", "<script>// </\\u0073cript\r></script>" ),
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\\u0073cript\r\n></script>" ),
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The expected output contains double backslashes '\u0073' which appears incorrect. It should be '\u0073' (single backslash) to represent the Unicode escape sequence properly.

Suggested change
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\\u0073cript\r\n></script>" ),
'Tricky script open tag with \r' => array( '<script></script>', "<!-- <script\r>", "<script><!-- <\u0073cript\r></script>" ),
'Tricky script open tag with \r\n' => array( '<script></script>', "<!-- <script\r\n>", "<script><!-- <\u0073cript\r\n></script>" ),
'Tricky script close tag with \r' => array( '<script></script>', "// </script\r>", "<script>// </\u0073cript\r></script>" ),
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\u0073cript\r\n></script>" ),

Copilot uses AI. Check for mistakes.
'Tricky script open tag with \r' => array( '<script></script>', "<!-- <script\r>", "<script><!-- <\\u0073cript\r></script>" ),
'Tricky script open tag with \r\n' => array( '<script></script>', "<!-- <script\r\n>", "<script><!-- <\\u0073cript\r\n></script>" ),
'Tricky script close tag with \r' => array( '<script></script>', "// </script\r>", "<script>// </\\u0073cript\r></script>" ),
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\\u0073cript\r\n></script>" ),
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The expected output contains double backslashes '\u0073' which appears incorrect. It should be '\u0073' (single backslash) to represent the Unicode escape sequence properly.

Suggested change
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\\u0073cript\r\n></script>" ),
'Tricky script open tag with \r' => array( '<script></script>', "<!-- <script\r>", "<script><!-- <\u0073cript\r></script>" ),
'Tricky script open tag with \r\n' => array( '<script></script>', "<!-- <script\r\n>", "<script><!-- <\u0073cript\r\n></script>" ),
'Tricky script close tag with \r' => array( '<script></script>', "// </script\r>", "<script>// </\u0073cript\r></script>" ),
'Tricky script close tag with \r\n' => array( '<script></script>', "// </script\r\n>", "<script>// </\u0073cript\r\n></script>" ),

Copilot uses AI. Check for mistakes.
*/
if ( $this->is_javascript_script_tag() ) {
$plaintext_content = preg_replace_callback(
'~<(/?)(s)(cript)([\t\r\n\f />])~i',
Copy link
Owner Author

Choose a reason for hiding this comment

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

We're only interested in 3 parts:

  • Leading
  • s or S (relevant)
  • trailing

Fewer groups and later concats.

@sirreal sirreal force-pushed the fix/62797-inline-script-tag-escape-alternate branch from d58831e to 410089d Compare December 12, 2025 18:36
This ensures that not only the return values match the expected results, but also that their type is the same.

Going forward, stricter type checking by using `assertSame()` should generally be preferred to `assertEquals()` where appropriate, to make the tests more reliable.

Follow-up to [61032], [61045].

See #64324.

git-svn-id: https://develop.svn.wordpress.org/trunk@61373 602fd350-edb4-49c9-b593-d223f7449a82
@sirreal sirreal force-pushed the fix/62797-inline-script-tag-escape-alternate branch from 8b87248 to e4679ca Compare December 12, 2025 22:34
@sirreal sirreal requested a review from Copilot December 12, 2025 22:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.



/**
* @ticket TBD
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

The ticket number is marked as "TBD" (To Be Determined). This should be replaced with the actual WordPress Trac ticket number before merging.

Suggested change
* @ticket TBD
* @ticket 62835

Copilot uses AI. Check for mistakes.
}

/**
* @ticket TBD
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

The ticket number is marked as "TBD" (To Be Determined). This should be replaced with the actual WordPress Trac ticket number before merging.

Copilot uses AI. Check for mistakes.
}

/**
* @ticket TBD
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

The ticket number is marked as "TBD" (To Be Determined). This should be replaced with the actual WordPress Trac ticket number before merging.

Suggested change
* @ticket TBD
* @ticket 12345

Copilot uses AI. Check for mistakes.
/**
* Indicates if the currently matched tag is a JSON script tag.
*
* @since {WP_VERSION}
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

The @SInCE tag contains a placeholder "{WP_VERSION}" that should be replaced with the actual WordPress version number (e.g., "6.8.0") before merging.

Suggested change
* @since {WP_VERSION}
* @since 6.8.0

Copilot uses AI. Check for mistakes.
SergeyBiryukov and others added 11 commits December 13, 2025 20:10
…nzip_file` filters.

This commit ensures that the original `$file` argument passed to the function is not unintentionally overwritten by the use of the same variable name in two `foreach` loops.

Follow-up to [56689].

Props sanchothefat, westonruter, mukesh27, SergeyBiryukov.
Fixes #64398.

git-svn-id: https://develop.svn.wordpress.org/trunk@61374 602fd350-edb4-49c9-b593-d223f7449a82
Fixes PHPCS warning: `Squiz.WhiteSpace.SuperfluousWhitespace.EndLine`.

Follow-up to [61374].

See #64398.


git-svn-id: https://develop.svn.wordpress.org/trunk@61375 602fd350-edb4-49c9-b593-d223f7449a82
… when invalid taxonomy supplied.

Developed in WordPress#10621

Props owolter, westonruter.
Fixes #64406.


git-svn-id: https://develop.svn.wordpress.org/trunk@61376 602fd350-edb4-49c9-b593-d223f7449a82
This ensures that the `url` parameter is not marked as invalid due to `wp_http_validate_url()` failing because of a `gethostbyname()` failure.

Follow-up to [51973].

Props westonruter, swissspidy.
See #54358.
Fixes #64412.


git-svn-id: https://develop.svn.wordpress.org/trunk@61377 602fd350-edb4-49c9-b593-d223f7449a82
…tick` may fire before `DOMContentLoaded`.

Developed in WordPress#10624

Follow-up to [23805], [50547].

Props westonruter, ArtZ91, siliconforks.
See #23295.
Fixes #64403.


git-svn-id: https://develop.svn.wordpress.org/trunk@61379 602fd350-edb4-49c9-b593-d223f7449a82
…ency.

Follow-up to [19687], [41746].

See #64224.

git-svn-id: https://develop.svn.wordpress.org/trunk@61380 602fd350-edb4-49c9-b593-d223f7449a82
This helps demonstrate the effectiveness.
@sirreal sirreal force-pushed the fix/62797-inline-script-tag-escape-alternate branch from 1953d44 to 8cf2527 Compare December 15, 2025 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants