From caaf70a66cd9fe857a235306520ec6481e393f10 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 22 Sep 2026 21:18:45 -0700 Subject: [PATCH] fix(viewer): real errors, no leftover temp files, refuse read-only saves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix round 1 for Task 3, addressing task-3-review.md's I1-I3 (reproduced under dash) plus M3 and M10 from the same review. - I1: WRITE_SCRIPT read the target's hash through `sha256sum | cut … || exit 1`. POSIX sh has no pipefail, so that `|| exit 1` tested only cut's exit status — an unreadable target (EACCES, EIO) left $actual empty, which never equals $expect, so the script silently reported exit 3 (conflict) instead of a real error. The user got a misleading "changed on disk" banner whose "Overwrite on save" could never succeed, since the next poll hit the same read error. Fixed by reading the hash from a plain command substitution (`actual=$(sha256sum -- "$target") || exit 1`) and splitting out the hash field in shell instead of piping into `cut`. - I2 (+ M3): a failed `cp` into the staged file (ENOSPC, quota, EFBIG, EIO) left a partial `..triple-c-` behind in the user's own directory — the EXIT trap only ever removed $tmp. Fixed by creating the staged file with `mktemp` (M3: exclusive, unpredictable name, so it can't be planted or follow an existing symlink) and adding it to the trap as soon as it's assigned (`trap 'rm -f -- "$tmp" ${staged:+"$staged"}' EXIT`), so any later failure — cp, chmod, mv, or a signal — cleans it up too. - I3 (controller ruling): the script only ever checked `[ -w "$dir" ]`, so a 0444 file (or one owned by another uid) was silently replaced via rename, defeating the file's own write protection even though spec §5 step 3 reads that way literally. Added `[ -w "$target" ]` before the branch; a non-writable target is refused with "The file is read-only for the container user." on stderr and a distinct exit code (5, `EXIT_READ_ONLY`) that `classify_write` maps to that same message rather than falling into the generic clipped-stderr arm. - M10: added six `#[cfg(unix)]` tests that run WRITE_SCRIPT for real via `sh -c` against a temp directory on the host (not just needle matches against the script text) — clean save, stale-base conflict, gone target, unreadable target (I1), read-only target (I3), and a failed stage leaving no partial file behind (I2). The unreadable/ read-only tests self-skip with a message if permission bits turn out not to block root, rather than false-failing under a root test runner. Verified: `cargo test --offline file_viewer` — 19/19 passing, pristine (up from 12; 6 new host-execution tests plus 1 for the new exit-5 classify_write arm). `cargo clippy --offline` (and `--tests`) — no warnings in file_viewer::write; the 28 warnings clippy reports are all pre-existing, in unrelated files. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/src-tauri/src/file_viewer/write.rs | 262 ++++++++++++++++++++++++- 1 file changed, 255 insertions(+), 7 deletions(-) diff --git a/app/src-tauri/src/file_viewer/write.rs b/app/src-tauri/src/file_viewer/write.rs index 8697911..334e0cd 100644 --- a/app/src-tauri/src/file_viewer/write.rs +++ b/app/src-tauri/src/file_viewer/write.rs @@ -25,29 +25,56 @@ pub fn is_sha256_hex(s: &str) -> bool { } /// `$1` target, `$2` staged payload in /tmp, `$3` the hash the editor loaded from. -/// Exit 3 = changed on disk, 4 = gone; stdout on success is `sha256sum` of the target. +/// Exit 1 = a step failed (unreadable target, a failed stage/replace, …), 3 = changed +/// on disk, 4 = gone, 5 = the target is not writable by the container user; stdout on +/// success is `sha256sum` of the target. /// /// P15: `sha256sum -- "$target"` prefixes its whole line with `\` when the path /// contains a backslash or a newline, so `$actual` has that prefix stripped before /// it is compared with `$expect` (which never carries one) — otherwise such a path /// would conflict forever. +/// +/// I1: `$actual` is read from a plain `sha256sum` command substitution, not a +/// pipeline into `cut` — POSIX sh has no `pipefail`, so `cmd | cut … || exit 1` tests +/// only `cut`'s exit status and an unreadable file (EACCES, EIO) fell through as a +/// false "changed on disk" conflict (empty `$actual` never equals `$expect`) instead +/// of a real error, hiding the actual failure from the user and from `classify_write`. +/// +/// I2/M3: `$staged` is created by `mktemp` (exclusive — never follows a planted +/// symlink or stale leftover at that name) and is part of the `EXIT` trap from the +/// moment it is assigned, so a failure at any later step (`cp`, `chmod`, `mv`) cannot +/// leave a partial `..triple-c-` behind in the user's own directory — +/// including on a signal, for the steps after the trap covers it. pub const WRITE_SCRIPT: &str = r#"target=$1; tmp=$2; expect=$3 -trap 'rm -f -- "$tmp"' EXIT +staged= +trap 'rm -f -- "$tmp" ${staged:+"$staged"}' EXIT test -f "$target" || exit 4 -actual=$(sha256sum -- "$target" | cut -d' ' -f1) || exit 1 -actual=${actual#\\} +actual=$(sha256sum -- "$target") || exit 1 +actual=${actual%% *}; actual=${actual#\\} [ "$actual" = "$expect" ] || exit 3 +# I3: the file's own mode is a boundary the user set from outside the container (0444, +# a different owning uid, a read-only bind mount, …). Replacing it via rename or +# truncating it in place would silently cross that boundary even though `claude` is +# allowed to — an editor such as vim, or a plain `echo > file` in the user's own shell, +# would refuse. This is stricter than spec §5 step 3's literal "if the directory is +# writable" branch, which never looks at the file's own permissions; the branch below +# only ever chooses *how* to write, never *whether*. +[ -w "$target" ] || { echo "The file is read-only for the container user." >&2; exit 5; } dir=$(dirname -- "$target"); name=$(basename -- "$target") if [ -w "$dir" ]; then - staged="$dir/.$name.triple-c-$$" + staged=$(mktemp -- "$dir/.$name.triple-c-XXXXXX") || exit 1 cp -- "$tmp" "$staged" || exit 1 chmod --reference="$target" "$staged" 2>/dev/null - mv -f -- "$staged" "$target" || { rm -f -- "$staged"; exit 1; } + mv -f -- "$staged" "$target" || exit 1 else cat -- "$tmp" > "$target" || exit 1 fi sha256sum -- "$target""#; +/// I3: distinct from the generic failure code so the caller can hand back a specific, +/// readable message instead of whatever the script's own diagnostic text says. +const EXIT_READ_ONLY: i64 = 5; + pub enum WriteOutcome { Saved(String), Conflict, @@ -59,6 +86,9 @@ pub fn classify_write(code: i64, stdout: &str, stderr: &str) -> WriteOutcome { match code { 3 => WriteOutcome::Conflict, 4 => WriteOutcome::Gone, + EXIT_READ_ONLY => { + WriteOutcome::Failed("The file is read-only for the container user.".into()) + } 0 => match stdout .split_whitespace() .next() @@ -152,6 +182,17 @@ mod tests { assert!(matches!(classify_write(0, "junk", ""), WriteOutcome::Failed(_))); } + /// I3: exit 5 is the script's read-only refusal, and it must not be swallowed by + /// the generic `_ => Failed(stderr)` arm — the caller gets a fixed, readable + /// message regardless of exactly what the script printed. + #[test] + fn exit_five_is_a_distinct_read_only_refusal() { + assert!(matches!( + classify_write(5, "", "The file is read-only for the container user."), + WriteOutcome::Failed(m) if m.contains("read-only") + )); + } + /// P15: a target path with a backslash makes `sha256sum` prefix the line; /// the parsed hash must still be recognised as the saved hash. #[test] @@ -171,10 +212,18 @@ mod tests { "chmod --reference=\"$target\"", "mv -f --", "cat -- \"$tmp\" > \"$target\"", - "trap 'rm -f -- \"$tmp\"' EXIT", + // I2/M3: the trap covers the staged file too, and it comes from `mktemp`. + "trap 'rm -f -- \"$tmp\" ${staged:+\"$staged\"}' EXIT", + "mktemp -- \"$dir/.$name.triple-c-XXXXXX\"", + // I1: a plain command substitution, not a pipeline `cut` could mask. + "actual=$(sha256sum -- \"$target\") || exit 1", + // I3: a read-only target is refused before any write is attempted. + "[ -w \"$target\" ] || { echo \"The file is read-only for the container user.\" >&2; exit 5; }", ] { assert!(WRITE_SCRIPT.contains(needle), "missing: {}", needle); } + // The old pipeline form must be gone, not merely superseded. + assert!(!WRITE_SCRIPT.contains("cut -d' ' -f1")); } /// P8: the write script's test list is binding, and the argument order is @@ -205,4 +254,203 @@ mod tests { assert!(check_write_input(MAX_WRITE_BYTES + 1, h).is_err()); assert!(check_write_input(0, "not-a-hash").is_err()); } + + // ── M10: WRITE_SCRIPT run for real, against a temp dir on the host ────────── + // + // The needle test above only proves the script *contains* certain substrings; it + // cannot catch the pipefail-shaped bug I1 was (the needle text was correct, the + // shell semantics were not). These run the exact `sh -c SCRIPT save target tmp + // hash` invocation `write_command` builds, so they pin the exit codes and cleanup + // behaviour that `write_file`/`classify_write` actually depend on. `sh` and the + // coreutils used here (`sha256sum`, `mktemp`, `dirname`, `basename`) are present + // on dev machines and CI alike. + + #[cfg(unix)] + fn run_write_script( + target: &std::path::Path, + tmp: &std::path::Path, + base_hash: &str, + ) -> (i32, String, String) { + let out = std::process::Command::new("sh") + .arg("-c") + .arg(WRITE_SCRIPT) + .arg("save") + .arg(target) + .arg(tmp) + .arg(base_hash) + .output() + .expect("sh must be on PATH to run this test"); + ( + out.status.code().unwrap_or(-1), + String::from_utf8_lossy(&out.stdout).into_owned(), + String::from_utf8_lossy(&out.stderr).into_owned(), + ) + } + + #[cfg(unix)] + fn unique_test_dir(name: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("tc-write-{}-{}", name, uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + #[cfg(unix)] + #[test] + fn on_the_host_a_clean_save_replaces_the_file_and_cleans_up() { + let dir = unique_test_dir("clean"); + let target = dir.join("t.txt"); + let tmp = dir.join("payload"); + std::fs::write(&target, b"old\n").unwrap(); + std::fs::write(&tmp, b"new\n").unwrap(); + let base = sha256_hex(b"old\n"); + + let (code, stdout, stderr) = run_write_script(&target, &tmp, &base); + + assert_eq!(code, 0, "stdout={stdout} stderr={stderr}"); + let new_hash = sha256_hex(b"new\n"); + assert!(stdout.contains(&new_hash), "stdout={stdout}"); + assert_eq!(std::fs::read(&target).unwrap(), b"new\n"); + assert!(!tmp.exists(), "the staged /tmp payload must be cleaned up"); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[cfg(unix)] + #[test] + fn on_the_host_a_stale_base_hash_conflicts_and_leaves_everything_untouched() { + let dir = unique_test_dir("stale"); + let target = dir.join("t.txt"); + let tmp = dir.join("payload"); + std::fs::write(&target, b"old\n").unwrap(); + std::fs::write(&tmp, b"new\n").unwrap(); + let wrong_base = sha256_hex(b"not what is on disk\n"); + + let (code, _stdout, stderr) = run_write_script(&target, &tmp, &wrong_base); + + assert_eq!(code, 3, "stderr={stderr}"); + assert_eq!(std::fs::read(&target).unwrap(), b"old\n", "must be untouched"); + assert!(!tmp.exists(), "the staged /tmp payload must still be cleaned up"); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[cfg(unix)] + #[test] + fn on_the_host_a_missing_target_reports_gone() { + let dir = unique_test_dir("gone"); + let target = dir.join("does-not-exist"); + let tmp = dir.join("payload"); + std::fs::write(&tmp, b"new\n").unwrap(); + + let (code, _stdout, stderr) = run_write_script(&target, &tmp, &sha256_hex(b"whatever")); + + assert_eq!(code, 4, "stderr={stderr}"); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// I1: a real read failure must be a real error (exit 1), never the exit-3 + /// conflict a bare `sha256sum | cut` pipeline (no `pipefail` in POSIX sh) would + /// silently produce. + #[cfg(unix)] + #[test] + fn on_the_host_an_unreadable_target_is_an_error_not_a_conflict() { + use std::os::unix::fs::PermissionsExt; + let dir = unique_test_dir("unreadable"); + let target = dir.join("t.txt"); + let tmp = dir.join("payload"); + std::fs::write(&target, b"old\n").unwrap(); + std::fs::write(&tmp, b"new\n").unwrap(); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o000)).unwrap(); + + if std::fs::read(&target).is_ok() { + // Running as root (or some other bypass): 0o000 does not block reads, + // so this scenario cannot be reproduced here. + eprintln!("skipping: still able to read a 0o000 file (root?)"); + let _ = std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)); + let _ = std::fs::remove_dir_all(&dir); + return; + } + + let (code, _stdout, stderr) = run_write_script(&target, &tmp, &sha256_hex(b"old\n")); + + assert_eq!( + code, 1, + "an unreadable target must be a real error, not exit 3; stderr={stderr}" + ); + assert!(!tmp.exists(), "the staged /tmp payload must still be cleaned up"); + + let _ = std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)); + let _ = std::fs::remove_dir_all(&dir); + } + + /// I3: a target the container user cannot write is refused outright, never + /// replaced via rename. + #[cfg(unix)] + #[test] + fn on_the_host_a_read_only_target_is_refused_not_replaced() { + use std::os::unix::fs::PermissionsExt; + let dir = unique_test_dir("readonly"); + let target = dir.join("t.txt"); + let tmp = dir.join("payload"); + std::fs::write(&target, b"old\n").unwrap(); + std::fs::write(&tmp, b"new\n").unwrap(); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o444)).unwrap(); + + if std::fs::OpenOptions::new().write(true).open(&target).is_ok() { + eprintln!("skipping: still able to write a 0o444 file (root?)"); + let _ = std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)); + let _ = std::fs::remove_dir_all(&dir); + return; + } + + let (code, _stdout, stderr) = run_write_script(&target, &tmp, &sha256_hex(b"old\n")); + + assert_eq!(code as i64, EXIT_READ_ONLY, "stderr={stderr}"); + assert!(stderr.contains("read-only"), "stderr={stderr}"); + assert_eq!( + std::fs::read(&target).unwrap(), + b"old\n", + "a read-only file must not be replaced" + ); + assert!(!tmp.exists(), "the staged /tmp payload must still be cleaned up"); + + let _ = std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)); + let _ = std::fs::remove_dir_all(&dir); + } + + /// I2: a failed stage (here: an unreadable source payload, so `cp` fails after + /// `mktemp` has already created the destination) must not leave a partial + /// `..triple-c-` behind in the user's own directory. + #[cfg(unix)] + #[test] + fn on_the_host_a_failed_stage_leaves_no_partial_file_behind() { + use std::os::unix::fs::PermissionsExt; + let dir = unique_test_dir("cpfail"); + let target = dir.join("t.txt"); + let tmp = dir.join("payload"); + std::fs::write(&target, b"old\n").unwrap(); + std::fs::write(&tmp, b"new\n").unwrap(); + std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o000)).unwrap(); + + if std::fs::read(&tmp).is_ok() { + eprintln!("skipping: still able to read a 0o000 file (root?)"); + let _ = std::fs::remove_dir_all(&dir); + return; + } + + let (code, _stdout, stderr) = run_write_script(&target, &tmp, &sha256_hex(b"old\n")); + + assert_eq!(code, 1, "stderr={stderr}"); + assert_eq!(std::fs::read(&target).unwrap(), b"old\n", "must be untouched"); + let leftovers: Vec<_> = std::fs::read_dir(&dir) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.starts_with(".t.txt.triple-c-")) + .collect(); + assert!(leftovers.is_empty(), "staged file(s) left behind: {leftovers:?}"); + + let _ = std::fs::remove_dir_all(&dir); + } }