From e4eb116a5c0e8361c24dc1323c703068e9f3d575 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 9 Jan 2026 09:31:14 +0000 Subject: [PATCH] Merge pull request #30609 from overleaf/bg-additional-check-for-upload-file-leak handle file tree operation errors in uploadFile GitOrigin-RevId: 00ebb8318c38cb13b45733774d1328ae20ebc97a --- .../Uploads/ProjectUploadController.mjs | 35 +++++++++++-------- .../Uploads/ProjectUploadController.test.mjs | 27 ++++++++++++++ 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/services/web/app/src/Features/Uploads/ProjectUploadController.mjs b/services/web/app/src/Features/Uploads/ProjectUploadController.mjs index 0ce933fd19..cc02e5bce7 100644 --- a/services/web/app/src/Features/Uploads/ProjectUploadController.mjs +++ b/services/web/app/src/Features/Uploads/ProjectUploadController.mjs @@ -78,21 +78,26 @@ async function uploadFile(req, res, next) { }) } - // preserve the directory structure from an uploaded folder - const { relativePath } = req.body - // NOTE: Uppy sends a "null" string for `relativePath` when the file is not nested in a folder - if (relativePath && relativePath !== 'null') { - const { path } = await ProjectLocator.promises.findElement({ - project_id: projectId, - element_id: folderId, - type: 'folder', - }) - const { lastFolder } = await EditorController.promises.mkdirp( - projectId, - Path.dirname(Path.join('/', path.fileSystem, relativePath)), - userId - ) - folderId = lastFolder._id + try { + // preserve the directory structure from an uploaded folder + const { relativePath } = req.body + // NOTE: Uppy sends a "null" string for `relativePath` when the file is not nested in a folder + if (relativePath && relativePath !== 'null') { + const { path } = await ProjectLocator.promises.findElement({ + project_id: projectId, + element_id: folderId, + type: 'folder', + }) + const { lastFolder } = await EditorController.promises.mkdirp( + projectId, + Path.dirname(Path.join('/', path.fileSystem, relativePath)), + userId + ) + folderId = lastFolder._id + } + } catch (error) { + fs.unlink(path, function () {}) + throw error } return FileSystemImportManager.addEntity( diff --git a/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs b/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs index b2cad85584..53a6d05c37 100644 --- a/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs +++ b/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs @@ -313,6 +313,33 @@ describe('ProjectUploadController', function () { }) }) + describe('when looking up the folder structure fails', function () { + beforeEach(async function (ctx) { + await new Promise(resolve => { + ctx.error = new Error('woops') + ctx.ProjectLocator.promises.findElement = sinon + .stub() + .rejects(ctx.error) + ctx.req.body.relativePath = 'foo/bar/' + ctx.fileName + + ctx.next = error => { + ctx.nextError = error + resolve() + } + + ctx.ProjectUploadController.uploadFile(ctx.req, ctx.res, ctx.next) + }) + }) + + it('should unlink the file', function (ctx) { + ctx.fs.unlink.should.have.been.calledWith(ctx.path) + }) + + it('should call next with the error', function (ctx) { + expect(ctx.nextError).to.equal(ctx.error) + }) + }) + describe('when FileSystemImportManager.addEntity returns a generic error', function () { beforeEach(function (ctx) { ctx.FileSystemImportManager.addEntity = sinon