zxcvbn-ts / zxcvbn

Low-Budget Password Strength Estimation
https://www.usenix.org/conference/usenixsecurity16/technical-sessions/presentation/wheeler
MIT License
907 stars 72 forks source link

Performance issues #31

Closed henrywoody closed 3 years ago

henrywoody commented 3 years ago

I've set up an app with zxcvbn-ts to show password strength and offer suggestions to the user. Nothing special, the options are exactly as show in the docs. Also want to say zxcvbn is a great project and I appreciate the work you're doing to keep it going.

I've noticed, though, that zxcvbn-ts is not fast enough to run on each keypress (value update in React). When rendering the password strength component with zxcvbn-ts, there is noticeable lag while typing (especially when typing as fast as possible as a stress test). The base zxcvbn package, on the other hand, is able to calculate password strength on keypress without any noticeable delay.

Is there any way to improve performance when using this package?

I've seen this line in the README: "usable without dictionaries at all, which reduce the scoring efficiency rapidly." (I'm assuming it means reduce the scoring time or increase efficiency), however it's followed by "This is not recommended."

Happy to help out with this project too if you can point me in the direction of areas that might be affecting performance.

For reference here are my options:

const zxcvbnOptions = {
    dictionary: {
        ...zxcvbnCommonPackage.dictionary,
        ...zxcvbnEnPackage.dictionary,
    },
    translations: zxcvbnEnPackage.translations,
};
MrWook commented 3 years ago

Hey, thanks for the report i tried it on the demo page it was a little laggy for me too if i tried to type as fast as i can with random keys.

I guess it's because we always set those HUGE dictionaries every time we type something. I build an Options Singleton which handles the zxcvbn options. This means we could separate the options handling from the typing. This should be in the examples, i guess Basically what I mean is something like that, which you can already test by yourself:

import zxcvbn from '@zxcvbn-ts/core'
import ZxcvbnOptions from '@zxcvbn-ts/core/Options'
import zxcvbnCommonPackage from '@zxcvbn-ts/language-common'
import zxcvbnEnPackage from '@zxcvbn-ts/language-en'

// Set the options somewhere else for example on page load or after lazyloading the dictionaries 
ZxcvbnOptions.setOptions({
  dictionary: {
    ...zxcvbnCommonPackage.dictionary,
    ...zxcvbnEnPackage.dictionary,
  },
  translations: zxcvbnEnPackage.translations,
})

// only use the value for the zxcvbn call in the type handler
function typeHandler(value) {
  return zxcvbn(value)
}

On my local machine for the demo page this got rid of the small lags on fast typing till a certain password length. Do you mind testing this on your end, too?

I guess we always want to set the options separately, which means we would need to remove the setOptions from the zxcvbn(somePassword) call and adjust the documentation for it.

MrWook commented 3 years ago

Ah and by the way. With which reduce the scoring efficiency rapidly. i meant that the scoring will lose its significance without dictionaries.

I know that my english isn't top tier :D So if you wanna help out feel free to open a PR :)

henrywoody commented 3 years ago

Hey MrWook, thanks for the response. That looks like it should be the solution however I've come into a couple issues with this.

First I'm not able to import '@zxcvbn-ts/core/Options' as you show. I encounter Error: Can't resolve '@zxcvbn-ts/core/Options'.

I can import using '@zxcvbn-ts/core/dist/Options' without error, but this approach definitely does not set the dictionaries correctly (e.g. "password123" receives a score of 4/4).

I think including a named export of Options in the main index.ts could resolve this.

Can you test things out in an external package that imports @zxcvbn-ts/zxcvbn?

Also regarding the docs line, that makes more sense, thanks.

MrWook commented 3 years ago

I'm sry about the confusion with the dist folder didn't thought about that.

I tested it in a separated environment where i imported it with the package from npm. Something to keep in mind is that the Options file is a singleton which means you need to use the same filetype as the default import from the core. You probably using webpack or another bundler if you are using react, which means that the esm packages is used. Can you check out if there are improvements with the correct score if you use @zxcvbn-ts/core/dist/Options.esm

If thats not the case try to comment out the setOptions line in the zxcvbn-core library directly. This should definitely improve the performance.

I will remove this line and try to improve the handling to set/extend the options. Also i will try to export the Options singelton inside the index.js.

MrWook commented 3 years ago

The fix is published with the newest version 0.3.0

oliviercperrier commented 1 year ago

@MrWook I get this with the version 3.0.2.

It super slow, some password need more than 3 secondes to compute strength.

MrWook commented 1 year ago

@oliviercperrier are the passwords that you typed slow on the demo page too? https://zxcvbn-ts.github.io/zxcvbn/demo/

oliviercperrier commented 1 year ago

@oliviercperrier are the passwords that you typed slow on the demo page too? https://zxcvbn-ts.github.io/zxcvbn/demo/

@MrWook Yes, i used the orignal lib from dropbox with all dictionnaries and i get a calc time of like 40ms. But with zxcvbn-ts the calc time with the same password is 2400ms..

MrWook commented 1 year ago

@oliviercperrier This is a weird question but could you give me an example password? Not the real password that you are trying, except if its just a test password than you can give it to me.

What you can try on the demo page is disable the pwned matcher and the levenshtein distance. Both of them are new features which can take longer. The pwned matcher because it makes network request and the levenshtein distance gets slower for long passwords with a lot of special characters like ()"§=%&!*'_:§=$/"%§)"%§'*_:;+-*/=/*-+§$)§()/§"$'*_:;

BioSurienDG commented 1 year ago

@MrWook Here is an example (see screenshot). It takes almost 4 seconds to get a result while the other lib is very fast (31ms). Example Password: !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

image image
BioSurienDG commented 1 year ago

@MrWook Since there is a known example of this issue can we reopen the ticket? Also, any thoughts on when this may be fixed?

ghaskins99 commented 1 year ago

fyi @MrWook I checked in Chrome's performance tools and it looks like it's (at least partially) the l33t matcher. Disabling (setting the l33tMaxSubstitutions to 0) on the demo page brings performance back; sometimes 70ms, sometimes ~230ms. Setting it to 100 is still slow.

(fyi also the default for l33tMaxSubs in code appears to be 512, but the docs say 100. Should it be 512 or 100?)

using l33tMaxSubstitutions: 0 in the demo page:

image

it also appears that the repeated !!! is part of the issue; I'm assuming since it's a possible-l33t character. using all alphanumeric characters with l33tMax: 512 is still fast at 30ms, but add a few "l33t" characters (ex. !) and see the time spike:

image
MrWook commented 1 year ago

@ghaskins99 @BioSurienDG let's move this into https://github.com/zxcvbn-ts/zxcvbn/issues/228 as this closed issue is about the options handling