feat: Warn when multitenancy omits contextToOrchestratorId - #11411
Conversation
A tenant-dependent driverFactory combined with the default global orchestrator id silently shares one tenant's database connection with every other tenant. getOrchestratorApi() returns the cached OrchestratorApi by orchestrator id without consulting the current request context, and the driver factory closure memoizes the driver resolved for whichever tenant queried first, so a second tenant ends up executing its query through the first tenant's connection. contextToOrchestratorId already prevents this and multitenancy.mdx documents it, but the dedicated "custom data source for each tenant" recipe and its shipped example configured tenant-specific contextToAppId and driverFactory while omitting it, so following the official recipe produced the unsafe configuration.
github-actions
Bot
added
the
javascript
|
Claude finished @ovr's task in 3m 5s —— View job Review complete — 1 high, 1 medium, 2 low. Core diagnosis is correct; the runtime warning's trigger condition is too broad.Full reviewTodos
Verification of the premiseThe PR body's mechanism checks out. The doc statement that Findings
Positives
Security / performanceNo security issue introduced — the change is diagnostic and documentary, and it reduces a cross-tenant data-exposure footgun. No performance concern: the check runs once at option initialization. Note that the remedy the docs now recommend does carry a real resource cost (one connection pool + queue + pre-agg cache per tenant), which is the basis for finding 1. Not doneI could not execute |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if ( | ||
| opts.contextToAppId && | ||
| !opts.contextToOrchestratorId && | ||
| this.isCustomDriverFactory() | ||
| ) { | ||
| this.core.logger('Multitenancy Without ContextToOrchestratorId', { | ||
| warning: ( | ||
| 'You are using multitenancy with a custom driverFactory but without ' + | ||
| 'configuring contextToOrchestratorId: ' + | ||
| 'https://cube.dev/docs/reference/configuration/config#context_to_orchestrator_id' | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
High — this will fire (and give harmful advice) for a large class of safe configurations.
The condition is contextToAppId && !contextToOrchestratorId && isCustomDriverFactory(). Since dbType was removed in v1.7.0, a custom driverFactory is now the normal way to configure a single database — including for tenants that share one physical DB and isolate via queryRewrite / securityContext filters. Those setups are correct today, and:
- they will now see a scary warning on every boot, and
- if they follow it, they fragment one orchestrator into N (connection pool + queue + pre-agg cache per tenant) and inherit the
preAggregationsSchemaclash problem documented inconfig.mdx— i.e. the warning pushes them toward a worse configuration.
The actual unsafe condition is "driverFactory result depends on securityContext", which isCustomDriverFactory() doesn't approximate. Two options:
- Narrow the check with a cheap heuristic on the user-supplied factory (available as
this.createOptions.driverFactory), e.g. only warn when its source mentionssecurityContext:Not airtight (a factory could read the context indirectly), but for a warning it removes the bulk of false positives.const userFactory = this.createOptions.driverFactory; const factoryUsesSecurityContext = typeof userFactory === 'function' && /securityContext/.test(userFactory.toString());
- If you'd rather keep it broad, at least make the message conditional in tone — "if your
driverFactoryselects a connection based onsecurityContext, you must also configurecontextToOrchestratorId; if it returns the same configuration for all tenants you can ignore this" — and mention that overridingcontextToOrchestratorIdalso requires overridingpreAggregationsSchema. As written the message reads as an unconditional misconfiguration.
| caching key for various in-memory structures like data model compilation | ||
| results, etc. | ||
|
|
||
| It does not key database connections, execution queues, or pre-aggregation table | ||
| caches — those use [`context_to_orchestrator_id`][self-orchestrator-id], which | ||
| you must also define if [`driver_factory`][self-driver-factory] selects a | ||
| connection based on the security context. | ||
|
|
There was a problem hiding this comment.
Medium — this correctly contradicts another docs page, which should be fixed in the same PR.
The new sentence ("It does not key database connections…") matches the implementation: drivers are memoized inside the closure created per orchestratorId in getOrchestratorApi (packages/cubejs-server-core/src/core/server.ts:572-620), never per app id.
But docs-mintlify/embedding/multitenancy.mdx:282-284 still says the opposite:
The App ID (the result of
contextToAppId) is used as a caching key for various in-memory structures like data model compilation results, connection pool.
That's exactly the misconception this PR is trying to kill, on the primary multitenancy page. Worth dropping "connection pool" from that sentence here so the two pages agree.
| Overriding it is **required** whenever [`driver_factory`][self-driver-factory] | ||
| selects a connection based on the security context, and the id must be derived | ||
| from the same tenant identifier. Setting | ||
| [`context_to_app_id`][self-opts-ctx-to-appid] alone is not enough: the app id keys | ||
| the data model compilation cache, while the driver is resolved once per data | ||
| source per *orchestrator* id. With the default single global orchestrator id, | ||
| every tenant reuses the connection resolved for whichever tenant queried first, | ||
| so one tenant receives another tenant's data. |
There was a problem hiding this comment.
Low — "required" is stated a bit more absolutely than the mechanism warrants.
Two edges where the blanket "required … whenever driver_factory selects a connection based on the security context" isn't quite the whole story:
- A
driver_factorythat branches onsecurityContextbut returns configurations that are equivalent for the tenants involved isn't broken; conversely a factory that ignoressecurityContextentirely is safe even in a heavily multitenant setup. The precise rule is "the connection returned must not vary within one orchestrator id". - This paragraph now sits immediately above the existing
pre_aggregations_schema<Warning>, which is good, but the causality reads backwards to a reader arriving from the recipe: they're told they must overridecontext_to_orchestrator_id, then told overriding it needs another override. Consider one sentence linking them explicitly ("because you're now creating one orchestrator per tenant, also overridepre_aggregations_schema— see below") so nobody applies half the change.
| test('must not warn without a custom driverFactory', () => { | ||
| process.env.CUBEJS_DB_TYPE = 'postgres'; | ||
|
|
||
| const warnings = buildCore({ | ||
| contextToAppId: tenantId, | ||
| }); | ||
|
|
||
| expect(warnings).toHaveLength(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Low — test gaps and env leakage.
process.env.CUBEJS_DB_TYPE = 'postgres'is set without restoring it. It's consistent with the rest of this file, so not blocking, but this is the lastdescribein the file and the assignment is what makes the "no custom driverFactory" path reachable — anafterEach(() => { delete process.env.CUBEJS_DB_TYPE; })inside this block would keep it self-contained.- Missing the case that matters most for false positives: a static
driverFactory(one that ignoressecurityContext) pluscontextToAppId. Today that warns; per my comment onOptsHandler.tsit arguably shouldn't. Whichever behaviour you settle on, pinning it with a test would document the intent. - Also uncovered:
driverFactorypresent but nocontextToAppId(single-tenant) — currently silent, which is intended but untested.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #11411 +/- ##
===========================================
- Coverage 83.92% 59.02% -24.91%
===========================================
Files 257 223 -34
Lines 80755 17894 -62861
Branches 0 3638 +3638
===========================================
- Hits 67777 10562 -57215
+ Misses 12978 6815 -6163
- Partials 0 517 +517
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
A tenant-dependent driverFactory combined with the default global orchestrator id silently shares one tenant's database connection with every other tenant. getOrchestratorApi() returns the cached OrchestratorApi by orchestrator id without consulting the current request context, and the driver factory closure memoizes the driver resolved for whichever tenant queried first, so a second tenant ends up executing its query through the first tenant's connection.
contextToOrchestratorId already prevents this and multitenancy.mdx documents it, but the dedicated "custom data source for each tenant" recipe and its shipped example configured tenant-specific contextToAppId and driverFactory while omitting it, so following the official recipe produced the unsafe configuration.