From b1ccaa0e080f59cfd71a136f6fcc60a4f2d60fba Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 30 Jul 2026 00:45:02 +0000 Subject: [PATCH] Avoid overwriting symlinked migration targets (#36051) ## Why External-agent migration treated symlinked empty text targets as overwritable files. Writing migrated configuration through such a target could modify a file outside the repository. ## What changed - Use symlink metadata when checking whether a migration target is missing or empty, so only regular files are considered overwritable. - Preserve symlinked `AGENTS.md` and `.codex/hooks.json` targets during both detection and import. ## Testing Added Unix regression coverage for existing and dangling symlink targets for both guidance and hooks migration. See https://github.com/openai/codex/pull/26021. GitOrigin-RevId: 13c78e7458d1c02abdb22b38ba88ed66bb5a154b --- .../src/hooks_common.rs | 12 +- .../src/service_tests/general/repo_import.rs | 152 ++++++++++++++++++ .../external-agent-migration/src/utils.rs | 10 +- 3 files changed, 159 insertions(+), 15 deletions(-) diff --git a/codex-rs/external-agent-migration/src/hooks_common.rs b/codex-rs/external-agent-migration/src/hooks_common.rs index 4a692f5541..5fdf288c90 100644 --- a/codex-rs/external-agent-migration/src/hooks_common.rs +++ b/codex-rs/external-agent-migration/src/hooks_common.rs @@ -1,4 +1,5 @@ use crate::invalid_data_error; +use crate::utils::is_missing_or_empty_text_file; use serde_json::Value as JsonValue; use std::fs; use std::io; @@ -271,17 +272,6 @@ pub(crate) fn json_u64(value: &JsonValue) -> Option { value.as_u64().or_else(|| value.as_str()?.parse().ok()) } -fn is_missing_or_empty_text_file(path: &Path) -> io::Result { - if !path.exists() { - return Ok(true); - } - if !path.is_file() { - return Ok(false); - } - - Ok(fs::read_to_string(path)?.trim().is_empty()) -} - pub(crate) fn external_agent_config_dir() -> String { format!(".{SOURCE_EXTERNAL_AGENT_NAME}") } diff --git a/codex-rs/external-agent-migration/src/service_tests/general/repo_import.rs b/codex-rs/external-agent-migration/src/service_tests/general/repo_import.rs index 49214a67ef..8321b52e39 100644 --- a/codex-rs/external-agent-migration/src/service_tests/general/repo_import.rs +++ b/codex-rs/external-agent-migration/src/service_tests/general/repo_import.rs @@ -140,6 +140,81 @@ async fn import_repo_agents_md_overwrites_empty_targets() { ); } +#[cfg(unix)] +#[tokio::test] +async fn repo_agents_md_migration_skips_symlink_targets() { + let root = TempDir::new().expect("create tempdir"); + let service = service_for_paths( + root.path().join(EXTERNAL_AGENT_DIR), + root.path().join(".codex"), + ); + + for (repo_name, initial_target_contents) in [ + ("repo-with-existing-target", Some("")), + ("repo-with-dangling-target", None), + ] { + let repo_root = root.path().join(repo_name); + let linked_target = root.path().join(format!("{repo_name}-target")); + let agents_md = repo_root.join("AGENTS.md"); + fs::create_dir_all(repo_root.join(".git")).expect("create git"); + fs::write( + repo_root.join(EXTERNAL_AGENT_CONFIG_MD), + format!("{SOURCE_EXTERNAL_AGENT_DISPLAY_NAME} code guidance"), + ) + .expect("write source"); + if let Some(contents) = initial_target_contents { + fs::write(&linked_target, contents).expect("write linked target"); + } + std::os::unix::fs::symlink(&linked_target, &agents_md).expect("create symlink"); + + let items = service + .detect(ExternalAgentConfigDetectOptions { + include_home: false, + include_memory: false, + cwds: Some(vec![repo_root.clone()]), + }) + .await + .expect("detect"); + assert_eq!(items, Vec::::new()); + + let outcome = service + .import(vec![ExternalAgentConfigMigrationItem { + item_type: ExternalAgentConfigMigrationItemType::AgentsMd, + description: String::new(), + cwd: Some(repo_root.clone()), + details: None, + }]) + .await; + assert_eq!( + outcome.item_results, + vec![ExternalAgentConfigImportItemResult { + item_type: ExternalAgentConfigMigrationItemType::AgentsMd, + description: String::new(), + cwd: Some(repo_root), + success_count: 0, + error_count: 0, + successes: Vec::new(), + raw_errors: Vec::new(), + }] + ); + + assert_eq!( + fs::read_link(&agents_md).expect("read migration target symlink"), + linked_target + ); + match initial_target_contents { + Some(contents) => assert_eq!( + fs::read_to_string(&linked_target).expect("read linked target"), + contents + ), + None => assert!( + !linked_target.exists(), + "dangling migration symlink target must not be created" + ), + } + } +} + #[tokio::test] async fn detect_repo_prefers_non_empty_external_agent_agents_source() { let root = TempDir::new().expect("create tempdir"); @@ -255,6 +330,83 @@ async fn import_repo_hooks_preserves_disabled_codex_hooks_feature() { ); } +#[cfg(unix)] +#[tokio::test] +async fn repo_hooks_migration_skips_symlink_targets() { + let root = TempDir::new().expect("create tempdir"); + let service = service_for_paths( + root.path().join(EXTERNAL_AGENT_DIR), + root.path().join(".codex"), + ); + + for (repo_name, initial_target_contents) in [ + ("repo-with-existing-hook-target", Some("")), + ("repo-with-dangling-hook-target", None), + ] { + let repo_root = root.path().join(repo_name); + let linked_target = root.path().join(format!("{repo_name}-target")); + let hooks_json = repo_root.join(".codex").join("hooks.json"); + fs::create_dir_all(repo_root.join(".git")).expect("create git dir"); + fs::create_dir_all(repo_root.join(EXTERNAL_AGENT_DIR)).expect("create external agent dir"); + fs::create_dir_all(repo_root.join(".codex")).expect("create codex dir"); + fs::write( + repo_root.join(EXTERNAL_AGENT_DIR).join("settings.json"), + r#"{"hooks":{"Stop":[{"hooks":[{"command":"echo done"}]}]}}"#, + ) + .expect("write hooks"); + if let Some(contents) = initial_target_contents { + fs::write(&linked_target, contents).expect("write linked target"); + } + std::os::unix::fs::symlink(&linked_target, &hooks_json).expect("create symlink"); + + let items = service + .detect(ExternalAgentConfigDetectOptions { + include_home: false, + include_memory: false, + cwds: Some(vec![repo_root.clone()]), + }) + .await + .expect("detect"); + assert_eq!(items, Vec::::new()); + + let outcome = service + .import(vec![ExternalAgentConfigMigrationItem { + item_type: ExternalAgentConfigMigrationItemType::Hooks, + description: String::new(), + cwd: Some(repo_root.clone()), + details: None, + }]) + .await; + assert_eq!( + outcome.item_results, + vec![ExternalAgentConfigImportItemResult { + item_type: ExternalAgentConfigMigrationItemType::Hooks, + description: String::new(), + cwd: Some(repo_root), + success_count: 0, + error_count: 0, + successes: Vec::new(), + raw_errors: Vec::new(), + }] + ); + + assert_eq!( + fs::read_link(&hooks_json).expect("read migration target symlink"), + linked_target + ); + match initial_target_contents { + Some(contents) => assert_eq!( + fs::read_to_string(&linked_target).expect("read linked target"), + contents + ), + None => assert!( + !linked_target.exists(), + "dangling migration symlink target must not be created" + ), + } + } +} + #[tokio::test] async fn import_repo_mcp_uses_home_settings_toggles_when_repo_settings_missing() { let root = TempDir::new().expect("create tempdir"); diff --git a/codex-rs/external-agent-migration/src/utils.rs b/codex-rs/external-agent-migration/src/utils.rs index 6de98835a5..b1b43fd0df 100644 --- a/codex-rs/external-agent-migration/src/utils.rs +++ b/codex-rs/external-agent-migration/src/utils.rs @@ -25,10 +25,12 @@ pub(crate) fn read_json_file(path: &Path) -> io::Result> { } pub(super) fn is_missing_or_empty_text_file(path: &Path) -> io::Result { - if !path.exists() { - return Ok(true); - } - if !path.is_file() { + let metadata = match fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(true), + Err(err) => return Err(err), + }; + if !metadata.is_file() { return Ok(false); }