diff --git a/api/app/clients/BaseClient.js b/api/app/clients/BaseClient.js index 5da4b34da4..1cacd4666e 100644 --- a/api/app/clients/BaseClient.js +++ b/api/app/clients/BaseClient.js @@ -1149,6 +1149,11 @@ class BaseClient { async loadHistory(conversationId, parentMessageId = null) { logger.debug('[BaseClient] Loading history:', { conversationId, parentMessageId }); + /** No message has the root sentinel as its id, so the chain walk from it is empty. */ + if (parentMessageId === Constants.NO_PARENT) { + return []; + } + const messages = (await db.getMessages({ conversationId, user: this.user })) ?? []; if (messages.length === 0) { @@ -1389,15 +1394,19 @@ class BaseClient { const orderedMessages = []; let currentMessageId = parentMessageId; const visitedMessageIds = new Set(); + const messagesById = new Map(); + for (const msg of messages) { + const messageId = msg.messageId ?? msg.id; + if (!messagesById.has(messageId)) { + messagesById.set(messageId, msg); + } + } while (currentMessageId) { if (visitedMessageIds.has(currentMessageId)) { break; } - const message = messages.find((msg) => { - const messageId = msg.messageId ?? msg.id; - return messageId === currentMessageId; - }); + const message = messagesById.get(currentMessageId); visitedMessageIds.add(currentMessageId); diff --git a/api/app/clients/specs/BaseClient.test.js b/api/app/clients/specs/BaseClient.test.js index af6a09a208..bf0e025926 100644 --- a/api/app/clients/specs/BaseClient.test.js +++ b/api/app/clients/specs/BaseClient.test.js @@ -1,4 +1,5 @@ const { Constants, ContentTypes } = require('librechat-data-provider'); +const BaseClientClass = require('../BaseClient'); const { ContentFilterError } = require('@librechat/api'); const { FakeClient, initializeFakeClient } = require('./FakeClient'); @@ -226,6 +227,39 @@ describe('BaseClient', () => { expect(result.messagesToRefine).toEqual(expectedMessagesToRefine); }); + describe('loadHistory', () => { + const receiver = Object.assign(Object.create(BaseClientClass.prototype), { + user: 'user-1', + getMessageMapMethod: null, + shouldSummarize: false, + addPreviousAttachments: async (messages) => messages, + }); + const loadHistory = (parentMessageId) => receiver.loadHistory('convo-1', parentMessageId); + + beforeEach(() => { + getMessages.mockClear(); + }); + + test('skips the database when the parent is the root sentinel: no message can match it', async () => { + const result = await loadHistory(Constants.NO_PARENT); + + expect(result).toEqual([]); + expect(getMessages).not.toHaveBeenCalled(); + }); + + test('still loads and walks the chain for a real parent', async () => { + getMessages.mockResolvedValueOnce([ + { messageId: 'root', parentMessageId: Constants.NO_PARENT, text: 'a' }, + { messageId: 'reply', parentMessageId: 'root', text: 'b' }, + ]); + + const result = await loadHistory('reply'); + + expect(getMessages).toHaveBeenCalledTimes(1); + expect(result.map((m) => m.messageId)).toEqual(['root', 'reply']); + }); + }); + describe('getMessagesForConversation', () => { it('should return an empty array if the parentMessageId does not exist', () => { const result = TestClient.constructor.getMessagesForConversation({ diff --git a/api/server/middleware/validate/convoAccess.js b/api/server/middleware/validate/convoAccess.js index ef1eea8f37..929b43dc1e 100644 --- a/api/server/middleware/validate/convoAccess.js +++ b/api/server/middleware/validate/convoAccess.js @@ -1,4 +1,5 @@ const { isEnabled } = require('@librechat/api'); +const { logger } = require('@librechat/data-schemas'); const { Constants, ViolationTypes, Time } = require('librechat-data-provider'); const denyRequest = require('~/server/middleware/denyRequest'); const { logViolation, getLogStores } = require('~/cache'); @@ -51,9 +52,13 @@ const validateConvoAccess = async (req, res, next) => { } } - const conversation = await searchConversation(conversationId); + /** One read serves the subagent guard, agent initialization, and the first save via + * `req.resolvedConversation`. `messages` is the only unbounded field and no consumer + * reads it, so it stays excluded — ownership is not yet known at this point. */ + const conversation = await searchConversation(conversationId, '-messages'); if (!conversation) { + req.resolvedConversation = null; return next(); } @@ -70,8 +75,13 @@ const validateConvoAccess = async (req, res, next) => { } if (cache) { - await cache.set(key, 'authorized', Time.TEN_MINUTES); + /** The marker only short-circuits the next check; the violations store is file-backed + * without Redis and its debounced write takes ~100ms, so it must not gate this request. */ + cache.set(key, 'authorized', Time.TEN_MINUTES).catch((error) => { + logger.warn('[validateConvoAccess] Failed to cache conversation access', error); + }); } + req.resolvedConversation = conversation; next(); } catch (error) { console.error('Error validating conversation access:', error); diff --git a/api/server/middleware/validate/convoAccess.spec.js b/api/server/middleware/validate/convoAccess.spec.js new file mode 100644 index 0000000000..49a751e7eb --- /dev/null +++ b/api/server/middleware/validate/convoAccess.spec.js @@ -0,0 +1,126 @@ +const mongoose = require('mongoose'); +const { MongoMemoryServer } = require('mongodb-memory-server'); +const { ViolationTypes } = require('librechat-data-provider'); + +const mockCache = { + get: jest.fn(), + set: jest.fn(), +}; + +jest.mock('~/cache', () => ({ + getLogStores: jest.fn(() => mockCache), + logViolation: jest.fn(), +})); + +jest.mock('~/server/middleware/denyRequest', () => jest.fn(async () => undefined)); + +const denyRequest = require('~/server/middleware/denyRequest'); +const { Conversation } = require('~/db/models'); +const validateConvoAccess = require('./convoAccess'); + +const OWNER_ID = new mongoose.Types.ObjectId().toString(); +const OTHER_ID = new mongoose.Types.ObjectId().toString(); +const CONVERSATION_ID = 'conversation-under-test'; + +function createRequest(userId, conversationId) { + return { + user: { id: userId }, + body: { conversationId, text: 'hello' }, + }; +} + +describe('validateConvoAccess', () => { + let mongoServer; + let res; + let next; + + beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + await mongoose.connect(mongoServer.getUri()); + await Conversation.create({ + conversationId: CONVERSATION_ID, + user: OWNER_ID, + endpoint: 'agents', + title: 'Owned conversation', + files: ['file-1', 'file-2'], + messages: Array.from({ length: 50 }, () => new mongoose.Types.ObjectId()), + }); + }); + + afterAll(async () => { + await mongoose.disconnect(); + await mongoServer.stop(); + }); + + beforeEach(() => { + mockCache.get.mockReset().mockResolvedValue(undefined); + mockCache.set.mockReset().mockResolvedValue(true); + denyRequest.mockClear(); + res = { status: jest.fn().mockReturnThis(), json: jest.fn() }; + next = jest.fn(); + }); + + it('stashes the full conversation document for downstream readers when access is granted', async () => { + const req = createRequest(OWNER_ID, CONVERSATION_ID); + + await validateConvoAccess(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(denyRequest).not.toHaveBeenCalled(); + expect(req.resolvedConversation).toMatchObject({ + conversationId: CONVERSATION_ID, + user: OWNER_ID, + title: 'Owned conversation', + files: ['file-1', 'file-2'], + }); + expect(req.resolvedConversation).not.toHaveProperty('messages'); + }); + + it('stashes null when the conversation does not exist so later readers skip their own lookup', async () => { + const req = createRequest(OWNER_ID, 'never-created'); + + await validateConvoAccess(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(Object.prototype.hasOwnProperty.call(req, 'resolvedConversation')).toBe(true); + expect(req.resolvedConversation).toBeNull(); + }); + + it("denies another user's conversation without exposing the document on the request", async () => { + const req = createRequest(OTHER_ID, CONVERSATION_ID); + + await validateConvoAccess(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(denyRequest).toHaveBeenCalledTimes(1); + expect(Object.prototype.hasOwnProperty.call(req, 'resolvedConversation')).toBe(false); + }); + + it('does not wait for the access marker to be written before continuing', async () => { + mockCache.set.mockImplementation(() => new Promise(() => undefined)); + const req = createRequest(OWNER_ID, CONVERSATION_ID); + + await validateConvoAccess(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(mockCache.set).toHaveBeenCalledWith( + expect.stringContaining(`${OWNER_ID}:${CONVERSATION_ID}`), + 'authorized', + expect.any(Number), + ); + }); + + it('skips the database entirely when access is already cached', async () => { + mockCache.get.mockResolvedValue('authorized'); + const findOne = jest.spyOn(Conversation, 'findOne'); + const req = createRequest(OWNER_ID, CONVERSATION_ID); + + await validateConvoAccess(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(findOne).not.toHaveBeenCalled(); + expect(Object.prototype.hasOwnProperty.call(req, 'resolvedConversation')).toBe(false); + expect(require('~/cache').getLogStores).toHaveBeenCalledWith(ViolationTypes.CONVO_ACCESS); + findOne.mockRestore(); + }); +}); diff --git a/api/server/routes/agents/v1.js b/api/server/routes/agents/v1.js index 1977ee2875..0a6487b574 100644 --- a/api/server/routes/agents/v1.js +++ b/api/server/routes/agents/v1.js @@ -1,7 +1,7 @@ const express = require('express'); const { generateCheckAccess } = require('@librechat/api'); const { PermissionTypes, Permissions, PermissionBits } = require('librechat-data-provider'); -const { requireJwtAuth, configMiddleware, canAccessAgentResource } = require('~/server/middleware'); +const { configMiddleware, canAccessAgentResource } = require('~/server/middleware'); const v1 = require('~/server/controllers/agents/v1'); const { getRoleByName } = require('~/models'); const actions = require('./actions'); @@ -21,8 +21,6 @@ const checkAgentCreate = generateCheckAccess({ getRoleByName, }); -router.use(requireJwtAuth); - /** * Agent actions route. * @route GET|POST /agents/actions diff --git a/packages/api/src/agents/guard.spec.ts b/packages/api/src/agents/guard.spec.ts index 8ad2979ba7..07cf516ea3 100644 --- a/packages/api/src/agents/guard.spec.ts +++ b/packages/api/src/agents/guard.spec.ts @@ -52,6 +52,7 @@ function createApp( store: SubagentThreadTaskStore, getEventBinding?: AllMethods['getAgentEventBinding'], isHumanResumeAllowed?: () => Promise, + preResolved?: { conversation: IConversation | null }, ) { const app = express(); app.use(express.json()); @@ -59,6 +60,10 @@ function createApp( req.user = { id: 'user-1', tenantId: 'tenant-1' }; (req as typeof req & { _isAgentTrigger?: boolean })._isAgentTrigger = req.get('x-test-trigger') === '1'; + if (preResolved) { + (req as typeof req & { resolvedConversation?: IConversation | null }).resolvedConversation = + preResolved.conversation; + } next(); }); const guard = createSubagentThreadTurnGuard({ @@ -110,6 +115,39 @@ describe('subagent child-thread write policy', () => { expect(getConvo).toHaveBeenCalledTimes(1); }); + it('reuses a conversation an earlier middleware already read instead of re-reading it', async () => { + const getConvo = jest.fn(); + const store = makeStore(); + + const ordinary = await request( + createApp(getConvo, store, undefined, undefined, { + conversation: { + conversationId: 'ordinary-conversation', + endpoint: 'agents', + } as IConversation, + }), + ) + .post('/chat') + .send({ conversationId: 'ordinary-conversation' }); + const child = await request( + createApp(getConvo, store, undefined, undefined, { conversation: childConversation() }), + ) + .post('/chat') + .send({ conversationId: 'child-conversation', agent_id: 'child-agent' }); + const absent = await request( + createApp(getConvo, store, undefined, undefined, { conversation: null }), + ) + .post('/chat') + .send({ conversationId: 'missing-conversation' }); + + expect(ordinary.status).toBe(200); + expect(ordinary.body).toEqual({ ok: true, resolvedConversationId: 'ordinary-conversation' }); + expect(child.status).toBe(409); + expect(child.body).toEqual({ error: CHILD_THREAD_READ_ONLY_ERROR }); + expect(absent.status).toBe(200); + expect(getConvo).not.toHaveBeenCalled(); + }); + it('rejects every model-bound human turn against a durable child conversation', async () => { const store = makeStore(); const response = await request( diff --git a/packages/api/src/agents/guard.ts b/packages/api/src/agents/guard.ts index 94d69f2460..51dab509d7 100644 --- a/packages/api/src/agents/guard.ts +++ b/packages/api/src/agents/guard.ts @@ -30,6 +30,8 @@ export interface SubagentThreadWriteTarget { userId: string; conversationId: string; tenantId?: string; + /** Conversation already read earlier in the request (`null` = looked up, absent). */ + conversation?: IConversation | null; } interface SubagentThreadWriteResolution { @@ -112,18 +114,23 @@ async function isBoundEventContinuation( async function resolveSubagentThreadWrite( { getConvo, store }: SubagentThreadWriteGuardDeps, - { userId, conversationId, tenantId }: SubagentThreadWriteTarget, + target: SubagentThreadWriteTarget, ): Promise { + const { userId, conversationId, tenantId } = target; + const readConversation = (): Promise => + target.conversation !== undefined + ? Promise.resolve(target.conversation) + : getConvo(userId, conversationId); /** New child IDs are returned synchronously by the SDK before Mongo creation can * finish. Their reserved UUID namespace closes that brief window on every replica. */ if (isReservedSubagentThreadId(conversationId)) { - const conversation = await getConvo(userId, conversationId); + const conversation = await readConversation(); return { blocked: true, conversation }; } if (store.isThreadActiveForOwner(userId, conversationId, tenantId)) { return { blocked: true }; } - const conversation = await getConvo(userId, conversationId); + const conversation = await readConversation(); return { blocked: conversation?.subagentThread != null, conversation }; } @@ -159,10 +166,14 @@ export function createSubagentThreadTurnGuard(deps: SubagentThreadWriteGuardDeps typeof user?.tenantId === 'string' && user.tenantId !== '' ? user.tenantId : undefined; try { + const resolvedRequest = request as ResolvedConversationRequest; const resolved = await resolveSubagentThreadWrite(deps, { userId, conversationId: candidateConversationId, ...(tenantId == null ? {} : { tenantId }), + ...(Object.prototype.hasOwnProperty.call(request, 'resolvedConversation') + ? { conversation: resolvedRequest.resolvedConversation } + : {}), }); if (resolved.conversation !== undefined) { (request as ResolvedConversationRequest).resolvedConversation = resolved.conversation; @@ -171,7 +182,6 @@ export function createSubagentThreadTurnGuard(deps: SubagentThreadWriteGuardDeps next(); return; } - const resolvedRequest = request as ResolvedConversationRequest; const resolvedConversation = resolved.conversation; const lineage = resolvedConversation?.subagentThread; const humanResume = deps.isHumanResumeAllowed; diff --git a/packages/api/src/agents/initialize.files.spec.ts b/packages/api/src/agents/initialize.files.spec.ts new file mode 100644 index 0000000000..9cf553641b --- /dev/null +++ b/packages/api/src/agents/initialize.files.spec.ts @@ -0,0 +1,45 @@ +import { readResolvedConversationFiles } from './initialize'; + +describe('readResolvedConversationFiles', () => { + const conversationId = 'conversation-1'; + + it('leaves the database read in place when no middleware resolved the conversation', () => { + expect(readResolvedConversationFiles({}, conversationId)).toBeUndefined(); + }); + + it('reports no files when the conversation was looked up and does not exist', () => { + expect(readResolvedConversationFiles({ resolvedConversation: null }, conversationId)).toEqual( + [], + ); + }); + + it('uses the resolved document when it carries the files field', () => { + expect( + readResolvedConversationFiles( + { resolvedConversation: { conversationId, files: ['file-1'] } }, + conversationId, + ), + ).toEqual(['file-1']); + expect( + readResolvedConversationFiles( + { resolvedConversation: { conversationId, files: [] } }, + conversationId, + ), + ).toEqual([]); + }); + + it('falls back to the database when the resolved document omits files or is another conversation', () => { + expect( + readResolvedConversationFiles( + { resolvedConversation: { conversationId, agent_id: 'child-agent' } }, + conversationId, + ), + ).toBeUndefined(); + expect( + readResolvedConversationFiles( + { resolvedConversation: { conversationId: 'other', files: ['file-1'] } }, + conversationId, + ), + ).toBeUndefined(); + }); +}); diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index fffa9d93c5..c831f34e20 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -188,6 +188,32 @@ function appendAdditionalInstructions(agent: Agent, text?: string | null): void .join('\n\n'); } +/** + * The request middleware already read this conversation once (`null` = looked up, absent). + * Only a resolved document that actually carries `files` can stand in for the database: + * bound agent-event continuations stash a partial document built from lineage alone. + */ +export function readResolvedConversationFiles( + req: Pick, + conversationId: string, +): string[] | undefined { + if (!Object.prototype.hasOwnProperty.call(req, 'resolvedConversation')) { + return undefined; + } + const resolved = req.resolvedConversation; + if (resolved === null) { + return []; + } + if ( + resolved == null || + resolved.conversationId !== conversationId || + !Object.prototype.hasOwnProperty.call(resolved, 'files') + ) { + return undefined; + } + return resolved.files ?? []; +} + function getMaxCatalogSkills(req: ServerRequest): number | undefined { const endpoints = req.config?.endpoints as | Record @@ -987,7 +1013,7 @@ export async function initializeAgent( * every code-output ref. */ const [convoFileIds, threadMessages] = await Promise.all([ - db.getConvoFiles(conversationId), + readResolvedConversationFiles(req, conversationId) ?? db.getConvoFiles(conversationId), needsThreadWalk && getThreadMessages ? getThreadMessages({ conversationId }, 'messageId parentMessageId files attachments') : null, diff --git a/packages/api/src/types/http.ts b/packages/api/src/types/http.ts index b3a34df383..df1d6893e2 100644 --- a/packages/api/src/types/http.ts +++ b/packages/api/src/types/http.ts @@ -1,5 +1,5 @@ -import type { TConversation, TEndpointOption } from 'librechat-data-provider'; -import type { IUser, AppConfig } from '@librechat/data-schemas'; +import type { IUser, AppConfig, IConversation } from '@librechat/data-schemas'; +import type { TEndpointOption } from 'librechat-data-provider'; import type { Request } from 'express'; /** @@ -25,8 +25,9 @@ export type ServerRequest = Request & { config?: AppConfig; /** Server-captured conversation creation time used to anchor dynamic prompt variables. */ conversationCreatedAt?: string; - /** Conversation loaded while resolving the prompt timestamp anchor, reused by save logic. */ - resolvedConversation?: Partial | null; + /** Conversation read by request middleware (`null` = looked up, absent), reused by the + * subagent guard, agent initialization, and the first save instead of re-reading it. */ + resolvedConversation?: Partial | null; /** Passport strategy that populated req.user for this request. */ authStrategy?: string; }; diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index b09cbca203..f756075f80 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -115,7 +115,10 @@ async function refreshChatProjectStatsInBatches( export interface ConversationMethods { getConvoFiles(conversationId: string): Promise; - searchConversation(conversationId: string): Promise; + searchConversation( + conversationId: string, + fieldsToSelect?: string | null, + ): Promise; deleteNullOrEmptyConversations(): Promise<{ conversations: { deletedCount?: number }; messages: { deletedCount?: number }; @@ -259,13 +262,14 @@ export function createConversationMethods( /** * Searches for a conversation by conversationId and returns a lean document with only conversationId and user. */ - async function searchConversation(conversationId: string) { + /** `fieldsToSelect: null` returns the full document so one read can serve the whole request. */ + async function searchConversation( + conversationId: string, + fieldsToSelect: string | null = 'conversationId user', + ) { try { const Conversation = mongoose.models.Conversation as Model; - return await Conversation.findOne( - { conversationId }, - 'conversationId user', - ).lean(); + return await Conversation.findOne({ conversationId }, fieldsToSelect).lean(); } catch (error) { logger.error('[searchConversation] Error searching conversation', error); throw new Error('Error searching conversation');