From 4fe02f4fcf408125882b103b7d2f85ff1f2a4837 Mon Sep 17 00:00:00 2001 From: xl-openai Date: Tue, 23 Jun 2026 20:13:11 -0700 Subject: [PATCH] [plugins] Enforce marketplace source admission requirements (#29753) ## Why Managed marketplace source requirements only become effective when every local marketplace mutation path applies the same admission decision. This change centralizes that decision so CLI, app-server, and external-agent migration flows cannot add, install from, or refresh a disallowed source. ## What changed - Match exact normalized Git repository URLs with an optional exact `ref`. - Match Git hosts with managed regular expressions. - Match local marketplaces by exact absolute path. - Preserve the expected path/name boundary for managed OpenAI marketplaces. - Enforce source admission during marketplace add, plugin install, and configured Git marketplace upgrade. - Continue upgrading independent marketplaces when one source is rejected and return a per-marketplace error. - Load the effective requirements stack at CLI, app-server, and external-agent migration entry points. This PR does not filter already configured marketplaces at runtime; that remains in draft follow-up #29691. ## Stack This is PR 2 of 3 and is based on #29690, which introduces the requirements data shape and merge behavior. ## Test plan - Source matcher coverage for Git URL/ref, host-pattern, local-path, and managed marketplace cases. - Marketplace add and plugin install coverage for allowed and rejected sources. - Marketplace upgrade coverage for rejection and per-marketplace continuation. --- codex-rs/Cargo.lock | 2 + .../src/config/external_agent_config.rs | 46 +- .../marketplace_processor.rs | 2 + .../src/request_processors/plugins.rs | 5 +- codex-rs/cli/src/marketplace_cmd.rs | 11 +- codex-rs/cli/src/plugin_cmd.rs | 11 +- codex-rs/core-plugins/Cargo.toml | 2 + .../core-plugins/src/discoverable_tests.rs | 18 +- codex-rs/core-plugins/src/lib.rs | 3 + codex-rs/core-plugins/src/manager.rs | 67 ++- codex-rs/core-plugins/src/manager_tests.rs | 219 ++++++-- codex-rs/core-plugins/src/marketplace_add.rs | 116 +++- .../src/marketplace_add/source.rs | 2 +- .../core-plugins/src/marketplace_policy.rs | 392 ++++++++++++++ .../src/marketplace_policy_tests.rs | 500 ++++++++++++++++++ .../core-plugins/src/marketplace_upgrade.rs | 181 ++++--- .../src/marketplace_upgrade_tests.rs | 221 ++++++++ .../handlers/request_plugin_install_tests.rs | 17 +- 18 files changed, 1621 insertions(+), 194 deletions(-) create mode 100644 codex-rs/core-plugins/src/marketplace_policy.rs create mode 100644 codex-rs/core-plugins/src/marketplace_policy_tests.rs create mode 100644 codex-rs/core-plugins/src/marketplace_upgrade_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 0ae004843..2170c529f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2769,6 +2769,7 @@ dependencies = [ "codex-protocol", "codex-tools", "codex-utils-absolute-path", + "codex-utils-path", "codex-utils-path-uri", "codex-utils-plugins", "dirs", @@ -2776,6 +2777,7 @@ dependencies = [ "indexmap 2.14.0", "libc", "pretty_assertions", + "regex", "reqwest 0.12.28", "semver", "serde", diff --git a/codex-rs/app-server/src/config/external_agent_config.rs b/codex-rs/app-server/src/config/external_agent_config.rs index ce10d6e81..8c8c67d53 100644 --- a/codex-rs/app-server/src/config/external_agent_config.rs +++ b/codex-rs/app-server/src/config/external_agent_config.rs @@ -877,6 +877,16 @@ impl ExternalAgentConfigService { "plugins migration item is missing details".to_string(), )); }; + let config = ConfigBuilder::default() + .codex_home(self.codex_home.clone()) + .fallback_cwd(Some( + cwd.map(Path::to_path_buf) + .unwrap_or_else(|| self.codex_home.clone()), + )) + .build() + .await + .map_err(|err| io::Error::other(format!("failed to load config: {err}")))?; + let requirements = config.config_layer_stack.requirements().clone(); let mut outcome = PluginImportOutcome::default(); let plugins_manager = PluginsManager::new(self.codex_home.clone()); for plugin_group in plugins { @@ -916,7 +926,8 @@ impl ExternalAgentConfigService { ref_name: import_source.ref_name, sparse_paths: Vec::new(), }; - let add_marketplace_outcome = add_marketplace(self.codex_home.clone(), request).await; + let add_marketplace_outcome = + add_marketplace(self.codex_home.clone(), requirements.clone(), request).await; let marketplace_path = match add_marketplace_outcome { Ok(add_marketplace_outcome) => { let Some(marketplace_path) = find_marketplace_manifest_path( @@ -954,12 +965,37 @@ impl ExternalAgentConfigService { continue; } }; + let install_config = match ConfigBuilder::default() + .codex_home(self.codex_home.clone()) + .fallback_cwd(Some( + cwd.map(Path::to_path_buf) + .unwrap_or_else(|| self.codex_home.clone()), + )) + .build() + .await + { + Ok(config) => config, + Err(err) => { + record_plugin_import_errors( + &mut outcome, + cwd, + &plugin_ids, + "plugin_import", + format!("failed to reload config after adding marketplace: {err}"), + ); + outcome.failed_plugin_ids.extend(plugin_ids); + continue; + } + }; for plugin_name in plugin_names { match plugins_manager - .install_plugin(PluginInstallRequest { - plugin_name: plugin_name.clone(), - marketplace_path: marketplace_path.clone(), - }) + .install_plugin( + &install_config.config_layer_stack, + PluginInstallRequest { + plugin_name: plugin_name.clone(), + marketplace_path: marketplace_path.clone(), + }, + ) .await { Ok(_) => outcome diff --git a/codex-rs/app-server/src/request_processors/marketplace_processor.rs b/codex-rs/app-server/src/request_processors/marketplace_processor.rs index 1a0950741..cba73c4ff 100644 --- a/codex-rs/app-server/src/request_processors/marketplace_processor.rs +++ b/codex-rs/app-server/src/request_processors/marketplace_processor.rs @@ -105,8 +105,10 @@ impl MarketplaceRequestProcessor { &self, params: MarketplaceAddParams, ) -> Result { + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; add_marketplace_to_codex_home( self.config.codex_home.to_path_buf(), + config.config_layer_stack.requirements().clone(), MarketplaceAddRequest { source: params.source, ref_name: params.ref_name, diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index 6008c0ddd..a98fddb5b 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -1449,7 +1449,10 @@ impl PluginRequestProcessor { marketplace_path, }; - let result = match plugins_manager.install_plugin(request).await { + let result = match plugins_manager + .install_plugin(&config.config_layer_stack, request) + .await + { Ok(result) => result, Err(err) => { warn!( diff --git a/codex-rs/cli/src/marketplace_cmd.rs b/codex-rs/cli/src/marketplace_cmd.rs index f94447734..e4e2b6337 100644 --- a/codex-rs/cli/src/marketplace_cmd.rs +++ b/codex-rs/cli/src/marketplace_cmd.rs @@ -132,7 +132,7 @@ impl MarketplaceCli { .map_err(anyhow::Error::msg)?; match subcommand { - MarketplaceSubcommand::Add(args) => run_add(args).await?, + MarketplaceSubcommand::Add(args) => run_add(overrides, args).await?, MarketplaceSubcommand::List(args) => run_list(overrides, args).await?, MarketplaceSubcommand::Upgrade(args) => run_upgrade(overrides, args).await?, MarketplaceSubcommand::Remove(args) => run_remove(args).await?, @@ -142,7 +142,7 @@ impl MarketplaceCli { } } -async fn run_add(args: AddMarketplaceArgs) -> Result<()> { +async fn run_add(overrides: Vec<(String, toml::Value)>, args: AddMarketplaceArgs) -> Result<()> { let AddMarketplaceArgs { source, ref_name, @@ -150,9 +150,12 @@ async fn run_add(args: AddMarketplaceArgs) -> Result<()> { json, } = args; - let codex_home = find_codex_home().context("failed to resolve CODEX_HOME")?; + let config = Config::load_with_cli_overrides(overrides) + .await + .context("failed to load configuration")?; let outcome = add_marketplace( - codex_home.to_path_buf(), + config.codex_home.to_path_buf(), + config.config_layer_stack.requirements().clone(), MarketplaceAddRequest { source, ref_name, diff --git a/codex-rs/cli/src/plugin_cmd.rs b/codex-rs/cli/src/plugin_cmd.rs index b1cf21aa5..8c9144dff 100644 --- a/codex-rs/cli/src/plugin_cmd.rs +++ b/codex-rs/cli/src/plugin_cmd.rs @@ -148,10 +148,13 @@ pub async fn run_plugin_add( &plugin_name, )?; let outcome = manager - .install_plugin(PluginInstallRequest { - plugin_name, - marketplace_path: marketplace.path, - }) + .install_plugin( + &plugins_input.config_layer_stack, + PluginInstallRequest { + plugin_name, + marketplace_path: marketplace.path, + }, + ) .await?; if json { diff --git a/codex-rs/core-plugins/Cargo.toml b/codex-rs/core-plugins/Cargo.toml index b90192bfe..448723828 100644 --- a/codex-rs/core-plugins/Cargo.toml +++ b/codex-rs/core-plugins/Cargo.toml @@ -29,6 +29,7 @@ codex-plugin = { workspace = true } codex-protocol = { workspace = true } codex-tools = { workspace = true } codex-utils-absolute-path = { workspace = true } +codex-utils-path = { workspace = true } codex-utils-path-uri = { workspace = true } codex-utils-plugins = { workspace = true } chrono = { workspace = true } @@ -36,6 +37,7 @@ dirs = { workspace = true } flate2 = { workspace = true } indexmap = { workspace = true, features = ["serde"] } reqwest = { workspace = true } +regex = { workspace = true } semver = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } diff --git a/codex-rs/core-plugins/src/discoverable_tests.rs b/codex-rs/core-plugins/src/discoverable_tests.rs index 5b817e6a7..6e411cdda 100644 --- a/codex-rs/core-plugins/src/discoverable_tests.rs +++ b/codex-rs/core-plugins/src/discoverable_tests.rs @@ -932,14 +932,18 @@ fn string_set(values: &[&str]) -> HashSet { async fn install_marketplace_plugin(codex_home: &Path, marketplace_root: &Path, plugin_name: &str) { write_curated_plugin_sha_with(codex_home, TEST_CURATED_PLUGIN_SHA); + let config = load_plugins_config(codex_home, marketplace_root).await; PluginsManager::new(codex_home.to_path_buf()) - .install_plugin(PluginInstallRequest { - plugin_name: plugin_name.to_string(), - marketplace_path: AbsolutePathBuf::try_from( - marketplace_root.join(".agents/plugins/marketplace.json"), - ) - .expect("marketplace path"), - }) + .install_plugin( + &config.config_layer_stack, + PluginInstallRequest { + plugin_name: plugin_name.to_string(), + marketplace_path: AbsolutePathBuf::try_from( + marketplace_root.join(".agents/plugins/marketplace.json"), + ) + .expect("marketplace path"), + }, + ) .await .expect("plugin should install"); } diff --git a/codex-rs/core-plugins/src/lib.rs b/codex-rs/core-plugins/src/lib.rs index 08e77606f..7ee5ddd11 100644 --- a/codex-rs/core-plugins/src/lib.rs +++ b/codex-rs/core-plugins/src/lib.rs @@ -6,6 +6,7 @@ mod manager; pub mod manifest; pub mod marketplace; pub mod marketplace_add; +mod marketplace_policy; pub mod marketplace_remove; pub mod marketplace_upgrade; mod plugin_bundle_archive; @@ -23,6 +24,8 @@ mod tool_suggest_metadata; pub const OPENAI_CURATED_MARKETPLACE_NAME: &str = "openai-curated"; pub const OPENAI_API_CURATED_MARKETPLACE_NAME: &str = "openai-api-curated"; pub const OPENAI_BUNDLED_MARKETPLACE_NAME: &str = "openai-bundled"; +pub(crate) const OPENAI_BUNDLED_ALPHA_MARKETPLACE_NAME: &str = "openai-bundled-alpha"; +pub(crate) const OPENAI_PRIMARY_RUNTIME_MARKETPLACE_NAME: &str = "openai-primary-runtime"; pub fn is_openai_curated_marketplace_name(marketplace_name: &str) -> bool { marketplace_name == OPENAI_CURATED_MARKETPLACE_NAME diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 72f2ad68e..b357c5b53 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -34,9 +34,9 @@ use crate::marketplace::find_installable_marketplace_plugin; use crate::marketplace::find_marketplace_plugin; use crate::marketplace::list_marketplaces; use crate::marketplace::plugin_interface_with_marketplace_category; +use crate::marketplace_policy::MarketplacePolicy; use crate::marketplace_upgrade::ConfiguredMarketplaceUpgradeError; use crate::marketplace_upgrade::ConfiguredMarketplaceUpgradeOutcome; -use crate::marketplace_upgrade::configured_git_marketplace_names; use crate::marketplace_upgrade::upgrade_configured_git_marketplaces; use crate::remote::REMOTE_GLOBAL_MARKETPLACE_NAME; use crate::remote::RecommendedPluginsMode; @@ -1253,19 +1253,10 @@ impl PluginsManager { pub async fn install_plugin( &self, + config_layer_stack: &ConfigLayerStack, request: PluginInstallRequest, ) -> Result { - let resolved = match find_installable_marketplace_plugin( - &request.marketplace_path, - &request.plugin_name, - self.restriction_product, - ) { - Ok(resolved) => resolved, - Err(err) => { - self.track_plugin_install_resolution_failed(&err); - return Err(err.into()); - } - }; + let resolved = self.resolve_installable_plugin(config_layer_stack, &request)?; let plugin_id = resolved.plugin_id.clone(); match self.install_resolved_plugin(resolved).await { Ok(outcome) => Ok(outcome), @@ -1280,12 +1271,11 @@ impl PluginsManager { } } - pub async fn install_plugin_with_remote_sync( + fn resolve_installable_plugin( &self, - config: &PluginsConfigInput, - auth: Option<&CodexAuth>, - request: PluginInstallRequest, - ) -> Result { + config_layer_stack: &ConfigLayerStack, + request: &PluginInstallRequest, + ) -> Result { let resolved = match find_installable_marketplace_plugin( &request.marketplace_path, &request.plugin_name, @@ -1297,6 +1287,32 @@ impl PluginsManager { return Err(err.into()); } }; + if let Err(message) = + MarketplacePolicy::from_requirements(config_layer_stack.requirements()) + .validate_install( + config_layer_stack, + self.codex_home.as_path(), + &request.marketplace_path, + &resolved.plugin_id.marketplace_name, + ) + { + let err = MarketplaceError::InvalidMarketplaceFile { + path: request.marketplace_path.to_path_buf(), + message, + }; + self.track_plugin_install_resolution_failed(&err); + return Err(err.into()); + } + Ok(resolved) + } + + pub async fn install_plugin_with_remote_sync( + &self, + config: &PluginsConfigInput, + auth: Option<&CodexAuth>, + request: PluginInstallRequest, + ) -> Result { + let resolved = self.resolve_installable_plugin(&config.config_layer_stack, &request)?; let plugin_id = resolved.plugin_id.as_key(); // This only forwards the backend mutation before the local install flow. if let Err(err) = crate::remote_legacy::enable_remote_plugin( @@ -1995,21 +2011,18 @@ impl PluginsManager { config: &PluginsConfigInput, marketplace_name: Option<&str>, ) -> Result { - if let Some(marketplace_name) = marketplace_name - && !configured_git_marketplace_names(&config.config_layer_stack) - .iter() - .any(|name| name == marketplace_name) - { - return Err(format!( - "marketplace `{marketplace_name}` is not configured as a Git marketplace" - )); - } - let mut outcome = upgrade_configured_git_marketplaces( self.codex_home.as_path(), &config.config_layer_stack, marketplace_name, ); + if let Some(marketplace_name) = marketplace_name + && outcome.selected_marketplaces.is_empty() + { + return Err(format!( + "marketplace `{marketplace_name}` is not configured as a Git marketplace" + )); + } if !outcome.upgraded_roots.is_empty() { match refresh_non_curated_plugin_cache_force_reinstall( self.codex_home.as_path(), diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 60bbe351d..7131a2c97 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -35,6 +35,9 @@ use codex_config::ConfigRequirementsToml; use codex_config::McpServerConfig; use codex_config::McpServerOAuthConfig; use codex_config::McpServerToolConfig; +use codex_config::RequirementSource; +use codex_config::RequirementsLayerEntry; +use codex_config::compose_requirements; use codex_config::types::McpServerTransportConfig; use codex_core_skills::PluginSkillSnapshots; use codex_core_skills::SkillsLoadInput; @@ -64,6 +67,39 @@ use wiremock::matchers::query_param; const MAX_CAPABILITY_SUMMARY_DESCRIPTION_LEN: usize = 1024; +fn unrestricted_config_layer_stack() -> ConfigLayerStack { + ConfigLayerStack::default() +} + +fn config_layer_stack_with_requirements( + codex_home: &Path, + user_config: &str, + requirements: &str, +) -> ConfigLayerStack { + let with_sources = compose_requirements([RequirementsLayerEntry::from_toml( + RequirementSource::Unknown, + requirements, + )]) + .expect("compose requirements") + .expect("requirements should be present"); + let requirements_toml = with_sources.clone().into_toml(); + let requirements = ConfigRequirements::try_from(with_sources).expect("normalize requirements"); + let config_file = + AbsolutePathBuf::try_from(codex_home.join(CONFIG_TOML_FILE)).expect("absolute config path"); + ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: config_file, + profile: None, + }, + toml::from_str(user_config).expect("parse user config"), + )], + requirements, + requirements_toml, + ) + .expect("build config layer stack") +} + #[test] fn plugins_manager_tracks_auth_mode() { let tmp = TempDir::new().unwrap(); @@ -2402,13 +2438,16 @@ async fn install_plugin_updates_config_with_relative_path_and_plugin_key() { .unwrap(); let result = PluginsManager::new(tmp.path().to_path_buf()) - .install_plugin(PluginInstallRequest { - plugin_name: "sample-plugin".to_string(), - marketplace_path: AbsolutePathBuf::try_from( - repo_root.join(".agents/plugins/marketplace.json"), - ) - .unwrap(), - }) + .install_plugin( + &unrestricted_config_layer_stack(), + PluginInstallRequest { + plugin_name: "sample-plugin".to_string(), + marketplace_path: AbsolutePathBuf::try_from( + repo_root.join(".agents/plugins/marketplace.json"), + ) + .unwrap(), + }, + ) .await .unwrap(); @@ -2428,6 +2467,85 @@ async fn install_plugin_updates_config_with_relative_path_and_plugin_key() { assert!(config.contains("enabled = true")); } +#[tokio::test] +async fn strict_install_requires_allowed_local_marketplace_to_be_added_first() { + let codex_home = TempDir::new().expect("create Codex home"); + let marketplace_root = codex_home.path().join("company-marketplace"); + write_plugin(&marketplace_root, "sample", "sample"); + write_file( + &marketplace_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "company", + "plugins": [ + { + "name": "sample", + "source": {"source": "local", "path": "./sample"} + } + ] +}"#, + ); + let marketplace_root = marketplace_root + .canonicalize() + .expect("canonical marketplace root"); + let requirements = format!( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.company] +source = "local" +path = {marketplace_root:?} +"# + ); + let config = config_layer_stack_with_requirements(codex_home.path(), "", &requirements); + let marketplace_path = + AbsolutePathBuf::try_from(marketplace_root.join(".agents/plugins/marketplace.json")) + .expect("absolute marketplace path"); + let manager = PluginsManager::new(codex_home.path().to_path_buf()); + + let err = manager + .install_plugin( + &config, + PluginInstallRequest { + plugin_name: "sample".to_string(), + marketplace_path: marketplace_path.clone(), + }, + ) + .await + .expect_err("unconfigured local marketplace should not be installable in strict mode"); + assert!(matches!( + err, + PluginInstallError::Marketplace(MarketplaceError::InvalidMarketplaceFile { .. }) + )); + assert!(err.to_string().contains("must be added to config")); + assert!(!codex_home.path().join(CONFIG_TOML_FILE).exists()); + + let user_config = format!( + r#" +[marketplaces.company] +source_type = "local" +source = {marketplace_root:?} +"# + ); + write_file(&codex_home.path().join(CONFIG_TOML_FILE), &user_config); + let config = + config_layer_stack_with_requirements(codex_home.path(), &user_config, &requirements); + let outcome = manager + .install_plugin( + &config, + PluginInstallRequest { + plugin_name: "sample".to_string(), + marketplace_path, + }, + ) + .await + .expect("configured allowlisted marketplace should be installable"); + assert_eq!( + outcome.plugin_id, + PluginId::new("sample".to_string(), "company".to_string()).expect("plugin id") + ); +} + #[tokio::test] async fn install_openai_curated_plugin_uses_short_sha_cache_version() { let tmp = tempfile::tempdir().unwrap(); @@ -2436,13 +2554,16 @@ async fn install_openai_curated_plugin_uses_short_sha_cache_version() { write_curated_plugin_sha(tmp.path(), TEST_CURATED_PLUGIN_SHA); let result = PluginsManager::new(tmp.path().to_path_buf()) - .install_plugin(PluginInstallRequest { - plugin_name: "slack".to_string(), - marketplace_path: AbsolutePathBuf::try_from( - curated_root.join(".agents/plugins/marketplace.json"), - ) - .unwrap(), - }) + .install_plugin( + &unrestricted_config_layer_stack(), + PluginInstallRequest { + plugin_name: "slack".to_string(), + marketplace_path: AbsolutePathBuf::try_from( + curated_root.join(".agents/plugins/marketplace.json"), + ) + .unwrap(), + }, + ) .await .unwrap(); @@ -2494,13 +2615,16 @@ async fn install_plugin_uses_manifest_version_for_non_curated_plugins() { .unwrap(); let result = PluginsManager::new(tmp.path().to_path_buf()) - .install_plugin(PluginInstallRequest { - plugin_name: "sample-plugin".to_string(), - marketplace_path: AbsolutePathBuf::try_from( - repo_root.join(".agents/plugins/marketplace.json"), - ) - .unwrap(), - }) + .install_plugin( + &unrestricted_config_layer_stack(), + PluginInstallRequest { + plugin_name: "sample-plugin".to_string(), + marketplace_path: AbsolutePathBuf::try_from( + repo_root.join(".agents/plugins/marketplace.json"), + ) + .unwrap(), + }, + ) .await .unwrap(); @@ -2554,13 +2678,16 @@ async fn install_plugin_writes_marketplace_manifest_fallback_when_missing_plugin .unwrap(); let result = PluginsManager::new(tmp.path().to_path_buf()) - .install_plugin(PluginInstallRequest { - plugin_name: "quality-review".to_string(), - marketplace_path: AbsolutePathBuf::try_from( - repo_root.join(".agents/plugins/marketplace.json"), - ) - .unwrap(), - }) + .install_plugin( + &unrestricted_config_layer_stack(), + PluginInstallRequest { + plugin_name: "quality-review".to_string(), + marketplace_path: AbsolutePathBuf::try_from( + repo_root.join(".agents/plugins/marketplace.json"), + ) + .unwrap(), + }, + ) .await .unwrap(); @@ -2643,13 +2770,16 @@ async fn install_plugin_supports_git_subdir_marketplace_sources() { .unwrap(); let result = PluginsManager::new(tmp.path().to_path_buf()) - .install_plugin(PluginInstallRequest { - plugin_name: "toolkit".to_string(), - marketplace_path: AbsolutePathBuf::try_from( - repo_root.join(".agents/plugins/marketplace.json"), - ) - .unwrap(), - }) + .install_plugin( + &unrestricted_config_layer_stack(), + PluginInstallRequest { + plugin_name: "toolkit".to_string(), + marketplace_path: AbsolutePathBuf::try_from( + repo_root.join(".agents/plugins/marketplace.json"), + ) + .unwrap(), + }, + ) .await .unwrap(); @@ -2694,13 +2824,16 @@ async fn install_plugin_supports_relative_git_subdir_marketplace_sources() { .unwrap(); let result = PluginsManager::new(tmp.path().to_path_buf()) - .install_plugin(PluginInstallRequest { - plugin_name: "toolkit".to_string(), - marketplace_path: AbsolutePathBuf::try_from( - repo_root.join(".agents/plugins/marketplace.json"), - ) - .unwrap(), - }) + .install_plugin( + &unrestricted_config_layer_stack(), + PluginInstallRequest { + plugin_name: "toolkit".to_string(), + marketplace_path: AbsolutePathBuf::try_from( + repo_root.join(".agents/plugins/marketplace.json"), + ) + .unwrap(), + }, + ) .await .unwrap(); @@ -4094,7 +4227,7 @@ source = "{remote_repo_url}" run_git(&remote_repo, &["add", "."]); run_git(&remote_repo, &["commit", "-m", "update plugin"]); let upgrade = manager - .upgrade_configured_marketplaces_for_config(&config, /*marketplace_name*/ None) + .upgrade_configured_marketplaces_for_config(&config, Some("debug")) .expect("marketplace upgrade should succeed"); assert_eq!(upgrade.errors, Vec::new()); assert_eq!(upgrade.upgraded_roots.len(), 1); diff --git a/codex-rs/core-plugins/src/marketplace_add.rs b/codex-rs/core-plugins/src/marketplace_add.rs index ff7ccbd8f..008927b70 100644 --- a/codex-rs/core-plugins/src/marketplace_add.rs +++ b/codex-rs/core-plugins/src/marketplace_add.rs @@ -1,5 +1,7 @@ use crate::installed_marketplaces::marketplace_install_root; -use crate::is_openai_curated_marketplace_name; +use crate::marketplace_policy::validate_marketplace_name_for_add; +use crate::marketplace_policy::validate_marketplace_source_for_add; +use codex_config::ConfigRequirements; use codex_utils_absolute_path::AbsolutePathBuf; use std::fs; use std::path::Path; @@ -19,7 +21,7 @@ use metadata::MarketplaceInstallMetadata; use metadata::find_marketplace_root_by_name; use metadata::installed_marketplace_root_for_source; use metadata::record_added_marketplace_entry; -use source::MarketplaceSource; +pub(crate) use source::MarketplaceSource; pub(crate) use source::parse_marketplace_source; use source::stage_marketplace_source; use source::validate_marketplace_source_root; @@ -49,11 +51,14 @@ pub enum MarketplaceAddError { pub async fn add_marketplace( codex_home: PathBuf, + requirements: ConfigRequirements, request: MarketplaceAddRequest, ) -> Result { - tokio::task::spawn_blocking(move || add_marketplace_sync(codex_home.as_path(), request)) - .await - .map_err(|err| MarketplaceAddError::Internal(format!("failed to add marketplace: {err}")))? + tokio::task::spawn_blocking(move || { + add_marketplace_sync(codex_home.as_path(), &requirements, request) + }) + .await + .map_err(|err| MarketplaceAddError::Internal(format!("failed to add marketplace: {err}")))? } pub fn is_local_marketplace_source( @@ -68,13 +73,15 @@ pub fn is_local_marketplace_source( fn add_marketplace_sync( codex_home: &Path, + requirements: &ConfigRequirements, request: MarketplaceAddRequest, ) -> Result { - add_marketplace_sync_with_cloner(codex_home, request, clone_git_source) + add_marketplace_sync_with_cloner(codex_home, requirements, request, clone_git_source) } fn add_marketplace_sync_with_cloner( codex_home: &Path, + requirements: &ConfigRequirements, request: MarketplaceAddRequest, clone_source: F, ) -> Result @@ -87,6 +94,9 @@ where sparse_paths, } = request; let source = parse_marketplace_source(&source, ref_name)?; + let managed_marketplace_name = + validate_marketplace_source_for_add(codex_home, requirements, &source) + .map_err(MarketplaceAddError::InvalidRequest)?; if !sparse_paths.is_empty() && !matches!(source, MarketplaceSource::Git { .. }) { return Err(MarketplaceAddError::InvalidRequest( "--sparse is only supported for git marketplace sources".to_string(), @@ -106,6 +116,8 @@ where installed_marketplace_root_for_source(codex_home, &install_root, &install_metadata)? { let marketplace_name = validate_marketplace_source_root(&existing_root)?; + validate_marketplace_name_for_add(managed_marketplace_name, &marketplace_name) + .map_err(MarketplaceAddError::InvalidRequest)?; record_added_marketplace_entry(codex_home, &marketplace_name, &install_metadata)?; return Ok(MarketplaceAddOutcome { marketplace_name, @@ -121,11 +133,8 @@ where if let MarketplaceSource::Local { path } = &source { let marketplace_name = validate_marketplace_source_root(path)?; - if is_openai_curated_marketplace_name(&marketplace_name) { - return Err(MarketplaceAddError::InvalidRequest(format!( - "marketplace '{marketplace_name}' is reserved and cannot be added from this source" - ))); - } + validate_marketplace_name_for_add(managed_marketplace_name, &marketplace_name) + .map_err(MarketplaceAddError::InvalidRequest)?; if find_marketplace_root_by_name(codex_home, &install_root, &marketplace_name)?.is_some() { return Err(MarketplaceAddError::InvalidRequest(format!( "marketplace '{marketplace_name}' is already added from a different source; remove it before adding this source" @@ -165,11 +174,8 @@ where stage_marketplace_source(&source, &sparse_paths, &staged_root, clone_source)?; let marketplace_name = validate_marketplace_source_root(&staged_root)?; - if is_openai_curated_marketplace_name(&marketplace_name) { - return Err(MarketplaceAddError::InvalidRequest(format!( - "marketplace '{marketplace_name}' is reserved and cannot be added from this source" - ))); - } + validate_marketplace_name_for_add(managed_marketplace_name, &marketplace_name) + .map_err(MarketplaceAddError::InvalidRequest)?; let destination = install_root.join(safe_marketplace_dir_name(&marketplace_name)?); ensure_marketplace_destination_is_inside_install_root(&install_root, &destination)?; @@ -178,7 +184,6 @@ where "marketplace '{marketplace_name}' is already added from a different source; remove it before adding this source" ))); } - replace_marketplace_root(&staged_root, &destination).map_err(|err| { MarketplaceAddError::Internal(format!( "failed to install marketplace at {}: {err}", @@ -213,9 +218,23 @@ where mod tests { use super::*; use anyhow::Result; + use codex_config::RequirementSource; + use codex_config::RequirementsLayerEntry; + use codex_config::compose_requirements; use pretty_assertions::assert_eq; + use std::cell::Cell; use tempfile::TempDir; + fn requirements(requirements_toml: &str) -> ConfigRequirements { + let with_sources = compose_requirements([RequirementsLayerEntry::from_toml( + RequirementSource::Unknown, + requirements_toml, + )]) + .expect("compose requirements") + .expect("requirements should be present"); + ConfigRequirements::try_from(with_sources).expect("normalize requirements") + } + #[test] fn add_marketplace_sync_installs_marketplace_and_updates_config() -> Result<()> { let codex_home = TempDir::new()?; @@ -224,6 +243,7 @@ mod tests { let result = add_marketplace_sync_with_cloner( codex_home.path(), + &ConfigRequirements::default(), MarketplaceAddRequest { source: "https://github.com/owner/repo.git".to_string(), ref_name: None, @@ -253,6 +273,47 @@ mod tests { Ok(()) } + #[test] + fn denied_git_marketplace_does_not_clone_or_create_install_root() { + let codex_home = TempDir::new().expect("create Codex home"); + let requirements = requirements( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.company] +source = "git" +url = "https://github.com/example/allowed.git" +"#, + ); + let cloner_called = Cell::new(false); + + let err = add_marketplace_sync_with_cloner( + codex_home.path(), + &requirements, + MarketplaceAddRequest { + source: "https://github.com/example/blocked.git".to_string(), + ref_name: None, + sparse_paths: Vec::new(), + }, + |_url, _ref_name, _sparse_paths, _destination| { + cloner_called.set(true); + Ok(()) + }, + ) + .expect_err("blocked marketplace should fail"); + + assert!(err.to_string().contains("is not allowed by requirements")); + assert!(!cloner_called.get()); + assert!(!marketplace_install_root(codex_home.path()).exists()); + assert!( + !codex_home + .path() + .join(codex_config::CONFIG_TOML_FILE) + .exists() + ); + } + #[test] fn add_marketplace_sync_installs_local_directory_source_and_updates_config() -> Result<()> { let codex_home = TempDir::new()?; @@ -261,6 +322,7 @@ mod tests { let result = add_marketplace_sync_with_cloner( codex_home.path(), + &ConfigRequirements::default(), MarketplaceAddRequest { source: source_root.path().display().to_string(), ref_name: None, @@ -305,6 +367,7 @@ mod tests { let err = add_marketplace_sync_with_cloner( codex_home.path(), + &ConfigRequirements::default(), MarketplaceAddRequest { source: source_root.path().display().to_string(), ref_name: None, @@ -341,16 +404,23 @@ mod tests { ref_name: None, sparse_paths: Vec::new(), }; - let first_result = add_marketplace_sync_with_cloner(codex_home.path(), request.clone(), { + let requirements = ConfigRequirements::default(); + let first_result = add_marketplace_sync_with_cloner( + codex_home.path(), + &requirements, + request.clone(), |_url, _ref_name, _sparse_paths, _destination| { panic!("git cloner should not be called for local marketplace sources") - } - })?; - let second_result = add_marketplace_sync_with_cloner(codex_home.path(), request, { + }, + )?; + let second_result = add_marketplace_sync_with_cloner( + codex_home.path(), + &requirements, + request, |_url, _ref_name, _sparse_paths, _destination| { panic!("git cloner should not be called for local marketplace sources") - } - })?; + }, + )?; assert!(!first_result.already_added); assert!(second_result.already_added); diff --git a/codex-rs/core-plugins/src/marketplace_add/source.rs b/codex-rs/core-plugins/src/marketplace_add/source.rs index 3cec30949..6845129e5 100644 --- a/codex-rs/core-plugins/src/marketplace_add/source.rs +++ b/codex-rs/core-plugins/src/marketplace_add/source.rs @@ -202,7 +202,7 @@ fn is_github_shorthand_segment(segment: &str) -> bool { } impl MarketplaceSource { - pub(super) fn display(&self) -> String { + pub(crate) fn display(&self) -> String { match self { Self::Git { url, ref_name } => match ref_name { Some(ref_name) => format!("{url}#{ref_name}"), diff --git a/codex-rs/core-plugins/src/marketplace_policy.rs b/codex-rs/core-plugins/src/marketplace_policy.rs new file mode 100644 index 000000000..8e41fb893 --- /dev/null +++ b/codex-rs/core-plugins/src/marketplace_policy.rs @@ -0,0 +1,392 @@ +use crate::OPENAI_API_CURATED_MARKETPLACE_NAME; +use crate::OPENAI_BUNDLED_ALPHA_MARKETPLACE_NAME; +use crate::OPENAI_BUNDLED_MARKETPLACE_NAME; +use crate::OPENAI_CURATED_MARKETPLACE_NAME; +use crate::OPENAI_PRIMARY_RUNTIME_MARKETPLACE_NAME; +use crate::installed_marketplaces::marketplace_install_root; +use crate::installed_marketplaces::resolve_configured_marketplace_root; +use crate::is_openai_curated_marketplace_name; +use crate::marketplace::marketplace_root_dir; +use crate::marketplace_add::MarketplaceSource; +use crate::marketplace_add::parse_marketplace_source; +use crate::startup_sync::curated_plugins_api_marketplace_path; +use crate::startup_sync::curated_plugins_repo_path; +use codex_config::ConfigLayerStack; +use codex_config::ConfigRequirements; +use codex_config::MarketplaceAllowedSourceKind; +use codex_config::MarketplaceAllowedSourceToml; +use codex_config::RequirementSource; +use codex_config::types::MarketplaceConfig; +use codex_config::types::MarketplaceSourceType; +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path::paths_match_after_normalization; +use regex::Regex; +use std::path::Path; +use std::path::PathBuf; +use url::Url; + +enum AllowedMarketplaceSource { + GitUrl { + url: String, + ref_name: Option, + }, + GitHostPattern(Regex), + Local(AbsolutePathBuf), +} + +pub(crate) struct MarketplacePolicy { + restricted: Option, +} + +struct RestrictedMarketplacePolicy { + allowed_sources: Result, String>, + source: RequirementSource, +} + +impl MarketplacePolicy { + pub(crate) fn from_requirements(requirements: &ConfigRequirements) -> Self { + let Some(requirements) = requirements.marketplaces.as_ref().filter(|requirements| { + requirements + .value + .restrict_to_allowed_sources + .unwrap_or(false) + }) else { + return Self { restricted: None }; + }; + + let allowed_sources = requirements + .value + .allowed_sources + .iter() + .map(|(key, allowed_source)| { + compile_allowed_source(key, allowed_source, &requirements.source) + }) + .collect(); + Self { + restricted: Some(RestrictedMarketplacePolicy { + allowed_sources, + source: requirements.source.clone(), + }), + } + } + + fn is_restricted(&self) -> bool { + self.restricted.is_some() + } + + fn validate_source(&self, source: &MarketplaceSource) -> Result<(), String> { + let Some(RestrictedMarketplacePolicy { + allowed_sources, + source: requirement_source, + }) = &self.restricted + else { + return Ok(()); + }; + let allowed_sources = allowed_sources.as_ref().map_err(Clone::clone)?; + if allowed_sources + .iter() + .any(|allowed_source| allowed_source.matches(source)) + { + return Ok(()); + } + + Err(format!( + "marketplace source `{}` is not allowed by requirements from {requirement_source}", + source.display() + )) + } + + pub(crate) fn validate_install( + &self, + config_layer_stack: &ConfigLayerStack, + codex_home: &Path, + marketplace_path: &AbsolutePathBuf, + marketplace_name: &str, + ) -> Result<(), String> { + if !self.is_restricted() { + return Ok(()); + } + + let root = marketplace_root_dir(marketplace_path).map_err(|err| err.to_string())?; + if let Some(expected_name) = managed_marketplace_name(codex_home, marketplace_path, &root) { + return validate_expected_marketplace_name(expected_name, marketplace_name); + } + + let user_config = config_layer_stack.effective_user_config().ok_or_else(|| { + format!( + "marketplace `{marketplace_name}` must be added to config before plugins can be installed while marketplace source restrictions are enabled" + ) + })?; + let marketplace = user_config + .get("marketplaces") + .and_then(toml::Value::as_table) + .and_then(|marketplaces| marketplaces.get(marketplace_name)) + .ok_or_else(|| { + format!( + "marketplace `{marketplace_name}` must be added to config before plugins can be installed while marketplace source restrictions are enabled" + ) + })?; + self.validate_configured_marketplace(marketplace_name, marketplace)?; + + let configured_root = resolve_configured_marketplace_root( + marketplace_name, + marketplace, + &marketplace_install_root(codex_home), + ) + .ok_or_else(|| { + format!("configured marketplace `{marketplace_name}` does not have a usable root") + })?; + if !paths_match_after_normalization(&configured_root, root.as_path()) { + return Err(format!( + "marketplace path `{}` does not match configured marketplace `{marketplace_name}`", + root.as_path().display() + )); + } + Ok(()) + } + + pub(crate) fn validate_git_source( + &self, + source: &str, + ref_name: Option, + ) -> Result, String> { + if !self.is_restricted() { + return Ok(None); + } + let source = parse_marketplace_source(source, ref_name).map_err(|err| err.to_string())?; + if !matches!(source, MarketplaceSource::Git { .. }) { + return Err("configured Git marketplace source is not a Git URL".to_string()); + } + self.validate_source(&source)?; + Ok(Some(source)) + } + + fn validate_configured_marketplace( + &self, + marketplace_name: &str, + marketplace: &toml::Value, + ) -> Result<(), String> { + let source = configured_marketplace_source(marketplace_name, marketplace)?; + self.validate_source(&source) + } +} + +impl AllowedMarketplaceSource { + fn matches(&self, source: &MarketplaceSource) -> bool { + match (self, source) { + ( + Self::GitUrl { + url: allowed_url, + ref_name: allowed_ref, + }, + MarketplaceSource::Git { url, ref_name }, + ) => { + allowed_url == url + && allowed_ref + .as_ref() + .is_none_or(|allowed_ref| Some(allowed_ref) == ref_name.as_ref()) + } + (Self::GitHostPattern(pattern), MarketplaceSource::Git { url, .. }) => { + git_hostname(url).is_some_and(|hostname| pattern.is_match(&hostname)) + } + (Self::Local(allowed), MarketplaceSource::Local { path }) => { + paths_match_after_normalization(allowed.as_path(), path) + } + (Self::GitUrl { .. } | Self::GitHostPattern(_), MarketplaceSource::Local { .. }) + | (Self::Local(_), MarketplaceSource::Git { .. }) => false, + } + } +} + +pub(crate) fn validate_marketplace_source_for_add( + codex_home: &Path, + requirements: &ConfigRequirements, + source: &MarketplaceSource, +) -> Result, String> { + let policy = MarketplacePolicy::from_requirements(requirements); + if !policy.is_restricted() { + return Ok(None); + } + if let MarketplaceSource::Local { path } = source + && let Some(expected_name) = managed_local_marketplace_name(codex_home, path) + { + return Ok(Some(expected_name)); + } + policy.validate_source(source)?; + Ok(None) +} + +pub(crate) fn validate_marketplace_name_for_add( + expected_name: Option<&'static str>, + marketplace_name: &str, +) -> Result<(), String> { + if let Some(expected_name) = expected_name { + return validate_expected_marketplace_name(expected_name, marketplace_name); + } + if is_openai_curated_marketplace_name(marketplace_name) { + return Err(format!( + "marketplace `{marketplace_name}` is reserved and cannot be added from this source" + )); + } + Ok(()) +} + +fn compile_allowed_source( + key: &str, + allowed_source: &MarketplaceAllowedSourceToml, + requirement_source: &RequirementSource, +) -> Result { + let invalid = |reason: &str| { + format!("invalid marketplace allowed source `{key}` in {requirement_source}: {reason}") + }; + let source = allowed_source + .source + .ok_or_else(|| invalid("missing source"))?; + match source { + MarketplaceAllowedSourceKind::Git => { + let url = allowed_source + .url + .as_deref() + .map(str::trim) + .filter(|url| !url.is_empty()) + .ok_or_else(|| invalid("missing url"))?; + let ref_name = match allowed_source.ref_name.as_deref() { + Some(ref_name) if ref_name.trim().is_empty() => { + return Err(invalid("ref must not be empty")); + } + Some(ref_name) => Some(ref_name.trim().to_string()), + None => None, + }; + let source = + parse_marketplace_source(url, ref_name).map_err(|err| invalid(&err.to_string()))?; + let MarketplaceSource::Git { url, ref_name } = source else { + return Err(invalid("expected a Git URL")); + }; + Ok(AllowedMarketplaceSource::GitUrl { url, ref_name }) + } + MarketplaceAllowedSourceKind::HostPattern => { + let host_pattern = allowed_source + .host_pattern + .as_deref() + .map(str::trim) + .filter(|host_pattern| !host_pattern.is_empty()) + .ok_or_else(|| invalid("missing host_pattern"))?; + Regex::new(host_pattern) + .map(AllowedMarketplaceSource::GitHostPattern) + .map_err(|err| invalid(&err.to_string())) + } + MarketplaceAllowedSourceKind::Local => { + let path = allowed_source + .path + .as_ref() + .filter(|path| !path.as_os_str().is_empty()) + .ok_or_else(|| invalid("missing path"))?; + if !path.is_absolute() { + return Err(invalid("local path must be absolute")); + } + let path = AbsolutePathBuf::from_absolute_path_checked(path) + .map_err(|_| invalid("local path must be absolute"))?; + Ok(AllowedMarketplaceSource::Local(path)) + } + } +} + +fn configured_marketplace_source( + marketplace_name: &str, + marketplace: &toml::Value, +) -> Result { + let MarketplaceConfig { + source_type, + source, + ref_name, + .. + } = marketplace + .clone() + .try_into() + .map_err(|err| format!("invalid config for marketplace `{marketplace_name}`: {err}"))?; + let source_type = source_type.ok_or_else(|| { + format!("configured marketplace `{marketplace_name}` is missing source_type") + })?; + let source = source + .ok_or_else(|| format!("configured marketplace `{marketplace_name}` is missing source"))?; + match source_type { + MarketplaceSourceType::Local => Ok(MarketplaceSource::Local { + path: PathBuf::from(source), + }), + MarketplaceSourceType::Git => { + let parsed = parse_marketplace_source(&source, ref_name).map_err(|err| { + format!("invalid source for marketplace `{marketplace_name}`: {err}") + })?; + if matches!(parsed, MarketplaceSource::Git { .. }) { + Ok(parsed) + } else { + Err(format!( + "configured marketplace `{marketplace_name}` source does not match source_type `git`" + )) + } + } + } +} + +fn validate_expected_marketplace_name( + expected_name: &str, + marketplace_name: &str, +) -> Result<(), String> { + (marketplace_name == expected_name) + .then_some(()) + .ok_or_else(|| { + format!( + "marketplace manifest name `{marketplace_name}` does not match managed marketplace `{expected_name}`" + ) + }) +} + +fn managed_marketplace_name( + codex_home: &Path, + marketplace_path: &AbsolutePathBuf, + root: &AbsolutePathBuf, +) -> Option<&'static str> { + if paths_match_after_normalization( + marketplace_path.as_path(), + curated_plugins_api_marketplace_path(codex_home), + ) { + return Some(OPENAI_API_CURATED_MARKETPLACE_NAME); + } + if paths_match_after_normalization(root.as_path(), curated_plugins_repo_path(codex_home)) { + return Some(OPENAI_CURATED_MARKETPLACE_NAME); + } + managed_local_marketplace_name(codex_home, root.as_path()) +} + +fn managed_local_marketplace_name(codex_home: &Path, root: &Path) -> Option<&'static str> { + for marketplace_name in [ + OPENAI_BUNDLED_MARKETPLACE_NAME, + OPENAI_BUNDLED_ALPHA_MARKETPLACE_NAME, + ] { + let expected_root = codex_home + .join(".tmp/bundled-marketplaces") + .join(marketplace_name); + if paths_match_after_normalization(root, &expected_root) { + return Some(marketplace_name); + } + } + + let runtime_root = dirs::cache_dir()? + .join("codex-runtimes/codex-primary-runtime/plugins") + .join(OPENAI_PRIMARY_RUNTIME_MARKETPLACE_NAME); + paths_match_after_normalization(root, &runtime_root) + .then_some(OPENAI_PRIMARY_RUNTIME_MARKETPLACE_NAME) +} + +fn git_hostname(url: &str) -> Option { + if let Ok(url) = Url::parse(url) { + return url.host_str().map(str::to_ascii_lowercase); + } + let (_, host_and_path) = url.split_once('@')?; + let (hostname, _) = host_and_path.split_once(':')?; + (!hostname.is_empty()).then(|| hostname.to_ascii_lowercase()) +} + +#[cfg(test)] +#[path = "marketplace_policy_tests.rs"] +mod tests; diff --git a/codex-rs/core-plugins/src/marketplace_policy_tests.rs b/codex-rs/core-plugins/src/marketplace_policy_tests.rs new file mode 100644 index 000000000..706139be9 --- /dev/null +++ b/codex-rs/core-plugins/src/marketplace_policy_tests.rs @@ -0,0 +1,500 @@ +use super::*; +use crate::marketplace_upgrade::upgrade_configured_git_marketplaces; +use codex_app_server_protocol::ConfigLayerSource; +use codex_config::ConfigLayerEntry; +use codex_config::RequirementSource; +use codex_config::RequirementsLayerEntry; +use codex_config::compose_requirements; +use pretty_assertions::assert_eq; +use std::fs; +use tempfile::TempDir; + +fn config_layer_stack(requirements_toml: &str) -> ConfigLayerStack { + config_layer_stack_with_user_config(requirements_toml, /*user_config*/ None) +} + +fn config_layer_stack_with_user_config( + requirements_toml: &str, + user_config: Option<(&str, AbsolutePathBuf)>, +) -> ConfigLayerStack { + let with_sources = compose_requirements([RequirementsLayerEntry::from_toml( + RequirementSource::Unknown, + requirements_toml, + )]) + .expect("compose requirements") + .expect("requirements should be present"); + let requirements_toml = with_sources.clone().into_toml(); + let requirements = + codex_config::ConfigRequirements::try_from(with_sources).expect("normalize requirements"); + let layers = user_config + .map(|(contents, file)| { + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file, + profile: None, + }, + toml::from_str(contents).expect("parse user config"), + )] + }) + .unwrap_or_default(); + ConfigLayerStack::new(layers, requirements, requirements_toml) + .expect("build config layer stack") +} + +fn parse_source(source: &str, ref_name: Option<&str>) -> MarketplaceSource { + parse_marketplace_source(source, ref_name.map(str::to_string)).expect("parse source") +} + +fn validate_source(stack: &ConfigLayerStack, source: &MarketplaceSource) -> Result<(), String> { + MarketplacePolicy::from_requirements(stack.requirements()).validate_source(source) +} + +#[test] +fn exact_git_rule_matches_url_and_ref() { + let stack = config_layer_stack( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.company] +source = "git" +url = "https://github.com/example/plugins" +ref = "main" +"#, + ); + + assert_eq!( + validate_source( + &stack, + &parse_source("https://github.com/example/plugins.git", Some("main")), + ), + Ok(()) + ); + for denied in [ + parse_source("https://github.com/example/plugins.git", Some("release")), + parse_source("https://github.com/other/plugins.git", Some("main")), + ] { + assert!(validate_source(&stack, &denied).is_err()); + } + let normalized = MarketplacePolicy::from_requirements(stack.requirements()) + .validate_git_source("example/plugins", Some("main".to_string())) + .expect("allowlisted shorthand should validate") + .expect("restricted policy should normalize the source"); + assert_eq!( + normalized, + MarketplaceSource::Git { + url: "https://github.com/example/plugins.git".to_string(), + ref_name: Some("main".to_string()), + } + ); +} + +#[test] +fn git_rule_without_ref_allows_any_ref_for_the_same_repository() { + let stack = config_layer_stack( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.company] +source = "git" +url = "https://github.com/example/plugins" +"#, + ); + + assert_eq!( + validate_source( + &stack, + &parse_source("https://github.com/example/plugins.git", Some("release")), + ), + Ok(()) + ); +} + +#[test] +fn git_host_pattern_matches_https_and_ssh_sources() { + let stack = config_layer_stack( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.internal] +source = "host_pattern" +host_pattern = '^git\.example\.com$' +url = "https://github.com/example/ignored.git" +ref = "ignored" +"#, + ); + + for source in [ + "https://git.example.com/team/plugins.git", + "ssh://git@git.example.com/team/plugins.git", + "git@git.example.com:team/plugins.git", + ] { + assert_eq!( + validate_source(&stack, &parse_source(source, /*ref_name*/ None)), + Ok(()) + ); + } + assert!( + validate_source( + &stack, + &parse_source( + "https://github.com/example/plugins.git", + /*ref_name*/ None, + ), + ) + .is_err() + ); +} + +#[test] +fn exact_local_rule_rejects_other_directories() { + let allowed = TempDir::new().expect("create allowed marketplace directory"); + let denied = TempDir::new().expect("create denied marketplace directory"); + let allowed = allowed + .path() + .canonicalize() + .expect("canonical allowed path"); + let denied = denied.path().canonicalize().expect("canonical denied path"); + let stack = config_layer_stack(&format!( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.local] +source = "local" +path = {allowed:?} +"# + )); + + assert_eq!( + validate_source( + &stack, + &parse_source(allowed.to_string_lossy().as_ref(), /*ref_name*/ None), + ), + Ok(()) + ); + assert!( + validate_source( + &stack, + &parse_source(denied.to_string_lossy().as_ref(), /*ref_name*/ None), + ) + .is_err() + ); +} + +#[test] +fn restriction_flag_controls_empty_allowlist() { + for (restricted, expected_allowed) in [(true, false), (false, true)] { + let stack = config_layer_stack(&format!( + r#" +[marketplaces] +restrict_to_allowed_sources = {restricted} +"# + )); + let result = validate_source( + &stack, + &parse_source( + "https://github.com/example/plugins.git", + /*ref_name*/ None, + ), + ); + assert_eq!(result.is_ok(), expected_allowed); + } +} + +#[test] +fn strict_install_validates_configured_name_source_and_root() { + let codex_home = TempDir::new().expect("create Codex home"); + let configured_root = TempDir::new().expect("create configured marketplace"); + let other_root = TempDir::new().expect("create other marketplace"); + let configured_root = configured_root + .path() + .canonicalize() + .expect("canonical configured root"); + let other_root = other_root + .path() + .canonicalize() + .expect("canonical other root"); + let config_file = AbsolutePathBuf::try_from(codex_home.path().join("config.toml")) + .expect("absolute config path"); + let stack = config_layer_stack_with_user_config( + &format!( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.company] +source = "local" +path = {configured_root:?} +"# + ), + Some(( + &format!( + r#" +[marketplaces.company] +source_type = "local" +source = {configured_root:?} +"# + ), + config_file, + )), + ); + let policy = MarketplacePolicy::from_requirements(stack.requirements()); + let configured_path = + AbsolutePathBuf::try_from(configured_root.join(".agents/plugins/marketplace.json")) + .expect("configured marketplace path"); + let other_path = AbsolutePathBuf::try_from(other_root.join(".agents/plugins/marketplace.json")) + .expect("other marketplace path"); + + assert_eq!( + policy.validate_install(&stack, codex_home.path(), &configured_path, "company"), + Ok(()) + ); + assert!( + policy + .validate_install(&stack, codex_home.path(), &configured_path, "other") + .expect_err("unconfigured name should fail") + .contains("must be added to config") + ); + assert!( + policy + .validate_install(&stack, codex_home.path(), &other_path, "company") + .expect_err("mismatched root should fail") + .contains("does not match configured marketplace") + ); +} + +#[test] +fn blocked_configured_source_is_not_installable() { + let codex_home = TempDir::new().expect("create Codex home"); + let config_file = AbsolutePathBuf::try_from(codex_home.path().join("config.toml")) + .expect("absolute config path"); + let stack = config_layer_stack_with_user_config( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.company] +source = "git" +url = "https://github.com/example/allowed.git" +"#, + Some(( + r#" +[marketplaces.debug] +source_type = "git" +source = "https://github.com/example/blocked.git" +"#, + config_file, + )), + ); + let marketplace_path = AbsolutePathBuf::try_from( + marketplace_install_root(codex_home.path()).join("debug/.agents/plugins/marketplace.json"), + ) + .expect("absolute marketplace path"); + + let err = MarketplacePolicy::from_requirements(stack.requirements()) + .validate_install(&stack, codex_home.path(), &marketplace_path, "debug") + .expect_err("blocked marketplace install should fail"); + assert!(err.contains("is not allowed by requirements")); +} + +#[test] +fn bare_relative_local_config_source_is_not_parsed_as_git_shorthand() { + let marketplace: toml::Value = toml::from_str( + r#" +source_type = "local" +source = "marketplaces/company" +"#, + ) + .expect("parse marketplace config"); + + assert_eq!( + configured_marketplace_source("company", &marketplace), + Ok(MarketplaceSource::Local { + path: PathBuf::from("marketplaces/company"), + }) + ); +} + +#[test] +fn curated_marketplace_requires_its_expected_name() { + let codex_home = TempDir::new().expect("create Codex home"); + let stack = config_layer_stack( + r#" +[marketplaces] +restrict_to_allowed_sources = true +"#, + ); + let marketplace_path = AbsolutePathBuf::try_from( + curated_plugins_repo_path(codex_home.path()).join(".agents/plugins/marketplace.json"), + ) + .expect("absolute marketplace path"); + let policy = MarketplacePolicy::from_requirements(stack.requirements()); + + assert_eq!( + policy.validate_install( + &stack, + codex_home.path(), + &marketplace_path, + crate::OPENAI_CURATED_MARKETPLACE_NAME, + ), + Ok(()) + ); + assert!( + policy + .validate_install( + &stack, + codex_home.path(), + &marketplace_path, + crate::OPENAI_API_CURATED_MARKETPLACE_NAME, + ) + .is_err() + ); +} + +#[test] +fn managed_bundled_source_is_bound_to_its_expected_name() { + let codex_home = TempDir::new().expect("create Codex home"); + let bundled_root = codex_home + .path() + .join(".tmp/bundled-marketplaces") + .join(crate::OPENAI_BUNDLED_MARKETPLACE_NAME); + fs::create_dir_all(&bundled_root).expect("create bundled marketplace root"); + let stack = config_layer_stack( + r#" +[marketplaces] +restrict_to_allowed_sources = true +"#, + ); + let source = parse_source( + bundled_root.to_string_lossy().as_ref(), + /*ref_name*/ None, + ); + + let expected_name = + validate_marketplace_source_for_add(codex_home.path(), stack.requirements(), &source) + .expect("managed marketplace source should bypass restrictions"); + assert_eq!( + validate_marketplace_name_for_add(expected_name, crate::OPENAI_BUNDLED_MARKETPLACE_NAME,), + Ok(()) + ); + assert!(validate_marketplace_name_for_add(expected_name, "other").is_err()); +} + +#[test] +fn blocked_upgrade_is_rejected_before_marketplace_installation() { + let codex_home = TempDir::new().expect("create Codex home"); + let config_file = AbsolutePathBuf::try_from(codex_home.path().join("config.toml")) + .expect("absolute config path"); + let stack = config_layer_stack_with_user_config( + r#" +[marketplaces] +restrict_to_allowed_sources = true +"#, + Some(( + r#" +[marketplaces.debug] +source_type = "git" +source = "https://github.com/example/blocked.git" +"#, + config_file, + )), + ); + + let outcome = upgrade_configured_git_marketplaces(codex_home.path(), &stack, Some("debug")); + + assert_eq!(outcome.selected_marketplaces, vec!["debug".to_string()]); + assert_eq!(outcome.upgraded_roots, Vec::new()); + assert_eq!(outcome.errors.len(), 1); + assert!( + outcome.errors[0] + .message + .contains("is not allowed by requirements") + ); + assert!(!marketplace_install_root(codex_home.path()).exists()); +} + +#[test] +fn invalid_active_rule_fails_closed_even_when_another_rule_matches() { + let stack = config_layer_stack( + r#" +[marketplaces] +restrict_to_allowed_sources = true + +[marketplaces.allowed_sources.allowed] +source = "git" +url = "https://github.com/example/plugins.git" + +[marketplaces.allowed_sources.invalid] +source = "host_pattern" +host_pattern = "(" +"#, + ); + + let err = validate_source( + &stack, + &parse_source( + "https://github.com/example/plugins.git", + /*ref_name*/ None, + ), + ) + .expect_err("invalid active rule should fail closed"); + assert!(err.contains("invalid marketplace allowed source `invalid`")); +} + +#[test] +fn invalid_allowed_source_shapes_fail_closed() { + for (rule, expected_error) in [ + ( + r#" +[marketplaces.allowed_sources.invalid] +url = "https://github.com/example/plugins.git" +"#, + "missing source", + ), + ( + r#" +[marketplaces.allowed_sources.invalid] +source = "git" +"#, + "missing url", + ), + ( + r#" +[marketplaces.allowed_sources.invalid] +source = "git" +url = "https://github.com/example/plugins.git" +ref = " " +"#, + "ref must not be empty", + ), + ( + r#" +[marketplaces.allowed_sources.invalid] +source = "local" +path = "../plugins" +"#, + "local path must be absolute", + ), + ] { + let stack = config_layer_stack(&format!( + r#" +[marketplaces] +restrict_to_allowed_sources = true +{rule} +"# + )); + + let err = validate_source( + &stack, + &parse_source( + "https://github.com/example/plugins.git", + /*ref_name*/ None, + ), + ) + .expect_err("invalid rule should fail closed"); + assert!(err.contains(expected_error), "{err}"); + } +} diff --git a/codex-rs/core-plugins/src/marketplace_upgrade.rs b/codex-rs/core-plugins/src/marketplace_upgrade.rs index 010694c8b..01daa90f2 100644 --- a/codex-rs/core-plugins/src/marketplace_upgrade.rs +++ b/codex-rs/core-plugins/src/marketplace_upgrade.rs @@ -6,8 +6,10 @@ use self::activation::installed_marketplace_metadata_matches; use self::activation::write_installed_marketplace_metadata; use self::git::clone_git_source; use self::git::git_remote_revision; -use crate::marketplace::find_marketplace_manifest_path; +use crate::installed_marketplaces::marketplace_install_root; use crate::marketplace::validate_marketplace_root; +use crate::marketplace_add::MarketplaceSource; +use crate::marketplace_policy::MarketplacePolicy; use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigLayerStack; use codex_config::MarketplaceConfigUpdate; @@ -16,13 +18,9 @@ use codex_config::types::MarketplaceConfig; use codex_config::types::MarketplaceSourceType; use codex_plugin::validate_plugin_segment; use codex_utils_absolute_path::AbsolutePathBuf; -use std::collections::HashMap; use std::path::Path; -use std::path::PathBuf; use std::time::Duration; -use tracing::warn; -const INSTALLED_MARKETPLACES_DIR: &str = ".tmp/marketplaces"; const MARKETPLACE_UPGRADE_GIT_TIMEOUT: Duration = Duration::from_secs(30); #[derive(Debug, Clone, PartialEq, Eq)] @@ -47,6 +45,12 @@ struct ConfiguredGitMarketplace { last_revision: Option, } +#[derive(Default)] +struct ConfiguredGitMarketplaceLoadOutcome { + marketplaces: Vec, + errors: Vec, +} + impl ConfiguredMarketplaceUpgradeOutcome { pub fn all_succeeded(&self) -> bool { self.errors.is_empty() @@ -54,7 +58,8 @@ impl ConfiguredMarketplaceUpgradeOutcome { } pub fn configured_git_marketplace_names(config_layer_stack: &ConfigLayerStack) -> Vec { - let mut names = configured_git_marketplaces(config_layer_stack) + let mut names = load_configured_git_marketplaces(config_layer_stack) + .marketplaces .into_iter() .map(|marketplace| marketplace.name) .collect::>(); @@ -67,23 +72,49 @@ pub fn upgrade_configured_git_marketplaces( config_layer_stack: &ConfigLayerStack, marketplace_name: Option<&str>, ) -> ConfiguredMarketplaceUpgradeOutcome { - let marketplaces = configured_git_marketplaces(config_layer_stack) + let loaded = load_configured_git_marketplaces(config_layer_stack); + let marketplaces = loaded + .marketplaces .into_iter() .filter(|marketplace| marketplace_name.is_none_or(|name| marketplace.name.as_str() == name)) .collect::>(); - if marketplaces.is_empty() { + let mut errors = loaded + .errors + .into_iter() + .filter(|error| marketplace_name.is_none_or(|name| error.marketplace_name.as_str() == name)) + .collect::>(); + if marketplaces.is_empty() && errors.is_empty() { return ConfiguredMarketplaceUpgradeOutcome::default(); } let install_root = marketplace_install_root(codex_home); - let selected_marketplaces = marketplaces + let mut selected_marketplaces = marketplaces .iter() .map(|marketplace| marketplace.name.clone()) - .collect(); + .chain(errors.iter().map(|error| error.marketplace_name.clone())) + .collect::>(); + selected_marketplaces.sort_unstable(); + selected_marketplaces.dedup(); let mut upgraded_roots = Vec::new(); - let mut errors = Vec::new(); + let policy = MarketplacePolicy::from_requirements(config_layer_stack.requirements()); for marketplace in marketplaces { - match upgrade_configured_git_marketplace(codex_home, &install_root, &marketplace) { + let normalized_source = + match policy.validate_git_source(&marketplace.source, marketplace.ref_name.clone()) { + Ok(normalized_source) => normalized_source, + Err(message) => { + errors.push(ConfiguredMarketplaceUpgradeError { + marketplace_name: marketplace.name, + message, + }); + continue; + } + }; + match upgrade_configured_git_marketplace( + codex_home, + &install_root, + &marketplace, + normalized_source.as_ref(), + ) { Ok(Some(upgraded_root)) => upgraded_roots.push(upgraded_root), Ok(None) => {} Err(err) => { @@ -102,42 +133,50 @@ pub fn upgrade_configured_git_marketplaces( } } -fn marketplace_install_root(codex_home: &Path) -> PathBuf { - codex_home.join(INSTALLED_MARKETPLACES_DIR) -} - -fn configured_git_marketplaces( +fn load_configured_git_marketplaces( config_layer_stack: &ConfigLayerStack, -) -> Vec { +) -> ConfiguredGitMarketplaceLoadOutcome { let Some(user_config) = config_layer_stack.effective_user_config() else { - return Vec::new(); + return ConfiguredGitMarketplaceLoadOutcome::default(); }; - let Some(marketplaces_value) = user_config.get("marketplaces") else { - return Vec::new(); - }; - let marketplaces = match marketplaces_value - .clone() - .try_into::>() - { - Ok(marketplaces) => marketplaces, - Err(err) => { - warn!("invalid marketplaces config while preparing auto-upgrade: {err}"); - return Vec::new(); - } + let Some(marketplaces) = user_config + .get("marketplaces") + .and_then(toml::Value::as_table) + else { + return ConfiguredGitMarketplaceLoadOutcome::default(); }; - let mut configured = marketplaces - .into_iter() - .filter_map(|(name, marketplace)| configured_git_marketplace_from_config(name, marketplace)) - .collect::>(); - configured.sort_unstable_by(|left, right| left.name.cmp(&right.name)); - configured + let mut outcome = ConfiguredGitMarketplaceLoadOutcome::default(); + for (name, marketplace) in marketplaces { + match parse_configured_git_marketplace(name, marketplace) { + Ok(Some(marketplace)) => outcome.marketplaces.push(marketplace), + Ok(None) => {} + Err(message) => outcome.errors.push(ConfiguredMarketplaceUpgradeError { + marketplace_name: name.clone(), + message, + }), + } + } + outcome + .marketplaces + .sort_unstable_by(|left, right| left.name.cmp(&right.name)); + outcome + .errors + .sort_unstable_by(|left, right| left.marketplace_name.cmp(&right.marketplace_name)); + outcome } -fn configured_git_marketplace_from_config( - name: String, - marketplace: MarketplaceConfig, -) -> Option { +fn parse_configured_git_marketplace( + name: &str, + marketplace: &toml::Value, +) -> Result, String> { + if marketplace.get("source_type").and_then(toml::Value::as_str) != Some("git") { + return Ok(None); + } + let marketplace = marketplace + .clone() + .try_into::() + .map_err(|err| format!("invalid configured Git marketplace: {err}"))?; let MarketplaceConfig { last_updated: _, last_revision, @@ -147,37 +186,37 @@ fn configured_git_marketplace_from_config( sparse_paths, } = marketplace; if source_type != Some(MarketplaceSourceType::Git) { - return None; + return Ok(None); } - let Some(source) = source else { - warn!( - marketplace = name, - "ignoring configured Git marketplace without source" - ); - return None; - }; - Some(ConfiguredGitMarketplace { - name, + let source = + source.ok_or_else(|| "configured Git marketplace is missing source".to_string())?; + Ok(Some(ConfiguredGitMarketplace { + name: name.to_string(), source, ref_name, sparse_paths: sparse_paths.unwrap_or_default(), last_revision, - }) + })) } fn upgrade_configured_git_marketplace( codex_home: &Path, install_root: &Path, marketplace: &ConfiguredGitMarketplace, + normalized_source: Option<&MarketplaceSource>, ) -> Result, String> { validate_plugin_segment(&marketplace.name, "marketplace name")?; - let remote_revision = git_remote_revision( - &marketplace.source, - marketplace.ref_name.as_deref(), - MARKETPLACE_UPGRADE_GIT_TIMEOUT, - )?; + let (source, ref_name) = match normalized_source { + Some(MarketplaceSource::Git { url, ref_name }) => (url.as_str(), ref_name.as_deref()), + Some(MarketplaceSource::Local { .. }) => { + return Err("validated Git marketplace source resolved to a local path".to_string()); + } + None => (marketplace.source.as_str(), marketplace.ref_name.as_deref()), + }; + let remote_revision = git_remote_revision(source, ref_name, MARKETPLACE_UPGRADE_GIT_TIMEOUT)?; let destination = install_root.join(&marketplace.name); - if find_marketplace_manifest_path(&destination).is_some() + if validate_marketplace_root(&destination) + .is_ok_and(|marketplace_name| marketplace_name == marketplace.name) && marketplace.last_revision.as_deref() == Some(remote_revision.as_str()) && installed_marketplace_metadata_matches(&destination, marketplace, &remote_revision) { @@ -202,8 +241,8 @@ fn upgrade_configured_git_marketplace( })?; let activated_revision = clone_git_source( - &marketplace.source, - marketplace.ref_name.as_deref(), + source, + ref_name, &marketplace.sparse_paths, staged_dir.path(), MARKETPLACE_UPGRADE_GIT_TIMEOUT, @@ -280,18 +319,16 @@ fn read_configured_git_marketplace( config_path.display() ) })?; - let Some(marketplaces_value) = config.get("marketplaces") else { + let Some(marketplace) = config + .get("marketplaces") + .and_then(toml::Value::as_table) + .and_then(|marketplaces| marketplaces.get(marketplace_name)) + else { return Ok(None); }; - let mut marketplaces = marketplaces_value - .clone() - .try_into::>() - .map_err(|err| format!("invalid marketplaces config while checking auto-upgrade: {err}"))?; - let Some(marketplace) = marketplaces.remove(marketplace_name) else { - return Ok(None); - }; - Ok(configured_git_marketplace_from_config( - marketplace_name.to_string(), - marketplace, - )) + parse_configured_git_marketplace(marketplace_name, marketplace) } + +#[cfg(test)] +#[path = "marketplace_upgrade_tests.rs"] +mod tests; diff --git a/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs b/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs new file mode 100644 index 000000000..72df8f30a --- /dev/null +++ b/codex-rs/core-plugins/src/marketplace_upgrade_tests.rs @@ -0,0 +1,221 @@ +use super::*; +use codex_app_server_protocol::ConfigLayerSource; +use codex_config::ConfigLayerEntry; +use codex_config::ConfigRequirements; +use codex_config::ConfigRequirementsToml; +use pretty_assertions::assert_eq; +use std::path::Path; +use std::process::Command; +use tempfile::TempDir; + +#[test] +fn readback_ignores_unrelated_malformed_marketplace() { + let codex_home = TempDir::new().expect("create Codex home"); + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#" +[marketplaces.bad] +source_type = "git" +source = 17 + +[marketplaces.good] +source_type = "git" +source = "https://github.com/example/good.git" +ref = "main" +sparse_paths = ["plugins"] +last_revision = "abc123" +"#, + ) + .expect("write config"); + + assert_eq!( + read_configured_git_marketplace(codex_home.path(), "good") + .expect("read configured marketplace"), + Some(ConfiguredGitMarketplace { + name: "good".to_string(), + source: "https://github.com/example/good.git".to_string(), + ref_name: Some("main".to_string()), + sparse_paths: vec!["plugins".to_string()], + last_revision: Some("abc123".to_string()), + }) + ); +} + +#[test] +fn one_upgrade_failure_does_not_block_another_marketplace() { + let codex_home = TempDir::new().expect("create Codex home"); + let remote_repo = TempDir::new().expect("create remote repository"); + init_marketplace_repo(remote_repo.path(), "good"); + let good_url = url::Url::from_directory_path(remote_repo.path()) + .expect("remote repository URL") + .to_string(); + let missing_url = url::Url::from_directory_path(codex_home.path().join("missing-repository")) + .expect("missing repository URL") + .to_string(); + let config = format!( + r#" +[marketplaces.bad] +source_type = "git" +source = {missing_url:?} + +[marketplaces.good] +source_type = "git" +source = {good_url:?} +"# + ); + std::fs::write(codex_home.path().join(CONFIG_TOML_FILE), &config).expect("write config"); + let stack = config_layer_stack(codex_home.path(), &config); + + let outcome = upgrade_configured_git_marketplaces( + codex_home.path(), + &stack, + /*marketplace_name*/ None, + ); + + assert_eq!( + outcome.selected_marketplaces, + vec!["bad".to_string(), "good".to_string()] + ); + assert_eq!(outcome.errors.len(), 1); + assert_eq!(outcome.errors[0].marketplace_name, "bad"); + assert_eq!( + outcome.upgraded_roots, + vec![ + AbsolutePathBuf::try_from(marketplace_install_root(codex_home.path()).join("good")) + .expect("installed marketplace root") + ] + ); +} + +#[test] +fn upgrade_uses_validated_source_for_git_operations() { + let codex_home = TempDir::new().expect("create Codex home"); + let remote_repo = TempDir::new().expect("create remote repository"); + init_marketplace_repo(remote_repo.path(), "good"); + let normalized_url = url::Url::from_directory_path(remote_repo.path()) + .expect("remote repository URL") + .to_string(); + let raw_source = codex_home.path().join("missing-raw-source"); + let raw_source = raw_source.to_string_lossy().into_owned(); + let config = format!( + r#" +[marketplaces.good] +source_type = "git" +source = {raw_source:?} +ref = "missing-ref" +"# + ); + std::fs::write(codex_home.path().join(CONFIG_TOML_FILE), config).expect("write config"); + let marketplace = ConfiguredGitMarketplace { + name: "good".to_string(), + source: raw_source, + ref_name: Some("missing-ref".to_string()), + sparse_paths: Vec::new(), + last_revision: None, + }; + let normalized_source = MarketplaceSource::Git { + url: normalized_url, + ref_name: Some("HEAD".to_string()), + }; + let install_root = marketplace_install_root(codex_home.path()); + + let upgraded_root = upgrade_configured_git_marketplace( + codex_home.path(), + &install_root, + &marketplace, + Some(&normalized_source), + ) + .expect("upgrade should use the validated source") + .expect("marketplace should be upgraded"); + + assert_eq!( + upgraded_root, + AbsolutePathBuf::try_from(install_root.join("good")).expect("installed marketplace root") + ); +} + +#[test] +fn up_to_date_fast_path_validates_marketplace_name() { + const REVISION: &str = "0123456789abcdef0123456789abcdef01234567"; + let codex_home = TempDir::new().expect("create Codex home"); + let install_root = marketplace_install_root(codex_home.path()); + let destination = install_root.join("good"); + let manifest_dir = destination.join(".agents/plugins"); + std::fs::create_dir_all(&manifest_dir).expect("create marketplace manifest directory"); + std::fs::write( + manifest_dir.join("marketplace.json"), + r#"{"name":"wrong","plugins":[]}"#, + ) + .expect("write mismatched marketplace manifest"); + let missing_source = codex_home.path().join("missing-source"); + let missing_source = missing_source.to_string_lossy().into_owned(); + let marketplace = ConfiguredGitMarketplace { + name: "good".to_string(), + source: missing_source.clone(), + ref_name: Some(REVISION.to_string()), + sparse_paths: Vec::new(), + last_revision: Some(REVISION.to_string()), + }; + super::activation::write_installed_marketplace_metadata(&destination, &marketplace, REVISION) + .expect("write installed marketplace metadata"); + let normalized_source = MarketplaceSource::Git { + url: missing_source, + ref_name: Some(REVISION.to_string()), + }; + + let err = upgrade_configured_git_marketplace( + codex_home.path(), + &install_root, + &marketplace, + Some(&normalized_source), + ) + .expect_err("mismatched marketplace name must not use the up-to-date fast path"); + + assert!(err.contains("git clone marketplace source failed")); +} + +fn config_layer_stack(codex_home: &Path, config: &str) -> ConfigLayerStack { + let config_file = + AbsolutePathBuf::try_from(codex_home.join(CONFIG_TOML_FILE)).expect("absolute config path"); + ConfigLayerStack::new( + vec![ConfigLayerEntry::new( + ConfigLayerSource::User { + file: config_file, + profile: None, + }, + toml::from_str(config).expect("parse config"), + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("build config layer stack") +} + +fn init_marketplace_repo(repo: &Path, marketplace_name: &str) { + let manifest_dir = repo.join(".agents/plugins"); + std::fs::create_dir_all(&manifest_dir).expect("create marketplace manifest directory"); + std::fs::write( + manifest_dir.join("marketplace.json"), + format!(r#"{{"name":"{marketplace_name}","plugins":[]}}"#), + ) + .expect("write marketplace manifest"); + run_git(repo, &["init"]); + run_git(repo, &["config", "user.email", "codex-test@example.com"]); + run_git(repo, &["config", "user.name", "Codex Test"]); + run_git(repo, &["add", "."]); + run_git(repo, &["commit", "-m", "initial"]); +} + +fn run_git(repo: &Path, args: &[&str]) { + let output = Command::new("git") + .arg("-C") + .arg(repo) + .args(args) + .output() + .expect("run git"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} diff --git a/codex-rs/core/src/tools/handlers/request_plugin_install_tests.rs b/codex-rs/core/src/tools/handlers/request_plugin_install_tests.rs index 62c8afaa6..f7eadd150 100644 --- a/codex-rs/core/src/tools/handlers/request_plugin_install_tests.rs +++ b/codex-rs/core/src/tools/handlers/request_plugin_install_tests.rs @@ -39,13 +39,16 @@ async fn verified_plugin_install_completed_requires_installed_plugin() { )); plugins_manager - .install_plugin(PluginInstallRequest { - plugin_name: "sample".to_string(), - marketplace_path: AbsolutePathBuf::try_from( - curated_root.join(".agents/plugins/marketplace.json"), - ) - .expect("marketplace path"), - }) + .install_plugin( + &config.config_layer_stack, + PluginInstallRequest { + plugin_name: "sample".to_string(), + marketplace_path: AbsolutePathBuf::try_from( + curated_root.join(".agents/plugins/marketplace.json"), + ) + .expect("marketplace path"), + }, + ) .await .expect("plugin should install");