Skip to content

Improve password validation#2

Open
Rosuav wants to merge 2 commits into
Thinkful-Ed:masterfrom
Rosuav:fix-password
Open

Improve password validation#2
Rosuav wants to merge 2 commits into
Thinkful-Ed:masterfrom
Rosuav:fix-password

Conversation

@Rosuav

@Rosuav Rosuav commented Sep 10, 2019

Copy link
Copy Markdown

Couple of simple fixes to password validation.

Comment thread src/user/user-service.js
const bcrypt = require('bcryptjs')

const REGEX_UPPER_LOWER_NUMBER_SPECIAL = /(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[!@#\$%\^&])[\S]+/
const REGEX_UPPER_LOWER_NUMBER_SPECIAL = /((?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[-_!@#\$%\^&])[\S]+)|.{11,}/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The |.{11,} will make passwords like 11111111111 and aaaaaaaaaaa pass. Despite 11 characters being quite long, strings of the same character are still in lookup tables.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if we're adding special characters, a bit of love for the British and a £ would be nice ;D

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will ALWAYS be weak passwords that get through the validation. Using the same string as both username and password is weak. The purpose of this regex is to provide some simple checks, ideally without excluding good passwords; the current regex permits password#1 which I think you'd agree is not exactly strong, but it's not a regex's job to see whether your password includes any dictionary words.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got no problem with adding more characters. Maybe we can actually define things by ranges instead?

@tomatau tomatau Sep 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, it's not about getting perfectly safe passwords, it's about checking what it can conveniently. Enforcing things like using a wider ranger of characters than simply lower case letters will help. The |.{11} kind of undermines the rest of the checks. If we only want 11 characters or more then we should skip the regex and stick to length checks of between 11 and 72. The 11 character check isn't tested and isn't given as user feedback either.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raw entropy isn't the only factor for password strength. Literally aaaaaaaaaaa passing is really not good. Having less code means you have tens of thousands less entries on a lookup table that will pass.

(Also testing this stuff so someone doesn't change it to |{8,0} and it slip through.)

Regarding feedback, just because another site does something doesn't mean that it's good UX. A message like "Password is not complex enough" isn't giving guidance to users. The message can be:

- 'Password must contain one upper case, lower case, number and special character'
+ 'Password must contain one upper case, lower case, number and special character or be 11 characters or more'

And then a user knows how to make it "complex enough".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe be really flexible and say "any character other than a letter or number"??? [^A-Za-z0-9]

LGTM 👍

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that message works. There's a balancing act between listing every possible way to be "complex enough" and keeping the message simple. People who actually know how to create good passwords will tend to just try them before seeing the failure message (a field that just says "Password: [ ]" doesn't say how complex it needs to be, so if your first try is accepted, you never see the message).

If someone crafts a stupid password that still gets through the regex, congrats. There's no regex that can prevent https://xkcd.com/792/ anyway. Just keep it simple.

Honestly, I would actually be fine with eliminating the "password complexity" rules in favour of just saying "your password must be at least 12 characters long" and that's that. It'd probably work out better for security AND it's way way simpler.

@tomatau tomatau Sep 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd probably work out better for security

They don't. Try it for yourself both Pa$$w0rd and aaAA11!! (8 characters with mixed number, case and symbol) are a higher entropy than 111111111111 (12 characters).

Yes I know there are many articles on Ycombinator and an XKDC from years back explaining why most longer passwords will always be harder to crack by brute force, but this is ignoring dictionary attacks.

Anyway I'm happy to go with this change, my only request is for it to be reflected in tests and UX.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what does that strength test actually MEAN? If you're going to go with that site, then it's happy with aaaaaaaaaaaaaaa (15 characters), so I don't think there's a lot to be learned there other than that maybe 11 isn't enough to satisfy all tools. But mainly, it's a fairly arbitrary calculation. For instance, it says that mine-site-hours-noticed has 100 bits of entropy, but I would say it has roughly 40-45 bits of real entropy - it's four words selected individually from a list of 1024 (that's 10 bits per word), plus a few additional bits because you can't be sure exactly what's on my list, plus passwords of this nature might follow a few different forms. Maybe 50 bits at the absolute most. It also says that Tr0ub4dor&3 has 51 bits of entropy but the whole point of XKCD 936 is that standard transformations on a word add very little actual entropy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants