By default, passwords can't be all numbers because of a weak type check in user.js

[notbrain]

I would think a password that is all numbers would be a bit more secure than those that are of the well-known "letmein" or "password123" variety, especially if we enforce 8 digits.

Because of this line in user.js, passwords cannot be all numbers:

https://github.com/strongloop/loopback/blob/master/common/models/user.js#L610

This needs to be more intelligent and allow a full string of numbers for a password, or at the very least be highly publicized instead of buried inside the built-in code. As is, this is a bug that I had to go diving into the code to figure out why a bunch of our signups were being rejected for not having a password upon model creation, with no indication as to why the password was not making it all the way through to the database.

[notbrain]

Is there any harm in adding an explicit conversion like this? Not sure of the security ramifications but password requirements should be up to the app owners. Kind of pointless to do a toString() and immediately after check that it's a string, so there's some thinking to do around this.

UserModel.setter.password = function(plain) {

  plain = plain.toString();

  if (typeof plain !== 'string') {
    return;
  }
  if (plain.indexOf('$2a$') === 0 && plain.length === 60) {
    // The password is already hashed. It can be the case
    // when the instance is loaded from DB
    this.$password = plain;
  } else {
    this.$password = this.constructor.hashPassword(plain);
  }
};

[richardpringle]

@notbrain, good question about security ramifications. I think it might be better to check for the following:

if (typeof plain === 'number') {
  plain = plain.toString();
  // or at the very least throw a meaningful error
}

Do you want to submit the PR? You found the bug, you should get the credit for the fix!

[notbrain]

Thanks, yeah I like checking for number type and only converting at that point.

[notbrain]

#2335

[notbrain]

Ironically there is a test that tests for missing email that uses an all number password. Heh.

https://github.com/strongloop/loopback/blob/master/test/user.test.js#L114

Will try to carve out some time to add a test to include one that is valid with an all number password.

[richardpringle]

Yeah weird. I only tried to reproduce the issue through strong-remoting. Once you write your test, we should know if a different change needs to be made.