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

westonruter · 2023-07-01T02:24:08Z

The scope here is now limited to the frontend (including Customizer preview) as well as the login screen (wp-login.php). Admin screens are not included since this would increase the scope significantly. Additionally, the core themes have not been updated since wp_print_inline_script_tag() was introduced in WP 5.7, and the last theme to include any custom scripts (Twenty Twenty-One) supports WordPress 5.3 and above. So to take advantage of the example Strict CSP plugin with core themes, you have to use one of these themes which don't manually construct scripts:

Twenty Eleven (only uses manual script tag in IE conditional comment)
Twenty Twelve (only uses manual script tag in IE conditional comment)
Twenty Thirteen
Twenty Fourteen (only uses manual script tag in IE conditional comment)
Twenty Sixteen
Twenty Nineteen
Twenty Twenty-Two (a block theme, so no custom JS)
Twenty Twenty-Three (a block theme, so no custom JS)

Previously: 10up#58

See WordPress/gutenberg#54637 for syncing block change to Gutenberg.

Testing Instructions

Install and activate the Strict CSP plugin
Activate Twenty Twenty-Four
Navigate the frontend, the wp-login screen, and the Customizer preview (ideally so all scripts touched end up being printed) with the console open, making sure that there are no CSP warnings like:

Refused to load the script 'http://localhost:8889/wp-includes/blocks/navigation/view.min.js?ver=5687b52bb8c84fb4ae68' because it violates the following Content Security Policy directive: "script-src 'nonce-6aa1ba7b65' 'unsafe-inline' 'strict-dynamic' https: http:". Note that 'strict-dynamic' is present, so host-based allowlisting is disabled. Note that 'script-src-elem' was not explicitly set, so 'script-src' is used as a fallback.

Trac ticket: https://core.trac.wordpress.org/ticket/58664

Commit Message

Script Loader: Use wp_get_script_tag() and wp_get_inline_script_tag()/wp_print_inline_script_tag() helper functions to output scripts on frontend and login screen.

Using script tag helper functions allows plugins to employ the wp_script_attributes and wp_inline_script_attributes filters to inject the nonce attribute to apply Content Security Policy (e.g. Strict CSP). Use of helper functions also simplifies logic in WP_Scripts.

Update wp_get_inline_script_tag() to wrap inline script in CDATA blocks for XHTML-compatibility.
Ensure the type attribute is printed first in wp_get_inline_script_tag() for back-compat.
Wrap existing <script> tags in output buffering to retain IDE supports.
In wp_get_inline_script_tag(), append the newline to $javascript before it is passed into the wp_inline_script_attributes filter so that the CSP hash can be computed properly.
In the_block_template_skip_link(), opt to enqueue the inline script rather than print it.
Add ext-php to composer.json under suggest as previously it was an undeclared dependency for running PHPUnit tests.
Update tests to rely on DOMDocument to compare script markup, normalizing unsemantic differences.

Props westonruter, spacedmonkey, flixos90, 10upsimon, dmsnell, mukesh27, joemcgill, swissspidy, azaozz.
Fixes #58664.
See #39941.

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.

…into trac-58664

westonruter · 2023-08-31T20:30:50Z

src/wp-includes/script-loader.php

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


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.

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?

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:

Results in an XML parse error:

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:

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 🙃

Even though there is a comment about the escaping?

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('…');</script> * * In an HTML document this would print "…" 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 </script>. 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.

westonruter · 2023-08-31T22:08:43Z

src/wp-includes/script-loader.php

@@ -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";


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.

mukeshpanchal27

Thanks @westonruter for the PR.

Do we need to update wp_print_community_events_templates() ?

src/wp-admin/includes/class-wp-list-table.php

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

westonruter · 2023-09-01T17:14:17Z

Do we need to update wp_print_community_events_templates() ?

No. Since these are not script tags containing JavaScript, they do not have to be updated to use wp_print_inline_script_tag() in order to apply CSP. The nonce attribute is only needed for JS script tags.

westonruter · 2023-09-01T17:22:21Z

src/wp-admin/includes/media.php

+	ob_start();
 	?>
-	<script type="text/javascript">
+	<script>
 	addLoadEvent = function(func){if(typeof jQuery!=='undefined')jQuery(function(){func();});else if(typeof wpOnload!=='function'){wpOnload=func;}else{var oldonload=wpOnload;wpOnload=function(){oldonload();func();}}};
 	var ajaxurl = '<?php echo esc_js( admin_url( 'admin-ajax.php', 'relative' ) ); ?>', pagenow = 'media-upload-popup', adminpage = 'media-upload-popup',
 	isRtl = <?php echo (int) is_rtl(); ?>;
 	</script>
 	<?php
+	wp_print_inline_script_tag( trim( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) ) );


I'm not really happy with the boilerplate ob_start() followed by an inline script followed by:

wp_print_inline_script_tag( trim( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) ) );

I think ideally there would be a helper to do this automatically. Consider, for example, if wp_print_inline_script_tag() actually allowed a closure to be provided for the $javascript parameter in addition to a string. It could handle the output buffering automatically, for instance:

wp_print_inline_script_tag( static function () { ?> <script> addLoadEvent = function(func){if(typeof jQuery!=='undefined')jQuery(function(){func();});else if(typeof wpOnload!=='function'){wpOnload=func;}else{var oldonload=wpOnload;wpOnload=function(){oldonload();func();}}}; var ajaxurl = '<?php echo esc_js( admin_url( 'admin-ajax.php', 'relative' ) ); ?>', pagenow = 'media-upload-popup', adminpage = 'media-upload-popup', isRtl = <?php echo (int) is_rtl(); ?>; </script> <?php } );

When a closure is passed, it could automatically start and end output buffering, strip the script start and end tags, and trim whitespace. If the output buffer lacks a <script> it could issue a _doing_it_wrong().

The only purpose for having the script tags in the PHP code is to enable IDEs to do syntax-highlighting, syntax checking, autocompletion, etc. This is valuable so I think we should facilitate it somehow.

This modification to wp_get_inline_script_tag() has since been reverted in this PR.

…into trac-58664

westonruter · 2023-09-01T21:41:38Z

Update: The scope has been reduced to not include wp-admin.

There are still quite a few places where scripts are being manually printed, primarily in the admin:

ack output

wp-includes/class-wp-customize-widgets.php
1315:           <script type="text/javascript">

wp-includes/class-wp-embed.php
91:<script type="text/javascript">

wp-includes/theme-previews.php
72:     <script type="text/javascript">

wp-admin/includes/class-bulk-upgrader-skin.php
112:            echo '<script type="text/javascript">jQuery(\'.waiting-' . esc_js( $this->upgrader->update_current ) . '\').hide();</script>';
133:            echo '<script type="text/javascript">jQuery(\'.waiting-' . esc_js( $this->upgrader->update_current ) . '\').css("display", "inline-block");</script>';
151:                    echo '<script type="text/javascript">jQuery(\'#progress-' . esc_js( $this->upgrader->update_current ) . '\').show();</script>';
161:                    echo '<script type="text/javascript">jQuery(\'.waiting-' . esc_js( $this->upgrader->update_current ) . '\').hide();</script>';

wp-admin/includes/class-custom-image-header.php
377:<script type="text/javascript">
432:<script type="text/javascript">

wp-admin/includes/class-wp-internal-pointers.php
121:            <script type="text/javascript">

wp-admin/includes/class-wp-upgrader-skin.php
241:                    echo '<script type="text/javascript">
247:                    echo '<script type="text/javascript">

wp-admin/includes/meta-boxes.php
918:                    <script type="text/javascript">jQuery(function(){commentsBox.get(<?php echo $total; ?>, 10);});</script>

wp-admin/includes/update-core.php
1727:<script type="text/javascript">

wp-admin/includes/deprecated.php
1514:   <script type="text/javascript">

wp-admin/includes/ms.php
839:<script type="text/javascript">
1000:<script type="text/javascript">

wp-admin/includes/media.php
275:    <script type="text/javascript">
825:            <script type="text/javascript">
2073:   echo '<script type="text/javascript">post_id = ' . $post_id . ';</script>';
2212:   <script type="text/javascript">
2354:   <script type="text/javascript">
2420:   <script type="text/javascript">
2561:   <script type="text/javascript">
2890:   <script type="text/javascript">

wp-admin/network/upgrade.php
126:            <script type="text/javascript">

wp-admin/network/site-users.php
219:<script type="text/javascript">

wp-admin/edit-form-advanced.php
739:<script type="text/javascript">

wp-admin/media-new.php
80:     <script type="text/javascript">

wp-admin/customize.php
154:<script type="text/javascript">

wp-admin/install.php
457:<script type="text/javascript">var t = document.getElementById('weblog_title'); if (t){ t.focus(); }</script>
463:<script type="text/javascript">

wp-admin/update-core.php
214:            <script type="text/javascript">
922:    <script type="text/javascript">

See https://www.jetbrains.com/help/phpstorm/using-language-injections.html

…into trac-58664

…d update more non-admin scripts

westonruter · 2023-09-14T00:09:46Z

src/wp-includes/class-wp-customize-widgets.php

-		<?php
+		wp_print_inline_script_tag(
+			// language=JavaScript
+			sprintf( 'var _wpWidgetCustomizerPreviewSettings = %s;', wp_json_encode( $settings ) )


PhpStorm flags the sprintf placeholder as a syntax error:

Expression expected

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

src/wp-includes/theme-templates.php

spacedmonkey · 2023-09-19T11:57:10Z

@westonruter There seem to be number of places where we should use this function.

wordpress-develop/src/wp-includes/theme-previews.php

Lines 69 to 76 in 1cd9877

    
           function wp_block_theme_activate_nonce() { 
        
           	$nonce_handle = 'switch-theme_' . wp_get_theme_preview_path(); 
        
           	?> 
        
           	<script type="text/javascript"> 
        
           		window.WP_BLOCK_THEME_ACTIVATE_NONCE = <?php echo wp_json_encode( wp_create_nonce( $nonce_handle ) ); ?>; 
        
           	</script> 
        
           	<?php 
        
           }

wordpress-develop/src/wp-includes/class-wp-embed.php

Lines 91 to 95 in 1cd9877

    
           <script type="text/javascript"> 
        
           	jQuery( function($) { 
        
           		$.get("<?php echo esc_url( admin_url( 'admin-ajax.php', 'relative' ) ) . '?action=oembed-cache&post=' . $post->ID; ?>"); 
        
           	} ); 
        
           </script>

wordpress-develop/src/wp-includes/class-wp-editor.php

Line 977 in 1cd9877

wordpress-develop/src/wp-includes/class-wp-editor.php

Line 1563 in 1cd9877

    
           echo "<script type='text/javascript'>\n" . self::wp_mce_translation() . "</script>\n";

wordpress-develop/src/wp-includes/class-wp-editor.php

Line 1620 in 1cd9877

There are also 50 other examples in /wp-admin

spacedmonkey

This is getting close. There are 5 places in wp-includes that should be updated. We should also consider using wp_add_inline_script in some places.

westonruter · 2023-09-19T15:28:10Z

@westonruter There seem to be number of places where we should use this function.

There are also 50 other examples in /wp-admin

@spacedmonkey I've intentionally excluded instances that are specifically used in the wp-admin. This is to reduce scope. The changes in this PR are intended to only relate to the frontend and to the login screen. Some instances in wp-includes are only used in wp-admin (generally) so that's why they aren't included. I'll double-check the ones you identified.

I think the wp-admin will need to be a separate effort. For one, there are many many more inline script tags, and secondly, the block/site editor screens have manual script construction in JS which breaks Strict CSP. So that will need to be addressed in Gutenberg.

westonruter · 2023-09-19T16:40:43Z

@westonruter There seem to be number of places where we should use this function.

@spacedmonkey:

wordpress-develop/src/wp-includes/theme-previews.php

Lines 69 to 76 in 1cd9877

function wp_block_theme_activate_nonce() {

$nonce_handle = 'switch-theme_' . wp_get_theme_preview_path();

?>

<script type="text/javascript">

window.WP_BLOCK_THEME_ACTIVATE_NONCE = <?php echo wp_json_encode( wp_create_nonce( $nonce_handle ) ); ?>;

</script>

<?php

}

This is used exclusively at the admin_head action, so it's out of scope.

wordpress-develop/src/wp-includes/class-wp-embed.php

Lines 91 to 95 in 1cd9877

<script type="text/javascript">

jQuery( function($) {

$.get("<?php echo esc_url( admin_url( 'admin-ajax.php', 'relative' ) ) . '?action=oembed-cache&post=' . $post->ID; ?>");

} );

</script>

This is used exclusively at the edit_form_advanced and edit_page_form actions on the classic post edit screen in the admin, so it's out of scope.

wordpress-develop/src/wp-includes/class-wp-editor.php

Line 977 in 1cd9877

<script type="text/javascript">

wordpress-develop/src/wp-includes/class-wp-editor.php

Line 1563 in 1cd9877

echo "<script type='text/javascript'>\n" . self::wp_mce_translation() . "</script>\n";

wordpress-develop/src/wp-includes/class-wp-editor.php

Line 1620 in 1cd9877

<script type="text/javascript">

Since these are for the classic editor which is (usually) only used in the admin, I think it is out of scope.

There are also 50 other examples in /wp-admin

See #4773 (comment)

spacedmonkey · 2023-09-19T16:42:06Z

I've intentionally excluded instances that are specifically used in the wp-admin. This is to reduce scope.

I agree that would make this PR too big. But I would add a todo or other code comment on the ones wp-includes. It is clear now why you have done what you done, but it may stop a future developer trying to "fix" the issue, it is not using this function is by design.

Side note, wp_block_theme_activate_nonce should use wp_add_inline_script.

westonruter · 2023-09-19T16:51:08Z

I agree that would make this PR too big. But I would add a todo or other code comment on the ones wp-includes. It is clear now why you have done what you done, but it may stop a future developer trying to "fix" the issue, it is not using this function is by design.

I'm not sure this is necessary. If someone wants to fix up other instances, more power to them. Using the function won't hurt. It's just that it won't get all the benefits since there are some blockers for complete coverage. Adding comments seems like it would just create noise. As long it is clear in the ticket that the scope is limited to the frontend and wp-login, I think this is sufficient.

Side note, wp_block_theme_activate_nonce should use wp_add_inline_script.

Yes, but since it's only used in wp-admin then we can defer it to fix later.

spacedmonkey

This looks good to me. I would like the follow on ticket for changing the admin scripts to be created before this is committed, just so we don't lose track of it.

felixarntz

@westonruter This basically looks good to me, though I have a few non-blocking concerns that would be great to get your thoughts on.

felixarntz · 2023-09-19T22:47:27Z

src/wp-includes/class-wp-customize-nav-menus.php

@@ -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 ) ) );


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

felixarntz · 2023-09-19T22:50:05Z

src/wp-includes/comment-template.php

@@ -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';}})();" );


Same question here.

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

felixarntz · 2023-09-19T22:50:53Z

src/wp-includes/script-loader.php

-		$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
+		);


felixarntz · 2023-09-19T22:54:20Z

src/wp-includes/script-loader.php

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


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?

felixarntz · 2023-09-19T23:01:40Z

tests/phpunit/tests/dependencies/scripts.php

+		$expected = str_replace( " type='text/javascript'", '', $expected );
+		$expected = str_replace( ' type="text/javascript"', '', $expected );
+		$expected = str_replace( "/* <![CDATA[ */\n", '', $expected );
+		$expected = str_replace( "\n/* ]]> */", '', $expected );
+		$expected = str_replace( ' defer="defer"', ' defer', $expected );
+		$expected = str_replace( ' async="async"', ' async', $expected );
+
+		$actual = str_replace( " type='text/javascript'", '', $actual );
+		$actual = str_replace( ' type="text/javascript"', '', $actual );
+		$actual = str_replace( "/* <![CDATA[ */\n", '', $actual );
+		$actual = str_replace( "\n/* ]]> */", '', $actual );
+		$actual = str_replace( ' defer="defer"', ' defer', $actual );
+		$actual = str_replace( ' async="async"', ' async', $actual );


Not sure I'm a fan of having all these exceptions here. Can we somehow ensure the tests pass the relevant values instead? The CDATA one feels okay to me, but otherwise, I think starting to have exceptions here sets a bad precedent that could eventually move this assertion further away from asserting "equal markup".

Maybe instead create a more specific assertEqualScriptTag or something like that with these exceptions?

How about 3ba5135? I improved the normalization process to use the DOM instead, so it will be much more robust and safe. I also made it clear from the method description that it compares with normalizations applied. Lastly, since this method is contained in the Tests_Dependencies_Scripts class I think this also sufficiently indicates that it is specific to checking script tag markup.

My feedback wasn't really about how this was implemented, only about that there are now these "exceptions" where it's not actually equal.

I don't feel strongly about that, so happy to keep it as long as it's only used in this scripts.php test file. The name is mostly what throws me off. I'd be on board if this was called something more specific to script markup. Maybe we can just rename the assertion since it's (as far as I can tell) only used in this class anyway?

Regarding DOMDocument, I personally don't trust it. It has so many weird quirks, so IMO the str_replace from before was more accessible to other developers (FWIW I don't understand half the code you added for the DOMDocument approach) and less error-prone.

The benefit of DOMDocument is PHPUnit supports comparing two instances in assertEquals, and it normalizes non-semantic differences (e.g. attribute order). See #4773 (comment)

Also https://weston.ruter.net/2023/07/01/comparing-markup-with-phpunit/

In other words, assertEquals is intentionally not strict in how it compares equality. It checks semantic equality as opposed to assertSame. So == instead of ===. So I think it makes sense.

src/wp-includes/blocks/categories.php

…into trac-58664

westonruter · 2023-09-20T22:38:33Z

composer.json

@@ -12,6 +12,9 @@
 	"require": {
 		"php": ">=7.0"
 	},
+	"suggest": {
+		"ext-dom": "*"


I could have put this in require since the extension is required when running tests (and not just the tests being introduced in this PR). But since it is not required to actually run WordPress, I went with suggest.

PhpStorm, at least, rightly identifies this as having been missing:

Another reason to probably not use this approach and stick to the simple str_replace that doesn't rely on new dependencies :)

Actually, it was already using DOMDocument. It turns out that assertEquals in PHPUnit supports comparing two DOMElement instances, and this then accounts for differences in attribute order or whether single vs double quotes are used automatically. So I'm just extending that a bit further in assertEqualMarkup to normalize a bit more for HTML5 vs XHTML.

And DOMDocument is being used in other tests as well, so it's not a new dependency. It's just a dependency that wasn't declared before.

westonruter · 2023-09-25T21:17:03Z

Committed in r56687.

dmsnell

left some post-hoc comments. I think there are opportunities to follow-up on this, and particularly do something about str_replace( '<script>', '' ) because that seems dangerous and needlessly inefficient.

overall it looks better than it was though.

dmsnell · 2023-09-25T22:06:26Z

src/wp-includes/class-wp-customize-manager.php

@@ -2106,6 +2109,7 @@ public function remove_frameless_preview_messenger_channel() {
 		} )();
 		</script>
 		<?php
+		wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );


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.

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.

Private helper function added in #5301

dmsnell · 2023-09-25T22:16:11Z

src/wp-includes/class-wp-customize-manager.php

@@ -2106,6 +2109,7 @@ public function remove_frameless_preview_messenger_channel() {
 		} )();


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 ); }

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.

Filed this in Core-59480

dmsnell · 2023-09-25T22:23:42Z

src/wp-includes/class-wp-customize-manager.php

@@ -5012,6 +5019,7 @@ public function customize_pane_settings() {
 			?>
 		</script>
 		<?php
+		wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) );


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 );

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

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"?

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:

The entire string is longer than the opener and closer, and

The string starts with $opener, and

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><script></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.

I've implemented this in #5301

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.

dmsnell · 2023-09-25T22:26:23Z

src/wp-includes/class-wp-customize-widgets.php

-		<?php
+		wp_print_inline_script_tag(
+			// language=JavaScript
+			sprintf( 'var _wpWidgetCustomizerPreviewSettings = %s;', wp_json_encode( $settings ) )


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

dmsnell · 2023-09-25T22:34:18Z

src/wp-includes/script-loader.php

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


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 🙃

dmsnell · 2023-09-25T22:36:41Z

tests/phpunit/tests/customize/manager.php

@@ -3136,7 +3136,7 @@ public function test_remove_frameless_preview_messenger_channel() {
 		ob_start();
 		$manager->remove_frameless_preview_messenger_channel();
 		$output = ob_get_clean();
-		$this->assertStringContainsString( '<script>', $output );
+		$this->assertStringContainsString( '<script', $output );


would be nice to know what this assertion is supposed to be testing. what's the point here? confirm that we have a SCRIPT element in the output?

$processor = new WP_HTML_Tag_Processor( $output ); $this->assertTrue( $processor->next_tag( 'script' ), 'Failed to find expected SCRIPT element in output.' );

😉

Done in 2b764e6 as part of #5301

dmsnell · 2023-09-25T22:38:10Z

tests/phpunit/tests/dependencies/scripts.php

 			array(
 				'id' => 'ms-isa-1-js-after',
 			)
 		);
-		$this->assertSame( $expected, $output, 'Inline scripts in the "after" position, that are attached to a deferred main script, are failing to print/execute.' );
+		$this->assertEqualMarkup( $expected, $output, 'Inline scripts in the "after" position, that are attached to a deferred main script, are failing to print/execute.' );


❤️ all of these. great.

also, did you know that we're finally going to get a spec-compliant HTML5/DOM parser in PHP?

doesn't remove the need for the HTML API (mainly because of memory and performance and interface needs) but it will go a long way in our tests to eliminating false errors.

dmsnell · 2023-09-25T22:41:47Z

tests/phpunit/tests/dependencies/scripts.php

@@ -257,7 +260,7 @@ public function test_blocking_dependent_with_delayed_dependency( $strategy ) {
 		wp_enqueue_script( 'main-script-a3', '/main-script-a3.js', array(), null, compact( 'strategy' ) );
 		wp_enqueue_script( 'dependent-script-a3', '/dependent-script-a3.js', array( 'main-script-a3' ), null );
 		$output   = get_echo( 'wp_print_scripts' );
-		$expected = "<script type='text/javascript' src='/main-script-a3.js' id='main-script-a3-js' data-wp-strategy='{$strategy}'></script>";
+		$expected = str_replace( "'", '"', "<script type='text/javascript' src='/main-script-a3.js' id='main-script-a3-js' data-wp-strategy='{$strategy}'></script>" );


should this be a assertEqualMarkup() scenario?
if not, the HEREDOC might make it less quirky

$expected = <<<HTML <script type="text/javascript" src="/main-script-a3.js" id="main-script-a3-js" data-wp-strategy="{$strategy}"></script> HTML;

the initial and terminal newlines are stripped automatically, so this is a trimmed string.

Ah, well, it's actually checking for only one of the script tags of the two. But I can just add the other script so it is checking for both.

See 9e0c1f5 as part of #5301

dmsnell · 2023-09-25T22:43:44Z

tests/phpunit/tests/dependencies/scripts.php

+					array(
+						"/* <![CDATA[ */\n",
+						"\n/* ]]> */",
+					),


this is worrisome because it conflates syntax and content. I guess it's probably low-risk in our tests

westonruter added 4 commits June 30, 2023 19:22

Use script tag helper functions instead of manual construction

0d3ae2d

WIP: Updating tests

86d74f0

Merge branch 'trunk' of https://github.com/WordPress/wordpress-develop …

61c877c

…into trac-58664

Fix Squiz.Strings.DoubleQuoteUsage.NotRequired

9ad9e6b

westonruter commented Aug 31, 2023

View reviewed changes

westonruter force-pushed the trac-58664 branch from 0f687cb to c02d68a Compare August 31, 2023 22:00

Fix Tests_Dependencies_Scripts

b52c335

westonruter force-pushed the trac-58664 branch from c02d68a to b52c335 Compare August 31, 2023 22:06

westonruter commented Aug 31, 2023

View reviewed changes

westonruter added 3 commits August 31, 2023 15:11

Remove extra hyphen in id for translations script

8355a8c

Use wp_print_inline_script_tag() in the_block_template_skip_link()

b9ebf8b

Use wp_print_inline_script_tag() for various scripts in admin screens

d05efbe

westonruter force-pushed the trac-58664 branch from a182421 to d05efbe Compare September 1, 2023 00:09

westonruter marked this pull request as ready for review September 1, 2023 00:28

Use wp_print_inline_script_tag() for admin scripts

3ac9a6e

mukeshpanchal27 reviewed Sep 1, 2023

View reviewed changes

src/wp-admin/includes/class-wp-list-table.php Outdated Show resolved Hide resolved

Fix inline script for list table

adfef39

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

westonruter commented Sep 1, 2023

View reviewed changes

westonruter added 3 commits September 1, 2023 10:52

Merge branch 'trunk' of https://github.com/WordPress/wordpress-develop …

6ba21ef

…into trac-58664

Extend script tag printing functions to accept closure

c19f75d

Use wp_print_inline_script_tag in more places

50aafac

westonruter added 6 commits September 1, 2023 14:49

Utilize language injections to annotate JS strings

0e27a38

See https://www.jetbrains.com/help/phpstorm/using-language-injections.html

Remove incorrect static closures

3b9b278

Update test_remove_frameless_preview_messenger_channel

96b1e7a

Merge branch 'trunk' of https://github.com/WordPress/wordpress-develop …

96788c1

…into trac-58664

Revert wp-admin changes

def2778

Revert changes to wp-admin, closure on wp_print_script_inline_tag, an…

8bcd7a2

…d update more non-admin scripts

westonruter force-pushed the trac-58664 branch from ff4473a to 8bcd7a2 Compare September 13, 2023 23:24

westonruter commented Sep 14, 2023

View reviewed changes

westonruter requested review from spacedmonkey and felixarntz September 18, 2023 15:36

spacedmonkey reviewed Sep 19, 2023

View reviewed changes

src/wp-includes/theme-templates.php Outdated Show resolved Hide resolved

spacedmonkey requested changes Sep 19, 2023

View reviewed changes

westonruter mentioned this pull request Sep 19, 2023

Use inline styles. #4824

Closed

Enqueue script-link script instead of printing

205b8de

spacedmonkey approved these changes Sep 19, 2023

View reviewed changes

felixarntz approved these changes Sep 19, 2023

View reviewed changes

westonruter commented Sep 19, 2023

View reviewed changes

src/wp-includes/blocks/categories.php Outdated Show resolved Hide resolved

westonruter mentioned this pull request Sep 19, 2023

Use wp_get_inline_script_tag() in build_dropdown_script_block_core_categories() WordPress/gutenberg#54637

Merged

westonruter requested a review from azaozz September 20, 2023 21:32

westonruter added 3 commits September 20, 2023 15:03

Merge branch 'trunk' of https://github.com/WordPress/wordpress-develop …

fd9028d

…into trac-58664

Suggest missing ext-dom in composer.json

2db59e5

Use DOM to normalize scripts in document fragment

3ba5135

westonruter commented Sep 20, 2023

View reviewed changes

Escape wrapped CDATA sections

ecc29a9

westonruter force-pushed the trac-58664 branch from 31b32ad to ecc29a9 Compare September 20, 2023 23:27

westonruter added 3 commits September 25, 2023 11:09

Merge branch 'trunk' into trac-58664

832c315

Remove language injection comments for now

3840c54

Revert Gutenberg upstream change

97ad256

westonruter closed this Sep 25, 2023

dmsnell reviewed Sep 25, 2023

View reviewed changes

westonruter mentioned this pull request Sep 26, 2023

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

Closed

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

		@@ -2106,6 +2109,7 @@ public function remove_frameless_preview_messenger_channel() {
		} )();

	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><script></code>',
	'<code>wp_add_inline_script()</code>'
	),
	'4.5.0'
	);
	$data = trim( preg_replace( '#<script[^>]>(.)</script>#is', '$1', $data ) );
	}

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

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

Conversation

westonruter commented Jul 1, 2023 • edited Loading

Testing Instructions

Commit Message

westonruter Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

westonruter commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacedmonkey commented Sep 19, 2023

spacedmonkey left a comment

Choose a reason for hiding this comment

westonruter commented Sep 19, 2023

westonruter commented Sep 19, 2023

spacedmonkey commented Sep 19, 2023

westonruter commented Sep 19, 2023

spacedmonkey left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Sep 25, 2023

dmsnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jul 1, 2023 •

edited

Loading

westonruter Aug 31, 2023 •

edited

Loading

dmsnell Sep 26, 2023 •

edited

Loading

westonruter commented Sep 1, 2023 •

edited

Loading

westonruter Sep 21, 2023 •

edited

Loading