From 13dbecb3c77fff81c5e7df16871d4e1db22c936a Mon Sep 17 00:00:00 2001 From: Henry Park Date: Mon, 6 Apr 2026 22:15:33 -0700 Subject: [PATCH 1/5] fix(ownership): remove silent cross-tenant credential fallback in WASM wrappers (#2069, #2070) WASM tool credential resolution silently fell back to looking up secrets under the hardcoded "default" scope when the calling user had no credential configured, leaking the instance owner's API keys to other users without error or audit trail. - Remove "default" fallback in resolve_host_credentials(); return Err(ToolError::NotAuthorized) with actionable message instead of silently skipping missing credentials - Fix resolve_websocket_identify_message() to accept owner_scope_id parameter instead of hardcoding "default" - Document legacy broadcast metadata fallback with removal tracking - Document setup.rs boot-time owner_id lookups as intentional instance-level resource ownership - Add regression tests proving cross-tenant credentials do not leak Co-Authored-By: Claude Haiku 4.5 --- src/channels/wasm/setup.rs | 10 ++ src/channels/wasm/wrapper.rs | 75 ++++++++- src/tools/wasm/wrapper.rs | 288 +++++++++++++++++++++-------------- 3 files changed, 254 insertions(+), 119 deletions(-) diff --git a/src/channels/wasm/setup.rs b/src/channels/wasm/setup.rs index 130955bee14..5700a0c8461 100644 --- a/src/channels/wasm/setup.rs +++ b/src/channels/wasm/setup.rs @@ -2,6 +2,15 @@ //! //! Encapsulates the logic for loading WASM channels, registering their //! webhook routes, and injecting credentials from the secrets store. +//! +//! # Ownership model +//! +//! Boot-time secret lookups use `config.owner_id` because channels are +//! **instance-level resources** — they run as the instance operator, not as +//! individual users. This is intentional and distinct from tool-level +//! credential resolution, which is scoped to the calling user's `user_id`. +//! +//! See `docs/superpowers/specs/2026-04-01-ownership-model-design.md`. use std::collections::HashSet; use std::sync::Arc; @@ -179,6 +188,7 @@ async fn register_channel( let sig_key_secret_name = loaded.signature_key_secret_name(); let hmac_secret_name = loaded.hmac_secret_name(); + // Channel-level secrets: owner_id is correct — channels are instance resources. let webhook_secret = if let Some(secrets) = secrets_store { secrets .get_decrypted(&config.owner_id, &secret_name) diff --git a/src/channels/wasm/wrapper.rs b/src/channels/wasm/wrapper.rs index 8660882091e..e762978101c 100644 --- a/src/channels/wasm/wrapper.rs +++ b/src/channels/wasm/wrapper.rs @@ -1032,6 +1032,14 @@ impl WasmChannel { } /// Load broadcast metadata from settings store on startup. + /// + /// # Legacy migration (remove after ownership model rollout — see #2069) + /// + /// If no metadata is found under `self.owner_scope_id`, a second lookup + /// under `"default"` is attempted for backward compatibility with instances + /// that stored broadcast metadata before the ownership model migration. + /// Remove this fallback once all deployments have run the + /// `migrate_default_owner` bootstrap step and restarted at least once. async fn load_broadcast_metadata(&self) { if let Some(ref store) = self.settings_store { match store @@ -1046,6 +1054,7 @@ impl WasmChannel { ); } Ok(_) => { + // LEGACY MIGRATION: remove after ownership model rollout — see #2069 if self.owner_scope_id != "default" { match store .get_setting("default", &self.broadcast_metadata_key()) @@ -1163,9 +1172,12 @@ impl WasmChannel { ); let queue_path = websocket_queue_path(&channel_name); let processing_queue_path = websocket_processing_queue_path(&channel_name); - let identify_payload = - resolve_websocket_identify_message(&config, websocket_secrets_store.as_deref()) - .await; + let identify_payload = resolve_websocket_identify_message( + &config, + websocket_secrets_store.as_deref(), + &owner_scope_id, + ) + .await; let mut session_state = WebsocketSessionState::new(identify_payload.as_deref()); 'reconnect: loop { @@ -3120,14 +3132,19 @@ fn websocket_processing_queue_path(channel_name: &str) -> String { format!("channels/{channel_name}/{WEBSOCKET_EVENT_PROCESSING_QUEUE_RELATIVE_PATH}") } -async fn resolve_websocket_identify_message( +pub(crate) async fn resolve_websocket_identify_message( config: &WebsocketRuntimeConfig, store: Option<&(dyn SecretsStore + Send + Sync)>, + owner_scope_id: &str, ) -> Option { let identify = config.identify.clone()?; let secret_name = config.identify_secret_name.as_ref()?; let store = store?; - let secret = store.get_decrypted("default", secret_name).await.ok()?; + // Channel runtime secrets are instance-owned, resolved under the channel's owner scope. + let secret = store + .get_decrypted(owner_scope_id, secret_name) + .await + .ok()?; build_websocket_identify_message(&identify, secret.expose()) } @@ -4180,9 +4197,9 @@ mod tests { WebsocketRuntimeConfig, build_discord_gateway_presence_update, build_websocket_identify_message, build_websocket_resume_message, discord_gateway_presence_status, drain_guest_logs, parse_websocket_invalid_session, - parse_websocket_ready_session, should_warn_on_heartbeat_interval, - uses_owner_broadcast_target, websocket_heartbeat_sleep_duration, - websocket_reconnect_backoff, + parse_websocket_ready_session, resolve_websocket_identify_message, + should_warn_on_heartbeat_interval, uses_owner_broadcast_target, + websocket_heartbeat_sleep_duration, websocket_reconnect_backoff, }; use crate::pairing::PairingStore; use crate::testing::credentials::TEST_TELEGRAM_BOT_TOKEN; @@ -4291,6 +4308,48 @@ mod tests { assert_eq!(json["d"]["intents"], serde_json::json!(513)); } + /// Regression test for #2069: websocket identify must use owner_scope_id, + /// not hardcoded "default". + #[tokio::test] + async fn test_resolve_websocket_identify_message_uses_owner_scope() { + use crate::secrets::SecretsStore; + use crate::testing::credentials::test_secrets_store; + + let store = test_secrets_store(); + + // Store secret under a specific owner, NOT under "default" + store + .create( + "owner_42", + crate::secrets::CreateSecretParams::new("discord_bot_token", "real_bot_token"), + ) + .await + .expect("store token"); // safety: test code only + + let config = WebsocketRuntimeConfig { + url: "wss://gateway.discord.gg/?v=10&encoding=json".to_string(), + connect_on_start: true, + identify: Some(serde_json::json!({ + "intents": 513, + "properties": { "os": "linux", "browser": "ironclaw", "device": "ironclaw" } + })), + identify_secret_name: Some("discord_bot_token".to_string()), + }; + + // Should find the secret under "owner_42" + let payload = resolve_websocket_identify_message(&config, Some(&store), "owner_42").await; + assert!(payload.is_some(), "should resolve from owner scope"); // safety: test code only + let json: serde_json::Value = serde_json::from_str(payload.as_ref().unwrap()).unwrap(); // safety: test code only + assert_eq!(json["d"]["token"], serde_json::json!("real_bot_token")); + + // Must NOT find the secret under "default" + let no_payload = resolve_websocket_identify_message(&config, Some(&store), "default").await; + assert!( + no_payload.is_none(), + "default scope must not find owner_42's secret" + ); + } + #[test] fn test_websocket_runtime_config_requires_allowlisted_host() { let tool_capabilities = ToolCapabilities { diff --git a/src/tools/wasm/wrapper.rs b/src/tools/wasm/wrapper.rs index 85b7b037ac3..684eb799b3a 100644 --- a/src/tools/wasm/wrapper.rs +++ b/src/tools/wasm/wrapper.rs @@ -84,6 +84,7 @@ impl OAuthRefreshConfig { /// Built before each WASM execution by decrypting secrets from the store. /// Applied per-request by matching the URL host against `host_patterns`. /// WASM tools never see the raw secret values. +#[derive(Debug)] struct ResolvedHostCredential { /// Host patterns this credential applies to (e.g., "www.googleapis.com"). host_patterns: Vec, @@ -1151,7 +1152,7 @@ impl Tool for WasmToolWrapper { credential_user_id, self.oauth_refresh.as_ref(), ) - .await; + .await?; // Serialize context for WASM let context_json = serde_json::to_string(ctx).ok(); @@ -1463,15 +1464,15 @@ async fn persist_refreshed_oauth_tokens( /// If an `OAuthRefreshConfig` is provided and the access token is expired /// (or within 5 minutes of expiry), attempts a transparent refresh first. /// -/// Silently skips credentials that can't be resolved (e.g., missing secrets). -/// The tool will get a 401/403 from the API, which is the expected UX when -/// auth hasn't been configured yet. +/// Returns `Err(ToolError::NotAuthorized)` when a required credential is +/// missing for the given `user_id`. No cross-tenant fallback: each user +/// must have their own credentials configured. async fn resolve_host_credentials( capabilities: &Capabilities, store: Option<&(dyn SecretsStore + Send + Sync)>, user_id: &str, oauth_refresh: Option<&OAuthRefreshConfig>, -) -> Vec { +) -> Result, ToolError> { let store = match store { Some(s) => s, None => { @@ -1479,12 +1480,11 @@ async fn resolve_host_credentials( if let Some(http_cap) = &capabilities.http && !http_cap.credentials.is_empty() { - tracing::warn!( - user_id = %user_id, - "WASM tool requires credentials but secrets_store is not configured - authentication will fail" - ); + return Err(ToolError::NotAuthorized(format!( + "secrets store not configured; cannot resolve credentials for user '{user_id}'" + ))); } - return Vec::new(); + return Ok(Vec::new()); } }; @@ -1518,11 +1518,11 @@ async fn resolve_host_credentials( let http_cap = match &capabilities.http { Some(cap) => cap, - None => return Vec::new(), + None => return Ok(Vec::new()), }; if http_cap.credentials.is_empty() { - return Vec::new(); + return Ok(Vec::new()); } let mut resolved = Vec::new(); @@ -1536,46 +1536,15 @@ async fn resolve_host_credentials( continue; } - // Try to get credential under the provided user_id first. - // If not found and user_id != "default", fallback to "default" (global credentials). - // This handles OAuth tokens stored globally under "default" but accessed from routine contexts. + // Look up credential under the provided user_id only. + // No cross-tenant fallback: each user must configure their own credentials. let secret = match store.get_decrypted(user_id, &mapping.secret_name).await { - Ok(s) => Some(s), - Err(e) => { - tracing::trace!( - user_id = %user_id, - secret_name = %mapping.secret_name, - error = %e, - "No matching host credential resolved for WASM tool in the requested scope" - ); - - // If lookup fails and we're not already looking up "default", try "default" as fallback - if user_id != "default" { - tracing::debug!( - secret_name = %mapping.secret_name, - user_id = %user_id, - error = %e, - "Credential not found for user, trying default global credentials" - ); - store - .get_decrypted("default", &mapping.secret_name) - .await - .ok() - } else { - None - } - } - }; - - let secret = match secret { - Some(s) => s, - None => { - tracing::warn!( - secret_name = %mapping.secret_name, - user_id = %user_id, - "Could not resolve credential for WASM tool (not found in user context or default)" - ); - continue; + Ok(s) => s, + Err(_) => { + return Err(ToolError::NotAuthorized(format!( + "credential '{}' not found for user '{}'; configure it via `ironclaw secrets set`", + mapping.secret_name, user_id + ))); } }; @@ -1601,7 +1570,7 @@ async fn resolve_host_credentials( ); } - resolved + Ok(resolved) } /// Extract the hostname from a URL string. @@ -2311,10 +2280,46 @@ mod tests { use crate::tools::wasm::wrapper::resolve_host_credentials; let caps = Capabilities::default(); - let result = resolve_host_credentials(&caps, None, "user1", None).await; + let result = resolve_host_credentials(&caps, None, "user1", None) + .await + .expect("no http cap means Ok(empty)"); // safety: test code only assert!(result.is_empty()); } + #[tokio::test] + async fn test_resolve_host_credentials_no_store_with_credentials_errors() { + use crate::secrets::{CredentialLocation, CredentialMapping}; + use crate::tools::wasm::capabilities::HttpCapability; + use crate::tools::wasm::wrapper::resolve_host_credentials; + + let mut credentials = HashMap::new(); + credentials.insert( + "api_token".to_string(), + CredentialMapping { + secret_name: "api_token".to_string(), + location: CredentialLocation::AuthorizationBearer, + host_patterns: vec!["api.example.com".to_string()], + }, + ); + let caps = Capabilities { + http: Some(HttpCapability { + credentials, + ..Default::default() + }), + ..Default::default() + }; + + // No store but credentials required — must error + let err = resolve_host_credentials(&caps, None, "user1", None) + .await + .expect_err("no store with required credentials must error"); // safety: test code only + assert!( + err.to_string().contains("secrets store not configured"), + "error message: {}", + err + ); + } + #[tokio::test] async fn test_resolve_host_credentials_no_http_cap() { use crate::tools::wasm::wrapper::resolve_host_credentials; @@ -2322,7 +2327,9 @@ mod tests { let store = test_secrets_store(); let caps = Capabilities::default(); - let result = resolve_host_credentials(&caps, Some(&store), "user1", None).await; + let result = resolve_host_credentials(&caps, Some(&store), "user1", None) + .await + .expect("no http cap means Ok(empty)"); // safety: test code only assert!(result.is_empty()); } @@ -2362,7 +2369,9 @@ mod tests { ..Default::default() }; - let result = resolve_host_credentials(&caps, Some(&store), "user1", None).await; + let result = resolve_host_credentials(&caps, Some(&store), "user1", None) + .await + .expect("user1 has the credential"); // safety: test code only assert_eq!(result.len(), 1); assert_eq!(result[0].host_patterns, vec!["www.googleapis.com"]); assert_eq!( @@ -2408,7 +2417,9 @@ mod tests { ..Default::default() }; - let result = resolve_host_credentials(&caps, Some(&store), &ctx.user_id, None).await; + let result = resolve_host_credentials(&caps, Some(&store), &ctx.user_id, None) + .await + .expect("owner-scope has the credential"); // safety: test code only assert_eq!(result.len(), 1); assert_eq!( result[0].headers.get("Authorization"), @@ -2466,14 +2477,14 @@ mod tests { } #[tokio::test] - async fn test_resolve_host_credentials_missing_secret() { + async fn test_resolve_host_credentials_missing_secret_returns_error() { use crate::secrets::{CredentialLocation, CredentialMapping}; use crate::tools::wasm::capabilities::HttpCapability; use crate::tools::wasm::wrapper::resolve_host_credentials; let store = test_secrets_store(); - // No secret stored, should silently skip + // No secret stored — must fail with NotAuthorized, not silently skip let mut credentials = HashMap::new(); credentials.insert( "missing_token".to_string(), @@ -2492,7 +2503,53 @@ mod tests { ..Default::default() }; - let result = resolve_host_credentials(&caps, Some(&store), "user1", None).await; + let err = resolve_host_credentials(&caps, Some(&store), "user1", None) + .await + .expect_err("missing credential must return error"); // safety: test code only + let msg = err.to_string(); + assert!( + msg.contains("missing_token"), + "error should name the missing credential: {msg}" + ); + assert!(msg.contains("user1"), "error should name the user: {msg}"); + } + + /// UrlPath credentials are resolved via URL placeholder substitution, not + /// the secrets store lookup in `resolve_host_credentials`. A missing + /// UrlPath credential must NOT trigger `NotAuthorized`. + #[tokio::test] + async fn test_resolve_host_credentials_skips_urlpath_credentials() { + use crate::secrets::{CredentialLocation, CredentialMapping}; + use crate::tools::wasm::capabilities::HttpCapability; + use crate::tools::wasm::wrapper::resolve_host_credentials; + + let store = test_secrets_store(); + + // Only a UrlPath credential — no secret stored for it + let mut credentials = HashMap::new(); + credentials.insert( + "api_key".to_string(), + CredentialMapping { + secret_name: "api_key".to_string(), + location: CredentialLocation::UrlPath { + placeholder: "{api_key}".to_string(), + }, + host_patterns: vec!["api.example.com".to_string()], + }, + ); + + let caps = Capabilities { + http: Some(HttpCapability { + credentials, + ..Default::default() + }), + ..Default::default() + }; + + // UrlPath creds are skipped — result is Ok(empty), not an error + let result = resolve_host_credentials(&caps, Some(&store), "user1", None) + .await + .expect("UrlPath credentials should be skipped, not cause an error"); // safety: test code only assert!(result.is_empty()); } @@ -2546,8 +2603,9 @@ mod tests { }; // Should resolve the existing fresh token without attempting refresh - let result = - resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)).await; + let result = resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)) + .await + .expect("user1 has a fresh token"); // safety: test code only assert_eq!(result.len(), 1); assert_eq!( result[0].headers.get("Authorization"), @@ -2593,9 +2651,11 @@ mod tests { ..Default::default() }; - // No OAuth config, expired token can't be resolved (get_decrypted returns Expired) - let result = resolve_host_credentials(&caps, Some(&store), "user1", None).await; - assert!(result.is_empty()); + // No OAuth config, expired token can't be resolved — returns NotAuthorized error + let err = resolve_host_credentials(&caps, Some(&store), "user1", None) + .await + .expect_err("expired credential without refresh config must error"); // safety: test code only + assert!(err.to_string().contains("my_token")); } #[tokio::test] @@ -2646,8 +2706,9 @@ mod tests { }; // Should use the legacy token directly without attempting refresh - let result = - resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)).await; + let result = resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)) + .await + .expect("user1 has a legacy token"); // safety: test code only assert_eq!(result.len(), 1); assert_eq!( result[0].headers.get("Authorization"), @@ -2711,8 +2772,9 @@ mod tests { provider: Some("google".to_string()), }; - let resolved = - resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)).await; + let resolved = resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)) + .await + .expect("refresh should succeed via proxy"); // safety: test code only assert_eq!(resolved.len(), 1); assert_eq!( resolved[0].headers.get("Authorization"), @@ -2820,9 +2882,10 @@ mod tests { provider: Some("google".to_string()), }; - let resolved = - resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)).await; - assert!(resolved.is_empty()); + let err = resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)) + .await + .expect_err("expired token without gateway_token should error"); // safety: test code only + assert!(err.to_string().contains("google_oauth_token")); let lookups = store.decrypted_lookups(); assert!(lookups.contains(&("user1".to_string(), "google_oauth_token".to_string()))); @@ -2887,9 +2950,10 @@ mod tests { provider: Some("google".to_string()), }; - let resolved = - resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)).await; - assert!(resolved.is_empty()); + let err = resolve_host_credentials(&caps, Some(&store), "user1", Some(&oauth_config)) + .await + .expect_err("expired token with invalid direct URL should error"); // safety: test code only + assert!(err.to_string().contains("google_oauth_token")); let lookups = store.decrypted_lookups(); assert!(lookups.contains(&("user1".to_string(), "google_oauth_token".to_string()))); @@ -3092,15 +3156,17 @@ mod tests { ); } + /// Regression test for #2069: credentials stored under "default" must NOT + /// leak to a different user's tool execution. #[tokio::test] - async fn test_resolve_host_credentials_fallback_to_default_user() { + async fn test_resolve_host_credentials_no_cross_tenant_fallback() { use crate::secrets::{CredentialLocation, CredentialMapping, SecretsStore}; use crate::tools::wasm::capabilities::HttpCapability; use crate::tools::wasm::wrapper::resolve_host_credentials; let store = test_secrets_store(); - // Store a token under the "default" global user + // Store a token under the "default" user only store .create( "default", @@ -3109,7 +3175,6 @@ mod tests { .await .expect("Failed to store global token"); // safety: test code only - // Create capabilities requiring this credential let mut creds = std::collections::HashMap::new(); creds.insert( "google_oauth_token".to_string(), @@ -3131,12 +3196,19 @@ mod tests { ..Default::default() }; - // Resolve credentials for a different user (routine context) - // Should fallback to "default" and find the token - let result = resolve_host_credentials(&caps, Some(&store), "routine_user_123", None).await; - - assert!(!result.is_empty(), "fallback to default"); // safety: test code only - assert_eq!(result[0].secret_value, "global_token_value"); // safety: test code only + // Must NOT fall back to "default" — returns NotAuthorized error + let err = resolve_host_credentials(&caps, Some(&store), "routine_user_123", None) + .await + .expect_err("cross-tenant credential must not leak"); // safety: test code only + let msg = err.to_string(); + assert!( + msg.contains("routine_user_123"), + "error names the user: {msg}" + ); + assert!( + msg.contains("google_oauth_token"), + "error names the credential: {msg}" + ); } fn test_capabilities_with_google_oauth() -> Capabilities { @@ -3166,21 +3238,12 @@ mod tests { } #[tokio::test] - async fn test_resolve_host_credentials_prefers_user_specific_over_default() { + async fn test_resolve_host_credentials_returns_user_specific_token() { use crate::secrets::SecretsStore; use crate::tools::wasm::wrapper::resolve_host_credentials; let store = test_secrets_store(); - // Store token under "default" (global) - store - .create( - "default", - crate::secrets::CreateSecretParams::new("google_oauth_token", "global_token"), - ) - .await - .expect("Failed to store global token"); // safety: test code only - // Store token under user_123 (user-specific) store .create( @@ -3193,25 +3256,25 @@ mod tests { .await .expect("Failed to store user token"); // safety: test code only - // Create capabilities let caps = test_capabilities_with_google_oauth(); - // Resolve credentials for user_123 - // Should prefer user_123's token over default - let result = resolve_host_credentials(&caps, Some(&store), "user_123", None).await; + // Resolve credentials for user_123 — finds user's own token directly + let result = resolve_host_credentials(&caps, Some(&store), "user_123", None) + .await + .expect("user_123 has the credential"); // safety: test code only assert!(!result.is_empty(), "has user credentials"); // safety: test code only assert_eq!(result[0].secret_value, "user_specific_token", "user token"); // safety: test code only } #[tokio::test] - async fn test_resolve_host_credentials_no_fallback_when_already_default() { + async fn test_resolve_host_credentials_direct_lookup_for_default_user() { use crate::secrets::SecretsStore; use crate::tools::wasm::wrapper::resolve_host_credentials; let store = test_secrets_store(); - // Only store token under "default" (not a duplicate) + // Store token under "default" store .create( "default", @@ -3220,33 +3283,36 @@ mod tests { .await .expect("Failed to store default token"); // safety: test code only - // Create capabilities let caps = test_capabilities_with_google_oauth(); - // Resolve credentials for "default" user - // Should NOT attempt fallback (already looking up default) - let result = resolve_host_credentials(&caps, Some(&store), "default", None).await; + // Direct lookup for "default" user — finds its own token + let result = resolve_host_credentials(&caps, Some(&store), "default", None) + .await + .expect("default user has the credential"); // safety: test code only assert!(!result.is_empty(), "Should find default token"); // safety: test code only assert_eq!(result[0].secret_value, "default_token"); // safety: test code only } #[tokio::test] - async fn test_resolve_host_credentials_missing_secret_warns() { + async fn test_resolve_host_credentials_missing_secret_returns_not_authorized() { use crate::tools::wasm::wrapper::resolve_host_credentials; let store = test_secrets_store(); // Don't store any token - - // Create capabilities expecting a credential let caps = test_capabilities_with_google_oauth(); - // Resolve credentials when neither user nor default has the token - let result = resolve_host_credentials(&caps, Some(&store), "user_456", None).await; - - // Should return empty since credential can't be found anywhere - assert!(result.is_empty(), "no credentials found"); // safety: test code only + // Missing credential must return actionable error + let err = resolve_host_credentials(&caps, Some(&store), "user_456", None) + .await + .expect_err("missing credential must error"); // safety: test code only + let msg = err.to_string(); + assert!(msg.contains("user_456"), "error names the user: {msg}"); + assert!( + msg.contains("ironclaw secrets set"), + "error suggests fix: {msg}" + ); } // --- needs_content_length_zero (regression for #1529) --- From 9c38c94dea84c3f0031e007289e1043155f5f886 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Mon, 6 Apr 2026 22:42:00 -0700 Subject: [PATCH 2/5] fix(review): differentiate expired vs missing credential errors, exclude UrlPath from store check Address PR review feedback: - Filter out UrlPath credentials in the no-store check so tools with only UrlPath mappings don't incorrectly get NotAuthorized - Match SecretError::Expired separately to produce "has expired" message instead of misleading "not found" - Add tests for both: UrlPath-only no-store (Ok), expired credential (specific error message) Co-Authored-By: Claude Haiku 4.5 --- src/tools/wasm/wrapper.rs | 101 +++++++++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 2 deletions(-) diff --git a/src/tools/wasm/wrapper.rs b/src/tools/wasm/wrapper.rs index 684eb799b3a..5e8a40de580 100644 --- a/src/tools/wasm/wrapper.rs +++ b/src/tools/wasm/wrapper.rs @@ -1476,9 +1476,16 @@ async fn resolve_host_credentials( let store = match store { Some(s) => s, None => { - // If tool requires credentials but has no secrets store, this is a configuration error + // If tool requires non-UrlPath credentials but has no secrets store, this is + // a configuration error. UrlPath credentials are handled by placeholder + // substitution and don't need the secrets store. if let Some(http_cap) = &capabilities.http - && !http_cap.credentials.is_empty() + && http_cap.credentials.values().any(|m| { + !matches!( + m.location, + crate::secrets::CredentialLocation::UrlPath { .. } + ) + }) { return Err(ToolError::NotAuthorized(format!( "secrets store not configured; cannot resolve credentials for user '{user_id}'" @@ -1540,6 +1547,12 @@ async fn resolve_host_credentials( // No cross-tenant fallback: each user must configure their own credentials. let secret = match store.get_decrypted(user_id, &mapping.secret_name).await { Ok(s) => s, + Err(crate::secrets::SecretError::Expired) => { + return Err(ToolError::NotAuthorized(format!( + "credential '{}' for user '{}' has expired; refresh or re-set via `ironclaw secrets set`", + mapping.secret_name, user_id + ))); + } Err(_) => { return Err(ToolError::NotAuthorized(format!( "credential '{}' not found for user '{}'; configure it via `ironclaw secrets set`", @@ -2320,6 +2333,90 @@ mod tests { ); } + #[tokio::test] + async fn test_resolve_host_credentials_no_store_with_only_urlpath_ok() { + use crate::secrets::{CredentialLocation, CredentialMapping}; + use crate::tools::wasm::capabilities::HttpCapability; + use crate::tools::wasm::wrapper::resolve_host_credentials; + + let mut credentials = HashMap::new(); + credentials.insert( + "api_key".to_string(), + CredentialMapping { + secret_name: "api_key".to_string(), + location: CredentialLocation::UrlPath { + placeholder: "{api_key}".to_string(), + }, + host_patterns: vec!["api.example.com".to_string()], + }, + ); + let caps = Capabilities { + http: Some(HttpCapability { + credentials, + ..Default::default() + }), + ..Default::default() + }; + + // No store but only UrlPath credentials — should be Ok(empty), not an error + let result = resolve_host_credentials(&caps, None, "user1", None) + .await + .expect("UrlPath-only credentials should not require a secrets store"); // safety: test code only + assert!(result.is_empty()); + } + + #[tokio::test] + async fn test_resolve_host_credentials_expired_credential_returns_specific_error() { + use crate::secrets::{ + CreateSecretParams, CredentialLocation, CredentialMapping, SecretsStore, + }; + use crate::tools::wasm::capabilities::HttpCapability; + use crate::tools::wasm::wrapper::resolve_host_credentials; + + let store = test_secrets_store(); + + // Store an expired token + store + .create( + "user1", + CreateSecretParams::new("my_token", "expired-value") + .with_expiry(chrono::Utc::now() - chrono::Duration::hours(1)), + ) + .await + .unwrap(); + + let mut credentials = HashMap::new(); + credentials.insert( + "my_token".to_string(), + CredentialMapping { + secret_name: "my_token".to_string(), + location: CredentialLocation::AuthorizationBearer, + host_patterns: vec!["api.example.com".to_string()], + }, + ); + let caps = Capabilities { + http: Some(HttpCapability { + credentials, + ..Default::default() + }), + ..Default::default() + }; + + // Expired credential — error message should say "expired", not "not found" + let err = resolve_host_credentials(&caps, Some(&store), "user1", None) + .await + .expect_err("expired credential must error"); // safety: test code only + let msg = err.to_string(); + assert!( + msg.contains("expired"), + "error should mention expiry: {msg}" + ); + assert!( + msg.contains("my_token"), + "error should name the credential: {msg}" + ); + } + #[tokio::test] async fn test_resolve_host_credentials_no_http_cap() { use crate::tools::wasm::wrapper::resolve_host_credentials; From 5bac8f2a9e214b061da9b1ed5c70e90f1852c313 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Tue, 7 Apr 2026 07:58:02 -0700 Subject: [PATCH 3/5] fix(review): map backend SecretErrors to ExecutionFailed, not NotAuthorized MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address @serrrfirat review: Database, DecryptionFailed, KeychainError, and other backend errors were incorrectly mapped to "not found". Now: - NotFound → ToolError::NotAuthorized ("not found, configure via secrets set") - Expired → ToolError::NotAuthorized ("has expired, refresh or re-set") - All others → ToolError::ExecutionFailed (preserves real cause) Co-Authored-By: Claude Opus 4.6 (1M context) --- src/tools/wasm/wrapper.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/tools/wasm/wrapper.rs b/src/tools/wasm/wrapper.rs index 5e8a40de580..2f21f6a8c93 100644 --- a/src/tools/wasm/wrapper.rs +++ b/src/tools/wasm/wrapper.rs @@ -1547,15 +1547,21 @@ async fn resolve_host_credentials( // No cross-tenant fallback: each user must configure their own credentials. let secret = match store.get_decrypted(user_id, &mapping.secret_name).await { Ok(s) => s, + Err(crate::secrets::SecretError::NotFound(_)) => { + return Err(ToolError::NotAuthorized(format!( + "credential '{}' not found for user '{}'; configure it via `ironclaw secrets set`", + mapping.secret_name, user_id + ))); + } Err(crate::secrets::SecretError::Expired) => { return Err(ToolError::NotAuthorized(format!( "credential '{}' for user '{}' has expired; refresh or re-set via `ironclaw secrets set`", mapping.secret_name, user_id ))); } - Err(_) => { - return Err(ToolError::NotAuthorized(format!( - "credential '{}' not found for user '{}'; configure it via `ironclaw secrets set`", + Err(e) => { + return Err(ToolError::ExecutionFailed(format!( + "failed to resolve credential '{}' for user '{}': {e}", mapping.secret_name, user_id ))); } From 72899c16e165b4cad9d9d15f43bbf4f234f53794 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Tue, 7 Apr 2026 08:18:54 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix(review):=20AccessDenied=20=E2=86=92=20N?= =?UTF-8?q?otAuthorized,=20fix=20issue=20refs,=20reduce=20visibility?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Map SecretError::AccessDenied to ToolError::NotAuthorized (not ExecutionFailed) since it's an authorization failure - Update legacy fallback comments to reference #2100 (the tracking issue) instead of #2069 - Revert resolve_websocket_identify_message to private — test uses super:: import instead of pub(crate) path Co-Authored-By: Claude Opus 4.6 (1M context) --- src/channels/wasm/wrapper.rs | 13 +++++++------ src/tools/wasm/wrapper.rs | 6 ++++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/channels/wasm/wrapper.rs b/src/channels/wasm/wrapper.rs index e762978101c..4d73c31ee14 100644 --- a/src/channels/wasm/wrapper.rs +++ b/src/channels/wasm/wrapper.rs @@ -1033,7 +1033,7 @@ impl WasmChannel { /// Load broadcast metadata from settings store on startup. /// - /// # Legacy migration (remove after ownership model rollout — see #2069) + /// # Legacy migration (remove after ownership model rollout — tracked in #2100) /// /// If no metadata is found under `self.owner_scope_id`, a second lookup /// under `"default"` is attempted for backward compatibility with instances @@ -1054,7 +1054,7 @@ impl WasmChannel { ); } Ok(_) => { - // LEGACY MIGRATION: remove after ownership model rollout — see #2069 + // LEGACY MIGRATION: remove after ownership model rollout — tracked in #2100 if self.owner_scope_id != "default" { match store .get_setting("default", &self.broadcast_metadata_key()) @@ -3132,7 +3132,7 @@ fn websocket_processing_queue_path(channel_name: &str) -> String { format!("channels/{channel_name}/{WEBSOCKET_EVENT_PROCESSING_QUEUE_RELATIVE_PATH}") } -pub(crate) async fn resolve_websocket_identify_message( +async fn resolve_websocket_identify_message( config: &WebsocketRuntimeConfig, store: Option<&(dyn SecretsStore + Send + Sync)>, owner_scope_id: &str, @@ -4197,9 +4197,9 @@ mod tests { WebsocketRuntimeConfig, build_discord_gateway_presence_update, build_websocket_identify_message, build_websocket_resume_message, discord_gateway_presence_status, drain_guest_logs, parse_websocket_invalid_session, - parse_websocket_ready_session, resolve_websocket_identify_message, - should_warn_on_heartbeat_interval, uses_owner_broadcast_target, - websocket_heartbeat_sleep_duration, websocket_reconnect_backoff, + parse_websocket_ready_session, should_warn_on_heartbeat_interval, + uses_owner_broadcast_target, websocket_heartbeat_sleep_duration, + websocket_reconnect_backoff, }; use crate::pairing::PairingStore; use crate::testing::credentials::TEST_TELEGRAM_BOT_TOKEN; @@ -4312,6 +4312,7 @@ mod tests { /// not hardcoded "default". #[tokio::test] async fn test_resolve_websocket_identify_message_uses_owner_scope() { + use super::resolve_websocket_identify_message; use crate::secrets::SecretsStore; use crate::testing::credentials::test_secrets_store; diff --git a/src/tools/wasm/wrapper.rs b/src/tools/wasm/wrapper.rs index 2f21f6a8c93..1b962fca306 100644 --- a/src/tools/wasm/wrapper.rs +++ b/src/tools/wasm/wrapper.rs @@ -1559,6 +1559,12 @@ async fn resolve_host_credentials( mapping.secret_name, user_id ))); } + Err(crate::secrets::SecretError::AccessDenied) => { + return Err(ToolError::NotAuthorized(format!( + "access denied to credential '{}' for user '{}'", + mapping.secret_name, user_id + ))); + } Err(e) => { return Err(ToolError::ExecutionFailed(format!( "failed to resolve credential '{}' for user '{}': {e}", From bf6ad07011b111a024558bae1123c82be9294e2e Mon Sep 17 00:00:00 2001 From: Henry Park Date: Tue, 7 Apr 2026 09:39:04 -0700 Subject: [PATCH 5/5] fix(review): redact secrets in Debug impl, document all-or-nothing invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace #[derive(Debug)] on ResolvedHostCredential with custom impl that redacts secret_value and auth headers to prevent latent leakage - Add comment documenting that all declared non-UrlPath credentials are required — tool execution fails on first missing credential rather than running with partial auth Co-Authored-By: Claude Opus 4.6 (1M context) --- src/tools/wasm/wrapper.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/tools/wasm/wrapper.rs b/src/tools/wasm/wrapper.rs index 1b962fca306..c890d07a1a5 100644 --- a/src/tools/wasm/wrapper.rs +++ b/src/tools/wasm/wrapper.rs @@ -84,7 +84,6 @@ impl OAuthRefreshConfig { /// Built before each WASM execution by decrypting secrets from the store. /// Applied per-request by matching the URL host against `host_patterns`. /// WASM tools never see the raw secret values. -#[derive(Debug)] struct ResolvedHostCredential { /// Host patterns this credential applies to (e.g., "www.googleapis.com"). host_patterns: Vec, @@ -96,6 +95,22 @@ struct ResolvedHostCredential { secret_value: String, } +// Custom Debug impl to prevent accidental secret leakage in logs or panics. +// Headers contain auth tokens and secret_value is the raw decrypted secret. +impl std::fmt::Debug for ResolvedHostCredential { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ResolvedHostCredential") + .field("host_patterns", &self.host_patterns) + .field("headers", &format_args!("[{} entries]", self.headers.len())) + .field( + "query_params", + &format_args!("[{} entries]", self.query_params.len()), + ) + .field("secret_value", &"[REDACTED]") + .finish() + } +} + /// Store data for WASM tool execution. /// /// Contains the resource limiter, host state, WASI context, and injected @@ -1534,6 +1549,10 @@ async fn resolve_host_credentials( let mut resolved = Vec::new(); + // All declared non-UrlPath credentials are required. If any credential + // is missing, expired, or inaccessible, the entire tool execution fails + // rather than running with partial auth. This prevents silent 401s from + // confusing users. See #2099 discussion for rationale. for mapping in http_cap.credentials.values() { // Skip UrlPath credentials, they're handled by placeholder substitution if matches!(