Sails: Trying to get model validation messages to NOT include the failing data

[ksylvan]

My use case is user passwords.

I have added some custom validations, as documented here: http://sailsjs.org/documentation/concepts/models-and-orm/validations in the User model:

types: {
    hasUpperCase: function(s) { return /[A-Z]/.test(s) },
    hasLowerCase: function(s) { return /[a-z]/.test(s) },
    hasNumber: function(s) { return /\d/.test(s) },
    notGuessable: function(s) { return (PasswordService.strength(s).score > 2) }
},

And the password field definition:

password: {
  type: 'string',
  columnName: 'encrypted_password',
  minLength: 8,
  maxLength: 128,
  required: true,
  hasUpperCase: true,
  hasLowerCase: true,
  hasNumber: true,
  notGuessable: true
},

I'd like to be able to specify somehow that validation messages should redact the actual password that the user tried to set.

For example, a User.create() with the password "abc" yields this error:

{"error":"E_VALIDATION",
    "status":400,
    "summary":"1 attribute is invalid",
    "model":"User",
    "invalidAttributes":{
        "password":[
            {"rule":"minLength","message":"\"minLength\" validation rule failed for input: 'abc'"},
            {"rule":"hasUpperCase","message":"\"hasUpperCase\" validation rule failed for input: 'abc'"},
            {"rule":"hasNumber","message":"\"hasNumber\" validation rule failed for input: 'abc'"},
            {"rule":"notGuessable","message":"\"notGuessable\" validation rule failed for input: 'abc'"}
        ]
    }
}

The problem is that the error messages all include the bad (failing) password: "abc"

I have not figured out an elegant way to fix this, since the message is set in the matchRule.js file in the project's node_modules/sails/node_modules/anchor/lib/match/matchRule.js

Any suggestions?

[fernandolguevara]

@ksylvan, try with this hook

[mikermcneil]

@fernandolguevara looks neat!

@ksylvan What we normally do is take care of this in a custom action. I'm working on better documentation for this today (and re: https://github.com/balderdashy/sails/pull/3462) so I can write up an example if that would be helpful.

[ksylvan]

@mikermcneil Yes, an example would be great.

[ksylvan]

I ended up doing this in my UserController action, but if there's a way to do it that is more elegant, I would love that:

  User.create(
    {
      email: params.email,
      password: params.password
    })
  .then(function (createdUser) {
    sails.log.debug('Created user', createdUser);
    [...]
  })
  .catch(function (err) {
     if (err.invalidAttributes && err.invalidAttributes
        && err.invalidAttributes.password) {
     var newArr = err.invalidAttributes.password.map(function (t) {
       var newObj = {};
       for (var k in t) {
         newObj[k] = t[k].replace(params.password, '****');
       }
       return newObj;
     });
     err.invalidAttributes.password = newArr;
  }
  return res.badRequest({error: err});

[mikermcneil]

UPDATE

See below, things have progressed since Sails v1

Hey @ksylvan, you're right on target! Appreciate you saving me the time and putting this together.

Just for familiarity/reference for those folks who might be landing here from google, here's more or less the same thing written up using the style we've been using in the book (i.e. .exec(), bcrypt via mp-passwords, and lodash for data scrambling to avoid accidentally throwing in callbacks):

var Passwords = require('machinepack-passwords');

// Encrypt a string using the BCrypt algorithm.
Passwords.encryptPassword({
  password: req.param('password'),
}).exec({
  // An unexpected error occurred encrypting the password.
  error: function (err){
    return res.serverError(err);
  },
  // OK.
  success: function (encryptedPassword) {

    // Create a user record in the database.
    User.create({
      email: req.param('email'),
      password: encryptedPassword
    }).exec(function (err, newUser) {
      // If there was an error, we negotiate it.
      if (err) {

        // If this is NOT a waterline validation error, it is a mysterious error indeed.
        var isWLValidationErr = _.isObject(err) && _.isObject(err.invalidAttributes);
        if (!isWLValidationErr) {
          return res.serverError(err);
        }

        // Otherwise, it must be a waterline validation error.

        // If it doesn't contain a problem with the password, then just handle is
        // using `res.badRequest()` like normal.
        if ( !_.isArray(err.invalidAttributes.password) ) {
          return res.badRequest(err);
        }

        // Otherwise, something was wrong with the provided encrypted password.
        // So in this case, we'll modify the validation error in place to improve the error output
        // and so that we don't inadvertently reveal info about the encrypted password.
        // (specifically, we loop over the array of attribute errors and modify them).
        err.invalidAttributes.password = _.map(err.invalidAttributes.password, function eachPasswordErr (passwordError) {
          return _.reduce(passwordError, function (memo, val, key) {
            var allOccurrencesOfEncryptedPassMatcher = new RegExp(_.escapeRegExp(encryptedPassword),'g');
            memo[key] = val.replace(allOccurrencesOfEncryptedPassMatcher, '****');
            return memo;
          }, {});
        });

        // Finally, respond with the modified waterline validation error and a 400 status code.
        return res.badRequest(err);

      }//</if (err)>

      // Otherwise, `err` was falsy, so it worked!  The user was created.
      // (maybe do other stuff here, or just send a 200 OK response)
      return res.ok();

    });//</User.create>
  }
});//</Passwords.encryptPassword>

@ksylvan heads up: I have not had a chance to actually drop that ^^ into an app and test it yet. Docs for password encryption for reference.

[ksylvan]

:+1: Thanks.

Any reason to prefer the .exec(function(err, data) { ... } style over the .then(function(data) {...})...catch(function(err) { ... }) style?

[mikermcneil]

@ksylvan Great question!

A couple of years ago I probably would have told you something like:

"Promises, promises. Historically the Node community has been rather divided on the dichotomy, but I believe Ryan Dahl et al made the right decision to demonstrate only callback usage within examples in the official documentation for Node core. We should follow his example."

But since then, I've learned a lot from reading issues, SO questions, and tutorials about promises in Sails, using them myself (particularly in Angular and JQuery), and watching other developers on our team. My opinion has changed somewhat.

I have no problem with promises (in some cases I use them too). I'm _really_ fond of the way they allow you to assign separate scopes for the various parallel universes that could exist after an asynchronous function runs. I don't think promises truly solve the problem of implementing asynchronous versions of return/try/catch/finally as a language feature _yet_, but I'm a big fan of the motivations, and the related standardization efforts-- frankly, anything with the mission of improving the UX of building stuff with JavaScript is awesome.

However, I don't think it's productive for people to learn about promises _before_ or _instead of_ learning about callbacks. I've found that learning from the beginning with promises _rather than_ callbacks risks a kind of "shallow" understanding of asynchronous flow control that can eventually dry up (if you've ever read His Dark Materials, think of the moment when Lyra realizes she is no longer able to read the alethiometer).

In other words, if I'm helping a developer get started with intermediate-to-advanced JavaScript, and there's only time to learn one or the other, I'd much rather she take that time to obtain a true understanding of callbacks and the event loop. If you understand callbacks, then understanding promises down the road is as easy as trying to write your own promise library, which you can do in a couple of (albeit painful) hours. But if you start with promises, you may or may not have a true understanding of callbacks, and that can come back to bite you; e.g. when you throw an error inside of a vanilla setTimeout() callback, or when you up inheriting code which uses a promise library with nonstandard syntax (think of $.ajax vs. angular.http).

So to sum up, there's a reason why we support promises in Waterline, and I'm happy to do anything I can to support folks who like to use them in their Sails apps. I just think we're best served using traditional callbacks in examples, tutorials, and documentation -- unless of course it's a tutorial about using promises :)

[ksylvan]

:+1: Nice reference to "Promises, Promises" :-)

Thanks for the detailed explanation. I tend to agree with you.

My rule of thumb is to write it in whatever style promotes clarity, because I'll probably look at my own code in a week or two and if it requires me to _think_ at all to figure out what is happening, then I am screwed. ;-)

[mikermcneil]

My rule of thumb is to write it in whatever style promotes clarity, because I'll probably look at my own code in a week or two and if it requires me to think at all to figure out what is happening, then I am screwed. ;-)

:+1:

[ksylvan]

http://www.commitstrip.com/en/2016/01/18/what-idiot-wrote-this-code/

[mikermcneil]

Note: For future reference, Waterline's error codes are getting more granular in Sails v1. In general, you can now check err.name === 'UsageError' to see if there was something wrong with the arguments you passed in to any given model method. And you can check e.code to learn more about exactly what happened.

As for uniqueness errors, you'll find they now have err.code === 'E_UNIQUE' at the top level (for simpler programmatic negotiation) as well as err.name === 'AdapterError' (because they're a kind of error that is standardized and identified by the database adapter.)

[antzhdanov]

How do I get the exact model fields that failed validation? UsageError that is passed to callback gives a plain text describing errors but fields and actual errors are getting lost in waterline/utils/query/forge-stage-two-query.js.

[hlozancic]

@antzhdanov +1 why is there no modelIdentity and attrNames key inside UsageError just like E_UNIQUE has

[mikermcneil]

@antzhdanov @hlozancic it'd be possible to do this, and the data is available on internal, wrapped errors. Currently though, those internal errors get wrapped in order to improve the actual message.

I don't personally think it's a good idea to rely on model validation errors in your front-end code. Anything usage errors from model validations (so i.e. most form errors other than E_UNIQUE) should really be handled in client-side code, via client-side validations. It makes for a better user experience and allows for error messages that provide the appropriate context relative to the user
interface where they're being displayed.

Still, I get that it's convenient, especially for quick stuff, to be able to do things this way. Short term, the solution is to use the .validate() method, which exposes this.

---

Longer term, I'm ok with this being exposed for .update(), .createEach(), and .create() -- as long as it's done cleanly. The trick is to go into waterline core and change update.js create-each.js and create.js to explicitly include the relevant underlying props in the call to flaverr.
(There's already a switch statement with cases checking explicitly for each kind of relevant underlying error).

Tracked as a roadmap item here: https://trello.com/c/Nm8TvnBF

[mikermcneil]

See also https://github.com/balderdashy/sails/issues/3425