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

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

Open
5 tasks done
teppokoivula opened this issue Oct 5, 2022 · 7 comments
Open
5 tasks done
Labels

Comments

@teppokoivula
Copy link

Terms

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

  1. 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 );
		},
	]);
});
  1. Call aforementioned REST API route
  2. 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
Copy link
Member

QWp6t commented Oct 5, 2022

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
Copy link
Member

swalkinshaw commented Oct 6, 2022

Looking at that function, there's an early return for API requests that would prevent setting the password:

if (
! class_exists('WP_Application_Passwords') ||
empty($passwords = WP_Application_Passwords::get_user_application_passwords($user_id))
) {
return;
}

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
Copy link
Author

teppokoivula commented Oct 9, 2022

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 #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
Copy link

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
Copy link

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
Copy link

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
Copy link

Tsjippy commented Dec 6, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants