From 21568c9eecea427754366b452b47e066054dc180 Mon Sep 17 00:00:00 2001 From: shiny-code-bot Date: Wed, 17 Jun 2026 10:39:58 -0400 Subject: [PATCH] fix: harden auth profile storage --- codex-rs/login/src/auth/auth_tests.rs | 62 +++++++++++- codex-rs/login/src/auth/manager.rs | 2 +- codex-rs/login/src/auth_profiles.rs | 130 +++++++++++++++++++++++--- codex-rs/login/tests/suite/logout.rs | 51 ++++++++++ 4 files changed, 226 insertions(+), 19 deletions(-) diff --git a/codex-rs/login/src/auth/auth_tests.rs b/codex-rs/login/src/auth/auth_tests.rs index e7b5502ebcd6..edef054e785b 100644 --- a/codex-rs/login/src/auth/auth_tests.rs +++ b/codex-rs/login/src/auth/auth_tests.rs @@ -795,7 +795,37 @@ fn remove_access_token_env_var() -> EnvVarGuard { #[tokio::test] #[serial(codex_auth_env)] -async fn load_auth_reads_access_token_from_env() { +async fn load_auth_ignores_access_token_env_when_env_auth_disabled() { + let codex_home = tempdir().unwrap(); + let expected_record = agent_identity_record(WORKSPACE_ID_ALLOWED); + let agent_identity = + signed_agent_identity_jwt(&expected_record, json!(expected_record.plan_type)) + .expect("signed agent identity"); + let _access_token_guard = EnvVarGuard::set(CODEX_ACCESS_TOKEN_ENV_VAR, &agent_identity); + + let server = MockServer::start().await; + let chatgpt_base_url = format!("{}/backend-api", server.uri()); + let _authapi_guard = + EnvVarGuard::set("CODEX_AGENT_IDENTITY_AUTHAPI_BASE_URL", &chatgpt_base_url); + let auth = super::load_auth( + codex_home.path(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + Some(&chatgpt_base_url), + ) + .await + .expect("auth load should succeed"); + + assert_eq!(auth, None); + assert!( + !get_auth_file(codex_home.path()).exists(), + "env auth should not write auth.json" + ); +} + +#[tokio::test] +#[serial(codex_auth_env)] +async fn load_auth_reads_access_token_from_env_when_env_auth_enabled() { let codex_home = tempdir().unwrap(); let expected_record = agent_identity_record(WORKSPACE_ID_ALLOWED); let agent_identity = @@ -823,7 +853,7 @@ async fn load_auth_reads_access_token_from_env() { EnvVarGuard::set("CODEX_AGENT_IDENTITY_AUTHAPI_BASE_URL", &chatgpt_base_url); let auth = super::load_auth( codex_home.path(), - /*enable_codex_api_key_env*/ false, + /*enable_codex_api_key_env*/ true, AuthCredentialsStoreMode::File, Some(&chatgpt_base_url), ) @@ -845,7 +875,29 @@ async fn load_auth_reads_access_token_from_env() { #[tokio::test] #[serial(codex_auth_env)] -async fn load_auth_reads_personal_access_token_from_env() { +async fn load_auth_ignores_personal_access_token_env_when_env_auth_disabled() { + let codex_home = tempdir().unwrap(); + let _access_token_guard = EnvVarGuard::set(CODEX_ACCESS_TOKEN_ENV_VAR, "at-env-test"); + + let auth = super::load_auth( + codex_home.path(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*chatgpt_base_url*/ None, + ) + .await + .expect("auth load should succeed"); + + assert_eq!(auth, None); + assert!( + !get_auth_file(codex_home.path()).exists(), + "env auth should not write auth.json" + ); +} + +#[tokio::test] +#[serial(codex_auth_env)] +async fn load_auth_reads_personal_access_token_from_env_when_env_auth_enabled() { let codex_home = tempdir().unwrap(); let server = MockServer::start().await; Mock::given(method("GET")) @@ -867,7 +919,7 @@ async fn load_auth_reads_personal_access_token_from_env() { ] { let auth = super::load_auth( codex_home.path(), - /*enable_codex_api_key_env*/ false, + /*enable_codex_api_key_env*/ true, auth_credentials_store_mode, /*chatgpt_base_url*/ None, ) @@ -917,7 +969,7 @@ async fn personal_access_token_does_not_offer_unauthorized_recovery() { let manager = Arc::new( AuthManager::new( codex_home.path().to_path_buf(), - /*enable_codex_api_key_env*/ false, + /*enable_codex_api_key_env*/ true, AuthCredentialsStoreMode::File, /*chatgpt_base_url*/ None, ) diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index 9e255a99a3eb..ef67965a1783 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -828,7 +828,7 @@ async fn load_auth( return Ok(Some(auth)); } - if let Some(access_token) = read_codex_access_token_from_env() { + if enable_codex_api_key_env && let Some(access_token) = read_codex_access_token_from_env() { return match classify_codex_access_token(&access_token) { CodexAccessToken::PersonalAccessToken(access_token) => { CodexAuth::from_personal_access_token(access_token) diff --git a/codex-rs/login/src/auth_profiles.rs b/codex-rs/login/src/auth_profiles.rs index 4403a99d7635..1e093a73be6a 100644 --- a/codex-rs/login/src/auth_profiles.rs +++ b/codex-rs/login/src/auth_profiles.rs @@ -124,25 +124,105 @@ pub fn load_auth_profiles(codex_home: &Path) -> io::Result { pub fn save_auth_profiles(codex_home: &Path, profiles: &AuthProfilesFile) -> io::Result<()> { fs::create_dir_all(codex_home)?; let path = profile_metadata_path(codex_home); - let tmp_path = path.with_extension("json.tmp"); let raw = serde_json::to_string_pretty(profiles).map_err(io::Error::other)?; - let mut options = OpenOptions::new(); - options.write(true).create(true).truncate(true); - - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - options.mode(0o600); - } - - let mut file = options.open(&tmp_path)?; + let (tmp_path, mut file) = create_metadata_tmp_file(&path)?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; file.set_permissions(fs::Permissions::from_mode(0o600))?; } file.write_all(raw.as_bytes())?; - fs::rename(tmp_path, path)?; + file.flush()?; + drop(file); + if let Err(err) = replace_metadata_file(&tmp_path, &path) { + let _ = fs::remove_file(&tmp_path); + return Err(err); + } + Ok(()) +} + +fn create_metadata_tmp_file(path: &Path) -> io::Result<(PathBuf, fs::File)> { + let file_name = path + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or(PROFILE_METADATA_FILE); + let parent = path.parent().unwrap_or_else(|| Path::new(".")); + for attempt in 0..100 { + let tmp_path = parent.join(format!( + ".{file_name}.{}.{}.tmp", + std::process::id(), + attempt + )); + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + match options.open(&tmp_path) { + Ok(file) => return Ok((tmp_path, file)), + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => continue, + Err(err) => return Err(err), + } + } + Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "failed to allocate temporary metadata path for {}", + path.display() + ), + )) +} + +fn replace_metadata_file(src: &Path, dst: &Path) -> io::Result<()> { + #[cfg(not(windows))] + { + fs::rename(src, dst) + } + #[cfg(windows)] + { + match replace_file_windows(src, dst) { + Ok(()) => Ok(()), + Err(err) if err.kind() == io::ErrorKind::NotFound => fs::rename(src, dst), + Err(err) => Err(err), + } + } +} + +#[cfg(windows)] +fn replace_file_windows(src: &Path, dst: &Path) -> io::Result<()> { + use std::ffi::c_void; + use std::os::windows::ffi::OsStrExt; + + const REPLACEFILE_IGNORE_MERGE_ERRORS: u32 = 0x0000_0002; + + unsafe extern "system" { + fn ReplaceFileW( + lp_replaced_file_name: *const u16, + lp_replacement_file_name: *const u16, + lp_backup_file_name: *const u16, + dw_replace_flags: u32, + lp_exclude: *mut c_void, + lp_reserved: *mut c_void, + ) -> i32; + } + + let dst_wide: Vec = dst.as_os_str().encode_wide().chain(Some(0)).collect(); + let src_wide: Vec = src.as_os_str().encode_wide().chain(Some(0)).collect(); + let replaced = unsafe { + ReplaceFileW( + dst_wide.as_ptr(), + src_wide.as_ptr(), + std::ptr::null(), + REPLACEFILE_IGNORE_MERGE_ERRORS, + std::ptr::null_mut(), + std::ptr::null_mut(), + ) + }; + if replaced == 0 { + return Err(io::Error::last_os_error()); + } Ok(()) } @@ -252,6 +332,27 @@ mod tests { assert_eq!(profiles[0].metadata.priming_enabled, Some(true)); } + #[test] + fn save_profiles_replaces_existing_metadata() { + let temp = TempDir::new().expect("tempdir"); + + upsert_auth_profile(temp.path(), "work", |metadata| { + metadata.email = Some("work@example.com".to_string()); + }) + .expect("first upsert"); + upsert_auth_profile(temp.path(), "work", |metadata| { + metadata.email = Some("updated@example.com".to_string()); + }) + .expect("second upsert"); + + let profiles = load_auth_profiles(temp.path()).expect("load profiles"); + assert_eq!(profiles.profiles.len(), 1); + assert_eq!( + profiles.profiles["work"].email.as_deref(), + Some("updated@example.com") + ); + } + #[cfg(unix)] #[test] fn saved_profiles_metadata_is_private() { @@ -275,7 +376,9 @@ mod tests { use std::os::unix::fs::PermissionsExt; let temp = TempDir::new().expect("tempdir"); - let tmp_path = profile_metadata_path(temp.path()).with_extension("json.tmp"); + let tmp_path = temp + .path() + .join(format!(".auth-profiles.json.{}.0.tmp", std::process::id())); fs::write(&tmp_path, "{}").expect("write stale tmp"); fs::set_permissions(&tmp_path, fs::Permissions::from_mode(0o644)) .expect("make stale tmp permissive"); @@ -288,6 +391,7 @@ mod tests { .mode() & 0o777; assert_eq!(mode, 0o600); + assert!(tmp_path.exists()); } #[test] diff --git a/codex-rs/login/tests/suite/logout.rs b/codex-rs/login/tests/suite/logout.rs index 2dd1bef83fdb..b55972b1a050 100644 --- a/codex-rs/login/tests/suite/logout.rs +++ b/codex-rs/login/tests/suite/logout.rs @@ -6,6 +6,7 @@ use codex_config::types::AuthCredentialsStoreMode; use codex_login::AuthDotJson; use codex_login::AuthManager; use codex_login::CLIENT_ID; +use codex_login::CODEX_ACCESS_TOKEN_ENV_VAR; use codex_login::REVOKE_TOKEN_URL_OVERRIDE_ENV_VAR; use codex_login::logout_with_revoke; use codex_login::save_auth; @@ -113,6 +114,56 @@ async fn logout_with_revoke_removes_auth_when_revoke_fails() -> Result<()> { Ok(()) } +#[serial_test::serial(logout_revoke)] +#[tokio::test] +async fn logout_with_revoke_ignores_access_token_env() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/revoke")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "message": "success" + }))) + .expect(1) + .mount(&server) + .await; + let _revoke_guard = EnvGuard::set( + REVOKE_TOKEN_URL_OVERRIDE_ENV_VAR, + format!("{}/oauth/revoke", server.uri()), + ); + let _access_token_guard = EnvGuard::set(CODEX_ACCESS_TOKEN_ENV_VAR, "at-env-test".to_string()); + + let codex_home = TempDir::new()?; + save_auth( + codex_home.path(), + &chatgpt_auth_with_refresh_token("profile-refresh-token"), + AuthCredentialsStoreMode::File, + )?; + + let removed = logout_with_revoke(codex_home.path(), AuthCredentialsStoreMode::File).await?; + + assert!(removed); + assert!(!codex_home.path().join("auth.json").exists()); + let requests = server + .received_requests() + .await + .context("failed to fetch revoke requests")?; + assert_eq!(requests.len(), 1); + assert_eq!( + requests[0] + .body_json::() + .context("revoke request should be JSON")?, + json!({ + "token": "profile-refresh-token", + "token_type_hint": "refresh_token", + "client_id": CLIENT_ID, + }) + ); + server.verify().await; + Ok(()) +} + #[serial_test::serial(logout_revoke)] #[tokio::test] async fn auth_manager_logout_with_revoke_uses_cached_auth() -> Result<()> {