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

Author: flovilmart

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

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