Password Policy Implementation#1611
Conversation
|
I know @Zach-O-Bates analyzed this package but I wanted to leave a few notes from my looking. Positive is it has > 1M downloads a week and it seems like a decent package for this. Having said that, it does not seem to be maintained even given that not many changes might be needed. The last change was in 2017 and issues/pull requests are not being processed, including ones with security issues. I also saw that they used English to build the model. That is okay but may mean it isn't as good in other languages and OED is internationalized for usage around the world. So, I think it is fine to use this package, esp. compared to doing nothing, but this and any thoughts by others may help with future reconsideration. Update: I did not try to find a lot of updates but I did find zxcvbn-ts that appears to be currently active. It isn't used as much but something to consider along with other ones that are found. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Zach-O-Bates Thank you for looking into this. I've made a number of comments. I tried to do a good review but given the possible changes that may be made, other thoughts may come at a later time. I also only did minimal testing at this point. Please let me know if anything is not clear, you have thoughts/questions or I can help.
| @@ -0,0 +1,66 @@ | |||
| const { chai, mocha, app } = require('../common'); | |||
There was a problem hiding this comment.
This and the other new file are missing the copyright notice.
| @@ -0,0 +1,33 @@ | |||
| const zxcvbn = require('zxcvbn'); | |||
There was a problem hiding this comment.
I think it might be good to put a TODO linking to https://github.com/dropbox/zxcvbn#script-load-latency. This isn't huge (400k) but it isn't trivial either. OED hopes to make a future effort to reduce the bundle size and these ideas might help. It might also be good to note that at this time it isn't used on the client.
| @@ -0,0 +1,33 @@ | |||
| const zxcvbn = require('zxcvbn'); | |||
|
|
|||
| //Temproarly list, until we decide a better approach like a specific file | |||
There was a problem hiding this comment.
spelling: Temproarly. Also, I think this should be a TODO as an action is needed. It also might be nice to say what a good list would contain.
|
|
||
| //Temproarly list, until we decide a better approach like a specific file | ||
| const COMMON_PASSWORDS = new Set([ | ||
| 'password', |
There was a problem hiding this comment.
Space intending instead of tab. Same in other file.
| '12345678', | ||
| ]); | ||
|
|
||
| function validatePasswordPolicy(password, username, role = []) { |
There was a problem hiding this comment.
JSDoc would be good.
I see where this is used in src/server/routes/users.js for /create. I'm wondering about all the other places where passwords are entered: Same route in /edit, src/server/services/user/createUser.js & src/server/services/user/editUser.js. Also, this could be used on the front-end to give feedback without contacting the server but that would mean putting this into the client bundle and this isn't done very often so I'm okay with not doing that now (or maybe ever).
|
|
||
| }); | ||
|
|
||
| const chaiHttp = require('chai-http'); |
There was a problem hiding this comment.
OED prefers all require/include at the top of a file.
|
|
||
| let adminToken; | ||
|
|
||
| mocha.before(async () => { |
| const res = await chai.request(app) | ||
| .post('/api/login') | ||
| .send({ | ||
| username: 'test@example.invalid', |
There was a problem hiding this comment.
This really should use testUser to get the info as in other tests.
| adminToken = res.body.token; | ||
| }); | ||
|
|
||
| mocha.it('Rejects weak password on create', async () => { |
There was a problem hiding this comment.
These will need to be expanded if other routes are used during password testing per another comment.
| bcrypt from silently truncating the password before hashing */ | ||
| const byteLength = Buffer.byteLength(password, 'utf8'); | ||
| if (byteLength > 72) { | ||
| return res.status(400).send({ message: 'Password must not exceed 72 bytes.' }); |
There was a problem hiding this comment.
All the numeric values are being replaced by constants. Thus, this should happen when PR 1581 is merged.
I also think the message could be more user friendly. Maybe "Password must not exceed 72 bytes which often means 72 characters."
|
I also forgot to add this: How will current site passwords be handled that do not meet the new standards? Do they need to be reentered or do they just stay until updated? If forcing to reenter then how should that be done? Any thoughts? |
2 participants
Description
This pull request resolves issues with password security and inconsistent validation behavior. A uniform password policy is now enforced across user creation and authentication, including minimum length requirements (8 characters for standard users and 14 for administrators), prevention of weak or common passwords using a strength estimator, and disallowing passwords that contain the username. Additionally, this PR addresses bcrypt’s 72-byte limitation by rejecting passwords that exceed this length using a UTF-8 byte check, preventing silent truncation before hashing. Tests were updated to validate both the password policy and truncation protection, ensuring these controls are consistently enforced.
Fixes #1612
Type of change
Checklist
Limitations
N/A