mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-04-03 14:27:20 +02:00
🛡️ refactor: Self-Healing Tenant Isolation Update Guard (#12506)
* refactor: self-healing tenant isolation update guard Replace the strict throw-on-any-tenantId guard with a strip-or-throw approach: - $set/$setOnInsert: strip when value matches current tenant or no context is active; throw only on cross-tenant mutations - $unset/$rename: always strip (unsetting/renaming tenantId is never valid) - Top-level tenantId: same logic as $set This eliminates the entire class of "tenantId in update payload" bugs at the plugin level while preserving the cross-tenant security invariant. * test: update mutation guard tests for self-healing behavior - Convert same-tenant $set/$setOnInsert tests to expect silent stripping instead of throws - Convert $unset test to expect silent stripping - Add cross-tenant throw tests for $set, $setOnInsert, top-level - Add same-tenant stripping tests for $set, $setOnInsert, top-level - Add $rename stripping test - Add no-context stripping test - Update error message assertions to match new cross-tenant message * revert: remove call-site tenantId stripping patches Revert the per-call-site tenantId stripping from #12498 and the excludedKeys patch from #12501. These are no longer needed since the self-healing guard handles tenantId in update payloads at the plugin level. Reverted patches: - conversation.ts: delete update.tenantId in saveConvo(), tenantId destructuring in bulkSaveConvos() - message.ts: delete update.tenantId in saveMessage() and recordMessage(), tenantId destructuring in bulkSaveMessages() and updateMessage() - config.ts: tenantId in excludedKeys Set - config.spec.ts: tenantId in excludedKeys test assertion * fix: strip tenantId from update documents in tenantSafeBulkWrite Mongoose middleware does not fire for bulkWrite, so the plugin-level guard never sees update payloads in bulk operations. Extend injectTenantId() to strip tenantId from update documents for updateOne/updateMany operations, preventing cross-tenant overwrites. * refactor: rename guard, add empty-op cleanup and strict-mode warning - Rename assertNoTenantIdMutation to sanitizeTenantIdMutation - Remove empty operator objects after stripping to avoid MongoDB errors - Log warning in strict mode when stripping tenantId without context - Fix $setOnInsert test to use upsert:true with non-matching filter * test: fix bulk-save tests and add negative excludedKeys assertion - Wrap bulkSaveConvos/bulkSaveMessages tests in tenantStorage.run() to exercise the actual multi-tenant stripping path - Assert tenantId equals the real tenant, not undefined - Add negative assertion: excludedKeys must NOT contain tenantId * fix: type-safe tenantId stripping in tenantSafeBulkWrite - Fix TS2345 error: replace conditional type inference with UpdateQuery<Record<string, unknown>> for stripTenantIdFromUpdate - Handle empty updates after stripping (e.g., $set: { tenantId } as sole field) by filtering null ops from the bulk array - Add 4 tests for bulk update tenantId stripping: plain-object update, $set stripping, $unset stripping, and sole-field-in-$set edge case * fix: resolve TS2345 in stripTenantIdFromUpdate parameter type Use Record<string, unknown> instead of UpdateQuery<> to avoid type incompatibility with Mongoose's AnyObject-based UpdateQuery resolution in CI. * fix: strip tenantId from bulk updates unconditionally Separate sanitization from injection in tenantSafeBulkWrite: tenantId is now stripped from all update documents before any tenant-context checks, closing the gap where no-context and system-context paths passed caller-supplied tenantId through to MongoDB unmodified. * refactor: address review findings in tenant isolation - Fix early-return gap in stripTenantIdFromUpdate that skipped operator-level tenantId when top-level was also present - Lazy-allocate copy in stripTenantIdFromUpdate (no allocation when no tenantId is present) - Document behavioral asymmetry: plugin throws on cross-tenant, bulkWrite strips silently (intentional, documented in JSDoc) - Remove double JSDoc on injectTenantId - Remove redundant cast in stripTenantIdFromUpdate - Use shared frozen EMPTY_BULK_RESULT constant - Remove Record<string, unknown> annotation in recordMessage - Isolate bulkSave* tests: pre-create docs then update with cross-tenant payload, read via runAsSystem to prove stripping is independent of filter injection * fix: no-op empty updates after tenantId sanitization When tenantId is the sole field in an update (e.g., { $set: { tenantId } }), sanitization leaves an empty update object that would fail with "Update document requires atomic operators." The updateGuard now detects this and short-circuits the query by adding an unmatchable filter condition and disabling upsert, matching the bulk-write handling that filters out null ops. * refactor: remove dead logger.warn branches, add mixed-case test - Remove unreachable logger.warn calls in sanitizeTenantIdMutation: queryMiddleware throws before updateGuard in strict+no-context, and isStrict() is false in non-strict+no-context - Add test for combined top-level + operator-level tenantId stripping to lock in the early-return fix * feat: ESLint rule to ban raw bulkWrite and collection.* in data-schemas Add no-restricted-syntax rules to the data-schemas ESLint config that flag direct Model.bulkWrite() and Model.collection.* calls. These bypass Mongoose middleware and the tenant isolation plugin — all bulk writes must use tenantSafeBulkWrite() instead. Test files are excluded since they intentionally use raw driver calls for fixture setup. Also migrate the one remaining raw bulkWrite in seedSystemGrants() to use tenantSafeBulkWrite() for consistency. * test: add findByIdAndUpdate coverage to mutation guard tests * fix: keep tenantSafeBulkWrite in seedSystemGrants, fix ESLint config - Revert to tenantSafeBulkWrite in seedSystemGrants (always runs under runAsSystem, so the wrapper passes through correctly) - Split data-schemas ESLint config: shared TS rules for all files, no-restricted-syntax only for production non-wrapper files - Fix unused destructure vars to use _tenantId pattern
This commit is contained in:
Danny Avila
GitHub
parent
7b368916d5
commit
aa575b274b
12 changed files with 503 additions and 98 deletions
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -48,7 +48,6 @@ export const excludedKeys = new Set([
|
|||
'isArchived',
|
||||
'tags',
|
||||
'user',
|
||||
'tenantId',
|
||||
'__v',
|
||||
'_id',
|
||||
'tools',
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -168,7 +168,6 @@ 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;
|
||||
|
|
@ -221,20 +220,17 @@ 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) => {
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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<IMessage>;
|
||||
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<IMessage>;
|
||||
const message: Record<string, unknown> = {
|
||||
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<IMessage>;
|
||||
const { messageId, tenantId: _tenantId, ...update } = message;
|
||||
const { messageId, ...update } = message;
|
||||
const updatedMessage = await Message.findOneAndUpdate({ messageId, user: userId }, update, {
|
||||
new: true,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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<string, string>,
|
||||
{ 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<string, string>,
|
||||
{ new: true },
|
||||
).lean(),
|
||||
);
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it('blocks tenantId in replaceOne replacement document', async () => {
|
||||
|
|
|
|||
|
|
@ -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<unknown> | 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<unknown> | 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<string, unknown> | 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<string, unknown>)[op];
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
for (const op of STRIP_OPERATORS) {
|
||||
const payload = update[op] as Record<string, unknown> | undefined;
|
||||
if (payload && 'tenantId' in payload) {
|
||||
delete payload.tenantId;
|
||||
if (Object.keys(payload).length === 0) {
|
||||
delete (update as Record<string, unknown>)[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<string, unknown>).tenantId;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -78,7 +109,13 @@ export function applyTenantIsolation(schema: Schema): void {
|
|||
if (tenantId === SYSTEM_TENANT_ID) {
|
||||
return;
|
||||
}
|
||||
assertNoTenantIdMutation(this.getUpdate() as UpdateQuery<unknown> | null);
|
||||
sanitizeTenantIdMutation(this.getUpdate() as UpdateQuery<unknown> | null);
|
||||
|
||||
const update = this.getUpdate() as Record<string, unknown> | null;
|
||||
if (update && Object.keys(update).length === 0) {
|
||||
this.where({ _id: { $in: [] } });
|
||||
this.setOptions({ upsert: false });
|
||||
}
|
||||
};
|
||||
|
||||
const replaceGuard = function (this: Query<unknown, unknown>) {
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>,
|
||||
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<string, unknown>,
|
||||
},
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -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<T>(
|
||||
model: Model<T>,
|
||||
|
|
@ -36,34 +43,65 @@ export async function tenantSafeBulkWrite<T>(
|
|||
): Promise<BulkWriteResult> {
|
||||
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<string, unknown>);
|
||||
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<string, unknown>);
|
||||
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<string, unknown>): Record<string, unknown> {
|
||||
let result: Record<string, unknown> | 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<string, unknown> | 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;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue