From c0033cd0cfc2f409c8f70e6aeb8340dddc926155 Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Fri, 19 May 2017 16:54:57 -0400 Subject: [PATCH 1/3] Adds test for 3835 --- spec/ParseRole.spec.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/spec/ParseRole.spec.js b/spec/ParseRole.spec.js index c61c4aed87..73e085ac0c 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); + }); + }); From 2ef969a425efb2b4f30edc0180cb30ea410d6951 Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Fri, 19 May 2017 16:55:32 -0400 Subject: [PATCH 2/3] Makes sure we run relational updates AFTER validating access to the object --- src/Controllers/DatabaseController.js | 64 ++++++++++++++++++--------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index db633981a5..46bdae9c18 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,8 @@ 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. -// 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 +335,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 +357,37 @@ 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 successfully when these are +// processed. +// This mutates update. +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 +535,12 @@ 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(() => this.handleRelationUpdates(className, null, object, relationUpdates)) .then(() => schemaController.enforceClassExists(className)) .then(() => schemaController.reloadData()) .then(() => schemaController.getOneSchema(className, true)) From 6e4dc98607180a84347844ad0b836bd22929b43b Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Sat, 20 May 2017 12:04:36 -0400 Subject: [PATCH 3/3] Always run relation udpates last --- src/Controllers/DatabaseController.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 46bdae9c18..40cbeddf0b 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -325,6 +325,9 @@ function sanitizeDatabaseResult(originalObject, result) { return Promise.resolve(response); } +// Collect all relation-updating operations from a REST-format update. +// Returns a list of all relation updates to perform +// This mutates update. DatabaseController.prototype.collectRelationUpdates = function(className, objectId, update) { var ops = []; var deleteMe = []; @@ -361,9 +364,7 @@ DatabaseController.prototype.collectRelationUpdates = function(className, object } // Processes relation-updating operations from a REST-format update. -// Returns a promise that resolves successfully when these are -// processed. -// This mutates 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; @@ -540,7 +541,6 @@ DatabaseController.prototype.create = function(className, object, { acl } = {}) .then(() => this.loadSchema()) .then(schemaController => { return (isMaster ? Promise.resolve() : schemaController.validatePermission(className, aclGroup, 'create')) - .then(() => this.handleRelationUpdates(className, null, object, relationUpdates)) .then(() => schemaController.enforceClassExists(className)) .then(() => schemaController.reloadData()) .then(() => schemaController.getOneSchema(className, true)) @@ -549,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]) + }); + }); }) };