collective
Browse / github.com/storytold/gridcraft / patch
fix: save through a symlink updates the target instead of replacing the link
Saving a file opened through a symlink replaces the symlink with a regular file (Linux) On Linux, when I open an .xlsx file through a symbolic link (e.g. a shortcut on the desktop pointing to the real file elsewhere) and save it, GridCraft replaces the symlink with a regular file instead of writing to the link’s target. Steps to reproduce ln -s /path/to/original/book.xlsx ~/Desktop/book.xlsx Open ~/Desktop/book.xlsx in GridCraft Edit a cell and save (Ctrl+S) Expected: the original file /path/to/original/book.xlsx is updated and the symlink stays in place. Actual: ~/Desktop/book.xlsx becomes a regular file containing the changes; the symlink is gone and the original file is le
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- SofiaAFigueredo (imported from pull request #2594, open)
- Base
ad10881cc3347c3a07bc5f04a4e876999647e839- Size
- +159 −7 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
d2314ebcbddd2d2b17b0c4f0
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd/file.rs crates/engine/src/io.rs crates/engine/src/lib.rs
@@ -127,7 +127,9 @@ fn save(s: &mut Session, p: &Json) -> Result<Json> { Some(kind) => crate::io::save_bytes_as(wb, &path, kind)?, None => crate::io::save_bytes(wb, &path)?, };- crate::io::write_file(&path, &bytes)?;+ // The document's own file is written only where it was opened from or last saved.+ let expected = d.resolved_path.clone().filter(|_| d.path.as_deref() == Some(path.as_str()));+ let written = crate::io::write_file_expecting(&path, &bytes, expected.as_deref())?; // Only a full-fidelity format clears the dirty flag and becomes the document's file. if matches!(kind, Some(crate::io::FileKind::Xlsx | crate::io::FileKind::Json)) { // The new file is the workbook's own now, so `CELL("filename")` reports it (#1489). Set@@ -139,6 +141,7 @@ fn save(s: &mut Session, p: &Json) -> Result<Json> { } d.saved = d.wb.clone(); d.path = Some(path.clone());+ d.resolved_path = Some(written); } Ok(json!({"path": path, "bytes": bytes.len()})) }@@ -265,8 +265,39 @@ pub fn save_file_bytes(wb: &Workbook, path: &str) -> Result<Vec<u8>> { } /// The folder of the file at `path`, which relative paths to linked workbooks are relative to.+///+/// A symlink is followed, since that is where Save writes (see [`write_file`]): links are+/// rebased against the folder the bytes actually land in. pub fn source_dir(path: &str) -> Option<std::path::PathBuf> {- std::path::absolute(path).ok()?.parent().map(std::path::Path::to_path_buf)+ resolve_destination(path)?.parent().map(std::path::Path::to_path_buf)+}++/// The file `path` really names once symlinks are followed: where a save through it lands.+/// `None` when it doesn't exist (yet) or there is no file system.+pub fn resolve_path(path: impl AsRef<std::path::Path>) -> Option<std::path::PathBuf> {+ #[cfg(not(target_arch = "wasm32"))]+ {+ std::fs::canonicalize(path.as_ref()).ok()+ }+ #[cfg(target_arch = "wasm32")]+ {+ let _ = path;+ None+ }+}++/// Where a save to `path` lands: the file itself with symlinks followed, or, for a file that+/// doesn't exist yet, its folder with symlinks followed and the name added. Falls back to the+/// absolute path when nothing resolves.+pub fn resolve_destination(path: &str) -> Option<std::path::PathBuf> {+ if let Some(file) = resolve_path(path) {+ return Some(file);+ }+ let absolute = std::path::absolute(path).ok()?;+ match (absolute.parent().and_then(resolve_path), absolute.file_name()) {+ (Some(dir), Some(name)) => Some(dir.join(name)),+ _ => Some(absolute),+ } } /// The file at `path` as `CELL("filename")` reports it: absolute, so the folder is the one the@@ -635,6 +666,16 @@ pub fn read_file(path: &str) -> Result<Vec<u8>> { #[cfg(not(target_arch = "wasm32"))] pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> {+ write_file_expecting(path, bytes, None).map(|_| ())+}++/// [`write_file`] for a document's own file: `expected` is what `path` resolved to when the+/// document was opened or last saved. If a symlink in `path` has been retargeted (or left+/// dangling) since, the save fails instead of overwriting a file the user never opened (Save+/// As picks a new place). Returns the file that was written, which is what the next save+/// should expect: resolving `path` again could already name something else.+#[cfg(not(target_arch = "wasm32"))]+pub fn write_file_expecting(path: &str, bytes: &[u8], expected: Option<&std::path::Path>) -> Result<std::path::PathBuf> { use std::io::Write; script_sandbox_check(path)?; // Write to a temporary file, then rename: a crash never leaves a half-written workbook.@@ -642,8 +683,24 @@ pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> { // path (planted by another local user) makes the save fail instead of truncating the // link's target; and it inherits the destination's access restrictions, so saving a // private workbook doesn't widen who can read it.- let tmp = format!("{path}.sctmp");- let prior = std::fs::metadata(path).ok().map(|m| m.permissions());+ //+ // A symlink is followed first: renaming over the link itself would replace it with a+ // regular file and leave the real workbook untouched, so the temporary file is created+ // beside the link's target and the rename lands on it. A new file is written in its+ // folder with symlinks followed.+ let destination = resolve_destination(path).ok_or_else(|| EngineError::Other(format!("{path}: not a valid path")))?;+ if let Some(then) = expected {+ let dangling = std::fs::symlink_metadata(path).is_ok_and(|m| m.file_type().is_symlink()) && resolve_path(path).is_none();+ if dangling || destination != then {+ return Err(EngineError::Other(format!("{path}: it no longer points to {}; use Save As to choose where to write", then.display())));+ }+ }+ // Paths stay `PathBuf`s from here: a link may point at a file whose name isn't UTF-8.+ let target = destination.as_path();+ let mut tmp_name = target.as_os_str().to_owned();+ tmp_name.push(".sctmp");+ let tmp = std::path::PathBuf::from(tmp_name);+ let prior = std::fs::metadata(target).ok().map(|m| m.permissions()); let mut opts = std::fs::OpenOptions::new(); opts.write(true).create_new(true); #[cfg(unix)]@@ -663,16 +720,22 @@ pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> { let _ = std::fs::remove_file(&tmp); EngineError::Other(format!("{path}: {e}")) })?;- std::fs::rename(&tmp, path).map_err(|e| {+ std::fs::rename(&tmp, target).map_err(|e| { // A write or rename that fails (full disk, the workbook locked by another app) leaves // no temporary file beside the user's workbook. let _ = std::fs::remove_file(&tmp); EngineError::Other(format!("{path}: {e}"))- })+ })?;+ Ok(destination) } #[cfg(target_arch = "wasm32")]-pub fn write_file(path: &str, _bytes: &[u8]) -> Result<()> {+pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> {+ write_file_expecting(path, bytes, None).map(|_| ())+}++#[cfg(target_arch = "wasm32")]+pub fn write_file_expecting(path: &str, _bytes: &[u8], _expected: Option<&std::path::Path>) -> Result<std::path::PathBuf> { script_sandbox_check(path)?; Err(EngineError::Other(format!("{path}: no file system in the browser"))) }@@ -783,6 +846,88 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[cfg(not(target_arch = "wasm32"))]+ #[cfg(unix)]+ #[test]+ fn save_through_a_symlink_updates_the_target_and_keeps_the_link() {+ let dir = std::env::temp_dir().join(format!("gridcraft-fs-safety-symlink-{}", std::process::id()));+ let _ = std::fs::remove_dir_all(&dir);+ std::fs::create_dir_all(dir.join("real")).unwrap();+ let real = dir.join("real").join("book.xlsx");+ std::fs::write(&real, b"v1").unwrap();+ let link = dir.join("book.xlsx");+ std::os::unix::fs::symlink(&real, &link).unwrap();+ write_file(link.to_str().unwrap(), b"v2").unwrap();+ assert!(std::fs::symlink_metadata(&link).unwrap().file_type().is_symlink(), "the link must stay a link");+ assert_eq!(std::fs::read(&real).unwrap(), b"v2");+ assert_eq!(std::fs::read(&link).unwrap(), b"v2");+ assert!(!dir.join("real").join("book.xlsx.sctmp").exists());+ let _ = std::fs::remove_dir_all(&dir);+ }++ #[cfg(not(target_arch = "wasm32"))]+ #[cfg(unix)]+ #[test]+ fn save_refuses_a_symlink_retargeted_since_open() {+ let dir = std::env::temp_dir().join(format!("gridcraft-fs-safety-retarget-{}", std::process::id()));+ let _ = std::fs::remove_dir_all(&dir);+ std::fs::create_dir_all(&dir).unwrap();+ let (real, other) = (dir.join("real.xlsx"), dir.join("other.xlsx"));+ std::fs::write(&real, b"mine").unwrap();+ std::fs::write(&other, b"theirs").unwrap();+ let link = dir.join("book.xlsx");+ std::os::unix::fs::symlink(&real, &link).unwrap();+ let opened = resolve_path(link.to_str().unwrap()).unwrap();+ write_file_expecting(link.to_str().unwrap(), b"v2", Some(&opened)).unwrap();+ assert_eq!(std::fs::read(&real).unwrap(), b"v2");+ std::fs::remove_file(&link).unwrap();+ std::os::unix::fs::symlink(&other, &link).unwrap();+ assert!(write_file_expecting(link.to_str().unwrap(), b"v3", Some(&opened)).is_err());+ assert_eq!(std::fs::read(&other).unwrap(), b"theirs");+ // The link's folder is not where a retargeted save would have landed: links rebase+ // against the folder of the file that is really written.+ std::fs::create_dir_all(dir.join("sub")).unwrap();+ let deep = dir.join("sub").join("deep.xlsx");+ std::fs::write(&deep, b"d").unwrap();+ let deep_link = dir.join("deep-link.xlsx");+ std::os::unix::fs::symlink(&deep, &deep_link).unwrap();+ assert_eq!(source_dir(deep_link.to_str().unwrap()).unwrap(), std::fs::canonicalize(dir.join("sub")).unwrap());+ // A new file below a symlinked folder rebases against the folder it lands in.+ let dir_link = dir.join("folder-link");+ std::os::unix::fs::symlink(dir.join("sub"), &dir_link).unwrap();+ let fresh = dir_link.join("new.xlsx");+ assert_eq!(source_dir(fresh.to_str().unwrap()).unwrap(), std::fs::canonicalize(dir.join("sub")).unwrap());+ let written = write_file_expecting(fresh.to_str().unwrap(), b"n", None).unwrap();+ assert_eq!(written, std::fs::canonicalize(dir.join("sub")).unwrap().join("new.xlsx"));+ assert_eq!(std::fs::read(dir.join("sub").join("new.xlsx")).unwrap(), b"n");+ // A link left dangling since open is refused, not replaced by a regular file; a+ // regular file deleted since open is simply written again.+ std::fs::remove_file(&other).unwrap();+ let expected = std::fs::canonicalize(&deep).unwrap();+ std::fs::remove_file(&deep_link).unwrap();+ std::os::unix::fs::symlink(dir.join("gone.xlsx"), &deep_link).unwrap();+ assert!(write_file_expecting(deep_link.to_str().unwrap(), b"x", Some(&expected)).is_err());+ assert!(std::fs::symlink_metadata(&deep_link).unwrap().file_type().is_symlink());+ // A target whose name isn't UTF-8 is still written through the link.+ {+ use std::os::unix::ffi::OsStrExt;+ let odd = dir.join("sub").join(std::ffi::OsStr::from_bytes(b"odd-\xff.xlsx"));+ std::fs::write(&odd, b"o1").unwrap();+ let odd_link = dir.join("odd-link.xlsx");+ std::os::unix::fs::symlink(&odd, &odd_link).unwrap();+ write_file(odd_link.to_str().unwrap(), b"o2").unwrap();+ assert!(std::fs::symlink_metadata(&odd_link).unwrap().file_type().is_symlink());+ assert_eq!(std::fs::read(&odd).unwrap(), b"o2");+ }+ let plain = dir.join("sub").join("plain.xlsx");+ std::fs::write(&plain, b"p").unwrap();+ let expected = std::fs::canonicalize(&plain).unwrap();+ std::fs::remove_file(&plain).unwrap();+ write_file_expecting(plain.to_str().unwrap(), b"again", Some(&expected)).unwrap();+ assert_eq!(std::fs::read(&plain).unwrap(), b"again");+ let _ = std::fs::remove_dir_all(&dir);+ }+ #[cfg(not(target_arch = "wasm32"))] #[cfg(unix)] #[test]@@ -178,6 +178,9 @@ pub struct DocState { pub undo: Vec<HistoryEntry>, pub redo: Vec<HistoryEntry>, pub path: Option<String>,+ /// What `path` resolved to (symlinks followed) when the document was opened or last saved.+ /// Save refuses to write if the path now resolves elsewhere.+ pub resolved_path: Option<std::path::PathBuf>, pub saved: Arc<Workbook>, pub revision: u64, pub uid: u64,@@ -290,6 +293,7 @@ impl DocState { selection: sel, undo: vec![], redo: vec![],+ resolved_path: path.as_deref().and_then(io::resolve_path), path, revision: 1, uid: NEXT_UID.fetch_add(1, std::sync::atomic::Ordering::Relaxed),