From b9319ce863a426a8de2b5c8a10f40a7683a1077e Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Fri, 5 Jun 2026 04:09:52 +0000 Subject: [PATCH 1/3] test(rmcp): cover OAuth refresh startup failures --- .../codex-mcp/src/connection_manager_tests.rs | 19 +- codex-rs/rmcp-client/src/oauth.rs | 34 +++ .../tests/streamable_http_oauth_startup.rs | 274 +++++++++++++++--- 3 files changed, 279 insertions(+), 48 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 60a0026eeb20..8cef561eda0c 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1279,7 +1279,9 @@ fn mcp_init_error_display_prompts_for_github_pat() { #[test] fn mcp_init_error_display_prompts_for_login_when_auth_required() { let server_name = "example"; - let err: StartupOutcomeError = anyhow::anyhow!("Auth required for server").into(); + let err: StartupOutcomeError = + anyhow::anyhow!("handshaking with MCP server failed: transport error: Auth required") + .into(); let display = mcp_init_error_display(server_name, /*entry*/ None, &err); @@ -1290,6 +1292,21 @@ fn mcp_init_error_display_prompts_for_login_when_auth_required() { assert_eq!(expected, display); } +#[test] +fn mcp_init_error_display_keeps_oauth_refresh_authorization_required_generic() { + let server_name = "example"; + let err: StartupOutcomeError = anyhow::anyhow!( + "handshaking with MCP server failed: transport error: Auth error: OAuth authorization required" + ) + .into(); + + let display = mcp_init_error_display(server_name, /*entry*/ None, &err); + + let expected = format!("MCP client for `{server_name}` failed to start: {err:#}"); + assert_eq!(expected, display); + assert!(!display.contains("codex mcp login")); +} + #[test] fn mcp_init_error_display_reports_generic_errors() { let server_name = "custom"; diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index c348460795de..1c989715de70 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -699,6 +699,21 @@ mod tests { Ok(()) } + #[test] + fn load_oauth_tokens_from_keyring_propagates_errors() -> Result<()> { + let _env = TempCodexHome::new(); + let store = MockKeyringStore::default(); + let tokens = sample_tokens(); + let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; + store.set_error(&key, KeyringError::Invalid("error".into(), "load".into())); + + let error = super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url) + .expect_err("explicit keyring reads should propagate keyring failures"); + + assert!(error.to_string().contains("error")); + Ok(()) + } + #[test] fn save_oauth_tokens_prefers_keyring_when_available() -> Result<()> { let _env = TempCodexHome::new(); @@ -751,6 +766,25 @@ mod tests { Ok(()) } + #[test] + fn save_oauth_tokens_to_keyring_propagates_errors() -> Result<()> { + let _env = TempCodexHome::new(); + let store = MockKeyringStore::default(); + let tokens = sample_tokens(); + let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; + store.set_error(&key, KeyringError::Invalid("error".into(), "save".into())); + + let error = super::save_oauth_tokens_with_keyring(&store, &tokens.server_name, &tokens) + .expect_err("explicit keyring writes should propagate keyring failures"); + + assert!( + error + .to_string() + .contains("failed to write OAuth tokens to keyring") + ); + Ok(()) + } + #[test] fn delete_oauth_tokens_removes_all_storage() -> Result<()> { let _env = TempCodexHome::new(); diff --git a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs index 1c18f2c98bda..c5c050f503c3 100644 --- a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs +++ b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs @@ -1,5 +1,7 @@ mod streamable_http_test_support; +use std::fs; +use std::path::PathBuf; use std::time::Duration; use codex_config::types::OAuthCredentialsStoreMode; @@ -33,10 +35,211 @@ const EXPIRED_ACCESS_TOKEN: &str = "expired-access-token"; const REFRESH_TOKEN: &str = "valid-refresh-token"; const REFRESHED_ACCESS_TOKEN: &str = "refreshed-access-token"; const CHILD_SERVER_URL_ENV: &str = "MCP_TEST_OAUTH_STARTUP_SERVER_URL"; +const CHILD_SCENARIO_ENV: &str = "MCP_TEST_OAUTH_STARTUP_SCENARIO"; +const CREDENTIALS_FILENAME: &str = ".credentials.json"; +const SCENARIO_SUCCESS: &str = "success"; +const SCENARIO_INVALID_GRANT: &str = "invalid_grant"; +const SCENARIO_TRANSIENT_FAILURE: &str = "transient_failure"; +const SCENARIO_LOAD_FAILURE: &str = "load_failure"; +const SCENARIO_PERSIST_FAILURE: &str = "persist_failure"; #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn refreshes_expired_persisted_token_before_initialize() -> anyhow::Result<()> { let server = MockServer::start().await; + mount_oauth_metadata(&server).await; + mount_successful_refresh(&server, /*expected_calls*/ 1).await; + mount_successful_mcp_requests(&server, /*expected_calls*/ 2).await; + + run_oauth_startup_child(&server, SCENARIO_SUCCESS).await?; + server.verify().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn invalid_grant_stops_before_initialize() -> anyhow::Result<()> { + let server = MockServer::start().await; + mount_oauth_metadata(&server).await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(400).set_body_json(json!({ + "error": "invalid_grant", + "error_description": "refresh token revoked", + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/mcp")) + .respond_with(ResponseTemplate::new(500)) + .expect(0) + .mount(&server) + .await; + + run_oauth_startup_child(&server, SCENARIO_INVALID_GRANT).await?; + server.verify().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn transient_refresh_failure_stops_before_initialize() -> anyhow::Result<()> { + let server = MockServer::start().await; + mount_oauth_metadata(&server).await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .respond_with(ResponseTemplate::new(503).set_body_string("temporarily unavailable")) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/mcp")) + .respond_with(ResponseTemplate::new(500)) + .expect(0) + .mount(&server) + .await; + + run_oauth_startup_child(&server, SCENARIO_TRANSIENT_FAILURE).await?; + server.verify().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn credential_load_failure_sends_unauthenticated_initialize() -> anyhow::Result<()> { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/mcp")) + .respond_with(|request: &Request| { + assert!( + !request.headers.contains_key("authorization"), + "credential load failure should fall through to an unauthenticated initialize" + ); + ResponseTemplate::new(401).insert_header("www-authenticate", "Bearer") + }) + .expect(1) + .mount(&server) + .await; + + run_oauth_startup_child(&server, SCENARIO_LOAD_FAILURE).await?; + server.verify().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn credential_persist_failure_is_swallowed_after_initialize() -> anyhow::Result<()> { + let server = MockServer::start().await; + mount_oauth_metadata(&server).await; + mount_successful_refresh(&server, /*expected_calls*/ 2).await; + mount_successful_mcp_requests(&server, /*expected_calls*/ 3).await; + + run_oauth_startup_child(&server, SCENARIO_PERSIST_FAILURE).await?; + server.verify().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +#[ignore = "spawned by OAuth startup integration tests"] +async fn oauth_startup_child() -> anyhow::Result<()> { + let server_url = std::env::var(CHILD_SERVER_URL_ENV)?; + let scenario = std::env::var(CHILD_SCENARIO_ENV)?; + let credentials_path = credentials_path()?; + + if scenario == SCENARIO_LOAD_FAILURE { + fs::create_dir(&credentials_path)?; + } else { + save_expired_tokens(&server_url)?; + } + + // This mirrors create_client's transport and initialization setup, except + // it omits the direct bearer token. Supplying that token would bypass the + // persisted OAuth credentials and the startup refresh under test. + let client = RmcpClient::new_streamable_http_client( + SERVER_NAME, + &server_url, + /*bearer_token*/ None, + /*http_headers*/ None, + /*env_http_headers*/ None, + OAuthCredentialsStoreMode::File, + Environment::default_for_tests().get_http_client(), + /*auth_provider*/ None, + ) + .await?; + + let stale_credentials_backup = credentials_path.with_extension("json.stale"); + if scenario == SCENARIO_PERSIST_FAILURE { + fs::rename(&credentials_path, &stale_credentials_backup)?; + fs::create_dir(&credentials_path)?; + } + + let result = initialize_client(&client).await; + match scenario.as_str() { + SCENARIO_SUCCESS => result?, + SCENARIO_INVALID_GRANT | SCENARIO_TRANSIENT_FAILURE => { + let error = result.expect_err("refresh failure should stop startup"); + let error = format!("{error:#}"); + assert!( + error.contains("Auth error: OAuth authorization required"), + "unexpected refresh failure: {error}" + ); + assert!( + !error.contains("invalid_grant") && !error.contains("temporarily unavailable"), + "RMCP should collapse refresh details to authorization required: {error}" + ); + } + SCENARIO_LOAD_FAILURE => { + let error = result.expect_err("unauthenticated initialize should be rejected"); + let error = format!("{error:#}"); + assert!( + error.contains("Auth required"), + "unexpected credential load failure: {error}" + ); + } + SCENARIO_PERSIST_FAILURE => { + result?; + client + .list_tools( + /*params*/ None, + /*timeout*/ Some(Duration::from_secs(5)), + ) + .await?; + assert!( + credentials_path.is_dir(), + "failed persistence should leave the blocking directory in place" + ); + let stale_credentials = fs::read_to_string(stale_credentials_backup)?; + assert!( + stale_credentials.contains(EXPIRED_ACCESS_TOKEN), + "the only persisted credentials should remain stale" + ); + assert!( + !stale_credentials.contains(REFRESHED_ACCESS_TOKEN), + "refreshed credentials should not have been persisted" + ); + } + other => anyhow::bail!("unknown OAuth startup scenario: {other}"), + } + Ok(()) +} + +async fn run_oauth_startup_child(server: &MockServer, scenario: &str) -> anyhow::Result<()> { + let codex_home = TempDir::new()?; + let server_url = format!("{}/mcp", server.uri()); + + // Credential storage resolves CODEX_HOME from the process environment. + // Run the client half of the test in an ignored helper test so it can use + // an isolated home without mutating the parent test runner's environment. + let status = Command::new(std::env::current_exe()?) + .args(["oauth_startup_child", "--exact", "--ignored", "--nocapture"]) + .env("CODEX_HOME", codex_home.path()) + .env(CHILD_SERVER_URL_ENV, server_url) + .env(CHILD_SCENARIO_ENV, scenario) + .status() + .await?; + assert!(status.success(), "OAuth startup child failed: {status}"); + Ok(()) +} + +async fn mount_oauth_metadata(server: &MockServer) { Mock::given(method("GET")) .and(path("/.well-known/oauth-authorization-server/mcp")) .respond_with(ResponseTemplate::new(200).set_body_json(json!({ @@ -45,8 +248,11 @@ async fn refreshes_expired_persisted_token_before_initialize() -> anyhow::Result "scopes_supported": [""], }))) .expect(1) - .mount(&server) + .mount(server) .await; +} + +async fn mount_successful_refresh(server: &MockServer, expected_calls: u64) { Mock::given(method("POST")) .and(path("/oauth/token")) .and(body_string_contains("grant_type=refresh_token")) @@ -59,9 +265,12 @@ async fn refreshes_expired_persisted_token_before_initialize() -> anyhow::Result "expires_in": 7200, "refresh_token": REFRESH_TOKEN, }))) - .expect(1) - .mount(&server) + .expect(expected_calls) + .mount(server) .await; +} + +async fn mount_successful_mcp_requests(server: &MockServer, expected_calls: u64) { Mock::given(method("POST")) .and(path("/mcp")) .and(header( @@ -87,38 +296,23 @@ async fn refreshes_expired_persisted_token_before_initialize() -> anyhow::Result }, })), Some("notifications/initialized") => ResponseTemplate::new(202), + Some("tools/list") => ResponseTemplate::new(200).set_body_json(json!({ + "jsonrpc": "2.0", + "id": body.get("id").cloned().unwrap_or(Value::Null), + "result": { + "tools": [], + }, + })), method => ResponseTemplate::new(400) .set_body_string(format!("unexpected JSON-RPC method: {method:?}")), } }) - .expect(2) - .mount(&server) + .expect(expected_calls) + .mount(server) .await; - - let codex_home = TempDir::new()?; - let server_url = format!("{}/mcp", server.uri()); - - // Credential storage resolves CODEX_HOME from the process environment. - // Run the client half of the test in an ignored helper test so it can use - // an isolated home without mutating the parent test runner's environment. - let status = Command::new(std::env::current_exe()?) - .args(["oauth_startup_child", "--exact", "--ignored", "--nocapture"]) - .env("CODEX_HOME", codex_home.path()) - .env(CHILD_SERVER_URL_ENV, server_url) - .status() - .await?; - assert!(status.success(), "OAuth startup child failed: {status}"); - server.verify().await; - Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -#[ignore = "spawned by refreshes_expired_persisted_token_before_initialize"] -async fn oauth_startup_child() -> anyhow::Result<()> { - let server_url = std::env::var(CHILD_SERVER_URL_ENV)?; - - // Save an expired access token with a valid refresh token so startup must - // refresh before sending the initialize request. +fn save_expired_tokens(server_url: &str) -> anyhow::Result<()> { let mut response = OAuthTokenResponse::new( AccessToken::new(EXPIRED_ACCESS_TOKEN.to_string()), BasicTokenType::Bearer, @@ -128,28 +322,14 @@ async fn oauth_startup_child() -> anyhow::Result<()> { response.set_expires_in(Some(&Duration::from_secs(7200))); let tokens = StoredOAuthTokens { server_name: SERVER_NAME.to_string(), - url: server_url.clone(), + url: server_url.to_string(), client_id: "test-client-id".to_string(), token_response: WrappedOAuthTokenResponse(response), expires_at: Some(0), }; - save_oauth_tokens(SERVER_NAME, &tokens, OAuthCredentialsStoreMode::File)?; - - // This mirrors create_client's transport and initialization setup, except - // it omits the direct bearer token. Supplying that token would bypass the - // persisted OAuth credentials and the startup refresh under test. - let client = RmcpClient::new_streamable_http_client( - SERVER_NAME, - &server_url, - /*bearer_token*/ None, - /*http_headers*/ None, - /*env_http_headers*/ None, - OAuthCredentialsStoreMode::File, - Environment::default_for_tests().get_http_client(), - /*auth_provider*/ None, - ) - .await?; + save_oauth_tokens(SERVER_NAME, &tokens, OAuthCredentialsStoreMode::File) +} - initialize_client(&client).await?; - Ok(()) +fn credentials_path() -> anyhow::Result { + Ok(PathBuf::from(std::env::var("CODEX_HOME")?).join(CREDENTIALS_FILENAME)) } From d67504e362f4f84ac3b816889b5641239d9b3562 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Fri, 5 Jun 2026 04:43:55 +0000 Subject: [PATCH 2/3] fix(rmcp): classify OAuth startup failures --- codex-rs/codex-mcp/src/connection_manager.rs | 6 +- .../codex-mcp/src/connection_manager_tests.rs | 36 +++++- codex-rs/rmcp-client/src/lib.rs | 1 + codex-rs/rmcp-client/src/oauth.rs | 66 +++++++++-- codex-rs/rmcp-client/src/rmcp_client.rs | 13 +-- .../tests/streamable_http_oauth_startup.rs | 107 ++++++++++++------ 6 files changed, 170 insertions(+), 59 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index d15a2e150047..5a6f82056d88 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -55,6 +55,7 @@ use codex_protocol::protocol::McpStartupFailure; use codex_protocol::protocol::McpStartupStatus; use codex_protocol::protocol::McpStartupUpdateEvent; use codex_rmcp_client::ElicitationResponse; +use codex_rmcp_client::OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR; use rmcp::model::ElicitationCapability; use rmcp::model::ListResourceTemplatesResult; use rmcp::model::ListResourcesResult; @@ -860,7 +861,10 @@ fn startup_outcome_error_message(error: StartupOutcomeError) -> String { fn is_mcp_client_auth_required_error(error: &StartupOutcomeError) -> bool { match error { - StartupOutcomeError::Failed { error } => error.contains("Auth required"), + StartupOutcomeError::Failed { error } => { + error.contains("Auth required") + || error.contains(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR) + } _ => false, } } diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 8cef561eda0c..67b8794715db 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -26,6 +26,7 @@ use codex_protocol::mcp::McpServerInfo; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::McpAuthStatus; +use codex_rmcp_client::OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR; use futures::FutureExt; use pretty_assertions::assert_eq; use rmcp::model::CreateElicitationRequestParams; @@ -1293,12 +1294,37 @@ fn mcp_init_error_display_prompts_for_login_when_auth_required() { } #[test] -fn mcp_init_error_display_keeps_oauth_refresh_authorization_required_generic() { +fn mcp_init_error_display_prompts_for_login_when_refresh_token_is_rejected() { let server_name = "example"; - let err: StartupOutcomeError = anyhow::anyhow!( - "handshaking with MCP server failed: transport error: Auth error: OAuth authorization required" - ) - .into(); + let err: StartupOutcomeError = + anyhow::anyhow!(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR).into(); + + let display = mcp_init_error_display(server_name, /*entry*/ None, &err); + + let expected = format!( + "The {server_name} MCP server is not logged in. Run `codex mcp login {server_name}`." + ); + assert_eq!(expected, display); +} + +#[test] +fn mcp_init_error_display_keeps_transient_oauth_refresh_failure_generic() { + let server_name = "example"; + let err: StartupOutcomeError = + anyhow::anyhow!("OAuth token endpoint refresh failed: 503 Service Unavailable").into(); + + let display = mcp_init_error_display(server_name, /*entry*/ None, &err); + + let expected = format!("MCP client for `{server_name}` failed to start: {err:#}"); + assert_eq!(expected, display); + assert!(!display.contains("codex mcp login")); +} + +#[test] +fn mcp_init_error_display_keeps_credential_store_failure_generic() { + let server_name = "example"; + let err: StartupOutcomeError = + anyhow::anyhow!("failed to read OAuth credentials: secure storage is locked").into(); let display = mcp_init_error_display(server_name, /*entry*/ None, &err); diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index e1ee18c75324..f3d2270d03f6 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -17,6 +17,7 @@ pub use auth_status::discover_streamable_http_oauth; pub use auth_status::supports_oauth_login; pub use codex_protocol::protocol::McpAuthStatus; pub use in_process_transport::InProcessTransportFactory; +pub use oauth::OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR; pub use oauth::StoredOAuthTokens; pub use oauth::WrappedOAuthTokenResponse; pub use oauth::delete_oauth_tokens; diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 1c989715de70..38a48d0af052 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -19,6 +19,7 @@ use anyhow::Context; use anyhow::Error; use anyhow::Result; +use anyhow::anyhow; use codex_config::types::OAuthCredentialsStoreMode; use oauth2::AccessToken; use oauth2::RefreshToken; @@ -45,6 +46,7 @@ use tracing::warn; use codex_keyring_store::DefaultKeyringStore; use codex_keyring_store::KeyringStore; +use rmcp::transport::auth::AuthError; use rmcp::transport::auth::AuthorizationManager; use tokio::sync::Mutex; @@ -52,6 +54,8 @@ use codex_utils_home_dir::find_codex_home; const KEYRING_SERVICE: &str = "Codex MCP Credentials"; const REFRESH_SKEW_MILLIS: u64 = 30_000; +pub const OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR: &str = + "OAuth refresh token was rejected; reauthentication required"; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct StoredOAuthTokens { @@ -321,8 +325,9 @@ impl OAuthPersistor { expires_at, }; if last_credentials.as_ref() != Some(&stored) { + // Preserve the refreshed in-memory state even when durable storage fails. + *last_credentials = Some(stored.clone()); save_oauth_tokens(&self.inner.server_name, &stored, self.inner.store_mode)?; - *last_credentials = Some(stored); } } None => { @@ -345,6 +350,40 @@ impl OAuthPersistor { Ok(()) } + #[expect( + clippy::await_holding_invalid_type, + reason = "AuthorizationManager async access must be serialized through its mutex" + )] + pub(crate) async fn refresh_for_startup_if_needed(&self) -> Result<()> { + let expires_at = { + let guard = self.inner.last_credentials.lock().await; + guard.as_ref().and_then(|tokens| tokens.expires_at) + }; + + if !token_needs_refresh(expires_at) { + return Ok(()); + } + + let manager = self.inner.authorization_manager.clone(); + let guard = manager.lock().await; + match guard.refresh_token().await { + Ok(_) => Ok(()), + Err(AuthError::TokenRefreshFailed(message)) if message.contains("invalid_grant") => { + Err(anyhow!(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR)) + } + Err(AuthError::TokenRefreshFailed(message)) => Err(anyhow!( + "OAuth token endpoint refresh failed for server {}: {message}", + self.inner.server_name + )), + Err(error) => Err(error).with_context(|| { + format!( + "failed to refresh OAuth tokens for server {}", + self.inner.server_name + ) + }), + } + } + #[expect( clippy::await_holding_invalid_type, reason = "AuthorizationManager async access must be serialized through its mutex" @@ -574,7 +613,8 @@ fn write_fallback_file(store: &FallbackFile) -> Result<()> { } let serialized = serde_json::to_string(store)?; - fs::write(&path, serialized)?; + fs::write(&path, serialized) + .with_context(|| format!("failed to write credentials file at {}", path.display()))?; #[cfg(unix)] { @@ -705,12 +745,18 @@ mod tests { let store = MockKeyringStore::default(); let tokens = sample_tokens(); let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; - store.set_error(&key, KeyringError::Invalid("error".into(), "load".into())); + let injected_error = + KeyringError::Invalid("injected-load".into(), "underlying load failure".into()); + let injected_cause = injected_error.to_string(); + store.set_error(&key, injected_error); let error = super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url) .expect_err("explicit keyring reads should propagate keyring failures"); - assert!(error.to_string().contains("error")); + assert_eq!( + error.chain().map(ToString::to_string).collect::>(), + vec![injected_cause] + ); Ok(()) } @@ -772,15 +818,17 @@ mod tests { let store = MockKeyringStore::default(); let tokens = sample_tokens(); let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; - store.set_error(&key, KeyringError::Invalid("error".into(), "save".into())); + let injected_error = + KeyringError::Invalid("injected-save".into(), "underlying save failure".into()); + let injected_cause = injected_error.to_string(); + store.set_error(&key, injected_error); let error = super::save_oauth_tokens_with_keyring(&store, &tokens.server_name, &tokens) .expect_err("explicit keyring writes should propagate keyring failures"); - assert!( - error - .to_string() - .contains("failed to write OAuth tokens to keyring") + assert_eq!( + error.chain().last().map(ToString::to_string), + Some(injected_cause) ); Ok(()) } diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 90b09d724c3d..25caba19bd98 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -9,6 +9,7 @@ use std::sync::atomic::Ordering; use std::time::Duration; use std::time::Instant; +use anyhow::Context; use anyhow::Result; use anyhow::anyhow; use codex_api::SharedAuthProvider; @@ -737,13 +738,9 @@ impl RmcpClient { && auth_provider.is_none() && !default_headers.contains_key(AUTHORIZATION) { - match load_oauth_tokens(server_name, url, *store_mode) { - Ok(tokens) => tokens, - Err(err) => { - warn!("failed to read tokens for server `{server_name}`: {err}"); - None - } - } + load_oauth_tokens(server_name, url, *store_mode).with_context(|| { + format!("failed to read OAuth credentials for MCP server `{server_name}`") + })? } else { None }; @@ -1058,6 +1055,8 @@ async fn create_oauth_transport_and_runtime( Some(initial_tokens), ); + runtime.refresh_for_startup_if_needed().await?; + Ok((transport, runtime)) } diff --git a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs index c5c050f503c3..dced61f84a77 100644 --- a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs +++ b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs @@ -6,6 +6,7 @@ use std::time::Duration; use codex_config::types::OAuthCredentialsStoreMode; use codex_exec_server::Environment; +use codex_rmcp_client::OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR; use codex_rmcp_client::RmcpClient; use codex_rmcp_client::StoredOAuthTokens; use codex_rmcp_client::WrappedOAuthTokenResponse; @@ -105,18 +106,12 @@ async fn transient_refresh_failure_stops_before_initialize() -> anyhow::Result<( } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn credential_load_failure_sends_unauthenticated_initialize() -> anyhow::Result<()> { +async fn credential_load_failure_stops_before_initialize() -> anyhow::Result<()> { let server = MockServer::start().await; Mock::given(method("POST")) .and(path("/mcp")) - .respond_with(|request: &Request| { - assert!( - !request.headers.contains_key("authorization"), - "credential load failure should fall through to an unauthenticated initialize" - ); - ResponseTemplate::new(401).insert_header("www-authenticate", "Bearer") - }) - .expect(1) + .respond_with(ResponseTemplate::new(500)) + .expect(0) .mount(&server) .await; @@ -129,7 +124,7 @@ async fn credential_load_failure_sends_unauthenticated_initialize() -> anyhow::R async fn credential_persist_failure_is_swallowed_after_initialize() -> anyhow::Result<()> { let server = MockServer::start().await; mount_oauth_metadata(&server).await; - mount_successful_refresh(&server, /*expected_calls*/ 2).await; + mount_successful_refresh(&server, /*expected_calls*/ 1).await; mount_successful_mcp_requests(&server, /*expected_calls*/ 3).await; run_oauth_startup_child(&server, SCENARIO_PERSIST_FAILURE).await?; @@ -163,37 +158,71 @@ async fn oauth_startup_child() -> anyhow::Result<()> { Environment::default_for_tests().get_http_client(), /*auth_provider*/ None, ) - .await?; + .await; + + if matches!( + scenario.as_str(), + SCENARIO_INVALID_GRANT | SCENARIO_TRANSIENT_FAILURE | SCENARIO_LOAD_FAILURE + ) { + let error = match client { + Ok(_) => anyhow::bail!("OAuth startup should fail for scenario {scenario}"), + Err(error) => format!("{error:#}"), + }; + match scenario.as_str() { + SCENARIO_INVALID_GRANT => { + assert!( + error.contains(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR), + "unexpected invalid_grant failure: {error}" + ); + assert!( + !error.contains("invalid_grant") && !error.contains("refresh token revoked"), + "revoked-token provider details should be hidden: {error}" + ); + } + SCENARIO_TRANSIENT_FAILURE => { + assert!( + error.contains("OAuth token endpoint refresh failed"), + "unexpected transient refresh failure: {error}" + ); + assert!( + !error.contains(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR), + "transient refresh failure should not request reauthentication: {error}" + ); + } + SCENARIO_LOAD_FAILURE => { + assert!( + error.contains("failed to read OAuth credentials for MCP server"), + "unexpected credential load failure: {error}" + ); + assert!( + error.contains("failed to read credentials file"), + "credential storage cause should be preserved: {error}" + ); + assert!( + !error.contains("Auth required"), + "credential storage failure should not be classified as unauthenticated: {error}" + ); + } + _ => unreachable!(), + } + return Ok(()); + } + let client = client?; - let stale_credentials_backup = credentials_path.with_extension("json.stale"); + let original_permissions = fs::metadata(&credentials_path)?.permissions(); if scenario == SCENARIO_PERSIST_FAILURE { - fs::rename(&credentials_path, &stale_credentials_backup)?; - fs::create_dir(&credentials_path)?; + let mut readonly_permissions = original_permissions.clone(); + readonly_permissions.set_readonly(true); + fs::set_permissions(&credentials_path, readonly_permissions)?; + let stale_credentials = fs::read_to_string(&credentials_path)?; + let write_error = fs::write(&credentials_path, &stale_credentials) + .expect_err("read-only credentials file should reject writes"); + assert_eq!(write_error.kind(), std::io::ErrorKind::PermissionDenied); } let result = initialize_client(&client).await; match scenario.as_str() { SCENARIO_SUCCESS => result?, - SCENARIO_INVALID_GRANT | SCENARIO_TRANSIENT_FAILURE => { - let error = result.expect_err("refresh failure should stop startup"); - let error = format!("{error:#}"); - assert!( - error.contains("Auth error: OAuth authorization required"), - "unexpected refresh failure: {error}" - ); - assert!( - !error.contains("invalid_grant") && !error.contains("temporarily unavailable"), - "RMCP should collapse refresh details to authorization required: {error}" - ); - } - SCENARIO_LOAD_FAILURE => { - let error = result.expect_err("unauthenticated initialize should be rejected"); - let error = format!("{error:#}"); - assert!( - error.contains("Auth required"), - "unexpected credential load failure: {error}" - ); - } SCENARIO_PERSIST_FAILURE => { result?; client @@ -202,11 +231,12 @@ async fn oauth_startup_child() -> anyhow::Result<()> { /*timeout*/ Some(Duration::from_secs(5)), ) .await?; + fs::set_permissions(&credentials_path, original_permissions)?; assert!( - credentials_path.is_dir(), - "failed persistence should leave the blocking directory in place" + credentials_path.is_file(), + "failed persistence should leave the credentials file in place" ); - let stale_credentials = fs::read_to_string(stale_credentials_backup)?; + let stale_credentials = fs::read_to_string(&credentials_path)?; assert!( stale_credentials.contains(EXPIRED_ACCESS_TOKEN), "the only persisted credentials should remain stale" @@ -216,6 +246,9 @@ async fn oauth_startup_child() -> anyhow::Result<()> { "refreshed credentials should not have been persisted" ); } + SCENARIO_INVALID_GRANT | SCENARIO_TRANSIENT_FAILURE | SCENARIO_LOAD_FAILURE => { + unreachable!() + } other => anyhow::bail!("unknown OAuth startup scenario: {other}"), } Ok(()) From 4a3e98a74b086e18aab815387acbb9c93944c6ba Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Fri, 5 Jun 2026 05:23:37 +0000 Subject: [PATCH 3/3] fix(rmcp): retain refreshed OAuth credentials --- .../codex-mcp/src/connection_manager_tests.rs | 25 +- codex-rs/rmcp-client/src/oauth.rs | 382 ++++++++++++---- codex-rs/rmcp-client/src/rmcp_client.rs | 33 +- .../tests/streamable_http_oauth_startup.rs | 421 +++++++++++------- .../tests/streamable_http_test_support.rs | 9 +- 5 files changed, 623 insertions(+), 247 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 67b8794715db..8981e2f91cff 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1321,15 +1321,34 @@ fn mcp_init_error_display_keeps_transient_oauth_refresh_failure_generic() { } #[test] -fn mcp_init_error_display_keeps_credential_store_failure_generic() { +fn mcp_init_error_display_preserves_credential_store_read_cause() { let server_name = "example"; - let err: StartupOutcomeError = - anyhow::anyhow!("failed to read OAuth credentials: secure storage is locked").into(); + let err: StartupOutcomeError = anyhow::anyhow!( + "failed to read OAuth credentials for MCP server `example`: failed to read credentials file at /tmp/.credentials.json: Is a directory" + ) + .into(); + + let display = mcp_init_error_display(server_name, /*entry*/ None, &err); + + let expected = format!("MCP client for `{server_name}` failed to start: {err:#}"); + assert_eq!(expected, display); + assert!(display.contains("Is a directory")); + assert!(!display.contains("codex mcp login")); +} + +#[test] +fn mcp_init_error_display_preserves_credential_store_write_cause() { + let server_name = "example"; + let err: StartupOutcomeError = anyhow::anyhow!( + "failed to write credentials file at /tmp/.credentials.json: Permission denied" + ) + .into(); let display = mcp_init_error_display(server_name, /*entry*/ None, &err); let expected = format!("MCP client for `{server_name}` failed to start: {err:#}"); assert_eq!(expected, display); + assert!(display.contains("Permission denied")); assert!(!display.contains("codex mcp login")); } diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 38a48d0af052..22863d1d54f6 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -39,6 +39,7 @@ use std::fs; use std::io::ErrorKind; use std::path::PathBuf; use std::sync::Arc; +use std::sync::Mutex as StdMutex; use std::time::Duration; use std::time::SystemTime; use std::time::UNIX_EPOCH; @@ -54,6 +55,9 @@ use codex_utils_home_dir::find_codex_home; const KEYRING_SERVICE: &str = "Codex MCP Credentials"; const REFRESH_SKEW_MILLIS: u64 = 30_000; +const MAX_OAUTH_ERROR_CAUSE_CHARS: usize = 512; +const MISSING_REFRESH_TOKEN_ERROR: &str = "No refresh token available"; +const OAUTH_SERVER_ERROR_PREFIX: &str = "Server returned error response: "; pub const OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR: &str = "OAuth refresh token was rejected; reauthentication required"; @@ -197,10 +201,8 @@ fn save_oauth_tokens_with_keyring( Ok(()) } Err(error) => { - let message = format!( - "failed to write OAuth tokens to keyring: {}", - error.message() - ); + let cause = format_bounded_oauth_error_message(&error.message()); + let message = format!("failed to write OAuth tokens to keyring: {cause}"); warn!("{message}"); Err(Error::new(error.into_error()).context(message)) } @@ -269,7 +271,12 @@ struct OAuthPersistorInner { url: String, authorization_manager: Arc>, store_mode: OAuthCredentialsStoreMode, - last_credentials: Mutex>, + credential_state: StdMutex, +} + +struct OAuthCredentialState { + current: Option, + persisted: Option, } impl OAuthPersistor { @@ -286,7 +293,10 @@ impl OAuthPersistor { url, authorization_manager, store_mode, - last_credentials: Mutex::new(initial_credentials), + credential_state: StdMutex::new(OAuthCredentialState { + current: initial_credentials.clone(), + persisted: initial_credentials, + }), }), } } @@ -305,34 +315,15 @@ impl OAuthPersistor { }?; match maybe_credentials { - Some(credentials) => { - let mut last_credentials = self.inner.last_credentials.lock().await; - let new_token_response = WrappedOAuthTokenResponse(credentials.clone()); - let same_token = last_credentials - .as_ref() - .map(|prev| prev.token_response == new_token_response) - .unwrap_or(false); - let expires_at = if same_token { - last_credentials.as_ref().and_then(|prev| prev.expires_at) - } else { - compute_expires_at_millis(&credentials) - }; - let stored = StoredOAuthTokens { - server_name: self.inner.server_name.clone(), - url: self.inner.url.clone(), - client_id, - token_response: new_token_response, - expires_at, - }; - if last_credentials.as_ref() != Some(&stored) { - // Preserve the refreshed in-memory state even when durable storage fails. - *last_credentials = Some(stored.clone()); - save_oauth_tokens(&self.inner.server_name, &stored, self.inner.store_mode)?; - } - } + Some(credentials) => self.persist_credentials(client_id, credentials)?, None => { - let mut last_serialized = self.inner.last_credentials.lock().await; - if last_serialized.take().is_some() + let mut state = self + .inner + .credential_state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + state.current = None; + if state.persisted.take().is_some() && let Err(error) = delete_oauth_tokens( &self.inner.server_name, &self.inner.url, @@ -340,8 +331,9 @@ impl OAuthPersistor { ) { warn!( - "failed to remove OAuth tokens for server {}: {error}", - self.inner.server_name + "failed to remove OAuth tokens for server {}: {}", + self.inner.server_name, + format_bounded_oauth_error(&error) ); } } @@ -350,66 +342,139 @@ impl OAuthPersistor { Ok(()) } - #[expect( - clippy::await_holding_invalid_type, - reason = "AuthorizationManager async access must be serialized through its mutex" - )] pub(crate) async fn refresh_for_startup_if_needed(&self) -> Result<()> { - let expires_at = { - let guard = self.inner.last_credentials.lock().await; - guard.as_ref().and_then(|tokens| tokens.expires_at) + let current = { + let state = self + .inner + .credential_state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + state.current.clone() + }; + let Some(current) = current else { + return Ok(()); }; - if !token_needs_refresh(expires_at) { + if !token_needs_refresh(current.expires_at) { return Ok(()); } - let manager = self.inner.authorization_manager.clone(); - let guard = manager.lock().await; - match guard.refresh_token().await { - Ok(_) => Ok(()), - Err(AuthError::TokenRefreshFailed(message)) if message.contains("invalid_grant") => { - Err(anyhow!(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR)) + let credentials = match self.refresh_authorization_manager().await { + Ok(credentials) => credentials, + Err(error) if refresh_requires_reauthentication(&error) => { + return Err(anyhow!(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR)); } - Err(AuthError::TokenRefreshFailed(message)) => Err(anyhow!( - "OAuth token endpoint refresh failed for server {}: {message}", - self.inner.server_name - )), - Err(error) => Err(error).with_context(|| { - format!( - "failed to refresh OAuth tokens for server {}", - self.inner.server_name - ) - }), + Err(AuthError::TokenRefreshFailed(message)) => { + return Err(anyhow!( + "OAuth token endpoint refresh failed for server {}: {}", + self.inner.server_name, + format_bounded_oauth_error_message(&message) + )); + } + Err(error) => { + return Err(error).with_context(|| { + format!( + "failed to refresh OAuth tokens for server {}", + self.inner.server_name + ) + }); + } + }; + + if let Err(error) = self.persist_credentials(current.client_id, credentials) { + warn!( + "failed to persist refreshed OAuth tokens before initialize: {}", + format_bounded_oauth_error(&error) + ); } + + Ok(()) } - #[expect( - clippy::await_holding_invalid_type, - reason = "AuthorizationManager async access must be serialized through its mutex" - )] pub(crate) async fn refresh_if_needed(&self) -> Result<()> { - let expires_at = { - let guard = self.inner.last_credentials.lock().await; - guard.as_ref().and_then(|tokens| tokens.expires_at) + let current = { + let state = self + .inner + .credential_state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + state.current.clone() + }; + let Some(current) = current else { + return Ok(()); }; - if !token_needs_refresh(expires_at) { + if !token_needs_refresh(current.expires_at) { return Ok(()); } - { - let manager = self.inner.authorization_manager.clone(); - let guard = manager.lock().await; - guard.refresh_token().await.with_context(|| { + let credentials = self + .refresh_authorization_manager() + .await + .with_context(|| { format!( "failed to refresh OAuth tokens for server {}", self.inner.server_name ) })?; + + if let Err(error) = self.persist_credentials(current.client_id, credentials) { + warn!( + "failed to persist refreshed OAuth tokens for server {}: {}", + self.inner.server_name, + format_bounded_oauth_error(&error) + ); } - self.persist_if_needed().await + Ok(()) + } + + #[expect( + clippy::await_holding_invalid_type, + reason = "AuthorizationManager async access must be serialized through its mutex" + )] + async fn refresh_authorization_manager( + &self, + ) -> std::result::Result { + let manager = self.inner.authorization_manager.clone(); + let guard = manager.lock().await; + guard.refresh_token().await + } + + fn persist_credentials( + &self, + client_id: String, + credentials: OAuthTokenResponse, + ) -> Result<()> { + let mut state = self + .inner + .credential_state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let new_token_response = WrappedOAuthTokenResponse(credentials.clone()); + let same_token = state + .current + .as_ref() + .map(|prev| prev.token_response == new_token_response) + .unwrap_or(false); + let expires_at = if same_token { + state.current.as_ref().and_then(|prev| prev.expires_at) + } else { + compute_expires_at_millis(&credentials) + }; + let stored = StoredOAuthTokens { + server_name: self.inner.server_name.clone(), + url: self.inner.url.clone(), + client_id, + token_response: new_token_response, + expires_at, + }; + state.current = Some(stored.clone()); + if state.persisted.as_ref() != Some(&stored) { + save_oauth_tokens(&self.inner.server_name, &stored, self.inner.store_mode)?; + state.persisted = Some(stored); + } + Ok(()) } } @@ -582,10 +647,11 @@ fn read_fallback_file() -> Result> { Ok(contents) => contents, Err(err) if err.kind() == ErrorKind::NotFound => return Ok(None), Err(err) => { - return Err(err).context(format!( - "failed to read credentials file at {}", + let message = format!( + "failed to read credentials file at {}: {err}", path.display() - )); + ); + return Err(Error::new(err).context(message)); } }; @@ -613,8 +679,13 @@ fn write_fallback_file(store: &FallbackFile) -> Result<()> { } let serialized = serde_json::to_string(store)?; - fs::write(&path, serialized) - .with_context(|| format!("failed to write credentials file at {}", path.display()))?; + if let Err(error) = fs::write(&path, serialized) { + let message = format!( + "failed to write credentials file at {}: {error}", + path.display() + ); + return Err(Error::new(error).context(message)); + } #[cfg(unix)] { @@ -637,6 +708,63 @@ fn sha_256_prefix(value: &Value) -> Result { Ok(truncated.to_string()) } +pub(crate) fn format_bounded_oauth_error(error: &Error) -> String { + let context = error.to_string(); + let root_cause = error.root_cause().to_string(); + if context == root_cause { + return format_bounded_oauth_error_message(&context); + } + + let combined = format!("{context}: {root_cause}"); + if combined.chars().count() <= MAX_OAUTH_ERROR_CAUSE_CHARS { + return combined; + } + + let root_budget = MAX_OAUTH_ERROR_CAUSE_CHARS / 2; + let context_budget = MAX_OAUTH_ERROR_CAUSE_CHARS + .saturating_sub(root_budget) + .saturating_sub(2); + format!( + "{}: {}", + format_bounded_oauth_error_message_with_limit(&context, context_budget), + format_bounded_oauth_error_message_with_limit(&root_cause, root_budget) + ) +} + +fn format_bounded_oauth_error_message(message: &str) -> String { + format_bounded_oauth_error_message_with_limit(message, MAX_OAUTH_ERROR_CAUSE_CHARS) +} + +fn format_bounded_oauth_error_message_with_limit(message: &str, limit: usize) -> String { + if message.chars().count() <= limit { + return message.to_string(); + } + + let prefix = message + .chars() + .take(limit.saturating_sub(3)) + .collect::(); + format!("{prefix}...") +} + +fn refresh_requires_reauthentication(error: &AuthError) -> bool { + match error { + AuthError::AuthorizationRequired => true, + AuthError::TokenRefreshFailed(message) => { + if message == MISSING_REFRESH_TOKEN_ERROR { + return true; + } + message + .strip_prefix(OAUTH_SERVER_ERROR_PREFIX) + .is_some_and(|response| { + let error_code_end = response.find([':', ' ']).unwrap_or(response.len()); + &response[..error_code_end] == "invalid_grant" + }) + } + _ => false, + } +} + #[cfg(test)] mod tests { use super::*; @@ -833,6 +961,106 @@ mod tests { Ok(()) } + #[test] + fn save_oauth_tokens_to_file_preserves_write_cause() -> Result<()> { + let _env = TempCodexHome::new(); + let tokens = sample_tokens(); + super::save_oauth_tokens_to_file(&tokens)?; + let path = super::fallback_file_path()?; + let original_permissions = fs::metadata(&path)?.permissions(); + let mut readonly_permissions = original_permissions.clone(); + readonly_permissions.set_readonly(true); + fs::set_permissions(&path, readonly_permissions)?; + + let write_cause = fs::write(&path, "blocked") + .expect_err("read-only credentials file should reject writes") + .to_string(); + let mut updated_tokens = tokens; + updated_tokens.client_id = "updated-client-id".to_string(); + let error = super::save_oauth_tokens_to_file(&updated_tokens) + .expect_err("production credential write should fail"); + + fs::set_permissions(&path, original_permissions)?; + assert!( + error.to_string().contains(&write_cause), + "credential write cause was not preserved: {error:#}" + ); + Ok(()) + } + + #[test] + fn startup_refresh_reauthentication_classification_is_bounded_to_error_code() { + let cases = [ + ( + AuthError::AuthorizationRequired, + true, + "authorization required", + ), + ( + AuthError::TokenRefreshFailed(MISSING_REFRESH_TOKEN_ERROR.to_string()), + true, + "missing refresh token", + ), + ( + AuthError::TokenRefreshFailed( + "Server returned error response: invalid_grant: revoked".to_string(), + ), + true, + "invalid_grant error code", + ), + ( + AuthError::TokenRefreshFailed( + "Server returned error response: invalid_grant (see https://example.test/error)" + .to_string(), + ), + true, + "invalid_grant error code with URI", + ), + ( + AuthError::TokenRefreshFailed( + "Server returned error response: invalid_request: invalid_grant is only description text" + .to_string(), + ), + false, + "invalid_grant description text", + ), + ( + AuthError::TokenRefreshFailed( + "network response mentioned invalid_grant".to_string(), + ), + false, + "unstructured invalid_grant text", + ), + ]; + + assert_eq!( + cases + .iter() + .map(|(error, _, label)| { + (*label, super::refresh_requires_reauthentication(error)) + }) + .collect::>(), + cases + .iter() + .map(|(_, expected, label)| (*label, *expected)) + .collect::>() + ); + } + + #[test] + fn oauth_error_cause_formatting_is_bounded() { + let cause = "x".repeat(MAX_OAUTH_ERROR_CAUSE_CHARS + 100); + let formatted = super::format_bounded_oauth_error_message(&cause); + + assert_eq!(formatted.chars().count(), MAX_OAUTH_ERROR_CAUSE_CHARS); + assert!(formatted.ends_with("...")); + + let error = anyhow!("actionable root cause").context("x".repeat(600)); + let formatted_chain = super::format_bounded_oauth_error(&error); + assert!(formatted_chain.chars().count() <= MAX_OAUTH_ERROR_CAUSE_CHARS); + assert!(formatted_chain.contains("actionable root cause")); + } + #[test] fn delete_oauth_tokens_removes_all_storage() -> Result<()> { let _env = TempCodexHome::new(); diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 25caba19bd98..25223931f398 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -9,7 +9,6 @@ use std::sync::atomic::Ordering; use std::time::Duration; use std::time::Instant; -use anyhow::Context; use anyhow::Result; use anyhow::anyhow; use codex_api::SharedAuthProvider; @@ -67,6 +66,7 @@ use crate::in_process_transport::InProcessTransportFactory; use crate::load_oauth_tokens; use crate::oauth::OAuthPersistor; use crate::oauth::StoredOAuthTokens; +use crate::oauth::format_bounded_oauth_error; use crate::stdio_server_launcher::StdioServerCommand; use crate::stdio_server_launcher::StdioServerLauncher; use crate::stdio_server_launcher::StdioServerProcessHandle; @@ -738,8 +738,11 @@ impl RmcpClient { && auth_provider.is_none() && !default_headers.contains_key(AUTHORIZATION) { - load_oauth_tokens(server_name, url, *store_mode).with_context(|| { - format!("failed to read OAuth credentials for MCP server `{server_name}`") + load_oauth_tokens(server_name, url, *store_mode).map_err(|error| { + anyhow!( + "failed to read OAuth credentials for MCP server `{server_name}`: {}", + format_bounded_oauth_error(&error) + ) })? } else { None @@ -842,17 +845,23 @@ impl RmcpClient { ), }; - let service = match timeout { - Some(duration) => time::timeout(duration, transport) - .await - .map_err(|_| anyhow!("timed out handshaking with MCP server after {duration:?}"))? - .map_err(|err| anyhow!("handshaking with MCP server failed: {err}"))?, - None => transport + let connect = async { + if let Some(runtime) = oauth_persistor.as_ref() { + runtime.refresh_for_startup_if_needed().await?; + } + + let service = transport .await - .map_err(|err| anyhow!("handshaking with MCP server failed: {err}"))?, + .map_err(|err| anyhow!("handshaking with MCP server failed: {err}"))?; + Ok((Arc::new(service), oauth_persistor)) }; - Ok((Arc::new(service), oauth_persistor)) + match timeout { + Some(duration) => time::timeout(duration, connect) + .await + .map_err(|_| anyhow!("timed out handshaking with MCP server after {duration:?}"))?, + None => connect.await, + } } async fn run_service_operation( @@ -1055,8 +1064,6 @@ async fn create_oauth_transport_and_runtime( Some(initial_tokens), ); - runtime.refresh_for_startup_if_needed().await?; - Ok((transport, runtime)) } diff --git a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs index dced61f84a77..5c23e037d68c 100644 --- a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs +++ b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs @@ -30,104 +30,142 @@ use wiremock::matchers::method; use wiremock::matchers::path; use streamable_http_test_support::initialize_client; +use streamable_http_test_support::initialize_client_with_timeout; const SERVER_NAME: &str = "test-streamable-http-oauth-startup"; const EXPIRED_ACCESS_TOKEN: &str = "expired-access-token"; const REFRESH_TOKEN: &str = "valid-refresh-token"; const REFRESHED_ACCESS_TOKEN: &str = "refreshed-access-token"; +const REFRESHED_REFRESH_TOKEN: &str = "rotated-refresh-token"; const CHILD_SERVER_URL_ENV: &str = "MCP_TEST_OAUTH_STARTUP_SERVER_URL"; const CHILD_SCENARIO_ENV: &str = "MCP_TEST_OAUTH_STARTUP_SCENARIO"; const CREDENTIALS_FILENAME: &str = ".credentials.json"; const SCENARIO_SUCCESS: &str = "success"; const SCENARIO_INVALID_GRANT: &str = "invalid_grant"; +const SCENARIO_HANDSHAKE_TIMEOUT_AFTER_REFRESH: &str = "handshake_timeout_after_refresh"; +const SCENARIO_REFRESH_TIMEOUT: &str = "refresh_timeout"; const SCENARIO_TRANSIENT_FAILURE: &str = "transient_failure"; const SCENARIO_LOAD_FAILURE: &str = "load_failure"; const SCENARIO_PERSIST_FAILURE: &str = "persist_failure"; -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn refreshes_expired_persisted_token_before_initialize() -> anyhow::Result<()> { - let server = MockServer::start().await; - mount_oauth_metadata(&server).await; - mount_successful_refresh(&server, /*expected_calls*/ 1).await; - mount_successful_mcp_requests(&server, /*expected_calls*/ 2).await; - - run_oauth_startup_child(&server, SCENARIO_SUCCESS).await?; - server.verify().await; - Ok(()) -} - -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn invalid_grant_stops_before_initialize() -> anyhow::Result<()> { - let server = MockServer::start().await; - mount_oauth_metadata(&server).await; - Mock::given(method("POST")) - .and(path("/oauth/token")) - .and(body_string_contains("grant_type=refresh_token")) - .respond_with(ResponseTemplate::new(400).set_body_json(json!({ - "error": "invalid_grant", - "error_description": "refresh token revoked", - }))) - .expect(1) - .mount(&server) - .await; - Mock::given(method("POST")) - .and(path("/mcp")) - .respond_with(ResponseTemplate::new(500)) - .expect(0) - .mount(&server) - .await; - - run_oauth_startup_child(&server, SCENARIO_INVALID_GRANT).await?; - server.verify().await; - Ok(()) +#[derive(Clone, Copy)] +enum PersistedCredentialsAtInitialize { + Refreshed, + Stale, } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn transient_refresh_failure_stops_before_initialize() -> anyhow::Result<()> { - let server = MockServer::start().await; - mount_oauth_metadata(&server).await; - Mock::given(method("POST")) - .and(path("/oauth/token")) - .and(body_string_contains("grant_type=refresh_token")) - .respond_with(ResponseTemplate::new(503).set_body_string("temporarily unavailable")) - .expect(1) - .mount(&server) - .await; - Mock::given(method("POST")) - .and(path("/mcp")) - .respond_with(ResponseTemplate::new(500)) - .expect(0) - .mount(&server) - .await; - - run_oauth_startup_child(&server, SCENARIO_TRANSIENT_FAILURE).await?; - server.verify().await; +async fn oauth_startup_refresh_and_storage_scenarios() -> anyhow::Result<()> { + for scenario in [ + SCENARIO_SUCCESS, + SCENARIO_INVALID_GRANT, + SCENARIO_TRANSIENT_FAILURE, + SCENARIO_REFRESH_TIMEOUT, + SCENARIO_HANDSHAKE_TIMEOUT_AFTER_REFRESH, + SCENARIO_LOAD_FAILURE, + SCENARIO_PERSIST_FAILURE, + ] { + run_parent_scenario(scenario).await?; + } Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn credential_load_failure_stops_before_initialize() -> anyhow::Result<()> { +async fn run_parent_scenario(scenario: &str) -> anyhow::Result<()> { let server = MockServer::start().await; - Mock::given(method("POST")) - .and(path("/mcp")) - .respond_with(ResponseTemplate::new(500)) - .expect(0) - .mount(&server) - .await; + let codex_home = TempDir::new()?; + if scenario != SCENARIO_LOAD_FAILURE { + mount_oauth_metadata(&server).await; + } - run_oauth_startup_child(&server, SCENARIO_LOAD_FAILURE).await?; - server.verify().await; - Ok(()) -} + match scenario { + SCENARIO_SUCCESS | SCENARIO_PERSIST_FAILURE | SCENARIO_HANDSHAKE_TIMEOUT_AFTER_REFRESH => { + mount_successful_refresh(&server, /*expected_calls*/ 1).await; + } + SCENARIO_INVALID_GRANT => { + mount_token_response( + &server, + ResponseTemplate::new(400).set_body_json(json!({ + "error": "invalid_grant", + "error_description": "refresh token revoked", + })), + /*expected_calls*/ 1, + ) + .await; + } + SCENARIO_TRANSIENT_FAILURE => { + mount_token_response( + &server, + ResponseTemplate::new(503).set_body_json(json!({ + "error": "temporarily_unavailable", + "error_description": "HTTP 503 Service Unavailable from token endpoint", + })), + /*expected_calls*/ 1, + ) + .await; + } + SCENARIO_REFRESH_TIMEOUT => { + mount_token_response( + &server, + successful_refresh_response().set_delay(Duration::from_millis(250)), + /*expected_calls*/ 1, + ) + .await; + } + SCENARIO_LOAD_FAILURE => {} + other => anyhow::bail!("unknown OAuth startup scenario: {other}"), + } -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn credential_persist_failure_is_swallowed_after_initialize() -> anyhow::Result<()> { - let server = MockServer::start().await; - mount_oauth_metadata(&server).await; - mount_successful_refresh(&server, /*expected_calls*/ 1).await; - mount_successful_mcp_requests(&server, /*expected_calls*/ 3).await; + let credentials_path = codex_home.path().join(CREDENTIALS_FILENAME); + match scenario { + SCENARIO_SUCCESS => { + mount_successful_mcp_requests( + &server, + /*expected_calls*/ 2, + credentials_path.clone(), + PersistedCredentialsAtInitialize::Refreshed, + ) + .await; + } + SCENARIO_PERSIST_FAILURE => { + mount_successful_mcp_requests( + &server, + /*expected_calls*/ 3, + credentials_path.clone(), + PersistedCredentialsAtInitialize::Stale, + ) + .await; + } + SCENARIO_HANDSHAKE_TIMEOUT_AFTER_REFRESH => { + Mock::given(method("POST")) + .and(path("/mcp")) + .and(header( + "authorization", + format!("Bearer {REFRESHED_ACCESS_TOKEN}"), + )) + .respond_with(ResponseTemplate::new(500).set_delay(Duration::from_millis(250))) + .expect(1) + .mount(&server) + .await; + } + _ => { + Mock::given(method("POST")) + .and(path("/mcp")) + .respond_with(ResponseTemplate::new(500)) + .expect(0) + .mount(&server) + .await; + } + } - run_oauth_startup_child(&server, SCENARIO_PERSIST_FAILURE).await?; + run_oauth_startup_child(&server, scenario, &codex_home).await?; + if scenario == SCENARIO_HANDSHAKE_TIMEOUT_AFTER_REFRESH { + let persisted = fs::read_to_string(credentials_path)?; + assert!( + persisted.contains(REFRESHED_ACCESS_TOKEN) + && persisted.contains(REFRESHED_REFRESH_TOKEN), + "rotated credentials were lost when the handshake timed out: {persisted}" + ); + } server.verify().await; Ok(()) } @@ -139,11 +177,17 @@ async fn oauth_startup_child() -> anyhow::Result<()> { let scenario = std::env::var(CHILD_SCENARIO_ENV)?; let credentials_path = credentials_path()?; - if scenario == SCENARIO_LOAD_FAILURE { + let expected_read_cause = if scenario == SCENARIO_LOAD_FAILURE { fs::create_dir(&credentials_path)?; + Some( + fs::read_to_string(&credentials_path) + .expect_err("credentials directory should reject file reads") + .to_string(), + ) } else { save_expired_tokens(&server_url)?; - } + None + }; // This mirrors create_client's transport and initialization setup, except // it omits the direct bearer token. Supplying that token would bypass the @@ -160,51 +204,25 @@ async fn oauth_startup_child() -> anyhow::Result<()> { ) .await; - if matches!( - scenario.as_str(), - SCENARIO_INVALID_GRANT | SCENARIO_TRANSIENT_FAILURE | SCENARIO_LOAD_FAILURE - ) { + if scenario == SCENARIO_LOAD_FAILURE { let error = match client { - Ok(_) => anyhow::bail!("OAuth startup should fail for scenario {scenario}"), + Ok(_) => anyhow::bail!("credential read failure should stop client construction"), Err(error) => format!("{error:#}"), }; - match scenario.as_str() { - SCENARIO_INVALID_GRANT => { - assert!( - error.contains(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR), - "unexpected invalid_grant failure: {error}" - ); - assert!( - !error.contains("invalid_grant") && !error.contains("refresh token revoked"), - "revoked-token provider details should be hidden: {error}" - ); - } - SCENARIO_TRANSIENT_FAILURE => { - assert!( - error.contains("OAuth token endpoint refresh failed"), - "unexpected transient refresh failure: {error}" - ); - assert!( - !error.contains(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR), - "transient refresh failure should not request reauthentication: {error}" - ); - } - SCENARIO_LOAD_FAILURE => { - assert!( - error.contains("failed to read OAuth credentials for MCP server"), - "unexpected credential load failure: {error}" - ); - assert!( - error.contains("failed to read credentials file"), - "credential storage cause should be preserved: {error}" - ); - assert!( - !error.contains("Auth required"), - "credential storage failure should not be classified as unauthenticated: {error}" - ); - } - _ => unreachable!(), - } + assert!( + error.contains("failed to read OAuth credentials for MCP server"), + "unexpected credential load failure: {error}" + ); + let expected_read_cause = + expected_read_cause.expect("load failure should capture the operating-system cause"); + assert!( + error.contains(&expected_read_cause), + "credential storage cause should be preserved: {error}" + ); + assert!( + !error.contains("Auth required"), + "credential storage failure should not be classified as unauthenticated: {error}" + ); return Ok(()); } let client = client?; @@ -220,42 +238,98 @@ async fn oauth_startup_child() -> anyhow::Result<()> { assert_eq!(write_error.kind(), std::io::ErrorKind::PermissionDenied); } - let result = initialize_client(&client).await; + let result = if matches!( + scenario.as_str(), + SCENARIO_REFRESH_TIMEOUT | SCENARIO_HANDSHAKE_TIMEOUT_AFTER_REFRESH + ) { + initialize_client_with_timeout(&client, Duration::from_millis(50)).await + } else { + initialize_client(&client).await + }; match scenario.as_str() { SCENARIO_SUCCESS => result?, + SCENARIO_INVALID_GRANT => { + let error = format!( + "{:#}", + result.expect_err("permanent refresh failure should stop startup") + ); + assert!( + error.contains(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR), + "unexpected reauthentication failure: {error}" + ); + assert!( + !error.contains("invalid_grant") && !error.contains("refresh token revoked"), + "revoked-token provider details should be hidden: {error}" + ); + } + SCENARIO_TRANSIENT_FAILURE => { + let error = format!( + "{:#}", + result.expect_err("transient refresh failure should stop startup") + ); + assert!( + error.contains("OAuth token endpoint refresh failed"), + "unexpected transient refresh failure: {error}" + ); + assert!( + error.contains("temporarily_unavailable") + && error.contains("HTTP 503 Service Unavailable from token endpoint"), + "transient token endpoint status and cause should be preserved: {error}" + ); + assert!( + !error.contains(OAUTH_REFRESH_REAUTHENTICATION_REQUIRED_ERROR), + "transient refresh failure should not request reauthentication: {error}" + ); + } + SCENARIO_REFRESH_TIMEOUT | SCENARIO_HANDSHAKE_TIMEOUT_AFTER_REFRESH => { + let error = format!( + "{:#}", + result.expect_err("slow startup refresh should consume the initialize timeout") + ); + assert!( + error.contains("timed out handshaking with MCP server after 50ms"), + "startup refresh escaped the initialize timeout budget: {error}" + ); + } SCENARIO_PERSIST_FAILURE => { result?; + let stale_credentials = fs::read_to_string(&credentials_path)?; + assert!( + stale_credentials.contains(EXPIRED_ACCESS_TOKEN), + "failed pre-handshake and post-handshake writes should remain stale" + ); + fs::set_permissions(&credentials_path, original_permissions)?; client .list_tools( /*params*/ None, /*timeout*/ Some(Duration::from_secs(5)), ) .await?; - fs::set_permissions(&credentials_path, original_permissions)?; assert!( credentials_path.is_file(), "failed persistence should leave the credentials file in place" ); - let stale_credentials = fs::read_to_string(&credentials_path)?; + let refreshed_credentials = fs::read_to_string(&credentials_path)?; assert!( - stale_credentials.contains(EXPIRED_ACCESS_TOKEN), - "the only persisted credentials should remain stale" + refreshed_credentials.contains(REFRESHED_ACCESS_TOKEN), + "dirty refreshed credentials should persist after writes recover" ); assert!( - !stale_credentials.contains(REFRESHED_ACCESS_TOKEN), - "refreshed credentials should not have been persisted" + refreshed_credentials.contains(REFRESHED_REFRESH_TOKEN), + "the rotated refresh token should persist on retry" ); } - SCENARIO_INVALID_GRANT | SCENARIO_TRANSIENT_FAILURE | SCENARIO_LOAD_FAILURE => { - unreachable!() - } + SCENARIO_LOAD_FAILURE => unreachable!(), other => anyhow::bail!("unknown OAuth startup scenario: {other}"), } Ok(()) } -async fn run_oauth_startup_child(server: &MockServer, scenario: &str) -> anyhow::Result<()> { - let codex_home = TempDir::new()?; +async fn run_oauth_startup_child( + server: &MockServer, + scenario: &str, + codex_home: &TempDir, +) -> anyhow::Result<()> { let server_url = format!("{}/mcp", server.uri()); // Credential storage resolves CODEX_HOME from the process environment. @@ -286,48 +360,89 @@ async fn mount_oauth_metadata(server: &MockServer) { } async fn mount_successful_refresh(server: &MockServer, expected_calls: u64) { + mount_token_response(server, successful_refresh_response(), expected_calls).await; +} + +fn successful_refresh_response() -> ResponseTemplate { + ResponseTemplate::new(200).set_body_json(json!({ + "access_token": REFRESHED_ACCESS_TOKEN, + "token_type": "Bearer", + "expires_in": 7200, + "refresh_token": REFRESHED_REFRESH_TOKEN, + })) +} + +async fn mount_token_response( + server: &MockServer, + response: ResponseTemplate, + expected_calls: u64, +) { Mock::given(method("POST")) .and(path("/oauth/token")) .and(body_string_contains("grant_type=refresh_token")) .and(body_string_contains(format!( "refresh_token={REFRESH_TOKEN}" ))) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "access_token": REFRESHED_ACCESS_TOKEN, - "token_type": "Bearer", - "expires_in": 7200, - "refresh_token": REFRESH_TOKEN, - }))) + .respond_with(response) .expect(expected_calls) .mount(server) .await; } -async fn mount_successful_mcp_requests(server: &MockServer, expected_calls: u64) { +async fn mount_successful_mcp_requests( + server: &MockServer, + expected_calls: u64, + credentials_path: PathBuf, + persistence_expectation: PersistedCredentialsAtInitialize, +) { Mock::given(method("POST")) .and(path("/mcp")) .and(header( "authorization", format!("Bearer {REFRESHED_ACCESS_TOKEN}"), )) - .respond_with(|request: &Request| { - let body: Value = request.body_json().expect("valid JSON-RPC request"); + .respond_with(move |request: &Request| { + let Ok(body): Result = request.body_json() else { + return ResponseTemplate::new(400).set_body_string("invalid JSON-RPC request"); + }; match body.get("method").and_then(Value::as_str) { - Some("initialize") => ResponseTemplate::new(200).set_body_json(json!({ - "jsonrpc": "2.0", - "id": body.get("id").cloned().unwrap_or(Value::Null), - "result": { - "protocolVersion": body - .pointer("/params/protocolVersion") - .cloned() - .unwrap_or_else(|| json!("2025-06-18")), - "capabilities": {}, - "serverInfo": { - "name": "oauth-startup-test", - "version": "0.0.0-test", + Some("initialize") => { + let Ok(persisted) = fs::read_to_string(&credentials_path) else { + return ResponseTemplate::new(500) + .set_body_string("persisted credentials are unreadable"); + }; + match persistence_expectation { + PersistedCredentialsAtInitialize::Refreshed => { + assert!( + persisted.contains(REFRESHED_ACCESS_TOKEN) + && persisted.contains(REFRESHED_REFRESH_TOKEN), + "rotated credentials were not persisted before initialize: {persisted}" + ); + } + PersistedCredentialsAtInitialize::Stale => { + assert!( + persisted.contains(EXPIRED_ACCESS_TOKEN) + && !persisted.contains(REFRESHED_ACCESS_TOKEN), + "failed credential write should leave stale storage before initialize: {persisted}" + ); + } + } + ResponseTemplate::new(200).set_body_json(json!({ + "jsonrpc": "2.0", + "id": body.get("id").cloned().unwrap_or(Value::Null), + "result": { + "protocolVersion": body + .pointer("/params/protocolVersion") + .cloned() + .unwrap_or_else(|| json!("2025-06-18")), + "capabilities": {}, + "serverInfo": { + "name": "oauth-startup-test", + "version": "0.0.0-test", + }, }, - }, - })), + })) + } Some("notifications/initialized") => ResponseTemplate::new(202), Some("tools/list") => ResponseTemplate::new(200).set_body_json(json!({ "jsonrpc": "2.0", diff --git a/codex-rs/rmcp-client/tests/streamable_http_test_support.rs b/codex-rs/rmcp-client/tests/streamable_http_test_support.rs index 03ee79392cd9..f46c4198564d 100644 --- a/codex-rs/rmcp-client/tests/streamable_http_test_support.rs +++ b/codex-rs/rmcp-client/tests/streamable_http_test_support.rs @@ -92,10 +92,17 @@ pub(crate) async fn create_client(base_url: &str) -> anyhow::Result } pub(crate) async fn initialize_client(client: &RmcpClient) -> anyhow::Result<()> { + initialize_client_with_timeout(client, Duration::from_secs(5)).await +} + +pub(crate) async fn initialize_client_with_timeout( + client: &RmcpClient, + timeout: Duration, +) -> anyhow::Result<()> { client .initialize( init_params(), - Some(Duration::from_secs(5)), + Some(timeout), Box::new(|_, _| { async { Ok(ElicitationResponse {