diff --git a/CHANGELOG.md b/CHANGELOG.md index a068c06..e302bc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ All notable changes to Diskern are documented here. The format follows ## [Unreleased] +### Fixed + +- A relative scan root no longer hides every finding a root-anchored rule + would have made. `diskern scan tmp` from `/var` reported nothing to + clean; roots are resolved to absolute paths before the walk +- Scanning a root inside an excluded directory says so, instead of walking + it to an empty report that reads like a clean disk + ## [0.2.0] — 2026-09-07 ### Added diff --git a/crates/diskern-cli/tests/relative_root.rs b/crates/diskern-cli/tests/relative_root.rs new file mode 100644 index 0000000..133420d --- /dev/null +++ b/crates/diskern-cli/tests/relative_root.rs @@ -0,0 +1,81 @@ +//! Issue #103. A relative scan root produced entries no anchored rule could +//! match, so the scan reported nothing to clean for a directory full of +//! matches. Driving the real binary with a working directory is the only +//! honest way to pin this: changing the current directory inside a test would +//! race every other test in the same binary. +//! +//! Unix only, and gated at the module level so the imports go with it: +//! Windows paths normalize to `c:/...`, so a `/`-anchored pattern cannot +//! match there and neither can the bug. Gating each item instead left these +//! imports unused on Windows, which is four errors under the `-D warnings` +//! clippy run CONTRIBUTING asks for. +#![cfg(unix)] + +use serde_json::json; +use std::fs; +use std::process::Command; +use tempfile::tempdir; + +/// A rule anchored at the filesystem root, like the shipped `/tmp/**` and +/// `/var/log/**`. +fn write_anchored_rules(path: &std::path::Path) { + let rules = json!({ + "version": 1, + "rules": [{ + "id": "anchored-marker", + "patterns": ["/**/*.marker"], + "category": "temp_file", + "verdict": "review", + "description": "Anchored rule for the relative-root regression." + }] + }); + fs::write(path, serde_json::to_vec(&rules).unwrap()).unwrap(); +} + +#[test] +fn a_relative_root_still_reaches_anchored_rules() { + let root = tempdir().unwrap(); + fs::write(root.path().join("scratch.marker"), b"temp").unwrap(); + let rules = root.path().join("rules.json"); + write_anchored_rules(&rules); + + // "." as the root, resolved against the child's working directory. + let output = Command::new(env!("CARGO_BIN_EXE_diskern")) + .current_dir(root.path()) + .args(["scan", ".", "--rules", "rules.json", "--top", "0"]) + .output() + .expect("diskern should start"); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("matched rule anchored-marker"), "{stdout}"); + // The printed path is the one the manifest would record on quarantine. + assert!(stdout.contains("/scratch.marker"), "{stdout}"); +} + +#[test] +fn an_absolute_root_reaches_the_same_rule() { + let root = tempdir().unwrap(); + fs::write(root.path().join("scratch.marker"), b"temp").unwrap(); + let rules = root.path().join("rules.json"); + write_anchored_rules(&rules); + + let output = Command::new(env!("CARGO_BIN_EXE_diskern")) + .args([ + "scan", + &root.path().to_string_lossy(), + "--rules", + &rules.to_string_lossy(), + "--top", + "0", + ]) + .output() + .expect("diskern should start"); + + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("matched rule anchored-marker"), "{stdout}"); +} diff --git a/crates/diskern-core/src/lib.rs b/crates/diskern-core/src/lib.rs index 25a5013..3f105b0 100644 --- a/crates/diskern-core/src/lib.rs +++ b/crates/diskern-core/src/lib.rs @@ -107,6 +107,8 @@ pub enum GenomeError { #[source] source: std::io::Error, }, + #[error("scan root {path} is inside excluded directory {exclude}")] + ExcludedRoot { path: PathBuf, exclude: String }, #[error("scan cancelled")] Cancelled, #[error("rules database error: {0}")] diff --git a/crates/diskern-core/src/scanner.rs b/crates/diskern-core/src/scanner.rs index 3646117..b67a237 100644 --- a/crates/diskern-core/src/scanner.rs +++ b/crates/diskern-core/src/scanner.rs @@ -63,25 +63,74 @@ impl ScanProgress { /// so the UI can render results while the scan runs. pub fn scan(opts: &ScanOptions, progress: Arc) -> Result> { let mut out = Vec::new(); + // Normalized once for the whole scan: the exclude list never changes, and + // both the root check below and every directory the walk opens use it. + let excludes: Vec = opts.excludes.iter().map(|e| normalize_exclude(e)).collect(); for root in &opts.roots { if progress.cancelled.load(Ordering::Relaxed) { return Err(crate::GenomeError::Cancelled); } - walk_root(root, opts, &progress, &mut out)?; + let root = absolute_root(root)?; + // Say so, rather than walking a root whose every entry the exclude + // list will drop. `/run/user/` holds real caches and sits under + // the `/run` exclude, so this is reachable — and an empty report is + // indistinguishable from a clean disk, which is the answer issue #103 + // and #82 are both about not giving. + if let Some(exclude) = excluded_by(&root, &excludes) { + return Err(crate::GenomeError::ExcludedRoot { + path: root, + exclude: exclude.to_string(), + }); + } + walk_root(&root, &excludes, opts, &progress, &mut out)?; } Ok(out) } +/// Which exclude, if any, contains `path`. +fn excluded_by<'a>(path: &Path, excludes: &'a [String]) -> Option<&'a str> { + let path = path.to_string_lossy(); + excludes + .iter() + .find(|ex| is_within(&path, ex)) + .map(String::as_str) +} + +/// Issue #103. The rules are written against absolute paths, and the ones +/// anchored at the filesystem root — `/tmp/**`, `/var/log/**` — are anchored +/// on purpose: that is what keeps `/tmp` out of `~/tmp`. `jwalk` builds every +/// entry's path from the root as it was handed in, so `diskern scan tmp` from +/// `/var` produced `tmp/systemd-private/x`, which no anchored pattern can +/// match. The walk found the files and the rules could not tell where they +/// were, so the scan reported nothing to clean. +/// +/// `absolute`, not `canonicalize`: it touches no filesystem, works on a path +/// that does not exist, and leaves symlinks alone. `canonicalize` would +/// rewrite `/var/tmp` to `/private/var/tmp` on macOS, scanning somewhere +/// other than what was asked for. +/// +/// It resolves a leading `.` but leaves `..` in place, since `a/../b` is only +/// `b` when `a` isn't a symlink. A root spelled with `..` therefore still +/// misses anchored rules; making it absolute is what the rules need, and +/// guessing past a symlink is not. +fn absolute_root(root: &Path) -> Result { + std::path::absolute(root).map_err(|source| crate::GenomeError::Io { + path: root.to_path_buf(), + source, + }) +} + fn walk_root( root: &Path, + excludes: &[String], opts: &ScanOptions, progress: &ScanProgress, out: &mut Vec, ) -> Result<()> { - // Normalized once, not per directory: `process_read_dir` runs on every - // directory the walk opens, and the exclude list never changes. - let excludes: Vec = opts.excludes.iter().map(|e| normalize_exclude(e)).collect(); + // `process_read_dir` runs on every directory the walk opens, so the list + // arrives already normalized rather than being rebuilt here. + let excludes = excludes.to_vec(); let walker = jwalk::WalkDir::new(root) .follow_links(opts.follow_symlinks) @@ -275,6 +324,83 @@ mod tests { ); } + /// Issue #103. Anchored rules only match absolute paths, so a relative + /// root has to be resolved before the walk, not after. + #[test] + fn a_relative_root_is_made_absolute() { + let cwd = std::env::current_dir().unwrap(); + assert_eq!(absolute_root(Path::new("tmp")).unwrap(), cwd.join("tmp")); + assert_eq!(absolute_root(Path::new(".")).unwrap(), cwd); + } + + /// An absolute root is already what the rules expect and must survive + /// untouched — in particular `/var/tmp` must not become the symlink + /// target `/private/var/tmp` that `canonicalize` would produce on macOS. + #[cfg(unix)] + #[test] + fn an_absolute_root_is_left_alone() { + let root = Path::new("/var/tmp"); + assert_eq!(absolute_root(root).unwrap(), root); + } + + /// The same guarantee on Windows, where it needs a different path to + /// state. `/var/tmp` is not absolute there — it is rooted but has no + /// drive, so `absolute` resolves it against the current one, which is + /// the right answer and not the one this test is about. + #[cfg(windows)] + #[test] + fn an_absolute_root_is_left_alone() { + let root = Path::new(r"C:\Users\example\AppData\Local\Temp"); + assert_eq!(absolute_root(root).unwrap(), root); + } + + /// A root the exclude list covers is an error, not an empty scan. The + /// walk would drop every entry and the report would look like a clean + /// disk, which is the answer #103 exists to stop giving. + #[test] + fn a_root_inside_an_exclude_is_refused() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("a.bin"), b"x").unwrap(); + + let err = scan( + &ScanOptions { + roots: vec![dir.path().to_path_buf()], + excludes: vec![dir.path().to_string_lossy().into_owned()], + ..Default::default() + }, + Arc::new(ScanProgress::default()), + ) + .unwrap_err(); + + assert!( + matches!(err, crate::GenomeError::ExcludedRoot { .. }), + "{err:?}" + ); + assert!(err.to_string().contains("excluded directory"), "{err}"); + } + + /// The check is about containment, not a shared prefix: `/runtime-data` + /// is not inside `/run`, the same property `is_within` is written for. + #[test] + fn a_root_merely_sharing_a_prefix_with_an_exclude_is_scanned() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("runtime-data"); + std::fs::create_dir_all(&root).unwrap(); + std::fs::write(root.join("a.bin"), b"x").unwrap(); + + let entries = scan( + &ScanOptions { + roots: vec![root], + excludes: vec![dir.path().join("run").to_string_lossy().into_owned()], + ..Default::default() + }, + Arc::new(ScanProgress::default()), + ) + .unwrap(); + + assert_eq!(entries.len(), 1); + } + #[test] fn scans_a_temp_tree() { let dir = tempfile::tempdir().unwrap();