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

Add support for non-ASCII character sets #4

Closed
shssoichiro opened this issue Dec 21, 2016 · 8 comments
Closed

Add support for non-ASCII character sets #4

shssoichiro opened this issue Dec 21, 2016 · 8 comments

Comments

@shssoichiro
Copy link
Owner

shssoichiro commented Dec 21, 2016

Currently all of zxcvbn's operations are byte-based rather than character based, which makes operations on unicode strings likely to panic. The Repeat matcher also relies on having the ASCII character code of each character.

@rafapaez
Copy link
Contributor

rafapaez commented Jan 8, 2018

@shssoichiro Is there any feasible solution to support non-ascii passwords? Currently this is a significant restriction.

@rafapaez
Copy link
Contributor

Hi @shssoichiro . Given the current implementation, how hard would be to allow non-ascii characters?

@shssoichiro
Copy link
Owner Author

I think the current implementation is close to allowing non-ascii characters, as in it can parse those passwords without crashing. I'll just need to write some tests and remove the restriction on it.

As far as getting useful scores for foreign language passwords, that will take more work. With the current implementation, a password with non-ASCII characters will automatically get a high score, because those characters won't be found in any English-language dictionary or sequence that matches on an English keyboard.

@vks
Copy link
Contributor

vks commented Jan 18, 2018

As far as getting useful scores for foreign language passwords, that will take more work.

To address this, I think it would be enough to make keyboard layouts and dictionaries configurable. Note that this is AFAIK still unsolved in the original library.

@rafapaez
Copy link
Contributor

I've tried it to make it work, removing the NonAsciiPassword raises. Unfortunately it doesn't work without changing some of code related with the regex (it seems an easy change though).

16: zxcvbn::matching::spatial_match_helper::{{closure}}
             at src/matching/mod.rs:408
  17: zxcvbn::matching::omnimatch::{{closure}}
             at src/matching/mod.rs:32

I think the goal should be just make it as similar as the original Javascript one. If not possible, and supposing you're using it together with the client one, it would be acceptable for users if this one returns higher scores as it won't contradict the client's validation. In any case, I vote for removing the ascii restriction.

@shssoichiro
Copy link
Owner Author

shssoichiro commented Jan 19, 2018

Yes, I think it's reasonable as well. I'll see if I can get it working today or tomorrow. I did a refactor last week as the majority of work needed for this, to look at strings as unicode code points (chars) instead of as bytes, so this just needs to get over the finish line.

@rafapaez
Copy link
Contributor

Hey @shssoichiro . Sorry to bothering you. I couldn't managed to get this library working for non ascii symbols. This is the only issue preventing me to use this library in production. Would be awesome if you could get this done. 🙏

@shssoichiro
Copy link
Owner Author

Sorry for the delay. I wanted to update that I am working on this, however I've run into difficulties in getting the unicode tests to pass. I hope to finalize this and merge it to master soon.

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

No branches or pull requests

3 participants