From adf4e25c8fdebeacb4bb99752861b61f556dc8db Mon Sep 17 00:00:00 2001 From: Pierre LE GUEN <26087574+PierreLeGuen@users.noreply.github.com> Date: Thu, 26 Mar 2026 15:49:05 -0500 Subject: [PATCH] fix(extensions): channel-relay auth dead-end, observability, and URL override (#1681) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(extensions): channel-relay auth dead-end, add observability and relay URL override Fix a bug where clicking Activate on the Slack relay extension produces a dead-end "Authentication required" error with no OAuth URL. The root cause: `auth_channel_relay()` used `is_relay_channel()` to check auth status, but that function returns true as soon as the extension is *installed* (in-memory set), before OAuth completes. This short-circuits the OAuth flow so the authorization URL is never offered. Changes: 1. **Bug fix** — `auth_channel_relay()` now uses `has_stored_team_id()` which only checks the persistent settings store for an actual team_id. The extension list `authenticated` field uses the same check so the UI accurately reflects OAuth completion status. 2. **Observability** — Added debug/warn/info tracing to all channel-relay code paths that were previously silent on failure: - `activate_channel_relay`: team_id retrieval, relay config, signing secret fetch, hot_add, cache operations - `auth_channel_relay`: auth check, OAuth initiation, nonce storage - `extensions_activate_handler`: request entry, auth fallback flow - `slack_relay_oauth_callback_handler`: team_id persistence (was silently ignored with `let _`) - `RelayClient`: initiate_oauth, get_signing_secret, proxy_provider all log URL, status, and errors - `has_stored_team_id`: store read success/failure 3. **Per-extension relay URL override** — Users can now override the CHANNEL_RELAY_URL via Settings > Extensions > Reconfigure. Stored under `extensions.{name}.relay_url` in settings. Both auth and activate read this override before falling back to the env default. Co-Authored-By: Claude Opus 4.6 (1M context) * style: cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) * fix: address review feedback — clear relay_url override and improve log message 1. Allow clearing the relay_url override: when an optional setup field with a setting_path is submitted empty, delete the stored setting so the system reverts to the env/default value. Previously empty values were silently skipped, making it impossible to undo an override from the UI. 2. Improve the OAuth callback team_id persistence error log to be self-contained without referencing implementation details. Co-Authored-By: Claude Opus 4.6 (1M context) * style: collapse nested if per clippy::collapsible_if Co-Authored-By: Claude Opus 4.6 (1M context) * fix: address review feedback — security, scope consistency, and error handling 1. OAuth callback team_id persistence is now fatal: if set_setting fails, the callback returns an error instead of proceeding to activate (which would re-read from the store and fail anyway). 2. effective_relay_url uses owner scope (self.user_id) for reads, matching configure() which writes under the same scope. Prevents multi-user mismatch where an override saved via Reconfigure was invisible during auth/activation. 3. has_stored_team_id uses owner scope for the same reason — the OAuth callback stores team_id under state.owner_id (= self.user_id). 4. Security: effective_relay_url validates the override URL — only http/https without embedded credentials (userinfo) is accepted. This prevents API-key exfiltration if a user points relay_url at an attacker-controlled host. Logs only host portion, not full URL. 5. Fixed effective_relay_url docstring to match behavior (returns Option, callers handle the fallback). 6. get_setup_schema for ChannelRelay now logs a warning on settings store errors instead of silently returning None. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/channels/relay/client.rs | 67 +++++- src/channels/web/server.rs | 53 ++++- src/extensions/manager.rs | 419 ++++++++++++++++++++++++++++++++--- 3 files changed, 500 insertions(+), 39 deletions(-) diff --git a/src/channels/relay/client.rs b/src/channels/relay/client.rs index 81fbb56c..b67f2c5e 100644 --- a/src/channels/relay/client.rs +++ b/src/channels/relay/client.rs @@ -122,18 +122,32 @@ impl RelayClient { /// instance_url in chat-api. IronClaw only passes an optional CSRF nonce /// for validating the callback — no URLs. pub async fn initiate_oauth(&self, state_nonce: Option<&str>) -> Result { + let url = format!("{}/oauth/slack/auth", self.base_url); + tracing::debug!(relay_url = %url, "RelayClient::initiate_oauth: sending request"); let mut query: Vec<(&str, &str)> = vec![]; if let Some(nonce) = state_nonce { query.push(("state_nonce", nonce)); } let resp = self .http - .get(format!("{}/oauth/slack/auth", self.base_url)) + .get(&url) .bearer_auth(self.api_key.expose_secret()) .query(&query) .send() .await - .map_err(|e| RelayError::Network(e.to_string()))?; + .map_err(|e| { + tracing::warn!( + relay_url = %url, + error = %e, + "RelayClient::initiate_oauth: network request failed" + ); + RelayError::Network(e.to_string()) + })?; + tracing::debug!( + relay_url = %url, + status = %resp.status(), + "RelayClient::initiate_oauth: received response" + ); let status = resp.status(); if status.is_redirection() { @@ -224,20 +238,39 @@ impl RelayClient { method: &str, body: serde_json::Value, ) -> Result { + let url = format!("{}/proxy/{}/{}", self.base_url, provider, method); + tracing::debug!( + relay_url = %url, + provider = %provider, + method = %method, + "RelayClient::proxy_provider: sending request" + ); let query: Vec<(&str, &str)> = vec![("team_id", team_id)]; let resp = self .http - .post(format!("{}/proxy/{}/{}", self.base_url, provider, method)) + .post(&url) .bearer_auth(self.api_key.expose_secret()) .query(&query) .json(&body) .send() .await - .map_err(|e| RelayError::Network(e.to_string()))?; + .map_err(|e| { + tracing::warn!( + relay_url = %url, + error = %e, + "RelayClient::proxy_provider: network request failed" + ); + RelayError::Network(e.to_string()) + })?; if !resp.status().is_success() { let status = resp.status().as_u16(); let body = resp.text().await.unwrap_or_default(); + tracing::warn!( + relay_url = %url, + status = status, + "RelayClient::proxy_provider: channel-relay returned error" + ); return Err(RelayError::Api { status, message: body, @@ -255,23 +288,45 @@ impl RelayClient { /// 32-byte secret. Called once at activation time; the result is cached in the /// extension manager so subsequent calls to `relay_signing_secret()` use it. pub async fn get_signing_secret(&self, team_id: &str) -> Result, RelayError> { + let url = format!("{}/relay/signing-secret", self.base_url); + tracing::debug!( + relay_url = %url, + "RelayClient::get_signing_secret: fetching signing secret" + ); let resp = self .http - .get(format!("{}/relay/signing-secret", self.base_url)) + .get(&url) .bearer_auth(self.api_key.expose_secret()) .query(&[("team_id", team_id)]) .send() .await - .map_err(|e| RelayError::Network(e.to_string()))?; + .map_err(|e| { + tracing::warn!( + relay_url = %url, + error = %e, + "RelayClient::get_signing_secret: network request failed" + ); + RelayError::Network(e.to_string()) + })?; if !resp.status().is_success() { let status = resp.status().as_u16(); let body = resp.text().await.unwrap_or_default(); + tracing::warn!( + relay_url = %url, + status = status, + body = %body, + "RelayClient::get_signing_secret: channel-relay returned error" + ); return Err(RelayError::Api { status, message: body, }); } + tracing::debug!( + relay_url = %url, + "RelayClient::get_signing_secret: received successful response" + ); let body: serde_json::Value = resp .json() diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index 4bf4de37..26c005d4 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -1177,11 +1177,31 @@ async fn slack_relay_oauth_callback_handler( // Store team_id in settings let team_id_key = format!("relay:{}:team_id", DEFAULT_RELAY_NAME); - let _ = store + tracing::info!( + relay = DEFAULT_RELAY_NAME, + owner_id = %state.owner_id, + team_id_key = %team_id_key, + "relay OAuth callback: storing team_id in settings" + ); + store .set_setting(&state.owner_id, &team_id_key, &serde_json::json!(team_id)) - .await; + .await + .map_err(|e| { + tracing::error!( + relay = DEFAULT_RELAY_NAME, + owner_id = %state.owner_id, + error = %e, + "relay OAuth callback: failed to persist team_id to settings store" + ); + format!("Failed to persist relay team_id: {e}") + })?; // Activate the relay channel + tracing::info!( + relay = DEFAULT_RELAY_NAME, + owner_id = %state.owner_id, + "relay OAuth callback: activating relay channel" + ); ext_mgr .activate_stored_relay(DEFAULT_RELAY_NAME, &state.owner_id) .await @@ -2181,6 +2201,11 @@ async fn extensions_activate_handler( AuthenticatedUser(user): AuthenticatedUser, Path(name): Path, ) -> Result, (StatusCode, String)> { + tracing::debug!( + extension = %name, + user_id = %user.user_id, + "extensions_activate_handler: received activate request" + ); let ext_mgr = state.extension_manager.as_ref().ok_or(( StatusCode::NOT_IMPLEMENTED, "Extension manager not available (secrets store required)".to_string(), @@ -2188,6 +2213,10 @@ async fn extensions_activate_handler( match ext_mgr.activate(&name, &user.user_id).await { Ok(result) => { + tracing::info!( + extension = %name, + "extensions_activate_handler: activation succeeded" + ); // Activation loaded the WASM module. Check if the tool needs // OAuth scope expansion (e.g., adding google-docs when gmail // already has a token but missing the documents scope). @@ -2206,6 +2235,13 @@ async fn extensions_activate_handler( crate::extensions::ExtensionError::AuthRequired ); + tracing::debug!( + extension = %name, + error = %activate_err, + needs_auth = needs_auth, + "extensions_activate_handler: activation failed, attempting auth fallback" + ); + if !needs_auth { return Ok(Json(ActionResponse::fail(activate_err.to_string()))); } @@ -2213,10 +2249,21 @@ async fn extensions_activate_handler( // Activation failed due to auth; try authenticating first. match ext_mgr.auth(&name, &user.user_id).await { Ok(auth_result) if auth_result.is_authenticated() => { + tracing::debug!( + extension = %name, + "extensions_activate_handler: auth reports authenticated, retrying activate" + ); // Auth succeeded, retry activation. match ext_mgr.activate(&name, &user.user_id).await { Ok(result) => Ok(Json(ActionResponse::ok(result.message))), - Err(e) => Ok(Json(ActionResponse::fail(e.to_string()))), + Err(e) => { + tracing::warn!( + extension = %name, + error = %e, + "extensions_activate_handler: retry after auth still failed" + ); + Ok(Json(ActionResponse::fail(e.to_string()))) + } } } Ok(auth_result) => { diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index 90920767..47b45a0f 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -659,6 +659,66 @@ impl ExtensionManager { }) } + /// Resolve the relay URL override for an extension from settings. + /// + /// Returns `Some(url)` if a non-empty per-extension `relay_url` override is + /// set for the given extension; otherwise returns `None` and callers should + /// fall back to the env-level `RelayConfig`. + /// + /// Uses `self.user_id` (owner scope) for consistency with `configure()`, + /// which also writes setting_path fields under the owner scope. + /// + /// The override is validated: only `http` / `https` schemes are accepted + /// and the URL must not contain userinfo (embedded credentials). This + /// prevents a malicious override from exfiltrating the instance-wide relay + /// API key to an attacker-controlled host. + async fn effective_relay_url(&self, name: &str) -> Option { + if let Some(ref store) = self.store { + let key = format!("extensions.{name}.relay_url"); + if let Ok(Some(v)) = store.get_setting(&self.user_id, &key).await { + let url = v + .as_str() + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()); + if let Some(ref u) = url { + // Validate the override to prevent API-key exfiltration: + // only allow http(s) with no embedded credentials. + match url::Url::parse(u) { + Ok(parsed) + if (parsed.scheme() == "http" || parsed.scheme() == "https") + && parsed.username().is_empty() + && parsed.password().is_none() => + { + tracing::debug!( + extension = %name, + relay_url_host = %parsed.host_str().unwrap_or("unknown"), + "effective_relay_url: using per-extension override from settings" + ); + return url; + } + Ok(parsed) => { + tracing::warn!( + extension = %name, + scheme = %parsed.scheme(), + has_userinfo = !parsed.username().is_empty() || parsed.password().is_some(), + "effective_relay_url: rejecting override — \ + only http/https without embedded credentials is allowed" + ); + } + Err(e) => { + tracing::warn!( + extension = %name, + error = %e, + "effective_relay_url: rejecting override — invalid URL" + ); + } + } + } + } + } + None + } + /// Get the shared relay event sender for the webhook endpoint. pub fn relay_event_tx( &self, @@ -892,6 +952,46 @@ impl ExtensionManager { false } + /// Check whether a stored `team_id` setting exists for the given relay extension. + /// + /// Unlike [`is_relay_channel`], this does **not** consult the in-memory + /// `installed_relay_extensions` set — it only looks at the persistent settings + /// store. This distinction matters for `auth_channel_relay`: an extension can + /// be *installed* (present in the in-memory set) but not yet *authenticated* + /// (no OAuth completed, no team_id stored). + async fn has_stored_team_id(&self, name: &str, _user_id: &str) -> bool { + if let Some(ref store) = self.store { + let key = format!("relay:{}:team_id", name); + // Use owner scope (self.user_id) for consistency: the OAuth callback + // stores team_id under state.owner_id which maps to self.user_id. + match store.get_setting(&self.user_id, &key).await { + Ok(Some(v)) => { + let has_id = v.as_str().is_some_and(|s| !s.is_empty()); + tracing::debug!( + extension = %name, + has_team_id = has_id, + "has_stored_team_id: checked store" + ); + return has_id; + } + Ok(None) => { + tracing::debug!( + extension = %name, + "has_stored_team_id: no team_id setting found" + ); + } + Err(e) => { + tracing::warn!( + extension = %name, + error = %e, + "has_stored_team_id: failed to read from settings store" + ); + } + } + } + false + } + /// Restore persisted relay channels after startup. /// /// Loads the persisted active channel list, filters to relay types (those with @@ -1418,7 +1518,7 @@ impl ExtensionManager { let errors = self.activation_errors.read().await; for name in installed.iter() { let active = active_names.contains(name); - let authenticated = self.is_relay_channel(name, user_id).await; + let authenticated = self.has_stored_team_id(name, user_id).await; let activation_error = errors.get(name).cloned(); let registry_entry = self .registry @@ -4191,20 +4291,69 @@ impl ExtensionManager { name: &str, user_id: &str, ) -> Result { - // Check if already authenticated (team_id setting exists) - if self.is_relay_channel(name, user_id).await { + tracing::debug!( + extension = %name, + user_id = %user_id, + "auth_channel_relay: starting" + ); + + // Check if already authenticated by looking for a stored team_id. + // We intentionally skip the `installed_relay_extensions` in-memory set + // here because that set only tracks *installed* extensions — an extension + // can be installed (via registry) but not yet authenticated (no OAuth + // completed). Checking just `is_relay_channel()` would short-circuit + // to "authenticated" even when no team_id exists, preventing the OAuth + // flow from being offered to the user. + if self.has_stored_team_id(name, user_id).await { + tracing::debug!( + extension = %name, + "auth_channel_relay: already authenticated (team_id in store)" + ); return Ok(AuthResult::authenticated(name, ExtensionKind::ChannelRelay)); } + tracing::debug!( + extension = %name, + "auth_channel_relay: no stored team_id, initiating OAuth" + ); + // Use relay config captured at startup - let relay_config = self.relay_config()?; + let relay_config = self.relay_config().map_err(|e| { + tracing::warn!( + extension = %name, + error = %e, + "auth_channel_relay: relay config not available — \ + CHANNEL_RELAY_URL and CHANNEL_RELAY_API_KEY must be set" + ); + e + })?; + + // Allow per-extension URL override from settings + let effective_url = self + .effective_relay_url(name) + .await + .unwrap_or_else(|| relay_config.url.clone()); + + tracing::debug!( + extension = %name, + relay_url = %effective_url, + "auth_channel_relay: creating relay client for OAuth" + ); let client = crate::channels::relay::RelayClient::new( - relay_config.url.clone(), + effective_url.clone(), relay_config.api_key.clone(), relay_config.request_timeout_secs, ) - .map_err(|e| ExtensionError::Config(e.to_string()))?; + .map_err(|e| { + tracing::warn!( + extension = %name, + relay_url = %effective_url, + error = %e, + "auth_channel_relay: failed to create relay HTTP client" + ); + ExtensionError::Config(e.to_string()) + })?; // Generate CSRF nonce — IronClaw validates this on the callback to ensure // the OAuth completion is legitimate. Channel-relay embeds it in the signed @@ -4216,18 +4365,44 @@ impl ExtensionManager { self.secrets .create(user_id, CreateSecretParams::new(&state_key, &state_nonce)) .await - .map_err(|e| ExtensionError::AuthFailed(format!("Failed to store OAuth state: {e}")))?; + .map_err(|e| { + tracing::warn!( + extension = %name, + error = %e, + "auth_channel_relay: failed to store OAuth state nonce" + ); + ExtensionError::AuthFailed(format!("Failed to store OAuth state: {e}")) + })?; // Channel-relay derives all URLs from trusted instance_url in chat-api. // We only pass the nonce for CSRF validation on the callback. + tracing::debug!( + extension = %name, + relay_url = %effective_url, + "auth_channel_relay: calling initiate_oauth on channel-relay" + ); match client.initiate_oauth(Some(&state_nonce)).await { - Ok(auth_url) => Ok(AuthResult::awaiting_authorization( - name, - ExtensionKind::ChannelRelay, - auth_url, - "redirect".to_string(), - )), - Err(e) => Err(ExtensionError::AuthFailed(e.to_string())), + Ok(auth_url) => { + tracing::info!( + extension = %name, + "auth_channel_relay: OAuth URL obtained, awaiting user authorization" + ); + Ok(AuthResult::awaiting_authorization( + name, + ExtensionKind::ChannelRelay, + auth_url, + "redirect".to_string(), + )) + } + Err(e) => { + tracing::warn!( + extension = %name, + relay_url = %effective_url, + error = %e, + "auth_channel_relay: initiate_oauth call to channel-relay failed" + ); + Err(ExtensionError::AuthFailed(e.to_string())) + } } } @@ -4237,40 +4412,112 @@ impl ExtensionManager { name: &str, user_id: &str, ) -> Result { + tracing::debug!( + extension = %name, + user_id = %user_id, + "activate_channel_relay: starting" + ); + let team_id_key = format!("relay:{}:team_id", name); // Get team_id from settings (stored by the OAuth callback) let team_id = if let Some(ref store) = self.store { - store - .get_setting(user_id, &team_id_key) - .await - .ok() - .flatten() - .and_then(|v| v.as_str().map(|s| s.to_string())) - .unwrap_or_default() + match store.get_setting(user_id, &team_id_key).await { + Ok(Some(v)) => { + let id = v.as_str().map(|s| s.to_string()).unwrap_or_default(); + tracing::debug!( + extension = %name, + team_id_empty = id.is_empty(), + "activate_channel_relay: loaded team_id from store" + ); + id + } + Ok(None) => { + tracing::debug!( + extension = %name, + setting_key = %team_id_key, + "activate_channel_relay: no team_id in settings store" + ); + String::new() + } + Err(e) => { + tracing::warn!( + extension = %name, + error = %e, + "activate_channel_relay: failed to read team_id from settings store" + ); + String::new() + } + } } else { + tracing::debug!( + extension = %name, + "activate_channel_relay: no settings store available" + ); String::new() }; if team_id.is_empty() { + tracing::debug!( + extension = %name, + "activate_channel_relay: team_id is empty, returning AuthRequired" + ); return Err(ExtensionError::AuthRequired); } // Use relay config captured at startup - let relay_config = self.relay_config()?; + let relay_config = self.relay_config().map_err(|e| { + tracing::warn!( + extension = %name, + error = %e, + "activate_channel_relay: relay config not available" + ); + e + })?; + + // Allow per-extension URL override from settings + let effective_url = self + .effective_relay_url(name) + .await + .unwrap_or_else(|| relay_config.url.clone()); + + tracing::debug!( + extension = %name, + relay_url = %effective_url, + "activate_channel_relay: relay config loaded" + ); let instance_id = self.relay_instance_id(relay_config, user_id); let client = crate::channels::relay::RelayClient::new( - relay_config.url.clone(), + effective_url.clone(), relay_config.api_key.clone(), relay_config.request_timeout_secs, ) - .map_err(|e| ExtensionError::ActivationFailed(e.to_string()))?; + .map_err(|e| { + tracing::warn!( + extension = %name, + relay_url = %effective_url, + error = %e, + "activate_channel_relay: failed to create relay HTTP client" + ); + ExtensionError::ActivationFailed(e.to_string()) + })?; // Fetch the per-instance signing secret from channel-relay. // This must succeed — there is no fallback. + tracing::debug!( + extension = %name, + relay_url = %effective_url, + "activate_channel_relay: fetching signing secret from channel-relay" + ); let signing_secret = client.get_signing_secret(&team_id).await.map_err(|e| { + tracing::warn!( + extension = %name, + relay_url = %effective_url, + error = %e, + "activate_channel_relay: failed to fetch signing secret from channel-relay" + ); ExtensionError::Config(format!("Failed to fetch relay signing secret: {e}")) })?; @@ -4289,16 +4536,29 @@ impl ExtensionManager { // Hot-add to channel manager let cm_guard = self.relay_channel_manager.read().await; let channel_mgr = cm_guard.as_ref().ok_or_else(|| { + tracing::warn!( + extension = %name, + "activate_channel_relay: channel manager not initialized" + ); ExtensionError::ActivationFailed("Channel manager not initialized".to_string()) })?; - channel_mgr - .hot_add(Box::new(channel)) - .await - .map_err(|e| ExtensionError::ActivationFailed(e.to_string()))?; + channel_mgr.hot_add(Box::new(channel)).await.map_err(|e| { + tracing::warn!( + extension = %name, + error = %e, + "activate_channel_relay: hot_add to channel manager failed" + ); + ExtensionError::ActivationFailed(e.to_string()) + })?; if let Ok(mut cache) = self.relay_signing_secret_cache.lock() { *cache = Some(signing_secret); + } else { + tracing::warn!( + extension = %name, + "activate_channel_relay: failed to cache signing secret (mutex poisoned)" + ); } // Store the event sender so the web gateway's relay webhook endpoint can push events @@ -4316,6 +4576,12 @@ impl ExtensionManager { self.broadcast_extension_status(name, "active", Some(&status_msg)) .await; + tracing::info!( + extension = %name, + instance_id = %instance_id, + "activate_channel_relay: relay channel activated successfully" + ); + Ok(ActivateResult { name: name.to_string(), kind: ExtensionKind::ChannelRelay, @@ -4595,6 +4861,41 @@ impl ExtensionManager { } Ok(ExtensionSetupSchema { secrets, fields }) } + ExtensionKind::ChannelRelay => { + let relay_url_key = format!("extensions.{name}.relay_url"); + let current_url = if let Some(ref store) = self.store { + match store.get_setting(&self.user_id, &relay_url_key).await { + Ok(value_opt) => value_opt + .and_then(|v| v.as_str().map(|s| s.to_string())) + .filter(|s| !s.is_empty()), + Err(e) => { + tracing::warn!( + extension = %name, + setting_key = %relay_url_key, + error = %e, + "get_setup_schema: failed to read relay_url from settings" + ); + None + } + } + } else { + None + }; + let env_url = self.relay_config.as_ref().map(|c| c.url.as_str()); + Ok(ExtensionSetupSchema { + secrets: Vec::new(), + fields: vec![crate::channels::web::types::SetupFieldInfo { + name: "relay_url".to_string(), + prompt: format!( + "Channel-relay service URL (leave empty to use env default{})", + env_url.map(|u| format!(": {u}")).unwrap_or_default() + ), + optional: true, + provided: current_url.is_some(), + input_type: crate::tools::wasm::ToolSetupFieldInputType::Text, + }], + }) + } _ => Ok(ExtensionSetupSchema { secrets: Vec::new(), fields: Vec::new(), @@ -4997,7 +5298,17 @@ impl ExtensionManager { names.insert(server.token_secret_name()); (names, Vec::new()) } - ExtensionKind::ChannelRelay => (std::collections::HashSet::new(), Vec::new()), + ExtensionKind::ChannelRelay => { + let relay_fields = vec![crate::tools::wasm::ToolFieldSetupSchema { + name: "relay_url".to_string(), + prompt: "Channel-relay service URL override".to_string(), + optional: true, + setting_path: Some(format!("extensions.{name}.relay_url")), + input_type: crate::tools::wasm::ToolSetupFieldInputType::Text, + restart_required: false, + }]; + (std::collections::HashSet::new(), relay_fields) + } }; let allowed_fields: std::collections::HashSet = @@ -5088,13 +5399,28 @@ impl ExtensionManager { ))); } let trimmed = field_value.trim(); + let field_def = setup_field_defs.get(field_name); + + // Empty value on an optional field with a setting_path: clear the + // stored override so the system reverts to the env/default value. if trimmed.is_empty() { + if let Some(def) = field_def + && def.optional + { + stored_fields.remove(field_name); + if let Some(setting_path) = &def.setting_path { + Self::validate_setup_setting_path(name, setting_path)?; + if let Some(store) = self.store.as_ref() { + let _ = store.delete_setting(&self.user_id, setting_path).await; + } + } + } continue; } stored_fields.insert(field_name.clone(), trimmed.to_string()); - if let Some(field_def) = setup_field_defs.get(field_name) { + if let Some(field_def) = field_def { if field_def.restart_required { restart_required = true; } @@ -7058,6 +7384,39 @@ mod tests { ); } + /// Regression: installed-but-not-authenticated relay must NOT short-circuit + /// `auth_channel_relay()` to "authenticated". Previously, `auth_channel_relay` + /// called `is_relay_channel()` which checked the in-memory + /// `installed_relay_extensions` set; that returned `true` even when no team_id + /// existed in the store, so the OAuth URL was never offered. + #[tokio::test] + async fn test_auth_channel_relay_installed_without_team_id_is_not_authenticated() { + let dir = tempfile::tempdir().expect("temp dir"); + let mgr = make_test_manager(None, dir.path().to_path_buf()); + + // Mark as installed (simulates clicking Install in the UI) + mgr.installed_relay_extensions + .write() + .await + .insert("slack-relay".to_string()); + + // Without a stored team_id, auth should NOT return authenticated. + // It should fail because relay config is missing (no CHANNEL_RELAY_URL), + // but the key assertion is that it does NOT return Ok(authenticated). + let result = mgr.auth_channel_relay("slack-relay", "test").await; + match result { + Ok(ref auth_result) if auth_result.is_authenticated() => { + panic!( + "auth_channel_relay returned authenticated for installed-but-no-team-id relay; \ + expected either an OAuth URL or a config error" + ); + } + _ => { + // Config error (no relay URL) or awaiting_authorization — both are correct + } + } + } + #[tokio::test] async fn test_remove_relay_shuts_down_via_relay_channel_manager() { // Regression: remove() only checked channel_runtime for shutdown, missing