Skip to content
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

Jump to bottom

Eliminate manual construction of script tags in WP_Scripts and pass other scripts through wp_print_inline_script_tag() #4773

Closed
wants to merge 28 commits into from
Closed

Eliminate manual construction of script tags in WP_Scripts and pass other scripts through wp_print_inline_script_tag() #4773

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
0d3ae2d
Use script tag helper functions instead of manual construction
westonruter Jul 1, 2023
86d74f0
WIP: Updating tests
westonruter Jul 1, 2023
61c877c
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter Aug 31, 2023
9ad9e6b
Fix Squiz.Strings.DoubleQuoteUsage.NotRequired
westonruter Aug 31, 2023
b52c335
Fix Tests_Dependencies_Scripts
westonruter Aug 31, 2023
8355a8c
Remove extra hyphen in id for translations script
westonruter Aug 31, 2023
b9ebf8b
Use wp_print_inline_script_tag() in the_block_template_skip_link()
westonruter Aug 31, 2023
d05efbe
Use wp_print_inline_script_tag() for various scripts in admin screens
westonruter Sep 1, 2023
3ac9a6e
Use wp_print_inline_script_tag() for admin scripts
westonruter Sep 1, 2023
adfef39
Fix inline script for list table
westonruter Sep 1, 2023
6ba21ef
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter Sep 1, 2023
c19f75d
Extend script tag printing functions to accept closure
westonruter Sep 1, 2023
50aafac
Use wp_print_inline_script_tag in more places
westonruter Sep 1, 2023
0e27a38
Utilize language injections to annotate JS strings
westonruter Sep 1, 2023
3b9b278
Remove incorrect static closures
westonruter Sep 1, 2023
96b1e7a
Update test_remove_frameless_preview_messenger_channel
westonruter Sep 6, 2023
96788c1
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter Sep 13, 2023
def2778
Revert wp-admin changes
westonruter Sep 13, 2023
8bcd7a2
Revert changes to wp-admin, closure on wp_print_script_inline_tag, an…
westonruter Sep 13, 2023
1194afe
Use wp_print_inline_script_tag() on login screen
westonruter Sep 13, 2023
205b8de
Enqueue script-link script instead of printing
westonruter Sep 19, 2023
fd9028d
Merge branch 'trunk' of https://github.com/WordPress/wordpress-develo…
westonruter Sep 20, 2023
2db59e5
Suggest missing ext-dom in composer.json
westonruter Sep 20, 2023
3ba5135
Use DOM to normalize scripts in document fragment
westonruter Sep 20, 2023
ecc29a9
Escape wrapped CDATA sections
westonruter Sep 20, 2023
832c315
Merge branch 'trunk' into trac-58664
westonruter Sep 25, 2023
3840c54
Remove language injection comments for now
westonruter Sep 25, 2023
97ad256
Revert Gutenberg upstream change
westonruter Sep 25, 2023
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
6 changes: 2 additions & 4 deletions src/wp-includes/blocks/categories.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ function render_block_core_categories( $attributes ) {
function build_dropdown_script_block_core_categories( $dropdown_id ) {
ob_start();
?>
<script type='text/javascript'>
/* <![CDATA[ */
<script>
( function() {
var dropdown = document.getElementById( '<?php echo esc_js( $dropdown_id ); ?>' );
function onCatChange() {
Expand All @@ -81,10 +80,9 @@ function onCatChange() {
}
dropdown.onchange = onCatChange;
})();
/* ]]> */
</script>
<?php
return ob_get_clean();
return wp_get_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
14 changes: 11 additions & 3 deletions src/wp-includes/class-wp-customize-manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,8 @@ protected function wp_die( $ajax_message, $message = null ) {
),
'error' => $ajax_message,
);
$message .= ob_get_clean();
ob_start();
?>
<script>
( function( api, settings ) {
Expand All @@ -472,7 +474,7 @@ protected function wp_die( $ajax_message, $message = null ) {
} )( wp.customize, <?php echo wp_json_encode( $settings ); ?> );
</script>
<?php
$message .= ob_get_clean();
$message .= wp_get_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );
}

wp_die( $message );
Expand Down Expand Up @@ -2083,6 +2085,7 @@ public function remove_frameless_preview_messenger_channel() {
if ( ! $this->messenger_channel ) {
return;
}
ob_start();
?>
<script>
( function() {
Expand All @@ -2106,6 +2109,7 @@ public function remove_frameless_preview_messenger_channel() {
} )();
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is out of scope for this PR, but looks like we could prevent some other corruption here by using URLSearchParams instead of string-searching the query of the URL. no need for DOM either.

url = new URL( location.href );
queryParams = url.searchParams;

if ( queryParams.has( 'customize_messenger_channel' ) ) {
	queryParams.delete( 'customize_messenger_channel' );
	url.search = queryParams;
	location.replace( url );
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, this code dates back to a time before we could rely on URL or URLSearchParams. I made a similar change recently to wp-embed (r56383). Probably should be put into a new defect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed this in Core-59480

</script>
<?php
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );
Copy link
Member

Choose a reason for hiding this comment

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

oops, we don't want to arbitrarily replace these tags. we know they exist at the front and back of the string, so by manually removing them we can avoid unintentional HTML syntax poisoning.

$script_html = ob_get_clean();
$script_html = substr( $script_html, strlen( '<script>' ), -strlen( '</script>' ) );

the strlen calls shouldn't add any overhead because that's stored in the string object, which here is a string literal, which we've already created in this patch inside the array.

this change is both safer, deterministic, and more efficient, particularly in the worse-case, though given that we expect very short args here I would be surprised if it's a measurable impact. still, doing less is faster than doing more.

Copy link
Member

Choose a reason for hiding this comment

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

Great catch @dmsnell!

@westonruter IMO this is worth a quick follow up commit applying this throughout.

If we feel strongly about introducing a helper at this point, I wouldn't be opposed to that either. Though I think that should remain an @access private function if we add it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Private helper function added in #5301

}

/**
Expand Down Expand Up @@ -2201,8 +2205,9 @@ public function customize_preview_settings() {
}
}

ob_start();
?>
<script type="text/javascript">
<script>
var _wpCustomizeSettings = <?php echo wp_json_encode( $settings ); ?>;
_wpCustomizeSettings.values = {};
(function( v ) {
Expand All @@ -2225,6 +2230,7 @@ public function customize_preview_settings() {
})( _wpCustomizeSettings.values );
</script>
<?php
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );
}

/**
Expand Down Expand Up @@ -4976,8 +4982,9 @@ public function customize_pane_settings() {
}
}

ob_start();
?>
<script type="text/javascript">
<script>
var _wpCustomizeSettings = <?php echo wp_json_encode( $settings ); ?>;
_wpCustomizeSettings.initialClientTimestamp = _.now();
_wpCustomizeSettings.controls = {};
Expand Down Expand Up @@ -5012,6 +5019,7 @@ public function customize_pane_settings() {
?>
</script>
<?php
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );
Copy link
Member

Choose a reason for hiding this comment

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

would it help to create a helper function for this specific task?

/**
 * Removes leading and trailing _empty_ script tags.
 *
 * This is a helper meant to be used for literal script tag construction
 * within wp_print_inline_script_tag(). It removes the literal values of
 * "<script>" and "</script>" from around an inline script.
 *
 * @since 6.4.0
 *
 * @param string $contents Script body with manually created SCRIPT tag literals.
 * @return string Script body without surrounding script tag literals, or
 *                original contents if both exact literals aren't present.
 */
function wp_remove_surrounding_empty_script_tags( $contents ) {
	$opener = '<script>';
	$closer = '</script>';

	$has_both_empty_tags = (
		strlen( $opener ) + strlen( $closer ) > strlen( $contents ) ||
		substr( $contents, 0, strlen( $opener ) ) !== $opener ) ||
		substr( $contents, -strlen( $closer ) ) !== $closer
	);

	return $has_both_empty_tags
		? substr( $contents, strlen( $opener ), -strlen( $closer ) )
		: $contents;
}

then these repetitive calls could be marginally nicer. even with the checks for the leading and trailing SCRIPT tags, this should still be faster than str_replace(), but more importantly, more secure and resilient against breaking syntax.

$script_body = wp_remove_surrounding_empty_script_tags( ob_get_clean() );
wp_print_inline_script_tag( $script_body );

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a syntax error in the $has_both_empty_tags condition. Could you fix and add comments explaining the conditions?

Copy link
Member

Choose a reason for hiding this comment

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

sure; also just a note - all my examples are almost always wrong and are only meant to convey ideas. if you ever get the impulse, please never copy code of mine from a comment and paste it into a project. don't trust me 😄 I don't test these examples or vet them for WPCS' prefences.

/**
 * Removes leading and trailing _empty_ script tags.
 *
 * This is a helper meant to be used for literal script tag construction
 * within wp_print_inline_script_tag(). It removes the literal values of
 * "<script>" and "</script>" from around an inline script.
 *
 * @since 6.4.0
 *
 * @param string $contents Script body with manually created SCRIPT tag literals.
 * @return string Script body without surrounding script tag literals, or
 *                original contents if both exact literals aren't present.
 */
function wp_remove_surrounding_empty_script_tags( $contents ) {
	$opener = '<script>';
	$closer = '</script>';

	$has_both_empty_tags = (
		strlen( $opener ) + strlen( $closer ) > strlen( $contents ) ||
		substr( $contents, 0, strlen( $opener ) ) !== $opener ||
		substr( $contents, -strlen( $closer ) ) !== $closer
	);

	/*
	 * What should happen if the given contents are not surrounded by
	 * the exact literal script tags? This question opens up a can of
	 * worms with no obvious or clear answer. Removing one of the tags
	 * could lead to just as much trouble as leaving one in place.
	 * This code leaves the string untouched if it can't find both the
	 * exact tags. Maybe someone added an attribute to the opening tag
	 * or maybe someone added a space; it's impossible to know from
	 * here.
	 *
	 * It would be possible to return `null` or `false` here to indicate
	 * the failure, but people probably wouldn't be checking the result
	 * and that could introduce corruption. An empty string could be
	 * another viable return and that would quickly signal that something
	 * is wrong and needs fixing.
	 */
	return $has_both_empty_tags
		? substr( $contents, strlen( $opener ), -strlen( $closer ) )
		: $contents;
}

Maybe it's best to return an empty string here if the syntax isn't a perfect match. Is this what you were asking for in "add comments explaining the conditions"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I meant more the conditions for $has_both_empty_tags as I wasn't understanding exactly they were doing. But it seems that's because the logic wasn't quite right. Also seems worthwhile to normalize the prefix and suffix to upper case. So I believe the condition should actually be:

	$has_both_empty_tags = (
		strlen( $contents ) > strlen( $opener ) + strlen( $closer ) &&
		strtolower( substr( $contents, 0, strlen( $opener ) ) ) === $opener &&
		strtolower( substr( $contents, -strlen( $closer ) ) ) === $closer
	);

It has both empty tags if:

  1. The entire string is longer than the opener and closer, and
  2. The string starts with $opener, and
  3. The string ends with $closer

When those conditions are satisfied, then substr( $contents, strlen( $opener ), -strlen( $closer ) ) should be done.

In testing, I see that the first line of the function should also be:

$contents = trim( $contents );

In regards to the comment comment before:

	return $has_both_empty_tags
		? substr( $contents, strlen( $opener ), -strlen( $closer ) )
		: $contents;

What do you think about actually just doing:

if ( ! $has_both_empty_tags ) {
	_doing_it_wrong( __FUNCTION__, '6.4', __( 'Expected string to begin with an empty script tag and close with a script tag.' ) );
	return '';
}

I'll note that this is actually the reverse of what wp_add_inline_script() does:

wordpress-develop/src/wp-includes/functions.wp-scripts.php

Lines 133 to 145 in 6fa2ce4

if ( false !== stripos( $data, '</script>' ) ) {
_doing_it_wrong(
__FUNCTION__,
sprintf(
/* translators: 1: <script>, 2: wp_add_inline_script() */
__( 'Do not pass %1$s tags to %2$s.' ),
'<code>&lt;script&gt;</code>',
'<code>wp_add_inline_script()</code>'
),
'4.5.0'
);
$data = trim( preg_replace( '#<script[^>]*>(.*)</script>#is', '$1', $data ) );
}

Note also how it does the string replacement, so somewhat the naive approach which I originally committed.

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 implemented this in #5301

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about actually just doing:

Seems great. There's never a case to call this without the SCRIPT tags, so that makes sense to me. Make it visible as early as possible.

seems worthwhile to normalize the prefix and suffix to upper case

I have no strong opinion on this. Seems fine. The one thing is that <sCRiPt> is not exactly a string literal match, but that's fine. Whatever happens here I think it will work out, and if something goes awry, with the help of the _doing_it_wrong() someone will figure it out quickly enough.

}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/wp-includes/class-wp-customize-nav-menus.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1559,7 +1559,7 @@ public function export_preview_data() {
$exports = array(
'navMenuInstanceArgs' => $this->preview_nav_menu_instance_args,
);
printf( '<script>var _wpCustomizePreviewNavMenusExports = %s;</script>', wp_json_encode( $exports ) );
wp_print_inline_script_tag( sprintf( /** @lang JavaScript */ 'var _wpCustomizePreviewNavMenusExports = %s;', wp_json_encode( $exports ) ) );
Copy link
Member Author

Choose a reason for hiding this comment

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

PhpStorm flags the sprintf placeholder as a syntax error:

Expression expected

image

Copy link
Member

Choose a reason for hiding this comment

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

Is that why you added the /** @lang JavaScript */ here? Is that standardized somehow? Asking since I haven't seen that before.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are language injection comments. Supported by PhpStorm, at least: https://www.jetbrains.com/help/phpstorm/using-language-injections.html

Copy link
Member

Choose a reason for hiding this comment

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

Unless this is a standard, I think we should remove those annotations. We shouldn't change code to support arbitrary IDEs, unless this is something multiple independent tools use.

Copy link
Member Author

@westonruter westonruter Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But I don't think it hurts, and I believe at least a majority of WP developers have used PhpStorm. Such annotations are also in #4824 by @spacedmonkey.

Copy link
Member

Choose a reason for hiding this comment

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

I get the reason for it, but I'm not sold on the justification to support an arbitrary IDE here. What if other IDEs also have a proprietary means for that that looks different? Would we add 2+ comments to those lines to satisfy all the IDEs? I don't think it's right for WP to "pick winners", which I think it does give preference to one by adding annotations that only that specific IDE benefits from.

It would be different if this was a standard pattern, but if it's not I'd prefer to remove it. I didn't spot it in #4824 before, but it applies there as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but I don't see this as being supporting an arbitrary IDE. If it's the most popular IDE for WP core development, it seems worthwhile.

Copy link
Member

Choose a reason for hiding this comment

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

Even if it's the most popular one, it's still arbitrary if there's no standard.

If it's the most popular IDE for WP core development, it seems worthwhile.

Maybe that is true, but we don't know that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe that is true, but we don't know that.

Bard says it is 😄 https://g.co/bard/share/e9560a1c3ce5

Anyway, I'd like to get a second opinion.

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 decided to move this into a new ticket: Core-59444

}

/**
Expand Down
10 changes: 4 additions & 6 deletions src/wp-includes/class-wp-customize-widgets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1310,12 +1310,10 @@ public function export_preview_data() {
foreach ( $settings['registeredWidgets'] as &$registered_widget ) {
unset( $registered_widget['callback'] ); // May not be JSON-serializeable.
}

?>
<script type="text/javascript">
var _wpWidgetCustomizerPreviewSettings = <?php echo wp_json_encode( $settings ); ?>;
</script>
<?php
wp_print_inline_script_tag(
// language=JavaScript
sprintf( 'var _wpWidgetCustomizerPreviewSettings = %s;', wp_json_encode( $settings ) )
Copy link
Member Author

Choose a reason for hiding this comment

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

PhpStorm flags the sprintf placeholder as a syntax error:

Expression expected

image

Copy link
Member

Choose a reason for hiding this comment

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

presumably because of the // language=JavaScript comment, which shouldn't be there since this is still PHP?

);
}

/**
Expand Down
59 changes: 15 additions & 44 deletions src/wp-includes/class-wp-scripts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,17 +122,6 @@ class WP_Scripts extends WP_Dependencies {
*/
public $default_dirs;

/**
* Holds a string which contains the type attribute for script tag.
*
* If the active theme does not declare HTML5 support for 'script',
* then it initializes as `type='text/javascript'`.
*
* @since 5.3.0
* @var string
*/
private $type_attr = '';

/**
* Holds a mapping of dependents (as handles) for a given script handle.
* Used to optimize recursive dependency tree checks.
Expand Down Expand Up @@ -167,14 +156,6 @@ public function __construct() {
* @since 3.4.0
*/
public function init() {
if (
function_exists( 'is_admin' ) && ! is_admin()
&&
function_exists( 'current_theme_supports' ) && ! current_theme_supports( 'html5', 'script' )
) {
$this->type_attr = " type='text/javascript'";
}

/**
* Fires when the WP_Scripts instance is initialized.
*
Expand Down Expand Up @@ -245,20 +226,7 @@ public function print_extra_script( $handle, $display = true ) {
return $output;
}

printf( "<script%s id='%s-js-extra'>\n", $this->type_attr, esc_attr( $handle ) );

// CDATA is not needed for HTML 5.
if ( $this->type_attr ) {
echo "/* <![CDATA[ */\n";
}

echo "$output\n";

if ( $this->type_attr ) {
echo "/* ]]> */\n";
}

echo "</script>\n";
wp_print_inline_script_tag( $output, array( 'id' => "{$handle}-js-extra" ) );

return true;
}
Expand Down Expand Up @@ -335,7 +303,7 @@ public function do_item( $handle, $group = false ) {

$translations = $this->print_translations( $handle, false );
if ( $translations ) {
$translations = sprintf( "<script%s id='%s-js-translations'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), $translations );
$translations = wp_get_inline_script_tag( $translations, array( 'id' => "{$handle}-js-translations" ) );
}

if ( $this->do_concat ) {
Expand Down Expand Up @@ -403,21 +371,24 @@ public function do_item( $handle, $group = false ) {
}

/** This filter is documented in wp-includes/class-wp-scripts.php */
$src = esc_url( apply_filters( 'script_loader_src', $src, $handle ) );
$src = esc_url_raw( apply_filters( 'script_loader_src', $src, $handle ) );

if ( ! $src ) {
return true;
}

$tag = $translations . $cond_before . $before_script;
$tag .= sprintf(
"<script%s src='%s' id='%s-js'%s%s></script>\n",
$this->type_attr,
$src, // Value is escaped above.
esc_attr( $handle ),
$strategy ? " {$strategy}" : '',
$intended_strategy ? " data-wp-strategy='{$intended_strategy}'" : ''
$attr = array(
'src' => $src,
'id' => "{$handle}-js",
);
if ( $strategy ) {
$attr[ $strategy ] = true;
}
if ( $intended_strategy ) {
$attr['data-wp-strategy'] = $intended_strategy;
}
$tag = $translations . $cond_before . $before_script;
$tag .= wp_get_script_tag( $attr );
$tag .= $after_script . $cond_after;

/**
Expand Down Expand Up @@ -720,7 +691,7 @@ public function print_translations( $handle, $display = true ) {
JS;

if ( $display ) {
printf( "<script%s id='%s-js-translations'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), $output );
wp_print_inline_script_tag( $output, array( 'id' => "{$handle}-js-translations" ) );
}

return $output;
Expand Down
2 changes: 1 addition & 1 deletion src/wp-includes/comment-template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,7 @@ function wp_comment_form_unfiltered_html_nonce() {

if ( current_user_can( 'unfiltered_html' ) ) {
wp_nonce_field( 'unfiltered-html-comment_' . $post_id, '_wp_unfiltered_html_comment_disabled', false );
echo "<script>(function(){if(window===window.parent){document.getElementById('_wp_unfiltered_html_comment_disabled').name='_wp_unfiltered_html_comment';}})();</script>\n";
wp_print_inline_script_tag( /** @lang JavaScript */ "(function(){if(window===window.parent){document.getElementById('_wp_unfiltered_html_comment_disabled').name='_wp_unfiltered_html_comment';}})();" );
Copy link
Member

Choose a reason for hiding this comment

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

Same question here.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are language injection comments. Supported by PhpStorm, at least: https://www.jetbrains.com/help/phpstorm/using-language-injections.html

}
}

Expand Down
2 changes: 1 addition & 1 deletion src/wp-includes/customize/class-wp-customize-selective-refresh.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public function export_preview_data() {
);

// Export data to JS.
printf( '<script>var _customizePartialRefreshExports = %s;</script>', wp_json_encode( $exports ) );
wp_print_inline_script_tag( sprintf( /** @lang JavaScript */ 'var _customizePartialRefreshExports = %s;', wp_json_encode( $exports ) ) );
Copy link
Member Author

Choose a reason for hiding this comment

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

PhpStorm flags the placeholder as a syntax error:

Expression expected

image

}

/**
Expand Down
2 changes: 2 additions & 0 deletions src/wp-includes/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7655,6 +7655,7 @@ function wp_post_preview_js() {
// Has to match the window name used in post_submit_meta_box().
$name = 'wp-preview-' . (int) $post->ID;

ob_start();
?>
<script>
( function() {
Expand All @@ -7670,6 +7671,7 @@ function wp_post_preview_js() {
}());
</script>
<?php
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );
}

/**
Expand Down
25 changes: 20 additions & 5 deletions src/wp-includes/script-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2787,7 +2787,11 @@ function wp_sanitize_script_attributes( $attributes ) {
*/
function wp_get_script_tag( $attributes ) {
if ( ! isset( $attributes['type'] ) && ! is_admin() && ! current_theme_supports( 'html5', 'script' ) ) {
$attributes['type'] = 'text/javascript';
// Keep the type attribute as the first for legacy reasons (it has always been this way in core).
$attributes = array_merge(
array( 'type' => 'text/javascript' ),
$attributes
);
Comment on lines -2790 to +2794
Copy link
Member

Choose a reason for hiding this comment

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

👍

}
/**
* Filters attributes to be added to a script tag.
Expand Down Expand Up @@ -2830,9 +2834,22 @@ function wp_print_script_tag( $attributes ) {
* @return string String containing inline JavaScript code wrapped around `<script>` tag.
*/
function wp_get_inline_script_tag( $javascript, $attributes = array() ) {
if ( ! isset( $attributes['type'] ) && ! is_admin() && ! current_theme_supports( 'html5', 'script' ) ) {
$attributes['type'] = 'text/javascript';
$is_html5 = current_theme_supports( 'html5', 'script' ) || is_admin();
if ( ! isset( $attributes['type'] ) && ! $is_html5 ) {
// Keep the type attribute as the first for legacy reasons (it has always been this way in core).
$attributes = array_merge(
array( 'type' => 'text/javascript' ),
$attributes
);
}

// Ensure markup is XHTML compatible if not HTML5.
if ( ! $is_html5 ) {
$javascript = sprintf( "/* <![CDATA[ */\n%s\n/* ]]> */", $javascript );
Copy link
Member Author

@westonruter westonruter Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To ensure XML-compatibility, the $javascript string should have any instances of ]]> escaped. It's ugly, but apparently this is what has to be done:

Suggested change
$javascript = sprintf( "/* <![CDATA[ */\n%s\n/* ]]> */", $javascript );
$javascript = str_replace( ']]>', ']]]]><![CDATA[>', $javascript );
$javascript = sprintf( "/* <![CDATA[ */\n%s\n/* ]]> */", $javascript );

Nevertheless, it is likely exceedingly rare that a WordPress site is actually being served with Content-Type: application/xhtml+xml (true confessions, I used to do it, but the draconian XML parse error handling was painful). Still, it is unlikely for ]]> to occur in a script. I say this because if a site is being served as Content-Type: text/html then the parser HTML parser will ignore these CDATA sections, and it could end up causing a parse error when the script is passed to the JS interpreter.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any concern from adding this now in case the $javascript passed to this function is already wrapped with that CDATA markup? Very unlikely I assume, but we may want to add a check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good call. I've done this in ecc29a9. This shows that it is indeed necessary to do the escaping. Without the escaping, doing the following:

wp_print_inline_script_tag( "/* <![CDATA[ */ console.log( 'Hello World!' ); /* ]]> */" );

Results in the following output:

<script type="text/javascript">
/* <![CDATA[ */
/* <![CDATA[ */ console.log( 'Hello World!' ); /* ]]> */
/* ]]> */
</script>

Pasting this into the W3C Validator as a fragment under the XHTML Strict doctype:

image

Results in an XML parse error:

image

However, when the escaping is present, the PHP outputs:

<script type="text/javascript">
/* <![CDATA[ */
/* <![CDATA[ */ console.log( 'Hello World!' ); /* ]]]]><![CDATA[> */
/* ]]> */
</script>

And this is valid:

image

Copy link
Member

Choose a reason for hiding this comment

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

something as bizarre as this could definitely use a link to an XML spec or the source of the reason why these have to be escaped. someone is going to look at that and "fix" it and "improve quality" by removing the escaping 🙃

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though there is a comment about the escaping?

Copy link
Member

@dmsnell dmsnell Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yeah because it mentions that it should be compatible but not how or why.
like, how does this fix ensure compatibility?

when I saw it I immediately wondered what rule necessitates this or what breaks without it. I could imagine something like this, to see if I'm properly understanding the code.

/*
 * XHTML extracts the contents of the SCRIPT element and then the XML parser
 * decodes character references and other syntax elements. This can lead to
 * misinterpretation of the script contents or invalid XHTML documents.
 *
 * Wrapping the contents in a CDATA section instructs the XML parser not to
 * transform the contents of the SCRIPT element before passing them to the
 * JavaScript engine.
 *
 * Example
 *
 *     <script>console.log('&hellip;');</script>
 * 
 *     In an HTML document this would print "&hellip;" to the console,
 *     but in an XHTML document it would print "…" to the console.
 * 
 *
 *     <script>console.log('An image is <img> in HTML');</script>
 * 
 *     In an HTML document this would print "An image is <img> in HTML",
 *     but it's an invalid XHTML document because it interprets the `<img>`
 *     as an empty tag missing its closing `/`.
 *
 * @see https://www.w3.org/TR/xhtml1/#h-4.8
 */
if ( ! $is_html5 ) {
	/*
	 * If the string `]]>` exists within the JavaScript it would break
	 * out of any wrapping CDATA section added here, so to start, it's
	 * necessary to escape that sequence which requires splitting the
	 * content into two CDATA sections wherever it's found.
	 *
	 * Note: it's only necessary to escape the closing `]]>` because
	 * an additional `<![CDATA[` leaves the contents unchanged.
	 */
	$javascript = str_replace( ']]>', ']]]]><![CDATA[>', $javascript );

	// Wrap the entire escaped script inside a CDATA section.
	$javascript = sprintf( "/* <![CDATA[ */\n%s\n/* ]]> */", $javascript );
}

yeah I know it's wordy and lengthy, but it was really confusing to me, and this code is now going to be responsible for general code generation, and may get a lot of eyes on it.

by the way it seems like this will not trigger if the HTTP Content-type: text/html is what serves the document. I could not uncover these failures without serving as Content-type: application/xhtml+xml or by directly opening the file with a .xml extension. The exact same file contents stored with .html as its extension leads to HTML semantics, thus this doesn't effect it.

Finally, and thankfully, it doesn't appear to be a security issue because Safari, Firefox, and Chrome all prevent escaping from the script using tricks like &lt;/script&gt;. It will change those things into </script>, but that leads to a JavaScript syntax error, or data corruption if it's found within a JS string.

Including <img> was fun because that broke the entire page render. It became invalid XML. Again, if served as .html or with Content-type: text/html or anything but explicit out-of-band information that it's XML, none of these problem arise.

}

$javascript = "\n" . trim( $javascript, "\n\r " ) . "\n";

/**
* Filters attributes to be added to a script tag.
*
Expand All @@ -2845,8 +2862,6 @@ function wp_get_inline_script_tag( $javascript, $attributes = array() ) {
*/
$attributes = apply_filters( 'wp_inline_script_attributes', $attributes, $javascript );

$javascript = "\n" . trim( $javascript, "\n\r " ) . "\n";
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This needs to move up above the applying of the wp_inline_script_attributes filters so that the final $javascript is available for computing a CSP hash.


return sprintf( "<script%s>%s</script>\n", wp_sanitize_script_attributes( $attributes ), $javascript );
}

Expand Down
8 changes: 7 additions & 1 deletion src/wp-includes/theme-templates.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,9 @@ function the_block_template_skip_link() {
</style>
<?php
/**
* Print the skip-link script.
* Enqueue the skip-link script.
*/
ob_start();
?>
<script>
( function() {
Expand Down Expand Up @@ -202,6 +203,11 @@ function the_block_template_skip_link() {
}() );
</script>
<?php
$skip_link_script = str_replace( array( '<script>', '</script>' ), '', ob_get_clean() );
$script_handle = 'wp-block-template-skip-link';
wp_register_script( $script_handle, false );
wp_add_inline_script( $script_handle, $skip_link_script );
wp_enqueue_script( $script_handle );
}

/**
Expand Down
5 changes: 3 additions & 2 deletions src/wp-includes/theme.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3742,9 +3742,9 @@ function wp_customize_support_script() {
$admin_origin = parse_url( admin_url() );
$home_origin = parse_url( home_url() );
$cross_domain = ( strtolower( $admin_origin['host'] ) !== strtolower( $home_origin['host'] ) );
$type_attr = current_theme_supports( 'html5', 'script' ) ? '' : ' type="text/javascript"';
ob_start();
?>
<script<?php echo $type_attr; ?>>
<script>
(function() {
var request, b = document.body, c = 'className', cs = 'customize-support', rcs = new RegExp('(^|\\s+)(no-)?'+cs+'(\\s+|$)');

Expand All @@ -3760,6 +3760,7 @@ function wp_customize_support_script() {
}());
</script>
<?php
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );
}

/**
Expand Down
8 changes: 3 additions & 5 deletions src/wp-includes/widgets/class-wp-widget-archives.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,15 @@ public function widget( $args, $instance ) {
$label = __( 'Select Post' );
break;
}

$type_attr = current_theme_supports( 'html5', 'script' ) ? '' : ' type="text/javascript"';
?>

<option value=""><?php echo esc_html( $label ); ?></option>
<?php wp_get_archives( $dropdown_args ); ?>

</select>

<script<?php echo $type_attr; ?>>
/* <![CDATA[ */
<?php ob_start(); ?>
<script>
(function() {
var dropdown = document.getElementById( "<?php echo esc_js( $dropdown_id ); ?>" );
function onSelectChange() {
Expand All @@ -120,9 +118,9 @@ function onSelectChange() {
}
dropdown.onchange = onSelectChange;
})();
/* ]]> */
</script>
<?php
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );
} else {
$format = current_theme_supports( 'html5', 'navigation-widgets' ) ? 'html5' : 'xhtml';

Expand Down
Loading
Loading