WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 25 hours ago

#23165 new enhancement

Admin validation errors on form nonce element IDs (_wpnonce)

Reported by: bpetty Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Security Keywords: needs-codex has-patch needs-refresh
Focuses: administration Cc:

Description

The wp_nonce_field() method has a design flaw in that the name parameter is optional, but its default value ("_wpnonce") is used for the field element id. This wouldn't be bad if only one form is used on each page, but that is rarely the case in the admin panel. Currently, this only results in harmless HTML validation errors in admin, and in very rare situations (depending on plugins installed), can result in javascript scope-related issues breaking forms. This is an issue that keeps being reported and fixed several times (see #5962, #6564, r14737 from #13383, and the latest #22712) by simply specifying the name in all locations that don't at the time in core. There's at least three different approaches that I can see where we could prevent this from popping back up in the future and encourage better coding habits.

A: Require the Name Parameter

The most obvious approach is to simply adds a _doing_it_wrong() call (though it could just be _deprecated_argument() instead) for any calls to wp_nonce_field() that don't specify the name parameter, forcing everyone to give nonce fields unique IDs (and names). A side-effect of this change is that it will also warn developers they are doing it wrong if they use wp_nonce_field() without the (also currently optional) action parameter, which is actually a good thing since the docs already warn that this is a security hole.

There are 46 locations in core that would need updated with this change that make this mistake, and the fix for each of them with this change is exactly the same as what every other commit to fix this problem has always done: just specify the name to give a unique ID.

The downside to this approach is that this pushes for an immediate change not only to all calls to wp_nonce_field(), but also their corresponding check_admin_referer() calls (to update the changed field name) in a single WP release, and considering how frequently it's called this way in core, I don't even have to look at plugins to see how often it happens there (it's going to be too many to count).

This approach still results in an explicitly named ID that really doesn't need to be on the field in the first place in most locations.

I gave this approach a shot anyway, and started on a patch just to see how much work it would be at least in core, but I started to find a few locations that could benefit from unique IDs, but similar field names. I also found myself already spending hours updating and testing just 9 of the 46 locations (unit tests don't cover this stuff) where I was forced to make changes to IDs that would be best left alone for now. I have attached this unfinished patch anyway if anyone is curious to see how it works, what it does, and to see what kinds of changes it ends up requiring.

It's still possible to just pass "_wpnonce" as the name with this change, so the rest of the locations could simply use that to progressively work towards unique IDs, but I just don't see this as a good solution overall, and it seems like a lot of work for very little gain on a minor issue.

B: Use Action for ID or Remove ID Entirely

Another option might be to remove ID entirely, or use (and require) the action parameter as the ID. This would improve results with unique IDs fixing most locations where wp_nonce_field() is used without requiring a change to each one individually for most uses (except for nonces used with ajax calls). All locations in core at least use action already, so the case for using action as ID instead seems better than using name.

Unfortunately, either removing or using the action parameter for ID (changing the ID) would obviously break a few plugins that made the mistake of relying on the default _wpnonce (or the current name value) value for id from within JavaScript code for existing form endpoints. In the WP.org plugin repo, this appears to happen in the following plugins (searching up through plugins starting with 'r') for forms using the default value:

automatic-youtube-video-posts
buddypress
buddypress-backwards-compatibility
canalblog-importer
community-submitted-news
eyes-only-plus
facepress-ii
globalfeed
lazyest-gallery
lazyest-maps
maven-member
pagely-reseller-management
pluginbuddy-yourls
post-event2
qtranslate-separate-comments
repostus
...

There's likely way more plugins that post to core forms using a non-default name that would break though as well, and even though I think ID should have never been added to the nonce field attributes and was never necessary in the first place, compatibility rules would need to be broken if this approach was used. This of course means this option is most likely not really an option at all.

C: Use an Option Array Parameter in Place of Name

To keep this concise, this last approach basically involves changing wp_nonce_field() to:

function wp_nonce_field( $options = array() ) { ... }

The options parameter is a standard arguments array used with wp_parse_args(). This accomplishes not just one, but two different goals. First, if $options is not an array or the method was called with anything other than 1 parameter, we know this is an older call that needs to use the name parameter for both name and id, using func_get_args() for the original arguments, and can do so for full 100% backwards compatibility. Second, unrelated to this issue, the function has been refactored to adhere to coding standards in regards to self-explanatory flag values as far as the referrer and echo options are concerned.

The options array can then be made to accept the following settings:

  • action: The same unique name included in the nonce hash as is the case now.
  • name: The field name (and *only* used for the name attribute). (Default: "_wpnonce")
  • id: The field ID if this really is still desired, and can even be different than the name, but by default, the field won't have an ID at all.
  • referrer: Whether to set the referer field for validation. (Default: true)
  • echo: Whether to display or return hidden form field. (Default: true)

The only downside here is that each wp_nonce_field() call will be more verbose in most cases, especially if they do need to use an ID. For example, this:

wp_nonce_field('add-tag', '_wpnonce_add-tag');

Will now become this:

wp_nonce_field( array( 'action' => 'add-tag', 'name' => '_wpnonce_add-tag' ) );

I see plenty of benefits to this last solution though, and it requires very few changes up front, so I think it comes down to this approach ultimately. Please see attached patch.

Attachments (3)

23165.approach-c.patch (63.5 KB) - added by bpetty 2 years ago.
23165.approach-a-unfinished.patch (19.7 KB) - added by bpetty 2 years ago.
23165-c-r24432.patch (63.2 KB) - added by bpetty 2 years ago.

Download all attachments as: .zip

Change History (13)

@bpetty2 years ago

comment:1 @alexvorn22 years ago

  • Type changed from defect (bug) to enhancement

maybe introduce wp_nonce_field2() for backwards compatibility?

comment:2 @ocean902 years ago

#23273 was marked as a duplicate.

comment:3 @WraithKenny2 years ago

  • Cc Ken@… added

approach c is very interesting.

comment:4 @WraithKenny2 years ago

For Approach A patch it looks as tho you are creating unique names for all of core's nonces. Would it be sufficient for now just to add the _doing_it_wrong check, explicitly pass the name "_wpnonce" for the main core nonces (to avoid _doing_it_wrong), and add unique names only to any secondary nonce that don't have them (if any)? It'd at least cut down on the check_admin_referer edits in the patch, and be easier to test (mostly unchanged output). (It's late, forgive me if I'm just missing something. :)

@bpetty2 years ago

comment:5 @bpetty2 years ago

Refreshed approach C patch with latest trunk (r24432).

comment:6 @nacin16 months ago

  • Component changed from Validation to Administration

comment:7 @ocean9016 months ago

  • Component changed from Administration to Security
  • Focuses administration added

comment:9 @jdgrimes7 months ago

I think approach C is the best solution here. The patch could be updated to use hash notation for the inline docs, though.

comment:10 @MikeHansenMe25 hours ago

  • Keywords needs-refresh added
Note: See TracTickets for help on using tickets.