🎛️ refactor: Scope App-Config Override Cache by Isolation Context - #13455
Conversation
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.
There was a problem hiding this comment.
Pull request overview
Fixes a multi-tenant cache leak in getAppConfig: the per-principal override cache key only considered the explicit tenantId argument, so tenant-scoped requests (which rely on the ALS tenant context) all collapsed to a shared __default__ bucket, allowing tenant A's merged config to be served to tenant B. The fix falls back to the ALS tenant via getTenantId() before __default__, aligning the cache key with the DB scoping already applied by the Mongoose tenant plugin.
Changes:
overrideCacheKeynow usestenantId || getTenantId() || '__default__'so ALS-scoped calls get tenant-specific cache keys.- Strict-mode "no tenantId" warning is tightened to fire only when neither the argument nor ALS provides a tenant.
- Adds two tests covering ALS-keyed cache scoping and the cross-tenant isolation guarantee.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/api/src/app/service.ts | Cache key falls back to ALS tenant; strict-mode warning gated on missing ALS tenant too; comment updated. |
| packages/api/src/app/service.spec.ts | Adds two tests verifying ALS-scoped cache key and no cross-tenant config bleed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
GitNexus: 🚀 deployedThe |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…breChat-AI#13455) 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.
…breChat-AI#13455) 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.
Summary
getAppConfigcaches per-principal merged config overrides (model specs, endpoints, interface flags) under a key produced byoverrideCacheKey(role, userId, tenantId):The key derives the tenant from the
tenantIdargument only. But callers that go through the tenant-context middleware — the common request path — pass no explicittenantIdand rely on theAsyncLocalStoragetenant context. Those calls are keyed under the shared__default__bucket:tenantIdarg, ALS = A) →getApplicableConfigsruns under A's context (the Mongoose plugin scopes the DB query to A), the merged result is cached under_OVERRIDE_:__default__:USER.tenantIdarg, ALS = B) → same__default__key → cache hit → tenant A's merged config is served to tenant B.So the cache key disagreed with the DB scoping, leaking config across tenants. (This affects strict mode too whenever principals are non-empty — the existing empty-principals early-return doesn't cover the role/user case.)
Fix
Fall back to
getTenantId()(the ALS tenant) before__default__, so the cache key reflects the actual scope the DB query already uses:getTenantId()is undefined for single-tenant deployments (no ALS context, noDEFAULT_TENANT_ID), so the key stays__default__and behavior is unchanged there. The strict-mode "no tenantId" warning is also tightened to fire only when there's genuinely no tenant anywhere (neither argument nor ALS), since the ALS case is now scoped rather than defaulted.Tests
Adds two tests to the existing
service.spec.ts(realMap-backed cache):_OVERRIDE_:tenant-a:USER, never__default__) when notenantIdargument is passed;getApplicableConfigscalled once per tenant).Verified both fail without the fix and pass with it. The existing 27
service.spec.tstests are unchanged and green.