🔐 fix: Add User Filter to Message Deletion (#12220)

* fix: add user filter to message deletion to prevent IDOR

* refactor: streamline DELETE request syntax in messages-delete test

- Simplified the DELETE request syntax in the messages-delete.spec.js test file by combining multiple lines into a single line for improved readability. This change enhances the clarity of the test code without altering its functionality.

* fix: address review findings for message deletion IDOR fix

* fix: add user filter to message deletion in conversation tests

- Included a user filter in the message deletion test to ensure proper handling of user-specific deletions, enhancing the accuracy of the test case and preventing potential IDOR vulnerabilities.

* chore: lint

api/models/Conversation.js

@@ -228,7 +228,7 @@ module.exports = {
             },
           ],
         };
-      } catch (err) {
+      } catch (_err) {
         logger.warn('[getConvosByCursor] Invalid cursor format, starting from beginning');
       }
       if (cursorFilter) {
@@ -361,6 +361,7 @@ module.exports = {
 
       const deleteMessagesResult = await deleteMessages({
         conversationId: { $in: conversationIds },
+        user,
       });
 
       return { ...deleteConvoResult, messages: deleteMessagesResult };

api/models/Conversation.spec.js

@@ -549,6 +549,7 @@ describe('Conversation Operations', () => {
       expect(result.messages.deletedCount).toBe(5);
       expect(deleteMessages).toHaveBeenCalledWith({
         conversationId: { $in: [mockConversationData.conversationId] },
+        user: 'user123',
       });
 
       // Verify conversation was deleted

api/server/routes/__tests__/messages-delete.spec.js

@@ -0,0 +1,200 @@
+const mongoose = require('mongoose');
+const express = require('express');
+const request = require('supertest');
+const { v4: uuidv4 } = require('uuid');
+const { MongoMemoryServer } = require('mongodb-memory-server');
+
+jest.mock('@librechat/agents', () => ({
+  sleep: jest.fn(),
+}));
+
+jest.mock('@librechat/api', () => ({
+  unescapeLaTeX: jest.fn((x) => x),
+  countTokens: jest.fn().mockResolvedValue(10),
+}));
+
+jest.mock('@librechat/data-schemas', () => ({
+  ...jest.requireActual('@librechat/data-schemas'),
+  logger: {
+    debug: jest.fn(),
+    info: jest.fn(),
+    warn: jest.fn(),
+    error: jest.fn(),
+  },
+}));
+
+jest.mock('librechat-data-provider', () => ({
+  ...jest.requireActual('librechat-data-provider'),
+}));
+
+jest.mock('~/models', () => ({
+  saveConvo: jest.fn(),
+  getMessage: jest.fn(),
+  saveMessage: jest.fn(),
+  getMessages: jest.fn(),
+  updateMessage: jest.fn(),
+  deleteMessages: jest.fn(),
+}));
+
+jest.mock('~/server/services/Artifacts/update', () => ({
+  findAllArtifacts: jest.fn(),
+  replaceArtifactContent: jest.fn(),
+}));
+
+jest.mock('~/server/middleware/requireJwtAuth', () => (req, res, next) => next());
+
+jest.mock('~/server/middleware', () => ({
+  requireJwtAuth: (req, res, next) => next(),
+  validateMessageReq: (req, res, next) => next(),
+}));
+
+jest.mock('~/models/Conversation', () => ({
+  getConvosQueried: jest.fn(),
+}));
+
+jest.mock('~/db/models', () => ({
+  Message: {
+    findOne: jest.fn(),
+    find: jest.fn(),
+    meiliSearch: jest.fn(),
+  },
+}));
+
+/* ─── Model-level tests: real MongoDB, proves cross-user deletion is prevented ─── */
+
+const { messageSchema } = require('@librechat/data-schemas');
+
+describe('deleteMessages – model-level IDOR prevention', () => {
+  let mongoServer;
+  let Message;
+
+  const ownerUserId = 'user-owner-111';
+  const attackerUserId = 'user-attacker-222';
+
+  beforeAll(async () => {
+    mongoServer = await MongoMemoryServer.create();
+    Message = mongoose.models.Message || mongoose.model('Message', messageSchema);
+    await mongoose.connect(mongoServer.getUri());
+  });
+
+  afterAll(async () => {
+    await mongoose.disconnect();
+    await mongoServer.stop();
+  });
+
+  beforeEach(async () => {
+    await Message.deleteMany({});
+  });
+
+  it("should NOT delete another user's message when attacker supplies victim messageId", async () => {
+    const conversationId = uuidv4();
+    const victimMsgId = 'victim-msg-001';
+
+    await Message.create({
+      messageId: victimMsgId,
+      conversationId,
+      user: ownerUserId,
+      text: 'Sensitive owner data',
+    });
+
+    await Message.deleteMany({ messageId: victimMsgId, user: attackerUserId });
+
+    const victimMsg = await Message.findOne({ messageId: victimMsgId }).lean();
+    expect(victimMsg).not.toBeNull();
+    expect(victimMsg.user).toBe(ownerUserId);
+    expect(victimMsg.text).toBe('Sensitive owner data');
+  });
+
+  it("should delete the user's own message", async () => {
+    const conversationId = uuidv4();
+    const ownMsgId = 'own-msg-001';
+
+    await Message.create({
+      messageId: ownMsgId,
+      conversationId,
+      user: ownerUserId,
+      text: 'My message',
+    });
+
+    const result = await Message.deleteMany({ messageId: ownMsgId, user: ownerUserId });
+    expect(result.deletedCount).toBe(1);
+
+    const deleted = await Message.findOne({ messageId: ownMsgId }).lean();
+    expect(deleted).toBeNull();
+  });
+
+  it('should scope deletion by conversationId, messageId, and user together', async () => {
+    const convoA = uuidv4();
+    const convoB = uuidv4();
+
+    await Message.create([
+      { messageId: 'msg-a1', conversationId: convoA, user: ownerUserId, text: 'A1' },
+      { messageId: 'msg-b1', conversationId: convoB, user: ownerUserId, text: 'B1' },
+    ]);
+
+    await Message.deleteMany({ messageId: 'msg-a1', conversationId: convoA, user: attackerUserId });
+
+    const remaining = await Message.find({ user: ownerUserId }).lean();
+    expect(remaining).toHaveLength(2);
+  });
+});
+
+/* ─── Route-level tests: supertest + mocked deleteMessages ─── */
+
+describe('DELETE /:conversationId/:messageId – route handler', () => {
+  let app;
+  const { deleteMessages } = require('~/models');
+
+  const authenticatedUserId = 'user-owner-123';
+
+  beforeAll(() => {
+    const messagesRouter = require('../messages');
+
+    app = express();
+    app.use(express.json());
+    app.use((req, res, next) => {
+      req.user = { id: authenticatedUserId };
+      next();
+    });
+    app.use('/api/messages', messagesRouter);
+  });
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('should pass user and conversationId in the deleteMessages filter', async () => {
+    deleteMessages.mockResolvedValue({ deletedCount: 1 });
+
+    await request(app).delete('/api/messages/convo-1/msg-1');
+
+    expect(deleteMessages).toHaveBeenCalledTimes(1);
+    expect(deleteMessages).toHaveBeenCalledWith({
+      messageId: 'msg-1',
+      conversationId: 'convo-1',
+      user: authenticatedUserId,
+    });
+  });
+
+  it('should return 204 on successful deletion', async () => {
+    deleteMessages.mockResolvedValue({ deletedCount: 1 });
+
+    const response = await request(app).delete('/api/messages/convo-1/msg-owned');
+
+    expect(response.status).toBe(204);
+    expect(deleteMessages).toHaveBeenCalledWith({
+      messageId: 'msg-owned',
+      conversationId: 'convo-1',
+      user: authenticatedUserId,
+    });
+  });
+
+  it('should return 500 when deleteMessages throws', async () => {
+    deleteMessages.mockRejectedValue(new Error('DB failure'));
+
+    const response = await request(app).delete('/api/messages/convo-1/msg-1');
+
+    expect(response.status).toBe(500);
+    expect(response.body).toEqual({ error: 'Internal server error' });
+  });
+});

api/server/routes/messages.js

@@ -404,8 +404,8 @@ router.put('/:conversationId/:messageId/feedback', validateMessageReq, async (re
 
 router.delete('/:conversationId/:messageId', validateMessageReq, async (req, res) => {
   try {
-    const { messageId } = req.params;
+    const { conversationId, messageId } = req.params;
-    await deleteMessages({ messageId });
+    await deleteMessages({ messageId, conversationId, user: req.user.id });
     res.status(204).send();
   } catch (error) {
     logger.error('Error deleting message:', error);