Merge pull request #12848 from overleaf/bg-fix-path-exception

add exception handling for  path.join in ProjectEntityHandler

GitOrigin-RevId: dad305057fd6b2821525ca5b6d1933824989e241

services/web/app/src/Features/Compile/ClsiManager.js

@@ -717,7 +717,12 @@ const ClsiManager = {
   },
 
   getContentFromDocUpdaterIfMatch(projectId, project, options, callback) {
-    const projectStateHash = ClsiStateManager.computeHash(project, options)
+    let projectStateHash
+    try {
+      projectStateHash = ClsiStateManager.computeHash(project, options)
+    } catch (err) {
+      return callback(err)
+    }
     DocumentUpdaterHandler.getProjectDocsIfMatch(
       projectId,
       projectStateHash,
@@ -763,7 +768,16 @@ const ClsiManager = {
     docUpdaterDocs,
     callback
   ) {
-    const docPath = ProjectEntityHandler.getAllDocPathsFromProject(project)
+    let docPath
+    try {
+      docPath = ProjectEntityHandler.getAllDocPathsFromProject(project)
+    } catch (err) {
+      return callback(
+        OError.tag(err, 'Failed to get all doc paths from project', {
+          projectId,
+        })
+      )
+    }
     const docs = {}
     for (const doc of docUpdaterDocs || []) {
       const path = docPath[doc._id]

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

@@ -394,8 +394,13 @@ const ProjectController = {
       if (err != null) {
         return next(err)
       }
-      const { docs, files } =
-        ProjectEntityHandler.getAllEntitiesFromProject(project)
+      let docs, files
+      try {
+        ;({ docs, files } =
+          ProjectEntityHandler.getAllEntitiesFromProject(project))
+      } catch (err) {
+        return next(err)
+      }
       const entities = docs
         .concat(files)
         // Sort by path ascending

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

@@ -30,12 +30,16 @@ const ProjectEntityHandler = {
           for (const doc of iterablePaths(folder, 'docs')) {
             const content = docContents[doc._id.toString()]
             if (content != null) {
-              docs[path.join(folderPath, doc.name)] = {
-                _id: doc._id,
-                name: doc.name,
-                lines: content.lines,
-                rev: content.rev,
-                folder,
+              try {
+                docs[path.join(folderPath, doc.name)] = {
+                  _id: doc._id,
+                  name: doc.name,
+                  lines: content.lines,
+                  rev: content.rev,
+                  folder,
+                }
+              } catch (err) {
+                return callback(err)
               }
             }
           }
@@ -55,7 +59,11 @@ const ProjectEntityHandler = {
       for (const { path: folderPath, folder } of folders) {
         for (const file of iterablePaths(folder, 'fileRefs')) {
           if (file != null) {
-            files[path.join(folderPath, file.name)] = { ...file, folder }
+            try {
+              files[path.join(folderPath, file.name)] = { ...file, folder }
+            } catch (err) {
+              return callback(err)
+            }
           }
         }
       }
@@ -71,9 +79,12 @@ const ProjectEntityHandler = {
       if (project == null) {
         return callback(new Errors.NotFoundError('project not found'))
       }
-
-      const entities = ProjectEntityHandler.getAllEntitiesFromProject(project)
-      callback(null, entities)
+      try {
+        const entities = ProjectEntityHandler.getAllEntitiesFromProject(project)
+        callback(null, entities)
+      } catch (err) {
+        callback(err)
+      }
     })
   },
 
@@ -104,8 +115,12 @@ const ProjectEntityHandler = {
       if (project == null) {
         return callback(Errors.NotFoundError('no project'))
       }
-      const docPaths = ProjectEntityHandler.getAllDocPathsFromProject(project)
-      callback(null, docPaths)
+      try {
+        const docPaths = ProjectEntityHandler.getAllDocPathsFromProject(project)
+        callback(null, docPaths)
+      } catch (err) {
+        callback(err)
+      }
     })
   },
 
@@ -186,12 +201,16 @@ const ProjectEntityHandler = {
         return null
       }
     }
-    const docPath = recursivelyFindDocInFolder(
-      '/',
-      docId,
-      project.rootFolder[0]
-    )
-    callback(null, docPath)
+    try {
+      const docPath = recursivelyFindDocInFolder(
+        '/',
+        docId,
+        project.rootFolder[0]
+      )
+      callback(null, docPath)
+    } catch (err) {
+      callback(err)
+    }
   },
 
   _getAllFolders(projectId, callback) {
@@ -202,8 +221,12 @@ const ProjectEntityHandler = {
       if (project == null) {
         return callback(new Errors.NotFoundError('no project'))
       }
-      const folders = ProjectEntityHandler._getAllFoldersFromProject(project)
-      callback(null, folders)
+      try {
+        const folders = ProjectEntityHandler._getAllFoldersFromProject(project)
+        callback(null, folders)
+      } catch (err) {
+        callback(err)
+      }
     })
   },
 

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

@@ -1396,8 +1396,13 @@ const ProjectEntityUpdateHandler = {
             return callback(error)
           }
 
-          let { docs, files, folders } =
-            ProjectEntityHandler.getAllEntitiesFromProject(project)
+          let docs, files, folders
+          try {
+            ;({ docs, files, folders } =
+              ProjectEntityHandler.getAllEntitiesFromProject(project))
+          } catch (error) {
+            return callback(error)
+          }
           // _checkFileTree() must be passed the folders before docs and
           // files
           ProjectEntityUpdateHandler._checkFiletree(
@@ -1489,7 +1494,11 @@ const ProjectEntityUpdateHandler = {
           // the case only because getAllEntitiesFromProject() returns folders
           // in that order and resyncProjectHistory() calls us with the folders
           // first.
-          adjustPathsAfterFolderRename(entity.path, newPath)
+          try {
+            adjustPathsAfterFolderRename(entity.path, newPath)
+          } catch (error) {
+            return callback(error)
+          }
         }
       }
 

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

@@ -1558,6 +1558,17 @@ describe('ProjectController', function () {
       }
       this.ProjectController.projectEntitiesJson(this.req, this.res, this.next)
     })
+
+    it('should call next with an error if the project file tree is invalid', function (done) {
+      this.ProjectEntityHandler.getAllEntitiesFromProject = sinon
+        .stub()
+        .throws()
+      this.next = err => {
+        expect(err).to.be.an.instanceof(Error)
+        done()
+      }
+      this.ProjectController.projectEntitiesJson(this.req, this.res, this.next)
+    })
   })
 
   describe('_buildProjectViewModel', function () {

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

@@ -264,6 +264,190 @@ describe('ProjectEntityHandler', function () {
     })
   })
 
+  describe('with an invalid file tree', function () {
+    beforeEach(function () {
+      this.project.rootFolder = [
+        {
+          docs: [
+            (this.doc1 = {
+              name: null, // invalid doc name
+              _id: 'doc1_id',
+            }),
+          ],
+          fileRefs: [
+            (this.file1 = {
+              rev: 1,
+              _id: 'file1_id',
+              name: null, // invalid file name
+            }),
+          ],
+          folders: [
+            (this.folder1 = {
+              name: null, // invalid folder name
+              docs: [
+                (this.doc2 = {
+                  name: 'doc2',
+                  _id: 'doc2_id',
+                }),
+              ],
+              fileRefs: [
+                (this.file2 = {
+                  rev: 2,
+                  name: 'file2',
+                  _id: 'file2_id',
+                }),
+              ],
+              folders: null,
+            }),
+            null, // invalid folder
+          ],
+        },
+      ]
+      this.ProjectGetter.getProjectWithoutDocLines = sinon
+        .stub()
+        .yields(null, this.project)
+    })
+
+    describe('getAllDocs', function () {
+      beforeEach(function () {
+        this.docs = [
+          {
+            _id: this.doc1._id,
+            lines: (this.lines1 = ['one']),
+            rev: (this.rev1 = 1),
+          },
+          {
+            _id: this.doc2._id,
+            lines: (this.lines2 = ['two']),
+            rev: (this.rev2 = 2),
+          },
+        ]
+        this.DocstoreManager.getAllDocs = sinon
+          .stub()
+          .callsArgWith(1, null, this.docs)
+        this.ProjectEntityHandler.getAllDocs(projectId, this.callback)
+      })
+
+      it('should get the doc lines and rev from the docstore', function () {
+        this.DocstoreManager.getAllDocs.calledWith(projectId).should.equal(true)
+      })
+
+      it('should call the callback with an error', function () {
+        this.callback.should.have.been.calledWith(sinon.match.defined)
+      })
+    })
+
+    describe('getAllFiles', function () {
+      beforeEach(function () {
+        this.callback = sinon.stub()
+        this.ProjectEntityHandler.getAllFiles(projectId, this.callback)
+      })
+
+      it('should call the callback with and error', function () {
+        this.callback.should.have.been.calledWith(sinon.match.defined)
+      })
+    })
+
+    describe('getDocPathByProjectIdAndDocId', function () {
+      beforeEach(function () {
+        this.callback = sinon.stub()
+      })
+      it('should call the callback with an error for an existing doc id at the root level', function () {
+        this.ProjectEntityHandler.getDocPathByProjectIdAndDocId(
+          projectId,
+          this.doc1._id,
+          this.callback
+        )
+        this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error))
+      })
+
+      it('should call the callback with an error for an existing doc id nested within a folder', function () {
+        this.ProjectEntityHandler.getDocPathByProjectIdAndDocId(
+          projectId,
+          this.doc2._id,
+          this.callback
+        )
+        this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error))
+      })
+
+      it('should call the callback with an error for a non-existing doc', function () {
+        this.ProjectEntityHandler.getDocPathByProjectIdAndDocId(
+          projectId,
+          'non-existing-id',
+          this.callback
+        )
+        this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error))
+      })
+
+      it('should call the callback with an error for an existing file', function () {
+        this.ProjectEntityHandler.getDocPathByProjectIdAndDocId(
+          projectId,
+          this.file1._id,
+          this.callback
+        )
+        this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error))
+      })
+    })
+
+    describe('_getAllFolders', function () {
+      beforeEach(function () {
+        this.callback = sinon.stub()
+        this.ProjectEntityHandler._getAllFolders(projectId, this.callback)
+      })
+
+      it('should get the project without the docs lines', function () {
+        this.ProjectGetter.getProjectWithoutDocLines
+          .calledWith(projectId)
+          .should.equal(true)
+      })
+
+      it('should call the callback with an error', function () {
+        this.callback.should.have.been.calledWith(sinon.match.defined)
+      })
+    })
+
+    describe('getAllEntities', function () {
+      beforeEach(function () {
+        this.ProjectGetter.getProject = sinon.stub().yields(null, this.project)
+        this.callback = sinon.stub()
+        this.ProjectEntityHandler.getAllEntities(projectId, this.callback)
+      })
+
+      it('should call the callback with an error', function () {
+        this.callback.should.have.been.calledWith(sinon.match.defined)
+      })
+    })
+
+    describe('getAllDocPathsFromProjectById', function () {
+      beforeEach(function () {
+        this.callback = sinon.stub()
+        this.ProjectEntityHandler.getAllDocPathsFromProjectById(
+          projectId,
+          this.callback
+        )
+      })
+
+      it('should call the callback with an error', function () {
+        this.callback.should.have.been.calledWith(sinon.match.defined)
+      })
+    })
+
+    describe('getDocPathFromProjectByDocId', function () {
+      beforeEach(function () {
+        this.callback = sinon.stub()
+        this.ProjectEntityHandler.getDocPathFromProjectByDocId(
+          projectId,
+          this.doc1._id,
+          this.callback
+        )
+      })
+
+      it('should call the callback with an error', function () {
+        this.callback.should.have.been.calledWith(sinon.match.defined)
+      })
+    })
+  })
+
   describe('getDoc', function () {
     beforeEach(function () {
       this.lines = ['mock', 'doc', 'lines']

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

@@ -2374,6 +2374,24 @@ describe('ProjectEntityUpdateHandler', function () {
         ).to.have.been.calledWith(projectId, projectHistoryId, docs, files)
       })
     })
+
+    describe('a project with an invalid file tree', function () {
+      beforeEach(function () {
+        this.callback = sinon.stub()
+        this.ProjectGetter.getProject.yields(null, this.project)
+        this.ProjectEntityHandler.getAllEntitiesFromProject.throws()
+        this.ProjectEntityUpdateHandler.resyncProjectHistory(
+          projectId,
+          this.callback
+        )
+      })
+
+      it('calls the callback with an error', function () {
+        expect(this.callback).to.have.been.calledWith(
+          sinon.match.instanceOf(Error)
+        )
+      })
+    })
   })
 
   describe('_cleanUpEntity', function () {