mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Reject symbolic links in memory workspaces (#39205)
## What changed - Reject a symbolic link used as the memory root. - Recursively remove symbolic links before preparing or seeding the memory workspace. - Remove and reject symbolic links created during consolidation, including when the worker fails, so they cannot affect files outside the workspace. ## Testing Add Unix startup tests covering pre-existing extension links and links created by both successful and failed consolidation workers. GitOrigin-RevId: f96e03812773f7beaa16455dd77370ee47d8a71f
This commit is contained in:
@@ -130,5 +130,18 @@ pub fn raw_memories_file(root: &Path) -> PathBuf {
|
||||
}
|
||||
|
||||
pub async fn ensure_layout(root: &Path) -> std::io::Result<()> {
|
||||
tokio::fs::create_dir_all(root).await?;
|
||||
if tokio::fs::symlink_metadata(root)
|
||||
.await?
|
||||
.file_type()
|
||||
.is_symlink()
|
||||
{
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidInput,
|
||||
format!("memory root cannot be a symbolic link: {}", root.display()),
|
||||
));
|
||||
}
|
||||
|
||||
workspace::remove_memory_symlinks(root).await?;
|
||||
tokio::fs::create_dir_all(rollout_summaries_dir(root)).await
|
||||
}
|
||||
|
||||
@@ -11,6 +11,7 @@ use crate::runtime::SpawnedConsolidationAgent;
|
||||
use crate::sync_rollout_summaries_from_memories;
|
||||
use crate::workspace::memory_workspace_diff;
|
||||
use crate::workspace::prepare_memory_workspace;
|
||||
use crate::workspace::remove_memory_symlinks;
|
||||
use crate::workspace::reset_memory_workspace_baseline;
|
||||
use crate::workspace::validate_consolidation_artifacts;
|
||||
use crate::workspace::write_workspace_diff;
|
||||
@@ -478,6 +479,9 @@ mod agent {
|
||||
}
|
||||
}
|
||||
} else if !agent_completed {
|
||||
if let Err(err) = remove_memory_symlinks(&memory_root).await {
|
||||
tracing::error!("failed removing memory workspace symbolic links: {err}");
|
||||
}
|
||||
job::failed(context.as_ref(), &db, &claim, "failed_agent").await;
|
||||
}
|
||||
});
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use crate::ensure_layout;
|
||||
use crate::extensions::seed_extension_instructions;
|
||||
use crate::guard;
|
||||
use crate::memory_root;
|
||||
@@ -52,8 +53,8 @@ pub fn start_memories_startup_task(
|
||||
|
||||
tokio::spawn(async move {
|
||||
let root = memory_root(&config.codex_home);
|
||||
if let Err(err) = tokio::fs::create_dir_all(&root).await {
|
||||
warn!("failed creating memories root: {err}");
|
||||
if let Err(err) = ensure_layout(&root).await {
|
||||
warn!("failed preparing memories root: {err}");
|
||||
return;
|
||||
}
|
||||
if let Err(err) = seed_extension_instructions(&root).await {
|
||||
|
||||
@@ -37,7 +37,11 @@ use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
use core_test_support::responses::mount_sse_once;
|
||||
#[cfg(unix)]
|
||||
use core_test_support::responses::mount_sse_once_match;
|
||||
use core_test_support::responses::sse;
|
||||
#[cfg(unix)]
|
||||
use core_test_support::responses::sse_failed;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::test_codex::TestCodex;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
@@ -65,6 +69,110 @@ async fn memories_startup_creates_memory_root() -> anyhow::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn memories_startup_removes_symlinked_extensions_before_seeding() -> anyhow::Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let home = Arc::new(TempDir::new()?);
|
||||
let memory_root = home.path().join("memories");
|
||||
let outside = home.path().join("outside");
|
||||
tokio::fs::create_dir_all(&memory_root).await?;
|
||||
tokio::fs::create_dir_all(&outside).await?;
|
||||
std::os::unix::fs::symlink(&outside, memory_root.join("extensions"))?;
|
||||
|
||||
let test = build_test_codex(&server, home).await?;
|
||||
trigger_memories_startup(&test).await;
|
||||
wait_for_dir(&memory_root.join("extensions/ad_hoc")).await?;
|
||||
|
||||
assert!(
|
||||
tokio::fs::symlink_metadata(memory_root.join("extensions"))
|
||||
.await?
|
||||
.is_dir()
|
||||
);
|
||||
assert!(
|
||||
!outside.join("ad_hoc").exists(),
|
||||
"extension seeding must not create directories through the removed symbolic link"
|
||||
);
|
||||
|
||||
shutdown_test_codex(&test).await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn memories_startup_fails_consolidation_when_worker_creates_extension_symlink()
|
||||
-> anyhow::Result<()> {
|
||||
let responses = [
|
||||
sse(vec![
|
||||
ev_response_created("resp-phase2-complete"),
|
||||
ev_assistant_message("msg-phase2-complete", "phase2 complete"),
|
||||
ev_completed("resp-phase2-complete"),
|
||||
]),
|
||||
sse_failed("resp-phase2-failed", "server_error", "worker failed"),
|
||||
];
|
||||
|
||||
for response in responses {
|
||||
let server = start_mock_server().await;
|
||||
let home = Arc::new(TempDir::new()?);
|
||||
let db = init_state_db(&home).await?;
|
||||
let root = home.path().join("memories");
|
||||
seed_stage1_output(
|
||||
db.as_ref(),
|
||||
home.path(),
|
||||
chrono::Utc::now(),
|
||||
"raw memory",
|
||||
"rollout summary",
|
||||
"worker-symlink",
|
||||
)
|
||||
.await?;
|
||||
seed_required_memory_artifacts(&root).await?;
|
||||
reset_git_repository(&root).await?;
|
||||
|
||||
let target = home.path().join("outside.md");
|
||||
tokio::fs::write(&target, "outside content").await?;
|
||||
let link = root.join("extensions/external_agent_import/instructions.md");
|
||||
let worker_target = target.clone();
|
||||
let worker_link = link.clone();
|
||||
let phase2 = mount_sse_once_match(
|
||||
&server,
|
||||
move |_request: &wiremock::Request| {
|
||||
std::fs::create_dir_all(worker_link.parent().expect("extension directory"))
|
||||
.expect("create extension directory");
|
||||
std::os::unix::fs::symlink(&worker_target, &worker_link)
|
||||
.expect("create worker symbolic link");
|
||||
true
|
||||
},
|
||||
response,
|
||||
)
|
||||
.await;
|
||||
let test = build_test_codex(&server, home).await?;
|
||||
|
||||
trigger_memories_startup(&test).await;
|
||||
wait_for_single_request(&phase2).await;
|
||||
|
||||
assert_eq!(
|
||||
wait_for_phase2_job_to_finish(db.as_ref()).await?,
|
||||
Phase2JobClaimOutcome::SkippedRetryUnavailable
|
||||
);
|
||||
assert_eq!(tokio::fs::read_to_string(&target).await?, "outside content");
|
||||
assert_eq!(
|
||||
tokio::fs::symlink_metadata(&link)
|
||||
.await
|
||||
.expect_err("worker-created symbolic link should be removed")
|
||||
.kind(),
|
||||
std::io::ErrorKind::NotFound
|
||||
);
|
||||
assert!(
|
||||
root.join("phase2_workspace_diff.md").exists(),
|
||||
"a rejected consolidation result must not reset the trusted baseline"
|
||||
);
|
||||
|
||||
shutdown_test_codex(&test).await?;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn memories_startup_phase2_tracks_workspace_diff_across_runs() -> anyhow::Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
|
||||
@@ -3,6 +3,8 @@ use codex_git_utils::GitBaselineDiff;
|
||||
use codex_git_utils::diff_since_latest_init;
|
||||
use codex_git_utils::ensure_git_baseline_repository;
|
||||
use codex_git_utils::reset_git_repository;
|
||||
#[cfg(windows)]
|
||||
use std::os::windows::fs::FileTypeExt;
|
||||
use std::path::Path;
|
||||
|
||||
/// Prepares the memory directory for git-baseline diffing.
|
||||
@@ -11,9 +13,9 @@ use std::path::Path;
|
||||
/// metadata is missing or unusable, and removes any stale generated `phase2_workspace_diff.md` file
|
||||
/// so that the next diff does not include a previous prompt artifact.
|
||||
pub async fn prepare_memory_workspace(root: &Path) -> anyhow::Result<()> {
|
||||
tokio::fs::create_dir_all(root)
|
||||
crate::ensure_layout(root)
|
||||
.await
|
||||
.with_context(|| format!("create memory workspace {}", root.display()))?;
|
||||
.with_context(|| format!("prepare memory workspace {}", root.display()))?;
|
||||
remove_workspace_diff(root).await?;
|
||||
ensure_git_baseline_repository(root).await?;
|
||||
Ok(())
|
||||
@@ -47,6 +49,12 @@ pub async fn reset_memory_workspace_baseline(root: &Path) -> anyhow::Result<()>
|
||||
|
||||
/// Verifies that a completed consolidation run left the required memory artifacts in place.
|
||||
pub async fn validate_consolidation_artifacts(root: &Path) -> anyhow::Result<()> {
|
||||
let removed_symlinks = remove_memory_symlinks(root).await?;
|
||||
anyhow::ensure!(
|
||||
removed_symlinks == 0,
|
||||
"removed {removed_symlinks} symbolic links from consolidated memory workspace"
|
||||
);
|
||||
|
||||
let memory_path = root.join("MEMORY.md");
|
||||
let memory_metadata = tokio::fs::metadata(&memory_path).await.with_context(|| {
|
||||
format!(
|
||||
@@ -73,6 +81,39 @@ pub async fn validate_consolidation_artifacts(root: &Path) -> anyhow::Result<()>
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) async fn remove_memory_symlinks(root: &Path) -> std::io::Result<usize> {
|
||||
let mut directories = vec![root.to_path_buf()];
|
||||
let mut removed = 0;
|
||||
|
||||
while let Some(directory) = directories.pop() {
|
||||
let mut entries = tokio::fs::read_dir(directory).await?;
|
||||
while let Some(entry) = entries.next_entry().await? {
|
||||
let path = entry.path();
|
||||
let file_type = entry.file_type().await?;
|
||||
if file_type.is_symlink() {
|
||||
#[cfg(windows)]
|
||||
if file_type.is_symlink_dir() {
|
||||
tokio::fs::remove_dir(&path).await?;
|
||||
} else {
|
||||
tokio::fs::remove_file(&path).await?;
|
||||
}
|
||||
#[cfg(not(windows))]
|
||||
tokio::fs::remove_file(&path).await?;
|
||||
|
||||
tracing::warn!(
|
||||
"removed symbolic link from memory workspace: {}",
|
||||
path.display()
|
||||
);
|
||||
removed += 1;
|
||||
} else if file_type.is_dir() {
|
||||
directories.push(path);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Ok(removed)
|
||||
}
|
||||
|
||||
/// Removes the generated `phase2_workspace_diff.md` prompt artifact.
|
||||
///
|
||||
/// This does not remove `.git/`, reset the baseline, or delete memory content. It is used before
|
||||
|
||||
Reference in New Issue
Block a user