diff --git a/Cargo.lock b/Cargo.lock index aac3921eee..6d188d30c5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4056,6 +4056,7 @@ dependencies = [ "arboard", "chrono", "image", + "ironclaw_common", "pulldown-cmark", "ratatui", "serde", diff --git a/crates/ironclaw_common/src/event.rs b/crates/ironclaw_common/src/event.rs index 859f97e309..473b6c95a5 100644 --- a/crates/ironclaw_common/src/event.rs +++ b/crates/ironclaw_common/src/event.rs @@ -5,6 +5,7 @@ //! frames, but other subsystems (agent loop, orchestrator, extensions) //! produce and consume them too. +use crate::identity::ExtensionName; use serde::{Deserialize, Serialize}; /// A single step in a plan progress update (SSE DTO). @@ -64,7 +65,7 @@ impl OnboardingStateDto { /// post-pairing) from silently disagreeing when new fields land on /// `AppEvent::OnboardingState`. pub fn pairing_required( - extension_name: impl Into, + extension_name: ExtensionName, request_id: Option, thread_id: Option, message: Option, @@ -72,7 +73,7 @@ impl OnboardingStateDto { onboarding: Option, ) -> AppEvent { AppEvent::OnboardingState { - extension_name: extension_name.into(), + extension_name, state: Self::PairingRequired, request_id, message, @@ -161,7 +162,7 @@ pub enum AppEvent { }, #[serde(rename = "onboarding_state")] OnboardingState { - extension_name: String, + extension_name: ExtensionName, state: OnboardingStateDto, #[serde(skip_serializing_if = "Option::is_none")] request_id: Option, @@ -186,7 +187,7 @@ pub enum AppEvent { description: String, parameters: String, #[serde(skip_serializing_if = "Option::is_none")] - extension_name: Option, + extension_name: Option, resume_kind: serde_json::Value, #[serde(skip_serializing_if = "Option::is_none")] thread_id: Option, @@ -281,7 +282,7 @@ pub enum AppEvent { /// Extension activation status change (WASM channels). #[serde(rename = "extension_status")] ExtensionStatus { - extension_name: String, + extension_name: ExtensionName, status: String, #[serde(skip_serializing_if = "Option::is_none")] message: Option, @@ -453,7 +454,7 @@ mod tests { allow_always: false, }, AppEvent::OnboardingState { - extension_name: String::new(), + extension_name: ExtensionName::from_trusted(String::new()), state: OnboardingStateDto::AuthRequired, request_id: None, message: None, @@ -524,7 +525,7 @@ mod tests { thread_id: None, }, AppEvent::ExtensionStatus { - extension_name: String::new(), + extension_name: ExtensionName::from_trusted(String::new()), status: String::new(), message: None, }, @@ -579,7 +580,7 @@ mod tests { #[test] fn pairing_required_constructor_sets_invariant_fields() { let event = OnboardingStateDto::pairing_required( - "telegram", + ExtensionName::new("telegram").unwrap(), Some("req-1".to_string()), Some("thread-1".to_string()), Some("Paired!".to_string()), @@ -618,7 +619,14 @@ mod tests { #[test] fn pairing_required_constructor_serializes_to_onboarding_state_event() { - let event = OnboardingStateDto::pairing_required("telegram", None, None, None, None, None); + let event = OnboardingStateDto::pairing_required( + ExtensionName::new("telegram").unwrap(), + None, + None, + None, + None, + None, + ); let json = serde_json::to_value(&event).unwrap(); assert_eq!(json["type"], "onboarding_state"); assert_eq!(json["state"], "pairing_required"); diff --git a/crates/ironclaw_tui/Cargo.toml b/crates/ironclaw_tui/Cargo.toml index df6289548b..a8c624942a 100644 --- a/crates/ironclaw_tui/Cargo.toml +++ b/crates/ironclaw_tui/Cargo.toml @@ -15,6 +15,7 @@ default = ["clipboard"] clipboard = ["dep:arboard", "dep:image"] [dependencies] +ironclaw_common = { path = "../ironclaw_common", version = "0.2.0" } ratatui = { version = "0.29", features = ["crossterm"] } tui-textarea = { version = "0.7", features = ["crossterm"] } serde = { version = "1", features = ["derive"] } diff --git a/crates/ironclaw_tui/src/event.rs b/crates/ironclaw_tui/src/event.rs index f6c28b59a4..a27165efbe 100644 --- a/crates/ironclaw_tui/src/event.rs +++ b/crates/ironclaw_tui/src/event.rs @@ -6,6 +6,7 @@ use std::collections::VecDeque; +use ironclaw_common::ExtensionName; use ratatui::crossterm::event::KeyEvent; /// A single log entry displayed in the TUI Logs tab. @@ -280,13 +281,13 @@ pub enum TuiEvent { /// Extension needs user authentication. AuthRequired { - extension_name: String, + extension_name: ExtensionName, instructions: Option, }, /// Extension auth completed. AuthCompleted { - extension_name: String, + extension_name: ExtensionName, success: bool, message: String, }, diff --git a/scripts/pre-commit-safety.sh b/scripts/pre-commit-safety.sh index 5a48dafa7e..5c9fbb6324 100755 --- a/scripts/pre-commit-safety.sh +++ b/scripts/pre-commit-safety.sh @@ -12,12 +12,14 @@ # 5. Multi-step DB operations without transaction wrapping # 6. .unwrap(), .expect(), assert!() in production code (panics) # 7. Gateway/CLI handlers bypassing ToolDispatcher (must go through tools) +# 8. CredentialName referenced in web-layer code (wrong identity at boundary) # # Also runs check-i18n-parity.sh when crates/ironclaw_gateway/static/i18n/*.js # files are staged, to ensure every language pack has the same key set. # # Suppress individual lines with an inline "// safety: " comment. # For check #7, use "// dispatch-exempt: " instead. +# For check #8, use "// web-identity-exempt: " instead. set -euo pipefail @@ -333,10 +335,41 @@ if [ -n "$DISPATCH_DIFF" ]; then fi fi +# 8. CredentialName referenced in web-layer code. +# CredentialName is a backend/secrets-store identity. Web routes and +# web DTOs take ExtensionName; the dispatcher and auth_manager resolve +# credential identity from the extension name server-side. An explicit +# `CredentialName` reference in src/channels/web/** (except inside +# `#[cfg(test)] mod tests` blocks) means the wrong identity is reaching +# the web boundary. See src/channels/web/CLAUDE.md "Identity types at +# the web boundary" and .claude/rules/types.md. +# +# Suppress with "// web-identity-exempt: " when the reference +# is genuinely reading an already-typed value off a backend struct +# (e.g., destructuring `ResumeKind::Authentication` to log the name). +WEB_IDENTITY_DIFF=$(git diff --cached -U0 -- 'src/channels/web/*.rs' 'src/channels/web/**/*.rs' 2>/dev/null || true) +if [ -z "$WEB_IDENTITY_DIFF" ]; then + WEB_IDENTITY_DIFF=$(git diff "$(resolve_base_ref)" -U0 -- 'src/channels/web/*.rs' 'src/channels/web/**/*.rs' 2>/dev/null || true) +fi +if [ -n "$WEB_IDENTITY_DIFF" ]; then + # Strip lines inside `#[cfg(test)] mod tests` blocks using the same + # precomputed boundaries used for other prod-only checks. + WEB_IDENTITY_PROD=$(printf '%s\n' "$WEB_IDENTITY_DIFF" | strip_test_mod_lines) + WEB_IDENTITY_HITS=$(echo "$WEB_IDENTITY_PROD" | grep -nE '^\+' \ + | grep -E '\bCredentialName\b' \ + | grep -vE '// web-identity-exempt:|// safety:|^\+\+\+' \ + | head -5 || true) + if [ -n "$WEB_IDENTITY_HITS" ]; then + warn "CREDNAME" "\`CredentialName\` referenced in src/channels/web/** — web code takes \`ExtensionName\`; credential identity stays backend-side. Push the mapping into bridge::auth_manager or annotate with '// web-identity-exempt: '." + echo "$WEB_IDENTITY_HITS" | sed 's/^/ /' + fi +fi + if [ "$WARNINGS" -gt 0 ]; then echo "" echo "Found $WARNINGS potential issue(s). Fix them or add '// safety: ' to suppress." echo "(For DISPATCH warnings, use '// dispatch-exempt: ' instead.)" + echo "(For CREDNAME warnings, use '// web-identity-exempt: ' instead.)" echo "" exit 1 fi diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index da961ff134..8185fb3286 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -15,6 +15,7 @@ use crate::channels::{ChannelManager, IncomingMessage, StatusUpdate}; use crate::context::JobContext; use crate::error::Error; use async_trait::async_trait; +use ironclaw_common::ExtensionName; use crate::agent::agentic_loop::{ AgenticLoopConfig, LoopDelegate, LoopOutcome, LoopSignal, TextAction, @@ -1086,7 +1087,7 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { } // === Phase 3: Post-flight (sequential, in original order) === - let mut selected_auth_prompt: Option<(String, ParsedAuthData)> = None; + let mut selected_auth_prompt: Option<(ExtensionName, ParsedAuthData)> = None; let mut tool_failure_count: usize = 0; let total_tools = preflight.len(); @@ -1334,7 +1335,7 @@ pub(super) async fn execute_chat_tool_standalone( /// Parsed auth result fields for emitting StatusUpdate::AuthRequired. #[derive(Debug, Clone, PartialEq, Eq)] pub(super) struct ParsedAuthData { - pub(super) extension_name: Option, + pub(super) extension_name: Option, pub(super) instructions: Option, pub(super) auth_url: Option, pub(super) setup_url: Option, @@ -1343,11 +1344,8 @@ pub(super) struct ParsedAuthData { const DEFAULT_AUTH_TOKEN_INSTRUCTIONS: &str = "Please provide your API token/key."; -fn normalize_extension_name(value: Option<&str>) -> Option { - value - .map(str::trim) - .filter(|value| !value.is_empty()) - .map(ToOwned::to_owned) +fn normalize_extension_name(value: Option<&str>) -> Option { + value.and_then(|raw| ExtensionName::new(raw).ok()) } pub(super) use crate::auth::oauth::sanitize_auth_url; @@ -1359,9 +1357,9 @@ pub(super) fn auth_instructions_or_default(instructions: Option<&str>) -> String } pub(super) fn persist_selected_auth_prompt( - selected: Option<&(String, ParsedAuthData)>, + selected: Option<&(ExtensionName, ParsedAuthData)>, ) -> Option { - selected.and_then(|(extension_name, auth_data)| { + selected.map(|(extension_name, auth_data)| { PendingAuthPrompt::new( extension_name.clone(), auth_data.instructions.clone(), @@ -1374,25 +1372,33 @@ pub(super) fn persist_selected_auth_prompt( pub(super) fn restore_selected_auth_prompt( pending: Option, -) -> Option<(String, ParsedAuthData)> { - // Re-validate via the constructor so deserialized rows go through the - // same trim/non-empty invariant as freshly constructed prompts. +) -> Option<(ExtensionName, ParsedAuthData)> { let pending = pending?; - let validated = PendingAuthPrompt::new( - pending.extension_name, - pending.instructions, - pending.auth_url, - pending.setup_url, - pending.awaiting_token, - )?; + // The deserialized `PendingAuthPrompt.extension_name` is `#[serde(transparent)]`, + // which does not re-validate the inner string. Re-validate on restore so a + // legacy-persisted invalid name (empty, uppercase, path separator, etc.) + // drops the prompt instead of propagating through the auth-card path. + // Pre-PR2 the equivalent `PendingAuthPrompt::new` rejected empty strings; + // this upgrade extends that rejection to the full identity invariant. + let extension_name = match ExtensionName::new(pending.extension_name.as_str()) { + Ok(name) => name, + Err(error) => { + tracing::warn!( + raw = %pending.extension_name, + %error, + "Dropping restored PendingAuthPrompt whose extension_name no longer satisfies the identity rule" + ); + return None; + } + }; Some(( - validated.extension_name.clone(), + extension_name.clone(), ParsedAuthData { - extension_name: Some(validated.extension_name), - instructions: validated.instructions, - auth_url: validated.auth_url, - setup_url: validated.setup_url, - awaiting_token: validated.awaiting_token, + extension_name: Some(extension_name), + instructions: pending.instructions, + auth_url: pending.auth_url, + setup_url: pending.setup_url, + awaiting_token: pending.awaiting_token, }, )) } @@ -1461,7 +1467,7 @@ pub(super) fn extract_auth_prompt( pub(super) async fn emit_auth_required_status( channels: &ChannelManager, message: &IncomingMessage, - extension_name: String, + extension_name: ExtensionName, instructions: Option, auth_url: Option, setup_url: Option, @@ -1484,7 +1490,7 @@ pub(super) async fn emit_auth_required_status( /// Keep only the first actionable auth prompt seen in a turn. pub(super) fn capture_auth_prompt( - selected: &mut Option<(String, ParsedAuthData)>, + selected: &mut Option<(ExtensionName, ParsedAuthData)>, tool_name: &str, result: &Result, ) { @@ -1508,7 +1514,7 @@ pub(super) fn capture_auth_prompt( pub(super) fn check_auth_required( tool_name: &str, result: &Result, -) -> Option<(String, String)> { +) -> Option<(ExtensionName, String)> { let auth_data = extract_auth_prompt(tool_name, result)?; if !auth_data.awaiting_token { return None; @@ -1768,6 +1774,7 @@ mod tests { }; use crate::agent::session::PendingAuthPrompt; use crate::generated_images::GeneratedImageSentinel; + use ironclaw_common::ExtensionName; /// Minimal LLM provider for unit tests that always returns a static response. struct StaticLlmProvider; @@ -2265,13 +2272,13 @@ mod tests { reasoning: None, }, ], - selected_auth_prompt: Some(crate::agent::session::PendingAuthPrompt { - extension_name: "gmail".to_string(), - instructions: Some("Authorize Gmail".to_string()), - auth_url: Some("https://example.com/oauth".to_string()), - setup_url: None, - awaiting_token: false, - }), + selected_auth_prompt: Some(crate::agent::session::PendingAuthPrompt::new( + ExtensionName::new("gmail").unwrap(), + Some("Authorize Gmail".to_string()), + Some("https://example.com/oauth".to_string()), + None, + false, + )), user_timezone: None, allow_always: true, }; @@ -2413,17 +2420,36 @@ mod tests { ); } - #[test] - fn test_restore_selected_auth_prompt_rejects_blank_extension_name() { - let pending = PendingAuthPrompt { - extension_name: " ".to_string(), - instructions: Some("Connect Gmail".to_string()), - auth_url: Some("https://accounts.google.com/o/oauth2/auth".to_string()), - setup_url: None, - awaiting_token: false, - }; + // Note: `PendingAuthPrompt` now carries an `ExtensionName` that carries the + // non-empty invariant itself. The "blank extension name" rejection case + // lives in `ironclaw_common::identity` tests; there is no intermediate + // stringly-typed rejection path in the prompt layer anymore. - assert!(restore_selected_auth_prompt(Some(pending)).is_none()); + /// Regression for PR #2617 Copilot review: `PendingAuthPrompt` is + /// `#[serde(transparent)]`, so deserialization does not re-validate the + /// inner `ExtensionName` string. A legacy-persisted row holding an + /// invalid identity (empty, uppercase, path-separator, etc.) must be + /// rejected by `restore_selected_auth_prompt` rather than propagating + /// as a typed extension name. Mirrors the pre-PR2 behaviour where the + /// old string-trim constructor returned `None` on invalid input. + #[test] + fn test_restore_selected_auth_prompt_rejects_invalid_legacy_row() { + // Forge a legacy prompt by deserializing an invalid extension_name + // straight through serde — bypasses the normal `::new` entry point + // and simulates a bad row in `pending_gates.json`. + let bad_rows = [ + r#"{"extension_name":"","instructions":null,"auth_url":null,"setup_url":null,"awaiting_token":false}"#, + r#"{"extension_name":"Bad__Case","instructions":null,"auth_url":null,"setup_url":null,"awaiting_token":false}"#, + r#"{"extension_name":"../traversal","instructions":null,"auth_url":null,"setup_url":null,"awaiting_token":false}"#, + ]; + for raw in bad_rows { + let legacy: PendingAuthPrompt = + serde_json::from_str(raw).expect("serde transparent accepts any string"); + assert!( + restore_selected_auth_prompt(Some(legacy)).is_none(), + "legacy row {raw:?} should be dropped on restore" + ); + } } #[test] @@ -2468,7 +2494,10 @@ mod tests { .to_string()); let auth_data = extract_auth_prompt("tool_activate", &result).expect("auth prompt"); - assert_eq!(auth_data.extension_name.as_deref(), Some("gmail")); + assert_eq!( + auth_data.extension_name.as_ref().map(|e| e.as_str()), + Some("gmail") + ); assert_eq!( auth_data.auth_url.as_deref(), Some("https://accounts.google.com/o/oauth2/v2/auth?client_id=test") @@ -2515,39 +2544,23 @@ mod tests { #[test] fn test_pending_auth_prompt_new_rejects_empty_name() { - assert!( - PendingAuthPrompt::new( - "".to_string(), - None, - Some("https://example.com".to_string()), - None, - false, - ) - .is_none() - ); - assert!( - PendingAuthPrompt::new( - " ".to_string(), - None, - Some("https://example.com".to_string()), - None, - false, - ) - .is_none() - ); + // Empty / whitespace extension names are rejected by the identity + // validator; `PendingAuthPrompt::new` itself is now infallible and + // only accepts an already-validated `ExtensionName`. + assert!(ExtensionName::new("").is_err()); + assert!(ExtensionName::new(" ").is_err()); } #[test] fn test_pending_auth_prompt_new_accepts_valid_name() { let prompt = PendingAuthPrompt::new( - "gmail".to_string(), + ExtensionName::new("gmail").unwrap(), None, Some("https://example.com".to_string()), None, false, ); - assert!(prompt.is_some()); - assert_eq!(prompt.unwrap().extension_name, "gmail"); + assert_eq!(prompt.extension_name.as_str(), "gmail"); } #[test] diff --git a/src/agent/session.rs b/src/agent/session.rs index c3fbbdf272..2e6c92ec00 100644 --- a/src/agent/session.rs +++ b/src/agent/session.rs @@ -18,7 +18,7 @@ use uuid::Uuid; use crate::generated_images::GeneratedImageSentinel; use crate::llm::{ChatMessage, ToolCall, generate_tool_call_id}; -use ironclaw_common::truncate_preview; +use ironclaw_common::{ExtensionName, truncate_preview}; /// A session containing one or more threads. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -172,7 +172,7 @@ const AUTH_MODE_TTL: TimeDelta = TimeDelta::seconds(AUTH_MODE_TTL_SECS); #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PendingAuth { /// Extension name to authenticate. - pub extension_name: String, + pub extension_name: ExtensionName, /// When this auth mode was entered. Used for TTL expiry. #[serde(default = "Utc::now")] pub created_at: DateTime, @@ -188,13 +188,22 @@ impl PendingAuth { /// Auth prompt captured during a tool turn and persisted if that turn pauses /// for approval before the prompt can be surfaced to the user. /// -/// Callers should use [`PendingAuthPrompt::new()`] which trims and validates -/// that `extension_name` is non-empty. Fields are `pub(crate)` so external -/// callers cannot bypass the constructor; serde still round-trips them. +/// Fields are `pub(crate)` so external callers cannot bypass the constructor; +/// serde still round-trips them. Use [`PendingAuthPrompt::new`] to construct +/// from an already-typed [`ExtensionName`]. The non-empty / canonical-form +/// invariant for `extension_name` is carried by the [`ExtensionName`] type +/// itself — validated at its own construction sites (`ExtensionName::new` / +/// `TryFrom`). Deserialization uses `#[serde(transparent)]`, which does not +/// re-validate; callers that rehydrate prompts from persistence (e.g. +/// `restore_selected_auth_prompt` in `dispatcher.rs`) re-run +/// `ExtensionName::new` so legacy invalid rows drop the prompt rather than +/// propagating. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct PendingAuthPrompt { - /// Extension name to authenticate (must be non-empty, trimmed). - pub(crate) extension_name: String, + /// Installed extension identity for this auth prompt. Canonical at every + /// validated construction site; rehydration from persistence re-checks + /// via `ExtensionName::new` before the prompt is used. + pub(crate) extension_name: ExtensionName, /// Optional instructions shown alongside the auth prompt. #[serde(default)] pub(crate) instructions: Option, @@ -210,26 +219,23 @@ pub struct PendingAuthPrompt { } impl PendingAuthPrompt { - /// Create a new `PendingAuthPrompt`. Trims `extension_name` and returns - /// `None` if the trimmed value is empty. + /// Create a new `PendingAuthPrompt` from an already-validated + /// [`ExtensionName`]. Infallible — the identity type carries the + /// non-empty invariant that this constructor used to check. pub(crate) fn new( - extension_name: String, + extension_name: ExtensionName, instructions: Option, auth_url: Option, setup_url: Option, awaiting_token: bool, - ) -> Option { - let extension_name = extension_name.trim().to_owned(); - if extension_name.is_empty() { - return None; - } - Some(Self { + ) -> Self { + Self { extension_name, instructions, auth_url, setup_url, awaiting_token, - }) + } } } @@ -471,7 +477,7 @@ impl Thread { /// Enter auth mode: next user message will be routed directly to /// the credential store, bypassing the normal pipeline entirely. - pub fn enter_auth_mode(&mut self, extension_name: String) { + pub fn enter_auth_mode(&mut self, extension_name: ExtensionName) { self.pending_auth = Some(PendingAuth { extension_name, created_at: Utc::now(), @@ -993,10 +999,10 @@ mod tests { let mut thread = Thread::new(Uuid::new_v4(), None); assert!(thread.pending_auth.is_none()); - thread.enter_auth_mode("telegram".to_string()); + thread.enter_auth_mode(ExtensionName::new("telegram").unwrap()); assert!(thread.pending_auth.is_some()); let pending = thread.pending_auth.as_ref().unwrap(); - assert_eq!(pending.extension_name, "telegram"); + assert_eq!(pending.extension_name.as_str(), "telegram"); assert!(pending.created_at >= before); assert!(!pending.is_expired()); } @@ -1004,12 +1010,12 @@ mod tests { #[test] fn test_take_pending_auth() { let mut thread = Thread::new(Uuid::new_v4(), None); - thread.enter_auth_mode("notion".to_string()); + thread.enter_auth_mode(ExtensionName::new("notion").unwrap()); let pending = thread.take_pending_auth(); assert!(pending.is_some()); let pending = pending.unwrap(); - assert_eq!(pending.extension_name, "notion"); + assert_eq!(pending.extension_name.as_str(), "notion"); assert!(!pending.is_expired()); // Should be cleared after take assert!(thread.pending_auth.is_none()); @@ -1019,7 +1025,7 @@ mod tests { #[test] fn test_pending_auth_serialization() { let mut thread = Thread::new(Uuid::new_v4(), None); - thread.enter_auth_mode("openai".to_string()); + thread.enter_auth_mode(ExtensionName::new("openai").unwrap()); let json = serde_json::to_string(&thread).expect("should serialize"); assert!(json.contains("pending_auth")); @@ -1029,14 +1035,14 @@ mod tests { let restored: Thread = serde_json::from_str(&json).expect("should deserialize"); assert!(restored.pending_auth.is_some()); let pending = restored.pending_auth.unwrap(); - assert_eq!(pending.extension_name, "openai"); + assert_eq!(pending.extension_name.as_str(), "openai"); assert!(!pending.is_expired()); } #[test] fn test_pending_auth_expiry() { let mut pending = PendingAuth { - extension_name: "test".to_string(), + extension_name: ExtensionName::new("test").unwrap(), created_at: Utc::now(), }; assert!(!pending.is_expired()); diff --git a/src/agent/thread_ops.rs b/src/agent/thread_ops.rs index 5f48153a6e..38177d4c01 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -2231,7 +2231,7 @@ impl Agent { session: &Arc>, thread_id: Uuid, message: &IncomingMessage, - ext_name: String, + ext_name: ironclaw_common::ExtensionName, instructions: String, auth_data: &ParsedAuthData, ) { @@ -2337,11 +2337,11 @@ impl Agent { let result = if let Some(auth_manager) = auth_manager { auth_manager - .submit_auth_token(&pending.extension_name, token, &message.user_id) + .submit_auth_token(pending.extension_name.as_str(), token, &message.user_id) .await } else if let Some(ext_mgr) = self.deps.extension_manager.as_ref() { ext_mgr - .configure_token(&pending.extension_name, token, &message.user_id) + .configure_token(pending.extension_name.as_str(), token, &message.user_id) .await } else { return Ok(Some("Extension manager not available.".to_string())); diff --git a/src/auth/mod.rs b/src/auth/mod.rs index 9be2f7c284..c746ec98b9 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -6,6 +6,7 @@ use std::future::Future; use std::sync::{Arc, Weak}; use std::time::Duration; +use ironclaw_common::ExtensionName; use serde::{Deserialize, Serialize}; use crate::db::{SettingsStore, UserStore}; @@ -103,7 +104,7 @@ pub struct PendingOAuthLaunch { } pub struct PendingOAuthLaunchParams { - pub extension_name: String, + pub extension_name: ExtensionName, pub display_name: String, pub authorization_url: String, pub token_url: String, diff --git a/src/auth/oauth.rs b/src/auth/oauth.rs index 2eb6240846..f19b7f8193 100644 --- a/src/auth/oauth.rs +++ b/src/auth/oauth.rs @@ -10,6 +10,7 @@ use std::time::Duration; use crate::tools::wasm::{ssrf_safe_client_builder_for_target, validate_and_resolve_http_target}; use base64::{Engine, engine::general_purpose::URL_SAFE_NO_PAD}; +use ironclaw_common::ExtensionName; use rand::RngCore; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; @@ -486,7 +487,7 @@ pub async fn validate_oauth_token( /// `/oauth/callback` handler when running in hosted mode. pub struct PendingOAuthFlow { /// Extension name (e.g., "google_calendar"). - pub extension_name: String, + pub extension_name: ExtensionName, /// Human-readable display name (e.g., "Google Calendar"). pub display_name: String, /// OAuth token exchange URL. diff --git a/src/bridge/CLAUDE.md b/src/bridge/CLAUDE.md new file mode 100644 index 0000000000..5570712542 --- /dev/null +++ b/src/bridge/CLAUDE.md @@ -0,0 +1,44 @@ +# Bridge Module + +Adapter layer between the engine v2 (`ironclaw_engine`) and the host +crate's execution, auth, LLM, and persistence surfaces. Channels, +handlers, and tool runtimes must not re-implement auth or identity +resolution — they call through these adapters. + +## Files + +| File | Role | +|------|------| +| `auth_manager.rs` | Centralized authentication state machine. Pre-flight credential checks, setup instruction lookup, auth-flow extension-name resolution. **Single source of truth for turning a credential/action into an `ExtensionName`.** | +| `router.rs` | `handle_with_engine()` — maps engine outcomes to channel responses. Owns auth-gate display + submit target resolution. | +| `effect_adapter.rs` | Implements `EffectExecutor` for the engine. Wraps the host `ToolRegistry` with safety + rate limits. | +| `llm_adapter.rs` | Implements `LlmBackend` for the engine. | +| `store_adapter.rs` | Implements `Store` for the engine (threads, steps, events, memory docs). | +| `cost_guard_gate.rs` | Engine gate that checks cost budget before LLM calls. | +| `skill_migration.rs` | One-shot migration of legacy skill metadata into the engine's capability registry. | +| `workspace_reader.rs` | Read-side adapter between the engine memory store and the workspace. | + +## Auth-flow extension resolution: one place, no re-derivation + +The single authority that maps an auth gate or tool-call context to the installed extension identity is the free function: + +**`bridge::auth_manager::resolve_auth_flow_extension_name(action_name, params, credential_fallback, user_id, tool_registry, extension_manager) -> ExtensionName`** + +Its precedence order: + +1. **User-influenced** — explicit `name` param on `tool_install` / `tool_activate` / `tool_auth` invocations (comes from the model's tool arguments, so it's validated via `ExtensionName::new`; invalid values fall through). +2. The action's provider extension, via `ToolRegistry::provider_extension_for_tool`. +3. Canonicalized `action_name` if the extension manager has an installed extension by that name. +4. The caller-supplied `credential_fallback` — last-resort, used only when no extension owns the action. + +Every surface that needs an extension name for auth flow MUST call this free function (or delegate through a thin wrapper). The approved wrappers are: + +- `AuthManager::resolve_extension_name_for_auth_flow(...) -> ExtensionName` — delegates with `self.tools` and `self.extension_manager`. Used by `bridge::router`. +- `bridge::router::resolve_auth_gate_extension_name(pending) -> Option` — used for `GateRequired` SSE and `send_pending_gate_status`. +- `channels::web::server::pending_gate_extension_name(state, ...) -> Option` — used for `HistoryResponse.pending_gate` and rehydration. Calls the free function directly so the bare-test-harness path (no `AuthManager` built yet) still runs every branch, not a drift-prone subset. + +Wrappers **delegate**; they must not duplicate the precedence rules, reconstruct names from credential prefixes, or fall back to `format!()`-built strings. + +**Why it's centralized:** four identity-confusion bugs (#2561, #2473, #2512, #2574) were the same pattern — two layers independently mapping credential→extension, each reaching a different answer when either one drifted. Newtypes (`CredentialName`, `ExtensionName`) prevent the *type* mix-up; this invariant prevents the *value* mix-up. PR #2617 (Copilot review on `server.rs:1420`) caught a near-fifth: the `pending_gate_extension_name` wrapper's no-auth-manager fallback had grown a three-branch copy of the resolver's precedence that quietly skipped branch 3 (canonicalize + installed-extension check). Extracting the free function collapsed the duplicate and restored the invariant. + +If you think you need a new derivation path, stop and consolidate into the shared resolver instead. See `.claude/rules/types.md` ("Typed Internals") and `src/channels/web/CLAUDE.md` ("Identity types at the web boundary") for the broader rule. diff --git a/src/bridge/auth_manager.rs b/src/bridge/auth_manager.rs index 8fe8fca35b..6e1961c6c6 100644 --- a/src/bridge/auth_manager.rs +++ b/src/bridge/auth_manager.rs @@ -22,7 +22,7 @@ use crate::secrets::SecretsStore; use crate::tools::ToolRegistry; use crate::tools::builtin::extract_host_from_params; use crate::tools::wasm::SharedCredentialRegistry; -use ironclaw_common::CredentialName; +use ironclaw_common::{CredentialName, ExtensionName as CommonExtensionName}; use ironclaw_skills::{SkillCredentialSpec, SkillRegistry}; /// Result of checking whether a tool call has the credentials it needs. @@ -100,6 +100,79 @@ pub struct AuthManager { tools: Option>, } +/// Canonical four-branch auth-flow extension-name resolver, extracted to a +/// free function so every surface shares the exact same precedence. +/// +/// Branches (in precedence order): +/// +/// 1. **User-influenced** `name` parameter on +/// `tool_install` / `tool_activate` / `tool_auth` actions. This string +/// comes from the model's tool arguments, so it must pass +/// `ExtensionName::new` — invalid values (path traversal, uppercase, +/// etc.) fall through to the next branch instead of tainting the +/// typed identity. +/// 2. **Provider-extension hint** declared by the tool itself +/// (`Tool::provider_extension`). Sourced from the Rust tool +/// registration, so the identity is trusted by the point it reaches +/// here. +/// 3. **Canonicalized action name** matching an installed extension. +/// `canonicalize_extension_name` enforces the identity rule; if the +/// extension manager confirms the extension is installed, the name is +/// canonical. +/// 4. **Credential-name fallback** passed by the caller. Invariant: +/// `CredentialName::as_str()` of a typed upstream value. This is the +/// legacy "no extension owns the action" path — see CLAUDE.md +/// "Extension/Auth Invariants". +/// +/// Callers (as of this commit): [`AuthManager::resolve_extension_name_for_auth_flow`] +/// and `src/channels/web/server.rs::pending_gate_extension_name`. Do not +/// re-implement the precedence elsewhere — see `.claude/rules/types.md` +/// and `src/bridge/CLAUDE.md`. +pub(crate) async fn resolve_auth_flow_extension_name( + action_name: &str, + parameters: &serde_json::Value, + credential_fallback: &str, + user_id: &str, + tool_registry: Option<&ToolRegistry>, + extension_manager: Option<&crate::extensions::ExtensionManager>, +) -> CommonExtensionName { + // 1. User-influenced: validate via ExtensionName::new, fall through on failure. + // Match both underscore and hyphen variants for every install/activate/auth + // action so the hyphenated tool names dispatched from Python land the + // same as the canonical underscore form. + if matches!( + action_name, + "tool_install" + | "tool-install" + | "tool_activate" + | "tool-activate" + | "tool_auth" + | "tool-auth" + ) && let Some(raw) = parameters.get("name").and_then(|v| v.as_str()) + && let Ok(name) = CommonExtensionName::new(raw) + { + return name; + } + + // 2. Provider-extension hint off the tool registry (trusted upstream). + if let Some(tools) = tool_registry + && let Some(name) = tools.provider_extension_for_tool(action_name).await + { + return CommonExtensionName::from_trusted(name); + } + + // 3. Canonicalized action_name + confirmed-installed extension. + if let Some(ext_mgr) = extension_manager + && let Ok(canonical) = canonicalize_extension_name(action_name) + && ext_mgr.extension_info(&canonical, user_id).await.is_ok() + { + return CommonExtensionName::from_trusted(canonical); + } + + // 4. Caller-supplied credential-name fallback. + CommonExtensionName::from_trusted(credential_fallback.to_string()) +} + impl AuthManager { pub fn new( secrets_store: Arc, @@ -324,41 +397,28 @@ impl AuthManager { /// to operate on the installed extension name (for example `telegram`), /// while secrets remain stored under the declared credential name /// (for example `telegram_bot_token`). + /// + /// Thin delegator to [`resolve_auth_flow_extension_name`], which owns + /// the precedence logic. Every surface that needs to resolve an + /// auth-flow extension name (this method, the web wrapper + /// `pending_gate_extension_name`, future channels) must call the free + /// function so the four branches stay in one place. pub async fn resolve_extension_name_for_auth_flow( &self, action_name: &str, parameters: &serde_json::Value, credential_fallback: &str, user_id: &str, - ) -> String { - if matches!( + ) -> CommonExtensionName { + resolve_auth_flow_extension_name( action_name, - "tool_install" | "tool-install" | "tool_activate" | "tool_auth" - ) { - let trimmed = parameters - .get("name") - .and_then(|v| v.as_str()) - .map(str::trim) - .unwrap_or(""); - if !trimmed.is_empty() { - return trimmed.to_string(); - } - } - - if let Some(tools) = self.tools.as_ref() - && let Some(name) = tools.provider_extension_for_tool(action_name).await - { - return name; - } - - if let Some(ext_mgr) = self.extension_manager.as_ref() - && let Ok(canonical) = canonicalize_extension_name(action_name) - && ext_mgr.extension_info(&canonical, user_id).await.is_ok() - { - return canonical; - } - - credential_fallback.to_string() + parameters, + credential_fallback, + user_id, + self.tools.as_deref(), + self.extension_manager.as_deref(), + ) + .await } pub async fn latent_extension_actions(&self) -> Vec { @@ -597,7 +657,9 @@ impl AuthManager { }); let launch = build_pending_oauth_launch(PendingOAuthLaunchParams { - extension_name: credential_name.to_string(), + extension_name: ironclaw_common::ExtensionName::from_trusted( + credential_name.to_string(), + ), display_name: spec.provider.clone(), authorization_url: oauth.authorization_url.clone(), token_url: oauth.token_url.clone(), diff --git a/src/bridge/router.rs b/src/bridge/router.rs index 06969e011e..fb79a9e043 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -87,8 +87,10 @@ async fn resolve_extension_for_action( parameters: &serde_json::Value, credential_fallback: &str, user_id: &str, -) -> String { +) -> ironclaw_common::ExtensionName { if let Some(auth_manager) = auth_manager { + // Resolver enforces identity validation on user-influenced branches + // and returns a typed `ExtensionName` directly — no wrap needed. return auth_manager .resolve_extension_name_for_auth_flow( action_name, @@ -98,25 +100,35 @@ async fn resolve_extension_for_action( ) .await; } - tools + // No auth manager (bare test harness): try the tool registry's + // provider-extension hint, else fall back to the credential-name + // string the caller already typed upstream. + let fallback = tools .provider_extension_for_tool(action_name) .await - .unwrap_or_else(|| credential_fallback.to_string()) + .unwrap_or_else(|| credential_fallback.to_string()); + ironclaw_common::ExtensionName::from_trusted(fallback) } -/// Resolve the user-facing name to use when surfacing an authentication -/// gate to a channel. Thin wrapper around `resolve_extension_for_action` -/// that handles the non-Authentication ResumeKind variants by falling back -/// to the action name (since they don't have a credential name to use). -async fn resolve_auth_gate_display_name( +/// Resolve the installed extension identifier that owns an authentication +/// gate, for surfacing that gate on a channel. +/// +/// Returns `Some(ExtensionName)` only for `Authentication` gates — the +/// resolver delegates to [`resolve_extension_for_action`]. Non-auth +/// gate variants (`Approval`, `External`) don't have an extension +/// identity and return `None`. +async fn resolve_auth_gate_extension_name( auth_manager: Option<&AuthManager>, tools: &crate::tools::ToolRegistry, pending: &PendingGate, -) -> String { - if let ironclaw_engine::ResumeKind::Authentication { +) -> Option { + let ironclaw_engine::ResumeKind::Authentication { credential_name, .. } = &pending.resume_kind - { + else { + return None; + }; + Some( resolve_extension_for_action( auth_manager, tools, @@ -125,19 +137,15 @@ async fn resolve_auth_gate_display_name( credential_name.as_str(), &pending.user_id, ) - .await - } else { - // Non-authentication gates don't use this string; return - // something innocuous. - pending.action_name.clone() - } + .await, + ) } async fn send_pending_gate_status( agent: &Agent, message: &IncomingMessage, pending: &PendingGate, - auth_display_name: &str, + extension_name: Option<&ironclaw_common::ExtensionName>, ) { let display_parameters = gate_display_parameters(pending); @@ -163,12 +171,23 @@ async fn send_pending_gate_status( auth_url, .. } => { + // `resolve_auth_gate_extension_name` always returns `Some` for + // Authentication gates; a `None` here would be an upstream + // plumbing bug (wrong variant reached this arm). + let Some(extension_name) = extension_name else { + tracing::warn!( + gate = %pending.gate_name, + request_id = %pending.request_id, + "Authentication gate reached send_pending_gate_status without a resolved extension name" + ); + return; + }; let _ = agent .channels .send_status( &message.channel, StatusUpdate::AuthRequired { - extension_name: auth_display_name.to_string(), + extension_name: extension_name.clone(), instructions: Some(instructions.clone()), auth_url: auth_url.clone(), setup_url: None, @@ -358,7 +377,7 @@ async fn notify_pending_gate( message: &IncomingMessage, pending: &PendingGate, ) -> Result { - let auth_display_name = resolve_auth_gate_display_name(auth_manager, tools, pending).await; + let extension_name = resolve_auth_gate_extension_name(auth_manager, tools, pending).await; if let ironclaw_engine::ResumeKind::External { callback_id } = &pending.resume_kind { tracing::debug!( @@ -383,11 +402,7 @@ async fn notify_pending_gate( description: pending.description.clone(), parameters: serde_json::to_string_pretty(&display_parameters) .unwrap_or_else(|_| display_parameters.to_string()), - extension_name: matches!( - &pending.resume_kind, - ironclaw_engine::ResumeKind::Authentication { .. } - ) - .then(|| auth_display_name.clone()), + extension_name: extension_name.clone(), resume_kind: serde_json::to_value(&pending.resume_kind).unwrap_or_default(), thread_id: pending .scope_thread_id @@ -396,7 +411,7 @@ async fn notify_pending_gate( }, ); } - send_pending_gate_status(agent, message, pending, &auth_display_name).await; + send_pending_gate_status(agent, message, pending, extension_name.as_ref()).await; Ok(BridgeOutcome::Pending) } @@ -2042,7 +2057,7 @@ pub async fn resolve_gate( } if let Some(ref auth_manager) = state.auth_manager { match auth_manager - .submit_auth_token(&submit_target, &token, &message.user_id) + .submit_auth_token(submit_target.as_str(), &token, &message.user_id) .await { Ok(result) @@ -2072,7 +2087,7 @@ pub async fn resolve_gate( onboarding, } => { let next_pending = - requeue_pairing_pending_gate(state, &pending, &display_name) + requeue_pairing_pending_gate(state, &pending, display_name.as_str()) .await?; if let Some(ref sse) = state.sse { sse.broadcast_for_user( @@ -3238,7 +3253,9 @@ async fn await_thread_outcome( .send_status( &message.channel, StatusUpdate::AuthRequired { - extension_name: cred_name.clone(), + extension_name: ironclaw_common::ExtensionName::from_trusted( + cred_name.clone(), + ), instructions: Some(setup_hint.clone()), auth_url: None, setup_url: None, @@ -3319,13 +3336,13 @@ async fn await_thread_outcome( // (agent_loop) detects the pending gate and maps to // HandleOutcome::Pending. { - let auth_display_name = resolve_auth_gate_display_name( + let extension_name = resolve_auth_gate_extension_name( state.auth_manager.as_deref(), state.effect_adapter.tools(), &pending, ) .await; - send_pending_gate_status(agent, message, &pending, &auth_display_name).await; + send_pending_gate_status(agent, message, &pending, extension_name.as_ref()).await; } Ok(BridgeOutcome::Pending) } @@ -3596,7 +3613,7 @@ async fn forward_event_to_channel( .send_status( channel_name, StatusUpdate::AuthRequired { - extension_name: cred_name, + extension_name: ironclaw_common::ExtensionName::from_trusted(cred_name), instructions: Some( "Store the credential with: ironclaw secret set " .into(), @@ -5318,7 +5335,7 @@ mod tests { .. } if tool_name == "shell" && *event_thread_id == thread_id.to_string() - && *extension_name == expected_extension_name + && extension_name.as_str() == expected_extension_name.as_str() ), "expected GateRequired auth event, got: {event:?}" ); diff --git a/src/channels/channel.rs b/src/channels/channel.rs index 88820b2573..a016cd376f 100644 --- a/src/channels/channel.rs +++ b/src/channels/channel.rs @@ -6,6 +6,7 @@ use std::pin::Pin; use async_trait::async_trait; use chrono::{DateTime, Utc}; use futures::Stream; +use ironclaw_common::ExtensionName; use uuid::Uuid; use crate::error::ChannelError; @@ -377,7 +378,7 @@ pub enum StatusUpdate { }, /// Extension needs user authentication (token or OAuth). AuthRequired { - extension_name: String, + extension_name: ExtensionName, instructions: Option, auth_url: Option, setup_url: Option, @@ -385,7 +386,7 @@ pub enum StatusUpdate { }, /// Extension authentication completed. AuthCompleted { - extension_name: String, + extension_name: ExtensionName, success: bool, message: String, }, diff --git a/src/channels/repl.rs b/src/channels/repl.rs index a35941a1be..7c04930a13 100644 --- a/src/channels/repl.rs +++ b/src/channels/repl.rs @@ -905,7 +905,7 @@ mod tests { repl.send_status( StatusUpdate::AuthRequired { - extension_name: "google_oauth_token".to_string(), + extension_name: ironclaw_common::ExtensionName::new("google_oauth_token").unwrap(), instructions: Some("Paste your token".to_string()), auth_url: None, setup_url: Some("http://127.0.0.1:8080/auth".to_string()), diff --git a/src/channels/wasm/wrapper.rs b/src/channels/wasm/wrapper.rs index 4d80a8781b..2ea501ff39 100644 --- a/src/channels/wasm/wrapper.rs +++ b/src/channels/wasm/wrapper.rs @@ -5722,7 +5722,7 @@ mod tests { let metadata = serde_json::json!({"chat_id": 42}); let wit = status_to_wit( &crate::channels::StatusUpdate::AuthRequired { - extension_name: "weather".to_string(), + extension_name: ironclaw_common::ExtensionName::new("weather").unwrap(), instructions: Some("Paste your token".to_string()), auth_url: Some("https://example.com/auth".to_string()), setup_url: None, @@ -5887,7 +5887,7 @@ mod tests { let metadata = serde_json::json!(null); let wit = status_to_wit( &crate::channels::StatusUpdate::AuthCompleted { - extension_name: "weather".to_string(), + extension_name: ironclaw_common::ExtensionName::new("weather").unwrap(), success: true, message: "Token saved".to_string(), }, @@ -5910,7 +5910,7 @@ mod tests { let metadata = serde_json::json!(null); let wit = status_to_wit( &crate::channels::StatusUpdate::AuthCompleted { - extension_name: "weather".to_string(), + extension_name: ironclaw_common::ExtensionName::new("weather").unwrap(), success: false, message: "Invalid token".to_string(), }, diff --git a/src/channels/web/CLAUDE.md b/src/channels/web/CLAUDE.md index fdd31d1e9b..2e894cd228 100644 --- a/src/channels/web/CLAUDE.md +++ b/src/channels/web/CLAUDE.md @@ -142,13 +142,46 @@ Rules: - Generic auth cards are only for non-extension credential prompts or OAuth-only flows that do not have extension setup UI. - If an auth-related change adds a new identity derivation path, stop and consolidate it into the shared backend resolver instead. +Identity types at the web boundary: + +These rules are enforced by check #8 in `scripts/pre-commit-safety.sh` +(`CREDNAME`). Suppress individual intentional uses with +`// web-identity-exempt: `. + +- **Setup / configure / activate routes take `ExtensionName`, not `String`.** + Any handler on `/api/extensions/{name}/...` whose path segment is the + extension identity MUST parse it at entry via + `ExtensionName::new(&name).map_err(|e| (StatusCode::BAD_REQUEST, ...))?` + before the value reaches extension lookup, SSE broadcast, or any + `from_trusted` wrap. A path-traversal or malformed slug must return 400. + +- **Web request/response DTOs and web handlers must not reference + `CredentialName`.** Credential identity is a backend concern. The web + layer accepts and emits `ExtensionName`; the dispatcher / auth manager + resolves credential identity from it server-side. If you find yourself + importing `CredentialName` in `src/channels/web/**`, you're on the + wrong side of the boundary — push the resolution into + `bridge::auth_manager` and have the handler consume its output. + +- **Auth-flow extension resolution happens in one place.** The only + supported way to map an auth gate → extension name is + `AuthManager::resolve_extension_name_for_auth_flow`. Web handlers, + TUI channels, relay adapters, and SSE broadcasters must call through + it rather than re-deriving an extension name from `pending.action_name`, + a credential-name prefix, or a format-string. Four recent identity + bugs (#2561, #2473, #2512, #2574) were duplicate-resolution drift — + this rule exists to make a fifth impossible. + Current consolidation points: -- `src/bridge/auth_manager.rs`: `resolve_extension_name_for_auth_flow(...)` -- `src/bridge/router.rs`: auth-gate display and submit target resolution -- `src/channels/web/server.rs`: pending-gate/history normalization +- `src/bridge/auth_manager.rs`: `resolve_extension_name_for_auth_flow(...)` — **canonical resolver, single source of truth** +- `src/bridge/router.rs`: `resolve_auth_gate_extension_name(...)` — thin wrapper for gate display/submit +- `src/channels/web/server.rs`: `pending_gate_extension_name(...)` — thin wrapper for history/pending-gate hydration - `crates/ironclaw_gateway/static/app.js`: `handleOnboardingState(...)` as the canonical client entrypoint +All three of the backend wrappers above delegate to the canonical resolver +or return `Option`; they must not duplicate its logic. + Legacy cleanup note: - The only remaining browser compatibility path for engine v1 auth mode is `pending_auth` token submit/cancel through `/api/chat/auth-token` and `/api/chat/auth-cancel`. diff --git a/src/channels/web/features/oauth/mod.rs b/src/channels/web/features/oauth/mod.rs index ff037d1ec0..fa300823af 100644 --- a/src/channels/web/features/oauth/mod.rs +++ b/src/channels/web/features/oauth/mod.rs @@ -61,7 +61,7 @@ fn oauth_error_page(label: &str) -> axum::response::Response { fn redact_oauth_state_for_logs(state: &str) -> String { let digest = Sha256::digest(state.as_bytes()); let mut short_hash = String::with_capacity(12); - for byte in &digest[..6] { + for byte in digest.iter().take(6) { use std::fmt::Write as _; let _ = write!(&mut short_hash, "{byte:02x}"); } @@ -322,7 +322,7 @@ pub(crate) async fn oauth_callback_handler( let final_message = if success && flow.auto_activate_extension { match ext_mgr .ensure_extension_ready( - &flow.extension_name, + flow.extension_name.as_str(), &flow.user_id, crate::extensions::EnsureReadyIntent::ExplicitActivate, ) @@ -737,7 +737,7 @@ pub(crate) async fn slack_relay_oauth_callback_handler( // Broadcast event to notify the web UI. state.sse.broadcast(AppEvent::OnboardingState { - extension_name: relay_extension_name.clone(), + extension_name: ironclaw_common::ExtensionName::from_trusted(relay_extension_name.clone()), state: if success { crate::channels::web::types::OnboardingStateDto::Ready } else { diff --git a/src/channels/web/onboarding.rs b/src/channels/web/onboarding.rs index 824b396a5e..41bb5cedd7 100644 --- a/src/channels/web/onboarding.rs +++ b/src/channels/web/onboarding.rs @@ -1,5 +1,6 @@ use crate::channels::web::types::{AppEvent, ChannelOnboardingState, OnboardingStateDto}; use crate::extensions::ConfigureResult; +use ironclaw_common::ExtensionName; pub(crate) enum ConfigureFlowOutcome { Ready, @@ -45,7 +46,7 @@ pub(crate) fn classify_configure_result(result: &ConfigureResult) -> ConfigureFl } pub(crate) fn event_from_configure_result( - extension_name: String, + extension_name: ExtensionName, result: &ConfigureResult, thread_id: Option, ) -> AppEvent { @@ -83,6 +84,7 @@ mod tests { use super::{ConfigureFlowOutcome, classify_configure_result, event_from_configure_result}; use crate::channels::web::types::ChannelOnboardingState; use crate::extensions::ConfigureResult; + use ironclaw_common::ExtensionName; #[test] fn classify_configure_result_treats_oauth_continuation_as_auth_required() { @@ -112,7 +114,11 @@ mod tests { onboarding: None, }; - let event = event_from_configure_result("notion".to_string(), &result, Some("t1".into())); + let event = event_from_configure_result( + ExtensionName::new("notion").unwrap(), + &result, + Some("t1".into()), + ); match event { crate::channels::web::types::AppEvent::OnboardingState { state, diff --git a/src/channels/web/platform/static_files.rs b/src/channels/web/platform/static_files.rs index 95fba12257..80cc3e0882 100644 --- a/src/channels/web/platform/static_files.rs +++ b/src/channels/web/platform/static_files.rs @@ -97,8 +97,8 @@ pub(crate) fn build_csp(nonce: Option<&str>) -> String { /// nonce. Falls back to a minimally-permissive `default-src 'self'` if the /// assembled value somehow fails to parse as a `HeaderValue` — in practice /// the assembled string is pure ASCII and this branch is unreachable, but -/// production code in this repo doesn't use `.expect()` on request-path -/// values. +/// production code in this repo avoids panics on request-path values, so +/// we fall back instead of calling `expect`. pub(crate) static BASE_CSP_HEADER: std::sync::LazyLock = std::sync::LazyLock::new(|| { header::HeaderValue::from_str(&build_csp(None)) @@ -392,7 +392,7 @@ pub(crate) fn css_etag(body: &str) -> String { let digest = Sha256::digest(body.as_bytes()); let hex = hex::encode(digest); // 16 hex chars = 64 bits, plenty for content addressing. - format!("\"sha256-{}\"", &hex[..16]) + format!("\"sha256-{}\"", &hex[..16]) // safety: hex::encode is pure ASCII, char-boundary safe } pub(crate) async fn css_handler( diff --git a/src/channels/web/responses_api.rs b/src/channels/web/responses_api.rs index b99e73b9a3..0ad1d452dd 100644 --- a/src/channels/web/responses_api.rs +++ b/src/channels/web/responses_api.rs @@ -1756,7 +1756,7 @@ mod tests { tool_name: "tool_install".to_string(), description: "Need auth".to_string(), parameters: "{\"name\":\"notion\"}".to_string(), - extension_name: Some("notion".to_string()), + extension_name: Some(ironclaw_common::ExtensionName::new("notion").unwrap()), resume_kind: serde_json::json!({ "Authentication": { "credential_name": "notion_api_token", diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index b5817d8f36..7a7add10ba 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -336,11 +336,11 @@ pub(crate) async fn chat_auth_token_handler( async fn restore_pending_auth_mode( session: &Arc>, thread_id: Uuid, - extension_name: &str, + extension_name: &ironclaw_common::ExtensionName, ) { let mut sess = session.lock().await; if let Some(thread) = sess.threads.get_mut(&thread_id) { - thread.enter_auth_mode(extension_name.to_string()); + thread.enter_auth_mode(extension_name.clone()); } } @@ -421,11 +421,11 @@ pub(crate) async fn handle_legacy_auth_token_submission( let result = if let Some(auth_manager) = state.auth_manager.as_ref() { auth_manager - .submit_auth_token(&pending_auth.extension_name, token, user_id) + .submit_auth_token(pending_auth.extension_name.as_str(), token, user_id) .await } else if let Some(ext_mgr) = state.extension_manager.as_ref() { ext_mgr - .configure_token(&pending_auth.extension_name, token, user_id) + .configure_token(pending_auth.extension_name.as_str(), token, user_id) .await } else { restore_pending_auth_mode(&session, thread_id, &pending_auth.extension_name).await; @@ -635,7 +635,7 @@ async fn pending_gate_extension_name( tool_name: &str, parameters: &str, resume_kind: &ironclaw_engine::ResumeKind, -) -> Option { +) -> Option { let ironclaw_engine::ResumeKind::Authentication { credential_name, .. } = resume_kind @@ -646,43 +646,24 @@ async fn pending_gate_extension_name( let parsed_parameters = serde_json::from_str::(parameters).unwrap_or(serde_json::Value::Null); - if let Some(auth_manager) = state.auth_manager.as_ref() { - return Some( - auth_manager - .resolve_extension_name_for_auth_flow( - tool_name, - &parsed_parameters, - credential_name.as_str(), - user_id, - ) - .await, - ); - } - - if matches!( - tool_name, - "tool_install" - | "tool-install" - | "tool_activate" - | "tool-activate" - | "tool_auth" - | "tool-auth" - ) && let Some(name) = parsed_parameters.get("name").and_then(|v| v.as_str()) - && !name.trim().is_empty() - { - return Some(name.to_string()); - } - - if let Some(tools) = state.tool_registry.as_ref() - && let Some(name) = tools.provider_extension_for_tool(tool_name).await - { - return Some(name); - } - - // auth_manager is None only when no secrets backend exists (e.g. bare - // test harness). Fall back to the raw credential name rather than - // duplicating AuthManager resolution logic here. - Some(credential_name.as_str().to_string()) + // Both the "auth manager present" and "bare test harness" paths + // delegate to the single canonical resolver (see + // `src/bridge/auth_manager.rs::resolve_auth_flow_extension_name`) so + // the four branches stay aligned. Without this delegation the wrapper + // would drift — check #8 in `scripts/pre-commit-safety.sh` and the + // "one resolver" rule in `src/bridge/CLAUDE.md` exist to prevent + // exactly that drift. + Some( + crate::bridge::auth_manager::resolve_auth_flow_extension_name( + tool_name, + &parsed_parameters, + credential_name.as_str(), + user_id, + state.tool_registry.as_deref(), + state.extension_manager.as_deref(), + ) + .await, + ) } async fn engine_pending_gate_info( @@ -1670,8 +1651,17 @@ pub(crate) async fn extensions_activate_handler( AuthenticatedUser(user): AuthenticatedUser, Path(name): Path, ) -> Result, (StatusCode, String)> { + // The URL path segment is user input — validate at the boundary via + // `ExtensionName::new` and use the canonical form for all downstream + // extension-manager calls and response formatting. + let name = ironclaw_common::ExtensionName::new(&name).map_err(|e| { + ( + StatusCode::BAD_REQUEST, + format!("Invalid extension name: {e}"), + ) + })?; tracing::trace!( - extension = %name, + extension = %name.as_str(), user_id = %user.user_id, "extensions_activate_handler: received activate request" ); @@ -1682,14 +1672,14 @@ pub(crate) async fn extensions_activate_handler( match ext_mgr .ensure_extension_ready( - &name, + name.as_str(), &user.user_id, crate::extensions::EnsureReadyIntent::ExplicitActivate, ) .await { Ok(readiness) => { - let mut resp = ActionResponse::ok(format!("Extension '{}' is ready.", name)); + let mut resp = ActionResponse::ok(format!("Extension '{}' is ready.", name.as_str())); apply_extension_readiness_to_response(&mut resp, readiness, false); Ok(Json(resp)) } @@ -1702,12 +1692,21 @@ pub(crate) async fn extensions_remove_handler( AuthenticatedUser(user): AuthenticatedUser, Path(name): Path, ) -> Result, (StatusCode, String)> { + // Validate user-controlled path segment before it reaches the extension + // manager — rejects path-traversal, invalid characters, and malformed + // slugs with a 400. + let name = ironclaw_common::ExtensionName::new(&name).map_err(|e| { + ( + StatusCode::BAD_REQUEST, + format!("Invalid extension name: {e}"), + ) + })?; let ext_mgr = state.extension_manager.as_ref().ok_or(( StatusCode::NOT_IMPLEMENTED, "Extension manager not available (secrets store required)".to_string(), ))?; - match ext_mgr.remove(&name, &user.user_id).await { + match ext_mgr.remove(name.as_str(), &user.user_id).await { Ok(message) => Ok(Json(ActionResponse::ok(message))), Err(e) => Ok(Json(ActionResponse::fail(e.to_string()))), } @@ -1782,13 +1781,21 @@ pub(crate) async fn extensions_setup_handler( AuthenticatedUser(user): AuthenticatedUser, Path(name): Path, ) -> Result, (StatusCode, String)> { + // Validate user-controlled path segment at entry. Downstream lookups + // (`get_setup_schema`, `list().find(...)`) consume the canonical form. + let name = ironclaw_common::ExtensionName::new(&name).map_err(|e| { + ( + StatusCode::BAD_REQUEST, + format!("Invalid extension name: {e}"), + ) + })?; let ext_mgr = state.extension_manager.as_ref().ok_or(( StatusCode::NOT_IMPLEMENTED, "Extension manager not available (secrets store required)".to_string(), ))?; let setup = ext_mgr - .get_setup_schema(&name, &user.user_id) + .get_setup_schema(name.as_str(), &user.user_id) .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; @@ -1796,12 +1803,12 @@ pub(crate) async fn extensions_setup_handler( .list(None, false, &user.user_id) .await .ok() - .and_then(|list| list.into_iter().find(|e| e.name == name)) + .and_then(|list| list.into_iter().find(|e| e.name == name.as_str())) .map(|e| e.kind.to_string()) .unwrap_or_default(); Ok(Json(ExtensionSetupResponse { - name, + name: name.as_str().to_string(), kind, secrets: setup.secrets, fields: setup.fields, @@ -1821,12 +1828,23 @@ pub(crate) async fn extensions_setup_submit_handler( "Extension manager not available (secrets store required)".to_string(), ))?; + // The URL path segment is user input — validate at the boundary via + // `ExtensionName::new`. Reject path-traversal, invalid characters, or + // malformed slugs with a 400 before the value reaches extension + // lookup, SSE broadcast, or any `from_trusted` wrap below. + let name = ironclaw_common::ExtensionName::new(&name).map_err(|e| { + ( + StatusCode::BAD_REQUEST, + format!("Invalid extension name: {e}"), + ) + })?; + // Clear auth mode regardless of outcome so the next user message goes // through to the LLM instead of being intercepted as a token. clear_auth_mode(&state, &user.user_id).await; match ext_mgr - .configure(&name, &req.secrets, &req.fields, &user.user_id) + .configure(name.as_str(), &req.secrets, &req.fields, &user.user_id) .await { Ok(result) => { @@ -1868,7 +1886,7 @@ pub(crate) async fn extensions_setup_submit_handler( &user.user_id, request_id, Some(thread_id), - &name, + name.as_str(), ) .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? @@ -2026,7 +2044,7 @@ pub(crate) async fn pairing_approve_handler( state.sse.broadcast_for_user( &user.user_id, AppEvent::OnboardingState { - extension_name: channel.clone(), + extension_name: ironclaw_common::ExtensionName::from_trusted(channel.clone()), state: crate::channels::web::types::OnboardingStateDto::Failed, request_id: None, message: Some(message.clone()), @@ -2044,7 +2062,7 @@ pub(crate) async fn pairing_approve_handler( state.sse.broadcast_for_user( &user.user_id, AppEvent::OnboardingState { - extension_name: channel.clone(), + extension_name: ironclaw_common::ExtensionName::from_trusted(channel.clone()), state: crate::channels::web::types::OnboardingStateDto::Ready, request_id: None, message: Some("Pairing approved.".to_string()), @@ -3057,7 +3075,10 @@ mod tests { ) .await; - assert_eq!(extension_name.as_deref(), Some("telegram")); + assert_eq!( + extension_name.as_ref().map(|n| n.as_str()), + Some("telegram") + ); } #[tokio::test] @@ -3079,7 +3100,10 @@ mod tests { ) .await; - assert_eq!(extension_name.as_deref(), Some("telegram")); + assert_eq!( + extension_name.as_ref().map(|n| n.as_str()), + Some("telegram") + ); } #[tokio::test] @@ -3134,7 +3158,7 @@ mod tests { ) .await; - assert_eq!(extension_name.as_deref(), Some("notion")); + assert_eq!(extension_name.as_ref().map(|n| n.as_str()), Some("notion")); } /// Build a test router with just the OAuth callback route. @@ -3321,7 +3345,7 @@ mod tests { let thread_id = { let thread = sess.create_thread(Some("gateway")); let thread_id = thread.id; - thread.enter_auth_mode("telegram".to_string()); + thread.enter_auth_mode(ironclaw_common::ExtensionName::new("telegram").unwrap()); thread_id }; sess.switch_thread(thread_id); @@ -3378,9 +3402,9 @@ mod tests { let target_thread_id = Uuid::new_v4(); let other_thread_id = Uuid::new_v4(); sess.create_thread_with_id(target_thread_id, Some("gateway")) - .enter_auth_mode("telegram".to_string()); + .enter_auth_mode(ironclaw_common::ExtensionName::new("telegram").unwrap()); sess.create_thread_with_id(other_thread_id, Some("gateway")) - .enter_auth_mode("notion".to_string()); + .enter_auth_mode(ironclaw_common::ExtensionName::new("notion").unwrap()); sess.switch_thread(other_thread_id); } @@ -3527,7 +3551,7 @@ mod tests { let thread = sess.create_thread(Some("gateway")); let thread_id = thread.id; thread.pending_auth = Some(crate::agent::session::PendingAuth { - extension_name: "telegram".to_string(), + extension_name: ironclaw_common::ExtensionName::new("telegram").unwrap(), created_at: chrono::Utc::now() - chrono::Duration::minutes(16), }); sess.switch_thread(thread_id); @@ -4083,7 +4107,7 @@ mod tests { oauth_proxy_auth_token: Option, ) -> crate::auth::oauth::PendingOAuthFlow { crate::auth::oauth::PendingOAuthFlow { - extension_name: "test_tool".to_string(), + extension_name: ironclaw_common::ExtensionName::new("test_tool").unwrap(), display_name: "Test Tool".to_string(), token_url: "https://example.com/token".to_string(), client_id: "client123".to_string(), @@ -4108,6 +4132,138 @@ mod tests { } } + /// Regression for the PR #2617 review (Gemini HIGH/security): the + /// `extensions_setup_submit_handler` used to wrap the URL path segment + /// in `ExtensionName::from_trusted`, skipping the newtype's path- + /// traversal and invalid-character rejection. A handler-level test (not + /// an `identity.rs`-level test) locks in the boundary: a malformed path + /// must produce a 400 before the value reaches any downstream + /// `from_trusted` wrap, extension lookup, or SSE broadcast. + #[tokio::test] + async fn test_extensions_setup_submit_rejects_path_traversal_name() { + use axum::body::Body; + use tower::ServiceExt; + + let secrets = test_secrets_store(); + let (ext_mgr, _wasm_tools_dir, _wasm_channels_dir) = test_ext_mgr(secrets); + + let state = test_gateway_state(Some(ext_mgr)); + let app = Router::new() + .route( + "/api/extensions/{name}/setup", + post(extensions_setup_submit_handler), + ) + .with_state(state); + + // Each of these slugs would have silently reached extension lookup + // under the old `from_trusted(name)` wrap. All must reject at 400. + // We use axum::http::uri::PathAndQuery-safe escape where needed so + // the path extractor still decodes into a valid `String`. + for bad in [ + "..%2Ftraversal", + "slash%2Fname", + "BadCase", + "has%20space", + "trailing_", + ] { + let req_body = serde_json::json!({"secrets": {}}); + let mut req = axum::http::Request::builder() + .method("POST") + .uri(format!("/api/extensions/{bad}/setup")) + .header("content-type", "application/json") + .body(Body::from(req_body.to_string())) + .expect("request"); + req.extensions_mut().insert(UserIdentity { + user_id: "test".to_string(), + role: "admin".to_string(), + workspace_read_scopes: Vec::new(), + }); + + let resp = ServiceExt::>::oneshot(app.clone(), req) + .await + .expect("response"); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "expected 400 for malformed extension name {bad:?}, got {:?}", + resp.status() + ); + } + } + + /// Regression for the PR #2617 Copilot review: the sibling + /// `/api/extensions/{name}/...` handlers (`activate`, `remove`, setup GET) + /// used to accept `Path` and hand it straight to the extension + /// manager, leaving path-traversal / malformed slugs unvalidated at the + /// web boundary. All three must now reject at 400 before any downstream + /// lookup — same guarantee as `extensions_setup_submit_handler`. + #[tokio::test] + async fn test_extensions_sibling_handlers_reject_path_traversal_name() { + use axum::body::Body; + use axum::routing::{get, post}; + use tower::ServiceExt; + + let secrets = test_secrets_store(); + let (ext_mgr, _wasm_tools_dir, _wasm_channels_dir) = test_ext_mgr(secrets); + + let state = test_gateway_state(Some(ext_mgr)); + let app = Router::new() + .route( + "/api/extensions/{name}/activate", + post(extensions_activate_handler), + ) + .route( + "/api/extensions/{name}/remove", + post(extensions_remove_handler), + ) + .route( + "/api/extensions/{name}/setup", + get(extensions_setup_handler), + ) + .with_state(state); + + let bad_names = [ + "..%2Ftraversal", + "slash%2Fname", + "BadCase", + "has%20space", + "trailing_", + ]; + let routes = [("POST", "activate"), ("POST", "remove"), ("GET", "setup")]; + + for bad in bad_names { + for (method, suffix) in routes { + let mut builder = axum::http::Request::builder() + .method(method) + .uri(format!("/api/extensions/{bad}/{suffix}")); + if method == "POST" { + builder = builder.header("content-type", "application/json"); + } + let body = if method == "POST" { + Body::from("{}") + } else { + Body::empty() + }; + let mut req = builder.body(body).expect("request"); + req.extensions_mut().insert(UserIdentity { + user_id: "test".to_string(), + role: "admin".to_string(), + workspace_read_scopes: Vec::new(), + }); + + let resp = ServiceExt::>::oneshot(app.clone(), req) + .await + .expect("response"); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "expected 400 for {method} {suffix} with malformed name {bad:?}, got {:?}", + resp.status() + ); + } + } + } + #[tokio::test] async fn test_extensions_setup_submit_returns_failure_when_not_activated() { use axum::body::Body; @@ -5159,7 +5315,7 @@ mod tests { // Insert an expired flow. let flow = crate::auth::oauth::PendingOAuthFlow { - extension_name: "test_tool".to_string(), + extension_name: ironclaw_common::ExtensionName::new("test_tool").unwrap(), display_name: "Test Tool".to_string(), token_url: "https://example.com/token".to_string(), client_id: "client123".to_string(), @@ -5231,7 +5387,7 @@ mod tests { return; }; let flow = crate::auth::oauth::PendingOAuthFlow { - extension_name: "test_tool".to_string(), + extension_name: ironclaw_common::ExtensionName::new("test_tool").unwrap(), display_name: "Test Tool".to_string(), token_url: "https://example.com/token".to_string(), client_id: "client123".to_string(), @@ -5344,7 +5500,7 @@ mod tests { return; }; let flow = crate::auth::oauth::PendingOAuthFlow { - extension_name: "test_tool".to_string(), + extension_name: ironclaw_common::ExtensionName::new("test_tool").unwrap(), display_name: "Test Tool".to_string(), token_url: "https://example.com/token".to_string(), client_id: "client123".to_string(), @@ -5434,7 +5590,7 @@ mod tests { return; }; let flow = crate::auth::oauth::PendingOAuthFlow { - extension_name: "test_tool".to_string(), + extension_name: ironclaw_common::ExtensionName::new("test_tool").unwrap(), display_name: "Test Tool".to_string(), token_url: "https://example.com/token".to_string(), client_id: "client123".to_string(), @@ -5518,7 +5674,7 @@ mod tests { return; }; let flow = crate::auth::oauth::PendingOAuthFlow { - extension_name: "test_tool".to_string(), + extension_name: ironclaw_common::ExtensionName::new("test_tool").unwrap(), display_name: "Test Tool".to_string(), token_url: "https://example.com/token".to_string(), client_id: "client123".to_string(), diff --git a/src/channels/web/types.rs b/src/channels/web/types.rs index a54f666e8c..98947ce0ca 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -127,7 +127,7 @@ pub struct PendingGateInfo { pub description: String, pub parameters: String, #[serde(skip_serializing_if = "Option::is_none")] - pub extension_name: Option, + pub extension_name: Option, pub resume_kind: serde_json::Value, } @@ -1393,7 +1393,7 @@ mod tests { #[test] fn test_app_event_onboarding_state_auth_required_serialize() { let event = AppEvent::OnboardingState { - extension_name: "notion".to_string(), + extension_name: ironclaw_common::ExtensionName::new("notion").unwrap(), state: OnboardingStateDto::AuthRequired, request_id: Some("req-123".to_string()), message: None, @@ -1418,7 +1418,7 @@ mod tests { #[test] fn test_app_event_onboarding_state_ready_serialize() { let event = AppEvent::OnboardingState { - extension_name: "notion".to_string(), + extension_name: ironclaw_common::ExtensionName::new("notion").unwrap(), state: OnboardingStateDto::Ready, request_id: None, message: Some("notion authenticated (3 tools loaded)".to_string()), @@ -1440,7 +1440,7 @@ mod tests { #[test] fn test_ws_server_from_app_event_onboarding_state_auth_required() { let event = AppEvent::OnboardingState { - extension_name: "openai".to_string(), + extension_name: ironclaw_common::ExtensionName::new("openai").unwrap(), state: OnboardingStateDto::AuthRequired, request_id: None, message: None, @@ -1464,7 +1464,7 @@ mod tests { #[test] fn test_app_event_onboarding_state_pairing_required_serialize() { let event = AppEvent::OnboardingState { - extension_name: "telegram".to_string(), + extension_name: ironclaw_common::ExtensionName::new("telegram").unwrap(), state: OnboardingStateDto::PairingRequired, request_id: None, message: None, @@ -1489,7 +1489,7 @@ mod tests { #[test] fn test_ws_server_from_app_event_onboarding_state_failed() { let event = AppEvent::OnboardingState { - extension_name: "slack".to_string(), + extension_name: ironclaw_common::ExtensionName::new("slack").unwrap(), state: OnboardingStateDto::Failed, request_id: None, message: Some("Invalid token".to_string()), diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index 74c71a210b..3cc23e5d7f 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -1438,7 +1438,7 @@ impl ExtensionManager { async fn broadcast_extension_status(&self, name: &str, status: &str, message: Option<&str>) { if let Some(ref sse) = *self.sse_manager.read().await { sse.broadcast(ironclaw_common::AppEvent::ExtensionStatus { - extension_name: name.to_string(), + extension_name: ironclaw_common::ExtensionName::from_trusted(name.to_string()), status: status.to_string(), message: message.map(|m| m.to_string()), }); @@ -2179,7 +2179,7 @@ impl ExtensionManager { self.pending_oauth_flows .write() .await - .retain(|_, flow| flow.extension_name != name); + .retain(|_, flow| flow.extension_name.as_str() != name); match kind { ExtensionKind::McpServer => { @@ -3840,7 +3840,7 @@ impl ExtensionManager { extra_params.insert("resource".to_string(), resource.clone()); let launch = build_pending_oauth_launch(PendingOAuthLaunchParams { - extension_name: name.to_string(), + extension_name: ironclaw_common::ExtensionName::from_trusted(name.to_string()), display_name: server.name.clone(), authorization_url, token_url: token_url.clone(), @@ -4215,7 +4215,9 @@ impl ExtensionManager { .await .unwrap_or(ExtensionKind::WasmChannel); let launch = build_pending_oauth_launch(PendingOAuthLaunchParams { - extension_name: extension_name.to_string(), + extension_name: ironclaw_common::ExtensionName::from_trusted( + extension_name.to_string(), + ), display_name: display_name.to_string(), authorization_url: oauth.authorization_url.clone(), token_url: oauth.token_url.clone(), @@ -4810,7 +4812,7 @@ impl ExtensionManager { .unwrap_or_else(|| name.to_string()); let launch = build_pending_oauth_launch(PendingOAuthLaunchParams { - extension_name: name.to_string(), + extension_name: ironclaw_common::ExtensionName::from_trusted(name.to_string()), display_name: display_name.clone(), authorization_url: oauth.authorization_url.clone(), token_url: oauth.token_url.clone(), @@ -4947,7 +4949,7 @@ impl ExtensionManager { if let Some(ref sse) = sse_manager { sse.broadcast(ironclaw_common::AppEvent::OnboardingState { - extension_name: ext_name, + extension_name: ironclaw_common::ExtensionName::from_trusted(ext_name), state: if success { ironclaw_common::OnboardingStateDto::Ready } else { @@ -7238,7 +7240,7 @@ impl ExtensionManager { sse.broadcast_for_user( user_id, ironclaw_common::OnboardingStateDto::pairing_required( - name.clone(), + ironclaw_common::ExtensionName::from_trusted(name.clone()), None, None, None, @@ -10273,7 +10275,7 @@ mod tests { mgr.pending_oauth_flows().write().await.insert( "gmail-state".to_string(), crate::auth::oauth::PendingOAuthFlow { - extension_name: "gmail".to_string(), + extension_name: ironclaw_common::ExtensionName::new("gmail").unwrap(), display_name: "Gmail".to_string(), token_url: "https://example.com/token".to_string(), client_id: "client123".to_string(), @@ -10300,7 +10302,7 @@ mod tests { mgr.pending_oauth_flows().write().await.insert( "other-state".to_string(), crate::auth::oauth::PendingOAuthFlow { - extension_name: "web-search".to_string(), + extension_name: ironclaw_common::ExtensionName::new("web-search").unwrap(), display_name: "Web Search".to_string(), token_url: "https://example.com/token".to_string(), client_id: "client456".to_string(), diff --git a/tests/e2e_live.rs b/tests/e2e_live.rs index 0c93c2c160..4516f59f09 100644 --- a/tests/e2e_live.rs +++ b/tests/e2e_live.rs @@ -382,7 +382,7 @@ mod live_tests { .collect(); let drive_gate = auth_required_events .iter() - .find(|(ext, _, _)| ext.contains("google") || ext.contains("drive")); + .find(|(ext, _, _)| ext.as_str().contains("google") || ext.as_str().contains("drive")); assert!( drive_gate.is_some(), "Phase A: expected an AuthRequired event for the Google Drive extension, \ @@ -710,7 +710,8 @@ mod live_tests { matches!( s, StatusUpdate::AuthRequired { extension_name, .. } - if extension_name.contains("google") || extension_name.contains("drive") + if extension_name.as_str().contains("google") + || extension_name.as_str().contains("drive") ) }); assert!( diff --git a/tests/support/replay_outcome.rs b/tests/support/replay_outcome.rs index 8f28a2ce8f..8ec5077666 100644 --- a/tests/support/replay_outcome.rs +++ b/tests/support/replay_outcome.rs @@ -188,7 +188,9 @@ impl ReplayOutcome { } StatusUpdate::AuthRequired { extension_name, .. } => { *kind_counts.entry("AuthRequired".into()).or_default() += 1; - EventSummary::AuthRequired { extension_name } + EventSummary::AuthRequired { + extension_name: extension_name.into(), + } } StatusUpdate::AuthCompleted { extension_name, @@ -197,7 +199,7 @@ impl ReplayOutcome { } => { *kind_counts.entry("AuthCompleted".into()).or_default() += 1; EventSummary::AuthCompleted { - extension_name, + extension_name: extension_name.into(), success, } }