From 5b66196f5839e304699f0b54d76bf631a406dad5 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 18 May 2026 17:34:30 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=AA=20fix:=20Scope=20Message=20Convers?= =?UTF-8?q?ation=20Access=20(#13183)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Scope message conversation access * style: Format message route query --- api/app/clients/BaseClient.js | 2 +- api/app/clients/specs/BaseClient.test.js | 25 +++++- .../__tests__/validateMessageReq.spec.js | 74 ++++++++++++++++ api/server/middleware/validateMessageReq.js | 20 +++-- .../routes/__tests__/messages-delete.spec.js | 85 +++++++++++++++++++ api/server/routes/messages.js | 11 ++- api/server/services/Threads/manage.js | 2 +- .../src/methods/conversation.spec.ts | 2 +- .../data-schemas/src/methods/conversation.ts | 2 +- 9 files changed, 208 insertions(+), 15 deletions(-) create mode 100644 api/server/middleware/__tests__/validateMessageReq.spec.js diff --git a/api/app/clients/BaseClient.js b/api/app/clients/BaseClient.js index e641017c74..d36e54f015 100644 --- a/api/app/clients/BaseClient.js +++ b/api/app/clients/BaseClient.js @@ -707,7 +707,7 @@ class BaseClient { async loadHistory(conversationId, parentMessageId = null) { logger.debug('[BaseClient] Loading history:', { conversationId, parentMessageId }); - const messages = (await db.getMessages({ conversationId })) ?? []; + const messages = (await db.getMessages({ conversationId, user: this.user })) ?? []; if (messages.length === 0) { return []; diff --git a/api/app/clients/specs/BaseClient.test.js b/api/app/clients/specs/BaseClient.test.js index eb6ae656e9..1e8fc13ba2 100644 --- a/api/app/clients/specs/BaseClient.test.js +++ b/api/app/clients/specs/BaseClient.test.js @@ -1,5 +1,5 @@ const { Constants } = require('librechat-data-provider'); -const { initializeFakeClient } = require('./FakeClient'); +const { FakeClient, initializeFakeClient } = require('./FakeClient'); jest.mock('~/db/connect'); jest.mock('~/server/services/Config', () => ({ @@ -38,7 +38,7 @@ jest.mock('~/models', () => ({ updateFileUsage: jest.fn(), })); -const { getConvo, saveConvo, saveMessage } = require('~/models'); +const { getConvo, getMessages, saveConvo, saveMessage } = require('~/models'); jest.mock('@librechat/agents', () => { const actual = jest.requireActual('@librechat/agents'); @@ -622,6 +622,27 @@ describe('BaseClient', () => { expect(chatMessages2[chatMessages2.length - 1].text).toEqual("What's up"); }); + test('loadHistory should scope database reads to the current user', async () => { + const user = 'user-123'; + TestClient = new FakeClient(apiKey, options); + TestClient.user = user; + getMessages.mockResolvedValueOnce([ + { + role: 'user', + isCreatedByUser: true, + text: 'Hello', + messageId: '1', + conversationId, + }, + ]); + + const chatMessages = await TestClient.loadHistory(conversationId, '1'); + + expect(getMessages).toHaveBeenCalledWith({ conversationId, user }); + expect(chatMessages).toHaveLength(1); + expect(chatMessages[0].text).toBe('Hello'); + }); + /* Most of the new sendMessage logic revolving around edited/continued AI messages * can be summarized by the following test. The condition will load the entire history up to * the message that is being edited, which will trigger the AI API to 'continue' the response. diff --git a/api/server/middleware/__tests__/validateMessageReq.spec.js b/api/server/middleware/__tests__/validateMessageReq.spec.js new file mode 100644 index 0000000000..861fe969c2 --- /dev/null +++ b/api/server/middleware/__tests__/validateMessageReq.spec.js @@ -0,0 +1,74 @@ +jest.mock('~/models', () => ({ + getConvo: jest.fn(), +})); + +const validateMessageReq = require('../validateMessageReq'); +const { getConvo } = require('~/models'); + +function createResponse() { + const res = { + json: jest.fn(), + send: jest.fn(), + status: jest.fn(), + }; + res.status.mockReturnValue(res); + return res; +} + +describe('validateMessageReq', () => { + const userId = 'user-123'; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should reject requests when URL and body conversationId values differ', async () => { + const req = { + params: { conversationId: 'convo-owned' }, + body: { conversationId: 'convo-victim' }, + user: { id: userId }, + }; + const res = createResponse(); + const next = jest.fn(); + + await validateMessageReq(req, res, next); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith({ error: 'Conversation ID mismatch' }); + expect(getConvo).not.toHaveBeenCalled(); + expect(next).not.toHaveBeenCalled(); + }); + + it('should reject requests when URL and nested message conversationId values differ', async () => { + const req = { + params: { conversationId: 'convo-owned' }, + body: { message: { conversationId: 'convo-victim' } }, + user: { id: userId }, + }; + const res = createResponse(); + const next = jest.fn(); + + await validateMessageReq(req, res, next); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith({ error: 'Conversation ID mismatch' }); + expect(getConvo).not.toHaveBeenCalled(); + expect(next).not.toHaveBeenCalled(); + }); + + it('should validate ownership against the URL conversationId when values match', async () => { + const req = { + params: { conversationId: 'convo-owned' }, + body: { conversationId: 'convo-owned' }, + user: { id: userId }, + }; + const res = createResponse(); + const next = jest.fn(); + getConvo.mockResolvedValue({ conversationId: 'convo-owned', user: userId }); + + await validateMessageReq(req, res, next); + + expect(getConvo).toHaveBeenCalledWith(userId, 'convo-owned'); + expect(next).toHaveBeenCalledTimes(1); + }); +}); diff --git a/api/server/middleware/validateMessageReq.js b/api/server/middleware/validateMessageReq.js index 430444a172..2e3ed56bcd 100644 --- a/api/server/middleware/validateMessageReq.js +++ b/api/server/middleware/validateMessageReq.js @@ -2,16 +2,26 @@ const { getConvo } = require('~/models'); // Middleware to validate conversationId and user relationship const validateMessageReq = async (req, res, next) => { - let conversationId = req.params.conversationId || req.body.conversationId; + const body = req.body ?? {}; + const paramConversationId = req.params?.conversationId; + const bodyConversationId = body.conversationId; + const nestedConversationId = body.message?.conversationId; + + if ( + (paramConversationId && + ((bodyConversationId && paramConversationId !== bodyConversationId) || + (nestedConversationId && paramConversationId !== nestedConversationId))) || + (bodyConversationId && nestedConversationId && bodyConversationId !== nestedConversationId) + ) { + return res.status(400).json({ error: 'Conversation ID mismatch' }); + } + + const conversationId = paramConversationId || bodyConversationId || nestedConversationId; if (conversationId === 'new') { return res.status(200).send([]); } - if (!conversationId && req.body.message) { - conversationId = req.body.message.conversationId; - } - const conversation = await getConvo(req.user.id, conversationId); if (!conversation) { diff --git a/api/server/routes/__tests__/messages-delete.spec.js b/api/server/routes/__tests__/messages-delete.spec.js index 714d497719..36c4e8e9e6 100644 --- a/api/server/routes/__tests__/messages-delete.spec.js +++ b/api/server/routes/__tests__/messages-delete.spec.js @@ -197,3 +197,88 @@ describe('DELETE /:conversationId/:messageId – route handler', () => { expect(response.body).toEqual({ error: 'Internal server error' }); }); }); + +describe('message route conversation ownership filters', () => { + let app; + const { getMessages, saveConvo, saveMessage } = 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 save POST messages with the validated URL conversationId', async () => { + const urlConversationId = '11111111-1111-4111-8111-111111111111'; + const bodyConversationId = '22222222-2222-4222-8222-222222222222'; + const savedMessage = { + messageId: 'message-1', + conversationId: urlConversationId, + text: 'hello', + user: authenticatedUserId, + }; + + saveMessage.mockResolvedValue(savedMessage); + saveConvo.mockResolvedValue({ conversationId: urlConversationId }); + + const response = await request(app).post(`/api/messages/${urlConversationId}`).send({ + messageId: savedMessage.messageId, + conversationId: bodyConversationId, + text: savedMessage.text, + }); + + expect(response.status).toBe(201); + expect(saveMessage).toHaveBeenCalledWith( + expect.objectContaining({ userId: authenticatedUserId }), + expect.objectContaining({ + messageId: savedMessage.messageId, + conversationId: urlConversationId, + text: savedMessage.text, + user: authenticatedUserId, + }), + { context: 'POST /api/messages/:conversationId' }, + ); + expect(saveMessage.mock.calls[0][1].conversationId).not.toBe(bodyConversationId); + expect(saveConvo).toHaveBeenCalledWith( + expect.objectContaining({ userId: authenticatedUserId }), + savedMessage, + { context: 'POST /api/messages/:conversationId' }, + ); + }); + + it('should filter conversation message reads by authenticated user', async () => { + getMessages.mockResolvedValue([{ messageId: 'message-1', conversationId: 'convo-1' }]); + + const response = await request(app).get('/api/messages/convo-1'); + + expect(response.status).toBe(200); + expect(getMessages).toHaveBeenCalledWith( + { conversationId: 'convo-1', user: authenticatedUserId }, + '-_id -__v -user', + ); + }); + + it('should filter single message reads by authenticated user', async () => { + getMessages.mockResolvedValue([{ messageId: 'message-1', conversationId: 'convo-1' }]); + + const response = await request(app).get('/api/messages/convo-1/message-1'); + + expect(response.status).toBe(200); + expect(getMessages).toHaveBeenCalledWith( + { conversationId: 'convo-1', messageId: 'message-1', user: authenticatedUserId }, + '-_id -__v -user', + ); + }); +}); diff --git a/api/server/routes/messages.js b/api/server/routes/messages.js index 21b2b23fea..a07293c0e2 100644 --- a/api/server/routes/messages.js +++ b/api/server/routes/messages.js @@ -269,7 +269,7 @@ router.post('/artifact/:messageId', async (req, res) => { router.get('/:conversationId', validateMessageReq, async (req, res) => { try { const { conversationId } = req.params; - const messages = await db.getMessages({ conversationId }, '-_id -__v -user'); + const messages = await db.getMessages({ conversationId, user: req.user.id }, '-_id -__v -user'); res.status(200).json(messages); } catch (error) { logger.error('Error fetching messages:', error); @@ -279,7 +279,7 @@ router.get('/:conversationId', validateMessageReq, async (req, res) => { router.post('/:conversationId', validateMessageReq, async (req, res) => { try { - const message = req.body; + const message = { ...req.body, conversationId: req.params.conversationId }; const reqCtx = { userId: req?.user?.id, isTemporary: req?.body?.isTemporary, @@ -304,7 +304,10 @@ router.post('/:conversationId', validateMessageReq, async (req, res) => { router.get('/:conversationId/:messageId', validateMessageReq, async (req, res) => { try { const { conversationId, messageId } = req.params; - const message = await db.getMessages({ conversationId, messageId }, '-_id -__v -user'); + const message = await db.getMessages( + { conversationId, messageId, user: req.user.id }, + '-_id -__v -user', + ); if (!message) { return res.status(404).json({ error: 'Message not found' }); } @@ -331,7 +334,7 @@ router.put('/:conversationId/:messageId', validateMessageReq, async (req, res) = } const message = ( - await db.getMessages({ conversationId, messageId }, 'content tokenCount') + await db.getMessages({ conversationId, messageId, user: req.user.id }, 'content tokenCount') )?.[0]; if (!message) { return res.status(404).json({ error: 'Message not found' }); diff --git a/api/server/services/Threads/manage.js b/api/server/services/Threads/manage.js index 27520f38a5..772cbd977b 100644 --- a/api/server/services/Threads/manage.js +++ b/api/server/services/Threads/manage.js @@ -467,7 +467,7 @@ async function checkMessageGaps({ apiMessages.push(currentMessage); } - const dbMessages = await getMessages({ conversationId }); + const dbMessages = await getMessages({ conversationId, user: openai.req.user.id }); const assistant_id = dbMessages?.[0]?.model; const syncedMessages = await syncMessages({ diff --git a/packages/data-schemas/src/methods/conversation.spec.ts b/packages/data-schemas/src/methods/conversation.spec.ts index 0b6e0857cd..166e5b51c8 100644 --- a/packages/data-schemas/src/methods/conversation.spec.ts +++ b/packages/data-schemas/src/methods/conversation.spec.ts @@ -136,7 +136,7 @@ describe('Conversation Operations', () => { // Verify that getMessages was called with correct parameters expect(getMessages).toHaveBeenCalledWith( - { conversationId: mockConversationData.conversationId }, + { conversationId: mockConversationData.conversationId, user: mockCtx.userId }, '_id', ); }); diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index f02ea8dbe0..82c22f3947 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -178,7 +178,7 @@ export function createConversationMethods( logger.debug(`[saveConvo] ${metadata.context}`); } - const messages = await getMessages({ conversationId }, '_id'); + const messages = await getMessages({ conversationId, user: userId }, '_id'); const update: Record = { ...convo, messages, user: userId }; if (newConversationId) {