#2849 - Generate random password for users #2866
Conversation
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
sebin/task/#2849-random-generated-password
|
| Run status |
|
| Run duration | 14m 12s |
| Commit |
|
| Committer | Sebin Song |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
116
|
| View all changes introduced in this branch ↗︎ | |
…ndom-generated-password
| generateRandomPassword (pwLen = 32) { | ||
| let genPassword = '' | ||
| if (window?.Cypress) { |
There was a problem hiding this comment.
I'd use process.env.CI here. If this code makes it to production and someone is using Cypress or some extension that defines Cypress, we wouldn't want to accidentally use a weak password.
Also, not sure why window?, since it should always be defined.
There was a problem hiding this comment.
will remove ? typo here. Correct me if I'm wrong but process.env.CI is only true when tested as part of GH action workflow?
| } | ||
| }, | ||
| methods: { | ||
| generateRandomPassword (pwLen = 32) { |
There was a problem hiding this comment.
Based on the crypto primitives we use for encryption, I think 128 bits (i.e., 32 chars long passwords, assuming hex) is a reasonable value.
| const bytes = new Uint8Array(Math.ceil(pwLen / 2)) | ||
| crypto.getRandomValues(bytes) | ||
| genPassword = Array.from(bytes).map(b => b.toString(36) // [0-9a-z] => 36 characters |
There was a problem hiding this comment.
This choice of alphabet together with how it's done seemed like it'd be prone to bias. After looking at it more closely, it looks fine (as in, the output is biased towards certain characters but I don't think it impacts the entropy in the output).
There was a problem hiding this comment.
Coming back to this point: could you consider maybe using base64 or base64url, or even the multiformats library for base58? This way we'd use the larger alphabet more efficiently and have shorter passwords (*) (this is using a 36 character alphabet but still requiring 2 bytes per encoded byte, i.e., about the same efficiency as just using hex)
(*) About 23 characters. Or alternatively, keep the 32-character passwords with more entropy in them, although the extra entropy probably is not needed.
There was a problem hiding this comment.
@corrideat Updated the PR again to use base58btc from multiformats library. I don't have much knowledge regarding this encode/decode stuff so it was based on my google/chatGPT search. pls write/leave a suggested change for generateBase58Password if any.
…ndom-generated-password
There was a problem hiding this comment.
Looks good. A few general comments summarising my other comments:
- I'd use an env variable rather than
window.Cypressto avoid accidentally weak passwords - Security-wise it looks fine, but I'd reconsider the choice of alphabet (or encoding) to attain shorter passwords, or to pack more entropy into the existing password length.
|
Thanks @corrideat for reviewing this multiple times! |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
PR is updated again. |
closes #2849
Below is the updated sign-up form:
@taoeffect Currently the random-generated password is always 32 characters long (32 here is just an arbitrary number I chose), but let me know if you have other opinions re this behavior.