Make WordPress Core

Opened 2 months ago

Closed 8 weeks ago

Last modified 2 weeks ago

#63851 closed task (blessed) (fixed)

Audit wp_json_encode usage with script tags

Reported by: jonsurrell's profile jonsurrell Owned by: jonsurrell's profile jonsurrell
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: General Keywords: good-first-bug has-patch
Focuses: javascript Cc:

Description (last modified by jonsurrell)

wp_json_encode() is used to print data in script tags in many places without the appropriate JSON flags.

#62797 is an example of the general problem where wp_json_encode( $data ) is insufficient to safely print data in a script tag.

A detailed analysis of the problem can be found here where I recommend wp_json_encode( $data, JSON_HEX_TAG | JSON_UNESCAPED_SLASHES ) to encode data safely for use in script tags.

Note that safe in this context means that the content will not alter the expected HTML document structure. The SCRIPT element will not close prematurely, nor will the element be prevented from closing at its apparent </script> close tag. Safe in this context has nothing to do with the behavior of the JavaScript when evaluated by the browser. The JavaScript behavior in unaffected by these changes.

A good example of the ongoing issue is wp_localize_script(). The following snippet breaks the containing script tag and prevents the script element from closing:

<?php
wp_enqueue_script( '_example', '/ex.js', array(), '0.0', false );
wp_localize_script( '_example', '_', array( '<!--' => '<script>' ) );

This produces HTML like

<script id="_example-js-extra">
var _ = {"<!--":"<script>"};
</script>
<script src="http://localhost:8888/ex.js?ver=0.0" id="_example-js"></script>
…the rest of the HTML page

Resulting in a tree like

└─SCRIPT id="_example-js-extra"
  └─#text  var _ = {"<!--":"<script>"}; </script> <script src="http://localhost:8888/ex.js?ver=0.0" id="_example-js"></script> …the rest of the HTML page

The SCRIPT element does not close as expected and the rest of the HTML page is part of the script contents.

[60648] fixed the issue in #62797 and is a good example of the necessary changes.

Change History (10)

#1 @jonsurrell
2 months ago

  • Keywords good-first-bug added

I'm adding "good-first-bug" to this ticket. The necessary changes should be clear and not too invlied. [60648] is a good example for folks to follow.

#2 @jonsurrell
2 months ago

  • Milestone changed from Awaiting Review to 6.9

#3 @jonsurrell
2 months ago

  • Description modified (diff)

#4 @jonsurrell
2 months ago

  • Description modified (diff)

#5 follow-up: @siliconforks
2 months ago

Would it be better for wp_json_encode() to do this automatically?

#6 in reply to: ↑ 5 @jonsurrell
2 months ago

Replying to siliconforks:

Would it be better for wp_json_encode() to do this automatically?

Good question! How JSON (or anything) should be encoded for an HTML page depends on the context. This focuses on JSON in script tags. Elsewhere, JSON should be encoded in other ways with different flags. For example, JSON inside of a <pre> tag would use a completely encoding strategy.

I wouldn't change the default behavior of wp_json_encode() (largely analogous to PHP's json_encode()).

#51159 suggests adding different flavors of wp_json_encode() for different contexts. It's a good idea in theory, but it's difficult to know what contexts should be supported. Even then, some sites may want to use JSON_UNESCAPED_UNICODE, but that isn't appropriate for all sites.

I'd like to focus on addressing these potential problems in Core on this ticket and leave the addition and implementation of new or different functionality aside.

This ticket was mentioned in PR #9557 on WordPress/wordpress-develop by @devasheeshkaul.


2 months ago
#7

  • Keywords has-patch added

This patch audits the use of wp_json_encode() where encoded data is used inside script tags/elements and updates those calls to use safer encoding flags (JSON_HEX_TAG and JSON_UNESCAPED_SLASHES)

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

@devasheeshkaul commented on PR #9557:


2 months ago
#8

How comprehensive is this audit? I believe there were other usages that included some JSON flags. Did you consider those cases and whether they use appropriate flags, or is this only the cases with no flags?

Thank you for the review, @sirreal! Yes, I went through every instance of wp_json_encode() that is (or could be) used inside a script tag. From what I saw, the cases that already had JSON flags were also using the correct ones to prevent this issue.

I'll do another pass to double check all usages and update the patch if I spot anything missed.

#9 @jonsurrell
8 weeks ago

  • Owner set to jonsurrell
  • Resolution set to fixed
  • Status changed from new to closed

In 60681:

Scripts: Use appropriate JSON encoding flags for script tags.

wp_json_encode() with default arguments is insufficient to safely escape JSON for script tags. Use JSON_HEX_TAG | JSON_UNESCAPED_SLASHES flags.

Developed in https://github.com/WordPress/wordpress-develop/pull/9557.

Props devasheeshkaul, jonsurrell, siliconforks.
Fixes #63851.

#10 @westonruter
2 weeks ago

In 60899:

Emoji: Convert the emoji loader from an inline blocking script to a (deferred) script module.

This modernizes the emoji loader script by converting it from a blocking inline script with an IIFE to a script module. Using a script module improves the performance of the FCP and LCP metrics since it does not block the HTML parser. Since script modules are deferred and run immediately before DOMContentLoaded, the logic to wait until that event is also now removed. Additionally, since the script is loaded as a module, it has been modernized to use const, let, and arrow functions. The sourceURL comment is also added. See #63887.

The emoji settings are now passed via a separate script tag with a type of application/json, following the new "pull" paradigm introduced for exporting data from PHP to script modules. See #58873. The JSON data is also now encoded in a more resilient way according to #63851. When the wp-emoji-loader.js script module executes, it continues to populate the window._wpemojiSettings global for backwards-compatibility for any extensions that may be using it.

A new uglify:emoji-loader grunt task is added which ensures wp-includes/js/wp-emoji-loader.js is processed as a module and that top-level symbols are minified.

Follow-up to [60681].

Props westonruter, jonsurrell, adamsilverstein, peterwilsoncc.
See #63851, #63887.
Fixes #63842.

Note: See TracTickets for help on using tickets.