Merge pull request #30609 from overleaf/bg-additional-check-for-upload-file-leak

handle file tree operation errors in uploadFile

GitOrigin-RevId: 00ebb8318c38cb13b45733774d1328ae20ebc97a

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(

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