From 5e789f589f54406e3bf0502760edad4e1748b48e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 1 Apr 2026 11:16:39 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=8F=20fix:=20Strip=20Unnecessary=20Fie?= =?UTF-8?q?lds=20Across=20Write=20Paths=20in=20Conversation=20&=20Message?= =?UTF-8?q?=20Methods=20(#12498)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../src/methods/conversation.spec.ts | 34 +++++++++ .../data-schemas/src/methods/conversation.ts | 23 +++--- .../data-schemas/src/methods/message.spec.ts | 72 +++++++++++++++++++ packages/data-schemas/src/methods/message.ts | 25 ++++--- 4 files changed, 136 insertions(+), 18 deletions(-) diff --git a/packages/data-schemas/src/methods/conversation.spec.ts b/packages/data-schemas/src/methods/conversation.spec.ts index ae19efaf68..fc1d837e9a 100644 --- a/packages/data-schemas/src/methods/conversation.spec.ts +++ b/packages/data-schemas/src/methods/conversation.spec.ts @@ -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(); + }); + }); }); diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index abfe16bf2d..cf24dbebd0 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -168,6 +168,7 @@ export function createConversationMethods( const messages = await getMessages({ conversationId }, '_id'); const update: Record = { ...convo, messages, user: userId }; + delete update.tenantId; if (newConversationId) { update.conversationId = newConversationId; @@ -220,14 +221,20 @@ export function createConversationMethods( async function bulkSaveConvos(conversations: Array>) { try { const Conversation = mongoose.models.Conversation as Model; - 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; diff --git a/packages/data-schemas/src/methods/message.spec.ts b/packages/data-schemas/src/methods/message.spec.ts index ac85a035b7..5ca51ace0c 100644 --- a/packages/data-schemas/src/methods/message.spec.ts +++ b/packages/data-schemas/src/methods/message.spec.ts @@ -21,6 +21,7 @@ let deleteMessages: ReturnType['deleteMessages']; let bulkSaveMessages: ReturnType['bulkSaveMessages']; let updateMessageText: ReturnType['updateMessageText']; let deleteMessagesSince: ReturnType['deleteMessagesSince']; +let recordMessage: ReturnType['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(); + }); + }); }); diff --git a/packages/data-schemas/src/methods/message.ts b/packages/data-schemas/src/methods/message.ts index 2e638b6bfb..46d6bc7342 100644 --- a/packages/data-schemas/src/methods/message.ts +++ b/packages/data-schemas/src/methods/message.ts @@ -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; - 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; - const message = { + const message: Record = { 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; - const { messageId, ...update } = message; + const { messageId, tenantId: _tenantId, ...update } = message; const updatedMessage = await Message.findOneAndUpdate({ messageId, user: userId }, update, { new: true, });