zxcvbn-ts / zxcvbn

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

refactor(pwned): use `pathe.join` to create `haveIBeenPwned` URL #240

Closed DamianGlowala closed 1 year ago

DamianGlowala commented 1 year ago

This improves DX by removing a need for a user to remember to add a trailing slash at the end of the custom URL used by haveIBeenPwned.ts:

const matcherPwned = matcherPwnedFactory(fetch, zxcvbnOptions, {
-  url: "/api/pwnedpasswords/"
+  url: "/api/pwnedpasswords"
});
MrWook commented 1 year ago

Hey thank you for you contribution. I hesitation to include an entire library just to append a trailing slash to an URL when a simple if statement can accomplish the task. For example:

if (!url.endsWith('/')) {
  url = `${url}/`;
}

While libraries like pathe are excellent for file system operations, they're not necessary in this context, as we're only making HTTP requests using fetch.

Furthermore, it's important to consider that the URL might be customized by users who want to implement their own version of the pwned API. For example, the URL could be something like https://www.my-custom-domain.com/pwned?range=, which makes the trailing / less universally applicable.

So i think that i will not merge this Feature 🤔

DamianGlowala commented 1 year ago

Thank you for your explanation. I agree with the points you raised and I'm happy to close this PR.

Instead, what would you say about being able to provide a custom fetching function for most flexibility? For instace, Nuxt provides $fetch, which I'd love to use in the following way instead of e.g. native fetch provided by the Node.js runtime. This allows for full freedom in combining custom URL with a range param:

const matcherFactory = matcherPwnedFactory(
  (range) => $fetch(`http://localhost:3000/api/pwnedpasswords/${range}`),
  ...
)