WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#7648 closed defect (bug) (invalid)

js_escape() wrongly escapes too many characters

Reported by: Viper007Bond Owned by: azaozz
Milestone: Priority: low
Severity: minor Version: 2.6.1
Component: JavaScript Keywords: needs-patch needs-testing
Focuses: Cc:

Description

Using js_escape(), there is no way to output <, >, or " as they are converted to HTML entities.

Example:

<script type="text/javascript">
<?php
echo 'alert("' . js_escape('HTML \'for\' "bold": <strong>bold text!</strong>') . '");';
?>
</script>

This is the output:

alert("HTML \'for\' &quot;bold&quot;: &lt;strong&gt;bold text!&lt;/strong&gt;");

Rather than this as expected:

alert("HTML \'for\' \"bold\": <strong>bold text!</strong>");

Attachments (3)

7648.patch (626 bytes) - added by Viper007Bond 6 years ago.
7648.2.patch (686 bytes) - added by Viper007Bond 6 years ago.
Don't escape literal \n's
7648.3.patch (838 bytes) - added by Viper007Bond 5 years ago.
Patch refresh and simplification

Download all attachments as: .zip

Change History (23)

Viper007Bond6 years ago

Viper007Bond6 years ago

Don't escape literal \n's

comment:1 markjaquith5 years ago

  • Milestone changed from 2.7 to 2.8

Punting to 2.8

comment:2 follow-up: Denis-de-Bernardy5 years ago

mmm... recently, I got a validation error due to the fact that one of my scripts contained < rather than &lt;, and " rather than &quot; in a string. Maybe double check the facts, at least on quote chars?

comment:3 DD325 years ago

@Denis-de-Bernardy: Correct, In HTML Attributes.

However, Not in <script> blocks as presented by Viper007Bond.

However, I think the js_escape() function might be mis-used in this case, In that, I thought it was designed for escaping for attribute-usage, much like attribute_escape().. though i could be wrong.

comment:4 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch removed

broken patch. :-|

comment:5 Denis-de-Bernardy5 years ago

actually, his example seems like one where it actually should get escaped.

comment:6 Viper007Bond5 years ago

Well it is called js_escape() for a reason, no? There are other functions out there for escaping HTML attributes and such.

comment:7 in reply to: ↑ 2 Viper007Bond5 years ago

Replying to Denis-de-Bernardy:

mmm... recently, I got a validation error due to the fact that one of my scripts contained < rather than &lt;, and " rather than &quot; in a string. Maybe double check the facts, at least on quote chars?

This is because you didn't use CDATA or similiar, which you need to use to ensure validation.

Viper007Bond5 years ago

Patch refresh and simplification

comment:8 Viper007Bond5 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:9 Viper007Bond5 years ago

See #9650 BTW. Might be worth deprecating js_escape() in favor of something like esc_js().

comment:10 Viper007Bond5 years ago

  • Component changed from General to Formatting
  • Owner anonymous deleted

comment:11 follow-up: Denis-de-Bernardy5 years ago

patch is b0rke

comment:13 in reply to: ↑ 11 Viper007Bond5 years ago

Replying to Denis-de-Bernardy:

patch is b0rke

Only due to esc_js(). The patch is still valid-ish, it just needs to affect esc_js() instead now.

comment:14 Denis-de-Bernardy5 years ago

  • Component changed from Formatting to JavaScript
  • Owner set to azaozz

comment:15 azaozz5 years ago

The js_escape()/esc_js() seem to work as expected in echoed html attributes:

echo 'onclick="' . "if ( confirm('" . esc_js(__("You are about to delete ...

Perhaps it needs an arg to specify whether to convert html special chars to entities.

comment:16 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch removed
  • Priority changed from normal to low
  • Severity changed from normal to minor

@Viper007Bond - still current/needing a new patch, right?

comment:17 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.8 to 2.9

punting per IRC discusion

comment:18 azaozz4 years ago

  • Milestone 2.9 deleted
  • Resolution set to invalid
  • Status changed from new to closed

Seems to work properly. It is intended for escaping strings used in JS context in a tag attribute, for example:

<a onclick="alert('<?php echo esc_js($my_text); ?>'); return false;" ...

comment:19 follow-up: Viper007Bond4 years ago

We should rename the function to esc_attr_js() or something then.

comment:20 in reply to: ↑ 19 nacin4 years ago

Replying to Viper007Bond:

We should rename the function to esc_attr_js() or something then.

And/or update the phpdoc: "Escapes text strings for echoing in JS, both inline (for example in onclick="...") and inside <script> tag."

Note: See TracTickets for help on using tickets.