From 89bb3079bda9c190f07c80895b3a18d7869053a8 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Fri, 19 Oct 2018 11:56:51 -0700 Subject: [PATCH] Report circular or deep query objects --- lib/dao.js | 64 +++++----- lib/include.js | 3 +- lib/include_utils.js | 4 +- lib/utils.js | 91 +++++++------ test/model-definition.test.js | 234 ++++++++++++++++++---------------- test/util.test.js | 54 ++++++-- 6 files changed, 254 insertions(+), 196 deletions(-) diff --git a/lib/dao.js b/lib/dao.js index f119403b..c128b07b 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -26,7 +26,7 @@ var geo = require('./geo'); var Memory = require('./connectors/memory').Memory; var utils = require('./utils'); var fieldsToArray = utils.fieldsToArray; -var removeUndefined = utils.removeUndefined; +var sanitizeQueryOrData = utils.sanitizeQuery; var setScopeValuesFromWhere = utils.setScopeValuesFromWhere; var idEquals = utils.idEquals; var mergeQuery = utils.mergeQuery; @@ -406,7 +406,7 @@ DataAccessObject.create = function(data, options, cb) { obj.trigger('create', function(createDone) { obj.trigger('save', function(saveDone) { var _idName = idName(Model); - var val = removeUndefined(obj.toObject(true)); + var val = Model._sanitize(obj.toObject(true)); function createCallback(err, id, rev) { if (id) { obj.__data[_idName] = id; @@ -635,7 +635,7 @@ DataAccessObject.upsert = function(data, options, cb) { } function callConnector() { - update = removeUndefined(update); + update = Model._sanitize(update); context = { Model: Model, where: ctx.where, @@ -805,10 +805,10 @@ DataAccessObject.upsertWithWhere = function(where, data, options, cb) { try { // Support an optional where object var handleUndefined = Model._getSetting('normalizeUndefinedInQuery'); - // alter configuration of how removeUndefined handles undefined values - ctx.where = removeUndefined(ctx.where, handleUndefined); + // alter configuration of how sanitizeQuery handles undefined values + ctx.where = Model._sanitize(ctx.where, {handleUndefined: handleUndefined}); ctx.where = Model._coerce(ctx.where, options); - update = removeUndefined(update); + update = Model._sanitize(update); update = Model._coerce(update, options); } catch (err) { return process.nextTick(function() { @@ -989,7 +989,7 @@ DataAccessObject.replaceOrCreate = function replaceOrCreate(data, options, cb) { }, update, options); function callConnector() { - update = removeUndefined(update); + update = Model._sanitize(update); context = { Model: Model, where: where, @@ -1170,7 +1170,7 @@ DataAccessObject.findOrCreate = function findOrCreate(query, data, options, cb) }); } - data = removeUndefined(data); + data = Model._sanitize(data); var context = { Model: Model, where: query.where, @@ -1580,8 +1580,8 @@ DataAccessObject._normalize = function(filter, options) { } var handleUndefined = this._getSetting('normalizeUndefinedInQuery'); - // alter configuration of how removeUndefined handles undefined values - filter = removeUndefined(filter, handleUndefined); + // alter configuration of how sanitizeQuery handles undefined values + filter = this._sanitize(filter, {handleUndefined: handleUndefined}); this._coerce(filter.where, options); return filter; }; @@ -1664,6 +1664,18 @@ DataAccessObject._getProtectedProperties = function() { return result; }; +DataAccessObject._sanitize = function(query, options) { + options = options || {}; + + // Check violation of keys + var prohibitedKeys = this._getHiddenProperties(); + if (options.excludeProtectedProperties) { + prohibitedKeys = prohibitedKeys.concat(this._getProtectedProperties()); + } + return sanitizeQueryOrData(query, + Object.assign({prohibitedKeys: prohibitedKeys}, options)); +}; + /* * Coerce values based the property types * @param {Object} where The where clause @@ -1674,19 +1686,11 @@ DataAccessObject._getProtectedProperties = function() { */ DataAccessObject._coerce = function(where, options) { var self = this; - if (!where) { + if (where == null) { return where; } - options = options || {}; - // Check violation of keys - var prohibitedKeys = this._getHiddenProperties(); - if (options.excludeProtectedProperties) { - prohibitedKeys = prohibitedKeys.concat(this._getProtectedProperties()); - } - utils.validateKeys(where, prohibitedKeys); - var err; if (typeof where !== 'object' || Array.isArray(where)) { err = new Error(g.f('The where clause %j is not an {{object}}', where)); @@ -2322,8 +2326,8 @@ DataAccessObject.destroyAll = function destroyAll(where, options, cb) { try { // Support an optional where object var handleUndefined = Model._getSetting('normalizeUndefinedInQuery'); - // alter configuration of how removeUndefined handles undefined values - where = removeUndefined(where, handleUndefined); + // alter configuration of how sanitizeQuery handles undefined values + where = Model._sanitize(where, {handleUndefined: handleUndefined}); where = Model._coerce(where, options); } catch (err) { return process.nextTick(function() { @@ -2477,8 +2481,8 @@ DataAccessObject.count = function(where, options, cb) { try { var handleUndefined = Model._getSetting('normalizeUndefinedInQuery'); - // alter configuration of how removeUndefined handles undefined values - where = removeUndefined(where, handleUndefined); + // alter configuration of how sanitizeQuery handles undefined values + where = Model._sanitize(where, {handleUndefined: handleUndefined}); where = this._coerce(where, options); } catch (err) { process.nextTick(function() { @@ -2584,7 +2588,7 @@ DataAccessObject.prototype.save = function(options, cb) { function save() { inst.trigger('save', function(saveDone) { inst.trigger('update', function(updateDone) { - data = removeUndefined(data); + data = Model._sanitize(data); function saveCallback(err, unusedData, result) { if (err) { return cb(err, inst); @@ -2772,10 +2776,10 @@ DataAccessObject.updateAll = function(where, data, options, cb) { try { // Support an optional where object var handleUndefined = Model._getSetting('normalizeUndefinedInQuery'); - // alter configuration of how removeUndefined handles undefined values - where = removeUndefined(where, handleUndefined); + // alter configuration of how sanitizeQuery handles undefined values + where = Model._sanitize(where, {handleUndefined: handleUndefined}); where = Model._coerce(where, options); - data = removeUndefined(data); + data = Model._sanitize(data); data = Model._coerce(data, options); } catch (err) { return process.nextTick(function() { @@ -3108,7 +3112,7 @@ DataAccessObject.replaceById = function(id, data, options, cb) { function validateAndCallConnector(err, data) { if (err) return cb(err); - data = removeUndefined(data); + data = Model._sanitize(data); // update instance's properties inst.setAttributes(data); @@ -3260,7 +3264,7 @@ function(data, options, cb) { if (data instanceof Model) { data = data.toObject(false); } - data = removeUndefined(data); + data = Model._sanitize(data); // Make sure id(s) cannot be changed var idNames = Model.definition.idNames(); @@ -3333,7 +3337,7 @@ function(data, options, cb) { inst.trigger('update', function(done) { copyData(data, inst); var typedData = convertSubsetOfPropertiesByType(inst, data); - context.data = removeUndefined(typedData); + context.data = Model._sanitize(typedData); // Depending on the connector, the database response can // contain information about the updated record(s), but also diff --git a/lib/include.js b/lib/include.js index 18e94f37..04530236 100644 --- a/lib/include.js +++ b/lib/include.js @@ -205,7 +205,8 @@ Inclusion.include = function(objects, include, options, cb) { */ function findWithForeignKeysByPage(model, filter, fkName, pageSize, options, cb) { try { - model._coerce(filter.where, {excludeProtectedProperties: true}); + model._sanitize(filter.where, {excludeProtectedProperties: true}); + model._coerce(filter.where); } catch (e) { return cb(e); } diff --git a/lib/include_utils.js b/lib/include_utils.js index 70d70597..e08d722f 100644 --- a/lib/include_utils.js +++ b/lib/include_utils.js @@ -5,6 +5,8 @@ 'use strict'; +var g = require('strong-globalize')(); + module.exports.buildOneToOneIdentityMapWithOrigKeys = buildOneToOneIdentityMapWithOrigKeys; module.exports.buildOneToManyIdentityMapWithOrigKeys = buildOneToManyIdentityMapWithOrigKeys; module.exports.join = join; @@ -15,7 +17,7 @@ const util = require('util'); function getId(obj, idName) { var id = obj && obj[idName]; if (id == null) { - const msg = util.format('ID property "%s" is missing for included item: %j. ' + + const msg = g.f('ID property "%s" is missing for included item: %j. ' + 'Please make sure `fields` include "%s" if it\'s present in the `filter`', idName, obj, idName); const err = new Error(msg); diff --git a/lib/utils.js b/lib/utils.js index 4942f2f1..089cd077 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -7,7 +7,7 @@ exports.safeRequire = safeRequire; exports.fieldsToArray = fieldsToArray; exports.selectFields = selectFields; -exports.removeUndefined = removeUndefined; +exports.sanitizeQuery = sanitizeQuery; exports.parseSettings = parseSettings; exports.mergeSettings = exports.deepMerge = deepMerge; exports.deepMergeProperty = deepMergeProperty; @@ -26,7 +26,6 @@ exports.findIndexOf = findIndexOf; exports.collectTargetIds = collectTargetIds; exports.idName = idName; exports.rankArrayElements = rankArrayElements; -exports.validateKeys = validateKeys; var g = require('strong-globalize')(); var traverse = require('traverse'); @@ -304,18 +303,53 @@ function selectFields(fields) { } /** - * Remove undefined values from the query object - * @param query - * @param handleUndefined {String} either "nullify", "throw" or "ignore" (default: "ignore") - * @returns {exports.map|*} + * Sanitize the query object + * @param query {object} The query object + * @param options + * @property handleUndefined {String} either "nullify", "throw" or "ignore" (default: "ignore") + * @property prohibitedKeys {String[]} An array of prohibited keys to be removed + * @returns {*} */ -function removeUndefined(query, handleUndefined) { +function sanitizeQuery(query, options) { + debug('Sanitizing query object: %j', query); if (typeof query !== 'object' || query === null) { return query; } + options = options || {}; + if (typeof options === 'string') { + // Keep it backward compatible + options = {handleUndefined: options}; + } + const prohibitedKeys = options.prohibitedKeys; + const offendingKeys = []; + const handleUndefined = options.handleUndefined; + const maxDepth = options.maxDepth || 10; // WARNING: [rfeng] Use map() will cause mongodb to produce invalid BSON // as traverse doesn't transform the ObjectId correctly - return traverse(query).forEach(function(x) { + const result = traverse(query).forEach(function(x) { + /** + * Security risk if the client passes in a very deep where object + */ + if (this.level > maxDepth || this.circular) { + const msg = g.f('The query object is too deep or circular'); + const err = new Error(msg); + err.statusCode = 400; + err.code = 'WHERE_OBJECT_TOO_DEEP'; + throw err; + } + /** + * Make sure prohibited keys are removed from the query to prevent + * sensitive values from being guessed + */ + if (prohibitedKeys && prohibitedKeys.indexOf(this.key) !== -1) { + offendingKeys.push(this.key); + this.remove(); + return; + } + + /** + * Handle undefined values + */ if (x === undefined) { switch (handleUndefined) { case 'nullify': @@ -323,7 +357,6 @@ function removeUndefined(query, handleUndefined) { break; case 'throw': throw new Error(g.f('Unexpected `undefined` in query')); - break; case 'ignore': default: this.remove(); @@ -339,44 +372,20 @@ function removeUndefined(query, handleUndefined) { return x; }); -} -/** - * Check the where clause to report prohibited keys - * @param {object} where Where object - * @param {string[]} [prohibitedKeys] An array of prohibited keys - */ -function validateKeys(where, prohibitedKeys) { - if (!prohibitedKeys || prohibitedKeys.length === 0) return where; - if (typeof where !== 'object' || where === null) { - return where; - } - prohibitedKeys = prohibitedKeys || []; - const offendingKeys = []; - // WARNING: [rfeng] Use traverse.map() will cause mongodb to produce invalid BSON - // as traverse doesn't transform the ObjectId correctly. Instead we have to use - // traverse(where).forEach(...) - const result = traverse(where).forEach(function(x) { - if (prohibitedKeys.indexOf(this.key) !== -1) { - offendingKeys.push(this.key); - } - return x; - }); if (offendingKeys.length) { - const msg = 'Invalid properties are used in query'; - const err = new Error(msg); - err.code = 'PROPERTY_NOT_ALLOWED_IN_QUERY'; - err.statusCode = 400; - err.details = {where: where}; - debug('Hidden or protected properties %j are used in query: %j', - offendingKeys, where, err); - throw err; + console.error( + g.f( + 'Potential security alert: hidden/protected properties %j are used in query.', + offendingKeys + ) + ); } return result; } -var url = require('url'); -var qs = require('qs'); +const url = require('url'); +const qs = require('qs'); /** * Parse a URL into a settings object diff --git a/test/model-definition.test.js b/test/model-definition.test.js index e11dfead..e8082962 100644 --- a/test/model-definition.test.js +++ b/test/model-definition.test.js @@ -277,11 +277,14 @@ describe('ModelDefinition class', function() { Parent.create({ name: 'parent', }, function(err, parent) { + if (err) return done(err); parent.children.create({ name: 'child', protectedProperty: 'protectedValue', }, function(err, child) { + if (err) return done(err); Parent.find({include: 'children'}, function(err, parents) { + if (err) return done(err); var serialized = parents[0].toJSON(); var child = serialized.children[0]; assert.equal(child.name, 'child'); @@ -315,11 +318,14 @@ describe('ModelDefinition class', function() { Parent.create({ name: 'parent', }, function(err, parent) { + if (err) return done(err); parent.children.create({ name: 'child', secret: 'secret', }, function(err, child) { + if (err) return done(err); Parent.find({include: 'children'}, function(err, parents) { + if (err) return done(err); var serialized = parents[0].toJSON(); var child = serialized.children[0]; assert.equal(child.name, 'child'); @@ -330,134 +336,138 @@ describe('ModelDefinition class', function() { }); }); - function assertPropertyNotAllowed(err) { - should.exist(err); - err.message.should.match(/Invalid properties are used in query/); - err.code.should.equal('PROPERTY_NOT_ALLOWED_IN_QUERY'); - err.statusCode.should.equal(400); - err.details.should.have.property('where'); + describe('hidden properties', function() { + var Child; + beforeEach(givenChildren); + + it('should be removed if used in where', function() { + return Child.find({ + where: {secret: 'guess'}, + }).then(assertHiddenPropertyIsIgnored); + }); + + it('should be removed if used in where.and', function() { + return Child.find({ + where: {and: [{secret: 'guess'}]}, + }).then(assertHiddenPropertyIsIgnored); + }); + + /** + * Create two children with a hidden property, one with a matching + * value, the other with a non-matching value + */ + function givenChildren() { + Child = memory.createModel('child', {}, {hidden: ['secret']}); + return Child.create([{ + name: 'childA', + secret: 'secret', + }, { + name: 'childB', + secret: 'guess', + }]); + } + + function assertHiddenPropertyIsIgnored(children) { + // All children are found whether the `secret` condition matches or not + // as the condition is removed because it's hidden + children.length.should.equal(2); + } + }); + + function assertParentIncludeChildren(parents) { + parents[0].toJSON().children.length.should.equal(1); } - it('should report errors if a hidden property is used in where', function(done) { - var Child = memory.createModel('child', {}, {hidden: ['secret']}); - Child.create({ - name: 'child', - secret: 'secret', - }, function(err) { - if (err) return done(err); - Child.find({ - where: {secret: 'secret'}, - }, function(err) { - assertPropertyNotAllowed(err); - done(); - }); - }); - }); + describe('protected properties', function() { + var Parent; + var Child; + beforeEach(givenParentAndChild); - it('should report errors if a hidden property is used in where.and', function(done) { - var Child = memory.createModel('child', {}, {hidden: ['secret']}); - Child.create({ - name: 'child', - secret: 'secret', - }, function(err) { - if (err) return done(err); - Child.find({ - where: {and: [{secret: 'secret'}]}, - }, function(err) { - assertPropertyNotAllowed(err); - done(); - }); - }); - }); - - it('should report errors if a protected property is used in include scope', function(done) { - var Parent = memory.createModel('parent'); - var Child = memory.createModel('child', {}, {protected: ['secret']}); - Parent.hasMany(Child); - Parent.create({ - name: 'parent', - }, function(err, parent) { - if (err) return done(err); - parent.children.create({ - name: 'child', - secret: 'secret', - }, function(err) { - if (err) return done(err); - Parent.find({ - include: { - relation: 'children', - scope: { - where: { - secret: 'x', - }, + it('should be removed if used in include scope', function() { + Parent.find({ + include: { + relation: 'children', + scope: { + where: { + secret: 'x', }, }, - }, function(err) { - assertPropertyNotAllowed(err); - done(); - }); - }); + }, + }).then(assertParentIncludeChildren); }); - }); - it('should report errors if a protected property is used in include scope.where.and', function(done) { - var Parent = memory.createModel('parent'); - var Child = memory.createModel('child', {}, {protected: ['secret']}); - Parent.hasMany(Child); - Parent.create({ - name: 'parent', - }, function(err, parent) { - if (err) return done(err); - parent.children.create({ - name: 'child', - secret: 'secret', - }, function(err) { - if (err) return done(err); - Parent.find({ - include: { - relation: 'children', - scope: { - where: { - and: [{secret: 'x'}], - }, + it('should be rejected if used in include scope.where.and', function() { + return Parent.find({ + include: { + relation: 'children', + scope: { + where: { + and: [{secret: 'x'}], }, }, - }, function(err) { - assertPropertyNotAllowed(err); - done(); - }); - }); + }, + }).then(assertParentIncludeChildren); }); - }); - it('should report errors if a hidden property is used in include scope', function(done) { - var Parent = memory.createModel('parent'); - var Child = memory.createModel('child', {}, {hidden: ['secret']}); - Parent.hasMany(Child); - Parent.create({ - name: 'parent', - }, function(err, parent) { - if (err) return done(err); - parent.children.create({ - name: 'child', - secret: 'secret', - }, function(err) { - if (err) return done(err); - Parent.find({ - include: { - relation: 'children', - scope: { - where: { - secret: 'x', - }, + it('should be removed if a hidden property is used in include scope', function() { + return Parent.find({ + include: { + relation: 'children', + scope: { + where: { + secret: 'x', }, }, - }, function(err) { - assertPropertyNotAllowed(err); - done(); + }, + }).then(assertParentIncludeChildren); + }); + + function givenParentAndChild() { + Parent = memory.createModel('parent'); + Child = memory.createModel('child', {}, {protected: ['secret']}); + Parent.hasMany(Child); + return Parent.create({ + name: 'parent', + }).then(parent => { + return parent.children.create({ + name: 'child', + secret: 'secret', }); }); + } + }); + + describe('hidden properties in include', function() { + var Parent; + var Child; + beforeEach(givenParentAndChildWithHiddenProperty); + + it('should be rejected if used in scope', function() { + return Parent.find({ + include: { + relation: 'children', + scope: { + where: { + secret: 'x', + }, + }, + }, + }).then(assertParentIncludeChildren); }); + + function givenParentAndChildWithHiddenProperty() { + Parent = memory.createModel('parent'); + Child = memory.createModel('child', {}, {hidden: ['secret']}); + Parent.hasMany(Child); + return Parent.create({ + name: 'parent', + }).then(parent => { + return parent.children.create({ + name: 'child', + secret: 'secret', + }); + }); + } }); it('should throw error for property names containing dot', function() { diff --git a/test/util.test.js b/test/util.test.js index 9da2d512..59587971 100644 --- a/test/util.test.js +++ b/test/util.test.js @@ -8,7 +8,7 @@ var should = require('./init.js'); var utils = require('../lib/utils'); var ObjectID = require('bson').ObjectID; var fieldsToArray = utils.fieldsToArray; -var removeUndefined = utils.removeUndefined; +var sanitizeQuery = utils.sanitizeQuery; var deepMerge = utils.deepMerge; var rankArrayElements = utils.rankArrayElements; var mergeIncludes = utils.mergeIncludes; @@ -52,33 +52,65 @@ describe('util.fieldsToArray', function() { }); }); -describe('util.removeUndefined', function() { +describe('util.sanitizeQuery', function() { it('Remove undefined values from the query object', function() { var q1 = {where: {x: 1, y: undefined}}; - should.deepEqual(removeUndefined(q1), {where: {x: 1}}); + should.deepEqual(sanitizeQuery(q1), {where: {x: 1}}); var q2 = {where: {x: 1, y: 2}}; - should.deepEqual(removeUndefined(q2), {where: {x: 1, y: 2}}); + should.deepEqual(sanitizeQuery(q2), {where: {x: 1, y: 2}}); var q3 = {where: {x: 1, y: {in: [2, undefined]}}}; - should.deepEqual(removeUndefined(q3), {where: {x: 1, y: {in: [2]}}}); + should.deepEqual(sanitizeQuery(q3), {where: {x: 1, y: {in: [2]}}}); - should.equal(removeUndefined(null), null); + should.equal(sanitizeQuery(null), null); - should.equal(removeUndefined(undefined), undefined); + should.equal(sanitizeQuery(undefined), undefined); - should.equal(removeUndefined('x'), 'x'); + should.equal(sanitizeQuery('x'), 'x'); var date = new Date(); var q4 = {where: {x: 1, y: date}}; - should.deepEqual(removeUndefined(q4), {where: {x: 1, y: date}}); + should.deepEqual(sanitizeQuery(q4), {where: {x: 1, y: date}}); // test handling of undefined var q5 = {where: {x: 1, y: undefined}}; - should.deepEqual(removeUndefined(q5, 'nullify'), {where: {x: 1, y: null}}); + should.deepEqual(sanitizeQuery(q5, 'nullify'), {where: {x: 1, y: null}}); + + q5 = {where: {x: 1, y: undefined}}; + should.deepEqual(sanitizeQuery(q5, {handleUndefined: 'nullify'}), {where: {x: 1, y: null}}); var q6 = {where: {x: 1, y: undefined}}; - (function() { removeUndefined(q6, 'throw'); }).should.throw(/`undefined` in query/); + (function() { sanitizeQuery(q6, 'throw'); }).should.throw(/`undefined` in query/); + }); + + it('Report errors for circular or deep query objects', function() { + var q7 = {where: {x: 1}}; + q7.where.y = q7; + (function() { sanitizeQuery(q7); }).should.throw( + /The query object is too deep or circular/ + ); + + var q8 = {where: {and: [{and: [{and: [{and: [{and: [{and: + [{and: [{and: [{and: [{x: 1}]}]}]}]}]}]}]}]}]}}; + (function() { sanitizeQuery(q8); }).should.throw( + /The query object is too deep or circular/ + ); + + var q9 = {where: {and: [{and: [{and: [{and: [{x: 1}]}]}]}]}}; + (function() { sanitizeQuery(q8, {maxDepth: 4}); }).should.throw( + /The query object is too deep or circular/ + ); + }); + + it('Removed prohibited properties in query objects', function() { + var q1 = {where: {secret: 'guess'}}; + sanitizeQuery(q1, {prohibitedKeys: ['secret']}); + q1.where.should.eql({}); + + var q2 = {and: [{secret: 'guess'}, {x: 1}]}; + sanitizeQuery(q2, {prohibitedKeys: ['secret']}); + q2.should.eql({and: [{}, {x: 1}]}); }); });