diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e6fff9c..5866f8b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,7 +34,9 @@ jobs: - 'Cargo.lock' - '.github/workflows/ci.yml' - test: + # Formatting and lints answer the same on every platform, so they run + # once rather than three times. + lint: runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v7 @@ -42,10 +44,35 @@ jobs: with: components: clippy, rustfmt - uses: swatinem/rust-cache@v2 - # Core + CLI only: building the Tauri crate needs webkitgtk, which CI - # for library tests doesn't need. The release workflow covers the app. + # Core + CLI only: building the Tauri crate needs webkitgtk, which + # lints of the library don't need. `app-tauri` below covers the shell. + # + # --all-targets so tests are linted too. `cargo test` compiles test + # code but applies no lints to it, so without this the suite — by now + # a good half of the crate — drifts unlinted while CI stays green. - run: cargo fmt --check - - run: cargo clippy -p diskern-core -p diskern-cli -- -D warnings + - run: cargo clippy -p diskern-core -p diskern-cli --all-targets -- -D warnings + + # The engine's behaviour is platform-specific in ways a run on one OS + # cannot reach, and it ships on more than one: `default_excludes` has a + # `cfg` arm per platform, most of the rules database describes Windows + # and macOS paths, and quarantine flattens drive letters and separators + # that only exist on Windows. Until this matrix, all of that was checked + # against string literals on Linux and executed on Windows for the first + # time in the release build — after the tag was pushed. + # + # fail-fast off: when a change breaks two platforms, seeing one of them + # is half a bug report. + test: + strategy: + fail-fast: false + matrix: + os: [ubuntu-22.04, windows-latest, macos-latest] + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@v7 + - uses: dtolnay/rust-toolchain@stable + - uses: swatinem/rust-cache@v2 - run: cargo test -p diskern-core -p diskern-cli # Type-checks app/src-tauri against the current diskern-core. Until this @@ -56,16 +83,34 @@ jobs: # clippy, not a full `cargo build` — the goal is catching signature drift # and lint regressions in the shell; the release workflow still owns # producing an actual bundle. + # + # Every platform the shell is built for, because the shells differ: + # Linux links webkitgtk and the GTK3 stack, Windows links WebView2, + # macOS links WKWebView, and a `cfg`-shaped mistake compiles fine on one + # and not another. Windows used to be compiled for the first time during + # the release build. + # + # macOS is here even though its release entry is still commented out. + # Leaving it off would hand whoever re-enables that entry the exact + # failure this job exists to prevent — a first compile after the tag is + # pushed. It costs a few minutes on PRs that touch `crates/` or `app/`, + # and nothing on any other PR: `changes` gates the whole job. app-tauri: needs: changes if: needs.changes.outputs.app == 'true' - runs-on: ubuntu-22.04 + strategy: + fail-fast: false + matrix: + os: [ubuntu-22.04, windows-latest, macos-latest] + runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v7 # Same apt line as release.yml. Kept in sync by hand; there is no - # cheaper way to share it between workflows. + # cheaper way to share it between workflows. Windows needs none of + # this — WebView2 ships with the OS. - name: Linux system deps (Tauri v2 / webkitgtk 4.1) + if: matrix.os == 'ubuntu-22.04' run: | sudo apt-get update sudo apt-get install -y libwebkit2gtk-4.1-dev libappindicator3-dev \ diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 5d7f949..f965935 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -566,12 +566,28 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let q = dir.path().join("quarantine"); // Raw bytes no UTF-8 decoder accepts, in an otherwise ordinary - // name: a legal filename on this platform, and one a scan of a - // real disk turns up in downloads unpacked from old archives. + // name: a legal filename on Linux, and one a scan of a real disk + // turns up in downloads unpacked from old archives. let victim = dir .path() .join(std::ffi::OsStr::from_bytes(b"photo-\xff\xfe.dmg")); - std::fs::write(&victim, b"payload").unwrap(); + + // `EILSEQ` on macOS: the errno APFS answers a filename that is + // not valid UTF-8 with. + const MACOS_EILSEQ: i32 = 92; + + match std::fs::write(&victim, b"payload") { + Ok(()) => {} + // macOS validates filenames as UTF-8 in the filesystem itself, + // so a name like this cannot exist there and the loss it + // caused is not reachable. Linux imposes no such rule, which + // is why this is checked rather than skipped by a `cfg`: if + // the fixture ever stops being creatable, that is a failure + // and not a quiet pass. Only the one refusal that means + // "this platform forbids the name" is allowed through. + Err(e) if cfg!(target_os = "macos") && e.raw_os_error() == Some(MACOS_EILSEQ) => return, + Err(e) => panic!("could not create the fixture: {e}"), + } let err = quarantine(&victim, Verdict::Review, &q).unwrap_err(); assert!( @@ -686,4 +702,58 @@ mod tests { assert_eq!(summary.bytes_removed, 0); assert!(summary.failed.is_empty()); } + + /// Quarantine names are built from the original path, and on Windows + /// that path opens `C:\` — a colon and backslashes, none of which are + /// legal in a filename. Nothing may survive the flattening. + #[test] + fn a_quarantine_name_carries_no_separator_from_the_original() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let nested = dir.path().join("a").join("b"); + std::fs::create_dir_all(&nested).unwrap(); + let victim = nested.join("victim.txt"); + std::fs::write(&victim, b"data").unwrap(); + + let record = quarantine(&victim, Verdict::Safe, &q).unwrap(); + let name = record + .quarantined_to + .file_name() + .expect("a quarantined file has a name") + .to_string_lossy() + .into_owned(); + + for separator in ['/', '\\', ':'] { + assert!( + !name.contains(separator), + "{name} still carries {separator:?}" + ); + } + assert!(record.quarantined_to.exists()); + assert_eq!(list(&q).unwrap().len(), 1); + } + + /// The Windows half of `a_path_that_cannot_be_recorded_is_not_moved`. + /// + /// Windows filenames are UTF-16 and may hold an unpaired surrogate, + /// which serde rejects exactly as it rejects invalid UTF-8 on Unix — + /// so the same data loss was reachable here. Creating such a file + /// through the Win32 API is not dependable, so this pins the guard + /// that stops the move rather than the whole flow. + #[cfg(windows)] + #[test] + fn a_windows_path_that_cannot_be_recorded_is_refused() { + use std::os::windows::ffi::OsStringExt; + + // "a\u{D800}.dat" — a lone high surrogate in an ordinary name. + let name = std::ffi::OsString::from_wide(&[0x0061, 0xD800, 0x002E, 0x0064, 0x0061, 0x0074]); + let record = QuarantineRecord { + original: PathBuf::from(name), + quarantined_to: PathBuf::from("quarantine"), + at_epoch: 0, + }; + + let err = encode(&record).unwrap_err(); + assert!(err.to_string().contains("will not be moved"), "{err}"); + } } diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index 1275eef..4f9d33a 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -421,16 +421,19 @@ mod tests { ) .unwrap(); - let find = |needle: &str| { + // Compared as paths, not as substrings of one: Windows separates + // with `\`, so "live/node_modules" matches nothing there. + let find = |root: &std::path::Path| { + let wanted = root.join("node_modules").join("react").join("index.js"); report .findings .iter() - .find(|f| f.entry.path.to_string_lossy().contains(needle)) - .unwrap() + .find(|f| f.entry.path == wanted) + .unwrap_or_else(|| panic!("no finding for {}", wanted.display())) .clone() }; - let referenced = find("live/node_modules"); + let referenced = find(&live); assert_eq!(referenced.verdict, Verdict::Risky); assert!(referenced .reasons @@ -440,7 +443,7 @@ mod tests { // offer either. assert_eq!(referenced.reclaimable, 0); - let abandoned = find("dead/node_modules"); + let abandoned = find(&dead); assert_eq!(abandoned.verdict, Verdict::Review); assert!(!abandoned .reasons diff --git a/crates/diskern-core/src/scanner.rs b/crates/diskern-core/src/scanner.rs index 570429d..3646117 100644 --- a/crates/diskern-core/src/scanner.rs +++ b/crates/diskern-core/src/scanner.rs @@ -246,6 +246,35 @@ mod tests { assert!(!is_excluded(Path::new("/process-data/x"), &excludes)); } + /// The folding in `is_within` against a directory that really exists. + /// + /// The filesystem's own case rules have no bearing on this, on any + /// platform: `is_excluded` compares two strings and never opens a + /// file. Drop the ASCII folding and this fails on Windows, macOS and + /// Linux alike — which is what makes it worth running on all three. + #[test] + fn an_exclude_matches_a_real_directory_whatever_its_case() { + let dir = tempfile::tempdir().unwrap(); + let cache = dir.path().join("Cache"); + std::fs::create_dir(&cache).unwrap(); + std::fs::write(cache.join("blob.bin"), b"cached").unwrap(); + std::fs::write(dir.path().join("keep.txt"), b"keep").unwrap(); + + let opts = ScanOptions { + roots: vec![dir.path().to_path_buf()], + // Written lowercase; the directory on disk is not. + excludes: vec![cache.to_string_lossy().to_lowercase()], + ..Default::default() + }; + let entries = scan(&opts, Arc::new(ScanProgress::default())).unwrap(); + + assert_eq!(entries.len(), 1, "{entries:#?}"); + assert_eq!( + entries[0].path.file_name().unwrap().to_string_lossy(), + "keep.txt" + ); + } + #[test] fn scans_a_temp_tree() { let dir = tempfile::tempdir().unwrap();