Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51951 closed feature request (invalid)

nonce_user_logged_out filter not consistent with $user->exists()

Reported by: chamois_blanc's profile chamois_blanc 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?

Change History (2)

#1 @chamois_blanc
4 years ago

  • Resolution set to invalid
  • Status changed from new to closed

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'.

#2 @desrosj
4 years ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.