From b5a2042d1287198e491ebb5c3ac871e0fb7fad17 Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Mon, 22 May 2017 12:34:00 -0400 Subject: [PATCH] Fixes issue #3835 affecting relation updates (#3836) * Adds test for 3835 * Makes sure we run relational updates AFTER validating access to the object * Always run relation udpates last --- spec/ParseRole.spec.js | 20 ++++++++ src/Controllers/DatabaseController.js | 68 +++++++++++++++++++-------- 2 files changed, 68 insertions(+), 20 deletions(-) diff --git a/spec/ParseRole.spec.js b/spec/ParseRole.spec.js index c61c4aed..73e085ac 100644 --- a/spec/ParseRole.spec.js +++ b/spec/ParseRole.spec.js @@ -428,4 +428,24 @@ describe('Parse Role testing', () => { }); }); + it('should be secure (#3835)', (done) => { + const acl = new Parse.ACL(); + acl.getPublicReadAccess(true); + const role = new Parse.Role('admin', acl); + role.save().then(() => { + const user = new Parse.User(); + return user.signUp({username: 'hello', password: 'world'}); + }).then((user) => { + role.getUsers().add(user) + return role.save(); + }).then(done.fail, () => { + const query = role.getUsers().query(); + return query.find({useMasterKey: true}); + }).then((results) => { + expect(results.length).toBe(0); + done(); + }) + .catch(done.fail); + }); + }); diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index db633981..40cbeddf 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -235,17 +235,18 @@ DatabaseController.prototype.update = function(className, query, update, { many, upsert, } = {}, skipSanitization = false) { + const originalQuery = query; const originalUpdate = update; // Make a copy of the object, so we don't mutate the incoming data. update = deepcopy(update); - + var relationUpdates = []; var isMaster = acl === undefined; var aclGroup = acl || []; return this.loadSchema() .then(schemaController => { return (isMaster ? Promise.resolve() : schemaController.validatePermission(className, aclGroup, 'update')) - .then(() => this.handleRelationUpdates(className, query.objectId, update)) .then(() => { + relationUpdates = this.collectRelationUpdates(className, originalQuery.objectId, update); if (!isMaster) { query = this.addPointerPermissions(schemaController, className, 'update', query, aclGroup); } @@ -295,6 +296,10 @@ DatabaseController.prototype.update = function(className, query, update, { if (!result) { return Promise.reject(new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'Object not found.')); } + return this.handleRelationUpdates(className, originalQuery.objectId, update, relationUpdates).then(() => { + return result; + }); + }).then((result) => { if (skipSanitization) { return Promise.resolve(result); } @@ -320,12 +325,11 @@ function sanitizeDatabaseResult(originalObject, result) { return Promise.resolve(response); } -// Processes relation-updating operations from a REST-format update. -// Returns a promise that resolves successfully when these are -// processed. +// Collect all relation-updating operations from a REST-format update. +// Returns a list of all relation updates to perform // This mutates update. -DatabaseController.prototype.handleRelationUpdates = function(className, objectId, update) { - var pending = []; +DatabaseController.prototype.collectRelationUpdates = function(className, objectId, update) { + var ops = []; var deleteMe = []; objectId = update.objectId || objectId; @@ -334,20 +338,12 @@ DatabaseController.prototype.handleRelationUpdates = function(className, objectI return; } if (op.__op == 'AddRelation') { - for (const object of op.objects) { - pending.push(this.addRelation(key, className, - objectId, - object.objectId)); - } + ops.push({key, op}); deleteMe.push(key); } if (op.__op == 'RemoveRelation') { - for (const object of op.objects) { - pending.push(this.removeRelation(key, className, - objectId, - object.objectId)); - } + ops.push({key, op}); deleteMe.push(key); } @@ -364,6 +360,35 @@ DatabaseController.prototype.handleRelationUpdates = function(className, objectI for (const key of deleteMe) { delete update[key]; } + return ops; +} + +// Processes relation-updating operations from a REST-format update. +// Returns a promise that resolves when all updates have been performed +DatabaseController.prototype.handleRelationUpdates = function(className, objectId, update, ops) { + var pending = []; + objectId = update.objectId || objectId; + ops.forEach(({key, op}) => { + if (!op) { + return; + } + if (op.__op == 'AddRelation') { + for (const object of op.objects) { + pending.push(this.addRelation(key, className, + objectId, + object.objectId)); + } + } + + if (op.__op == 'RemoveRelation') { + for (const object of op.objects) { + pending.push(this.removeRelation(key, className, + objectId, + object.objectId)); + } + } + }); + return Promise.all(pending); }; @@ -511,12 +536,11 @@ DatabaseController.prototype.create = function(className, object, { acl } = {}) var isMaster = acl === undefined; var aclGroup = acl || []; - + const relationUpdates = this.collectRelationUpdates(className, null, object); return this.validateClassName(className) .then(() => this.loadSchema()) .then(schemaController => { return (isMaster ? Promise.resolve() : schemaController.validatePermission(className, aclGroup, 'create')) - .then(() => this.handleRelationUpdates(className, null, object)) .then(() => schemaController.enforceClassExists(className)) .then(() => schemaController.reloadData()) .then(() => schemaController.getOneSchema(className, true)) @@ -525,7 +549,11 @@ DatabaseController.prototype.create = function(className, object, { acl } = {}) flattenUpdateOperatorsForCreate(object); return this.adapter.createObject(className, SchemaController.convertSchemaToAdapterSchema(schema), object); }) - .then(result => sanitizeDatabaseResult(originalObject, result.ops[0])); + .then(result => { + return this.handleRelationUpdates(className, null, object, relationUpdates).then(() => { + return sanitizeDatabaseResult(originalObject, result.ops[0]) + }); + }); }) };