[wp-trac] [WordPress Trac] #23165: Admin validation errors on form nonce element IDs (_wpnonce)

[WordPress Trac]

#23165: Admin validation errors on form nonce element IDs (_wpnonce)
-----------------------------------+--------------------------
 Reporter:  bpetty                 |       Type:  defect (bug)
   Status:  new                    |   Priority:  normal
Milestone:  Awaiting Review        |  Component:  Validation
  Version:                         |   Severity:  normal
 Keywords:  needs-codex has-patch  |
-----------------------------------+--------------------------
 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 [http://core.trac.wordpress.org/ticket/22712#comment:11
 #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
 [http://codex.wordpress.org/WordPress_Coding_Standards#Self-
 Explanatory_Flag_Values_for_Function_Arguments 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.

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/23165>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software