From 900bb014860d87651dd1da011d9daf1860bdfeab Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sun, 14 Sep 2025 19:48:18 -0700 Subject: [PATCH] When logging in using ChatGPT, make sure to overwrite API key (#3611) When logging in using ChatGPT using the `codex login` command, a successful login should write a new `auth.json` file with the ChatGPT token information. The old code attempted to retain the API key and merge the token information into the existing `auth.json` file. With the new simplified login mechanism, `auth.json` should have auth information for only ChatGPT or API Key, not both. The `codex login --api-key ` code path was already doing the right thing here, but the `codex login` command was incorrect. This PR fixes the problem and adds test cases for both commands. --- codex-rs/core/src/auth.rs | 26 +++++++++++ codex-rs/login/src/server.rs | 44 +++++++------------ .../login/tests/suite/login_server_e2e.rs | 24 ++++++++-- 3 files changed, 61 insertions(+), 33 deletions(-) diff --git a/codex-rs/core/src/auth.rs b/codex-rs/core/src/auth.rs index 09ad13b4..a2158310 100644 --- a/codex-rs/core/src/auth.rs +++ b/codex-rs/core/src/auth.rs @@ -408,6 +408,32 @@ mod tests { assert_eq!(auth_dot_json, same_auth_dot_json); } + #[test] + fn login_with_api_key_overwrites_existing_auth_json() { + let dir = tempdir().unwrap(); + let auth_path = dir.path().join("auth.json"); + let stale_auth = json!({ + "OPENAI_API_KEY": "sk-old", + "tokens": { + "id_token": "stale.header.payload", + "access_token": "stale-access", + "refresh_token": "stale-refresh", + "account_id": "stale-acc" + } + }); + std::fs::write( + &auth_path, + serde_json::to_string_pretty(&stale_auth).unwrap(), + ) + .unwrap(); + + super::login_with_api_key(dir.path(), "sk-new").expect("login_with_api_key should succeed"); + + let auth = super::try_read_auth_json(&auth_path).expect("auth.json should parse"); + assert_eq!(auth.openai_api_key.as_deref(), Some("sk-new")); + assert!(auth.tokens.is_none(), "tokens should be cleared"); + } + #[tokio::test] async fn pro_account_with_no_api_key_uses_chatgpt_auth() { let codex_home = tempdir().unwrap(); diff --git a/codex-rs/login/src/server.rs b/codex-rs/login/src/server.rs index 2cf1189f..26f341b9 100644 --- a/codex-rs/login/src/server.rs +++ b/codex-rs/login/src/server.rs @@ -238,8 +238,8 @@ async fn process_request( &opts.codex_home, api_key.clone(), tokens.id_token.clone(), - Some(tokens.access_token.clone()), - Some(tokens.refresh_token.clone()), + tokens.access_token.clone(), + tokens.refresh_token.clone(), ) .await { @@ -446,8 +446,8 @@ async fn persist_tokens_async( codex_home: &Path, api_key: Option, id_token: String, - access_token: Option, - refresh_token: Option, + access_token: String, + refresh_token: String, ) -> io::Result<()> { // Reuse existing synchronous logic but run it off the async runtime. let codex_home = codex_home.to_path_buf(); @@ -459,43 +459,29 @@ async fn persist_tokens_async( std::fs::create_dir_all(parent).map_err(io::Error::other)?; } - let mut auth = read_or_default(&auth_file); - if let Some(key) = api_key { - auth.openai_api_key = Some(key); - } - let tokens = auth.tokens.get_or_insert_with(TokenData::default); - tokens.id_token = parse_id_token(&id_token).map_err(io::Error::other)?; - // Persist chatgpt_account_id if present in claims + let mut tokens = TokenData { + id_token: parse_id_token(&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()); } - if let Some(at) = access_token { - tokens.access_token = at; - } - if let Some(rt) = refresh_token { - tokens.refresh_token = rt; - } - auth.last_refresh = Some(Utc::now()); + let auth = AuthDotJson { + openai_api_key: api_key, + tokens: Some(tokens), + last_refresh: Some(Utc::now()), + }; codex_core::auth::write_auth_json(&auth_file, &auth) }) .await .map_err(|e| io::Error::other(format!("persist task failed: {e}")))? } -fn read_or_default(path: &Path) -> AuthDotJson { - match codex_core::auth::try_read_auth_json(path) { - Ok(auth) => auth, - Err(_) => AuthDotJson { - openai_api_key: None, - tokens: None, - last_refresh: None, - }, - } -} - fn compose_success_url(port: u16, issuer: &str, id_token: &str, access_token: &str) -> String { let token_claims = jwt_auth_claims(id_token); let access_claims = jwt_auth_claims(access_token); diff --git a/codex-rs/login/tests/suite/login_server_e2e.rs b/codex-rs/login/tests/suite/login_server_e2e.rs index 9309ef41..dd2cb904 100644 --- a/codex-rs/login/tests/suite/login_server_e2e.rs +++ b/codex-rs/login/tests/suite/login_server_e2e.rs @@ -90,6 +90,22 @@ async fn end_to_end_login_flow_persists_auth_json() { let tmp = tempdir().unwrap(); let codex_home = tmp.path().to_path_buf(); + // Seed auth.json with stale API key + tokens that should be overwritten. + let stale_auth = serde_json::json!({ + "OPENAI_API_KEY": "sk-stale", + "tokens": { + "id_token": "stale.header.payload", + "access_token": "stale-access", + "refresh_token": "stale-refresh", + "account_id": "stale-acc" + } + }); + std::fs::write( + codex_home.join("auth.json"), + serde_json::to_string_pretty(&stale_auth).unwrap(), + ) + .unwrap(); + let state = "test_state_123".to_string(); // Run server in background @@ -122,10 +138,10 @@ async fn end_to_end_login_flow_persists_auth_json() { let auth_path = codex_home.join("auth.json"); let data = std::fs::read_to_string(&auth_path).unwrap(); let json: serde_json::Value = serde_json::from_str(&data).unwrap(); - assert!( - !json["OPENAI_API_KEY"].is_null(), - "OPENAI_API_KEY should be set" - ); + // The following assert is here because of the old oauth flow that exchanges tokens for an + // API key. See obtain_api_key in server.rs for details. Once we remove this old mechanism + // from the code, this test should be updated to expect that the API key is no longer present. + assert_eq!(json["OPENAI_API_KEY"], "access-123"); assert_eq!(json["tokens"]["access_token"], "access-123"); assert_eq!(json["tokens"]["refresh_token"], "refresh-123"); assert_eq!(json["tokens"]["account_id"], "acc-123");