Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async matcher does not handle rejections #129

Closed
RossCurry opened this issue Jul 20, 2022 · 7 comments · Fixed by #130
Closed

Async matcher does not handle rejections #129

RossCurry opened this issue Jul 20, 2022 · 7 comments · Fixed by #130
Labels
bug Something isn't working

Comments

@RossCurry
Copy link

Rejections thrown by async matchers are not handled correctly. They end up as Uncaught (in Promise) type errors

To reproduce this:

  • go to https://zxcvbn-ts.github.io/zxcvbn/demo/
  • In the chrome developer toolbar, disable your internet connection
  • Type something in the password field
  • You'll notice the uncaught errors popping up in the developer toolbar

err

Not having a proper mechanism for error handling means that we cannot use things like the pnwd matcher in production

@MrWook MrWook added the bug Something isn't working label Jul 20, 2022
@MrWook
Copy link
Collaborator

MrWook commented Jul 26, 2022

@RossCurry what do you except what should happen? I would simply return false for the matcher in case of an network error.

@LaurensRietveld
Copy link

Not sure about @RossCurry (on holiday atm), but I'd expect the promise to be rejected. Are there any considerations for not propagating errors like that?

@MrWook
Copy link
Collaborator

MrWook commented Jul 27, 2022

The problem is that if an error is thrown inside zxcvbn-ts it will not be further executed. Which means a user won't get a scoring for the password.
I would assume in most cases the Form has a validation for the password strength.
For example this would mean the user can not register anymore and will be pretty annoyed. An error like "Our password scoring is currently not working" is not something that the user will understand.

@LaurensRietveld
Copy link

That's a fair point, and I agree it's a sensible default for most usecases. For other usecases it's a dangerous default though as we'd fail silently. If we'd apply zxcvbn on a server with incorrect network policies (e.g. whitelisting domains that the server is allowed to access, where the pwnd domain is not whitelisten), then a sysadmin would never detect this misconfiguration.

That being said, I'm not sure how to best cater to all usecases though. Possibly a configuration function handleNetworkError: (error:Err) => boolean | Error (where the default is false)?

@MrWook
Copy link
Collaborator

MrWook commented Jul 27, 2022

I thought about a config parameter too but this is a bit tricky so first let's get rid of the major issue and than make it better 👍

@MrWook
Copy link
Collaborator

MrWook commented Jul 27, 2022

@LaurensRietveld i published a new version for the pwned matcher

@LaurensRietveld
Copy link

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants