Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clearing auth cookies when visiting wp-login.php while being already logged-in #659

Open
kasparsd opened this issue Jan 10, 2025 · 2 comments · May be fixed by #660
Open

Clearing auth cookies when visiting wp-login.php while being already logged-in #659

kasparsd opened this issue Jan 10, 2025 · 2 comments · May be fixed by #660
Milestone

Comments

@kasparsd
Copy link
Collaborator

Describe the bug

When a user who is logged-in (and has two-factor configured) visits the wp-login.php page, their authentication state is cleared. The clearing should happen only during the two-factor login workflow.

By default, WP core redirects users to the dashboard if they visit wp-login.php while being already logged in. This is not happening and prevents other plugins that rely on the logged-in state on the wp-login.php page from functioning.

This happens because the function attached to wp_login is running the logic even outside of the login flow:

public static function wp_login( $user_login, $user ) {
if ( ! self::is_user_using_two_factor( $user->ID ) ) {
return;
}
// Invalidate the current login session to prevent from being re-used.
self::destroy_current_session_for_user( $user );
// Also clear the cookies which are no longer valid.
wp_clear_auth_cookie();
self::show_two_factor_login( $user );
exit;

Steps to Reproduce

  1. Login at a site.
  2. Configure and enable any two-factor method.
  3. Visit /wp-login.php and notice that a login form is shown or just the two-factor prompt.

Screenshots, screen recording, code snippet

Image

Environment information

  • Two-factor 0.11.0
  • Default theme 2025
  • No other plugins

Please confirm that you have searched existing issues in this repository.

Yes

Please confirm that you have tested with all plugins deactivated except Two-Factor.

Yes

@kasparsd
Copy link
Collaborator Author

This is also somewhat related to #592.

There is no reason to fire the wp_login callback on every request -- rather, it should get applied only during the two-factor login flow. Ideally, we would apply the filter from another function that is known to fire during the two-factor login process.

We already run a similar logic for clearing the auth cookies:

public static function filter_authenticate_block_cookies( $user ) {
/*
* NOTE: The `login_init` action is checked for here to ensure we're within the regular login flow,
* rather than through an unsupported 3rd-party login process which this plugin doesn't support.
*/
if ( $user instanceof WP_User && self::is_user_using_two_factor( $user->ID ) && did_action( 'login_init' ) ) {
add_filter( 'send_auth_cookies', '__return_false', PHP_INT_MAX );
}
return $user;
}

so we could move the add_action( 'wp_login', ... registration to there as well.

@kasparsd kasparsd linked a pull request Jan 10, 2025 that will close this issue
@kasparsd
Copy link
Collaborator Author

kasparsd commented Jan 10, 2025

Here is a patch for anyone interested in applying this until a permanent fix is released:

// Disable two-factor authentication for valid user sessions without a login attempt (lack of username input).
add_filter(
	'authenticate',
	function ( $user, $username ) {
		if ( $user instanceof WP_User && ! strlen( $username ) && class_exists( Two_Factor_Core::class ) ) {
			remove_action( 'wp_login', [ Two_Factor_Core::class, 'wp_login' ], 10, 2 );
			remove_action( 'authenticate', [ Two_Factor_Core::class, 'filter_authenticate_block_cookies' ], PHP_INT_MAX );
		}

		return $user;
	},
	35,
	2
);

@jeffpaul jeffpaul added this to the 0.12.0 milestone Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants