diff --git a/services/web/app/src/Features/Uploads/ProjectUploadController.mjs b/services/web/app/src/Features/Uploads/ProjectUploadController.mjs index bfd9a42834..79c83f3383 100644 --- a/services/web/app/src/Features/Uploads/ProjectUploadController.mjs +++ b/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( - defaultsDeep( - { - dest: Settings.path.uploadFolder, - limits: { - fileSize: Settings.maxUploadSize, - }, - }, - Settings.multerOptions - ) -) +const upload = multerUploadHandler({ + dest: Settings.path.uploadFolder, + limits: { + fileSize: Settings.maxUploadSize, + }, +}) /** * @param {any} req diff --git a/services/web/app/src/Features/Uploads/UploadsRouter.mjs b/services/web/app/src/Features/Uploads/UploadsRouter.mjs index ce03ea3040..d3a11d419b 100644 --- a/services/web/app/src/Features/Uploads/UploadsRouter.mjs +++ b/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 ) } }, diff --git a/services/web/app/src/infrastructure/Multer.mjs b/services/web/app/src/infrastructure/Multer.mjs new file mode 100644 index 0000000000..d48d6cf780 --- /dev/null +++ b/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) +} diff --git a/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs b/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs index 8d3fb9def3..9d34563406 100644 --- a/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs +++ b/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs @@ -82,8 +82,9 @@ describe('ProjectUploadController', function () { }, } - vi.doMock('multer', () => ({ - default: sinon.stub(), + vi.doMock('../../../../app/src/infrastructure/Multer.mjs', () => ({ + 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({ - single: sinon.stub().returns(ctx.uploadSingleMiddleware), - }) - multerFn.MulterError = MulterError - - vi.doMock('multer', () => ({ default: multerFn })) + vi.doMock('../../../../app/src/infrastructure/Multer.mjs', () => ({ + multer: { MulterError }, + multerUploadHandler: sinon.stub().returns({ + single: sinon.stub().returns(ctx.uploadSingleMiddleware), + }), + })) vi.doMock('@overleaf/settings', () => ({ default: { path: {} } })) vi.doMock('@overleaf/metrics', () => ({ default: ctx.metrics })) vi.doMock( diff --git a/services/web/test/unit/src/infrastructure/Multer.test.mjs b/services/web/test/unit/src/infrastructure/Multer.test.mjs new file mode 100644 index 0000000000..7cc75f326a --- /dev/null +++ b/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) + }) + }) +})