Eliminate manual construction of script tags in WP_Scripts and of inline scripts on frontend/login screen

[westonruter]

Helper functions for constructing script tags (wp_print_inline_script_tag(), wp_get_inline_script_tag(), and wp_get_script_tag()) were ​added in WP 5.7. However, they were not implemented in WP_Scripts for where core prints the majority of its scripts. Some of the instances were replaced in [56033] for #12009, specifically for inline before/after scripts.

1. ​Main registered scripts
2. ​Extra scripts (i.e. from wp_localize_script())
3. ​Translation ​scripts

Using the helper functions also makes the code much more readable as well as more robust by automatically escaping attribute values and allowing the wp_script_attributes and wp_inline_script_attributes filters to apply to the attributes being printed. It also ensures the non-HTML5 CDATA wrapper comments are added consistently. This would seem to be a logical follow-up to #39941 which introduced these functions but didn't make use of them in WP_Scripts. This will facilitate adding CSP attributes to scripts that core prints.

Caveat: Some plugins are (ab)using the clean_url filter to inject async/defer attributes into script tags. Such plugins will break with the adoption of these helper functions. Any such plugins should be updated to use the new script loading strategies instead, or inject attributes with the script_loader_tag filter which is a much better fit for this purpose.

A ​stale pull request is exists which drafted this change.

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

[oglekler]

Hi @westonruter, this ticket was discussed during a bug scrub.

Your patch is still in the draft and there are also failed unit and CS tests. It looks like Unit tests needs to be adjusted. When the patch will be updated, please remove draft status )

Add props to @mukesh27

[westonruter]

Replying to oglekler:

Your patch is still in the draft and there are also failed unit and CS tests. It looks like Unit tests needs to be adjusted. When the patch will be updated, please remove draft status )

Thanks for the ping. The changes were originally planned for 6.3 but we punted to 6.4 due to the large number of other changes being made to scripts for the loading strategies. It remained a draft due to not having the unit tests updated. So yeah, it's due to get refreshed and finalized.

[westonruter]

I've marked the PR ready for initial review. There are some outstanding questions for how best we can migrate existing inline scripts over to using wp_print_inline_script_tag(), but it's ready to be checked out.

Here's a proof of concept plugin that enables Script CSP on the frontend and wp-login: ​https://gist.github.com/westonruter/c8b49406391a8d86a5864fb41a523ae9

[westonruter]

I realize I expanded the scope to include scripts printed outside of WP_Scripts as well. If it turns out expanding the scope too much, we can extract that non-WP_Scripts changes to another ticket. But in order to be able to apply Strict CSP, all scripts need to be printed in the same way so the attributes can be filtered.

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.

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

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

[westonruter]

For a discussion about the use of output buffering to ensure IDE intelligence for inline script tags with wp_print_inline_script_tag() extended to support passing a closure, see ​thread in #core-js. For example:

<?php
wp_print_inline_script_tag(
        static function () {
                ?>
                <script>
                        var foo = 'bar';
                        /* JS code goes here */
                </script>
                <?php
        }
);

[azaozz]

Replying to westonruter:

For a discussion about the use of output buffering to ensure IDE intelligence for inline script tags with wp_print_inline_script_tag() extended to support passing a closure, see ​thread in #core-js. For example:

<?php
wp_print_inline_script_tag(
        static function () {
                ?>
                <script>
                        var foo = 'bar';
                        /* JS code goes here */
                </script>
                <?php
        }
);

Using a lambda function there looks a bit like a "weird hack" :)

Seems wp_print_inline_script_tag() is not particularly suitable for this kind of usage. Wouldn't it be better to try to improve it instead of wrapping all "normal" scripts in WP in that PHP code?

Also I don't think it is a good change for wp_print_inline_script_tag() to accept a callback as well as a string. Seems to just make it all more complex and maybe a bit confusing without much benefits. These are all "hard coded" scripts in wp-admin that can (and should) be tweaked or updated as needed.

[westonruter]

Replying to azaozz:

Replying to westonruter:

For a discussion about the use of output buffering to ensure IDE intelligence for inline script tags with wp_print_inline_script_tag() extended to support passing a closure, see ​thread in #core-js. For example:

<?php
wp_print_inline_script_tag(
        static function () {
                ?>
                <script>
                        var foo = 'bar';
                        /* JS code goes here */
                </script>
                <?php
        }
);

Using a lambda function there looks a bit like a "weird hack" :)

It does look weird. But the benefit is cross-IDE syntax highlighting and intelligence.

Seems wp_print_inline_script_tag() is not particularly suitable for this kind of usage. Wouldn't it be better to try to improve it instead of wrapping all "normal" scripts in WP in that PHP code?

The issue isn't that wp_print_inline_script_tag() needs to be improved per se, but that there should be a way to construct inline script tags using that function while also retaining IDE features. Since inline scripts in PHP don't get any linting performed, it's important to get as much IDE support as possible to catch bugs.

Also I don't think it is a good change for wp_print_inline_script_tag() to accept a callback as well as a string. Seems to just make it all more complex and maybe a bit confusing without much benefits. These are all "hard coded" scripts in wp-admin that can (and should) be tweaked or updated as needed.

The benefits are:

1) Retaining syntax-highlighting and IDE intelligence features.
2) Constructing the ultimate script tag with filters applying on the attributes, allowing nonce to be added for CSP.

If the JS isn't passed through wp_print_inline_script_tag(), then there would have to be an alternative way to obtain the nonce attribute for CSP. For example there could be the_script_tag_attributes():

<script <?php the_script_tag_attributes(); ?>>
var foo = 'bar';
/* JS code goes here */
</script>

And this the_script_tag_attributes() could apply the same wp_inline_script_attributes filters as wp_get_inline_script_tag() does. But the problem is that it wouldn't have the $javascript argument to pass to those filters.

[westonruter]

I've reverted the ability to pass a closure to wp_print_inline_script_tag().

I've also reverted changes to the wp-admin.

As I've just updated the ​PR description:

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 (uses manual script tag in IE conditional comment)
• Twenty Twelve (uses manual script tag in IE conditional comment)
• Twenty Thirteen
• Twenty Fourteen (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)

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

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/theme-previews.php#L69-L76

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/class-wp-embed.php#L91-L95

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/class-wp-editor.php#L977

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/class-wp-editor.php#L1563

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/class-wp-editor.php#L1620

There are also 50 other examples in /wp-admin

@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 There seem to be number of places where we should use this function.

@spacedmonkey:

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/theme-previews.php#L69-L76

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

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/class-wp-embed.php#L91-L95

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.

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/class-wp-editor.php#L977

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/class-wp-editor.php#L1563

​https://github.com/WordPress/wordpress-develop/blob/1cd98779f712991363071131ad58a47a3604abe8/src/wp-includes/class-wp-editor.php#L1620

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 ​https://github.com/WordPress/wordpress-develop/pull/4773#issuecomment-1725892406

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.

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.

[westonruter]

In 56687:

Script Loader: Use wp_get_script_tag() and wp_get_inline_script_tag()/wp_print_inline_script_tag() helper functions to output scripts on the 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 when not using HTML5.
• 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.

[westonruter]

Needs dev note because of existing ecosystem code that may filter clean_url instead of script_loader_tag to inject async & defer attributes. For example, ​WP Engine article and ​WPdirectory search. This legacy method of injecting async and defer should be replaced with what was introduced in #12009, the script loading strategies. Using clean_url will no longer work since the script URL is being passed into esc_url_raw() (within which the clean_url filter is applied) and then the resulting URL is passed into wp_sanitize_script_attributes which ensures the attribute values are all properly escaped. Previously, no escaping was done on the return value of esc_url() meaning the clean_url filter could be abused for HTML attribute injection: this was incredibly brittle since it relied on single quoted attribute values to be used and it also inefficient since it applied on all escaped URLs, not just script URLs.

Committed in r56687.

[westonruter]

Given aforementioned plugin code for injecting defer via the clean_url filter:

<?php
function defer_parsing_of_js ( $url ) {
        if ( FALSE === strpos( $url, '.js' ) ) return $url;
        if ( strpos( $url, 'jquery.js' ) ) return $url;
        return "$url' defer ";
}
add_filter( 'clean_url', 'defer_parsing_of_js', 11, 1 );

This is resulting in the following being rendered for underscore:

Since the script has ver query parameter, the resulting URL is parsed by the browser to be:

http://localhost:8889/wp-includes/js/underscore.js?ver=1.13.4%27%20defer

And this does not result in a 404. So I'd say that's graceful degradation.

However, if a script lacked a ver query parameter (which is unusual) then this could indeed result in a 404. If we were concerned about that, we could add backwards-compatibility with something like this:

src/wp-includes/class-wp-scripts.php

@@ -373 +373 @@
                 /** This filter is documented in wp-includes/class-wp-scripts.php */
                 $src = esc_url_raw( apply_filters( 'script_loader_src', $src, $handle ) );
 
+                if ( preg_match( "/^(.+?)' (.*)$/", $src, $matches ) ) {
+                        _deprecated_hook( 'clean_url', '6.3', 'script loading strategies', __( 'Do not use clean_url filter to inject script tag attributes.' ) );
+                        $src = $matches[1];
+                }
+
                 if ( ! $src ) {
                         return true;
                 }

[westonruter]

See #59446 for continuing this effort to the rest of the wp-admin.

[westonruter]

In 56748:

Script Loader: Harden removal of script tag wrappers.

• Add wp_remove_surrounding_empty_script_tags() to more precisely remove script tag wrappers and warn when doing it wrong.
• Add clarifying comments for XML escaping logic in wp_get_inline_script_tag().
• Leverage WP_HTML_Tag_Processor in test_remove_frameless_preview_messenger_channel.
• Reuse assertEqualMarkup in test_blocking_dependent_with_delayed_dependency.
• Normalize whitespace in parse_markup_fragment for assertEqualMarkup.

Follow-up to [56687].
Props dmsnell, westonruter, flixos90.
See #58664.

[westonruter]

In 56750:

Script Loader: Remove erroneous type attribute from script tag wrapper on login screen.

This was causing a _doing_it_wrong() notice from wp_remove_surrounding_empty_script_tags(). In fact, the type attribute was added in [56748] to test this incorrect usage notice. This commit reverts that change.

Follow-up to [56748].
Unprops westonruter.
See #58664.

[westonruter]

In 56932:

Script Loader: Enqueue inline style for block template skip link in head instead of footer.

• Introduce wp_enqueue_block_template_skip_link() to replace the_block_template_skip_link(). Add to wp_enqueue_scripts action instead of wp_footer.
• Keep inline script for skip link in footer.
• Restore original the_block_template_skip_link() from 6.3 and move to deprecated.php.
• Preserve back-compat for unhooking skip-link by removing the_block_template_skip_link from wp_footer action.

Follow-up to [56682] and [56687].

Props sabernhardt, plugindevs, westonruter, spacedmonkey.
Fixes #59505.
See #58775.
See #58664.

[codente]

@westonruter Is there a draft dev note for this?

We're tracking this dev note for documentation over at:
​https://github.com/WordPress/Documentation-Issue-Tracker/issues/1187

[westonruter]

Replying to codente:

@westonruter Is there a draft dev note for this?

Dev note: ​https://make.wordpress.org/core/2023/10/17/script-loading-changes-in-wordpress-6-4/

[swissspidy]

#43248 was marked as a duplicate.