WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 5 years ago

Last modified 5 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 7 years ago.
7648.2.patch (686 bytes) - added by Viper007Bond 7 years ago.
Don't escape literal \n's
7648.3.patch (838 bytes) - added by Viper007Bond 6 years ago.
Patch refresh and simplification

Download all attachments as: .zip

Change History (23)

@Viper007Bond7 years ago

@Viper007Bond7 years ago

Don't escape literal \n's

comment:1 @markjaquith6 years ago

  • Milestone changed from 2.7 to 2.8

Punting to 2.8

comment:2 follow-up: @Denis-de-Bernardy6 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 @DD326 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-Bernardy6 years ago

  • Keywords needs-patch added; has-patch removed

broken patch. :-|

comment:5 @Denis-de-Bernardy6 years ago

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

comment:6 @Viper007Bond6 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 @Viper007Bond6 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.

@Viper007Bond6 years ago

Patch refresh and simplification

comment:8 @Viper007Bond6 years ago

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

comment:9 @Viper007Bond6 years ago

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

comment:10 @Viper007Bond6 years ago

  • Component changed from General to Formatting
  • Owner anonymous deleted

comment:11 follow-up: @Denis-de-Bernardy6 years ago

patch is b0rke

comment:13 in reply to: ↑ 11 @Viper007Bond6 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-Bernardy6 years ago

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

comment:15 @azaozz6 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-Bernardy6 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-Bernardy6 years ago

  • Milestone changed from 2.8 to 2.9

punting per IRC discusion

comment:18 @azaozz5 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: @Viper007Bond5 years ago

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

comment:20 in reply to: ↑ 19 @nacin5 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.