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

Incorrect code comment on users-permission register controller. #7481

Closed
psikoi opened this issue Aug 14, 2020 · 6 comments
Closed

Incorrect code comment on users-permission register controller. #7481

psikoi opened this issue Aug 14, 2020 · 6 comments
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature severity: low If the issue only affects a very niche base of users and an easily implemented workaround can solve

Comments

@psikoi
Copy link

psikoi commented Aug 14, 2020

Describe the bug
Nothing major, this is probably a typo or an outdated code comment:

    // Throw an error if the password selected by the user
    // contains more than two times the symbol '$'.
    if (strapi.plugins['users-permissions'].services.user.isHashed(params.password)) {
      return ctx.badRequest(
        null,
        formatError({
          id: 'Auth.form.error.password.format',
          message: 'Your password cannot contain more than three times the symbol `$`.',
        })
      );
    }

Link to snippet:

// Throw an error if the password selected by the user

As you can see, the comment says two times, while the error message AND the isHashed function say three is the correct amount.

This seems to still be in master code as of this post.

@lauriejim lauriejim added severity: low If the issue only affects a very niche base of users and an easily implemented workaround can solve issue: enhancement Issue suggesting an enhancement to an existing feature labels Aug 15, 2020
@lauriejim
Copy link
Contributor

Thank you, can you please submit a PR to update this comment.
Thank you.

@psikoi
Copy link
Author

psikoi commented Aug 15, 2020

Will do.

@Cr0s4k Cr0s4k mentioned this issue Sep 3, 2020
@derrickmehaffy
Copy link
Member

Fixed in above PR

@borm
Copy link

borm commented Jan 8, 2021

@derrickmehaffy, can u explain please why user cant have more than two times the symbol '$' in his password?
any security risks?

@derrickmehaffy
Copy link
Member

I'm not aware of that limitation, I would offer a guess to the cause as being the regex we use to validate the password. Can you open a new bug report @borm

@jscottsf
Copy link

jscottsf commented Sep 9, 2021

It's being use to detect whether the password has been bcrypted. hashPassword calls isHashed which does the check. Even worse, the validation is NOT done in resetPassword so a user could type in a password with enough dollar signs to "appear" hashed and inadvertently set their password to null.

Overall this feels wonky and inconsistent. Hmmm.

Decided to override the controller/service files and use a regex check for bcryptiness ^[$]2[abxy]?[$](?:0[4-9]|[12][0-9]|3[01])[$][./0-9a-zA-Z]{53}$. Also adding a validation to resetPassword. Hope this helps someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature severity: low If the issue only affects a very niche base of users and an easily implemented workaround can solve
Projects
None yet
Development

No branches or pull requests

5 participants