WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 3 months ago

#4221 closed enhancement (wontfix)

Allow pure wp_nonce_url urls

Reported by: filosofo Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.3
Component: Security Keywords:
Focuses: Cc:

Description

wp_nonce_url sends the url through wp_specialchars, which converts & into &

That's great for standards-compliant links, but if you're trying, for example, to use a nonce in a url passed to JavaScript, it can cause problems, as at least Firefox chokes when JavaScript requests a url with & instead of &.

My patch allows you to toggle off the wp_specialchars; the default behavior remains the same.

Attachments (3)

wp-includes_functions.php.diff (829 bytes) - added by filosofo 7 years ago.
patch.diff (791 bytes) - added by filosofo 7 years ago.
4221.docs.patch (608 bytes) - added by c3mdigital 8 months ago.
Change @return statement to document returned url is escaped

Download all attachments as: .zip

Change History (14)

filosofo7 years ago

comment:1 filosofo7 years ago

The patches are the same, but for the second I removed the new line stuff that messes up Trac's rendering.

comment:2 vladimir_kolesnikov5 years ago

  • Cc vladimir@… added
  • Keywords has-patch added

comment:3 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 2.9 to Future Release
  • Type changed from defect (bug) to enhancement
DB:~/Sites/wp $ wptest http://core.trac.wordpress.org/attachment/ticket/4221/patch.diff
patching file wp-includes/functions.php
Hunk #1 FAILED at 1023.
patch: **** malformed patch at line 19:  

DB:~/Sites/wp $ wptest http://core.trac.wordpress.org/attachment/ticket/4221/wp-includes_functions.php.diff
patching file wp-includes/functions.php
Hunk #1 FAILED at 1023.
Hunk #2 FAILED at 1516.
2 out of 2 hunks FAILED -- saving rejects to file wp-includes/functions.php.rej

comment:4 follow-up: crashutah4 years ago

I'm having this same issue. Any reason why this patch wasn't ever addressed?

My issue happens when I try to create a nonce URL on a redirect to a page that includes a bunch of vars. However, after the redirect, the URL has & instead of just & and it interprets amp; as part of the variable name (which is obviously a problem).

Seems like the code for this function has changed since this patch so that it now does a str_replace( '&', '&', $actionurl ) instead of the call this does, but it still causes the same problem.

comment:5 in reply to: ↑ 4 solarissmoke3 years ago

  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from new to closed

This was fixed in #4884.

Replying to crashutah:

My issue happens when I try to create a nonce URL on a redirect to a page that includes a bunch of vars. However, after the redirect, the URL has & instead of just & and it interprets amp; as part of the variable name (which is obviously a problem).

This is a separate issue: wp_nonce_url() assumes the URL is going to be output (this is how it is used everywhere in the core), hence everything is HTML-entitied. I suggest using add_query_arg() to add the nonce manually if you want to use it raw.

comment:6 nacin3 years ago

  • Keywords close added
  • Resolution fixed deleted
  • Status changed from closed to reopened

No, wp_nonce_url() still runs esc_html() over the output. Re-opening, but suggesting wontfix at this point. You can always create the nonce yourself and pass that through JS, which is something we often do.

comment:7 solarissmoke3 years ago

Maybe worth making it clear in the inline docs that it returns an escaped string?

comment:8 charliespider21 months ago

I realize this is old, but I am experiencing the same thing as crashutah, where combining wp_nonce_url with wp_redirect results in & being converted to &

Then, after the redirect, the & gets split with amp; getting prepended to your GET variable.

For example:

$query_args = array( 'action' => 'action_value');
$admin_url = admin_url( 'admin.php?page=myadminpage' );
$redirect_url = wp_nonce_url( add_query_arg( $query_args, $admin_url ), 'my_nonce' ); 
wp_redirect( $redirect_url );   
exit();

results in :

printr_r( $_REQUEST );
Array
(
    [page] => myadminpage
    [amp;action] => action_value
    [amp;_wpnonce] => c0306b37d5
)

Don't know if this deserves to be in a separate ticket.

c3mdigital8 months ago

Change @return statement to document returned url is escaped

comment:9 follow-up: c3mdigital8 months ago

  • Keywords has-patch added; close removed

Ticket is a wontfix but it should be mentioned in docs that the returned url is escaped. See 4221.docs.patch

comment:10 DrewAPicture8 months ago

  • Keywords commit added

comment:11 in reply to: ↑ 9 ocean903 months ago

  • Component changed from Administration to Security
  • Keywords has-patch commit removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Replying to c3mdigital:

Ticket is a wontfix but it should be mentioned in docs that the returned url is escaped. See 4221.docs.patch

Done in [27070].

Note: See TracTickets for help on using tickets.