From b7649f5358f2c59fa241e72454aa6a86232af027 Mon Sep 17 00:00:00 2001 From: Louis Date: Thu, 25 Jun 2026 22:38:40 +0800 Subject: [PATCH] fix(codex): strip failover routing contamination and preserve modelCatalog on backfill After main's clean-write rework only two of the originally reported issues still reproduce; this trims the fix set accordingly. Kept: - Failover snapshot routing contamination. build_failover_live_snapshot merges the user's live config into each failover target's snapshot with a None base (2-way add-only). The live config carries the active provider's routing, so it leaks into other providers' snapshots: a failover to the official provider keeps the previous third-party model_provider and routes to the old endpoint, and (with preserve_codex_official_auth_on_switch) the third-party experimental_bearer_token leaks into another provider's snapshot. Strip provider-owned fields from the live backup before the merge so each target only inherits user customizations. - modelCatalog wipe on backfill. backfill_codex_current read config.toml text directly and stored it without the modelCatalog field, so switching away from a provider dropped its catalog; switching back then stripped model_catalog_json from config.toml, disabling the catalog. backfill now reads the live catalog via read_codex_model_catalog_simplified_from_live (the same source read_codex_live_settings_with_model_catalog uses) so it round-trips. Dropped (fixed by main's clean-write rework): base_url first-match (config.toml now carries only the active provider and extract_codex_base_url_from_toml follows model_provider), and snapshot cross-contamination / switching-to-official routing (the add-only merge no longer accumulates [model_providers] sections). Tests: regression tests for failover to third-party, to official, bearer-token leak, and modelCatalog round-trip, plus strip_codex_provider_routing unit tests. --- src-tauri/src/codex_config.rs | 78 ++++++++++ src-tauri/src/services/provider/codex.rs | 9 ++ .../provider/codex_openai_auth_tests.rs | 68 ++++++++ src-tauri/src/services/proxy.rs | 147 +++++++++++++++++- 4 files changed, 301 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/codex_config.rs b/src-tauri/src/codex_config.rs index 3e0c57de..cee781b8 100644 --- a/src-tauri/src/codex_config.rs +++ b/src-tauri/src/codex_config.rs @@ -563,6 +563,37 @@ fn set_codex_model_catalog_json_field( Ok(doc.to_string()) } +/// Strip provider-owned fields from a Codex `config.toml` text so that a live +/// config used to seed a failover snapshot cannot contaminate another provider. +/// +/// Removed keys: `model`, `model_provider`, `base_url`, `model_providers`, +/// `experimental_bearer_token`, and `model_catalog_json`. These are the +/// active provider's routing, selected model, credentials, and catalog +/// reference; each failover target brings its own values via its snapshot, so +/// the live config must only contribute user-level customizations (e.g. +/// `model_reasoning_effort`, `disable_response_storage`, `[projects.*]`). +/// +/// Note: `experimental_bearer_token` carries the active provider's API key when +/// `preserve_codex_official_auth_on_switch` is set, so leaving it in would leak +/// one provider's credential into another provider's failover snapshot. +pub fn strip_codex_provider_routing(config_text: &str) -> String { + let Ok(mut doc) = config_text.parse::() else { + return config_text.to_string(); + }; + let root = doc.as_table_mut(); + for key in [ + "model", + "model_provider", + "base_url", + "model_providers", + "experimental_bearer_token", + "model_catalog_json", + ] { + root.remove(key); + } + doc.to_string() +} + #[derive(Clone, Debug)] pub struct PreparedCodexConfigText { pub config_text: String, @@ -1345,6 +1376,53 @@ mod tests { use std::ffi::OsString; use tempfile::TempDir; + #[test] + fn strip_codex_provider_routing_removes_routing_and_keeps_other_keys() { + let input = "\nmodel = \"gpt-4o\"\nmodel_provider = \"alpha\"\nbase_url = \"https://legacy.example/v1\"\nexperimental_bearer_token = \"sk-alpha-secret\"\nmodel_catalog_json = \"cc-switch-model-catalog.json\"\nmodel_reasoning_effort = \"high\"\n\n[model_providers.alpha]\nbase_url = \"https://alpha.example/v1\"\nwire_api = \"chat\"\n\n[projects.foo]\nmodel = \"gpt-4o\"\n"; + let stripped = strip_codex_provider_routing(input); + + assert!( + !stripped.contains("model_provider"), + "model_provider removed: {stripped}" + ); + assert!( + !stripped.contains("model_providers"), + "model_providers table removed: {stripped}" + ); + assert!( + !stripped.contains("alpha"), + "no alpha routing leaked: {stripped}" + ); + assert!( + !stripped.contains("sk-alpha-secret"), + "experimental_bearer_token (credential) removed: {stripped}" + ); + assert!( + !stripped.contains("model_catalog_json"), + "model_catalog_json removed: {stripped}" + ); + assert!( + !stripped.contains("base_url"), + "top-level base_url removed: {stripped}" + ); + assert!( + stripped.contains("model_reasoning_effort"), + "non-routing top-level key preserved: {stripped}" + ); + assert!( + stripped.contains("[projects.foo]"), + "non-routing table preserved: {stripped}" + ); + } + + #[test] + fn strip_codex_provider_routing_handles_empty_and_invalid_toml() { + assert_eq!(strip_codex_provider_routing(""), ""); + let invalid = "this is not = = valid toml }}}"; + let result = strip_codex_provider_routing(invalid); + assert_eq!(result, invalid, "invalid toml returned unchanged"); + } + struct CodexHomeEnvGuard { original: Option, } diff --git a/src-tauri/src/services/provider/codex.rs b/src-tauri/src/services/provider/codex.rs index a67c9e6c..617e974e 100644 --- a/src-tauri/src/services/provider/codex.rs +++ b/src-tauri/src/services/provider/codex.rs @@ -394,6 +394,15 @@ impl ProviderService { raw_settings.insert("auth".to_string(), auth); } raw_settings.insert("config".to_string(), Value::String(text)); + // Preserve the live model catalog so it round-trips across switches. + // Without this the outgoing provider's snapshot loses `modelCatalog`, + // and switching back to it strips `model_catalog_json` from config.toml + // (effectively disabling the catalog). Mirrors read_codex_live_settings_with_model_catalog. + if let Ok(Some(model_catalog)) = + crate::codex_config::read_codex_model_catalog_simplified_from_live() + { + raw_settings.insert("modelCatalog".to_string(), model_catalog); + } snapshot_provider.settings_config = Value::Object(raw_settings); snapshot_provider = Self::migrate_provider_snapshot_for_storage( &AppType::Codex, diff --git a/src-tauri/src/services/provider/codex_openai_auth_tests.rs b/src-tauri/src/services/provider/codex_openai_auth_tests.rs index 0c84333f..52de6487 100644 --- a/src-tauri/src/services/provider/codex_openai_auth_tests.rs +++ b/src-tauri/src/services/provider/codex_openai_auth_tests.rs @@ -247,3 +247,71 @@ fn migrate_legacy_codex_config_preserves_extra_keys() { "should preserve disable_response_storage: {result}" ); } + +#[test] +#[serial] +fn codex_model_catalog_survives_round_trip_switch() { + let temp_home = TempDir::new().expect("create temp home"); + let _env = TestEnvGuard::isolated(temp_home.path()); + std::fs::create_dir_all(crate::codex_config::get_codex_config_dir()) + .expect("create ~/.codex (initialized)"); + + let catalog = json!({ + "models": [{ "model": "gpt-4o", "displayName": "GPT-4o" }] + }); + + let mut config = MultiAppConfig::default(); + config.ensure_app(&AppType::Codex); + { + let manager = config + .get_manager_mut(&AppType::Codex) + .expect("codex manager"); + manager.providers.insert( + "alpha".to_string(), + Provider::with_id( + "alpha".to_string(), + "Alpha".to_string(), + json!({ + "auth": { "OPENAI_API_KEY": "sk-alpha" }, + "config": "model_provider = \"alpha\"\nmodel = \"gpt-4o\"\n\n[model_providers.alpha]\nbase_url = \"https://alpha.example/v1\"\nwire_api = \"chat\"\n", + "modelCatalog": catalog, + }), + None, + ), + ); + manager.providers.insert( + "beta".to_string(), + Provider::with_id( + "beta".to_string(), + "Beta".to_string(), + json!({ + "auth": { "OPENAI_API_KEY": "sk-beta" }, + "config": "model_provider = \"beta\"\nmodel = \"gpt-4o\"\n\n[model_providers.beta]\nbase_url = \"https://beta.example/v1\"\nwire_api = \"chat\"\n", + }), + None, + ), + ); + } + + let state = state_from_config(config); + + // Switch to alpha: config.toml should install the model_catalog_json reference. + ProviderService::switch(&state, AppType::Codex, "alpha").expect("switch to alpha"); + let after_alpha = + std::fs::read_to_string(get_codex_config_path()).expect("read config after alpha"); + assert!( + after_alpha.contains("model_catalog_json"), + "alpha switch should install model_catalog_json: {after_alpha}" + ); + + // Round-trip: away to beta, then back to alpha. + ProviderService::switch(&state, AppType::Codex, "beta").expect("switch to beta"); + ProviderService::switch(&state, AppType::Codex, "alpha").expect("switch back to alpha"); + + let after_round_trip = + std::fs::read_to_string(get_codex_config_path()).expect("read config after round trip"); + assert!( + after_round_trip.contains("model_catalog_json"), + "model_catalog_json must survive a round-trip switch: {after_round_trip}" + ); +} diff --git a/src-tauri/src/services/proxy.rs b/src-tauri/src/services/proxy.rs index 8f1bd015..c75ad4fa 100644 --- a/src-tauri/src/services/proxy.rs +++ b/src-tauri/src/services/proxy.rs @@ -1597,7 +1597,37 @@ impl ProxyService { provider, &mut provider_snapshot, )?; - Self::merge_live_backup_snapshot(app_type, Some(original_live), None, provider_snapshot) + + // For Codex, the user's live config carries the currently-active + // provider's routing (`model_provider` + `[model_providers.*]`). Strip it + // before the 2-way merge so it cannot contaminate another provider's + // failover snapshot; each target brings its own routing via + // `provider_snapshot`. Without this a failover to the official provider + // inherits the previous third-party `model_provider` and keeps routing to + // the old endpoint. + let stripped; + let existing: &Value = if matches!(app_type, AppType::Codex) { + stripped = Self::codex_live_backup_without_routing(original_live); + &stripped + } else { + original_live + }; + + Self::merge_live_backup_snapshot(app_type, Some(existing), None, provider_snapshot) + } + + /// Return a copy of a Codex live backup with provider-routing fields removed + /// from its embedded `config` text. Non-routing keys are left untouched. + fn codex_live_backup_without_routing(original_live: &Value) -> Value { + let Some(config_text) = original_live.get("config").and_then(Value::as_str) else { + return original_live.clone(); + }; + let stripped = crate::codex_config::strip_codex_provider_routing(config_text); + let mut clone = original_live.clone(); + if let Some(obj) = clone.as_object_mut() { + obj.insert("config".to_string(), Value::String(stripped)); + } + clone } async fn save_failover_live_snapshot( @@ -4022,6 +4052,121 @@ mod tests { assert_eq!(env.get(key).and_then(Value::as_str), expected, "{key}"); } + #[test] + fn codex_failover_snapshot_excludes_foreign_provider_routing() { + let db = Arc::new(Database::memory().expect("create database")); + let service = ProxyService::new(db); + + // The currently-live provider (alpha) carries its own routing in config.toml. + let original_live = json!({ + "config": "\nmodel_provider = \"alpha\"\n\n[model_providers.alpha]\nbase_url = \"https://alpha.example/v1\"\nwire_api = \"chat\"\n", + "auth": {} + }); + // The failover target (beta) has its own routing. + let provider_beta = Provider::with_id( + "beta".to_string(), + "Beta".to_string(), + json!({ + "auth": {}, + "config": "\nmodel_provider = \"beta\"\n\n[model_providers.beta]\nbase_url = \"https://beta.example/v1\"\nwire_api = \"chat\"\n", + }), + None, + ); + + let merged = service + .build_failover_live_snapshot(&AppType::Codex, &original_live, &provider_beta) + .expect("build beta failover snapshot"); + + let cfg = merged + .get("config") + .and_then(Value::as_str) + .expect("merged config text"); + assert!( + cfg.contains("model_providers.beta"), + "beta's own routing must be present: {cfg}" + ); + assert!( + !cfg.contains("alpha"), + "beta's failover snapshot leaked alpha's routing: {cfg}" + ); + } + + #[test] + fn codex_failover_snapshot_to_official_drops_third_party_routing() { + let db = Arc::new(Database::memory().expect("create database")); + let service = ProxyService::new(db); + + // Currently-live third-party provider carries its own routing. + let original_live = json!({ + "config": "\nmodel_provider = \"alpha\"\n\n[model_providers.alpha]\nbase_url = \"https://alpha.example/v1\"\nwire_api = \"chat\"\n", + "auth": {} + }); + // The failover target is the official provider, which carries NO routing + // (no `model_provider`, no `[model_providers]` section) in config.toml. + let official = Provider::with_id( + "codex-official".to_string(), + "Codex Official".to_string(), + json!({ + "auth": { "OPENAI_API_KEY": "official-key" }, + "config": "\n# official codex: no model_provider routing\n", + }), + None, + ); + + let merged = service + .build_failover_live_snapshot(&AppType::Codex, &original_live, &official) + .expect("build official failover snapshot"); + + let cfg = merged + .get("config") + .and_then(Value::as_str) + .expect("merged config text"); + assert!( + !cfg.contains("model_provider"), + "official failover snapshot must not keep third-party model_provider routing: {cfg}" + ); + assert!( + !cfg.contains("alpha"), + "official failover snapshot leaked alpha's routing: {cfg}" + ); + } + + #[test] + fn codex_failover_snapshot_does_not_leak_bearer_token() { + // With preserve_codex_official_auth_on_switch the active provider's API + // key rides in config.toml as experimental_bearer_token. It must not leak + // into another provider's failover snapshot. + let db = Arc::new(Database::memory().expect("create database")); + let service = ProxyService::new(db); + + let original_live = json!({ + "config": "\nmodel_provider = \"alpha\"\nexperimental_bearer_token = \"sk-alpha-secret\"\n\n[model_providers.alpha]\nbase_url = \"https://alpha.example/v1\"\nwire_api = \"chat\"\n", + "auth": {} + }); + let provider_beta = Provider::with_id( + "beta".to_string(), + "Beta".to_string(), + json!({ + "auth": { "OPENAI_API_KEY": "sk-beta" }, + "config": "\nmodel_provider = \"beta\"\n\n[model_providers.beta]\nbase_url = \"https://beta.example/v1\"\nwire_api = \"chat\"\n", + }), + None, + ); + + let merged = service + .build_failover_live_snapshot(&AppType::Codex, &original_live, &provider_beta) + .expect("build beta failover snapshot"); + + let cfg = merged + .get("config") + .and_then(Value::as_str) + .expect("merged config text"); + assert!( + !cfg.contains("sk-alpha-secret"), + "beta's failover snapshot leaked alpha's bearer token: {cfg}" + ); + } + #[test] fn managed_account_claude_takeover_uses_api_key_placeholder() { let mut provider = Provider::with_id(