WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#24063 closed enhancement (wontfix)

Introduce some more _doing_it_wrong() calls in nonce functions

Reported by: johnbillion Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.2
Component: Security Keywords: has-patch
Focuses: Cc:

Description

check_admin_referer() will tell you that you're _doing_it_wrong() if you don't specify the $action parameter. The corresponding wp_nonce_field() and wp_nonce_url() functions should behave the same way.

check_ajax_referer() should also behave the same as check_admin_referer() but I'm not sure if potentially raising notices in AJAX calls is a good idea. I've included it in the patch anyway.

Attachments (2)

24063.patch (1.6 KB) - added by johnbillion 12 months ago.
ticket-24063-req-nonce-action.2.patch (1.7 KB) - added by bpetty 8 weeks ago.

Download all attachments as: .zip

Change History (7)

johnbillion12 months ago

comment:1 johnbillion12 months ago

  • Keywords has-patch added

Patch

comment:2 bpetty8 weeks ago

Refreshed patch, still makes sense to me.

Related: #23165

comment:3 in reply to: ↑ description SergeyBiryukov8 weeks ago

Replying to johnbillion:

check_ajax_referer() should also behave the same as check_admin_referer() but I'm not sure if potentially raising notices in AJAX calls is a good idea.

FWIW, I don't think it's a good idea.

Also, wp_nonce_url() is used directly in href attributes, so the notice would result in a badly broken link:

<a href="<br />
<b>Notice</b>:  wp_nonce_url was called <strong>incorrectly</strong>. You should specify a nonce action as the second parameter. Please see <a href="http://codex.wordpress.org/Debugging_in_WordPress">Debugging in WordPress</a> for more information. (This message was added in version 3.9.) in <b>wp-includes/functions.php</b> on line <b>3069</b><br />
index.php?_wpnonce=dedea08d5f">Link text</a>

Which is displayed like this:

Debugging in WordPress for more information. (This message was added in version 3.9.) in wp-includes/functions.php on line 3069
index.php?_wpnonce=dedea08d5f">Link text

comment:4 johnbillion7 weeks ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I'll have to agree with Sergey here, displaying a notice inside an attribute would break the layout. We can't even just do it in wp_nonce_field() because its value can be returned instead of echoed and a notice may show up in an unwanted place.

This is a wontfix unless anyone has any better ideas.

comment:5 nacin7 weeks ago

The security team had at one point discussed making $action a non-optional parameter, like what we did with wpdb::prepare(). But it probably wouldn't go over well.

Note: See TracTickets for help on using tickets.