Loopback: Password validators operate on hashed password, making `length` validators useless

[STRML]

Modifying the User model in the following way does not trigger a validation error when attempting to create a user with an invalid password:

var User = app.models.User;
User.validatesLengthOf('password', {min: 6, message: {min: 'Password is too short'}});
var u = new User({email: "[email protected]", password: "short"});
assert(u.isValid()) // Error

Tracing it with node-inspector shows that the length is being compared to the hashed password, which is obviously always quite long.

[ritch]

Thanks for the bug report. We probably have to implement a custom validator for the password field. The length is lost as soon as the value is set. I'm thinking of moving the validation to the setter to prevent ever setting an invalid password. The issue is that the validator doesn't support this.

[bajtos]

Mailing-list discussion: https://groups.google.com/forum/#!topic/loopbackjs/SdWN5n3hMzw

[jonathanxie]

I'm not too familiar with the internals of loopback yet, but for this bug, I'm wondering if this will work for a fix in user.js

I tried to put some code in the UserMode.beforeRemote('create',...) method to do the hashing but that didn't work.

      UserModel.setter.password = function(plain) {
      //var salt = bcrypt.genSaltSync(this.constructor.settings.saltWorkFactor || SALT_WORK_FACTOR);
      //this.$password = bcrypt.hashSync(plain, salt);
      this.$password = plain;
    };

    UserModel.beforeCreate = function(next, user) {
      var password = user.password;
      console.log("user.password before hash = ", password);
      var salt = bcrypt.genSaltSync(this.constructor.settings.saltWorkFactor || SALT_WORK_FACTOR);
      user.password = bcrypt.hashSync(user.password, salt);
      console.log("user.password after hash = ", password);
      next();
    };

    // Make sure emailVerified is not set by creation
    UserModel.beforeRemote('create', function(ctx, user, next) {
      var body = ctx.req.body;
      if (body && body.emailVerified) {
        body.emailVerified = false;
      }
      next();
    });

[jonathanxie]

Actually, what I posted isn't going to work because if I call UserModel.beforeCreate here, and then I call beforeCreate in a custom Model that extends LoopBack's built-in User model and user.js file, then the createModel defined in Loopback's user.js file won't ever get called.

So for a workaround, I just did the following:

    UserModel.setter.password = function(plain) {
      //var salt = bcrypt.genSaltSync(this.constructor.settings.saltWorkFactor || SALT_WORK_FACTOR);
      //this.$password = bcrypt.hashSync(plain, salt);
      this.$password = plain;
    };

And then in my file that extends the built-in User model, I did this:

    UserModel.beforeCreate = function(next, user) {
      var password = user.password;
      console.log("user.password before hash = ", password);
      var salt = bcrypt.genSaltSync(this.constructor.settings.saltWorkFactor || SALT_WORK_FACTOR);
      user.password = bcrypt.hashSync(user.password, salt);
      console.log("user.password after hash = ", password);
      next();
    };

Works for a temporary workaround solution for now...

[bajtos]

Password validators operate on hashed password, making length validators useless

@STRML is this still an issue for you? I see we landed https://github.com/strongloop/loopback/pull/941 which allows users to implement a workaround. Is that good enough?

@raymondfeng @ritch do you remember if we have any idea what a proper solution for this problem would look like?

Then there is https://github.com/strongloop/loopback/pull/941#issuecomment-98756858 mentioning this issue should be easy to fix/rework once we have the "persist" hook in place, which has happened quite some time ago - see https://github.com/strongloop/loopback-datasource-juggler/issues/559.

Perhaps that's the way to go? To keep the password plaintext all the way until the "persist" hook that is called after any validations were passed.

Thoughts?

[av-k]

For example you can use context for temp data.

`
User.beforeRemote('create' , function(ctx, user, next) {
var context = User.app.loopback.getCurrentContext();

if (context) {
  context.set('currentPassword', ctx.req.body.password);
}

next();

});
`

And after validate it:
` User.validateAsync('password', function (err, done) {
var context = User.app.loopback.getCurrentContext(),
currentPassword = context.get('currentPassword');

if (currentPassword.length < config.validations.password.min) {
  err();
}

done();

}, {message: 'Password is too short (minimum 6 symbols)'});`

[JohnLindahlTech]

Is there any updates on this issue, what is current best practice?

I am not feeling overly confident in reimplementing a proper hashing myself and @av-k solution feels outdated now that getCurrentContext() is deprecated AFAIK (?)

[akapaul]

I've found another way to do this. In common model User there is a method called validatePassword. If we extend our UserModel from User, we can redefine this method in JS, like following:

var g = require('loopback/lib/globalize');

module.exports = function(UserModel) {
  UserModel.validatePassword = function(plain) {
    var err,
        passwordProperties = UserModel.definition.properties.password;

    if (plain.length > passwordProperties.max) {
      err = new Error (g.f('Password too long: %s (maximum %d symbols)', plain, passwordProperties.max));
      err.code = 'PASSWORD_TOO_LONG';
    } else if (plain.length < passwordProperties.min) {
      err = new Error(g.f('Password too short: %s (minimum %d symbols)', plain, passwordProperties.min));
      err.code = 'PASSWORD_TOO_SHORT';
    } else if(!(new RegExp(passwordProperties.pattern, 'g').test(plain))) {
      err =  new Error(g.f('Invalid password: %s (symbols and numbers are allowed)', plain));
      err.code = 'INVALID_PASSWORD';
    } else {
      return true;
    }
    err.statusCode = 422;
    throw err;
  };
};

This works for me. I don't think that g (globalize) object is required here, but I added this, just in case. Also, I've added my validator options in JSON definition of UserModel, because of Loopback docs

[pinicius]

Hi akapaul, I've tried your solution. This is my code:

module.exports = function(Investigator) {

  Investigator.validatePassword = function(plain) {
      var err,
          passwordProperties = Investigator.definition.properties.password;
      if (plain.length > passwordProperties.max) {
        err = new Error (g.f('Password too long: %s (maximum %d symbols)', plain, passwordProperties.max));
        err.code = 'PASSWORD_TOO_LONG';
      } else if (plain.length < passwordProperties.min) {
        err = new Error(g.f('Password too short: %s (minimum %d symbols)', plain, passwordProperties.min));
        err.code = 'PASSWORD_TOO_SHORT';
      } else {
        return true;
      }
      err.statusCode = 422;
      throw err;
    };

  Investigator.validatesPresenceOf('email');
  Investigator.validatePassword('password', {min: 6, max: 16, message: {min: 'Too sort', max: 'Too long'}});
  Investigator.validatesInclusionOf('type', {in: ['normal', 'boss']});
  Investigator.validatesUniquenessOf('email', {message: 'email is not unique'});

};

And investigator.json file:

{
  "name": "investigator",
  "base": "User",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "properties": {
    "password": {
      "type": "string",
      "required": true,
      "min": 6,
      "max": 16
    },
...
}

My question is how can I throw a validation exception instead an Error? I'm newbie with strongloop

Thanks in advance

[akapaul]

Hi @pinicius!

You should not call the validatePassword method directly in model definition. This is called inside loopback code while validating User model, so the validation exception will be thrown by loopback. If you want to understand the logic, how user validation occurs, you can look inside native User model, see lines 589, 640, 645

[pinicius]

Hi @akapaul and thanks for your quick response!

My code finally after your advise

module.exports = function(Investigator) {

  Investigator.validatePassword = function(plain) {
        var err,
            passwordProperties = Investigator.definition.properties.password;

        if (plain.length > passwordProperties.max) {
          err = new Error (g.f('Password too long: %s (maximum %d symbols)', plain, passwordProperties.max));
          err.code = 'PASSWORD_TOO_LONG';
        } else if (plain.length < passwordProperties.min) {
          err = new Error(g.f('Password too short: %s (minimum %d symbols)', plain, passwordProperties.min));
          err.code = 'PASSWORD_TOO_SHORT';
        } else if(!(new RegExp(passwordProperties.pattern, 'g').test(plain))) {
          err =  new Error(g.f('Invalid password: %s (symbols and numbers are allowed)', plain));
          err.code = 'INVALID_PASSWORD';
        } else {
          return true;
        }
        err.statusCode = 422;
        throw err;
      };

  Investigator.validatesInclusionOf('type', {in: ['normal', 'boss']});
  Investigator.validatesUniquenessOf('email', {message: 'email is not unique'});
  Investigator.validatesLengthOf('password', {min: 6, max: 16, message: {min: 'Too short', max: 'Too long'}});

};

investigator.json file:

{
  "name": "investigator",
  "base": "User",
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "properties": {
    "password": {
      "type": "string",
      "required": true,
      "min": 6,
      "max": 16
    },
...
}

However, it continues raised Error exception instead of captures it like Validation Exception.

I have defined a route for user registering like this (routes.js):

//create a new account
    router.post('/register', function(req, res) {
      Investigator.create({
        type: req.body.type,
        username: req.body.username,
        email: req.body.email,
        password: req.body.password
      }, function(err, accountInstance) {
        if (err) {
          console.log(err)
          res.render('register', {
            type: '',
            username: '',
            email: '',
            password: '',
            registerFailed: true,
            errorMessage: err.message
          });
        return;
       }

       res.render('login', {
         username: req.body.username,
         password: '',
         loginFailed: false
       });
      });
    });

What I'm doing wrong??

[akapaul]

Hi @pinicius!

What error do you see? Have you imported g module, using the line:

var g = require('loopback/lib/globalize');

It is not required, anyway, but if you omit it, you should simplify strings you pass to Error object constructor, like passing a concatenated strings, without formatting symbols, e.g. replacing:

err = new Error (g.f('Password too long: %s (maximum %d symbols)', plain, passwordProperties.max));

to

err = new Error ('Password too long: ' + plain + ' (maximum ' + passwordProperties.max + ' symbols)');

Also, you should not define own routes in loopback using the way you mentioned, because it creates them from model definitions.

[pinicius]

Hi @akapaul !

you should not define own routes in loopback using the way you mentioned, because it creates them from model definitions.

I did not know that, I supposed It would create the new models from the my model (with rewrite method)

Thanks so much, It works :) :+1:

[kooliokey]

@akapaul's suggestion works for me, except I need access to the model's other data inside this method to make sure they aren't using something like their name for their password. Suggestions for accessing this data? this looks to be set to the server instance, not the model.

[rashmihunt]

@STRML As per the above conversation, looks like there is solution for originally reported problem. We will be closing this soon. Let us know.

[STRML]

So your suggestion is to override User.validatePassword?

[rashmihunt]

@STRML As far as I can understand following above discussion, per @bajtos
comments above https://github.com/strongloop/loopback/issues/251#issuecomment-163717176, there are couple of possible workarounds - override validatePassword(), use 'persist' hook.

"
I see we landed https://github.com/strongloop/loopback/pull/941 which allows users to implement a workaround. Is that good enough?
…….

Then there is https://github.com/strongloop/loopback/pull/941#issuecomment-98756858 (comment) mentioning this issue should be easy to fix/rework once we have the "persist" hook in place, which has happened quite some time ago - see strongloop/loopback-datasource-juggler#559.

Perhaps that's the way to go? To keep the password plaintext all the way until the "persist" hook that is called after any validations were passed.

"

[STRML]

Sounds to me like now that persist is in, then there just needs to be a fix implemented that knows if you're trying to validate 'password', uses the right hook, then this can be closed. But it shouldn't be closed just with a workaround.

[raymondfeng]

In general, we need to have a better way to handle calculated/transformed properties such as password upon 'read/write`. I guess we should open an issue for the broader feature.

[rashmihunt]

@raymondfeng opened feature https://github.com/strongloop/loopback/issues/3468
Will close this bug as couple of workaround available and broader feature is tracked through #3468

[jannyHou]

Hi @rashmihunt, would you mind if I close this issue and track the feature progress in your new issue https://github.com/strongloop/loopback/issues/3468?

[bajtos]

IMO, we should keep this issue open. As I understand it, #3468 will lay groundwork necessary to fix password validators. With this foundation in place, we will still need to rework the way how password are handled in LoopBack.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.