From 6cf2ff11b391fdd83d688b1740623ce55acf1a85 Mon Sep 17 00:00:00 2001 From: papayo-oai Date: Fri, 18 Sep 2026 17:05:56 +0000 Subject: [PATCH] Bind executor plugin measurements to the trusted plugin version (#46528) ## Why Identical script contents across plugin versions do not establish that the same measurement declaration applies. Resolving measurements through generic command attribution could use a different version's declaration, while shared helper paths could make attribution ambiguous. ## What changed - Match canonical executor paths against trusted plugin identities before comparing script contents. Require an exact version match for measurements while retaining attribution across versions with matching contents. - Keep measurement declarations bound to the selected trusted root, and allow distinct versions to coexist when extending trusted roots. - Add `PluginMeasurementTarget` to extract an untrusted plugin/version hint from canonical remote cache paths, respecting Windows and POSIX path conventions. - Skip executor lookups for unrelated scripts so attribution does not wait for executor provisioning. - Increase the login unit test timeout in Bazel to `long`. ## Testing Add regression coverage for multiple plugins and versions, canonical aliases, symlink escapes, path casing, and executor lookup avoidance. Add a remote execution integration test verifying that mismatched versions retain command attribution but receive no metrics sidecar, and matching versions use the trusted measurement declaration. GitOrigin-RevId: d63430b37bf2de360c8d09af7f7453a5b5024d72 --- codex-rs/core-plugins/src/lib.rs | 1 + .../core-plugins/src/script_attribution.rs | 327 ++++++++--- .../src/script_attribution_tests.rs | 509 +++++++++++++++++- codex-rs/core/tests/suite/plugins.rs | 161 ++++++ codex-rs/login/BUILD.bazel | 1 + 5 files changed, 900 insertions(+), 99 deletions(-) diff --git a/codex-rs/core-plugins/src/lib.rs b/codex-rs/core-plugins/src/lib.rs index 20b6fc0e47..c67f33e343 100644 --- a/codex-rs/core-plugins/src/lib.rs +++ b/codex-rs/core-plugins/src/lib.rs @@ -108,5 +108,6 @@ pub use remote::RecommendedPlugin; pub use remote::RecommendedPluginsMode; pub use remote_metadata::remote_catalog_metadata_eq; pub use script_attribution::PluginCommandAttribution; +pub use script_attribution::PluginMeasurementTarget; pub use script_attribution::TrustedPluginRoots; pub use script_attribution::command_script_arguments; diff --git a/codex-rs/core-plugins/src/script_attribution.rs b/codex-rs/core-plugins/src/script_attribution.rs index 97e752d661..9a10ea77bc 100644 --- a/codex-rs/core-plugins/src/script_attribution.rs +++ b/codex-rs/core-plugins/src/script_attribution.rs @@ -16,6 +16,7 @@ use crate::startup_sync::read_curated_plugins_sha; use crate::store::DEFAULT_PLUGIN_VERSION; use crate::store::PluginStore; use crate::store::plugin_version_for_source; +use crate::store::validate_plugin_version_segment; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::GetMetadataOptions; use codex_exec_server::ReadFileOptions; @@ -25,6 +26,7 @@ use codex_shell_command::bash::extract_bash_command; use codex_shell_command::bash::parse_shell_lc_plain_commands; use codex_shell_command::parse_command::is_pathish; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; use std::collections::BTreeMap; use std::collections::HashSet; @@ -34,6 +36,7 @@ use std::path::Path; #[derive(Clone, Debug, PartialEq, Eq)] struct TrustedPluginRoot { plugin_id: PluginId, + version: String, root: AbsolutePathBuf, metrics_operations_by_path: BTreeMap, } @@ -63,7 +66,81 @@ pub struct TrustedPluginRoots { roots: Vec, } +/// An untrusted hint from a canonical executor script path, never authorization. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct PluginMeasurementTarget { + pub(crate) plugin_id: PluginId, + pub(crate) version: String, + pub(crate) path_convention: PathConvention, +} + +impl PluginMeasurementTarget { + /// Selects one remote plugin using the same command parser as attribution. + /// Skip paths outside the remote plugin cache before consulting the executor. + pub async fn from_command( + command: &[String], + cwd: &PathUri, + file_system: &dyn ExecutorFileSystem, + ) -> Option { + let command = single_plain_command(command)?; + let invocation = script_invocation(&command)?; + let script = cwd.join(invocation.script).ok()?; + Self::from_script_path(&script)?; + let script = file_system + .canonicalize(&script, /*sandbox*/ None) + .await + .ok()?; + Self::from_script_path(&script) + } + + fn from_script_path(script: &PathUri) -> Option { + let path_convention = script.infer_path_convention()?; + let native_path = script.inferred_native_path_string(); + // Normalize only the hint so equivalent Windows paths share preparation. + // Store identities must retain the authenticated catalog's spelling. + let native_path = match path_convention { + PathConvention::Windows => native_path.to_ascii_lowercase(), + PathConvention::Posix => native_path, + }; + let components = path_convention + .path_segments(&native_path) + .collect::>(); + let mut candidates = components.windows(6).filter_map(|parts| { + if parts[..3] != ["plugins", "cache", REMOTE_GLOBAL_MARKETPLACE_NAME] + || parts[4] == DEFAULT_PLUGIN_VERSION + { + return None; + } + validate_plugin_version_segment(parts[4]).ok()?; + Some(Self { + plugin_id: PluginId::new(parts[3].to_owned(), parts[2].to_owned()).ok()?, + version: parts[4].to_owned(), + path_convention, + }) + }); + let target = candidates.next()?; + candidates.next().is_none().then_some(target) + } +} + impl TrustedPluginRoots { + /// Adds already-trusted roots, preserving the existing root for each plugin and version. + /// Distinct versions remain eligible for exact executor-version matching. + pub fn extend(&mut self, other: &Self) { + let mut seen = self + .roots + .iter() + .map(|root| (root.plugin_id.as_key(), root.version.clone())) + .collect::>(); + self.roots.extend( + other + .roots + .iter() + .filter(|root| seen.insert((root.plugin_id.as_key(), root.version.clone()))) + .cloned(), + ); + } + pub fn from_plugin_load_outcome(loaded_plugins: &PluginLoadOutcome, codex_home: &Path) -> Self { let primary_runtime_marketplace_root = primary_runtime_marketplace_root(); let Ok(store) = PluginStore::try_new(codex_home.to_path_buf()) else { @@ -85,9 +162,11 @@ impl TrustedPluginRoots { if plugin.root != expected_root || !expected_root.as_path().is_dir() { return None; } + let version = expected_root.as_path().file_name()?.to_str()?.to_owned(); let root = expected_root.canonicalize().ok()?; root.as_path().is_dir().then(|| TrustedPluginRoot { plugin_id, + version, metrics_operations_by_path: load_plugin_metrics_operations(&root) .unwrap_or_default(), root, @@ -175,6 +254,24 @@ impl TrustedPluginRoots { command: &[String], cwd: &AbsolutePathBuf, ) -> Option { + self.local_candidate(command, cwd) + .map(|candidate| candidate.attribution()) + } + + /// Resolves one exact command to one trusted manifest-declared operation. + pub fn resolve_metrics_operation( + &self, + command: &[String], + cwd: &AbsolutePathBuf, + ) -> Option { + self.local_candidate(command, cwd)?.metrics_operation() + } + + fn local_candidate( + &self, + command: &[String], + cwd: &AbsolutePathBuf, + ) -> Option> { let command = single_plain_command(command)?; let invocation = script_invocation(command.as_slice())?; let script = if Path::new(invocation.script).is_absolute() { @@ -194,8 +291,8 @@ impl TrustedPluginRoots { .strip_prefix(root.root.as_path()) .ok() .filter(|relative_path| !relative_path.as_os_str().is_empty())?; - Some(PluginCommandAttribution { - plugin_id: root.plugin_id.clone(), + Some(MatchedPluginScript { + root, normalized_relative_path: normalized_relative_script_path(relative_path)?, }) }); @@ -203,58 +300,67 @@ impl TrustedPluginRoots { matches.next().is_none().then_some(attribution) } - /// Resolves one exact command to one trusted manifest-declared operation. - pub fn resolve_metrics_operation( - &self, - command: &[String], - cwd: &AbsolutePathBuf, - ) -> Option { - let attribution = self.resolve_attribution(command, cwd)?; - self.metrics_operation_for_attribution(attribution) - } - - fn metrics_operation_for_attribution( - &self, - attribution: PluginCommandAttribution, - ) -> Option { - let mut matches = self.roots.iter().filter_map(|root| { - (root.plugin_id == attribution.plugin_id) - .then(|| { - root.metrics_operations_by_path - .get(&attribution.normalized_relative_path) - }) - .flatten() - }); - let operation = matches.next()?.clone(); - matches - .next() - .is_none() - .then_some(ResolvedPluginMetricsOperation { - plugin_id: attribution.plugin_id, - operation, - }) - } - /// Resolves a trusted script on the selected executor filesystem. /// /// Remote commands can use a path convention that the app-server host cannot - /// canonicalize. Match the target-native path to one trusted local plugin - /// script, then require the executor-side file to have the same contents. + /// canonicalize. Resolve aliases on that executor first, then match the + /// canonical target to a trusted local plugin script with the same contents. + /// Generic attribution accepts matching script bytes across plugin versions. pub async fn resolve_executor_attribution( &self, command: &[String], cwd: &PathUri, file_system: &dyn ExecutorFileSystem, ) -> Option { + self.executor_candidate(command, cwd, file_system, PluginVersionMatch::Any) + .await + .map(|candidate| candidate.matched.attribution()) + } + + /// Resolves one trusted executor script to one manifest-declared operation, + /// requiring the executor's version to match the trusted declaration's version. + pub async fn resolve_metrics_operation_in_filesystem( + &self, + command: &[String], + cwd: &PathUri, + file_system: &dyn ExecutorFileSystem, + ) -> Option { + self.executor_candidate(command, cwd, file_system, PluginVersionMatch::Exact) + .await? + .matched + .metrics_operation() + } + + async fn executor_candidate( + &self, + command: &[String], + cwd: &PathUri, + file_system: &dyn ExecutorFileSystem, + version_match: PluginVersionMatch, + ) -> Option> { + if self.roots.is_empty() { + return None; + } let command = single_plain_command(command)?; let invocation = script_invocation(command.as_slice())?; let script = cwd.join(invocation.script).ok()?; - let candidate = self.local_candidate_for_executor_script(&script)?; + // Preserve known script suffixes (including directory aliases), but avoid + // an executor round trip for unrelated workspace scripts. + let suffixes = normalized_script_suffixes(&script); + if !self.roots.iter().any(|root| { + suffixes.iter().any(|suffix| { + executor_plugin_root_matches(&script, root, suffix, PluginVersionMatch::Any) + || root.root.join(suffix).as_path().is_file() + }) + }) { + return None; + } let script = file_system .canonicalize(&script, /*sandbox*/ None) .await .ok()?; - if !executor_plugin_root_matches(&script, &candidate.attribution) { + let candidates = self.local_candidates_for_executor_script(&script, version_match); + if candidates.is_empty() { return None; } let metadata = file_system @@ -265,62 +371,98 @@ impl TrustedPluginRoots { ) .await .ok()?; - if !metadata.is_file || metadata.size != candidate.contents.len() as u64 { + if !metadata.is_file + || !candidates + .iter() + .any(|candidate| metadata.size == candidate.contents.len() as u64) + { return None; } let contents = file_system .read_file(&script, ReadFileOptions::default(), /*sandbox*/ None) .await .ok()?; - (contents == candidate.contents).then_some(candidate.attribution) + let mut matches = candidates + .into_iter() + .filter(|candidate| contents == candidate.contents); + let candidate = matches.next()?; + match version_match { + PluginVersionMatch::Any => matches + .all(|other| other.matched.attribution() == candidate.matched.attribution()) + .then_some(candidate), + PluginVersionMatch::Exact => matches.next().is_none().then_some(candidate), + } } - /// Resolves one trusted executor script to one manifest-declared operation. - pub async fn resolve_metrics_operation_in_filesystem( - &self, - command: &[String], - cwd: &PathUri, - file_system: &dyn ExecutorFileSystem, - ) -> Option { - let attribution = self - .resolve_executor_attribution(command, cwd, file_system) - .await?; - self.metrics_operation_for_attribution(attribution) - } - - fn local_candidate_for_executor_script( + fn local_candidates_for_executor_script( &self, script: &PathUri, - ) -> Option { + version_match: PluginVersionMatch, + ) -> Vec> { let suffixes = normalized_script_suffixes(script); - let mut matches = self.roots.iter().filter_map(|root| { - let (script, normalized_relative_path) = suffixes.iter().find_map(|suffix| { - let script = root.root.join(suffix).canonicalize().ok()?; - let relative_path = script.as_path().strip_prefix(root.root.as_path()).ok()?; - if !script.as_path().is_file() { - return None; - } - let normalized_relative_path = normalized_relative_script_path(relative_path)?; - Some((script, normalized_relative_path)) - })?; - Some(ExecutorAttributionCandidate { - attribution: PluginCommandAttribution { - plugin_id: root.plugin_id.clone(), - normalized_relative_path, - }, - contents: std::fs::read(script.as_path()).ok()?, + self.roots + .iter() + .filter_map(|root| { + let (script, normalized_relative_path) = suffixes.iter().find_map(|suffix| { + if !executor_plugin_root_matches(script, root, suffix, version_match) { + return None; + } + let script = root.root.join(suffix).canonicalize().ok()?; + let relative_path = script.as_path().strip_prefix(root.root.as_path()).ok()?; + if !script.as_path().is_file() { + return None; + } + let normalized_relative_path = normalized_relative_script_path(relative_path)?; + Some((script, normalized_relative_path)) + })?; + Some(ExecutorAttributionCandidate { + matched: MatchedPluginScript { + root, + normalized_relative_path, + }, + contents: std::fs::read(script.as_path()).ok()?, + }) }) - }); - let candidate = matches.next()?; - matches.next().is_none().then_some(candidate) + .collect() } } -struct ExecutorAttributionCandidate { - attribution: PluginCommandAttribution, +struct MatchedPluginScript<'a> { + root: &'a TrustedPluginRoot, + normalized_relative_path: String, +} + +impl MatchedPluginScript<'_> { + fn attribution(&self) -> PluginCommandAttribution { + PluginCommandAttribution { + plugin_id: self.root.plugin_id.clone(), + normalized_relative_path: self.normalized_relative_path.clone(), + } + } + + fn metrics_operation(&self) -> Option { + Some(ResolvedPluginMetricsOperation { + plugin_id: self.root.plugin_id.clone(), + operation: self + .root + .metrics_operations_by_path + .get(&self.normalized_relative_path)? + .clone(), + }) + } +} + +struct ExecutorAttributionCandidate<'a> { + matched: MatchedPluginScript<'a>, contents: Vec, } +#[derive(Clone, Copy)] +enum PluginVersionMatch { + Any, + Exact, +} + fn normalized_script_suffixes(script: &PathUri) -> Vec { let path = script.inferred_native_path_string().replace('\\', "/"); let components = path @@ -335,24 +477,31 @@ fn normalized_script_suffixes(script: &PathUri) -> Vec { .collect() } -fn executor_plugin_root_matches(script: &PathUri, attribution: &PluginCommandAttribution) -> bool { - let relative_depth = attribution.normalized_relative_path.split('/').count(); +fn executor_plugin_root_matches( + script: &PathUri, + trusted: &TrustedPluginRoot, + relative_path: &str, + version_match: PluginVersionMatch, +) -> bool { + let relative_depth = relative_path.split('/').count(); let Some(root) = script.ancestors().nth(relative_depth) else { return false; }; - let path = root.inferred_native_path_string().replace('\\', "/"); - let components = path - .split('/') - .filter(|component| !component.is_empty()) - .collect::>(); - let [.., plugins, cache, marketplace, plugin, version] = components.as_slice() else { + let Some(cache_parent) = root.ancestors().nth(5) else { return false; }; - *plugins == "plugins" - && *cache == "cache" - && *marketplace == attribution.plugin_id.marketplace_name.as_str() - && *plugin == attribution.plugin_id.plugin_name.as_str() - && !version.is_empty() + let Ok(plugin_root) = cache_parent.join(&format!( + "plugins/cache/{}/{}", + trusted.plugin_id.marketplace_name, trusted.plugin_id.plugin_name + )) else { + return false; + }; + match version_match { + PluginVersionMatch::Any => root.parent().as_ref() == Some(&plugin_root), + PluginVersionMatch::Exact => plugin_root + .join(&trusted.version) + .is_ok_and(|expected| expected == root), + } } /// Returns the structurally parsed arguments following a single script command. diff --git a/codex-rs/core-plugins/src/script_attribution_tests.rs b/codex-rs/core-plugins/src/script_attribution_tests.rs index 8e79f8c630..b4fc63abcf 100644 --- a/codex-rs/core-plugins/src/script_attribution_tests.rs +++ b/codex-rs/core-plugins/src/script_attribution_tests.rs @@ -10,15 +10,24 @@ use crate::test_support::TEST_CURATED_PLUGIN_SHA; use crate::test_support::write_curated_plugin_sha_with; use crate::test_support::write_openai_api_curated_marketplace; use crate::test_support::write_openai_curated_marketplace; +use codex_exec_server::ExecServerError; +use codex_exec_server::LOCAL_FS; +use codex_exec_server::NoiseChannelPublicKey; +use codex_exec_server::NoiseRendezvousConnectBundle; +use codex_exec_server::NoiseRendezvousConnectProvider; +use codex_exec_server_test_support::environment_manager_without_environments; use codex_plugin::PluginLoadOutcome; use codex_utils_path_uri::PathUri; use codex_utils_plugins::SkillDiscoveryMode; +use futures::FutureExt; +use futures::future::BoxFuture; use pretty_assertions::assert_eq; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; use std::collections::HashSet; use std::fs; +use std::sync::Arc; use tempfile::TempDir; const ENABLED: bool = true; const DISABLED: bool = false; @@ -151,6 +160,7 @@ fn resolves_primary_runtime_scripts_from_the_installed_plugin_cache() { let roots = TrustedPluginRoots { roots: vec![TrustedPluginRoot { plugin_id: plugin_id.clone(), + version: "0.1.29".to_string(), metrics_operations_by_path: BTreeMap::new(), root: plugin_root.canonicalize().expect("canonical plugin root"), }], @@ -193,22 +203,20 @@ async fn resolves_relocated_script_through_executor_filesystem() { let executor = TempDir::new().expect("executor temp dir"); let executor_root = executor .path() - .join("plugins/cache/openai-curated/sample/remote-version"); + .join("plugins/cache/openai-curated/sample") + .join(curated_plugin_cache_version(TEST_CURATED_PLUGIN_SHA)); let executor_script = executor_root.join("scripts/run.py"); fs::create_dir_all(executor_script.parent().expect("script parent")) .expect("create executor scripts"); fs::copy(script.as_path(), &executor_script).expect("copy script to executor"); let cwd = PathUri::from_host_native_path(&executor_root).expect("executor root URI"); - let environment = - codex_exec_server::Environment::create_for_tests(/*exec_server_url*/ None) - .expect("local executor environment"); assert_eq!( roots .resolve_executor_attribution( &command(&["python", "scripts/run.py"]), &cwd, - environment.get_filesystem().as_ref(), + LOCAL_FS.as_ref(), ) .await, Some(PluginCommandAttribution { @@ -223,7 +231,7 @@ async fn resolves_relocated_script_through_executor_filesystem() { .resolve_executor_attribution( &command(&["python", "scripts/run.py"]), &cwd, - environment.get_filesystem().as_ref(), + LOCAL_FS.as_ref(), ) .await, None @@ -232,17 +240,35 @@ async fn resolves_relocated_script_through_executor_filesystem() { #[test] fn recognizes_windows_executor_plugin_cache_root() { - let attribution = PluginCommandAttribution { + let reference = TempDir::new().expect("reference root"); + let trusted = TrustedPluginRoot { plugin_id: PluginId::parse("presentations@openai-primary-runtime").expect("plugin id"), - normalized_relative_path: - "skills/presentations/container_tools/mark_artifact_operation_started.mjs".to_string(), + version: "0.1.29".to_string(), + root: path(reference.path()), + metrics_operations_by_path: BTreeMap::new(), }; + let relative_path = "skills/presentations/container_tools/mark_artifact_operation_started.mjs"; let script = PathUri::parse( "file:///C:/Users/user/.codex/plugins/cache/openai-primary-runtime/presentations/0.1.29/skills/presentations/container_tools/mark_artifact_operation_started.mjs", ) .expect("Windows script URI"); - assert!(executor_plugin_root_matches(&script, &attribution)); + assert!(executor_plugin_root_matches( + &script, + &trusted, + relative_path, + PluginVersionMatch::Exact, + )); + let wrong_version = PathUri::parse( + "file:///C:/Users/user/.codex/plugins/cache/openai-primary-runtime/presentations/0.1.28/skills/presentations/container_tools/mark_artifact_operation_started.mjs", + ) + .expect("other-version Windows script URI"); + assert!(!executor_plugin_root_matches( + &wrong_version, + &trusted, + relative_path, + PluginVersionMatch::Exact, + )); } fn assert_invalid_metrics_manifest(codex_home: &Path, root: &AbsolutePathBuf, manifest: &str) { fs::write(root.join("analytics.yaml"), manifest).expect("write analytics manifest"); @@ -321,17 +347,20 @@ fn trusted_roots_require_verified_curated_or_remote_cache() { vec![ TrustedPluginRoot { plugin_id: PluginId::parse("sample@openai-curated").expect("plugin id"), + version: curated_plugin_cache_version(TEST_CURATED_PLUGIN_SHA), metrics_operations_by_path: BTreeMap::new(), root: root.canonicalize().expect("canonical root"), }, TrustedPluginRoot { plugin_id: PluginId::parse("api-sample@openai-api-curated").expect("plugin id"), + version: curated_plugin_cache_version(TEST_CURATED_PLUGIN_SHA), metrics_operations_by_path: BTreeMap::new(), root: api_root.canonicalize().expect("canonical root"), }, TrustedPluginRoot { plugin_id: PluginId::parse("remote-sample@openai-curated-remote") .expect("plugin id"), + version: "1.2.3".to_string(), metrics_operations_by_path: BTreeMap::new(), root: remote_root.canonicalize().expect("canonical root"), }, @@ -651,11 +680,13 @@ fn rejects_ambiguous_commands_overlaps_and_symlink_escapes() { roots: vec![ TrustedPluginRoot { plugin_id: PluginId::parse("sample@openai-curated").expect("plugin id"), + version: curated_plugin_cache_version(TEST_CURATED_PLUGIN_SHA), metrics_operations_by_path: BTreeMap::new(), root: root.canonicalize().expect("canonical root"), }, TrustedPluginRoot { plugin_id: PluginId::parse("nested@openai-curated").expect("plugin id"), + version: curated_plugin_cache_version(TEST_CURATED_PLUGIN_SHA), metrics_operations_by_path: BTreeMap::new(), root: root.join("scripts").canonicalize().expect("nested root"), }, @@ -688,3 +719,461 @@ fn rejects_ambiguous_commands_overlaps_and_symlink_escapes() { } } } + +const REFERENCE_HELPER: &str = "scripts/install-dependencies.sh"; + +fn reference_metrics_operation(variant: &str) -> PluginMetricsOperation { + PluginMetricsOperation { + operation_name: "dependency_install".to_string(), + measurements: BTreeMap::from([( + "duration_ms".to_string(), + PluginMeasurementDefinition { + enum_dimensions: BTreeMap::from([( + "variant".to_string(), + BTreeSet::from([variant.to_string()]), + )]), + }, + )]), + } +} + +fn reference_identity_fixture(root: &Path, variant: &str, contents: &str) -> AbsolutePathBuf { + fs::create_dir_all(root.join("scripts")).expect("create reference scripts"); + fs::write(root.join(REFERENCE_HELPER), contents).expect("write helper"); + fs::write( + root.join("analytics.yaml"), + format!( + "version: 1\noperations:\n dependency_install:\n path: ./{REFERENCE_HELPER}\n measurements:\n duration_ms:\n dimensions:\n variant: [{variant}]\n" + ), + ) + .expect("write operation manifest"); + path(root).canonicalize().expect("canonical reference root") +} + +fn reference_fixture(bundles: &[(&str, &str, &str, &str)]) -> (TempDir, TrustedPluginRoots) { + let home = TempDir::new().expect("references"); + let roots = TrustedPluginRoots { + roots: bundles + .iter() + .map(|(name, version, variant, contents)| { + let root = reference_identity_fixture( + &home.path().join(name).join(version), + variant, + contents, + ); + TrustedPluginRoot { + plugin_id: PluginId::parse(&format!("{name}@openai-curated-remote")) + .expect("plugin id"), + version: version.to_string(), + metrics_operations_by_path: load_plugin_metrics_operations(&root) + .expect("valid fixture analytics"), + root, + } + }) + .collect(), + }; + (home, roots) +} + +fn executor_plugin(home: &Path, name: &str, version: &str, contents: &str) -> AbsolutePathBuf { + reference_identity_fixture( + &home.join(format!( + "plugins/cache/openai-curated-remote/{name}/{version}" + )), + "untrusted_executor_metadata", + contents, + ) +} + +// Check both APIs: ordinary attribution can survive version differences, while +// measurements must use the exact authenticated declaration (never executor YAML). +async fn assert_executor_resolution( + roots: &TrustedPluginRoots, + command: &[String], + cwd: &PathUri, + plugin_id: &PluginId, + variant: Option<&str>, +) { + assert_eq!( + roots + .resolve_executor_attribution(command, cwd, LOCAL_FS.as_ref()) + .await, + Some(PluginCommandAttribution { + plugin_id: plugin_id.clone(), + normalized_relative_path: REFERENCE_HELPER.to_string(), + }), + "attribution for {command:?} at {cwd:?}" + ); + assert_eq!( + roots + .resolve_metrics_operation_in_filesystem(command, cwd, LOCAL_FS.as_ref()) + .await, + variant.map(|variant| ResolvedPluginMetricsOperation { + plugin_id: plugin_id.clone(), + operation: reference_metrics_operation(variant), + }), + "measurements for {command:?} at {cwd:?}" + ); +} + +#[test] +fn executor_cache_identity_obeys_windows_and_posix_case_conventions() { + let (_reference, roots) = + reference_fixture(&[("sites", "2.0.0-RC1", "selected", "printf shared\\n\n")]); + let plugin_id = PluginId::parse("sites@openai-curated-remote").expect("plugin id"); + for prefix in ["file:///C:/Users/user/.codex", "file://server/share/.codex"] { + let script = PathUri::parse(&format!( + "{prefix}/PLUGINS/CACHE/OPENAI-CURATED-REMOTE/SITES/2.0.0-rc1/{REFERENCE_HELPER}" + )) + .expect("Windows script"); + let target = PluginMeasurementTarget::from_script_path(&script) + .expect("Windows cache preselection follows the resolver's path identity"); + assert_eq!( + target, + PluginMeasurementTarget { + plugin_id: plugin_id.clone(), + version: "2.0.0-rc1".to_string(), + path_convention: PathConvention::Windows, + } + ); + let equivalent = PathUri::parse(&format!( + "{prefix}/plugins/cache/openai-curated-remote/sites/2.0.0-RC1/{REFERENCE_HELPER}" + )) + .expect("equivalent Windows script"); + assert_eq!( + PluginMeasurementTarget::from_script_path(&equivalent), + Some(target) + ); + let candidates = + roots.local_candidates_for_executor_script(&script, PluginVersionMatch::Exact); + let candidate = candidates + .first() + .expect("Windows identity casing must not prevent canonicalization"); + assert_eq!( + candidate.matched.metrics_operation(), + Some(ResolvedPluginMetricsOperation { + plugin_id: plugin_id.clone(), + operation: reference_metrics_operation("selected"), + }) + ); + } + for script in [ + format!( + "file:///home/user/.codex/PLUGINS/CACHE/OPENAI-CURATED-REMOTE/SITES/2.0.0-rc1/{REFERENCE_HELPER}" + ), + format!( + "file:///home/user/.codex/plugins/cache/openai-curated-remote/SITES/2.0.0-RC1/{REFERENCE_HELPER}" + ), + format!( + "file:///C:/Users/user/.codex/PLUGINS/CACHE/OPENAI-CURATED-REMOTE/SITES/2.0.0-rc2/{REFERENCE_HELPER}" + ), + format!( + "file:///C:/Users/user/.codex/PLUGINS/CACHE/OPENAI-CURATED-REMOTE/OTHER/2.0.0-rc1/{REFERENCE_HELPER}" + ), + ] { + let script = PathUri::parse(&script).expect("script"); + let path_convention = script.infer_path_convention().expect("path convention"); + let expected = PluginMeasurementTarget { + plugin_id: plugin_id.clone(), + version: match path_convention { + PathConvention::Windows => "2.0.0-rc1", + PathConvention::Posix => "2.0.0-RC1", + } + .to_string(), + path_convention, + }; + assert_ne!( + PluginMeasurementTarget::from_script_path(&script), + Some(expected), + "preselection must not match a differently cased POSIX identity or another version" + ); + assert!( + roots + .local_candidates_for_executor_script(&script, PluginVersionMatch::Exact) + .is_empty(), + "POSIX casing and different Windows versions must remain distinct" + ); + } + for version in ["2.0.0-RC1", "2.0.0-rc1"] { + let script = PathUri::parse(&format!( + "file:///home/user/.codex/plugins/cache/openai-curated-remote/sites/{version}/{REFERENCE_HELPER}" + )) + .expect("POSIX script"); + let target = + PluginMeasurementTarget::from_script_path(&script).expect("POSIX cache preselection"); + assert_eq!( + target, + PluginMeasurementTarget { + plugin_id: plugin_id.clone(), + version: version.to_string(), + path_convention: PathConvention::Posix, + } + ); + } +} + +#[test] +fn extending_trusted_roots_preserves_existing_plugins_and_deduplicates_snapshots() { + let contents = "printf shared\\n\n"; + let (_frontend, mut roots) = reference_fixture(&[("sites", "1.2.3", "primary", contents)]); + let original = roots.clone(); + let (_snapshot, additional) = reference_fixture(&[ + ("sites", "1.2.3", "copied", contents), + ("other", "1.2.3", "additional", contents), + ]); + + roots.extend(&additional); + let expected = TrustedPluginRoots { + roots: vec![original.roots[0].clone(), additional.roots[1].clone()], + }; + assert_eq!(roots, expected); + roots.extend(&additional); + assert_eq!(roots, expected, "repeated snapshots must remain idempotent"); +} + +#[tokio::test] +async fn authenticated_v2_reference_coexists_with_loaded_frontend_v1() { + let frontend = TempDir::new().expect("frontend"); + let v1 = installed_remote_plugin_root(frontend.path(), "sites"); + reference_identity_fixture(v1.as_path(), "install_v1", "printf v1\\n\n"); + let plugin_id = PluginId::parse("sites@openai-curated-remote").expect("plugin id"); + let mut roots = roots_for( + frontend.path(), + vec![loaded_plugin(&plugin_id.as_key(), v1.as_path(), ENABLED)], + ); + let (_reference, references) = + reference_fixture(&[("sites", "2.0.0", "install_v2", "printf v2\\n\n")]); + roots.extend(&references); + // The operation must stay bound to the selected root when two versions share + // a plugin ID and helper path, for both local and executor resolution. + for (root, variant) in [ + (&v1, "install_v1"), + (&references.roots[0].root, "install_v2"), + ] { + assert_eq!( + roots.resolve_metrics_operation(&command(&["sh", REFERENCE_HELPER]), root), + Some(ResolvedPluginMetricsOperation { + plugin_id: plugin_id.clone(), + operation: reference_metrics_operation(variant), + }) + ); + } + let executor = TempDir::new().expect("executor"); + let root = executor_plugin(executor.path(), "sites", "2.0.0", "printf v2\\n\n"); + let cwd = PathUri::from_host_native_path(root.as_path()).expect("executor URI"); + assert_executor_resolution( + &roots, + &command(&["sh", REFERENCE_HELPER]), + &cwd, + &plugin_id, + Some("install_v2"), + ) + .await; +} + +#[tokio::test] +async fn same_helper_path_in_two_plugins_resolves_by_executor_identity() { + // Identical bytes: neither suffix nor content identifies a plugin. + let contents = "printf shared\\n\n"; + let (_reference, roots) = reference_fixture(&[ + ("sites", "2.0.0", "sites_install", contents), + ("other", "2.0.0", "other_install", contents), + ]); + let executor = TempDir::new().expect("executor"); + for (name, variant) in [("sites", "sites_install"), ("other", "other_install")] { + let root = executor_plugin(executor.path(), name, "2.0.0", contents); + let cwd = PathUri::from_host_native_path(root.as_path()).expect("executor URI"); + let plugin_id = + PluginId::parse(&format!("{name}@openai-curated-remote")).expect("plugin id"); + assert_executor_resolution( + &roots, + &command(&["sh", REFERENCE_HELPER]), + &cwd, + &plugin_id, + Some(variant), + ) + .await; + } +} + +#[tokio::test] +async fn identical_script_bytes_do_not_authorize_another_executor_version() { + let contents = "printf shared\\n\n"; + let (_reference, roots) = + reference_fixture(&[("sites", "2.0.0", "v2_only_operation", contents)]); + let plugin_id = PluginId::parse("sites@openai-curated-remote").expect("plugin id"); + let executor = TempDir::new().expect("executor"); + for version in ["1.2.3", "2.0.0"] { + let root = executor_plugin(executor.path(), "sites", version, contents); + let cwd = PathUri::from_host_native_path(root.as_path()).expect("executor URI"); + let candidate = command(&["bash", "-lc", &format!("sh {REFERENCE_HELPER}")]); + assert_eq!( + PluginMeasurementTarget::from_command(&candidate, &cwd, LOCAL_FS.as_ref()).await, + Some(PluginMeasurementTarget { + plugin_id: plugin_id.clone(), + version: version.to_string(), + path_convention: PathConvention::native(), + }) + ); + assert_executor_resolution( + &roots, + &command(&["sh", REFERENCE_HELPER]), + &cwd, + &plugin_id, + (version == "2.0.0").then_some("v2_only_operation"), + ) + .await; + } +} + +#[cfg(unix)] +#[tokio::test] +async fn executor_symlink_cannot_change_authenticated_plugin_or_version() { + let contents = "printf shared\\n\n"; + let (_reference, roots) = + reference_fixture(&[("sites", "2.0.0", "v2_only_operation", contents)]); + let executor = TempDir::new().expect("executor"); + let original = executor + .path() + .join("plugins/cache/openai-curated-remote/sites/2.0.0"); + fs::create_dir_all(original.join("scripts")).expect("original scripts directory"); + let cwd = PathUri::from_host_native_path(&original).expect("original executor URI"); + for target_identity in [ + "openai-curated-remote/other/2.0.0", + "openai-curated-remote/sites/1.2.3", + "openai-curated/sites/2.0.0", + "../outside-cache", + ] { + let target = reference_identity_fixture( + &executor.path().join("plugins/cache").join(target_identity), + "ignored_executor_metadata", + contents, + ); + std::os::unix::fs::symlink( + target.join(REFERENCE_HELPER), + original.join(REFERENCE_HELPER), + ) + .expect("redirect helper to another identity"); + assert_eq!( + roots + .resolve_metrics_operation_in_filesystem( + &command(&["sh", REFERENCE_HELPER]), + &cwd, + LOCAL_FS.as_ref(), + ) + .await, + None, + "canonical identity changed to {target_identity}" + ); + fs::remove_file(original.join(REFERENCE_HELPER)).expect("remove helper symlink"); + } +} + +#[cfg(unix)] +#[tokio::test] +async fn executor_aliases_use_canonical_plugin_paths() { + let contents = "printf shared\\n\n"; + let (_reference, roots) = reference_fixture(&[ + ("sites", "1.2.3", "older", contents), + ("sites", "2.0.0", "selected", contents), + ]); + let older_roots = TrustedPluginRoots { + roots: vec![roots.roots[0].clone()], + }; + let plugin_id = PluginId::parse("sites@openai-curated-remote").expect("plugin id"); + let executor = TempDir::new().expect("executor"); + let target = executor_plugin(executor.path(), "sites", "2.0.0", contents); + let alias = executor.path().join("plugin-link"); + std::os::unix::fs::symlink(target.as_path(), &alias).expect("directory alias"); + let version_alias = executor + .path() + .join("plugins/cache/openai-curated-remote/sites/1.2.3/scripts/renamed-entry.sh"); + fs::create_dir_all(version_alias.parent().expect("alias parent")).expect("V1 alias directory"); + std::os::unix::fs::symlink(target.join(REFERENCE_HELPER), &version_alias).expect("V1 alias"); + let cwd = PathUri::from_host_native_path(executor.path()).expect("executor URI"); + // An already loaded reference still supports directory aliases. Preparation + // only probes cache paths, so ordinary workspace scripts need no executor I/O. + assert_executor_resolution( + &roots, + &command(&["sh", &alias.join(REFERENCE_HELPER).to_string_lossy()]), + &cwd, + &plugin_id, + Some("selected"), + ) + .await; + let command = command(&["sh", &version_alias.to_string_lossy()]); + assert_eq!( + PluginMeasurementTarget::from_command(&command, &cwd, LOCAL_FS.as_ref()).await, + Some(PluginMeasurementTarget { + plugin_id: plugin_id.clone(), + version: "2.0.0".to_string(), + path_convention: PathConvention::Posix, + }) + ); + assert_executor_resolution(&roots, &command, &cwd, &plugin_id, Some("selected")).await; + assert_eq!( + older_roots + .resolve_metrics_operation_in_filesystem(&command, &cwd, LOCAL_FS.as_ref()) + .await, + None, + "a V1-shaped alias and identical bytes cannot authorize V2 metrics" + ); +} + +#[tokio::test] +async fn measurement_target_rejects_inline_commands_and_local_versions() -> anyhow::Result<()> { + let executor = TempDir::new()?; + let root = reference_identity_fixture( + &executor + .path() + .join("plugins/cache/openai-curated-remote/sites/local"), + "ignored", + "printf fixture\n", + ); + let cwd = PathUri::from_host_native_path(root.as_path())?; + for args in [&["node", "-e", "42"][..], &["sh", REFERENCE_HELPER][..]] { + let command = command(args); + let fs = codex_exec_server::LOCAL_FS.as_ref(); + assert_eq!( + PluginMeasurementTarget::from_command(&command, &cwd, fs).await, + None + ); + } + Ok(()) +} + +struct UnusedConnectProvider; + +impl NoiseRendezvousConnectProvider for UnusedConnectProvider { + fn connect_bundle( + &self, + _: NoiseChannelPublicKey, + ) -> BoxFuture<'_, Result> { + panic!("unrelated script must not connect to the executor") + } +} + +#[tokio::test] +async fn unrelated_scripts_skip_executor_lookup() -> anyhow::Result<()> { + let (_reference, roots) = + reference_fixture(&[("sites", "2.0.0", "selected", "printf shared\\n\n")]); + let manager = environment_manager_without_environments(); + let pending = manager.materialize_pending_noise_environment( + "tools".to_string(), + Arc::new(UnusedConnectProvider), + )?; + let fs = pending.get_filesystem(); + let cwd = PathUri::parse("file:///workspace")?; + let command = command(&["node", "build.js"]); + // An executor lookup waits for provisioning; unrelated scripts must finish immediately. + assert_eq!( + futures::future::join3( + roots.resolve_executor_attribution(&command, &cwd, fs.as_ref()), + roots.resolve_metrics_operation_in_filesystem(&command, &cwd, fs.as_ref()), + PluginMeasurementTarget::from_command(&command, &cwd, fs.as_ref()), + ) + .now_or_never(), + Some((None, None, None)), + ); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index b868d1620f..a6498c7a66 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -498,6 +498,167 @@ async fn persisted_remote_plugin_command_attribution_flows_through_turn_context( Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_plugin_measurements_require_the_frontend_version() -> Result<()> { + skip_if_no_network!(Ok(())); + // This fixture needs a real remote transport with separately controlled frontend + // and executor caches, rather than the runner's automatic executor selection. + let executor = super::multi_exec_server_sandbox::ExecServerProcess::start().await?; + let server = start_mock_server().await; + let home = Arc::new(TempDir::new()?); + let frontend_script = write_remote_plugin_script_and_config(home.as_ref()); + let script = r#"if [ -n "${CODEX_PLUGIN_METRICS_OUTPUT:-}" ]; then + printf '%s' '{"version":1,"measurements":[{"name":"duration_ms","value":7,"dimensions":{"release":"v1"}}]}' > "$CODEX_PLUGIN_METRICS_OUTPUT" +else + printf 'no metrics sidecar\n' +fi +"#; + std::fs::write(&frontend_script, script)?; + std::fs::write( + frontend_script + .parent() + .unwrap() + .parent() + .unwrap() + .join("analytics.yaml"), + "version: 1\noperations:\n dependency_install:\n path: ./scripts/run.sh\n measurements:\n duration_ms:\n dimensions:\n release: [v1]\n", + )?; + let executor_home = TempDir::new()?; + let mut responses = Vec::new(); + for (version, call_id) in [("2.0.0", "wrong-version"), ("1.2.3", "matching-version")] { + let root = executor_home.path().join(format!( + "plugins/cache/openai-curated-remote/sample/{version}" + )); + std::fs::create_dir_all(root.join("scripts"))?; + let path = root.join("scripts/run.sh"); + std::fs::write(&path, script)?; + // The executor's declaration is never an authority, even for matching bytes. + std::fs::write( + root.join("analytics.yaml"), + "version: 1\noperations: {untrusted: {path: ./scripts/run.sh, measurements: {other: {}}}}\n", + )?; + // A plugin-shaped alias must use the canonical plugin's identity and version. + let alias = executor_home.path().join(format!( + "plugins/cache/openai-curated-remote/apparent-plugin/{version}" + )); + std::fs::create_dir_all(alias.parent().unwrap())?; + std::os::unix::fs::symlink(&root, &alias)?; + let alias_script = alias.join("scripts/run.sh"); + let command = shlex::try_join(["/bin/sh", alias_script.to_string_lossy().as_ref()])?; + responses.push(sse(vec![ + ev_response_created(call_id), + ev_function_call( + call_id, + "exec_command", + &serde_json::json!({ + "cmd": command, + "login": false, + "environment_id": codex_exec_server::REMOTE_ENVIRONMENT_ID, + "yield_time_ms": 1000, + }) + .to_string(), + ), + ev_completed(call_id), + ])); + } + responses.push(sse(vec![ + ev_response_created("done"), + ev_assistant_message("done-message", "done"), + ev_completed("done"), + ])); + let response_mock = mount_sse_sequence(&server, responses).await; + let base_url = server.uri(); + let mut builder = test_codex() + .with_exec_server_url(executor.websocket_url.clone()) + .with_home(home) + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_model("gpt-5.2") + .with_config(move |config| { + config.chatgpt_base_url = base_url; + config.analytics_enabled = Some(true); + config + .features + .enable(Feature::SkipHostSkillDiscovery) + .unwrap(); + }); + let test = builder.build(&server).await?; + let manager = test.thread_manager.environment_manager(); + let remote = manager.default_environment().unwrap(); + tokio::time::timeout(Duration::from_secs(30), remote.wait_until_ready()).await??; + assert!(remote.is_remote()); + let (sandbox_policy, permission_profile) = + turn_permission_fields(PermissionProfile::Disabled, test.config.cwd.as_path()); + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "run both plugin versions".into(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::Never), + sandbox_policy: Some(sandbox_policy), + permission_profile, + ..Default::default() + }), + ) + .await?; + for call_id in ["wrong-version", "matching-version"] { + let expected = (Some(REMOTE_PLUGIN_CONFIG_NAME), Some("scripts/run.sh")); + let begin = wait_for_event_match(&test.codex, |event| match event { + EventMsg::ExecCommandBegin(event) if event.call_id == call_id => Some(event.clone()), + _ => None, + }) + .await; + let end = wait_for_event_match(&test.codex, |event| match event { + EventMsg::ExecCommandEnd(event) if event.call_id == call_id => Some(event.clone()), + _ => None, + }) + .await; + assert_eq!(end.exit_code, 0, "{}", end.aggregated_output); + assert_eq!( + (begin.plugin_id.as_deref(), begin.script_path.as_deref()), + expected + ); + assert_eq!( + (end.plugin_id.as_deref(), end.script_path.as_deref()), + expected + ); + } + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + assert!( + response_mock + .function_call_output_text("wrong-version") + .unwrap() + .contains("no metrics sidecar") + ); + let event = wait_for_analytics_event(&server, "codex_plugin_measurement_event").await; + assert_eq!( + serde_json::json!({ + "plugin_id": event["event_params"]["plugin_id"], + "operation": event["event_params"]["operation"], + "measurement_name": event["event_params"]["measurement_name"], + "number_value": event["event_params"]["number_value"], + "dimensions": event["event_params"]["dimensions"], + "thread_id": event["event_params"]["thread_id"], + "item_id": event["event_params"]["item_id"], + }), + serde_json::json!({ + "plugin_id": REMOTE_PLUGIN_CONFIG_NAME, + "operation": "dependency_install", + "measurement_name": "duration_ms", + "number_value": 7.0, + "dimensions": {"release": "v1"}, + "thread_id": test.session_configured.thread_id.to_string(), + "item_id": "matching-version", + }) + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn thread_disabled_plugins_filter_skills_and_tools_without_changing_shared_plugins() -> Result<()> { diff --git a/codex-rs/login/BUILD.bazel b/codex-rs/login/BUILD.bazel index 42c5385c53..561176b3ed 100644 --- a/codex-rs/login/BUILD.bazel +++ b/codex-rs/login/BUILD.bazel @@ -8,4 +8,5 @@ codex_rust_crate( "src/assets/success_legacy.html", ], crate_name = "codex_login", + unit_test_timeout = "long", )