Make WordPress Core

Opened 9 months ago

Last modified 2 months ago

#58901 assigned enhancement

Flush 'user_activation_key' after successfully login

Reported by: nsinelnikov's profile nsinelnikov Owned by: rajinsharwar's profile rajinsharwar
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch has-unit-tests needs-testing
Focuses: Cc:

Description

Hi all,

Let's imagine the next steps:

  1. User goes to {site_url}/wp-login.php?action=lostpassword for getting reset password link to its email.
  1. Then go to email and open the reset password link with an expiration time (DAY_IN_SECONDS by default). It has been resolved a long time ago. But then he remembers his old password and login using a second web browser with its username and old password. At the same time, the link to reset the password remains active in the first browser for a whole day.
  1. If it's a public laptop anybody can use the reset password link and login with new credentials and make some hacker things.

Suggestions:

Flush the 'user_activation_key' after successful login:

wp-includes/user.php::line 113 before

do_action( 'wp_login', $user->user_login, $user );

Can be added this line:

global $wpdb;
$wpdb->update(
    $wpdb->users,
    array(
	'user_activation_key' => '',
    ),
    array( 'ID' => $user->ID )
);

Best Regards!

Attachments (1)

58901.diff (2.0 KB) - added by Rahmohn 7 months ago.
Update to clear the user_activation_key in the user object and add a test

Download all attachments as: .zip

Change History (16)

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


9 months ago
#1

  • Keywords has-patch added

Flushing 'user_activation_key' after successful login

Trac ticket: https://core.trac.wordpress.org/ticket/58901

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


9 months ago
#2

Flushing 'user_activation_key' after successful login

Trac ticket: https://core.trac.wordpress.org/ticket/58901

@nsinelnikov commented on PR #4899:


9 months ago
#3

The proper PR is here

#4 @rajinsharwar
8 months ago

Looks great to me. Placing this for 6.4

Last edited 8 months ago by rajinsharwar (previous) (diff)

#5 @rajinsharwar
8 months ago

  • Milestone changed from Awaiting Review to 6.4
  • Version 6.3 deleted

#6 @rajinsharwar
8 months ago

  • Keywords needs-unit-tests added
  • Owner set to rajinsharwar
  • Status changed from new to assigned

I believe we will be needing unit tests for this.

@Rahmohn
7 months ago

Update to clear the user_activation_key in the user object and add a test

#7 @Rahmohn
7 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Hi @nsinelnikov

I've added a patch that updates to clear the user_activation_key property from the user object and adds a test.

#8 follow-up: @oglekler
7 months ago

  • Keywords needs-testing added

I wonder if clean_user_cache( $user_id ) is a better way to clear the cache.

#9 in reply to: ↑ 8 @Rahmohn
7 months ago

Replying to oglekler:

I wonder if clean_user_cache( $user_id ) is a better way to clear the cache.

Hi @oglekler

If I'm not wrong, `clean_user_cache( $user_id ) just removes the cache for the user ID, login, nicename, email and meta but the link to reset the password will still be valid. That way, cleaning the user cache after a successful login doesn't seem to help in this case.

#10 @oglekler
7 months ago

  • Keywords needs-refresh added

I was suggesting use clean_user_cache( $user_id ); instead of $user->__set( 'user_activation_key', '' );

I've tested the last patch, it works as expected, but I was unable to actually apply the patch, tests need a rebase.

Last edited 7 months ago by oglekler (previous) (diff)

This ticket was mentioned in Slack in #core by oglekler. View the logs.


7 months ago

#12 @hellofromTonya
7 months ago

  • Milestone changed from 6.4 to 6.5

In 58901.diff, directly invoking the __set() magic method is not preferred. And 'user_activation_key' is a dynamic property (i.e. undeclared property of WP_User) that is incompatible with PHP 8.2 and beyond.

I think this needs more consideration of how to reset the cached activation key. More test reports before and after would also be helpful.

Tomorrow being Beta 1. Moving it to 6.5 to give the ticket more time.

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


3 months ago
#13

  • Keywords needs-refresh removed

Clear the activation key if exists in the DB during login.

Trac ticket: https://core.trac.wordpress.org/ticket/58901

#14 @rajinsharwar
3 months ago

So, here, I am just flushing the activation_key using the $wpdb update, just the way we are doing in case of a successful password reset. Also, I don't think we need to clear any sort of user cache, because, we are just logging the user in using the old password, and removing the activation key, we are not changing anything in the User Data.

Also, I added an initial check if $user->user_activation_key is empty or not, and then ran that update statement, to avoid unnecessary running of query in the case where activation_key will be already empty (In most cases)

Feel free to share any suggestions and any test reports! Let's get this committed before the Beta 1!

#15 @swissspidy
2 months ago

  • Milestone changed from 6.5 to 6.6
Note: See TracTickets for help on using tickets.