WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 weeks ago

#20771 accepted enhancement

esc_url() instead of esc_html() in wp_nonce_url()

Reported by: jkudish Owned by: johnbillion
Milestone: Future Release 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)

20771-1.diff (518 bytes) - added by jkudish 4 years ago.
20771-2.diff (19.2 KB) - added by jkudish 4 years ago.
remove all occurrences of esc_url( wp_nonce_url( ... ) )

Download all attachments as: .zip

Change History (21)

@jkudish
4 years ago

#1 follow-up: @SergeyBiryukov
4 years ago

  • Keywords 3.5-early added
  • Milestone changed from Awaiting Review to Future Release

wp_specialchars() was added in [3974] and changed to esc_html() in [11380].

In come cases, wp_nonce_url() result is already escaped with esc_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.

#2 in reply to: ↑ 1 @jkudish
4 years ago

Replying to SergeyBiryukov:

In come cases, wp_nonce_url() result is already escaped with esc_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?

Last edited 4 years ago by jkudish (previous) (diff)

@jkudish
4 years ago

remove all occurrences of esc_url( wp_nonce_url( ... ) )

#3 @SergeyBiryukov
4 years ago

  • Keywords dev-feedback added

#4 @TobiasBg
3 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.

#5 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 3.6

#6 @SergeyBiryukov
3 years ago

  • Keywords commit added

20771-1.diff seems good.

Related: #23266

#7 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 23411:

Use correct escaping function. props jkudish. fixes #20771.

#8 @johnbillion
3 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.

Last edited 3 years ago by johnbillion (previous) (diff)

#9 @nacin
3 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.

#10 @nacin
3 years ago

In 23637:

Revert [23411] until encoding differences are worked out. see #20771.

#11 @ryan
3 years ago

  • Milestone changed from 3.6 to Future Release

#13 @chriscct7
11 months ago

  • Keywords 3.6-early commit removed

#14 @johnbillion
8 months ago

In 33851:

Add some more passing unit tests for esc_url() in preparation for upcoming changes.

See #23605, #20771, #16859

#15 @johnbillion
8 months ago

In 33855:

More unit tests for esc_url() and esc_url_raw().

See #23605, #20771, #16859

#16 @johnbillion
8 months 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

#17 @johnbillion
7 months ago

#31335 was marked as a duplicate.

#18 @johnbillion
7 months ago

  • Milestone changed from 4.4 to Future Release

#19 @omarreiss
4 weeks ago

Created #36397 to deal with the bug in add_query_args which causes numbered html entities to be mistaken for hash fragments.

Note: See TracTickets for help on using tickets.