flag esc_* inside <script> tag

[david-binda]

It's almost always meant to be wp_json_encode

[david-binda]

We could check via sniff whether there is an open HTML attribute prior esc_js, and if not, it's in 99% of cases wrongly used function.

[GaryJones]

Please provide some more context here, such as example code that should be reported, and example code that shouldn't.

[david-binda]

Some more context also containing code examples can be found here: https://vip.wordpress.com/documentation/vip-go/vip-code-review/javascript-security-best-practices/#escaping-dynamic-javascript-values

Some other examples can be found in comments in the WordPress documentation for the esc_js function: https://developer.wordpress.org/reference/functions/esc_js/

esc_js should be used only in HTML attributes, like following, correct, example:

<input type="text" onfocus="if ( this.value == '<?php echo esc_js( $instance['input_text'] ); ?>') { this.value = ''; }" name="email" />

and should not really be used inside script tag, where wp_json_encode should be used instead. Bad example:

<script type="text/javascript>
var foo = <?php echo esc_js( $bar ); ?>;
</script>

~Using wp_json_encode in the above example is the correct way to escape the $bar variable:~ (Edit by Gary: No it's not - see https://github.com/Automattic/vip-code-samples/tree/master/10-security)

<script type="text/javascript">
var foo = <?php echo wp_json_encode( $bar ); ?>;
</script>

[GaryJones]

Is this something that would benefit everyone? i.e. not VIP-specific?

Seems like https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/master/WordPress/Sniffs/Security/EscapeOutputSniff.php would be a good location for this to be addressed in?

[GaryJones]

While esc_js() is almost certainly wrong, I think we'd be better pointing to https://github.com/Automattic/vip-code-samples/tree/master/10-security#escaping-dynamic-javascript-values or similar and letting the developer work out what the correct code should be.

[rebeccahum]

@GaryJones Agreed, since context is important in this aspect. However, I think flagging esc_js() usage would be a valid sniff.