From f49f3683555de0f65a53918d9feb37ca4d0eeecb Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sat, 28 Mar 2026 06:46:45 -0700 Subject: [PATCH] Clean up extension credentials on uninstall (#1718) * Clean up extension credentials on uninstall * Address PR review feedback * Cover channel webhook secrets on uninstall * Harden tool secret cleanup detection --- src/extensions/manager.rs | 650 +++++++++++++++++- tests/e2e/CLAUDE.md | 2 + tests/e2e/conftest.py | 109 +++ .../test_extension_uninstall_cleanup.py | 266 +++++++ 4 files changed, 1026 insertions(+), 1 deletion(-) create mode 100644 tests/e2e/scenarios/test_extension_uninstall_cleanup.py diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index 8c0534f2..82dc5471 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -53,6 +53,38 @@ struct HostedOAuthFlowStart { flow: crate::cli::oauth_defaults::PendingOAuthFlow, } +#[derive(Debug, Default)] +struct SecretCleanupPlan { + base_secrets: HashSet, + companion_secrets: HashMap>, +} + +impl SecretCleanupPlan { + fn add_base_secret(&mut self, secret_name: impl AsRef) { + self.base_secrets + .insert(secret_name.as_ref().to_lowercase()); + } + + fn add_companion_secret( + &mut self, + base_secret_name: impl AsRef, + companion_secret_name: impl AsRef, + ) { + self.companion_secrets + .entry(base_secret_name.as_ref().to_lowercase()) + .or_default() + .insert(companion_secret_name.as_ref().to_lowercase()); + } +} + +fn oauth_refresh_secret_name(secret_name: &str) -> String { + format!("{}_refresh_token", secret_name.to_lowercase()) +} + +fn oauth_scopes_secret_name(secret_name: &str) -> String { + format!("{}_scopes", secret_name.to_lowercase()) +} + fn normalize_oauth_callback_path(path: &str) -> String { let trimmed_path = path.trim_end_matches('/'); if trimmed_path.is_empty() { @@ -1602,6 +1634,10 @@ impl ExtensionManager { match kind { ExtensionKind::McpServer => { + let cleanup_plan = self + .collect_secret_cleanup_plan(name, kind, user_id) + .await?; + // Unregister tools with this server's prefix let tool_names: Vec = self .tool_registry @@ -1623,6 +1659,9 @@ impl ExtensionManager { .await .map_err(|e| ExtensionError::Config(e.to_string()))?; + self.cleanup_uninstalled_extension_secrets(cleanup_plan, user_id) + .await; + Ok(format!( "Removed MCP server '{}' and {} tool(s)", name, @@ -1630,6 +1669,10 @@ impl ExtensionManager { )) } ExtensionKind::WasmTool => { + let cleanup_plan = self + .collect_secret_cleanup_plan(name, kind, user_id) + .await?; + // Unregister from tool registry self.tool_registry.unregister(name).await; @@ -1674,9 +1717,16 @@ impl ExtensionManager { let _ = tokio::fs::remove_file(&cap_path).await; } + self.cleanup_uninstalled_extension_secrets(cleanup_plan, user_id) + .await; + Ok(format!("Removed WASM tool '{}'", name)) } ExtensionKind::WasmChannel => { + let cleanup_plan = self + .collect_secret_cleanup_plan(name, kind, user_id) + .await?; + // Remove from active set and persist self.active_channel_names.write().await.remove(name); self.persist_active_channels(user_id).await; @@ -1702,6 +1752,9 @@ impl ExtensionManager { let _ = tokio::fs::remove_file(&cap_path).await; } + self.cleanup_uninstalled_extension_secrets(cleanup_plan, user_id) + .await; + Ok(format!( "Removed channel '{}'. Restart IronClaw for the change to take effect.", name @@ -2999,6 +3052,258 @@ impl ExtensionManager { crate::tools::wasm::CapabilitiesFile::from_bytes(&cap_bytes).ok() } + async fn load_channel_capabilities( + &self, + name: &str, + ) -> Option { + let cap_path = self + .wasm_channels_dir + .join(format!("{}.capabilities.json", name)); + let cap_bytes = tokio::fs::read(&cap_path).await.ok()?; + crate::channels::wasm::ChannelCapabilitiesFile::from_bytes(&cap_bytes).ok() + } + + async fn collect_secret_cleanup_plan( + &self, + name: &str, + kind: ExtensionKind, + user_id: &str, + ) -> Result { + let mut plan = SecretCleanupPlan::default(); + + match kind { + ExtensionKind::WasmTool => { + if let Some(cap) = self.load_tool_capabilities(name).await { + for secret_name in Self::tool_secret_names(&cap) { + plan.add_base_secret(secret_name); + } + + if let Some(auth) = cap.auth { + plan.add_base_secret(&auth.secret_name); + plan.add_companion_secret( + &auth.secret_name, + oauth_refresh_secret_name(&auth.secret_name), + ); + plan.add_companion_secret( + &auth.secret_name, + oauth_scopes_secret_name(&auth.secret_name), + ); + } + } + } + ExtensionKind::WasmChannel => { + if let Some(cap) = self.load_channel_capabilities(name).await { + for secret_name in Self::channel_secret_names(&cap) { + plan.add_base_secret(secret_name); + } + } + } + ExtensionKind::McpServer => { + let server = self + .get_mcp_server(name, user_id) + .await + .map_err(|e| ExtensionError::Config(e.to_string()))?; + let token_secret_name = server.token_secret_name(); + plan.add_base_secret(&token_secret_name); + plan.add_base_secret(server.client_id_secret_name()); + // MCP OAuth can persist companion secrets through two paths: + // the MCP auth helper uses `mcp__refresh_token`, while the + // hosted gateway callback stores companions alongside the access + // token secret (`_refresh_token` / `_scopes`). + plan.add_companion_secret(&token_secret_name, server.refresh_token_secret_name()); + plan.add_companion_secret( + &token_secret_name, + oauth_refresh_secret_name(&token_secret_name), + ); + plan.add_companion_secret( + &token_secret_name, + oauth_scopes_secret_name(&token_secret_name), + ); + } + ExtensionKind::ChannelRelay => {} + } + + Ok(plan) + } + + async fn cleanup_uninstalled_extension_secrets(&self, plan: SecretCleanupPlan, user_id: &str) { + let referenced_secrets = match self.collect_referenced_secret_names(user_id).await { + Ok(secret_names) => secret_names, + Err(error) => { + tracing::warn!( + user_id, + error, + "Failed to determine which secrets are still referenced; keeping secrets" + ); + return; + } + }; + + for base_secret in &plan.base_secrets { + if referenced_secrets.contains(base_secret) { + continue; + } + + self.delete_secret_best_effort(user_id, base_secret).await; + + if let Some(companion_secrets) = plan.companion_secrets.get(base_secret) { + for companion_secret in companion_secrets { + if !referenced_secrets.contains(companion_secret) { + self.delete_secret_best_effort(user_id, companion_secret) + .await; + } + } + } + } + } + + async fn delete_secret_best_effort(&self, user_id: &str, secret_name: &str) { + if let Err(error) = self.secrets.delete(user_id, secret_name).await { + tracing::warn!( + user_id, + secret_name, + error = %error, + "Failed to delete secret while uninstalling extension" + ); + } + } + + async fn collect_referenced_secret_names( + &self, + user_id: &str, + ) -> Result, String> { + let mut referenced_secret_names = HashSet::new(); + + let tools = discover_tools(&self.wasm_tools_dir) + .await + .map_err(|e| format!("discover tools: {e}"))?; + for (tool_name, discovered_tool) in &tools { + let cap = self + .load_tool_capabilities(tool_name) + .await + .ok_or_else(|| { + let path = discovered_tool + .capabilities_path + .as_ref() + .map(|path| path.display().to_string()) + .unwrap_or_else(|| format!("{} (missing)", tool_name)); + format!("load tool capabilities for {tool_name}: {path}") + })?; + referenced_secret_names.extend(Self::tool_secret_names(&cap)); + } + + let channels = crate::channels::wasm::discover_channels(&self.wasm_channels_dir) + .await + .map_err(|e| format!("discover channels: {e}"))?; + for (channel_name, discovered_channel) in &channels { + let cap = self + .load_channel_capabilities(channel_name) + .await + .ok_or_else(|| { + let path = discovered_channel + .capabilities_path + .as_ref() + .map(|path| path.display().to_string()) + .unwrap_or_else(|| format!("{} (missing)", channel_name)); + format!("load channel capabilities for {channel_name}: {path}") + })?; + referenced_secret_names.extend(Self::channel_secret_names(&cap)); + } + + let mcp_servers = self + .load_mcp_servers(user_id) + .await + .map_err(|e| format!("load MCP servers: {e}"))?; + for server in &mcp_servers.servers { + referenced_secret_names.extend(Self::mcp_server_secret_names(server)); + } + + Ok(referenced_secret_names) + } + + fn tool_secret_names(cap: &crate::tools::wasm::CapabilitiesFile) -> HashSet { + let mut names = HashSet::new(); + + if let Some(auth) = &cap.auth { + names.insert(auth.secret_name.to_lowercase()); + } + if let Some(setup) = &cap.setup { + names.extend( + setup + .required_secrets + .iter() + .map(|secret| secret.name.to_lowercase()), + ); + } + if let Some(http) = &cap.http { + names.extend( + http.credentials + .values() + .map(|credential| credential.secret_name.to_lowercase()), + ); + } + if let Some(webhook) = &cap.webhook { + if let Some(secret_name) = &webhook.secret_name { + names.insert(secret_name.to_lowercase()); + } + if let Some(secret_name) = &webhook.signature_key_secret_name { + names.insert(secret_name.to_lowercase()); + } + if let Some(secret_name) = &webhook.hmac_secret_name { + names.insert(secret_name.to_lowercase()); + } + } + + names + } + + fn channel_secret_names( + cap: &crate::channels::wasm::ChannelCapabilitiesFile, + ) -> HashSet { + let mut names: HashSet = cap + .setup + .required_secrets + .iter() + .map(|secret| secret.name.to_lowercase()) + .collect(); + + if let Some(http) = cap.capabilities.tool.http.as_ref() { + names.extend( + http.credentials + .values() + .map(|credential| credential.secret_name.to_lowercase()), + ); + } + + if let Some(webhook) = cap + .capabilities + .channel + .as_ref() + .and_then(|channel| channel.webhook.as_ref()) + { + if webhook.secret_header.is_some() || webhook.secret_name.is_some() { + names.insert(cap.webhook_secret_name().to_lowercase()); + } + if let Some(secret_name) = cap.signature_key_secret_name() { + names.insert(secret_name.to_lowercase()); + } + if let Some(secret_name) = cap.hmac_secret_name() { + names.insert(secret_name.to_lowercase()); + } + } + + names + } + + fn mcp_server_secret_names(server: &McpServerConfig) -> HashSet { + [ + server.token_secret_name().to_lowercase(), + server.client_id_secret_name().to_lowercase(), + ] + .into_iter() + .collect() + } + /// Collect merged OAuth scopes from all installed tools sharing the same secret_name. /// /// When multiple tools share an OAuth provider (e.g., google-calendar and google-drive @@ -6033,6 +6338,8 @@ mod tests { ExtensionError, ExtensionKind, ExtensionSource, InstallResult, VerificationChallenge, }; use crate::pairing::PairingStore; + use crate::secrets::CreateSecretParams; + use crate::tools::mcp::McpServerConfig; fn require(condition: bool, message: impl Into) -> Result<(), String> { if condition { @@ -6353,6 +6660,38 @@ mod tests { tools_dir } + fn write_test_channel( + dir: &std::path::Path, + name: &str, + capabilities_json: &str, + ) -> std::path::PathBuf { + let channels_dir = dir.join("channels"); + std::fs::create_dir_all(&channels_dir).expect("channels dir"); + std::fs::write( + channels_dir.join(format!("{name}.wasm")), + b"not-a-real-wasm", + ) + .expect("wasm"); + std::fs::write( + channels_dir.join(format!("{name}.capabilities.json")), + capabilities_json, + ) + .expect("capabilities"); + channels_dir + } + + async fn store_test_secret( + manager: &crate::extensions::manager::ExtensionManager, + name: &str, + value: &str, + ) { + manager + .secrets + .create("test", CreateSecretParams::new(name, value)) + .await + .expect("store secret"); + } + #[test] fn test_setting_value_is_present() { assert!( @@ -7423,7 +7762,13 @@ mod tests { // Regression: remove() only checked channel_runtime for shutdown, missing // relay-only mode where only relay_channel_manager is set. let dir = tempfile::tempdir().expect("temp dir"); - let mgr = make_test_manager(None, dir.path().to_path_buf()); + let (store, _db_dir) = make_test_store().await; + let mgr = make_test_manager_with_dirs( + None, + dir.path().join("tools"), + dir.path().join("channels"), + Some(store), + ); // Set up relay channel manager with a stub channel let cm = Arc::new(crate::channels::ChannelManager::new()); @@ -7450,6 +7795,8 @@ mod tests { .await .expect("store team_id"); } + store_test_secret(&mgr, "relay:slack-relay:oauth_state", "nonce").await; + store_test_secret(&mgr, "relay:slack-relay:stream_token", "legacy-token").await; // Verify channel exists before removal assert!(cm.get_channel("slack-relay").await.is_some()); @@ -7478,6 +7825,30 @@ mod tests { cm.get_channel("slack-relay").await.is_none(), "relay channel should be removed from the channel manager" ); + assert!( + !mgr.secrets + .exists("test", "relay:slack-relay:oauth_state") + .await + .expect("oauth state exists query"), + "relay oauth_state secret should be removed" + ); + assert!( + !mgr.secrets + .exists("test", "relay:slack-relay:stream_token") + .await + .expect("stream token exists query"), + "relay legacy stream token should be removed" + ); + assert_eq!( + mgr.store + .as_ref() + .expect("store") + .get_setting("test", "relay:slack-relay:team_id") + .await + .expect("team_id query"), + None, + "relay team_id setting should be removed" + ); } #[tokio::test] @@ -7585,6 +7956,185 @@ mod tests { ); } + #[tokio::test] + async fn test_remove_wasm_tool_deletes_unique_secrets() { + let dir = tempfile::tempdir().expect("temp dir"); + let tools_dir = write_test_tool( + dir.path(), + "github", + r#"{ + "name": "github", + "auth": { "secret_name": "github_token" }, + "setup": { + "required_secrets": [ + { "name": "github_client_secret", "prompt": "GitHub client secret for testing cleanup behavior." } + ] + }, + "http": { + "credentials": { + "service_token": { + "secret_name": "github_service_token", + "location": { "type": "bearer" } + } + } + }, + "webhook": { + "hmac_secret_name": "github_webhook_secret" + } + }"#, + ); + let mgr = make_test_manager_with_dirs(None, tools_dir, dir.path().join("channels"), None); + + store_test_secret(&mgr, "github_token", "access-token").await; + store_test_secret(&mgr, "github_token_refresh_token", "refresh-token").await; + store_test_secret(&mgr, "github_token_scopes", "repo workflow").await; + store_test_secret(&mgr, "github_client_secret", "client-secret").await; + store_test_secret(&mgr, "github_service_token", "service-token").await; + store_test_secret(&mgr, "github_webhook_secret", "webhook-secret").await; + + mgr.remove("github", "test") + .await + .expect("remove should succeed"); + + for secret_name in [ + "github_token", + "github_token_refresh_token", + "github_token_scopes", + "github_client_secret", + "github_service_token", + "github_webhook_secret", + ] { + assert!( + !mgr.secrets + .exists("test", secret_name) + .await + .expect("exists query"), + "secret {secret_name} should be deleted" + ); + } + } + + #[tokio::test] + async fn test_remove_wasm_tool_keeps_secrets_when_other_tool_capabilities_missing() { + let dir = tempfile::tempdir().expect("temp dir"); + let tools_dir = write_test_tool( + dir.path(), + "github", + r#"{ + "name": "github", + "auth": { "secret_name": "shared_token" } + }"#, + ); + std::fs::write(tools_dir.join("broken.wasm"), b"fake-tool").expect("write tool"); + + let mgr = make_test_manager_with_dirs(None, tools_dir, dir.path().join("channels"), None); + store_test_secret(&mgr, "shared_token", "access-token").await; + store_test_secret(&mgr, "shared_token_refresh_token", "refresh-token").await; + store_test_secret(&mgr, "shared_token_scopes", "repo").await; + + mgr.remove("github", "test") + .await + .expect("remove should succeed"); + + for secret_name in [ + "shared_token", + "shared_token_refresh_token", + "shared_token_scopes", + ] { + assert!( + mgr.secrets + .exists("test", secret_name) + .await + .expect("exists query"), + "secret {secret_name} should be retained when reference detection is uncertain" + ); + } + } + + #[tokio::test] + async fn test_remove_wasm_tool_keeps_shared_secrets_until_last_extension() { + let dir = tempfile::tempdir().expect("temp dir"); + write_test_tool( + dir.path(), + "google-calendar", + r#"{ + "name": "google-calendar", + "auth": { "secret_name": "google_oauth_token" }, + "setup": { + "required_secrets": [ + { "name": "google_oauth_client_id", "prompt": "Google OAuth client id for cleanup testing." }, + { "name": "google_oauth_client_secret", "prompt": "Google OAuth client secret for cleanup testing." } + ] + } + }"#, + ); + let tools_dir = write_test_tool( + dir.path(), + "google-drive", + r#"{ + "name": "google-drive", + "auth": { "secret_name": "google_oauth_token" }, + "setup": { + "required_secrets": [ + { "name": "google_oauth_client_id", "prompt": "Google OAuth client id for cleanup testing." }, + { "name": "google_oauth_client_secret", "prompt": "Google OAuth client secret for cleanup testing." } + ] + } + }"#, + ); + let mgr = make_test_manager_with_dirs(None, tools_dir, dir.path().join("channels"), None); + + for (secret_name, value) in [ + ("google_oauth_token", "access-token"), + ("google_oauth_token_refresh_token", "refresh-token"), + ("google_oauth_token_scopes", "calendar drive"), + ("google_oauth_client_id", "client-id"), + ("google_oauth_client_secret", "client-secret"), + ] { + store_test_secret(&mgr, secret_name, value).await; + } + + mgr.remove("google-calendar", "test") + .await + .expect("first remove should succeed"); + + for secret_name in [ + "google_oauth_token", + "google_oauth_token_refresh_token", + "google_oauth_token_scopes", + "google_oauth_client_id", + "google_oauth_client_secret", + ] { + assert!( + mgr.secrets + .exists("test", secret_name) + .await + .expect("exists query"), + "shared secret {secret_name} should remain while google-drive is still installed" + ); + } + + mgr.remove("google-drive", "test") + .await + .expect("second remove should succeed"); + + for secret_name in [ + "google_oauth_token", + "google_oauth_token_refresh_token", + "google_oauth_token_scopes", + "google_oauth_client_id", + "google_oauth_client_secret", + ] { + assert!( + !mgr.secrets + .exists("test", secret_name) + .await + .expect("exists query"), + "shared secret {secret_name} should be deleted after the last tool is removed" + ); + } + } + #[tokio::test] async fn test_remove_wasm_channel_clears_activation_error_and_deletes_files() { let dir = tempfile::tempdir().expect("temp dir"); @@ -7619,6 +8169,104 @@ mod tests { ); } + #[tokio::test] + async fn test_remove_wasm_channel_deletes_setup_secrets() { + let dir = tempfile::tempdir().expect("temp dir"); + let channels_dir = write_test_channel( + dir.path(), + "telegram", + r#"{ + "type": "channel", + "name": "telegram", + "setup": { + "required_secrets": [ + { + "name": "telegram_bot_token", + "prompt": "Telegram bot token used to verify uninstall cleanup behavior." + } + ] + }, + "capabilities": { + "http": { + "credentials": { + "tenant_token": { + "secret_name": "telegram_service_token", + "location": { "type": "bearer" } + } + } + }, + "channel": { + "webhook": { + "secret_header": "X-Telegram-Bot-Api-Secret-Token", + "secret_name": "telegram_webhook_secret" + } + } + } + }"#, + ); + let mgr = make_test_manager_with_dirs(None, dir.path().join("tools"), channels_dir, None); + + store_test_secret(&mgr, "telegram_bot_token", "123:telegram-token").await; + store_test_secret(&mgr, "telegram_service_token", "tenant-service-token").await; + store_test_secret(&mgr, "telegram_webhook_secret", "webhook-secret").await; + + mgr.remove("telegram", "test") + .await + .expect("remove should succeed"); + + for secret_name in [ + "telegram_bot_token", + "telegram_service_token", + "telegram_webhook_secret", + ] { + assert!( + !mgr.secrets + .exists("test", secret_name) + .await + .expect("exists query"), + "channel secret {secret_name} should be deleted" + ); + } + } + + #[tokio::test] + async fn test_remove_mcp_server_deletes_stored_secrets() { + let dir = tempfile::tempdir().expect("temp dir"); + let (store, _db_dir) = make_test_store().await; + let mgr = make_test_manager_with_dirs( + None, + dir.path().join("tools"), + dir.path().join("channels"), + Some(Arc::clone(&store)), + ); + let server = McpServerConfig::new("notion", "https://example.com/mcp"); + mgr.add_mcp_server(server.clone(), "test") + .await + .expect("add mcp server"); + + store_test_secret(&mgr, &server.token_secret_name(), "access-token").await; + store_test_secret(&mgr, &server.refresh_token_secret_name(), "refresh-token").await; + store_test_secret(&mgr, &server.client_id_secret_name(), "client-id").await; + + mgr.remove("notion", "test") + .await + .expect("remove should succeed"); + + for secret_name in [ + server.token_secret_name(), + server.refresh_token_secret_name(), + server.client_id_secret_name(), + ] { + assert!( + !mgr.secrets + .exists("test", &secret_name) + .await + .expect("exists query"), + "MCP secret {secret_name} should be deleted" + ); + } + } + #[test] fn test_sanitize_url_with_query_params() { let url = "https://api.example.com/path?api_key=secret123&token=abc"; diff --git a/tests/e2e/CLAUDE.md b/tests/e2e/CLAUDE.md index 46b7b752..fedd873f 100644 --- a/tests/e2e/CLAUDE.md +++ b/tests/e2e/CLAUDE.md @@ -53,6 +53,7 @@ HEADED=1 pytest scenarios/ | `test_skills.py` | Skills tab UI visibility, ClawHub search (skipped if registry unreachable), install + remove lifecycle | | `test_sse_reconnect.py` | SSE reconnects after programmatic `eventSource.close()` + `connectSSE()`; history is reloaded after reconnect | | `test_tool_approval.py` | Approval card appears, buttons disable on approve/deny, parameters toggle via `page.evaluate("showApproval(...)")`; the waiting-approval regression uses a real HTTP tool call | +| `test_extension_uninstall_cleanup.py` | Real install/setup/remove coverage for WASM tools, WASM channels, OAuth-backed shared Google tools, and MCP servers; verifies uninstall deletes stored secrets from the libSQL `secrets` table while preserving shared credentials until the last referencing extension is removed | | `test_oauth_refresh.py` | Hosted Gmail OAuth regression: complete setup via `/oauth/callback`, expire the stored access token in libSQL, trigger a real `gmail` tool call through `/api/chat/send`, and verify refresh goes through the mock `/oauth/refresh` proxy without forwarding `client_secret` | ## `helpers.py` @@ -77,6 +78,7 @@ All fixtures are defined in `tests/e2e/conftest.py`. Running `pytest scenarios/` | `mock_llm_server` | Starts `mock_llm.py --port 0`, reads the assigned port from stdout, waits for `/v1/models` to return 200. Yields the base URL. | | `ironclaw_server` | Starts the ironclaw binary with a minimal env (see below), waits for `/api/health` (timeout 60s). Yields the base URL. On teardown sends **SIGINT** (not SIGTERM) so the tokio ctrl_c handler triggers a graceful shutdown and LLVM coverage data is flushed. | | `hosted_oauth_refresh_server` | Starts a second ironclaw instance with a dedicated libSQL DB and `GOOGLE_OAUTH_CLIENT_ID=hosted-google-client-id`, while still pointing `IRONCLAW_OAUTH_EXCHANGE_URL` at `mock_llm.py`. Yields a dict with `base_url`, `db_path`, `gateway_user_id`, and `mock_llm_url` for the hosted refresh regression scenario. | +| `extension_cleanup_server` | Starts an isolated ironclaw instance with its own temp DB/home/WASM dirs, `SECRETS_MASTER_KEY`, and hosted-style OAuth env so uninstall-cleanup scenarios can inspect the `secrets` table without interfering with the shared E2E server state. | | `browser` | Launches a single Chromium instance (headless by default; set `HEADED=1` for headed). Shared across all tests. | ### Function-scoped fixtures diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 9f4b02f3..7d223a47 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -443,6 +443,115 @@ async def hosted_oauth_refresh_server( home_tmpdir.cleanup() +@pytest.fixture(scope="session") +async def extension_cleanup_server( + ironclaw_binary, + mock_llm_server, +): + """Start an isolated ironclaw instance for uninstall secret cleanup E2E tests.""" + reserved = _reserve_loopback_sockets(2) + db_tmpdir = tempfile.TemporaryDirectory(prefix="ironclaw-e2e-cleanup-db-") + home_tmpdir = tempfile.TemporaryDirectory(prefix="ironclaw-e2e-cleanup-home-") + tools_tmpdir = tempfile.TemporaryDirectory(prefix="ironclaw-e2e-cleanup-tools-") + channels_tmpdir = tempfile.TemporaryDirectory(prefix="ironclaw-e2e-cleanup-channels-") + + try: + gateway_port = reserved[0].getsockname()[1] + http_port = reserved[1].getsockname()[1] + for sock in reserved: + if sock.fileno() != -1: + sock.close() + + db_path = os.path.join(db_tmpdir.name, "extension-cleanup.db") + home_dir = home_tmpdir.name + env = { + "PATH": os.environ.get("PATH", "/usr/bin:/bin"), + "HOME": home_dir, + "IRONCLAW_BASE_DIR": os.path.join(home_dir, ".ironclaw"), + "RUST_LOG": "ironclaw=info", + "RUST_BACKTRACE": "1", + "IRONCLAW_OWNER_ID": OWNER_SCOPE_ID, + "GATEWAY_ENABLED": "true", + "GATEWAY_HOST": "127.0.0.1", + "GATEWAY_PORT": str(gateway_port), + "GATEWAY_AUTH_TOKEN": AUTH_TOKEN, + "GATEWAY_USER_ID": OWNER_SCOPE_ID, + "HTTP_HOST": "127.0.0.1", + "HTTP_PORT": str(http_port), + "HTTP_WEBHOOK_SECRET": HTTP_WEBHOOK_SECRET, + "CLI_ENABLED": "false", + "LLM_BACKEND": "openai_compatible", + "LLM_BASE_URL": mock_llm_server, + "LLM_MODEL": "mock-model", + "DATABASE_BACKEND": "libsql", + "LIBSQL_PATH": db_path, + "SECRETS_MASTER_KEY": "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + "SANDBOX_ENABLED": "false", + "SKILLS_ENABLED": "true", + "ROUTINES_ENABLED": "true", + "HEARTBEAT_ENABLED": "false", + "EMBEDDING_ENABLED": "false", + "WASM_ENABLED": "true", + "WASM_TOOLS_DIR": tools_tmpdir.name, + "WASM_CHANNELS_DIR": channels_tmpdir.name, + "ONBOARD_COMPLETED": "true", + "IRONCLAW_OAUTH_CALLBACK_URL": "https://oauth.test.example/oauth/callback", + "IRONCLAW_OAUTH_EXCHANGE_URL": mock_llm_server, + "GOOGLE_OAUTH_CLIENT_ID": "hosted-google-client-id", + } + _forward_coverage_env(env) + + proc = await asyncio.create_subprocess_exec( + ironclaw_binary, "--no-onboard", + stdin=asyncio.subprocess.DEVNULL, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + env=env, + ) + startup_kill_attempted = False + base_url = f"http://127.0.0.1:{gateway_port}" + try: + await wait_for_ready(f"{base_url}/api/health", timeout=60) + yield { + "base_url": base_url, + "db_path": db_path, + "gateway_user_id": OWNER_SCOPE_ID, + "mock_llm_url": mock_llm_server, + } + except TimeoutError: + if proc.returncode is None: + startup_kill_attempted = True + await _stop_process(proc, timeout=2) + returncode = proc.returncode + stderr_bytes = b"" + if proc.stderr: + try: + stderr_bytes = await asyncio.wait_for(proc.stderr.read(8192), timeout=2) + except asyncio.TimeoutError: + pass + stderr_text = stderr_bytes.decode("utf-8", errors="replace") + pytest.fail( + f"extension cleanup server failed to start on port {gateway_port} " + f"(returncode={returncode}).\nstderr:\n{stderr_text}" + ) + finally: + if proc.returncode is None: + if startup_kill_attempted: + await _stop_process(proc, timeout=2) + else: + await _stop_process(proc, sig=signal.SIGINT, timeout=10) + if proc.returncode is None: + await _stop_process(proc, timeout=2) + finally: + for sock in reserved: + if sock.fileno() != -1: + sock.close() + db_tmpdir.cleanup() + home_tmpdir.cleanup() + tools_tmpdir.cleanup() + channels_tmpdir.cleanup() + + @pytest.fixture(scope="session") async def http_channel_server(ironclaw_server, server_ports): """HTTP webhook channel base URL.""" diff --git a/tests/e2e/scenarios/test_extension_uninstall_cleanup.py b/tests/e2e/scenarios/test_extension_uninstall_cleanup.py new file mode 100644 index 00000000..e105bd09 --- /dev/null +++ b/tests/e2e/scenarios/test_extension_uninstall_cleanup.py @@ -0,0 +1,266 @@ +"""Extension uninstall secret cleanup E2E tests. + +Exercises real install/setup/auth/remove flows and verifies the backing +secrets table is cleaned up when extensions are uninstalled. +""" + +import sqlite3 +from urllib.parse import parse_qs, urlparse + +import httpx + +from helpers import api_get, api_post + + +def _extract_state(auth_url: str) -> str: + parsed = urlparse(auth_url) + state = parse_qs(parsed.query).get("state", [None])[0] + assert state, f"auth_url should include state: {auth_url}" + return state + + +def _secret_exists(db_path: str, user_id: str, name: str) -> bool: + with sqlite3.connect(db_path) as conn: + row = conn.execute( + "SELECT 1 FROM secrets WHERE user_id = ?1 AND name = ?2 LIMIT 1", + (user_id, name), + ).fetchone() + return row is not None + + +def _secret_names(db_path: str, user_id: str) -> set[str]: + with sqlite3.connect(db_path) as conn: + rows = conn.execute( + "SELECT name FROM secrets WHERE user_id = ?1", + (user_id,), + ).fetchall() + return {row[0] for row in rows} + + +async def _get_extension(base_url: str, name: str) -> dict | None: + response = await api_get(base_url, "/api/extensions", timeout=15) + response.raise_for_status() + for extension in response.json().get("extensions", []): + if extension["name"] == name: + return extension + return None + + +async def _ensure_removed(base_url: str, name: str) -> None: + extension = await _get_extension(base_url, name) + if extension is not None: + response = await api_post(base_url, f"/api/extensions/{name}/remove", timeout=30) + assert response.status_code == 200, response.text + assert response.json().get("success") is True, response.text + + +async def _install_extension( + base_url: str, + name: str, + *, + kind: str | None = None, + url: str | None = None, +) -> None: + payload = {"name": name} + if kind is not None: + payload["kind"] = kind + if url is not None: + payload["url"] = url + + response = await api_post( + base_url, + "/api/extensions/install", + json=payload, + timeout=180, + ) + assert response.status_code == 200, response.text + assert response.json().get("success") is True, response.text + + +async def test_remove_wasm_tool_deletes_unique_secret(extension_cleanup_server): + server = extension_cleanup_server["base_url"] + db_path = extension_cleanup_server["db_path"] + user_id = extension_cleanup_server["gateway_user_id"] + + await _ensure_removed(server, "web-search") + + await _install_extension(server, "web-search") + + setup_response = await api_post( + server, + "/api/extensions/web-search/setup", + json={"secrets": {"brave_api_key": "cleanup-test-key"}}, + timeout=30, + ) + assert setup_response.status_code == 200, setup_response.text + assert setup_response.json().get("success") is True, setup_response.text + assert _secret_exists(db_path, user_id, "brave_api_key") + + remove_response = await api_post( + server, + "/api/extensions/web-search/remove", + timeout=30, + ) + assert remove_response.status_code == 200, remove_response.text + assert remove_response.json().get("success") is True, remove_response.text + assert not _secret_exists(db_path, user_id, "brave_api_key") + + +async def test_remove_wasm_channel_deletes_setup_secrets(extension_cleanup_server): + server = extension_cleanup_server["base_url"] + db_path = extension_cleanup_server["db_path"] + user_id = extension_cleanup_server["gateway_user_id"] + + await _ensure_removed(server, "discord") + + await _install_extension(server, "discord", kind="wasm_channel") + + setup_response = await api_post( + server, + "/api/extensions/discord/setup", + json={ + "secrets": { + "discord_bot_token": "cleanup-discord-bot-token", + "discord_public_key": "cleanup-discord-public-key", + } + }, + timeout=30, + ) + assert setup_response.status_code == 200, setup_response.text + assert setup_response.json().get("success") is True, setup_response.text + assert _secret_exists(db_path, user_id, "discord_bot_token") + assert _secret_exists(db_path, user_id, "discord_public_key") + + remove_response = await api_post( + server, + "/api/extensions/discord/remove", + timeout=30, + ) + assert remove_response.status_code == 200, remove_response.text + assert remove_response.json().get("success") is True, remove_response.text + assert not _secret_exists(db_path, user_id, "discord_bot_token") + assert not _secret_exists(db_path, user_id, "discord_public_key") + + +async def test_remove_shared_google_oauth_secrets_after_last_tool(extension_cleanup_server): + server = extension_cleanup_server["base_url"] + db_path = extension_cleanup_server["db_path"] + user_id = extension_cleanup_server["gateway_user_id"] + + await _ensure_removed(server, "gmail") + await _ensure_removed(server, "google-drive") + + await _install_extension(server, "gmail") + await _install_extension(server, "google-drive") + + setup_response = await api_post( + server, + "/api/extensions/gmail/setup", + json={"secrets": {}}, + timeout=30, + ) + assert setup_response.status_code == 200, setup_response.text + auth_url = setup_response.json().get("auth_url") + assert auth_url, setup_response.text + + async with httpx.AsyncClient() as client: + callback_response = await client.get( + f"{server}/oauth/callback", + params={"code": "mock_auth_code", "state": _extract_state(auth_url)}, + timeout=30, + follow_redirects=True, + ) + assert callback_response.status_code == 200, callback_response.text[:400] + + shared_secrets = [ + "google_oauth_token", + "google_oauth_token_refresh_token", + "google_oauth_token_scopes", + ] + for secret_name in shared_secrets: + assert _secret_exists(db_path, user_id, secret_name), f"expected {secret_name} to exist" + + gmail_remove_response = await api_post( + server, + "/api/extensions/gmail/remove", + timeout=30, + ) + assert gmail_remove_response.status_code == 200, gmail_remove_response.text + assert gmail_remove_response.json().get("success") is True, gmail_remove_response.text + for secret_name in shared_secrets: + assert _secret_exists(db_path, user_id, secret_name), ( + f"{secret_name} should remain while google-drive is still installed" + ) + + drive_remove_response = await api_post( + server, + "/api/extensions/google-drive/remove", + timeout=30, + ) + assert drive_remove_response.status_code == 200, drive_remove_response.text + assert drive_remove_response.json().get("success") is True, drive_remove_response.text + for secret_name in shared_secrets: + assert not _secret_exists(db_path, user_id, secret_name), ( + f"{secret_name} should be deleted after the last Google tool is removed" + ) + + +async def test_remove_mcp_server_deletes_stored_secrets(extension_cleanup_server): + server = extension_cleanup_server["base_url"] + db_path = extension_cleanup_server["db_path"] + user_id = extension_cleanup_server["gateway_user_id"] + mcp_url = f"{extension_cleanup_server['mock_llm_url']}/mcp" + + await _ensure_removed(server, "mock-mcp") + + await _install_extension(server, "mock-mcp", kind="mcp_server", url=mcp_url) + + setup_response = await api_post( + server, + "/api/extensions/mock-mcp/setup", + json={"secrets": {}}, + timeout=30, + ) + assert setup_response.status_code == 200, setup_response.text + auth_url = setup_response.json().get("auth_url") + if auth_url is None: + activate_response = await api_post( + server, + "/api/extensions/mock-mcp/activate", + timeout=30, + ) + assert activate_response.status_code == 200, activate_response.text + auth_url = activate_response.json().get("auth_url") + assert auth_url, "mock-mcp should require OAuth in E2E" + + async with httpx.AsyncClient() as client: + callback_response = await client.get( + f"{server}/oauth/callback", + params={"code": "mock_mcp_code", "state": _extract_state(auth_url)}, + timeout=30, + follow_redirects=True, + ) + assert callback_response.status_code == 200, callback_response.text[:400] + + expected_mcp_secrets = [ + "mcp_mock-mcp_access_token", + "mcp_mock-mcp_client_id", + ] + stored_secret_names = _secret_names(db_path, user_id) + for secret_name in expected_mcp_secrets: + assert secret_name in stored_secret_names, ( + f"expected {secret_name} to exist; stored secrets were {sorted(stored_secret_names)}" + ) + + remove_response = await api_post( + server, + "/api/extensions/mock-mcp/remove", + timeout=30, + ) + assert remove_response.status_code == 200, remove_response.text + assert remove_response.json().get("success") is True, remove_response.text + remaining_secret_names = _secret_names(db_path, user_id) + assert not any(name.startswith("mcp_mock-mcp_") for name in remaining_secret_names), ( + f"mock-mcp secrets should be deleted on remove; remaining secrets were " + f"{sorted(remaining_secret_names)}" + )