From 87cda81c8475fb9213d94ac352c3d2d78639c268 Mon Sep 17 00:00:00 2001 From: David Arvelo Date: Mon, 5 May 2014 21:45:08 -0400 Subject: [PATCH] Sanitize models' attributes/options before passing to bookshelf/knex closes #2653 - enforce strict whitelists for model methods - create a class method that reports a model method's valid options - create a class method that filters a model's valid attributes from data - create a class method that filters valid options from a model method's options hash --- core/server/models/app.js | 21 ++++++++++++++ core/server/models/base.js | 48 +++++++++++++++++++++++++++---- core/server/models/permission.js | 21 ++++++++++++++ core/server/models/post.js | 49 ++++++++++++++++++++++++++++---- core/server/models/role.js | 22 ++++++++++++++ core/server/models/session.js | 2 +- core/server/models/settings.js | 27 ++++++++++++++++++ core/server/models/tag.js | 21 ++++++++++++++ core/server/models/user.js | 31 ++++++++++++++++++-- 9 files changed, 228 insertions(+), 14 deletions(-) diff --git a/core/server/models/app.js b/core/server/models/app.js index 612ce95d8b..ee83b78f3b 100644 --- a/core/server/models/app.js +++ b/core/server/models/app.js @@ -14,6 +14,27 @@ App = ghostBookshelf.Model.extend({ settings: function () { return this.belongsToMany(AppSetting, 'app_settings'); } +}, { + /** + * Returns an array of keys permitted in a method's `options` hash, depending on the current method. + * @param {String} methodName The name of the method to check valid options for. + * @return {Array} Keys allowed in the `options` hash of the model's method. + */ + permittedOptions: function (methodName) { + var options = ghostBookshelf.Model.permittedOptions(), + + // whitelists for the `options` hash argument on methods, by method name. + // these are the only options that can be passed to Bookshelf / Knex. + validOptions = { + findOne: ['withRelated'] + }; + + if (validOptions[methodName]) { + options = options.concat(validOptions[methodName]); + } + + return options; + } }); Apps = ghostBookshelf.Collection.extend({ diff --git a/core/server/models/base.js b/core/server/models/base.js index eb1a1c3afb..942142525b 100644 --- a/core/server/models/base.js +++ b/core/server/models/base.js @@ -156,6 +156,41 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ // ## Model Data Functions + /** + * Returns an array of keys permitted in every method's `options` hash. + * Can be overridden and added to by a model's `permittedOptions` method. + * @return {Array} Keys allowed in the `options` hash of every model's method. + */ + permittedOptions: function () { + // terms to whitelist for all methods. + return ['include', 'transacting']; + }, + + /** + * Filters potentially unsafe model attributes, so you can pass them to Bookshelf / Knex. + * @param {Object} data Has keys representing the model's attributes/fields in the database. + * @return {Object} The filtered results of the passed in data, containing only what's allowed in the schema. + */ + filterData: function (data) { + var permittedAttributes = this.prototype.permittedAttributes(), + filteredData = _.pick(data, permittedAttributes); + + return filteredData; + }, + + /** + * Filters potentially unsafe `options` in a model method's arguments, so you can pass them to Bookshelf / Knex. + * @param {Object} options Represents options to filter in order to be passed to the Bookshelf query. + * @param {String} methodName The name of the method to check valid options for. + * @return {Object} The filtered results of `options`. + */ + filterOptions: function (options, methodName) { + var permittedOptions = this.permittedOptions(methodName), + filteredOptions = _.pick(options, permittedOptions); + + return filteredOptions; + }, + /** * ### Find All * Naive find all fetches all the data for a particular model @@ -163,7 +198,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * @return {Promise(ghostBookshelf.Collection)} Collection of all Models */ findAll: function (options) { - options = options || {}; + options = this.filterOptions(options, 'findAll'); return ghostBookshelf.Collection.forge([], {model: this}).fetch(options).then(function (result) { if (options.include) { _.each(result.models, function (item) { @@ -182,7 +217,8 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * @return {Promise(ghostBookshelf.Model)} Single Model */ findOne: function (data, options) { - options = options || {}; + data = this.filterData(data); + options = this.filterOptions(options, 'findOne'); return this.forge(data, {include: options.include}).fetch(options); }, @@ -194,7 +230,8 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * @return {Promise(ghostBookshelf.Model)} Edited Model */ edit: function (data, options) { - options = options || {}; + data = this.filterData(data); + options = this.filterOptions(options, 'edit'); return this.forge({id: data.id}).fetch(options).then(function (object) { if (object) { return object.save(data, options); @@ -210,7 +247,8 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * @return {Promise(ghostBookshelf.Model)} Newly Added Model */ add: function (data, options) { - options = options || {}; + data = this.filterData(data); + options = this.filterOptions(options, 'add'); var instance = this.forge(data); // We allow you to disable timestamps // when importing posts so that @@ -231,7 +269,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * @return {Promise(ghostBookshelf.Model)} Empty Model */ destroy: function (data, options) { - options = options || {}; + options = this.filterOptions(options, 'destroy'); return this.forge({id: data}).destroy(options); }, diff --git a/core/server/models/permission.js b/core/server/models/permission.js index 6e13469ee1..034ea82da7 100644 --- a/core/server/models/permission.js +++ b/core/server/models/permission.js @@ -21,6 +21,27 @@ Permission = ghostBookshelf.Model.extend({ apps: function () { return this.belongsToMany(App); } +}, { + /** + * Returns an array of keys permitted in a method's `options` hash, depending on the current method. + * @param {String} methodName The name of the method to check valid options for. + * @return {Array} Keys allowed in the `options` hash of the model's method. + */ + permittedOptions: function (methodName) { + var options = ghostBookshelf.Model.permittedOptions(), + + // whitelists for the `options` hash argument on methods, by method name. + // these are the only options that can be passed to Bookshelf / Knex. + validOptions = { + add: ['user'] + }; + + if (validOptions[methodName]) { + options = options.concat(validOptions[methodName]); + } + + return options; + }, }); Permissions = ghostBookshelf.Collection.extend({ diff --git a/core/server/models/post.js b/core/server/models/post.js index 1c81f94bb9..51ca73bd04 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -232,6 +232,47 @@ Post = ghostBookshelf.Model.extend({ }, { + /** + * Returns an array of keys permitted in a method's `options` hash, depending on the current method. + * @param {String} methodName The name of the method to check valid options for. + * @return {Array} Keys allowed in the `options` hash of the model's method. + */ + permittedOptions: function (methodName) { + var options = ghostBookshelf.Model.permittedOptions(), + + // whitelists for the `options` hash argument on methods, by method name. + // these are the only options that can be passed to Bookshelf / Knex. + validOptions = { + findAll: ['withRelated'], + findOne: ['user', 'importing', 'withRelated'], + findPage: ['page', 'limit', 'status', 'staticPages'], + add: ['user', 'importing'], + edit: ['user'] + }; + + if (validOptions[methodName]) { + options = options.concat(validOptions[methodName]); + } + + return options; + }, + + /** + * Filters potentially unsafe model attributes, so you can pass them to Bookshelf / Knex. + * @param {Object} data Has keys representing the model's attributes/fields in the database. + * @return {Object} The filtered results of the passed in data, containing only what's allowed in the schema. + */ + filterData: function (data) { + var permittedAttributes = this.prototype.permittedAttributes(), + filteredData; + + // manually add 'tags' attribute since it's not in the schema + permittedAttributes.push('tags'); + + filteredData = _.pick(data, permittedAttributes); + + return filteredData; + }, // #### findAll // Extends base model findAll to eager-fetch author and user relationships. @@ -266,11 +307,9 @@ Post = ghostBookshelf.Model.extend({ options = options || {}; var postCollection = Posts.forge(), - tagInstance = options.tag !== undefined ? Tag.forge({slug: options.tag}) : false, - permittedOptions = ['page', 'limit', 'status', 'staticPages', 'include']; + tagInstance = options.tag !== undefined ? Tag.forge({slug: options.tag}) : false; - // sanitize options so we are not automatically passing through any and all query strings to Bookshelf / Knex. - options = _.pick(options, permittedOptions); + options = this.filterOptions(options, 'findPage'); // Set default settings for options options = _.extend({ @@ -451,7 +490,7 @@ Post = ghostBookshelf.Model.extend({ }); }, destroy: function (_identifier, options) { - options = options || {}; + options = this.filterOptions(options, 'destroy'); return this.forge({id: _identifier}).fetch({withRelated: ['tags']}).then(function destroyTags(post) { var tagIds = _.pluck(post.related('tags').toJSON(), 'id'); diff --git a/core/server/models/role.js b/core/server/models/role.js index f9d134e441..d6b2eb93c6 100644 --- a/core/server/models/role.js +++ b/core/server/models/role.js @@ -16,6 +16,28 @@ Role = ghostBookshelf.Model.extend({ permissions: function () { return this.belongsToMany(Permission); } +}, { + /** + * Returns an array of keys permitted in a method's `options` hash, depending on the current method. + * @param {String} methodName The name of the method to check valid options for. + * @return {Array} Keys allowed in the `options` hash of the model's method. + */ + permittedOptions: function (methodName) { + var options = ghostBookshelf.Model.permittedOptions(), + + // whitelists for the `options` hash argument on methods, by method name. + // these are the only options that can be passed to Bookshelf / Knex. + validOptions = { + findOne: ['withRelated'], + add: ['user'] + }; + + if (validOptions[methodName]) { + options = options.concat(validOptions[methodName]); + } + + return options; + }, }); Roles = ghostBookshelf.Collection.extend({ diff --git a/core/server/models/session.js b/core/server/models/session.js index 25f204ab19..81dfcee78f 100644 --- a/core/server/models/session.js +++ b/core/server/models/session.js @@ -23,7 +23,7 @@ Session = ghostBookshelf.Model.extend({ }, { destroyAll: function (options) { - options = options || {}; + options = this.filterOptions(options, 'destroyAll'); return ghostBookshelf.Collection.forge([], {model: this}).fetch() .then(function (collection) { collection.invokeThen('destroy', options); diff --git a/core/server/models/settings.js b/core/server/models/settings.js index 0278d0dde0..5c52478e47 100644 --- a/core/server/models/settings.js +++ b/core/server/models/settings.js @@ -58,6 +58,28 @@ Settings = ghostBookshelf.Model.extend({ } }, { + /** + * Returns an array of keys permitted in a method's `options` hash, depending on the current method. + * @param {String} methodName The name of the method to check valid options for. + * @return {Array} Keys allowed in the `options` hash of the model's method. + */ + permittedOptions: function (methodName) { + var options = ghostBookshelf.Model.permittedOptions(), + + // whitelists for the `options` hash argument on methods, by method name. + // these are the only options that can be passed to Bookshelf / Knex. + validOptions = { + add: ['user'], + edit: ['user'] + }; + + if (validOptions[methodName]) { + options = options.concat(validOptions[methodName]); + } + + return options; + }, + findOne: function (_key) { // Allow for just passing the key instead of attributes if (!_.isObject(_key)) { @@ -67,6 +89,8 @@ Settings = ghostBookshelf.Model.extend({ }, edit: function (_data, options) { + var self = this; + options = this.filterOptions(options, 'edit'); if (!Array.isArray(_data)) { _data = [_data]; @@ -78,6 +102,9 @@ Settings = ghostBookshelf.Model.extend({ if (!(_.isString(item.key) && item.key.length > 0)) { return when.reject({type: 'ValidationError', message: 'Setting key cannot be empty.'}); } + + item = self.filterData(item); + return Settings.forge({ key: item.key }).fetch(options).then(function (setting) { if (setting) { diff --git a/core/server/models/tag.js b/core/server/models/tag.js index 743910b7b5..7ed3af746b 100644 --- a/core/server/models/tag.js +++ b/core/server/models/tag.js @@ -37,6 +37,27 @@ Tag = ghostBookshelf.Model.extend({ return attrs; } +}, { + /** + * Returns an array of keys permitted in a method's `options` hash, depending on the current method. + * @param {String} methodName The name of the method to check valid options for. + * @return {Array} Keys allowed in the `options` hash of the model's method. + */ + permittedOptions: function (methodName) { + var options = ghostBookshelf.Model.permittedOptions(), + + // whitelists for the `options` hash argument on methods, by method name. + // these are the only options that can be passed to Bookshelf / Knex. + validOptions = { + add: ['user'] + }; + + if (validOptions[methodName]) { + options = options.concat(validOptions[methodName]); + } + + return options; + }, }); Tags = ghostBookshelf.Collection.extend({ diff --git a/core/server/models/user.js b/core/server/models/user.js index 7f07dcd09e..25e59e0c0c 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -65,7 +65,7 @@ User = ghostBookshelf.Model.extend({ var attrs = ghostBookshelf.Model.prototype.toJSON.call(this, options); // remove password hash for security reasons delete attrs.password; - + return attrs; }, @@ -82,6 +82,28 @@ User = ghostBookshelf.Model.extend({ } }, { + /** + * Returns an array of keys permitted in a method's `options` hash, depending on the current method. + * @param {String} methodName The name of the method to check valid options for. + * @return {Array} Keys allowed in the `options` hash of the model's method. + */ + permittedOptions: function (methodName) { + var options = ghostBookshelf.Model.permittedOptions(), + + // whitelists for the `options` hash argument on methods, by method name. + // these are the only options that can be passed to Bookshelf / Knex. + validOptions = { + findOne: ['withRelated'], + add: ['user'], + edit: ['user'] + }; + + if (validOptions[methodName]) { + options = options.concat(validOptions[methodName]); + } + + return options; + }, /** * Naive user add @@ -93,7 +115,10 @@ User = ghostBookshelf.Model.extend({ var self = this, // Clone the _user so we don't expose the hashed password unnecessarily - userData = _.extend({}, _user); + userData = this.filterData(_user); + + options = this.filterOptions(options, 'add'); + /** * This only allows one user to be added to the database, otherwise fails. * @param {object} user @@ -314,7 +339,7 @@ User = ghostBookshelf.Model.extend({ var diff = 0, i; - // check if the token lenght is correct + // check if the token length is correct if (token.length !== generatedToken.length) { diff = 1; }