Improved validation layer (#9427)

refs https://github.com/TryGhost/Ghost/issues/3658

- the `validateSchema` helper was a bit broken
  - if you add a user without email, you will receive a database error
  - but the validation error should catch that email is passed with null
- it was broken, because:
  - A: it called `toJSON` -> this can remove properties from the output (e.g. password)
  - B: we only validated fields, which were part of the JSON data (model.hasOwnProperty)
- we now differentiate between schema validation for update and insert
- fixed one broken import test
  - if you import a post without a status, it should not error
  - it falls back to the default value
- removed user model `onValidate`
  - the user model added a custom implementation of `onValidate`, because of a bug which we experienced (see https://github.com/TryGhost/Ghost/issues/3638)
  - with the refactoring this is no longer required - we only validate fields which have changed when updating resources
  - also, removed extra safe catch when logging in (no longer needed - unit tested)
- add lot's of unit tests to proof the code change
- always call the base class, except you have a good reason

core/server/data/validation/index.js

@@ -161,19 +161,40 @@ validatePassword = function validatePassword(password, email, blogTitle) {
     return validationResult;
 };
 
-// Validation against schema attributes
-// values are checked against the validation objects from schema.js
-validateSchema = function validateSchema(tableName, model) {
+/**
+ * Validate model against schema.
+ *
+ * ## on model update
+ * - only validate changed fields
+ * - otherwise we could throw errors which the user is out of control
+ * - e.g.
+ *   - we add a new field without proper validation, release goes out
+ *   - we add proper validation for a single field
+ * - if you call `user.save()` the default fallback in bookshelf is `options.method=update`.
+ * - we set `options.method` explicit for adding resources (because otherwise bookshelf uses `update`)
+ *
+ * ## on model add
+ * - validate everything to catch required fields
+ */
+validateSchema = function validateSchema(tableName, model, options) {
+    options = options || {};
+
     var columns = _.keys(schema[tableName]),
         validationErrors = [];
 
     _.each(columns, function each(columnKey) {
         var message = '',
-            strVal = _.toString(model[columnKey]);
+            strVal = _.toString(model.get(columnKey)); // KEEP: Validator.js only validates strings.
+
+        if (options.method !== 'insert' && !_.has(model.changed, columnKey)) {
+            return;
+        }
 
         // check nullable
-        if (model.hasOwnProperty(columnKey) && schema[tableName][columnKey].hasOwnProperty('nullable')
-            && schema[tableName][columnKey].nullable !== true) {
+        if (schema[tableName][columnKey].hasOwnProperty('nullable') &&
+            schema[tableName][columnKey].nullable !== true &&
+            !schema[tableName][columnKey].hasOwnProperty('defaultTo')
+        ) {
             if (validator.empty(strVal)) {
                 message = common.i18n.t('notices.data.validation.index.valueCannotBeBlank', {
                     tableName: tableName,
@@ -187,7 +208,7 @@ validateSchema = function validateSchema(tableName, model) {
         }
 
         // validate boolean columns
-        if (model.hasOwnProperty(columnKey) && schema[tableName][columnKey].hasOwnProperty('type')
+        if (schema[tableName][columnKey].hasOwnProperty('type')
             && schema[tableName][columnKey].type === 'bool') {
             if (!(validator.isBoolean(strVal) || validator.empty(strVal))) {
                 message = common.i18n.t('notices.data.validation.index.valueMustBeBoolean', {
@@ -202,7 +223,7 @@ validateSchema = function validateSchema(tableName, model) {
         }
 
         // TODO: check if mandatory values should be enforced
-        if (model[columnKey] !== null && model[columnKey] !== undefined) {
+        if (model.get(columnKey) !== null && model.get(columnKey) !== undefined) {
             // check length
             if (schema[tableName][columnKey].hasOwnProperty('maxlength')) {
                 if (!validator.isLength(strVal, 0, schema[tableName][columnKey].maxlength)) {

core/server/models/base/index.js

@@ -78,11 +78,6 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
         return _.keys(schema.tables[this.tableName]);
     },
 
-    // Bookshelf `defaults` - default values setup on every model creation
-    defaults: function defaults() {
-        return {};
-    },
-
     // When loading an instance, subclasses can specify default to fetch
     defaultColumnsToFetch: function defaultColumnsToFetch() {
         return [];
@@ -136,8 +131,12 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
         proto.initialize.call(this);
     },
 
-    onValidate: function onValidate() {
-        return validation.validateSchema(this.tableName, this.toJSON());
+    /**
+     * Do not call `toJSON`. This can remove properties e.g. password.
+     * @returns {*}
+     */
+    onValidate: function onValidate(model, columns, options) {
+        return validation.validateSchema(this.tableName, this, options);
     },
 
     /**

core/server/models/post.js

@@ -20,6 +20,22 @@ Post = ghostBookshelf.Model.extend({
 
     tableName: 'posts',
 
+    /**
+     * ## NOTE:
+     * We define the defaults on the schema (db) and model level.
+     * When inserting resources, the defaults are available **after** calling `.save`.
+     * But they are available when the model hooks are triggered (e.g. onSaving).
+     * It might be useful to keep them in the model layer for any connected logic.
+     *
+     * e.g. if `model.get('status') === draft; do something;
+     */
+    defaults: function defaults() {
+        return {
+            uuid: uuid.v4(),
+            status: 'draft'
+        };
+    },
+
     relationships: ['tags'],
 
     /**
@@ -49,13 +65,6 @@ Post = ghostBookshelf.Model.extend({
         common.events.emit(resourceType + '.' + event, this, options);
     },
 
-    defaults: function defaults() {
-        return {
-            uuid: uuid.v4(),
-            status: 'draft'
-        };
-    },
-
     /**
      * We update the tags after the Post was inserted.
      * We update the tags before the Post was updated, see `onSaving` event.

core/server/models/settings.js

@@ -77,12 +77,12 @@ Settings = ghostBookshelf.Model.extend({
     },
 
     onValidate: function onValidate() {
-        var self = this,
-            setting = this.toJSON();
+        var self = this;
 
-        return validation.validateSchema(self.tableName, setting).then(function then() {
-            return validation.validateSettings(getDefaultSettings(), self);
-        });
+        return ghostBookshelf.Model.prototype.onValidate.apply(this, arguments)
+            .then(function then() {
+                return validation.validateSettings(getDefaultSettings(), self);
+            });
     }
 }, {
     findOne: function (data, options) {

core/server/models/user.js

@@ -24,11 +24,9 @@ User = ghostBookshelf.Model.extend({
     tableName: 'users',
 
     defaults: function defaults() {
-        var baseDefaults = ghostBookshelf.Model.prototype.defaults.call(this);
-
-        return _.merge({
+        return {
             password: security.identifier.uid(50)
-        }, baseDefaults);
+        };
     },
 
     emitChange: function emitChange(event, options) {
@@ -90,6 +88,22 @@ User = ghostBookshelf.Model.extend({
 
         ghostBookshelf.Model.prototype.onSaving.apply(this, arguments);
 
+        /**
+         * Bookshelf call order:
+         *   - onSaving
+         *   - onValidate (validates the model against the schema)
+         *
+         * Before we can generate a slug, we have to ensure that the name is not blank.
+         */
+        if (!this.get('name')) {
+            throw new common.errors.ValidationError({
+                message: common.i18n.t('notices.data.validation.index.valueCannotBeBlank', {
+                    tableName: this.tableName,
+                    columnKey: 'name'
+                })
+            });
+        }
+
         // If the user's email is set & has changed & we are not importing
         if (self.hasChanged('email') && self.get('email') && !options.importing) {
             tasks.gravatar = (function lookUpGravatar() {
@@ -169,24 +183,6 @@ User = ghostBookshelf.Model.extend({
         return Promise.props(tasks);
     },
 
-    // For the user model ONLY it is possible to disable validations.
-    // This is used to bypass validation during the credential check, and must never be done with user-provided data
-    // Should be removed when #3691 is done
-    onValidate: function validate() {
-        var opts = arguments[1],
-            userData;
-
-        if (opts && _.has(opts, 'validate') && opts.validate === false) {
-            return;
-        }
-
-        // use the base toJSON since this model's overridden toJSON
-        // removes fields and we want everything to run through the validator.
-        userData = ghostBookshelf.Model.prototype.toJSON.call(this);
-
-        return validation.validateSchema(this.tableName, userData);
-    },
-
     toJSON: function toJSON(unfilteredOptions) {
         var options = User.filterOptions(unfilteredOptions, 'toJSON'),
             attrs = ghostBookshelf.Model.prototype.toJSON.call(this, options);
@@ -658,50 +654,41 @@ User = ghostBookshelf.Model.extend({
     check: function check(object) {
         var self = this;
 
-        return this.getByEmail(object.email).then(function then(user) {
-            if (!user) {
-                return Promise.reject(new common.errors.NotFoundError({
-                    message: common.i18n.t('errors.models.user.noUserWithEnteredEmailAddr')
-                }));
-            }
+        return this.getByEmail(object.email)
+            .then(function then(user) {
+                if (!user) {
+                    throw new common.errors.NotFoundError({
+                        message: common.i18n.t('errors.models.user.noUserWithEnteredEmailAddr')
+                    });
+                }
 
-            if (user.isLocked()) {
-                return Promise.reject(new common.errors.NoPermissionError({
-                    message: common.i18n.t('errors.models.user.accountLocked')
-                }));
-            }
+                if (user.isLocked()) {
+                    throw new common.errors.NoPermissionError({
+                        message: common.i18n.t('errors.models.user.accountLocked')
+                    });
+                }
 
-            if (user.isInactive()) {
-                return Promise.reject(new common.errors.NoPermissionError({
-                    message: common.i18n.t('errors.models.user.accountSuspended')
-                }));
-            }
+                if (user.isInactive()) {
+                    throw new common.errors.NoPermissionError({
+                        message: common.i18n.t('errors.models.user.accountSuspended')
+                    });
+                }
 
-            return self.isPasswordCorrect({plainPassword: object.password, hashedPassword: user.get('password')})
-                .then(function then() {
-                    return Promise.resolve(user.set({status: 'active', last_seen: new Date()}).save({validate: false}))
-                        .catch(function handleError(err) {
-                            // If we get a validation or other error during this save, catch it and log it, but don't
-                            // cause a login error because of it. The user validation is not important here.
-                            common.logging.error(new common.errors.GhostError({
-                                err: err,
-                                context: common.i18n.t('errors.models.user.userUpdateError.context'),
-                                help: common.i18n.t('errors.models.user.userUpdateError.help')
-                            }));
+                return self.isPasswordCorrect({plainPassword: object.password, hashedPassword: user.get('password')})
+                    .then(function then() {
+                        user.set({status: 'active', last_seen: new Date()});
+                        return user.save();
+                    });
+            })
+            .catch(function (err) {
+                if (err.message === 'NotFound' || err.message === 'EmptyResponse') {
+                    throw new common.errors.NotFoundError({
+                        message: common.i18n.t('errors.models.user.noUserWithEnteredEmailAddr')
+                    });
+                }
 
-                            return user;
-                        });
-                })
-                .catch(function onError(err) {
-                    return Promise.reject(err);
-                });
-        }, function handleError(error) {
-            if (error.message === 'NotFound' || error.message === 'EmptyResponse') {
-                return Promise.reject(new common.errors.NotFoundError({message: common.i18n.t('errors.models.user.noUserWithEnteredEmailAddr')}));
-            }
-
-            return Promise.reject(error);
-        });
+                throw err;
+            });
     },
 
     isPasswordCorrect: function isPasswordCorrect(object) {

core/test/integration/data/importer/importers/data_spec.js

@@ -69,6 +69,23 @@ describe('Import', function () {
             }).catch(done);
         });
 
+        it('can import user with missing allowed fields', function (done) {
+            var exportData;
+
+            testUtils.fixtures.loadExportFixture('export-003-missing-allowed-fields', {lts: true}).then(function (exported) {
+                exportData = exported;
+                return dataImporter.doImport(exportData, importOptions);
+            }).then(function (importResult) {
+                importResult.data.users[0].hasOwnProperty('website');
+                importResult.data.users[0].hasOwnProperty('bio');
+                importResult.data.users[0].hasOwnProperty('accessibility');
+                importResult.data.users[0].hasOwnProperty('cover_image');
+                importResult.problems.length.should.eql(1);
+
+                done();
+            }).catch(done);
+        });
+
         it('removes duplicate tags and updates associations', function (done) {
             var exportData;
 
@@ -488,14 +505,11 @@ describe('Import', function () {
             }).then(function () {
                 done(new Error('Allowed import of invalid post data'));
             }).catch(function (response) {
-                response.length.should.equal(2, response);
+                response.length.should.equal(1, response);
 
                 response[0].errorType.should.equal('ValidationError');
                 response[0].message.should.eql('Value in [posts.title] cannot be blank.');
 
-                response[1].errorType.should.equal('ValidationError');
-                response[1].message.should.eql('Value in [posts.status] cannot be blank.');
-
                 done();
             }).catch(done);
         });

core/test/unit/data/validation/index_spec.js

@@ -1,8 +1,18 @@
+'use strict';
+
 var should = require('should'),
+    _ = require('lodash'),
+    ObjectId = require('bson-objectid'),
+    testUtils = require('../../../utils'),
+    models = require('../../../../server/models'),
     validation = require('../../../../server/data/validation');
 
 // Validate our customisations
 describe('Validation', function () {
+    before(function () {
+        models.init();
+    });
+
     it('should export our required functions', function () {
         should.exist(validation);
 
@@ -18,7 +28,103 @@ describe('Validation', function () {
         validation.validator.should.have.properties(['empty', 'notContains', 'isTimezone', 'isEmptyOrURL', 'isSlug']);
     });
 
-    describe('Validator customisations', function () {
+    describe('Validate Schema', function () {
+        describe('models.add', function () {
+            it('blank model', function () {
+                // NOTE: Fields with `defaultTo` are getting ignored. This is handled on the DB level.
+                return validation.validateSchema('posts', models.Post.forge(), {method: 'insert'})
+                    .then(function () {
+                        throw new Error('Expected ValidationError.');
+                    })
+                    .catch(function (err) {
+                        if (!_.isArray(err)) {
+                            throw err;
+                        }
+
+                        err.length.should.eql(7);
+
+                        const errorMessages = _.map(err, function (object) {
+                            return object.message;
+                        }).join(',');
+
+                        // NOTE: Some of these fields are auto-filled in the model layer (e.g. author_id, created_at etc.)
+                        ['id', 'uuid', 'slug', 'title', 'author_id', 'created_at', 'created_by'].forEach(function (attr) {
+                            errorMessages.should.match(new RegExp('posts.' + attr));
+                        });
+                    });
+            });
+
+            it('blank id', function () {
+                const postModel = models.Post.forge(testUtils.DataGenerator.forKnex.createPost({
+                    id: null,
+                    slug: 'test'
+                }));
+
+                return validation.validateSchema('posts', postModel, {method: 'insert'})
+                    .then(function () {
+                        throw new Error('Expected ValidationError.');
+                    })
+                    .catch(function (err) {
+                        if (!_.isArray(err)) {
+                            throw err;
+                        }
+
+                        err.length.should.eql(1);
+                        err[0].message.should.match(/posts\.id/);
+                    });
+            });
+
+            it('should pass', function () {
+                return validation.validateSchema(
+                    'posts',
+                    models.Post.forge(testUtils.DataGenerator.forKnex.createPost({slug: 'title'})),
+                    {method: 'insert'}
+                );
+            });
+        });
+
+        describe('models.edit', function () {
+            it('uuid is invalid', function () {
+                const postModel = models.Post.forge({id: ObjectId.generate(), uuid: '1234'});
+
+                postModel.changed = {uuid: postModel.get('uuid')};
+
+                return validation.validateSchema('posts', postModel)
+                    .then(function () {
+                        throw new Error('Expected ValidationError.');
+                    })
+                    .catch(function (err) {
+                        if (!_.isArray(err)) {
+                            throw err;
+                        }
+
+                        err.length.should.eql(1);
+                        err[0].message.should.match(/isUUID/);
+                    });
+            });
+
+            it('date is null', function () {
+                const postModel = models.Post.forge({id: ObjectId.generate(), created_at: null});
+
+                postModel.changed = {created_at: postModel.get('updated_at')};
+
+                return validation.validateSchema('posts', postModel)
+                    .then(function () {
+                        throw new Error('Expected ValidationError.');
+                    })
+                    .catch(function (err) {
+                        if (!_.isArray(err)) {
+                            throw err;
+                        }
+
+                        err.length.should.eql(1);
+                        err[0].message.should.match(/posts\.created_at/);
+                    });
+            });
+        });
+    });
+
+    describe('Assert the Validator dependency', function () {
         var validator = validation.validator;
 
         it('isEmptyOrUrl filters javascript urls', function () {

core/test/unit/models/user_spec.js

@@ -3,12 +3,13 @@
 const should = require('should'),
     sinon = require('sinon'),
     models = require('../../../server/models'),
+    validation = require('../../../server/data/validation'),
     common = require('../../../server/lib/common'),
     security = require('../../../server/lib/security'),
     testUtils = require('../../utils'),
     sandbox = sinon.sandbox.create();
 
-describe('Models: User', function () {
+describe('Unit: models/user', function () {
     let knexMock;
 
     before(function () {
@@ -66,9 +67,105 @@ describe('Models: User', function () {
                     });
             });
         });
+
+        describe('blank', function () {
+            it('name cannot be blank', function () {
+                return models.User.add({email: 'test@ghost.org'})
+                    .then(function () {
+                        throw new Error('expected ValidationError');
+                    })
+                    .catch(function (err) {
+                        (err instanceof common.errors.ValidationError).should.be.true;
+                        err.message.should.match(/users\.name/);
+                    });
+            });
+
+            it('email cannot be blank', function () {
+                let data = {name: 'name'};
+                sandbox.stub(models.User, 'findOne').resolves(null);
+
+                return models.User.add(data)
+                    .then(function () {
+                        throw new Error('expected ValidationError');
+                    })
+                    .catch(function (err) {
+                        err.should.be.an.Array();
+                        (err[0] instanceof common.errors.ValidationError).should.eql.true;
+                        err[0].message.should.match(/users\.email/);
+                    });
+            });
+        });
     });
 
-    describe('Permissible', function () {
+    describe('fn: check', function () {
+        before(function () {
+            knexMock = new testUtils.mocks.knex();
+            knexMock.mock();
+        });
+
+        beforeEach(function () {
+            sandbox.stub(security.password, 'hash').resolves('$2a$10$we16f8rpbrFZ34xWj0/ZC.LTPUux8ler7bcdTs5qIleN6srRHhilG');
+        });
+
+        after(function () {
+            knexMock.unmock();
+        });
+
+        it('user status is warn', function () {
+            sandbox.stub(security.password, 'compare').resolves(true);
+
+            // NOTE: Add a user with a broken field to ensure we only validate changed fields on login
+            sandbox.stub(validation, 'validateSchema').resolves();
+
+            const user = testUtils.DataGenerator.forKnex.createUser({
+                status: 'warn-1',
+                email: 'test-9@example.de',
+                website: '!!!!!this-is-not-a-website!!!!'
+            });
+
+            return models.User.add(user)
+                .then(function (model) {
+                    validation.validateSchema.restore();
+
+                    return models.User.check({email: model.get('email'), password: 'test'});
+                });
+        });
+
+        it('user status is active', function () {
+            sandbox.stub(security.password, 'compare').resolves(true);
+
+            return models.User.check({email: testUtils.DataGenerator.Content.users[1].email, password: 'test'});
+        });
+
+        it('password is incorrect', function () {
+            sandbox.stub(security.password, 'compare').resolves(false);
+
+            return models.User.check({email: testUtils.DataGenerator.Content.users[1].email, password: 'test'})
+                .catch(function (err) {
+                    (err instanceof common.errors.ValidationError).should.be.true;
+                });
+        });
+
+        it('user not found', function () {
+            sandbox.stub(security.password, 'compare').resolves(true);
+
+            return models.User.check({email: 'notfound@example.to', password: 'test'})
+                .catch(function (err) {
+                    (err instanceof common.errors.NotFoundError).should.be.true;
+                });
+        });
+
+        it('user not found', function () {
+            sandbox.stub(security.password, 'compare').resolves(true);
+
+            return models.User.check({email: null, password: 'test'})
+                .catch(function (err) {
+                    (err instanceof common.errors.NotFoundError).should.be.true;
+                });
+        });
+    });
+
+    describe('fn: permissible', function () {
         function getUserModel(id, role) {
             var hasRole = sandbox.stub();
 

core/test/utils/fixtures/data-generator.js

@@ -407,6 +407,8 @@ DataGenerator.forKnex = (function () {
     }
 
     function createPost(overrides) {
+        overrides = overrides || {};
+
         var newObj = _.cloneDeep(overrides),
             mobiledoc = JSON.parse(overrides.mobiledoc || '{}');
 

core/test/utils/fixtures/export/lts/export-003-missing-allowed-fields.json

@@ -0,0 +1,29 @@
+{
+    "meta": {
+        "exported_on": 1388318311015,
+        "version": "003"
+    },
+    "data": {
+        "users": [
+            {
+                "id": 2,
+                "uuid": "e5188224-4742-4c32-a2d6-e9c5c5d4c123",
+                "name": "Jimmy",
+                "slug": "jimmy",
+                "password": "$2a$10$.pZeeBE0gHXd0PTnbT/ph.GEKgd0Wd3q2pWna3ynTGBkPKnGIKABC",
+                "email": "jimmy@example.com",
+                "image": null,
+                "cover": null,
+                "status": "active",
+                "language": "en_US",
+                "meta_title": null,
+                "meta_description": null,
+                "last_login": null,
+                "created_at": "2017-07-17T12:02:54.000Z",
+                "created_by": 1,
+                "updated_at": null,
+                "updated_by": null
+            }
+        ]
+    }
+}

core/test/utils/mocks/knex.js

@@ -43,10 +43,10 @@ class KnexMock {
 
         if (options.autoMock) {
             this.tracker.on('query', (query) => {
+                query.sql = query.sql.replace(/`/g, '"');
+
                 if (query.method === 'select') {
                     if (query.bindings.length && query.sql.match(/where/)) {
-                        query.sql = query.sql.replace(/`/g, '"');
-
                         const tableName = query.sql.match(/from\s\"(\w+)\"/)[1],
                             where = query.sql.match(/\"(\w+)\"\s\=\s\?/)[1],
                             value = query.bindings[0],
@@ -66,8 +66,6 @@ class KnexMock {
                         query.response(this.db[tableName]);
                     }
                 } else if (query.method === 'insert') {
-                    query.sql = query.sql.replace(/`/g, '"');
-
                     const tableName = query.sql.match(/into\s\"(\w+)\"/)[1];
                     let keys = query.sql.match(/\(([^)]+)\)/)[1],
                         entry = {};
@@ -87,8 +85,6 @@ class KnexMock {
                     this.db[tableName].push(entry);
                     query.response(entry);
                 } else if (query.method === 'update') {
-                    query.sql = query.sql.replace(/`/g, '"');
-
                     const tableName = query.sql.match(/update\s\"(\w+)\"/)[1],
                         where = query.sql.match(/where\s\"(\w+)\"\s\=\s\?/)[1],
                         value = query.bindings.slice(-1)[0],