fix(gateway): refuse customized index in multi-tenant mode (PR #1725 blocker)

Cross-tenant cache leak — `frontend_html_cache` is a single
`Arc<RwLock<Option<FrontendHtmlCache>>>` per `GatewayState` with no
user dimension, and `build_frontend_html` reads `state.workspace`
directly. In multi-tenant deployments
(`resolve_workspace(&state, &user)` driven by `workspace_pool`) this
is unsafe in two compounding ways:

1. **Latent**: even without the cache, `build_frontend_html` reading
   `state.workspace` ignores the per-user pool entirely. If the
   single-user fallback workspace is also populated, every user sees
   that one global workspace's `layout.json` / widgets — one
   operator's branding, hidden tabs, and registered widgets leak to
   every other tenant on the same gateway.

2. **Cache pin**: even if (1) were fixed, the cache key is just
   `(.system/gateway/layout.json mtime, .system/gateway/widgets/
   mtime)` against the global workspace — there is no `user_id` in
   the key. Once the slot is populated, every subsequent `GET /` hits
   the same HTML.

Root cause: the customization assembly path is fundamentally
single-tenant. `index_handler` (`GET /`) is the unauthenticated
bootstrap route — no user identity is available at request time, so
there is no way to resolve the *correct* per-user workspace inside
`build_frontend_html`. The reviewer flagged this as a cache bug; it's
actually an architectural mismatch that the cache makes visible.

**Fix:** in multi-tenant mode (`workspace_pool` set),
`build_frontend_html` short-circuits with `return None` BEFORE
reading `state.workspace` and BEFORE the cache write at the bottom of
the function. The embedded default `INDEX_HTML` is then served to
every user, the static CSP layer applies unchanged (no inline
scripts, no nonce needed), and the cache slot stays empty so it
cannot pin any leaked HTML.

This is the minimal fix that makes the gateway safe to ship in
multi-tenant mode. Per-user customization in multi-tenant deployments
will land in a follow-up PR via a JS-side `fetch('/api/frontend/layout')`
after auth — that endpoint already exists and already routes through
`resolve_workspace(&state, &user)`, so it returns the right workspace.
The layout-config IIFE in `crates/ironclaw_gateway/static/app.js`
already reads `window.__IRONCLAW_LAYOUT__`, which a future change can
populate from that fetch instead of from server-side HTML injection.

Documented the constraint in the doc comment on `build_frontend_html`
so future contributors understand WHY the early return is there
(hands-tied at the unauthenticated route, not laziness) and what the
correct path forward looks like.

Regression test:
`test_build_frontend_html_returns_none_in_multi_tenant_mode` (gated
on `feature = "libsql"` for the workspace backend). The test seeds a
*global* workspace with a hostile-looking layout
(`{"branding":{"title":"TENANT-LEAK-BAIT"}}`) AND a `WorkspacePool`,
attaches both to the GatewayState via `Arc::get_mut`, and asserts:

  1. `build_frontend_html` returns `None` — if it ever reads
     `state.workspace` again in multi-tenant mode, the bait title
     would land in the assembled HTML and this test would fail loudly
     with an actionable diagnostic.
  2. `state.frontend_html_cache` slot is still `None` after the call
     — the early return must short-circuit BEFORE the cache write at
     the bottom of the function, otherwise a poisoned entry would
     serve the leaked HTML to subsequent requests even after the bug
     is fixed.

Both contracts are independent — a future regression that breaks one
without the other is still caught.

Quality gate:
- `cargo fmt` clean
- `cargo clippy --no-default-features --features libsql --tests` zero
  warnings
- `cargo test --no-default-features --features libsql --lib channels::web`
  — 335 passed (was 334; +1 for the new multi-tenant guard test)
- `cargo test -p ironclaw_gateway` — 52 unit + 1 doctest passed

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Illia Polosukhin
2026-04-08 08:24:52 +00:00
parent 564611deb9
commit b9da40e7ff

View File

@@ -1057,7 +1057,32 @@ async fn compute_frontend_cache_key(workspace: &crate::workspace::Workspace) ->
/// mtimes (computed with a single `list()` call). A cache hit skips reading
/// every widget manifest / JS / CSS file, which would otherwise fire on every
/// page load.
///
/// **Multi-tenant safety.** In multi-user mode (`workspace_pool` set) this
/// function ALWAYS returns `None`, regardless of whether `state.workspace` is
/// also populated. The customization assembly path is fundamentally
/// single-tenant: `index_handler` (`GET /`) is the unauthenticated bootstrap
/// route — no user identity is available at request time, so there is no way
/// to resolve the *correct* per-user workspace inside this function. Reading
/// `state.workspace` instead would expose one global workspace's
/// customizations to every user, and the process-wide
/// `frontend_html_cache` would pin the leak across requests. We refuse the
/// path entirely and serve the embedded default to all users; per-user
/// customization can ride a future JS-side fetch against
/// `/api/frontend/layout`, which is authenticated and routes through
/// `resolve_workspace(&state, &user)` so it returns the right workspace.
/// See `crates/ironclaw_gateway/static/app.js` — the layout-config IIFE
/// already reads `window.__IRONCLAW_LAYOUT__`, which a future change can
/// populate from a `fetch('/api/frontend/layout')` after auth.
async fn build_frontend_html(state: &GatewayState) -> Option<String> {
if state.workspace_pool.is_some() {
// Multi-tenant: refuse the assembly path entirely. See the function
// doc comment above for the full rationale. The cache write below
// is unreachable on this branch, so the cache stays empty and
// cannot leak one user's customizations to another.
return None;
}
let ws = state.workspace.as_ref()?;
// Fast path — cache hit. One workspace `list()` call, no file reads.
@@ -4708,6 +4733,85 @@ mod tests {
assert_eq!(resp.status(), StatusCode::OK);
}
/// Multi-tenant cache safety: when `workspace_pool` is set,
/// `build_frontend_html` must refuse the assembly path entirely and
/// return `None` regardless of what `state.workspace` contains.
///
/// Background: `index_handler` (`GET /`) is the unauthenticated
/// bootstrap route, so it has no user identity at request time.
/// Reading `state.workspace` in multi-tenant mode would expose one
/// global workspace's customizations to every user, and the
/// process-wide `frontend_html_cache` would pin the leak across
/// requests. The bait here is a global workspace seeded with a
/// hostile-looking layout — if the function ever stops short-
/// circuiting on `workspace_pool.is_some()`, that layout would land
/// in the assembled HTML and this test would fail loudly.
#[cfg(feature = "libsql")]
#[tokio::test]
async fn test_build_frontend_html_returns_none_in_multi_tenant_mode() {
use crate::config::{WorkspaceConfig, WorkspaceSearchConfig};
use crate::db::Database as _;
use crate::db::libsql::LibSqlBackend;
use crate::workspace::EmbeddingCacheConfig;
let dir = tempfile::tempdir().expect("tempdir");
let backend = LibSqlBackend::new_local(&dir.path().join("multi_tenant_index.db"))
.await
.expect("backend");
backend.run_migrations().await.expect("migrations");
let db: Arc<dyn Database> = Arc::new(backend);
// Bait: a *global* workspace with customizations. If
// build_frontend_html ever read state.workspace in multi-tenant
// mode, the title "TENANT-LEAK-BAIT" would appear in the
// assembled HTML for every user. The assertions below pin the
// refusal contract — both the return value AND the cache slot.
let global_ws = Arc::new(Workspace::new_with_db("tenant-leak-bait", Arc::clone(&db)));
global_ws
.write(
".system/gateway/layout.json",
r#"{"branding":{"title":"TENANT-LEAK-BAIT"}}"#,
)
.await
.expect("seed bait layout");
let pool = Arc::new(WorkspacePool::new(
Arc::clone(&db),
None,
EmbeddingCacheConfig::default(),
WorkspaceSearchConfig::default(),
WorkspaceConfig::default(),
));
// Build state via the standard test helper, then mutate the
// workspace + workspace_pool fields. `Arc::get_mut` succeeds here
// because no other strong reference exists yet — the helper just
// returned the freshly-constructed Arc.
let mut state = test_gateway_state(None);
let state_mut = Arc::get_mut(&mut state).expect("test state must be uniquely owned");
state_mut.workspace = Some(global_ws);
state_mut.workspace_pool = Some(pool);
// Contract 1: build_frontend_html refuses to assemble.
let html = build_frontend_html(&state).await;
assert!(
html.is_none(),
"build_frontend_html must return None in multi-tenant mode \
(got Some HTML — bait layout may have leaked across tenants)"
);
// Contract 2: the cache slot is still empty. The early return
// above MUST short-circuit before the cache write at the bottom
// of the function — otherwise a poisoned cache entry would serve
// the leaked HTML to subsequent requests even after the bug is
// fixed.
let cache = state.frontend_html_cache.read().await;
assert!(
cache.is_none(),
"frontend_html_cache must remain empty in multi-tenant mode"
);
}
#[tokio::test]
async fn test_oauth_callback_missing_params() {
use axum::body::Body;