mirror of
https://github.com/openai/codex.git
synced 2026-09-03 14:59:03 +00:00
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
This commit is contained in:
@@ -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<u64> {
|
||||
value.as_u64().or_else(|| value.as_str()?.parse().ok())
|
||||
}
|
||||
|
||||
fn is_missing_or_empty_text_file(path: &Path) -> io::Result<bool> {
|
||||
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}")
|
||||
}
|
||||
|
||||
@@ -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::<ExternalAgentConfigMigrationItem>::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::<ExternalAgentConfigMigrationItem>::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");
|
||||
|
||||
@@ -25,10 +25,12 @@ pub(crate) fn read_json_file(path: &Path) -> io::Result<Option<JsonValue>> {
|
||||
}
|
||||
|
||||
pub(super) fn is_missing_or_empty_text_file(path: &Path) -> io::Result<bool> {
|
||||
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);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user