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

fix field name of reset password token, add verifier and set status #161

Closed
wants to merge 8 commits into from

Conversation

tomivm
Copy link
Collaborator

@tomivm tomivm commented Apr 15, 2021

this PR fixes a bug that allows changing the password of the users of Cboard only with their emails.
-Don't send the reset password token on /user/forgot response
-Use resBcrypt variable on bcrypt.compare() callback to verify if the token is correct.
-Set the status field of the document to true after changing the password. ( It's better to use this field or delete the document? )

After this merge is necesary modify test of user endpoint because the token is no more available on forgot response. PR #144

@tomivm tomivm added the bug label Apr 15, 2021
@tomivm tomivm linked an issue Apr 15, 2021 that may be closed by this pull request
Copy link
Collaborator

@sylvansson sylvansson left a comment

Choose a reason for hiding this comment

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

It would be great if you could add some tests directly in this PR to be sure that your changes work as intended. And to answer your question, we can keep the document in order to keep a record of the password reset.

Great work identifying this bug and fixing it 🙌

api/controllers/user.js Outdated Show resolved Hide resolved
api/controllers/user.js Outdated Show resolved Hide resolved
api/controllers/user.js Outdated Show resolved Hide resolved
api/controllers/user.js Outdated Show resolved Hide resolved
@sylvansson sylvansson force-pushed the 160-fix-store-password-vulnerability branch from 52ba887 to bef4fde Compare April 18, 2021 00:32
@sylvansson
Copy link
Collaborator

@tomivm I think I accidentally pushed to your branch and undid some of your changes. Sorry!

@sylvansson
Copy link
Collaborator

@tomivm I think I was able to restore your changes.

api/controllers/user.js Outdated Show resolved Hide resolved
api/controllers/user.js Outdated Show resolved Hide resolved
@sylvansson
Copy link
Collaborator

@tomivm It looks like this PR was closed automatically when I merged #166 because you referenced it with the "resolve" keyword in your other PR. I'm reopening it.

image

@sylvansson sylvansson reopened this Apr 25, 2021
@tomivm
Copy link
Collaborator Author

tomivm commented Apr 25, 2021

I can't write Mocha test for this endpoint yet because the Token now is coming only via Email. I did some manual test and register it with images.
image
with out UserId and Token:
image
without token:
image
Recived mail:
image
Request and response using the link to change password:
borrar
Changing expired time to 5 seconds and trying to change password:
image
With the actual code on forgot-password is only able to forgot one Time the password. what I mean is that if I change My password one Time i cannot request change the password again. when i tried it the response is: 'ERROR: create reset password FAILED '

@sylvansson sylvansson force-pushed the 160-fix-store-password-vulnerability branch from 0589087 to e71200c Compare April 29, 2021 02:27
@martinbedouret
Copy link
Collaborator

@tomivm I am closing this because it is too old. Could you please check it out and apply the same fix on a new branch in case this is still applicable?

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

Successfully merging this pull request may close these issues.

it's possible change a password without owning the account.
3 participants