mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-03 06:17:21 +02:00
🔏 fix: Strip Unnecessary Fields Across Write Paths in Conversation & Message Methods (#12498)
* fix: Exclude field from conversation and message updates * fix: Remove tenantId from conversation and message update objects to prevent unintended data exposure. * refactor: Adjust update logic in createConversationMethods and createMessageMethods to ensure tenantId is not included in the updates, maintaining data integrity. * fix: Strip tenantId from all write paths in conversation and message methods Extends the existing tenantId stripping to bulkSaveConvos, bulkSaveMessages, recordMessage, and updateMessage — all of which previously passed caller-supplied tenantId straight through to the update document. Renames discard alias from _t to _tenantId for clarity. Adds regression tests for all six write paths. * fix: Eliminate double-copy overhead and strengthen test assertions Replace destructure-then-spread with spread-once-then-delete for saveConvo, saveMessage, and recordMessage — removes one O(n) copy per call on hot paths. Add missing not-null and positive data assertions to tenantId stripping tests. * test: Add positive data assertions to bulkSaveMessages and recordMessage tests
This commit is contained in:
parent
419613fdaf
commit
5e789f589f
4 changed files with 136 additions and 18 deletions
|
|
@ -907,4 +907,38 @@ describe('Conversation Operations', () => {
|
|||
expect(result?.nextCursor).toBeNull(); // No next page
|
||||
});
|
||||
});
|
||||
|
||||
describe('tenantId stripping', () => {
|
||||
it('saveConvo should not write caller-supplied tenantId to the document', async () => {
|
||||
const conversationId = uuidv4();
|
||||
const result = await saveConvo(
|
||||
{ userId: 'user123' },
|
||||
{ conversationId, tenantId: 'malicious-tenant', title: 'Tenant Test' },
|
||||
);
|
||||
|
||||
expect(result).not.toBeNull();
|
||||
const doc = await Conversation.findOne({ conversationId }).lean();
|
||||
expect(doc).not.toBeNull();
|
||||
expect(doc?.title).toBe('Tenant Test');
|
||||
expect(doc?.tenantId).toBeUndefined();
|
||||
});
|
||||
|
||||
it('bulkSaveConvos should not write caller-supplied tenantId to documents', async () => {
|
||||
const conversationId = uuidv4();
|
||||
await methods.bulkSaveConvos([
|
||||
{
|
||||
conversationId,
|
||||
user: 'user123',
|
||||
title: 'Bulk Tenant Test',
|
||||
tenantId: 'malicious-tenant',
|
||||
endpoint: EModelEndpoint.openAI,
|
||||
},
|
||||
]);
|
||||
|
||||
const doc = await Conversation.findOne({ conversationId }).lean();
|
||||
expect(doc).not.toBeNull();
|
||||
expect(doc?.title).toBe('Bulk Tenant Test');
|
||||
expect(doc?.tenantId).toBeUndefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -168,6 +168,7 @@ export function createConversationMethods(
|
|||
|
||||
const messages = await getMessages({ conversationId }, '_id');
|
||||
const update: Record<string, unknown> = { ...convo, messages, user: userId };
|
||||
delete update.tenantId;
|
||||
|
||||
if (newConversationId) {
|
||||
update.conversationId = newConversationId;
|
||||
|
|
@ -220,14 +221,20 @@ export function createConversationMethods(
|
|||
async function bulkSaveConvos(conversations: Array<Record<string, unknown>>) {
|
||||
try {
|
||||
const Conversation = mongoose.models.Conversation as Model<IConversation>;
|
||||
const bulkOps = conversations.map((convo) => ({
|
||||
updateOne: {
|
||||
filter: { conversationId: convo.conversationId, user: convo.user },
|
||||
update: convo,
|
||||
upsert: true,
|
||||
timestamps: false,
|
||||
},
|
||||
}));
|
||||
const bulkOps = conversations.map((convo) => {
|
||||
const { tenantId: _tenantId, ...convoWithoutTenant } = convo;
|
||||
return {
|
||||
updateOne: {
|
||||
filter: {
|
||||
conversationId: convoWithoutTenant.conversationId,
|
||||
user: convoWithoutTenant.user,
|
||||
},
|
||||
update: convoWithoutTenant,
|
||||
upsert: true,
|
||||
timestamps: false,
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
const result = await tenantSafeBulkWrite(Conversation, bulkOps);
|
||||
return result;
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@ let deleteMessages: ReturnType<typeof createMessageMethods>['deleteMessages'];
|
|||
let bulkSaveMessages: ReturnType<typeof createMessageMethods>['bulkSaveMessages'];
|
||||
let updateMessageText: ReturnType<typeof createMessageMethods>['updateMessageText'];
|
||||
let deleteMessagesSince: ReturnType<typeof createMessageMethods>['deleteMessagesSince'];
|
||||
let recordMessage: ReturnType<typeof createMessageMethods>['recordMessage'];
|
||||
|
||||
beforeAll(async () => {
|
||||
mongoServer = await MongoMemoryServer.create();
|
||||
|
|
@ -38,6 +39,7 @@ beforeAll(async () => {
|
|||
bulkSaveMessages = methods.bulkSaveMessages;
|
||||
updateMessageText = methods.updateMessageText;
|
||||
deleteMessagesSince = methods.deleteMessagesSince;
|
||||
recordMessage = methods.recordMessage;
|
||||
|
||||
await mongoose.connect(mongoUri);
|
||||
});
|
||||
|
|
@ -937,4 +939,74 @@ describe('Message Operations', () => {
|
|||
expect(result?.messages).toHaveLength(5);
|
||||
});
|
||||
});
|
||||
|
||||
describe('tenantId stripping', () => {
|
||||
it('saveMessage should not write caller-supplied tenantId to the document', async () => {
|
||||
const messageId = uuidv4();
|
||||
const conversationId = uuidv4();
|
||||
const result = await saveMessage(
|
||||
{ userId: 'user123' },
|
||||
{ messageId, conversationId, text: 'Tenant test', tenantId: 'malicious-tenant' },
|
||||
);
|
||||
|
||||
expect(result).not.toBeNull();
|
||||
expect(result).toBeDefined();
|
||||
const doc = await Message.findOne({ messageId }).lean();
|
||||
expect(doc).not.toBeNull();
|
||||
expect(doc?.text).toBe('Tenant test');
|
||||
expect(doc?.tenantId).toBeUndefined();
|
||||
});
|
||||
|
||||
it('bulkSaveMessages should not write caller-supplied tenantId to documents', async () => {
|
||||
const messageId = uuidv4();
|
||||
const conversationId = uuidv4();
|
||||
await bulkSaveMessages([
|
||||
{
|
||||
messageId,
|
||||
conversationId,
|
||||
user: 'user123',
|
||||
text: 'Bulk tenant test',
|
||||
tenantId: 'malicious-tenant',
|
||||
},
|
||||
]);
|
||||
|
||||
const doc = await Message.findOne({ messageId }).lean();
|
||||
expect(doc).not.toBeNull();
|
||||
expect(doc?.text).toBe('Bulk tenant test');
|
||||
expect(doc?.tenantId).toBeUndefined();
|
||||
});
|
||||
|
||||
it('recordMessage should not write caller-supplied tenantId to the document', async () => {
|
||||
const messageId = uuidv4();
|
||||
const conversationId = uuidv4();
|
||||
await recordMessage({
|
||||
user: 'user123',
|
||||
messageId,
|
||||
conversationId,
|
||||
text: 'Record tenant test',
|
||||
tenantId: 'malicious-tenant',
|
||||
});
|
||||
|
||||
const doc = await Message.findOne({ messageId }).lean();
|
||||
expect(doc).not.toBeNull();
|
||||
expect(doc?.text).toBe('Record tenant test');
|
||||
expect(doc?.tenantId).toBeUndefined();
|
||||
});
|
||||
|
||||
it('updateMessage should not write caller-supplied tenantId to the document', async () => {
|
||||
const messageId = uuidv4();
|
||||
const conversationId = uuidv4();
|
||||
await saveMessage({ userId: 'user123' }, { messageId, conversationId, text: 'Original' });
|
||||
|
||||
await updateMessage('user123', {
|
||||
messageId,
|
||||
text: 'Updated',
|
||||
tenantId: 'malicious-tenant',
|
||||
});
|
||||
|
||||
const doc = await Message.findOne({ messageId }).lean();
|
||||
expect(doc?.text).toBe('Updated');
|
||||
expect(doc?.tenantId).toBeUndefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -90,6 +90,7 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa
|
|||
user: userId,
|
||||
messageId: params.newMessageId || params.messageId,
|
||||
};
|
||||
delete update.tenantId;
|
||||
|
||||
if (isTemporary) {
|
||||
try {
|
||||
|
|
@ -158,14 +159,17 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa
|
|||
) {
|
||||
try {
|
||||
const Message = mongoose.models.Message as Model<IMessage>;
|
||||
const bulkOps = messages.map((message) => ({
|
||||
updateOne: {
|
||||
filter: { messageId: message.messageId },
|
||||
update: message,
|
||||
timestamps: !overrideTimestamp,
|
||||
upsert: true,
|
||||
},
|
||||
}));
|
||||
const bulkOps = messages.map((message) => {
|
||||
const { tenantId: _tenantId, ...messageWithoutTenant } = message;
|
||||
return {
|
||||
updateOne: {
|
||||
filter: { messageId: messageWithoutTenant.messageId },
|
||||
update: messageWithoutTenant,
|
||||
timestamps: !overrideTimestamp,
|
||||
upsert: true,
|
||||
},
|
||||
};
|
||||
});
|
||||
const result = await tenantSafeBulkWrite(Message, bulkOps);
|
||||
return result;
|
||||
} catch (err) {
|
||||
|
|
@ -194,7 +198,7 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa
|
|||
}) {
|
||||
try {
|
||||
const Message = mongoose.models.Message as Model<IMessage>;
|
||||
const message = {
|
||||
const message: Record<string, unknown> = {
|
||||
user,
|
||||
endpoint,
|
||||
messageId,
|
||||
|
|
@ -202,6 +206,7 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa
|
|||
parentMessageId,
|
||||
...rest,
|
||||
};
|
||||
delete message.tenantId;
|
||||
|
||||
return await Message.findOneAndUpdate({ user, messageId }, message, {
|
||||
upsert: true,
|
||||
|
|
@ -239,7 +244,7 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa
|
|||
) {
|
||||
try {
|
||||
const Message = mongoose.models.Message as Model<IMessage>;
|
||||
const { messageId, ...update } = message;
|
||||
const { messageId, tenantId: _tenantId, ...update } = message;
|
||||
const updatedMessage = await Message.findOneAndUpdate({ messageId, user: userId }, update, {
|
||||
new: true,
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue