From 13229fb6a47785841e16857ae89ba012318da4d2 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Thu, 26 Jun 2014 04:52:04 +0000 Subject: [PATCH] Fix server-side validation Closes #3122 -Fix validation so that all values are validated instead of just values that evaluate to true. -Ensure validation methods consistently return promises and switch error handling from try/catch to promise.catch to get rid of unhandled rejection warnings. -Add 0 and 1 to list of acceptable values in boolean validation. --- core/client/controllers/debug.js | 2 +- core/server/api/db.js | 12 +++++------- core/server/data/schema.js | 10 +++++----- core/server/data/validation/index.js | 13 ++++++++++--- core/server/models/settings.js | 2 +- 5 files changed, 22 insertions(+), 17 deletions(-) diff --git a/core/client/controllers/debug.js b/core/client/controllers/debug.js index 9089808783..03e9b9a56a 100644 --- a/core/client/controllers/debug.js +++ b/core/client/controllers/debug.js @@ -24,7 +24,7 @@ var DebugController = Ember.Controller.extend(Ember.Evented, { }).then(function () { self.notifications.showSuccess('Import successful.'); }).catch(function (response) { - self.notifications.showErrors(response); + self.notifications.showAPIError(response); }).finally(function () { self.set('uploadButtonText', 'Import'); self.trigger('reset'); diff --git a/core/server/api/db.js b/core/server/api/db.js index 5f993130f4..dd374f9124 100644 --- a/core/server/api/db.js +++ b/core/server/api/db.js @@ -101,16 +101,14 @@ db = { _.each(_.keys(importData.data), function (tableName) { _.each(importData.data[tableName], function (importValues) { - try { - validation.validateSchema(tableName, importValues); - } catch (err) { - error += error !== "" ? "
" : ""; + validation.validateSchema(tableName, importValues).catch(function (err) { + error += error !== '' ? '
' : ''; error += err.message; - } + }); }); }); - if (error !== "") { + if (error !== '') { return when.reject(new Error(error)); } // Import for the current version @@ -119,7 +117,7 @@ db = { }).then(function importSuccess() { return api.settings.updateSettingsCache(); }).then(function () { - return when.resolve({ message: 'Import successful', db: [] }); + return when.resolve({ db: [] }); }).otherwise(function importFailure(error) { return when.reject(new errors.InternalServerError(error.message || error)); }).finally(function () { diff --git a/core/server/data/schema.js b/core/server/data/schema.js index 835c848ce6..f03ce48298 100644 --- a/core/server/data/schema.js +++ b/core/server/data/schema.js @@ -7,8 +7,8 @@ var db = { markdown: {type: 'text', maxlength: 16777215, fieldtype: 'medium', nullable: true}, html: {type: 'text', maxlength: 16777215, fieldtype: 'medium', nullable: true}, image: {type: 'text', maxlength: 2000, nullable: true}, - featured: {type: 'bool', nullable: false, defaultTo: false, validations: {'isIn': [[false, true]]}}, - page: {type: 'bool', nullable: false, defaultTo: false, validations: {'isIn': [[false, true]]}}, + featured: {type: 'bool', nullable: false, defaultTo: false, validations: {'isIn': [[0, 1, false, true]]}}, + page: {type: 'bool', nullable: false, defaultTo: false, validations: {'isIn': [[0, 1, false, true]]}}, status: {type: 'string', maxlength: 150, nullable: false, defaultTo: 'draft'}, language: {type: 'string', maxlength: 6, nullable: false, defaultTo: 'en_US'}, meta_title: {type: 'string', maxlength: 150, nullable: true}, @@ -118,7 +118,7 @@ var db = { }, apps: { id: {type: 'increments', nullable: false, primary: true}, - uuid: {type: 'string', maxlength: 36, nullable: false}, + uuid: {type: 'string', maxlength: 36, nullable: false, validations: {'isUUID': true}}, name: {type: 'string', maxlength: 150, nullable: false, unique: true}, slug: {type: 'string', maxlength: 150, nullable: false, unique: true}, version: {type: 'string', maxlength: 150, nullable: false}, @@ -130,7 +130,7 @@ var db = { }, app_settings: { id: {type: 'increments', nullable: false, primary: true}, - uuid: {type: 'string', maxlength: 36, nullable: false}, + uuid: {type: 'string', maxlength: 36, nullable: false, validations: {'isUUID': true}}, key: {type: 'string', maxlength: 150, nullable: false, unique: true}, value: {type: 'text', maxlength: 65535, nullable: true}, app_id: {type: 'integer', nullable: false, unsigned: true, references: 'apps.id'}, @@ -141,7 +141,7 @@ var db = { }, app_fields: { id: {type: 'increments', nullable: false, primary: true}, - uuid: {type: 'string', maxlength: 36, nullable: false}, + uuid: {type: 'string', maxlength: 36, nullable: false, validations: {'isUUID': true}}, key: {type: 'string', maxlength: 150, nullable: false}, value: {type: 'text', maxlength: 65535, nullable: true}, type: {type: 'string', maxlength: 150, nullable: false, defaultTo: 'html'}, diff --git a/core/server/data/validation/index.js b/core/server/data/validation/index.js index 49a34b99e4..c5e24f6674 100644 --- a/core/server/data/validation/index.js +++ b/core/server/data/validation/index.js @@ -27,6 +27,7 @@ validateSchema = function (tableName, model) { _.each(columns, function (columnKey) { var message = ''; + // check nullable if (model.hasOwnProperty(columnKey) && schema[tableName][columnKey].hasOwnProperty('nullable') && schema[tableName][columnKey].nullable !== true) { @@ -35,8 +36,9 @@ validateSchema = function (tableName, model) { validationErrors.push(new errors.ValidationError(message, tableName + '.' + columnKey)); } } + // TODO: check if mandatory values should be enforced - if (model[columnKey]) { + if (model[columnKey] !== null && model[columnKey] !== undefined) { // check length if (schema[tableName][columnKey].hasOwnProperty('maxlength')) { if (!validator.isLength(model[columnKey], 0, schema[tableName][columnKey].maxlength)) { @@ -54,7 +56,7 @@ validateSchema = function (tableName, model) { //check type if (schema[tableName][columnKey].hasOwnProperty('type')) { if (schema[tableName][columnKey].type === 'integer' && !validator.isInt(model[columnKey])) { - message = 'Value in [' + tableName + '.' + columnKey + '] is no valid integer.'; + message = 'Value in [' + tableName + '.' + columnKey + '] is not an integer.'; validationErrors.push(new errors.ValidationError(message, tableName + '.' + columnKey)); } } @@ -64,6 +66,8 @@ validateSchema = function (tableName, model) { if (validationErrors.length !== 0) { return when.reject(validationErrors); } + + return when.resolve(); }; // Validation for settings @@ -81,6 +85,8 @@ validateSettings = function (defaultSettings, model) { if (validationErrors.length !== 0) { return when.reject(validationErrors); } + + return when.resolve(); }; // Validate default settings using the validator module. @@ -102,6 +108,7 @@ validateSettings = function (defaultSettings, model) { // available validators: https://github.com/chriso/validator.js#validators validate = function (value, key, validations) { var validationErrors = []; + _.each(validations, function (validationOptions, validationName) { var goodResult = true; @@ -116,7 +123,7 @@ validate = function (value, key, validations) { // equivalent of validator.isSomething(option1, option2) if (validator[validationName].apply(validator, validationOptions) !== goodResult) { - validationErrors.push(new errors.ValidationError('Settings validation (' + validationName + ') failed for ' + key, key)); + validationErrors.push(new errors.ValidationError('Validation (' + validationName + ') failed for ' + key, key)); } validationOptions.shift(); diff --git a/core/server/models/settings.js b/core/server/models/settings.js index bc509adea0..af0727870e 100644 --- a/core/server/models/settings.js +++ b/core/server/models/settings.js @@ -49,7 +49,7 @@ Settings = ghostBookshelf.Model.extend({ validate: function () { var self = this; - return when(validation.validateSchema(self.tableName, self.toJSON())).then(function () { + return validation.validateSchema(self.tableName, self.toJSON()).then(function () { return validation.validateSettings(getDefaultSettings(), self); }); },