From ca79a03135cde32eb112b4292ab02f80e83bcc69 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 13 Mar 2026 23:40:44 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9A=A6=20fix:=20Add=20Rate=20Limiting=20t?= =?UTF-8?q?o=20Conversation=20Duplicate=20Endpoint=20(#12218)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add rate limiting to conversation duplicate endpoint * chore: linter * fix: address review findings for conversation duplicate rate limiting * refactor: streamline test mocks for conversation routes - Consolidated mock implementations into a dedicated `convos-route-mocks.js` file to enhance maintainability and readability of test files. - Updated tests in `convos-duplicate-ratelimit.spec.js` and `convos.spec.js` to utilize the new mock structure, improving clarity and reducing redundancy. - Enhanced the `duplicateConversation` function to accept an optional title parameter for better flexibility in conversation duplication. * chore: rename files --- .../middleware/limiters/forkLimiters.js | 2 +- .../__test-utils__/convos-route-mocks.js | 92 ++++++++++++ .../convos-duplicate-ratelimit.spec.js | 135 ++++++++++++++++++ api/server/routes/__tests__/convos.spec.js | 119 +++------------ api/server/routes/convos.js | 3 +- api/server/utils/import/fork.js | 14 +- 6 files changed, 252 insertions(+), 113 deletions(-) create mode 100644 api/server/routes/__test-utils__/convos-route-mocks.js create mode 100644 api/server/routes/__tests__/convos-duplicate-ratelimit.spec.js diff --git a/api/server/middleware/limiters/forkLimiters.js b/api/server/middleware/limiters/forkLimiters.js index e0aa65700c..f1e9b15f11 100644 --- a/api/server/middleware/limiters/forkLimiters.js +++ b/api/server/middleware/limiters/forkLimiters.js @@ -48,7 +48,7 @@ const createForkHandler = (ip = true) => { }; await logViolation(req, res, type, errorMessage, forkViolationScore); - res.status(429).json({ message: 'Too many conversation fork requests. Try again later' }); + res.status(429).json({ message: 'Too many requests. Try again later' }); }; }; diff --git a/api/server/routes/__test-utils__/convos-route-mocks.js b/api/server/routes/__test-utils__/convos-route-mocks.js new file mode 100644 index 0000000000..ca5bafeda9 --- /dev/null +++ b/api/server/routes/__test-utils__/convos-route-mocks.js @@ -0,0 +1,92 @@ +module.exports = { + agents: () => ({ sleep: jest.fn() }), + + api: (overrides = {}) => ({ + isEnabled: jest.fn(), + createAxiosInstance: jest.fn(() => ({ + get: jest.fn(), + post: jest.fn(), + put: jest.fn(), + delete: jest.fn(), + })), + logAxiosError: jest.fn(), + ...overrides, + }), + + dataSchemas: () => ({ + logger: { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }, + createModels: jest.fn(() => ({ + User: {}, + Conversation: {}, + Message: {}, + SharedLink: {}, + })), + }), + + dataProvider: (overrides = {}) => ({ + CacheKeys: { GEN_TITLE: 'GEN_TITLE' }, + EModelEndpoint: { + azureAssistants: 'azureAssistants', + assistants: 'assistants', + }, + ...overrides, + }), + + conversationModel: () => ({ + getConvosByCursor: jest.fn(), + getConvo: jest.fn(), + deleteConvos: jest.fn(), + saveConvo: jest.fn(), + }), + + toolCallModel: () => ({ deleteToolCalls: jest.fn() }), + + sharedModels: () => ({ + deleteAllSharedLinks: jest.fn(), + deleteConvoSharedLink: jest.fn(), + }), + + requireJwtAuth: () => (req, res, next) => next(), + + middlewarePassthrough: () => ({ + createImportLimiters: jest.fn(() => ({ + importIpLimiter: (req, res, next) => next(), + importUserLimiter: (req, res, next) => next(), + })), + createForkLimiters: jest.fn(() => ({ + forkIpLimiter: (req, res, next) => next(), + forkUserLimiter: (req, res, next) => next(), + })), + configMiddleware: (req, res, next) => next(), + validateConvoAccess: (req, res, next) => next(), + }), + + forkUtils: () => ({ + forkConversation: jest.fn(), + duplicateConversation: jest.fn(), + }), + + importUtils: () => ({ importConversations: jest.fn() }), + + logStores: () => jest.fn(), + + multerSetup: () => ({ + storage: {}, + importFileFilter: jest.fn(), + }), + + multerLib: () => + jest.fn(() => ({ + single: jest.fn(() => (req, res, next) => { + req.file = { path: '/tmp/test-file.json' }; + next(); + }), + })), + + assistantEndpoint: () => ({ initializeClient: jest.fn() }), +}; diff --git a/api/server/routes/__tests__/convos-duplicate-ratelimit.spec.js b/api/server/routes/__tests__/convos-duplicate-ratelimit.spec.js new file mode 100644 index 0000000000..788119a569 --- /dev/null +++ b/api/server/routes/__tests__/convos-duplicate-ratelimit.spec.js @@ -0,0 +1,135 @@ +const express = require('express'); +const request = require('supertest'); + +const MOCKS = '../__test-utils__/convos-route-mocks'; + +jest.mock('@librechat/agents', () => require(MOCKS).agents()); +jest.mock('@librechat/api', () => require(MOCKS).api({ limiterCache: jest.fn(() => undefined) })); +jest.mock('@librechat/data-schemas', () => require(MOCKS).dataSchemas()); +jest.mock('librechat-data-provider', () => + require(MOCKS).dataProvider({ ViolationTypes: { FILE_UPLOAD_LIMIT: 'file_upload_limit' } }), +); + +jest.mock('~/cache/logViolation', () => jest.fn().mockResolvedValue(undefined)); +jest.mock('~/cache/getLogStores', () => require(MOCKS).logStores()); +jest.mock('~/models/Conversation', () => require(MOCKS).conversationModel()); +jest.mock('~/models/ToolCall', () => require(MOCKS).toolCallModel()); +jest.mock('~/models', () => require(MOCKS).sharedModels()); +jest.mock('~/server/middleware/requireJwtAuth', () => require(MOCKS).requireJwtAuth()); + +jest.mock('~/server/middleware', () => { + const { createForkLimiters } = jest.requireActual('~/server/middleware/limiters/forkLimiters'); + return { + createImportLimiters: jest.fn(() => ({ + importIpLimiter: (req, res, next) => next(), + importUserLimiter: (req, res, next) => next(), + })), + createForkLimiters, + configMiddleware: (req, res, next) => next(), + validateConvoAccess: (req, res, next) => next(), + }; +}); + +jest.mock('~/server/utils/import/fork', () => require(MOCKS).forkUtils()); +jest.mock('~/server/utils/import', () => require(MOCKS).importUtils()); +jest.mock('~/server/routes/files/multer', () => require(MOCKS).multerSetup()); +jest.mock('multer', () => require(MOCKS).multerLib()); +jest.mock('~/server/services/Endpoints/azureAssistants', () => require(MOCKS).assistantEndpoint()); +jest.mock('~/server/services/Endpoints/assistants', () => require(MOCKS).assistantEndpoint()); + +describe('POST /api/convos/duplicate - Rate Limiting', () => { + let app; + let duplicateConversation; + const savedEnv = {}; + + beforeAll(() => { + savedEnv.FORK_USER_MAX = process.env.FORK_USER_MAX; + savedEnv.FORK_USER_WINDOW = process.env.FORK_USER_WINDOW; + savedEnv.FORK_IP_MAX = process.env.FORK_IP_MAX; + savedEnv.FORK_IP_WINDOW = process.env.FORK_IP_WINDOW; + }); + + afterAll(() => { + for (const key of Object.keys(savedEnv)) { + if (savedEnv[key] === undefined) { + delete process.env[key]; + } else { + process.env[key] = savedEnv[key]; + } + } + }); + + const setupApp = () => { + jest.clearAllMocks(); + jest.isolateModules(() => { + const convosRouter = require('../convos'); + ({ duplicateConversation } = require('~/server/utils/import/fork')); + + app = express(); + app.use(express.json()); + app.use((req, res, next) => { + req.user = { id: 'rate-limit-test-user' }; + next(); + }); + app.use('/api/convos', convosRouter); + }); + + duplicateConversation.mockResolvedValue({ + conversation: { conversationId: 'duplicated-conv' }, + }); + }; + + describe('user limit', () => { + beforeEach(() => { + process.env.FORK_USER_MAX = '2'; + process.env.FORK_USER_WINDOW = '1'; + process.env.FORK_IP_MAX = '100'; + process.env.FORK_IP_WINDOW = '1'; + setupApp(); + }); + + it('should return 429 after exceeding the user rate limit', async () => { + const userMax = parseInt(process.env.FORK_USER_MAX, 10); + + for (let i = 0; i < userMax; i++) { + const res = await request(app) + .post('/api/convos/duplicate') + .send({ conversationId: 'conv-123' }); + expect(res.status).toBe(201); + } + + const res = await request(app) + .post('/api/convos/duplicate') + .send({ conversationId: 'conv-123' }); + expect(res.status).toBe(429); + expect(res.body.message).toMatch(/too many/i); + }); + }); + + describe('IP limit', () => { + beforeEach(() => { + process.env.FORK_USER_MAX = '100'; + process.env.FORK_USER_WINDOW = '1'; + process.env.FORK_IP_MAX = '2'; + process.env.FORK_IP_WINDOW = '1'; + setupApp(); + }); + + it('should return 429 after exceeding the IP rate limit', async () => { + const ipMax = parseInt(process.env.FORK_IP_MAX, 10); + + for (let i = 0; i < ipMax; i++) { + const res = await request(app) + .post('/api/convos/duplicate') + .send({ conversationId: 'conv-123' }); + expect(res.status).toBe(201); + } + + const res = await request(app) + .post('/api/convos/duplicate') + .send({ conversationId: 'conv-123' }); + expect(res.status).toBe(429); + expect(res.body.message).toMatch(/too many/i); + }); + }); +}); diff --git a/api/server/routes/__tests__/convos.spec.js b/api/server/routes/__tests__/convos.spec.js index 931ef006d0..3bdeac32db 100644 --- a/api/server/routes/__tests__/convos.spec.js +++ b/api/server/routes/__tests__/convos.spec.js @@ -1,109 +1,24 @@ const express = require('express'); const request = require('supertest'); -jest.mock('@librechat/agents', () => ({ - sleep: jest.fn(), -})); +const MOCKS = '../__test-utils__/convos-route-mocks'; -jest.mock('@librechat/api', () => ({ - isEnabled: jest.fn(), - createAxiosInstance: jest.fn(() => ({ - get: jest.fn(), - post: jest.fn(), - put: jest.fn(), - delete: jest.fn(), - })), - logAxiosError: jest.fn(), -})); - -jest.mock('@librechat/data-schemas', () => ({ - logger: { - debug: jest.fn(), - info: jest.fn(), - warn: jest.fn(), - error: jest.fn(), - }, - createModels: jest.fn(() => ({ - User: {}, - Conversation: {}, - Message: {}, - SharedLink: {}, - })), -})); - -jest.mock('~/models/Conversation', () => ({ - getConvosByCursor: jest.fn(), - getConvo: jest.fn(), - deleteConvos: jest.fn(), - saveConvo: jest.fn(), -})); - -jest.mock('~/models/ToolCall', () => ({ - deleteToolCalls: jest.fn(), -})); - -jest.mock('~/models', () => ({ - deleteAllSharedLinks: jest.fn(), - deleteConvoSharedLink: jest.fn(), -})); - -jest.mock('~/server/middleware/requireJwtAuth', () => (req, res, next) => next()); - -jest.mock('~/server/middleware', () => ({ - createImportLimiters: jest.fn(() => ({ - importIpLimiter: (req, res, next) => next(), - importUserLimiter: (req, res, next) => next(), - })), - createForkLimiters: jest.fn(() => ({ - forkIpLimiter: (req, res, next) => next(), - forkUserLimiter: (req, res, next) => next(), - })), - configMiddleware: (req, res, next) => next(), - validateConvoAccess: (req, res, next) => next(), -})); - -jest.mock('~/server/utils/import/fork', () => ({ - forkConversation: jest.fn(), - duplicateConversation: jest.fn(), -})); - -jest.mock('~/server/utils/import', () => ({ - importConversations: jest.fn(), -})); - -jest.mock('~/cache/getLogStores', () => jest.fn()); - -jest.mock('~/server/routes/files/multer', () => ({ - storage: {}, - importFileFilter: jest.fn(), -})); - -jest.mock('multer', () => { - return jest.fn(() => ({ - single: jest.fn(() => (req, res, next) => { - req.file = { path: '/tmp/test-file.json' }; - next(); - }), - })); -}); - -jest.mock('librechat-data-provider', () => ({ - CacheKeys: { - GEN_TITLE: 'GEN_TITLE', - }, - EModelEndpoint: { - azureAssistants: 'azureAssistants', - assistants: 'assistants', - }, -})); - -jest.mock('~/server/services/Endpoints/azureAssistants', () => ({ - initializeClient: jest.fn(), -})); - -jest.mock('~/server/services/Endpoints/assistants', () => ({ - initializeClient: jest.fn(), -})); +jest.mock('@librechat/agents', () => require(MOCKS).agents()); +jest.mock('@librechat/api', () => require(MOCKS).api()); +jest.mock('@librechat/data-schemas', () => require(MOCKS).dataSchemas()); +jest.mock('librechat-data-provider', () => require(MOCKS).dataProvider()); +jest.mock('~/models/Conversation', () => require(MOCKS).conversationModel()); +jest.mock('~/models/ToolCall', () => require(MOCKS).toolCallModel()); +jest.mock('~/models', () => require(MOCKS).sharedModels()); +jest.mock('~/server/middleware/requireJwtAuth', () => require(MOCKS).requireJwtAuth()); +jest.mock('~/server/middleware', () => require(MOCKS).middlewarePassthrough()); +jest.mock('~/server/utils/import/fork', () => require(MOCKS).forkUtils()); +jest.mock('~/server/utils/import', () => require(MOCKS).importUtils()); +jest.mock('~/cache/getLogStores', () => require(MOCKS).logStores()); +jest.mock('~/server/routes/files/multer', () => require(MOCKS).multerSetup()); +jest.mock('multer', () => require(MOCKS).multerLib()); +jest.mock('~/server/services/Endpoints/azureAssistants', () => require(MOCKS).assistantEndpoint()); +jest.mock('~/server/services/Endpoints/assistants', () => require(MOCKS).assistantEndpoint()); describe('Convos Routes', () => { let app; diff --git a/api/server/routes/convos.js b/api/server/routes/convos.js index bb9c4ebea9..5f0c35fa0a 100644 --- a/api/server/routes/convos.js +++ b/api/server/routes/convos.js @@ -224,6 +224,7 @@ router.post('/update', validateConvoAccess, async (req, res) => { }); const { importIpLimiter, importUserLimiter } = createImportLimiters(); +/** Fork and duplicate share one rate-limit budget (same "clone" operation class) */ const { forkIpLimiter, forkUserLimiter } = createForkLimiters(); const upload = multer({ storage: storage, fileFilter: importFileFilter }); @@ -280,7 +281,7 @@ router.post('/fork', forkIpLimiter, forkUserLimiter, async (req, res) => { } }); -router.post('/duplicate', async (req, res) => { +router.post('/duplicate', forkIpLimiter, forkUserLimiter, async (req, res) => { const { conversationId, title } = req.body; try { diff --git a/api/server/utils/import/fork.js b/api/server/utils/import/fork.js index c4ce8cb5d4..f896de378c 100644 --- a/api/server/utils/import/fork.js +++ b/api/server/utils/import/fork.js @@ -358,16 +358,15 @@ function splitAtTargetLevel(messages, targetMessageId) { * @param {object} params - The parameters for duplicating the conversation. * @param {string} params.userId - The ID of the user duplicating the conversation. * @param {string} params.conversationId - The ID of the conversation to duplicate. + * @param {string} [params.title] - Optional title override for the duplicate. * @returns {Promise<{ conversation: TConversation, messages: TMessage[] }>} The duplicated conversation and messages. */ -async function duplicateConversation({ userId, conversationId }) { - // Get original conversation +async function duplicateConversation({ userId, conversationId, title }) { const originalConvo = await getConvo(userId, conversationId); if (!originalConvo) { throw new Error('Conversation not found'); } - // Get original messages const originalMessages = await getMessages({ user: userId, conversationId, @@ -383,14 +382,11 @@ async function duplicateConversation({ userId, conversationId }) { cloneMessagesWithTimestamps(messagesToClone, importBatchBuilder); - const result = importBatchBuilder.finishConversation( - originalConvo.title, - new Date(), - originalConvo, - ); + const duplicateTitle = title || originalConvo.title; + const result = importBatchBuilder.finishConversation(duplicateTitle, new Date(), originalConvo); await importBatchBuilder.saveBatch(); logger.debug( - `user: ${userId} | New conversation "${originalConvo.title}" duplicated from conversation ID ${conversationId}`, + `user: ${userId} | New conversation "${duplicateTitle}" duplicated from conversation ID ${conversationId}`, ); const conversation = await getConvo(userId, result.conversation.conversationId);