From 983a33fbad1866a12c72e4efb7d212238aa06652 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 1 Jun 2026 18:00:53 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=9B=EF=B8=8F=20refactor:=20Scope=20App?= =?UTF-8?q?-Config=20Override=20Cache=20by=20Isolation=20Context=20(#13455?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getAppConfig caches per-principal merged config overrides under a key built by overrideCacheKey(role, userId, tenantId). The key used the tenantId *argument* only — but callers that go through the tenant middleware (the common path) pass no explicit tenantId and rely on the AsyncLocalStorage tenant context. Those calls were keyed under the shared '__default__' bucket, so the DB query (correctly scoped to the ALS tenant by the Mongoose plugin) produced a merged config that was then cached and served to the next tenant resolving the same role/user — leaking model specs, endpoints, and interface flags across tenants. Fall back to getTenantId() before '__default__' so the cache key reflects the actual tenant scope (param or ALS). Tighten the strict-mode warning to fire only when there is genuinely no tenant anywhere (param nor ALS), since the ALS case is now scoped rather than defaulted. No-op for single-tenant deployments, where getTenantId() is undefined and the key stays '__default__'. Adds tests (real Map-backed cache) proving the ALS tenant scopes the key and that two tenants resolving the same role each get their own config with no cache collision. --- packages/api/src/app/service.spec.ts | 45 ++++++++++++++++++++++++++++ packages/api/src/app/service.ts | 17 ++++++----- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/packages/api/src/app/service.spec.ts b/packages/api/src/app/service.spec.ts index b692aa6eb2..248d42dacd 100644 --- a/packages/api/src/app/service.spec.ts +++ b/packages/api/src/app/service.spec.ts @@ -327,6 +327,51 @@ describe('createAppConfigService', () => { expect(deps.getApplicableConfigs).toHaveBeenCalledWith([]); }); + + it('scopes the override cache key to the ALS tenant when no tenantId param is given', async () => { + const { tenantStorage } = jest.requireActual('@librechat/data-schemas'); + const deps = createDeps({ + getApplicableConfigs: jest + .fn() + .mockResolvedValue([{ priority: 10, overrides: { x: 1 }, isActive: true }]), + }); + const { getAppConfig } = createAppConfigService(deps); + + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + getAppConfig({ role: 'USER' }), + ); + + const overrideKey = [...deps._cache._store.keys()].find((k: string) => + k.includes('_OVERRIDE_:'), + ); + expect(overrideKey).toBe('app_config:_OVERRIDE_:tenant-a:USER'); + expect(overrideKey).not.toContain('__default__'); + }); + + it('does not serve one tenant a cached config built for another tenant', async () => { + const { tenantStorage, getTenantId } = jest.requireActual('@librechat/data-schemas'); + // Each tenant's DB overrides carry a marker derived from the active ALS tenant. + const deps = createDeps({ + getApplicableConfigs: jest + .fn() + .mockImplementation(async () => [ + { priority: 10, overrides: { whoami: getTenantId() }, isActive: true }, + ]), + }); + const { getAppConfig } = createAppConfigService(deps); + + const configA = (await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + getAppConfig({ role: 'USER' }), + )) as TestConfig & { whoami?: string }; + const configB = (await tenantStorage.run({ tenantId: 'tenant-b' }, async () => + getAppConfig({ role: 'USER' }), + )) as TestConfig & { whoami?: string }; + + expect(configA.whoami).toBe('tenant-a'); + expect(configB.whoami).toBe('tenant-b'); + // A cache collision would short-circuit the second tenant's DB read. + expect(deps.getApplicableConfigs).toHaveBeenCalledTimes(2); + }); }); it('does not cache on buildPrincipals error — retries on next request', async () => { diff --git a/packages/api/src/app/service.ts b/packages/api/src/app/service.ts index 613c428fca..658056503f 100644 --- a/packages/api/src/app/service.ts +++ b/packages/api/src/app/service.ts @@ -73,7 +73,10 @@ export function _resetOverrideStrictCache(): void { } function overrideCacheKey(role?: string, userId?: string, tenantId?: string): string { - const tenant = tenantId || '__default__'; + // Fall back to the ALS tenant context before `__default__`: callers that rely on the + // tenant middleware (the common path) pass no explicit tenantId, so without this the + // entry is keyed under the shared `__default__` bucket and leaks across tenants. + const tenant = tenantId || getTenantId() || '__default__'; if (userId && role) { return `_OVERRIDE_:${tenant}:${role}:${userId}`; } @@ -174,16 +177,16 @@ export function createAppConfigService(deps: AppConfigServiceDeps) { return baseConfig; } - // Strict-isolation + no tenant (param or ALS) = pathological path (middleware bypass or - // unauthenticated startup). Pre-tenant calls use baseOnly:true; admin calls carry tenantId. - // If ALS has a tenant, Mongoose scopes queries to that tenant's overrides — must fall through. - // Not cached: the cache key doesn't include ALS context, so a cached __default__ entry would - // be served to later ALS-scoped calls that share the same param-derived key. + // Strict isolation + no tenant anywhere (neither param nor ALS) is pathological: a + // middleware bypass or an unauthenticated startup call. Pre-tenant calls should use + // baseOnly:true and admin calls carry an explicit tenantId. Return the base config + // without caching it under the shared `__default__` bucket. When ALS has a tenant, + // overrideCacheKey scopes the key to it, so we fall through and cache per-tenant. if (principals.length === 0 && !tenantId && !getTenantId() && isStrictOverrideMode()) { return baseConfig; } - if (!tenantId && isStrictOverrideMode() && !_warnedNoTenantInStrictMode) { + if (!tenantId && !getTenantId() && isStrictOverrideMode() && !_warnedNoTenantInStrictMode) { _warnedNoTenantInStrictMode = true; logger.warn( '[getAppConfig] No tenantId in strict mode — falling back to __default__. ' +