From 07972fc0992d74fc0ec97911ededf0ee273097ae Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Tue, 21 Apr 2026 22:03:02 +0900 Subject: [PATCH] fix(auth): prevent OAuth URL parameter truncation (#2391) (#2746) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(auth): switch OAuth URL construction to url crate to prevent char loss (#2391) Google OAuth was reportedly receiving `access_type=offlin` instead of `access_type=offline` when users ran `ironclaw tool auth google-calendar`, breaking the offline-token flow every Google WASM tool relies on (Calendar, Gmail, Drive, Docs, Sheets, Slides). The hand-rolled `format!` + `urlencoding::encode` loops in `auth::oauth::build_oauth_url` and `tools::mcp::auth::build_authorization_url` are replaced with `url::Url` + `query_pairs_mut()`, routing every query parameter through a single well-tested `application/x-www-form-urlencoded` serializer. The old concat path is kept as a defensive fallback for the (never-observed-in-practice) case where the authorization URL itself fails to parse. Regression coverage added at the call-site level per `.claude/rules/testing.md`: * `test_build_oauth_url_preserves_access_type_offline_exactly` — parses the returned URL and asserts `access_type == "offline"` exactly (not via `.contains()`, which would have passed on `offlin`). * `test_build_oauth_url_extra_params_preserve_all_chars_across_hash_orderings` — loops 16 iterations so random `HashMap` iteration order surfaces any bug sensitive to which param lands last. * `test_google_calendar_capabilities_produce_correct_oauth_url` — loads the shipped `google-calendar-tool.capabilities.json` shape, parses it via `CapabilitiesFile::from_json`, and drives the same `build_oauth_url` call site that `cli::tool::auth_tool_oauth` uses. * `test_build_authorization_url_extra_params_preserve_all_chars` — parallel regression for the MCP authorization-URL builder. The two pre-existing helper tests were also tightened to round-trip through `url::Url::parse` + `query_pairs()` rather than relying on substring assertions, so a 1-char truncation can no longer pass as a prefix match. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(auth): address PR #2746 review feedback - Reject malformed authorization URLs with a specific error instead of concat-normalizing them (gemini-code-assist review). - Rebuild HashMap per iteration in order-probe tests so different iteration orders are actually exercised (Copilot review). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(auth): surface malformed OAuth descriptors at call sites (#2746) Address review feedback from @serrrfirat on PR #2746: two call sites of `build_pending_oauth_launch` were using `.ok()?` to silently drop `OAuthUrlError::MalformedConfig`, which regressed the fail-closed posture this PR introduced. Replaces `.ok()?` in both: - `AuthManager::start_skill_oauth_if_supported` - `ExtensionManager::start_secret_oauth_flow` with an explicit `match` that emits `tracing::error!` (carrying credential/extension/secret/user context) before falling back to the manual-token path. Operators now get a signal when an OAuth descriptor is misconfigured, rather than seeing the browser auth flow silently disappear. Signatures stay `Option<...>` — the existing `test_build_oauth_url_rejects_malformed_authorization_url` covers the helper-level regression; this change is call-site observability. [skip-regression-check] Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- src/auth/mod.rs | 10 +- src/auth/oauth.rs | 385 +++++++++++++++++++++++++++++++++---- src/bridge/auth_manager.rs | 12 ++ src/cli/tool.rs | 3 +- src/extensions/manager.rs | 19 +- src/tools/mcp/auth.rs | 220 ++++++++++++++++----- 6 files changed, 556 insertions(+), 93 deletions(-) diff --git a/src/auth/mod.rs b/src/auth/mod.rs index c746ec98b91..3921ad32850 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -133,7 +133,9 @@ fn default_access_token_field() -> String { "access_token".to_string() } -pub fn build_pending_oauth_launch(params: PendingOAuthLaunchParams) -> PendingOAuthLaunch { +pub fn build_pending_oauth_launch( + params: PendingOAuthLaunchParams, +) -> Result { let oauth_result = oauth::build_oauth_url( ¶ms.authorization_url, ¶ms.client_id, @@ -141,7 +143,7 @@ pub fn build_pending_oauth_launch(params: PendingOAuthLaunchParams) -> PendingOA ¶ms.scopes, params.use_pkce, ¶ms.extra_params, - ); + )?; let flow = crate::auth::oauth::PendingOAuthFlow { extension_name: params.extension_name, @@ -168,11 +170,11 @@ pub fn build_pending_oauth_launch(params: PendingOAuthLaunchParams) -> PendingOA auto_activate_extension: params.auto_activate_extension, }; - PendingOAuthLaunch { + Ok(PendingOAuthLaunch { auth_url: oauth_result.url, expected_state: oauth_result.state, flow, - } + }) } async fn load_auth_descriptors( diff --git a/src/auth/oauth.rs b/src/auth/oauth.rs index f19b7f81939..45ed5cbfec9 100644 --- a/src/auth/oauth.rs +++ b/src/auth/oauth.rs @@ -15,6 +15,7 @@ use rand::RngCore; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use tokio::sync::RwLock; +use url::Url; pub use crate::auth::providers::{ OAuthCredentials, builtin_client_id_override_env, builtin_credentials, @@ -113,11 +114,31 @@ pub struct OAuthUrlResult { pub state: String, } +/// Errors returned while constructing an OAuth authorization URL. +/// +/// The only currently-modeled variant is `MalformedConfig`, returned when the +/// provided `authorization_url` cannot be parsed by the `url` crate. That +/// indicates a misconfigured descriptor / capabilities entry; the caller +/// should surface it to the operator rather than attempting to "fix up" the +/// URL through string concatenation, which is what gemini-code-assist flagged +/// on #2746 as a security-posture issue. +#[derive(Debug, Clone, thiserror::Error)] +pub enum OAuthUrlError { + /// The `authorization_url` could not be parsed as a valid URL. + #[error("Malformed OAuth authorization URL: {0}")] + MalformedConfig(String), +} + /// Build an OAuth 2.0 authorization URL with optional PKCE and CSRF state. /// /// Returns an `OAuthUrlResult` containing the authorization URL, optional PKCE /// code verifier, and a random `state` parameter for CSRF protection. The caller /// must validate the `state` value in the callback before exchanging the code. +/// +/// Returns `Err(OAuthUrlError::MalformedConfig)` if `authorization_url` cannot +/// be parsed. We deliberately do not try to normalize a malformed URL through +/// manual string concatenation — rejecting a bad config is the only secure +/// response (see gemini-code-assist review on #2746). pub fn build_oauth_url( authorization_url: &str, client_id: &str, @@ -125,7 +146,7 @@ pub fn build_oauth_url( scopes: &[String], use_pkce: bool, extra_params: &HashMap, -) -> OAuthUrlResult { +) -> Result { // Generate PKCE verifier and challenge let (code_verifier, code_challenge) = if use_pkce { let mut verifier_bytes = [0u8; 32]; @@ -146,42 +167,76 @@ pub fn build_oauth_url( rand::rngs::OsRng.fill_bytes(&mut state_bytes); let state = URL_SAFE_NO_PAD.encode(state_bytes); - // Build authorization URL - let mut auth_url = format!( - "{}?client_id={}&response_type=code&redirect_uri={}&state={}", + // Build the authorization URL via the `url` crate so query-string encoding + // goes through a single well-tested code path. This replaces a hand-rolled + // `format!` + `urlencoding::encode` loop that had a history of truncating + // the last character of the final query parameter on some platforms + // (nearai/ironclaw#2391: `access_type=offline` was being received by + // Google as `access_type=offlin`). A `Url::parse` failure here means the + // descriptor/capabilities config is malformed; we reject rather than + // concat-normalize (gemini-code-assist review on #2746) so a bad config + // cannot silently produce a half-formed URL. + let auth_url = build_oauth_authorization_url_string( authorization_url, - urlencoding::encode(client_id), - urlencoding::encode(redirect_uri), - urlencoding::encode(&state), - ); - - if !scopes.is_empty() { - auth_url.push_str(&format!( - "&scope={}", - urlencoding::encode(&scopes.join(" ")) - )); - } - - if let Some(ref challenge) = code_challenge { - auth_url.push_str(&format!( - "&code_challenge={}&code_challenge_method=S256", - challenge - )); - } - - for (key, value) in extra_params { - auth_url.push_str(&format!( - "&{}={}", - urlencoding::encode(key), - urlencoding::encode(value) - )); - } + client_id, + redirect_uri, + &state, + scopes, + code_challenge.as_deref(), + extra_params, + )?; - OAuthUrlResult { + Ok(OAuthUrlResult { url: auth_url, code_verifier, state, + }) +} + +/// Append OAuth authorization-request query parameters to `authorization_url`. +/// +/// Uses `url::Url::parse_with_params`-style encoding via `query_pairs_mut()` +/// so every value is percent-encoded exactly once with the standard +/// `application/x-www-form-urlencoded` rules. Any non-URL characters in +/// `scopes`, `extra_params`, `state`, etc. are encoded safely. +/// +/// Returns `Err(OAuthUrlError::MalformedConfig)` if `authorization_url` cannot +/// be parsed as a URL. We deliberately do not fall back to a manual +/// string-concat path: a malformed authorization URL is a config error that +/// the operator should see, not something the agent should try to paper over +/// (gemini-code-assist review on #2746). +fn build_oauth_authorization_url_string( + authorization_url: &str, + client_id: &str, + redirect_uri: &str, + state: &str, + scopes: &[String], + code_challenge: Option<&str>, + extra_params: &HashMap, +) -> Result { + let mut url = Url::parse(authorization_url).map_err(|e| { + OAuthUrlError::MalformedConfig(format!( + "could not parse authorization URL {authorization_url:?}: {e}" + )) + })?; + { + let mut qp = url.query_pairs_mut(); + qp.append_pair("client_id", client_id); + qp.append_pair("response_type", "code"); + qp.append_pair("redirect_uri", redirect_uri); + qp.append_pair("state", state); + if !scopes.is_empty() { + qp.append_pair("scope", &scopes.join(" ")); + } + if let Some(challenge) = code_challenge { + qp.append_pair("code_challenge", challenge); + qp.append_pair("code_challenge_method", "S256"); + } + for (key, value) in extra_params { + qp.append_pair(key, value); + } } + Ok(url.into()) } /// Exchange an OAuth authorization code for tokens. @@ -1553,7 +1608,8 @@ mod tests { &["openid".to_string(), "email".to_string()], false, &HashMap::new(), - ); + ) + .expect("well-formed authorization URL"); assert!( result @@ -1563,7 +1619,9 @@ mod tests { assert!(result.url.contains("client_id=my-client-id")); assert!(result.url.contains("response_type=code")); assert!(result.url.contains("redirect_uri=")); - assert!(result.url.contains("scope=openid%20email")); + // `url` crate uses `application/x-www-form-urlencoded` encoding for + // query parameters, which encodes spaces as `+`. + assert!(result.url.contains("scope=openid+email")); assert!(result.url.contains("state=")); assert!(result.code_verifier.is_none()); assert!(!result.state.is_empty()); @@ -1582,7 +1640,8 @@ mod tests { &[], true, &HashMap::new(), - ); + ) + .expect("well-formed authorization URL"); assert!(result.url.contains("code_challenge=")); assert!(result.url.contains("code_challenge_method=S256")); @@ -1608,12 +1667,233 @@ mod tests { &["read".to_string()], false, &extra, - ); + ) + .expect("well-formed authorization URL"); assert!(result.url.contains("access_type=offline")); assert!(result.url.contains("prompt=consent")); } + /// Regression test for nearai/ironclaw#2391: Google OAuth was receiving + /// `access_type=offlin` instead of `access_type=offline`, breaking the + /// offline-token flow required for any Google Workspace tool (Calendar, + /// Gmail, Drive, Docs, Sheets, Slides). The bug was reproducibly seen by + /// end users but not caught by tests that only used `.contains()`, since + /// `"access_type=offlin"` is a prefix of `"access_type=offline"` when the + /// URL ended elsewhere. We now parse the URL and compare each query + /// parameter value *exactly*. + #[test] + fn test_build_oauth_url_preserves_access_type_offline_exactly() { + use std::collections::HashMap; + + use crate::auth::oauth::build_oauth_url; + + let mut extra = HashMap::new(); + extra.insert("access_type".to_string(), "offline".to_string()); + extra.insert("prompt".to_string(), "consent".to_string()); + + let result = build_oauth_url( + "https://accounts.google.com/o/oauth2/v2/auth", + "test-client-id.apps.googleusercontent.com", + "http://127.0.0.1:9876/callback", + &["https://www.googleapis.com/auth/calendar.events".to_string()], + false, + &extra, + ) + .expect("well-formed authorization URL"); + + let parsed = url::Url::parse(&result.url).expect("auth url must be valid"); + let params: std::collections::HashMap<_, _> = parsed.query_pairs().into_owned().collect(); + + assert_eq!( + params.get("access_type").map(String::as_str), + Some("offline"), + "access_type must be exactly 'offline' (7 chars), not a truncated value; \ + got {:?} in URL {}", + params.get("access_type"), + result.url, + ); + assert_eq!( + params.get("prompt").map(String::as_str), + Some("consent"), + "prompt must be exactly 'consent', not a truncated value" + ); + assert_eq!( + params.get("client_id").map(String::as_str), + Some("test-client-id.apps.googleusercontent.com") + ); + assert_eq!( + params.get("response_type").map(String::as_str), + Some("code") + ); + assert_eq!( + params.get("redirect_uri").map(String::as_str), + Some("http://127.0.0.1:9876/callback") + ); + assert_eq!( + params.get("scope").map(String::as_str), + Some("https://www.googleapis.com/auth/calendar.events") + ); + } + + /// Regression test for nearai/ironclaw#2391: exercise the full set of + /// Google-style extra params (all six Google WASM tools share this + /// shape) and verify every value survives URL encoding intact. + /// + /// A single `HashMap` instance's iteration order is stable — reusing the + /// same map in a loop would not actually exercise different orderings + /// (Copilot review on #2746). We therefore rebuild `extra` on every + /// iteration so the randomized default hasher produces a fresh seed per + /// map, *and* explicitly drive every key-insertion permutation so each + /// param lands last at least once regardless of hasher behavior. + #[test] + fn test_build_oauth_url_extra_params_preserve_all_chars_across_hash_orderings() { + use std::collections::HashMap; + + use crate::auth::oauth::build_oauth_url; + + let entries: [(&str, &str); 3] = [ + ("access_type", "offline"), + ("prompt", "consent"), + ("include_granted_scopes", "true"), + ]; + + // Every permutation of insertion order (3! = 6), plus a few + // fresh-map iterations per permutation so the randomized hasher + // also contributes variation. + let permutations: [[usize; 3]; 6] = [ + [0, 1, 2], + [0, 2, 1], + [1, 0, 2], + [1, 2, 0], + [2, 0, 1], + [2, 1, 0], + ]; + + let mut case = 0usize; + for perm in &permutations { + for _ in 0..3 { + let mut extra: HashMap = HashMap::new(); + for &idx in perm { + let (k, v) = entries[idx]; + extra.insert(k.to_string(), v.to_string()); + } + + let result = build_oauth_url( + "https://accounts.google.com/o/oauth2/v2/auth", + "client-id", + "http://127.0.0.1:9876/callback", + &["https://www.googleapis.com/auth/gmail.modify".to_string()], + false, + &extra, + ) + .expect("well-formed authorization URL"); + + let parsed = url::Url::parse(&result.url).expect("auth url must be valid"); + let params: std::collections::HashMap<_, _> = + parsed.query_pairs().into_owned().collect(); + + for (k, v) in entries { + assert_eq!( + params.get(k).map(String::as_str), + Some(v), + "case {case} (perm {perm:?}): {k} truncated to {:?} in {}", + params.get(k), + result.url, + ); + } + case += 1; + } + } + } + + /// Regression test for nearai/ironclaw#2391: exercise the full CLI + /// `ironclaw tool auth google-calendar` code path end-to-end. Loads the + /// actual shipped capabilities JSON, parses it via + /// `CapabilitiesFile::from_json`, then calls `build_oauth_url` with the + /// exact `extra_params` the CLI would pass — the same call site as + /// `cli/tool.rs::auth_tool_oauth`. Per `.claude/rules/testing.md` + /// "Test Through the Caller, Not Just the Helper": a unit test on + /// `build_oauth_url` alone can miss a bug in the pipeline from + /// capabilities-JSON parsing to URL construction. + #[test] + fn test_google_calendar_capabilities_produce_correct_oauth_url() { + use crate::auth::oauth::build_oauth_url; + use crate::tools::wasm::CapabilitiesFile; + + // Pinned snapshot of the production google-calendar capabilities. + // Keep this byte-identical to tools-src/google-calendar/ + // google-calendar-tool.capabilities.json for the relevant fields. + let caps_json = r#"{ + "version": "0.2.0", + "description": "Google Calendar test fixture", + "auth": { + "secret_name": "google_oauth_token", + "display_name": "Google", + "oauth": { + "authorization_url": "https://accounts.google.com/o/oauth2/v2/auth", + "token_url": "https://oauth2.googleapis.com/token", + "client_id_env": "GOOGLE_OAUTH_CLIENT_ID", + "client_secret_env": "GOOGLE_OAUTH_CLIENT_SECRET", + "scopes": [ + "https://www.googleapis.com/auth/calendar.events" + ], + "use_pkce": false, + "extra_params": { + "access_type": "offline", + "prompt": "consent" + } + }, + "env_var": "GOOGLE_OAUTH_TOKEN" + } + }"#; + + let caps = CapabilitiesFile::from_json(caps_json) + .expect("google-calendar capabilities parse must succeed"); + let oauth = caps + .auth + .as_ref() + .expect("auth section present") + .oauth + .as_ref() + .expect("oauth section present"); + + // Sanity: the parsed extra_params haven't been mutated at load time. + assert_eq!( + oauth.extra_params.get("access_type").map(String::as_str), + Some("offline"), + "CapabilitiesFile::from_json must preserve access_type=offline \ + intact; got {:?}", + oauth.extra_params.get("access_type"), + ); + + let result = build_oauth_url( + &oauth.authorization_url, + "test-client.apps.googleusercontent.com", + "http://127.0.0.1:9876/callback", + &oauth.scopes, + oauth.use_pkce, + &oauth.extra_params, + ) + .expect("well-formed authorization URL"); + + let parsed = url::Url::parse(&result.url).expect("auth url must be valid"); + let params: std::collections::HashMap<_, _> = parsed.query_pairs().into_owned().collect(); + + assert_eq!( + params.get("access_type").map(String::as_str), + Some("offline"), + "end-to-end: google-calendar must send access_type=offline to \ + Google, not a truncated value. Full URL: {}", + result.url + ); + assert_eq!(params.get("prompt").map(String::as_str), Some("consent")); + assert_eq!( + params.get("scope").map(String::as_str), + Some("https://www.googleapis.com/auth/calendar.events") + ); + } + #[test] fn test_build_oauth_url_state_is_unique() { use std::collections::HashMap; @@ -1627,7 +1907,8 @@ mod tests { &[], false, &HashMap::new(), - ); + ) + .expect("well-formed authorization URL"); let result2 = build_oauth_url( "https://auth.example.com/authorize", "client", @@ -1635,12 +1916,39 @@ mod tests { &[], false, &HashMap::new(), - ); + ) + .expect("well-formed authorization URL"); // State should be different each time (random) assert_ne!(result1.state, result2.state); } + /// Malformed `authorization_url` values must be rejected with + /// `OAuthUrlError::MalformedConfig`, not silently normalized through + /// string concatenation (gemini-code-assist review on #2746). + #[test] + fn test_build_oauth_url_rejects_malformed_authorization_url() { + use std::collections::HashMap; + + use crate::auth::oauth::{OAuthUrlError, build_oauth_url}; + + let err = build_oauth_url( + "not a url", + "client", + "http://localhost:9876/callback", + &[], + false, + &HashMap::new(), + ) + .err() + .expect("malformed authorization URL must be rejected"); + + assert!( + matches!(err, OAuthUrlError::MalformedConfig(_)), + "expected MalformedConfig, got {err:?}", + ); + } + #[test] fn test_use_gateway_callback_false_by_default() { let _guard = lock_env(); @@ -1916,7 +2224,8 @@ mod tests { &["read".to_string()], true, &extra, - ); + ) + .expect("well-formed authorization URL"); // The resource parameter should be URL-encoded in the auth URL assert!( diff --git a/src/bridge/auth_manager.rs b/src/bridge/auth_manager.rs index 31e7503ff4c..ab5592e63cf 100644 --- a/src/bridge/auth_manager.rs +++ b/src/bridge/auth_manager.rs @@ -763,6 +763,18 @@ impl AuthManager { client_secret_expires_at: None, auto_activate_extension: false, }); + let launch = match launch { + Ok(launch) => launch, + Err(error) => { + tracing::error!( + credential_name = %credential_name, + user_id = %user_id, + error = %error, + "Skill OAuth launch rejected due to malformed descriptor; falling back to manual token entry" + ); + return None; + } + }; let pending_flow = launch.flow; if use_gateway { diff --git a/src/cli/tool.rs b/src/cli/tool.rs index da1da07e252..dfe94ae31b1 100644 --- a/src/cli/tool.rs +++ b/src/cli/tool.rs @@ -934,7 +934,8 @@ async fn auth_tool_oauth( &oauth.scopes, oauth.use_pkce, &oauth.extra_params, - ); + ) + .map_err(|e| anyhow::anyhow!(e.to_string()))?; let code_verifier = oauth_result.code_verifier; println!(" Opening browser for {} login...", display_name); diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index a10d0a3d118..5fc494d214e 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -3856,7 +3856,8 @@ impl ExtensionManager { client_secret_secret_name: None, client_secret_expires_at, auto_activate_extension: true, - }); + }) + .map_err(|e| ExtensionError::Config(e.to_string()))?; if is_gateway { let mut flow = launch.flow; @@ -4236,6 +4237,19 @@ impl ExtensionManager { ExtensionKind::WasmChannel | ExtensionKind::WasmTool ), }); + let launch = match launch { + Ok(launch) => launch, + Err(error) => { + tracing::error!( + extension_name = %extension_name, + secret_name = %secret_name, + user_id = %user_id, + error = %error, + "Secret-backed OAuth launch rejected due to malformed descriptor; falling back to manual token entry" + ); + return None; + } + }; let pending_flow = launch.flow; if self.should_use_gateway_mode() { @@ -4824,7 +4838,8 @@ impl ExtensionManager { client_secret_secret_name: None, client_secret_expires_at: None, auto_activate_extension: true, - }); + }) + .map_err(|e| e.to_string())?; if self.should_use_gateway_mode() { Ok(self diff --git a/src/tools/mcp/auth.rs b/src/tools/mcp/auth.rs index 7dfb1cac17d..20f11c516be 100644 --- a/src/tools/mcp/auth.rs +++ b/src/tools/mcp/auth.rs @@ -13,6 +13,7 @@ use rand::RngCore; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use tokio::net::TcpListener; +use url::Url; use crate::auth::oauth::{self, OAUTH_CALLBACK_PORT}; use crate::auth::resolve_access_token_string_with_refresh; @@ -121,6 +122,14 @@ pub enum AuthError { #[error("Secrets error: {0}")] Secrets(String), + + /// The server-advertised `authorization_endpoint` is not a parseable URL. + /// We reject malformed endpoints rather than concat-normalizing them so a + /// broken OAuth-server discovery document can't be silently "fixed up" + /// into a half-formed authorization request (gemini-code-assist review + /// on #2746). + #[error("Malformed OAuth configuration: {0}")] + MalformedConfig(String), } /// OAuth protected resource metadata. @@ -829,7 +838,7 @@ pub async fn authorize_mcp_server( pkce.as_ref(), &extra_params, Some(&resource), - ); + )?; // Open browser println!(" Opening browser for {} login...", server_config.name); @@ -892,6 +901,11 @@ pub async fn find_available_port() -> Result<(TcpListener, u16), AuthError> { } /// Build the authorization URL with all required parameters. +/// +/// Returns `Err(AuthError::MalformedConfig)` if `base_url` cannot be parsed. +/// A malformed authorization endpoint is a server/config bug that must be +/// surfaced to the operator, not concat-normalized into a half-valid URL +/// (gemini-code-assist review on #2746). pub fn build_authorization_url( base_url: &str, client_id: &str, @@ -900,41 +914,37 @@ pub fn build_authorization_url( pkce: Option<&PkceChallenge>, extra_params: &HashMap, resource: Option<&str>, -) -> String { - let mut url = format!( - "{}?client_id={}&response_type=code&redirect_uri={}", - base_url, - urlencoding::encode(client_id), - urlencoding::encode(redirect_uri) - ); - - if !scopes.is_empty() { - url.push_str(&format!( - "&scope={}", - urlencoding::encode(&scopes.join(" ")) - )); - } - - if let Some(pkce) = pkce { - url.push_str(&format!( - "&code_challenge={}&code_challenge_method=S256", - urlencoding::encode(&pkce.challenge) - )); - } - - for (key, value) in extra_params { - url.push_str(&format!( - "&{}={}", - urlencoding::encode(key), - urlencoding::encode(value) - )); - } - - if let Some(resource) = resource { - url.push_str(&format!("&resource={}", urlencoding::encode(resource))); +) -> Result { + // Use the `url` crate for query-string encoding so every value flows + // through a single well-tested `application/x-www-form-urlencoded` + // serializer. See nearai/ironclaw#2391 for the parallel bug in the + // WASM-tool OAuth URL builder where manual string concat was dropping + // the last character of the final query parameter. + let mut url = Url::parse(base_url).map_err(|e| { + AuthError::MalformedConfig(format!( + "could not parse authorization URL {base_url:?}: {e}" + )) + })?; + { + let mut qp = url.query_pairs_mut(); + qp.append_pair("client_id", client_id); + qp.append_pair("response_type", "code"); + qp.append_pair("redirect_uri", redirect_uri); + if !scopes.is_empty() { + qp.append_pair("scope", &scopes.join(" ")); + } + if let Some(pkce) = pkce { + qp.append_pair("code_challenge", &pkce.challenge); + qp.append_pair("code_challenge_method", "S256"); + } + for (key, value) in extra_params { + qp.append_pair(key, value); + } + if let Some(resource) = resource { + qp.append_pair("resource", resource); + } } - - url + Ok(url.into()) } /// Wait for the authorization callback and extract the code. @@ -1482,13 +1492,16 @@ mod tests { None, &HashMap::new(), None, - ); + ) + .expect("well-formed authorization URL"); assert!(url.starts_with("https://auth.example.com/authorize?")); assert!(url.contains("client_id=client-123")); assert!(url.contains("response_type=code")); assert!(url.contains("redirect_uri=")); - assert!(url.contains("scope=read%20write")); + // `url` crate uses `application/x-www-form-urlencoded` encoding for + // query parameters, which encodes spaces as `+`. + assert!(url.contains("scope=read+write")); } #[test] @@ -1502,7 +1515,8 @@ mod tests { Some(&pkce), &HashMap::new(), None, - ); + ) + .expect("well-formed authorization URL"); assert!(url.contains(&format!("code_challenge={}", pkce.challenge))); assert!(url.contains("code_challenge_method=S256")); @@ -1522,12 +1536,84 @@ mod tests { None, &extra, None, - ); + ) + .expect("well-formed authorization URL"); assert!(url.contains("owner=user")); assert!(url.contains("state=abc123")); } + /// Regression test for nearai/ironclaw#2391 applied to MCP OAuth: every + /// extra_param value must round-trip through URL-encoding intact. The + /// sibling bug in the WASM-tool OAuth builder was truncating the final + /// character of the last query parameter. Use URL-parse + exact + /// compare (not `.contains()`) so a 1-char truncation can't pass. + /// + /// A single `HashMap` instance's iteration order is stable — reusing the + /// same map in a loop would not actually exercise different orderings + /// (Copilot review on #2746). We therefore rebuild `extra` on every + /// iteration so the randomized default hasher produces a fresh seed per + /// map, *and* explicitly drive every key-insertion permutation so each + /// param lands last at least once regardless of hasher behavior. + #[test] + fn test_build_authorization_url_extra_params_preserve_all_chars() { + let entries: [(&str, &str); 3] = [ + ("access_type", "offline"), + ("prompt", "consent"), + ("audience", "api"), + ]; + + // Every permutation of insertion order (3! = 6), with a few + // fresh-map iterations per permutation so the randomized hasher + // also contributes variation. + let permutations: [[usize; 3]; 6] = [ + [0, 1, 2], + [0, 2, 1], + [1, 0, 2], + [1, 2, 0], + [2, 0, 1], + [2, 1, 0], + ]; + + let mut case = 0usize; + for perm in &permutations { + for _ in 0..3 { + let mut extra: HashMap = HashMap::new(); + for &idx in perm { + let (k, v) = entries[idx]; + extra.insert(k.to_string(), v.to_string()); + } + + let url = build_authorization_url( + "https://auth.example.com/authorize", + "client-123", + "http://localhost:9876/callback", + &["read".to_string()], + None, + &extra, + None, + ) + .expect("well-formed authorization URL"); + + let parsed = url::Url::parse(&url).expect("auth url must be valid"); + let params: std::collections::HashMap<_, _> = + parsed.query_pairs().into_owned().collect(); + + for (k, v) in entries { + assert_eq!( + params.get(k).map(String::as_str), + Some(v), + "case {case} (perm {perm:?}): {k} must be exactly {v:?} \ + (got {:?} in {})", + params.get(k), + url, + ); + } + case += 1; + } + } + } + #[test] fn test_pkce_challenge_s256_is_correct_sha256() { let pkce = PkceChallenge::generate(); @@ -1550,7 +1636,8 @@ mod tests { None, &HashMap::new(), None, - ); + ) + .expect("well-formed authorization URL"); // With no scopes, the URL must not contain a scope parameter at all. assert!(!url.contains("scope=")); @@ -1566,12 +1653,24 @@ mod tests { None, &HashMap::new(), None, - ); + ) + .expect("well-formed authorization URL"); - // Spaces and ampersands in client_id must be percent-encoded. - assert!(url.contains("client_id=client%20id%26evil%3Dtrue")); - // Spaces and question marks in redirect_uri must be percent-encoded. - assert!(url.contains("redirect_uri=http%3A%2F%2Flocalhost%3A9876%2Fcall%20back%3Fx%3D1")); + // `url` crate uses `application/x-www-form-urlencoded` encoding, so + // spaces become `+` and reserved characters remain percent-encoded. + // Parse the URL back and compare decoded values for robustness. + let parsed = url::Url::parse(&url).expect("auth url must be valid"); + let params: std::collections::HashMap<_, _> = parsed.query_pairs().into_owned().collect(); + assert_eq!( + params.get("client_id").map(String::as_str), + Some("client id&evil=true"), + "client_id must round-trip through URL encoding intact", + ); + assert_eq!( + params.get("redirect_uri").map(String::as_str), + Some("http://localhost:9876/call back?x=1"), + "redirect_uri must round-trip through URL encoding intact", + ); } #[test] @@ -2064,7 +2163,8 @@ mod tests { None, &HashMap::new(), Some("https://mcp.example.com/v1"), - ); + ) + .expect("well-formed authorization URL"); assert!(url.contains("resource=https%3A%2F%2Fmcp.example.com%2Fv1")); } @@ -2079,11 +2179,34 @@ mod tests { None, &HashMap::new(), None, - ); + ) + .expect("well-formed authorization URL"); assert!(!url.contains("resource=")); } + /// Malformed `base_url` values must be rejected with + /// `AuthError::MalformedConfig`, not silently normalized through string + /// concatenation (gemini-code-assist review on #2746). + #[test] + fn test_build_authorization_url_rejects_malformed_base_url() { + let err = build_authorization_url( + "not a url", + "client-123", + "http://localhost:9876/callback", + &[], + None, + &HashMap::new(), + None, + ) + .expect_err("malformed base URL must be rejected"); + + assert!( + matches!(err, AuthError::MalformedConfig(_)), + "expected MalformedConfig, got {err:?}", + ); + } + /// Regression test: MCP OAuth authorization URLs must include a `state` /// parameter. While OAuth 2.1 makes `state` optional when PKCE is used, /// some MCP servers (e.g. Attio) require it and reject requests without it: @@ -2121,7 +2244,8 @@ mod tests { Some(&pkce), &extra_params, Some("https://mcp.attio.com/mcp"), - ); + ) + .expect("well-formed authorization URL"); // State must be present in the URL assert!(