Enforce wp_login guard for two-factor auth#793
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| add_filter( 'authenticate', array( __CLASS__, 'filter_authenticate' ), 31 ); | ||
|
|
||
| // 2. Render two-factor UI after WP core has validated username/password during `wp_signon()`. | ||
| add_action( 'wp_login', array( __CLASS__, 'wp_login' ), PHP_INT_MAX, 2 ); |
There was a problem hiding this comment.
In #660 this registration was moved to filter_authenticate (where we also disable login cookies for first-step password auth). Let's register the wp_login early on in case the login flow doesn't fire authenticate for some reason.
There was a problem hiding this comment.
Pull request overview
This pull request is a follow-up to #660 that ensures the wp_login hook is always registered upfront, rather than conditionally during authentication. This change improves security by guaranteeing that two-factor authentication cannot be bypassed through non-standard authentication flows.
Changes:
- Move
wp_loginandfilter_authenticatehook registrations to the top ofadd_hooks()method for unconditional registration - Simplify
filter_authenticate()function signature to accept only the user parameter - Remove conditional username check and dynamic wp_login hook registration logic
- Add documentation improvements and fix trailing whitespace throughout the file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| public static function filter_authenticate( $user, $username, $password ) { | ||
| if ( strlen( $username ) && $user instanceof WP_User && self::is_user_using_two_factor( $user->ID ) ) { | ||
| public static function filter_authenticate( $user ) { |
There was a problem hiding this comment.
The function signature has been changed to accept only one parameter, but the WordPress authenticate filter passes three parameters (user, username, password). According to WordPress core documentation, the authenticate filter signature is:
apply_filters( 'authenticate', null|WP_User|WP_Error $user, string $username, string $password )
While PHP will not error when extra parameters are passed to a function, this signature change means:
- The function is not documenting the actual filter signature it's registered with
- The function cannot access username/password parameters if needed in the future
- It may confuse developers reading the code
Although the username check (strlen($username)) was removed intentionally (as the logic now always registers the wp_login hook), the function signature should still match the filter it's registered with for clarity and future maintainability.
There was a problem hiding this comment.
@copilot WP filters only pass as many arguments are registered by each filter -- in this case we're accepting default number which is 1.
| add_filter( 'wpmu_users_columns', array( __CLASS__, 'filter_manage_users_columns' ) ); | ||
| add_filter( 'manage_users_custom_column', array( __CLASS__, 'manage_users_custom_column' ), 10, 3 ); | ||
|
|
||
| // 1. Prevent WP core from sending login cookies after username/password authentication (priority 30). |
There was a problem hiding this comment.
The comment states "after username/password authentication (priority 30)" but this is slightly misleading. The authenticate filter at priority 31 runs after ALL WordPress core authentication methods including:
- username/password authentication (priority 20)
- cookie authentication (priority 30)
The comment should be clarified to indicate it runs "after WP core authentication methods (including cookie auth at priority 30)" to avoid confusion about when this filter executes.
| // 1. Prevent WP core from sending login cookies after username/password authentication (priority 30). | |
| // 1. Prevent WP core from sending login cookies after WP core authentication methods (including cookie auth at priority 30). |
There was a problem hiding this comment.
@copilot Cookies are only sent only after the username/password auth, right? Regular cookie checks don't send new cookies unless they are soon to expire, no?
1 participant
What?
wp_loginhook to render the two-factor UI, instead of registering it only during theauthenticatefilter where we disable login cookies (equally critical feature).Why?
A follow-up to #660.
How?
Testing Instructions
Screenshots or screencast
Changelog Entry