Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

flag esc_* inside <script> tag #113

Open
david-binda opened this issue Nov 30, 2017 · 6 comments · May be fixed by #623
Open

flag esc_* inside <script> tag #113

david-binda opened this issue Nov 30, 2017 · 6 comments · May be fixed by #623

Comments

@david-binda
Copy link
Contributor

It's almost always meant to be wp_json_encode

@david-binda
Copy link
Contributor Author

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
Copy link
Contributor

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

@david-binda
Copy link
Contributor Author

david-binda commented Oct 18, 2018

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
Copy link
Contributor

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?

@rebeccahum rebeccahum changed the title flag esc_js inside <script> tag flag esc_* inside <script> tag Oct 26, 2018
@GaryJones
Copy link
Contributor

GaryJones commented Jul 11, 2020

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
Copy link
Contributor

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

@GaryJones GaryJones added this to the 2.2.0 milestone Jul 13, 2020
@GaryJones GaryJones modified the milestones: 2.2.0, 2.3.0 Aug 29, 2020
@rebeccahum rebeccahum linked a pull request Feb 23, 2021 that will close this issue
@rebeccahum rebeccahum modified the milestones: 2.3.0, 2.4.0 Apr 15, 2021
@GaryJones GaryJones modified the milestones: 2.4.0, 3.x Aug 21, 2023
@rebeccahum rebeccahum modified the milestones: 3.0.1, 3.x May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants