diff --git a/api/server/routes/__test-utils__/convos-route-mocks.js b/api/server/routes/__test-utils__/convos-route-mocks.js index 769d2b61d8..8fa2f937b2 100644 --- a/api/server/routes/__test-utils__/convos-route-mocks.js +++ b/api/server/routes/__test-utils__/convos-route-mocks.js @@ -72,8 +72,8 @@ module.exports = { forkIpLimiter: (req, res, next) => next(), forkUserLimiter: (req, res, next) => next(), })), - configMiddleware: (req, res, next) => next(), - validateConvoAccess: (req, res, next) => next(), + configMiddleware: jest.fn((req, res, next) => next()), + validateConvoAccess: jest.fn((req, res, next) => next()), }), forkUtils: () => ({ diff --git a/api/server/routes/__tests__/convos.spec.js b/api/server/routes/__tests__/convos.spec.js index 344b8bf171..763397459b 100644 --- a/api/server/routes/__tests__/convos.spec.js +++ b/api/server/routes/__tests__/convos.spec.js @@ -27,6 +27,7 @@ describe('Convos Routes', () => { deleteAllSharedLinksWithCleanup, deleteConvoSharedLinksWithCleanup, } = require('@librechat/api'); + const { configMiddleware, validateConvoAccess } = require('~/server/middleware'); beforeAll(() => { convosRouter = require('../convos'); @@ -47,6 +48,30 @@ describe('Convos Routes', () => { jest.clearAllMocks(); }); + describe('conversation mutation middleware', () => { + it.each([ + ['/api/convos/archive', { arg: { conversationId: 'conv-123', isArchived: true } }], + ['/api/convos/pin', { arg: { conversationId: 'conv-123', pinned: true } }], + ['/api/convos/update', { arg: { conversationId: 'conv-123', title: 'Updated' } }], + ])('loads config before an access denial on %s', async (path, body) => { + configMiddleware.mockImplementationOnce((req, res, next) => { + req.config = { interfaceConfig: { retentionMode: 'ephemeral' } }; + next(); + }); + validateConvoAccess.mockImplementationOnce((req, res) => { + res.status(403).json({ configLoaded: req.config != null }); + }); + + const response = await request(app).post(path).send(body); + + expect(response.status).toBe(403); + expect(response.body).toEqual({ configLoaded: true }); + expect(configMiddleware).toHaveBeenCalledTimes(1); + expect(validateConvoAccess).toHaveBeenCalledTimes(1); + expect(saveConvo).not.toHaveBeenCalled(); + }); + }); + describe('DELETE /all', () => { it('prunes the deleted conversations’ agent checkpoints (bulk, ids from deleteConvos)', async () => { // HITL: a paused conversation's durable checkpoint must not outlive the conversation. diff --git a/api/server/routes/convos.js b/api/server/routes/convos.js index 7df086b940..e0049834f0 100644 --- a/api/server/routes/convos.js +++ b/api/server/routes/convos.js @@ -186,7 +186,7 @@ router.delete('/all', configMiddleware, async (req, res) => { * @param {boolean} req.body.arg.isArchived - Whether to archive (true) or unarchive (false). * @returns {object} 200 - The updated conversation object. */ -router.post('/archive', validateConvoAccess, configMiddleware, async (req, res) => { +router.post('/archive', configMiddleware, validateConvoAccess, async (req, res) => { const { conversationId, isArchived } = req.body?.arg ?? {}; if (!conversationId) { @@ -214,7 +214,7 @@ router.post('/archive', validateConvoAccess, configMiddleware, async (req, res) } }); -router.post('/pin', validateConvoAccess, configMiddleware, async (req, res) => { +router.post('/pin', configMiddleware, validateConvoAccess, async (req, res) => { const { conversationId, pinned } = req.body?.arg ?? {}; if (!conversationId) { @@ -256,7 +256,7 @@ const MAX_CONVO_TITLE_LENGTH = 1024; * @param {string} req.body.arg.title - The new title for the conversation. * @returns {object} 201 - The updated conversation object. */ -router.post('/update', validateConvoAccess, configMiddleware, async (req, res) => { +router.post('/update', configMiddleware, validateConvoAccess, async (req, res) => { const { conversationId, title } = req.body?.arg ?? {}; if (!conversationId) { diff --git a/packages/data-schemas/src/methods/chatProject.spec.ts b/packages/data-schemas/src/methods/chatProject.spec.ts index f21a075665..faa818f050 100644 --- a/packages/data-schemas/src/methods/chatProject.spec.ts +++ b/packages/data-schemas/src/methods/chatProject.spec.ts @@ -308,23 +308,25 @@ describe('ChatProject methods', () => { }); it('forces ephemeral retention on a permanent chat, its messages, shares, and files when assigning', async () => { - const project = await methods.createChatProject(user, { name: 'Ephemeral' }); - await createConversation(user, 'convo-1', 'Permanent'); + const ownerObjectId = new mongoose.Types.ObjectId(); + const owner = ownerObjectId.toString(); + const project = await methods.createChatProject(owner, { name: 'Ephemeral' }); + await createConversation(owner, 'convo-1', 'Permanent'); await Message.create([ - { messageId: uuidv4(), conversationId: 'convo-1', user, text: 'first' }, - { messageId: uuidv4(), conversationId: 'convo-1', user, text: 'second' }, + { messageId: uuidv4(), conversationId: 'convo-1', user: owner, text: 'first' }, + { messageId: uuidv4(), conversationId: 'convo-1', user: owner, text: 'second' }, ]); - await SharedLink.create({ conversationId: 'convo-1', user, shareId: uuidv4() }); + await SharedLink.create({ conversationId: 'convo-1', user: owner, shareId: uuidv4() }); const fileId = uuidv4(); await File.collection.insertOne({ file_id: fileId, conversationId: 'convo-1', - user: new mongoose.Types.ObjectId(), + user: ownerObjectId, expiredAt: null, }); const result = await methods.assignConversationToProject( - user, + owner, 'convo-1', project._id!.toString(), ephemeralConfig, @@ -335,20 +337,26 @@ describe('ChatProject methods', () => { expect(result?.conversation.chatProjectId).toBe(project._id!.toString()); const conversation = await Conversation.findOne({ - user, + user: owner, conversationId: 'convo-1', }).lean(); expect(conversation?.isTemporary).toBe(true); expect(conversation?.expiredAt).toBeInstanceOf(Date); - const messages = await Message.find({ user, conversationId: 'convo-1' }).lean(); + const messages = await Message.find({ + user: owner, + conversationId: 'convo-1', + }).lean(); expect(messages).toHaveLength(2); for (const message of messages) { expect(message.isTemporary).toBe(true); expect(message.expiredAt).toBeInstanceOf(Date); } - const share = await SharedLink.findOne({ user, conversationId: 'convo-1' }).lean(); + const share = await SharedLink.findOne({ + user: owner, + conversationId: 'convo-1', + }).lean(); expect(share?.expiredAt).toBeInstanceOf(Date); const file = await File.findOne({ file_id: fileId }).lean(); @@ -386,25 +394,32 @@ describe('ChatProject methods', () => { }); it('forces ephemeral retention on member chats when deleting a project', async () => { - const project = await methods.createChatProject(user, { name: 'Ephemeral' }); + const ownerObjectId = new mongoose.Types.ObjectId(); + const owner = ownerObjectId.toString(); + const project = await methods.createChatProject(owner, { name: 'Ephemeral' }); const projectId = project._id!.toString(); - await createConversation(user, 'convo-1', 'First'); - await createConversation(user, 'convo-2', 'Second'); - await methods.assignConversationToProject(user, 'convo-1', projectId); - await methods.assignConversationToProject(user, 'convo-2', projectId); - await Message.create({ messageId: uuidv4(), conversationId: 'convo-1', user, text: 'hi' }); + await createConversation(owner, 'convo-1', 'First'); + await createConversation(owner, 'convo-2', 'Second'); + await methods.assignConversationToProject(owner, 'convo-1', projectId); + await methods.assignConversationToProject(owner, 'convo-2', projectId); + await Message.create({ + messageId: uuidv4(), + conversationId: 'convo-1', + user: owner, + text: 'hi', + }); const fileId = uuidv4(); await File.collection.insertOne({ file_id: fileId, conversationId: 'convo-1', - user: new mongoose.Types.ObjectId(), + user: ownerObjectId, expiredAt: null, }); - await methods.deleteChatProject(user, projectId, ephemeralConfig); + await methods.deleteChatProject(owner, projectId, ephemeralConfig); const conversations = await Conversation.find({ - user, + user: owner, conversationId: { $in: ['convo-1', 'convo-2'] }, }).lean(); expect(conversations).toHaveLength(2); @@ -414,7 +429,10 @@ describe('ChatProject methods', () => { expect(conversation.expiredAt).toBeInstanceOf(Date); } - const message = await Message.findOne({ user, conversationId: 'convo-1' }).lean(); + const message = await Message.findOne({ + user: owner, + conversationId: 'convo-1', + }).lean(); expect(message?.isTemporary).toBe(true); expect(message?.expiredAt).toBeInstanceOf(Date); diff --git a/packages/data-schemas/src/methods/message.spec.ts b/packages/data-schemas/src/methods/message.spec.ts index b4a9f4df82..24778fbd1f 100644 --- a/packages/data-schemas/src/methods/message.spec.ts +++ b/packages/data-schemas/src/methods/message.spec.ts @@ -1554,10 +1554,12 @@ describe('Message Operations', () => { describe('applyForcedRetentionToTag', () => { const Conversation = () => mongoose.models.Conversation as mongoose.Model; const SharedLink = () => mongoose.models.SharedLink as mongoose.Model; + const File = () => mongoose.models.File as mongoose.Model; beforeEach(async () => { await Conversation().deleteMany({}); await SharedLink().deleteMany({}); + await File().deleteMany({}); }); it('converts every permanent conversation carrying the tag under ephemeral mode', async () => { @@ -1689,6 +1691,45 @@ describe('Message Operations', () => { expect(permanentShare?.expiredAt?.getTime()).toBeGreaterThan(soonerExpiry.getTime()); }); + it('owner-scopes bulk conversation file caps when conversation ids collide', async () => { + const ownerObjectId = new mongoose.Types.ObjectId(); + const foreignOwnerObjectId = new mongoose.Types.ObjectId(); + const owner = ownerObjectId.toString(); + const foreignOwner = foreignOwnerObjectId.toString(); + const conversationId = uuidv4(); + const ownerFileId = uuidv4(); + const foreignFileId = uuidv4(); + + await Conversation().create([ + { conversationId, user: owner, endpoint: 'openAI', tags: ['work'] }, + { conversationId, user: foreignOwner, endpoint: 'openAI', tags: ['personal'] }, + ]); + await File().collection.insertMany([ + { file_id: ownerFileId, conversationId, user: ownerObjectId, expiredAt: null }, + { + file_id: foreignFileId, + conversationId, + user: foreignOwnerObjectId, + expiredAt: null, + }, + ]); + + await applyForcedRetentionToTag( + { + userId: owner, + interfaceConfig: { temporaryChatRetention: 24, retentionMode: RetentionMode.EPHEMERAL }, + }, + { tag: 'work' }, + { context: 'PUT /api/tags/:tag' }, + ); + + const ownerFile = await File().findOne({ file_id: ownerFileId }).lean(); + expect(ownerFile?.expiredAt).toBeInstanceOf(Date); + + const foreignFile = await File().findOne({ file_id: foreignFileId }).lean(); + expect(foreignFile?.expiredAt ?? null).toBeNull(); + }); + it('is a no-op outside forced retention', async () => { const conversationId = uuidv4(); await Conversation().create({ @@ -1813,10 +1854,12 @@ describe('Message Operations', () => { const permanentFileId = uuidv4(); const soonerFileId = uuidv4(); const unrelatedFileId = uuidv4(); + const ownerObjectId = new mongoose.Types.ObjectId(); + const owner = ownerObjectId.toString(); await Conversation().create({ conversationId, - user: 'user123', + user: owner, endpoint: 'openAI', isTemporary: false, }); @@ -1824,19 +1867,19 @@ describe('Message Operations', () => { { file_id: permanentFileId, conversationId, - user: new mongoose.Types.ObjectId(), + user: ownerObjectId, expiredAt: null, }, { file_id: soonerFileId, conversationId, - user: new mongoose.Types.ObjectId(), + user: ownerObjectId, expiredAt: soonerExpiry, }, { file_id: unrelatedFileId, conversationId: unrelatedConversationId, - user: new mongoose.Types.ObjectId(), + user: ownerObjectId, expiredAt: null, }, ]); @@ -1858,19 +1901,21 @@ describe('Message Operations', () => { const parentDeadline = new Date(Date.now() + 60 * 60 * 1000); const conversationId = uuidv4(); const fileId = uuidv4(); + const ownerObjectId = new mongoose.Types.ObjectId(); + const owner = ownerObjectId.toString(); await Conversation().create({ conversationId, - user: 'user123', + user: owner, endpoint: 'openAI', isTemporary: true, expiredAt: parentDeadline, }); - await SharedLink().create({ conversationId, user: 'user123', shareId: uuidv4() }); + await SharedLink().create({ conversationId, user: owner, shareId: uuidv4() }); await File().collection.insertOne({ file_id: fileId, conversationId, - user: new mongoose.Types.ObjectId(), + user: ownerObjectId, expiredAt: null, }); @@ -2067,6 +2112,53 @@ describe('Message Operations', () => { expect(converted?.expiredAt?.getTime()).toBe(forcedExpiredAt.getTime()); }); + it('does not extend a parent shortened after the sweep cursor read', async () => { + const forcedExpiredAt = new Date(Date.now() + 24 * 60 * 60 * 1000); + const soonerExpiry = new Date(Date.now() + 30 * 60 * 1000); + const owner = new mongoose.Types.ObjectId().toString(); + const conversationId = uuidv4(); + const convo = await Conversation().create({ + conversationId, + user: owner, + endpoint: 'openAI', + isTemporary: false, + }); + const FileModel = File(); + const updateMany = FileModel.updateMany.bind(FileModel); + const racingUpdateMany = jest.fn( + async ( + filter: mongoose.FilterQuery, + update: mongoose.UpdateQuery, + ) => { + await Conversation().collection.updateOne( + { _id: convo._id }, + { $set: { isTemporary: true, expiredAt: soonerExpiry } }, + ); + return updateMany(filter, update); + }, + ); + Object.assign(FileModel, { updateMany: racingUpdateMany }); + const result = await (async () => { + try { + return await sweepForcedRetention( + Conversation(), + Message, + SharedLink(), + FileModel, + forcedExpiredAt, + ); + } finally { + Object.assign(FileModel, { updateMany }); + } + })(); + + expect(racingUpdateMany).toHaveBeenCalledTimes(1); + expect(result).toEqual({ conversations: 0, aligned: 0, errors: 0, projects: [] }); + const parent = await Conversation().findById(convo._id).lean(); + expect(parent?.isTemporary).toBe(true); + expect(parent?.expiredAt?.getTime()).toBe(soonerExpiry.getTime()); + }); + it('scopes the sweep to the active tenant context, leaving other tenants untouched', async () => { const forcedExpiredAt = new Date(Date.now() + 24 * 60 * 60 * 1000); const tenantAConversationId = uuidv4(); @@ -2127,6 +2219,9 @@ describe('Message Operations', () => { const convoFileId = uuidv4(); const threadFileId = uuidv4(); const foreignFileId = uuidv4(); + const conversationFileId = uuidv4(); + const foreignConversationFileId = uuidv4(); + const foreignOwnerObjectId = new mongoose.Types.ObjectId(); await Conversation().create({ conversationId, @@ -2153,6 +2248,13 @@ describe('Message Operations', () => { { file_id: convoFileId, user: ownerObjectId, expiredAt: null }, { file_id: threadFileId, user: ownerObjectId, expiredAt: null }, { file_id: foreignFileId, user: new mongoose.Types.ObjectId(), expiredAt: null }, + { file_id: conversationFileId, conversationId, user: ownerObjectId, expiredAt: null }, + { + file_id: foreignConversationFileId, + conversationId, + user: foreignOwnerObjectId, + expiredAt: null, + }, ]); await cascadeForcedConversationRetention( @@ -2177,12 +2279,20 @@ describe('Message Operations', () => { const threadFile = await File().findOne({ file_id: threadFileId }).lean(); expect(threadFile?.expiredAt?.getTime()).toBe(forcedExpiredAt.getTime()); + const conversationFile = await File().findOne({ file_id: conversationFileId }).lean(); + expect(conversationFile?.expiredAt?.getTime()).toBe(forcedExpiredAt.getTime()); + /** * A crafted reference to another user's file must never shorten that file's retention: * the referenced-id cap is owner-scoped, so the foreign row stays permanent. */ const foreignFile = await File().findOne({ file_id: foreignFileId }).lean(); expect(foreignFile?.expiredAt ?? null).toBeNull(); + + const foreignConversationFile = await File() + .findOne({ file_id: foreignConversationFileId }) + .lean(); + expect(foreignConversationFile?.expiredAt ?? null).toBeNull(); }); it('caps lagging children even when the parent conversation already conforms', async () => { @@ -2190,19 +2300,21 @@ describe('Message Operations', () => { const parentDeadline = new Date(Date.now() + 60 * 60 * 1000); const conversationId = uuidv4(); const fileId = uuidv4(); + const ownerObjectId = new mongoose.Types.ObjectId(); + const owner = ownerObjectId.toString(); await Conversation().create({ conversationId, - user: 'user123', + user: owner, endpoint: 'openAI', isTemporary: true, expiredAt: parentDeadline, }); - await SharedLink().create({ conversationId, user: 'user123', shareId: uuidv4() }); + await SharedLink().create({ conversationId, user: owner, shareId: uuidv4() }); await File().collection.insertOne({ file_id: fileId, conversationId, - user: new mongoose.Types.ObjectId(), + user: ownerObjectId, expiredAt: null, }); @@ -2211,7 +2323,7 @@ describe('Message Operations', () => { Message, SharedLink(), File(), - 'user123', + owner, conversationId, forcedExpiredAt, ); @@ -2229,9 +2341,10 @@ describe('Message Operations', () => { it('leaves the conversation non-conforming when a child backfill fails so a re-run retries it', async () => { const forcedExpiredAt = new Date(Date.now() + 24 * 60 * 60 * 1000); const conversationId = uuidv4(); + const owner = new mongoose.Types.ObjectId().toString(); await Conversation().create({ conversationId, - user: 'user123', + user: owner, endpoint: 'openAI', isTemporary: false, }); @@ -2246,7 +2359,7 @@ describe('Message Operations', () => { Message, SharedLink(), throwingFile, - 'user123', + owner, conversationId, forcedExpiredAt, ), @@ -2261,7 +2374,7 @@ describe('Message Operations', () => { Message, SharedLink(), File(), - 'user123', + owner, conversationId, forcedExpiredAt, ); diff --git a/packages/data-schemas/src/utils/retention.ts b/packages/data-schemas/src/utils/retention.ts index 6f5a7bdbd3..a3577aca2c 100644 --- a/packages/data-schemas/src/utils/retention.ts +++ b/packages/data-schemas/src/utils/retention.ts @@ -226,21 +226,23 @@ export const conversationSeedFileIds = (convo: { file_ids?: string[] | null; }): string[] => [...(convo.files ?? []), ...(convo.file_ids ?? [])]; -/** - * Builds the owner-scoped referenced-file-id branch of a file-cap filter. File ids inside - * message/conversation documents are caller-supplied, so a crafted reference to another user's - * file must never shorten that file's retention: the branch always filters on `File.user`. - * `File.user` is an ObjectId, so when the caller's user id is not a castable ObjectId string - * (legacy/test data) the branch is dropped entirely — fail closed rather than cap unverified - * rows. - */ -const ownedFileIdScope = ( +/** Builds an owner-scoped file-cap filter for conversation and referenced-file matches. */ +const ownedFileScope = ( userId: string, + conversationId: string | { $in: string[] }, fileIds: string[], -): { file_id: { $in: string[] }; user: string } | null => - fileIds.length > 0 && isValidObjectIdString(userId) - ? { file_id: { $in: fileIds }, user: userId } - : null; +): FilterQuery | null => { + if (!isValidObjectIdString(userId)) { + return null; + } + const conversationScope = { conversationId, user: userId }; + if (fileIds.length === 0) { + return conversationScope; + } + return { + $or: [conversationScope, { file_id: { $in: fileIds }, user: userId }], + }; +}; /** * Caps a conversation's uploaded files to the forced deadline. Files use a retention-scoped @@ -251,10 +253,9 @@ const ownedFileIdScope = ( * already expires sooner. Under ephemeral retention every conversation-scoped file is meant to * expire (persistent agent files are not retained), so no agent-file exclusion is needed here. * - * Matches by `conversationId` (a globally unique per-conversation id whose File rows are - * server-created by this conversation's own processes; a colliding id can only shorten the - * colliding owner's files, never extend) plus any referenced `fileIds`, which are - * owner-scoped via {@link ownedFileIdScope} because references are caller-supplied. + * Matches by `conversationId` plus any referenced `fileIds`. Both branches are scoped by + * `File.user` because conversation ids are unique only together with their owner and tenant, + * while referenced ids can be caller-supplied. * A shared file referenced from several chats is capped to the earliest converting chat's * deadline, consistent with cap-don't-extend. */ @@ -265,8 +266,10 @@ export const capConversationFiles = async ( forcedExpiredAt: Date, fileIds: string[] = [], ): Promise => { - const fileIdScope = ownedFileIdScope(userId, fileIds); - const scope = fileIdScope ? { $or: [{ conversationId }, fileIdScope] } : { conversationId }; + const scope = ownedFileScope(userId, conversationId, fileIds); + if (scope == null) { + return 0; + } const result = await File.updateMany( { $and: [scope, { $or: [{ expiredAt: null }, { expiredAt: { $gt: forcedExpiredAt } }] }], @@ -453,10 +456,10 @@ const cascadeForcedRetentionForConversationSet = async ( { $set: { expiredAt } }, ); const fileIds = await collectConversationFileIds(Message, userId, conversationIds, seedFileIds); - const fileIdScope = ownedFileIdScope(userId, fileIds); - const fileScope = fileIdScope - ? { $or: [{ conversationId: { $in: conversationIds } }, fileIdScope] } - : { conversationId: { $in: conversationIds } }; + const fileScope = ownedFileScope(userId, { $in: conversationIds }, fileIds); + if (fileScope == null) { + continue; + } await File.updateMany( { $and: [fileScope, { $or: [{ expiredAt: null }, { expiredAt: { $gt: expiredAt } }] }], @@ -622,7 +625,13 @@ export const sweepForcedRetention = async ( await forceConversationMessagesTemporary(Message, user, conversationId, expiredAt); await capConversationSharedLinks(SharedLink, user, conversationId, expiredAt); await capConversationFiles(File, user, conversationId, expiredAt, fileIds); - await Conversation.updateOne({ _id: convo._id }, { $set: { isTemporary: true, expiredAt } }); + const convoResult = await Conversation.updateOne( + { _id: convo._id, ...forcedRetentionGapFilter(expiredAt) }, + { $set: { isTemporary: true, expiredAt } }, + ); + if ((convoResult.modifiedCount ?? 0) === 0) { + continue; + } result.conversations += 1; if (typeof chatProjectId === 'string' && chatProjectId.length > 0) { const key = `${user}|${chatProjectId}`;