Flag any echos inside of `<script>` tags if `wp_json_encode()` not used and ensure JSON is encoded with the best flags

[westonruter]

In order to guard against XSS vulnerabilities, inside of a <script> tag, any echo, printf, or other means of data being inserted into the JS code should be flagged as an error if wp_json_encode() is not used. Additionally, using wp_json_encode() on its own is not entirely robust, as @sirreal has identified in his post Safe JSON in script tags: How not to break a site.

What's more is that manual construction of <script> tags should itself be flagged as an error to ensure compatibility with any Content Security Policy. See https://github.com/WordPress/WordPress-Coding-Standards/issues/2575.

This should probably exclude any use of file_get_contents() for passing through a JS file as an inline script.

Bad

There's no guarantee that $data_as_json is valid JSON from a sniff point of view:

<script type="application/json">
<?php echo $data_as_json; ?>
</script>

Encoding/escaping not done at point of printing as in the following, but it's not ideal either because it lacks the best JSON flags:

<script type="application/json">
<?php echo wp_json_encode( $data ); ?>
</script>

Better

<script type="application/json">
<?php
echo wp_json_encode(
	$data,
	JSON_HEX_TAG | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_LINE_TERMINATORS
);
?>
</script>

Best

See https://github.com/WordPress/WordPress-Coding-Standards/issues/2575

wp_print_inline_script_tag(
	wp_json_encode(
		$data,
		JSON_HEX_TAG | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_LINE_TERMINATORS
	),
	array( 'type' => 'application/json' )
);

---

Old description from 2013:

Relates to XSS checking script, but we should restrict it further.

[jrfnl]

What about if the echo uses esc_js() ?

[JDGrimes]

esc_js() is intended for escaping string within JS in an HTML attribute like onclick. wp_json_encode() should be used instead inside of <script> blocks.

[westonruter]

I don't really see the value of using esc_js() anymore. If I ever needed to do an inline script attribute, I would do the following, which seems easier to read and maintain:

<?php
$onfocus = sprintf( 
    'if ( %s === this.value ) { this.value = ""; }',
    wp_json_encode( $instance['input_text'] )
);
$onblur = sprintf(
    'if ( "" === this.value ) { this.value = %s; }',
    wp_json_encode( $instance['input_text'] )
);
?>
<input id="subbox" type="text" name="email"
    value="<?php echo esc_attr( $instance['input_text'] ); ?>"
    onfocus="<?php echo esc_attr( $onfocus ); ?>"
    onblur="<?php echo esc_attr( $onblur ); ?>" />

[jrfnl]

I generally still use esc_js() within wp_localize_script() when setting up the variables. Is that not correct ?

[westonruter]

@jrfnl do you have an example?

[JDGrimes]

I generally still use esc_js() within wp_localize_script() when setting up the variables. Is that not correct ?

wp_localize_script() already serializes everything on output with wp_json_encode(). So I don't see why escaping would be needed there at all. 😉

[grappler]

Noting that today wp_add_inline_script() should be used instead of a script snippet.

[westonruter]

Blast from the past!

This issue has come up again recently. I've re-written the description to flesh it out and update it for changes in the past 12 years. See comments by @sirreal and myself on Core-58873. While wp_add_inline_script() could be used, we also now (since 5.7 in 2021) have wp_print_inline_script_tag(). See also the alternative mechanism for exporting data from PHP to script modules via JSON scripts.

I've also filed this related issue:

• https://github.com/WordPress/WordPress-Coding-Standards/issues/2575

[rodrigoprimo]

I've been looking into this issue and wanted to share some thoughts on a possible approach.

Please, correct me if I'm wrong, but I believe that reliably detecting whether wp_json_encode() is being used inside a <script> tag is not trivial (or maybe not even possible in all cases).

With that in mind, an alternative approach would be to create a sniff similar to WordPress.WP.GetMetaSingle that warns when wp_json_encode() and other related functions are called without the $flags parameter. The sniff wouldn't recommend specific flags since the appropriate value depends on context. The warning message could point to a wiki page with guidance on which flags to use in different contexts.

This way, we give the developer an opportunity to consider the context in which the function call is used to decide which flags should be passed, if any, without risking false positives or false negatives. How does that sound?

[anomiex]

With that in mind, an alternative approach would be to create a sniff similar to WordPress.WP.GetMetaSingle that warns when wp_json_encode() and other related functions are called without the $flags parameter. The sniff wouldn't recommend specific flags since the appropriate value depends on context. The warning message could point to a wiki page with guidance on which flags to use in different contexts.

Note some of the recent discussion here is due to https://github.com/Automattic/jetpack/pull/46227 adding a sniff doing almost exactly this in Jetpack's codesniffer rules. The only differences are that it links a markdown document rather than a wiki page, and it also flags passing 0 for the flags. 🙂

Which reminds me, I still need to look into the possibility of relicensing Jetpack's codesniffer package to be more compatible with this and PHPCS upstream.

[rodrigoprimo]

it also flags passing 0 for the flags

I'm suggesting that the sniff does not flag passing 0 for $flags, because, if I understand correctly, there are valid scenarios where no flag is needed.

[anomiex]

The Jetpack sniff emits a separate code for that, so it can easily be disabled in the config if someone doesn't want to flag it.

In Jetpack we instead chose to do like

0 // phpcs:ignore Jetpack.Functions.JsonEncodeFlags.ZeroFound -- No `json_encode()` flags because this needs to match whatever is calculating the hash on the other end.

for those cases to document why 0 was being passed (in most cases because it's being used to stringify data for calculation of a hash or detached signature for an API call, and both ends need to serialize the data in the same way).

[sirreal]

there are valid scenarios where no flag is needed

I can't think of a situation where the default behavior is ideal. The default escaping is unnecessary in some situations and insufficient in others. Writing to a file should use unescape flags, printing as HTML text should use unescape flags and later HTML escaping, writing in a SCRIPT tag should use the flags described above to unescape slashes and escape < characters (at least JSON_HEX_TAG | JSON_UNESCAPED_SLASHES).

Use of the defaults may be justified for cases like @anomiex mentioned where the data just needs to match. More ideal forms with unescape flags could be used, but that's not always possible or worth the effort in established systems.

The goal is putting helpful and timely information in front of a developer, then trusting them to act accordingly. 0 seems like something helpful to flag, I suppose it's used to access the $depth argument without changing the default flags. In that case it's susceptible to exactly the same problems and is helpful to flag.

[anomiex]

Which reminds me, I still need to look into the possibility of relicensing Jetpack's codesniffer package to be more compatible with this and PHPCS upstream.

They cleared the change: https://github.com/Automattic/jetpack/pull/46328