From 6e80d8b8f2bbac1f07e4a3c376ebb2dd5d2ed8f0 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 13 Apr 2026 11:29:01 -0400 Subject: [PATCH] Fix OpenAI Codex OAuth device auth Align the OpenAI device-auth flow with the live Codex endpoints and allow API-backed OpenAI resolution to fall back to the stored openai_codex credential. This makes provider login work against the current OpenAI response shape and lets doctor/workflows use the saved credential without OPENAI_API_KEY in the environment. --- lib/crates/fabro-auth/src/resolve.rs | 36 +- .../fabro-auth/src/strategies/codex_device.rs | 349 ++++++++++++++++-- lib/crates/fabro-auth/src/strategy.rs | 4 +- 3 files changed, 347 insertions(+), 42 deletions(-) diff --git a/lib/crates/fabro-auth/src/resolve.rs b/lib/crates/fabro-auth/src/resolve.rs index 604aa3262..c99ab41ab 100644 --- a/lib/crates/fabro-auth/src/resolve.rs +++ b/lib/crates/fabro-auth/src/resolve.rs @@ -272,7 +272,7 @@ fn credential_ids_for(provider: Provider, usage: CredentialUsage) -> &'static [& (Provider::OpenAi, CredentialUsage::CliAgent(CliAgentKind::Codex)) => { &["openai_codex", "openai"] } - (Provider::OpenAi, _) => &["openai"], + (Provider::OpenAi, _) => &["openai", "openai_codex"], (Provider::Anthropic, _) => &["anthropic"], (Provider::Gemini, _) => &["gemini"], (Provider::Kimi, _) => &["kimi"], @@ -357,6 +357,40 @@ mod tests { ); } + #[tokio::test] + async fn resolve_openai_api_request_falls_back_to_codex_oauth_credential() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault_set_credential( + &mut vault, + "openai_codex", + &oauth_credential( + "https://auth.openai.com/oauth/token".to_string(), + Utc::now() + Duration::hours(1), + ), + ) + .unwrap(); + let resolver = test_resolver(vault, Arc::new(|_| None)); + + let resolved = resolver + .resolve(Provider::OpenAi, CredentialUsage::ApiRequest) + .await + .unwrap(); + + let ResolvedCredential::Api(api) = resolved else { + panic!("expected api credential"); + }; + assert_eq!( + api.auth_header, + ApiKeyHeader::Bearer("expired-access".to_string()) + ); + assert!(api.codex_mode); + assert_eq!( + api.base_url.as_deref(), + Some("https://chatgpt.com/backend-api/codex") + ); + } + #[tokio::test] async fn resolve_returns_not_configured_for_missing_provider() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-auth/src/strategies/codex_device.rs b/lib/crates/fabro-auth/src/strategies/codex_device.rs index d5d5442ec..3a7197a90 100644 --- a/lib/crates/fabro-auth/src/strategies/codex_device.rs +++ b/lib/crates/fabro-auth/src/strategies/codex_device.rs @@ -3,6 +3,7 @@ use std::time::{Duration, Instant}; use async_trait::async_trait; use base64::Engine; use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use chrono::{DateTime, Utc}; use fabro_http::HttpClient; use serde::{Deserialize, Serialize}; use serde_json::json; @@ -14,8 +15,8 @@ use crate::credential::{ }; use crate::strategy::AuthStrategy; -const DEVICE_AUTH_TIMEOUT: Duration = Duration::from_secs(15 * 60); const DEVICE_AUTH_POLL_INTERVAL: Duration = Duration::from_secs(2); +const CODEX_DEVICE_VERIFICATION_URI: &str = "https://auth.openai.com/codex/device"; fn http_client() -> anyhow::Result { #[cfg(test)] @@ -80,14 +81,32 @@ pub fn extract_chatgpt_account_id(id_token: &str) -> Option { }) } +#[derive(Debug, Deserialize)] +#[serde(untagged)] +enum U64OrString { + U64(u64), + String(String), +} + +impl U64OrString { + fn as_u64(&self) -> Option { + match self { + Self::U64(value) => Some(*value), + Self::String(value) => value.parse().ok(), + } + } +} + #[derive(Debug, Deserialize)] struct DeviceCodeInitResponse { - device_auth_id: String, - user_code: String, - #[serde(alias = "verificationUrl", alias = "verification_uri")] - verification_uri: String, + device_auth_id: String, + user_code: String, #[serde(default)] - expires_in: Option, + interval: Option, + #[serde(default)] + expires_in: Option, + #[serde(default)] + expires_at: Option>, } #[derive(Debug, Deserialize)] @@ -96,21 +115,34 @@ struct DeviceCodePollResponse { status: Option, #[serde(default)] authorization_code: Option, + #[serde(default)] + code_verifier: Option, } #[derive(Debug, Serialize)] struct DeviceCodeInitRequest<'a> { - client_id: &'a str, + client_id: &'a str, #[serde(skip_serializing_if = "Option::is_none")] - code_challenge: Option<&'a str>, - #[serde(skip_serializing_if = "Option::is_none")] - scope: Option, + scope: Option, +} + +#[derive(Debug)] +struct PendingDeviceAuth { + device_auth_id: String, + user_code: String, + poll_interval: Duration, + deadline: Instant, +} + +#[derive(Debug)] +struct DeviceAuthorization { + authorization_code: String, + code_verifier: String, } pub struct CodexDeviceStrategy { - config: OAuthConfig, - device_auth_id: Option, - code_verifier: Option, + config: OAuthConfig, + pending: Option, } impl CodexDeviceStrategy { @@ -118,27 +150,59 @@ impl CodexDeviceStrategy { pub fn new(config: OAuthConfig) -> Self { Self { config, - device_auth_id: None, - code_verifier: None, + pending: None, } } - async fn poll_codex_device(&self, device_auth_id: &str) -> anyhow::Result { + fn init_expiry_seconds(payload: &DeviceCodeInitResponse) -> u64 { + if let Some(expires_at) = payload.expires_at { + let seconds = expires_at.signed_duration_since(Utc::now()).num_seconds(); + if seconds > 0 { + return seconds.cast_unsigned(); + } + } + if let Some(seconds) = payload.expires_in.as_ref().and_then(U64OrString::as_u64) { + return seconds; + } + 300 + } + + fn init_poll_interval(payload: &DeviceCodeInitResponse) -> Duration { + Duration::from_secs( + payload + .interval + .as_ref() + .and_then(U64OrString::as_u64) + .unwrap_or(DEVICE_AUTH_POLL_INTERVAL.as_secs()), + ) + } + + async fn poll_codex_device( + &self, + pending: &PendingDeviceAuth, + ) -> anyhow::Result { let client = http_client()?; - let deadline = Instant::now() + DEVICE_AUTH_TIMEOUT; let url = join_url(&self.config.auth_url, "/api/accounts/deviceauth/token"); loop { - if Instant::now() >= deadline { + if Instant::now() >= pending.deadline { return Err(anyhow::anyhow!("device auth timed out after 15 minutes")); } let response = client .post(&url) - .json(&json!({ "device_auth_id": device_auth_id })) + .header("originator", "fabro") + .json(&json!({ + "device_auth_id": pending.device_auth_id, + "user_code": pending.user_code, + })) .send() .await?; let status = response.status(); + if status.as_u16() == 403 || status.as_u16() == 404 { + sleep(pending.poll_interval).await; + continue; + } if !status.is_success() { let body = response.text().await.unwrap_or_default(); return Err(anyhow::anyhow!( @@ -147,17 +211,27 @@ impl CodexDeviceStrategy { } let payload: DeviceCodePollResponse = response.json().await?; - if let Some(code) = payload.authorization_code { - return Ok(code); + if let (Some(authorization_code), Some(code_verifier)) = + (payload.authorization_code, payload.code_verifier) + { + return Ok(DeviceAuthorization { + authorization_code, + code_verifier, + }); } match payload.status.as_deref() { - Some("pending" | "running") | None => { - sleep(DEVICE_AUTH_POLL_INTERVAL).await; + Some("pending" | "running") => { + sleep(pending.poll_interval).await; } Some(other) => { return Err(anyhow::anyhow!("device code exchange failed: {other}")); } + None => { + return Err(anyhow::anyhow!( + "device auth response missing authorization_code or code_verifier" + )); + } } } } @@ -166,18 +240,14 @@ impl CodexDeviceStrategy { #[async_trait] impl AuthStrategy for CodexDeviceStrategy { async fn init(&mut self) -> anyhow::Result { - let pkce = self.config.use_pkce.then(fabro_oauth::generate_pkce); - self.code_verifier = pkce.as_ref().map(|codes| codes.verifier.clone()); - let client = http_client()?; let url = join_url(&self.config.auth_url, "/api/accounts/deviceauth/usercode"); let response = client .post(&url) + .header("originator", "fabro") .json(&DeviceCodeInitRequest { - client_id: &self.config.client_id, - code_challenge: pkce.as_ref().map(|codes| codes.challenge.as_str()), - scope: (!self.config.scopes.is_empty()) - .then(|| self.config.scopes.join(" ")), + client_id: &self.config.client_id, + scope: (!self.config.scopes.is_empty()).then(|| self.config.scopes.join(" ")), }) .send() .await?; @@ -190,12 +260,19 @@ impl AuthStrategy for CodexDeviceStrategy { } let payload: DeviceCodeInitResponse = response.json().await?; - self.device_auth_id = Some(payload.device_auth_id); + let expires_in = Self::init_expiry_seconds(&payload); + let poll_interval = Self::init_poll_interval(&payload); + self.pending = Some(PendingDeviceAuth { + device_auth_id: payload.device_auth_id, + user_code: payload.user_code.clone(), + poll_interval, + deadline: Instant::now() + Duration::from_secs(expires_in), + }); Ok(AuthContextRequest::DeviceCode { - user_code: payload.user_code, - verification_uri: payload.verification_uri, - expires_in: payload.expires_in.unwrap_or(900), + user_code: payload.user_code, + verification_uri: CODEX_DEVICE_VERIFICATION_URI.to_string(), + expires_in, }) } @@ -205,19 +282,19 @@ impl AuthStrategy for CodexDeviceStrategy { "expected device code confirmation response" )), AuthContextResponse::DeviceCodeConfirmed => { - let device_auth_id = self - .device_auth_id + let pending = self + .pending .take() .ok_or_else(|| anyhow::anyhow!("device auth flow was not initialized"))?; - let authorization_code = self.poll_codex_device(&device_auth_id).await?; + let authorization = self.poll_codex_device(&pending).await?; let token_response = fabro_oauth::exchange_code( fabro_oauth::OAuthEndpoint { token_url: &self.config.token_url, client_id: &self.config.client_id, }, - &authorization_code, + &authorization.authorization_code, self.config.redirect_uri.as_deref(), - self.code_verifier.as_deref(), + Some(&authorization.code_verifier), ) .await .map_err(anyhow::Error::msg)?; @@ -244,7 +321,27 @@ impl AuthStrategy for CodexDeviceStrategy { #[cfg(test)] mod tests { + use httpmock::Method::POST; + use httpmock::MockServer; + use super::*; + use crate::strategy::codex_oauth_config; + + fn test_config(server: &MockServer) -> OAuthConfig { + OAuthConfig { + auth_url: server.url(""), + token_url: server.url("/oauth/token"), + client_id: "test-client".to_string(), + scopes: vec![ + "openid".to_string(), + "profile".to_string(), + "email".to_string(), + "offline_access".to_string(), + ], + redirect_uri: Some("https://auth.openai.com/deviceauth/callback".to_string()), + use_pkce: false, + } + } fn make_test_jwt(claims: &serde_json::Value) -> String { let header = URL_SAFE_NO_PAD.encode(r#"{"alg":"RS256"}"#); @@ -280,4 +377,178 @@ mod tests { })); assert_eq!(extract_chatgpt_account_id(&jwt).as_deref(), Some("org_123")); } + + #[test] + fn codex_oauth_config_disables_pkce() { + let config = codex_oauth_config(); + assert!(!config.use_pkce); + } + + #[tokio::test] + async fn init_accepts_live_openai_shape_and_uses_hardcoded_verification_url() { + let server = MockServer::start_async().await; + let init_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/accounts/deviceauth/usercode") + .header("content-type", "application/json") + .header("originator", "fabro") + .json_body(serde_json::json!({ + "client_id": "test-client", + "scope": "openid profile email offline_access" + })); + then.status(200) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "device_auth_id": "deviceauth_123", + "user_code": "ABCD-EFGH", + "interval": "5", + "expires_at": "2026-04-13T15:00:23.011951+00:00" + }) + .to_string(), + ); + }) + .await; + + let mut strategy = CodexDeviceStrategy::new(test_config(&server)); + + let request = strategy.init().await.unwrap(); + + assert_eq!(request, AuthContextRequest::DeviceCode { + user_code: "ABCD-EFGH".to_string(), + verification_uri: "https://auth.openai.com/codex/device".to_string(), + expires_in: 300, + }); + init_mock.assert_async().await; + } + + #[tokio::test] + async fn complete_retries_403_and_uses_server_code_verifier() { + let server = MockServer::start_async().await; + let init_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/accounts/deviceauth/usercode") + .header("originator", "fabro") + .header("content-type", "application/json") + .json_body(serde_json::json!({ + "client_id": "test-client", + "scope": "openid profile email offline_access" + })); + then.status(200) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "device_auth_id": "deviceauth_123", + "user_code": "ABCD-EFGH", + "interval": "0", + "expires_at": "2026-04-13T15:00:23.011951+00:00" + }) + .to_string(), + ); + }) + .await; + let pending_poll_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/accounts/deviceauth/token") + .header("content-type", "application/json") + .header("originator", "fabro") + .json_body(serde_json::json!({ + "device_auth_id": "deviceauth_123", + "user_code": "ABCD-EFGH" + })); + then.status(403) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "error": { + "message": "authorization pending", + "code": "authorization_pending" + } + }) + .to_string(), + ); + }) + .await; + let success_poll_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/accounts/deviceauth/token") + .header("content-type", "application/json") + .header("originator", "fabro") + .json_body(serde_json::json!({ + "device_auth_id": "deviceauth_123", + "user_code": "ABCD-EFGH" + })); + then.status(200) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "authorization_code": "auth-code-123", + "code_verifier": "returned-verifier-456" + }) + .to_string(), + ); + }) + .await; + let token_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/oauth/token") + .header("content-type", "application/x-www-form-urlencoded") + .form_urlencoded_tuple("grant_type", "authorization_code") + .form_urlencoded_tuple("code", "auth-code-123") + .form_urlencoded_tuple( + "redirect_uri", + "https://auth.openai.com/deviceauth/callback", + ) + .form_urlencoded_tuple("client_id", "test-client") + .form_urlencoded_tuple("code_verifier", "returned-verifier-456"); + then.status(200) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "access_token": "new-access-token", + "refresh_token": "new-refresh-token", + "expires_in": 3600, + "id_token": make_test_jwt(&json!({ + "chatgpt_account_id": "acct_123" + })) + }) + .to_string(), + ); + }) + .await; + + let mut strategy = CodexDeviceStrategy::new(test_config(&server)); + strategy.init().await.unwrap(); + + let complete = tokio::spawn(async move { + strategy + .complete(AuthContextResponse::DeviceCodeConfirmed) + .await + }); + while pending_poll_mock.calls_async().await == 0 { + sleep(Duration::from_millis(10)).await; + } + assert!(pending_poll_mock.calls_async().await > 0); + pending_poll_mock.delete_async().await; + + let credential = complete.await.unwrap().unwrap(); + + let AuthDetails::CodexOAuth { + tokens, account_id, .. + } = credential.details + else { + panic!("expected codex oauth credential"); + }; + assert_eq!(tokens.access_token, "new-access-token"); + assert_eq!(tokens.refresh_token.as_deref(), Some("new-refresh-token")); + assert_eq!(account_id.as_deref(), Some("acct_123")); + init_mock.assert_async().await; + success_poll_mock.assert_async().await; + token_mock.assert_async().await; + } } diff --git a/lib/crates/fabro-auth/src/strategy.rs b/lib/crates/fabro-auth/src/strategy.rs index 2ade66ad1..1d28afccc 100644 --- a/lib/crates/fabro-auth/src/strategy.rs +++ b/lib/crates/fabro-auth/src/strategy.rs @@ -35,7 +35,7 @@ pub fn codex_oauth_config() -> OAuthConfig { "offline_access".to_string(), ], redirect_uri: Some(format!("{CODEX_AUTH_URL}/deviceauth/callback")), - use_pkce: true, + use_pkce: false, } } @@ -65,7 +65,7 @@ mod tests { assert_eq!(config.auth_url, CODEX_AUTH_URL); assert_eq!(config.token_url, CODEX_TOKEN_URL); assert_eq!(config.client_id, CODEX_CLIENT_ID); - assert!(config.use_pkce); + assert!(!config.use_pkce); assert!(config.scopes.contains(&"offline_access".to_string())); }