Opened 15 years ago
Closed 9 years ago
#13051 closed defect (bug) (wontfix)
admin_url() and site_url() shouldn't need esc_url()
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 3.0 |
Component: | Security | Keywords: | needs-patch close |
Focuses: | Cc: |
Description
I noticed that the 3.0 codeline includes the addition of esc_url() around admin_url() like:
esc_url(admin_url());
I believe that admin_url() and site_url() should be "safe" functions to use and should not need escaping. Perhaps they should call esc_url() internally?
I cannot think of a viable reason to allow unsafe results from admin_url() and site_url(), though perhaps there are some internationalization edge cases that I'm not aware of.
If you really need raw access to an unsafe value in wp_options, you can use get_option() to get to it.
Another issue to consider here is input validation and stripping before saving to these fields.
If this is approved in principle, I'd be happy to produce a diff against the current code base.
I think this is very important to address before 3.0 is released as it has a significant impact on theme and plugin developers.
Attachments (2)
Change History (31)
#1
@
15 years ago
- Component changed from Administration to Formatting
- Priority changed from high to normal
- Severity changed from major to normal
- Type changed from defect (bug) to enhancement
#3
@
15 years ago
What about this combination, used throughout the admin area:
wp_nonce_url(admin_url('...');
Note that wp_nonce_url() already calls esc_html() internally.
#4
@
15 years ago
- Keywords 2nd-opinion added
Also, behaviour should be consistent among all these functions:
site_url, home_url, includes_url, admin_url, plugins_url
network_site_url, network_home_url, network_admin_url
#5
@
15 years ago
- Component changed from Formatting to Security
- Owner set to ryan
- Type changed from enhancement to defect (bug)
I don't see how passing a sanitized URL to the wp_nonce_url function hurts anything.
The issue I'm trying to raise here is that the results of the built in *_url() functions should be safe to use in attributes without additional escaping.
Every plugin and theme I can think of offhand already treats the functions this way, and the WP admin code did as well prior to 3.0. Rather than requiring all plugins and themes to add additional wrapper functions, I think that the wrapper functions added in wp-admin in 3.0 should be removed and the output of the *_url() functions should be made safe to use without them.
#6
@
15 years ago
Big patch attached, adds $esc_url parameter (defaults to true) to most of the _url() functions
#11
@
15 years ago
- Keywords needs-patch added; has-patch 2nd-opinion removed
- Severity changed from normal to critical
#12
@
15 years ago
- Keywords needs-testing added; needs-patch removed
- Severity changed from critical to normal
#13
follow-up:
↓ 14
@
15 years ago
- Keywords needs-patch added
Another one caused by r14347:
In plugin-install.php?tab=search, when I click Install Now I’m redirected to the “Are you sure you want to do this?” screen
#14
in reply to:
↑ 13
@
15 years ago
Replying to demetris:
Another one caused by r14347:
In plugin-install.php?tab=search, when I click Install Now I’m redirected to the “Are you sure you want to do this?” screen
Linked to #13224 I suspect.
install_plugin_install_status()
has a lot of calls to admin_url with url arguments which this change breaks.
#15
@
15 years ago
this is breaking a lot of what were quite normal usages of admin_url and will break a lot of plugins maybe we need to reconsider it.
#16
@
15 years ago
While discussing this yesterday, one potential avenue we did not end up pursuing on first pass was having sanitize_redirect unescape ampersands. Maybe we can consider that as well.
#17
follow-up:
↓ 22
@
15 years ago
Something seems odd here. In the example above:
http://example.com/wp-admin/edit.php#038;action=edit
That should be a question mark (?), not an ampersand. However the entity (#038;) is for an ampersand.
When we reviewed the redirects, we saw that none of them actually used ampersands, though a dozen or so used question marks.
Why would this:
wp_redirect( admin_url( 'edit.php?action=edit' ) );
result in a URL like this:
http://example.com/wp-admin/edit.php#038;action=edit
or, with the entity unencoded:
http://example.com/wp-admin/edit.php&action=edit
#19
@
15 years ago
I wanted this to work, but it is breaking too many things. Let's try to remedy this for good in 3.0. Sorry Alex.
#22
in reply to:
↑ 17
@
15 years ago
Replying to alexkingorg:
Something seems odd here. In the example above:
http://example.com/wp-admin/edit.php#038;action=edit
That should be a question mark (?), not an ampersand. However the entity (#038;) is for an ampersand.
When we reviewed the redirects, we saw that none of them actually used ampersands, though a dozen or so used question marks.
Why would this:
wp_redirect( admin_url( 'edit.php?action=edit' ) );
result in a URL like this:
http://example.com/wp-admin/edit.php#038;action=edit
or, with the entity unencoded:
http://example.com/wp-admin/edit.php&action=edit
The url has lost a bit more in the processing too.
The actual stuff passed to admin_url is more like:
edit.php?post_type=post&id=1&action=edit
#23
@
15 years ago
Ah, so then we just need to convert & and &038; to & in the redirect call.
I see a couple of things happening here.
- Attached is a patch to do the replace in
wp_sanitize_redirect
and callwp_sanitize_redirect
inwp_nonce_url
- I see that the str_replace was already added to wp_nonce_url, but it was added before a query arg was added, and it wasn't checking for the #038; version of the encoded & to replace
I think we can get rid of that str_replace and use wp_sanitize_redirect instead. In the patch I left it in, commented out.
With this patch in place as well as the previous patch I can install plugins, etc. without nonce errors.
#24
@
15 years ago
There may be some places that don't call wp_sanitize_redirect()
and could still have issues, but it seems to me that adding the sanitization call in those places is the right approach.
#26
@
14 years ago
- Keywords 2nd-opinion added; early removed
- Milestone changed from Awaiting Triage to Future Release
#29
@
9 years ago
- Keywords close added; 2nd-opinion removed
- Milestone Future Release deleted
- Resolution set to wontfix
- Status changed from assigned to closed
This is definitely a wontfix at this point.
esc_url()
is used to escape URLs so they are safe for outputting into HTML. Applying esc_url()
to functions such as site_url()
and admin_url()
means that everything not in the context of HTML will get a URL with encoded ampersands, which is not desirable at best, and breaks things at worst.
Consider this:
wp_redirect( admin_url( 'tools.php?page=foo' ) );
This is a common pattern not only in plugins but in core, too. Adding esc_url()
to admin_url()
breaks this piece of code.
Even if a $context
parameter was added to site_url()
, admin_url()
etc in order to control the context passed to esc_url()
(which is a terrible design pattern in itself), the context would have to default to something other than display
for backwards compatibility, which mostly negates the point of adding esc_url()
to these functions in the first place.
-1, since esc_url() has multiple contexts.
For example, plugins could use admin_url() to generate links that are meant to be stored in the db later, which would be escaped with esc_url_raw().