Use PHP native double encoding prevention in htmlspecialchars()

[nbachiyski]

Since PHP 5.2.3 the htmlspecialchars() function has an optional $double_encode parameter, which we could use. This can save us a few expensive kses/html decoding calls.

We need to make sure it works the same way as our implementation.

[miqrogroove]

This may be more than a 'minor' enhancement if it works with minimal fuss.

[miqrogroove]

Added some basic unit tests. Take note of some quirks in the existing non-double-encoding mode: KSES normalizes the number of digits in numeric references and is currently not HTML5 compatible.

[miqrogroove]

After patching, there were only two differences in test results:

• Number of digits is not normalized in numeric references.
• ' fails the double-encoding test in PHP for some reason.

IMO, neither of these is a deal breaker and we should consider making the change.

[miqrogroove]

Adds a test exception for apos (')

[miqrogroove]

Should add more tests to determine behaviors when function input is already double-encoded.

[miqrogroove]

See also #8767, #12284, and #32556. I'm not sure there was ever a valid reason for calling specialchars_decode but it is worth reviewing.

[miqrogroove]

Test coverage for unexpected decoding of entities.

[miqrogroove]

Added test results in patched version.

[miqrogroove]

After patching, there are three differences in test results:

• Number of digits is not normalized in numeric references.
• ' fails the double-encoding test in PHP for some reason.
• Entities that were double-encoded at input will not get decoded for output.

IMO, these all remain acceptable differences. The original call to specialchars_decode appears to be a mistake in [10297] and I am looking for any additional information about why that was committed in the first place.

[miqrogroove]

I see where the decoding bug was introduced now. Here is an explanation:

• In [10297] the strategy to prevent double encoding was to decode specialchars before the call to encode specialchars. This was a harmless, but ultimately futile algorithm because it wouldn't do anything.
• In [10298] a placeholder strategy was added to accomplish actual avoidance of double encoding. It appears the author failed to remove the decode command from the patch, resulting in unnecessary decoding prior to the placeholder insertion.
• In #12284 although the bug was mentioned there, I didn't dig this far to find out what was the original problem.

I'm open to other opinions, but it looks like the reference decoding by this function is entirely unintentional.

[azaozz]

Replying to miqrogroove:

Unfortunately I don't remember anything about these patches, it's been more than 6 years :)

Seems like at the time (and maybe still) _wp_specialchars() is getting double encoded strings and is expected to return single encoded. It is "undocumented behaviour" but not sure if we can remove it.

[mdgl]

Related #31190?

[miqrogroove]

#31190 was marked as a duplicate.

[miqrogroove]

Replying to azaozz:

Seems like at the time (and maybe still) _wp_specialchars() is getting double encoded strings and is expected to return single encoded.

That would be a significant bug in itself and I am very skeptical of it.

[obenland]

@wonderboymusic, could you review and commit/punt when you get a chance?

[miqrogroove]

Fixes tests affecting other functions.

[wonderboymusic]

In 32850:

Since PHP 5.2.3, the htmlspecialchars() function has an optional $double_encode parameter, which we can now use. This will save us a few expensive kses/html decoding calls.

Adds unit tests.

Props miqrogroove.
Fixes #17780.

[miqrogroove]

Test results from @netweb showed that prior to PHP 5.4, the $double_encode parameter does not perform entity validation.

[netweb]

Replying to miqrogroove:

Test results from @netweb showed that prior to PHP 5.4, the $double_encode parameter does not perform entity validation.

Here's the Travis-CI build: ​https://travis-ci.org/aaronjorbin/develop.wordpress/builds/67442185

• PHP 5.2 Job -> ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/67442187
• PHP 5.3 Job -> ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/67442188

[johnbillion]

What's the deal with '?

[azaozz]

Replying to johnbillion:

Officially it is defined in XML and XHTML, but not in HTML: ​https://en.wikipedia.org/?title=List_of_XML_and_HTML_character_entity_references#Entities_representing_special_characters_in_XHTML

[miqrogroove]

Compatibility for PHP < 5.4.

[netweb]

Test results of miqro-17780-compat.patch​:

• Travis-CI Build -> ​https://travis-ci.org/ntwb/wordpress/builds/67456349
• PHP 5.2 Job -> ​https://travis-ci.org/ntwb/wordpress/jobs/67456351
• PHP 5.3 Job -> ​https://travis-ci.org/ntwb/wordpress/jobs/67456352

All tests passed :)

[miqrogroove]

Expanded comments.

[netweb]

Tests pass via latest patch -> ​https://travis-ci.org/ntwb/wordpress/builds/67458355

[miqrogroove]

Just a note about the ' encoding. I tested ENT_XHTML flag which is not well documented, and found that it gives different output when ENT_QUOTES is also set. It converts each ' into ' which is not necessarily more desirable than converting ' into '. But it is something we can use later if needed.

[azaozz]

In 32851:

Fix using htmlspecialchars() whit the $double_encode parameter. PHP < 5.4 doesn't validate the entities.
Props miqrogroove. Fixes #17780.

[azaozz]

Seems like at the time (and maybe still) _wp_specialchars() is getting double encoded strings and is expected to return single encoded. It is "undocumented behaviour" but not sure if we can remove it.

This broke post titles when there is a & (ampersand) in them. To reproduce:

• Create new post with an ampersand in the title.
• Save draft several times.

[chriscct7]

Replying to azaozz:

Seems like at the time (and maybe still) _wp_specialchars() is getting double encoded strings and is expected to return single encoded. It is "undocumented behaviour" but not sure if we can remove it.

This broke post titles when there is a & (ampersand) in them. To reproduce:

• Create new post with an ampersand in the title.
• Save draft several times.

This would seem to indicate we're missing a unit test for creating a post with many special characters in the title (one for & in particular), and then pulling it out and seeing if it matches.

[JanR]

Replying to azaozz:

Seems like at the time (and maybe still) _wp_specialchars() is getting double encoded strings and is expected to return single encoded. It is "undocumented behaviour" but not sure if we can remove it.

This broke post titles when there is a & (ampersand) in them. To reproduce:

• Create new post with an ampersand in the title.
• Save draft several times.

For what it's worth: WordPress 4.2.2 doesn't double encode the subject, but 4.3 beta does.
It not only affects the ampersand &, it affects ", <, > too.

[miqrogroove]

Guys, please post your test results. I am on the road so getting vague messages about ampersands is frustrating.

I did a quick test of my own using an admin account and found at least two bugs:

1. Post permalinks are not HTML escaped outside of the tag, at least in my theme. Attributes and link elements do not seem affected.
2. The post editor title box is displaying a double-encoded post title.

So after typing a test post title, I found places where my input was returned not encoded, once encoded, and also twice encoded. Further testing is needed.

[wonderboymusic]

In 33148:

Revert [32851] and [32850] for now, tl;dr encoding issues.

See #17780.

[miqrogroove]

Fixes post editor bug

[miqrogroove]

New patch includes bugfix for editing post titles.

[miqrogroove]

Tested using post title input Title encoding & test < blah > my &copy; your &amp; Checked admin and non-admin accounts. Found patched trunk behavior identical to 4.2.2.

[samuelsidler]

I think we should punt this from 4.3 and get it in early in the 4.4 cycle. No 4.3 features / tickets are dependent on this ticket and we clearly didn't have enough test coverage when it was committed the first time. We should add more comprehensive tests and commit it at the very start of the 4.4 cycle to ensure there are no other, similar issues to comment:27.

[miqrogroove]

I'd like to see the patch in beta 3. If you really think this is going to cause some damage then the right thing to do here may be to deprecate esc_attr() and related functions and replace them all with corrected functions. And that wouldn't be ready for one or two versions for sure.

[azaozz]

Looking back at the changes to the post_title field: seems this is a long existing bug that was masked by the "unusual" behaviour of _wp_specialchars(). The post_title is escaped first with sanitize_post_field() then with htmlspecialchars() and finally with esc_attr().

As far as I see this is the only place where esc_attr() and htmlspecialchars() are nested. Not sure if fixing this at the beginning of a cycle will be any different than fixing it now. In both cases plugins that have copied that particular code from core (and don't follow WordPress development) will break. Chances are that most plugin authors will test their plugins in RC or perhaps when they receive the "What's new in 4.3" email :)

[miqrogroove]

Replying to chriscct7:

This would seem to indicate we're missing a unit test for creating a post with many special characters in the title (one for & in particular), and then pulling it out and seeing if it matches.

I would like to humbly suggest that this is beyond the scope of this ticket. New ticket #32990.

[wonderboymusic]

Gonna review this

[wonderboymusic]

In 33271:

After [33148]:
Don't nest esc_attr() and htmlspecialchars() when escaping the post title on the edit post screen.

Unrevert parts of [32851] and [32850].

Adds/alters unit tests.

Props miqrogroove.
Fixes #17780.