WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 months ago

#13051 closed defect (bug) (wontfix)

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

Reported by: alexkingorg 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)

patch.diff (74.0 KB) - added by alexkingorg 6 years ago.
removal of many esc_url calls and addition of that feature to the _url() functions
patch2.diff (1.1 KB) - added by alexkingorg 6 years ago.
Unescape encoded ampersands in wp_sanitize_redirect

Download all attachments as: .zip

Change History (31)

#1 @scribu
6 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

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

#2 @alexkingorg
6 years ago

Can you elaborate with an example?

#3 @scribu
6 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 @scribu
6 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 @alexkingorg
6 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 @alexkingorg
6 years ago

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

@alexkingorg
6 years ago

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

#7 @scribu
6 years ago

  • Keywords has-patch added

#8 @ryan
6 years ago

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

#9 @nacin
6 years ago

First bug in the wild? #13223

#10 @demetris
6 years ago

  • Cc dkikizas@… added

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

#11 @scribu
6 years ago

  • Keywords needs-patch added; has-patch 2nd-opinion removed
  • Severity changed from normal to critical

#12 @westi
6 years ago

  • Keywords needs-testing added; needs-patch removed
  • Severity changed from critical to normal

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

#13 follow-up: @demetris
6 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 @westi
6 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 @westi
6 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 @nacin
6 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: @alexkingorg
6 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

#18 @ryan
6 years ago

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

#19 @ryan
6 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.

#20 @markmcwilliams
6 years ago

  • Milestone changed from 3.0 to 3.1

#21 @nacin
6 years ago

  • Keywords early added; needs-testing removed

#22 in reply to: ↑ 17 @westi
6 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 @alexkingorg
6 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.

  1. Attached is a patch to do the replace in wp_sanitize_redirect and call wp_sanitize_redirect in wp_nonce_url
  1. 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.

@alexkingorg
6 years ago

Unescape encoded ampersands in wp_sanitize_redirect

#24 @alexkingorg
6 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.

#25 @scribu
6 years ago

Related: #14556

#26 @nacin
5 years ago

  • Keywords 2nd-opinion added; early removed
  • Milestone changed from Awaiting Triage to Future Release

#27 @jdgrimes
2 years ago

  • Cc jdg@… added

#28 @ryan
19 months ago

  • Owner ryan deleted
  • Status changed from new to assigned

#29 @johnbillion
5 months 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.

Note: See TracTickets for help on using tickets.