#51951 closed feature request (invalid)
nonce_user_logged_out filter not consistent with $user->exists()
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Milestone: | Priority: | normal | |
| Severity: | minor | Version: | 5.6 |
| Component: | Security | Keywords: | 2nd-opinion |
| Focuses: | Cc: |
Description
The nonce_user_logged_out name seems to suggest that it takes effect when a user is logged out. But in the code, it only takes effect when the user ID is zero, which is not quite the same thing:
$user->ID != 0 is not equivalent to $user->exists()
Some plugins assign a user ID to distinguish non-logged in users. WooCommerce, for instance, assigns a user ID when an item is added to the cart.
I'm not sure how important the inconsistency is but I thought it was worth mentioning. On the flip side, I can see how calling $user->exists() may be slower and therefore undesirable.
It would be useful to add another filter on $uid in both wp_create_nonce and wp_verify_nonce, so that it can be changed if $user->exists() returns false (i.e. the user is not logged in). The code would look something like this:
$user = wp_get_current_user();
$uid = (int) $user->ID;
if ( ! $uid ) {
/**
* Filters whether the user who generated the nonce is logged out.
*
* @since 3.5.0
*
* @param int $uid ID of the nonce-owning user.
* @param string $action The nonce action.
*/
$uid = apply_filters( 'nonce_user_logged_out', $uid, $action );
}
/**
* Filters the user ID. [ADDED]
*
* @since ???
*
* @param int $uid ID of the nonce-owning user.
* @param string $action The nonce action.
*/
$uid = apply_filters( 'nonce_user_id', $uid, $action );
This would allow to set $uid to zero (or some other value) for non-logged in users and avoid issues with nonce in such cases. Another solution is obviously not to check nonce for non-logged in users in each ajax action function, but that is probably less safe.
Thoughts?
After some more research, I found out WooCommerce is using the nonce_user_logged_out filter to change the user ID. I think I can put my own filter on top to override it for my custom actions.
I think there is some truth to my observation, but probably not quite enough to make it worth adding a new filter. I will close this feature request as 'invalid'.