From 64d807bd0ae0768e25bb7d4a8658b627217da835 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 4 Sep 2026 12:05:35 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=9B=EF=B8=8F=20refactor:=20Extract=20E?= =?UTF-8?q?ngine-Neutral=20Tenant=20Policy=20(#15575)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tenant isolation was defined twice — once as Mongoose query middleware in `applyTenantIsolation`, once again inside `tenantSafeBulkWrite` for the bulk path middleware cannot intercept — with two independently cached strict-mode flags and two slightly different sets of rules. Both are now bindings over a single policy in `~/tenant/policy`, which is written against plain objects and imports nothing from Mongoose. The Mongoose middleware stays where it is: it is the only enforcement point that also covers `doc.save()`, `populate()` and other engine-internal paths, so removing it would lose coverage. What changes is that it no longer *owns* the rules. - `sanitizeTenantMutation` unifies the guard (throws cross-tenant) and strip (silent) behaviours behind one `mode` parameter. - Sanitizing is copy-on-write; caller payloads are no longer mutated. - `guard` mode now leaves system-scoped payloads untouched inside the policy rather than relying on each caller to check first. - The two strict-mode caches collapse into one. `policy.spec.ts` exercises the whole contract with no database and no model — 33 tests in 0.3s — which is the property a second engine needs. No behaviour change: 78 suites / 2749 tests green. --- .../src/models/plugins/tenantIsolation.ts | 186 +++-------- .../data-schemas/src/tenant/policy.spec.ts | 295 ++++++++++++++++++ packages/data-schemas/src/tenant/policy.ts | 244 +++++++++++++++ .../data-schemas/src/utils/tenantBulkWrite.ts | 97 ++---- 4 files changed, 616 insertions(+), 206 deletions(-) create mode 100644 packages/data-schemas/src/tenant/policy.spec.ts create mode 100644 packages/data-schemas/src/tenant/policy.ts diff --git a/packages/data-schemas/src/models/plugins/tenantIsolation.ts b/packages/data-schemas/src/models/plugins/tenantIsolation.ts index 1450610345..9173215024 100644 --- a/packages/data-schemas/src/models/plugins/tenantIsolation.ts +++ b/packages/data-schemas/src/models/plugins/tenantIsolation.ts @@ -1,79 +1,33 @@ -import type { Schema, Query, Aggregate, UpdateQuery } from 'mongoose'; -import { getTenantId, SYSTEM_TENANT_ID } from '~/config/tenantContext'; -import logger from '~/config/winston'; +import type { Schema, Query, Aggregate } from 'mongoose'; +import type { TenantDocument, TenantUpdate } from '~/tenant/policy'; +import { + tenantFilter, + scopeReplacement, + currentTenantScope, + resolveTenantScope, + stampTenantOnDocument, + sanitizeTenantMutation, + resetTenantStrictCache, + warnOnInvalidStrictSetting, +} from '~/tenant/policy'; -let _strictMode: boolean | undefined; - -function isStrict(): boolean { - return (_strictMode ??= process.env.TENANT_ISOLATION_STRICT === 'true'); -} +/** + * Mongoose binding for the engine-neutral tenant-isolation policy. + * + * Every rule enforced here is defined in `~/tenant/policy`; this module only + * adapts Mongoose's middleware surface to it, so a second storage engine can + * reuse the same decisions without reimplementing them. + */ /** Resets the cached strict-mode flag. Exposed for test teardown only. */ export function _resetStrictCache(): void { - _strictMode = undefined; + resetTenantStrictCache(); } -if ( - process.env.TENANT_ISOLATION_STRICT && - process.env.TENANT_ISOLATION_STRICT !== 'true' && - process.env.TENANT_ISOLATION_STRICT !== 'false' -) { - logger.warn( - `[TenantIsolation] TENANT_ISOLATION_STRICT="${process.env.TENANT_ISOLATION_STRICT}" ` + - 'is not "true" or "false"; defaulting to non-strict mode.', - ); -} +warnOnInvalidStrictSetting(); const TENANT_ISOLATION_APPLIED = Symbol.for('librechat:tenantIsolation'); -const VALUE_OPERATORS = ['$set', '$setOnInsert'] as const; -const STRIP_OPERATORS = ['$unset', '$rename'] as const; - -/** - * 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; - } - - const currentTenantId = getTenantId(); - - for (const op of VALUE_OPERATORS) { - const payload = update[op] as Record | undefined; - if (payload && 'tenantId' in payload) { - 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) { - if (currentTenantId && update.tenantId !== currentTenantId) { - throw new Error('[TenantIsolation] Cross-tenant tenantId mutation is not allowed'); - } - delete (update as Record).tenantId; - } -} - /** * Mongoose schema plugin that enforces tenant-level data isolation. * @@ -91,47 +45,38 @@ export function applyTenantIsolation(schema: Schema): void { s[TENANT_ISOLATION_APPLIED] = true; const queryMiddleware = function (this: Query) { - const tenantId = getTenantId(); - - if (!tenantId && isStrict()) { - throw new Error('[TenantIsolation] Query attempted without tenant context in strict mode'); + const filter = tenantFilter(resolveTenantScope('Query')); + if (filter) { + this.where(filter); } - - if (!tenantId || tenantId === SYSTEM_TENANT_ID) { - return; - } - - this.where({ tenantId }); }; const updateGuard = function (this: Query) { - const tenantId = getTenantId(); - if (tenantId === SYSTEM_TENANT_ID) { + const scope = currentTenantScope(); + if (scope.kind === 'system') { return; } - sanitizeTenantIdMutation(this.getUpdate() as UpdateQuery | null); - const update = this.getUpdate() as Record | null; - if (update && Object.keys(update).length === 0) { + const result = sanitizeTenantMutation(scope, this.getUpdate() as TenantUpdate | null, 'guard'); + if (result.changed) { + this.setUpdate(result.update); + } + + if (result.emptied) { this.where({ _id: { $in: [] } }); this.setOptions({ upsert: false }); } }; const replaceGuard = function (this: Query) { - const tenantId = getTenantId(); - if (tenantId === SYSTEM_TENANT_ID) { + const scope = currentTenantScope(); + if (scope.kind === 'system') { return; } - const replacement = this.getUpdate() as Record | null; - if (!replacement) { - return; - } - if ('tenantId' in replacement && replacement.tenantId !== tenantId) { - throw new Error('[TenantIsolation] Modifying tenantId via replacement is not allowed'); - } - if (tenantId && !('tenantId' in replacement)) { - replacement.tenantId = tenantId; + + const result = scopeReplacement(scope, this.getUpdate() as TenantDocument | null); + if (result.changed && result.replacement) { + this.setUpdate(result.replacement); } }; @@ -156,60 +101,27 @@ export function applyTenantIsolation(schema: Schema): void { schema.pre('findOneAndReplace', replaceGuard); schema.pre('aggregate', function (this: Aggregate) { - const tenantId = getTenantId(); - - if (!tenantId && isStrict()) { - throw new Error( - '[TenantIsolation] Aggregate attempted without tenant context in strict mode', - ); + const filter = tenantFilter(resolveTenantScope('Aggregate')); + if (filter) { + this.pipeline().unshift({ $match: filter }); } - - if (!tenantId || tenantId === SYSTEM_TENANT_ID) { - return; - } - - this.pipeline().unshift({ $match: { tenantId } }); }); schema.pre('save', function () { - const tenantId = getTenantId(); - - if (!tenantId && isStrict()) { - throw new Error('[TenantIsolation] Save attempted without tenant context in strict mode'); - } - - if (tenantId && tenantId !== SYSTEM_TENANT_ID) { - if (!this.tenantId) { - this.tenantId = tenantId; - } else if (isStrict() && this.tenantId !== tenantId) { - throw new Error( - '[TenantIsolation] Document tenantId does not match current tenant context', - ); - } - } + stampTenantOnDocument(resolveTenantScope('Save'), this as unknown as TenantDocument); }); schema.pre('insertMany', function (next, docs) { - const tenantId = getTenantId(); - - if (!tenantId && isStrict()) { - return next( - new Error('[TenantIsolation] insertMany attempted without tenant context in strict mode'), - ); - } - - if (tenantId && tenantId !== SYSTEM_TENANT_ID && Array.isArray(docs)) { - for (const doc of docs) { - if (!doc.tenantId) { - doc.tenantId = tenantId; - } else if (isStrict() && doc.tenantId !== tenantId) { - return next( - new Error('[TenantIsolation] Document tenantId does not match current tenant context'), - ); + try { + const scope = resolveTenantScope('insertMany'); + if (Array.isArray(docs)) { + for (const doc of docs) { + stampTenantOnDocument(scope, doc as TenantDocument); } } + } catch (error) { + return next(error as Error); } - next(); }); } diff --git a/packages/data-schemas/src/tenant/policy.spec.ts b/packages/data-schemas/src/tenant/policy.spec.ts new file mode 100644 index 0000000000..d72143bcee --- /dev/null +++ b/packages/data-schemas/src/tenant/policy.spec.ts @@ -0,0 +1,295 @@ +import type { TenantScope } from './policy'; +import { + tenantFilter, + scopeReplacement, + currentTenantScope, + resolveTenantScope, + TenantIsolationError, + stampTenantOnDocument, + sanitizeTenantMutation, + resetTenantStrictCache, +} from './policy'; +import { tenantStorage, SYSTEM_TENANT_ID } from '~/config/tenantContext'; + +/** + * The tenant policy is engine-neutral: these tests run with no database and no + * Mongoose model. A storage engine that satisfies this contract inherits tenant + * isolation without reimplementing any of its rules. + */ + +const SCOPED: TenantScope = { kind: 'scoped', tenantId: 'tenant-a' }; +const SYSTEM: TenantScope = { kind: 'system' }; +const UNSCOPED: TenantScope = { kind: 'unscoped' }; + +const withStrict = async (value: boolean, fn: () => void | Promise): Promise => { + const previous = process.env.TENANT_ISOLATION_STRICT; + process.env.TENANT_ISOLATION_STRICT = String(value); + resetTenantStrictCache(); + try { + await fn(); + } finally { + if (previous === undefined) { + delete process.env.TENANT_ISOLATION_STRICT; + } else { + process.env.TENANT_ISOLATION_STRICT = previous; + } + resetTenantStrictCache(); + } +}; + +afterEach(() => { + resetTenantStrictCache(); +}); + +describe('currentTenantScope', () => { + it('reports unscoped with no ambient context', () => { + expect(currentTenantScope()).toEqual({ kind: 'unscoped' }); + }); + + it('reports the active tenant', async () => { + const scope = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + currentTenantScope(), + ); + expect(scope).toEqual({ kind: 'scoped', tenantId: 'tenant-a' }); + }); + + it('reports system for the cross-tenant sentinel', async () => { + const scope = await tenantStorage.run({ tenantId: SYSTEM_TENANT_ID }, async () => + currentTenantScope(), + ); + expect(scope).toEqual({ kind: 'system' }); + }); + + it('never throws in strict mode', async () => { + await withStrict(true, () => { + expect(currentTenantScope()).toEqual({ kind: 'unscoped' }); + }); + }); +}); + +describe('resolveTenantScope', () => { + it('fails closed in strict mode and names the operation', async () => { + await withStrict(true, () => { + expect(() => resolveTenantScope('Query')).toThrow( + '[TenantIsolation] Query attempted without tenant context in strict mode', + ); + expect(() => resolveTenantScope('Query')).toThrow(TenantIsolationError); + }); + }); + + it('passes through when not strict', async () => { + await withStrict(false, () => { + expect(resolveTenantScope('Query')).toEqual({ kind: 'unscoped' }); + }); + }); + + it('lets an active tenant through strict mode', async () => { + await withStrict(true, async () => { + const scope = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + resolveTenantScope('Query'), + ); + expect(scope).toEqual({ kind: 'scoped', tenantId: 'tenant-a' }); + }); + }); + + it('lets the system sentinel through strict mode', async () => { + await withStrict(true, async () => { + const scope = await tenantStorage.run({ tenantId: SYSTEM_TENANT_ID }, async () => + resolveTenantScope('Query'), + ); + expect(scope).toEqual({ kind: 'system' }); + }); + }); +}); + +describe('tenantFilter', () => { + it('restricts to the active tenant', () => { + expect(tenantFilter(SCOPED)).toEqual({ tenantId: 'tenant-a' }); + }); + + it('does not restrict system or unscoped reads', () => { + expect(tenantFilter(SYSTEM)).toBeUndefined(); + expect(tenantFilter(UNSCOPED)).toBeUndefined(); + }); +}); + +describe('sanitizeTenantMutation (guard mode)', () => { + it('refuses a cross-tenant $set', () => { + expect(() => + sanitizeTenantMutation(SCOPED, { $set: { tenantId: 'tenant-b' } }, 'guard'), + ).toThrow('[TenantIsolation] Cross-tenant tenantId mutation is not allowed'); + }); + + it('refuses a cross-tenant top-level value', () => { + expect(() => sanitizeTenantMutation(SCOPED, { tenantId: 'tenant-b' }, 'guard')).toThrow( + TenantIsolationError, + ); + }); + + it('strips a matching tenantId rather than writing it', () => { + const result = sanitizeTenantMutation( + SCOPED, + { $set: { tenantId: 'tenant-a', name: 'x' } }, + 'guard', + ); + expect(result.update).toEqual({ $set: { name: 'x' } }); + expect(result.changed).toBe(true); + expect(result.emptied).toBe(false); + }); + + it('drops an operator left empty by stripping', () => { + const result = sanitizeTenantMutation(SCOPED, { $set: { tenantId: 'tenant-a' } }, 'guard'); + expect(result.update).toEqual({}); + expect(result.emptied).toBe(true); + }); + + it('always strips $unset and $rename without throwing', () => { + const result = sanitizeTenantMutation( + SCOPED, + { $unset: { tenantId: '' }, $rename: { tenantId: 'other' } }, + 'guard', + ); + expect(result.update).toEqual({}); + expect(result.emptied).toBe(true); + }); + + it('strips silently when no tenant is active', () => { + const result = sanitizeTenantMutation(UNSCOPED, { $set: { tenantId: 'tenant-b' } }, 'guard'); + expect(result.update).toEqual({}); + }); + + it('leaves untouched payloads by reference', () => { + const update = { $set: { name: 'x' } }; + const result = sanitizeTenantMutation(SCOPED, update, 'guard'); + expect(result.update).toBe(update); + expect(result.changed).toBe(false); + }); + + it('never mutates the caller payload', () => { + const update = { $set: { tenantId: 'tenant-a', name: 'x' }, $inc: { n: 1 } }; + sanitizeTenantMutation(SCOPED, update, 'guard'); + expect(update).toEqual({ $set: { tenantId: 'tenant-a', name: 'x' }, $inc: { n: 1 } }); + }); + + it('leaves a system-scoped payload alone so cross-tenant writes stay possible', () => { + const update = { $set: { tenantId: 'tenant-b', name: 'x' } }; + const result = sanitizeTenantMutation(SYSTEM, update, 'guard'); + expect(result.update).toBe(update); + expect(result.changed).toBe(false); + }); + + it('treats a missing payload as empty', () => { + expect(sanitizeTenantMutation(SCOPED, null, 'guard')).toEqual({ + update: {}, + emptied: false, + changed: false, + }); + }); +}); + +describe('sanitizeTenantMutation (strip mode)', () => { + it('strips a cross-tenant value instead of throwing', () => { + const result = sanitizeTenantMutation( + SCOPED, + { $set: { tenantId: 'tenant-b', name: 'x' } }, + 'strip', + ); + expect(result.update).toEqual({ $set: { name: 'x' } }); + }); + + it('strips every operator and the top level', () => { + const result = sanitizeTenantMutation( + SCOPED, + { + tenantId: 'tenant-b', + $set: { tenantId: 'tenant-b' }, + $setOnInsert: { tenantId: 'tenant-b' }, + $unset: { tenantId: '' }, + $rename: { tenantId: 'x' }, + }, + 'strip', + ); + expect(result.update).toEqual({}); + expect(result.emptied).toBe(true); + }); +}); + +describe('scopeReplacement', () => { + it('stamps the active tenant when the replacement omits it', () => { + const result = scopeReplacement(SCOPED, { name: 'x' }); + expect(result.replacement).toEqual({ name: 'x', tenantId: 'tenant-a' }); + expect(result.changed).toBe(true); + }); + + it('accepts a replacement naming the active tenant', () => { + const replacement = { name: 'x', tenantId: 'tenant-a' }; + const result = scopeReplacement(SCOPED, replacement); + expect(result.replacement).toBe(replacement); + expect(result.changed).toBe(false); + }); + + it('refuses a replacement naming another tenant', () => { + expect(() => scopeReplacement(SCOPED, { tenantId: 'tenant-b' })).toThrow( + '[TenantIsolation] Modifying tenantId via replacement is not allowed', + ); + }); + + it('refuses any asserted tenantId when no tenant is active', () => { + expect(() => scopeReplacement(UNSCOPED, { tenantId: 'tenant-b' })).toThrow( + TenantIsolationError, + ); + }); + + it('leaves system replacements alone', () => { + const replacement = { name: 'x', tenantId: 'tenant-b' }; + const result = scopeReplacement(SYSTEM, replacement); + expect(result.replacement).toBe(replacement); + expect(result.changed).toBe(false); + }); + + it('never mutates the caller replacement', () => { + const replacement = { name: 'x' }; + scopeReplacement(SCOPED, replacement); + expect(replacement).toEqual({ name: 'x' }); + }); +}); + +describe('stampTenantOnDocument', () => { + it('stamps the active tenant onto a new document', () => { + const document: Record = { name: 'x' }; + stampTenantOnDocument(SCOPED, document); + expect(document.tenantId).toBe('tenant-a'); + }); + + it('leaves a matching tenantId in place', () => { + const document: Record = { tenantId: 'tenant-a' }; + stampTenantOnDocument(SCOPED, document); + expect(document.tenantId).toBe('tenant-a'); + }); + + it('tolerates a mismatched tenantId when not strict', async () => { + await withStrict(false, () => { + const document: Record = { tenantId: 'tenant-b' }; + stampTenantOnDocument(SCOPED, document); + expect(document.tenantId).toBe('tenant-b'); + }); + }); + + it('refuses a mismatched tenantId in strict mode', async () => { + await withStrict(true, () => { + expect(() => stampTenantOnDocument(SCOPED, { tenantId: 'tenant-b' })).toThrow( + '[TenantIsolation] Document tenantId does not match current tenant context', + ); + }); + }); + + it('does not stamp system or unscoped writes', () => { + const systemDocument: Record = { name: 'x' }; + stampTenantOnDocument(SYSTEM, systemDocument); + expect(systemDocument.tenantId).toBeUndefined(); + + const unscopedDocument: Record = { name: 'x' }; + stampTenantOnDocument(UNSCOPED, unscopedDocument); + expect(unscopedDocument.tenantId).toBeUndefined(); + }); +}); diff --git a/packages/data-schemas/src/tenant/policy.ts b/packages/data-schemas/src/tenant/policy.ts new file mode 100644 index 0000000000..c693ec310b --- /dev/null +++ b/packages/data-schemas/src/tenant/policy.ts @@ -0,0 +1,244 @@ +import { getTenantId, SYSTEM_TENANT_ID } from '~/config/tenantContext'; +import logger from '~/config/winston'; + +/** + * Engine-neutral tenant-isolation policy. + * + * Every rule that decides what tenant scoping means lives here as a pure + * function over plain objects. Storage engines bind these rules to their own + * query surface (see `~/models/plugins/tenantIsolation` for the Mongoose + * binding); no rule in this module may reference an engine's types. + */ + +/** Resolved tenant scope for the current async context. */ +export type TenantScope = + | { readonly kind: 'scoped'; readonly tenantId: string } + | { readonly kind: 'system' } + | { readonly kind: 'unscoped' }; + +/** A document-shaped payload the policy may stamp or inspect. */ +export type TenantDocument = Record & { tenantId?: unknown }; + +/** An update payload expressed with MongoDB-style mutation operators. */ +export type TenantUpdate = Record; + +/** + * How a payload carrying `tenantId` is handled. + * + * - `guard` — a cross-tenant value throws; a matching value is stripped. + * - `strip` — every value is stripped silently, never throws. + */ +export type TenantMutationMode = 'guard' | 'strip'; + +export class TenantIsolationError extends Error { + constructor(message: string) { + super(message); + this.name = 'TenantIsolationError'; + } +} + +const SYSTEM_SCOPE: TenantScope = { kind: 'system' }; +const UNSCOPED: TenantScope = { kind: 'unscoped' }; + +const VALUE_OPERATORS = ['$set', '$setOnInsert'] as const; +const STRIP_OPERATORS = ['$unset', '$rename'] as const; + +let strictMode: boolean | undefined; + +/** True when `TENANT_ISOLATION_STRICT=true`, i.e. missing context fails closed. */ +export function isTenantIsolationStrict(): boolean { + return (strictMode ??= process.env.TENANT_ISOLATION_STRICT === 'true'); +} + +/** Resets the cached strict-mode flag. Exposed for test teardown only. */ +export function resetTenantStrictCache(): void { + strictMode = undefined; +} + +export function warnOnInvalidStrictSetting(): void { + const raw = process.env.TENANT_ISOLATION_STRICT; + if (raw && raw !== 'true' && raw !== 'false') { + logger.warn( + `[TenantIsolation] TENANT_ISOLATION_STRICT="${raw}" ` + + 'is not "true" or "false"; defaulting to non-strict mode.', + ); + } +} + +/** + * Reads the tenant scope without enforcing strict mode. + * Use for guards that run after a scoping check has already fired. + */ +export function currentTenantScope(): TenantScope { + const tenantId = getTenantId(); + if (!tenantId) { + return UNSCOPED; + } + if (tenantId === SYSTEM_TENANT_ID) { + return SYSTEM_SCOPE; + } + return { kind: 'scoped', tenantId }; +} + +/** + * Reads the tenant scope, failing closed when strict mode is on and no + * context is active. `operation` names the caller in the thrown message. + */ +export function resolveTenantScope(operation: string): TenantScope { + const scope = currentTenantScope(); + if (scope.kind === 'unscoped' && isTenantIsolationStrict()) { + throw new TenantIsolationError( + `[TenantIsolation] ${operation} attempted without tenant context in strict mode`, + ); + } + return scope; +} + +/** The filter fragment that restricts reads and writes to the active tenant. */ +export function tenantFilter(scope: TenantScope): { tenantId: string } | undefined { + return scope.kind === 'scoped' ? { tenantId: scope.tenantId } : undefined; +} + +/** Result of sanitizing an update payload against the active scope. */ +export interface TenantMutationResult { + /** The payload to send to the engine — the input itself when nothing changed. */ + readonly update: TenantUpdate; + /** True when sanitizing removed every remaining instruction. */ + readonly emptied: boolean; + /** True when the payload differs from the input and must be written back. */ + readonly changed: boolean; +} + +/** + * Removes caller-supplied `tenantId` from an update payload — top level and + * inside mutation operators. Application code never controls `tenantId`. + * + * `guard` mode leaves system-scoped payloads untouched: a deliberate + * cross-tenant operation is the one caller allowed to set `tenantId`. + * `strip` mode ignores the scope and always strips. + * + * Copy-on-write: the input is never mutated, and the same reference is + * returned when there was nothing to strip. + */ +export function sanitizeTenantMutation( + scope: TenantScope, + update: TenantUpdate | null | undefined, + mode: TenantMutationMode, +): TenantMutationResult { + if (!update) { + return { update: {}, emptied: false, changed: false }; + } + + if (mode === 'guard' && scope.kind === 'system') { + return { update, emptied: false, changed: false }; + } + + let next: TenantUpdate | null = null; + const draft = (): TenantUpdate => (next ??= { ...update }); + + for (const operator of VALUE_OPERATORS) { + const payload = update[operator] as Record | undefined; + if (!payload || !('tenantId' in payload)) { + continue; + } + assertSameTenant(scope, payload.tenantId, mode); + const { tenantId: _stripped, ...rest } = payload; + writeOperator(draft(), operator, rest); + } + + for (const operator of STRIP_OPERATORS) { + const payload = update[operator] as Record | undefined; + if (!payload || !('tenantId' in payload)) { + continue; + } + const { tenantId: _stripped, ...rest } = payload; + writeOperator(draft(), operator, rest); + } + + if ('tenantId' in update) { + assertSameTenant(scope, update.tenantId, mode); + delete draft().tenantId; + } + + const result = next ?? update; + return { + update: result, + emptied: Object.keys(result).length === 0, + changed: next != null, + }; +} + +function writeOperator( + target: TenantUpdate, + operator: string, + rest: Record, +): void { + if (Object.keys(rest).length === 0) { + delete target[operator]; + return; + } + target[operator] = rest; +} + +function assertSameTenant(scope: TenantScope, value: unknown, mode: TenantMutationMode): void { + if (mode === 'strip') { + return; + } + if (scope.kind === 'scoped' && value !== scope.tenantId) { + throw new TenantIsolationError( + '[TenantIsolation] Cross-tenant tenantId mutation is not allowed', + ); + } +} + +/** + * Validates a whole-document replacement and stamps the active tenant onto it. + * + * Copy-on-write, like `sanitizeTenantMutation`. A replacement that names any + * tenant other than the active one is refused — including when no context is + * active, where no `tenantId` may be asserted at all. + */ +export function scopeReplacement( + scope: TenantScope, + replacement: TenantDocument | null | undefined, +): { readonly replacement: TenantDocument | null; readonly changed: boolean } { + if (!replacement || scope.kind === 'system') { + return { replacement: replacement ?? null, changed: false }; + } + + const tenantId = scope.kind === 'scoped' ? scope.tenantId : undefined; + + if ('tenantId' in replacement && replacement.tenantId !== tenantId) { + throw new TenantIsolationError( + '[TenantIsolation] Modifying tenantId via replacement is not allowed', + ); + } + + if (tenantId && !('tenantId' in replacement)) { + return { replacement: { ...replacement, tenantId }, changed: true }; + } + + return { replacement, changed: false }; +} + +/** + * Stamps the active tenant onto a document being inserted. + * + * A document that already names a different tenant is refused in strict mode + * and passed through otherwise, preserving pre-tenancy backfill behaviour. + * Mutates in place — insert paths own the documents they hand us. + */ +export function stampTenantOnDocument(scope: TenantScope, document: TenantDocument): void { + if (scope.kind !== 'scoped') { + return; + } + if (!document.tenantId) { + document.tenantId = scope.tenantId; + return; + } + if (isTenantIsolationStrict() && document.tenantId !== scope.tenantId) { + throw new TenantIsolationError( + '[TenantIsolation] Document tenantId does not match current tenant context', + ); + } +} diff --git a/packages/data-schemas/src/utils/tenantBulkWrite.ts b/packages/data-schemas/src/utils/tenantBulkWrite.ts index e8e104ca72..a3ac63b662 100644 --- a/packages/data-schemas/src/utils/tenantBulkWrite.ts +++ b/packages/data-schemas/src/utils/tenantBulkWrite.ts @@ -1,17 +1,17 @@ import type { AnyBulkWriteOperation, Model, MongooseBulkWriteOptions } from 'mongoose'; import type { BulkWriteResult } from 'mongodb'; -import { getTenantId, SYSTEM_TENANT_ID } from '~/config/tenantContext'; +import type { TenantScope, TenantUpdate } from '~/tenant/policy'; +import { + resolveTenantScope, + resetTenantStrictCache, + sanitizeTenantMutation, + isTenantIsolationStrict, +} from '~/tenant/policy'; import logger from '~/config/winston'; -let _strictMode: boolean | undefined; - -function isStrict(): boolean { - return (_strictMode ??= process.env.TENANT_ISOLATION_STRICT === 'true'); -} - /** Resets the cached strict-mode flag. Exposed for test teardown only. */ export function _resetBulkWriteStrictCache(): void { - _strictMode = undefined; + resetTenantStrictCache(); } /** @@ -25,10 +25,10 @@ export function _resetBulkWriteStrictCache(): void { * 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. + * Unlike the query middleware, which throws on cross-tenant values, this wrapper + * strips silently (`strip` mode). 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): sanitize + inject into filters/documents. @@ -41,27 +41,18 @@ export async function tenantSafeBulkWrite( ops: AnyBulkWriteOperation[], options?: MongooseBulkWriteOptions, ): Promise { - const tenantId = getTenantId(); + const scope = resolveTenantScope(`bulkWrite on ${model.modelName}`); // 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); + const sanitized = ops + .map((op) => sanitizeBulkOp(op, scope)) + .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 sanitized.length > 0 ? model.bulkWrite(sanitized, options) : EMPTY_BULK_RESULT; - } + const prepared = + scope.kind === 'scoped' ? sanitized.map((op) => injectTenantId(op, scope.tenantId)) : sanitized; - if (tenantId === SYSTEM_TENANT_ID) { - return sanitized.length > 0 ? model.bulkWrite(sanitized, options) : EMPTY_BULK_RESULT; - } - - const injected = sanitized.map((op) => injectTenantId(op, tenantId)); - return injected.length > 0 ? model.bulkWrite(injected, options) : EMPTY_BULK_RESULT; + return prepared.length > 0 ? model.bulkWrite(prepared, options) : EMPTY_BULK_RESULT; } /** Returned when all ops are dropped after sanitization. Single shared instance. */ @@ -76,19 +67,20 @@ const EMPTY_BULK_RESULT = Object.freeze({ }) as unknown as BulkWriteResult; /** Strips tenantId from update documents. Returns null if the op becomes empty. */ -function sanitizeBulkOp(op: AnyBulkWriteOperation): AnyBulkWriteOperation | null { +function sanitizeBulkOp( + op: AnyBulkWriteOperation, + scope: TenantScope, +): 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 } }; + const result = sanitizeTenantMutation(scope, update as TenantUpdate, 'strip'); + return result.emptied ? null : { updateOne: { ...rest, update: result.update } }; } 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 } }; + const result = sanitizeTenantMutation(scope, update as TenantUpdate, 'strip'); + return result.emptied ? null : { updateMany: { ...rest, update: result.update } }; } return op; @@ -135,7 +127,7 @@ function injectTenantId(op: AnyBulkWriteOperation, tenantId: string): AnyBulkWri }; } - if (isStrict()) { + if (isTenantIsolationStrict()) { throw new Error( '[TenantIsolation] Unknown bulkWrite operation type in strict mode — refusing to pass through without tenant injection', ); @@ -145,36 +137,3 @@ 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; -}