admin_url() and site_url() shouldn't need esc_url()

[alexkingorg]

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.

-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().

Can you elaborate with an example?

What about this combination, used throughout the admin area:

wp_nonce_url(admin_url('...');

Note that wp_nonce_url() already calls esc_html() internally.

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

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.

Big patch attached, adds $esc_url parameter (defaults to true) to most of the _url() functions

removal of many esc_url calls and addition of that feature to the _url() functions

(In [14347]) Escape links by default. Props alexkingorg. see #13051

First bug in the wild? #13223

I am seeing the same bug (#13223), which appears only after r14347.

On saving, I am redirected to:

http://example.com/wp-admin/edit.php#038;action=edit

(in [14372]) Fix the edit post link to cope with the change in behaviour of admin_url to always escape the url. See #13051.

That should fix the edit link regression.

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

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.

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.

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.

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

(In [14374]) Revert [14347] and [14372]. It broke more than we expected. Try again in 3.1. see #13051

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.

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

Ah, so then we just need to convert & and &038; to & in the redirect call.

I see a couple of things happening here.

1. Attached is a patch to do the replace in wp_sanitize_redirect and call wp_sanitize_redirect in wp_nonce_url

2. 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.

Unescape encoded ampersands in wp_sanitize_redirect

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.

Related: #14556