mirror of
https://github.com/storytold/figcraft.git
synced 2026-10-26 11:26:50 -06:00
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.
This commit is contained in:
Generated
+1
@@ -1624,6 +1624,7 @@ dependencies = [
|
||||
"image",
|
||||
"log",
|
||||
"regex",
|
||||
"same-file",
|
||||
"serde",
|
||||
"serde_json",
|
||||
"thiserror 2.0.21",
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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<ExitCode> {
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -117,7 +117,7 @@ pub(crate) fn within_app_folder(path: &Path, folder: &Path) -> bool {
|
||||
|
||||
pub fn same_file(a: impl AsRef<Path>, b: impl AsRef<Path>) -> 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.
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user