From 0c58612c3c178428f38c907912cd52888ae5d6b9 Mon Sep 17 00:00:00 2001 From: cooper-oai Date: Sat, 7 Nov 2026 01:50:05 +0000 Subject: [PATCH] Fix authentication for git-attribution settings requests (#60035) ## Why Git-attribution settings requests need to honor runtime authentication for their destination. Repeated authorization failures after a credential refresh must also let world-state contribution finish. ## What changed - Use the thread's `ProviderAuthContext` credentials and runtime authentication when available. - Bind the settings client's network policy to the authenticated account and select authentication through the runtime-aware backend client. - Stop policy resolution after an unsuccessful settings lookup or timeout instead of restarting it when credentials changed during recovery. ## Testing Add app-server coverage for `dual_header` and `assertion_only` authentication with default and custom destinations, including attribution instructions in model input. Add a regression test verifying that repeated `401` responses trigger only one credential refresh and omit attribution instructions. GitOrigin-RevId: 4824bde7d9ec11c241f1caebc754d203dbb0480c --- codex-rs/Cargo.lock | 3 + .../tests/suite/v2/git_attribution.rs | 149 ++++++++++++++++++ codex-rs/ext/git-attribution/Cargo.toml | 3 + .../src/git_attribution_tests.rs | 100 +++++++++++- codex-rs/ext/git-attribution/src/lib.rs | 29 ++-- codex-rs/ext/git-attribution/src/policy.rs | 26 ++- 6 files changed, 289 insertions(+), 21 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 6b6c0e6f0d..5f5ffdf3b9 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3645,8 +3645,11 @@ version = "0.0.0" dependencies = [ "codex-backend-client", "codex-extension-api", + "codex-features", "codex-http-client", "codex-login", + "codex-model-provider", + "codex-runtime-agent-instance", "serde_json", "tokio", "wiremock", diff --git a/codex-rs/app-server/tests/suite/v2/git_attribution.rs b/codex-rs/app-server/tests/suite/v2/git_attribution.rs index 87a9c824e9..73456974e9 100644 --- a/codex-rs/app-server/tests/suite/v2/git_attribution.rs +++ b/codex-rs/app-server/tests/suite/v2/git_attribution.rs @@ -64,6 +64,155 @@ enum LegacyAttribution { UnlinkedPullRequest, } +#[test_case("dual_header", false; "dual_header")] +#[test_case("assertion_only", false; "assertion_only")] +#[test_case("dual_header", true; "dual_header_custom_destination")] +#[test_case("assertion_only", true; "assertion_only_custom_destination")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn git_attribution_runtime_auth_uses_request_destination( + mode: &str, + custom_destination: bool, +) -> Result<()> { + use base64::Engine; + use core_test_support::runtime_auth::PROBE_SERVER; + use core_test_support::runtime_auth::run_harness_through_proxy; + + skip_if_no_network!(Ok(())); + let token = format!( + "e30.{}.c2ln", + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(serde_json::to_vec(&json!({ + "exp": chrono::Utc::now().timestamp() + 3600, + "session_id": "session-a", "iss": "https://auth.openai.com", + "https://api.openai.com/auth": { + "chatgpt_user_id": "user-123", "chatgpt_account_id": "workspace-enabled", + }, + }))?) + ); + if let Ok(server_url) = std::env::var(PROBE_SERVER) { + let home = TempDir::new()?; + let base_url = if custom_destination { + format!("{server_url}/backend-api") + } else { + "https://chatgpt.com/backend-api".into() + }; + write_mock_responses_config_toml_with_chatgpt_base_url( + home.path(), + &server_url, + &base_url, + )?; + let config_path = home.path().join("config.toml"); + let config = std::fs::read_to_string(&config_path)?; + std::fs::write( + config_path, + format!( + "{config}\n[features.runtime_agent_instances]\nenabled = {}\nmode = {mode:?}\n", + !custom_destination + ), + )?; + write_chatgpt_auth( + home.path(), + ChatGptAuthFixture::new(&token) + .account_id("workspace-enabled") + .chatgpt_user_id("user-123") + .plan_type("enterprise"), + AuthCredentialsStoreMode::File, + )?; + let mut app = TestAppServer::builder() + .with_codex_home(home.path()) + .with_env_overrides(&[("OPENAI_API_KEY", None), ("CODEX_ACCESS_TOKEN", None)]) + .build_initialized_with_timeout(DEFAULT_READ_TIMEOUT) + .await?; + let id = app + .send_thread_start_request_with_auto_env(ThreadStartParams { + // Extensions retain the harness URL, even when a thread overrides it. + config: Some(HashMap::from([ + ( + "chatgpt_base_url".into(), + json!("https://chatgpt.com/backend-api"), + ), + ( + "features.runtime_agent_instances".into(), + json!({"enabled": true, "mode": mode}), + ), + ])), + ..Default::default() + }) + .await?; + let ThreadStartResponse { thread, .. } = read_response(&mut app, id).await?; + run_turn(&mut app, &thread.id, "Hello").await?; + return Ok(()); + } + + let server = responses::start_mock_server().await; + let response_mock = responses::mount_sse_sequence( + &server, + vec![create_final_assistant_message_sse_response("Done")?], + ) + .await; + Mock::given(method("GET")) + .and(path("/backend-api/wham/config/bundle")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/agent/register")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "runtime_agent_instance_id": "runtime-a", "agent_host_id": "host-a", + "actor": {"kind": "chatgpt", "user_id": "user-123", "account_id": "workspace-enabled"}, + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/backend-api/wham/settings/user")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "commit_attribution_enabled": true, + }))) + .expect(1) + .mount(&server) + .await; + let case = if custom_destination { + format!("{mode}_custom_destination") + } else { + mode.into() + }; + run_harness_through_proxy( + &server, + &format!("suite::v2::git_attribution::git_attribution_runtime_auth_uses_request_destination::{case}"), + /*realtime_target*/ None, + ) + .await?; + + let requests = server + .received_requests() + .await + .context("captured requests")?; + let settings = requests + .iter() + .find(|request| request.url.path() == "/backend-api/wham/settings/user") + .context("git-attribution settings request")?; + let authorization = settings.headers["authorization"].to_str()?; + if custom_destination || mode == "dual_header" { + assert!(authorization.starts_with("Bearer e30.")); + } else { + assert!(authorization.starts_with("AgentAssertion 1.")); + } + assert_eq!( + settings.headers.contains_key("x-openai-agent-assertion"), + !custom_destination && mode == "dual_header", + ); + let model_requests = response_mock.requests(); + assert_eq!(model_requests.len(), 1); + assert!( + model_requests[0] + .message_input_texts("developer") + .join("\n") + .contains(COMMIT_ATTRIBUTION) + ); + server.verify().await; + Ok(()) +} + #[tokio::test] async fn git_attribution_follows_authenticated_workspace_policy() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/ext/git-attribution/Cargo.toml b/codex-rs/ext/git-attribution/Cargo.toml index e96d0dbcd5..cd15444946 100644 --- a/codex-rs/ext/git-attribution/Cargo.toml +++ b/codex-rs/ext/git-attribution/Cargo.toml @@ -15,8 +15,11 @@ workspace = true [dependencies] codex-backend-client = { workspace = true } codex-extension-api = { workspace = true } +codex-features = { workspace = true } codex-http-client = { workspace = true } codex-login = { workspace = true } +codex-model-provider = { workspace = true } +codex-runtime-agent-instance = { workspace = true } serde_json = { workspace = true } tokio = { workspace = true, features = ["time"] } diff --git a/codex-rs/ext/git-attribution/src/git_attribution_tests.rs b/codex-rs/ext/git-attribution/src/git_attribution_tests.rs index 0123f5b4e8..38b0346067 100644 --- a/codex-rs/ext/git-attribution/src/git_attribution_tests.rs +++ b/codex-rs/ext/git-attribution/src/git_attribution_tests.rs @@ -78,6 +78,7 @@ async fn policy_resolution_recovers_after_unauthorized() { &auth_manager, &format!("{}/backend-api", server.uri()), &http_client_factory(), + /*runtime_auth*/ None, ) .await .expect("policy resolution should not time out") @@ -121,10 +122,15 @@ async fn policy_resolution_retries_after_auth_refresh() { let auth_manager = auth_manager.clone(); let base_url = format!("{}/backend-api", server.uri()); async move { - resolve_attribution_policy(&auth_manager, &base_url, &http_client_factory()) - .await - .ok() - .flatten() + resolve_attribution_policy( + &auth_manager, + &base_url, + &http_client_factory(), + /*runtime_auth*/ None, + ) + .await + .ok() + .flatten() } }); @@ -142,3 +148,89 @@ async fn policy_resolution_retries_after_auth_refresh() { ); server.verify().await; } + +struct RotatingExternalAuth(AtomicUsize); + +impl ExternalAuth for RotatingExternalAuth { + fn resolve(&self) -> ExternalAuthFuture<'_, CodexAuth> { + Box::pin(async { + let generation = self.0.load(Ordering::SeqCst); + CodexAuth::from_external_chatgpt_tokens( + &format!("e30.e30.signature-{generation}"), + "workspace-123", + Some("enterprise"), + ) + }) + } + + fn refresh(&self, _context: ExternalAuthRefreshContext) -> ExternalAuthFuture<'_, CodexAuth> { + self.0.fetch_add(/*val*/ 1, Ordering::SeqCst); + self.resolve() + } +} + +#[tokio::test] +async fn world_state_stops_after_rejected_refresh() { + use codex_extension_api::ContextContributor; + use codex_extension_api::ExtensionData; + use codex_extension_api::PreviousWorldStateSection; + use codex_extension_api::WorldStateContributionInput; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/backend-api/wham/settings/user")) + .respond_with(ResponseTemplate::new(401)) + .expect(2) + .mount(&server) + .await; + let source = Arc::new(RotatingExternalAuth(AtomicUsize::new(/*v*/ 0))); + let auth_manager = enterprise_auth_manager(); + auth_manager + .set_external_auth(source.clone()) + .await + .expect("external credentials should load"); + let extension = super::GitAttributionExtension { + auth_manager, + base_url: format!("{}/backend-api", server.uri()), + http_client_factory: http_client_factory(), + }; + let model_info = serde_json::from_value(serde_json::json!({ + "slug": "test-model", + "display_name": "Test model", + "supported_reasoning_levels": [], + "shell_type": "shell_command", + "visibility": "list", + "supported_in_api": true, + "priority": 0, + "support_verbosity": false, + "truncation_policy": {"mode": "bytes", "limit": 10000}, + "experimental_supported_tools": [] + })) + .expect("model metadata should parse"); + let store = ExtensionData::new("test"); + let sections = tokio::time::timeout( + Duration::from_secs(2), + extension.contribute_world_state(WorldStateContributionInput { + thread_id: Default::default(), + turn_id: "turn", + model_info: &model_info, + environments: &[], + extension_metrics: None, + session_store: &store, + thread_store: &store, + turn_store: &store, + step_store: &store, + previous_world_state: None, + }), + ) + .await + .expect("an optional settings failure must not keep refreshing credentials"); + assert_eq!(source.0.load(Ordering::SeqCst), 1); + assert!(sections.iter().all(|section| { + section + .render_diff(PreviousWorldStateSection::Absent) + .1 + .is_none() + })); + server.verify().await; +} diff --git a/codex-rs/ext/git-attribution/src/lib.rs b/codex-rs/ext/git-attribution/src/lib.rs index a0a3e4dcbf..e206766908 100644 --- a/codex-rs/ext/git-attribution/src/lib.rs +++ b/codex-rs/ext/git-attribution/src/lib.rs @@ -11,6 +11,7 @@ use codex_extension_api::WorldStateContributionInput; use codex_extension_api::WorldStateSectionContribution; use codex_http_client::HttpClientFactory; use codex_login::AuthManager; +use codex_model_provider::ProviderAuthContext; use crate::policy::GitAttributionPolicy; use crate::policy::GitAttributionRetry; @@ -35,8 +36,15 @@ impl ContextContributor for GitAttributionExtension { input: WorldStateContributionInput<'a>, ) -> ExtensionFuture<'a, Vec> { Box::pin(async move { + let auth_context = input.thread_store.get::(); + let auth_manager = auth_context + .as_ref() + .map_or(&self.auth_manager, |context| &context.credentials); + let runtime_auth = auth_context + .as_ref() + .and_then(|context| context.runtime_auth.as_ref()); let enabled = loop { - let current_auth_generation = auth_generation(self.auth_manager.as_ref()); + let current_auth_generation = auth_generation(auth_manager.as_ref()); let policy = match cached_attribution_policy( input.thread_store, input.turn_store, @@ -51,9 +59,10 @@ impl ContextContributor for GitAttributionExtension { } None => { match resolve_attribution_policy( - &self.auth_manager, + auth_manager, &self.base_url, &self.http_client_factory, + runtime_auth, ) .await { @@ -62,30 +71,26 @@ impl ContextContributor for GitAttributionExtension { policy } Ok(None) => { - let policy = GitAttributionPolicy { + input.turn_store.insert(GitAttributionPolicy { auth_generation: current_auth_generation, enabled: false, - }; - input.turn_store.insert(policy.clone()); - policy + }); + break false; } Err(_) => { - let auth_generation = auth_generation(self.auth_manager.as_ref()); + let auth_generation = auth_generation(auth_manager.as_ref()); if auth_generation == current_auth_generation { input.thread_store.insert(GitAttributionRetry { auth_generation, retry_at: Instant::now() + POLICY_RETRY_DELAY, }); } - GitAttributionPolicy { - auth_generation: current_auth_generation, - enabled: false, - } + break false; } } } }; - if policy.auth_generation == auth_generation(self.auth_manager.as_ref()) { + if policy.auth_generation == auth_generation(auth_manager.as_ref()) { break policy.enabled; } }; diff --git a/codex-rs/ext/git-attribution/src/policy.rs b/codex-rs/ext/git-attribution/src/policy.rs index cf607d64d6..df7568203a 100644 --- a/codex-rs/ext/git-attribution/src/policy.rs +++ b/codex-rs/ext/git-attribution/src/policy.rs @@ -4,8 +4,10 @@ use std::time::Instant; use codex_backend_client::Client as BackendClient; use codex_extension_api::ExtensionData; +use codex_features::RuntimeAuthMode; use codex_http_client::HttpClientFactory; use codex_login::AuthManager; +use codex_runtime_agent_instance::RuntimeAuthManager; use tokio::time::timeout; #[derive(Clone, Debug)] @@ -53,6 +55,7 @@ pub(super) async fn resolve_attribution_policy( auth_manager: &Arc, base_url: &str, http_client_factory: &HttpClientFactory, + runtime_auth: Option<&(Arc, RuntimeAuthMode)>, ) -> Result, tokio::time::error::Elapsed> { timeout(POLICY_RESOLUTION_TIMEOUT, async { let mut recovery_generation = auth_generation(auth_manager); @@ -63,15 +66,28 @@ pub(super) async fn resolve_attribution_policy( auth_recovery = auth_manager.unauthorized_recovery(); recovery_generation = auth_generation_at_start; } - let auth = auth_manager.auth().await; + let auth = auth_manager.auth_with_http_client_factory().await; if auth_generation(auth_manager) != auth_generation_at_start { continue; } let enabled = match auth { - Some(auth) if auth.uses_codex_backend() => { - let client = - BackendClient::from_auth(base_url, &auth, http_client_factory.clone()); - let settings = client.get_user_settings().await; + Some((auth, captured_factory)) if auth.uses_codex_backend() => { + let policy = http_client_factory + .network_policy() + .clone() + .with_account_binding(captured_factory.network_policy())?; + let client = BackendClient::from_auth_with_runtime( + base_url, + &auth, + http_client_factory.clone().with_network_policy(policy), + runtime_auth, + /*headers*/ &Default::default(), + ) + .await; + if auth_generation(auth_manager) != auth_generation_at_start { + continue; + } + let settings = client.ok()?.get_user_settings().await; if auth_generation(auth_manager) != auth_generation_at_start { continue; }