diff --git a/FEATURE_PARITY.md b/FEATURE_PARITY.md index 85348de5..e0002a41 100644 --- a/FEATURE_PARITY.md +++ b/FEATURE_PARITY.md @@ -465,7 +465,7 @@ This document tracks feature parity between IronClaw (Rust implementation) and O | Device pairing | ✅ | ❌ | | | Tailscale identity | ✅ | ❌ | | | Trusted-proxy auth | ✅ | ❌ | Header-based reverse proxy auth | -| OAuth flows | ✅ | 🚧 | NEAR AI OAuth | +| OAuth flows | ✅ | 🚧 | NEAR AI OAuth plus hosted extension/MCP OAuth broker; external auth-proxy rollout still pending | | DM pairing verification | ✅ | ✅ | ironclaw pairing approve, host APIs | | Allowlist/blocklist | ✅ | 🚧 | allow_from + pairing store | | Per-group tool policies | ✅ | ❌ | | diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index ab697951..ea3341c0 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -19,6 +19,7 @@ use axum::{ routing::{get, post}, }; use serde::Deserialize; +use sha2::{Digest, Sha256}; use tokio::sync::{mpsc, oneshot}; use tokio_stream::StreamExt; use tower_http::cors::{AllowHeaders, CorsLayer}; @@ -63,6 +64,16 @@ pub type PromptQueue = Arc< pub type RoutineEngineSlot = Arc>>>; +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()) +} + /// Simple sliding-window rate limiter. /// /// Tracks the number of requests in the current window. Resets when the window expires. @@ -566,22 +577,35 @@ async fn oauth_callback_handler( } }; - // Strip instance prefix from state for registry lookup. - // Platform nginx sends `state=instance:nonce` but flows are keyed by nonce only. - let lookup_key = oauth_defaults::strip_instance_prefix(&state_param); + let decoded_state = match oauth_defaults::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).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); + .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 = %state_param, - lookup_key = %lookup_key, + state = %redacted_state, + lookup_key = %redacted_lookup_key, "OAuth callback received with unknown or expired state" ); clear_auth_mode(&state).await; @@ -608,33 +632,29 @@ async fn oauth_callback_handler( } // Exchange the authorization code for tokens. - // Use the platform exchange proxy when configured (keeps client_secret off container), - // otherwise call the provider's token URL directly. - let exchange_proxy_url = std::env::var("IRONCLAW_OAUTH_EXCHANGE_URL").ok(); + // Use the platform exchange proxy when configured, otherwise call the + // provider's token URL directly. + let exchange_proxy_url = oauth_defaults::exchange_proxy_url(); let result: Result<(), String> = async { - let token_response = if let (Some(proxy_url), None) = (&exchange_proxy_url, &flow.resource) - { - // Use the platform exchange proxy when configured and no resource - // parameter is needed. The proxy holds client_secret server-side so - // the container never sees it. MCP flows (resource.is_some()) bypass - // the proxy because it doesn't forward the RFC 8707 resource param. + let token_response = if let Some(proxy_url) = &exchange_proxy_url { let gateway_token = flow.gateway_token.as_deref().unwrap_or_default(); - oauth_defaults::exchange_via_proxy( + oauth_defaults::exchange_via_proxy(oauth_defaults::ProxyTokenExchangeRequest { proxy_url, gateway_token, - &code, - &flow.redirect_uri, - flow.code_verifier.as_deref(), - &flow.access_token_field, - ) + 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 { - // Direct token exchange: uses exchange_oauth_code_with_resource so MCP - // flows can include the RFC 8707 `resource` parameter to scope the - // issued token to the specific MCP server. - oauth_defaults::exchange_oauth_code_with_resource( + oauth_defaults::exchange_oauth_code_with_params( &flow.token_url, &flow.client_id, flow.client_secret.as_deref(), @@ -642,7 +662,7 @@ async fn oauth_callback_handler( &flow.redirect_uri, flow.code_verifier.as_deref(), &flow.access_token_field, - flow.resource.as_deref(), + &flow.token_exchange_extra_params, ) .await .map_err(|e| e.to_string())? @@ -669,10 +689,8 @@ async fn oauth_callback_handler( .await .map_err(|e| e.to_string())?; - // For MCP OAuth flows (identified by resource field), persist the - // client_id so token refresh works without re-authentication. - // The CLI flow stores this in authorize_mcp_server(); the gateway - // callback must do the same. + // 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()); @@ -3311,7 +3329,7 @@ mod tests { secrets, sse_sender: None, gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at, }; @@ -3379,7 +3397,7 @@ mod tests { secrets, sse_sender: Some(sender), gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at, }; @@ -3482,7 +3500,7 @@ mod tests { secrets, sse_sender: None, gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, // Expired — handler will reject after lookup (no network I/O) created_at, @@ -3534,6 +3552,85 @@ mod tests { ); } + #[tokio::test] + async fn test_oauth_callback_accepts_versioned_hosted_state() { + use axum::body::Body; + use tower::ServiceExt; + + let secrets: Arc = + Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new( + crate::secrets::SecretsCrypto::new(secrecy::SecretString::from( + TEST_GATEWAY_CRYPTO_KEY.to_string(), + )) + .expect("crypto"), + ))); + let (ext_mgr, _wasm_tools_dir, _wasm_channels_dir) = test_ext_mgr(secrets.clone()); + + let Some(created_at) = expired_flow_created_at() else { + eprintln!("Skipping versioned OAuth state test: monotonic uptime below expiry window"); + return; + }; + let flow = crate::cli::oauth_defaults::PendingOAuthFlow { + extension_name: "test_tool".to_string(), + display_name: "Test Tool".to_string(), + token_url: "https://example.com/token".to_string(), + client_id: "client123".to_string(), + client_secret: None, + redirect_uri: "https://example.com/oauth/callback".to_string(), + code_verifier: None, + access_token_field: "access_token".to_string(), + secret_name: "test_token".to_string(), + provider: None, + validation_endpoint: None, + scopes: vec![], + user_id: "test".to_string(), + secrets, + sse_sender: None, + gateway_token: None, + token_exchange_extra_params: std::collections::HashMap::new(), + client_id_secret_name: None, + created_at, + }; + + ext_mgr + .pending_oauth_flows() + .write() + .await + .insert("test_nonce".to_string(), flow); + + let state = test_gateway_state(Some(ext_mgr.clone())); + let app = test_oauth_router(state); + let versioned_state = + crate::cli::oauth_defaults::encode_hosted_oauth_state("test_nonce", Some("myinstance")); + + let req = axum::http::Request::builder() + .uri(format!( + "/oauth/callback?code=fake_code&state={}", + urlencoding::encode(&versioned_state) + )) + .body(Body::empty()) + .expect("request"); + + let resp = ServiceExt::>::oneshot(app, req) + .await + .expect("response"); + assert_eq!(resp.status(), StatusCode::OK); + + let body = axum::body::to_bytes(resp.into_body(), 1024 * 64) + .await + .expect("body"); + let html = String::from_utf8_lossy(&body); + assert!(html.contains("Authorization Failed")); + assert!( + ext_mgr + .pending_oauth_flows() + .read() + .await + .get("test_nonce") + .is_none() + ); + } + // --- Slack relay OAuth CSRF tests --- fn test_relay_oauth_router(state: Arc) -> Router { diff --git a/src/cli/oauth_defaults.rs b/src/cli/oauth_defaults.rs index a625f718..874cff98 100644 --- a/src/cli/oauth_defaults.rs +++ b/src/cli/oauth_defaults.rs @@ -5,17 +5,10 @@ //! //! # Built-in Credentials //! -//! Many CLI tools (gcloud, rclone, gdrive) ship with default OAuth credentials -//! so users don't need to register their own OAuth app. Google explicitly -//! documents that client_secret for "Desktop App" / "Installed App" types -//! is NOT actually secret. -//! -//! Default credentials are hardcoded below. They can be overridden at: -//! -//! - **Compile time**: Set IRONCLAW_GOOGLE_CLIENT_ID / IRONCLAW_GOOGLE_CLIENT_SECRET -//! env vars before building to replace the hardcoded defaults. -//! - **Runtime**: Users can set GOOGLE_OAUTH_CLIENT_ID / GOOGLE_OAUTH_CLIENT_SECRET -//! env vars, which take priority over built-in defaults. +//! Some providers ship with built-in OAuth credentials so users don't need to +//! register their own OAuth app just to get started. Today this module only +//! includes built-in defaults for Google-family tools, and those defaults can +//! be overridden by provider-specific environment variables when needed. use std::collections::HashMap; use std::sync::Arc; @@ -23,6 +16,7 @@ use std::time::Duration; use base64::{Engine, engine::general_purpose::URL_SAFE_NO_PAD}; use rand::RngCore; +use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use tokio::sync::RwLock; @@ -60,6 +54,14 @@ pub fn builtin_credentials(secret_name: &str) -> Option { } } +/// Returns the compile-time override env var name, if this provider supports one. +pub fn builtin_client_id_override_env(secret_name: &str) -> Option<&'static str> { + match secret_name { + "google_oauth_token" => Some("IRONCLAW_GOOGLE_CLIENT_ID"), + _ => None, + } +} + // ── Shared callback server ────────────────────────────────────────────── // Core OAuth callback infrastructure is defined in `crate::llm::oauth_helpers` @@ -173,9 +175,8 @@ pub async fn exchange_oauth_code( code_verifier: Option<&str>, access_token_field: &str, ) -> Result { - // Delegates to exchange_oauth_code_with_resource with resource=None. - // Non-MCP OAuth flows don't need the RFC 8707 resource parameter. - exchange_oauth_code_with_resource( + let extra_token_params = HashMap::new(); + exchange_oauth_code_with_params( token_url, client_id, client_secret, @@ -183,16 +184,14 @@ pub async fn exchange_oauth_code( redirect_uri, code_verifier, access_token_field, - None, + &extra_token_params, ) .await } -/// Exchange an OAuth authorization code for tokens, with optional RFC 8707 `resource` parameter. -/// -/// The `resource` parameter scopes the issued token to a specific server (used by MCP OAuth). +/// Exchange an OAuth authorization code for tokens with generic extra form parameters. #[allow(clippy::too_many_arguments)] -pub async fn exchange_oauth_code_with_resource( +pub async fn exchange_oauth_code_with_params( token_url: &str, client_id: &str, client_secret: Option<&str>, @@ -200,7 +199,7 @@ pub async fn exchange_oauth_code_with_resource( redirect_uri: &str, code_verifier: Option<&str>, access_token_field: &str, - resource: Option<&str>, + extra_token_params: &HashMap, ) -> Result { let client = reqwest::Client::new(); let mut token_params = vec![ @@ -213,10 +212,8 @@ pub async fn exchange_oauth_code_with_resource( token_params.push(("code_verifier", verifier.to_string())); } - // RFC 8707: include the `resource` parameter so the authorization server - // scopes the issued token to the specific MCP server (protected resource). - if let Some(resource) = resource { - token_params.push(("resource", resource.to_string())); + for (key, value) in extra_token_params { + token_params.push((key.as_str(), value.clone())); } let mut request = client.post(token_url); @@ -276,6 +273,37 @@ pub async fn exchange_oauth_code_with_resource( }) } +/// Exchange an OAuth authorization code for tokens, with optional RFC 8707 `resource` parameter. +/// +/// The `resource` parameter scopes the issued token to a specific server (used by MCP OAuth). +#[allow(clippy::too_many_arguments)] +pub async fn exchange_oauth_code_with_resource( + token_url: &str, + client_id: &str, + client_secret: Option<&str>, + code: &str, + redirect_uri: &str, + code_verifier: Option<&str>, + access_token_field: &str, + resource: Option<&str>, +) -> Result { + let mut extra_token_params = HashMap::new(); + if let Some(resource) = resource { + extra_token_params.insert("resource".to_string(), resource.to_string()); + } + exchange_oauth_code_with_params( + token_url, + client_id, + client_secret, + code, + redirect_uri, + code_verifier, + access_token_field, + &extra_token_params, + ) + .await +} + /// Store OAuth tokens (access + refresh) in the secrets store. /// /// Also stores the granted scopes as `{secret_name}_scopes` so that scope @@ -423,9 +451,9 @@ pub struct PendingOAuthFlow { pub sse_sender: Option>, /// Gateway auth token for authenticating with the platform token exchange proxy. pub gateway_token: Option, - /// RFC 8707 resource parameter (MCP OAuth only). - /// Sent during token exchange to scope the token to a specific MCP server. - pub resource: Option, + /// Additional form params for the token exchange request. + /// Used for provider-specific requirements such as RFC 8707 `resource`. + pub token_exchange_extra_params: HashMap, /// Secret name for persisting the client ID (MCP OAuth only). /// Needed so token refresh can find the client_id after the session ends. pub client_id_secret_name: Option, @@ -459,9 +487,7 @@ pub fn new_pending_oauth_registry() -> PendingOAuthRegistry { /// URL, meaning the user's browser will redirect to a hosted gateway rather than /// localhost. pub fn use_gateway_callback() -> bool { - std::env::var("IRONCLAW_OAUTH_CALLBACK_URL") - .ok() - .filter(|v| !v.is_empty()) + crate::config::helpers::env_or_override("IRONCLAW_OAUTH_CALLBACK_URL") .map(|raw| { url::Url::parse(&raw) .ok() @@ -472,6 +498,13 @@ pub fn use_gateway_callback() -> bool { .unwrap_or(false) } +/// Returns the configured OAuth token-exchange proxy URL, if any. +pub fn exchange_proxy_url() -> Option { + crate::config::helpers::env_or_override("IRONCLAW_OAUTH_EXCHANGE_URL") + .map(|url| url.trim().to_string()) + .filter(|url| !url.is_empty()) +} + /// Maximum age for pending OAuth flows (5 minutes, matching TCP listener timeout). pub const OAUTH_FLOW_EXPIRY: Duration = Duration::from_secs(300); @@ -486,23 +519,117 @@ pub async fn sweep_expired_flows(registry: &PendingOAuthRegistry) { // ── Platform routing helpers ──────────────────────────────────────── -/// Prepend instance name to CSRF state for platform routing. +const HOSTED_STATE_PREFIX: &str = "ic2"; +const HOSTED_STATE_CHECKSUM_BYTES: usize = 12; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DecodedHostedOAuthState { + pub flow_id: String, + pub instance_name: Option, + pub is_legacy: bool, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct HostedOAuthStatePayload { + flow_id: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + instance_name: Option, + issued_at: u64, +} + +fn current_instance_name() -> Option { + crate::config::helpers::env_or_override("IRONCLAW_INSTANCE_NAME") + .or_else(|| crate::config::helpers::env_or_override("OPENCLAW_INSTANCE_NAME")) + .filter(|v| !v.is_empty()) +} + +fn hosted_state_checksum(payload_bytes: &[u8]) -> String { + let digest = Sha256::digest(payload_bytes); + URL_SAFE_NO_PAD.encode(&digest[..HOSTED_STATE_CHECKSUM_BYTES]) +} + +/// Build a versioned hosted OAuth state envelope. /// -/// The NEAR AI platform nginx proxy at `auth.DOMAIN` parses the instance name -/// from the `state` query parameter (format: `instance:nonce`) to route the -/// OAuth callback to the correct container. -/// -/// Returns the nonce unchanged when `IRONCLAW_INSTANCE_NAME` is not set -/// (local/non-platform mode). -pub fn build_platform_state(nonce: &str) -> String { - let instance = std::env::var("IRONCLAW_INSTANCE_NAME") - .or_else(|_| std::env::var("OPENCLAW_INSTANCE_NAME")) - .ok() - .filter(|v| !v.is_empty()); - match instance { - Some(name) => format!("{}:{}", name, nonce), - None => nonce.to_string(), +/// The encoded value is opaque to providers and can be decoded by both +/// IronClaw and the external auth proxy for routing and callback lookup. +pub fn encode_hosted_oauth_state(flow_id: &str, instance_name: Option<&str>) -> String { + let payload = HostedOAuthStatePayload { + flow_id: flow_id.to_string(), + instance_name: instance_name + .map(str::trim) + .filter(|v| !v.is_empty()) + .map(str::to_string), + issued_at: std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_secs(), + }; + let payload_json = match serde_json::to_vec(&payload) { + Ok(payload_json) => payload_json, + Err(error) => { + tracing::warn!(%error, flow_id, "Failed to serialize hosted OAuth state payload"); + return payload.flow_id; + } + }; + let payload = URL_SAFE_NO_PAD.encode(&payload_json); + let checksum = hosted_state_checksum(&payload_json); + format!("{HOSTED_STATE_PREFIX}.{payload}.{checksum}") +} + +/// Decode hosted OAuth state in either the new versioned format or the +/// legacy `instance:nonce`/`nonce` forms. +pub fn decode_hosted_oauth_state(state: &str) -> Result { + if let Some(rest) = state.strip_prefix(&format!("{HOSTED_STATE_PREFIX}.")) + && let Some((payload_b64, checksum)) = rest.rsplit_once('.') + && let Ok(payload_json) = URL_SAFE_NO_PAD.decode(payload_b64) + { + let expected_checksum = hosted_state_checksum(&payload_json); + if checksum != expected_checksum { + return Err("Hosted OAuth state checksum mismatch".to_string()); + } + if let Ok(payload) = serde_json::from_slice::(&payload_json) + && !payload.flow_id.trim().is_empty() + { + return Ok(DecodedHostedOAuthState { + flow_id: payload.flow_id, + instance_name: payload.instance_name.filter(|v| !v.is_empty()), + is_legacy: false, + }); + } } + + if let Some((instance_name, flow_id)) = state.split_once(':') { + if flow_id.is_empty() { + return Err("Hosted OAuth legacy state is missing flow_id".to_string()); + } + return Ok(DecodedHostedOAuthState { + flow_id: flow_id.to_string(), + instance_name: if instance_name.is_empty() { + None + } else { + Some(instance_name.to_string()) + }, + is_legacy: true, + }); + } + + if state.is_empty() { + return Err("Hosted OAuth state is empty".to_string()); + } + + Ok(DecodedHostedOAuthState { + flow_id: state.to_string(), + instance_name: None, + is_legacy: true, + }) +} + +/// Build the hosted callback state used by the public OAuth callback endpoint. +/// +/// New flows emit a versioned opaque envelope, while callback decoding accepts +/// both the envelope and the legacy `instance:nonce` contract. +pub fn build_platform_state(nonce: &str) -> String { + encode_hosted_oauth_state(nonce, current_instance_name().as_deref()) } /// Strip the instance prefix from a state parameter to recover the lookup nonce. @@ -517,43 +644,62 @@ pub fn strip_instance_prefix(state: &str) -> &str { .unwrap_or(state) } +pub struct ProxyTokenExchangeRequest<'a> { + pub proxy_url: &'a str, + pub gateway_token: &'a str, + pub token_url: &'a str, + pub client_id: &'a str, + pub client_secret: Option<&'a str>, + pub code: &'a str, + pub redirect_uri: &'a str, + pub code_verifier: Option<&'a str>, + pub access_token_field: &'a str, + pub extra_token_params: &'a HashMap, +} + /// Exchange an OAuth authorization code via the platform's token exchange proxy. /// -/// The proxy holds `client_secret` server-side so the container never sees it. -/// Authenticated via the gateway auth token (Bearer header). +/// Authenticated via the gateway auth token (Bearer header). The caller may +/// either rely on proxy-side secret lookup or forward a `client_secret` when +/// the provider requires it. /// -/// The proxy expects form params `{code, redirect_uri, code_verifier}` and -/// returns a standard Google token response `{access_token, refresh_token, expires_in}`. +/// The proxy expects standard OAuth form params plus optional provider-specific +/// token params and returns a standard token response such as +/// `{access_token, refresh_token, expires_in}`. pub async fn exchange_via_proxy( - proxy_url: &str, - gateway_token: &str, - code: &str, - redirect_uri: &str, - code_verifier: Option<&str>, - access_token_field: &str, + request: ProxyTokenExchangeRequest<'_>, ) -> Result { - if gateway_token.is_empty() { + if request.gateway_token.is_empty() { return Err(OAuthCallbackError::Io( "Gateway auth token is required for proxy token exchange".to_string(), )); } - let exchange_url = format!("{}/oauth/exchange", proxy_url.trim_end_matches('/')); + let exchange_url = format!("{}/oauth/exchange", request.proxy_url.trim_end_matches('/')); let client = reqwest::Client::builder() .timeout(Duration::from_secs(60)) .build() .map_err(|e| OAuthCallbackError::Io(format!("Failed to build HTTP client: {}", e)))?; let mut params = vec![ - ("code", code.to_string()), - ("redirect_uri", redirect_uri.to_string()), + ("code", request.code.to_string()), + ("redirect_uri", request.redirect_uri.to_string()), + ("token_url", request.token_url.to_string()), + ("client_id", request.client_id.to_string()), + ("access_token_field", request.access_token_field.to_string()), ]; - if let Some(verifier) = code_verifier { + if let Some(verifier) = request.code_verifier { params.push(("code_verifier", verifier.to_string())); } + if let Some(secret) = request.client_secret { + params.push(("client_secret", secret.to_string())); + } + for (key, value) in request.extra_token_params { + params.push((key.as_str(), value.clone())); + } let response = client .post(&exchange_url) - .bearer_auth(gateway_token) + .bearer_auth(request.gateway_token) .form(¶ms) .send() .await @@ -576,7 +722,7 @@ pub async fn exchange_via_proxy( .map_err(|e| OAuthCallbackError::Io(format!("Failed to parse proxy response: {}", e)))?; let access_token = token_data - .get(access_token_field) + .get(request.access_token_field) .and_then(|v| v.as_str()) .ok_or_else(|| { let fields: Vec<&str> = token_data @@ -585,7 +731,7 @@ pub async fn exchange_via_proxy( .unwrap_or_default(); OAuthCallbackError::Io(format!( "No '{}' field in proxy response (fields present: {:?})", - access_token_field, fields + request.access_token_field, fields )) })? .to_string(); @@ -605,14 +751,10 @@ pub async fn exchange_via_proxy( #[cfg(test)] mod tests { - use std::sync::Mutex; - use crate::cli::oauth_defaults::{ builtin_credentials, callback_host, callback_url, is_loopback_host, landing_html, }; - - /// Serializes env-mutating tests to prevent parallel races. - static ENV_MUTEX: Mutex<()> = Mutex::new(()); + use crate::config::helpers::ENV_MUTEX; #[test] fn test_is_loopback_host() { @@ -935,7 +1077,7 @@ mod tests { #[test] fn test_build_platform_state_with_instance() { - use crate::cli::oauth_defaults::build_platform_state; + use crate::cli::oauth_defaults::{build_platform_state, decode_hosted_oauth_state}; let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_INSTANCE_NAME").ok(); @@ -943,7 +1085,11 @@ mod tests { unsafe { std::env::set_var("IRONCLAW_INSTANCE_NAME", "kind-deer"); } - assert_eq!(build_platform_state("abc123"), "kind-deer:abc123"); + let encoded = build_platform_state("abc123"); + let decoded = decode_hosted_oauth_state(&encoded).expect("decode hosted state"); + assert_eq!(decoded.flow_id, "abc123"); + assert_eq!(decoded.instance_name.as_deref(), Some("kind-deer")); + assert!(!decoded.is_legacy); unsafe { if let Some(val) = original { std::env::set_var("IRONCLAW_INSTANCE_NAME", val); @@ -955,7 +1101,7 @@ mod tests { #[test] fn test_build_platform_state_without_instance() { - use crate::cli::oauth_defaults::build_platform_state; + use crate::cli::oauth_defaults::{build_platform_state, decode_hosted_oauth_state}; let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_INSTANCE_NAME").ok(); @@ -965,7 +1111,11 @@ mod tests { std::env::remove_var("IRONCLAW_INSTANCE_NAME"); std::env::remove_var("OPENCLAW_INSTANCE_NAME"); } - assert_eq!(build_platform_state("abc123"), "abc123"); + let encoded = build_platform_state("abc123"); + let decoded = decode_hosted_oauth_state(&encoded).expect("decode hosted state"); + assert_eq!(decoded.flow_id, "abc123"); + assert_eq!(decoded.instance_name, None); + assert!(!decoded.is_legacy); unsafe { if let Some(val) = original { std::env::set_var("IRONCLAW_INSTANCE_NAME", val); @@ -978,7 +1128,7 @@ mod tests { #[test] fn test_build_platform_state_with_openclaw_instance() { - use crate::cli::oauth_defaults::build_platform_state; + use crate::cli::oauth_defaults::{build_platform_state, decode_hosted_oauth_state}; let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); let original_ic = std::env::var("IRONCLAW_INSTANCE_NAME").ok(); @@ -988,7 +1138,11 @@ mod tests { std::env::remove_var("IRONCLAW_INSTANCE_NAME"); std::env::set_var("OPENCLAW_INSTANCE_NAME", "quiet-lion"); } - assert_eq!(build_platform_state("xyz789"), "quiet-lion:xyz789"); + let encoded = build_platform_state("xyz789"); + let decoded = decode_hosted_oauth_state(&encoded).expect("decode hosted state"); + assert_eq!(decoded.flow_id, "xyz789"); + assert_eq!(decoded.instance_name.as_deref(), Some("quiet-lion")); + assert!(!decoded.is_legacy); unsafe { if let Some(val) = original_ic { std::env::set_var("IRONCLAW_INSTANCE_NAME", val); @@ -1017,6 +1171,42 @@ mod tests { assert_eq!(strip_instance_prefix(""), ""); } + #[test] + fn test_decode_hosted_oauth_state_accepts_legacy_formats() { + use crate::cli::oauth_defaults::decode_hosted_oauth_state; + + let decoded = decode_hosted_oauth_state("kind-deer:abc123").expect("legacy prefixed"); + assert_eq!(decoded.flow_id, "abc123"); + assert_eq!(decoded.instance_name.as_deref(), Some("kind-deer")); + assert!(decoded.is_legacy); + + let decoded = decode_hosted_oauth_state("abc123").expect("legacy raw"); + assert_eq!(decoded.flow_id, "abc123"); + assert_eq!(decoded.instance_name, None); + assert!(decoded.is_legacy); + } + + #[test] + fn test_decode_hosted_oauth_state_falls_back_for_non_envelope_ic2_prefix() { + use crate::cli::oauth_defaults::decode_hosted_oauth_state; + + let decoded = + decode_hosted_oauth_state("ic2.provider-owned-state").expect("prefixed fallback"); + assert_eq!(decoded.flow_id, "ic2.provider-owned-state"); + assert_eq!(decoded.instance_name, None); + assert!(decoded.is_legacy); + } + + #[test] + fn test_decode_hosted_oauth_state_rejects_tampered_checksum() { + use crate::cli::oauth_defaults::{decode_hosted_oauth_state, encode_hosted_oauth_state}; + + let encoded = encode_hosted_oauth_state("abc123", Some("kind-deer")); + let tampered = format!("{encoded}broken"); + let err = decode_hosted_oauth_state(&tampered).expect_err("tampered state should fail"); + assert!(err.contains("checksum"), "unexpected error: {err}"); + } + /// Verify that `build_oauth_url` includes the RFC 8707 `resource` parameter /// when passed through `extra_params`, which is how MCP OAuth gateway mode /// scopes tokens to a specific MCP server. diff --git a/src/cli/tool.rs b/src/cli/tool.rs index ac5d1b37..be684580 100644 --- a/src/cli/tool.rs +++ b/src/cli/tool.rs @@ -651,8 +651,8 @@ async fn auth_tool(name: String, dir: Option, user_id: String) -> anyho // Check for OAuth configuration if let Some(ref oauth) = auth.oauth { - // For providers with shared tokens (e.g., all Google tools share google_oauth_token), - // combine scopes from all installed tools so one auth covers everything. + // For providers with shared tokens, combine scopes from all installed + // tools so one auth covers everything. let combined = combine_provider_scopes(&tools_dir, &auth.secret_name, oauth).await; if combined.scopes.len() > oauth.scopes.len() { let extra = combined.scopes.len() - oauth.scopes.len(); @@ -670,8 +670,8 @@ async fn auth_tool(name: String, dir: Option, user_id: String) -> anyho } /// Scan the tools directory for all capabilities files sharing the same secret_name -/// and combine their OAuth scopes. This way, authing any Google tool requests scopes -/// for ALL installed Google tools, so one login covers everything. +/// and combine their OAuth scopes so one authorization covers the full shared +/// credential set. async fn combine_provider_scopes( tools_dir: &Path, secret_name: &str, @@ -736,11 +736,18 @@ async fn auth_tool_oauth( }) .or_else(|| builtin.as_ref().map(|c| c.client_id.to_string())) .ok_or_else(|| { - anyhow::anyhow!( + let mut message = format!( "OAuth client_id not configured.\n\ - Set {} env var, or build with IRONCLAW_GOOGLE_CLIENT_ID.", + Set {} env var", oauth.client_id_env.as_deref().unwrap_or("the client_id") - ) + ); + if let Some(override_env) = + oauth_defaults::builtin_client_id_override_env(&auth.secret_name) + { + message.push_str(&format!(", or build with {override_env}")); + } + message.push('.'); + anyhow::anyhow!(message) })?; // Get client_secret: capabilities file > runtime env var > built-in defaults diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index fbc06d5d..0762f3ed 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -45,6 +45,56 @@ struct PendingAuth { task_handle: Option>, } +struct HostedOAuthFlowStart { + name: String, + kind: ExtensionKind, + auth_url: String, + expected_state: String, + flow: crate::cli::oauth_defaults::PendingOAuthFlow, +} + +fn hosted_proxy_client_secret( + client_secret: &Option, + builtin: Option<&crate::cli::oauth_defaults::OAuthCredentials>, + exchange_proxy_configured: bool, +) -> Option { + if !exchange_proxy_configured { + return client_secret.clone(); + } + + let builtin_secret = builtin.map(|credentials| credentials.client_secret); + match (client_secret, builtin_secret) { + (Some(resolved), Some(baked_in)) if resolved == baked_in => None, + _ => client_secret.clone(), + } +} + +fn normalize_oauth_callback_path(path: &str) -> String { + let trimmed_path = path.trim_end_matches('/'); + if trimmed_path.is_empty() { + "/oauth/callback".to_string() + } else if trimmed_path.ends_with("/oauth/callback") { + trimmed_path.to_string() + } else { + format!("{trimmed_path}/oauth/callback") + } +} + +fn normalize_hosted_callback_url(callback_url: &str) -> String { + if let Ok(mut parsed) = url::Url::parse(callback_url) { + let normalized_path = normalize_oauth_callback_path(parsed.path()); + parsed.set_path(&normalized_path); + return parsed.to_string(); + } + + let normalized_callback_url = callback_url.trim_end_matches('/'); + if normalized_callback_url.ends_with("/oauth/callback") { + normalized_callback_url.to_string() + } else { + format!("{normalized_callback_url}/oauth/callback") + } +} + /// Runtime infrastructure needed for hot-activating WASM channels. /// /// Set after construction via [`ExtensionManager::set_channel_runtime`] once the @@ -547,7 +597,9 @@ impl ExtensionManager { async fn gateway_callback_redirect_uri(&self) -> Option { use crate::cli::oauth_defaults; if oauth_defaults::use_gateway_callback() { - return Some(format!("{}/oauth/callback", oauth_defaults::callback_url())); + return Some(normalize_hosted_callback_url( + &oauth_defaults::callback_url(), + )); } // Use gateway_base_url from enable_gateway_mode() if let Some(ref base) = *self.gateway_base_url.read().await { @@ -924,6 +976,98 @@ impl ExtensionManager { &self.pending_oauth_flows } + async fn clear_pending_extension_auth(&self, name: &str) { + { + let mut pending = self.pending_auth.write().await; + if let Some(old) = pending.remove(name) + && let Some(handle) = old.task_handle + { + handle.abort(); + } + } + + let mut flows = self.pending_oauth_flows.write().await; + flows.retain(|_, flow| flow.extension_name != name); + } + + fn rewrite_oauth_state_param( + auth_url: String, + expected_state: &str, + hosted_state: &str, + ) -> String { + if hosted_state == expected_state { + return auth_url; + } + + let Ok(mut parsed) = url::Url::parse(&auth_url) else { + return auth_url.replace( + &format!("state={}", urlencoding::encode(expected_state)), + &format!("state={}", urlencoding::encode(hosted_state)), + ); + }; + + let mut replaced = false; + let pairs: Vec<(String, String)> = parsed + .query_pairs() + .map(|(key, value)| { + if key == "state" { + replaced = true; + (key.into_owned(), hosted_state.to_string()) + } else { + (key.into_owned(), value.into_owned()) + } + }) + .collect(); + + { + let mut query_pairs = parsed.query_pairs_mut(); + query_pairs.clear(); + for (key, value) in pairs { + query_pairs.append_pair(&key, &value); + } + if !replaced { + query_pairs.append_pair("state", hosted_state); + } + } + + parsed.to_string() + } + + async fn start_gateway_oauth_flow(&self, request: HostedOAuthFlowStart) -> AuthResult { + use crate::cli::oauth_defaults; + + oauth_defaults::sweep_expired_flows(&self.pending_oauth_flows).await; + + let hosted_state = oauth_defaults::build_platform_state(&request.expected_state); + let auth_url = Self::rewrite_oauth_state_param( + request.auth_url, + &request.expected_state, + &hosted_state, + ); + + self.pending_oauth_flows + .write() + .await + .insert(request.expected_state, request.flow); + + self.pending_auth.write().await.insert( + request.name.clone(), + PendingAuth { + _name: request.name.clone(), + _kind: request.kind, + created_at: std::time::Instant::now(), + task_handle: None, + }, + ); + + AuthResult::awaiting_authorization( + request.name, + request.kind, + auth_url, + "gateway".to_string(), + ) + } + /// Broadcast an extension status change to the web UI via SSE. async fn broadcast_extension_status(&self, name: &str, status: &str, message: Option<&str>) { if let Some(ref sender) = *self.sse_sender.read().await { @@ -2383,6 +2527,7 @@ impl ExtensionManager { use crate::cli::oauth_defaults; let is_gateway = self.should_use_gateway_mode(); + self.clear_pending_extension_auth(name).await; // Build redirect URI: gateway uses the public callback URL, // local mode binds a random port. @@ -2440,19 +2585,8 @@ impl ExtensionManager { let code_verifier = oauth_result.code_verifier; if is_gateway { - // Gateway mode: store pending flow for the /oauth/callback handler. - oauth_defaults::sweep_expired_flows(&self.pending_oauth_flows).await; - - // Platform routing: prepend instance name to state - let platform_state = oauth_defaults::build_platform_state(&expected_state); - let auth_url = if platform_state != expected_state { - oauth_result.url.replace( - &format!("state={}", urlencoding::encode(&expected_state)), - &format!("state={}", urlencoding::encode(&platform_state)), - ) - } else { - oauth_result.url - }; + let mut token_exchange_extra_params = HashMap::new(); + token_exchange_extra_params.insert("resource".to_string(), resource.clone()); let flow = oauth_defaults::PendingOAuthFlow { extension_name: name.to_string(), @@ -2471,7 +2605,7 @@ impl ExtensionManager { secrets: Arc::clone(&self.secrets), sse_sender: self.sse_sender.read().await.clone(), gateway_token: self.gateway_token.clone(), - resource: Some(resource), + token_exchange_extra_params, client_id_secret_name: if server.oauth.is_none() { Some(server.client_id_secret_name()) } else { @@ -2480,27 +2614,15 @@ impl ExtensionManager { created_at: std::time::Instant::now(), }; - self.pending_oauth_flows - .write() - .await - .insert(expected_state, flow); - - self.pending_auth.write().await.insert( - name.to_string(), - PendingAuth { - _name: name.to_string(), - _kind: ExtensionKind::McpServer, - created_at: std::time::Instant::now(), - task_handle: None, - }, - ); - - Ok(AuthResult::awaiting_authorization( - name, - ExtensionKind::McpServer, - auth_url, - "gateway".to_string(), - )) + Ok(self + .start_gateway_oauth_flow(HostedOAuthFlowStart { + name: name.to_string(), + kind: ExtensionKind::McpServer, + auth_url: oauth_result.url, + expected_state, + flow, + }) + .await) } else { // Local mode: return URL for manual opening self.pending_auth.write().await.insert( @@ -2901,9 +3023,10 @@ impl ExtensionManager { Enter it in the Setup tab or set {} env var", name, env_name ); - // Only mention the Google-specific build flag for Google providers - if auth.secret_name.to_lowercase().contains("google") { - msg.push_str(", or build with IRONCLAW_GOOGLE_CLIENT_ID"); + if let Some(override_env) = + crate::cli::oauth_defaults::builtin_client_id_override_env(&auth.secret_name) + { + msg.push_str(&format!(", or build with {override_env}")); } msg.push('.'); msg @@ -2919,20 +3042,7 @@ impl ExtensionManager { ) .await; - // Cancel any existing pending auth for this tool (frees port 9876 in TCP mode) - { - let mut pending = self.pending_auth.write().await; - if let Some(old) = pending.remove(name) - && let Some(handle) = old.task_handle - { - handle.abort(); - } - } - // Also clean up any gateway-mode pending flows for this tool - { - let mut flows = self.pending_oauth_flows.write().await; - flows.retain(|_, flow| flow.extension_name != name); - } + self.clear_pending_extension_auth(name).await; let redirect_uri = self .gateway_callback_redirect_uri() @@ -2963,30 +3073,24 @@ impl ExtensionManager { .unwrap_or_else(|| name.to_string()); if self.should_use_gateway_mode() { - // Gateway mode: store pending flow state for the web gateway's - // `/oauth/callback` handler to complete the exchange. No TCP listener - // needed — the OAuth provider redirects to the gateway URL. - oauth_defaults::sweep_expired_flows(&self.pending_oauth_flows).await; - - // Wrap the CSRF nonce with instance name for platform routing. - // Nginx at auth.DOMAIN parses `instance:nonce` to route the callback - // to the correct container. The flow is keyed by the raw nonce. - let platform_state = oauth_defaults::build_platform_state(&expected_state); - let auth_url = if platform_state != expected_state { - auth_url.replace( - &format!("state={}", urlencoding::encode(&expected_state)), - &format!("state={}", urlencoding::encode(&platform_state)), - ) - } else { - auth_url - }; + // When an exchange proxy is configured, omit the client_secret if it + // was resolved from built-in defaults (desktop app credentials). The + // proxy holds the correct web-app secret for platform-registered OAuth + // apps. Sending the desktop secret would cause a client_id/secret + // mismatch because the container's GOOGLE_OAUTH_CLIENT_ID is the web + // app, not the desktop app. + let proxy_client_secret = hosted_proxy_client_secret( + &client_secret, + builtin.as_ref(), + oauth_defaults::exchange_proxy_url().is_some(), + ); let flow = oauth_defaults::PendingOAuthFlow { extension_name: name.to_string(), display_name: display_name.clone(), token_url: oauth.token_url.clone(), client_id: client_id.clone(), - client_secret: client_secret.clone(), + client_secret: proxy_client_secret, redirect_uri: redirect_uri.clone(), code_verifier, access_token_field: oauth.access_token_field.clone(), @@ -2998,35 +3102,20 @@ impl ExtensionManager { secrets: Arc::clone(&self.secrets), sse_sender: self.sse_sender.read().await.clone(), gateway_token: self.gateway_token.clone(), - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at: std::time::Instant::now(), }; - // Key by raw nonce (without instance prefix) — the callback handler - // strips the prefix before lookup. - self.pending_oauth_flows - .write() - .await - .insert(expected_state, flow); - - // Register pending auth without a task handle (gateway handles completion) - self.pending_auth.write().await.insert( - name.to_string(), - PendingAuth { - _name: name.to_string(), - _kind: ExtensionKind::WasmTool, - created_at: std::time::Instant::now(), - task_handle: None, - }, - ); - - Ok(AuthResult::awaiting_authorization( - name, - ExtensionKind::WasmTool, - auth_url, - "gateway".to_string(), - )) + Ok(self + .start_gateway_oauth_flow(HostedOAuthFlowStart { + name: name.to_string(), + kind: ExtensionKind::WasmTool, + auth_url, + expected_state, + flow, + }) + .await) } else { // TCP listener mode: bind port 9876 and spawn a background task // to wait for the callback. This is the original flow for local/desktop use. @@ -5241,7 +5330,8 @@ mod tests { use crate::extensions::manager::{ ChannelRuntimeState, FallbackDecision, TelegramBindingData, TelegramBindingResult, TelegramOwnerBindingState, build_wasm_channel_runtime_config_updates, - combine_install_errors, fallback_decision, infer_kind_from_url, send_telegram_text_message, + combine_install_errors, fallback_decision, hosted_proxy_client_secret, infer_kind_from_url, + normalize_hosted_callback_url, send_telegram_text_message, telegram_message_matches_verification_code, }; use crate::extensions::{ @@ -6510,7 +6600,7 @@ mod tests { secrets: Arc::clone(&secrets), sse_sender: None, gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at: std::time::Instant::now(), }, @@ -6534,7 +6624,7 @@ mod tests { secrets, sse_sender: None, gateway_token: None, - resource: None, + token_exchange_extra_params: std::collections::HashMap::new(), client_id_secret_name: None, created_at: std::time::Instant::now(), }, @@ -6701,9 +6791,6 @@ mod tests { // The root cause was that `should_use_gateway_mode()` only checked the // `IRONCLAW_OAUTH_CALLBACK_URL` env var, ignoring `self.tunnel_url`. - /// Serializes env-mutating tests to prevent parallel races. - static GATEWAY_ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); - /// Build a minimal ExtensionManager with a custom tunnel_url. fn make_manager_with_tunnel(tunnel_url: Option) -> ExtensionManager { use crate::secrets::{InMemorySecretsStore, SecretsCrypto}; @@ -6736,9 +6823,11 @@ mod tests { #[test] fn should_use_gateway_mode_true_for_tunnel_url() { - let _guard = GATEWAY_ENV_MUTEX.lock().expect("env mutex poisoned"); + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); - // SAFETY: Under GATEWAY_ENV_MUTEX, no concurrent env access. + // SAFETY: Under ENV_MUTEX, no concurrent env access. unsafe { std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); } @@ -6758,7 +6847,9 @@ mod tests { #[test] fn should_use_gateway_mode_false_without_tunnel() { - let _guard = GATEWAY_ENV_MUTEX.lock().expect("env mutex poisoned"); + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); unsafe { std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); @@ -6779,7 +6870,9 @@ mod tests { #[test] fn should_use_gateway_mode_false_for_loopback_tunnel() { - let _guard = GATEWAY_ENV_MUTEX.lock().expect("env mutex poisoned"); + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); unsafe { std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); @@ -6807,9 +6900,11 @@ mod tests { impl EnvGuard { fn new() -> Self { - let guard = GATEWAY_ENV_MUTEX.lock().expect("env mutex poisoned"); + let guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); - // SAFETY: Under GATEWAY_ENV_MUTEX, no concurrent env access. + // SAFETY: Under ENV_MUTEX, no concurrent env access. unsafe { std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); } @@ -6822,7 +6917,7 @@ mod tests { impl Drop for EnvGuard { fn drop(&mut self) { - // SAFETY: Under GATEWAY_ENV_MUTEX (still held by _mutex), no concurrent env access. + // SAFETY: Under ENV_MUTEX (still held by _mutex), no concurrent env access. unsafe { if let Some(ref val) = self.original { std::env::set_var("IRONCLAW_OAUTH_CALLBACK_URL", val); @@ -6863,6 +6958,90 @@ mod tests { ); } + #[test] + fn gateway_callback_redirect_uri_does_not_duplicate_callback_path_from_env() { + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); + let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); + unsafe { + std::env::set_var( + "IRONCLAW_OAUTH_CALLBACK_URL", + "https://oauth.test.example/oauth/callback", + ); + } + + let mgr = make_manager_with_tunnel(None); + assert_eq!( + tokio_test::block_on(mgr.gateway_callback_redirect_uri()), + Some("https://oauth.test.example/oauth/callback".to_string()), + ); + + unsafe { + if let Some(val) = original { + std::env::set_var("IRONCLAW_OAUTH_CALLBACK_URL", val); + } else { + std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); + } + } + } + + #[test] + fn gateway_callback_redirect_uri_trims_trailing_slash_from_env_callback() { + let _guard = crate::config::helpers::ENV_MUTEX + .lock() + .expect("env mutex poisoned"); + let original = std::env::var("IRONCLAW_OAUTH_CALLBACK_URL").ok(); + unsafe { + std::env::set_var( + "IRONCLAW_OAUTH_CALLBACK_URL", + "https://oauth.test.example/oauth/callback/", + ); + } + + let mgr = make_manager_with_tunnel(None); + assert_eq!( + tokio_test::block_on(mgr.gateway_callback_redirect_uri()), + Some("https://oauth.test.example/oauth/callback".to_string()), + ); + + unsafe { + if let Some(val) = original { + std::env::set_var("IRONCLAW_OAUTH_CALLBACK_URL", val); + } else { + std::env::remove_var("IRONCLAW_OAUTH_CALLBACK_URL"); + } + } + } + + #[test] + fn normalize_hosted_callback_url_preserves_query_params() { + assert_eq!( + normalize_hosted_callback_url("https://oauth.test.example?source=hosted"), + "https://oauth.test.example/oauth/callback?source=hosted" + ); + assert_eq!( + normalize_hosted_callback_url( + "https://oauth.test.example/oauth/callback?source=hosted" + ), + "https://oauth.test.example/oauth/callback?source=hosted" + ); + } + + #[test] + fn rewrite_oauth_state_param_updates_only_state_query_param() { + let auth_url = + "https://auth.example.com/authorize?client_id=abc&state=old-state&hint=state%3Dkeep"; + assert_eq!( + ExtensionManager::rewrite_oauth_state_param( + auth_url.to_string(), + "old-state", + "new-hosted-state", + ), + "https://auth.example.com/authorize?client_id=abc&state=new-hosted-state&hint=state%3Dkeep" + ); + } + #[tokio::test] async fn gateway_mode_enabled_explicitly() { let _env = EnvGuard::new(); @@ -7217,4 +7396,71 @@ mod tests { panic!("URL missing token: {url}"); // safety: test assertion } } + + // ── proxy_client_secret suppression ───────────────────────────── + + #[test] + fn test_proxy_client_secret_suppressed_when_builtin_matches_with_exchange_proxy() { + let builtin = crate::cli::oauth_defaults::builtin_credentials("google_oauth_token"); + let builtin_ref = builtin.as_ref(); + let secret = Some(builtin_ref.unwrap().client_secret.to_string()); + + let result = hosted_proxy_client_secret(&secret, builtin_ref, true); + assert_eq!( + result, None, + "built-in desktop secret must be suppressed when the exchange proxy is configured" + ); + } + + #[test] + fn test_proxy_client_secret_kept_when_not_builtin_with_exchange_proxy() { + let builtin = crate::cli::oauth_defaults::builtin_credentials("google_oauth_token"); + let secret = Some("user-entered-custom-secret".to_string()); + + let result = hosted_proxy_client_secret(&secret, builtin.as_ref(), true); + assert_eq!( + result, + Some("user-entered-custom-secret".to_string()), + "non-builtin secret must be kept even when the exchange proxy is configured" + ); + } + + #[test] + fn test_proxy_client_secret_kept_without_exchange_proxy_even_for_builtin_secret() { + let builtin = crate::cli::oauth_defaults::builtin_credentials("google_oauth_token"); + let builtin_ref = builtin.as_ref(); + let secret = Some(builtin_ref.unwrap().client_secret.to_string()); + + let result = hosted_proxy_client_secret(&secret, builtin_ref, false); + assert_eq!( + result, secret, + "built-in secret must be kept when the callback will exchange directly" + ); + } + + #[test] + fn test_proxy_client_secret_none_stays_none() { + let builtin = crate::cli::oauth_defaults::builtin_credentials("google_oauth_token"); + + let result = hosted_proxy_client_secret(&None, builtin.as_ref(), true); + assert_eq!( + result, None, + "None secret stays None even when the exchange proxy is configured" + ); + } + + #[test] + fn test_proxy_client_secret_no_builtin_provider() { + // MCP/non-Google providers have no builtin credentials + let builtin = crate::cli::oauth_defaults::builtin_credentials("mcp_notion_access_token"); + assert!(builtin.is_none()); + + let secret = Some("dcr-secret".to_string()); + let result = hosted_proxy_client_secret(&secret, builtin.as_ref(), true); + assert_eq!( + result, + Some("dcr-secret".to_string()), + "non-builtin provider secret must be kept" + ); + } } diff --git a/src/llm/oauth_helpers.rs b/src/llm/oauth_helpers.rs index 551fc04b..b63457fd 100644 --- a/src/llm/oauth_helpers.rs +++ b/src/llm/oauth_helpers.rs @@ -39,9 +39,7 @@ pub enum OAuthCallbackError { /// deployments where `127.0.0.1` is unreachable from the user's browser), /// then falls back to `http://{callback_host()}:{OAUTH_CALLBACK_PORT}`. pub fn callback_url() -> String { - std::env::var("IRONCLAW_OAUTH_CALLBACK_URL") - .ok() - .filter(|v| !v.is_empty()) + crate::config::helpers::env_or_override("IRONCLAW_OAUTH_CALLBACK_URL") .unwrap_or_else(|| format!("http://{}:{}", callback_host(), OAUTH_CALLBACK_PORT)) } @@ -57,7 +55,8 @@ pub fn callback_url() -> String { /// Note: this transmits the session token over plain HTTP — prefer SSH port /// forwarding (`ssh -L 9876:127.0.0.1:9876 user@host`) when possible. pub fn callback_host() -> String { - std::env::var("OAUTH_CALLBACK_HOST").unwrap_or_else(|_| "127.0.0.1".to_string()) + crate::config::helpers::env_or_override("OAUTH_CALLBACK_HOST") + .unwrap_or_else(|| "127.0.0.1".to_string()) } /// Returns `true` if `host` is a loopback address that only accepts local connections. diff --git a/tests/e2e/mock_llm.py b/tests/e2e/mock_llm.py index c27f2762..359c22d5 100644 --- a/tests/e2e/mock_llm.py +++ b/tests/e2e/mock_llm.py @@ -267,14 +267,24 @@ async def _stream_tool_call(request: web.Request, cid: str, tc: dict) -> web.Str async def oauth_exchange(request: web.Request) -> web.Response: """Mock OAuth token exchange proxy for E2E tests. - Accepts form params (code, redirect_uri, code_verifier) and returns - a fake token response. Called by ironclaw's exchange_via_proxy() when - IRONCLAW_OAUTH_EXCHANGE_URL is set. + Accepts the generic hosted OAuth proxy contract used by IronClaw and + returns a fake token response. MCP callback tests assert that provider- + specific token params such as RFC 8707 `resource` are forwarded here. """ data = await request.post() code = data.get("code", "") + access_token_field = data.get("access_token_field", "access_token") + + if code == "mock_mcp_code": + if not data.get("token_url", "").endswith("/oauth/token"): + return web.json_response({"error": "missing_token_url"}, status=400) + if not data.get("client_id"): + return web.json_response({"error": "missing_client_id"}, status=400) + if not data.get("resource"): + return web.json_response({"error": "missing_resource"}, status=400) + return web.json_response({ - "access_token": f"mock-token-{code}", + access_token_field: f"mock-token-{code}", "refresh_token": "mock-refresh-token", "expires_in": 3600, }) diff --git a/tests/e2e/scenarios/test_mcp_auth_flow.py b/tests/e2e/scenarios/test_mcp_auth_flow.py index 7de2bbe6..cc36aa2e 100644 --- a/tests/e2e/scenarios/test_mcp_auth_flow.py +++ b/tests/e2e/scenarios/test_mcp_auth_flow.py @@ -99,6 +99,10 @@ async def test_mcp_activate_triggers_auth(ironclaw_server): assert auth_url is not None or awaiting_token, ( f"Activate should require auth, got: {data}" ) + if auth_url is not None: + assert _extract_state(auth_url).startswith("ic2."), ( + f"Hosted MCP OAuth should emit versioned state, got: {auth_url}" + ) # ── Section C: OAuth Round-Trip ──────────────────────────────────────────