From a44afa527cf11114aed19ec893491bfc8d3931bb Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Tue, 29 Sep 2026 22:06:54 +0000 Subject: [PATCH] Track credential storage outcomes and redact sensitive errors (#49384) ## Why Successful file fallback can hide a secure-store failure. Storage diagnostics need to distinguish that fallback from explicit file storage while keeping credential values out of error output. ## What changed - Record credential load, save, delete, and fallback-file cleanup outcomes, including secure-store failures and fallback reasons. - Preserve the initiating client's attribution across app-server requests, login tasks, sessions, turns, and blocking token persistence. - Preserve storage error context and typed causes while redacting credential JSON data errors and payload-bearing keyring errors. - Include sanitized fallback completion warnings in the default `codex login` file logs. ## Testing Add coverage for storage policy and metric attribution, credential redaction, login diagnostics, concurrent app-server clients, device-code login, and token refresh during a turn. GitOrigin-RevId: 6d5651c1718d184016fa77befa02aa528750b2ae --- codex-rs/Cargo.lock | 1 + codex-rs/app-server/src/message_processor.rs | 23 +- .../request_processors/account_processor.rs | 11 +- codex-rs/app-server/tests/suite/v2/account.rs | 11 +- .../suite/v2/auth_storage_originator_tests.rs | 252 ++++++++++ .../suite/v2/connection_handling_websocket.rs | 3 +- codex-rs/app-server/tests/suite/v2/mod.rs | 2 + codex-rs/cli/src/login.rs | 8 +- codex-rs/cli/src/login_storage_tests.rs | 87 ++++ ...er_records_sanitized_storage_fallback.snap | 7 + ...rage_error_displays_context_and_cause.snap | 6 + codex-rs/cli/tests/daemon_telemetry.rs | 10 +- codex-rs/core/src/session/mod.rs | 6 +- codex-rs/core/src/tasks/mod.rs | 79 ++-- .../keyring-store/src/error_display_tests.rs | 51 ++ codex-rs/keyring-store/src/lib.rs | 20 +- codex-rs/login/Cargo.toml | 3 +- codex-rs/login/src/auth/storage.rs | 208 +++++--- codex-rs/login/src/auth/storage_error.rs | 49 ++ .../login/src/auth/storage_error_tests.rs | 146 ++++++ .../login/src/auth/storage_policy_tests.rs | 443 ++++++++++++++++++ codex-rs/login/src/auth/storage_telemetry.rs | 108 +++++ codex-rs/login/src/auth/storage_tests.rs | 206 +------- codex-rs/login/src/server.rs | 78 +-- 24 files changed, 1465 insertions(+), 353 deletions(-) create mode 100644 codex-rs/app-server/tests/suite/v2/auth_storage_originator_tests.rs create mode 100644 codex-rs/cli/src/login_storage_tests.rs create mode 100644 codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_default_filter_records_sanitized_storage_fallback.snap create mode 100644 codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_storage_error_displays_context_and_cause.snap create mode 100644 codex-rs/keyring-store/src/error_display_tests.rs create mode 100644 codex-rs/login/src/auth/storage_error.rs create mode 100644 codex-rs/login/src/auth/storage_error_tests.rs create mode 100644 codex-rs/login/src/auth/storage_policy_tests.rs create mode 100644 codex-rs/login/src/auth/storage_telemetry.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index c9162751bb..c81c7467c7 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3814,6 +3814,7 @@ dependencies = [ "jsonwebtoken", "keyring", "once_cell", + "opentelemetry_sdk", "os_info", "pretty_assertions", "rand 0.9.3", diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 13a28a1053..3e8f1fbdd0 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -83,6 +83,7 @@ use codex_feedback::CodexFeedback; use codex_goal_extension::GoalService; use codex_home::CodexHomeUserInstructionsProvider; use codex_login::AuthManager; +use codex_otel::auth_storage::AuthStorageOriginator; use codex_protocol::ThreadId; use codex_protocol::mcp::ClientMcpExtensions; use codex_protocol::protocol::SessionSource; @@ -1031,15 +1032,21 @@ impl MessageProcessor { return; } let processor_for_request = Arc::clone(&processor); + let originator = AuthStorageOriginator::from_client_name( + session.app_server_client_name().unwrap_or("none"), + ); // Keep queued requests small to avoid large stack temporaries during construction. - let result = Box::pin(processor_for_request.handle_initialized_client_request( - connection_request_id, - codex_request, - request_context, - session, - event_stream_ready, - )) - .await; + let result = originator + .scope(Box::pin( + processor_for_request.handle_initialized_client_request( + connection_request_id, + codex_request, + request_context, + session, + event_stream_ready, + ), + )) + .await; if let Err(error) = result { processor.outgoing.send_error(error_request_id, error).await; } diff --git a/codex-rs/app-server/src/request_processors/account_processor.rs b/codex-rs/app-server/src/request_processors/account_processor.rs index e1a0cd33d2..401b4cc584 100644 --- a/codex-rs/app-server/src/request_processors/account_processor.rs +++ b/codex-rs/app-server/src/request_processors/account_processor.rs @@ -13,6 +13,7 @@ use codex_login::LoginOnboardingEntrypoint; use codex_login::login_with_bedrock_access_keys; use codex_mcp::ema_auth_scope; use codex_model_provider::is_supported_amazon_bedrock_region; +use codex_otel::auth_storage::AuthStorageOriginator; use codex_rmcp_client::EnterpriseOAuthCredentialGuard; mod bedrock_setup; @@ -678,7 +679,7 @@ impl AccountRequestProcessor { let processor = self.clone(); let active_login = self.active_login.clone(); let auth_url = server.auth_url.clone(); - tokio::spawn(async move { + let task = async move { let (success, error_msg, onboarding_entrypoint) = match tokio::time::timeout( LOGIN_CHATGPT_TIMEOUT, server.block_until_done_with_callback_result(), @@ -715,7 +716,8 @@ impl AccountRequestProcessor { if guard.as_ref().map(ActiveLogin::login_id) == Some(login_id) { *guard = None; } - }); + }; + tokio::spawn(AuthStorageOriginator::current().scope(task)); Ok(LoginAccountResponse::Chatgpt { login_id: login_id.to_string(), @@ -759,7 +761,7 @@ impl AccountRequestProcessor { let processor = self.clone(); let active_login = self.active_login.clone(); - tokio::spawn(async move { + let task = async move { let (success, error_msg) = tokio::select! { _ = cancel.cancelled() => { (false, Some("Login was not completed".to_string())) @@ -785,7 +787,8 @@ impl AccountRequestProcessor { if guard.as_ref().map(ActiveLogin::login_id) == Some(login_id) { *guard = None; } - }); + }; + tokio::spawn(AuthStorageOriginator::current().scope(task)); Ok(LoginAccountResponse::ChatgptDeviceCode { login_id: login_id.to_string(), diff --git a/codex-rs/app-server/tests/suite/v2/account.rs b/codex-rs/app-server/tests/suite/v2/account.rs index 239ed7097f..e2a1c1a1b2 100644 --- a/codex-rs/app-server/tests/suite/v2/account.rs +++ b/codex-rs/app-server/tests/suite/v2/account.rs @@ -2166,6 +2166,7 @@ async fn login_account_chatgpt_device_code_succeeds_and_notifies() -> Result<()> }, )?; write_models_cache(codex_home.path()).await?; + let collector = super::auth_storage_originator::configure_collector(codex_home.path()).await?; mock_device_code_usercode(&mock_server, /*interval_seconds*/ 0).await; mock_device_code_token_success(&mock_server).await; @@ -2184,10 +2185,17 @@ async fn login_account_chatgpt_device_code_succeeds_and_notifies() -> Result<()> .without_auto_env() .with_env_overrides(&[ ("OPENAI_API_KEY", None), + ("OTEL_METRIC_EXPORT_INTERVAL", Some("200")), (LOGIN_ISSUER_ENV_VAR, Some(issuer.as_str())), ]) - .build_initialized_with_timeout(DEFAULT_READ_TIMEOUT) + .build() .await?; + mcp.initialize_with_client_info(ClientInfo { + name: "codex_vscode".into(), + title: None, + version: "1".into(), + }) + .await?; let request_id = mcp.send_login_account_chatgpt_device_code_request().await?; let login: LoginAccountResponse = @@ -2231,6 +2239,7 @@ async fn login_account_chatgpt_device_code_succeeds_and_notifies() -> Result<()> codex_home.path().join("auth.json").exists(), "auth.json should be created when device code login succeeds" ); + super::auth_storage_originator::assert_saved_originators(&collector, &["codex_vscode"]).await?; Ok(()) } diff --git a/codex-rs/app-server/tests/suite/v2/auth_storage_originator_tests.rs b/codex-rs/app-server/tests/suite/v2/auth_storage_originator_tests.rs new file mode 100644 index 0000000000..86817d5fe5 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/auth_storage_originator_tests.rs @@ -0,0 +1,252 @@ +//! Checks client attribution on exported storage metrics through the public RPC API. + +use super::connection_handling_websocket::connect_websocket; +use super::connection_handling_websocket::read_response_for_id; +use super::connection_handling_websocket::send_initialize_request; +use super::connection_handling_websocket::send_request; +use super::connection_handling_websocket::spawn_websocket_server; +use anyhow::Context; +use anyhow::Result; +use app_test_support::ChatGptAuthFixture; +use app_test_support::MockResponsesConfig; +use app_test_support::TestAppServer; +use app_test_support::write_chatgpt_auth; +use app_test_support::write_models_cache; +use codex_app_server_protocol::ClientInfo; +use codex_app_server_protocol::ThreadStartParams; +use codex_app_server_protocol::ThreadStartResponse; +use codex_app_server_protocol::TurnCompletedNotification; +use codex_app_server_protocol::TurnStartParams; +use codex_app_server_protocol::TurnStartResponse; +use codex_app_server_protocol::TurnStatus; +use codex_app_server_protocol::UserInput; +use codex_login::AuthCredentialsStoreMode; +use codex_login::REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR; +use core_test_support::responses; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; +use std::collections::BTreeMap; +use std::io::Write; +use std::path::Path; +use std::time::Duration; +use tempfile::TempDir; +use wiremock::Mock; +use wiremock::MockServer; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; +use wiremock::matchers::path; + +pub(super) async fn configure_collector(codex_home: &Path) -> Result { + let collector = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/metrics")) + .respond_with(ResponseTemplate::new(/*s*/ 200)) + .mount(&collector) + .await; + let endpoint = format!("{}/v1/metrics", collector.uri()); + let mut config = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(codex_home.join("config.toml"))?; + writeln!( + config, + "\n[analytics]\nenabled = true\n[otel.metrics_exporter.otlp-http]\nendpoint = {endpoint:?}\nprotocol = \"json\"" + )?; + Ok(collector) +} + +pub(super) async fn assert_saved_originators( + collector: &MockServer, + expected: &[&str], +) -> Result<()> { + let expected: BTreeMap<_, _> = expected + .iter() + .map(|value| (value.to_string(), 1_u64)) + .collect(); + let actual = tokio::time::timeout(Duration::from_secs(/*secs*/ 60), async { + loop { + let mut actual = BTreeMap::::new(); + for request in collector.received_requests().await.unwrap_or_default() { + let body: Value = serde_json::from_slice(&request.body)?; + for resource in body["resourceMetrics"].as_array().into_iter().flatten() { + for scope in resource["scopeMetrics"].as_array().into_iter().flatten() { + for metric in scope["metrics"].as_array().into_iter().flatten() { + if metric["name"] != "codex.auth_storage.operation" { + continue; + } + for point in + metric["sum"]["dataPoints"].as_array().into_iter().flatten() + { + let tags: BTreeMap<_, _> = point["attributes"] + .as_array() + .into_iter() + .flatten() + .filter_map(|tag| { + Some(( + tag["key"].as_str()?, + tag["value"]["stringValue"].as_str()?, + )) + }) + .collect(); + if tags.get("credential_kind") == Some(&"codex") + && tags.get("operation") == Some(&"save") + { + let count = point["asInt"] + .as_u64() + .or_else(|| point["asInt"].as_str()?.parse().ok()) + .context("storage count must be an integer")?; + *actual + .entry( + tags.get("originator") + .unwrap_or(&"missing") + .to_string(), + ) + .or_default() += count; + } + } + } + } + } + } + if actual.values().sum::() >= expected.len() as u64 { + break Ok::<_, anyhow::Error>(actual); + } + tokio::time::sleep(Duration::from_millis(/*millis*/ 25)).await; + } + }) + .await??; + assert_eq!(actual, expected); + Ok(()) +} + +#[tokio::test] +async fn concurrent_connections_export_their_own_storage_originator() -> Result<()> { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join("config.toml"), + "cli_auth_credentials_store = \"file\"\n", + )?; + let collector = configure_collector(codex_home.path()).await?; + let (_process, address) = spawn_websocket_server(codex_home.path()).await?; + let mut clients = Vec::new(); + for name in ["Codex Desktop", "codex_vscode", "private-client-name"] { + let mut client = connect_websocket(address).await?; + send_initialize_request(&mut client, /*id*/ 1, name).await?; + read_response_for_id(&mut client, /*id*/ 1).await?; + clients.push(client); + } + // All clients initialize before any storage request; a process-global identity fails this test. + for client in &mut clients { + send_request( + client, + "account/login/start", + /*id*/ 2, + Some(json!({"type": "apiKey", "apiKey": "sk-test-key"})), + ) + .await?; + } + for client in &mut clients { + let response = read_response_for_id(client, /*id*/ 2).await?; + assert_eq!(response.result, json!({"type": "apiKey"})); + } + assert_saved_originators(&collector, &["codex_desktop", "codex_vscode", "other"]).await +} + +#[tokio::test] +async fn turn_token_refresh_exports_thread_storage_originator() -> Result<()> { + let codex_home = TempDir::new()?; + let backend = MockServer::start().await; + MockResponsesConfig::new(&backend.uri()) + .with_root_config("cli_auth_credentials_store = \"file\"") + .with_provider_config("requires_openai_auth = true") + .write(codex_home.path())?; + write_models_cache(codex_home.path()).await?; + write_chatgpt_auth( + codex_home.path(), + ChatGptAuthFixture::new("initial-access-token") + .account_id("account-123") + .chatgpt_user_id("user-123"), + AuthCredentialsStoreMode::File, + )?; + let collector = configure_collector(codex_home.path()).await?; + + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(/*s*/ 200).set_body_json(json!({ + "access_token": "refreshed-access-token", + "refresh_token": "refreshed-refresh-token", + }))) + .expect(/*r*/ 1) + .mount(&backend) + .await; + // Fresh credentials prevent startup refresh. The turn first reloads storage on 401, + // then refreshes and persists the tokens when the retry is also unauthorized. + let responses_mock = responses::mount_response_sequence( + &backend, + vec![ + ResponseTemplate::new(/*s*/ 401), + ResponseTemplate::new(/*s*/ 401), + responses::sse_response(responses::sse(vec![ + responses::ev_response_created("resp-turn"), + responses::ev_completed("resp-turn"), + ])), + ], + ) + .await; + let refresh_url = format!("{}/oauth/token", backend.uri()); + let mut server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .with_env_overrides(&[ + ("OPENAI_API_KEY", None), + ( + REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR, + Some(refresh_url.as_str()), + ), + ]) + .build() + .await?; + server + .initialize_with_client_info(ClientInfo { + name: "codex_vscode".to_string(), + title: None, + version: "0.1.0".to_string(), + }) + .await?; + let request = server + .send_thread_start_request_with_auto_env(ThreadStartParams::default()) + .await?; + let thread: ThreadStartResponse = server.read_response(request).await?; + let request = server + .send_turn_start_request(TurnStartParams { + thread_id: thread.thread.id, + input: vec![UserInput::Text { + text: "Hello".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let _: TurnStartResponse = server.read_response(request).await?; + let notification = server + .read_stream_until_notification_message("turn/completed") + .await?; + let completed: TurnCompletedNotification = + serde_json::from_value(notification.params.context("turn/completed params")?)?; + assert_eq!(completed.turn.status, TurnStatus::Completed); + assert_eq!( + responses_mock + .requests() + .iter() + .map(|request| request.header("authorization")) + .collect::>(), + vec![ + Some("Bearer initial-access-token".to_string()), + Some("Bearer initial-access-token".to_string()), + Some("Bearer refreshed-access-token".to_string()), + ] + ); + assert_saved_originators(&collector, &["codex_vscode"]).await?; + backend.verify().await; + Ok(()) +} diff --git a/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs b/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs index 235d669d62..e083aaa765 100644 --- a/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs +++ b/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs @@ -507,7 +507,8 @@ pub(super) async fn spawn_websocket_server_with_args( .stdout(Stdio::null()) .stderr(Stdio::piped()) .env("CODEX_HOME", codex_home) - .env("RUST_LOG", "warn"); + .env("RUST_LOG", "warn") + .env("OTEL_METRIC_EXPORT_INTERVAL", "200"); let mut process = cmd .kill_on_drop(true) diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index d248969036..2d82aa8221 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -9,6 +9,8 @@ mod app_list; mod app_read; mod application_network; mod attestation; +#[path = "auth_storage_originator_tests.rs"] +mod auth_storage_originator; mod auto_env; mod bedrock_setup; mod client_metadata; diff --git a/codex-rs/cli/src/login.rs b/codex-rs/cli/src/login.rs index 8b9f081dad..1d6421b060 100644 --- a/codex-rs/cli/src/login.rs +++ b/codex-rs/cli/src/login.rs @@ -46,6 +46,8 @@ const API_KEY_LOGIN_DISABLED_MESSAGE: &str = const ACCESS_TOKEN_LOGIN_DISABLED_MESSAGE: &str = "Access token login is disabled. Use API key login instead."; const LOGIN_SUCCESS_MESSAGE: &str = "Successfully logged in"; +const DEFAULT_LOGIN_LOG_FILTER: &str = + "codex_cli=info,codex_core=info,codex_login=info,codex_otel::auth_storage=warn"; /// Installs a small file-backed tracing layer for direct `codex login` flows. /// @@ -94,7 +96,7 @@ fn init_login_file_logging(config: &Config) -> Option { let (non_blocking, guard) = non_blocking(log_file); let env_filter = EnvFilter::try_from_default_env() - .unwrap_or_else(|_| EnvFilter::new("codex_cli=info,codex_core=info,codex_login=info")); + .unwrap_or_else(|_| EnvFilter::new(DEFAULT_LOGIN_LOG_FILTER)); let file_layer = tracing_subscriber::fmt::layer() .with_writer(non_blocking) .with_target(true) @@ -619,6 +621,10 @@ fn safe_format_key(key: &str) -> String { format!("{prefix}***{suffix}") } +#[cfg(test)] +#[path = "login_storage_tests.rs"] +mod storage_tests; + #[cfg(test)] mod tests { use codex_config::types::AuthCredentialsStoreMode; diff --git a/codex-rs/cli/src/login_storage_tests.rs b/codex-rs/cli/src/login_storage_tests.rs new file mode 100644 index 0000000000..ded91dcfa1 --- /dev/null +++ b/codex-rs/cli/src/login_storage_tests.rs @@ -0,0 +1,87 @@ +//! Verifies storage failures remain visible in direct login diagnostics. + +use super::DEFAULT_LOGIN_LOG_FILTER; +use codex_config::types::AuthCredentialsStoreMode; +use codex_login::AuthKeyringBackendKind; +use codex_login::login_with_api_key; +use pretty_assertions::assert_eq; +use tempfile::tempdir; +use tracing_subscriber::EnvFilter; + +#[test] +fn login_default_filter_records_sanitized_storage_fallback() -> anyhow::Result<()> { + let home = tempdir()?; + // Fail before accessing the real keyring, then allow the plaintext fallback to succeed. + std::fs::write(home.path().join("secrets"), "not a directory")?; + let log_path = home.path().join("login.log"); + let subscriber = tracing_subscriber::fmt() + .without_time() + .with_ansi(false) + .with_env_filter(EnvFilter::new(DEFAULT_LOGIN_LOG_FILTER)) + .with_writer(std::fs::File::create(&log_path)?) + .finish(); + tracing::subscriber::with_default(subscriber, || { + login_with_api_key( + home.path(), + "private-api-key", + AuthCredentialsStoreMode::Auto, + AuthKeyringBackendKind::Secrets, + ) + })?; + + let logs = std::fs::read_to_string(log_path)?; + assert_eq!(logs.lines().count(), 2); + let failure = logs.lines().next().unwrap(); + assert!( + failure.contains("failed to save auth to keyring, falling back to file storage"), + "{logs}" + ); + assert!(failure.contains("failed to create secrets dir"), "{logs}"); + assert!( + logs.contains("credential storage file fallback completed"), + "{logs}" + ); + assert!(logs.contains("fallback_reason=\"secure_error\""), "{logs}"); + assert!(logs.contains("outcome=\"success\""), "{logs}"); + assert!(!logs.contains("private-api-key")); + let completion = logs.lines().last().unwrap(); + assert!(!completion.contains(home.path().to_str().unwrap())); + let cause = std::fs::create_dir_all(home.path().join("secrets")) + .unwrap_err() + .to_string(); + insta::assert_snapshot!( + logs.replace( + home.path().join("secrets").to_str().unwrap(), + "[SECRETS_PATH]" + ) + .replace(&cause, "[OS_ERROR]") + ); + Ok(()) +} + +#[test] +fn login_storage_error_displays_context_and_cause() -> anyhow::Result<()> { + let home = tempdir()?; + let secrets_path = home.path().join("secrets"); + std::fs::write(&secrets_path, "not a directory")?; + let cause = std::fs::create_dir_all(&secrets_path) + .unwrap_err() + .to_string(); + let error = login_with_api_key( + home.path(), + "private-api-key", + AuthCredentialsStoreMode::Keyring, + AuthKeyringBackendKind::Secrets, + ) + .unwrap_err(); + let message = error.to_string(); + assert!(message.contains(&cause), "{message}"); + assert!(!message.contains("private-api-key")); + // The temporary path and native filesystem error differ across platforms. + insta::assert_snapshot!( + message + .replace(secrets_path.to_str().unwrap(), "[SECRETS_PATH]") + .replace(&cause, "[OS_ERROR]") + ); + Ok(()) +} diff --git a/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_default_filter_records_sanitized_storage_fallback.snap b/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_default_filter_records_sanitized_storage_fallback.snap new file mode 100644 index 0000000000..8536486dcc --- /dev/null +++ b/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_default_filter_records_sanitized_storage_fallback.snap @@ -0,0 +1,7 @@ +--- +source: cli/src/login_storage_tests.rs +assertion_line: 47 +expression: "logs.replace(home.path().join(\"secrets\").to_str().unwrap(),\n\"[SECRETS_PATH]\").replace(&cause, \"[OS_ERROR]\")" +--- + WARN codex_login::auth::storage: failed to save auth to keyring, falling back to file storage: failed to write OAuth tokens to encrypted auth storage: failed to create secrets dir [SECRETS_PATH]: [OS_ERROR] + WARN codex_otel::auth_storage: credential storage file fallback completed credential_kind="codex" store_mode="auto" selected_store="secrets" actual_store="file" operation="save" secure_outcome="error" outcome="success" fallback_reason="secure_error" secure_error="other" storage_phase="policy" originator="none" diff --git a/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_storage_error_displays_context_and_cause.snap b/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_storage_error_displays_context_and_cause.snap new file mode 100644 index 0000000000..f4f7250e38 --- /dev/null +++ b/codex-rs/cli/src/snapshots/codex_cli__login__storage_tests__login_storage_error_displays_context_and_cause.snap @@ -0,0 +1,6 @@ +--- +source: cli/src/login_storage_tests.rs +assertion_line: 58 +expression: "message.replace(secrets_path.to_str().unwrap(),\n\"[SECRETS_PATH]\").replace(&cause, \"[OS_ERROR]\")" +--- +failed to write OAuth tokens to encrypted auth storage: failed to create secrets dir [SECRETS_PATH]: [OS_ERROR] diff --git a/codex-rs/cli/tests/daemon_telemetry.rs b/codex-rs/cli/tests/daemon_telemetry.rs index 48ea0b207e..5e265992f3 100644 --- a/codex-rs/cli/tests/daemon_telemetry.rs +++ b/codex-rs/cli/tests/daemon_telemetry.rs @@ -53,8 +53,14 @@ async fn foreground_update_respects_consent_and_reports_unconfirmed() -> anyhow: continue; } let body: Value = serde_json::from_slice(&requests[0].body)?; - let metric = &body["resourceMetrics"][0]["scopeMetrics"][0]["metrics"][0]; - assert_eq!(metric["name"], "codex.daemon.update"); + let metric = body["resourceMetrics"] + .as_array() + .into_iter() + .flatten() + .flat_map(|resource| resource["scopeMetrics"].as_array().into_iter().flatten()) + .flat_map(|scope| scope["metrics"].as_array().into_iter().flatten()) + .find(|metric| metric["name"] == "codex.daemon.update") + .context("daemon update metric")?; assert!( metric["sum"]["dataPoints"][0]["attributes"] .as_array() diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 99066080af..056f9f0ce9 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -91,6 +91,7 @@ use codex_models_manager::manager::SharedModelsManager; use codex_network_proxy::NetworkProxy; use codex_network_proxy::NetworkProxyAuditMetadata; use codex_network_proxy::normalize_host; +use codex_otel::auth_storage::AuthStorageOriginator; use codex_otel::current_span_trace_id; use codex_otel::current_span_w3c_trace_context; use codex_otel::set_parent_from_w3c_trace_context; @@ -822,6 +823,7 @@ impl Session { ); let service_tier = get_service_tier(config.service_tier.clone(), fast_mode_enabled, &model_info); + let storage_originator = AuthStorageOriginator::from_client_name(&originator); let session_configuration = SessionConfiguration { provider: create_model_provider( config.model_provider.clone(), @@ -930,11 +932,11 @@ impl Session { // This task will run until Op::Shutdown is received. let session_for_loop = Arc::clone(&session); - let session_loop_handle = tokio::spawn(async move { + let session_loop_handle = tokio::spawn(storage_originator.scope(async move { submission_loop(session_for_loop, configured_config, rx_sub) .instrument(info_span!("session_loop", thread_id = %thread_id)) .await; - }); + })); let io = SessionIo { tx_sub, rx_event, diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index 28a3b54b1c..4666142887 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -46,6 +46,7 @@ use codex_otel::TURN_MEMORY_METRIC; use codex_otel::TURN_NETWORK_PROXY_METRIC; use codex_otel::TURN_TOOL_CALL_METRIC; use codex_otel::TURN_UNIFIED_EXEC_RUNNING_PROCESSES_METRIC; +use codex_otel::auth_storage::AuthStorageOriginator; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::EventMsg; @@ -361,46 +362,46 @@ impl Session { codex.turn.token_usage.reasoning_output_tokens = field::Empty, codex.turn.token_usage.total_tokens = field::Empty, ); - let handle = tokio::spawn( - async move { - let ctx_for_finish = Arc::clone(&ctx); - let task_result = task_for_run - .run( - Arc::clone(&session), - ctx, - task_input, - task_cancellation_token.child_token(), - ) - .instrument(trace_span!("session_task.run")) - .await; - let sess = Arc::clone(&session); - // Private reviewers save their transcript together with the terminal event. - // Errors and cancellation retain their existing save path. - if (!sess.is_private_guardian_reviewer().await - || task_cancellation_token.is_cancelled() - || task_result.is_err()) - && let Err(err) = sess.flush_rollout().await - { - warn!("failed to flush rollout before completing turn: {err}"); - sess.send_event( - ctx_for_finish.as_ref(), - EventMsg::Warning(WarningEvent { - message: format!( - "Failed to save the conversation transcript; Codex will continue retrying. Error: {err}" - ), - }), - ) - .await; - } - if !task_cancellation_token.is_cancelled() { - // Finish uniformly from the spawn site so all tasks share the same lifecycle. - sess.on_task_finished(Arc::clone(&ctx_for_finish), task_result) - .await; - } - done_clone.notify_waiters(); + let storage_originator = AuthStorageOriginator::from_client_name(&turn_context.originator); + let task_future = async move { + let ctx_for_finish = Arc::clone(&ctx); + let task_result = task_for_run + .run( + Arc::clone(&session), + ctx, + task_input, + task_cancellation_token.child_token(), + ) + .instrument(trace_span!("session_task.run")) + .await; + let sess = Arc::clone(&session); + // Private reviewers save their transcript together with the terminal event. + // Errors and cancellation retain their existing save path. + if (!sess.is_private_guardian_reviewer().await + || task_cancellation_token.is_cancelled() + || task_result.is_err()) + && let Err(err) = sess.flush_rollout().await + { + warn!("failed to flush rollout before completing turn: {err}"); + sess.send_event( + ctx_for_finish.as_ref(), + EventMsg::Warning(WarningEvent { + message: format!( + "Failed to save the conversation transcript; Codex will continue retrying. Error: {err}" + ), + }), + ) + .await; } - .instrument(task_span), - ); + if !task_cancellation_token.is_cancelled() { + // Finish uniformly from the spawn site so all tasks share the same lifecycle. + sess.on_task_finished(Arc::clone(&ctx_for_finish), task_result) + .await; + } + done_clone.notify_waiters(); + } + .instrument(task_span); + let handle = tokio::spawn(storage_originator.scope(task_future)); let timer = turn_context .session_telemetry .start_timer(TURN_E2E_DURATION_METRIC, &[]) diff --git a/codex-rs/keyring-store/src/error_display_tests.rs b/codex-rs/keyring-store/src/error_display_tests.rs new file mode 100644 index 0000000000..a7ea7f244f --- /dev/null +++ b/codex-rs/keyring-store/src/error_display_tests.rs @@ -0,0 +1,51 @@ +//! Ensures credential payloads cannot escape through diagnostic error formatting. + +use super::*; +use keyring::credential::CredentialApi; +use pretty_assertions::assert_eq; + +#[test] +fn payload_bearing_errors_redact_credentials_and_end_the_source_chain() { + let credential = keyring::mock::MockCredential::default(); + credential.set_password("private-credential-value").unwrap(); + for (error, message) in [ + ( + KeyringError::Ambiguous(vec![Box::new(credential) as Box]), + "Entry is matched by 1 credentials", + ), + ( + KeyringError::BadEncoding(b"private-credential-value".to_vec()), + "Data is not UTF-8 encoded", + ), + ] { + let error = CredentialStoreError::new(error); + assert_eq!( + ( + error.to_string(), + error.message(), + format!("{error:?}"), + error.source().is_none() + ), + ( + message.to_string(), + message.to_string(), + message.to_string(), + true + ), + ); + } +} + +#[test] +fn debug_preserves_native_error_details() { + let error = CredentialStoreError::new(KeyringError::PlatformFailure(Box::new( + std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "permission denied by keyring", + ), + ))); + assert_eq!( + format!("{error:?}"), + "Platform secure storage failure: permission denied by keyring", + ); +} diff --git a/codex-rs/keyring-store/src/lib.rs b/codex-rs/keyring-store/src/lib.rs index 3b0a1c414f..bd38bc6ecd 100644 --- a/codex-rs/keyring-store/src/lib.rs +++ b/codex-rs/keyring-store/src/lib.rs @@ -5,7 +5,6 @@ use std::fmt; use std::fmt::Debug; use tracing::trace; -#[derive(Debug)] pub enum CredentialStoreError { Other(KeyringError), } @@ -16,9 +15,7 @@ impl CredentialStoreError { } pub fn message(&self) -> String { - match self { - Self::Other(error) => error.to_string(), - } + self.to_string() } pub fn into_error(self) -> KeyringError { @@ -31,19 +28,34 @@ impl CredentialStoreError { impl fmt::Display for CredentialStoreError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { + Self::Other(KeyringError::Ambiguous(items)) => { + write!(f, "Entry is matched by {} credentials", items.len()) + } Self::Other(error) => write!(f, "{error}"), } } } +impl fmt::Debug for CredentialStoreError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(self, f) + } +} + impl Error for CredentialStoreError { fn source(&self) -> Option<&(dyn Error + 'static)> { match self { + // Do not expose payload-bearing variants to error-chain formatters. + Self::Other(KeyringError::Ambiguous(_) | KeyringError::BadEncoding(_)) => None, Self::Other(error) => Some(error), } } } +#[cfg(test)] +#[path = "error_display_tests.rs"] +mod error_display_tests; + mod error_kind; impl From for std::io::Error { diff --git a/codex-rs/login/Cargo.toml b/codex-rs/login/Cargo.toml index 8772f46b58..7762770be8 100644 --- a/codex-rs/login/Cargo.toml +++ b/codex-rs/login/Cargo.toml @@ -9,6 +9,7 @@ workspace = true [dependencies] codex-utils-process = { workspace = true } +anyhow = { workspace = true } base64 = { workspace = true } chrono = { workspace = true, features = ["serde"] } codex-agent-identity = { workspace = true } @@ -44,7 +45,7 @@ urlencoding = { workspace = true } webbrowser = { workspace = true } [dev-dependencies] -anyhow = { workspace = true } +opentelemetry_sdk = { workspace = true, features = ["testing"] } core_test_support = { workspace = true } jsonwebtoken = { workspace = true } keyring = { workspace = true } diff --git a/codex-rs/login/src/auth/storage.rs b/codex-rs/login/src/auth/storage.rs index 8a9d75f448..4dccbc9138 100644 --- a/codex-rs/login/src/auth/storage.rs +++ b/codex-rs/login/src/auth/storage.rs @@ -1,5 +1,7 @@ use chrono::DateTime; use chrono::Utc; +use codex_otel::auth_storage::Operation; +use codex_otel::auth_storage::Store; use serde::Deserialize; use serde::Serialize; use sha2::Digest; @@ -18,6 +20,12 @@ use std::sync::Arc; use std::sync::Mutex; use tracing::warn; +#[path = "storage_error.rs"] +mod storage_error; + +#[path = "storage_telemetry.rs"] +mod storage_telemetry; + use super::BedrockAccessKeysAuth; use super::BedrockApiKeyAuth; use crate::token_data::TokenData; @@ -251,44 +259,50 @@ fn compute_store_key(codex_home: &Path) -> std::io::Result { #[derive(Clone, Debug)] struct DirectKeyringAuthStorage { codex_home: PathBuf, + mode: AuthCredentialsStoreMode, keyring_store: Arc, } impl DirectKeyringAuthStorage { - fn new(codex_home: PathBuf, keyring_store: Arc) -> Self { + fn new( + codex_home: PathBuf, + keyring_store: Arc, + mode: AuthCredentialsStoreMode, + ) -> Self { Self { codex_home, keyring_store, + mode, } } fn load_from_keyring(&self, key: &str) -> std::io::Result> { - match self.keyring_store.load(KEYRING_SERVICE, key) { - Ok(Some(serialized)) => serde_json::from_str(&serialized).map(Some).map_err(|err| { - std::io::Error::other(format!( - "failed to deserialize CLI auth from keyring: {err}" - )) + match self + .keyring_store + .load(KEYRING_SERVICE, key) + .map_err(std::io::Error::from) + .map_err(|error| { + storage_error::with_context("failed to load CLI auth from keyring", error) + })? { + Some(serialized) => serde_json::from_str(&serialized).map(Some).map_err(|err| { + storage_error::with_context("failed to deserialize CLI auth from keyring", err) }), - Ok(None) => Ok(None), - Err(error) => Err(std::io::Error::other(format!( - "failed to load CLI auth from keyring: {}", - error.message() - ))), + None => Ok(None), } } fn save_to_keyring(&self, key: &str, value: &str) -> std::io::Result<()> { - match self.keyring_store.save(KEYRING_SERVICE, key, value) { - Ok(()) => Ok(()), - Err(error) => { - let message = format!( - "failed to write OAuth tokens to keyring: {}", - error.message() - ); - warn!("{message}"); - Err(std::io::Error::other(message)) - } - } + self.keyring_store + .save(KEYRING_SERVICE, key, value) + .map_err(std::io::Error::from) + .map_err(|error| { + let error = + storage_error::with_context("failed to write OAuth tokens to keyring", error); + if self.mode == AuthCredentialsStoreMode::Keyring { + warn!("{error}"); + } + error + }) } } @@ -303,9 +317,17 @@ impl AuthStorageBackend for DirectKeyringAuthStorage { // Simpler error mapping per style: prefer method reference over closure let serialized = serde_json::to_string(auth).map_err(std::io::Error::other)?; self.save_to_keyring(&key, &serialized)?; - if let Err(err) = delete_file_if_exists(&self.codex_home) { + let mut telemetry = storage_telemetry::telemetry( + self.mode, + AuthKeyringBackendKind::Direct, + Operation::Cleanup, + ); + let result = delete_file_if_exists(&self.codex_home); + telemetry.record_delete_attempt(Store::File, &result); + if let Err(err) = &result { warn!("failed to remove CLI auth fallback file: {err}"); } + drop(telemetry); Ok(()) } @@ -314,8 +336,9 @@ impl AuthStorageBackend for DirectKeyringAuthStorage { let keyring_removed = self .keyring_store .delete(KEYRING_SERVICE, &key) - .map_err(|err| { - std::io::Error::other(format!("failed to delete auth from keyring: {err}")) + .map_err(std::io::Error::from) + .map_err(|error| { + storage_error::with_context("failed to delete auth from keyring", error) })?; let file_removed = delete_file_if_exists(&self.codex_home)?; Ok(keyring_removed || file_removed) @@ -325,6 +348,7 @@ impl AuthStorageBackend for DirectKeyringAuthStorage { #[derive(Clone)] struct SecretsKeyringAuthStorage { codex_home: PathBuf, + mode: AuthCredentialsStoreMode, direct_storage: DirectKeyringAuthStorage, secrets_manager: SecretsManager, } @@ -338,9 +362,13 @@ impl Debug for SecretsKeyringAuthStorage { } impl SecretsKeyringAuthStorage { - fn new(codex_home: PathBuf, keyring_store: Arc) -> Self { + fn new( + codex_home: PathBuf, + keyring_store: Arc, + mode: AuthCredentialsStoreMode, + ) -> Self { let direct_storage = - DirectKeyringAuthStorage::new(codex_home.clone(), Arc::clone(&keyring_store)); + DirectKeyringAuthStorage::new(codex_home.clone(), Arc::clone(&keyring_store), mode); let secrets_manager = SecretsManager::new_with_keyring_store_and_namespace( codex_home.clone(), SecretsBackendKind::Local, @@ -349,6 +377,7 @@ impl SecretsKeyringAuthStorage { ); Self { codex_home, + mode, direct_storage, secrets_manager, } @@ -360,15 +389,17 @@ impl AuthStorageBackend for SecretsKeyringAuthStorage { match self .secrets_manager .get(&SecretScope::Global, &CODEX_AUTH_SECRET_NAME) - .map_err(|err| { - std::io::Error::other(format!( - "failed to load CLI auth from encrypted auth storage: {err}" - )) + .map_err(|error| { + storage_error::with_context( + "failed to load CLI auth from encrypted auth storage", + error, + ) })? { Some(serialized) => serde_json::from_str(&serialized).map(Some).map_err(|err| { - std::io::Error::other(format!( - "failed to deserialize CLI auth from encrypted auth storage: {err}" - )) + storage_error::with_context( + "failed to deserialize CLI auth from encrypted auth storage", + err, + ) }), None => Ok(None), } @@ -378,15 +409,27 @@ impl AuthStorageBackend for SecretsKeyringAuthStorage { let serialized = serde_json::to_string(auth).map_err(std::io::Error::other)?; self.secrets_manager .set(&SecretScope::Global, &CODEX_AUTH_SECRET_NAME, &serialized) - .map_err(|err| { - let message = - format!("failed to write OAuth tokens to encrypted auth storage: {err}"); - warn!("{message}"); - std::io::Error::other(message) + .map_err(|error| { + let error = storage_error::with_context( + "failed to write OAuth tokens to encrypted auth storage", + error, + ); + if self.mode == AuthCredentialsStoreMode::Keyring { + warn!("{error}"); + } + error })?; - if let Err(err) = delete_file_if_exists(&self.codex_home) { + let mut telemetry = storage_telemetry::telemetry( + self.mode, + AuthKeyringBackendKind::Secrets, + Operation::Cleanup, + ); + let result = delete_file_if_exists(&self.codex_home); + telemetry.record_delete_attempt(Store::File, &result); + if let Err(err) = &result { warn!("failed to remove CLI auth fallback file: {err}"); } + drop(telemetry); Ok(()) } @@ -394,10 +437,11 @@ impl AuthStorageBackend for SecretsKeyringAuthStorage { let keyring_removed = self .secrets_manager .delete(&SecretScope::Global, &CODEX_AUTH_SECRET_NAME) - .map_err(|err| { - std::io::Error::other(format!( - "failed to delete auth from encrypted auth storage: {err}" - )) + .map_err(|error| { + storage_error::with_context( + "failed to delete auth from encrypted auth storage", + error, + ) })?; let file_removed = delete_file_if_exists(&self.codex_home)?; let direct_removed = self.direct_storage.delete()?; @@ -409,6 +453,7 @@ impl AuthStorageBackend for SecretsKeyringAuthStorage { struct AutoAuthStorage { keyring_storage: Arc, file_storage: Arc, + keyring_backend_kind: AuthKeyringBackendKind, } impl AutoAuthStorage { @@ -422,37 +467,76 @@ impl AutoAuthStorage { codex_home.clone(), keyring_store, keyring_backend_kind, + AuthCredentialsStoreMode::Auto, ), file_storage: Arc::new(FileAuthStorage::new(codex_home)), + keyring_backend_kind, } } } impl AuthStorageBackend for AutoAuthStorage { fn load(&self) -> std::io::Result> { - match self.keyring_storage.load() { + let mut telemetry = storage_telemetry::telemetry( + AuthCredentialsStoreMode::Auto, + self.keyring_backend_kind, + Operation::Load, + ); + let result = self.keyring_storage.load(); + telemetry.record_load_attempt( + storage_telemetry::keyring_store(self.keyring_backend_kind), + &result, + ); + match result { Ok(Some(auth)) => Ok(Some(auth)), - Ok(None) => self.file_storage.load(), + Ok(None) => { + let result = self.file_storage.load(); + telemetry.record_load_attempt(Store::File, &result); + result + } Err(err) => { warn!("failed to load CLI auth from keyring, falling back to file storage: {err}"); - self.file_storage.load() + telemetry.record_secure_error(&err); + let result = self.file_storage.load(); + telemetry.record_load_attempt(Store::File, &result); + result } } } fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> { - match self.keyring_storage.save(auth) { + let mut telemetry = storage_telemetry::telemetry( + AuthCredentialsStoreMode::Auto, + self.keyring_backend_kind, + Operation::Save, + ); + let result = self.keyring_storage.save(auth); + telemetry.record_save_attempt( + storage_telemetry::keyring_store(self.keyring_backend_kind), + &result, + ); + match result { Ok(()) => Ok(()), Err(err) => { warn!("failed to save auth to keyring, falling back to file storage: {err}"); - self.file_storage.save(auth) + telemetry.record_secure_error(&err); + let result = self.file_storage.save(auth); + telemetry.record_save_attempt(Store::File, &result); + result } } } fn delete(&self) -> std::io::Result { // Keyring storage will delete from disk as well - self.keyring_storage.delete() + let mut telemetry = storage_telemetry::telemetry( + AuthCredentialsStoreMode::Auto, + self.keyring_backend_kind, + Operation::Delete, + ); + let result = self.keyring_storage.delete(); + telemetry.record_delete_attempt(Store::Multiple, &result); + result } } @@ -514,10 +598,10 @@ fn create_auth_storage_with_store( keyring_store: Arc, keyring_backend_kind: AuthKeyringBackendKind, ) -> Arc { - match mode { + let storage: Arc = match mode { AuthCredentialsStoreMode::File => Arc::new(FileAuthStorage::new(codex_home)), AuthCredentialsStoreMode::Keyring => { - create_keyring_auth_storage(codex_home, keyring_store, keyring_backend_kind) + create_keyring_auth_storage(codex_home, keyring_store, keyring_backend_kind, mode) } AuthCredentialsStoreMode::Auto => Arc::new(AutoAuthStorage::new( codex_home, @@ -525,21 +609,27 @@ fn create_auth_storage_with_store( keyring_backend_kind, )), AuthCredentialsStoreMode::Ephemeral => Arc::new(EphemeralAuthStorage::new(codex_home)), - } + }; + storage_telemetry::observe(storage, mode, keyring_backend_kind) } fn create_keyring_auth_storage( codex_home: PathBuf, keyring_store: Arc, keyring_backend_kind: AuthKeyringBackendKind, + mode: AuthCredentialsStoreMode, ) -> Arc { match keyring_backend_kind { - AuthKeyringBackendKind::Direct => { - Arc::new(DirectKeyringAuthStorage::new(codex_home, keyring_store)) - } - AuthKeyringBackendKind::Secrets => { - Arc::new(SecretsKeyringAuthStorage::new(codex_home, keyring_store)) - } + AuthKeyringBackendKind::Direct => Arc::new(DirectKeyringAuthStorage::new( + codex_home, + keyring_store, + mode, + )), + AuthKeyringBackendKind::Secrets => Arc::new(SecretsKeyringAuthStorage::new( + codex_home, + keyring_store, + mode, + )), } } diff --git a/codex-rs/login/src/auth/storage_error.rs b/codex-rs/login/src/auth/storage_error.rs new file mode 100644 index 0000000000..b25daa0247 --- /dev/null +++ b/codex-rs/login/src/auth/storage_error.rs @@ -0,0 +1,49 @@ +//! Removes credential JSON data errors while preserving native storage diagnostics and typed causes. + +#[derive(thiserror::Error)] +struct CredentialStorageError { + context: &'static str, + #[source] + source: anyhow::Error, +} + +impl std::fmt::Display for CredentialStorageError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}: {:#}", self.context, self.source) + } +} + +impl std::fmt::Debug for CredentialStorageError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(self, f) + } +} + +pub(super) fn with_context( + context: &'static str, + source: impl Into, +) -> std::io::Error { + let mut source = source.into(); + let mut cause: Option<&(dyn std::error::Error + 'static)> = Some(source.as_ref()); + while let Some(error) = cause { + if let Some(error) = error.downcast_ref::() + && error.is_data() + { + // Discard the entire chain: serde errors and their context may contain credentials. + source = anyhow::anyhow!( + "invalid credential JSON ({:?} at line {} column {})", + error.classify(), + error.line(), + error.column(), + ); + break; + } + cause = if let Some(error) = error.downcast_ref::() { + // io::Error::source() skips its contained error. + error.get_ref().map(|error| error as &dyn std::error::Error) + } else { + error.source() + }; + } + std::io::Error::other(CredentialStorageError { context, source }) +} diff --git a/codex-rs/login/src/auth/storage_error_tests.rs b/codex-rs/login/src/auth/storage_error_tests.rs new file mode 100644 index 0000000000..8b44b7f6d2 --- /dev/null +++ b/codex-rs/login/src/auth/storage_error_tests.rs @@ -0,0 +1,146 @@ +//! Checks that storage diagnostics preserve useful causes without exposing credential values. + +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn secure_storage_errors_display_the_underlying_cause() -> anyhow::Result<()> { + for kind in [ + AuthKeyringBackendKind::Direct, + AuthKeyringBackendKind::Secrets, + ] { + for (error_kind, cause) in [ + ( + std::io::ErrorKind::PermissionDenied, + "permission denied by keyring", + ), + (std::io::ErrorKind::NotConnected, "keyring unavailable"), + ] { + let home = tempdir()?; + let keyring = MockKeyringStore::default(); + let storage = create_auth_storage_with_store( + home.path().to_path_buf(), + AuthCredentialsStoreMode::Keyring, + Arc::new(keyring.clone()), + kind, + ); + let auth = auth_with_prefix("private credential value"); + storage.save(&auth)?; + let key = match kind { + AuthKeyringBackendKind::Direct => compute_store_key(home.path())?, + AuthKeyringBackendKind::Secrets => { + compute_keyring_account(home.path(), LocalSecretsNamespace::CodexAuth) + } + }; + for operation in [Operation::Load, Operation::Save, Operation::Delete] { + keyring.set_error( + &key, + KeyringError::PlatformFailure(Box::new(std::io::Error::new(error_kind, cause))), + ); + let error = match operation { + Operation::Load => storage.load().unwrap_err(), + Operation::Save => storage.save(&auth).unwrap_err(), + Operation::Delete => storage.delete().unwrap_err(), + Operation::Cleanup | Operation::RefreshPersist => unreachable!(), + }; + let message = error.to_string(); + assert!(message.starts_with("failed to "), "{message}"); + assert!(message.contains(cause), "{message}"); + assert!(!message.contains("private credential value")); + let debug = format!("{error:?}"); + assert!(debug.contains(cause), "{debug}"); + assert!(!debug.contains("private credential value")); + } + } + } + Ok(()) +} + +#[test] +fn json_syntax_and_eof_errors_preserve_diagnostics() { + for (input, category) in [ + ( + r#"{"OPENAI_API_KEY" "private-credential-value"}"#, + serde_json::error::Category::Syntax, + ), + ( + r#"{"OPENAI_API_KEY":"private-credential-value""#, + serde_json::error::Category::Eof, + ), + ] { + let source = serde_json::from_str::(input).unwrap_err(); + assert_eq!(source.classify(), category); + let expected = format!("failed to load auth: decrypted auth JSON: {source}"); + let error = storage_error::with_context( + "failed to load auth", + anyhow::Error::new(source).context("decrypted auth JSON"), + ); + let inner = error.get_ref().unwrap(); + assert_eq!( + (error.to_string(), format!("{inner:?}")), + (expected.clone(), expected) + ); + assert!(!format!("{error:?}").contains("private-credential-value")); + } +} + +#[test] +fn json_data_errors_redact_credential_values() { + enum Wrapper { + Direct, + AnyhowContext, + Io, + Nested, + } + + for wrapper in [ + Wrapper::Direct, + Wrapper::AnyhowContext, + Wrapper::Io, + Wrapper::Nested, + ] { + let source = + serde_json::from_str::(r#"{"auth_mode":"private-credential-value"}"#) + .unwrap_err(); + assert!(source.to_string().contains("private-credential-value")); + let expected = format!( + "failed to load auth: invalid credential JSON (Data at line {} column {})", + source.line(), + source.column(), + ); + let source = match wrapper { + Wrapper::Direct => anyhow::Error::new(source), + Wrapper::AnyhowContext => anyhow::Error::new(source).context("decrypted auth JSON"), + Wrapper::Io => anyhow::Error::new(std::io::Error::other(source)), + Wrapper::Nested => anyhow::Error::new(std::io::Error::other( + anyhow::Error::new(std::io::Error::other(source)) + .context("private-credential-value"), + )) + .context("decrypted auth JSON"), + }; + let error = storage_error::with_context("failed to load auth", source); + let inner = error.get_ref().unwrap(); + assert_eq!( + (error.to_string(), format!("{inner:?}")), + (expected.clone(), expected) + ); + assert!(!format!("{error:?}").contains("private-credential-value")); + + let error = anyhow::Error::new(error).context("credential storage failed"); + for formatted in [format!("{error:#}"), format!("{error:?}")] { + assert!( + !formatted.contains("private-credential-value"), + "{formatted}" + ); + } + for cause in error.chain() { + assert!(cause.downcast_ref::().is_none()); + for formatted in [cause.to_string(), format!("{cause:?}")] { + assert!( + !formatted.contains("private-credential-value"), + "{formatted}" + ); + } + } + } +} diff --git a/codex-rs/login/src/auth/storage_policy_tests.rs b/codex-rs/login/src/auth/storage_policy_tests.rs new file mode 100644 index 0000000000..0d389b756c --- /dev/null +++ b/codex-rs/login/src/auth/storage_policy_tests.rs @@ -0,0 +1,443 @@ +//! Exercises configured storage policy and its emitted metadata. + +use super::*; +use opentelemetry_sdk::metrics::data::AggregatedMetrics; +use opentelemetry_sdk::metrics::data::MetricData; +use pretty_assertions::assert_eq; + +#[test] +fn storage_metrics_distinguish_explicit_file_from_secure_failure_fallback() -> anyhow::Result<()> { + if std::env::var_os("CODEX_AUTH_STORAGE_TEST_CHILD").is_none() { + let output = std::process::Command::new(std::env::current_exe()?) + .args([ + "--exact", + std::thread::current().name().unwrap(), + "--nocapture", + ]) + .env("CODEX_AUTH_STORAGE_TEST_CHILD", "1") + .output()?; + assert!( + output.status.success(), + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return Ok(()); + } + use std::collections::BTreeMap; + let metrics = codex_otel::install_global_metrics(codex_otel::MetricsClient::new( + codex_otel::MetricsConfig::in_memory("test", "test", "1", Default::default()) + .with_runtime_reader(), + )?); + let mut expected = BTreeMap::new(); + for kind in [ + AuthKeyringBackendKind::Direct, + AuthKeyringBackendKind::Secrets, + ] { + for (scenario, mode, operation, actual, secure, outcome, reason, error) in [ + ( + "success", + AuthCredentialsStoreMode::Auto, + "save", + storage_telemetry::keyring_store(kind).as_ref(), + "success", + "success", + "none", + "none", + ), + ( + "cleanup_failed", + AuthCredentialsStoreMode::Auto, + "save", + storage_telemetry::keyring_store(kind).as_ref(), + "success", + "success", + "none", + "none", + ), + ( + "success", + AuthCredentialsStoreMode::Keyring, + "save", + storage_telemetry::keyring_store(kind).as_ref(), + "success", + "success", + "none", + "none", + ), + ( + "cleanup_failed", + AuthCredentialsStoreMode::Keyring, + "save", + storage_telemetry::keyring_store(kind).as_ref(), + "success", + "success", + "none", + "none", + ), + ( + "denied", + AuthCredentialsStoreMode::Auto, + "save", + "file", + "error", + "success", + "secure_error", + "access_denied", + ), + ( + "explicit", + AuthCredentialsStoreMode::File, + "save", + "file", + "not_attempted", + "success", + "none", + "none", + ), + ( + "missing", + AuthCredentialsStoreMode::Auto, + "load", + "file", + "not_found", + "success", + "secure_entry_missing", + "none", + ), + ( + "failed_file", + AuthCredentialsStoreMode::Auto, + "load", + "file", + "error", + "error", + "secure_error", + "access_denied", + ), + ] { + let home = tempdir()?; + let keyring = MockKeyringStore::default(); + let key = match kind { + AuthKeyringBackendKind::Direct => compute_store_key(home.path())?, + AuthKeyringBackendKind::Secrets => { + compute_keyring_account(home.path(), LocalSecretsNamespace::CodexAuth) + } + }; + let auth = auth_with_prefix("private credential value"); + if operation == "load" || scenario == "success" { + FileAuthStorage::new(home.path().to_path_buf()).save(&auth)?; + } + if scenario == "cleanup_failed" { + std::fs::create_dir(get_auth_file(home.path()))?; + } + if scenario == "failed_file" { + std::fs::write(get_auth_file(home.path()), "invalid private file contents")?; + } + if error == "access_denied" { + keyring.set_error( + &key, + KeyringError::PlatformFailure(Box::new(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "private account path error", + ))), + ); + // Secrets loads consult the keyring only when an encrypted file exists. + if operation == "load" && kind == AuthKeyringBackendKind::Secrets { + let manager = SecretsManager::new_with_keyring_store_and_namespace( + home.path().to_path_buf(), + SecretsBackendKind::Local, + Arc::new(MockKeyringStore::default()), + LocalSecretsNamespace::CodexAuth, + ); + manager.set(&SecretScope::Global, &CODEX_AUTH_SECRET_NAME, "{}")?; + } + } + let storage = create_auth_storage_with_store( + home.path().to_path_buf(), + mode, + Arc::new(keyring), + kind, + ); + if operation == "load" { + let result = storage.load(); + if outcome == "error" { + assert!(result.is_err()); + } else { + assert_eq!(result?, Some(auth.clone())); + } + } else { + storage.save(&auth)?; + } + let store_mode = match mode { + AuthCredentialsStoreMode::Auto => "auto", + AuthCredentialsStoreMode::Keyring => "keyring", + AuthCredentialsStoreMode::File => "file", + AuthCredentialsStoreMode::Ephemeral => "ephemeral", + }; + let tags: BTreeMap = BTreeMap::from([ + ("credential_kind".into(), "codex".into()), + ("store_mode".into(), store_mode.into()), + ( + "selected_store".into(), + storage_telemetry::keyring_store(kind).as_ref().into(), + ), + ("actual_store".into(), actual.into()), + ("operation".into(), operation.into()), + ("secure_outcome".into(), secure.into()), + ("outcome".into(), outcome.into()), + ("fallback_reason".into(), reason.into()), + ("secure_error".into(), error.into()), + ("storage_phase".into(), "policy".into()), + ("originator".into(), "none".into()), + ]); + if secure == "success" { + let mut cleanup_tags = tags.clone(); + cleanup_tags.extend([ + ("actual_store".into(), "file".into()), + ("operation".into(), "cleanup".into()), + ("secure_outcome".into(), "not_attempted".into()), + ( + "outcome".into(), + if scenario == "cleanup_failed" { + "error" + } else { + "success" + } + .into(), + ), + ]); + *expected.entry(cleanup_tags).or_insert(0) += 1; + } + *expected.entry(tags).or_insert(0) += 1; + } + } + let snapshot = metrics.snapshot()?; + let mut actual = Vec::new(); + for metric in snapshot + .scope_metrics() + .flat_map(opentelemetry_sdk::metrics::data::ScopeMetrics::metrics) + { + if metric.name() == "codex.auth_storage.operation" + && let AggregatedMetrics::U64(MetricData::Sum(sum)) = metric.data() + { + for point in sum.data_points() { + let tags: BTreeMap = point + .attributes() + .map(|a| (a.key.to_string(), a.value.to_string())) + .collect(); + actual.push((point.value(), tags)); + } + } + } + let mut expected: Vec<_> = expected + .into_iter() + .map(|(tags, count)| (count, tags)) + .collect(); + actual.sort(); + expected.sort(); + assert_eq!(actual, expected); + metrics.shutdown()?; + Ok(()) +} + +#[test] +fn auto_auth_storage_load_prefers_keyring_value() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = AutoAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + AuthKeyringBackendKind::Secrets, + ); + let keyring_auth = auth_with_prefix("keyring"); + seed_secrets_backend_with_auth(&mock_keyring, codex_home.path(), &keyring_auth)?; + + let file_auth = auth_with_prefix("file"); + storage.file_storage.save(&file_auth)?; + + let loaded = storage.load()?; + assert_eq!(loaded, Some(keyring_auth)); + Ok(()) +} + +#[test] +fn auto_auth_storage_load_uses_file_when_keyring_empty() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = AutoAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring), + AuthKeyringBackendKind::Secrets, + ); + + let expected = auth_with_prefix("file-only"); + storage.file_storage.save(&expected)?; + + let loaded = storage.load()?; + assert_eq!(loaded, Some(expected)); + Ok(()) +} + +#[test] +fn auto_auth_storage_load_falls_back_when_keyring_errors() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = AutoAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + AuthKeyringBackendKind::Secrets, + ); + let key = compute_keyring_account(codex_home.path(), LocalSecretsNamespace::CodexAuth); + + let encrypted = auth_with_prefix("encrypted"); + seed_secrets_backend_with_auth(&mock_keyring, codex_home.path(), &encrypted)?; + mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "load".into())); + + let expected = auth_with_prefix("fallback"); + storage.file_storage.save(&expected)?; + + let loaded = storage.load()?; + assert_eq!(loaded, Some(expected)); + Ok(()) +} + +#[test] +fn auto_auth_storage_save_prefers_keyring() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = AutoAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + AuthKeyringBackendKind::Secrets, + ); + let stale = auth_with_prefix("stale"); + storage.file_storage.save(&stale)?; + + let expected = auth_with_prefix("to-save"); + storage.save(&expected)?; + + assert_keyring_saved_auth_and_removed_fallback(&mock_keyring, codex_home.path(), &expected)?; + Ok(()) +} + +#[test] +fn auto_auth_storage_save_falls_back_when_keyring_errors() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = AutoAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + AuthKeyringBackendKind::Secrets, + ); + let key = compute_keyring_account(codex_home.path(), LocalSecretsNamespace::CodexAuth); + mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "save".into())); + + let auth = auth_with_prefix("fallback"); + storage.save(&auth)?; + + let auth_file = get_auth_file(codex_home.path()); + assert!( + auth_file.exists(), + "fallback auth.json should be created when keyring save fails" + ); + let saved = storage + .file_storage + .load()? + .context("fallback auth should exist")?; + assert_eq!(saved, auth); + assert!( + mock_keyring.saved_value(&key).is_none(), + "keyring should not contain value when save fails" + ); + Ok(()) +} + +#[test] +fn auto_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = AutoAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + AuthKeyringBackendKind::Secrets, + ); + let auth = auth_with_prefix("to-delete"); + let auth_file = seed_secrets_backend_and_fallback_auth_file_for_delete( + &mock_keyring, + codex_home.path(), + &auth, + )?; + + let removed = storage.delete()?; + + assert!(removed, "delete should report removal"); + assert_eq!(storage.load()?, None, "encrypted auth should be removed"); + assert!( + !auth_file.exists(), + "fallback auth.json should be removed after delete" + ); + Ok(()) +} + +#[test] +fn direct_keyring_auth_storage_saves_legacy_keyring_entry() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = DirectKeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + AuthCredentialsStoreMode::Keyring, + ); + let auth_file = get_auth_file(codex_home.path()); + std::fs::write(&auth_file, "stale")?; + let auth = auth_with_prefix("direct"); + + storage.save(&auth)?; + + let legacy_key = compute_store_key(codex_home.path())?; + let saved_value = mock_keyring + .saved_value(&legacy_key) + .context("direct keyring auth entry should exist")?; + assert_eq!(saved_value, serde_json::to_string(&auth)?); + assert!(!encrypted_auth_file(codex_home.path()).exists()); + assert!( + !auth_file.exists(), + "fallback auth.json should be removed after keyring save" + ); + assert_eq!(storage.load()?, Some(auth)); + Ok(()) +} + +#[test] +fn secrets_keyring_auth_storage_save_persists_and_removes_fallback_file() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = SecretsKeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + AuthCredentialsStoreMode::Keyring, + ); + let auth_file = get_auth_file(codex_home.path()); + std::fs::write(&auth_file, "stale")?; + let auth = AuthDotJson { + auth_mode: Some(AuthMode::Chatgpt), + openai_api_key: None, + tokens: Some(TokenData { + id_token: Default::default(), + access_token: "access".to_string(), + refresh_token: "refresh".to_string(), + account_id: Some("account".to_string()), + }), + last_refresh: Some(Utc::now()), + agent_identity: None, + personal_access_token: None, + bedrock_api_key: None, + bedrock_access_keys: None, + }; + + storage.save(&auth)?; + + assert_keyring_saved_auth_and_removed_fallback(&mock_keyring, codex_home.path(), &auth)?; + Ok(()) +} diff --git a/codex-rs/login/src/auth/storage_telemetry.rs b/codex-rs/login/src/auth/storage_telemetry.rs new file mode 100644 index 0000000000..9fb4639981 --- /dev/null +++ b/codex-rs/login/src/auth/storage_telemetry.rs @@ -0,0 +1,108 @@ +//! Observes logical Codex credential operations without inspecting authentication material. + +use super::AuthDotJson; +use super::AuthStorageBackend; +use codex_config::types::AuthCredentialsStoreMode; +use codex_config::types::AuthKeyringBackendKind; +use codex_otel::auth_storage::AuthStorageOriginator; +use codex_otel::auth_storage::CredentialKind; +use codex_otel::auth_storage::Operation; +use codex_otel::auth_storage::StorageTelemetry; +use codex_otel::auth_storage::Store; +use codex_otel::auth_storage::StoreMode; +use std::sync::Arc; + +pub(super) fn keyring_store(kind: AuthKeyringBackendKind) -> Store { + match kind { + AuthKeyringBackendKind::Direct => Store::DirectKeyring, + AuthKeyringBackendKind::Secrets => Store::Secrets, + } +} + +pub(super) fn telemetry( + mode: AuthCredentialsStoreMode, + kind: AuthKeyringBackendKind, + operation: Operation, +) -> StorageTelemetry { + let mode = match mode { + AuthCredentialsStoreMode::File => StoreMode::File, + AuthCredentialsStoreMode::Auto => StoreMode::Auto, + AuthCredentialsStoreMode::Keyring => StoreMode::Keyring, + AuthCredentialsStoreMode::Ephemeral => StoreMode::Ephemeral, + }; + StorageTelemetry::new( + CredentialKind::Codex, + mode, + keyring_store(kind), + operation, + AuthStorageOriginator::current(), + ) +} + +pub(super) fn observe( + inner: Arc, + mode: AuthCredentialsStoreMode, + kind: AuthKeyringBackendKind, +) -> Arc { + // Auto records both attempts together so file fallback cannot hide a secure-store failure. + if mode == AuthCredentialsStoreMode::Auto { + return inner; + } + Arc::new(ObservedStorage { inner, mode, kind }) +} + +#[derive(Debug)] +struct ObservedStorage { + inner: Arc, + mode: AuthCredentialsStoreMode, + kind: AuthKeyringBackendKind, +} + +impl ObservedStorage { + fn store(&self) -> Store { + match self.mode { + AuthCredentialsStoreMode::File => Store::File, + AuthCredentialsStoreMode::Ephemeral => Store::Ephemeral, + AuthCredentialsStoreMode::Keyring | AuthCredentialsStoreMode::Auto => { + keyring_store(self.kind) + } + } + } +} + +impl AuthStorageBackend for ObservedStorage { + fn load(&self) -> std::io::Result> { + let mut telemetry = telemetry(self.mode, self.kind, Operation::Load); + let result = self.inner.load(); + telemetry.record_load_attempt(self.store(), &result); + if self.mode == AuthCredentialsStoreMode::Keyring + && let Err(error) = &result + { + telemetry.record_secure_error(error); + } + result + } + + fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> { + let mut telemetry = telemetry(self.mode, self.kind, Operation::Save); + let result = self.inner.save(auth); + telemetry.record_save_attempt(self.store(), &result); + if self.mode == AuthCredentialsStoreMode::Keyring + && let Err(error) = &result + { + telemetry.record_secure_error(error); + } + result + } + + fn delete(&self) -> std::io::Result { + let store = match self.mode { + AuthCredentialsStoreMode::File | AuthCredentialsStoreMode::Ephemeral => self.store(), + AuthCredentialsStoreMode::Auto | AuthCredentialsStoreMode::Keyring => Store::Multiple, + }; + let mut telemetry = telemetry(self.mode, self.kind, Operation::Delete); + let result = self.inner.delete(); + telemetry.record_delete_attempt(store, &result); + result + } +} diff --git a/codex-rs/login/src/auth/storage_tests.rs b/codex-rs/login/src/auth/storage_tests.rs index 79c51c871e..01c2e6f575 100644 --- a/codex-rs/login/src/auth/storage_tests.rs +++ b/codex-rs/login/src/auth/storage_tests.rs @@ -458,6 +458,7 @@ fn secrets_keyring_auth_storage_load_returns_deserialized_auth() -> anyhow::Resu let storage = SecretsKeyringAuthStorage::new( codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), + AuthCredentialsStoreMode::Keyring, ); let expected = AuthDotJson { auth_mode: Some(AuthMode::ApiKey), @@ -486,34 +487,6 @@ fn keyring_auth_storage_compute_store_key_for_home_directory() -> anyhow::Result Ok(()) } -#[test] -fn direct_keyring_auth_storage_saves_legacy_keyring_entry() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = DirectKeyringAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - ); - let auth_file = get_auth_file(codex_home.path()); - std::fs::write(&auth_file, "stale")?; - let auth = auth_with_prefix("direct"); - - storage.save(&auth)?; - - let legacy_key = compute_store_key(codex_home.path())?; - let saved_value = mock_keyring - .saved_value(&legacy_key) - .context("direct keyring auth entry should exist")?; - assert_eq!(saved_value, serde_json::to_string(&auth)?); - assert!(!encrypted_auth_file(codex_home.path()).exists()); - assert!( - !auth_file.exists(), - "fallback auth.json should be removed after keyring save" - ); - assert_eq!(storage.load()?, Some(auth)); - Ok(()) -} - #[test] fn direct_keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { let codex_home = tempdir()?; @@ -521,6 +494,7 @@ fn direct_keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Resu let storage = DirectKeyringAuthStorage::new( codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), + AuthCredentialsStoreMode::Keyring, ); let auth = auth_with_prefix("direct-delete"); storage.save(&auth)?; @@ -586,38 +560,6 @@ fn factory_uses_secrets_backend_only_when_requested() -> anyhow::Result<()> { Ok(()) } -#[test] -fn secrets_keyring_auth_storage_save_persists_and_removes_fallback_file() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = SecretsKeyringAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - ); - let auth_file = get_auth_file(codex_home.path()); - std::fs::write(&auth_file, "stale")?; - let auth = AuthDotJson { - auth_mode: Some(AuthMode::Chatgpt), - openai_api_key: None, - tokens: Some(TokenData { - id_token: Default::default(), - access_token: "access".to_string(), - refresh_token: "refresh".to_string(), - account_id: Some("account".to_string()), - }), - last_refresh: Some(Utc::now()), - agent_identity: None, - personal_access_token: None, - bedrock_api_key: None, - bedrock_access_keys: None, - }; - - storage.save(&auth)?; - - assert_keyring_saved_auth_and_removed_fallback(&mock_keyring, codex_home.path(), &auth)?; - Ok(()) -} - #[test] fn secrets_keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { let codex_home = tempdir()?; @@ -625,6 +567,7 @@ fn secrets_keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Res let storage = SecretsKeyringAuthStorage::new( codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), + AuthCredentialsStoreMode::Keyring, ); let auth = auth_with_prefix("to-delete"); let auth_file = seed_secrets_backend_and_fallback_auth_file_for_delete( @@ -651,11 +594,13 @@ fn secrets_keyring_auth_storage_delete_removes_legacy_direct_keyring_entry() -> let direct_storage = DirectKeyringAuthStorage::new( codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), + AuthCredentialsStoreMode::Keyring, ); direct_storage.save(&auth_with_prefix("legacy-direct"))?; let storage = SecretsKeyringAuthStorage::new( codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), + AuthCredentialsStoreMode::Keyring, ); let auth = auth_with_prefix("to-delete"); let auth_file = seed_secrets_backend_and_fallback_auth_file_for_delete( @@ -680,141 +625,8 @@ fn secrets_keyring_auth_storage_delete_removes_legacy_direct_keyring_entry() -> Ok(()) } -#[test] -fn auto_auth_storage_load_prefers_keyring_value() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - AuthKeyringBackendKind::Secrets, - ); - let keyring_auth = auth_with_prefix("keyring"); - seed_secrets_backend_with_auth(&mock_keyring, codex_home.path(), &keyring_auth)?; +#[path = "storage_policy_tests.rs"] +mod policy; - let file_auth = auth_with_prefix("file"); - storage.file_storage.save(&file_auth)?; - - let loaded = storage.load()?; - assert_eq!(loaded, Some(keyring_auth)); - Ok(()) -} - -#[test] -fn auto_auth_storage_load_uses_file_when_keyring_empty() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring), - AuthKeyringBackendKind::Secrets, - ); - - let expected = auth_with_prefix("file-only"); - storage.file_storage.save(&expected)?; - - let loaded = storage.load()?; - assert_eq!(loaded, Some(expected)); - Ok(()) -} - -#[test] -fn auto_auth_storage_load_falls_back_when_keyring_errors() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - AuthKeyringBackendKind::Secrets, - ); - let key = compute_keyring_account(codex_home.path(), LocalSecretsNamespace::CodexAuth); - - let encrypted = auth_with_prefix("encrypted"); - seed_secrets_backend_with_auth(&mock_keyring, codex_home.path(), &encrypted)?; - mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "load".into())); - - let expected = auth_with_prefix("fallback"); - storage.file_storage.save(&expected)?; - - let loaded = storage.load()?; - assert_eq!(loaded, Some(expected)); - Ok(()) -} - -#[test] -fn auto_auth_storage_save_prefers_keyring() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - AuthKeyringBackendKind::Secrets, - ); - let stale = auth_with_prefix("stale"); - storage.file_storage.save(&stale)?; - - let expected = auth_with_prefix("to-save"); - storage.save(&expected)?; - - assert_keyring_saved_auth_and_removed_fallback(&mock_keyring, codex_home.path(), &expected)?; - Ok(()) -} - -#[test] -fn auto_auth_storage_save_falls_back_when_keyring_errors() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - AuthKeyringBackendKind::Secrets, - ); - let key = compute_keyring_account(codex_home.path(), LocalSecretsNamespace::CodexAuth); - mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "save".into())); - - let auth = auth_with_prefix("fallback"); - storage.save(&auth)?; - - let auth_file = get_auth_file(codex_home.path()); - assert!( - auth_file.exists(), - "fallback auth.json should be created when keyring save fails" - ); - let saved = storage - .file_storage - .load()? - .context("fallback auth should exist")?; - assert_eq!(saved, auth); - assert!( - mock_keyring.saved_value(&key).is_none(), - "keyring should not contain value when save fails" - ); - Ok(()) -} - -#[test] -fn auto_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - AuthKeyringBackendKind::Secrets, - ); - let auth = auth_with_prefix("to-delete"); - let auth_file = seed_secrets_backend_and_fallback_auth_file_for_delete( - &mock_keyring, - codex_home.path(), - &auth, - )?; - - let removed = storage.delete()?; - - assert!(removed, "delete should report removal"); - assert_eq!(storage.load()?, None, "encrypted auth should be removed"); - assert!( - !auth_file.exists(), - "fallback auth.json should be removed after delete" - ); - Ok(()) -} +#[path = "storage_error_tests.rs"] +mod errors; diff --git a/codex-rs/login/src/server.rs b/codex-rs/login/src/server.rs index d9dcb32156..d65c95287c 100644 --- a/codex-rs/login/src/server.rs +++ b/codex-rs/login/src/server.rs @@ -61,6 +61,7 @@ use codex_http_client::HttpClient; use codex_http_client::HttpClientBuilder; use codex_http_client::HttpClientFactory; use codex_http_client::OutboundProxyPolicy; +use codex_otel::auth_storage::AuthStorageOriginator; use codex_protocol::auth::AuthMode; use codex_utils_template::Template; use serde_json::Value as JsonValue; @@ -226,7 +227,7 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result { let server_handle = { let shutdown_notify = shutdown_notify.clone(); let server = server; - tokio::spawn(async move { + let task = async move { let mut callback_result = LoginCallbackResult::default(); let result = loop { tokio::select! { @@ -311,7 +312,8 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result { // running `server.recv()` in a loop exits cleanly. server.unblock(); result - }) + }; + tokio::spawn(AuthStorageOriginator::current().scope(task)) }; Ok(LoginServer { @@ -839,38 +841,46 @@ pub(crate) async fn persist_tokens_async( ) -> io::Result<()> { // Reuse existing synchronous logic but run it off the async runtime. let codex_home = codex_home.to_path_buf(); - tokio::task::spawn_blocking(move || { - let mut tokens = TokenData { - id_token: parse_chatgpt_jwt_claims(&id_token).map_err(io::Error::other)?, - access_token, - refresh_token, - account_id: None, - }; - if let Some(acc) = jwt_auth_claims(&id_token) - .get("chatgpt_account_id") - .and_then(|v| v.as_str()) - { - tokens.account_id = Some(acc.to_string()); - } - let auth = AuthDotJson { - auth_mode: Some(AuthMode::Chatgpt), - openai_api_key: api_key, - tokens: Some(tokens), - last_refresh: Some(Utc::now()), - agent_identity: None, - personal_access_token: None, - bedrock_api_key: None, - bedrock_access_keys: None, - }; - save_auth( - &codex_home, - &auth, - auth_credentials_store_mode, - keyring_backend_kind, - ) - }) - .await - .map_err(|e| io::Error::other(format!("persist task failed: {e}")))? + // Blocking tasks don't inherit task-local context. + // Preserve the initiating client for credential-storage metrics. + let originator = AuthStorageOriginator::current(); + + let persist_task = tokio::task::spawn_blocking(move || { + originator.sync_scope(|| { + let mut tokens = TokenData { + id_token: parse_chatgpt_jwt_claims(&id_token).map_err(io::Error::other)?, + access_token, + refresh_token, + account_id: None, + }; + if let Some(acc) = jwt_auth_claims(&id_token) + .get("chatgpt_account_id") + .and_then(|v| v.as_str()) + { + tokens.account_id = Some(acc.to_string()); + } + let auth = AuthDotJson { + auth_mode: Some(AuthMode::Chatgpt), + openai_api_key: api_key, + tokens: Some(tokens), + last_refresh: Some(Utc::now()), + agent_identity: None, + personal_access_token: None, + bedrock_api_key: None, + bedrock_access_keys: None, + }; + save_auth( + &codex_home, + &auth, + auth_credentials_store_mode, + keyring_backend_kind, + ) + }) + }); + + persist_task + .await + .map_err(|e| io::Error::other(format!("persist task failed: {e}")))? } /// Validates the ID token against an optional workspace restriction.