From 381ed8539bd14e8099e8d0d9b6c093c751a57b25 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 16 Mar 2026 09:19:48 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=AA=20fix:=20Enforce=20Conversation=20?= =?UTF-8?q?Ownership=20Checks=20in=20Remote=20Agent=20Controllers=20(#1226?= =?UTF-8?q?3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿ”’ fix: Validate conversation ownership in remote agent API endpoints Add user-scoped ownership checks for client-supplied conversation IDs in OpenAI-compatible and Open Responses controllers to prevent cross-tenant file/message loading via IDOR. * ๐Ÿ”’ fix: Harden ownership checks against type confusion and unhandled errors - Add typeof string validation before getConvo to block NoSQL operator injection (e.g. { "$gt": "" }) bypassing the ownership check - Move ownership checks inside try/catch so DB errors produce structured JSON error responses instead of unhandled promise rejections - Add string type validation for conversation_id and previous_response_id in the upstream TS request validators (defense-in-depth) * ๐Ÿงช test: Add coverage for conversation ownership validation in remote agent APIs - Fix broken getConvo mock in openai.spec.js (was missing entirely) - Add tests for: owned conversation, unowned (404), non-string type (400), absent conversation_id (skipped), and DB error (500) โ€” both controllers --- .../agents/__tests__/openai.spec.js | 72 ++++++++++++++ .../agents/__tests__/responses.unit.spec.js | 96 +++++++++++++++++++ api/server/controllers/agents/openai.js | 21 +++- api/server/controllers/agents/responses.js | 21 +++- packages/api/src/agents/openai/service.ts | 8 ++ packages/api/src/agents/responses/service.ts | 7 ++ 6 files changed, 218 insertions(+), 7 deletions(-) diff --git a/api/server/controllers/agents/__tests__/openai.spec.js b/api/server/controllers/agents/__tests__/openai.spec.js index 835343e798..50c61b7288 100644 --- a/api/server/controllers/agents/__tests__/openai.spec.js +++ b/api/server/controllers/agents/__tests__/openai.spec.js @@ -99,6 +99,7 @@ jest.mock('~/server/services/PermissionService', () => ({ jest.mock('~/models/Conversation', () => ({ getConvoFiles: jest.fn().mockResolvedValue([]), + getConvo: jest.fn().mockResolvedValue(null), })); jest.mock('~/models/Agent', () => ({ @@ -160,6 +161,77 @@ describe('OpenAIChatCompletionController', () => { }; }); + describe('conversation ownership validation', () => { + it('should skip ownership check when conversation_id is not provided', async () => { + const { getConvo } = require('~/models/Conversation'); + await OpenAIChatCompletionController(req, res); + expect(getConvo).not.toHaveBeenCalled(); + }); + + it('should return 400 when conversation_id is not a string', async () => { + const { validateRequest } = require('@librechat/api'); + validateRequest.mockReturnValueOnce({ + request: { model: 'agent-123', messages: [], stream: false, conversation_id: { $gt: '' } }, + }); + + await OpenAIChatCompletionController(req, res); + expect(res.status).toHaveBeenCalledWith(400); + }); + + it('should return 404 when conversation is not owned by user', async () => { + const { validateRequest } = require('@librechat/api'); + const { getConvo } = require('~/models/Conversation'); + validateRequest.mockReturnValueOnce({ + request: { + model: 'agent-123', + messages: [], + stream: false, + conversation_id: 'convo-abc', + }, + }); + getConvo.mockResolvedValueOnce(null); + + await OpenAIChatCompletionController(req, res); + expect(getConvo).toHaveBeenCalledWith('user-123', 'convo-abc'); + expect(res.status).toHaveBeenCalledWith(404); + }); + + it('should proceed when conversation is owned by user', async () => { + const { validateRequest } = require('@librechat/api'); + const { getConvo } = require('~/models/Conversation'); + validateRequest.mockReturnValueOnce({ + request: { + model: 'agent-123', + messages: [], + stream: false, + conversation_id: 'convo-abc', + }, + }); + getConvo.mockResolvedValueOnce({ conversationId: 'convo-abc', user: 'user-123' }); + + await OpenAIChatCompletionController(req, res); + expect(getConvo).toHaveBeenCalledWith('user-123', 'convo-abc'); + expect(res.status).not.toHaveBeenCalledWith(404); + }); + + it('should return 500 when getConvo throws a DB error', async () => { + const { validateRequest } = require('@librechat/api'); + const { getConvo } = require('~/models/Conversation'); + validateRequest.mockReturnValueOnce({ + request: { + model: 'agent-123', + messages: [], + stream: false, + conversation_id: 'convo-abc', + }, + }); + getConvo.mockRejectedValueOnce(new Error('DB connection failed')); + + await OpenAIChatCompletionController(req, res); + expect(res.status).toHaveBeenCalledWith(500); + }); + }); + describe('token usage recording', () => { it('should call recordCollectedUsage after successful non-streaming completion', async () => { await OpenAIChatCompletionController(req, res); diff --git a/api/server/controllers/agents/__tests__/responses.unit.spec.js b/api/server/controllers/agents/__tests__/responses.unit.spec.js index 45ec31fc68..e34f0ccf73 100644 --- a/api/server/controllers/agents/__tests__/responses.unit.spec.js +++ b/api/server/controllers/agents/__tests__/responses.unit.spec.js @@ -189,6 +189,102 @@ describe('createResponse controller', () => { }; }); + describe('conversation ownership validation', () => { + it('should skip ownership check when previous_response_id is not provided', async () => { + const { getConvo } = require('~/models/Conversation'); + await createResponse(req, res); + expect(getConvo).not.toHaveBeenCalled(); + }); + + it('should return 400 when previous_response_id is not a string', async () => { + const { validateResponseRequest, sendResponsesErrorResponse } = require('@librechat/api'); + validateResponseRequest.mockReturnValueOnce({ + request: { + model: 'agent-123', + input: 'Hello', + stream: false, + previous_response_id: { $gt: '' }, + }, + }); + + await createResponse(req, res); + expect(sendResponsesErrorResponse).toHaveBeenCalledWith( + res, + 400, + 'previous_response_id must be a string', + 'invalid_request', + ); + }); + + it('should return 404 when conversation is not owned by user', async () => { + const { validateResponseRequest, sendResponsesErrorResponse } = require('@librechat/api'); + const { getConvo } = require('~/models/Conversation'); + validateResponseRequest.mockReturnValueOnce({ + request: { + model: 'agent-123', + input: 'Hello', + stream: false, + previous_response_id: 'resp_abc', + }, + }); + getConvo.mockResolvedValueOnce(null); + + await createResponse(req, res); + expect(getConvo).toHaveBeenCalledWith('user-123', 'resp_abc'); + expect(sendResponsesErrorResponse).toHaveBeenCalledWith( + res, + 404, + 'Conversation not found', + 'not_found', + ); + }); + + it('should proceed when conversation is owned by user', async () => { + const { validateResponseRequest, sendResponsesErrorResponse } = require('@librechat/api'); + const { getConvo } = require('~/models/Conversation'); + validateResponseRequest.mockReturnValueOnce({ + request: { + model: 'agent-123', + input: 'Hello', + stream: false, + previous_response_id: 'resp_abc', + }, + }); + getConvo.mockResolvedValueOnce({ conversationId: 'resp_abc', user: 'user-123' }); + + await createResponse(req, res); + expect(getConvo).toHaveBeenCalledWith('user-123', 'resp_abc'); + expect(sendResponsesErrorResponse).not.toHaveBeenCalledWith( + res, + 404, + expect.any(String), + expect.any(String), + ); + }); + + it('should return 500 when getConvo throws a DB error', async () => { + const { validateResponseRequest, sendResponsesErrorResponse } = require('@librechat/api'); + const { getConvo } = require('~/models/Conversation'); + validateResponseRequest.mockReturnValueOnce({ + request: { + model: 'agent-123', + input: 'Hello', + stream: false, + previous_response_id: 'resp_abc', + }, + }); + getConvo.mockRejectedValueOnce(new Error('DB connection failed')); + + await createResponse(req, res); + expect(sendResponsesErrorResponse).toHaveBeenCalledWith( + res, + 500, + expect.any(String), + expect.any(String), + ); + }); + }); + describe('token usage recording - non-streaming', () => { it('should call recordCollectedUsage after successful non-streaming completion', async () => { await createResponse(req, res); diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index bab81f1535..189cb29d8d 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -26,7 +26,7 @@ const { createToolEndCallback } = require('~/server/controllers/agents/callbacks const { findAccessibleResources } = require('~/server/services/PermissionService'); const { spendTokens, spendStructuredTokens } = require('~/models/spendTokens'); const { getMultiplier, getCacheMultiplier } = require('~/models/tx'); -const { getConvoFiles } = require('~/models/Conversation'); +const { getConvoFiles, getConvo } = require('~/models/Conversation'); const { getAgent, getAgents } = require('~/models/Agent'); const db = require('~/models'); @@ -151,8 +151,6 @@ const OpenAIChatCompletionController = async (req, res) => { } const responseId = `chatcmpl-${nanoid()}`; - const conversationId = request.conversation_id ?? nanoid(); - const parentMessageId = request.parent_message_id ?? null; const created = Math.floor(Date.now() / 1000); /** @type {import('@librechat/api').OpenAIResponseContext} โ€” key must be `requestId` to match the type used by createChunk/buildNonStreamingResponse */ @@ -178,6 +176,23 @@ const OpenAIChatCompletionController = async (req, res) => { }); try { + if (request.conversation_id != null) { + if (typeof request.conversation_id !== 'string') { + return sendErrorResponse( + res, + 400, + 'conversation_id must be a string', + 'invalid_request_error', + ); + } + if (!(await getConvo(req.user?.id, request.conversation_id))) { + return sendErrorResponse(res, 404, 'Conversation not found', 'invalid_request_error'); + } + } + + const conversationId = request.conversation_id ?? nanoid(); + const parentMessageId = request.parent_message_id ?? null; + // Build allowed providers set const allowedProviders = new Set( appConfig?.endpoints?.[EModelEndpoint.agents]?.allowedProviders, diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index bbf02580dd..30ccacdba8 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -292,10 +292,6 @@ const createResponse = async (req, res) => { // Generate IDs const responseId = generateResponseId(); - const conversationId = request.previous_response_id ?? uuidv4(); - const parentMessageId = null; - - // Create response context const context = createResponseContext(request, responseId); logger.debug( @@ -314,6 +310,23 @@ const createResponse = async (req, res) => { }); try { + if (request.previous_response_id != null) { + if (typeof request.previous_response_id !== 'string') { + return sendResponsesErrorResponse( + res, + 400, + 'previous_response_id must be a string', + 'invalid_request', + ); + } + if (!(await getConvo(req.user?.id, request.previous_response_id))) { + return sendResponsesErrorResponse(res, 404, 'Conversation not found', 'not_found'); + } + } + + const conversationId = request.previous_response_id ?? uuidv4(); + const parentMessageId = null; + // Build allowed providers set const allowedProviders = new Set( appConfig?.endpoints?.[EModelEndpoint.agents]?.allowedProviders, diff --git a/packages/api/src/agents/openai/service.ts b/packages/api/src/agents/openai/service.ts index 807ce8db71..90190ce7ce 100644 --- a/packages/api/src/agents/openai/service.ts +++ b/packages/api/src/agents/openai/service.ts @@ -289,6 +289,14 @@ export function validateRequest(body: unknown): ChatCompletionValidationResult { } } + if (request.conversation_id !== undefined && typeof request.conversation_id !== 'string') { + return { valid: false, error: 'conversation_id must be a string' }; + } + + if (request.parent_message_id !== undefined && typeof request.parent_message_id !== 'string') { + return { valid: false, error: 'parent_message_id must be a string' }; + } + return { valid: true, request: request as unknown as ChatCompletionRequest }; } diff --git a/packages/api/src/agents/responses/service.ts b/packages/api/src/agents/responses/service.ts index 2e49b1b979..575606123c 100644 --- a/packages/api/src/agents/responses/service.ts +++ b/packages/api/src/agents/responses/service.ts @@ -84,6 +84,13 @@ export function validateResponseRequest(body: unknown): RequestValidationResult } } + if ( + request.previous_response_id !== undefined && + typeof request.previous_response_id !== 'string' + ) { + return { valid: false, error: 'previous_response_id must be a string' }; + } + return { valid: true, request: request as unknown as ResponseRequest }; }