🛡️ fix: Enhance File Upload Security & Error Handling (#4705)

* fix: sanitize filename in multer storage callback

* fix: ensure temporary image upload file is deleted after processing

* fix: prevent cleanup flag from being set to false before actually deleted

* refactor: user avatar, typing, use 'file' for formData instead of 'input', add disk storage, use localization

* fix: update Avatar component to include image dimensions in formData and refactor editor reference type

* fix: refactor avatar upload handling to use fs for file reading and enhance file validation

* fix: ensure temporary image upload file is deleted after processing

* fix: refactor avatar upload routes and handlers for agents and assistants, improve file handling and validation

* fix: improve audio file validation and cleanup

* fix: add filename sanitization utility and integrate it into multer storage configuration

* fix: update group project ID check for null and refactor delete prompt group response type

* fix: invalid access control for deleting prompt groups

* fix: add error handling and logging to checkBan middleware

* fix: catch conversation parsing errors

* chore: revert unnecessary height and width parameters from avatar upload

* chore: update librechat-data-provider version to 0.7.55

* style: ensure KaTeX can spread across visible space

api/server/routes/agents/index.js

@@ -9,7 +9,7 @@ const {
   // messageUserLimiter,
 } = require('~/server/middleware');
 
-const v1 = require('./v1');
+const { v1 } = require('./v1');
 const chat = require('./chat');
 
 router.use(requireJwtAuth);

api/server/routes/agents/v1.js

@@ -1,4 +1,3 @@
-const multer = require('multer');
 const express = require('express');
 const { PermissionTypes, Permissions } = require('librechat-data-provider');
 const { requireJwtAuth, generateCheckAccess } = require('~/server/middleware');
@@ -6,8 +5,8 @@ const v1 = require('~/server/controllers/agents/v1');
 const actions = require('./actions');
 const tools = require('./tools');
 
-const upload = multer();
 const router = express.Router();
+const avatar = express.Router();
 
 const checkAgentAccess = generateCheckAccess(PermissionTypes.AGENTS, [Permissions.USE]);
 const checkAgentCreate = generateCheckAccess(PermissionTypes.AGENTS, [
@@ -81,12 +80,12 @@ router.get('/', checkAgentAccess, v1.getListAgents);
 
 /**
  * Uploads and updates an avatar for a specific agent.
- * @route POST /avatar/:agent_id
+ * @route POST /agents/:agent_id/avatar
  * @param {string} req.params.agent_id - The ID of the agent.
  * @param {Express.Multer.File} req.file - The avatar image file.
  * @param {string} [req.body.metadata] - Optional metadata for the agent's avatar.
  * @returns {Object} 200 - success response - application/json
  */
-router.post('/avatar/:agent_id', checkAgentAccess, upload.single('file'), v1.uploadAgentAvatar);
+avatar.post('/:agent_id/avatar/', checkAgentAccess, v1.uploadAgentAvatar);
 
-module.exports = router;
+module.exports = { v1: router, avatar };

api/server/routes/assistants/index.js

@@ -2,7 +2,7 @@ const express = require('express');
 const router = express.Router();
 const { uaParser, checkBan, requireJwtAuth } = require('~/server/middleware');
 
-const v1 = require('./v1');
+const { v1 } = require('./v1');
 const chatV1 = require('./chatV1');
 const v2 = require('./v2');
 const chatV2 = require('./chatV2');

api/server/routes/assistants/v1.js

@@ -1,12 +1,11 @@
-const multer = require('multer');
 const express = require('express');
 const controllers = require('~/server/controllers/assistants/v1');
 const documents = require('./documents');
 const actions = require('./actions');
 const tools = require('./tools');
 
-const upload = multer();
 const router = express.Router();
+const avatar = express.Router();
 
 /**
  * Assistant actions route.
@@ -71,12 +70,12 @@ router.get('/', controllers.listAssistants);
 
 /**
  * Uploads and updates an avatar for a specific assistant.
- * @route POST /avatar/:assistant_id
+ * @route POST /assistants/:assistant_id/avatar/
  * @param {string} req.params.assistant_id - The ID of the assistant.
  * @param {Express.Multer.File} req.file - The avatar image file.
  * @param {string} [req.body.metadata] - Optional metadata for the assistant's avatar.
  * @returns {Object} 200 - success response - application/json
  */
-router.post('/avatar/:assistant_id', upload.single('file'), controllers.uploadAssistantAvatar);
+avatar.post('/:assistant_id/avatar/', controllers.uploadAssistantAvatar);
 
-module.exports = router;
+module.exports = { v1: router, avatar };

api/server/routes/assistants/v2.js

@@ -1,4 +1,3 @@
-const multer = require('multer');
 const express = require('express');
 const v1 = require('~/server/controllers/assistants/v1');
 const v2 = require('~/server/controllers/assistants/v2');
@@ -6,7 +5,6 @@ const documents = require('./documents');
 const actions = require('./actions');
 const tools = require('./tools');
 
-const upload = multer();
 const router = express.Router();
 
 /**
@@ -78,6 +76,6 @@ router.get('/', v1.listAssistants);
  * @param {string} [req.body.metadata] - Optional metadata for the assistant's avatar.
  * @returns {Object} 200 - success response - application/json
  */
-router.post('/avatar/:assistant_id', upload.single('file'), v1.uploadAssistantAvatar);
+router.post('/avatar/:assistant_id', v1.uploadAssistantAvatar);
 
 module.exports = router;

api/server/routes/files/avatar.js

@@ -1,17 +1,18 @@
-const multer = require('multer');
+const fs = require('fs').promises;
 const express = require('express');
 const { getStrategyFunctions } = require('~/server/services/Files/strategies');
 const { resizeAvatar } = require('~/server/services/Files/images/avatar');
+const { filterFile } = require('~/server/services/Files/process');
 const { logger } = require('~/config');
 
-const upload = multer();
 const router = express.Router();
 
-router.post('/', upload.single('input'), async (req, res) => {
+router.post('/', async (req, res) => {
   try {
+    filterFile({ req, file: req.file, image: true, isAvatar: true });
     const userId = req.user.id;
     const { manual } = req.body;
-    const input = req.file.buffer;
+    const input = await fs.readFile(req.file.path);
 
     if (!userId) {
       throw new Error('User ID is undefined');
@@ -33,6 +34,13 @@ router.post('/', upload.single('input'), async (req, res) => {
     const message = 'An error occurred while uploading the profile picture';
     logger.error(message, error);
     res.status(500).json({ message });
+  } finally {
+    try {
+      await fs.unlink(req.file.path);
+      logger.debug('[/files/images/avatar] Temp. image upload file deleted');
+    } catch (error) {
+      logger.debug('[/files/images/avatar] Temp. image upload file already deleted');
+    }
   }
 });
 

api/server/routes/files/files.js

@@ -231,7 +231,6 @@ router.post('/', async (req, res) => {
   } catch (error) {
     let message = 'Error processing file';
     logger.error('[/files] Error processing file:', error);
-    cleanup = false;
 
     if (error.message?.includes('file_ids')) {
       message += ': ' + error.message;
@@ -240,6 +239,7 @@ router.post('/', async (req, res) => {
     // TODO: delete remote file if it exists
     try {
       await fs.unlink(file.path);
+      cleanup = false;
     } catch (error) {
       logger.error('[/files] Error deleting file:', error);
     }

api/server/routes/files/images.js

@@ -30,6 +30,13 @@ router.post('/', async (req, res) => {
       logger.error('[/files/images] Error deleting file:', error);
     }
     res.status(500).json({ message: 'Error processing file' });
+  } finally {
+    try {
+      await fs.unlink(req.file.path);
+      logger.debug('[/files/images] Temp. image upload file deleted');
+    } catch (error) {
+      logger.debug('[/files/images] Temp. image upload file already deleted');
+    }
   }
 });
 

api/server/routes/files/index.js

@@ -1,5 +1,7 @@
 const express = require('express');
 const { uaParser, checkBan, requireJwtAuth, createFileLimiters } = require('~/server/middleware');
+const { avatar: asstAvatarRouter } = require('~/server/routes/assistants/v1');
+const { avatar: agentAvatarRouter } = require('~/server/routes/agents/v1');
 const { createMulterInstance } = require('./multer');
 
 const files = require('./files');
@@ -13,18 +15,25 @@ const initialize = async () => {
   router.use(checkBan);
   router.use(uaParser);
 
+  const upload = await createMulterInstance();
+  router.post('/speech/stt', upload.single('audio'));
+
   /* Important: speech route must be added before the upload limiters */
   router.use('/speech', speech);
 
-  const upload = await createMulterInstance();
   const { fileUploadIpLimiter, fileUploadUserLimiter } = createFileLimiters();
   router.post('*', fileUploadIpLimiter, fileUploadUserLimiter);
   router.post('/', upload.single('file'));
   router.post('/images', upload.single('file'));
+  router.post('/images/avatar', upload.single('file'));
+  router.post('/images/agents/:agent_id/avatar', upload.single('file'));
+  router.post('/images/assistants/:assistant_id/avatar', upload.single('file'));
 
   router.use('/', files);
   router.use('/images', images);
   router.use('/images/avatar', avatar);
+  router.use('/images/agents', agentAvatarRouter);
+  router.use('/images/assistants', asstAvatarRouter);
   return router;
 };
 

api/server/routes/files/multer.js

@@ -3,6 +3,7 @@ const path = require('path');
 const crypto = require('crypto');
 const multer = require('multer');
 const { fileConfig: defaultFileConfig, mergeFileConfig } = require('librechat-data-provider');
+const { sanitizeFilename } = require('~/server/utils/handleText');
 const { getCustomConfig } = require('~/server/services/Config');
 
 const storage = multer.diskStorage({
@@ -16,7 +17,8 @@ const storage = multer.diskStorage({
   filename: function (req, file, cb) {
     req.file_id = crypto.randomUUID();
     file.originalname = decodeURIComponent(file.originalname);
-    cb(null, `${file.originalname}`);
+    const sanitizedFilename = sanitizeFilename(file.originalname);
+    cb(null, sanitizedFilename);
   },
 });
 
@@ -45,6 +47,10 @@ const createFileFilter = (customFileConfig) => {
       return cb(new Error('No file provided'), false);
     }
 
+    if (req.originalUrl.endsWith('/speech/stt') && file.mimetype.startsWith('audio/')) {
+      return cb(null, true);
+    }
+
     const endpoint = req.body.endpoint;
     const supportedTypes =
       customFileConfig?.endpoints?.[endpoint]?.supportedMimeTypes ??

api/server/routes/files/speech/stt.js

@@ -1,13 +1,8 @@
 const express = require('express');
-const router = express.Router();
-const multer = require('multer');
-const { requireJwtAuth } = require('~/server/middleware/');
 const { speechToText } = require('~/server/services/Files/Audio');
 
-const upload = multer();
+const router = express.Router();
 
-router.post('/', requireJwtAuth, upload.single('audio'), async (req, res) => {
-  await speechToText(req, res);
-});
+router.post('/', speechToText);
 
 module.exports = router;

api/server/routes/prompts.js

@@ -214,7 +214,7 @@ const deletePromptController = async (req, res) => {
     const { promptId } = req.params;
     const { groupId } = req.query;
     const author = req.user.id;
-    const query = { promptId, groupId, author, role: req.user.role };
+    const query = { promptId, groupId, author };
     if (req.user.role === SystemRoles.ADMIN) {
       delete query.author;
     }
@@ -226,11 +226,24 @@ const deletePromptController = async (req, res) => {
   }
 };
 
-router.delete('/:promptId', checkPromptCreate, deletePromptController);
+/**
+ * Delete a prompt group
+ * @param {ServerRequest} req
+ * @param {ServerResponse} res
+ * @returns {Promise<TDeletePromptGroupResponse>}
+ */
+const deletePromptGroupController = async (req, res) => {
+  try {
+    const { groupId: _id } = req.params;
+    const message = await deletePromptGroup({ _id, author: req.user.id, role: req.user.role });
+    res.send(message);
+  } catch (error) {
+    logger.error('Error deleting prompt group', error);
+    res.status(500).send({ message: 'Error deleting prompt group' });
+  }
+};
 
-router.delete('/groups/:groupId', checkPromptCreate, async (req, res) => {
-  const { groupId } = req.params;
-  res.status(200).send(await deletePromptGroup(groupId));
-});
+router.delete('/:promptId', checkPromptCreate, deletePromptController);
+router.delete('/groups/:groupId', checkPromptCreate, deletePromptGroupController);
 
 module.exports = router;