Hi, thanks for this, it seems to be a good addition. Will have a quick look in the next few days then merge.

I've just tried to merge but there was a conflict on a file so I needed to checkout locally for resolving. However, it now appears that the tests do not work, it is probably linked to the fact the module has been updated to work with Feathers v4 in the meantime. You can check my changes on this branch https://github.com/feathers-plus/feathers-authentication-management/tree/verify-signup-set-password. Could you have a look ? Thanks

I've just tried to merge but there was a conflict on a file so I needed to checkout locally for resolving. However, it now appears that the tests do not work, it is probably linked to the fact the module has been updated to work with Feathers v4 in the meantime. You can check my changes on this branch https://github.com/feathers-plus/feathers-authentication-management/tree/verify-signup-set-password. Could you have a look ? Thanks

@claustres I merged the upstream changes and updated for compatibility. Let me know if you have any questions or issues.

Good work, thanks.

Hi @mattyg

I think I have found a big security issue.

Because of this line it is possible to set a password without having the right token.

This line await eraseVerifyPropsSetPassword(user1, user1.isVerified, {}, password, field); set the password and patch the user.
Then next line throw an error but the password is already set...
Then the user is still not verified but the password is set.
In my case i don’t check for isVerified when login so then the user can log with this newly created password and take over new unverified account.
So a call to POST /authManagement
with this body

{
    "action": "verifySignupSetPasswordShort",
    "value": {
        "user": {
            "username": "+1234567890"
        },
        "token": "123456",
        "password": "password12345"
    }
}

Will set the password even if token !== verifyShortToken.
Did i miss something or is it a bug ?
Thanks.

Hi @GautierT, thanks a lot for this important comment and the PR! I tested it locally in my app and have to verify this security issue...

1. Create a new user
2. Send a request as described above with a random token.
3. Password is changed to the one from the request and can be used for a login 😳 (At least if the app does to check for isVerified === true)

This is possible only once. If you try it again, the package returns Verification token has expired., because the token has been deleted.

Best solution for this issue seems to be your PR #167.

PS: Also, I do not see why the verification token should be deleted if someone tries a request with a wrong token. It should stay until a user is using the correct token or until it expires. Or do I miss something?

It's probably not a good idea to let an attacker have chances to guess the token by performing massive tries and errors before the token expire, notably for short tokens, which are subject to brute force attack.

Sounds reasonable, @claustres. That probably why the token is removed in the standard verify-signup.js, too (see this line).

@GautierT, it is probably a good idea to update your PR and use a function like eraseVerifyProps() (see verify-signup.js), where all tokens are removed but no password is stored. Do you agree?

@claustres : I understand but what prevent the attacker to call resendVerifySignup after each failed attempt and get a new token and try to brute-force it again ?

@OnnoGabriel I will try to improve it now.

It seems to me that if the token is changing each time you call resendVerifySignup the probability to get it right is always 1 over your search space (N), but if you are able to make multiple tries (M) on the same token the probability is higher because each time you try you actually reduce your search space as you already know what tokens will not work and won't need to be tested again. I am not a mathematician but it seems to me that after M tries you get M / N probability vs M / (N - M) probability, so as M gets larger the second one becomes higher faster than the first one.

Anyway, as discussed here (#69), a more secure option for sure is to add a rate-limit layer on top of this package.

ah yes of course.

multiple tries on the same token the probability is higher because each time you try you actually reduce your search space

Did not though about that !

I added eraseVerifyProps to my PR to reduce risk of brute-force.
It's a copy paste from verify-signup.js