From 03f4e6e07081dffa6229f9a98d2040573fe70b5d Mon Sep 17 00:00:00 2001 From: Denis Gomes <6731020+denisg0mes@users.noreply.github.com> Date: Sun, 25 Oct 2026 18:41:58 -0400 Subject: [PATCH] M7: images-past-budget guard: hard links, and script output over its input Two gaps CodeRabbit found in the guard: - `same_file` compared paths, plain and resolved. A second hard link to the file is a different path to the same data, so `convert`, `run --out` and Save As to the link passed the guard and truncated the file. It now asks the file system (same-file: device and inode on Unix, volume and file index on Windows). same-file was already in the lock through walkdir. - `run --in X --out X` checked the tab the script left active. A script that ends on `file.duplicate` or `file.new` leaves a tab with no count of images left in X, so X was written over. The count is now taken when X is opened and checked against `--out` whatever tab is active; the active tab's own file is still checked as before. The script check is a separate function so it can be tested without a file past the 4 GiB budget. --- Cargo.lock | 1 + Cargo.toml | 1 + apps/figcraft-cli/src/files.rs | 41 ++++++++++++++++--- crates/engine/Cargo.toml | 1 + crates/engine/src/file_access.rs | 2 +- .../engine/src/images_left_in_file_tests.rs | 13 ++++++ 6 files changed, 52 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index eb904655..76da20b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1624,6 +1624,7 @@ dependencies = [ "image", "log", "regex", + "same-file", "serde", "serde_json", "thiserror 2.0.21", diff --git a/Cargo.toml b/Cargo.toml index 64a8c8a2..e39d2e46 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,6 +49,7 @@ serde = { version = "1", features = ["derive", "rc"] } serde_json = { version = "1", features = ["float_roundtrip"] } regex = "1.13" log = "0.4" +same-file = "1.0.6" # All date/time conversion goes through chrono (no hand-rolled calendar code beyond the 400-year range extension; AGENTS.md). chrono = { version = "0.4.45", default-features = false, features = ["std"] } proptest = "1" diff --git a/apps/figcraft-cli/src/files.rs b/apps/figcraft-cli/src/files.rs index 263e1ac9..1a4f27b1 100644 --- a/apps/figcraft-cli/src/files.rs +++ b/apps/figcraft-cli/src/files.rs @@ -301,6 +301,16 @@ fn refuse_dropping_images(input: &str, out: &str, images_left_out: usize) -> Cli Ok(()) } +fn refuse_dropping_images_from_script(input: Option<(&str, usize)>, active: &figcraft_engine::DocState, out: &str) -> CliResult<()> { + if let Some((input, images_left_out)) = input { + refuse_dropping_images(input, out, images_left_out)?; + } + if let Some(images_left_out) = active.images_lost_saving_to(out) { + refuse_dropping_images(out, out, images_left_out)?; + } + Ok(()) +} + pub fn convert(input: &str, out: &str) -> CliResult { let (loaded, _) = figcraft_engine::cmd::file::load_path(input, None).map_err(|e| CliError::Failed(format!("can't open {input}: {e}")))?; refuse_dropping_images(input, out, loaded.images_past_budget)?; @@ -363,13 +373,14 @@ pub fn run_script(script: &str, input: Option<&str>, out: Option<&str>) -> CliRe let bytes = figcraft_engine::file_access::read(script).map_err(|e| CliError::Failed(e.to_string()))?; let text = String::from_utf8(bytes).map_err(|_| CliError::Failed(format!("{script}: not UTF-8")))?; let steps = parse_script(&text).map_err(|e| CliError::Failed(format!("{script}: {e}")))?; - let mut s = match input { + let (mut s, input_images_left_out) = match input { Some(path) => { let mut s = Session::empty(); s.execute("file.open", &json!({"path": path})).map_err(|e| CliError::Failed(format!("can't open {path}: {e}")))?; - s + let left_out = s.ds().map_err(|e| CliError::Failed(e.to_string()))?.images_lost_saving_to(path).unwrap_or(0); + (s, left_out) } - None => Session::new(), + None => (Session::new(), 0), }; // One undo step and all-or-nothing, the way the same commands behave through `command.batch` // (MCP, the control channel, plugins): `{"$step": n}` resolves against the steps before it, and @@ -400,9 +411,7 @@ pub fn run_script(script: &str, input: Option<&str>, out: Option<&str>) -> CliRe } if let Some(out) = out { let ds = s.ds().map_err(|e| CliError::Failed(e.to_string()))?; - if let (Some(input), Some(left_out)) = (input, ds.images_lost_saving_to(out)) { - refuse_dropping_images(input, out, left_out)?; - } + refuse_dropping_images_from_script(input.map(|input| (input, input_images_left_out)), ds, out)?; if ext(out) == "figcraft" { // Through the engine, so the version history is kept. write(out, &figcraft_engine::cmd::file::file_bytes(ds).map_err(|e| CliError::Failed(e.to_string()))?)?; @@ -544,4 +553,24 @@ mod tests { assert!(!out.exists(), "a script that failed writes nothing"); std::fs::remove_dir_all(dir).unwrap(); } + + #[test] + fn a_script_output_over_its_input_is_refused_whichever_tab_the_script_left_active() { + let dir = temp_dir("script-images-left-out"); + let input = dir.join("big.figcraft"); + std::fs::write(&input, b"kept").unwrap(); + let link = dir.join("link.figcraft"); + std::fs::hard_link(&input, &link).unwrap(); + let (input, link, other) = (input.to_str().unwrap(), link.to_str().unwrap(), dir.join("other.figcraft")); + let duplicate = Session::new(); + let active = duplicate.ds().unwrap(); + + for out in [input, link] { + let refused = refuse_dropping_images_from_script(Some((input, 3)), active, out); + assert!(matches!(&refused, Err(CliError::Failed(m)) if m.contains("3 of its images")), "{out}"); + } + assert!(refuse_dropping_images_from_script(Some((input, 3)), active, other.to_str().unwrap()).is_ok()); + assert!(refuse_dropping_images_from_script(Some((input, 0)), active, input).is_ok()); + std::fs::remove_dir_all(dir).unwrap(); + } } diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index 545d872c..1ac9f0e1 100644 --- a/crates/engine/Cargo.toml +++ b/crates/engine/Cargo.toml @@ -28,6 +28,7 @@ thiserror = { workspace = true } log = { workspace = true } base64 = { workspace = true } regex = { workspace = true } +same-file = { workspace = true } web-time = "1.1.0" [dev-dependencies] diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs index 0fda5e92..7a8ab8c9 100644 --- a/crates/engine/src/file_access.rs +++ b/crates/engine/src/file_access.rs @@ -117,7 +117,7 @@ pub(crate) fn within_app_folder(path: &Path, folder: &Path) -> bool { pub fn same_file(a: impl AsRef, b: impl AsRef) -> bool { let (a, b) = (a.as_ref(), b.as_ref()); - a == b || matches!((std::fs::canonicalize(a), std::fs::canonicalize(b)), (Ok(x), Ok(y)) if x == y) + a == b || same_file::is_same_file(a, b).unwrap_or(false) } /// `p` without Windows' `\\?\` prefix, for messages. diff --git a/crates/engine/src/images_left_in_file_tests.rs b/crates/engine/src/images_left_in_file_tests.rs index 44456ced..98e1cd8b 100644 --- a/crates/engine/src/images_left_in_file_tests.rs +++ b/crates/engine/src/images_left_in_file_tests.rs @@ -66,3 +66,16 @@ fn a_file_without_images_left_out_saves_as_before() { assert!(!s.ds().unwrap().is_dirty()); let _ = std::fs::remove_dir_all(&dir); } + +#[test] +fn a_hard_link_to_the_file_is_not_saved_over() { + let (dir, path, mut s) = opened_with_images_left_in_file("hard-link"); + let link = dir.join("link.figcraft"); + let _ = std::fs::remove_file(&link); + std::fs::hard_link(&path, &link).unwrap(); + let before = std::fs::read(&path).unwrap(); + rect(&mut s, 20.0); + assert!(s.execute("file.saveAs", &json!({"path": link.to_string_lossy()})).is_err()); + assert_eq!(std::fs::read(&path).unwrap(), before); + let _ = std::fs::remove_dir_all(&dir); +}