From 1da76f0a8de46006c8ddd68cf7443def2b25b8ae Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 2 May 2023 09:18:57 +0100 Subject: [PATCH] Merge pull request #12848 from overleaf/bg-fix-path-exception add exception handling for path.join in ProjectEntityHandler GitOrigin-RevId: dad305057fd6b2821525ca5b6d1933824989e241 --- .../app/src/Features/Compile/ClsiManager.js | 18 +- .../src/Features/Project/ProjectController.js | 9 +- .../Features/Project/ProjectEntityHandler.js | 63 ++++-- .../Project/ProjectEntityUpdateHandler.js | 15 +- .../src/Project/ProjectControllerTests.js | 11 ++ .../src/Project/ProjectEntityHandlerTests.js | 184 ++++++++++++++++++ .../ProjectEntityUpdateHandlerTests.js | 18 ++ 7 files changed, 291 insertions(+), 27 deletions(-) diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index bf3824cf93..e71e44e4f5 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/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] diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index df1329c229..497927a6c9 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/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 diff --git a/services/web/app/src/Features/Project/ProjectEntityHandler.js b/services/web/app/src/Features/Project/ProjectEntityHandler.js index d2bc808c40..3c5940df10 100644 --- a/services/web/app/src/Features/Project/ProjectEntityHandler.js +++ b/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) + } }) }, diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index a0a0eb9a6b..1e7f7fd437 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/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) + } } } diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 15e198200a..0be0909f26 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/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 () { diff --git a/services/web/test/unit/src/Project/ProjectEntityHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityHandlerTests.js index 49efe9cad5..8f14a5ba42 100644 --- a/services/web/test/unit/src/Project/ProjectEntityHandlerTests.js +++ b/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'] diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js index 6d03521f11..178a900d75 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js +++ b/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 () {