why assert on an async callback
Closed this issue · 7 comments
using assert which throws an error if the data doesn't match the parameters feels a little odd for a library with an async callback -- why not call the callback with an error?
it feels very odd to protect myself from a crashing program with doing:
try {
pwd.verify(pass, hash, cb)
} catch (err) {
cb(err)
}Rather than just
pwd.verify(pass, has, cb)Would you be open to a PR changing the async methods to call the callback instead of throwing an AssertionError?
You should never try catch asserts. If something is throwing its because you are using it wrong and your program isn't working as expected.
You probably want to validate the input before passing it :)
🔥 this is also why promises are terrible at error handling 🔥
I agree with this on principal, and ideally you'd catch bad data before a user is able to present it to a critical function. However, as we all know, bad users are extremely good at finding other vulnerabilities -- if there was a way they could bypass checking or somehow otherwise get bad data into the database, they would have a trivial time making a denial of service attack against your application by causing it to crash every time the presented a login attempt.
There are other places where I feel throwing is the right approach (the database not being available for example), but password creation/verification is not one of them in my mind.
I'm currently on holidays but back next Monday and I will write a proper response. However the assertions made are requirements of the underlying algorithm and if any of the assertions throw it's a programmer bug. However I will change the hash decoding part to not throw but maybe return a "unrecognised hash format" or similar, since that might be because of legacy hashes from another algorithm :) I've also considered writing a migration module for users with legacy hashes
@toddself I have made some changes motivated by your issue. Thanks! I still believe that the assertions are made to help catch programmer errors, such as mistyped inputs, but I was too strict on the format of the input hash. Now there's a SecurePassword.INVALID_UNRECOGNIZED_HASH return type and legacy hashes will therefore no longer cause a program crash. This also means that unrecognized hashes are not attempted verified, which is much cheaper, and makes it possible to fall back to a legacy algorithm.
Closing for now. Please reopen if you have more feedback :)
@emilbayes thank you for this. this feels much better from an "end developer"