From 8bbdf6c8f9ecf4833479c64a4794c9ed6c2dab9b Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Tue, 28 Jul 2026 19:53:16 +0000 Subject: [PATCH] Use the shared HTTP client for TUI network checks (#35821) ## Why TUI update checks and local OSS provider detection constructed their own HTTP clients instead of using Codex's shared client behavior. ## What changed - Route update requests through the configured route-aware client pool while retaining the existing default headers and custom CA fallback. - Probe the hardcoded LM Studio and Ollama loopback endpoints with a shared direct client and a per-request timeout. - Limit the legacy invalid-custom-CA fallback to the default routing policy so system-proxy routing still reports certificate configuration errors. ## Testing Add coverage for local provider probes with invalid `CODEX_CA_CERTIFICATE` and `SSL_CERT_FILE` values, and for custom CA fallback under both routing policies. GitOrigin-RevId: b5c230b61e8964b3f1af3395052361ff716d6ce1 --- codex-rs/Cargo.lock | 1 + .../src/route_aware_client_pool_tests.rs | 75 +++++++++++++++++ codex-rs/tui/Cargo.toml | 1 + codex-rs/tui/src/oss_selection.rs | 84 ++++++++++++++++--- codex-rs/tui/src/updates.rs | 36 ++++++-- 5 files changed, 175 insertions(+), 22 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 6810d2c3da..266081995e 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4186,6 +4186,7 @@ dependencies = [ "codex-feedback", "codex-file-search", "codex-git-utils", + "codex-http-client", "codex-install-context", "codex-login", "codex-mcp", diff --git a/codex-rs/http-client/src/route_aware_client_pool_tests.rs b/codex-rs/http-client/src/route_aware_client_pool_tests.rs index 7cce63f90f..ab687d4033 100644 --- a/codex-rs/http-client/src/route_aware_client_pool_tests.rs +++ b/codex-rs/http-client/src/route_aware_client_pool_tests.rs @@ -65,6 +65,81 @@ async fn streams_request_bodies_without_exposing_reqwest_body() { assert!(requests[0].ends_with("\r\n\r\nhello")); } +#[tokio::test] +async fn legacy_custom_ca_fallback_is_limited_to_reqwest_default() { + const CHILD_POLICY_ENV: &str = "CODEX_HTTP_CLIENT_POOL_INVALID_CA_TEST_POLICY"; + + let Ok(policy_name) = std::env::var(CHILD_POLICY_ENV) else { + let temp_dir = tempfile::tempdir().expect("temporary directory should be created"); + let invalid_ca_path = temp_dir.path().join("invalid-ca.pem"); + std::fs::write(&invalid_ca_path, "not a PEM certificate") + .expect("invalid CA fixture should be written"); + + for ca_env in ["CODEX_CA_CERTIFICATE", "SSL_CERT_FILE"] { + for policy_name in ["reqwest-default", "respect-system-proxy"] { + let output = std::process::Command::new( + std::env::current_exe().expect("test executable should be available"), + ) + .arg("--exact") + .arg("route_aware_client_pool::tests::legacy_custom_ca_fallback_is_limited_to_reqwest_default") + .arg("--nocapture") + .env_remove("CODEX_CA_CERTIFICATE") + .env_remove("SSL_CERT_FILE") + .env(ca_env, &invalid_ca_path) + .env(CHILD_POLICY_ENV, policy_name) + .output() + .expect("isolated CA subprocess should run"); + + assert!( + output.status.success(), + "{policy_name} failed with invalid {ca_env}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + } + } + return; + }; + + let outbound_proxy_policy = match policy_name.as_str() { + "reqwest-default" => OutboundProxyPolicy::ReqwestDefault, + "respect-system-proxy" => OutboundProxyPolicy::RespectSystemProxy, + _ => panic!("unexpected test proxy policy: {policy_name}"), + }; + let pool = RouteAwareClientPool::with_chatgpt_cloudflare_cookies( + HttpClientFactory::new(outbound_proxy_policy), + ClientRouteClass::Other, + ) + .with_legacy_custom_ca_fallback(); + + match outbound_proxy_policy { + OutboundProxyPolicy::ReqwestDefault => { + let (address, server) = spawn_response_server(vec![ + "HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: close\r\n\r\n".to_string(), + ]); + let response = pool + .get(format!("http://{address}/update")) + .send() + .await + .expect("default-routed request should fall back to system roots"); + + assert_eq!(response.status(), StatusCode::OK); + let requests = server.join().expect("response server should finish"); + assert_eq!(requests.len(), 1); + } + OutboundProxyPolicy::RespectSystemProxy => { + let error = pool + .client_for_url_with_resolver("http://127.0.0.1/update", |_| async { + Ok(OutboundProxyRoute::Direct) + }) + .await + .expect_err("system-proxy routes should reject invalid custom CAs"); + + assert!(matches!(error, RouteAwareClientPoolError::Build(_))); + } + } +} + #[tokio::test] async fn without_url_redacts_transport_error_urls() { let listener = TcpListener::bind(("127.0.0.1", 0)).expect("listener should bind"); diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 7fd75990eb..e16a9c06c2 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -40,6 +40,7 @@ codex-features = { workspace = true } codex-feedback = { workspace = true } codex-file-search = { workspace = true } codex-git-utils = { workspace = true } +codex-http-client = { workspace = true } codex-login = { workspace = true } codex-message-history = { workspace = true } codex-model-provider = { workspace = true } diff --git a/codex-rs/tui/src/oss_selection.rs b/codex-rs/tui/src/oss_selection.rs index cbb8e676f2..72e0a8726a 100644 --- a/codex-rs/tui/src/oss_selection.rs +++ b/codex-rs/tui/src/oss_selection.rs @@ -4,6 +4,8 @@ use std::sync::LazyLock; use crate::key_hint; use crate::key_hint::KeyBinding; use crate::key_hint::KeyBindingListExt; +use codex_http_client::HttpClient; +use codex_http_client::HttpClientBuilder; use codex_model_provider_info::DEFAULT_LMSTUDIO_PORT; use codex_model_provider_info::DEFAULT_OLLAMA_PORT; use codex_model_provider_info::LMSTUDIO_OSS_PROVIDER_ID; @@ -314,9 +316,15 @@ pub(crate) struct OssProviderSelection { } pub async fn select_oss_provider() -> io::Result { + // These probes intentionally bypass proxy discovery because both targets are + // hardcoded plaintext loopback endpoints. Preserve the legacy custom-CA fallback so an + // invalid inherited certificate bundle cannot prevent best-effort provider detection. + #[allow(deprecated)] + let client = HttpClientBuilder::new().build_direct_with_custom_ca_fallback(); + // Check provider statuses first - let lmstudio_status = check_lmstudio_status().await; - let ollama_status = check_ollama_status().await; + let lmstudio_status = check_lmstudio_status(&client).await; + let ollama_status = check_ollama_status(&client).await; // Autoselect if only one is running match (&lmstudio_status, &ollama_status) { @@ -369,31 +377,31 @@ pub async fn select_oss_provider() -> io::Result { result } -async fn check_lmstudio_status() -> ProviderStatus { - match check_port_status(DEFAULT_LMSTUDIO_PORT).await { +async fn check_lmstudio_status(client: &HttpClient) -> ProviderStatus { + match check_port_status(client, DEFAULT_LMSTUDIO_PORT).await { Ok(true) => ProviderStatus::Running, Ok(false) => ProviderStatus::NotRunning, Err(_) => ProviderStatus::Unknown, } } -async fn check_ollama_status() -> ProviderStatus { - match check_port_status(DEFAULT_OLLAMA_PORT).await { +async fn check_ollama_status(client: &HttpClient) -> ProviderStatus { + match check_port_status(client, DEFAULT_OLLAMA_PORT).await { Ok(true) => ProviderStatus::Running, Ok(false) => ProviderStatus::NotRunning, Err(_) => ProviderStatus::Unknown, } } -async fn check_port_status(port: u16) -> io::Result { - let client = reqwest::Client::builder() - .timeout(Duration::from_secs(2)) - .build() - .map_err(io::Error::other)?; - +async fn check_port_status(client: &HttpClient, port: u16) -> io::Result { let url = format!("http://localhost:{port}"); - match client.get(&url).send().await { + match client + .get(&url) + .timeout(Duration::from_secs(2)) + .send() + .await + { Ok(response) => Ok(response.status().is_success()), Err(_) => Ok(false), // Connection failed = not running } @@ -414,4 +422,54 @@ mod tests { widget.handle_key_event(KeyEvent::new(KeyCode::Char('h'), KeyModifiers::CONTROL)); assert_eq!(widget.selected_option, 0); } + + #[tokio::test] + async fn localhost_probe_succeeds_with_invalid_inherited_ca_bundle() { + const CHILD_ENV: &str = "CODEX_OSS_SELECTION_INVALID_CA_TEST_CHILD"; + + if std::env::var_os(CHILD_ENV).is_none() { + let temp_dir = tempfile::tempdir().expect("temporary directory should be created"); + let invalid_ca_path = temp_dir.path().join("invalid-ca.pem"); + std::fs::write(&invalid_ca_path, "not a PEM certificate") + .expect("invalid CA fixture should be written"); + + for ca_env in ["CODEX_CA_CERTIFICATE", "SSL_CERT_FILE"] { + let output = std::process::Command::new( + std::env::current_exe().expect("test executable should be available"), + ) + .arg("--exact") + .arg("oss_selection::tests::localhost_probe_succeeds_with_invalid_inherited_ca_bundle") + .arg("--nocapture") + .env_remove("CODEX_CA_CERTIFICATE") + .env_remove("SSL_CERT_FILE") + .env(ca_env, &invalid_ca_path) + .env(CHILD_ENV, "1") + .output() + .expect("isolated CA subprocess should run"); + + assert!( + output.status.success(), + "localhost probe failed with invalid {ca_env}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + } + return; + } + + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("GET")) + .respond_with(wiremock::ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + + #[allow(deprecated)] + let client = HttpClientBuilder::new().build_direct_with_custom_ca_fallback(); + assert!( + check_port_status(&client, server.address().port()) + .await + .expect("localhost provider probe should complete") + ); + } } diff --git a/codex-rs/tui/src/updates.rs b/codex-rs/tui/src/updates.rs index 64e7f76344..948551a65e 100644 --- a/codex-rs/tui/src/updates.rs +++ b/codex-rs/tui/src/updates.rs @@ -13,7 +13,10 @@ use crate::updates_cache::read_version_info; use crate::updates_cache::version_filepath; use chrono::Duration; use chrono::Utc; -use codex_login::default_client::create_client; +use codex_http_client::ClientRouteClass; +use codex_http_client::HttpClientFactory; +use codex_http_client::RouteAwareClientPool; +use codex_login::default_client::default_headers; use serde::Deserialize; use std::path::Path; @@ -34,11 +37,12 @@ pub fn get_upgrade_version(config: &Config) -> Option { None => true, Some(info) => info.last_checked_at < Utc::now() - Duration::hours(20), } { + let http_client_factory = config.http_client_factory(); // Refresh the cached latest version in the background so TUI startup // isn’t blocked by a network call. The UI reads the previously cached // value (if any) for this run; the next run shows the banner if needed. tokio::spawn(async move { - check_for_update(&version_file, action) + check_for_update(&version_file, action, http_client_factory) .await .inspect_err(|e| tracing::error!("Failed to update version: {e}")) }); @@ -67,11 +71,21 @@ struct HomebrewCaskInfo { version: String, } -async fn check_for_update(version_file: &Path, action: Option) -> anyhow::Result<()> { +async fn check_for_update( + version_file: &Path, + action: Option, + http_client_factory: HttpClientFactory, +) -> anyhow::Result<()> { + let client_pool = RouteAwareClientPool::with_chatgpt_cloudflare_cookies( + http_client_factory, + ClientRouteClass::Other, + ) + .with_legacy_custom_ca_fallback(); let latest_version = match action { Some(UpdateAction::BrewUpgrade) => { - let HomebrewCaskInfo { version } = create_client() + let HomebrewCaskInfo { version } = client_pool .get(HOMEBREW_CASK_API_URL) + .headers(default_headers()) .send() .await? .error_for_status()? @@ -82,9 +96,10 @@ async fn check_for_update(version_file: &Path, action: Option) -> Some(UpdateAction::NpmGlobalLatest) | Some(UpdateAction::BunGlobalLatest) | Some(UpdateAction::PnpmGlobalLatest) => { - let latest_version = fetch_latest_github_release_version().await?; - let package_info = create_client() + let latest_version = fetch_latest_github_release_version(&client_pool).await?; + let package_info = client_pool .get(npm_registry::PACKAGE_URL) + .headers(default_headers()) .send() .await? .error_for_status()? @@ -94,7 +109,7 @@ async fn check_for_update(version_file: &Path, action: Option) -> latest_version } Some(UpdateAction::StandaloneUnix) | Some(UpdateAction::StandaloneWindows) | None => { - fetch_latest_github_release_version().await? + fetch_latest_github_release_version(&client_pool).await? } }; @@ -114,11 +129,14 @@ async fn check_for_update(version_file: &Path, action: Option) -> Ok(()) } -async fn fetch_latest_github_release_version() -> anyhow::Result { +async fn fetch_latest_github_release_version( + client_pool: &RouteAwareClientPool, +) -> anyhow::Result { let ReleaseInfo { tag_name: latest_tag_name, - } = create_client() + } = client_pool .get(LATEST_RELEASE_URL) + .headers(default_headers()) .send() .await? .error_for_status()?