From b8c31e73146673173ff3a337cb67a22af1d76fe9 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 12 Feb 2026 18:08:24 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=B1=20chore:=20Harden=20API=20Routes?= =?UTF-8?q?=20Against=20IDOR=20and=20DoS=20Attacks=20(#11760)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🔧 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 | 34 ++++ api/server/routes/__tests__/keys.spec.js | 174 +++++++++++++++++++++ api/server/routes/convos.js | 6 +- api/server/routes/keys.js | 6 +- 4 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 api/server/routes/__tests__/keys.spec.js diff --git a/api/server/routes/__tests__/convos.spec.js b/api/server/routes/__tests__/convos.spec.js index ef11b3cbbb..931ef006d0 100644 --- a/api/server/routes/__tests__/convos.spec.js +++ b/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'; diff --git a/api/server/routes/__tests__/keys.spec.js b/api/server/routes/__tests__/keys.spec.js new file mode 100644 index 0000000000..0c96dd3bcb --- /dev/null +++ b/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', + }); + }); + }); +}); diff --git a/api/server/routes/convos.js b/api/server/routes/convos.js index 75b3656f59..bb9c4ebea9 100644 --- a/api/server/routes/convos.js +++ b/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' }); diff --git a/api/server/routes/keys.js b/api/server/routes/keys.js index 620e4d234b..dfd68f69c4 100644 --- a/api/server/routes/keys.js +++ b/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(); });