Opened 8 years ago
Closed 8 years ago
#39145 closed defect (bug) (fixed)
custom-background URL escaped
Reported by: | futtta | Owned by: | 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)
Change History (13)
#1
@
8 years ago
- Focuses ui removed
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 4.7.1
#2
@
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.
#5
@
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
@
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
@
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
@
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
@
8 years ago
- Owner set to westonruter
- Resolution set to fixed
- Status changed from new to closed
In 39546:
#10
@
8 years ago
- Keywords fixed-major commit added
- Resolution fixed deleted
- Status changed from closed to reopened
Re-opening for 4.7.1.
Hello @futtta, welcome to WordPress Trac!
Thanks for the report, I can confirm this issue.