From dc1ce827be22940e25a61efb2ad2dfc461363efd Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Nov 2026 13:01:26 +0000 Subject: [PATCH] fix(assail): refuse an unwritable KYAML copy or sidecar before replacing the report The KYAML-mode preflight looked each output up but never tested that it could be written. A read-only report.kyaml or sidecar, or a directory the caller cannot create files in, passed the checks; its write then failed after report.json had already been replaced, leaving a new report beside stale companions. Codex raised this on #264 after it merged. Each output is now opened for writing during the preflight, without truncation; an absent one is created and removed again. The kernel makes the decision, so ACLs and ownership are honoured, not just mode bits. Two tests cover a read-only copy and an unwritable directory. Both fail without the fix and pass with it when run unprivileged; as root, which ignores permission bits, they skip. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 4 +++ src/main.rs | 25 ++++++++++++++++ tests/coverage_php_tests.rs | 58 +++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 9169f48..59d763f 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -12,6 +12,10 @@ may be the same file: a symbolic link at any of their paths, or two existing paths that are hard links to one file, is refused before anything is written. So is a path that exists but is not a regular file (a directory, say) or that cannot be looked up, such as a derived name over the filesystem's length limit. +Each output is also opened for writing before anything is written (an absent +one is created and removed again), so a read-only copy or sidecar, or a +directory the caller cannot create files in, is refused too, instead of +failing after the report has been replaced. These checks run before the writes, so a link swapped in between a check and its write is not caught, and a crash can leave a partial file: the planned generation-directory publish is meant to close both. diff --git a/src/main.rs b/src/main.rs index 1f1ee91..df68732 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1443,6 +1443,31 @@ fn kyaml_copy_path(out: &Path, sidecar: Option<&Path>) -> Result { } } } + // Lookup alone does not show that a path can be written: a read-only + // copy or sidecar, or a directory the caller may not create files in, + // would fail only after the report had been replaced. Open each output + // for writing now, without truncating it; an absent one is created and + // removed again. The kernel decides, so ACLs and ownership count too. + for (&(role, candidate), &existed) in outputs.iter().zip(&exists) { + let probe = if existed { + fs::OpenOptions::new().write(true).open(candidate).map(drop) + } else { + fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(candidate) + .and_then(|file| { + drop(file); + fs::remove_file(candidate) + }) + }; + probe.map_err(|err| { + anyhow::anyhow!( + "cannot write the {role} {}: {err}; nothing was written", + candidate.display() + ) + })?; + } Ok(path) } diff --git a/tests/coverage_php_tests.rs b/tests/coverage_php_tests.rs index 14ba67a..91032b0 100644 --- a/tests/coverage_php_tests.rs +++ b/tests/coverage_php_tests.rs @@ -505,3 +505,61 @@ fn kyaml_output_format_refuses_a_directory_at_an_output_path() { "the report was written before the copy failed" ); } + +/// Root ignores permission bits, so the permission tests below only mean +/// something when run unprivileged. They return early otherwise. +#[cfg(unix)] +fn writes_ignore_permissions(dir: &std::path::Path) -> bool { + use std::os::unix::fs::PermissionsExt; + let probe = dir.join("privilege-probe"); + std::fs::write(&probe, "").unwrap(); + std::fs::set_permissions(&probe, std::fs::Permissions::from_mode(0o444)).unwrap(); + let privileged = std::fs::OpenOptions::new().write(true).open(&probe).is_ok(); + std::fs::remove_file(&probe).unwrap(); + privileged +} + +#[cfg(unix)] +#[test] +fn kyaml_output_format_refuses_a_read_only_copy_before_writing() { + use std::os::unix::fs::PermissionsExt; + let (dir, project) = project_dir(); + if writes_ignore_permissions(dir.path()) { + eprintln!("skipped: running with privileges that ignore permission bits"); + return; + } + let out = dir.path().join("report.json"); + std::fs::write(&out, "old report\n").unwrap(); + let copy = dir.path().join("report.kyaml"); + std::fs::write(©, "old copy\n").unwrap(); + std::fs::set_permissions(©, std::fs::Permissions::from_mode(0o444)).unwrap(); + let output = assail_kyaml(&project, &out); + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("nothing was written"), "{stderr}"); + assert_eq!(std::fs::read_to_string(&out).unwrap(), "old report\n"); + assert_eq!(std::fs::read_to_string(©).unwrap(), "old copy\n"); +} + +#[cfg(unix)] +#[test] +fn kyaml_output_format_refuses_a_directory_it_cannot_create_in() { + use std::os::unix::fs::PermissionsExt; + let (dir, project) = project_dir(); + if writes_ignore_permissions(dir.path()) { + eprintln!("skipped: running with privileges that ignore permission bits"); + return; + } + let reports = dir.path().join("reports"); + std::fs::create_dir(&reports).unwrap(); + let out = reports.join("report.json"); + std::fs::write(&out, "old report\n").unwrap(); + std::fs::set_permissions(&reports, std::fs::Permissions::from_mode(0o555)).unwrap(); + let output = assail_kyaml_attested(&project, &out); + std::fs::set_permissions(&reports, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("nothing was written"), "{stderr}"); + assert_eq!(std::fs::read_to_string(&out).unwrap(), "old report\n"); + assert!(!reports.join("report.kyaml").exists()); +}