diff --git a/eslint.config.mjs b/eslint.config.mjs index 1dde65cda1..59c348374d 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -345,7 +345,7 @@ export default [ }, }, { - // **New Data-schemas configuration block** + // **Data-schemas — shared rules for all TS files** files: ['./packages/data-schemas/**/*.ts'], languageOptions: { parser: tsParser, @@ -367,4 +367,25 @@ export default [ ], }, }, + { + // **Data-schemas — ban raw bulkWrite/collection.* in production code** + // Tests and the tenantSafeBulkWrite wrapper itself are excluded. + files: ['./packages/data-schemas/**/*.ts'], + ignores: ['**/*.spec.ts', '**/*.test.ts', '**/utils/tenantBulkWrite.ts'], + rules: { + 'no-restricted-syntax': [ + 'error', + { + selector: "CallExpression[callee.property.name='bulkWrite']", + message: + 'Use tenantSafeBulkWrite() instead of Model.bulkWrite() — Mongoose middleware does not fire for bulkWrite, so the tenant isolation plugin cannot intercept it.', + }, + { + selector: "MemberExpression[property.name='collection'][parent.type='MemberExpression']", + message: + 'Avoid Model.collection.* — raw driver calls bypass all Mongoose middleware including tenant isolation. Use Mongoose model methods or tenantSafeBulkWrite() instead.', + }, + ], + }, + }, ]; diff --git a/packages/data-provider/src/config.spec.ts b/packages/data-provider/src/config.spec.ts index d7c81b02bc..6658a5af5a 100644 --- a/packages/data-provider/src/config.spec.ts +++ b/packages/data-provider/src/config.spec.ts @@ -14,12 +14,13 @@ const endpointsConfig: TEndpointsConfig = { }; describe('excludedKeys', () => { - it.each(['_id', 'user', 'conversationId', '__v', 'tenantId'])( - 'excludes system field "%s"', - (field) => { - expect(excludedKeys.has(field)).toBe(true); - }, - ); + it.each(['_id', 'user', 'conversationId', '__v'])('excludes system field "%s"', (field) => { + expect(excludedKeys.has(field)).toBe(true); + }); + + it('does not exclude tenantId (plugin-level guard owns this)', () => { + expect(excludedKeys.has('tenantId')).toBe(false); + }); }); describe('resolveEndpointType', () => { diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index 779885b456..ae3f5b9560 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -48,7 +48,6 @@ export const excludedKeys = new Set([ 'isArchived', 'tags', 'user', - 'tenantId', '__v', '_id', 'tools', diff --git a/packages/data-schemas/src/methods/conversation.spec.ts b/packages/data-schemas/src/methods/conversation.spec.ts index fc1d837e9a..9e4c2d2f5d 100644 --- a/packages/data-schemas/src/methods/conversation.spec.ts +++ b/packages/data-schemas/src/methods/conversation.spec.ts @@ -4,6 +4,7 @@ import { EModelEndpoint } from 'librechat-data-provider'; import type { IConversation } from '../types'; import { MongoMemoryServer } from 'mongodb-memory-server'; import { ConversationMethods, createConversationMethods } from './conversation'; +import { tenantStorage, runAsSystem } from '~/config/tenantContext'; import { createModels } from '../models'; jest.mock('~/config/winston', () => ({ @@ -923,22 +924,34 @@ describe('Conversation Operations', () => { expect(doc?.tenantId).toBeUndefined(); }); - it('bulkSaveConvos should not write caller-supplied tenantId to documents', async () => { + it('bulkSaveConvos should not overwrite tenantId via update payload', async () => { const conversationId = uuidv4(); - await methods.bulkSaveConvos([ - { + + await tenantStorage.run({ tenantId: 'real-tenant' }, async () => { + await Conversation.create({ conversationId, user: 'user123', - title: 'Bulk Tenant Test', - tenantId: 'malicious-tenant', + title: 'Original', endpoint: EModelEndpoint.openAI, - }, - ]); + }); + }); - const doc = await Conversation.findOne({ conversationId }).lean(); + await tenantStorage.run({ tenantId: 'real-tenant' }, async () => { + await methods.bulkSaveConvos([ + { + conversationId, + user: 'user123', + title: 'Updated', + tenantId: 'malicious-tenant', + endpoint: EModelEndpoint.openAI, + }, + ]); + }); + + const doc = await runAsSystem(async () => Conversation.findOne({ conversationId }).lean()); expect(doc).not.toBeNull(); - expect(doc?.title).toBe('Bulk Tenant Test'); - expect(doc?.tenantId).toBeUndefined(); + expect(doc?.title).toBe('Updated'); + expect(doc?.tenantId).toBe('real-tenant'); }); }); }); diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index cf24dbebd0..00b5cfee7a 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -168,7 +168,6 @@ export function createConversationMethods( const messages = await getMessages({ conversationId }, '_id'); const update: Record = { ...convo, messages, user: userId }; - delete update.tenantId; if (newConversationId) { update.conversationId = newConversationId; @@ -221,20 +220,17 @@ export function createConversationMethods( async function bulkSaveConvos(conversations: Array>) { try { const Conversation = mongoose.models.Conversation as Model; - 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 bulkOps = conversations.map((convo) => ({ + updateOne: { + filter: { + conversationId: convo.conversationId, + user: convo.user, }, - }; - }); + update: convo, + 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 5ca51ace0c..dfa34c0eec 100644 --- a/packages/data-schemas/src/methods/message.spec.ts +++ b/packages/data-schemas/src/methods/message.spec.ts @@ -3,6 +3,7 @@ import { v4 as uuidv4 } from 'uuid'; import { MongoMemoryServer } from 'mongodb-memory-server'; import type { IMessage } from '..'; import { createMessageMethods } from './message'; +import { tenantStorage, runAsSystem } from '~/config/tenantContext'; import { createModels } from '../models'; jest.mock('~/config/winston', () => ({ @@ -957,23 +958,35 @@ describe('Message Operations', () => { expect(doc?.tenantId).toBeUndefined(); }); - it('bulkSaveMessages should not write caller-supplied tenantId to documents', async () => { + it('bulkSaveMessages should not overwrite tenantId via update payload', async () => { const messageId = uuidv4(); const conversationId = uuidv4(); - await bulkSaveMessages([ - { + + await tenantStorage.run({ tenantId: 'real-tenant' }, async () => { + await Message.create({ messageId, conversationId, user: 'user123', - text: 'Bulk tenant test', - tenantId: 'malicious-tenant', - }, - ]); + text: 'Original', + }); + }); - const doc = await Message.findOne({ messageId }).lean(); + await tenantStorage.run({ tenantId: 'real-tenant' }, async () => { + await bulkSaveMessages([ + { + messageId, + conversationId, + user: 'user123', + text: 'Updated', + tenantId: 'malicious-tenant', + }, + ]); + }); + + const doc = await runAsSystem(async () => Message.findOne({ messageId }).lean()); expect(doc).not.toBeNull(); - expect(doc?.text).toBe('Bulk tenant test'); - expect(doc?.tenantId).toBeUndefined(); + expect(doc?.text).toBe('Updated'); + expect(doc?.tenantId).toBe('real-tenant'); }); it('recordMessage should not write caller-supplied tenantId to the document', async () => { diff --git a/packages/data-schemas/src/methods/message.ts b/packages/data-schemas/src/methods/message.ts index 46d6bc7342..2e638b6bfb 100644 --- a/packages/data-schemas/src/methods/message.ts +++ b/packages/data-schemas/src/methods/message.ts @@ -90,7 +90,6 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa user: userId, messageId: params.newMessageId || params.messageId, }; - delete update.tenantId; if (isTemporary) { try { @@ -159,17 +158,14 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa ) { try { const Message = mongoose.models.Message as Model; - const bulkOps = messages.map((message) => { - const { tenantId: _tenantId, ...messageWithoutTenant } = message; - return { - updateOne: { - filter: { messageId: messageWithoutTenant.messageId }, - update: messageWithoutTenant, - timestamps: !overrideTimestamp, - upsert: true, - }, - }; - }); + const bulkOps = messages.map((message) => ({ + updateOne: { + filter: { messageId: message.messageId }, + update: message, + timestamps: !overrideTimestamp, + upsert: true, + }, + })); const result = await tenantSafeBulkWrite(Message, bulkOps); return result; } catch (err) { @@ -198,7 +194,7 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa }) { try { const Message = mongoose.models.Message as Model; - const message: Record = { + const message = { user, endpoint, messageId, @@ -206,7 +202,6 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa parentMessageId, ...rest, }; - delete message.tenantId; return await Message.findOneAndUpdate({ user, messageId }, message, { upsert: true, @@ -244,7 +239,7 @@ export function createMessageMethods(mongoose: typeof import('mongoose')): Messa ) { try { const Message = mongoose.models.Message as Model; - const { messageId, tenantId: _tenantId, ...update } = message; + const { messageId, ...update } = message; const updatedMessage = await Message.findOneAndUpdate({ messageId, user: userId }, update, { new: true, }); diff --git a/packages/data-schemas/src/methods/systemGrant.ts b/packages/data-schemas/src/methods/systemGrant.ts index c43597c140..5b31619706 100644 --- a/packages/data-schemas/src/methods/systemGrant.ts +++ b/packages/data-schemas/src/methods/systemGrant.ts @@ -3,6 +3,7 @@ import type { Types, Model, ClientSession, FilterQuery } from 'mongoose'; import type { SystemCapability } from '~/types/admin'; import type { ISystemGrant } from '~/types'; import { SystemCapabilities, CapabilityImplications } from '~/admin/capabilities'; +import { tenantSafeBulkWrite } from '~/utils/tenantBulkWrite'; import { normalizePrincipalId } from '~/utils/principal'; import logger from '~/config/winston'; @@ -349,7 +350,9 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')) { } /** - * Seed the ADMIN role with all system capabilities (no tenantId — single-instance mode). + * Seed the ADMIN role with all system capabilities. + * Context-agnostic: caller provides tenant context (e.g., `runAsSystem()` for + * startup, `tenantStorage.run()` for future per-tenant provisioning). * Idempotent and concurrency-safe: uses bulkWrite with ordered:false so parallel * server instances (K8s rolling deploy, PM2 cluster) do not race on E11000. * Retries up to 3 times with exponential backoff on transient failures. @@ -379,7 +382,7 @@ export function createSystemGrantMethods(mongoose: typeof import('mongoose')) { upsert: true, }, })); - await SystemGrant.bulkWrite(ops, { ordered: false }); + await tenantSafeBulkWrite(SystemGrant, ops, { ordered: false }); return; } catch (err) { if (attempt < maxRetries) { diff --git a/packages/data-schemas/src/models/plugins/tenantIsolation.spec.ts b/packages/data-schemas/src/models/plugins/tenantIsolation.spec.ts index 52c40c54bc..82e74d2cea 100644 --- a/packages/data-schemas/src/models/plugins/tenantIsolation.spec.ts +++ b/packages/data-schemas/src/models/plugins/tenantIsolation.spec.ts @@ -320,23 +320,43 @@ describe('applyTenantIsolation', () => { await TestModel.create({ name: 'guarded', tenantId: 'tenant-a' }); }); - it('blocks $set of tenantId via findOneAndUpdate', async () => { + it('throws on cross-tenant $set of tenantId', async () => { await expect( tenantStorage.run({ tenantId: 'tenant-a' }, async () => TestModel.findOneAndUpdate({ name: 'guarded' }, { $set: { tenantId: 'tenant-b' } }), ), - ).rejects.toThrow('[TenantIsolation] Modifying tenantId via update operators is not allowed'); + ).rejects.toThrow('[TenantIsolation] Cross-tenant tenantId mutation is not allowed'); }); - it('blocks $unset of tenantId via updateOne', async () => { - await expect( - tenantStorage.run({ tenantId: 'tenant-a' }, async () => - TestModel.updateOne({ name: 'guarded' }, { $unset: { tenantId: 1 } }), - ), - ).rejects.toThrow('[TenantIsolation] Modifying tenantId via update operators is not allowed'); + it('strips same-tenant tenantId from $set', async () => { + const result = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.findOneAndUpdate( + { name: 'guarded' }, + { $set: { tenantId: 'tenant-a', name: 'updated-same' } }, + { new: true }, + ).lean(), + ); + + expect(result).not.toBeNull(); + expect(result!.name).toBe('updated-same'); + expect(result!.tenantId).toBe('tenant-a'); }); - it('blocks top-level tenantId in update payload', async () => { + it('strips tenantId from $unset', async () => { + const result = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.findOneAndUpdate( + { name: 'guarded' }, + { $unset: { tenantId: 1 }, $set: { name: 'unset-attempt' } }, + { new: true }, + ).lean(), + ); + + expect(result).not.toBeNull(); + expect(result!.name).toBe('unset-attempt'); + expect(result!.tenantId).toBe('tenant-a'); + }); + + it('throws on cross-tenant top-level tenantId', async () => { await expect( tenantStorage.run({ tenantId: 'tenant-a' }, async () => TestModel.updateOne({ name: 'guarded' }, { tenantId: 'tenant-b' } as Record< @@ -344,15 +364,63 @@ describe('applyTenantIsolation', () => { string >), ), - ).rejects.toThrow('[TenantIsolation] Modifying tenantId via update operators is not allowed'); + ).rejects.toThrow('[TenantIsolation] Cross-tenant tenantId mutation is not allowed'); }); - it('blocks $setOnInsert of tenantId via updateMany', async () => { + it('strips same-tenant top-level tenantId', async () => { + const result = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.findOneAndUpdate( + { name: 'guarded' }, + { tenantId: 'tenant-a', name: 'top-level-same' } as Record, + { new: true }, + ).lean(), + ); + + expect(result).not.toBeNull(); + expect(result!.name).toBe('top-level-same'); + expect(result!.tenantId).toBe('tenant-a'); + }); + + it('throws on cross-tenant $setOnInsert of tenantId', async () => { await expect( tenantStorage.run({ tenantId: 'tenant-a' }, async () => TestModel.updateMany({}, { $setOnInsert: { tenantId: 'tenant-b' } }), ), - ).rejects.toThrow('[TenantIsolation] Modifying tenantId via update operators is not allowed'); + ).rejects.toThrow('[TenantIsolation] Cross-tenant tenantId mutation is not allowed'); + }); + + it('strips same-tenant tenantId from $setOnInsert on upsert', async () => { + const uniqueName = `upsert-soi-${Date.now()}`; + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.updateOne( + { name: uniqueName }, + { $setOnInsert: { tenantId: 'tenant-a', name: uniqueName } }, + { upsert: true }, + ), + ); + + const doc = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.findOne({ name: uniqueName }).lean(), + ); + expect(doc).not.toBeNull(); + expect(doc!.tenantId).toBe('tenant-a'); + }); + + it('strips same-tenant tenantId from $set via findByIdAndUpdate', async () => { + const doc = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.findOne({ name: 'guarded' }).lean(), + ); + const result = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.findByIdAndUpdate( + doc!._id, + { $set: { tenantId: 'tenant-a', name: 'byId-same' } }, + { new: true }, + ).lean(), + ); + + expect(result).not.toBeNull(); + expect(result!.name).toBe('byId-same'); + expect(result!.tenantId).toBe('tenant-a'); }); it('allows updates that do not touch tenantId', async () => { @@ -382,10 +450,48 @@ describe('applyTenantIsolation', () => { expect(result!.tenantId).toBe('tenant-b'); }); - it('blocks tenantId mutation even without tenant context', async () => { - await expect( - TestModel.updateOne({ name: 'guarded' }, { $set: { tenantId: 'tenant-b' } }), - ).rejects.toThrow('[TenantIsolation] Modifying tenantId via update operators is not allowed'); + it('strips tenantId from $set without tenant context', async () => { + await TestModel.updateOne( + { name: 'guarded' }, + { $set: { tenantId: 'tenant-b', name: 'no-ctx' } }, + ); + + const doc = await TestModel.findOne({ name: 'no-ctx' }).lean(); + expect(doc).not.toBeNull(); + expect(doc!.tenantId).toBe('tenant-a'); + }); + + it('strips tenantId from $rename without tenant context', async () => { + await TestModel.updateOne({ name: 'guarded' }, { $rename: { tenantId: 'oldTenant' } }); + + const doc = await TestModel.findOne({ name: 'guarded' }).lean(); + expect(doc).not.toBeNull(); + expect(doc!.tenantId).toBe('tenant-a'); + }); + + it('no-ops when update contains only tenantId', async () => { + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.updateOne({ name: 'guarded' }, { $set: { tenantId: 'tenant-a' } }), + ); + + const doc = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.findOne({ name: 'guarded' }).lean(), + ); + expect(doc).not.toBeNull(); + expect(doc!.name).toBe('guarded'); + expect(doc!.tenantId).toBe('tenant-a'); + }); + + it('no-ops when top-level update contains only tenantId', async () => { + const result = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + TestModel.findOneAndUpdate( + { name: 'guarded' }, + { tenantId: 'tenant-a' } as Record, + { new: true }, + ).lean(), + ); + + expect(result).toBeNull(); }); it('blocks tenantId in replaceOne replacement document', async () => { diff --git a/packages/data-schemas/src/models/plugins/tenantIsolation.ts b/packages/data-schemas/src/models/plugins/tenantIsolation.ts index ddb98f9aa9..06f8e035fb 100644 --- a/packages/data-schemas/src/models/plugins/tenantIsolation.ts +++ b/packages/data-schemas/src/models/plugins/tenantIsolation.ts @@ -26,20 +26,51 @@ if ( const TENANT_ISOLATION_APPLIED = Symbol.for('librechat:tenantIsolation'); -const MUTATION_OPERATORS = ['$set', '$unset', '$setOnInsert', '$rename'] as const; +const VALUE_OPERATORS = ['$set', '$setOnInsert'] as const; +const STRIP_OPERATORS = ['$unset', '$rename'] as const; -function assertNoTenantIdMutation(update: UpdateQuery | null): void { +/** + * Strips `tenantId` from update payloads when it matches the current tenant + * (or no context is active). Throws on cross-tenant mutations. + * + * `$unset`/`$rename` always strip — unsetting/renaming tenantId is never valid. + * Empty operator objects are removed after stripping to avoid MongoDB errors. + */ +function sanitizeTenantIdMutation(update: UpdateQuery | null): void { if (!update) { return; } - for (const op of MUTATION_OPERATORS) { + + const currentTenantId = getTenantId(); + + for (const op of VALUE_OPERATORS) { const payload = update[op] as Record | undefined; if (payload && 'tenantId' in payload) { - throw new Error('[TenantIsolation] Modifying tenantId via update operators is not allowed'); + if (currentTenantId && payload.tenantId !== currentTenantId) { + throw new Error('[TenantIsolation] Cross-tenant tenantId mutation is not allowed'); + } + delete payload.tenantId; + if (Object.keys(payload).length === 0) { + delete (update as Record)[op]; + } } } + + for (const op of STRIP_OPERATORS) { + const payload = update[op] as Record | undefined; + if (payload && 'tenantId' in payload) { + delete payload.tenantId; + if (Object.keys(payload).length === 0) { + delete (update as Record)[op]; + } + } + } + if ('tenantId' in update) { - throw new Error('[TenantIsolation] Modifying tenantId via update operators is not allowed'); + if (currentTenantId && update.tenantId !== currentTenantId) { + throw new Error('[TenantIsolation] Cross-tenant tenantId mutation is not allowed'); + } + delete (update as Record).tenantId; } } @@ -78,7 +109,13 @@ export function applyTenantIsolation(schema: Schema): void { if (tenantId === SYSTEM_TENANT_ID) { return; } - assertNoTenantIdMutation(this.getUpdate() as UpdateQuery | null); + sanitizeTenantIdMutation(this.getUpdate() as UpdateQuery | null); + + const update = this.getUpdate() as Record | null; + if (update && Object.keys(update).length === 0) { + this.where({ _id: { $in: [] } }); + this.setOptions({ upsert: false }); + } }; const replaceGuard = function (this: Query) { diff --git a/packages/data-schemas/src/utils/tenantBulkWrite.spec.ts b/packages/data-schemas/src/utils/tenantBulkWrite.spec.ts index 059868b8a1..d2db95d448 100644 --- a/packages/data-schemas/src/utils/tenantBulkWrite.spec.ts +++ b/packages/data-schemas/src/utils/tenantBulkWrite.spec.ts @@ -275,6 +275,135 @@ describe('tenantSafeBulkWrite', () => { }); }); + describe('tenantId stripping from update documents', () => { + it('strips top-level tenantId from updateOne plain-object update', async () => { + const Model = createTestModel('stripPlain'); + + await runAsSystem(async () => { + await Model.create({ name: 'target', value: 1, tenantId: 'tenant-a' }); + }); + + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => { + await tenantSafeBulkWrite(Model, [ + { + updateOne: { + filter: { name: 'target' }, + update: { value: 99, tenantId: 'cross-tenant' } as Record, + upsert: true, + }, + }, + ]); + }); + + const doc = await runAsSystem(async () => Model.findOne({ name: 'target' }).lean()); + expect(doc?.value).toBe(99); + expect(doc?.tenantId).toBe('tenant-a'); + }); + + it('strips tenantId from $set in updateOne', async () => { + const Model = createTestModel('stripSet'); + + await runAsSystem(async () => { + await Model.create({ name: 'target', value: 1, tenantId: 'tenant-a' }); + }); + + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => { + await tenantSafeBulkWrite(Model, [ + { + updateOne: { + filter: { name: 'target' }, + update: { $set: { value: 50, tenantId: 'cross-tenant' } }, + }, + }, + ]); + }); + + const doc = await runAsSystem(async () => Model.findOne({ name: 'target' }).lean()); + expect(doc?.value).toBe(50); + expect(doc?.tenantId).toBe('tenant-a'); + }); + + it('strips tenantId from $unset in updateMany', async () => { + const Model = createTestModel('stripUnset'); + + await runAsSystem(async () => { + await Model.create([ + { name: 'batch', value: 1, tenantId: 'tenant-a' }, + { name: 'batch', value: 2, tenantId: 'tenant-a' }, + ]); + }); + + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => { + await tenantSafeBulkWrite(Model, [ + { + updateMany: { + filter: { name: 'batch' }, + update: { $unset: { tenantId: 1 }, $set: { value: 0 } }, + }, + }, + ]); + }); + + const docs = await runAsSystem(async () => Model.find({ name: 'batch' }).lean()); + expect(docs).toHaveLength(2); + for (const doc of docs) { + expect(doc.value).toBe(0); + expect(doc.tenantId).toBe('tenant-a'); + } + }); + + it('handles update where tenantId is the only field in $set', async () => { + const Model = createTestModel('stripOnlyField'); + + await runAsSystem(async () => { + await Model.create({ name: 'target', value: 5, tenantId: 'tenant-a' }); + }); + + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => { + await tenantSafeBulkWrite(Model, [ + { + updateOne: { + filter: { name: 'target' }, + update: { $set: { tenantId: 'cross-tenant' } }, + }, + }, + ]); + }); + + const doc = await runAsSystem(async () => Model.findOne({ name: 'target' }).lean()); + expect(doc?.tenantId).toBe('tenant-a'); + expect(doc?.value).toBe(5); + }); + }); + + describe('mixed top-level and operator tenantId', () => { + it('strips both top-level and $set tenantId when both are present', async () => { + const Model = createTestModel('stripBoth'); + + await runAsSystem(async () => { + await Model.create({ name: 'target', value: 1, tenantId: 'tenant-a' }); + }); + + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => { + await tenantSafeBulkWrite(Model, [ + { + updateOne: { + filter: { name: 'target' }, + update: { + tenantId: 'top-cross', + $set: { value: 99, tenantId: 'set-cross' }, + } as Record, + }, + }, + ]); + }); + + const doc = await runAsSystem(async () => Model.findOne({ name: 'target' }).lean()); + expect(doc?.value).toBe(99); + expect(doc?.tenantId).toBe('tenant-a'); + }); + }); + describe('edge cases', () => { it('handles empty ops array', async () => { const Model = createTestModel('emptyOps'); @@ -307,6 +436,27 @@ describe('tenantSafeBulkWrite', () => { expect(result.modifiedCount).toBe(1); }); + it('strips tenantId from updates even without tenant context', async () => { + const Model = createTestModel('noCtxStrip'); + + await runAsSystem(async () => { + await Model.create({ name: 'no-ctx-strip', value: 1, tenantId: 'original' }); + }); + + await tenantSafeBulkWrite(Model, [ + { + updateOne: { + filter: { name: 'no-ctx-strip' }, + update: { $set: { value: 50, tenantId: 'injected' } }, + }, + }, + ]); + + const doc = await runAsSystem(async () => Model.findOne({ name: 'no-ctx-strip' }).lean()); + expect(doc?.value).toBe(50); + expect(doc?.tenantId).toBe('original'); + }); + it('throws in strict mode', async () => { process.env.TENANT_ISOLATION_STRICT = 'true'; _resetBulkWriteStrictCache(); diff --git a/packages/data-schemas/src/utils/tenantBulkWrite.ts b/packages/data-schemas/src/utils/tenantBulkWrite.ts index 16ef5fa057..e8e104ca72 100644 --- a/packages/data-schemas/src/utils/tenantBulkWrite.ts +++ b/packages/data-schemas/src/utils/tenantBulkWrite.ts @@ -18,16 +18,23 @@ export function _resetBulkWriteStrictCache(): void { * Tenant-safe wrapper around Mongoose `Model.bulkWrite()`. * * Mongoose's `bulkWrite` does not trigger schema-level middleware hooks, so the - * `applyTenantIsolation` plugin cannot intercept it. This wrapper injects the - * current ALS tenant context into every operation's filter and/or document - * before delegating to the native `bulkWrite`. + * `applyTenantIsolation` plugin cannot intercept it. This wrapper: + * + * 1. **Sanitizes** every update document by stripping `tenantId` unconditionally + * (both top-level and inside `$set`/`$unset`/`$setOnInsert`/`$rename`). + * 2. **Injects** `tenantId` into operation filters and insert documents when a + * tenant context is active. + * + * Unlike the Mongoose middleware guard (`sanitizeTenantIdMutation`), which throws + * on cross-tenant values, this wrapper strips silently. Throwing mid-batch would + * abort the entire write for one bad field; the filter injection already scopes + * every operation to the correct tenant. * * Behavior: - * - **tenantId present** (normal request): injects `{ tenantId }` into every - * operation filter (updateOne, deleteOne, replaceOne) and document (insertOne). - * - **SYSTEM_TENANT_ID**: skips injection (cross-tenant system operation). + * - **tenantId present** (normal request): sanitize + inject into filters/documents. + * - **SYSTEM_TENANT_ID**: sanitize only, skip injection (cross-tenant system op). * - **No tenantId + strict mode**: throws (fail-closed, same as the plugin). - * - **No tenantId + non-strict**: passes through without injection (backward compat). + * - **No tenantId + non-strict**: sanitize only, no injection (backward compat). */ export async function tenantSafeBulkWrite( model: Model, @@ -36,34 +43,65 @@ export async function tenantSafeBulkWrite( ): Promise { const tenantId = getTenantId(); + // Strip tenantId from update documents unconditionally — application code + // must never control tenantId via update payloads regardless of context. + const sanitized = ops.map(sanitizeBulkOp).filter((op): op is AnyBulkWriteOperation => op != null); + if (!tenantId) { if (isStrict()) { throw new Error( `[TenantIsolation] bulkWrite on ${model.modelName} attempted without tenant context in strict mode`, ); } - return model.bulkWrite(ops, options); + return sanitized.length > 0 ? model.bulkWrite(sanitized, options) : EMPTY_BULK_RESULT; } if (tenantId === SYSTEM_TENANT_ID) { - return model.bulkWrite(ops, options); + return sanitized.length > 0 ? model.bulkWrite(sanitized, options) : EMPTY_BULK_RESULT; } - const injected = ops.map((op) => injectTenantId(op, tenantId)); - return model.bulkWrite(injected, options); + const injected = sanitized.map((op) => injectTenantId(op, tenantId)); + return injected.length > 0 ? model.bulkWrite(injected, options) : EMPTY_BULK_RESULT; +} + +/** Returned when all ops are dropped after sanitization. Single shared instance. */ +const EMPTY_BULK_RESULT = Object.freeze({ + insertedCount: 0, + matchedCount: 0, + modifiedCount: 0, + deletedCount: 0, + upsertedCount: 0, + upsertedIds: {}, + insertedIds: {}, +}) as unknown as BulkWriteResult; + +/** Strips tenantId from update documents. Returns null if the op becomes empty. */ +function sanitizeBulkOp(op: AnyBulkWriteOperation): AnyBulkWriteOperation | null { + if ('updateOne' in op) { + const { update, ...rest } = op.updateOne; + const stripped = stripTenantIdFromUpdate(update as Record); + return Object.keys(stripped).length === 0 ? null : { updateOne: { ...rest, update: stripped } }; + } + + if ('updateMany' in op) { + const { update, ...rest } = op.updateMany; + const stripped = stripTenantIdFromUpdate(update as Record); + return Object.keys(stripped).length === 0 + ? null + : { updateMany: { ...rest, update: stripped } }; + } + + return op; } /** - * Injects `tenantId` into a single bulk-write operation. + * Injects tenantId into every operation's filter and document. + * Assumes update payloads have already been sanitized by `sanitizeBulkOp`. * Returns a new operation object — does not mutate the original. */ function injectTenantId(op: AnyBulkWriteOperation, tenantId: string): AnyBulkWriteOperation { if ('insertOne' in op) { - return { - insertOne: { - document: { ...op.insertOne.document, tenantId }, - }, - }; + return { insertOne: { document: { ...op.insertOne.document, tenantId } } }; } if ('updateOne' in op) { @@ -107,3 +145,36 @@ function injectTenantId(op: AnyBulkWriteOperation, tenantId: string): AnyBulkWri ); return op; } + +const MUTATION_OPS = ['$set', '$unset', '$setOnInsert', '$rename'] as const; + +/** + * Strips `tenantId` from a bulk-write update document — both top-level + * and inside mutation operators. Allocates only when stripping is needed. + */ +function stripTenantIdFromUpdate(update: Record): Record { + let result: Record | null = null; + + if ('tenantId' in update) { + const { tenantId: _tenantId, ...rest } = update; + result = rest; + } + + for (const op of MUTATION_OPS) { + const source = result ?? update; + const payload = source[op] as Record | undefined; + if (payload && 'tenantId' in payload) { + if (!result) { + result = { ...update }; + } + const { tenantId: _tenantId, ...rest } = payload; + if (Object.keys(rest).length > 0) { + result[op] = rest; + } else { + delete result[op]; + } + } + } + + return result ?? update; +}