Invalid Passport – The Daily WTF

Gretchen wanted to, in development, disable password authentication. Just for a minute, while she was testing things. That’s when she found this approach to handling authentication.

passport.authenticate('local', { session: true }, async (err, user) => {
  if (err) {
    res.send({ success: false, message: 'Error authenticating user.' })
  } else if (!user) {
    User.query()
      .where({ username: req.body.username })
      .first()
      .then(targetUser => {
        if (targetUser) {
          const hash = User.hashPassword(
            targetUser.password_salt,
            req.body.password
          )
          if (hash === targetUser.password_hash) {
            res.send({
              success: false,
              message: 'Incorrect username or password.',
            })
          } else {
            res.send({
              success: false,
              message: 'Incorrect username or password.',
            })
          }
        } else {
          res.send({
            success: false,
            message: 'Incorrect username or password.',
          })
        }
      })
      .catch(err => {
        res.send({ success: false, message: 'Internal server error' })
      })
  } else if (user.firstLogin) {

  }
})(req, res, next);

passport.authenticate invokes its callback after attempting to authenticate. Now, normally, this is called as middleware on a route defined on the webserver- that is to say, you don’t call it from within your application code, but as part of your routing configuration. That’s not the case here, where this blob is inside of a controller.

That’s weird, but let’s just trace through this code. We attempt to authenticate. When the process completes, our callback function executes. If authentication failed, there’s an error, so we’ll send an error message. Then, if the user object isn’t populated, we attempt to look up the user. If we find a user with that user name, we then hash their password and check if the hash matches. If it does, we send an error message. If it doesn’t, we send an error message. If we didn’t find the user, we send an error message. If anything goes wrong, we send an error message.

Wait a second, back up: if the user exists and their password matches, we send an error message?

I’ll let Gretchen explain a bit more:

passport.authenticate returns an error if the authentication failed and a user object, if it succeeded. We check this immediately: if error is set, return an error message. But then, we check if the user does not exist (aka: the authentication failed).

Yes, the reason user would be null is because the authentication failed. So the error would be set. So that entire branch about validating the user won’t happen: either the authentication worked and we know who the user is, or it failed, in which case we’d have an error. There’s no reasonable world where there’s no error but also no user object.

So yes, if authentication failed, but you manually re-run the authentication and it succeeds for some reason, yeah, you probably should still return an error. But I don’t know if it’s “Incorrect username or password”. It probably should be “Invalid reality, please restart the universe and see if the problem persists.”