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

Refactor pwnedPassword.js to use hash range API #22

Closed
danieladams456 opened this issue Mar 2, 2018 · 3 comments
Closed

Refactor pwnedPassword.js to use hash range API #22

danieladams456 opened this issue Mar 2, 2018 · 3 comments

Comments

@danieladams456
Copy link
Contributor

Depending on my availability over the next bit, I would be interested in trying to implement this feature.

Currently the pwnedPassword check uses the v1 api, but it would be nice to use the more secure v2 hash range api. This module could depend upon pwnedPasswordRange.js and provide the higher level API without needing to search through the results.

I was envisioning using this as a type of password strength tester for user signup beyond the basic length/character set, so there could be a case where you were ok with 5 pwns but not 5,000. I would add a maxPwns: number parameter to the options object (convenient it's already there!) to facilitate that behavior. Default maxPwns would be 1, and the method would return a boolean as it does currently.

@wKovacs64
Copy link
Owner

Hey @danieladams456, thanks for bringing this up!

When I saw the new search-by-range feature, I rushed to create pwnedPasswordRange.js to support it. I left pwnedPassword.js alone in case consumers wanted to use it for some reason, but I agree it would be nice if it used the more secure API (I can't think of a reason you'd want/need to send the password over the wire).

I do wonder if maxPwns is too use-case specific. I'd like to keep the public API surface as small as possible and I'm not sure adding an option to save consumers a one-liner check downstream is worthwhile. Perhaps pwnedPassword should return a count (number of occurrences) and leave that logic to the consumer? It would be a breaking change, returning a number instead of a bool, but I'm totally OK with a major version bump if needed. Thoughts?

@danieladams456
Copy link
Contributor Author

One thing I appreciated about your library (I'm new-ish to JS) was how well organized it was for the tree shaking and test coverage. I agree that using the more generic case of returning an integer would be less limiting for the consumer since it more closely mirrors the upstream API. If you're good with a major version release that sounds like a plan to me. The nice thing is you could still do an if statement off of the truthiness of the integer returned and get the same behavior.

Since isAHash was previously needed for an API ambiguity, can we drop it and drop configuration on this function call all together? Since the hash is an unsalted SHA-1 and no one should be actually storing passwords that way, I wouldn't think someone would need the use case of submitting a password from a DB they don't have the plaintext for. One nice ability gained by keeping the switch would be for the consumer to hash the password before calling the function so he doesn't have to trust the library quite as much since he controls the hashing. What do you think?

@wKovacs64
Copy link
Owner

One thing I appreciated about your library (I'm new-ish to JS) was how well organized it was for the tree shaking and test coverage.

Thanks for noticing, I've tried to put enough emphasis on those things. 😅

The nice thing is you could still do an if statement off of the truthiness of the integer returned and get the same behavior.

Yeah, exactly. It probably won't even be breaking for most people (doing loose truthy/falsey checks) but technically it will be, so we'll bump major.

Since isAHash was previously needed for an API ambiguity, can we drop it and drop configuration on this function call all together?

Let's drop it. Consumers will lose the ability to submit a hash using pwnedPassword but they could still do it via pwnedPasswordRange if they really wanted to re-implement pwnedPassword functionality on their end to avoid the library processing the password (that's what I do now in the CLI package, which I'll be able to revert once we implement it here). This will be a breaking change, too, so bumping major definitely makes sense now if there was any question before.

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

2 participants