Bug: unexpected behavior when calling wp_set_password() via REST API

teppokoivula commented 1 year ago

Terms

[X] I have read the guidelines for Contributing to Roots Projects
[X] This request is not a duplicate of an existing issue
[X] I have read the docs and followed them (if applicable)
[X] I have seached the Roots Discourse for answers and followed them (if applicable)
[X] This is not a personal support request that should be posted on the Roots Discourse community

Description

What's wrong?

Trying to change user password within a custom REST API route by calling wp_set_password() doesn't seem to do anything at the moment.

What have you tried?

Among other things tried disabling wp-password-bcrypt, after which things started working again.

What insights have you gained?

If I disable "application_password_is_api_request" check by filter (add_filter('application_password_is_api_request', '__return_false')) before calling wp_set_password() in my REST route, changing password seems to work as expected.

Possible solutions

Not familiar enough with the implementation to make any real suggestions, except that if this can't be reliably fixed, then perhaps it should be documented?

Temporary workarounds

Calling add_filter('application_password_is_api_request', '__return_false') before wp_set_password() in REST route.

Steps To Reproduce

Create a REST API route and add call to wp_set_password('some-password-string', 1):

add_action( 'rest_api_init', function () {
    register_rest_route( 'my-namespace', '/pass-test', [
        'methods' => 'GET',
        'permission_callback' => '__return_true',
        'callback' => function( \WP_REST_Request $request ) {
            // add_filter( 'application_password_is_api_request', '__return_false' );
            wp_set_password( 'new-pass-here', 1 );
        },
    ]);
});

Call aforementioned REST API route
Try to log in with new password

Expected Behavior

New password should work.

Actual Behavior

New password doesn't work, but old password still works. If I uncomment the filter in the action hook above, things now work as expected.

Relevant Log Output

No response

Versions

1.1.0

QWp6t commented 1 year ago

I haven't dug into this yet, but I just wanted to say thanks for providing an informative bug report with along with workarounds that you've tried, research that you've done into this, and steps to reproduce. I appreciate that. 🙏

swalkinshaw commented 1 year ago

Looking at that function, there's an early return for API requests that would prevent setting the password: https://github.com/roots/wp-password-bcrypt/blob/9ad6c271f9c4f4575aaa03638fa0d2c9821aaf09/wp-password-bcrypt.php#L108-L113

I'm assuming WP_Application_Passwords exists (but you could verify that), which leaves the other condition as the likely culprit.

empty($passwords = WP_Application_Passwords::get_user_application_passwords($user_id))

So if there are no existing application passwords for the user, that function will just return without doing anything 🤔

Would you be able to debug and see which condition is causing the short-circuit? You could copy the plugin source and modify it.

teppokoivula commented 1 year ago

Just had a chance to dig in. First of all, the reason this method currently does nothing is indeed latter rule; there are no application passwords defined, so WP_Application_Passwords::get_user_application_passwords($user_id) returns an empty array.

Just to be extra clear: I'm not trying to set an application password, but rather change the user's "normal" password via REST API. (I hope that was obvious already, but one can never be too sure.)

Looking at https://github.com/roots/wp-password-bcrypt/pull/31 I have a rough idea why the method works the way it does, but it does also seem to prevent what I'm trying to do here, and differs in this regard from how the core version works. ~Just wondering if hooking into wp_update_application_password and wp_create_application_password could be an alternative approach with fewer side effects?~

Edit: Strike that, that wouldn't help with the problem mentioned in issue #22. But it doesn't seem quite right to assume that API requests are always about applications passwords either :)

calvinalkan commented 1 year ago

wp_set_password should never update application passwords. That's a straight violation of the implicit API of that function which leads to this bug.

Regarding application passwords in wp_check_password, you have to decide what to update based on what type of hash is checked.

If the stored hash (second Argument) matches the user's password hash (needs to be fetched using passed ID) the password is updated.

If any of the application passwords (need to be fetched) match the passed hash the application passwords are updated.

If none of the above is true nothing is updated.

calvinalkan commented 1 year ago

The are several plugins that call wp_check_password on strings that are in fact not the user's Login password.

Those will all reset the real password.

calvinalkan commented 1 year ago

To add to my comments,

I don't think bcrypt should be used for application passwords at all.

Application passwords are randomly generated and have roughly 140 bits of entropy. A "simple" hash function is more than enough.

Tsjippy commented 1 year ago

I have the same issue! Trying to reset the users password via rest api over AJAX, exits without any error because empty($passwords = WP_Application_Passwords::get_user_application_passwords($user_id)) is true.

At least an entry should be added to log in such case

roots / wp-password-bcrypt