From 9cbbeb6e85cc9d18e5860ce7ecf9b4b74ce13640 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 22 Jun 2026 10:46:04 +0200 Subject: [PATCH] fix: enforce forced retention on message edits, feedback, and error saves Two more message-write paths bypassed ephemeral enforcement: - The edit and feedback endpoints call updateMessage directly, without loading retention config, so editing an older permanent message after a switch to ephemeral left the message and its conversation non-temporary and visible. Load config on those routes and run a new applyForcedRetention helper after the update, which stamps the message and cascades the conversation/messages. - The sendError and denyRequest middleware save messages with retention config but never call saveConvo, so a validation/model error or denied-request message could outlive its conversation. Pass capExpiryToConversation like the other message-only paths. Extract the conversation cascade into a shared cascadeForcedConversationRetention helper used by both saveMessage and applyForcedRetention. --- api/server/middleware/denyRequest.js | 5 +- api/server/middleware/error.js | 1 + api/server/routes/messages.js | 214 ++++++++++-------- .../data-schemas/src/methods/message.spec.ts | 71 ++++++ packages/data-schemas/src/methods/message.ts | 87 +++++-- packages/data-schemas/src/utils/retention.ts | 24 +- 6 files changed, 290 insertions(+), 112 deletions(-) diff --git a/api/server/middleware/denyRequest.js b/api/server/middleware/denyRequest.js index 86054d0a23..41ba36b3bd 100644 --- a/api/server/middleware/denyRequest.js +++ b/api/server/middleware/denyRequest.js @@ -49,7 +49,10 @@ const denyRequest = async (req, res, errorMessage) => { interfaceConfig: req?.config?.interfaceConfig, }, { ...userMessage, user: req.user.id }, - { context: `api/server/middleware/denyRequest.js - ${responseText}` }, + { + context: `api/server/middleware/denyRequest.js - ${responseText}`, + capExpiryToConversation: true, + }, ); } diff --git a/api/server/middleware/error.js b/api/server/middleware/error.js index 5fa3562c30..5dce79f8f4 100644 --- a/api/server/middleware/error.js +++ b/api/server/middleware/error.js @@ -56,6 +56,7 @@ const sendError = async (req, res, options, callback) => { { ...errorMessage, user }, { context: 'api/server/utils/streamResponse.js - sendError', + capExpiryToConversation: true, }, ); } diff --git a/api/server/routes/messages.js b/api/server/routes/messages.js index effd4cda29..a405eb78ce 100644 --- a/api/server/routes/messages.js +++ b/api/server/routes/messages.js @@ -15,6 +15,17 @@ const db = require('~/models'); const router = express.Router(); router.use(requireJwtAuth); +/** + * Enforces forced (ephemeral) retention after a message-only update (edit/feedback), + * which bypasses the saveMessage/saveConvo enforcement. No-op outside forced retention. + */ +const enforceForcedRetention = (req, conversationId, messageId, context) => + db.applyForcedRetention( + { userId: req?.user?.id, interfaceConfig: req?.config?.interfaceConfig }, + { conversationId, messageId }, + { context, capExpiryToConversation: true }, + ); + router.get('/', async (req, res) => { try { const user = req.user.id ?? ''; @@ -323,108 +334,131 @@ router.get('/:conversationId/:messageId', validateMessageReq, async (req, res) = } }); -router.put('/:conversationId/:messageId', validateMessageReq, async (req, res) => { - try { - const { conversationId, messageId } = req.params; - const { text, index, model } = req.body; +router.put( + '/:conversationId/:messageId', + validateMessageReq, + configMiddleware, + async (req, res) => { + try { + const { conversationId, messageId } = req.params; + const { text, index, model } = req.body; - if (index === undefined) { - const tokenCount = await countTokens(text, model); - const result = await db.updateMessage(req?.user?.id, { messageId, text, tokenCount }); - return res.status(200).json(result); - } + if (index === undefined) { + const tokenCount = await countTokens(text, model); + const result = await db.updateMessage(req?.user?.id, { messageId, text, tokenCount }); + await enforceForcedRetention( + req, + conversationId, + messageId, + 'PUT /api/messages - edit text', + ); + return res.status(200).json(result); + } - if (typeof index !== 'number' || index < 0) { - return res.status(400).json({ error: 'Invalid index' }); - } + if (typeof index !== 'number' || index < 0) { + return res.status(400).json({ error: 'Invalid index' }); + } - const message = ( - await db.getMessages({ conversationId, messageId, user: req.user.id }, 'content tokenCount') - )?.[0]; - if (!message) { - return res.status(404).json({ error: 'Message not found' }); - } + const message = ( + await db.getMessages({ conversationId, messageId, user: req.user.id }, 'content tokenCount') + )?.[0]; + if (!message) { + return res.status(404).json({ error: 'Message not found' }); + } - const existingContent = message.content; - if (!Array.isArray(existingContent) || index >= existingContent.length) { - return res.status(400).json({ error: 'Invalid index' }); - } + const existingContent = message.content; + if (!Array.isArray(existingContent) || index >= existingContent.length) { + return res.status(400).json({ error: 'Invalid index' }); + } - const updatedContent = [...existingContent]; - if (!updatedContent[index]) { - return res.status(400).json({ error: 'Content part not found' }); - } + const updatedContent = [...existingContent]; + if (!updatedContent[index]) { + return res.status(400).json({ error: 'Content part not found' }); + } - const currentPartType = updatedContent[index].type; - if (currentPartType !== ContentTypes.TEXT && currentPartType !== ContentTypes.THINK) { - return res.status(400).json({ error: 'Cannot update non-text content' }); - } + const currentPartType = updatedContent[index].type; + if (currentPartType !== ContentTypes.TEXT && currentPartType !== ContentTypes.THINK) { + return res.status(400).json({ error: 'Cannot update non-text content' }); + } - const oldText = updatedContent[index][currentPartType]; - updatedContent[index] = { type: currentPartType, [currentPartType]: text }; + const oldText = updatedContent[index][currentPartType]; + updatedContent[index] = { type: currentPartType, [currentPartType]: text }; - let tokenCount = message.tokenCount; - if (tokenCount !== undefined) { - const oldTokenCount = await countTokens(oldText, model); - const newTokenCount = await countTokens(text, model); - tokenCount = Math.max(0, tokenCount - oldTokenCount) + newTokenCount; - } + let tokenCount = message.tokenCount; + if (tokenCount !== undefined) { + const oldTokenCount = await countTokens(oldText, model); + const newTokenCount = await countTokens(text, model); + tokenCount = Math.max(0, tokenCount - oldTokenCount) + newTokenCount; + } - const result = await db.updateMessage(req?.user?.id, { - messageId, - content: updatedContent, - tokenCount, - }); - return res.status(200).json(result); - } catch (error) { - logger.error('Error updating message:', error); - res.status(500).json({ error: 'Internal server error' }); - } -}); - -router.put('/:conversationId/:messageId/feedback', validateMessageReq, async (req, res) => { - try { - const { conversationId, messageId } = req.params; - const { feedback } = req.body; - - const updatedMessage = await db.updateMessage( - req?.user?.id, - { + const result = await db.updateMessage(req?.user?.id, { messageId, - feedback: feedback || null, - }, - { context: 'updateFeedback' }, - ); - - // Best-effort: Assistants messages do not have deterministic AgentRun traces. - if (!isAssistantsEndpoint(updatedMessage.endpoint)) { - sendFeedbackScore({ - traceId: traceIdForMessage(messageId), - feedback: updatedMessage.feedback, - metadata: { - messageId: updatedMessage.messageId ?? messageId, - parentMessageId: updatedMessage.parentMessageId, - conversationId: updatedMessage.conversationId ?? conversationId, - sessionId: updatedMessage.conversationId ?? conversationId, - userId: req?.user?.id, - endpoint: updatedMessage.endpoint, - sender: updatedMessage.sender, - isCreatedByUser: updatedMessage.isCreatedByUser, - tokenCount: updatedMessage.tokenCount, - }, - }).catch((err) => logger.error('[langfuse] feedback score failed:', err)); + content: updatedContent, + tokenCount, + }); + await enforceForcedRetention( + req, + conversationId, + messageId, + 'PUT /api/messages - edit content', + ); + return res.status(200).json(result); + } catch (error) { + logger.error('Error updating message:', error); + res.status(500).json({ error: 'Internal server error' }); } + }, +); - res.json({ - messageId, - conversationId, - feedback: updatedMessage.feedback, - }); - } catch (error) { - logger.error('Error updating message feedback:', error); - res.status(500).json({ error: 'Failed to update feedback' }); - } -}); +router.put( + '/:conversationId/:messageId/feedback', + validateMessageReq, + configMiddleware, + async (req, res) => { + try { + const { conversationId, messageId } = req.params; + const { feedback } = req.body; + + const updatedMessage = await db.updateMessage( + req?.user?.id, + { + messageId, + feedback: feedback || null, + }, + { context: 'updateFeedback' }, + ); + await enforceForcedRetention(req, conversationId, messageId, 'PUT /api/messages - feedback'); + + // Best-effort: Assistants messages do not have deterministic AgentRun traces. + if (!isAssistantsEndpoint(updatedMessage.endpoint)) { + sendFeedbackScore({ + traceId: traceIdForMessage(messageId), + feedback: updatedMessage.feedback, + metadata: { + messageId: updatedMessage.messageId ?? messageId, + parentMessageId: updatedMessage.parentMessageId, + conversationId: updatedMessage.conversationId ?? conversationId, + sessionId: updatedMessage.conversationId ?? conversationId, + userId: req?.user?.id, + endpoint: updatedMessage.endpoint, + sender: updatedMessage.sender, + isCreatedByUser: updatedMessage.isCreatedByUser, + tokenCount: updatedMessage.tokenCount, + }, + }).catch((err) => logger.error('[langfuse] feedback score failed:', err)); + } + + res.json({ + messageId, + conversationId, + feedback: updatedMessage.feedback, + }); + } catch (error) { + logger.error('Error updating message feedback:', error); + res.status(500).json({ error: 'Failed to update feedback' }); + } + }, +); router.delete('/:conversationId/:messageId', validateMessageReq, async (req, res) => { try { diff --git a/packages/data-schemas/src/methods/message.spec.ts b/packages/data-schemas/src/methods/message.spec.ts index 1c98c95f8e..f7a01def41 100644 --- a/packages/data-schemas/src/methods/message.spec.ts +++ b/packages/data-schemas/src/methods/message.spec.ts @@ -23,6 +23,7 @@ let saveMessage: ReturnType['saveMessage']; let getMessages: ReturnType['getMessages']; let getMessageTextStats: ReturnType['getMessageTextStats']; let updateMessage: ReturnType['updateMessage']; +let applyForcedRetention: ReturnType['applyForcedRetention']; let deleteMessages: ReturnType['deleteMessages']; let bulkSaveMessages: ReturnType['bulkSaveMessages']; let updateMessageText: ReturnType['updateMessageText']; @@ -42,6 +43,7 @@ beforeAll(async () => { getMessages = methods.getMessages; getMessageTextStats = methods.getMessageTextStats; updateMessage = methods.updateMessage; + applyForcedRetention = methods.applyForcedRetention; deleteMessages = methods.deleteMessages; bulkSaveMessages = methods.bulkSaveMessages; updateMessageText = methods.updateMessageText; @@ -972,6 +974,75 @@ describe('Message Operations', () => { }); }); + describe('applyForcedRetention', () => { + const Conversation = () => mongoose.models.Conversation as mongoose.Model; + + beforeEach(async () => { + await Conversation().deleteMany({}); + }); + + it('converts a permanent message and its conversation when editing under ephemeral mode', async () => { + const conversationId = uuidv4(); + const editedMessageId = uuidv4(); + await Conversation().create({ + conversationId, + user: 'user123', + endpoint: 'openAI', + title: 'Existing permanent chat', + }); + await Message.create([ + { messageId: editedMessageId, conversationId, user: 'user123', text: 'first' }, + { messageId: uuidv4(), conversationId, user: 'user123', text: 'second' }, + ]); + + await applyForcedRetention( + { + userId: 'user123', + interfaceConfig: { temporaryChatRetention: 24, retentionMode: RetentionMode.EPHEMERAL }, + }, + { conversationId, messageId: editedMessageId }, + { context: 'edit', capExpiryToConversation: true }, + ); + + const convo = await Conversation().findOne({ conversationId }).lean(); + expect(convo?.isTemporary).toBe(true); + expect(convo?.expiredAt).toBeInstanceOf(Date); + + const messages = await getMessages({ conversationId, user: 'user123' }); + expect(messages).toHaveLength(2); + for (const message of messages) { + expect(message.isTemporary).toBe(true); + expect(message.expiredAt).toBeInstanceOf(Date); + } + }); + + it('is a no-op outside forced retention', async () => { + const conversationId = uuidv4(); + const messageId = uuidv4(); + await Conversation().create({ + conversationId, + user: 'user123', + endpoint: 'openAI', + title: 'Existing permanent chat', + }); + await Message.create({ messageId, conversationId, user: 'user123', text: 'first' }); + + await applyForcedRetention( + { + userId: 'user123', + interfaceConfig: { temporaryChatRetention: 24, retentionMode: RetentionMode.TEMPORARY }, + }, + { conversationId, messageId }, + { context: 'edit', capExpiryToConversation: true }, + ); + + const convo = await Conversation().findOne({ conversationId }).lean(); + expect(convo?.expiredAt ?? null).toBeNull(); + const message = await Message.findOne({ messageId }).lean(); + expect(message?.expiredAt ?? null).toBeNull(); + }); + }); + describe('Message cursor pagination', () => { /** * Helper to create messages with specific timestamps diff --git a/packages/data-schemas/src/methods/message.ts b/packages/data-schemas/src/methods/message.ts index 01831317f4..f8bbe5980c 100644 --- a/packages/data-schemas/src/methods/message.ts +++ b/packages/data-schemas/src/methods/message.ts @@ -1,11 +1,7 @@ import { RetentionMode, isForcedTemporaryRetention } from 'librechat-data-provider'; import type { DeleteResult, FilterQuery, Model, PipelineStage } from 'mongoose'; import type { AppConfig, IConversation, IMessage } from '~/types'; -import { - createFallbackRetentionDate, - forceConversationMessagesTemporary, - forcedRetentionGapFilter, -} from '~/utils/retention'; +import { cascadeForcedConversationRetention, createFallbackRetentionDate } from '~/utils/retention'; import { createTempChatExpirationDate } from '~/utils/tempChatRetention'; import { tenantSafeBulkWrite } from '~/utils/tenantBulkWrite'; import logger from '~/config/winston'; @@ -55,6 +51,11 @@ export interface MessageMethods { message: Partial & { newMessageId?: string }, metadata?: { context?: string }, ): Promise>; + applyForcedRetention( + ctx: { userId: string; interfaceConfig?: AppConfig['interfaceConfig'] }, + params: { conversationId: string; messageId: string }, + metadata?: { context?: string; capExpiryToConversation?: boolean }, + ): Promise; deleteMessagesSince( userId: string, params: { messageId: string; conversationId: string }, @@ -213,22 +214,13 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa if (isForcedRetention && forcedExpiredAt instanceof Date) { const Conversation = mongoose.models.Conversation as Model; - const convoResult = await Conversation.updateOne( - { - conversationId, - user: userId, - ...forcedRetentionGapFilter(forcedExpiredAt), - }, - { $set: { isTemporary: true, expiredAt: forcedExpiredAt } }, + await cascadeForcedConversationRetention( + Conversation, + Message, + userId, + conversationId, + forcedExpiredAt, ); - if (convoResult.modifiedCount > 0) { - await forceConversationMessagesTemporary( - Message, - userId, - conversationId, - forcedExpiredAt, - ); - } } return message.toObject(); @@ -383,6 +375,60 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa } } + /** + * Enforces forced (ephemeral) retention on an existing message and its parent + * conversation. Message-write paths that only update a row — edits, feedback — bypass + * the `saveMessage`/`saveConvo` enforcement, so an older permanent chat touched after an + * install switches to ephemeral would otherwise stay visible and never expire. + */ + async function applyForcedRetention( + { userId, interfaceConfig }: { userId: string; interfaceConfig?: AppConfig['interfaceConfig'] }, + { conversationId, messageId }: { conversationId: string; messageId: string }, + metadata?: { context?: string; capExpiryToConversation?: boolean }, + ): Promise { + if (!isForcedTemporaryRetention(interfaceConfig?.retentionMode)) { + return; + } + + let forcedExpiredAt: Date; + try { + forcedExpiredAt = createTempChatExpirationDate(interfaceConfig); + } catch (err) { + logger.error('Error creating temporary chat expiration date:', err); + logger.info(`---\`applyForcedRetention\` context: ${metadata?.context}`); + forcedExpiredAt = createFallbackRetentionDate(); + } + + const Message = mongoose.models.Message as Model; + const Conversation = mongoose.models.Conversation as Model; + + if (metadata?.capExpiryToConversation === true) { + const parent = await Conversation.findOne( + { conversationId, user: userId }, + 'isTemporary expiredAt', + ).lean<{ isTemporary?: boolean | null; expiredAt?: Date | null } | null>(); + if ( + parent?.isTemporary === true && + parent.expiredAt != null && + parent.expiredAt.getTime() < forcedExpiredAt.getTime() + ) { + forcedExpiredAt = parent.expiredAt; + } + } + + await Message.updateOne( + { messageId, user: userId }, + { $set: { isTemporary: true, expiredAt: forcedExpiredAt } }, + ); + await cascadeForcedConversationRetention( + Conversation, + Message, + userId, + conversationId, + forcedExpiredAt, + ); + } + /** * Deletes messages in a conversation since a specific message. */ @@ -606,6 +652,7 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa recordMessage, updateMessageText, updateMessage, + applyForcedRetention, deleteMessagesSince, getMessages, getMessageTextStats, diff --git a/packages/data-schemas/src/utils/retention.ts b/packages/data-schemas/src/utils/retention.ts index 0f288cd648..fa79571783 100644 --- a/packages/data-schemas/src/utils/retention.ts +++ b/packages/data-schemas/src/utils/retention.ts @@ -1,5 +1,5 @@ import type { FilterQuery, Model } from 'mongoose'; -import type { IMessage } from '~/types'; +import type { IConversation, IMessage } from '~/types'; import { DEFAULT_RETENTION_HOURS } from './tempChatRetention'; export type RetentionFilterDocument = { @@ -90,3 +90,25 @@ export const forceConversationMessagesTemporary = async ( { $set: { isTemporary: true, expiredAt } }, ); }; + +/** + * Converts or re-caps a parent conversation to the forced deadline and, when that first + * brings the conversation into the forced window, backfills its lagging messages. Shared + * by every forced-retention message-write path so a single conversation/message rule is + * enforced regardless of which save touched the chat. + */ +export const cascadeForcedConversationRetention = async ( + Conversation: Model, + Message: Model, + userId: string, + conversationId: string, + forcedExpiredAt: Date, +): Promise => { + const convoResult = await Conversation.updateOne( + { conversationId, user: userId, ...forcedRetentionGapFilter(forcedExpiredAt) }, + { $set: { isTemporary: true, expiredAt: forcedExpiredAt } }, + ); + if (convoResult.modifiedCount > 0) { + await forceConversationMessagesTemporary(Message, userId, conversationId, forcedExpiredAt); + } +};