Merge pull request #30530 from overleaf/bg-fix-zip-upload-leak

clean up temporary folder on zip upload failure

GitOrigin-RevId: 83a934f41de7e4c737bb3e1123e6ac6b0cb15429

services/web/app/src/Features/Uploads/ProjectUploadManager.mjs

@@ -30,38 +30,44 @@ export default {
 
 async function createProjectFromZipArchive(ownerId, defaultName, zipPath) {
   const contentsPath = await _extractZip(zipPath)
-  const { path, content } =
-    await ProjectRootDocManager.promises.findRootDocFileFromDirectory(
-      contentsPath
-    )
-
-  const projectName =
-    DocumentHelper.getTitleFromTexContent(content || '') || defaultName
-  const uniqueName = await _generateUniqueName(ownerId, projectName)
-  const project = await ProjectCreationHandler.promises.createBlankProject(
-    ownerId,
-    uniqueName
-  )
   try {
-    await _initializeProjectWithZipContents(ownerId, project, contentsPath)
+    const { path, content } =
+      await ProjectRootDocManager.promises.findRootDocFileFromDirectory(
+        contentsPath
+      )
 
-    if (path) {
-      await ProjectRootDocManager.promises.setRootDocFromName(project._id, path)
-    }
-  } catch (err) {
-    // no need to wait for the cleanup here
-    ProjectDeleter.promises
-      .deleteProject(project._id)
-      .catch(err =>
-        logger.error(
-          { err, projectId: project._id },
-          'there was an error cleaning up project after importing a zip failed'
+    const projectName =
+      DocumentHelper.getTitleFromTexContent(content || '') || defaultName
+    const uniqueName = await _generateUniqueName(ownerId, projectName)
+    const project = await ProjectCreationHandler.promises.createBlankProject(
+      ownerId,
+      uniqueName
+    )
+    try {
+      await _initializeProjectWithZipContents(ownerId, project, contentsPath)
+
+      if (path) {
+        await ProjectRootDocManager.promises.setRootDocFromName(
+          project._id,
+          path
         )
-      )
-    throw err
+      }
+    } catch (err) {
+      // no need to wait for the cleanup here
+      ProjectDeleter.promises
+        .deleteProject(project._id)
+        .catch(err =>
+          logger.error(
+            { err, projectId: project._id },
+            'there was an error cleaning up project after importing a zip failed'
+          )
+        )
+      throw err
+    }
+    return project
+  } finally {
+    await fs.promises.rm(contentsPath, { recursive: true, force: true })
   }
-  await fs.promises.rm(contentsPath, { recursive: true, force: true })
-  return project
 }
 
 async function createProjectFromZipArchiveWithName(
@@ -71,30 +77,33 @@ async function createProjectFromZipArchiveWithName(
   attributes = {}
 ) {
   const contentsPath = await _extractZip(zipPath)
-  const uniqueName = await _generateUniqueName(ownerId, proposedName)
-  const project = await ProjectCreationHandler.promises.createBlankProject(
-    ownerId,
-    uniqueName,
-    attributes
-  )
-
   try {
-    await _initializeProjectWithZipContents(ownerId, project, contentsPath)
-    await ProjectRootDocManager.promises.setRootDocAutomatically(project._id)
-  } catch (err) {
-    // no need to wait for the cleanup here
-    ProjectDeleter.promises
-      .deleteProject(project._id)
-      .catch(err =>
-        logger.error(
-          { err, projectId: project._id },
-          'there was an error cleaning up project after importing a zip failed'
+    const uniqueName = await _generateUniqueName(ownerId, proposedName)
+    const project = await ProjectCreationHandler.promises.createBlankProject(
+      ownerId,
+      uniqueName,
+      attributes
+    )
+
+    try {
+      await _initializeProjectWithZipContents(ownerId, project, contentsPath)
+      await ProjectRootDocManager.promises.setRootDocAutomatically(project._id)
+    } catch (err) {
+      // no need to wait for the cleanup here
+      ProjectDeleter.promises
+        .deleteProject(project._id)
+        .catch(err =>
+          logger.error(
+            { err, projectId: project._id },
+            'there was an error cleaning up project after importing a zip failed'
+          )
         )
-      )
-    throw err
+      throw err
+    }
+    return project
+  } finally {
+    await fs.promises.rm(contentsPath, { recursive: true, force: true })
   }
-  await fs.promises.rm(contentsPath, { recursive: true, force: true })
-  return project
 }
 
 async function _extractZip(zipPath) {
@@ -102,7 +111,13 @@ async function _extractZip(zipPath) {
     Path.dirname(zipPath),
     `${Path.basename(zipPath, '.zip')}-${Date.now()}`
   )
-  await ArchiveManager.promises.extractZipArchive(zipPath, destination)
+  try {
+    await ArchiveManager.promises.extractZipArchive(zipPath, destination)
+  } catch (error) {
+    logger.debug({ zipPath, error }, 'error extracting from zip archive')
+    await fs.promises.rm(destination, { recursive: true, force: true })
+    throw error
+  }
   return destination
 }
 

services/web/test/unit/src/Uploads/ProjectUploadManager.test.mjs

@@ -323,6 +323,48 @@ describe('ProjectUploadManager', function () {
           .been.called
       })
     })
+
+    describe('when extraction fails', function () {
+      beforeEach(async function (ctx) {
+        ctx.ArchiveManager.promises.extractZipArchive.rejects(new Error('oops'))
+        await expect(
+          ctx.ProjectUploadManager.promises.createProjectFromZipArchive(
+            ctx.ownerId,
+            ctx.projectName,
+            ctx.zipPath
+          )
+        ).to.be.rejectedWith('oops')
+      })
+
+      it('should remove the destination directory', function (ctx) {
+        ctx.fs.promises.rm.should.have.been.calledWith(ctx.extractedZipPath, {
+          recursive: true,
+          force: true,
+        })
+      })
+    })
+
+    describe('when project creation fails', function () {
+      beforeEach(async function (ctx) {
+        ctx.ProjectCreationHandler.promises.createBlankProject.rejects(
+          new Error('oops')
+        )
+        await expect(
+          ctx.ProjectUploadManager.promises.createProjectFromZipArchive(
+            ctx.ownerId,
+            ctx.projectName,
+            ctx.zipPath
+          )
+        ).to.be.rejectedWith('oops')
+      })
+
+      it('should remove the destination directory', function (ctx) {
+        ctx.fs.promises.rm.should.have.been.calledWith(ctx.extractedZipPath, {
+          recursive: true,
+          force: true,
+        })
+      })
+    })
   })
 
   describe('createProjectFromZipArchiveWithName', function () {
@@ -406,6 +448,13 @@ describe('ProjectUploadManager', function () {
           ctx.project._id
         )
       })
+
+      it('should remove the destination directory', function (ctx) {
+        ctx.fs.promises.rm.should.have.been.calledWith(ctx.extractedZipPath, {
+          recursive: true,
+          force: true,
+        })
+      })
     })
 
     describe('when setting automatically the root doc fails', function () {
@@ -425,6 +474,33 @@ describe('ProjectUploadManager', function () {
           ctx.project._id
         )
       })
+
+      it('should remove the destination directory', function (ctx) {
+        ctx.fs.promises.rm.should.have.been.calledWith(ctx.extractedZipPath, {
+          recursive: true,
+          force: true,
+        })
+      })
+    })
+
+    describe('when extraction fails', function () {
+      beforeEach(async function (ctx) {
+        ctx.ArchiveManager.promises.extractZipArchive.rejects(new Error('oops'))
+        await expect(
+          ctx.ProjectUploadManager.promises.createProjectFromZipArchiveWithName(
+            ctx.ownerId,
+            ctx.projectName,
+            ctx.zipPath
+          )
+        ).to.be.rejectedWith('oops')
+      })
+
+      it('should remove the destination directory', function (ctx) {
+        ctx.fs.promises.rm.should.have.been.calledWith(ctx.extractedZipPath, {
+          recursive: true,
+          force: true,
+        })
+      })
     })
   })
 })