search

adonisjs / auth

Official Authentication package for AdonisJS
https://docs.adonisjs.com/guides/auth/introduction
MIT License
192 stars 65 forks source link

Removing UserNotFoundException #47

Closed ianwalter closed 7 years ago

ianwalter commented 7 years ago

adonis-auth shouldn't give clients information on whether an email address is registered or not. Also, adonis-auth should compare/verify a dummy password and return a PasswordMisMatchException when a user doesn't exist to help prevent user enumeration through timing attacks.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 97.697% when pulling 7428de3340698344060ca3255fc906c863be5627 on ianwalter:develop into f93b5193603a31008bda10ec04de417d66196dee on adonisjs:develop.

thetutlage commented 7 years ago

The UserNotFoundException is not for users and instead for developers to catch it and return a more informed message to the end user. More on catching exceptions http://adonisjs.com/docs/3.2/error-and-exceptions#_catching_exception

To avoid attacks, you should implement rate limiting on endpoints, instead of hacking the in-depth codebase to be slow.

Also the application errors messages should be written for users who are genuinely stuck into a situation, so that they can understand the problem by reading the error message.

ianwalter commented 7 years ago

Rate limiting doesn't really prevent the leak of information. It only slows it down.

One alternative is for developers to catch the UserNotFoundException and call the dummy compare/verify themselves. UserNotFoundException is a pretty generic name though. The developer doesn't know that the exception is thrown specifically by the login functionality.

On the other hand, it seems to go against the productive nature of Adonis to leave a security best-practice up to the users to implement when Adonis can implement it itself.

thetutlage commented 7 years ago

Okay can u tell me how AdonisJs knows what message needs to be returned for UserNotFoundException.

  1. What if you want to return message as a JSON payload.
  2. What if you want to return message in a different language than english.
  3. What if you have some structure of errors that you want to follow.

Regarding user doesn't know that this exception will thrown is, the developer should test the uses cases on what happens when login fails, or what happens when login passes.

ianwalter commented 7 years ago

Sorry, I'm not sure what you are asking. What I'm suggesting is that when a login attempt is made with a user that doesn't exist, adonis-auth should not throw a UserNotFoundException. Instead, it should perform a password compare (as it would when a user does exist but with a dummy password) and return a PasswordMisMatchException. Again, it would perform as it does when a user exists as to not give away any information on whether a user does or does not exist with the given email address / username. I can't think of an acceptable use case where the authentication system should reveal that information.

Side note: If you feel the name PasswordMisMatchException is too specific to contain this scenario then I would say it should be renamed to something like LoginFailureException, but up to you.