mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-19 06:06:34 +01:00
🪪 fix: Enforce Conversation Ownership Checks in Remote Agent Controllers (#12263)
* 🔒 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
This commit is contained in:
parent
951d261f5c
commit
381ed8539b
6 changed files with 218 additions and 7 deletions
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 };
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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 };
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue