bcrypt: reject passwords > 72 bytes

[alxndrsn]

What has been done to verify that this works as intended?

Manual testing - there are no existing tests for short passwords.

Why is this the best possible solution? Were any other approaches considered?

I think a better approach would be to move max-length validation into lib/util/crypto.js.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Should not affect existing users with very long passwords, but will prevent new passwords being saved with >72 bytes.

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

No - the docs do not currently mention any restrictions on password length.

Before submitting this PR, please make sure you have:

• run make test and confirmed all checks still pass OR confirm CircleCI build passes
• verified that any code from external sources are properly credited in comments or that everything is internally sourced

[lognaturel]

Are you thinking this is likely rare enough that we just leave a raw backend message (vs. introducing localizable frontend error messages)?

[alxndrsn]

What's the distinction? Should there be an accompanying PR for odk-central-frontend?

[lognaturel]

I think a better approach would be to move max-length validation into lib/util/crypto.js.

But you're keeping it there for now because the short length validation is already there, right?

This looks good to me provided there are no surprises in the answers to the clarifying questions I've asked.

@ktuite @sadiqkhoja FYI in case you want to take another quick look.

[sadiqkhoja]

What prompts this change? The datatype of password column is varchar(64), isn't that sufficient?

[alxndrsn]

The datatype of password column is varchar(64), isn't that sufficient?

We're not storing passwords in plaintext 😁

Also is there a reason for that column to be VARCHAR when bcrypt hashes have a fixed length of 60 characters?

[alxndrsn]

But you're keeping it there for now because the short length validation is already there, right?

Yes. Maybe they should both be moved into the hashPassword() function? "Password too long" seems a more fundamental failure than "too short" though.

[sadiqkhoja]

Only thing I would like to add is the reason i.e. "bcrypt truncates input to 72 bytes before hashing" in the PR description or commit message for future reference.

[sentry-io]

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

• ‼️ Error: data must be a string or Buffer and salt must either be a salt string or a number of rounds POST /users View Issue

_{Did you find this useful? React with a 👍 or 👎}

[alxndrsn]

This pull request was deployed and Sentry observed the following issues:

This issue seems unrelated, but has been filed at #1407