Merge pull request #7408 from overleaf/em-malformed-file-tree

Prevent malformed file trees

GitOrigin-RevId: 59104077bed80dd87a7c3813e06581bb6d9bd8d9

services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js

@@ -160,7 +160,7 @@ async function replaceFileWithNew(projectId, fileId, newFileRef) {
   })
   await _insertDeletedFileReference(projectId, fileRef)
   const newProject = await Project.findOneAndUpdate(
-    { _id: project._id },
+    { _id: project._id, [path.mongo]: { $exists: true } },
     {
       $set: {
         [`${path.mongo}._id`]: newFileRef._id,
@@ -173,14 +173,20 @@ async function replaceFileWithNew(projectId, fileId, newFileRef) {
         [`${path.mongo}.rev`]: 1,
       },
     },
+    // Note: Mongoose uses new:true to return the modified document
+    // https://mongoosejs.com/docs/api.html#model_Model.findOneAndUpdate
+    // but Mongo uses returnNewDocument:true instead
+    // https://docs.mongodb.com/manual/reference/method/db.collection.findOneAndUpdate/
+    // We are using Mongoose here, but if we ever switch to a direct mongo call
+    // the next line will need to be updated.
     { new: true }
   ).exec()
-  // Note: Mongoose uses new:true to return the modified document
-  // https://mongoosejs.com/docs/api.html#model_Model.findOneAndUpdate
-  // but Mongo uses returnNewDocument:true instead
-  // https://docs.mongodb.com/manual/reference/method/db.collection.findOneAndUpdate/
-  // We are using Mongoose here, but if we ever switch to a direct mongo call
-  // the next line will need to be updated.
+  if (newProject == null) {
+    throw new OError('Project not found or path not found in filetree', {
+      projectId,
+      path,
+    })
+  }
   return { oldFileRef: fileRef, project, path, newProject }
 }
 
@@ -196,7 +202,7 @@ async function replaceDocWithFile(projectId, docId, fileRef) {
   })
   const folderMongoPath = _getParentMongoPath(path.mongo)
   const newProject = await Project.findOneAndUpdate(
-    { _id: project._id },
+    { _id: project._id, [folderMongoPath]: { $exists: true } },
     {
       $pull: {
         [`${folderMongoPath}.docs`]: { _id: docId },
@@ -208,6 +214,12 @@ async function replaceDocWithFile(projectId, docId, fileRef) {
     },
     { new: true }
   ).exec()
+  if (newProject == null) {
+    throw new OError('Project not found or path not found in filetree', {
+      projectId,
+      path,
+    })
+  }
   return newProject
 }
 
@@ -223,7 +235,7 @@ async function replaceFileWithDoc(projectId, fileId, newDoc) {
   })
   const folderMongoPath = _getParentMongoPath(path.mongo)
   const newProject = await Project.findOneAndUpdate(
-    { _id: project._id },
+    { _id: project._id, [folderMongoPath]: { $exists: true } },
     {
       $pull: {
         [`${folderMongoPath}.fileRefs`]: { _id: fileId },
@@ -235,6 +247,12 @@ async function replaceFileWithDoc(projectId, fileId, newDoc) {
     },
     { new: true }
   ).exec()
+  if (newProject == null) {
+    throw new OError('Project not found or path not found in filetree', {
+      projectId,
+      path,
+    })
+  }
   return newProject
 }
 
@@ -414,10 +432,16 @@ async function renameEntity(
 
   // we need to increment the project version number for any structure change
   const newProject = await Project.findOneAndUpdate(
-    { _id: projectId },
+    { _id: projectId, [entPath.mongo]: { $exists: true } },
     { $set: { [`${entPath.mongo}.name`]: newName }, $inc: { version: 1 } },
     { new: true }
   ).exec()
+  if (newProject == null) {
+    throw new OError('Project not found or path not found in filetree', {
+      projectId,
+      path: entPath,
+    })
+  }
 
   const { docs: newDocs, files: newFiles } =
     ProjectEntityHandler.getAllEntitiesFromProject(newProject)
@@ -541,10 +565,16 @@ async function _putElement(project, folderId, element, type) {
   element._id = ObjectId(element._id.toString())
   const mongoPath = `${path.mongo}.${pathSegment}`
   const newProject = await Project.findOneAndUpdate(
-    { _id: project._id },
+    { _id: project._id, [path.mongo]: { $exists: true } },
     { $push: { [mongoPath]: element }, $inc: { version: 1 } },
     { new: true }
   ).exec()
+  if (newProject == null) {
+    throw new OError('Project not found or path not found in filetree', {
+      projectId: project._id,
+      path,
+    })
+  }
   return { result: { path: newPath }, project: newProject }
 }
 

services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js

@@ -210,7 +210,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
       const doc = { _id: ObjectId(), name: 'other.txt' }
       this.ProjectMock.expects('findOneAndUpdate')
         .withArgs(
-          { _id: this.project._id },
+          {
+            _id: this.project._id,
+            'rootFolder.0.folders.0': { $exists: true },
+          },
           {
             $push: { 'rootFolder.0.folders.0.docs': doc },
             $inc: { version: 1 },
@@ -247,7 +250,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
       this.newFile = { _id: ObjectId(), name: 'picture.jpg' }
       this.ProjectMock.expects('findOneAndUpdate')
         .withArgs(
-          { _id: this.project._id },
+          {
+            _id: this.project._id,
+            'rootFolder.0.folders.0': { $exists: true },
+          },
           {
             $push: { 'rootFolder.0.folders.0.fileRefs': this.newFile },
             $inc: { version: 1 },
@@ -314,7 +320,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
       })
       this.ProjectMock.expects('findOneAndUpdate')
         .withArgs(
-          { _id: this.project._id },
+          {
+            _id: this.project._id,
+            'rootFolder.0.folders.0': { $exists: true },
+          },
           {
             $push: {
               'rootFolder.0.folders.0.folders': sinon.match({
@@ -360,7 +369,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
       // Update the file in place
       this.ProjectMock.expects('findOneAndUpdate')
         .withArgs(
-          { _id: this.project._id },
+          {
+            _id: this.project._id,
+            'rootFolder.0.fileRefs.0': { $exists: true },
+          },
           {
             $set: {
               'rootFolder.0.fileRefs.0._id': newFile._id,
@@ -438,7 +450,7 @@ describe('ProjectEntityMongoUpdateHandler', function () {
         this.exactCaseMatch = false
         this.ProjectMock.expects('findOneAndUpdate')
           .withArgs(
-            { _id: this.project._id },
+            { _id: this.project._id, 'rootFolder.0': { $exists: true } },
             {
               $push: { 'rootFolder.0.folders': this.newFolder },
               $inc: { version: 1 },
@@ -481,7 +493,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
         this.FolderModel.returns(this.newFolder)
         this.ProjectMock.expects('findOneAndUpdate')
           .withArgs(
-            { _id: this.project._id },
+            {
+              _id: this.project._id,
+              'rootFolder.0.folders.0': { $exists: true },
+            },
             {
               $push: {
                 'rootFolder.0.folders.0.folders': sinon.match({
@@ -552,7 +567,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
           })
         this.ProjectMock.expects('findOneAndUpdate')
           .withArgs(
-            { _id: this.project._id },
+            {
+              _id: this.project._id,
+              'rootFolder.0.folders.0': { $exists: true },
+            },
             {
               $push: {
                 'rootFolder.0.folders.0.folders': sinon.match({
@@ -566,7 +584,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
           .resolves(this.project)
         this.ProjectMock.expects('findOneAndUpdate')
           .withArgs(
-            { _id: this.project._id },
+            {
+              _id: this.project._id,
+              'rootFolder.0.folders.0.folders.0': { $exists: true },
+            },
             {
               $push: {
                 'rootFolder.0.folders.0.folders.0.folders': sinon.match({
@@ -642,7 +663,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
 
         this.ProjectMock.expects('findOneAndUpdate')
           .withArgs(
-            { _id: this.project._id },
+            {
+              _id: this.project._id,
+              'rootFolder.0.folders.0': { $exists: true },
+            },
             {
               $push: { 'rootFolder.0.folders.0.docs': this.doc },
               $inc: { version: 1 },
@@ -758,7 +782,7 @@ describe('ProjectEntityMongoUpdateHandler', function () {
 
         this.ProjectMock.expects('findOneAndUpdate')
           .withArgs(
-            { _id: this.project._id },
+            { _id: this.project._id, 'rootFolder.0.docs.0': { $exists: true } },
             {
               $set: { 'rootFolder.0.docs.0.name': this.newName },
               $inc: { version: 1 },
@@ -816,7 +840,10 @@ describe('ProjectEntityMongoUpdateHandler', function () {
           this.newFile = { _id: ObjectId(), name: 'new file.png' }
           this.ProjectMock.expects('findOneAndUpdate')
             .withArgs(
-              { _id: this.project._id },
+              {
+                _id: this.project._id,
+                'rootFolder.0.folders.0': { $exists: true },
+              },
               {
                 $push: { 'rootFolder.0.folders.0.fileRefs': this.newFile },
                 $inc: { version: 1 },
@@ -946,7 +973,7 @@ describe('ProjectEntityMongoUpdateHandler', function () {
         this.newFile = { _id: ObjectId(), name: 'new file.png' }
         this.ProjectMock.expects('findOneAndUpdate')
           .withArgs(
-            { _id: this.project._id },
+            { _id: this.project._id, 'rootFolder.0': { $exists: true } },
             {
               $push: { 'rootFolder.0.fileRefs': this.newFile },
               $inc: { version: 1 },
@@ -1045,7 +1072,7 @@ describe('ProjectEntityMongoUpdateHandler', function () {
     it('should simultaneously remove the doc and add the file', async function () {
       this.ProjectMock.expects('findOneAndUpdate')
         .withArgs(
-          { _id: this.project._id },
+          { _id: this.project._id, 'rootFolder.0': { $exists: true } },
           {
             $pull: { 'rootFolder.0.docs': { _id: this.doc._id } },
             $push: { 'rootFolder.0.fileRefs': this.file },
@@ -1068,7 +1095,7 @@ describe('ProjectEntityMongoUpdateHandler', function () {
     it('should simultaneously remove the file and add the doc', async function () {
       this.ProjectMock.expects('findOneAndUpdate')
         .withArgs(
-          { _id: this.project._id },
+          { _id: this.project._id, 'rootFolder.0': { $exists: true } },
           {
             $pull: { 'rootFolder.0.fileRefs': { _id: this.file._id } },
             $push: { 'rootFolder.0.docs': this.doc },