#20771 closed enhancement (wontfix)
esc_url() instead of esc_html() in wp_nonce_url()
Reported by: | jkudish | Owned by: | johnbillion |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 3.4 |
Component: | Formatting | Keywords: | needs-unit-tests needs-patch |
Focuses: | Cc: |
Description
The wp_nonce_url()
function currently uses esc_html()
in its output, which doesn't really seem to be the appropriate escaping function since it's generating a URL.
Attached patch changes the output to use esc_url()
Attachments (2)
Change History (26)
#1
follow-up:
↓ 2
@
12 years ago
- Keywords 3.5-early added
- Milestone changed from Awaiting Review to Future Release
#2
in reply to:
↑ 1
@
12 years ago
Replying to SergeyBiryukov:
In come cases,
wp_nonce_url()
result is already escaped withesc_url()
on output:
http://core.trac.wordpress.org/browser/tags/3.3.2/wp-admin/includes/class-wp-ms-sites-list-table.php#L249
We should probably review all the instances.
We could remove all the uses of esc_url( wp_nonce_url( ... ) )
, but there isn't anything technically wrong with escaping twice. It's being overly cautious for sure, but not "wrong".
That being said, the revised attached patch removes all such occurrences.
This got me thinking about something though... Is there a good reason why other functions that generate URLs (e.g. admin_url()
, includes_url()
, etc...) don't use esc_url()
in their output?
#4
@
12 years ago
- Keywords 3.6-early added; 3.5-early removed
This is probably too late for 3.5, due to implications that this may have. This should however be considered for early 3.6.
#7
@
12 years ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 23411:
#8
@
12 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
This change has introduced an encoding bug, albeit an indirect one.
esc_html()
encodes ampersands as &
but esc_url()
encodes ampersands as &
. Passing a URL through add_query_arg()
will mangle the URL if it contains ampersands encoded as &
.
My User Switching plugin has broken since this change because the plugin uses add_query_arg()
on a URL that's already been passed through wp_nonce_url()
.
Example:
$url = add_query_arg( 'action', 'foo', wp_login_url() ); $url = wp_nonce_url( $url, 'foo' ); $url = add_query_arg( 'redirect_to', 'bar', $url );
The resulting URL will end up as:
http://example.com/wp-login.php?action=foo&redirect_to=bar#038;_wpnonce=abc123
instead of:
http://example.com/wp-login.php?action=foo&_wpnonce=abc123&redirect_to=bar"
Notice that the _wpnonce
parameter is stripped and treated as a URL hash instead of a query arg.
I'd like to recommend for now that r23411 is reverted. However, the core cause of the problem is due to the fact that wp_nonce_url()
should be sanitising the URL but not encoding it. This means it should use esc_url_raw()
instead of esc_url()
or esc_html()
.
I'm going to open another ticket later today to address the core issue as it's present in several places but generally goes unnoticed.
#9
@
12 years ago
We should ideally fix add_query_arg() to work for both & and &. At a glance, though, I'm not sure I see where add_query_arg() handles the former even right now.
We should also consider just using & in esc_url(). I can't think of a particular context where esc_url() may be used (and esc_html() isn't) where & is not a recognized entity.
For now, I agree with revert. I have considered this change more than once, and each time, avoided it as something would break.
#16
@
9 years ago
- Keywords needs-unit-tests needs-patch added; has-patch dev-feedback removed
- Milestone changed from Future Release to 4.4
- Owner changed from SergeyBiryukov to johnbillion
- Status changed from reopened to accepted
#19
@
9 years ago
Created #36397 to deal with the bug in add_query_args
which causes numbered html entities to be mistaken for hash fragments.
#20
@
7 years ago
I think
esc_url
doing HTML escaping is a bit dumb
I agree with this statement – I just ran into this again working on a plugin.
Core is surprisingly consistent as to how it wraps wp_nonce_url()
in esc_url()
; there are only a handful of places where the &
replacement in wp_nonce_url()
is actually necessary.
users.php#L161 is an old-school @nacin top 40 classic for the ages, according to the Casey Kasem revision history.
Otherwise, as dumb as it looks, it's working OK.
wp_nonce_url()
does seem, to me, like it's a function that doesn't know if it should echo
or return
, and so it escapes but doesn't output anything, which I don't like (at least use _get_
in the name.)
I think if anyone runs into this in the future and wants direction, my preferred approach would be to:
- Introduce a new function that doesn't escape, doesn't swap the
&
, and handles only the adding of the query nonce to a URL - Use this new function all across everything where
wp_nonce_url()
is currently used - Replace query-string URL patterns with arrays using
add_query_arg()
– this is alot of code - Somehow convince everyone this is actually worth doing compared to leaving it be
TL;DR - I think this can be closed either as wontfix or maybelater.
This ticket was mentioned in Slack in #core by jjj. View the logs.
7 years ago
#23
@
5 years ago
- Milestone Future Release deleted
- Resolution set to wontfix
- Status changed from accepted to closed
#24
@
4 years ago
Why not adding a new param in this function?
wp_nonce_url( $actionurl, $action = -1, $name = '_wpnonce', $context = 'display' );
When $context is 'display', the default value (hello retrocompat), we let the esc_html() since it's for displaying.
But it's not for display, like "redirect", esc_url() instead.
And if no context is given, no sanitize.
I think that way everyone is happy, we can still use it, retrocompat ok but still with a new possibility to use it for redirection without creating a new one or break anything.
Thougths?
cc @johnbillion @johnjamesjacoby
wp_specialchars()
was added in [3974] and changed toesc_html()
in [11380].In come cases,
wp_nonce_url()
result is already escaped withesc_url()
on output:http://core.trac.wordpress.org/browser/tags/3.3.2/wp-admin/includes/class-wp-ms-sites-list-table.php#L249
We should probably review all the instances.