WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 months ago

#13051 new defect (bug)

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

Reported by: alexkingorg Owned by: ryan
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Security Keywords: needs-patch 2nd-opinion
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 4 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 4 years ago.
Unescape encoded ampersands in wp_sanitize_redirect

Download all attachments as: .zip

Change History (29)

comment:1 scribu4 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().

comment:2 alexkingorg4 years ago

Can you elaborate with an example?

comment:3 scribu4 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.

comment:4 scribu4 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

comment:5 alexkingorg4 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.

comment:6 alexkingorg4 years ago

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

alexkingorg4 years ago

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

comment:7 scribu4 years ago

  • Keywords has-patch added

comment:8 ryan4 years ago

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

comment:9 nacin4 years ago

First bug in the wild? #13223

comment:10 demetris4 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

comment:11 scribu4 years ago

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

comment:12 westi4 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.

comment:13 follow-up: demetris4 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

comment:14 in reply to: ↑ 13 westi4 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.

comment:15 westi4 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.

comment:16 nacin4 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.

comment:17 follow-up: alexkingorg4 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

comment:18 ryan4 years ago

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

comment:19 ryan4 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.

comment:20 markmcwilliams4 years ago

  • Milestone changed from 3.0 to 3.1

comment:21 nacin4 years ago

  • Keywords early added; needs-testing removed

comment:22 in reply to: ↑ 17 westi4 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

comment:23 alexkingorg4 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.

alexkingorg4 years ago

Unescape encoded ampersands in wp_sanitize_redirect

comment:24 alexkingorg4 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.

comment:25 scribu4 years ago

Related: #14556

comment:26 nacin3 years ago

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

comment:27 jdgrimes4 months ago

  • Cc jdg@… added
Note: See TracTickets for help on using tickets.