Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#39145 closed defect (bug) (fixed)

custom-background URL escaped

Reported by: futtta's profile futtta Owned by: westonruter's profile westonruter
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch fixed-major commit
Focuses: administration Cc:

Description

In 4.7 the URL of a custom background-image (e.g. in TwentySixteen) added through the customizer is escaped (addslashed):

body.custom-background { background-image: url("http:\/\/localhost\/wordpress\/wp-content\/uploads\/layerslider\/Full-width-demo-slider\/left.png")

This at least [breaks Autoptimize of that CSS]https://wordpress.org/support/topic/incompatibility-with-wp-4-7-aggregate-inline-css/#post-8524535, but is likely to cause other problems as well.

Attachments (1)

39145.diff (596 bytes) - added by tyxla 8 years ago.
Use esc_url() instead of wp_json_encode() for custom background image URL

Download all attachments as: .zip

Change History (13)

#1 @ocean90
8 years ago

  • Focuses ui removed
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7.1

Hello @futtta, welcome to WordPress Trac!

Thanks for the report, I can confirm this issue.

@tyxla
8 years ago

Use esc_url() instead of wp_json_encode() for custom background image URL

#2 @tyxla
8 years ago

  • Keywords has-patch added; needs-patch removed

The issue appears to be caused by the usage of wp_json_encode on the image URL when building the custom background image CSS. The attached patch addresses this by using esc_url instead.

#3 @ocean90
8 years ago

Introduced in [38948].

#4 @futtta
8 years ago

I tested the patch, I can confirm it fixes the problem.

#5 @westonruter
8 years ago

In 39145.diff we should add double quotes to surround the esc_url() call. The wp_json_encode() function adds those double-quotes for us. If we could rely on there being a higher version of PHP, we could have used wp_json_encode() with JSON_UNESCAPED_SLASHES but unfortunately it is not available in PHP 5.2. I'm not totally sure that esc_url() will work for URLs that have characters that get encoded as entities. For example, will browsers decode &?

#6 @tyxla
8 years ago

we should add double quotes to surround the esc_url() call

We can add them, but I don't think we should. First, the surrounding quotes are totally optional (see https://www.w3.org/TR/CSS21/syndata.html#uri for more information), and second, most of the usages in the core CSS don't include the quotes.

So we're perfectly fine without adding the quotes at all. But if you insist to add them, I think it's much better to add them to the URL manually instead of using a more complex function that unnecessarily does more things and checks on the string.

I'm not totally sure that esc_url() will work for URLs that have characters that get encoded as entities.

You're right about this one, but having ampersands in these URLs is a super edge case (at this time it can only be achieved by hacking into the media upload functionality), and I think esc_url() is our best bet in that case. We can always use esc_url_raw(), but it's not as safe when displaying the URL (and that's what we're essentially doing).

#7 @westonruter
8 years ago

Well, is this not actually a problem with Autoptimize? CSS strings are defined as including escaped characters, also according to https://www.w3.org/TR/CSS21/syndata.html#uri

I haven't seen any reports of the browsers complaining about the escaped slashes, so it seems the problem is more on the side of plugin.

Also, it's not necessarily true that getting a background image URL with ampersands requires hacking media upload. In fact, all that is needed is a filter on theme_mod_background_image to supply a URL with ampersands or other “exotic” characters. I can imagine a real world use case for this too, for example an advertising plugin that overrides the background to serve up some wallpaper ad.

One problem I do see with using wp_json_encode(): JS and CSS don't use the same mechanism for escaping unicode characters. For example, a smiley is encoded in CSS as \1F601 wheres in JSON it is multi-byte \ud83d\ude01. We don't have a CSS string literal encoder available to us.

The main concern I have is to prevent the background image URL from “breaking out” of its url() definition in CSS. So if the URL contains a ) this would have that problem. If the string is double-quoted, then a ) won't break out. Also, the reason why json_encode() escapes the forward slashes is to prevent a string like something</script> (or something</style> in our case) from causing the script or style tag to terminate and open up an injection vulnerability opportunity. However, it looks like esc_url_raw() will get us what we need here:

  • Double-quote get stripped: esc_url_raw( 'http://example.com/evil.png?"very-evil' ) => http://example.com/evil.png?very-evil
  • Backslashes get stripped: esc_url_raw( 'http://example.com/evil.png\\)very evil' ) => http://example.com/evil.png)very%20evil

So I think that we'd be safe doing:

<?php
$image = " background-image: url(" . esc_url_raw( $background ) . ");"; 

The one caveat here is that the return value of esc_url_raw() gets passed through the clean_url filter and so ultimately we don't know what the returned value will be, unfortunately.

#8 @futtta
8 years ago

Well, is this not actually a problem with Autoptimize?

Can't say for sure, but there seems to be at least one non-Autoptimize user who landed on the support forum topic looking for a solution for this very same problem (see https://wordpress.org/support/topic/incompatibility-with-wp-4-7-aggregate-inline-css/#post-8529348 and follow-up posts). So this might just have more then just an impact on AO.

CSS strings are defined as including escaped characters, also according to https://www.w3.org/TR/CSS21/syndata.html#uri

True, but this is only explicitly stated for specific characters;

Some characters appearing in an unquoted URI, such as parentheses, white space characters, single quotes (') and double quotes ("), must be escaped with a backslash so that the resulting URI value is a URI token: '\(', '\)'.

I don't exactly see them advocating escaping slashes ;-)

#9 @westonruter
8 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 39546:

Customize: Use esc_url_raw() instead of wp_json_encode() to eliminate extraneous slashes when outputting background image URL in CSS url().

Props tyxla, westonruter.
See #22058.
Fixes #39145.

#10 @westonruter
8 years ago

  • Keywords fixed-major commit added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for 4.7.1.

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


8 years ago

#12 @dd32
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 39568:

Customize: Use esc_url_raw() instead of wp_json_encode() to eliminate extraneous slashes when outputting background image URL in CSS url().

Props tyxla, westonruter.
See #22058.
Merges [39546] to the 4.7 branch.
Fixes #39145.

Note: See TracTickets for help on using tickets.