Admin validation errors on form nonce element IDs (_wpnonce)

[bpetty]

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.

[alexvorn2]

maybe introduce wp_nonce_field2() for backwards compatibility?

[ocean90]

#23273 was marked as a duplicate.

[WraithKenny]

approach c is very interesting.

[WraithKenny]

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. :)

[bpetty]

Refreshed approach C patch with latest trunk (r24432).

[bpetty]

Related: #24063

[jdgrimes]

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

[info2grow]

This issue is still occurring 3 years later in WP 4.9.2 with WooCommerce 3.2.6 when logged out at checkout. Any updates?

[thomas_hoadley]

Still have issues with this. Updates needed.

[aeboi80]

I am encountering this as well. It appears there has not been an official resolution to this in many years. Has any further thought be given to this in the past 6 months?

WP: 4.9.6

WooCommerce: 3.4

[aduth]

It appears that latest versions of Chrome Canary will now emit warnings in the DevTools console when there are multiple elements on the page sharing the same ID.

[DOM] Found 2 elements with non-unique id #_wpnonce: (More info: ​https://goo.gl/9p2vKq)

It's still a "harmless HTML validation error", but has the potential to become quite noisy if this logging lands in a stable release of Chrome.

[aduth]

As an update to my previous comment, the duplicate ID console warnings appear to have landed in a stable version of Chrome. As of WordPress 5.3.2 in Chrome 79, navigating to the block editor (Posts > Add New) will display a console warning about non-unique IDs.

As noted in previous comments, this could be resolved by using unique names for the two fields using the default $name on the editor screen:

​https://github.com/WordPress/wordpress-develop/blob/2fad299/src/wp-admin/includes/post.php#L2262
​https://github.com/WordPress/wordpress-develop/blob/2fad299/src/wp-admin/includes/post.php#L2380

However, at least for the update-post nonce, this can have rippling side-effects, because the '_wpnonce' field name has hard-coded references elsewhere:

​https://github.com/WordPress/wordpress-develop/blob/2fad299/src/wp-admin/includes/post.php#L1934

At least in this case, it might be enough to update the other 'toggle-custom-fields' nonce field to avoid using the default name, but I was also curious to explore a solution at a framework level.

As mentioned in the original comment, the original application of an ID to this element is unfortunate, and now that it exists, there would be backwards-compatibility concerns in removing it. However, we might still be able to work within this constraint by generating unique IDs for all but the first use of nonce name.

The above patch attachment:23165-incrementing-id.diff​ takes this approach.

Considering default use of the function in using a $name value of '_wpnonce', this guarantees:

• The input name will always be '_wpnonce'
• Existing use relying on an ID of '_wpnonce' will continue to work as expected, since the ID will still be used for the first occurrence of the field in a page
• Any reuse of the name will assign a unique ID by tracking an incrementing count (per name) in global scope.

As far as implementation, a ​static variable scoped to the function could be used instead of a global variable. However, for the purpose of resetting this value between test cases, I admit to not being aware of an option to reset the internal static value.

[bobbingwide]

resetting this value between test cases, I admit to not being aware of an option to reset the internal static value.

I'd use a function like this:

function bw_gmap_map( $inc=true ) {
	static $map=0;
	if ( $inc ) {
		$map++;
	} elseif ( null === $inc ) {
		$map = 0;
	}
	return $map;
}

when a test wants to reset the value it calls the function passing a value of null

[SergeyBiryukov]

#51530 was marked as a duplicate.

[ocean90]

In 50255:

Block Editor: Use a unique name for the nonce of the custom fields toggle form.

Avoids a browser warning for having two elements with a non-unique id #_wpnonce on the post edit screen.

See #23165.
Fixes #51483.
Props vandestouwe, Mista-Flo.

[a4jp.com]

I'm getting the same error. Anyway to fix it?