Report circular or deep query objects

This commit is contained in:
Raymond Feng 2018-10-19 11:56:51 -07:00
parent 71f2e54de5
commit 89bb3079bd
6 changed files with 254 additions and 196 deletions

View File

@ -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

View File

@ -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);
}

View File

@ -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);

View File

@ -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

View File

@ -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,59 +336,54 @@ 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',
}]);
}
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();
});
});
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);
}
});
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();
});
});
});
function assertParentIncludeChildren(parents) {
parents[0].toJSON().children.length.should.equal(1);
}
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);
describe('protected properties', function() {
var Parent;
var Child;
beforeEach(givenParentAndChild);
it('should be removed if used in include scope', function() {
Parent.find({
include: {
relation: 'children',
@ -392,28 +393,11 @@ describe('ModelDefinition class', function() {
},
},
},
}, 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({
it('should be rejected if used in include scope.where.and', function() {
return Parent.find({
include: {
relation: 'children',
scope: {
@ -422,28 +406,11 @@ describe('ModelDefinition class', function() {
},
},
},
}, 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({
it('should be removed if a hidden property is used in include scope', function() {
return Parent.find({
include: {
relation: 'children',
scope: {
@ -452,12 +419,55 @@ describe('ModelDefinition class', function() {
},
},
},
}, 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() {

View File

@ -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}]});
});
});