🔱 chore: Harden API Routes Against IDOR and DoS Attacks (#11760)

* 🔧 feat: Update user key handling in keys route and add comprehensive tests

- Enhanced the PUT /api/keys route to destructure request body for better clarity and maintainability.
- Introduced a new test suite for keys route, covering key update, deletion, and retrieval functionalities, ensuring robust validation and IDOR prevention.
- Added tests to verify handling of extraneous fields and missing optional parameters in requests.

* 🔧 fix: Enhance conversation deletion route with parameter validation

- Updated the DELETE /api/convos route to handle cases where the request body is empty or the 'arg' parameter is null/undefined, returning a 400 status with an appropriate error message for DoS prevention.
- Added corresponding tests to ensure proper validation and error handling for these scenarios, enhancing the robustness of the API.

* 🔧 fix: Improve request body validation in keys and convos routes

- Updated the DELETE /api/convos and PUT /api/keys routes to validate the request body, returning a 400 status for null or invalid bodies to enhance security and prevent potential DoS attacks.
- Added corresponding tests to ensure proper error handling for these scenarios, improving the robustness of the API.

api/server/routes/__tests__/convos.spec.js

@@ -385,6 +385,40 @@ describe('Convos Routes', () => {
       expect(deleteConvoSharedLink).not.toHaveBeenCalled();
     });
 
+    it('should return 400 when request body is empty (DoS prevention)', async () => {
+      const response = await request(app).delete('/api/convos').send({});
+
+      expect(response.status).toBe(400);
+      expect(response.body).toEqual({ error: 'no parameters provided' });
+      expect(deleteConvos).not.toHaveBeenCalled();
+    });
+
+    it('should return 400 when arg is null (DoS prevention)', async () => {
+      const response = await request(app).delete('/api/convos').send({ arg: null });
+
+      expect(response.status).toBe(400);
+      expect(response.body).toEqual({ error: 'no parameters provided' });
+      expect(deleteConvos).not.toHaveBeenCalled();
+    });
+
+    it('should return 400 when arg is undefined (DoS prevention)', async () => {
+      const response = await request(app).delete('/api/convos').send({ arg: undefined });
+
+      expect(response.status).toBe(400);
+      expect(response.body).toEqual({ error: 'no parameters provided' });
+      expect(deleteConvos).not.toHaveBeenCalled();
+    });
+
+    it('should return 400 when request body is null (DoS prevention)', async () => {
+      const response = await request(app)
+        .delete('/api/convos')
+        .set('Content-Type', 'application/json')
+        .send('null');
+
+      expect(response.status).toBe(400);
+      expect(deleteConvos).not.toHaveBeenCalled();
+    });
+
     it('should return 500 if deleteConvoSharedLink fails', async () => {
       const mockConversationId = 'conv-error';
 

api/server/routes/__tests__/keys.spec.js

@@ -0,0 +1,174 @@
+const express = require('express');
+const request = require('supertest');
+
+jest.mock('~/models', () => ({
+  updateUserKey: jest.fn(),
+  deleteUserKey: jest.fn(),
+  getUserKeyExpiry: jest.fn(),
+}));
+
+jest.mock('~/server/middleware/requireJwtAuth', () => (req, res, next) => next());
+
+jest.mock('~/server/middleware', () => ({
+  requireJwtAuth: (req, res, next) => next(),
+}));
+
+describe('Keys Routes', () => {
+  let app;
+  const { updateUserKey, deleteUserKey, getUserKeyExpiry } = require('~/models');
+
+  beforeAll(() => {
+    const keysRouter = require('../keys');
+
+    app = express();
+    app.use(express.json());
+
+    app.use((req, res, next) => {
+      req.user = { id: 'test-user-123' };
+      next();
+    });
+
+    app.use('/api/keys', keysRouter);
+  });
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  describe('PUT /', () => {
+    it('should update a user key with the authenticated user ID', async () => {
+      updateUserKey.mockResolvedValue({});
+
+      const response = await request(app)
+        .put('/api/keys')
+        .send({ name: 'openAI', value: 'sk-test-key-123', expiresAt: '2026-12-31' });
+
+      expect(response.status).toBe(201);
+      expect(updateUserKey).toHaveBeenCalledWith({
+        userId: 'test-user-123',
+        name: 'openAI',
+        value: 'sk-test-key-123',
+        expiresAt: '2026-12-31',
+      });
+      expect(updateUserKey).toHaveBeenCalledTimes(1);
+    });
+
+    it('should not allow userId override via request body (IDOR prevention)', async () => {
+      updateUserKey.mockResolvedValue({});
+
+      const response = await request(app).put('/api/keys').send({
+        userId: 'attacker-injected-id',
+        name: 'openAI',
+        value: 'sk-attacker-key',
+      });
+
+      expect(response.status).toBe(201);
+      expect(updateUserKey).toHaveBeenCalledWith({
+        userId: 'test-user-123',
+        name: 'openAI',
+        value: 'sk-attacker-key',
+        expiresAt: undefined,
+      });
+    });
+
+    it('should ignore extraneous fields from request body', async () => {
+      updateUserKey.mockResolvedValue({});
+
+      const response = await request(app).put('/api/keys').send({
+        name: 'openAI',
+        value: 'sk-test-key',
+        expiresAt: '2026-12-31',
+        _id: 'injected-mongo-id',
+        __v: 99,
+        extra: 'should-be-ignored',
+      });
+
+      expect(response.status).toBe(201);
+      expect(updateUserKey).toHaveBeenCalledWith({
+        userId: 'test-user-123',
+        name: 'openAI',
+        value: 'sk-test-key',
+        expiresAt: '2026-12-31',
+      });
+    });
+
+    it('should handle missing optional fields', async () => {
+      updateUserKey.mockResolvedValue({});
+
+      const response = await request(app)
+        .put('/api/keys')
+        .send({ name: 'anthropic', value: 'sk-ant-key' });
+
+      expect(response.status).toBe(201);
+      expect(updateUserKey).toHaveBeenCalledWith({
+        userId: 'test-user-123',
+        name: 'anthropic',
+        value: 'sk-ant-key',
+        expiresAt: undefined,
+      });
+    });
+
+    it('should return 400 when request body is null', async () => {
+      const response = await request(app)
+        .put('/api/keys')
+        .set('Content-Type', 'application/json')
+        .send('null');
+
+      expect(response.status).toBe(400);
+      expect(updateUserKey).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('DELETE /:name', () => {
+    it('should delete a user key by name', async () => {
+      deleteUserKey.mockResolvedValue({});
+
+      const response = await request(app).delete('/api/keys/openAI');
+
+      expect(response.status).toBe(204);
+      expect(deleteUserKey).toHaveBeenCalledWith({
+        userId: 'test-user-123',
+        name: 'openAI',
+      });
+      expect(deleteUserKey).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  describe('DELETE /', () => {
+    it('should delete all keys when all=true', async () => {
+      deleteUserKey.mockResolvedValue({});
+
+      const response = await request(app).delete('/api/keys?all=true');
+
+      expect(response.status).toBe(204);
+      expect(deleteUserKey).toHaveBeenCalledWith({
+        userId: 'test-user-123',
+        all: true,
+      });
+    });
+
+    it('should return 400 when all query param is not true', async () => {
+      const response = await request(app).delete('/api/keys');
+
+      expect(response.status).toBe(400);
+      expect(response.body).toEqual({ error: 'Specify either all=true to delete.' });
+      expect(deleteUserKey).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('GET /', () => {
+    it('should return key expiry for a given key name', async () => {
+      const mockExpiry = { expiresAt: '2026-12-31' };
+      getUserKeyExpiry.mockResolvedValue(mockExpiry);
+
+      const response = await request(app).get('/api/keys?name=openAI');
+
+      expect(response.status).toBe(200);
+      expect(response.body).toEqual(mockExpiry);
+      expect(getUserKeyExpiry).toHaveBeenCalledWith({
+        userId: 'test-user-123',
+        name: 'openAI',
+      });
+    });
+  });
+});

api/server/routes/convos.js

@@ -98,7 +98,7 @@ router.get('/gen_title/:conversationId', async (req, res) => {
 
 router.delete('/', async (req, res) => {
   let filter = {};
-  const { conversationId, source, thread_id, endpoint } = req.body.arg;
+  const { conversationId, source, thread_id, endpoint } = req.body?.arg ?? {};
 
   // Prevent deletion of all conversations
   if (!conversationId && !source && !thread_id && !endpoint) {
@@ -160,7 +160,7 @@ router.delete('/all', async (req, res) => {
  * @returns {object} 200 - The updated conversation object.
  */
 router.post('/archive', validateConvoAccess, async (req, res) => {
-  const { conversationId, isArchived } = req.body.arg ?? {};
+  const { conversationId, isArchived } = req.body?.arg ?? {};
 
   if (!conversationId) {
     return res.status(400).json({ error: 'conversationId is required' });
@@ -194,7 +194,7 @@ const MAX_CONVO_TITLE_LENGTH = 1024;
  * @returns {object} 201 - The updated conversation object.
  */
 router.post('/update', validateConvoAccess, async (req, res) => {
-  const { conversationId, title } = req.body.arg ?? {};
+  const { conversationId, title } = req.body?.arg ?? {};
 
   if (!conversationId) {
     return res.status(400).json({ error: 'conversationId is required' });

api/server/routes/keys.js

@@ -5,7 +5,11 @@ const { requireJwtAuth } = require('~/server/middleware');
 const router = express.Router();
 
 router.put('/', requireJwtAuth, async (req, res) => {
-  await updateUserKey({ userId: req.user.id, ...req.body });
+  if (req.body == null || typeof req.body !== 'object') {
+    return res.status(400).send({ error: 'Invalid request body.' });
+  }
+  const { name, value, expiresAt } = req.body;
+  await updateUserKey({ userId: req.user.id, name, value, expiresAt });
   res.status(201).send();
 });