Opened 12 years ago
Last modified 4 years 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 (4)
Change History (24)
#4
@
12 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. :)
#9
@
10 years ago
I think approach C is the best solution here. The patch could be updated to use hash notation for the inline docs, though.
#11
@
7 years ago
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?
#14
@
5 years ago
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.
#15
@
5 years ago
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.
This ticket was mentioned in Slack in #core-editor by get_dave. View the logs.
5 years ago
#17
@
5 years ago
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
maybe introduce wp_nonce_field2() for backwards compatibility?