Merge pull request #35899 from overleaf/bg-custom-multer-workaround

add custom multer upload handler to prevent leaked files

GitOrigin-RevId: 7b390d326b4cf81fd82c532c53e9772cb4107a29

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

@@ -10,8 +10,6 @@ import EditorController from '../Editor/EditorController.mjs'
 import ProjectLocator from '../Project/ProjectLocator.mjs'
 import Settings from '@overleaf/settings'
 import { InvalidZipFileError } from './ArchiveErrors.mjs'
-import multer from 'multer'
-import lodash from 'lodash'
 import { expressify } from '@overleaf/promise-utils'
 import {
   DuplicateNameError,
@@ -22,10 +20,9 @@ import {
 import DocumentConversionManager from './DocumentConversionManager.mjs'
 import ProjectOptionsHandler from '../Project/ProjectOptionsHandler.mjs'
 import AnalyticsManager from '../Analytics/AnalyticsManager.mjs'
+import { multer, multerUploadHandler } from '../../infrastructure/Multer.mjs'
 import { parseReq, z, zz } from '../../infrastructure/Validation.mjs'
 
-const defaultsDeep = lodash.defaultsDeep
-
 const uploadMetaTypeSchema = z.string().optional()
 
 const uploadProjectSchema = z.object({
@@ -72,17 +69,12 @@ const importDocumentSchema = z.object({
   file: zz.uploadedFile(),
 })
 
-const upload = multer(
+const upload = multerUploadHandler({
-  defaultsDeep(
+  dest: Settings.path.uploadFolder,
-    {
+  limits: {
-      dest: Settings.path.uploadFolder,
+    fileSize: Settings.maxUploadSize,
-      limits: {
+  },
-        fileSize: Settings.maxUploadSize,
+})
-      },
-    },
-    Settings.multerOptions
-  )
-)
 
 /**
  * @param {any} req

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

@@ -5,6 +5,7 @@ import { RateLimiter } from '../../infrastructure/RateLimiter.mjs'
 import RateLimiterMiddleware from '../Security/RateLimiterMiddleware.mjs'
 import Settings from '@overleaf/settings'
 import AsyncLocalStorage from '../../infrastructure/AsyncLocalStorage.mjs'
+import { multerErrorHandler } from '../../infrastructure/Multer.mjs'
 import { getRawReqInput } from '../../infrastructure/Validation.mjs'
 
 const rateLimiters = {
@@ -25,7 +26,8 @@ export default {
       AuthenticationController.requireLogin(),
       RateLimiterMiddleware.rateLimit(rateLimiters.projectUpload),
       ProjectUploadController.multerMiddleware,
-      ProjectUploadController.uploadProject
+      ProjectUploadController.uploadProject,
+      multerErrorHandler
     )
 
     if (Settings.enablePandocConversions) {
@@ -34,7 +36,8 @@ export default {
         AuthenticationController.requireLogin(),
         RateLimiterMiddleware.rateLimit(rateLimiters.projectUpload),
         ProjectUploadController.multerMiddleware,
-        ProjectUploadController.importDocument
+        ProjectUploadController.importDocument,
+        multerErrorHandler
       )
       // Keep old route for backwards compatibility with old frontends that haven't reloaded
       webRouter.post(
@@ -46,7 +49,8 @@ export default {
           getRawReqInput(req).query.type = 'docx'
           next()
         },
-        ProjectUploadController.importDocument
+        ProjectUploadController.importDocument,
+        multerErrorHandler
       )
     }
 
@@ -64,7 +68,8 @@ export default {
         AsyncLocalStorage.middleware,
         AuthorizationMiddleware.ensureUserCanWriteProjectContent,
         ProjectUploadController.multerMiddleware,
-        ProjectUploadController.uploadFile
+        ProjectUploadController.uploadFile,
+        multerErrorHandler
       )
     } else {
       webRouter.post(
@@ -74,7 +79,8 @@ export default {
         AsyncLocalStorage.middleware,
         AuthorizationMiddleware.ensureUserCanWriteProjectContent,
         ProjectUploadController.multerMiddleware,
-        ProjectUploadController.uploadFile
+        ProjectUploadController.uploadFile,
+        multerErrorHandler
       )
     }
   },

services/web/app/src/infrastructure/Multer.mjs

@@ -0,0 +1,66 @@
+import fs from 'node:fs'
+import Path from 'node:path'
+import crypto from 'node:crypto'
+import multer from 'multer'
+import lodash from 'lodash'
+import Settings from '@overleaf/settings'
+import logger from '@overleaf/logger'
+
+const defaultsDeep = lodash.defaultsDeep
+
+export { multer }
+
+/* multer upload handler with automatic cleanup to work around
+ * https://github.com/expressjs/multer/issues/259
+ */
+
+function filenameWithCleanup(uploadDir) {
+  return function (req, file, cb) {
+    const name = crypto.randomUUID()
+    // add an event handler to clean up files on aborted requests
+    req.once('close', () => {
+      if (!req.complete) {
+        const uploadPath = Path.join(uploadDir, name)
+        fs.unlink(uploadPath, err => {
+          if (err && err.code !== 'ENOENT') {
+            logger.warn(
+              { err, uploadPath },
+              'error deleting uploaded file on multer cleanup'
+            )
+          }
+        })
+      }
+    })
+    cb(null, name)
+  }
+}
+
+export function multerUploadHandler(options = {}) {
+  const mergedOptions = defaultsDeep({}, options, Settings.multerOptions)
+  const { dest, ...multerConfig } = mergedOptions
+  // replace dest parameter with custom storage that cleans up leaked files
+  if (dest) {
+    multerConfig.storage = multer.diskStorage({
+      destination: dest,
+      filename: filenameWithCleanup(dest),
+    })
+  }
+  return multer(multerConfig)
+}
+
+/* remove the uploaded file when an error reaches the end of the route,
+ * so route handlers don't each have to unlink on every error path
+ */
+export function multerErrorHandler(err, req, res, next) {
+  if (req.file?.path) {
+    fs.unlink(req.file.path, unlinkErr => {
+      if (unlinkErr && unlinkErr.code !== 'ENOENT') {
+        logger.warn(
+          { err: unlinkErr, uploadPath: req.file.path },
+          'error deleting uploaded file on multer error cleanup'
+        )
+      }
+    })
+  }
+  next(err)
+}

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

@@ -82,8 +82,9 @@ describe('ProjectUploadController', function () {
       },
     }
 
-    vi.doMock('multer', () => ({
+    vi.doMock('../../../../app/src/infrastructure/Multer.mjs', () => ({
-      default: sinon.stub(),
+      multer: { MulterError: class MulterError extends Error {} },
+      multerUploadHandler: sinon.stub().returns({ single: sinon.stub() }),
     }))
 
     vi.doMock('@overleaf/settings', () => ({
@@ -905,12 +906,12 @@ describe('ProjectUploadController', function () {
       }
       ctx.MulterError = MulterError
 
-      const multerFn = sinon.stub().returns({
+      vi.doMock('../../../../app/src/infrastructure/Multer.mjs', () => ({
-        single: sinon.stub().returns(ctx.uploadSingleMiddleware),
+        multer: { MulterError },
-      })
+        multerUploadHandler: sinon.stub().returns({
-      multerFn.MulterError = MulterError
+          single: sinon.stub().returns(ctx.uploadSingleMiddleware),
-
+        }),
-      vi.doMock('multer', () => ({ default: multerFn }))
+      }))
       vi.doMock('@overleaf/settings', () => ({ default: { path: {} } }))
       vi.doMock('@overleaf/metrics', () => ({ default: ctx.metrics }))
       vi.doMock(

services/web/test/unit/src/infrastructure/Multer.test.mjs

@@ -0,0 +1,286 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+const modulePath = '../../../../app/src/infrastructure/Multer.mjs'
+const MB = 1024 * 1024
+
+describe('Multer', function () {
+  let Multer
+  let mockFs
+  let mockMulter
+  let mockMulterInstance
+  let mockSettings
+  let mockRandomUUID
+  let mockLogger
+
+  beforeEach(async function () {
+    mockRandomUUID = vi.fn()
+
+    mockFs = {
+      unlink: vi.fn(),
+    }
+
+    mockLogger = {
+      warn: vi.fn(),
+    }
+
+    const mockStorageInstance = { single: vi.fn() }
+    mockMulterInstance = { single: vi.fn() }
+
+    // Create a stub function for multer with diskStorage as a property
+    mockMulter = vi.fn().mockReturnValue(mockMulterInstance)
+    mockMulter.diskStorage = vi.fn().mockReturnValue(mockStorageInstance)
+
+    mockSettings = {
+      multerOptions: {
+        limits: {
+          fileSize: 100 * MB,
+        },
+      },
+    }
+
+    vi.doMock('node:fs', () => ({
+      default: mockFs,
+    }))
+
+    vi.doMock('multer', () => ({
+      default: mockMulter,
+    }))
+
+    vi.doMock('node:crypto', () => ({
+      default: {
+        randomUUID: mockRandomUUID,
+      },
+    }))
+
+    vi.doMock('@overleaf/settings', () => ({
+      default: mockSettings,
+    }))
+
+    vi.doMock('@overleaf/logger', () => ({
+      default: mockLogger,
+    }))
+
+    Multer = await import(modulePath)
+  })
+
+  afterEach(() => {
+    vi.restoreAllMocks()
+    vi.resetModules()
+  })
+
+  describe('multerUploadHandler options merging', function () {
+    it('returns the full multer handler object', function () {
+      const handler = Multer.multerUploadHandler()
+
+      expect(mockMulter).toHaveBeenCalledOnce()
+      expect(handler).toBe(mockMulterInstance)
+    })
+
+    it('uses Settings defaults when options are omitted', function () {
+      Multer.multerUploadHandler()
+
+      expect(mockMulter).toHaveBeenCalledOnce()
+      const config = mockMulter.mock.calls[0][0]
+      expect(config.limits.fileSize).toBe(
+        mockSettings.multerOptions.limits.fileSize
+      )
+    })
+
+    it('uses Settings defaults when options are empty', function () {
+      Multer.multerUploadHandler({})
+
+      expect(mockMulter).toHaveBeenCalledOnce()
+      const config = mockMulter.mock.calls[0][0]
+      expect(config.limits.fileSize).toBe(
+        mockSettings.multerOptions.limits.fileSize
+      )
+    })
+
+    it('preserves provided fileSize from options', function () {
+      const customLimit = 25 * MB
+
+      Multer.multerUploadHandler({
+        limits: { fileSize: customLimit },
+      })
+
+      expect(mockMulter).toHaveBeenCalledOnce()
+      const config = mockMulter.mock.calls[0][0]
+      expect(config.limits.fileSize).toBe(customLimit)
+    })
+  })
+
+  describe('diskStorage configuration', function () {
+    it('creates diskStorage when dest option provided', function () {
+      Multer.multerUploadHandler({
+        dest: '/tmp/uploads',
+      })
+
+      expect(mockMulter.diskStorage).toHaveBeenCalledOnce()
+    })
+
+    it('diskStorage receives destination and filename callback', function () {
+      Multer.multerUploadHandler({
+        dest: '/tmp/uploads',
+      })
+
+      const config = mockMulter.diskStorage.mock.calls[0][0]
+      expect(config.destination).toBeDefined()
+      expect(config.filename).toBeDefined()
+    })
+
+    it('passes storage to multer instead of dest', function () {
+      Multer.multerUploadHandler({
+        dest: '/tmp/uploads',
+      })
+
+      expect(mockMulter).toHaveBeenCalledOnce()
+      const config = mockMulter.mock.calls[0][0]
+      expect(config.storage).toBeDefined()
+      expect(config.dest).toBeUndefined()
+    })
+  })
+
+  describe('filenameWithCleanup callback', function () {
+    let filenameCallback
+    let req
+    let file
+
+    beforeEach(function () {
+      Multer.multerUploadHandler({
+        dest: '/tmp/uploads',
+      })
+
+      const storageConfig = mockMulter.diskStorage.mock.calls[0][0]
+      filenameCallback = storageConfig.filename
+
+      req = {
+        once: vi.fn(),
+        complete: true,
+      }
+      file = {
+        fieldname: 'qqfile',
+        originalname: 'document.pdf',
+      }
+
+      mockRandomUUID.mockReturnValue('test-uuid-1234')
+    })
+
+    it('calls callback with null error and filename', function () {
+      const cb = vi.fn()
+      filenameCallback(req, file, cb)
+      expect(cb).toHaveBeenCalledOnce()
+      const [err, filename] = cb.mock.calls[0]
+      expect(err).toBe(null)
+      expect(filename).toBe('test-uuid-1234')
+    })
+
+    it('sets up request close listener for cleanup', function () {
+      filenameCallback(req, file, vi.fn())
+      expect(req.once).toHaveBeenCalledOnce()
+      const [event] = req.once.mock.calls[0]
+      expect(event).toBe('close')
+    })
+
+    describe('cleanup on request abort', function () {
+      it('does not unlink if request completed successfully', function () {
+        req.complete = true
+        const cb = vi.fn()
+        filenameCallback(req, file, cb)
+
+        const closeHandler = req.once.mock.calls[0][1]
+        closeHandler()
+
+        expect(mockFs.unlink).not.toHaveBeenCalled()
+      })
+
+      it('unlinks file if request was aborted', function () {
+        req.complete = false
+        const cb = vi.fn()
+        mockRandomUUID.mockReturnValue('aborted-file-uuid')
+
+        filenameCallback(req, file, cb)
+
+        const closeHandler = req.once.mock.calls[0][1]
+        closeHandler()
+
+        expect(mockFs.unlink).toHaveBeenCalledOnce()
+        const [path] = mockFs.unlink.mock.calls[0]
+        expect(path).toBe('/tmp/uploads/aborted-file-uuid')
+        expect(mockRandomUUID).toHaveBeenCalledOnce()
+      })
+    })
+  })
+
+  describe('multerErrorHandler', function () {
+    let req, res, next, err
+
+    beforeEach(function () {
+      req = {}
+      res = {}
+      next = vi.fn()
+      err = new Error('some error')
+    })
+
+    it('calls next with the error when req.file is not set', function () {
+      Multer.multerErrorHandler(err, req, res, next)
+
+      expect(mockFs.unlink).not.toHaveBeenCalled()
+      expect(next).toHaveBeenCalledWith(err)
+    })
+
+    it('calls next with the error when req.file has no path', function () {
+      req.file = {}
+      Multer.multerErrorHandler(err, req, res, next)
+
+      expect(mockFs.unlink).not.toHaveBeenCalled()
+      expect(next).toHaveBeenCalledWith(err)
+    })
+
+    it('calls next with the error when req.file.path is empty', function () {
+      req.file = { path: '' }
+      Multer.multerErrorHandler(err, req, res, next)
+
+      expect(mockFs.unlink).not.toHaveBeenCalled()
+      expect(next).toHaveBeenCalledWith(err)
+    })
+
+    it('unlinks req.file.path and calls next with the error', function () {
+      req.file = { path: '/tmp/uploads/some-file' }
+      mockFs.unlink.mockImplementation((path, cb) => cb(null))
+
+      Multer.multerErrorHandler(err, req, res, next)
+
+      expect(mockFs.unlink).toHaveBeenCalledOnce()
+      expect(mockFs.unlink.mock.calls[0][0]).toBe('/tmp/uploads/some-file')
+      expect(next).toHaveBeenCalledWith(err)
+      expect(mockLogger.warn).not.toHaveBeenCalled()
+    })
+
+    it('swallows ENOENT errors from unlink without logging', function () {
+      req.file = { path: '/tmp/uploads/missing-file' }
+      const enoentErr = Object.assign(new Error('not found'), {
+        code: 'ENOENT',
+      })
+      mockFs.unlink.mockImplementation((path, cb) => cb(enoentErr))
+
+      Multer.multerErrorHandler(err, req, res, next)
+
+      expect(mockLogger.warn).not.toHaveBeenCalled()
+      expect(next).toHaveBeenCalledWith(err)
+    })
+
+    it('logs a warning when unlink fails for a reason other than ENOENT', function () {
+      req.file = { path: '/tmp/uploads/locked-file' }
+      const unlinkErr = new Error('permission denied')
+      mockFs.unlink.mockImplementation((path, cb) => cb(unlinkErr))
+
+      Multer.multerErrorHandler(err, req, res, next)
+
+      expect(mockLogger.warn).toHaveBeenCalledOnce()
+      const [fields] = mockLogger.warn.mock.calls[0]
+      expect(fields.err).toBe(unlinkErr)
+      expect(fields.uploadPath).toBe('/tmp/uploads/locked-file')
+      expect(next).toHaveBeenCalledWith(err)
+    })
+  })
+})