From b016dc963d64be6ee757d97e436f8998be5aa9ed Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 00:16:51 +0000 Subject: [PATCH 01/11] feat(assail): write a KYAML copy beside the sealed JSON report `--output-format kyaml` was ignored by assail, which always writes the JCS report. With `--output report.json` it now also writes report.kyaml beside it. The JSON stays the report of record: --attest seals its bytes, so the seal and `attest verify --report` keep working unchanged, while people get the estate's YAML dialect to read. An --output path that already ends in .kyaml would make the copy overwrite the sealed report, so it is refused before anything is written. Other output formats are unchanged for assail. Integration tests drive the CLI: the JSON stays canonical and sealed (report_hash checked with sha2), the KYAML copy parses back to the same data, and a .kyaml --output is refused with nothing written. The copy passes standards' kyaml-format.sh --check on a real scan. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 8 ++++++ README.adoc | 2 +- src/main.rs | 24 ++++++++++++++++ tests/coverage_php_tests.rs | 57 +++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 50f9828..1081da7 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -2,6 +2,14 @@ === [Unreleased] +==== Added — `+assail --output-format kyaml+` writes a KYAML copy + +* With `+--output-format kyaml+` and `+--output report.json+`, `+assail+` writes +`+report.kyaml+` beside the JSON report. The JSON stays the report of record: it +is what `+--attest+` seals. An `+--output+` path that already ends in `+.kyaml+` +is refused before anything is written. Other formats are unchanged for +`+assail+`. + ==== Fixed — KYAML keys holding a line separator * KYAML output now escapes U+2028 (LS) and U+2029 (PS) as `+\uXXXX+`, as it diff --git a/README.adoc b/README.adoc index ff2f193..2bcedff 100644 --- a/README.adoc +++ b/README.adoc @@ -172,7 +172,7 @@ Globally applied flags let automation pipelines control verbosity, output format | Flag | Default | Description | `--report-view` | `Accordion` | Controls the presentation mode (`Accordion`, `Summary`, `Timeline`) that the CLI writes and that PanLL mirrors. | `--expand-sections` | `false` | Expands extra detail blocks in the formatted report. -| `--output-format` | `json` | Chooses how saved reports are serialized: `json` (I-JSON/JCS), `yaml`, `kyaml` (the estate YAML dialect, byte-compatible with `yq -o kyaml`), `nickel` or `sarif`. The CLI reuses this when it writes PanLL exports or temporal diffs. `assail` always writes JSON. +| `--output-format` | `json` | Chooses how saved reports are serialized: `json` (I-JSON/JCS), `yaml`, `kyaml` (the estate YAML dialect, byte-compatible with `yq -o kyaml`), `nickel` or `sarif`. The CLI reuses this when it writes PanLL exports or temporal diffs. `assail` always writes the JSON report (the one `--attest` seals); with `kyaml` it also writes a `.kyaml` copy beside it, so `--output` must name a `.json` file. | `--pivot` | `false` | Rotates the narrative axis for boards that expect events-first storytelling. | `--store` | unset | Directory where generated reports, exports, and artifacts should land. | `--quiet` | `false` | Suppresses informational log lines for automation. diff --git a/src/main.rs b/src/main.rs index c68c2c2..8b7376d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1485,6 +1485,22 @@ fn run_main() -> Result<()> { // exactly these bytes, so any verifier can recompute them. let report_json = panic_attack::json::to_canonical_string(&report)?; + // `--output-format kyaml` adds a KYAML copy beside the JSON report. + // The JSON stays the report of record: the seal covers its bytes. + let kyaml_path = match &output { + Some(out) if cli.output_format == ReportOutputFormat::Kyaml => { + let path = out.with_extension("kyaml"); + anyhow::ensure!( + path != *out, + "assail --output names the JSON report; use a .json name and the \ + KYAML copy is written beside it as {}", + path.display() + ); + Some(path) + } + _ => None, + }; + if let Some(output_path) = &output { fs::write(output_path, &report_json)?; qprintln!( @@ -1492,6 +1508,14 @@ fn run_main() -> Result<()> { "Report saved to: {}", output_path.display() ); + if let Some(kyaml_path) = &kyaml_path { + fs::write(kyaml_path, panic_attack::kyaml::to_string(&report)?)?; + qprintln!( + cli.quiet || headless, + "KYAML copy saved to: {}", + kyaml_path.display() + ); + } } else if cli.quiet || headless { // Machine-readable mode: emit JSON to stdout for pipeline consumers. // `--headless` makes this explicit for CI callers that cannot pass --quiet. diff --git a/tests/coverage_php_tests.rs b/tests/coverage_php_tests.rs index fe41435..06b5cf9 100644 --- a/tests/coverage_php_tests.rs +++ b/tests/coverage_php_tests.rs @@ -254,3 +254,60 @@ fn oversize_source_is_not_reported_as_analyzed() { .contains("16 MiB")); assert!(report.coverage.incomplete()); } + +#[test] +fn kyaml_output_format_writes_a_copy_beside_the_sealed_json() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("clean.php"), " Date: Thu, 5 Nov 2026 01:34:09 +0000 Subject: [PATCH 02/11] fix(assail): refuse a KYAML copy path that aliases the sealed report The copy's path was only compared lexically with --output. Three ways round that let the KYAML copy overwrite the JSON report the attestation seals: - `--output report.KYAML`: on a case-insensitive filesystem the derived report.kyaml is the same file; - a report.kyaml symlink pointing at the report (even one that does not exist yet); - a report.kyaml hard link to an existing report. kyaml_copy_path now refuses each of these before anything is written: the extension check ignores letter case, symlinks are never written through, and an existing copy path is compared with the report using same_file::is_same_file (already in the lockfile via walkdir). Integration tests cover each refusal and assert the report is left untouched; all three fail without the fix. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 7 ++-- Cargo.lock | 1 + Cargo.toml | 1 + src/main.rs | 55 ++++++++++++++++++++++++++----- tests/coverage_php_tests.rs | 65 +++++++++++++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 1081da7..32af6ae 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -6,9 +6,10 @@ * With `+--output-format kyaml+` and `+--output report.json+`, `+assail+` writes `+report.kyaml+` beside the JSON report. The JSON stays the report of record: it -is what `+--attest+` seals. An `+--output+` path that already ends in `+.kyaml+` -is refused before anything is written. Other formats are unchanged for -`+assail+`. +is what `+--attest+` seals. The copy can never land on it: an `+--output+` path +ending in `+.kyaml+` in any letter case, or a copy path that is a symbolic link +or already the same file as the report, is refused before anything is written. +Other formats are unchanged for `+assail+`. ==== Fixed — KYAML keys holding a line separator diff --git a/Cargo.lock b/Cargo.lock index d10e538..3997621 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3061,6 +3061,7 @@ dependencies = [ "proptest", "rayon", "regex", + "same-file", "serde", "serde-saphyr", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index a412e58..704affe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,6 +42,7 @@ sha2 = "0.11" hex = "0.4" getrandom = "0.4" walkdir = "2.5" +same-file = "1.0" # assail: refuse a KYAML copy that aliases the sealed JSON report toml_edit = { version = "0.23", default-features = false, features = ["parse"], optional = true } dirs = "7.0" ed25519-dalek = { version = "3.0", optional = true } diff --git a/src/main.rs b/src/main.rs index 8b7376d..410d773 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1335,6 +1335,52 @@ fn parse_axis(value: &str) -> Option { } } +/// Where `assail --output-format kyaml` writes its KYAML copy of the report +/// at `out`: beside it, with a `.kyaml` extension. +/// +/// The JSON at `out` is the report of record, and an attestation seals its +/// bytes, so the copy must never land on it. This refuses, before anything +/// is written: +/// - an `--output` already named `.kyaml` in any letter case, since on a +/// case-insensitive filesystem `report.KYAML` and `report.kyaml` are one +/// file; +/// - a copy path that is a symbolic link, which could point at the report +/// (even before the report exists); +/// - a copy path that is already the same file as the report, such as a +/// hard link to it. +fn kyaml_copy_path(out: &Path) -> Result { + let path = out.with_extension("kyaml"); + let named_kyaml = out + .extension() + .is_some_and(|ext| ext.eq_ignore_ascii_case("kyaml")); + anyhow::ensure!( + !named_kyaml, + "assail --output names the JSON report; use a .json name and the \ + KYAML copy is written beside it as {}", + path.display() + ); + if let Ok(meta) = fs::symlink_metadata(&path) { + anyhow::ensure!( + !meta.file_type().is_symlink(), + "refusing to write the KYAML copy through the symbolic link {}", + path.display() + ); + if out.exists() { + anyhow::ensure!( + !same_file::is_same_file(out, &path).with_context(|| format!( + "comparing {} with {}", + out.display(), + path.display() + ))?, + "{} is the same file as the report {}; refusing to overwrite the report", + path.display(), + out.display() + ); + } + } + Ok(path) +} + fn default_amuck_report_path() -> PathBuf { let ts = chrono::Utc::now().format("%Y%m%d%H%M%S"); PathBuf::from(format!("reports/amuck-{}.json", ts)) @@ -1489,14 +1535,7 @@ fn run_main() -> Result<()> { // The JSON stays the report of record: the seal covers its bytes. let kyaml_path = match &output { Some(out) if cli.output_format == ReportOutputFormat::Kyaml => { - let path = out.with_extension("kyaml"); - anyhow::ensure!( - path != *out, - "assail --output names the JSON report; use a .json name and the \ - KYAML copy is written beside it as {}", - path.display() - ); - Some(path) + Some(kyaml_copy_path(out)?) } _ => None, }; diff --git a/tests/coverage_php_tests.rs b/tests/coverage_php_tests.rs index 06b5cf9..db1c75a 100644 --- a/tests/coverage_php_tests.rs +++ b/tests/coverage_php_tests.rs @@ -311,3 +311,68 @@ fn kyaml_output_format_refuses_a_kyaml_named_report() { assert!(!output.status.success()); assert!(!out.exists(), "nothing is written when the name is refused"); } + +/// Run `assail --output-format kyaml --output ` on a one-file project. +fn assail_kyaml(project: &std::path::Path, out: &std::path::Path) -> std::process::Output { + Command::new(env!("CARGO_BIN_EXE_panic-attack")) + .args(["--output-format", "kyaml", "assail"]) + .arg(project) + .arg("--output") + .arg(out) + .output() + .unwrap() +} + +#[test] +fn kyaml_output_format_refuses_a_kyaml_name_in_any_case() { + // On a case-insensitive filesystem report.KYAML and report.kyaml are + // one file, so the copy would overwrite the report. + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("clean.php"), " Date: Thu, 5 Nov 2026 01:42:33 +0000 Subject: [PATCH 03/11] fix(assail): refuse a symlinked report path as well as copy path A dangling report.json -> report.kyaml link passed the alias guard, which only inspected the copy path: the JSON write created report.kyaml and the KYAML write then overwrote the sealed bytes. The two outputs share a directory and differ only in extension, so they alias only through letter-case equality, a symlink in either final component, or a hard link. The guard now refuses a symlink in either position, completing that case split; the doc comment states it. New test kyaml_output_format_refuses_a_symlinked_report_path builds the dangling link; it fails before this change and passes after. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 5 ++-- src/main.rs | 56 ++++++++++++++++++++----------------- tests/coverage_php_tests.rs | 20 +++++++++++++ 3 files changed, 54 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 32af6ae..72f9e57 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -7,8 +7,9 @@ * With `+--output-format kyaml+` and `+--output report.json+`, `+assail+` writes `+report.kyaml+` beside the JSON report. The JSON stays the report of record: it is what `+--attest+` seals. The copy can never land on it: an `+--output+` path -ending in `+.kyaml+` in any letter case, or a copy path that is a symbolic link -or already the same file as the report, is refused before anything is written. +ending in `+.kyaml+` in any letter case, a report or copy path that is a +symbolic link, or a copy path that is already the same file as the report, is +refused before anything is written. Other formats are unchanged for `+assail+`. ==== Fixed — KYAML keys holding a line separator diff --git a/src/main.rs b/src/main.rs index 410d773..65d31e0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1339,15 +1339,16 @@ fn parse_axis(value: &str) -> Option { /// at `out`: beside it, with a `.kyaml` extension. /// /// The JSON at `out` is the report of record, and an attestation seals its -/// bytes, so the copy must never land on it. This refuses, before anything -/// is written: -/// - an `--output` already named `.kyaml` in any letter case, since on a -/// case-insensitive filesystem `report.KYAML` and `report.kyaml` are one -/// file; -/// - a copy path that is a symbolic link, which could point at the report -/// (even before the report exists); -/// - a copy path that is already the same file as the report, such as a -/// hard link to it. +/// bytes, so the two writes must never reach the same file. Both paths share +/// a directory and differ only in extension, so they can alias in exactly +/// three ways, and each is refused before anything is written: +/// - the names are equal ignoring letter case (`--output report.KYAML` on a +/// case-insensitive filesystem), so an `--output` named `.kyaml` in any +/// case is refused; +/// - either path is a symbolic link, which could point at the other, even a +/// dangling link that the first write would bring into existence; +/// - the copy path is already the same file as the report, such as a hard +/// link to it. fn kyaml_copy_path(out: &Path) -> Result { let path = out.with_extension("kyaml"); let named_kyaml = out @@ -1359,24 +1360,29 @@ fn kyaml_copy_path(out: &Path) -> Result { KYAML copy is written beside it as {}", path.display() ); - if let Ok(meta) = fs::symlink_metadata(&path) { + for (role, candidate) in [("report", out), ("KYAML copy", path.as_path())] { + let is_symlink = fs::symlink_metadata(candidate) + .map(|meta| meta.file_type().is_symlink()) + .unwrap_or(false); anyhow::ensure!( - !meta.file_type().is_symlink(), - "refusing to write the KYAML copy through the symbolic link {}", - path.display() + !is_symlink, + "refusing to write the {role} through the symbolic link {} \ + (it could alias the other output)", + candidate.display() + ); + } + // Neither path is a symlink now, so only a hard link can join them. + if out.exists() && path.exists() { + anyhow::ensure!( + !same_file::is_same_file(out, &path).with_context(|| format!( + "comparing {} with {}", + out.display(), + path.display() + ))?, + "{} is the same file as the report {}; refusing to overwrite the report", + path.display(), + out.display() ); - if out.exists() { - anyhow::ensure!( - !same_file::is_same_file(out, &path).with_context(|| format!( - "comparing {} with {}", - out.display(), - path.display() - ))?, - "{} is the same file as the report {}; refusing to overwrite the report", - path.display(), - out.display() - ); - } } Ok(path) } diff --git a/tests/coverage_php_tests.rs b/tests/coverage_php_tests.rs index db1c75a..928d2e0 100644 --- a/tests/coverage_php_tests.rs +++ b/tests/coverage_php_tests.rs @@ -376,3 +376,23 @@ fn kyaml_output_format_refuses_a_copy_path_hard_linked_to_the_report() { "the report is untouched" ); } + +#[cfg(unix)] +#[test] +fn kyaml_output_format_refuses_a_symlinked_report_path() { + // The reverse alias: a dangling report.json -> report.kyaml link. The JSON + // write would create report.kyaml and the KYAML write would then + // overwrite the sealed bytes. + let dir = tempfile::tempdir().unwrap(); + let project = dir.path().join("project"); + std::fs::create_dir(&project).unwrap(); + std::fs::write(project.join("clean.php"), " Date: Thu, 5 Nov 2026 01:50:36 +0000 Subject: [PATCH 04/11] fix(assail): one alias check over every output, and require a .json report Codex found two more ways the KYAML-mode outputs could collide: - with --attest, a .attestation.json symlink or hard link to the copy let the sidecar write overwrite the KYAML copy; - --output was documented as a .json name but only .kyaml was refused, so report.yaml or report.sarif received JSON bytes. Rather than patch another case, kyaml_copy_path now checks every file assail writes in this mode: report, copy, and the attestation sidecar, whose path comes from one shared attestation_sidecar_path helper used by both the check and the writer. --output must end in .json (any case), so the names S.json, S.kyaml and S.attestation.json are distinct even case-insensitively; that leaves only symlinks and hard links as ways to alias, and both are refused before anything is written. New tests cover the .json requirement and both sidecar aliases; each fails before this change and passes after. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 8 +-- src/main.rs | 104 +++++++++++++++++++++--------------- tests/coverage_php_tests.rs | 76 ++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 72f9e57..30cff8c 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -6,10 +6,10 @@ * With `+--output-format kyaml+` and `+--output report.json+`, `+assail+` writes `+report.kyaml+` beside the JSON report. The JSON stays the report of record: it -is what `+--attest+` seals. The copy can never land on it: an `+--output+` path -ending in `+.kyaml+` in any letter case, a report or copy path that is a -symbolic link, or a copy path that is already the same file as the report, is -refused before anything is written. +is what `+--attest+` seals. `+--output+` must end in `+.json+` (any letter case), +and no two of the files written (report, copy, and the `+--attest+` sidecar) +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. Other formats are unchanged for `+assail+`. ==== Fixed — KYAML keys holding a line separator diff --git a/src/main.rs b/src/main.rs index 65d31e0..c4e18ec 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1335,54 +1335,79 @@ fn parse_axis(value: &str) -> Option { } } +/// Where `assail --attest` writes its attestation sidecar: beside the report +/// at `output` as `.attestation.json`, or in the working directory when +/// the report goes to stdout. The writer and the alias check below both use +/// this, so the two cannot drift apart. +fn attestation_sidecar_path(output: Option<&Path>) -> PathBuf { + match output { + Some(out) => { + let stem = out.file_stem().unwrap_or_default().to_string_lossy(); + let parent = out.parent().unwrap_or(Path::new(".")); + parent.join(format!("{}.attestation.json", stem)) + } + None => PathBuf::from("assail-report.attestation.json"), + } +} + /// Where `assail --output-format kyaml` writes its KYAML copy of the report -/// at `out`: beside it, with a `.kyaml` extension. +/// at `out`: beside it, with a `.kyaml` extension. `sidecar` is the +/// attestation sidecar when `--attest` is on. /// -/// The JSON at `out` is the report of record, and an attestation seals its -/// bytes, so the two writes must never reach the same file. Both paths share -/// a directory and differ only in extension, so they can alias in exactly -/// three ways, and each is refused before anything is written: -/// - the names are equal ignoring letter case (`--output report.KYAML` on a -/// case-insensitive filesystem), so an `--output` named `.kyaml` in any -/// case is refused; -/// - either path is a symbolic link, which could point at the other, even a -/// dangling link that the first write would bring into existence; -/// - the copy path is already the same file as the report, such as a hard -/// link to it. -fn kyaml_copy_path(out: &Path) -> Result { +/// The JSON at `out` is the report of record and the attestation seals its +/// bytes, so no two of the files written here may be the same file. They +/// share a directory and are named `S.json`, `S.kyaml` and +/// `S.attestation.json`, so they can alias in only three ways, and each is +/// refused before anything is written: +/// - equal names ignoring letter case: `--output` must end in `.json` (in any +/// case), which keeps the three names distinct on case-insensitive +/// filesystems too; +/// - a symbolic link at any of the paths, which could point at another, even +/// a dangling link that an earlier write would bring into existence; +/// - two existing paths that are already one file, such as hard links. +fn kyaml_copy_path(out: &Path, sidecar: Option<&Path>) -> Result { let path = out.with_extension("kyaml"); - let named_kyaml = out + let named_json = out .extension() - .is_some_and(|ext| ext.eq_ignore_ascii_case("kyaml")); + .is_some_and(|ext| ext.eq_ignore_ascii_case("json")); anyhow::ensure!( - !named_kyaml, - "assail --output names the JSON report; use a .json name and the \ - KYAML copy is written beside it as {}", + named_json, + "with --output-format kyaml, assail --output names the JSON report and \ + must end in .json; the KYAML copy is written beside it as {}", path.display() ); - for (role, candidate) in [("report", out), ("KYAML copy", path.as_path())] { + let mut outputs = vec![("report", out), ("KYAML copy", path.as_path())]; + if let Some(sidecar) = sidecar { + outputs.push(("attestation sidecar", sidecar)); + } + for &(role, candidate) in &outputs { let is_symlink = fs::symlink_metadata(candidate) .map(|meta| meta.file_type().is_symlink()) .unwrap_or(false); anyhow::ensure!( !is_symlink, "refusing to write the {role} through the symbolic link {} \ - (it could alias the other output)", + (it could alias another output)", candidate.display() ); } - // Neither path is a symlink now, so only a hard link can join them. - if out.exists() && path.exists() { - anyhow::ensure!( - !same_file::is_same_file(out, &path).with_context(|| format!( - "comparing {} with {}", - out.display(), - path.display() - ))?, - "{} is the same file as the report {}; refusing to overwrite the report", - path.display(), - out.display() - ); + // No path is a symlink now, so only a hard link can join two of them. + for (i, &(role_a, a)) in outputs.iter().enumerate() { + for &(role_b, b) in &outputs[i + 1..] { + if a.exists() && b.exists() { + anyhow::ensure!( + !same_file::is_same_file(a, b).with_context(|| format!( + "comparing {} with {}", + a.display(), + b.display() + ))?, + "the {role_b} {} is the same file as the {role_a} {}; \ + refusing to overwrite it", + b.display(), + a.display() + ); + } + } } Ok(path) } @@ -1539,9 +1564,12 @@ fn run_main() -> Result<()> { // `--output-format kyaml` adds a KYAML copy beside the JSON report. // The JSON stays the report of record: the seal covers its bytes. + let sidecar_path = chain_builder + .is_some() + .then(|| attestation_sidecar_path(output.as_deref())); let kyaml_path = match &output { Some(out) if cli.output_format == ReportOutputFormat::Kyaml => { - Some(kyaml_copy_path(out)?) + Some(kyaml_copy_path(out, sidecar_path.as_deref())?) } _ => None, }; @@ -1579,18 +1607,10 @@ fn run_main() -> Result<()> { } // Seal and write attestation sidecar - if let Some(builder) = chain_builder { + if let (Some(builder), Some(sidecar_path)) = (chain_builder, sidecar_path) { let envelope = builder.seal(report_json.as_bytes(), signing_key.as_deref())?; let attestation_json = panic_attack::json::to_canonical_string(&envelope)?; - let sidecar_path = if let Some(out) = &output { - let stem = out.file_stem().unwrap_or_default().to_string_lossy(); - let parent = out.parent().unwrap_or(Path::new(".")); - parent.join(format!("{}.attestation.json", stem)) - } else { - PathBuf::from("assail-report.attestation.json") - }; - fs::write(&sidecar_path, attestation_json)?; qprintln!( cli.quiet || headless, diff --git a/tests/coverage_php_tests.rs b/tests/coverage_php_tests.rs index 928d2e0..79693d8 100644 --- a/tests/coverage_php_tests.rs +++ b/tests/coverage_php_tests.rs @@ -396,3 +396,79 @@ fn kyaml_output_format_refuses_a_symlinked_report_path() { assert!(stderr.contains("symbolic link"), "{stderr}"); assert!(!copy.exists(), "nothing is written through the link"); } + +/// `assail_kyaml` with `--attest`, so the sidecar is a third output. +fn assail_kyaml_attested(project: &std::path::Path, out: &std::path::Path) -> std::process::Output { + Command::new(env!("CARGO_BIN_EXE_panic-attack")) + .args(["--output-format", "kyaml", "assail"]) + .arg(project) + .arg("--output") + .arg(out) + .arg("--attest") + .output() + .unwrap() +} + +/// A temp dir holding a one-file project in `project/`. +fn project_dir() -> (tempfile::TempDir, std::path::PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let project = dir.path().join("project"); + std::fs::create_dir(&project).unwrap(); + std::fs::write(project.join("clean.php"), " Date: Thu, 5 Nov 2026 02:55:54 +0000 Subject: [PATCH 05/11] fix(assail): write KYAML-mode outputs by atomic rename The alias checks run before the writes, so a process that can modify the output directory could swap in a symlink or hard link between check and write and have fs::write go through it (CodeRabbit, #264). New panic_attack::atomic_write::write_replacing writes to a same- directory temporary file opened with create_new, syncs it, and renames it over the destination. The rename replaces the directory entry: a substituted symlink is replaced rather than followed, a hard-linked inode keeps its contents, and no reader sees a partial file. std only, no new dependency; rename replaces an existing destination on Unix and Windows alike. In KYAML mode the report, copy and attestation sidecar are all written this way; the preflight checks stay so an existing link is still refused with a clear error. Unit tests cover replacement, hard links, symlinks and dangling symlinks; the three link tests fail when the helper is swapped for fs::write. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 4 ++ src/atomic_write.rs | 137 ++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + src/main.rs | 24 +++++++- 4 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 src/atomic_write.rs diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 30cff8c..ccddc6b 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -10,6 +10,10 @@ is what `+--attest+` seals. `+--output+` must end in `+.json+` (any letter case) and no two of the files written (report, copy, and the `+--attest+` sidecar) 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. +In this mode each file is written by renaming a finished temporary file into +place (`+atomic_write::write_replacing+`), so a link swapped in after those +checks is replaced rather than written through, and no reader sees a partial +report. Other formats are unchanged for `+assail+`. ==== Fixed — KYAML keys holding a line separator diff --git a/src/atomic_write.rs b/src/atomic_write.rs new file mode 100644 index 0000000..a7b266e --- /dev/null +++ b/src/atomic_write.rs @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: MPL-2.0 +// SPDX-FileCopyrightText: 2026 Jonathan D.A. Jewell + +//! Replace a file's contents by renaming a fully written temporary file over +//! it. +//! +//! [`write_replacing`] writes the bytes to a new file in the destination's +//! directory, created with `create_new` so it never opens anything that +//! already exists, flushes it to disk, and renames it onto the destination. +//! The rename replaces the directory entry itself. So, unlike [`std::fs::write`]: +//! +//! - a symbolic link at the destination is replaced, never written through; +//! - a file hard-linked to the destination keeps its old contents; +//! - a reader sees the old file or the new one, never a partial write. +//! +//! A file swapped in at the destination between a caller's checks and the +//! write is therefore replaced rather than followed. The replacement is a new +//! file, so it takes default permissions, not the old file's. + +use std::fs::{self, OpenOptions}; +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +/// Write `contents` to `path` by renaming a temporary file over it (see the +/// module documentation). +pub fn write_replacing(path: &Path, contents: impl AsRef<[u8]>) -> io::Result<()> { + let (tmp_path, mut file) = create_temp_beside(path)?; + let written = file + .write_all(contents.as_ref()) + .and_then(|()| file.sync_all()); + drop(file); + match written.and_then(|()| fs::rename(&tmp_path, path)) { + Ok(()) => Ok(()), + Err(err) => { + // Best effort: the temporary file is ours and holds nothing else. + let _ = fs::remove_file(&tmp_path); + Err(err) + } + } +} + +/// The directory `path` lives in: its parent, or `.` for a bare file name +/// (whose parent is the empty path). +fn parent_dir(path: &Path) -> &Path { + match path.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent, + _ => Path::new("."), + } +} + +/// Create a new, uniquely named file in `path`'s directory. +fn create_temp_beside(path: &Path) -> io::Result<(PathBuf, fs::File)> { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let dir = parent_dir(path); + let name = path + .file_name() + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no file name"))? + .to_string_lossy(); + let mut last_err = None; + for _ in 0..16 { + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let tmp = dir.join(format!(".{name}.{}.{n}.tmp", std::process::id())); + match OpenOptions::new().write(true).create_new(true).open(&tmp) { + Ok(file) => return Ok((tmp, file)), + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => last_err = Some(err), + Err(err) => return Err(err), + } + } + Err(last_err.unwrap_or_else(|| io::Error::other("no free temporary name"))) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn creates_and_replaces_a_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("report.json"); + write_replacing(&path, "first").unwrap(); + assert_eq!(fs::read_to_string(&path).unwrap(), "first"); + write_replacing(&path, "second").unwrap(); + assert_eq!(fs::read_to_string(&path).unwrap(), "second"); + // No temporary file is left behind. + let entries: Vec<_> = fs::read_dir(dir.path()).unwrap().collect(); + assert_eq!(entries.len(), 1); + } + + #[test] + fn a_hard_linked_file_keeps_its_contents() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("report.json"); + let other = dir.path().join("other"); + fs::write(&other, "untouched").unwrap(); + fs::hard_link(&other, &path).unwrap(); + write_replacing(&path, "new").unwrap(); + assert_eq!(fs::read_to_string(&path).unwrap(), "new"); + assert_eq!(fs::read_to_string(&other).unwrap(), "untouched"); + } + + #[cfg(unix)] + #[test] + fn a_symlink_is_replaced_not_followed() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("report.json"); + let target = dir.path().join("target"); + fs::write(&target, "untouched").unwrap(); + std::os::unix::fs::symlink(&target, &path).unwrap(); + write_replacing(&path, "new").unwrap(); + assert!(!fs::symlink_metadata(&path) + .unwrap() + .file_type() + .is_symlink()); + assert_eq!(fs::read_to_string(&path).unwrap(), "new"); + assert_eq!(fs::read_to_string(&target).unwrap(), "untouched"); + } + + #[cfg(unix)] + #[test] + fn a_dangling_symlink_does_not_create_its_target() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("report.json"); + let target = dir.path().join("report.kyaml"); + std::os::unix::fs::symlink(&target, &path).unwrap(); + write_replacing(&path, "new").unwrap(); + assert_eq!(fs::read_to_string(&path).unwrap(), "new"); + assert!(!target.exists()); + } + + #[test] + fn a_bare_file_name_lives_in_the_working_directory() { + assert_eq!(parent_dir(Path::new("report.json")), Path::new(".")); + assert_eq!(parent_dir(Path::new("out/report.json")), Path::new("out")); + assert_eq!(parent_dir(Path::new("/report.json")), Path::new("/")); + } +} diff --git a/src/lib.rs b/src/lib.rs index 6760498..1133cb3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -24,6 +24,7 @@ pub mod amuck; pub mod assail; pub mod assay; pub mod assemblyline; +pub mod atomic_write; pub mod attack; pub mod attestation; pub mod axial; diff --git a/src/main.rs b/src/main.rs index c4e18ec..07cc1d6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1365,6 +1365,11 @@ fn attestation_sidecar_path(output: Option<&Path>) -> PathBuf { /// - a symbolic link at any of the paths, which could point at another, even /// a dangling link that an earlier write would bring into existence; /// - two existing paths that are already one file, such as hard links. +/// +/// These checks run before the writes, so a link swapped in afterwards would +/// slip past them; the writes therefore go through +/// [`panic_attack::atomic_write::write_replacing`], which replaces a path's +/// directory entry instead of writing through it. fn kyaml_copy_path(out: &Path, sidecar: Option<&Path>) -> Result { let path = out.with_extension("kyaml"); let named_json = out @@ -1573,16 +1578,29 @@ fn run_main() -> Result<()> { } _ => None, }; + // In KYAML mode every output is written by renaming a finished + // temporary file into place, so a link swapped in after the checks + // above is replaced, never written through. + let write_output = |path: &Path, bytes: &[u8]| -> std::io::Result<()> { + if kyaml_path.is_some() { + panic_attack::atomic_write::write_replacing(path, bytes) + } else { + fs::write(path, bytes) + } + }; if let Some(output_path) = &output { - fs::write(output_path, &report_json)?; + write_output(output_path, report_json.as_bytes())?; qprintln!( cli.quiet || headless, "Report saved to: {}", output_path.display() ); if let Some(kyaml_path) = &kyaml_path { - fs::write(kyaml_path, panic_attack::kyaml::to_string(&report)?)?; + write_output( + kyaml_path, + panic_attack::kyaml::to_string(&report)?.as_bytes(), + )?; qprintln!( cli.quiet || headless, "KYAML copy saved to: {}", @@ -1611,7 +1629,7 @@ fn run_main() -> Result<()> { let envelope = builder.seal(report_json.as_bytes(), signing_key.as_deref())?; let attestation_json = panic_attack::json::to_canonical_string(&envelope)?; - fs::write(&sidecar_path, attestation_json)?; + write_output(&sidecar_path, attestation_json.as_bytes())?; qprintln!( cli.quiet || headless, "Attestation written to: {}", From 646f03ad7e31c49ec18d960a5ad24ad8dbd8fa14 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 03:03:30 +0000 Subject: [PATCH 06/11] fix(atomic_write): keep permissions and use a short temporary name Two regressions from the atomic-rename change (Codex, #264): - the temporary file was created with the default mode, so replacing a 0600 report under a 0022 umask left it 0644, world-readable. On Unix the destination's permission bits (when it is an existing regular file) are now copied onto the temporary file before any data is written; a new destination keeps fs::write's default; - the temporary name embedded the destination name, so a valid output name near the 255-byte component limit failed with ENAMETOOLONG. It is now `.pa-tmp..`, independent of the destination. New tests keep 0600/0640/0644, match fs::write's default for a new file, and write a 255-byte destination name; the permission and length tests fail on the previous commit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 3 +- src/atomic_write.rs | 94 ++++++++++++++++++++++++++++++++++++++------- 2 files changed, 83 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index ccddc6b..47481b4 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -13,7 +13,8 @@ paths that are hard links to one file, is refused before anything is written. In this mode each file is written by renaming a finished temporary file into place (`+atomic_write::write_replacing+`), so a link swapped in after those checks is replaced rather than written through, and no reader sees a partial -report. +report. On Unix a replaced file keeps the old file's permission bits, so a +private report stays private. Other formats are unchanged for `+assail+`. ==== Fixed — KYAML keys holding a line separator diff --git a/src/atomic_write.rs b/src/atomic_write.rs index a7b266e..95d53e3 100644 --- a/src/atomic_write.rs +++ b/src/atomic_write.rs @@ -14,8 +14,17 @@ //! - a reader sees the old file or the new one, never a partial write. //! //! A file swapped in at the destination between a caller's checks and the -//! write is therefore replaced rather than followed. The replacement is a new -//! file, so it takes default permissions, not the old file's. +//! write is therefore replaced rather than followed. +//! +//! Permissions: on Unix, when the destination is an existing regular file, +//! its permission bits are copied onto the temporary file before any data is +//! written, so replacing a `0600` report never exposes its contents. A new +//! destination gets the same default as `fs::write` (`0666` less the umask). +//! Ownership is not copied, and on other platforms the new file takes the +//! directory's default permissions. +//! +//! The temporary file has a short fixed-form name (`.pa-tmp..`), so a +//! destination name near the filesystem's length limit still works. use std::fs::{self, OpenOptions}; use std::io::{self, Write}; @@ -25,9 +34,15 @@ use std::sync::atomic::{AtomicU64, Ordering}; /// Write `contents` to `path` by renaming a temporary file over it (see the /// module documentation). pub fn write_replacing(path: &Path, contents: impl AsRef<[u8]>) -> io::Result<()> { - let (tmp_path, mut file) = create_temp_beside(path)?; - let written = file - .write_all(contents.as_ref()) + if path.file_name().is_none() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path has no file name", + )); + } + let (tmp_path, mut file) = create_temp_in(parent_dir(path))?; + let written = keep_permissions_of(path, &file) + .and_then(|()| file.write_all(contents.as_ref())) .and_then(|()| file.sync_all()); drop(file); match written.and_then(|()| fs::rename(&tmp_path, path)) { @@ -40,6 +55,25 @@ pub fn write_replacing(path: &Path, contents: impl AsRef<[u8]>) -> io::Result<() } } +/// Give `file` the permission bits of the regular file at `path`, if there +/// is one. A symlink or other non-regular entry lends nothing. +#[cfg(unix)] +fn keep_permissions_of(path: &Path, file: &fs::File) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + match fs::symlink_metadata(path) { + Ok(meta) if meta.is_file() => { + let mode = meta.permissions().mode() & 0o7777; + file.set_permissions(fs::Permissions::from_mode(mode)) + } + _ => Ok(()), + } +} + +#[cfg(not(unix))] +fn keep_permissions_of(_path: &Path, _file: &fs::File) -> io::Result<()> { + Ok(()) +} + /// The directory `path` lives in: its parent, or `.` for a bare file name /// (whose parent is the empty path). fn parent_dir(path: &Path) -> &Path { @@ -49,18 +83,13 @@ fn parent_dir(path: &Path) -> &Path { } } -/// Create a new, uniquely named file in `path`'s directory. -fn create_temp_beside(path: &Path) -> io::Result<(PathBuf, fs::File)> { +/// Create a new, uniquely named file in `dir`. +fn create_temp_in(dir: &Path) -> io::Result<(PathBuf, fs::File)> { static COUNTER: AtomicU64 = AtomicU64::new(0); - let dir = parent_dir(path); - let name = path - .file_name() - .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no file name"))? - .to_string_lossy(); let mut last_err = None; for _ in 0..16 { let n = COUNTER.fetch_add(1, Ordering::Relaxed); - let tmp = dir.join(format!(".{name}.{}.{n}.tmp", std::process::id())); + let tmp = dir.join(format!(".pa-tmp.{}.{n}", std::process::id())); match OpenOptions::new().write(true).create_new(true).open(&tmp) { Ok(file) => return Ok((tmp, file)), Err(err) if err.kind() == io::ErrorKind::AlreadyExists => last_err = Some(err), @@ -134,4 +163,43 @@ mod tests { assert_eq!(parent_dir(Path::new("out/report.json")), Path::new("out")); assert_eq!(parent_dir(Path::new("/report.json")), Path::new("/")); } + + #[cfg(unix)] + #[test] + fn an_existing_files_permissions_are_kept() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + for mode in [0o600, 0o640, 0o644] { + let path = dir.path().join(format!("report-{mode:o}.json")); + fs::write(&path, "old").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(mode)).unwrap(); + write_replacing(&path, "new").unwrap(); + let got = fs::metadata(&path).unwrap().permissions().mode() & 0o7777; + assert_eq!(got, mode, "mode {mode:o} not kept"); + } + } + + #[cfg(unix)] + #[test] + fn a_new_file_gets_the_fs_write_default() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let ours = dir.path().join("ours"); + let theirs = dir.path().join("theirs"); + write_replacing(&ours, "x").unwrap(); + fs::write(&theirs, "x").unwrap(); + let mode = |p: &Path| fs::metadata(p).unwrap().permissions().mode() & 0o7777; + assert_eq!(mode(&ours), mode(&theirs)); + } + + #[test] + fn a_destination_name_near_the_length_limit_works() { + // 250 bytes leaves room for `.json`/`.kyaml` within a 255-byte + // component limit; the temporary name must not grow with it. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join(format!("{}.kyaml", "r".repeat(249))); + assert_eq!(path.file_name().unwrap().len(), 255); + write_replacing(&path, "ok").unwrap(); + assert_eq!(fs::read_to_string(&path).unwrap(), "ok"); + } } From f2629a58429f2241366307bdffb66c09883eac7d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 03:13:28 +0000 Subject: [PATCH 07/11] fix(assail): no permission window on the temp file; unknown is not absent Two more findings on the KYAML-mode writes (Codex, #264): - the temporary file was created with the default mode and only then given the destination's, so another local user could open it in that window and keep a readable descriptor. It is now opened with the target mode (destination's, or 0666 for a new file) less the umask in the open call itself, then set exactly; it is never more readable than the file it replaces. Its name gains 8 random bytes from the existing getrandom dependency, so it is not predictable; - kyaml_copy_path treated every lookup failure as "absent", so a derived .kyaml name over the length limit let the report be written before the copy failed. Every output path is now looked up once before any write; only NotFound counts as absent, any other error stops the run with "nothing was written", and the same results drive the hard-link check. New tests: the temporary file's mode straight after open is no wider than the destination's (fails with the default open mode), and an overlong derived name is refused before the report is written (fails on the previous commit). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 5 +- src/atomic_write.rs | 93 ++++++++++++++++++++++++++++--------- src/main.rs | 34 +++++++++----- tests/coverage_php_tests.rs | 18 +++++++ 4 files changed, 117 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 47481b4..dd9617f 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -10,10 +10,13 @@ is what `+--attest+` seals. `+--output+` must end in `+.json+` (any letter case) and no two of the files written (report, copy, and the `+--attest+` sidecar) 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 cannot be looked up, such as a derived name over the +filesystem's length limit. In this mode each file is written by renaming a finished temporary file into place (`+atomic_write::write_replacing+`), so a link swapped in after those checks is replaced rather than written through, and no reader sees a partial -report. On Unix a replaced file keeps the old file's permission bits, so a +report. On Unix a replaced file keeps the old file's permission bits, and the +temporary file is never more readable than the file it replaces, so a private report stays private. Other formats are unchanged for `+assail+`. diff --git a/src/atomic_write.rs b/src/atomic_write.rs index 95d53e3..4baf197 100644 --- a/src/atomic_write.rs +++ b/src/atomic_write.rs @@ -16,15 +16,16 @@ //! A file swapped in at the destination between a caller's checks and the //! write is therefore replaced rather than followed. //! -//! Permissions: on Unix, when the destination is an existing regular file, -//! its permission bits are copied onto the temporary file before any data is -//! written, so replacing a `0600` report never exposes its contents. A new +//! Permissions: on Unix the temporary file is opened with the destination's +//! mode (less the umask) in the `open` call itself, then given that mode +//! exactly, so it is never readable more widely than the file it replaces and +//! replacing a `0600` report never exposes its contents. A new //! destination gets the same default as `fs::write` (`0666` less the umask). //! Ownership is not copied, and on other platforms the new file takes the //! directory's default permissions. //! -//! The temporary file has a short fixed-form name (`.pa-tmp..`), so a -//! destination name near the filesystem's length limit still works. +//! The temporary file has a short, unpredictable name (`.pa-tmp...`), +//! so a destination name near the filesystem's length limit still works. use std::fs::{self, OpenOptions}; use std::io::{self, Write}; @@ -40,8 +41,9 @@ pub fn write_replacing(path: &Path, contents: impl AsRef<[u8]>) -> io::Result<() "path has no file name", )); } - let (tmp_path, mut file) = create_temp_in(parent_dir(path))?; - let written = keep_permissions_of(path, &file) + let mode = existing_mode(path); + let (tmp_path, mut file) = create_temp_in(parent_dir(path), mode)?; + let written = restore_mode(&file, mode) .and_then(|()| file.write_all(contents.as_ref())) .and_then(|()| file.sync_all()); drop(file); @@ -55,22 +57,35 @@ pub fn write_replacing(path: &Path, contents: impl AsRef<[u8]>) -> io::Result<() } } -/// Give `file` the permission bits of the regular file at `path`, if there -/// is one. A symlink or other non-regular entry lends nothing. +/// The permission bits of the regular file at `path`, if there is one. A +/// symlink or other non-regular entry lends nothing. #[cfg(unix)] -fn keep_permissions_of(path: &Path, file: &fs::File) -> io::Result<()> { +fn existing_mode(path: &Path) -> Option { use std::os::unix::fs::PermissionsExt; - match fs::symlink_metadata(path) { - Ok(meta) if meta.is_file() => { - let mode = meta.permissions().mode() & 0o7777; - file.set_permissions(fs::Permissions::from_mode(mode)) - } - _ => Ok(()), + fs::symlink_metadata(path) + .ok() + .filter(fs::Metadata::is_file) + .map(|meta| meta.permissions().mode() & 0o7777) +} + +#[cfg(not(unix))] +fn existing_mode(_path: &Path) -> Option { + None +} + +/// Set the kept mode exactly. The file was opened with that mode less the +/// umask, so this can only add bits the destination already had. +#[cfg(unix)] +fn restore_mode(file: &fs::File, mode: Option) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + match mode { + Some(mode) => file.set_permissions(fs::Permissions::from_mode(mode)), + None => Ok(()), } } #[cfg(not(unix))] -fn keep_permissions_of(_path: &Path, _file: &fs::File) -> io::Result<()> { +fn restore_mode(_file: &fs::File, _mode: Option) -> io::Result<()> { Ok(()) } @@ -83,14 +98,34 @@ fn parent_dir(path: &Path) -> &Path { } } -/// Create a new, uniquely named file in `dir`. -fn create_temp_in(dir: &Path) -> io::Result<(PathBuf, fs::File)> { +/// Create a new file with an unpredictable name in `dir`. +/// +/// On Unix it is opened with `mode` (or `0666` for a new destination), less +/// the umask, in the `open` call itself, so it is never readable more widely +/// than the file it replaces, or than `fs::write` would create. +fn create_temp_in(dir: &Path, mode: Option) -> io::Result<(PathBuf, fs::File)> { static COUNTER: AtomicU64 = AtomicU64::new(0); + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(mode.unwrap_or(0o666) & 0o777); + } + #[cfg(not(unix))] + let _ = mode; let mut last_err = None; for _ in 0..16 { let n = COUNTER.fetch_add(1, Ordering::Relaxed); - let tmp = dir.join(format!(".pa-tmp.{}.{n}", std::process::id())); - match OpenOptions::new().write(true).create_new(true).open(&tmp) { + let mut random = [0_u8; 8]; + // A predictable name only costs a retry, so a failed RNG is not fatal. + let _ = getrandom::fill(&mut random); + let tmp = dir.join(format!( + ".pa-tmp.{}.{n}.{}", + std::process::id(), + hex::encode(random) + )); + match options.open(&tmp) { Ok(file) => return Ok((tmp, file)), Err(err) if err.kind() == io::ErrorKind::AlreadyExists => last_err = Some(err), Err(err) => return Err(err), @@ -202,4 +237,20 @@ mod tests { write_replacing(&path, "ok").unwrap(); assert_eq!(fs::read_to_string(&path).unwrap(), "ok"); } + + #[cfg(unix)] + #[test] + fn the_temporary_file_is_never_wider_than_the_destination() { + // Checked straight after `open`, before any `set_permissions`: a + // reader who opens the temporary file early must find it no more + // readable than the file it will replace. + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + for mode in [0o600, 0o640, 0o400] { + let (tmp, _file) = create_temp_in(dir.path(), Some(mode)).unwrap(); + let got = fs::metadata(&tmp).unwrap().permissions().mode() & 0o777; + assert_eq!(got & !mode, 0, "temp {got:o} wider than {mode:o}"); + fs::remove_file(tmp).unwrap(); + } + } } diff --git a/src/main.rs b/src/main.rs index 07cc1d6..afc0f85 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1385,21 +1385,33 @@ fn kyaml_copy_path(out: &Path, sidecar: Option<&Path>) -> Result { if let Some(sidecar) = sidecar { outputs.push(("attestation sidecar", sidecar)); } + // Look every path up before writing anything. Only "not found" counts as + // absent: any other failure (a name over the filesystem's length limit, + // a permission error) would make a later write fail after an earlier one + // had already landed, so it stops the run here instead. + let mut exists = Vec::with_capacity(outputs.len()); for &(role, candidate) in &outputs { - let is_symlink = fs::symlink_metadata(candidate) - .map(|meta| meta.file_type().is_symlink()) - .unwrap_or(false); - anyhow::ensure!( - !is_symlink, - "refusing to write the {role} through the symbolic link {} \ - (it could alias another output)", - candidate.display() - ); + match fs::symlink_metadata(candidate) { + Ok(meta) => { + anyhow::ensure!( + !meta.file_type().is_symlink(), + "refusing to write the {role} through the symbolic link {} \ + (it could alias another output)", + candidate.display() + ); + exists.push(true); + } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => exists.push(false), + Err(err) => anyhow::bail!( + "cannot use {} as the {role}: {err}; nothing was written", + candidate.display() + ), + } } // No path is a symlink now, so only a hard link can join two of them. for (i, &(role_a, a)) in outputs.iter().enumerate() { - for &(role_b, b) in &outputs[i + 1..] { - if a.exists() && b.exists() { + for (j, &(role_b, b)) in outputs.iter().enumerate().skip(i + 1) { + if exists[i] && exists[j] { anyhow::ensure!( !same_file::is_same_file(a, b).with_context(|| format!( "comparing {} with {}", diff --git a/tests/coverage_php_tests.rs b/tests/coverage_php_tests.rs index 79693d8..e9eb3d3 100644 --- a/tests/coverage_php_tests.rs +++ b/tests/coverage_php_tests.rs @@ -472,3 +472,21 @@ fn kyaml_output_format_refuses_a_sidecar_hard_linked_to_the_copy() { assert!(!out.exists(), "nothing is written"); assert_eq!(std::fs::read_to_string(©).unwrap(), "previous copy\n"); } + +#[test] +fn kyaml_output_format_refuses_an_overlong_derived_name_before_writing() { + // A 250-byte stem gives a valid 255-byte `.json` name, but the derived + // `.kyaml` name is 256 bytes, over the usual component limit. The run must + // stop before the report is written, not after. + let (dir, project) = project_dir(); + let out = dir.path().join(format!("{}.json", "r".repeat(250))); + assert_eq!(out.file_name().unwrap().len(), 255); + 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!( + !out.exists(), + "the report was written before the copy failed" + ); +} From 995e9d4581f6a2a55ae43cf4daf732e393acbba7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 03:26:19 +0000 Subject: [PATCH 08/11] fix(assail): refuse non-regular outputs; companions take the report's mode Two more findings on the KYAML-mode outputs (Codex, #264): - a directory (or other non-regular entry) at a derived path passed the preflight, so the report was replaced before the rename onto it failed. The preflight now refuses any existing output path that is not a regular file, before anything is written; - a new KYAML copy or sidecar took the default mode even when the report was private, exposing the same data. New atomic_write::write_replacing_like gives a new destination the permission bits of the file it accompanies; the copy and sidecar pass the report. An existing destination keeps its own bits. Tests: a 0600 report yields 0600 copy and sidecar, and a directory at report.kyaml is refused with the report unwritten (both fail on the previous commit); a unit test covers new versus existing companions. Concurrent runs targeting the same outputs (also raised) can interleave; that predates this change for the report and sidecar and is left for a separate decision. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 9 +++++---- src/atomic_write.rs | 33 ++++++++++++++++++++++++++++++- src/main.rs | 36 ++++++++++++++++++++++++---------- tests/coverage_php_tests.rs | 39 +++++++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index dd9617f..8a5a6c1 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -10,14 +10,15 @@ is what `+--attest+` seals. `+--output+` must end in `+.json+` (any letter case) and no two of the files written (report, copy, and the `+--attest+` sidecar) 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 cannot be looked up, such as a derived name over the -filesystem's length limit. +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. In this mode each file is written by renaming a finished temporary file into place (`+atomic_write::write_replacing+`), so a link swapped in after those checks is replaced rather than written through, and no reader sees a partial report. On Unix a replaced file keeps the old file's permission bits, and the -temporary file is never more readable than the file it replaces, so a -private report stays private. +temporary file is never more readable than the file it replaces; a new copy or +sidecar takes the report's permission bits, so a private report's companions +are private too. Other formats are unchanged for `+assail+`. ==== Fixed — KYAML keys holding a line separator diff --git a/src/atomic_write.rs b/src/atomic_write.rs index 4baf197..1dec3c1 100644 --- a/src/atomic_write.rs +++ b/src/atomic_write.rs @@ -35,13 +35,25 @@ use std::sync::atomic::{AtomicU64, Ordering}; /// Write `contents` to `path` by renaming a temporary file over it (see the /// module documentation). pub fn write_replacing(path: &Path, contents: impl AsRef<[u8]>) -> io::Result<()> { + write_replacing_like(path, contents, None) +} + +/// [`write_replacing`] for a file that accompanies another: when `path` does +/// not exist yet, it takes the permission bits of the regular file at `like` +/// rather than the default, so a copy of a private report is private too. An +/// existing `path` keeps its own bits. +pub fn write_replacing_like( + path: &Path, + contents: impl AsRef<[u8]>, + like: Option<&Path>, +) -> io::Result<()> { if path.file_name().is_none() { return Err(io::Error::new( io::ErrorKind::InvalidInput, "path has no file name", )); } - let mode = existing_mode(path); + let mode = existing_mode(path).or_else(|| like.and_then(existing_mode)); let (tmp_path, mut file) = create_temp_in(parent_dir(path), mode)?; let written = restore_mode(&file, mode) .and_then(|()| file.write_all(contents.as_ref())) @@ -253,4 +265,23 @@ mod tests { fs::remove_file(tmp).unwrap(); } } + + #[cfg(unix)] + #[test] + fn a_new_companion_takes_the_mode_of_the_file_it_accompanies() { + use std::os::unix::fs::PermissionsExt; + let mode = |p: &Path| fs::metadata(p).unwrap().permissions().mode() & 0o7777; + let dir = tempfile::tempdir().unwrap(); + let report = dir.path().join("report.json"); + fs::write(&report, "{}").unwrap(); + fs::set_permissions(&report, fs::Permissions::from_mode(0o600)).unwrap(); + // A new copy follows the report. + let copy = dir.path().join("report.kyaml"); + write_replacing_like(©, "new", Some(&report)).unwrap(); + assert_eq!(mode(©), 0o600); + // An existing file keeps its own bits. + fs::set_permissions(©, fs::Permissions::from_mode(0o640)).unwrap(); + write_replacing_like(©, "again", Some(&report)).unwrap(); + assert_eq!(mode(©), 0o640); + } } diff --git a/src/main.rs b/src/main.rs index afc0f85..0b69ebb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1399,6 +1399,14 @@ fn kyaml_copy_path(out: &Path, sidecar: Option<&Path>) -> Result { (it could alias another output)", candidate.display() ); + // A directory or other special entry can't be replaced by a + // rename, so the write would fail after earlier ones landed. + anyhow::ensure!( + meta.is_file(), + "the {role} path {} exists and is not a regular file; \ + nothing was written", + candidate.display() + ); exists.push(true); } Err(err) if err.kind() == std::io::ErrorKind::NotFound => exists.push(false), @@ -1592,17 +1600,20 @@ fn run_main() -> Result<()> { }; // In KYAML mode every output is written by renaming a finished // temporary file into place, so a link swapped in after the checks - // above is replaced, never written through. - let write_output = |path: &Path, bytes: &[u8]| -> std::io::Result<()> { - if kyaml_path.is_some() { - panic_attack::atomic_write::write_replacing(path, bytes) - } else { - fs::write(path, bytes) - } - }; + // above is replaced, never written through. A new copy or sidecar + // takes the report's permissions (`like`), so a private report's + // companions are private too. + let write_output = + |path: &Path, bytes: &[u8], like: Option<&Path>| -> std::io::Result<()> { + if kyaml_path.is_some() { + panic_attack::atomic_write::write_replacing_like(path, bytes, like) + } else { + fs::write(path, bytes) + } + }; if let Some(output_path) = &output { - write_output(output_path, report_json.as_bytes())?; + write_output(output_path, report_json.as_bytes(), None)?; qprintln!( cli.quiet || headless, "Report saved to: {}", @@ -1612,6 +1623,7 @@ fn run_main() -> Result<()> { write_output( kyaml_path, panic_attack::kyaml::to_string(&report)?.as_bytes(), + Some(output_path), )?; qprintln!( cli.quiet || headless, @@ -1641,7 +1653,11 @@ fn run_main() -> Result<()> { let envelope = builder.seal(report_json.as_bytes(), signing_key.as_deref())?; let attestation_json = panic_attack::json::to_canonical_string(&envelope)?; - write_output(&sidecar_path, attestation_json.as_bytes())?; + write_output( + &sidecar_path, + attestation_json.as_bytes(), + output.as_deref(), + )?; qprintln!( cli.quiet || headless, "Attestation written to: {}", diff --git a/tests/coverage_php_tests.rs b/tests/coverage_php_tests.rs index e9eb3d3..5f6993b 100644 --- a/tests/coverage_php_tests.rs +++ b/tests/coverage_php_tests.rs @@ -490,3 +490,42 @@ fn kyaml_output_format_refuses_an_overlong_derived_name_before_writing() { "the report was written before the copy failed" ); } + +#[cfg(unix)] +#[test] +fn kyaml_output_format_keeps_a_private_reports_companions_private() { + use std::os::unix::fs::PermissionsExt; + let mode = |p: &std::path::Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o777; + let (dir, project) = project_dir(); + let out = dir.path().join("report.json"); + std::fs::write(&out, "{}\n").unwrap(); + std::fs::set_permissions(&out, std::fs::Permissions::from_mode(0o600)).unwrap(); + let output = assail_kyaml_attested(&project, &out); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(mode(&out), 0o600, "report"); + assert_eq!(mode(&dir.path().join("report.kyaml")), 0o600, "KYAML copy"); + assert_eq!( + mode(&dir.path().join("report.attestation.json")), + 0o600, + "sidecar" + ); +} + +#[test] +fn kyaml_output_format_refuses_a_directory_at_an_output_path() { + let (dir, project) = project_dir(); + let out = dir.path().join("report.json"); + std::fs::create_dir(dir.path().join("report.kyaml")).unwrap(); + let output = assail_kyaml(&project, &out); + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("not a regular file"), "{stderr}"); + assert!( + !out.exists(), + "the report was written before the copy failed" + ); +} From 364d1be42c1f246e9b32415d0379788e9e0deb6a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 03:36:34 +0000 Subject: [PATCH 09/11] fix(atomic_write): carry only rwx bits, never set-ID or sticky bits The helper promised to keep the replaced file's permission bits, but it applied set-ID bits before writing: an unprivileged writer had them cleared by the kernel, while a root writer kept them (Codex, #264). A written report should never gain set-user-ID, set-group-ID or sticky bits, so existing_mode now keeps only mode & 0o777 and the docs say so. The result no longer depends on who runs assail. New test: replacing a 04755 file gives 0755 (the previous code kept 04755 when run as root). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- src/atomic_write.rs | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/atomic_write.rs b/src/atomic_write.rs index 1dec3c1..d3bb1da 100644 --- a/src/atomic_write.rs +++ b/src/atomic_write.rs @@ -21,8 +21,10 @@ //! exactly, so it is never readable more widely than the file it replaces and //! replacing a `0600` report never exposes its contents. A new //! destination gets the same default as `fs::write` (`0666` less the umask). -//! Ownership is not copied, and on other platforms the new file takes the -//! directory's default permissions. +//! Only the read, write and execute bits (`0o777`) carry over: set-user-ID, +//! set-group-ID and sticky bits are dropped on purpose, since a written report +//! should never gain them. Ownership is not copied, and on other platforms the +//! new file takes the directory's default permissions. //! //! The temporary file has a short, unpredictable name (`.pa-tmp...`), //! so a destination name near the filesystem's length limit still works. @@ -69,15 +71,16 @@ pub fn write_replacing_like( } } -/// The permission bits of the regular file at `path`, if there is one. A -/// symlink or other non-regular entry lends nothing. +/// The read, write and execute bits of the regular file at `path`, if there +/// is one (special bits are dropped; see the module docs). A symlink or other +/// non-regular entry lends nothing. #[cfg(unix)] fn existing_mode(path: &Path) -> Option { use std::os::unix::fs::PermissionsExt; fs::symlink_metadata(path) .ok() .filter(fs::Metadata::is_file) - .map(|meta| meta.permissions().mode() & 0o7777) + .map(|meta| meta.permissions().mode() & 0o777) } #[cfg(not(unix))] @@ -284,4 +287,19 @@ mod tests { write_replacing_like(©, "again", Some(&report)).unwrap(); assert_eq!(mode(©), 0o640); } + + #[cfg(unix)] + #[test] + fn special_bits_are_not_carried_over() { + // A report should never gain set-user-ID, set-group-ID or sticky + // bits from the file it replaces; the rwx bits still carry over. + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("report.json"); + fs::write(&path, "old").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o4755)).unwrap(); + write_replacing(&path, "new").unwrap(); + let got = fs::metadata(&path).unwrap().permissions().mode() & 0o7777; + assert_eq!(got, 0o755); + } } From f382867614787b48327c447aa4f39dd49c92b121 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 04:48:01 +0000 Subject: [PATCH 10/11] fix(atomic_write): flush the directory after the rename The temporary file was synced before the rename, but not the directory afterwards; on POSIX a file sync does not persist the directory entry, so a crash soon after could revert the rename, bringing back the old report or losing a new copy or sidecar (CodeRabbit, #264). After a successful rename, write_replacing_like now opens the parent directory (or "." for a bare name) and syncs it on Unix; elsewhere it is a no-op. A failed directory sync is returned without rolling back the rename, so the caller learns durability is uncertain. A unit test checks the sync reaches a real directory for bare and nested paths. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- CHANGELOG.adoc | 3 ++- src/atomic_write.rs | 29 +++++++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 8a5a6c1..00f0d7a 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -13,7 +13,8 @@ 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. In this mode each file is written by renaming a finished temporary file into -place (`+atomic_write::write_replacing+`), so a link swapped in after those +place (`+atomic_write::write_replacing+`, which also flushes the directory so +the rename survives a crash), so a link swapped in after those checks is replaced rather than written through, and no reader sees a partial report. On Unix a replaced file keeps the old file's permission bits, and the temporary file is never more readable than the file it replaces; a new copy or diff --git a/src/atomic_write.rs b/src/atomic_write.rs index d3bb1da..1f764d9 100644 --- a/src/atomic_write.rs +++ b/src/atomic_write.rs @@ -6,7 +6,8 @@ //! //! [`write_replacing`] writes the bytes to a new file in the destination's //! directory, created with `create_new` so it never opens anything that -//! already exists, flushes it to disk, and renames it onto the destination. +//! already exists, flushes it to disk, renames it onto the destination, and +//! (on Unix) flushes the directory so the rename itself survives a crash. //! The rename replaces the directory entry itself. So, unlike [`std::fs::write`]: //! //! - a symbolic link at the destination is replaced, never written through; @@ -62,7 +63,8 @@ pub fn write_replacing_like( .and_then(|()| file.sync_all()); drop(file); match written.and_then(|()| fs::rename(&tmp_path, path)) { - Ok(()) => Ok(()), + // The rename is in place; make its directory entry durable too. + Ok(()) => sync_parent_dir(path), Err(err) => { // Best effort: the temporary file is ours and holds nothing else. let _ = fs::remove_file(&tmp_path); @@ -104,6 +106,20 @@ fn restore_mode(_file: &fs::File, _mode: Option) -> io::Result<()> { Ok(()) } +/// Flush `path`'s directory so the rename survives a crash: on Unix a file +/// sync does not persist the directory entry that names it. An error here +/// means the new file is in place but its durability is unknown; the rename +/// is not rolled back. Elsewhere this is a no-op. +#[cfg(unix)] +fn sync_parent_dir(path: &Path) -> io::Result<()> { + fs::File::open(parent_dir(path))?.sync_all() +} + +#[cfg(not(unix))] +fn sync_parent_dir(_path: &Path) -> io::Result<()> { + Ok(()) +} + /// The directory `path` lives in: its parent, or `.` for a bare file name /// (whose parent is the empty path). fn parent_dir(path: &Path) -> &Path { @@ -302,4 +318,13 @@ mod tests { let got = fs::metadata(&path).unwrap().permissions().mode() & 0o7777; assert_eq!(got, 0o755); } + + #[test] + fn the_directory_sync_resolves_bare_and_nested_names() { + // A crash cannot be simulated here; this checks the sync reaches a + // real directory for both shapes of path. + let dir = tempfile::tempdir().unwrap(); + sync_parent_dir(&dir.path().join("report.json")).unwrap(); + sync_parent_dir(Path::new("report.json")).unwrap(); + } } From e895f6a0f21863d252f19934d403673940e8fb3a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 05:59:09 +0000 Subject: [PATCH 11/11] fix(atomic_write): open the directory before the rename f382867 opened the parent directory for its fsync only after the rename. A directory with write and search but not read permission accepts the rename and then refuses the open, so assail stopped with the report already replaced and the KYAML copy and sidecar unwritten (CodeRabbit, #264). The directory is now opened before the rename and its handle kept for the sync, so that failure happens with the destination untouched and the temporary file removed. New test: a 0300 output directory leaves an existing report unchanged and no temporary file behind. It skips itself when privileged; run as nobody it passes, and fails on f382867 ("new" written before the error). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --- src/atomic_write.rs | 62 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 12 deletions(-) diff --git a/src/atomic_write.rs b/src/atomic_write.rs index 1f764d9..6a793af 100644 --- a/src/atomic_write.rs +++ b/src/atomic_write.rs @@ -62,9 +62,13 @@ pub fn write_replacing_like( .and_then(|()| file.write_all(contents.as_ref())) .and_then(|()| file.sync_all()); drop(file); - match written.and_then(|()| fs::rename(&tmp_path, path)) { + // Open the directory before the rename, so a directory that can be + // written but not read fails here, with the destination untouched, + // rather than after it has been replaced. + let dir = written.and_then(|()| open_parent_dir(path)); + match dir.and_then(|dir| fs::rename(&tmp_path, path).map(|()| dir)) { // The rename is in place; make its directory entry durable too. - Ok(()) => sync_parent_dir(path), + Ok(dir) => sync_dir(dir), Err(err) => { // Best effort: the temporary file is ours and holds nothing else. let _ = fs::remove_file(&tmp_path); @@ -106,18 +110,24 @@ fn restore_mode(_file: &fs::File, _mode: Option) -> io::Result<()> { Ok(()) } -/// Flush `path`'s directory so the rename survives a crash: on Unix a file -/// sync does not persist the directory entry that names it. An error here -/// means the new file is in place but its durability is unknown; the rename -/// is not rolled back. Elsewhere this is a no-op. +/// Open `path`'s directory so it can be flushed after the rename: on Unix a +/// file sync does not persist the directory entry that names it. Elsewhere +/// there is nothing to open. #[cfg(unix)] -fn sync_parent_dir(path: &Path) -> io::Result<()> { - fs::File::open(parent_dir(path))?.sync_all() +fn open_parent_dir(path: &Path) -> io::Result> { + fs::File::open(parent_dir(path)).map(Some) } #[cfg(not(unix))] -fn sync_parent_dir(_path: &Path) -> io::Result<()> { - Ok(()) +fn open_parent_dir(_path: &Path) -> io::Result> { + Ok(None) +} + +/// Flush the directory opened by [`open_parent_dir`] so the rename survives a +/// crash. An error here means the new file is in place but its durability is +/// unknown; the rename is not rolled back. +fn sync_dir(dir: Option) -> io::Result<()> { + dir.map_or(Ok(()), |dir| dir.sync_all()) } /// The directory `path` lives in: its parent, or `.` for a bare file name @@ -324,7 +334,35 @@ mod tests { // A crash cannot be simulated here; this checks the sync reaches a // real directory for both shapes of path. let dir = tempfile::tempdir().unwrap(); - sync_parent_dir(&dir.path().join("report.json")).unwrap(); - sync_parent_dir(Path::new("report.json")).unwrap(); + sync_dir(open_parent_dir(&dir.path().join("report.json")).unwrap()).unwrap(); + sync_dir(open_parent_dir(Path::new("report.json")).unwrap()).unwrap(); + } + + #[cfg(unix)] + #[test] + fn an_unreadable_directory_fails_before_the_destination_is_replaced() { + // A directory that can be written but not read accepts the rename, + // but cannot be opened for the sync. That must fail first, leaving + // the old file in place. Privileged users bypass the permission, so + // the test skips itself when the directory opens anyway. + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let out = dir.path().join("out"); + fs::create_dir(&out).unwrap(); + let path = out.join("report.json"); + fs::write(&path, "old").unwrap(); + fs::set_permissions(&out, fs::Permissions::from_mode(0o300)).unwrap(); + let privileged = fs::File::open(&out).is_ok(); + let result = (!privileged).then(|| write_replacing(&path, "new")); + fs::set_permissions(&out, fs::Permissions::from_mode(0o700)).unwrap(); + if let Some(result) = result { + assert!(result.is_err(), "the write should fail"); + assert_eq!(fs::read_to_string(&path).unwrap(), "old"); + assert_eq!( + fs::read_dir(&out).unwrap().count(), + 1, + "temp file left behind" + ); + } } }