mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[login] revoke existing auth before starting login (#27674)
## Why
`codex login` previously persisted newly issued OAuth credentials and
only then attempted to revoke the superseded refresh token. The old
credential must be revoked before a replacement browser or device-code
flow starts, and successful login must not perform any post-login
revocation attempt.
## What changed
- Revoke and clear existing stored auth before browser or device-code
CLI login begins.
- Remove superseded-token detection and revocation from the shared token
persistence path; successful login now only saves the new credentials.
- Read the raw configured auth store during CLI cleanup so
environment-provided auth cannot mask the stored refresh token.
- Preserve `auto` storage fallback semantics when keyring deletion fails
by clearing the fallback auth file.
- Add a process-level CLI regression test that requires the revoke
request to precede every device-login request and occur exactly once.
If replacement login is canceled or fails, the previous local
credentials have already been cleared. Remote revocation remains best
effort, matching explicit logout behavior.
## Validation
### Process-level before/after reproduction
I compiled the real `codex` CLI from the pre-fix parent (`14df0e8833`)
and from the PR implementation (`25c002f23b`; the login behavior is
unchanged at the current head), then ran the same device-code flow
against a local HTTP mock OAuth authority.
Each run:
1. Used a fresh temporary `CODEX_HOME` configured with
`cli_auth_credentials_store = "file"`.
2. Seeded that temporary home with managed ChatGPT auth containing
`old-access` and `old-refresh` tokens.
3. Pointed `CODEX_REVOKE_TOKEN_URL_OVERRIDE` at the mock `/oauth/revoke`
endpoint.
4. Ran the compiled CLI as:
```shell
CODEX_HOME=<temporary-home> \
CODEX_REVOKE_TOKEN_URL_OVERRIDE=<mock-issuer>/oauth/revoke \
<compiled-codex> login --device-auth --experimental_issuer <mock-issuer>
```
5. Recorded every request received by the mock authority. The mock
marked `new-access` valid when `/oauth/token` issued it and invalidated
it if `/oauth/revoke` arrived afterward, reproducing the observed
session-invalidating failure mode. After login exited, the harness also
verified the persisted refresh token and probed a protected endpoint
with `new-access`.
| Build | Observed request order | CLI/persistence result | `new-access`
probe |
| --- | --- | --- | --- |
| Pre-fix | `usercode → device token → OAuth token →
revoke(old-refresh)` | Exit `0`; `new-refresh` persisted | `401` |
| PR | `revoke(old-refresh) → usercode → device token → OAuth token` |
Exit `0`; `new-refresh` persisted | `200` |
The PR run therefore issued exactly one revocation request, before any
request that initiated the replacement login, and issued no revocation
after token exchange.
### Regression coverage
`codex-rs/cli/tests/login.rs::device_login_revokes_existing_auth_before_requesting_new_tokens`
runs the real first-party `codex` binary against a `wiremock` OAuth
server with an isolated temporary `CODEX_HOME`. It asserts:
- the exact request sequence is `/oauth/revoke`,
`/api/accounts/deviceauth/usercode`, `/api/accounts/deviceauth/token`,
then `/oauth/token`;
- there is exactly one revoke request and its body contains
`old-refresh` with the `refresh_token` hint;
- the completed login persists `new-refresh`.
Local validation:
- `just test -p codex-login` — 130 passed
- `just test -p codex-cli` — 280 passed, including the new process-level
regression test
- `just bazel-lock-check`
This commit is contained in:
committed by
GitHub
Unverified
parent
b724f5966e
commit
d1aaf789ad
@@ -23,6 +23,7 @@ use codex_utils_cli::CliConfigOverrides;
|
||||
use std::fs::OpenOptions;
|
||||
use std::io::IsTerminal;
|
||||
use std::io::Read;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tracing_appender::non_blocking;
|
||||
use tracing_appender::non_blocking::WorkerGuard;
|
||||
@@ -113,11 +114,22 @@ fn print_login_server_start(actual_port: u16, auth_url: &str) {
|
||||
);
|
||||
}
|
||||
|
||||
async fn clear_existing_auth_before_login(
|
||||
codex_home: &Path,
|
||||
auth_credentials_store_mode: AuthCredentialsStoreMode,
|
||||
) {
|
||||
if let Err(err) = logout_with_revoke(codex_home, auth_credentials_store_mode).await {
|
||||
tracing::warn!("failed to clear existing auth before login: {err}");
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn login_with_chatgpt(
|
||||
codex_home: PathBuf,
|
||||
forced_chatgpt_workspace_id: Option<Vec<String>>,
|
||||
cli_auth_credentials_store_mode: AuthCredentialsStoreMode,
|
||||
) -> std::io::Result<()> {
|
||||
clear_existing_auth_before_login(&codex_home, cli_auth_credentials_store_mode).await;
|
||||
|
||||
let opts = ServerOptions::new(
|
||||
codex_home,
|
||||
CLIENT_ID.to_string(),
|
||||
@@ -277,6 +289,8 @@ pub async fn run_login_with_device_code(
|
||||
eprintln!("{CHATGPT_LOGIN_DISABLED_MESSAGE}");
|
||||
std::process::exit(1);
|
||||
}
|
||||
clear_existing_auth_before_login(&config.codex_home, config.cli_auth_credentials_store_mode)
|
||||
.await;
|
||||
let forced_chatgpt_workspace_id = config.forced_chatgpt_workspace_id.clone();
|
||||
let mut opts = ServerOptions::new(
|
||||
config.codex_home.to_path_buf(),
|
||||
@@ -315,6 +329,8 @@ pub async fn run_login_with_device_code_fallback_to_browser(
|
||||
eprintln!("{CHATGPT_LOGIN_DISABLED_MESSAGE}");
|
||||
std::process::exit(1);
|
||||
}
|
||||
clear_existing_auth_before_login(&config.codex_home, config.cli_auth_credentials_store_mode)
|
||||
.await;
|
||||
|
||||
let forced_chatgpt_workspace_id = config.forced_chatgpt_workspace_id.clone();
|
||||
let mut opts = ServerOptions::new(
|
||||
@@ -460,8 +476,32 @@ fn safe_format_key(key: &str) -> String {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use codex_config::types::AuthCredentialsStoreMode;
|
||||
use codex_login::load_auth_dot_json;
|
||||
use codex_login::login_with_api_key;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::tempdir;
|
||||
|
||||
use super::clear_existing_auth_before_login;
|
||||
use super::safe_format_key;
|
||||
|
||||
#[tokio::test]
|
||||
async fn clears_existing_auth_before_login() {
|
||||
let codex_home = tempdir().expect("create temporary Codex home");
|
||||
login_with_api_key(
|
||||
codex_home.path(),
|
||||
"sk-existing",
|
||||
AuthCredentialsStoreMode::File,
|
||||
)
|
||||
.expect("save existing auth");
|
||||
|
||||
clear_existing_auth_before_login(codex_home.path(), AuthCredentialsStoreMode::File).await;
|
||||
|
||||
let auth = load_auth_dot_json(codex_home.path(), AuthCredentialsStoreMode::File)
|
||||
.expect("load auth after cleanup");
|
||||
assert_eq!(auth, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn formats_long_key() {
|
||||
let key = "sk-proj-1234567890ABCDE";
|
||||
|
||||
Reference in New Issue
Block a user