Make WordPress Core

Opened 6 weeks ago

Last modified 2 weeks ago

#61366 new feature request

A password change should not destroy a user's session data.

Reported by: snicco's profile snicco Owned by:
Milestone: 6.7 Priority: normal
Severity: minor Version: 6.5.3
Component: Login and Registration Keywords: has-patch
Focuses: Cc:

Description

WordPress issues a new brand-new auth cookie, when a user changes their password on their profile page. This is because the password hash is part of the cookie, so to not invalidate the cookie, historically a new cookie was issued.

This means that the user also gets a new session token (the old one is kept dangling around) and any associated data of that session is "lost".

Instead, only a new cookie hash should be generated and the session token should be maintained.

This issue can be reproduced with the following code:


<?php
add_action('init', function () {
    
    $action = $_GET['session-test'] ?? null;
    
    if ($action === null || ! is_user_logged_in()){
        return;
    }
    
    $session_manager = WP_Session_Tokens::get_instance(get_current_user_id());
    $session = $session_manager->get(wp_get_session_token());
    
    if ($action === 'incr') {
        $session['counter'] = ($session['counter'] ?? 0) + 1;
        $session_manager->update(wp_get_session_token(), $session);
        
        wp_redirect('/?session-test=show');
    }
    
    echo 'Counter: ' . ($session['counter'] ?? 0);
    // Link to increment the counter
    echo '<br><a href="/?session-test=incr">Increment</a>';
    die();
});

Log in, then go to ?session-test=show, and increment the counter a couple times.

Then change your password.

The counter is now back at zero.

The responsible code is this in wp_update_user:

<?php
// Update the cookies if the password changed.
        $current_user = wp_get_current_user();
        if ( $current_user->ID == $user_id ) {
                if ( isset( $plaintext_pass ) ) {
                        wp_clear_auth_cookie();

                        /*
                         * Here we calculate the expiration length of the current auth cookie and compare it to the default expiration.
                         * If it's greater than this, then we know the user checked 'Remember Me' when they logged in.
                         */
                        $logged_in_cookie = wp_parse_auth_cookie( '', 'logged_in' );
                        /** This filter is documented in wp-includes/pluggable.php */
                        $default_cookie_life = apply_filters( 'auth_cookie_expiration', ( 2 * DAY_IN_SECONDS ), $user_id, false );
                        $remember            = false;
                        if ( false !== $logged_in_cookie && ( $logged_in_cookie['expiration'] - time() ) > $default_cookie_life ) {
                                $remember = true;
                        }

                        wp_set_auth_cookie( $user_id, $remember );
                }
        }

The last line should be changed to:

<?php
wp_set_auth_cookie( $user_id, $remember, '', $logged_in_cookie['token'] );

Change History (7)

#1 @lkraav
5 weeks ago

Hm, nuking old sessions seems prudent for security purposes?

If I changed my password, I think I'd want any other logged in sessions to get logged out?

#2 @ramon fincken
4 weeks ago

  • Severity changed from normal to minor
  • Type changed from defect (bug) to feature request

I am with @lkraav on this one.

#3 @snicco
4 weeks ago

Other sessions are always invalidated because the users password hash is part of the cookie. This has nothing to do with this ticket.

The issue described here is that the active(!) session of the user looses all data stored in that session.

Instead, just the session token / cookie should be changed and the data is maintained.

Did you try the example with the counter?

#4 @snicco
4 weeks ago

The current way is actually the worse of both:

  1. All stored data in the session is lost (see counter example)
  2. The old session is not removed from the database at all, just the cookie in the users browser changes.

#5 @ramon fincken
4 weeks ago

ah! Now it is clear to me

#6 @rajinsharwar
2 weeks ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.7

This ticket was mentioned in PR #6950 on WordPress/wordpress-develop by @narenin.


2 weeks ago
#7

  • Keywords has-patch added; needs-patch removed
Note: See TracTickets for help on using tickets.