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

[WIP] Fix: Update password should require re-authentication #309

Closed
wants to merge 3 commits into from

Conversation

kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented Dec 28, 2021

What kind of change does this PR introduce?

In case a user's authentication token is stolen, this fix prevents an attacker from updating the user's password and completely taking over the account. Password updates should now require the user to submit their current password for re-authentication.

What is the new behavior?

# PUT /user requires a new param current_password
# current_password is a compulsory field if password field is present
{
  "current_password": "current_password",
  "password": "new_password"
}
curl -X PUT "http://localhost:9999/user" -H "Authorization: Bearer access_token" -d '{"current_password": "current_password", "password": "new_password"}'

Password Update Rules

  1. Current password must be provided
  2. Current password provided must match password stored in db
  3. New password cannot be the same as the current password
  4. New password must be of a minimum length

Limitations

  • This is a BREAKING CHANGE so we should probably only merge it when we decide to release a major version (potentially with other breaking changes)
  • The JS and Dart libraries will need to be updated appropriately

Copy link
Member

@awalias awalias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but we'll need to put this check behind a new env var because of the breaking change. Which defaults to the old behavior, it will save us a huge headache rolling it out.

@awalias
Copy link
Member

awalias commented Jan 4, 2022

I thought of a scenario where the user may not know the "current" password, i.e. if they were a magic link signup originally, but now wish to set a password 🤔

Perhaps if REAUTHENTICATE_PASSWORD_UPDATES(?) is true then the the new password would have to be set via the admin route instead

@kangmingtay
Copy link
Member Author

I thought of a scenario where the user may not know the "current" password, i.e. if they were a magic link signup originally, but now wish to set a password 🤔

yeah good point, i discussed this issue with @dthyresson and we came up with an alternative implementation:

Instead of asking a user to provide their old password, we should send a confirm_password_update_otp to the user's email or phone number. We'll also change the existing update password endpoint to accept an OTP + new password which will be used as the "reauthentication" mechanism for a password update.

The rationale behind this is because gotrue allows a developer many options to sign-in (magiclinks, password-based, oauth) and asking for the old password doesn't really make sense for magiclinks & oauth, but we can be sure that there will at least be an email or phone number associated to the user.

@kangmingtay kangmingtay changed the title Fix: Update password should require re-authentication [WIP] Fix: Update password should require re-authentication Jan 16, 2022
@nodiesBlade
Copy link

Can we get this in? it's bizarre to me this was not incorporated when designing the user API's. If on a shared computer, or if a hacker gets a token credentials, the attack blast radius is much larger. They can literally change the user's password and get full access to the account

@kangmingtay
Copy link
Member Author

closing this in favour of #427

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants