From c6490c6f455a9db1e5f8b21291b64c9cdced06a7 Mon Sep 17 00:00:00 2001 From: mjamiv Date: Mon, 25 May 2026 21:50:25 +0000 Subject: [PATCH] fix(cli): preserve symlinks during sandbox upload --- crates/openshell-cli/src/ssh.rs | 238 ++++++++++++++++++++++++++------ 1 file changed, 199 insertions(+), 39 deletions(-) diff --git a/crates/openshell-cli/src/ssh.rs b/crates/openshell-cli/src/ssh.rs index 204128d34..823021050 100644 --- a/crates/openshell-cli/src/ssh.rs +++ b/crates/openshell-cli/src/ssh.rs @@ -484,20 +484,7 @@ fn write_upload_archive(writer: W, source: UploadSource) -> Result<()> local_path, tar_name, } => { - if local_path.is_file() { - archive - .append_path_with_name(&local_path, &tar_name) - .into_diagnostic()?; - } else if local_path.is_dir() { - archive - .append_dir_all(&tar_name, &local_path) - .into_diagnostic()?; - } else { - return Err(miette::miette!( - "local path does not exist: {}", - local_path.display() - )); - } + append_upload_path(&mut archive, &local_path, Path::new(&tar_name), false)?; } UploadSource::FileList { base_dir, @@ -509,24 +496,7 @@ fn write_upload_archive(writer: W, source: UploadSource) -> Result<()> let archive_path = archive_prefix .as_ref() .map_or_else(|| PathBuf::from(file), |prefix| prefix.join(file)); - if full_path.is_file() { - archive - .append_path_with_name(&full_path, &archive_path) - .into_diagnostic() - .wrap_err_with(|| { - format!("failed to add {} to tar archive", archive_path.display()) - })?; - } else if full_path.is_dir() { - archive - .append_dir_all(&archive_path, &full_path) - .into_diagnostic() - .wrap_err_with(|| { - format!( - "failed to add directory {} to tar archive", - archive_path.display() - ) - })?; - } + append_upload_path(&mut archive, &full_path, &archive_path, true)?; } } } @@ -534,6 +504,119 @@ fn write_upload_archive(writer: W, source: UploadSource) -> Result<()> Ok(()) } +fn append_upload_path( + archive: &mut tar::Builder, + local_path: &Path, + archive_path: &Path, + skip_missing: bool, +) -> Result<()> { + let metadata = match fs::symlink_metadata(local_path) { + Ok(metadata) => metadata, + Err(err) if skip_missing && err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => { + return Err(err) + .into_diagnostic() + .wrap_err_with(|| format!("failed to stat {}", local_path.display())); + } + }; + let file_type = metadata.file_type(); + + if file_type.is_file() { + archive + .append_path_with_name(local_path, archive_path) + .into_diagnostic() + .wrap_err_with(|| format!("failed to add {} to tar archive", archive_path.display()))?; + return Ok(()); + } + + if file_type.is_dir() { + let dir_archive_path = upload_archive_dir_entry_path(archive_path); + archive + .append_dir(&dir_archive_path, local_path) + .into_diagnostic() + .wrap_err_with(|| { + format!( + "failed to add directory {} to tar archive", + archive_path.display() + ) + })?; + append_upload_dir_contents(archive, local_path, archive_path)?; + return Ok(()); + } + + if file_type.is_symlink() { + append_upload_symlink(archive, local_path, archive_path, &metadata)?; + return Ok(()); + } + + Err(miette::miette!( + "unsupported file type for upload: {}", + local_path.display() + )) +} + +fn upload_archive_dir_entry_path(archive_path: &Path) -> PathBuf { + let mut path = archive_path.as_os_str().to_os_string(); + path.push("/"); + PathBuf::from(path) +} + +fn append_upload_dir_contents( + archive: &mut tar::Builder, + local_path: &Path, + archive_path: &Path, +) -> Result<()> { + let mut entries = fs::read_dir(local_path) + .into_diagnostic() + .wrap_err_with(|| format!("failed to read directory {}", local_path.display()))? + .collect::>>() + .into_diagnostic() + .wrap_err_with(|| format!("failed to read directory {}", local_path.display()))?; + entries.sort_by_key(fs::DirEntry::file_name); + + for entry in entries { + let entry_name = entry.file_name(); + let child_local_path = entry.path(); + let child_archive_path = archive_path.join(entry_name); + append_upload_path(archive, &child_local_path, &child_archive_path, false)?; + } + + Ok(()) +} + +fn append_upload_symlink( + archive: &mut tar::Builder, + local_path: &Path, + archive_path: &Path, + metadata: &fs::Metadata, +) -> Result<()> { + let target = fs::read_link(local_path) + .into_diagnostic() + .wrap_err_with(|| format!("failed to read symlink {}", local_path.display()))?; + let mut header = tar::Header::new_gnu(); + header.set_metadata(metadata); + header.set_entry_type(tar::EntryType::Symlink); + header.set_size(0); + header.set_cksum(); + archive + .append_link(&mut header, archive_path, target) + .into_diagnostic() + .wrap_err_with(|| { + format!( + "failed to add symlink {} to tar archive", + archive_path.display() + ) + })?; + Ok(()) +} + +fn local_upload_path_is_file_like(path: &Path) -> bool { + fs::symlink_metadata(path).is_ok_and(|metadata| { + let file_type = metadata.file_type(); + file_type.is_file() || file_type.is_symlink() + }) +} + /// Core tar-over-SSH upload: streams a tar archive into `dest_dir` on the /// sandbox. Callers are responsible for splitting the destination path so /// that `dest_dir` is always a directory. @@ -782,8 +865,9 @@ pub async fn sandbox_sync_up( // passed "/sandbox"), fall through to directory semantics instead. The // sandbox user cannot write to "/" and the intent is almost certainly // "put the file inside /sandbox", not "create a file named sandbox in /". + let local_path_is_file_like = local_upload_path_is_file_like(local_path); if let Some(path) = sandbox_path - && local_path.is_file() + && local_path_is_file_like && !path.ends_with('/') { let (parent, target_name) = split_sandbox_path(path); @@ -802,7 +886,7 @@ pub async fn sandbox_sync_up( } } - let tar_name = if local_path.is_file() { + let tar_name = if local_path_is_file_like { local_path .file_name() .ok_or_else(|| miette::miette!("path has no file name"))? @@ -1831,21 +1915,47 @@ mod tests { assert_eq!(file_list_archive_prefix(&file), None); } - fn upload_archive_paths(source: UploadSource) -> Vec { + #[derive(Debug)] + struct UploadArchiveEntry { + path: String, + entry_type: tar::EntryType, + link_name: Option, + } + + fn upload_archive_entries(source: UploadSource) -> Vec { let mut bytes = Vec::new(); write_upload_archive(&mut bytes, source).expect("write upload archive"); let mut archive = tar::Archive::new(std::io::Cursor::new(bytes)); let entries = archive.entries().expect("read archive entries"); - let mut paths = entries + let mut entries = entries .map(|entry| { - entry - .expect("read archive entry") + let entry = entry.expect("read archive entry"); + let path = entry .path() .expect("read archive path") .to_string_lossy() - .into_owned() + .into_owned(); + let entry_type = entry.header().entry_type(); + let link_name = entry + .link_name() + .expect("read archive link") + .map(|link| link.to_string_lossy().into_owned()); + UploadArchiveEntry { + path, + entry_type, + link_name, + } }) .collect::>(); + entries.sort_by(|left, right| left.path.cmp(&right.path)); + entries + } + + fn upload_archive_paths(source: UploadSource) -> Vec { + let mut paths = upload_archive_entries(source) + .into_iter() + .map(|entry| entry.path) + .collect::>(); paths.sort(); paths } @@ -1902,6 +2012,56 @@ mod tests { assert!(paths.iter().all(|path| path.starts_with("source-dir/"))); } + #[cfg(unix)] + #[test] + fn single_directory_archive_preserves_symlink_entries() { + let tmpdir = tempfile::tempdir().expect("create tmpdir"); + let source = tmpdir.path().join("source-dir"); + fs::create_dir_all(source.join("real-dir")).expect("create dirs"); + fs::write(source.join("real-dir/file.txt"), "file").expect("write file"); + std::os::unix::fs::symlink("real-dir", source.join("link-dir")).expect("create symlink"); + + let entries = upload_archive_entries(UploadSource::SinglePath { + local_path: source, + tar_name: "source-dir".into(), + }); + + let symlink = entries + .iter() + .find(|entry| entry.path == "source-dir/link-dir") + .expect("symlink archive entry"); + assert_eq!(symlink.entry_type, tar::EntryType::Symlink); + assert_eq!(symlink.link_name.as_deref(), Some("real-dir")); + assert!( + entries + .iter() + .all(|entry| entry.path != "source-dir/link-dir/file.txt"), + "symlink target should not be expanded into the archive: {entries:?}" + ); + } + + #[cfg(unix)] + #[test] + fn file_list_archive_preserves_symlink_entries() { + let tmpdir = tempfile::tempdir().expect("create tmpdir"); + let base_dir = tmpdir.path().join("nested"); + fs::create_dir_all(base_dir.join("real-dir")).expect("create dirs"); + fs::write(base_dir.join("real-dir/file.txt"), "file").expect("write file"); + std::os::unix::fs::symlink("real-dir", base_dir.join("link-dir")).expect("create symlink"); + + let entries = upload_archive_entries(UploadSource::FileList { + base_dir, + files: vec!["link-dir".into()], + archive_prefix: Some(PathBuf::from("nested")), + }); + + assert_eq!(entries.len(), 1, "unexpected archive entries: {entries:?}"); + let symlink = &entries[0]; + assert_eq!(symlink.path, "nested/link-dir"); + assert_eq!(symlink.entry_type, tar::EntryType::Symlink); + assert_eq!(symlink.link_name.as_deref(), Some("real-dir")); + } + #[test] fn split_sandbox_path_handles_root_and_bare_names() { // File directly under root