fix(extensions): channel-relay auth dead-end, observability, and URL override (#1681)

* 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) <[email protected]> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * 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) <[email protected]> * style: collapse nested if per clippy::collapsible_if Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * 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) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
2026-08-31 16:49:34 +00:00 · 2026-03-26 13:49:05 -07:00
parent 9c63d189b7
commit adf4e25c8f
3 changed files with 500 additions and 39 deletions
@@ -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<String, RelayError> {
+        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<serde_json::Value, RelayError> {
+        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<Vec<u8>, 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()
@@ -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<String>,
 ) -> Result<Json<ActionResponse>, (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) => {