From c289c223a6066051fe02833793ded21c5db4a03a Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 13:26:57 +0900 Subject: [PATCH 01/12] feat(common): add CredentialName and ExtensionName newtypes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce typed identifiers for the backend-secret vs user-facing extension identity split that the Extension/Auth Invariants section of CLAUDE.md describes. Four recent PRs (#2561, #2473, #2512, #2574) have been identity- confusion bugs with the same shape: a stringly-typed value passed through multiple layers with each layer meaning a different thing. Newtypes make each of those a compile error. This is PR 1 of 2. PR 1 lands the newtypes and migrates the core auth seam (ResumeKind::Authentication, MissingCredential, ToolReadiness::NeedsAuth, LatentActionExecution::NeedsAuth, extensions/naming.rs). PR 2 will migrate AppEvent.extension_name, OAuth/pending-flow stores, TUI events, and the remaining extension_name: String fields. Wire format is unchanged — both newtypes use #[serde(transparent)] so on- wire and on-disk representations stay plain strings and legacy persisted rows keep deserializing. Validation runs at explicit construction (::new / ::try_from / ::from_str), not at deserialize time. Also adds .claude/rules/types.md codifying the "no stringly-typed internals" rule. Regression coverage: 17 new unit tests in identity.rs; existing auth_manager, router, and gate tests (130+ cases) all pass unchanged. --- .claude/rules/types.md | 129 ++++++ Cargo.lock | 1 + crates/ironclaw_common/Cargo.toml | 1 + crates/ironclaw_common/src/identity.rs | 415 ++++++++++++++++++ crates/ironclaw_common/src/lib.rs | 2 + .../src/executor/structured.rs | 4 +- crates/ironclaw_engine/src/gate/mod.rs | 5 +- src/bridge/auth_manager.rs | 15 +- src/bridge/effect_adapter.rs | 16 +- src/bridge/router.rs | 36 +- src/channels/web/server.rs | 7 +- src/extensions/naming.rs | 51 +-- tests/engine_v2_gate_integration.rs | 6 +- 13 files changed, 611 insertions(+), 77 deletions(-) create mode 100644 .claude/rules/types.md create mode 100644 crates/ironclaw_common/src/identity.rs diff --git a/.claude/rules/types.md b/.claude/rules/types.md new file mode 100644 index 00000000000..cc413fc90b3 --- /dev/null +++ b/.claude/rules/types.md @@ -0,0 +1,129 @@ +--- +paths: + - "src/**" + - "crates/**" + - "tests/**" +--- +# Typed Internals — No Stringly-Typed Values Inside the System + +**Internal values must have types that reflect what they mean.** Raw `String` +is the boundary type — accepted from user input, JSON/HTTP payloads, the +database, and untrusted external APIs — and it should be converted to a +domain type as soon as possible. Everything that moves between internal +modules should carry a type that makes misuse a compile error. + +Concretely: + +- **Identifiers** → newtypes (`CredentialName`, `ExtensionName`, `ThreadId`, + `UserId`). Never `String`, `&str`, or `uuid::Uuid` alone. +- **Fixed small sets** → enums with `#[serde(rename_all = "snake_case")]` + or explicit `#[serde(rename = "...")]`. Never compare strings like + `status == "in_progress"`. +- **Units, shapes, modes** → enums (`SandboxPolicy`, `ExecutionMode`, + `ThreadState`). Never booleans-plus-magic-strings. + +If two values have the same shape (`String`, `u64`, whatever) but different +meanings, they must be different types. The compiler is the only durable +enforcement of "don't mix these up" — comments, naming, and code review +are not. + +## Why + +Identity confusion has shipped four times in recent history: + +| PR | Surface | What went wrong | +|----|---------|-----------------| +| #2561 | settings restart | `owner_id` round-tripped through a string, lost type on reload | +| #2473 | Slack relay OAuth | nonce stored under wrong scope — wrong `user_id` vs gateway owner id | +| #2512 | Slack relay OAuth | state lookup compared strings across two callers that had diverged | +| #2574 | auth-gate display | inline fallback re-derived extension name, returned `telegram_bot_token` where `telegram` was expected | + +All four bugs are the same shape: a string-typed value passes through more +than one layer, one layer treats it as one meaning, another treats it as a +different meaning, and the compiler has nothing to say. Newtypes would +have made each of these a type error. + +## The Extension/Auth identity invariant + +See `CLAUDE.md` → "Extension/Auth Invariants" for the routing rules. The +types live in `crates/ironclaw_common/src/identity.rs`: + +- [`CredentialName`] — backend secret identity (e.g. `telegram_bot_token`, + `google_oauth_token`). Used for secrets-store keys, gate resume + payloads, credential injection. +- [`ExtensionName`] — user-facing installed extension/channel identity + (e.g. `telegram`, `gmail`). Used for onboarding UI, setup/configure + routing, Python action dispatch. Hyphens fold to underscores at + construction time because extensions are invoked as Python attribute + accesses. + +Never cast between them. Never recompute one from the other by string +manipulation — resolve through `AuthManager::resolve_extension_name_for_auth_flow`. + +## When to add a newtype + +Add one when **all** of the following are true: + +1. The value is a *name*, *id*, or *key* — something whose shape is + incidental to its meaning. +2. It flows through **more than one module** or crosses a **type + boundary** (struct field, function parameter, return type). +3. Mixing it up with another same-shape value would be a **silent + runtime bug**, not a compile error. + +If all three hold, make it a newtype. Put it in +`crates/ironclaw_common/src/identity.rs` (or a module-local spot if its +blast radius is genuinely one crate). + +### Newtype template + +Use `#[serde(transparent)]` so on-wire and on-disk representation stays a +plain string — legacy persisted rows must keep deserializing cleanly. +Validate at explicit construction sites (`new`, `try_from`, `from_str`), +not on the wire. Provide a `from_trusted(String)` escape hatch for values +sourced from a typed upstream (DB row, registry entry) where the caller +already trusts the shape. + +```rust +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(transparent)] +pub struct MyId(String); + +impl MyId { + pub fn new(raw: impl AsRef) -> Result { ... } + pub fn from_trusted(raw: String) -> Self { Self(raw) } + pub fn as_str(&self) -> &str { &self.0 } +} + +impl TryFrom for MyId { ... } // validating +impl From for String { ... } // infallible +// Deliberately no `From` — that would silently bypass validation. +``` + +## Don'ts + +- **Don't add `From<&str>` or `From` for an identity newtype.** + That relaxes the invariant. If a caller has a raw string, they should + have to choose `new` (validate) or `from_trusted` (documented opt-out) + — the choice itself is the audit trail. +- **Don't compare a newtype against a format-string-built `String`.** + If you find yourself writing `format!("{}_token", extension_name) == + credential_name.as_str()`, you've rebuilt the bug #2574 fixed. Route + through the shared resolver instead. +- **Don't use `#[serde(try_from = "String")]` for identity newtypes + without a migration plan.** Existing persisted rows may not satisfy the + current rule; `transparent` + explicit validation at construction + preserves them while still gaining type distinctness. +- **Don't match on string literals for a value that should be an enum.** + `match status.as_str() { "ready" => ... }` means status should be an + enum. Fix the type. +- **Don't downgrade a typed value back to `String` except at a system + boundary.** A `String` returned from an internal function is a + regression — return the type. + +## Applies to + +`src/**`, `crates/**`, `tests/**`. Any code inside the IronClaw +workspace. The rule doesn't apply to wire payloads (which are `String` +by virtue of JSON), log lines, or error messages — those *are* the +boundary. diff --git a/Cargo.lock b/Cargo.lock index e7744a35bc6..5845c2fdb34 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3981,6 +3981,7 @@ dependencies = [ "chrono-tz", "serde", "serde_json", + "thiserror 2.0.18", "tracing", ] diff --git a/crates/ironclaw_common/Cargo.toml b/crates/ironclaw_common/Cargo.toml index 641fdffbccf..0959aef0918 100644 --- a/crates/ironclaw_common/Cargo.toml +++ b/crates/ironclaw_common/Cargo.toml @@ -16,4 +16,5 @@ dist = false chrono-tz = "0.10" serde = { version = "1", features = ["derive"] } serde_json = "1" +thiserror = "2" tracing = "0.1" diff --git a/crates/ironclaw_common/src/identity.rs b/crates/ironclaw_common/src/identity.rs new file mode 100644 index 00000000000..726a2b98abd --- /dev/null +++ b/crates/ironclaw_common/src/identity.rs @@ -0,0 +1,415 @@ +//! Typed identifiers for internal names. +//! +//! Two string-shaped values that must not be confused: +//! +//! - [`CredentialName`] — backend secret identity used for storage, injection, +//! and gate resume (e.g. `telegram_bot_token`, `google_oauth_token`). +//! - [`ExtensionName`] — user-facing installed extension/channel identity used +//! for setup routing, UI, and Python action dispatch (e.g. `telegram`, +//! `gmail`). +//! +//! See `.claude/rules/types.md` for why these are newtypes and +//! `CLAUDE.md` → "Extension/Auth Invariants" for the routing rules. +//! +//! # Wire compatibility +//! +//! Both types use `#[serde(transparent)]` so the on-wire and on-disk +//! representation is a plain JSON string — unchanged from when the fields +//! were `String`. Validation runs at construction (`try_from` / `from_str`) +//! and at any explicit `validate()` call, not at deserialize time. Legacy +//! persisted rows therefore continue to deserialize cleanly; any invalid +//! values surface the next time a typed accessor re-validates them. + +use std::fmt; +use std::str::FromStr; + +use serde::{Deserialize, Serialize}; + +/// Shared maximum length for both credential and extension names. +/// +/// Matches the pre-newtype `is_valid_credential_name` bound; extension names +/// had no explicit length cap but fit comfortably within this limit in +/// practice. +pub const MAX_NAME_LEN: usize = 64; + +/// Why a candidate string is not a valid identity name. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub enum IdentityError { + #[error("identity name must not be empty")] + Empty, + #[error("identity name '{0}' exceeds {MAX_NAME_LEN} characters")] + TooLong(String), + #[error("identity name '{0}': must not contain path separators or traversal characters")] + PathTraversal(String), + #[error("identity name '{0}': only lowercase letters, digits, and underscores are allowed")] + InvalidChar(String), + #[error("identity name '{0}': must start and end with a lowercase letter or digit")] + EdgeUnderscore(String), + #[error("identity name '{0}': consecutive underscores are not allowed")] + ConsecutiveUnderscores(String), +} + +/// Validate `raw` against the shared rule and return its canonical form. +/// +/// The canonical form trims surrounding whitespace and replaces `-` with `_` +/// (extension names are invoked as Python attribute accesses, which forbid +/// hyphens). After that normalization the result must be: +/// +/// - non-empty, at most [`MAX_NAME_LEN`] bytes +/// - ASCII lowercase letters, digits, and `_` only +/// - not start or end with `_` +/// - no consecutive `__` +/// - no path separators (`/`, `\`), parent-traversal (`..`), or NUL +fn canonicalize(raw: &str) -> Result { + if raw.contains('/') || raw.contains('\\') || raw.contains("..") || raw.contains('\0') { + return Err(IdentityError::PathTraversal(raw.to_string())); + } + + let canonical = raw.trim().replace('-', "_"); + if canonical.is_empty() { + return Err(IdentityError::Empty); + } + if canonical.len() > MAX_NAME_LEN { + return Err(IdentityError::TooLong(canonical)); + } + + let bytes = canonical.as_bytes(); + if bytes.first() == Some(&b'_') || bytes.last() == Some(&b'_') { + return Err(IdentityError::EdgeUnderscore(canonical)); + } + + let mut prev_underscore = false; + for ch in canonical.chars() { + let is_valid = ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_'; + if !is_valid { + return Err(IdentityError::InvalidChar(canonical)); + } + if ch == '_' { + if prev_underscore { + return Err(IdentityError::ConsecutiveUnderscores(canonical)); + } + prev_underscore = true; + } else { + prev_underscore = false; + } + } + + Ok(canonical) +} + +macro_rules! identity_newtype { + ($(#[$meta:meta])* $name:ident) => { + $(#[$meta])* + #[derive( + Debug, + Clone, + PartialEq, + Eq, + Hash, + PartialOrd, + Ord, + Serialize, + Deserialize, + )] + #[serde(transparent)] + pub struct $name(String); + + impl $name { + /// Construct from any string-like value, validating + canonicalizing. + pub fn new(raw: impl AsRef) -> Result { + canonicalize(raw.as_ref()).map(Self) + } + + /// Construct without validation. + /// + /// Use for values sourced from a typed upstream that the caller + /// already trusts — a DB row, a skill-manifest registry entry, + /// a `#[serde(transparent)]` deserialization whose wire contract + /// predates the newtype. Prefer [`Self::new`] for anything + /// touching user input, free-form text, or external-tool output. + pub fn from_trusted(raw: String) -> Self { + Self(raw) + } + + /// Borrow the inner canonical string. + pub fn as_str(&self) -> &str { + &self.0 + } + + /// Consume and return the inner `String`. + pub fn into_inner(self) -> String { + self.0 + } + } + + impl fmt::Display for $name { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } + } + + impl AsRef for $name { + fn as_ref(&self) -> &str { + &self.0 + } + } + + impl std::ops::Deref for $name { + type Target = str; + fn deref(&self) -> &str { + &self.0 + } + } + + impl TryFrom<&str> for $name { + type Error = IdentityError; + fn try_from(value: &str) -> Result { + Self::new(value) + } + } + + impl TryFrom for $name { + type Error = IdentityError; + fn try_from(value: String) -> Result { + Self::new(value) + } + } + + impl FromStr for $name { + type Err = IdentityError; + fn from_str(s: &str) -> Result { + Self::new(s) + } + } + + impl From<$name> for String { + fn from(value: $name) -> String { + value.0 + } + } + + impl PartialEq for $name { + fn eq(&self, other: &str) -> bool { + self.0 == other + } + } + + impl PartialEq<&str> for $name { + fn eq(&self, other: &&str) -> bool { + self.0 == *other + } + } + }; +} + +identity_newtype! { + /// Backend secret identity — e.g. `telegram_bot_token`, `google_oauth_token`. + /// + /// Used as the lookup key in the secrets store, in gate resume payloads, + /// and anywhere the system needs to refer to *which* credential slot is + /// being filled. Must not be used as a UI routing key — that is + /// [`ExtensionName`]'s job. + CredentialName +} + +identity_newtype! { + /// User-facing extension/channel identity — e.g. `telegram`, `gmail`. + /// + /// Used to route onboarding UI, setup/configure modals, and Python action + /// dispatch. Hyphens in input are folded to underscores at construction + /// time because extensions are invoked as attribute accesses in the + /// embedded Python interpreter. Must not be used as a secrets-store key — + /// that is [`CredentialName`]'s job. + ExtensionName +} + +impl ExtensionName { + /// The pre-v0.23 hyphenated variant of this name, if one exists. + /// + /// Returns `Some("google-calendar")` for `google_calendar`, `None` for + /// names without underscores. Used when locating older release artifacts + /// on disk. + pub fn legacy_alias(&self) -> Option { + let alias = self.0.replace('_', "-"); + (alias != self.0).then_some(alias) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn accepts_snake_case() { + assert_eq!( + ExtensionName::new("google_drive").unwrap().as_str(), + "google_drive" + ); + assert_eq!( + CredentialName::new("telegram_bot_token").unwrap().as_str(), + "telegram_bot_token" + ); + } + + #[test] + fn folds_hyphens_to_underscores() { + assert_eq!( + ExtensionName::new("web-search").unwrap().as_str(), + "web_search" + ); + assert_eq!( + CredentialName::new("github-token").unwrap().as_str(), + "github_token" + ); + } + + #[test] + fn trims_whitespace() { + assert_eq!(ExtensionName::new(" gmail ").unwrap().as_str(), "gmail"); + } + + #[test] + fn rejects_empty_and_whitespace_only() { + assert_eq!(ExtensionName::new(""), Err(IdentityError::Empty)); + assert_eq!(ExtensionName::new(" "), Err(IdentityError::Empty)); + } + + #[test] + fn rejects_uppercase() { + assert!(matches!( + ExtensionName::new("WebSearch"), + Err(IdentityError::InvalidChar(_)) + )); + assert!(matches!( + CredentialName::new("GitHub_Token"), + Err(IdentityError::InvalidChar(_)) + )); + } + + #[test] + fn rejects_consecutive_underscores() { + assert!(matches!( + ExtensionName::new("bad__name"), + Err(IdentityError::ConsecutiveUnderscores(_)) + )); + } + + #[test] + fn rejects_edge_underscores() { + assert!(matches!( + ExtensionName::new("_leading"), + Err(IdentityError::EdgeUnderscore(_)) + )); + assert!(matches!( + ExtensionName::new("trailing_"), + Err(IdentityError::EdgeUnderscore(_)) + )); + } + + #[test] + fn rejects_path_traversal() { + assert!(matches!( + ExtensionName::new("../bad"), + Err(IdentityError::PathTraversal(_)) + )); + assert!(matches!( + ExtensionName::new("a/b"), + Err(IdentityError::PathTraversal(_)) + )); + assert!(matches!( + ExtensionName::new("with\0nul"), + Err(IdentityError::PathTraversal(_)) + )); + } + + #[test] + fn rejects_too_long() { + let long = "a".repeat(MAX_NAME_LEN + 1); + assert!(matches!( + ExtensionName::new(&long), + Err(IdentityError::TooLong(_)) + )); + } + + #[test] + fn rejects_invalid_chars() { + assert!(matches!( + CredentialName::new("foo bar"), + Err(IdentityError::InvalidChar(_)) + )); + assert!(matches!( + CredentialName::new("foo.bar"), + Err(IdentityError::InvalidChar(_)) + )); + } + + #[test] + fn serde_is_transparent() { + let ext = ExtensionName::new("gmail").unwrap(); + let json = serde_json::to_string(&ext).unwrap(); + assert_eq!(json, "\"gmail\""); + + let round: ExtensionName = serde_json::from_str("\"gmail\"").unwrap(); + assert_eq!(round.as_str(), "gmail"); + } + + /// `#[serde(transparent)]` means we do not re-validate at deserialize + /// time — legacy persisted rows must keep loading. Validation happens at + /// construction sites, not on the wire. + #[test] + fn serde_does_not_revalidate() { + let legacy: ExtensionName = serde_json::from_str("\"Bad__Name\"").unwrap(); + assert_eq!(legacy.as_str(), "Bad__Name"); + } + + #[test] + fn credential_and_extension_are_distinct_types() { + let cred = CredentialName::new("github_token").unwrap(); + let ext = ExtensionName::new("github").unwrap(); + + // Compile-time check — passing one where the other is expected must + // not compile. We assert the runtime shape and trust the type system + // for the rest. + assert_eq!(cred.as_str(), "github_token"); + assert_eq!(ext.as_str(), "github"); + } + + #[test] + fn legacy_alias_roundtrip() { + let ext = ExtensionName::new("google_calendar").unwrap(); + assert_eq!(ext.legacy_alias().as_deref(), Some("google-calendar")); + + let no_underscore = ExtensionName::new("gmail").unwrap(); + assert_eq!(no_underscore.legacy_alias(), None); + } + + #[test] + fn display_matches_inner() { + let ext = ExtensionName::new("gmail").unwrap(); + assert_eq!(format!("{ext}"), "gmail"); + } + + #[test] + fn partial_eq_with_str() { + let ext = ExtensionName::new("gmail").unwrap(); + assert_eq!(ext, *"gmail"); + assert_eq!(ext, "gmail"); + } + + #[test] + fn preserves_existing_credential_shape() { + // Every credential name used in the codebase today (as of the + // pre-newtype `parse_credential_name` tests) must still validate. + for ok in [ + "github_token", + "github_pat", + "slack_token", + "gmail_oauth", + "linear_token", + "telegram_bot_token", + "google_oauth_token", + ] { + assert!(CredentialName::new(ok).is_ok(), "expected {ok} to validate",); + } + } +} diff --git a/crates/ironclaw_common/src/lib.rs b/crates/ironclaw_common/src/lib.rs index 387cf17643d..2118fdb659b 100644 --- a/crates/ironclaw_common/src/lib.rs +++ b/crates/ironclaw_common/src/lib.rs @@ -1,10 +1,12 @@ //! Shared types and utilities for the IronClaw workspace. mod event; +mod identity; mod timezone; mod util; pub use event::{AppEvent, OnboardingStateDto, PlanStepDto, ToolDecisionDto}; +pub use identity::{CredentialName, ExtensionName, IdentityError, MAX_NAME_LEN}; pub use timezone::{ValidTimezone, deserialize_option_lenient}; pub use util::truncate_preview; diff --git a/crates/ironclaw_engine/src/executor/structured.rs b/crates/ironclaw_engine/src/executor/structured.rs index b40dac21873..bcfcca22d84 100644 --- a/crates/ironclaw_engine/src/executor/structured.rs +++ b/crates/ironclaw_engine/src/executor/structured.rs @@ -858,7 +858,7 @@ mod tests { call_id: "call_auth_1".into(), parameters: Box::new(serde_json::json!({"url": "https://api.github.com/repos"})), resume_kind: Box::new(crate::gate::ResumeKind::Authentication { - credential_name: "github_token".into(), + credential_name: ironclaw_common::CredentialName::new("github_token").unwrap(), instructions: "Provide your github_token token".into(), auth_url: None, }), @@ -941,7 +941,7 @@ mod tests { call_id: "call_1".into(), parameters: Box::new(serde_json::json!({})), resume_kind: Box::new(crate::gate::ResumeKind::Authentication { - credential_name: "api_key".into(), + credential_name: ironclaw_common::CredentialName::new("api_key").unwrap(), instructions: "Provide your api_key token".into(), auth_url: None, }), diff --git a/crates/ironclaw_engine/src/gate/mod.rs b/crates/ironclaw_engine/src/gate/mod.rs index 4b32c1410b7..1019fa2358a 100644 --- a/crates/ironclaw_engine/src/gate/mod.rs +++ b/crates/ironclaw_engine/src/gate/mod.rs @@ -17,6 +17,7 @@ pub mod tool_tier; use std::collections::HashSet; use async_trait::async_trait; +use ironclaw_common::CredentialName; use serde::{Deserialize, Serialize}; use crate::types::capability::ActionDef; @@ -49,7 +50,7 @@ pub enum ResumeKind { /// User must provide a credential (token, API key, OAuth flow). Authentication { /// Name of the credential that is missing. - credential_name: String, + credential_name: CredentialName, /// User-facing setup instructions. instructions: String, /// Optional OAuth URL for browser-based flows. @@ -161,7 +162,7 @@ mod tests { ); assert_eq!( ResumeKind::Authentication { - credential_name: "x".into(), + credential_name: CredentialName::new("x").unwrap(), instructions: "y".into(), auth_url: None, } diff --git a/src/bridge/auth_manager.rs b/src/bridge/auth_manager.rs index 0909a8c6d23..8fe8fca35b5 100644 --- a/src/bridge/auth_manager.rs +++ b/src/bridge/auth_manager.rs @@ -22,6 +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_skills::{SkillCredentialSpec, SkillRegistry}; /// Result of checking whether a tool call has the credentials it needs. @@ -39,7 +40,7 @@ pub enum AuthCheckResult { #[derive(Debug, Clone)] pub struct MissingCredential { /// Secret name in the secrets store (e.g., "github_token"). - pub credential_name: String, + pub credential_name: CredentialName, /// Human-readable setup instructions from the skill spec. pub setup_instructions: Option, /// Optional OAuth URL that should be opened in the browser. @@ -53,7 +54,7 @@ pub enum ToolReadiness { Ready, /// Tool needs auth (OAuth or manual token) before it can work. NeedsAuth { - credential_name: String, + credential_name: CredentialName, instructions: Option, auth_url: Option, }, @@ -78,7 +79,7 @@ pub enum LatentActionExecution { available_actions: Vec, }, NeedsAuth { - credential_name: String, + credential_name: CredentialName, instructions: String, auth_url: Option, }, @@ -220,7 +221,7 @@ impl AuthManager { "Failed to resolve credential during pre-flight auth — assuming missing" ); missing.push(MissingCredential { - credential_name: mapping.secret_name.clone(), + credential_name: CredentialName::from_trusted(mapping.secret_name.clone()), setup_instructions: None, auth_url: None, }); @@ -414,7 +415,9 @@ impl AuthManager { credential_name, .. }) => Ok(LatentActionExecution::NeedsAuth { - credential_name: credential_name.unwrap_or(latent.provider_extension), + credential_name: CredentialName::from_trusted( + credential_name.unwrap_or(latent.provider_extension), + ), instructions: auth .instructions() .unwrap_or("Complete authentication to continue.") @@ -450,7 +453,7 @@ impl AuthManager { }; MissingCredential { - credential_name: credential_name.to_string(), + credential_name: CredentialName::from_trusted(credential_name.to_string()), setup_instructions, auth_url, } diff --git a/src/bridge/effect_adapter.rs b/src/bridge/effect_adapter.rs index 9763428c636..52df396fd12 100644 --- a/src/bridge/effect_adapter.rs +++ b/src/bridge/effect_adapter.rs @@ -161,11 +161,13 @@ impl EffectBridgeAdapter { context.current_call_id.as_deref(), parameters, ironclaw_engine::ResumeKind::Authentication { - credential_name: output_value - .get("credential_name") - .and_then(|v| v.as_str()) - .unwrap_or(name) - .to_string(), + credential_name: ironclaw_common::CredentialName::from_trusted( + output_value + .get("credential_name") + .and_then(|v| v.as_str()) + .unwrap_or(name) + .to_string(), + ), instructions: output_value .get("instructions") .and_then(|v| v.as_str()) @@ -1076,7 +1078,9 @@ impl EffectBridgeAdapter { context.current_call_id.as_deref(), parameters, ironclaw_engine::ResumeKind::Authentication { - credential_name: cred_name.clone(), + credential_name: ironclaw_common::CredentialName::from_trusted( + cred_name.clone(), + ), instructions: format!("Provide your {} token", cred_name), auth_url: None, }, diff --git a/src/bridge/router.rs b/src/bridge/router.rs index 625891e32b6..e81df36afaf 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -2138,7 +2138,8 @@ pub async fn resolve_gate( } } } else if let Some(ref ss) = state.secrets_store { - let params = crate::secrets::CreateSecretParams::new(credential_name, &token); + let params = + crate::secrets::CreateSecretParams::new(credential_name.as_str(), &token); ss.create(&message.user_id, params) .await .map_err(|e| engine_err("secrets", e))?; @@ -3200,7 +3201,9 @@ async fn await_thread_outcome( display_parameters: None, description: format!("Authentication required for '{}'.", cred_name), resume_kind: ironclaw_engine::ResumeKind::Authentication { - credential_name: cred_name.clone(), + credential_name: ironclaw_common::CredentialName::from_trusted( + cred_name.clone(), + ), instructions: setup_hint.clone(), auth_url: None, }, @@ -5214,7 +5217,8 @@ mod tests { "alice", thread_id, ironclaw_engine::ResumeKind::Authentication { - credential_name: expected_extension_name.clone(), + credential_name: ironclaw_common::CredentialName::new(&expected_extension_name) + .unwrap(), instructions: "Sign in with Google".to_string(), auth_url: Some("https://example.test/oauth".to_string()), }, @@ -5371,7 +5375,7 @@ mod tests { "alice", thread_id, ironclaw_engine::ResumeKind::Authentication { - credential_name: "github".into(), + credential_name: ironclaw_common::CredentialName::new("github").unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -5607,7 +5611,7 @@ mod tests { "alice", thread_a, ironclaw_engine::ResumeKind::Authentication { - credential_name: "github_token".into(), + credential_name: ironclaw_common::CredentialName::new("github_token").unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -5620,7 +5624,7 @@ mod tests { "alice", thread_b, ironclaw_engine::ResumeKind::Authentication { - credential_name: "linear_token".into(), + credential_name: ironclaw_common::CredentialName::new("linear_token").unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -5657,7 +5661,7 @@ mod tests { "alice", thread_a, ironclaw_engine::ResumeKind::Authentication { - credential_name: "github_token".into(), + credential_name: ironclaw_common::CredentialName::new("github_token").unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -5670,7 +5674,7 @@ mod tests { "alice", thread_b, ironclaw_engine::ResumeKind::Authentication { - credential_name: "linear_token".into(), + credential_name: ironclaw_common::CredentialName::new("linear_token").unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -5708,7 +5712,8 @@ mod tests { thread_a, auth_request_id, ironclaw_engine::ResumeKind::Authentication { - credential_name: "telegram_bot_token".into(), + credential_name: ironclaw_common::CredentialName::new("telegram_bot_token") + .unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -5760,7 +5765,8 @@ mod tests { thread_id, request_id, ironclaw_engine::ResumeKind::Authentication { - credential_name: "telegram_bot_token".into(), + credential_name: ironclaw_common::CredentialName::new("telegram_bot_token") + .unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -5797,7 +5803,8 @@ mod tests { thread_id, request_id, ironclaw_engine::ResumeKind::Authentication { - credential_name: "telegram_bot_token".into(), + credential_name: ironclaw_common::CredentialName::new("telegram_bot_token") + .unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -6225,7 +6232,7 @@ mod tests { action_name: "shell".into(), parameters: serde_json::json!({"cmd": "ls"}), resume_kind: ironclaw_engine::ResumeKind::Authentication { - credential_name: "github_token".into(), + credential_name: ironclaw_common::CredentialName::new("github_token").unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -6234,7 +6241,8 @@ mod tests { "alice", thread.id, ironclaw_engine::ResumeKind::Authentication { - credential_name: "github_token".into(), + credential_name: ironclaw_common::CredentialName::new("github_token") + .unwrap(), instructions: "paste token".into(), auth_url: None, }, @@ -7073,7 +7081,7 @@ mod tests { fn clamp_auth_resume_kind_clamps_to_false() { // Auth resumes have no "always" semantics; clamp regardless. let rk = ironclaw_engine::ResumeKind::Authentication { - credential_name: "github_token".into(), + credential_name: ironclaw_common::CredentialName::new("github_token").unwrap(), instructions: String::new(), auth_url: None, }; diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index e78813b7518..5c245423d05 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -2908,7 +2908,7 @@ async fn pending_gate_extension_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.clone()) + Some(credential_name.as_str().to_string()) } async fn engine_pending_gate_info( @@ -4654,7 +4654,8 @@ mod tests { "tool_install", r#"{"name":"telegram"}"#, &ironclaw_engine::ResumeKind::Authentication { - credential_name: "telegram_bot_token".to_string(), + credential_name: ironclaw_common::CredentialName::new("telegram_bot_token") + .unwrap(), instructions: "paste token".to_string(), auth_url: None, }, @@ -4709,7 +4710,7 @@ mod tests { "notion_search", "{}", &ironclaw_engine::ResumeKind::Authentication { - credential_name: "notion_token".to_string(), + credential_name: ironclaw_common::CredentialName::new("notion_token").unwrap(), instructions: "paste token".to_string(), auth_url: None, }, diff --git a/src/extensions/naming.rs b/src/extensions/naming.rs index 5cef7d4d062..1303d2466d1 100644 --- a/src/extensions/naming.rs +++ b/src/extensions/naming.rs @@ -1,47 +1,16 @@ +use ironclaw_common::ExtensionName; + use crate::extensions::ExtensionError; +/// Validate and canonicalize an extension name. +/// +/// Thin wrapper around [`ExtensionName::new`] that adapts the identity-layer +/// error to [`ExtensionError::InstallFailed`] so existing callers don't +/// change. New code should prefer [`ExtensionName`] directly. pub fn canonicalize_extension_name(name: &str) -> Result { - if name.contains('/') || name.contains('\\') || name.contains("..") || name.contains('\0') { - return Err(ExtensionError::InstallFailed(format!( - "Invalid extension name '{name}': contains path separator or traversal characters" - ))); - } - - let canonical = name.trim().replace('-', "_"); - if canonical.is_empty() { - return Err(ExtensionError::InstallFailed( - "Invalid extension name: must not be empty".to_string(), - )); - } - - let bytes = canonical.as_bytes(); - if bytes.first() == Some(&b'_') || bytes.last() == Some(&b'_') { - return Err(ExtensionError::InstallFailed(format!( - "Invalid extension name '{name}': must start and end with a lowercase letter or digit" - ))); - } - - let mut prev_underscore = false; - for ch in canonical.chars() { - let is_valid = ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_'; - if !is_valid { - return Err(ExtensionError::InstallFailed(format!( - "Invalid extension name '{name}': only lowercase letters, digits, and underscores are allowed" - ))); - } - if ch == '_' { - if prev_underscore { - return Err(ExtensionError::InstallFailed(format!( - "Invalid extension name '{name}': consecutive underscores are not allowed" - ))); - } - prev_underscore = true; - } else { - prev_underscore = false; - } - } - - Ok(canonical) + ExtensionName::new(name) + .map(String::from) + .map_err(|e| ExtensionError::InstallFailed(format!("Invalid extension name: {e}"))) } pub fn normalize_extension_names(names: I) -> Vec diff --git a/tests/engine_v2_gate_integration.rs b/tests/engine_v2_gate_integration.rs index ce0cf01fd1f..c3fa2c8b284 100644 --- a/tests/engine_v2_gate_integration.rs +++ b/tests/engine_v2_gate_integration.rs @@ -215,7 +215,7 @@ impl EffectExecutor for InstallThenAliasEffects { call_id: "call_install_1".into(), parameters: Box::new(parameters), resume_kind: Box::new(ResumeKind::Authentication { - credential_name: "github".into(), + credential_name: ironclaw_common::CredentialName::new("github").unwrap(), instructions: "Authenticate GitHub".into(), auth_url: None, }), @@ -294,7 +294,7 @@ impl EffectExecutor for GateMockEffects { call_id: "call_gate_2".into(), parameters: Box::new(parameters), resume_kind: Box::new(ResumeKind::Authentication { - credential_name: "notion".into(), + credential_name: ironclaw_common::CredentialName::new("notion").unwrap(), instructions: "Authenticate your Notion workspace".into(), auth_url: None, }), @@ -323,7 +323,7 @@ impl EffectExecutor for GateMockEffects { call_id: "call_gate_2".into(), parameters: Box::new(parameters), resume_kind: Box::new(ResumeKind::Authentication { - credential_name: "test_api_key".into(), + credential_name: ironclaw_common::CredentialName::new("test_api_key").unwrap(), instructions: "Provide your API key".into(), auth_url: None, }), From 8bbb9633cee3278355c0cf4ee360068f6f2db8e6 Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 16:29:49 +0900 Subject: [PATCH 02/12] fix(common): address PR #2611 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes from Copilot, Gemini, and Claude reviews: - **identity.rs docs**: drop reference to a non-existent `validate()` re-validation API. Document that instances represent "passed validation at some point in history" rather than "guaranteed valid right now" — by design. - **effect_adapter.rs**: the `awaiting_authorization` / `awaiting_token` gate path was using `CredentialName::from_trusted` to wrap a value read straight out of a tool's JSON output. Tool output is external/untrusted; use `CredentialName::new` (validating) with a cascade: external → tool name → `from_trusted(tool_name)` as final fallback. Closes a credential-name shape-injection vector. - **canonicalize()**: reorder checks cheapest-first against the trimmed slice so invalid inputs reject without allocating a canonicalized `String`. `replace('-', "_")` is deferred until after the structural checks pass; since `-`/`_` are both one byte, the earlier length check stays valid. - **Remove `Deref`** from identity newtypes, keep `AsRef`. Auto-deref let `&cred_name` silently coerce to `&str`, which is exactly the implicit-conversion pattern these newtypes exist to prevent. Callers that had a `&CredentialName` where `&str` was expected now write `.as_str()` explicitly. Added a regression test for the accessor contract and updated the rule template in `.claude/rules/types.md` to document the decision. Declined one review item (Claude): the remaining `to_string()` calls inside `IdentityError` variants are on the exception path; the common invalid-input case no longer allocates twice after the canonicalize reorder, and errors must carry owned strings so they can escape the function. Regression coverage: 5035 lib tests + 18 identity tests (one new — `explicit_accessors_work`) pass. Zero clippy warnings. --- .claude/rules/types.md | 8 ++- crates/ironclaw_common/src/identity.rs | 67 ++++++++++++++++++-------- src/bridge/effect_adapter.rs | 22 ++++++--- src/bridge/router.rs | 4 +- src/channels/web/server.rs | 2 +- 5 files changed, 73 insertions(+), 30 deletions(-) diff --git a/.claude/rules/types.md b/.claude/rules/types.md index cc413fc90b3..7d64b5e40c7 100644 --- a/.claude/rules/types.md +++ b/.claude/rules/types.md @@ -95,9 +95,15 @@ impl MyId { pub fn as_str(&self) -> &str { &self.0 } } +impl AsRef for MyId { ... } // explicit via `.as_ref()` impl TryFrom for MyId { ... } // validating impl From for String { ... } // infallible -// Deliberately no `From` — that would silently bypass validation. +// Deliberately no `From` or `From<&str>` — infallible +// conversion would silently bypass validation. +// Deliberately no `Deref` — auto-deref would let +// `&my_id` silently coerce to `&str`, which is the implicit-conversion +// pattern this whole module exists to prevent. Use `.as_str()` / +// `.as_ref()` at the call site so the boundary is visible. ``` ## Don'ts diff --git a/crates/ironclaw_common/src/identity.rs b/crates/ironclaw_common/src/identity.rs index 726a2b98abd..70b3495d741 100644 --- a/crates/ironclaw_common/src/identity.rs +++ b/crates/ironclaw_common/src/identity.rs @@ -15,10 +15,14 @@ //! //! Both types use `#[serde(transparent)]` so the on-wire and on-disk //! representation is a plain JSON string — unchanged from when the fields -//! were `String`. Validation runs at construction (`try_from` / `from_str`) -//! and at any explicit `validate()` call, not at deserialize time. Legacy -//! persisted rows therefore continue to deserialize cleanly; any invalid -//! values surface the next time a typed accessor re-validates them. +//! were `String`. Validation runs only when constructing through the +//! validated entry points (`new` / `try_from` / `from_str`), not at +//! deserialize time. Legacy persisted rows therefore continue to +//! deserialize cleanly; an invalid value is only surfaced if a later +//! code path re-constructs the name through a validated entry point. +//! There is no re-validation API on an existing instance — by design, +//! the type represents "something that passed validation at some point +//! in its history" rather than "something guaranteed valid right now". use std::fmt; use std::str::FromStr; @@ -60,19 +64,29 @@ pub enum IdentityError { /// - not start or end with `_` /// - no consecutive `__` /// - no path separators (`/`, `\`), parent-traversal (`..`), or NUL +/// +/// Checks are ordered cheapest-first against the trimmed slice so that an +/// invalid input rejects without allocating a canonicalized `String`. +/// `replace('-', "_")` runs only after the structural checks pass; since +/// `-` and `_` are both one byte, it cannot change the already-checked +/// length, so the fast-path length check stays valid. fn canonicalize(raw: &str) -> Result { - if raw.contains('/') || raw.contains('\\') || raw.contains("..") || raw.contains('\0') { - return Err(IdentityError::PathTraversal(raw.to_string())); - } - - let canonical = raw.trim().replace('-', "_"); - if canonical.is_empty() { + let trimmed = raw.trim(); + if trimmed.is_empty() { return Err(IdentityError::Empty); } - if canonical.len() > MAX_NAME_LEN { - return Err(IdentityError::TooLong(canonical)); + if trimmed.len() > MAX_NAME_LEN { + return Err(IdentityError::TooLong(trimmed.to_string())); + } + if trimmed.contains('/') + || trimmed.contains('\\') + || trimmed.contains("..") + || trimmed.contains('\0') + { + return Err(IdentityError::PathTraversal(trimmed.to_string())); } + let canonical = trimmed.replace('-', "_"); let bytes = canonical.as_bytes(); if bytes.first() == Some(&b'_') || bytes.last() == Some(&b'_') { return Err(IdentityError::EdgeUnderscore(canonical)); @@ -148,19 +162,18 @@ macro_rules! identity_newtype { } } + // `AsRef` is intentionally implemented so callers can opt into + // a `&str` view through a method call (`.as_ref()` / `.as_str()`), + // which makes the boundary crossing visible in the source. We do + // *not* implement `Deref`: auto-deref would let + // `&credential_name` silently coerce to `&str`, which is exactly the + // implicit-conversion behaviour these newtypes exist to prevent. impl AsRef for $name { fn as_ref(&self) -> &str { &self.0 } } - impl std::ops::Deref for $name { - type Target = str; - fn deref(&self) -> &str { - &self.0 - } - } - impl TryFrom<&str> for $name { type Error = IdentityError; fn try_from(value: &str) -> Result { @@ -396,6 +409,22 @@ mod tests { assert_eq!(ext, "gmail"); } + /// Guards the decision to *not* implement `Deref`: + /// auto-deref would let `&ext_name` silently coerce to `&str`, which + /// is the implicit-conversion pattern the newtypes exist to prevent. + /// Callers must go through `.as_str()` / `.as_ref()` — both explicit. + /// If a future edit adds `Deref`, this test will still compile but + /// the doc contract is broken; the rule lives in + /// `.claude/rules/types.md`. + #[test] + fn explicit_accessors_work() { + let ext = ExtensionName::new("gmail").unwrap(); + let via_as_str: &str = ext.as_str(); + let via_as_ref: &str = ext.as_ref(); + assert_eq!(via_as_str, "gmail"); + assert_eq!(via_as_ref, "gmail"); + } + #[test] fn preserves_existing_credential_shape() { // Every credential name used in the codebase today (as of the diff --git a/src/bridge/effect_adapter.rs b/src/bridge/effect_adapter.rs index 52df396fd12..cfa251fcfd3 100644 --- a/src/bridge/effect_adapter.rs +++ b/src/bridge/effect_adapter.rs @@ -161,13 +161,21 @@ impl EffectBridgeAdapter { context.current_call_id.as_deref(), parameters, ironclaw_engine::ResumeKind::Authentication { - credential_name: ironclaw_common::CredentialName::from_trusted( - output_value - .get("credential_name") - .and_then(|v| v.as_str()) - .unwrap_or(name) - .to_string(), - ), + // Validate the tool-declared credential name — it is + // external/untrusted input. Fall back to the tool's + // own name (structurally trusted, from the registry) + // if the external value fails validation; if even the + // tool name fails (shouldn't happen in practice), + // preserve the legacy passthrough so the gate can + // still reach the user. + credential_name: output_value + .get("credential_name") + .and_then(|v| v.as_str()) + .and_then(|raw| ironclaw_common::CredentialName::new(raw).ok()) + .or_else(|| ironclaw_common::CredentialName::new(name).ok()) + .unwrap_or_else(|| { + ironclaw_common::CredentialName::from_trusted(name.to_string()) + }), instructions: output_value .get("instructions") .and_then(|v| v.as_str()) diff --git a/src/bridge/router.rs b/src/bridge/router.rs index e81df36afaf..5ced43d5a4a 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -122,7 +122,7 @@ async fn resolve_auth_gate_display_name( tools, &pending.action_name, &pending.parameters, - credential_name, + credential_name.as_str(), &pending.user_id, ) .await @@ -2018,7 +2018,7 @@ pub async fn resolve_gate( state.effect_adapter.tools(), &pending.action_name, &pending.parameters, - credential_name, + credential_name.as_str(), &message.user_id, ) .await; diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index 5c245423d05..1120f91246a 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -2898,7 +2898,7 @@ async fn pending_gate_extension_name( .resolve_extension_name_for_auth_flow( tool_name, &parsed_parameters, - credential_name, + credential_name.as_str(), user_id, ) .await, From 97dfc2988787b1c6c783c644ccabf2749dd090f7 Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 14:32:04 +0900 Subject: [PATCH 03/12] feat(common): apply ExtensionName newtype to fan-out sites (PR 2/2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #2611. Migrates the remaining stringly-typed extension_name and credential_name fields to use the ExtensionName and CredentialName newtypes introduced in ironclaw_common::identity. Fields now typed: - AppEvent::{OnboardingState, GateRequired, ExtensionStatus}.extension_name (serde transparent — wire format unchanged) - StatusUpdate::{AuthRequired, AuthCompleted}.extension_name - TuiEvent::{AuthRequired, AuthCompleted}.extension_name (adds ironclaw_common dep to ironclaw_tui) - PendingOAuthLaunchParams.extension_name - PendingOAuthFlow.extension_name - PendingAuth.extension_name, PendingAuthPrompt.extension_name - ParsedAuthData.extension_name, selected_auth_prompt tuple - emit_auth_required_status() and Session::enter_auth_mode() parameters - event_from_configure_result() parameter - resolve_extension_for_action() and resolve_auth_gate_display_name() return types - normalize_extension_name() return type PendingAuthPrompt::new is now infallible (accepts ExtensionName directly) since the identity validator carries the non-empty invariant the constructor used to re-check. The "blank extension name" rejection test moved out — that logic lives in ironclaw_common::identity tests. Test updates use `ExtensionName::new("...").unwrap()` at construction sites and `from_trusted(...)` where a trusted upstream string is being adapted. Every site is a compile-time audit of where the type was crossing a boundary untyped. Regression coverage: existing 5034 lib tests + 26 engine_v2_gate integration tests + 40 ironclaw_common tests all pass. Zero clippy warnings across all features. --- Cargo.lock | 1 + crates/ironclaw_common/src/event.rs | 11 +-- crates/ironclaw_tui/Cargo.toml | 1 + crates/ironclaw_tui/src/event.rs | 5 +- src/agent/dispatcher.rs | 106 ++++++++++------------------ src/agent/session.rs | 39 +++++----- src/agent/thread_ops.rs | 2 +- src/auth/mod.rs | 3 +- src/auth/oauth.rs | 3 +- src/bridge/auth_manager.rs | 4 +- src/bridge/router.rs | 41 ++++++----- src/channels/channel.rs | 5 +- src/channels/wasm/wrapper.rs | 6 +- src/channels/web/onboarding.rs | 10 ++- src/channels/web/responses_api.rs | 2 +- src/channels/web/server.rs | 36 +++++----- src/channels/web/types.rs | 10 +-- src/extensions/manager.rs | 22 +++--- 18 files changed, 150 insertions(+), 157 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5845c2fdb34..a0563d13f9c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4055,6 +4055,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 b86daf9d624..077d93c289c 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). @@ -128,7 +129,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, @@ -153,7 +154,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, @@ -248,7 +249,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, @@ -419,7 +420,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, @@ -490,7 +491,7 @@ mod tests { thread_id: None, }, AppEvent::ExtensionStatus { - extension_name: String::new(), + extension_name: ExtensionName::from_trusted(String::new()), status: String::new(), message: None, }, diff --git a/crates/ironclaw_tui/Cargo.toml b/crates/ironclaw_tui/Cargo.toml index df6289548b9..a8c624942a9 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 f6c28b59a4a..a27165efbe0 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/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 193547fd8af..1c3b6791bee 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, @@ -1078,7 +1079,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(); @@ -1326,7 +1327,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, @@ -1335,11 +1336,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; @@ -1351,9 +1349,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(), @@ -1366,25 +1364,16 @@ 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, - )?; Some(( - validated.extension_name.clone(), + pending.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(pending.extension_name), + instructions: pending.instructions, + auth_url: pending.auth_url, + setup_url: pending.setup_url, + awaiting_token: pending.awaiting_token, }, )) } @@ -1453,7 +1442,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, @@ -1476,7 +1465,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, ) { @@ -1500,7 +1489,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; @@ -1760,6 +1749,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; @@ -2257,13 +2247,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, }; @@ -2405,18 +2395,10 @@ 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, - }; - - assert!(restore_selected_auth_prompt(Some(pending)).is_none()); - } + // 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. #[test] fn test_detect_auth_awaiting_positive() { @@ -2507,39 +2489,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 e0663e0e02b..8b12fbdad9b 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, @@ -194,7 +194,7 @@ impl PendingAuth { #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct PendingAuthPrompt { /// Extension name to authenticate (must be non-empty, trimmed). - pub(crate) extension_name: String, + pub(crate) extension_name: ExtensionName, /// Optional instructions shown alongside the auth prompt. #[serde(default)] pub(crate) instructions: Option, @@ -210,26 +210,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 +468,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(), @@ -989,10 +986,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()); } @@ -1000,12 +997,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()); @@ -1015,7 +1012,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")); @@ -1025,14 +1022,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 d012ceceae0..fb89ad387e1 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -1984,7 +1984,7 @@ impl Agent { session: &Arc>, thread_id: Uuid, message: &IncomingMessage, - ext_name: String, + ext_name: ironclaw_common::ExtensionName, instructions: String, auth_data: &ParsedAuthData, ) { diff --git a/src/auth/mod.rs b/src/auth/mod.rs index 9be2f7c284d..c746ec98b91 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 2eb62408469..f19b7f81939 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/auth_manager.rs b/src/bridge/auth_manager.rs index 8fe8fca35b5..bdf4d61ae0f 100644 --- a/src/bridge/auth_manager.rs +++ b/src/bridge/auth_manager.rs @@ -597,7 +597,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 5ced43d5a4a..35bddc30205 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -87,21 +87,26 @@ async fn resolve_extension_for_action( parameters: &serde_json::Value, credential_fallback: &str, user_id: &str, -) -> String { - if let Some(auth_manager) = auth_manager { - return auth_manager +) -> ironclaw_common::ExtensionName { + let raw = if let Some(auth_manager) = auth_manager { + auth_manager .resolve_extension_name_for_auth_flow( action_name, parameters, credential_fallback, user_id, ) - .await; - } - tools - .provider_extension_for_tool(action_name) - .await - .unwrap_or_else(|| credential_fallback.to_string()) + .await + } else { + tools + .provider_extension_for_tool(action_name) + .await + .unwrap_or_else(|| credential_fallback.to_string()) + }; + // Resolver returns a trusted identity string — either a real extension + // name from the manager, the provider-extension hint off the tool, or + // the credential-name fallback when no extension owns the action. + ironclaw_common::ExtensionName::from_trusted(raw) } /// Resolve the user-facing name to use when surfacing an authentication @@ -112,7 +117,7 @@ async fn resolve_auth_gate_display_name( auth_manager: Option<&AuthManager>, tools: &crate::tools::ToolRegistry, pending: &PendingGate, -) -> String { +) -> ironclaw_common::ExtensionName { if let ironclaw_engine::ResumeKind::Authentication { credential_name, .. } = &pending.resume_kind @@ -127,9 +132,9 @@ async fn resolve_auth_gate_display_name( ) .await } else { - // Non-authentication gates don't use this string; return + // Non-authentication gates don't use this name; return // something innocuous. - pending.action_name.clone() + ironclaw_common::ExtensionName::from_trusted(pending.action_name.clone()) } } @@ -137,7 +142,7 @@ async fn send_pending_gate_status( agent: &Agent, message: &IncomingMessage, pending: &PendingGate, - auth_display_name: &str, + auth_display_name: &ironclaw_common::ExtensionName, ) { let display_parameters = gate_display_parameters(pending); @@ -168,7 +173,7 @@ async fn send_pending_gate_status( .send_status( &message.channel, StatusUpdate::AuthRequired { - extension_name: auth_display_name.to_string(), + extension_name: auth_display_name.clone(), instructions: Some(instructions.clone()), auth_url: auth_url.clone(), setup_url: None, @@ -3224,7 +3229,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, @@ -3579,7 +3586,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(), @@ -5254,7 +5261,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 85249cce4a0..c31d7445062 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; @@ -375,7 +376,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, @@ -383,7 +384,7 @@ pub enum StatusUpdate { }, /// Extension authentication completed. AuthCompleted { - extension_name: String, + extension_name: ExtensionName, success: bool, message: String, }, diff --git a/src/channels/wasm/wrapper.rs b/src/channels/wasm/wrapper.rs index 8c53d198c22..6da81b8c7de 100644 --- a/src/channels/wasm/wrapper.rs +++ b/src/channels/wasm/wrapper.rs @@ -5597,7 +5597,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, @@ -5760,7 +5760,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(), }, @@ -5783,7 +5783,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/onboarding.rs b/src/channels/web/onboarding.rs index 824b396a5e7..41bb5cedd7f 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/responses_api.rs b/src/channels/web/responses_api.rs index 787b52f7a56..fc2339a31ef 100644 --- a/src/channels/web/responses_api.rs +++ b/src/channels/web/responses_api.rs @@ -1754,7 +1754,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 1120f91246a..e909a04a9b2 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -2254,7 +2254,7 @@ 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 { @@ -2586,7 +2586,9 @@ async fn restore_pending_auth_mode( ) { 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(ironclaw_common::ExtensionName::from_trusted( + extension_name.to_string(), + )); } } @@ -3978,7 +3980,7 @@ async fn extensions_setup_submit_handler( let outcome = crate::channels::web::onboarding::classify_configure_result(&result); let mut onboarding_event = crate::channels::web::onboarding::event_from_configure_result( - name.clone(), + ironclaw_common::ExtensionName::from_trusted(name.clone()), &result, req.thread_id.clone(), ); @@ -4008,7 +4010,9 @@ async fn extensions_setup_submit_handler( .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? { onboarding_event = AppEvent::OnboardingState { - extension_name: name.clone(), + extension_name: ironclaw_common::ExtensionName::from_trusted( + name.clone(), + ), state: crate::channels::web::types::OnboardingStateDto::PairingRequired, request_id: Some(next_request_id), @@ -4163,7 +4167,7 @@ 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()), @@ -4181,7 +4185,7 @@ 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()), @@ -4904,7 +4908,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); @@ -4961,9 +4965,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); } @@ -5110,7 +5114,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); @@ -5666,7 +5670,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(), @@ -6691,7 +6695,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(), @@ -6763,7 +6767,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(), @@ -6876,7 +6880,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(), @@ -6966,7 +6970,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(), @@ -7050,7 +7054,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 668c44c7e05..82d9adf9596 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -1374,7 +1374,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, @@ -1399,7 +1399,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()), @@ -1421,7 +1421,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, @@ -1445,7 +1445,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, @@ -1470,7 +1470,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 3bbf5d05a3b..29f6c121da3 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -1436,7 +1436,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()), }); @@ -2177,7 +2177,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 => { @@ -3838,7 +3838,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(), @@ -4213,7 +4213,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(), @@ -4808,7 +4810,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(), @@ -4945,7 +4947,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 { @@ -7228,7 +7230,9 @@ impl ExtensionManager { sse.broadcast_for_user( user_id, ironclaw_common::AppEvent::OnboardingState { - extension_name: name.clone(), + extension_name: ironclaw_common::ExtensionName::from_trusted( + name.clone(), + ), state: ironclaw_common::OnboardingStateDto::PairingRequired, request_id: None, message: None, @@ -10160,7 +10164,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(), @@ -10187,7 +10191,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(), From cc7ea40dee8afd128fcf1d7df3efa3cf951c043a Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 17:05:17 +0900 Subject: [PATCH 04/12] fix(web): return ExtensionName from pending_gate_extension_name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Claude's review comment on #2611: the function was doing `Some(credential_name.as_str().to_string())` in the fallback branch, defeating the newtype's purpose by re-stringifying the identity. Return `Option` instead. Plumbs through `PendingGateInfo. extension_name` (wire format unchanged — `#[serde(transparent)]`). The fallback path's cross-identity conversion (credential name → extension name) is now an explicit `ExtensionName::from_trusted` call, making the boundary crossing visible at the call site. Also fixes the `Deref` removal fallout that followed the rebase onto the updated PR 1: call sites that relied on auto-deref (`ext.contains(...)`, `auth_manager.submit_auth_token(&cred_name, ...)`) now explicitly call `.as_str()`. --- src/agent/dispatcher.rs | 5 +++- src/agent/thread_ops.rs | 4 +-- src/bridge/router.rs | 4 +-- src/channels/web/server.rs | 57 +++++++++++++++++++++++--------------- src/channels/web/types.rs | 2 +- tests/e2e_live.rs | 5 ++-- 6 files changed, 47 insertions(+), 30 deletions(-) diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 1c3b6791bee..36ded10d10c 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -2442,7 +2442,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") diff --git a/src/agent/thread_ops.rs b/src/agent/thread_ops.rs index fb89ad387e1..5293f92ee4c 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -2090,11 +2090,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/bridge/router.rs b/src/bridge/router.rs index 35bddc30205..853e45730d8 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -2047,7 +2047,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) @@ -2077,7 +2077,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( diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index e909a04a9b2..64d503b8fbb 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -1839,7 +1839,7 @@ 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, ) @@ -2669,14 +2669,14 @@ 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; + restore_pending_auth_mode(&session, thread_id, pending_auth.extension_name.as_str()).await; return Err(( StatusCode::SERVICE_UNAVAILABLE, "Extension manager not available".to_string(), @@ -2702,7 +2702,8 @@ pub(crate) async fn handle_legacy_auth_token_submission( Ok(ActionResponse::ok(result.message)) } Ok(result) => { - restore_pending_auth_mode(&session, thread_id, &pending_auth.extension_name).await; + restore_pending_auth_mode(&session, thread_id, pending_auth.extension_name.as_str()) + .await; state.sse.broadcast_for_user( user_id, AppEvent::OnboardingState { @@ -2721,7 +2722,8 @@ pub(crate) async fn handle_legacy_auth_token_submission( } Err(crate::extensions::ExtensionError::ValidationFailed(_)) => { let message = "Invalid token. Please try again.".to_string(); - restore_pending_auth_mode(&session, thread_id, &pending_auth.extension_name).await; + restore_pending_auth_mode(&session, thread_id, pending_auth.extension_name.as_str()) + .await; state.sse.broadcast_for_user( user_id, AppEvent::OnboardingState { @@ -2739,7 +2741,8 @@ pub(crate) async fn handle_legacy_auth_token_submission( Ok(ActionResponse::fail(message)) } Err(error) => { - restore_pending_auth_mode(&session, thread_id, &pending_auth.extension_name).await; + restore_pending_auth_mode(&session, thread_id, pending_auth.extension_name.as_str()) + .await; let message = error.to_string(); state.sse.broadcast_for_user( user_id, @@ -2883,7 +2886,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 @@ -2895,22 +2898,29 @@ async fn pending_gate_extension_name( 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, - ); + let resolved = auth_manager + .resolve_extension_name_for_auth_flow( + tool_name, + &parsed_parameters, + credential_name.as_str(), + user_id, + ) + .await; + // The resolver returns a trusted identity string — either a real + // extension name from the manager, the provider-extension hint off + // the tool, or the credential-name fallback when no extension owns + // the action. Either way it's sourced from typed upstream state. + return Some(ironclaw_common::ExtensionName::from_trusted(resolved)); } // 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()) + // duplicating AuthManager resolution logic here. This is an explicit + // cross-identity conversion — acknowledged via `from_trusted` so the + // boundary crossing is visible. + Some(ironclaw_common::ExtensionName::from_trusted( + credential_name.as_str().to_string(), + )) } async fn engine_pending_gate_info( @@ -4666,7 +4676,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] @@ -4721,7 +4734,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. diff --git a/src/channels/web/types.rs b/src/channels/web/types.rs index 82d9adf9596..f8b82d6b37c 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -118,7 +118,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, } diff --git a/tests/e2e_live.rs b/tests/e2e_live.rs index f1fb8bb1b1a..4382e256591 100644 --- a/tests/e2e_live.rs +++ b/tests/e2e_live.rs @@ -376,7 +376,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, \ @@ -704,7 +704,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!( From 7af6a7ee6468894231b86a445e79fc55d546b891 Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 18:23:57 +0900 Subject: [PATCH 05/12] fix(router,web): address PR #2617 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four Gemini review comments, all on the boundary between credential/ extension identifiers and user input. 1. [HIGH, security] extensions_setup_submit_handler was wrapping the URL path segment in ExtensionName::from_trusted, which skips the newtype's path-traversal / invalid-character validation. That path is user-controlled (`/api/extensions/{name}/setup`). Validate with ExtensionName::new at the handler entry and return 400 on failure; downstream uses switch to .as_str() or .clone() of the validated value, and the three in-handler from_trusted sites disappear. 2. Rename resolve_auth_gate_display_name -> resolve_auth_gate_extension_name. The function returns an identifier/slug, not a human-readable display name — the old name was a leftover from when the value was a String. 3. Return Option from the renamed function. Previously the non-Authentication gate branch fabricated an ExtensionName::from_trusted(pending.action_name), which was semantically wrong (an action name is not an extension identifier) and silently defeated the type's invariants. Now it returns None for Approval/External gates, and callers thread an Option through. send_pending_gate_status accepts Option<&ExtensionName> and only uses it on the Authentication arm, with a warn! log if upstream plumbing ever reaches the arm with None. The GateRequired SSE event's extension_name is now a clean .clone() of the Option. 4. Rename auth_display_name -> extension_name on send_pending_gate_status so the parameter name matches both its type and the StatusUpdate::AuthRequired.extension_name field it feeds. Regression: new test_extensions_setup_submit_rejects_path_traversal_name at the handler tier (per .claude/rules/testing.md "Test Through the Caller, Not Just the Helper") drives the handler with malformed path segments and asserts 400 before the value reaches extension lookup or any from_trusted wrap. 5035 lib tests pass, zero clippy warnings. --- src/bridge/router.rs | 59 ++++++++++++++++------------ src/channels/web/server.rs | 80 +++++++++++++++++++++++++++++++++++--- 2 files changed, 108 insertions(+), 31 deletions(-) diff --git a/src/bridge/router.rs b/src/bridge/router.rs index 853e45730d8..253d0c1affa 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -109,19 +109,25 @@ async fn resolve_extension_for_action( ironclaw_common::ExtensionName::from_trusted(raw) } -/// 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, -) -> ironclaw_common::ExtensionName { - 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, @@ -130,19 +136,15 @@ async fn resolve_auth_gate_display_name( credential_name.as_str(), &pending.user_id, ) - .await - } else { - // Non-authentication gates don't use this name; return - // something innocuous. - ironclaw_common::ExtensionName::from_trusted(pending.action_name.clone()) - } + .await, + ) } async fn send_pending_gate_status( agent: &Agent, message: &IncomingMessage, pending: &PendingGate, - auth_display_name: &ironclaw_common::ExtensionName, + extension_name: Option<&ironclaw_common::ExtensionName>, ) { let display_parameters = gate_display_parameters(pending); @@ -168,12 +170,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.clone(), + extension_name: extension_name.clone(), instructions: Some(instructions.clone()), auth_url: auth_url.clone(), setup_url: None, @@ -363,7 +376,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!( @@ -388,11 +401,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 @@ -401,7 +410,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) } @@ -3312,13 +3321,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) } diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index 64d503b8fbb..7c85b7d85d5 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -3967,12 +3967,23 @@ 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) => { @@ -3990,7 +4001,7 @@ async fn extensions_setup_submit_handler( let outcome = crate::channels::web::onboarding::classify_configure_result(&result); let mut onboarding_event = crate::channels::web::onboarding::event_from_configure_result( - ironclaw_common::ExtensionName::from_trusted(name.clone()), + name.clone(), &result, req.thread_id.clone(), ); @@ -4014,15 +4025,13 @@ 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()))? { onboarding_event = AppEvent::OnboardingState { - extension_name: ironclaw_common::ExtensionName::from_trusted( - name.clone(), - ), + extension_name: name.clone(), state: crate::channels::web::types::OnboardingStateDto::PairingRequired, request_id: Some(next_request_id), @@ -5708,6 +5717,65 @@ 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() + ); + } + } + #[tokio::test] async fn test_extensions_setup_submit_returns_failure_when_not_activated() { use axum::body::Body; From a3d489f2113fd547bc5fd985e5b711c514554b1c Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 18:46:32 +0900 Subject: [PATCH 06/12] docs(identity): codify web-boundary rules + add static check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three rule additions + one enforcement hook covering the identity boundary that PR #2617 review uncovered: - src/channels/web/CLAUDE.md — extend "Unified Extension Onboarding" with explicit rules: * Setup/configure/activate routes MUST validate `{name}` via `ExtensionName::new` at handler entry (return 400 on failure). * Web DTOs and handlers MUST NOT reference `CredentialName` — credential identity is backend-only; the dispatcher/auth_manager resolves it from the ExtensionName server-side. * Auth-flow extension resolution happens in *one* place (`AuthManager::resolve_extension_name_for_auth_flow`). Wrappers are thin and delegate; they must not duplicate the precedence logic or re-derive from credential prefixes. The four recent identity bugs (#2561, #2473, #2512, #2574) were duplicate- resolution drift. - src/bridge/CLAUDE.md — new module spec documenting auth_manager.rs as the single authority for auth-flow extension resolution, with the resolver's four-step precedence order and the approved wrapper call sites. - scripts/pre-commit-safety.sh — new check #8 (CREDNAME): flags `CredentialName` references in newly-added production lines under `src/channels/web/**`. Test-mod code is excluded via the existing `strip_test_mod_lines` filter. Suppression via `// web-identity-exempt: ` for the rare legitimate case of reading an already-typed value off a backend struct. Smoke-tested: * baseline (current branch) — no warnings * injected violation — fires with CREDNAME warning * injected violation + `// web-identity-exempt:` — suppressed The rules and the check live at the same level — humans read the rule, CI enforces it. --- scripts/pre-commit-safety.sh | 33 +++++++++++++++++++++++++++++ src/bridge/CLAUDE.md | 41 ++++++++++++++++++++++++++++++++++++ src/channels/web/CLAUDE.md | 39 +++++++++++++++++++++++++++++++--- 3 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 src/bridge/CLAUDE.md diff --git a/scripts/pre-commit-safety.sh b/scripts/pre-commit-safety.sh index 5a48dafa7ed..5c9fbb63242 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/bridge/CLAUDE.md b/src/bridge/CLAUDE.md new file mode 100644 index 00000000000..cd68b342200 --- /dev/null +++ b/src/bridge/CLAUDE.md @@ -0,0 +1,41 @@ +# 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 + +**`AuthManager::resolve_extension_name_for_auth_flow(action_name, params, credential_fallback, user_id) -> String`** is the single authority that maps an auth gate or tool-call context to the installed extension identity. + +The resolver's precedence order (defined in `auth_manager.rs`): + +1. Explicit `name` param on `tool_install` / `tool_activate` / `tool_auth` invocations. +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 function (or a thin wrapper around it). The approved wrappers are: + +- `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. + +Both wrappers **delegate** to the canonical resolver; they must not duplicate its 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. + +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/channels/web/CLAUDE.md b/src/channels/web/CLAUDE.md index b8a22225b09..9680fd35a4c 100644 --- a/src/channels/web/CLAUDE.md +++ b/src/channels/web/CLAUDE.md @@ -126,13 +126,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`. From c813caa9f0118f26f658de0639f2b9961fc8689b Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 22:50:48 +0900 Subject: [PATCH 07/12] fix(auth): validate user-influenced names at the resolver boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses four Copilot review comments on PR #2617 that all pointed at the same seam: the canonical `AuthManager::resolve_extension_name_for_auth_flow` returned a raw `String` whose first branch (the LLM-supplied `name` parameter on `tool_install` / `tool_activate` / `tool_auth` actions) passed through without `ExtensionName` validation. Both call sites then wrapped the result in `ExtensionName::from_trusted`, promoting an unvalidated user-influenced value to a typed identity. - **Resolver now returns `ExtensionName`.** Branch 1 validates the user-controlled name via `ExtensionName::new` and falls through on failure; branches 2–4 use `from_trusted` because their sources (tool registry hint, canonicalizer, typed credential fallback) are already trusted upstream. This consolidates validation in the single "resolve once" site documented in `src/bridge/CLAUDE.md`. - **router.rs and server.rs drop their wraps.** `resolve_extension_for_action` (router) and `pending_gate_extension_name` (server) return the resolver's typed output directly. The tool-registry fallback in router.rs (no-auth-manager path) keeps its `from_trusted` wrap since it operates on the same trusted sources as branch 2. - **`restore_selected_auth_prompt` re-validates rehydrated prompts.** `PendingAuthPrompt` is `#[serde(transparent)]`, so deserialize does not re-check the inner `ExtensionName` string. A legacy-persisted invalid name would previously have been dropped by the old `PendingAuthPrompt::new(String, ...)` empty-string rejection; now `restore_selected_auth_prompt` re-runs `ExtensionName::new` and drops + warns on failure, upgrading the old non-empty-only check to the full identity invariant. New test `test_restore_selected_auth_prompt_rejects_invalid_legacy_row` forges three invalid rows (empty / uppercase / path-traversal) straight through serde and asserts each is dropped. - **Docstring on `PendingAuthPrompt` refreshed.** The old comment claimed `::new` "trims and validates extension_name is non-empty", which is no longer true — `::new` is infallible and the invariant lives in `ExtensionName` itself. The new comment documents the split: validation runs at `ExtensionName::new` construction and at restore-from-persistence, not inside `PendingAuthPrompt`. Regression: 5063 lib tests pass (+1 new). Clippy zero warnings. --- src/agent/dispatcher.rs | 48 ++++++++++++++++++++++++++++++++++++-- src/agent/session.rs | 17 ++++++++++---- src/bridge/auth_manager.rs | 47 ++++++++++++++++++++++++++----------- src/bridge/router.rs | 27 ++++++++++----------- src/channels/web/server.rs | 25 ++++++++++---------- 5 files changed, 118 insertions(+), 46 deletions(-) diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 176924b5a42..536f8e2da9e 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -1368,10 +1368,27 @@ pub(super) fn restore_selected_auth_prompt( pending: Option, ) -> Option<(ExtensionName, ParsedAuthData)> { let pending = pending?; + // 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(( - pending.extension_name.clone(), + extension_name.clone(), ParsedAuthData { - extension_name: Some(pending.extension_name), + extension_name: Some(extension_name), instructions: pending.instructions, auth_url: pending.auth_url, setup_url: pending.setup_url, @@ -2402,6 +2419,33 @@ mod tests { // lives in `ironclaw_common::identity` tests; there is no intermediate // stringly-typed rejection path in the prompt layer anymore. + /// 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] fn test_detect_auth_awaiting_positive() { let result: Result = Ok(serde_json::json!({ diff --git a/src/agent/session.rs b/src/agent/session.rs index a3d6e6fc1da..2e6c92ec00b 100644 --- a/src/agent/session.rs +++ b/src/agent/session.rs @@ -188,12 +188,21 @@ 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). + /// 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)] diff --git a/src/bridge/auth_manager.rs b/src/bridge/auth_manager.rs index bdf4d61ae0f..f7f94eccad0 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. @@ -324,41 +324,60 @@ 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`). + /// + /// Returns a validated [`CommonExtensionName`]. Branch 1 (the + /// LLM-supplied `name` parameter on `tool_install` / `tool_activate` / + /// `tool_auth` invocations) is user-influenced and must pass + /// `ExtensionName::new` before it can promote to a typed identity — + /// invalid values fall through to the next branch. Branches 2–4 are + /// sourced from internal state (tool registry, canonicalizer, + /// caller-supplied credential fallback) and use `from_trusted`. pub async fn resolve_extension_name_for_auth_flow( &self, action_name: &str, parameters: &serde_json::Value, credential_fallback: &str, user_id: &str, - ) -> String { + ) -> CommonExtensionName { + // 1. Explicit `name` param on install/activate/auth actions. This + // string comes from the model's tool arguments, so it must be + // validated — an invalid value (path traversal, uppercase, etc.) + // falls through to the next branch instead of tainting the + // typed identity. if matches!( 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(); - } + ) && let Some(raw) = parameters.get("name").and_then(|v| v.as_str()) + && let Ok(name) = CommonExtensionName::new(raw) + { + return name; } + // 2. Provider-extension hint declared by the tool itself. Sourced + // from the Rust tool registration (`Tool::provider_extension`), + // so the identity is trusted by the point it reaches here. if let Some(tools) = self.tools.as_ref() && let Some(name) = tools.provider_extension_for_tool(action_name).await { - return name; + return CommonExtensionName::from_trusted(name); } + // 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. 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; + return CommonExtensionName::from_trusted(canonical); } - credential_fallback.to_string() + // 4. Caller-supplied credential-name fallback (invariant: the caller + // passes the `CredentialName::as_str()` of a typed upstream + // value). This is the legacy "no extension owns the action" + // path — see CLAUDE.md "Extension/Auth Invariants". + CommonExtensionName::from_trusted(credential_fallback.to_string()) } pub async fn latent_extension_actions(&self) -> Vec { diff --git a/src/bridge/router.rs b/src/bridge/router.rs index a311fcfca13..2e278fddffb 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -88,25 +88,26 @@ async fn resolve_extension_for_action( credential_fallback: &str, user_id: &str, ) -> ironclaw_common::ExtensionName { - let raw = if let Some(auth_manager) = auth_manager { - auth_manager + 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, parameters, credential_fallback, user_id, ) - .await - } else { - tools - .provider_extension_for_tool(action_name) - .await - .unwrap_or_else(|| credential_fallback.to_string()) - }; - // Resolver returns a trusted identity string — either a real extension - // name from the manager, the provider-extension hint off the tool, or - // the credential-name fallback when no extension owns the action. - ironclaw_common::ExtensionName::from_trusted(raw) + .await; + } + // 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()); + ironclaw_common::ExtensionName::from_trusted(fallback) } /// Resolve the installed extension identifier that owns an authentication diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index 208a943cb4b..14bed847ea3 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -1877,19 +1877,18 @@ async fn pending_gate_extension_name( serde_json::from_str::(parameters).unwrap_or(serde_json::Value::Null); if let Some(auth_manager) = state.auth_manager.as_ref() { - let resolved = auth_manager - .resolve_extension_name_for_auth_flow( - tool_name, - &parsed_parameters, - credential_name.as_str(), - user_id, - ) - .await; - // The resolver returns a trusted identity string — either a real - // extension name from the manager, the provider-extension hint off - // the tool, or the credential-name fallback when no extension owns - // the action. Either way it's sourced from typed upstream state. - return Some(ironclaw_common::ExtensionName::from_trusted(resolved)); + // The resolver enforces identity validation internally and returns + // a typed `ExtensionName` — no wrap needed here. + return Some( + auth_manager + .resolve_extension_name_for_auth_flow( + tool_name, + &parsed_parameters, + credential_name.as_str(), + user_id, + ) + .await, + ); } // auth_manager is None only when no secrets backend exists (e.g. bare From 4841f5811eae3b6aae8fea7bee1796d19abc991c Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 23:09:48 +0900 Subject: [PATCH 08/12] fix(ci): adapt post-merge-from-staging sites to ExtensionName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Staging shipped #2640 (repl unlock) and gateway refactor commits after my last merge. The CI build picked them up via auto-merge and hit three type mismatches my branch hadn't seen: - src/channels/repl.rs:908 — new test constructs `StatusUpdate::AuthRequired { extension_name: "google_oauth_token" .to_string(), ... }`. Typed field; now `ExtensionName::new(...).unwrap()`. - src/channels/web/server.rs:1405-1424 — staging added a no-auth-manager fallback chain to `pending_gate_extension_name` that returned raw `Some(String)` on three branches. Aligned with `AuthManager::resolve_extension_name_for_auth_flow`: branch 1 (user-influenced `tool_install`/`tool_activate`/`tool_auth` `name` param) validates via `ExtensionName::new` and falls through on failure; branches 2-3 (provider-extension hint, credential-name fallback) use `from_trusted` because they're sourced from typed upstream state. Mirrors the fix applied to the canonical resolver in c813caa9. - src/channels/web/server.rs:3831 — test used `.as_deref()` on the function's Option return; switched to `.as_ref().map(|n| n.as_str())` matching the pattern from the adjacent test. No new logic — just adapting two staging landings to the typed surface PR #2617 introduces. The validation behaviour for the fallback path is already locked in by the identity-layer tests in `ironclaw_common::identity` (rejects_path_traversal, rejects_uppercase, etc.) and by the regression test added in c813caa9 (test_restore_selected_auth_prompt_rejects_invalid_legacy_row). [skip-regression-check] — type adaptation to unblock CI, no behaviour change needing its own regression test. Clippy with `-D warnings` clean, 5074 lib tests pass. --- src/channels/repl.rs | 2 +- src/channels/web/server.rs | 25 +++++++++++++++---------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/channels/repl.rs b/src/channels/repl.rs index a35941a1be6..7c04930a13a 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/web/server.rs b/src/channels/web/server.rs index 907473de164..f2494d10d2d 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -1394,6 +1394,11 @@ async fn pending_gate_extension_name( ); } + // No auth manager available (bare test harness). The resolver can't run, + // so mirror its precedence order inline. Keep the branches aligned with + // `AuthManager::resolve_extension_name_for_auth_flow` — branch 1 is + // user-influenced and must validate; branches 2-3 source from typed + // upstream state. if matches!( tool_name, "tool_install" @@ -1402,23 +1407,20 @@ async fn pending_gate_extension_name( | "tool-activate" | "tool_auth" | "tool-auth" - ) && let Some(name) = parsed_parameters.get("name").and_then(|v| v.as_str()) - && !name.trim().is_empty() + ) && let Some(raw) = parsed_parameters.get("name").and_then(|v| v.as_str()) + && let Ok(name) = ironclaw_common::ExtensionName::new(raw) { - return Some(name.to_string()); + return Some(name); } if let Some(tools) = state.tool_registry.as_ref() && let Some(name) = tools.provider_extension_for_tool(tool_name).await { - return Some(name); + return Some(ironclaw_common::ExtensionName::from_trusted(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. This is an explicit - // cross-identity conversion — acknowledged via `from_trusted` so the - // boundary crossing is visible. + // Final fallback: credential-name string. Explicit cross-identity + // conversion via `from_trusted` so the boundary crossing is visible. Some(ironclaw_common::ExtensionName::from_trusted( credential_name.as_str().to_string(), )) @@ -3828,7 +3830,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] From 714f72283cc1034fd3c890f648cf557764bd88cc Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 23:29:59 +0900 Subject: [PATCH 09/12] fix(auth): extract shared resolver; wrapper delegates instead of duplicating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two Copilot comments on PR #2617 that surfaced the same architectural issue: the no-auth-manager fallback in `pending_gate_extension_name` had grown a three-branch copy of the resolver's precedence that quietly skipped branch 3 (canonicalize action_name + check `ExtensionManager::extension_info`). Exactly the duplicate-resolution drift the "one resolver" rule in `src/bridge/CLAUDE.md` warns against — four prior identity bugs (#2561, #2473, #2512, #2574) were the same pattern. - Extracted `pub(crate) async fn resolve_auth_flow_extension_name` to `src/bridge/auth_manager.rs` as the single site of the four-branch precedence. Takes `Option<&ToolRegistry>` + `Option<&ExtensionManager>` so both the `AuthManager` method (which passes its own fields) and the web wrapper (which passes `state.tool_registry` / `state.extension_manager`) share identical logic. - `AuthManager::resolve_extension_name_for_auth_flow` is now a 1-block delegator. - `pending_gate_extension_name` in `web/server.rs` drops its inline fallback entirely and calls the shared free function from both branches. The bare-test-harness path now runs branch 3 (canonicalize + installed-extension check) that it previously missed. - Updated `src/bridge/CLAUDE.md` to document the free function as the single authority, the three approved wrappers as thin delegators, and the return type as `ExtensionName` (was stale `String` from the pre-c813caa9 era). Regression coverage: the existing `resolve_extension_name_for_auth_flow_prefers_installed_channel_name` test passes unchanged — it exercises branch 3 through the method, which now reaches it via the extracted free function. --- src/bridge/CLAUDE.md | 17 ++--- src/bridge/auth_manager.rs | 123 +++++++++++++++++++++++-------------- src/channels/web/server.rs | 63 ++++++------------- 3 files changed, 106 insertions(+), 97 deletions(-) diff --git a/src/bridge/CLAUDE.md b/src/bridge/CLAUDE.md index cd68b342200..55707125425 100644 --- a/src/bridge/CLAUDE.md +++ b/src/bridge/CLAUDE.md @@ -20,22 +20,25 @@ resolution — they call through these adapters. ## Auth-flow extension resolution: one place, no re-derivation -**`AuthManager::resolve_extension_name_for_auth_flow(action_name, params, credential_fallback, user_id) -> String`** is the single authority that maps an auth gate or tool-call context to the installed extension identity. +The single authority that maps an auth gate or tool-call context to the installed extension identity is the free function: -The resolver's precedence order (defined in `auth_manager.rs`): +**`bridge::auth_manager::resolve_auth_flow_extension_name(action_name, params, credential_fallback, user_id, tool_registry, extension_manager) -> ExtensionName`** -1. Explicit `name` param on `tool_install` / `tool_activate` / `tool_auth` invocations. +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 function (or a thin wrapper around it). The approved wrappers are: +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. +- `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. -Both wrappers **delegate** to the canonical resolver; they must not duplicate its precedence rules, reconstruct names from credential prefixes, or fall back to `format!()`-built strings. +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. +**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 f7f94eccad0..2bc7d42736d 100644 --- a/src/bridge/auth_manager.rs +++ b/src/bridge/auth_manager.rs @@ -100,6 +100,71 @@ 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. + if matches!( + action_name, + "tool_install" | "tool-install" | "tool_activate" | "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, @@ -325,13 +390,11 @@ impl AuthManager { /// while secrets remain stored under the declared credential name /// (for example `telegram_bot_token`). /// - /// Returns a validated [`CommonExtensionName`]. Branch 1 (the - /// LLM-supplied `name` parameter on `tool_install` / `tool_activate` / - /// `tool_auth` invocations) is user-influenced and must pass - /// `ExtensionName::new` before it can promote to a typed identity — - /// invalid values fall through to the next branch. Branches 2–4 are - /// sourced from internal state (tool registry, canonicalizer, - /// caller-supplied credential fallback) and use `from_trusted`. + /// 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, @@ -339,45 +402,15 @@ impl AuthManager { credential_fallback: &str, user_id: &str, ) -> CommonExtensionName { - // 1. Explicit `name` param on install/activate/auth actions. This - // string comes from the model's tool arguments, so it must be - // validated — an invalid value (path traversal, uppercase, etc.) - // falls through to the next branch instead of tainting the - // typed identity. - if matches!( + resolve_auth_flow_extension_name( action_name, - "tool_install" | "tool-install" | "tool_activate" | "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 declared by the tool itself. Sourced - // from the Rust tool registration (`Tool::provider_extension`), - // so the identity is trusted by the point it reaches here. - if let Some(tools) = self.tools.as_ref() - && let Some(name) = tools.provider_extension_for_tool(action_name).await - { - return CommonExtensionName::from_trusted(name); - } - - // 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. - 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 CommonExtensionName::from_trusted(canonical); - } - - // 4. Caller-supplied credential-name fallback (invariant: the caller - // passes the `CredentialName::as_str()` of a typed upstream - // value). This is the legacy "no extension owns the action" - // path — see CLAUDE.md "Extension/Auth Invariants". - CommonExtensionName::from_trusted(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 { diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index f2494d10d2d..584a8898d19 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -1379,51 +1379,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() { - // The resolver enforces identity validation internally and returns - // a typed `ExtensionName` — no wrap needed here. - return Some( - auth_manager - .resolve_extension_name_for_auth_flow( - tool_name, - &parsed_parameters, - credential_name.as_str(), - user_id, - ) - .await, - ); - } - - // No auth manager available (bare test harness). The resolver can't run, - // so mirror its precedence order inline. Keep the branches aligned with - // `AuthManager::resolve_extension_name_for_auth_flow` — branch 1 is - // user-influenced and must validate; branches 2-3 source from typed - // upstream state. - if matches!( - tool_name, - "tool_install" - | "tool-install" - | "tool_activate" - | "tool-activate" - | "tool_auth" - | "tool-auth" - ) && let Some(raw) = parsed_parameters.get("name").and_then(|v| v.as_str()) - && let Ok(name) = ironclaw_common::ExtensionName::new(raw) - { - return Some(name); - } - - if let Some(tools) = state.tool_registry.as_ref() - && let Some(name) = tools.provider_extension_for_tool(tool_name).await - { - return Some(ironclaw_common::ExtensionName::from_trusted(name)); - } - - // Final fallback: credential-name string. Explicit cross-identity - // conversion via `from_trusted` so the boundary crossing is visible. - Some(ironclaw_common::ExtensionName::from_trusted( - 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( From c9b1d04d0b481b9e1c786e0d59e837ec6b3fb518 Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sat, 18 Apr 2026 23:46:37 +0900 Subject: [PATCH 10/12] Merge remote-tracking branch 'origin/staging' into feat/identity-newtypes-pr2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picks up #2644 (platform/ extraction) and #2645 (features/oauth/ move). Manual resolutions: - src/channels/web/server.rs: staging removed 720 lines of OAuth callback code (moved to features/oauth/mod.rs in #2645). My PR 2 ExtensionName changes to two of those functions (oauth_callback_handler, slack_relay_oauth_callback_handler) ported to the new location. - src/bridge/auth_manager.rs: extended the shared resolver's branch-1 action pattern to include 'tool-activate' and 'tool-auth' variants, matching staging's new pending_gate_extension_name_uses_install_parameters_for_hyphenated_activate_tool test expectation. Underscore + hyphen variants for all three actions. No new PR 2 logic — just aligning the type surface with two staging refactors. 5074 lib tests pass (+1 vs previous — the new staging hyphenated-tool test). Clippy -D warnings clean. --- src/bridge/auth_manager.rs | 10 +- src/channels/web/CLAUDE.md | 7 +- src/channels/web/features/mod.rs | 14 + src/channels/web/features/oauth/mod.rs | 775 ++++++++++++++++++++++++ src/channels/web/mod.rs | 15 +- src/channels/web/{ => platform}/auth.rs | 0 src/channels/web/platform/mod.rs | 24 +- src/channels/web/platform/router.rs | 6 +- src/channels/web/{ => platform}/sse.rs | 0 src/channels/web/{ => platform}/ws.rs | 0 src/channels/web/server.rs | 732 +--------------------- 11 files changed, 834 insertions(+), 749 deletions(-) create mode 100644 src/channels/web/features/mod.rs create mode 100644 src/channels/web/features/oauth/mod.rs rename src/channels/web/{ => platform}/auth.rs (100%) rename src/channels/web/{ => platform}/sse.rs (100%) rename src/channels/web/{ => platform}/ws.rs (100%) diff --git a/src/bridge/auth_manager.rs b/src/bridge/auth_manager.rs index 2bc7d42736d..6e1961c6c6f 100644 --- a/src/bridge/auth_manager.rs +++ b/src/bridge/auth_manager.rs @@ -137,9 +137,17 @@ pub(crate) async fn resolve_auth_flow_extension_name( 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_auth" + "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) { diff --git a/src/channels/web/CLAUDE.md b/src/channels/web/CLAUDE.md index 05ed813ccc7..b52c712408a 100644 --- a/src/channels/web/CLAUDE.md +++ b/src/channels/web/CLAUDE.md @@ -12,10 +12,11 @@ Browser-facing HTTP API and SSE/WebSocket real-time streaming. Axum-based, singl | `platform/state.rs` | `GatewayState`, `RateLimiter`, `PerUserRateLimiter`, `WorkspacePool`, `FrontendHtmlCache`, `FrontendCacheKey`, `ActiveConfigSnapshot`, `PromptQueue`, `RoutineEngineSlot`. Canonical home for shared gateway state. | | `platform/static_files.rs` | CSP directive set + `BASE_CSP_HEADER` (single source of truth), frontend HTML bundle assembly (`build_frontend_html`), and the unauthenticated static handlers: `/`, `/style.css`, `/app.js`, `/theme.css`, `/favicon.ico`, `/i18n/*`, `/admin*`, `/api/health`, plus the authenticated `/projects/{id}/...` file-serving routes. | | `types.rs` | Request/response DTOs and `SseEvent` enum (source of truth for SSE contract) | -| `sse.rs` | `SseManager` — broadcast channel that fans out `SseEvent` to all connected SSE clients | -| `ws.rs` | WebSocket handler (`handle_ws_connection`) + `WsConnectionTracker` | -| `auth.rs` | Bearer token middleware (`Authorization: Bearer `) | +| `platform/sse.rs` | `SseManager` — broadcast channel that fans out `SseEvent` to all connected SSE clients. Re-exported as `channels::web::sse` for backward compat. | +| `platform/ws.rs` | WebSocket handler (`handle_ws_connection`) + `WsConnectionTracker`. Re-exported as `channels::web::ws`. | +| `platform/auth.rs` | Bearer token middleware (`Authorization: Bearer `) + DB-token + OIDC extractors. Re-exported as `channels::web::auth`. | | `log_layer.rs` | Tracing layer that tees log lines to the `/api/logs/events` SSE stream | +| `features/oauth/` | First feature slice landed per ironclaw#2599 stage 4a: OAuth callback (`/oauth/callback`), channel-relay event webhook (`/relay/events`), and the Slack-specific relay OAuth completion flow (`/oauth/slack/callback`). Owns its private helpers (`oauth_error_page`, `redact_oauth_state_for_logs`). | | `handlers/` | Feature handler functions split by domain: `auth`, `chat`, `engine`, `extensions`, `frontend`, `jobs`, `llm`, `memory`, `routines`, `secrets`, `settings`, `skills`, `system_prompt`, `tokens`, `tool_policy`, `users`, `webhooks`. Targeted for migration into `features//` per ironclaw#2599. | | `openai_compat.rs` | OpenAI-compatible proxy (`/v1/chat/completions`, `/v1/models`) | | `util.rs` | Shared helpers (`build_turns_from_db_messages`, `truncate_preview`) | diff --git a/src/channels/web/features/mod.rs b/src/channels/web/features/mod.rs new file mode 100644 index 00000000000..32f75db3893 --- /dev/null +++ b/src/channels/web/features/mod.rs @@ -0,0 +1,14 @@ +//! Feature slices for the web gateway. +//! +//! Each submodule under `features/` owns a vertical slice of +//! browser-facing behavior end-to-end: request/response types (shared +//! ones still live in [`super::types`] for now), handler functions, and +//! any slice-local helpers. Feature modules depend on `super::platform` +//! for shared state and extractors; they must not depend on one another. +//! +//! The older `handlers/` folder is a transitional fallback. Handlers +//! will migrate into `features//` incrementally — see +//! `src/channels/web/CLAUDE.md` for the staged plan tracked in +//! ironclaw#2599. + +pub(crate) mod oauth; diff --git a/src/channels/web/features/oauth/mod.rs b/src/channels/web/features/oauth/mod.rs new file mode 100644 index 00000000000..fa300823af3 --- /dev/null +++ b/src/channels/web/features/oauth/mod.rs @@ -0,0 +1,775 @@ +//! OAuth + channel-relay callback endpoints. +//! +//! This feature slice owns the three public gateway routes that receive +//! browser redirects or webhook callbacks for OAuth-style flows: +//! +//! - [`oauth_callback_handler`] — generic OAuth callback for installable +//! extensions. Looks up the pending flow by CSRF state, exchanges the +//! authorization code for tokens, persists them via the secrets store, +//! and optionally auto-activates the extension. +//! - [`relay_events_handler`] — HMAC-signed webhook from `channel-relay` +//! that delivers inbound events to the relay channel. +//! - [`slack_relay_oauth_callback_handler`] — Slack-specific completion +//! flow that consumes a nonce, stores the `team_id`, and activates +//! the relay channel. +//! +//! All three are PUBLIC routes — none require the bearer token. The +//! first two are registered under the `public` router in +//! `platform::router`; the Slack callback goes through the same router. +//! +//! The helpers below (`oauth_error_page`, `redact_oauth_state_for_logs`) +//! are slice-local and must not be called from outside this module. + +use std::sync::Arc; + +use axum::{ + Json, + extract::{Query, State}, + http::{HeaderMap, StatusCode}, + response::IntoResponse, +}; +use sha2::{Digest, Sha256}; + +use crate::channels::relay::DEFAULT_RELAY_NAME; +use crate::channels::web::platform::state::{GatewayState, rate_limit_key_from_headers}; +use crate::channels::web::server::clear_auth_mode; +use crate::channels::web::types::AppEvent; +use crate::channels::web::util::web_incoming_message; +use crate::extensions::naming::extension_name_candidates; +use crate::secrets::SecretConsumeResult; + +/// Render the CSRF / opaque-error landing page. +/// +/// Kept as a thin wrapper over [`crate::auth::oauth::landing_html`] so the +/// callback handlers never leak internal error text to the browser — +/// every failure path lands on the same generic "Authorization Failed" +/// page, and the real reason goes to the tracing log. +fn oauth_error_page(label: &str) -> axum::response::Response { + let html = crate::auth::oauth::landing_html(label, false); + axum::response::Html(html).into_response() +} + +/// Produce a log-safe fingerprint of an OAuth `state` parameter. +/// +/// The raw `state` is a one-time CSRF token linked to an in-flight flow. +/// Logging it verbatim would (a) leak the token to anyone with log access +/// during the flow's validity window, and (b) inflate cardinality in +/// structured log sinks. The returned string is the first 6 bytes of the +/// SHA-256 digest plus the original length — enough to correlate repeated +/// callbacks for the same token in a single log stream, but not enough +/// to recover the token itself. +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.iter().take(6) { + use std::fmt::Write as _; + let _ = write!(&mut short_hash, "{byte:02x}"); + } + format!("sha256:{short_hash}:len={}", state.len()) +} + +/// OAuth callback handler for the web gateway. +/// +/// This is a PUBLIC route (no Bearer token required) because OAuth providers +/// redirect the user's browser here. The `state` query parameter correlates +/// the callback with a pending OAuth flow registered by `start_wasm_oauth()`. +/// +/// Used on hosted instances where `IRONCLAW_OAUTH_CALLBACK_URL` points to +/// the gateway (e.g., `https://kind-deer.agent1.near.ai/oauth/callback`). +/// Local/desktop mode continues to use the TCP listener on port 9876. +pub(crate) async fn oauth_callback_handler( + State(state): State>, + Query(params): Query>, +) -> impl IntoResponse { + use crate::auth::oauth; + + // Check for error from OAuth provider (e.g., user denied consent) + if let Some(error) = params.get("error") { + let description = params + .get("error_description") + .cloned() + .unwrap_or_else(|| error.clone()); + return oauth_error_page(&description); + } + + let state_param = match params.get("state") { + Some(s) if !s.is_empty() => s.clone(), + _ => { + return oauth_error_page("IronClaw"); + } + }; + + let code = match params.get("code") { + Some(c) if !c.is_empty() => c.clone(), + _ => { + return oauth_error_page("IronClaw"); + } + }; + + // Look up the pending flow by CSRF state (atomic remove prevents replay) + let ext_mgr = match state.extension_manager.as_ref() { + Some(mgr) => mgr, + None => { + return oauth_error_page("IronClaw"); + } + }; + + let decoded_state = match oauth::decode_hosted_oauth_state(&state_param) { + Ok(decoded) => decoded, + Err(error) => { + let redacted_state = redact_oauth_state_for_logs(&state_param); + tracing::warn!( + state = %redacted_state, + error = %error, + "OAuth callback received with malformed state" + ); + clear_auth_mode(&state, &state.owner_id).await; + return oauth_error_page("IronClaw"); + } + }; + let lookup_key = decoded_state.flow_id.clone(); + + let flow = ext_mgr + .pending_oauth_flows() + .write() + .await + .remove(&lookup_key); + + let flow = match flow { + Some(f) => f, + None => { + let redacted_state = redact_oauth_state_for_logs(&state_param); + let redacted_lookup_key = redact_oauth_state_for_logs(&lookup_key); + tracing::warn!( + state = %redacted_state, + lookup_key = %redacted_lookup_key, + "OAuth callback received with unknown or expired state" + ); + return oauth_error_page("IronClaw"); + } + }; + + // Check flow expiry (5 minutes, matching TCP listener timeout) + if flow.created_at.elapsed() > oauth::OAUTH_FLOW_EXPIRY { + tracing::warn!( + extension = %flow.extension_name, + "OAuth flow expired" + ); + // Notify UI so auth card can show error instead of staying stuck + if let Some(ref sse) = flow.sse_manager { + sse.broadcast_for_user( + &flow.user_id, + AppEvent::OnboardingState { + extension_name: flow.extension_name.clone(), + state: crate::channels::web::types::OnboardingStateDto::Failed, + request_id: None, + message: Some("OAuth flow expired. Please try again.".to_string()), + instructions: None, + auth_url: None, + setup_url: None, + onboarding: None, + thread_id: None, + }, + ); + } + clear_auth_mode(&state, &flow.user_id).await; + return oauth_error_page(&flow.display_name); + } + + // Exchange the authorization code for tokens. + // Use the platform exchange proxy when configured, otherwise call the + // provider's token URL directly. + let exchange_proxy_url = oauth::exchange_proxy_url(); + + let result: Result<(), String> = async { + let token_response = if let Some(proxy_url) = &exchange_proxy_url { + let oauth_proxy_auth_token = flow.oauth_proxy_auth_token().unwrap_or_default(); + oauth::exchange_via_proxy(oauth::ProxyTokenExchangeRequest { + proxy_url, + gateway_token: oauth_proxy_auth_token, + token_url: &flow.token_url, + client_id: &flow.client_id, + client_secret: flow.client_secret.as_deref(), + code: &code, + redirect_uri: &flow.redirect_uri, + code_verifier: flow.code_verifier.as_deref(), + access_token_field: &flow.access_token_field, + extra_token_params: &flow.token_exchange_extra_params, + }) + .await + .map_err(|e| e.to_string())? + } else { + oauth::exchange_oauth_code_with_params( + &flow.token_url, + &flow.client_id, + flow.client_secret.as_deref(), + &code, + &flow.redirect_uri, + flow.code_verifier.as_deref(), + &flow.access_token_field, + &flow.token_exchange_extra_params, + ) + .await + .map_err(|e| e.to_string())? + }; + + // Validate the token before storing (catches wrong account, etc.) + if let Some(ref validation) = flow.validation_endpoint { + oauth::validate_oauth_token(&token_response.access_token, validation) + .await + .map_err(|e| e.to_string())?; + } + + // Store tokens encrypted in the secrets store + oauth::store_oauth_tokens( + flow.secrets.as_ref(), + &flow.user_id, + &flow.secret_name, + flow.provider.as_deref(), + &token_response.access_token, + token_response.refresh_token.as_deref(), + token_response.expires_in, + &flow.scopes, + ) + .await + .map_err(|e| e.to_string())?; + + // Persist the client_id for flows that need it after the session ends + // (for example DCR-based MCP refresh). + if let Some(ref client_id_secret) = flow.client_id_secret_name { + let params = crate::secrets::CreateSecretParams::new(client_id_secret, &flow.client_id) + .with_provider(flow.provider.as_ref().cloned().unwrap_or_default()); + flow.secrets + .create(&flow.user_id, params) + .await + .map_err(|e| { + tracing::warn!( + extension = %flow.extension_name, + secret_name = %client_id_secret, + error = %e, + "Failed to store OAuth client_id secret after callback" + ); + "failed to store client credentials".to_string() + })?; + } + + if let (Some(client_secret_name), Some(client_secret)) = ( + flow.client_secret_secret_name.as_ref(), + flow.client_secret.as_deref(), + ) { + let mut params = + crate::secrets::CreateSecretParams::new(client_secret_name, client_secret) + .with_provider(flow.provider.as_ref().cloned().unwrap_or_default()); + if let Some(expires_at) = flow.client_secret_expires_at + && let Some(dt) = + chrono::DateTime::::from_timestamp(expires_at as i64, 0) + { + params = params.with_expiry(dt); + } + flow.secrets + .create(&flow.user_id, params) + .await + .map_err(|e| { + tracing::warn!( + extension = %flow.extension_name, + secret_name = %client_secret_name, + error = %e, + "Failed to store OAuth client_secret secret after callback" + ); + "failed to store client credentials".to_string() + })?; + } + + Ok(()) + } + .await; + + let (success, message) = match &result { + Ok(()) => ( + true, + format!("{} authenticated successfully", flow.display_name), + ), + Err(e) => ( + false, + format!("{} authentication failed: {}", flow.display_name, e), + ), + }; + + match &result { + Ok(()) => { + tracing::info!( + extension = %flow.extension_name, + "OAuth completed successfully via gateway callback" + ); + } + Err(e) => { + tracing::warn!( + extension = %flow.extension_name, + error = %e, + "OAuth failed via gateway callback" + ); + } + } + + // 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, &flow.user_id).await; + + // After successful OAuth, auto-activate the extension so it moves + // from "Installed (Authenticate)" → "Active" without a second click. + // OAuth success is independent of activation — tokens are already stored. + // Report auth as successful and attempt activation as a bonus step. + let final_message = if success && flow.auto_activate_extension { + match ext_mgr + .ensure_extension_ready( + flow.extension_name.as_str(), + &flow.user_id, + crate::extensions::EnsureReadyIntent::ExplicitActivate, + ) + .await + { + Ok(crate::extensions::EnsureReadyOutcome::Ready { activation, .. }) => activation + .map(|result| result.message) + .unwrap_or_else(|| format!("{} authenticated successfully", flow.display_name)), + Ok(crate::extensions::EnsureReadyOutcome::NeedsAuth { auth, .. }) => auth + .instructions() + .map(String::from) + .unwrap_or_else(|| format!("{} authenticated successfully", flow.display_name)), + Ok(crate::extensions::EnsureReadyOutcome::NeedsSetup { instructions, .. }) => { + instructions + } + Err(e) => { + tracing::warn!( + extension = %flow.extension_name, + error = %e, + "Auto-activation after OAuth failed" + ); + format!( + "{} authenticated successfully. Activation failed: {}. Try activating manually.", + flow.display_name, e + ) + } + } + } else if success { + format!("{} authenticated successfully", flow.display_name) + } else { + message + }; + + // Broadcast event to notify the web UI + let extension_name = flow.extension_name.clone(); + if let Some(ref sse) = flow.sse_manager { + sse.broadcast_for_user( + &flow.user_id, + AppEvent::OnboardingState { + extension_name: flow.extension_name, + state: if success { + crate::channels::web::types::OnboardingStateDto::Ready + } else { + crate::channels::web::types::OnboardingStateDto::Failed + }, + request_id: None, + message: Some(final_message.clone()), + instructions: None, + auth_url: None, + setup_url: None, + onboarding: None, + thread_id: None, + }, + ); + } + + if success { + match crate::bridge::resolve_engine_auth_callback(&flow.user_id, &flow.secret_name).await { + Ok(crate::bridge::AuthCallbackContinuation::ResolveGateExternal { + channel, + thread_scope, + request_id, + }) => { + if let Some(tx) = state.msg_tx.read().await.as_ref().cloned() { + let callback = + crate::agent::submission::Submission::ExternalCallback { request_id }; + match serde_json::to_string(&callback) { + Ok(content) => { + let msg = web_incoming_message( + &channel, + &flow.user_id, + content, + thread_scope.as_deref(), + ); + if let Err(e) = tx.send(msg).await { + tracing::warn!( + extension = %extension_name, + user_id = %flow.user_id, + error = %e, + "Failed to resolve pending engine auth gate after OAuth callback" + ); + } + } + Err(e) => { + tracing::warn!( + extension = %extension_name, + user_id = %flow.user_id, + error = %e, + "Failed to serialize external callback submission" + ); + } + } + } + } + Ok(crate::bridge::AuthCallbackContinuation::ReplayMessage { + channel, + thread_scope, + content, + }) => { + if let Some(tx) = state.msg_tx.read().await.as_ref().cloned() { + let msg = web_incoming_message( + &channel, + &flow.user_id, + content, + thread_scope.as_deref(), + ); + if let Err(e) = tx.send(msg).await { + tracing::warn!( + extension = %extension_name, + user_id = %flow.user_id, + error = %e, + "Failed to replay pending engine auth request after OAuth callback" + ); + } + } + } + Ok(crate::bridge::AuthCallbackContinuation::None) => {} + Err(e) => { + tracing::warn!( + extension = %extension_name, + user_id = %flow.user_id, + error = %e, + "Failed to resume pending engine auth gate after OAuth callback" + ); + } + } + } + + let html = oauth::landing_html(&flow.display_name, success); + axum::response::Html(html).into_response() +} + +/// Webhook endpoint for receiving relay events from channel-relay. +/// +/// PUBLIC route — authenticated via HMAC signature (X-Relay-Signature header). +pub(crate) async fn relay_events_handler( + State(state): State>, + headers: axum::http::HeaderMap, + body: axum::body::Bytes, +) -> impl IntoResponse { + let ext_mgr = match state.extension_manager.as_ref() { + Some(mgr) => mgr, + None => { + return (StatusCode::SERVICE_UNAVAILABLE, "not ready").into_response(); + } + }; + + let signing_secret = match ext_mgr.relay_signing_secret() { + Some(s) => s, + None => { + return (StatusCode::SERVICE_UNAVAILABLE, "relay not configured").into_response(); + } + }; + + // Verify signature + let signature = match headers + .get("x-relay-signature") + .and_then(|v| v.to_str().ok()) + { + Some(s) => s.to_string(), + None => { + return (StatusCode::UNAUTHORIZED, "missing signature").into_response(); + } + }; + + let timestamp = match headers + .get("x-relay-timestamp") + .and_then(|v| v.to_str().ok()) + { + Some(t) => t.to_string(), + None => { + return (StatusCode::UNAUTHORIZED, "missing timestamp").into_response(); + } + }; + + // Check timestamp freshness (5 min window) + let ts: i64 = match timestamp.parse() { + Ok(t) => t, + Err(_) => { + return (StatusCode::BAD_REQUEST, "malformed timestamp").into_response(); + } + }; + let now = chrono::Utc::now().timestamp(); + if (now - ts).abs() > 300 { + return (StatusCode::UNAUTHORIZED, "stale timestamp").into_response(); + } + + // Verify HMAC: sha256(secret, timestamp + "." + body) + if !crate::channels::relay::webhook::verify_relay_signature( + &signing_secret, + ×tamp, + &body, + &signature, + ) { + return (StatusCode::UNAUTHORIZED, "invalid signature").into_response(); + } + + // Parse event + let event: crate::channels::relay::client::ChannelEvent = match serde_json::from_slice(&body) { + Ok(e) => e, + Err(e) => { + tracing::warn!(error = %e, "relay callback invalid JSON"); + return (StatusCode::BAD_REQUEST, "invalid JSON").into_response(); + } + }; + + // Push to relay channel + let event_tx_guard = ext_mgr.relay_event_tx(); + let event_tx = event_tx_guard.lock().await; + match event_tx.as_ref() { + Some(tx) => { + if let Err(e) = tx.try_send(event) { + tracing::warn!(error = %e, "relay event channel full or closed"); + return (StatusCode::SERVICE_UNAVAILABLE, "event queue full").into_response(); + } + } + None => { + return (StatusCode::SERVICE_UNAVAILABLE, "relay channel not active").into_response(); + } + } + + Json(serde_json::json!({"ok": true})).into_response() +} + +/// OAuth callback for Slack via channel-relay. +/// +/// This is a PUBLIC route (no Bearer token required) because channel-relay +/// redirects the user's browser here after Slack OAuth completes. +/// Query params: `provider`, `team_id`. +pub(crate) async fn slack_relay_oauth_callback_handler( + State(state): State>, + headers: HeaderMap, + Query(params): Query>, +) -> impl IntoResponse { + // Rate limit + let ip = rate_limit_key_from_headers(&headers); + if !state.oauth_rate_limiter.check(&ip) { + return axum::response::Html( + "\ +

Too Many Requests

\ +

Please try again later.

\ + " + .to_string(), + ) + .into_response(); + } + + // Validate team_id format: empty or T followed by alphanumeric (max 20 chars) + let team_id = params.get("team_id").cloned().unwrap_or_default(); + if !team_id.is_empty() { + let valid_team_id = team_id.len() <= 21 + && team_id.starts_with('T') + && team_id[1..].chars().all(|c| c.is_ascii_alphanumeric()); + if !valid_team_id { + return axum::response::Html( + "\ +

Error

Invalid callback parameters.

" + .to_string(), + ) + .into_response(); + } + } + + // Validate provider: must be "slack" (only supported provider) + let provider = params + .get("provider") + .cloned() + .unwrap_or_else(|| "slack".into()); + if provider != "slack" { + return axum::response::Html( + "\ +

Error

Invalid callback parameters.

" + .to_string(), + ) + .into_response(); + } + + let ext_mgr = match state.extension_manager.as_ref() { + Some(mgr) => mgr, + None => { + return axum::response::Html( + "\ +

Error

Extension manager not available.

" + .to_string(), + ) + .into_response(); + } + }; + + // Validate CSRF state parameter + let state_param = match params.get("state") { + Some(s) if !s.is_empty() && s.len() <= 128 => s.clone(), + _ => { + return axum::response::Html( + "\ +

Error

Invalid or expired authorization.

" + .to_string(), + ) + .into_response(); + } + }; + + let relay_names = extension_name_candidates(DEFAULT_RELAY_NAME); + let relay_extension_name = relay_names[0].clone(); + let mut nonce_consumed = false; + let mut last_lookup_error = None; + for relay_name in &relay_names { + let state_key = format!("relay:{relay_name}:oauth_state"); + match ext_mgr + .secrets() + .consume_if_matches(&state.owner_id, &state_key, &state_param) + .await + { + Ok(SecretConsumeResult::Matched) => { + nonce_consumed = true; + break; + } + Ok(SecretConsumeResult::Mismatched) => { + return axum::response::Html( + "\ +

Error

Invalid or expired authorization.

" + .to_string(), + ) + .into_response(); + } + Ok(SecretConsumeResult::NotFound) => {} + Err(e) => { + last_lookup_error = Some((state_key, e.to_string())); + } + } + } + if !nonce_consumed { + let attempted_state_keys = relay_names + .iter() + .map(|relay_name| format!("relay:{relay_name}:oauth_state")) + .collect::>(); + let (state_key, error) = match last_lookup_error { + Some((state_key, error)) => (Some(state_key), error), + None => ( + None, + "stored nonce not found under any relay state key".to_string(), + ), + }; + tracing::warn!( + owner_id = %state.owner_id, + state_key = ?state_key, + attempted_state_keys = ?attempted_state_keys, + state = %redact_oauth_state_for_logs(&state_param), + error = %error, + "relay OAuth callback: failed to retrieve stored nonce" + ); + return axum::response::Html( + "\ +

Error

Invalid or expired authorization.

" + .to_string(), + ) + .into_response(); + } + + let result: Result<(), String> = async { + let store = state.store.as_ref().ok_or_else(|| { + "Relay activation requires persistent settings storage; no-db mode is unsupported." + .to_string() + })?; + + // Store team_id in settings + let team_id_key = format!("relay:{}:team_id", relay_extension_name); + tracing::info!( + relay = %relay_extension_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 + .map_err(|e| { + tracing::error!( + relay = %relay_extension_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 = %relay_extension_name, + owner_id = %state.owner_id, + "relay OAuth callback: activating relay channel" + ); + ext_mgr + .activate_stored_relay(&relay_extension_name, &state.owner_id) + .await + .map_err(|e| format!("Failed to activate relay channel: {}", e))?; + + Ok(()) + } + .await; + + let (success, message) = match &result { + Ok(()) => (true, "Slack connected successfully!".to_string()), + Err(e) => { + tracing::error!(error = %e, "Slack relay OAuth callback failed"); + ( + false, + "Connection failed. Check server logs for details.".to_string(), + ) + } + }; + + // Broadcast event to notify the web UI. + state.sse.broadcast(AppEvent::OnboardingState { + extension_name: ironclaw_common::ExtensionName::from_trusted(relay_extension_name.clone()), + state: if success { + crate::channels::web::types::OnboardingStateDto::Ready + } else { + crate::channels::web::types::OnboardingStateDto::Failed + }, + request_id: None, + message: Some(message.clone()), + instructions: None, + auth_url: None, + setup_url: None, + onboarding: None, + thread_id: None, + }); + + if success { + axum::response::Html( + "\ +

Slack Connected!

\ +

You can close this tab and return to IronClaw.

\ + \ + " + .to_string(), + ) + .into_response() + } else { + axum::response::Html(format!( + "\ +

Connection Failed

\ +

{}

\ + ", + message + )) + .into_response() + } +} diff --git a/src/channels/web/mod.rs b/src/channels/web/mod.rs index 33e6490cb52..e1950490a40 100644 --- a/src/channels/web/mod.rs +++ b/src/channels/web/mod.rs @@ -14,19 +14,26 @@ //! ◄── GET / ───────────────── Static HTML/CSS/JS //! ``` -pub mod auth; +pub(crate) mod features; pub(crate) mod handlers; pub mod log_layer; pub mod oauth; pub(crate) mod onboarding; pub mod openai_compat; -pub(crate) mod platform; +pub mod platform; pub mod responses_api; pub mod server; -pub mod sse; pub mod types; pub(crate) mod util; -pub mod ws; + +// Backward-compat re-exports for the ironclaw#2599 migration. The auth, +// SSE, and WebSocket modules moved to `platform::*` in stage 3; every +// existing `crate::channels::web::{auth,sse,ws}::...` call site +// continues to resolve via these re-exports until a follow-up PR +// updates them directly. +pub use platform::auth; +pub use platform::sse; +pub use platform::ws; /// Test helpers for gateway integration tests. /// diff --git a/src/channels/web/auth.rs b/src/channels/web/platform/auth.rs similarity index 100% rename from src/channels/web/auth.rs rename to src/channels/web/platform/auth.rs diff --git a/src/channels/web/platform/mod.rs b/src/channels/web/platform/mod.rs index 00b42a14fb4..51e792bb200 100644 --- a/src/channels/web/platform/mod.rs +++ b/src/channels/web/platform/mod.rs @@ -1,22 +1,24 @@ //! Platform layer for the web gateway. //! -//! This submodule holds the gateway's transport and framing concerns: shared -//! state, the Axum route composition, static asset serving, and (in later -//! stages of ironclaw#2599) auth / SSE / WS. +//! This submodule holds the gateway's transport and framing concerns: +//! shared state, the Axum route composition, static asset serving, bearer +//! / OIDC auth, and the SSE / WebSocket broadcast fan-out. //! //! **Dependency direction.** Feature handlers (under `handlers/` today, //! `features//` later) depend on platform types (`GatewayState`, -//! rate limiters, auth extractors). Platform *submodules* do **not** -//! reach back into feature handlers — with the single, intentional -//! exception of [`router`], which is the composition point. The router -//! imports every feature handler it registers; that is its job. The -//! "no back-edges" rule enforced by future CI (ironclaw#2599 stage 5) -//! applies to `platform/state.rs`, `platform/static_files.rs`, and the -//! auth/SSE/WS modules once they move here — not to `router`, whose -//! whole purpose is to wire features onto the transport. +//! rate limiters, auth extractors, `SseManager`, `WsConnectionTracker`). +//! Platform *submodules* do **not** reach back into feature handlers — +//! with the single, intentional exception of [`router`], which is the +//! composition point. The router imports every feature handler it +//! registers; that is its job. The "no back-edges" rule enforced by +//! future CI (ironclaw#2599 stage 5) applies to every platform module +//! except `router`. //! //! See `src/channels/web/CLAUDE.md` for the staged migration plan. +pub mod auth; pub mod router; +pub mod sse; pub mod state; pub mod static_files; +pub mod ws; diff --git a/src/channels/web/platform/router.rs b/src/channels/web/platform/router.rs index c39e063b987..e6d5f84478d 100644 --- a/src/channels/web/platform/router.rs +++ b/src/channels/web/platform/router.rs @@ -74,6 +74,9 @@ use crate::channels::web::platform::static_files::{ // Feature handlers still inline in `server.rs` pending migration into // `features//`. Kept `pub(crate)` so the router can reference them // without exposing them outside the crate. +use crate::channels::web::features::oauth::{ + oauth_callback_handler, relay_events_handler, slack_relay_oauth_callback_handler, +}; use crate::channels::web::server::{ chat_approval_handler, chat_auth_cancel_handler, chat_auth_token_handler, chat_gate_resolve_handler, chat_history_handler, chat_new_thread_handler, chat_send_handler, @@ -81,8 +84,7 @@ use crate::channels::web::server::{ extensions_list_handler, extensions_readiness_handler, extensions_registry_handler, extensions_remove_handler, extensions_setup_handler, extensions_setup_submit_handler, extensions_tools_handler, gateway_status_handler, logs_events_handler, logs_level_get_handler, - logs_level_set_handler, oauth_callback_handler, pairing_approve_handler, pairing_list_handler, - relay_events_handler, routines_runs_handler, slack_relay_oauth_callback_handler, + logs_level_set_handler, pairing_approve_handler, pairing_list_handler, routines_runs_handler, }; /// Start the gateway HTTP server. diff --git a/src/channels/web/sse.rs b/src/channels/web/platform/sse.rs similarity index 100% rename from src/channels/web/sse.rs rename to src/channels/web/platform/sse.rs diff --git a/src/channels/web/ws.rs b/src/channels/web/platform/ws.rs similarity index 100% rename from src/channels/web/ws.rs rename to src/channels/web/platform/ws.rs diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index 584a8898d19..dba670a3770 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -20,13 +20,9 @@ use axum::{ }, }; use serde::Deserialize; -use sha2::{Digest, Sha256}; use tokio_stream::StreamExt; use uuid::Uuid; -use axum::http::HeaderMap; - -use crate::channels::relay::DEFAULT_RELAY_NAME; use crate::channels::web::auth::{AdminUser, AuthenticatedUser}; use crate::channels::web::types::*; use crate::channels::web::util::{ @@ -34,8 +30,6 @@ use crate::channels::web::util::{ enforce_generated_image_history_budget, tool_error_for_display, tool_result_preview, web_incoming_message, }; -use crate::extensions::naming::extension_name_candidates; -use crate::secrets::SecretConsumeResult; // --- Backward-compat re-exports for the ironclaw#2599 migration --- // @@ -51,728 +45,6 @@ pub use crate::channels::web::platform::state::{ PromptQueue, RateLimiter, RoutineEngineSlot, WorkspacePool, }; -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] { - use std::fmt::Write as _; - let _ = write!(&mut short_hash, "{byte:02x}"); - } - format!("sha256:{short_hash}:len={}", state.len()) -} - -/// Return an OAuth error landing page response. -fn oauth_error_page(label: &str) -> axum::response::Response { - let html = crate::auth::oauth::landing_html(label, false); - axum::response::Html(html).into_response() -} - -/// OAuth callback handler for the web gateway. -/// -/// This is a PUBLIC route (no Bearer token required) because OAuth providers -/// redirect the user's browser here. The `state` query parameter correlates -/// the callback with a pending OAuth flow registered by `start_wasm_oauth()`. -/// -/// Used on hosted instances where `IRONCLAW_OAUTH_CALLBACK_URL` points to -/// the gateway (e.g., `https://kind-deer.agent1.near.ai/oauth/callback`). -/// Local/desktop mode continues to use the TCP listener on port 9876. -pub(crate) async fn oauth_callback_handler( - State(state): State>, - Query(params): Query>, -) -> impl IntoResponse { - use crate::auth::oauth; - - // Check for error from OAuth provider (e.g., user denied consent) - if let Some(error) = params.get("error") { - let description = params - .get("error_description") - .cloned() - .unwrap_or_else(|| error.clone()); - return oauth_error_page(&description); - } - - let state_param = match params.get("state") { - Some(s) if !s.is_empty() => s.clone(), - _ => { - return oauth_error_page("IronClaw"); - } - }; - - let code = match params.get("code") { - Some(c) if !c.is_empty() => c.clone(), - _ => { - return oauth_error_page("IronClaw"); - } - }; - - // Look up the pending flow by CSRF state (atomic remove prevents replay) - let ext_mgr = match state.extension_manager.as_ref() { - Some(mgr) => mgr, - None => { - return oauth_error_page("IronClaw"); - } - }; - - let decoded_state = match oauth::decode_hosted_oauth_state(&state_param) { - Ok(decoded) => decoded, - Err(error) => { - let redacted_state = redact_oauth_state_for_logs(&state_param); - tracing::warn!( - state = %redacted_state, - error = %error, - "OAuth callback received with malformed state" - ); - clear_auth_mode(&state, &state.owner_id).await; - return oauth_error_page("IronClaw"); - } - }; - let lookup_key = decoded_state.flow_id.clone(); - - let flow = ext_mgr - .pending_oauth_flows() - .write() - .await - .remove(&lookup_key); - - let flow = match flow { - Some(f) => f, - None => { - let redacted_state = redact_oauth_state_for_logs(&state_param); - let redacted_lookup_key = redact_oauth_state_for_logs(&lookup_key); - tracing::warn!( - state = %redacted_state, - lookup_key = %redacted_lookup_key, - "OAuth callback received with unknown or expired state" - ); - return oauth_error_page("IronClaw"); - } - }; - - // Check flow expiry (5 minutes, matching TCP listener timeout) - if flow.created_at.elapsed() > oauth::OAUTH_FLOW_EXPIRY { - tracing::warn!( - extension = %flow.extension_name, - "OAuth flow expired" - ); - // Notify UI so auth card can show error instead of staying stuck - if let Some(ref sse) = flow.sse_manager { - sse.broadcast_for_user( - &flow.user_id, - AppEvent::OnboardingState { - extension_name: flow.extension_name.clone(), - state: crate::channels::web::types::OnboardingStateDto::Failed, - request_id: None, - message: Some("OAuth flow expired. Please try again.".to_string()), - instructions: None, - auth_url: None, - setup_url: None, - onboarding: None, - thread_id: None, - }, - ); - } - clear_auth_mode(&state, &flow.user_id).await; - return oauth_error_page(&flow.display_name); - } - - // Exchange the authorization code for tokens. - // Use the platform exchange proxy when configured, otherwise call the - // provider's token URL directly. - let exchange_proxy_url = oauth::exchange_proxy_url(); - - let result: Result<(), String> = async { - let token_response = if let Some(proxy_url) = &exchange_proxy_url { - let oauth_proxy_auth_token = flow.oauth_proxy_auth_token().unwrap_or_default(); - oauth::exchange_via_proxy(oauth::ProxyTokenExchangeRequest { - proxy_url, - gateway_token: oauth_proxy_auth_token, - token_url: &flow.token_url, - client_id: &flow.client_id, - client_secret: flow.client_secret.as_deref(), - code: &code, - redirect_uri: &flow.redirect_uri, - code_verifier: flow.code_verifier.as_deref(), - access_token_field: &flow.access_token_field, - extra_token_params: &flow.token_exchange_extra_params, - }) - .await - .map_err(|e| e.to_string())? - } else { - oauth::exchange_oauth_code_with_params( - &flow.token_url, - &flow.client_id, - flow.client_secret.as_deref(), - &code, - &flow.redirect_uri, - flow.code_verifier.as_deref(), - &flow.access_token_field, - &flow.token_exchange_extra_params, - ) - .await - .map_err(|e| e.to_string())? - }; - - // Validate the token before storing (catches wrong account, etc.) - if let Some(ref validation) = flow.validation_endpoint { - oauth::validate_oauth_token(&token_response.access_token, validation) - .await - .map_err(|e| e.to_string())?; - } - - // Store tokens encrypted in the secrets store - oauth::store_oauth_tokens( - flow.secrets.as_ref(), - &flow.user_id, - &flow.secret_name, - flow.provider.as_deref(), - &token_response.access_token, - token_response.refresh_token.as_deref(), - token_response.expires_in, - &flow.scopes, - ) - .await - .map_err(|e| e.to_string())?; - - // Persist the client_id for flows that need it after the session ends - // (for example DCR-based MCP refresh). - if let Some(ref client_id_secret) = flow.client_id_secret_name { - let params = crate::secrets::CreateSecretParams::new(client_id_secret, &flow.client_id) - .with_provider(flow.provider.as_ref().cloned().unwrap_or_default()); - flow.secrets - .create(&flow.user_id, params) - .await - .map_err(|e| { - tracing::warn!( - extension = %flow.extension_name, - secret_name = %client_id_secret, - error = %e, - "Failed to store OAuth client_id secret after callback" - ); - "failed to store client credentials".to_string() - })?; - } - - if let (Some(client_secret_name), Some(client_secret)) = ( - flow.client_secret_secret_name.as_ref(), - flow.client_secret.as_deref(), - ) { - let mut params = - crate::secrets::CreateSecretParams::new(client_secret_name, client_secret) - .with_provider(flow.provider.as_ref().cloned().unwrap_or_default()); - if let Some(expires_at) = flow.client_secret_expires_at - && let Some(dt) = - chrono::DateTime::::from_timestamp(expires_at as i64, 0) - { - params = params.with_expiry(dt); - } - flow.secrets - .create(&flow.user_id, params) - .await - .map_err(|e| { - tracing::warn!( - extension = %flow.extension_name, - secret_name = %client_secret_name, - error = %e, - "Failed to store OAuth client_secret secret after callback" - ); - "failed to store client credentials".to_string() - })?; - } - - Ok(()) - } - .await; - - let (success, message) = match &result { - Ok(()) => ( - true, - format!("{} authenticated successfully", flow.display_name), - ), - Err(e) => ( - false, - format!("{} authentication failed: {}", flow.display_name, e), - ), - }; - - match &result { - Ok(()) => { - tracing::info!( - extension = %flow.extension_name, - "OAuth completed successfully via gateway callback" - ); - } - Err(e) => { - tracing::warn!( - extension = %flow.extension_name, - error = %e, - "OAuth failed via gateway callback" - ); - } - } - - // 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, &flow.user_id).await; - - // After successful OAuth, auto-activate the extension so it moves - // from "Installed (Authenticate)" → "Active" without a second click. - // OAuth success is independent of activation — tokens are already stored. - // Report auth as successful and attempt activation as a bonus step. - let final_message = if success && flow.auto_activate_extension { - match ext_mgr - .ensure_extension_ready( - flow.extension_name.as_str(), - &flow.user_id, - crate::extensions::EnsureReadyIntent::ExplicitActivate, - ) - .await - { - Ok(crate::extensions::EnsureReadyOutcome::Ready { activation, .. }) => activation - .map(|result| result.message) - .unwrap_or_else(|| format!("{} authenticated successfully", flow.display_name)), - Ok(crate::extensions::EnsureReadyOutcome::NeedsAuth { auth, .. }) => auth - .instructions() - .map(String::from) - .unwrap_or_else(|| format!("{} authenticated successfully", flow.display_name)), - Ok(crate::extensions::EnsureReadyOutcome::NeedsSetup { instructions, .. }) => { - instructions - } - Err(e) => { - tracing::warn!( - extension = %flow.extension_name, - error = %e, - "Auto-activation after OAuth failed" - ); - format!( - "{} authenticated successfully. Activation failed: {}. Try activating manually.", - flow.display_name, e - ) - } - } - } else if success { - format!("{} authenticated successfully", flow.display_name) - } else { - message - }; - - // Broadcast event to notify the web UI - let extension_name = flow.extension_name.clone(); - if let Some(ref sse) = flow.sse_manager { - sse.broadcast_for_user( - &flow.user_id, - AppEvent::OnboardingState { - extension_name: flow.extension_name, - state: if success { - crate::channels::web::types::OnboardingStateDto::Ready - } else { - crate::channels::web::types::OnboardingStateDto::Failed - }, - request_id: None, - message: Some(final_message.clone()), - instructions: None, - auth_url: None, - setup_url: None, - onboarding: None, - thread_id: None, - }, - ); - } - - if success { - match crate::bridge::resolve_engine_auth_callback(&flow.user_id, &flow.secret_name).await { - Ok(crate::bridge::AuthCallbackContinuation::ResolveGateExternal { - channel, - thread_scope, - request_id, - }) => { - if let Some(tx) = state.msg_tx.read().await.as_ref().cloned() { - let callback = - crate::agent::submission::Submission::ExternalCallback { request_id }; - match serde_json::to_string(&callback) { - Ok(content) => { - let msg = web_incoming_message( - &channel, - &flow.user_id, - content, - thread_scope.as_deref(), - ); - if let Err(e) = tx.send(msg).await { - tracing::warn!( - extension = %extension_name, - user_id = %flow.user_id, - error = %e, - "Failed to resolve pending engine auth gate after OAuth callback" - ); - } - } - Err(e) => { - tracing::warn!( - extension = %extension_name, - user_id = %flow.user_id, - error = %e, - "Failed to serialize external callback submission" - ); - } - } - } - } - Ok(crate::bridge::AuthCallbackContinuation::ReplayMessage { - channel, - thread_scope, - content, - }) => { - if let Some(tx) = state.msg_tx.read().await.as_ref().cloned() { - let msg = web_incoming_message( - &channel, - &flow.user_id, - content, - thread_scope.as_deref(), - ); - if let Err(e) = tx.send(msg).await { - tracing::warn!( - extension = %extension_name, - user_id = %flow.user_id, - error = %e, - "Failed to replay pending engine auth request after OAuth callback" - ); - } - } - } - Ok(crate::bridge::AuthCallbackContinuation::None) => {} - Err(e) => { - tracing::warn!( - extension = %extension_name, - user_id = %flow.user_id, - error = %e, - "Failed to resume pending engine auth gate after OAuth callback" - ); - } - } - } - - let html = oauth::landing_html(&flow.display_name, success); - axum::response::Html(html).into_response() -} - -/// Webhook endpoint for receiving relay events from channel-relay. -/// -/// PUBLIC route — authenticated via HMAC signature (X-Relay-Signature header). -pub(crate) async fn relay_events_handler( - State(state): State>, - headers: axum::http::HeaderMap, - body: axum::body::Bytes, -) -> impl IntoResponse { - let ext_mgr = match state.extension_manager.as_ref() { - Some(mgr) => mgr, - None => { - return (StatusCode::SERVICE_UNAVAILABLE, "not ready").into_response(); - } - }; - - let signing_secret = match ext_mgr.relay_signing_secret() { - Some(s) => s, - None => { - return (StatusCode::SERVICE_UNAVAILABLE, "relay not configured").into_response(); - } - }; - - // Verify signature - let signature = match headers - .get("x-relay-signature") - .and_then(|v| v.to_str().ok()) - { - Some(s) => s.to_string(), - None => { - return (StatusCode::UNAUTHORIZED, "missing signature").into_response(); - } - }; - - let timestamp = match headers - .get("x-relay-timestamp") - .and_then(|v| v.to_str().ok()) - { - Some(t) => t.to_string(), - None => { - return (StatusCode::UNAUTHORIZED, "missing timestamp").into_response(); - } - }; - - // Check timestamp freshness (5 min window) - let ts: i64 = match timestamp.parse() { - Ok(t) => t, - Err(_) => { - return (StatusCode::BAD_REQUEST, "malformed timestamp").into_response(); - } - }; - let now = chrono::Utc::now().timestamp(); - if (now - ts).abs() > 300 { - return (StatusCode::UNAUTHORIZED, "stale timestamp").into_response(); - } - - // Verify HMAC: sha256(secret, timestamp + "." + body) - if !crate::channels::relay::webhook::verify_relay_signature( - &signing_secret, - ×tamp, - &body, - &signature, - ) { - return (StatusCode::UNAUTHORIZED, "invalid signature").into_response(); - } - - // Parse event - let event: crate::channels::relay::client::ChannelEvent = match serde_json::from_slice(&body) { - Ok(e) => e, - Err(e) => { - tracing::warn!(error = %e, "relay callback invalid JSON"); - return (StatusCode::BAD_REQUEST, "invalid JSON").into_response(); - } - }; - - // Push to relay channel - let event_tx_guard = ext_mgr.relay_event_tx(); - let event_tx = event_tx_guard.lock().await; - match event_tx.as_ref() { - Some(tx) => { - if let Err(e) = tx.try_send(event) { - tracing::warn!(error = %e, "relay event channel full or closed"); - return (StatusCode::SERVICE_UNAVAILABLE, "event queue full").into_response(); - } - } - None => { - return (StatusCode::SERVICE_UNAVAILABLE, "relay channel not active").into_response(); - } - } - - Json(serde_json::json!({"ok": true})).into_response() -} - -/// OAuth callback for Slack via channel-relay. -/// -/// This is a PUBLIC route (no Bearer token required) because channel-relay -/// redirects the user's browser here after Slack OAuth completes. -/// Query params: `provider`, `team_id`. -pub(crate) async fn slack_relay_oauth_callback_handler( - State(state): State>, - headers: HeaderMap, - Query(params): Query>, -) -> impl IntoResponse { - // Rate limit - let ip = rate_limit_key_from_headers(&headers); - if !state.oauth_rate_limiter.check(&ip) { - return axum::response::Html( - "\ -

Too Many Requests

\ -

Please try again later.

\ - " - .to_string(), - ) - .into_response(); - } - - // Validate team_id format: empty or T followed by alphanumeric (max 20 chars) - let team_id = params.get("team_id").cloned().unwrap_or_default(); - if !team_id.is_empty() { - let valid_team_id = team_id.len() <= 21 - && team_id.starts_with('T') - && team_id[1..].chars().all(|c| c.is_ascii_alphanumeric()); - if !valid_team_id { - return axum::response::Html( - "\ -

Error

Invalid callback parameters.

" - .to_string(), - ) - .into_response(); - } - } - - // Validate provider: must be "slack" (only supported provider) - let provider = params - .get("provider") - .cloned() - .unwrap_or_else(|| "slack".into()); - if provider != "slack" { - return axum::response::Html( - "\ -

Error

Invalid callback parameters.

" - .to_string(), - ) - .into_response(); - } - - let ext_mgr = match state.extension_manager.as_ref() { - Some(mgr) => mgr, - None => { - return axum::response::Html( - "\ -

Error

Extension manager not available.

" - .to_string(), - ) - .into_response(); - } - }; - - // Validate CSRF state parameter - let state_param = match params.get("state") { - Some(s) if !s.is_empty() && s.len() <= 128 => s.clone(), - _ => { - return axum::response::Html( - "\ -

Error

Invalid or expired authorization.

" - .to_string(), - ) - .into_response(); - } - }; - - let relay_names = extension_name_candidates(DEFAULT_RELAY_NAME); - let relay_extension_name = relay_names[0].clone(); - let mut nonce_consumed = false; - let mut last_lookup_error = None; - for relay_name in &relay_names { - let state_key = format!("relay:{relay_name}:oauth_state"); - match ext_mgr - .secrets() - .consume_if_matches(&state.owner_id, &state_key, &state_param) - .await - { - Ok(SecretConsumeResult::Matched) => { - nonce_consumed = true; - break; - } - Ok(SecretConsumeResult::Mismatched) => { - return axum::response::Html( - "\ -

Error

Invalid or expired authorization.

" - .to_string(), - ) - .into_response(); - } - Ok(SecretConsumeResult::NotFound) => {} - Err(e) => { - last_lookup_error = Some((state_key, e.to_string())); - } - } - } - if !nonce_consumed { - let attempted_state_keys = relay_names - .iter() - .map(|relay_name| format!("relay:{relay_name}:oauth_state")) - .collect::>(); - let (state_key, error) = match last_lookup_error { - Some((state_key, error)) => (Some(state_key), error), - None => ( - None, - "stored nonce not found under any relay state key".to_string(), - ), - }; - tracing::warn!( - owner_id = %state.owner_id, - state_key = ?state_key, - attempted_state_keys = ?attempted_state_keys, - state = %redact_oauth_state_for_logs(&state_param), - error = %error, - "relay OAuth callback: failed to retrieve stored nonce" - ); - return axum::response::Html( - "\ -

Error

Invalid or expired authorization.

" - .to_string(), - ) - .into_response(); - } - - let result: Result<(), String> = async { - let store = state.store.as_ref().ok_or_else(|| { - "Relay activation requires persistent settings storage; no-db mode is unsupported." - .to_string() - })?; - - // Store team_id in settings - let team_id_key = format!("relay:{}:team_id", relay_extension_name); - tracing::info!( - relay = %relay_extension_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 - .map_err(|e| { - tracing::error!( - relay = %relay_extension_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 = %relay_extension_name, - owner_id = %state.owner_id, - "relay OAuth callback: activating relay channel" - ); - ext_mgr - .activate_stored_relay(&relay_extension_name, &state.owner_id) - .await - .map_err(|e| format!("Failed to activate relay channel: {}", e))?; - - Ok(()) - } - .await; - - let (success, message) = match &result { - Ok(()) => (true, "Slack connected successfully!".to_string()), - Err(e) => { - tracing::error!(error = %e, "Slack relay OAuth callback failed"); - ( - false, - "Connection failed. Check server logs for details.".to_string(), - ) - } - }; - - // Broadcast event to notify the web UI. - state.sse.broadcast(AppEvent::OnboardingState { - extension_name: ironclaw_common::ExtensionName::from_trusted(relay_extension_name.clone()), - state: if success { - crate::channels::web::types::OnboardingStateDto::Ready - } else { - crate::channels::web::types::OnboardingStateDto::Failed - }, - request_id: None, - message: Some(message.clone()), - instructions: None, - auth_url: None, - setup_url: None, - onboarding: None, - thread_id: None, - }); - - if success { - axum::response::Html( - "\ -

Slack Connected!

\ -

You can close this tab and return to IronClaw.

\ - \ - " - .to_string(), - ) - .into_response() - } else { - axum::response::Html(format!( - "\ -

Connection Failed

\ -

{}

\ - ", - message - )) - .into_response() - } -} - // --- Chat handlers --- /// Convert web gateway `ImageData` to `IncomingAttachment` objects. @@ -2965,7 +2237,11 @@ mod tests { use super::*; use crate::agent::SessionManager; use crate::auth::oauth; + use crate::channels::relay::DEFAULT_RELAY_NAME; use crate::channels::web::auth::{CombinedAuthState, UserIdentity}; + use crate::channels::web::features::oauth::{ + oauth_callback_handler, slack_relay_oauth_callback_handler, + }; use crate::channels::web::handlers::llm::{ llm_list_models_handler, llm_test_connection_handler, }; From e49a586edbe23eb273bc3f3084306326cdf13b29 Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sun, 19 Apr 2026 19:33:51 +0900 Subject: [PATCH 11/12] fix(web): address PR #2617 round-3 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Copilot findings from the 2026-04-18 review: 1. `/api/extensions/{name}/{activate,remove,setup}` handlers accepted `Path` and forwarded it to the extension manager without validating path-traversal, invalid characters, or case — only `extensions_setup_submit_handler` had the `ExtensionName::new` guard. Applied the same boundary validation to all three siblings. 2. `restore_pending_auth_mode` took `extension_name: &str` and re-wrapped it with `ExtensionName::from_trusted`, re-introducing an unvalidated string boundary even though every caller already held an `ExtensionName` (`pending_auth.extension_name`). Changed the helper to accept `&ExtensionName` so the identity stays typed end-to-end; `from_trusted` is no longer needed here. Regression: added `test_extensions_sibling_handlers_reject_path_traversal_name` covering activate / remove / setup-GET with the same malformed slugs the setup-submit test already locks in (path traversal, slash in segment, uppercase, space, trailing underscore). Drives the handlers through axum routing so the boundary is exercised end-to-end. --- src/channels/web/server.rs | 130 ++++++++++++++++++++++++++++++++----- 1 file changed, 112 insertions(+), 18 deletions(-) diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index dba670a3770..7a7add10ba5 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -336,13 +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(ironclaw_common::ExtensionName::from_trusted( - extension_name.to_string(), - )); + thread.enter_auth_mode(extension_name.clone()); } } @@ -430,7 +428,7 @@ pub(crate) async fn handle_legacy_auth_token_submission( .configure_token(pending_auth.extension_name.as_str(), token, user_id) .await } else { - restore_pending_auth_mode(&session, thread_id, pending_auth.extension_name.as_str()).await; + restore_pending_auth_mode(&session, thread_id, &pending_auth.extension_name).await; return Err(( StatusCode::SERVICE_UNAVAILABLE, "Extension manager not available".to_string(), @@ -456,8 +454,7 @@ pub(crate) async fn handle_legacy_auth_token_submission( Ok(ActionResponse::ok(result.message)) } Ok(result) => { - restore_pending_auth_mode(&session, thread_id, pending_auth.extension_name.as_str()) - .await; + restore_pending_auth_mode(&session, thread_id, &pending_auth.extension_name).await; state.sse.broadcast_for_user( user_id, AppEvent::OnboardingState { @@ -476,8 +473,7 @@ pub(crate) async fn handle_legacy_auth_token_submission( } Err(crate::extensions::ExtensionError::ValidationFailed(_)) => { let message = "Invalid token. Please try again.".to_string(); - restore_pending_auth_mode(&session, thread_id, pending_auth.extension_name.as_str()) - .await; + restore_pending_auth_mode(&session, thread_id, &pending_auth.extension_name).await; state.sse.broadcast_for_user( user_id, AppEvent::OnboardingState { @@ -495,8 +491,7 @@ pub(crate) async fn handle_legacy_auth_token_submission( Ok(ActionResponse::fail(message)) } Err(error) => { - restore_pending_auth_mode(&session, thread_id, pending_auth.extension_name.as_str()) - .await; + restore_pending_auth_mode(&session, thread_id, &pending_auth.extension_name).await; let message = error.to_string(); state.sse.broadcast_for_user( user_id, @@ -1656,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" ); @@ -1668,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)) } @@ -1688,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()))), } @@ -1768,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()))?; @@ -1782,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, @@ -4170,6 +4191,79 @@ mod tests { } } + /// 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; From 2a6b093fccd26b4b6b3c47d3dcd528dedfeee913 Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Sun, 19 Apr 2026 19:46:55 +0900 Subject: [PATCH 12/12] fix(ci): adapt replay_outcome to ExtensionName after staging merge Staging #2621 added `tests/support/replay_outcome.rs`, which destructures `StatusUpdate::{AuthRequired,AuthCompleted}.extension_name` into a `String` field of `EventSummary`. This PR made those `StatusUpdate` fields `ExtensionName`, so the post-merge build breaks in the replay snapshot gate and all-features clippy jobs. Convert to `String` at the destructure via `ExtensionName::into()` so the `EventSummary` shape (and the persisted `.snap` files) stay unchanged. The test-support / snapshot wire format is a legitimate String boundary per `.claude/rules/types.md`. --- tests/support/replay_outcome.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/support/replay_outcome.rs b/tests/support/replay_outcome.rs index 8f28a2ce8f2..8ec5077666c 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, } }