diff --git a/.cargo/audit.toml b/.cargo/audit.toml new file mode 100644 index 0000000..96ba478 --- /dev/null +++ b/.cargo/audit.toml @@ -0,0 +1,58 @@ +# cargo-audit configuration. +# +# Everything listed below is an *informational* advisory (unmaintained or +# unsound), not a vulnerability. `cargo audit` reports zero vulnerabilities +# in this lockfile and this file does not change that: it only suppresses +# the ids named here, so a real advisory — including a new one against any +# of these same crates — still fails the run. +# +# Every id here reaches the lockfile through `tauri`, is a Linux-desktop +# dependency, and has no version that clears it. Diskern cannot fix them by +# updating; Tauri has to move off GTK3 first. Keeping them permanently red +# would train everyone to ignore a red audit, which is a worse security +# outcome than an explicit, reviewed list. +# +# The reasoning, the review cadence, and how to check whether an entry can +# be dropped are in docs/DEPENDENCY-AUTOMATION.md. +# +# Reviewed: 2026-09-05. Re-check when Tauri ships a GTK4/webkit6 runtime. + +[advisories] +ignore = [ + # --------------------------------------------------------------- + # gtk-rs GTK3 bindings — archived upstream, no successor at 0.18. + # tauri 2.11 -> gtk 0.18 / webkit2gtk / tao / muda. The successor + # (gtk4-rs) is not what Tauri v2 links against on Linux. + # --------------------------------------------------------------- + "RUSTSEC-2024-0413", # atk + "RUSTSEC-2024-0416", # atk-sys + "RUSTSEC-2024-0412", # gdk + "RUSTSEC-2024-0418", # gdk-sys + "RUSTSEC-2024-0411", # gdkwayland-sys + "RUSTSEC-2024-0417", # gdkx11 + "RUSTSEC-2024-0414", # gdkx11-sys + "RUSTSEC-2024-0415", # gtk + "RUSTSEC-2024-0420", # gtk-sys + "RUSTSEC-2024-0419", # gtk3-macros + + # glib 0.18.5 — unsound VariantStrIter. The fix landed in the 0.19 + # line, which the archived GTK3 bindings above never moved to. The + # unsound function is glib's own GVariant iteration; nothing in + # Diskern calls it, and the GTK3 stack that does is the one Tauri + # drives. + "RUSTSEC-2024-0429", + + # proc-macro-error 1.0.4 — unmaintained, pulled in by glib-macros + # 0.18.5, so it leaves with the GTK3 bindings above. Build-time + # proc macro: it does not ship in the binary. + "RUSTSEC-2024-0370", + + # unic-* 0.9.0 — unmaintained, all five via urlpattern 0.3 -> + # tauri-utils. Pure Unicode tables, no network or filesystem + # surface, and replacing urlpattern is Tauri's call not ours. + "RUSTSEC-2025-0081", # unic-char-property + "RUSTSEC-2025-0075", # unic-char-range + "RUSTSEC-2025-0080", # unic-common + "RUSTSEC-2025-0100", # unic-ucd-ident + "RUSTSEC-2025-0098", # unic-ucd-version +] diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml index 8707290..cf2b898 100644 --- a/.github/workflows/audit.yml +++ b/.github/workflows/audit.yml @@ -12,7 +12,9 @@ on: push: branches: [main, master] pull_request: - paths: [Cargo.lock] + # The ignore list is part of what the audit means, so editing it has to + # re-run the audit just as moving the lockfile does. + paths: [Cargo.lock, .cargo/audit.toml] jobs: # PR-time advisory check: annotates the changed lockfile inline so the @@ -26,9 +28,23 @@ jobs: checks: write # the action reports advisories as a check run steps: - uses: actions/checkout@v7 + + # `rustsec/audit-check` does not read .cargo/audit.toml — it only + # takes an `ignore` input — while the plain `cargo audit` in the job + # below does. Reading the ids out of the file keeps one source of + # truth instead of a second list that drifts from the first. + - name: Read the ignored advisories + id: ignored + run: | + ids=$(sed -n '/ignore *= *\[/,/^]/p' .cargo/audit.toml \ + | grep -oE 'RUSTSEC-[0-9]{4}-[0-9]{4}' | paste -sd,) + echo "ids=$ids" >> "$GITHUB_OUTPUT" + echo "Ignoring: ${ids:-nothing}" + - uses: rustsec/audit-check@v2 with: token: ${{ secrets.GITHUB_TOKEN }} + ignore: ${{ steps.ignored.outputs.ids }} # Off-PR runs own a single tracking issue for the whole lockfile: one # issue, kept current, rather than a fresh one every Monday. @@ -57,6 +73,21 @@ jobs: gh label create automated-issue --force \ --color C5DEF5 --description "Filed by a workflow, not a human" + # Visible, not silent: an ignore list nobody ever reads is how a + # suppressed advisory outlives the reason it was suppressed. + - name: Report the ignored advisories + run: | + { + echo "### Ignored advisories" + echo + sed -n '/ignore *= *\[/,/^]/p' .cargo/audit.toml \ + | grep -oE 'RUSTSEC-[0-9]{4}-[0-9]{4}' | sed 's/^/- /' + echo + echo "Rationale and review cadence: docs/DEPENDENCY-AUTOMATION.md" + } >> "$GITHUB_STEP_SUMMARY" + + # Reads .cargo/audit.toml from the working directory, so the ids + # above are already excluded from these counts. - name: Run cargo audit id: audit run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 44ed707..96bfe77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,10 +20,44 @@ All notable changes to Diskern are documented here. The format follows dependency-update PR when an advisory lands - Community docs: contributing guide, code of conduct, security policy - Per-section READMEs and an architecture overview +- Quarantine keeps a manifest, so a quarantined file can be restored + after the app is closed. The desktop app gained a Quarantine panel with + per-file restore and an explicit purge +- The impact graph reaches the report: a dependency store a live project + depends on is now marked more cautiously than an abandoned one, with + "referenced by N projects" as the evidence ### Changed - Licensed under MIT +- Rule patterns are globs rather than substrings, so a rule stays inside + the directory it names. The Firefox rule covers `cache2` rather than + the whole profile, and `/tmp` no longer matches `~/tmp` +- Excludes are matched on whole path components after normalization, so + `/run` stops excluding `/runtime-data` and a differently-cased Windows + root still matches +- `min_file_size` moved from `ScanOptions` to `ReportOptions` as + `dedup_min_size`: it is a dedup knob and was hiding small files from + the whole report +- The reclaimable headline no longer counts the same bytes twice, and no + longer counts bytes on files the app refuses to act on +- `cargo audit` ignores seventeen unfixable transitive advisories from + Tauri's Linux GTK3 stack, listed with a reason each in + `.cargo/audit.toml` + +### Fixed + +- Restoring a quarantined file across filesystems no longer fails with + `EXDEV` +- Restore refuses when something is already at the original path, rather + than overwriting it — quarantine a cache, keep using the app, change + your mind, and the file the app rebuilt survives +- A file whose path can't be recorded in the manifest is not moved at + all, instead of being moved and then losing its record +- The scan progress ticker thread stops from a guard, so a panicking or + cancelled scan can't leave it emitting for the rest of the process +- Two files that flatten to the same quarantine name no longer overwrite + each other ## [0.1.0] diff --git a/Cargo.lock b/Cargo.lock index 9b202b1..62ce792 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -252,6 +252,16 @@ dependencies = [ "tinyvec", ] +[[package]] +name = "bstr" +version = "1.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6bb31b46c14244e20ee9984b11bf5c992b91fb6939fea616e3512c8baecdbe5f" +dependencies = [ + "memchr", + "serde_core", +] + [[package]] name = "bumpalo" version = "3.20.3" @@ -847,6 +857,7 @@ version = "0.1.0" dependencies = [ "blake3", "dashmap", + "globset", "jwalk", "petgraph", "rayon", @@ -1433,6 +1444,19 @@ version = "0.3.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e4eba85ea1d0a966a983acd07deee566e67395d2d96b6fb39e62b5a833f1eb0b" +[[package]] +name = "globset" +version = "0.4.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "07c34a9410465b45bd9787443bc7370f37735bad04b0f0cd57ff1a3186c98988" +dependencies = [ + "aho-corasick", + "bstr", + "log", + "regex-automata", + "regex-syntax", +] + [[package]] name = "gobject-sys" version = "0.18.0" diff --git a/Cargo.toml b/Cargo.toml index 091df29..bc770b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,9 @@ rayon = "1" petgraph = "0.8" dashmap = "6" +# Path matching for the rules database +globset = "0.4" + [profile.release] lto = "thin" strip = true diff --git a/app/src-tauri/src/commands.rs b/app/src-tauri/src/commands.rs index f6d6737..482e8d9 100644 --- a/app/src-tauri/src/commands.rs +++ b/app/src-tauri/src/commands.rs @@ -33,6 +33,75 @@ struct ScanProgressPayload { bytes_seen: u64, } +/// Everything a running scan owns outside itself: the ticker thread that +/// emits `scan-progress`, and this scan's entry in the shared +/// [`ActiveScan`] slot. Both are released on drop. +/// +/// They used to be released by statements after the `.await`, which only +/// run if control reaches them. A `?` between the two — there was one — +/// returned on a panicking blocking task and left the ticker emitting +/// every 150ms for the rest of the process, behind whatever the UI showed +/// next. Dropping the command's future, which is how a Tauri command is +/// cancelled, skipped the cleanup entirely. A guard cannot be skipped. +struct ScanRun<'a> { + stop: Arc, + ticker: Option>, + state: &'a ActiveScan, + progress: Arc, +} + +impl<'a> ScanRun<'a> { + fn start(window: &Window, state: &'a ActiveScan, progress: Arc) -> Self { + let stop = Arc::new(std::sync::atomic::AtomicBool::new(false)); + + // A plain OS thread keeps this independent of whatever async + // runtime Tauri is using internally. + let ticker = { + let progress = progress.clone(); + let stop = stop.clone(); + let window = window.clone(); + std::thread::spawn(move || { + while !stop.load(Ordering::Relaxed) { + let payload = ScanProgressPayload { + files_seen: progress.files_seen.load(Ordering::Relaxed), + bytes_seen: progress.bytes_seen.load(Ordering::Relaxed), + }; + let _ = window.emit("scan-progress", payload); + std::thread::sleep(Duration::from_millis(150)); + } + }) + }; + + Self { + stop, + ticker: Some(ticker), + state, + progress, + } + } +} + +impl Drop for ScanRun<'_> { + fn drop(&mut self) { + self.stop.store(true, Ordering::Relaxed); + if let Some(ticker) = self.ticker.take() { + let _ = ticker.join(); + } + + // Clear only if the slot still holds *this* scan. An unconditional + // `= None` would let a short scan that started second erase a + // longer one's handle when it finished first, leaving the survivor + // with a Cancel button wired to nothing. + let mut slot = self.state.slot(); + if slot + .as_ref() + .is_some_and(|current| Arc::ptr_eq(current, &self.progress)) + { + *slot = None; + } + } +} + /// Read-only scan. Safe to expose; touches nothing. /// /// Emits a `scan-progress` event roughly every 150ms while running, so the @@ -54,32 +123,12 @@ pub async fn start_scan( // here on purpose — holding it across the await below would make this // future non-Send. *state.slot() = Some(progress.clone()); - let stop = Arc::new(std::sync::atomic::AtomicBool::new(false)); - - // Ticker thread: emits progress snapshots on an interval until `stop` - // is set once the scan below finishes. A plain OS thread keeps this - // independent of whatever async runtime Tauri is using internally. - let ticker = { - let progress = progress.clone(); - let stop = stop.clone(); - let window = window.clone(); - std::thread::spawn(move || { - while !stop.load(Ordering::Relaxed) { - let payload = ScanProgressPayload { - files_seen: progress.files_seen.load(Ordering::Relaxed), - bytes_seen: progress.bytes_seen.load(Ordering::Relaxed), - }; - let _ = window.emit("scan-progress", payload); - std::thread::sleep(Duration::from_millis(150)); - } - }) - }; + + // Everything this scan has to undo, undone on the way out however the + // way out happens. + let run = ScanRun::start(&window, &state, progress.clone()); let progress_for_scan = progress.clone(); - // Note the missing `?`: the join result is unwrapped *after* the cleanup - // below. Returning early here would leave the ticker running and this - // scan still listed as in-flight, so cancelling would target a scan that - // had already ended. let joined = tauri::async_runtime::spawn_blocking(move || { let opts = scanner::ScanOptions { roots, @@ -102,21 +151,7 @@ pub async fn start_scan( }) .await; - stop.store(true, Ordering::Relaxed); - let _ = ticker.join(); - // Clear only if the slot still holds *this* scan. An unconditional - // `= None` would let a short scan that started second erase a longer - // one's handle when it finished first, leaving the survivor with a - // Cancel button wired to nothing. - { - let mut slot = state.slot(); - if slot - .as_ref() - .is_some_and(|current| Arc::ptr_eq(current, &progress)) - { - *slot = None; - } - } + drop(run); // One final snapshot so the UI's last-seen count matches the real total. let _ = window.emit( @@ -149,8 +184,14 @@ pub fn cancel_scan(state: State<'_, ActiveScan>) -> bool { } } -/// The ONLY mutating command. Re-classifies server-side before acting — -/// the frontend's claimed verdict is never trusted. +/// The mutating commands below all take `quarantine_dir` from the +/// frontend, which resolves it to `/Quarantine`. Nothing +/// here trusts a verdict the frontend claims — `quarantine_finding` +/// re-classifies, and the restore/purge commands only ever touch paths +/// the manifest in that directory says Diskern put there itself. +/// +/// Re-classifies server-side before acting — the frontend's claimed +/// verdict is never trusted. #[tauri::command] pub async fn quarantine_finding( path: PathBuf, @@ -169,3 +210,51 @@ pub async fn quarantine_finding( .await .map_err(|e| e.to_string())? } + +/// Everything still in quarantine, read from the manifest on disk. +/// +/// Read-only. This is what makes quarantine reversible across restarts: +/// before the manifest existed, the record of where a file came from lived +/// only in the value `quarantine_finding` returned, and the UI dropped it. +#[tauri::command] +pub async fn list_quarantine( + quarantine_dir: PathBuf, +) -> Result, String> { + tauri::async_runtime::spawn_blocking(move || { + actions::list(&quarantine_dir).map_err(|e| e.to_string()) + }) + .await + .map_err(|e| e.to_string())? +} + +/// Put one quarantined file back where it came from. +/// +/// Addressed by its path *in quarantine*, which is the unique one — the +/// same original can be quarantined, restored and quarantined again. The +/// manifest is consulted first, so a path the frontend invented reaches +/// no file. +#[tauri::command] +pub async fn restore_quarantined( + quarantine_dir: PathBuf, + quarantined_to: PathBuf, +) -> Result { + tauri::async_runtime::spawn_blocking(move || { + actions::restore_from_manifest(&quarantine_dir, &quarantined_to).map_err(|e| e.to_string()) + }) + .await + .map_err(|e| e.to_string())? +} + +/// Empty quarantine for good. +/// +/// The one command in Diskern that deletes anything, and it deletes only +/// the files the manifest lists — the ones this app moved there. The UI +/// confirms first; this is past the point of no return. +#[tauri::command] +pub async fn purge_quarantine(quarantine_dir: PathBuf) -> Result { + tauri::async_runtime::spawn_blocking(move || { + actions::purge(&quarantine_dir).map_err(|e| e.to_string()) + }) + .await + .map_err(|e| e.to_string())? +} diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 3867d69..dc719ee 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -19,6 +19,9 @@ pub fn run() { commands::start_scan, commands::cancel_scan, commands::quarantine_finding, + commands::list_quarantine, + commands::restore_quarantined, + commands::purge_quarantine, ]) .run(tauri::generate_context!()) .expect("error while running Diskern"); diff --git a/app/src/App.jsx b/app/src/App.jsx index 9f970e7..bf4522f 100644 --- a/app/src/App.jsx +++ b/app/src/App.jsx @@ -1,4 +1,4 @@ -import React, { useState, useMemo, useRef, useEffect } from "react"; +import React, { useState, useMemo, useRef, useEffect, useCallback } from "react"; import { invoke } from "@tauri-apps/api/core"; import { open } from "@tauri-apps/plugin-dialog"; import { listen } from "@tauri-apps/api/event"; @@ -92,7 +92,10 @@ function FindingRow({ f, quarantineDir, onQuarantined }) {
  • {f.entry.path} {(f.entry.size / 1e6).toFixed(1)} MB - {f.reasons[0]} + {/* Every reason, not just the matched rule: "referenced by 3 + projects" is what explains a risky row, and it is never the + first one. */} + {f.reasons.join(" · ")} {canQuarantine && ( @@ -199,6 +202,145 @@ function DuplicatesSection({ sets }) { ); } +/** + * What is sitting in quarantine right now, read back from the manifest on + * disk rather than from anything this session remembers. + * + * That distinction is the point. Quarantine is only "reversible" if the + * record of where a file came from outlives the window it was moved in — + * before the manifest existed, closing the app stranded every quarantined + * file with a flattened filename nobody could read an original path out + * of. So this renders whether or not a scan has been run. + */ +function QuarantineSection({ quarantineDir, refreshKey, onRestored }) { + const [records, setRecords] = useState([]); + const [isOpen, setIsOpen] = useState(false); + const [error, setError] = useState(null); + const [busyPath, setBusyPath] = useState(null); + const [purgePhase, setPurgePhase] = useState("idle"); // idle | confirming | working + const [purgeNotice, setPurgeNotice] = useState(null); + + const reload = useCallback(async () => { + if (!quarantineDir) return; + try { + setRecords(await invoke("list_quarantine", { quarantineDir })); + setError(null); + } catch (e) { + setError(String(e)); + } + }, [quarantineDir]); + + useEffect(() => { + reload(); + }, [reload, refreshKey]); + + async function restore(record) { + setError(null); + setBusyPath(record.quarantined_to); + try { + await invoke("restore_quarantined", { + quarantineDir, + quarantinedTo: record.quarantined_to, + }); + onRestored(record); + await reload(); + } catch (e) { + setError(String(e)); + } finally { + setBusyPath(null); + } + } + + async function purge() { + setError(null); + setPurgePhase("working"); + try { + const summary = await invoke("purge_quarantine", { quarantineDir }); + setPurgeNotice( + `Deleted ${summary.files_removed} file${summary.files_removed === 1 ? "" : "s"}` + + ` · ${(summary.bytes_removed / 1e6).toFixed(1)} MB freed` + + (summary.failed.length ? ` · ${summary.failed.length} could not be removed` : "") + ); + await reload(); + } catch (e) { + setError(String(e)); + } finally { + setPurgePhase("idle"); + } + } + + // Nothing quarantined and nothing to say about it: stay out of the way. + if (records.length === 0 && !error && !purgeNotice) return null; + + return ( +
    + + {isOpen && ( +
    +

    + Moved here, not deleted. Restore puts a file back where it came from. + Purge is the only thing in Diskern that deletes, and it deletes only + what is listed here. +

    +
      + {records.map((r) => ( +
    • + {r.original} + + {new Date(r.at_epoch * 1000).toLocaleString()} + + + {busyPath === r.quarantined_to ? ( + Restoring… + ) : ( + + )} + +
    • + ))} +
    + + {records.length > 0 && ( +
    + {purgePhase === "idle" && ( + + )} + {purgePhase === "confirming" && ( + + + Delete {records.length} file{records.length === 1 ? "" : "s"} for good? + This cannot be undone. + + + + + )} + {purgePhase === "working" && Deleting…} +
    + )} + + {purgeNotice &&

    {purgeNotice}

    } + {error &&

    {error}

    } +
    + )} +
    + ); +} + /** * Live "is this actually working" feedback while a scan runs. * @@ -243,6 +385,9 @@ export default function App() { const [quarantinedPaths, setQuarantinedPaths] = useState(() => new Set()); const [reclaimed, setReclaimed] = useState(0); const [quarantineDir, setQuarantineDir] = useState(null); + // Bumped whenever this session moves a file in, so the quarantine list + // re-reads the manifest rather than guessing at what changed. + const [quarantineVersion, setQuarantineVersion] = useState(0); const unlistenRef = useRef(null); // Resolve a sensible, always-writable quarantine location once on mount: @@ -279,6 +424,22 @@ export default function App() { return next; }); setReclaimed((prev) => prev + finding.reclaimable); + setQuarantineVersion((v) => v + 1); + } + + // A restored file is back on disk, so it belongs back in the report it + // came from — as a row again, and out of the reclaimed running total. + // A record with no matching finding (restored after a different scan, or + // after a restart) just isn't in this report; the list still refreshes. + function handleRestored(record) { + const finding = report?.findings.find((f) => f.entry.path === record.original); + setQuarantinedPaths((prev) => { + if (!prev.has(record.original)) return prev; + const next = new Set(prev); + next.delete(record.original); + return next; + }); + if (finding) setReclaimed((prev) => Math.max(0, prev - finding.reclaimable)); } // Cancelling races the scan finishing on its own; the command returns @@ -366,6 +527,13 @@ export default function App() { )} {error &&

    {error}

    } {notice &&

    {notice}

    } + {/* Rendered before any scan too: files quarantined in an earlier + session are restorable without scanning again. */} + )} @@ -392,6 +560,12 @@ export default function App() { {error &&

    {error}

    } {notice &&

    {notice}

    } + + 9} {reason}", ""); } } diff --git a/crates/diskern-core/Cargo.toml b/crates/diskern-core/Cargo.toml index 532702c..80d0779 100644 --- a/crates/diskern-core/Cargo.toml +++ b/crates/diskern-core/Cargo.toml @@ -22,6 +22,7 @@ jwalk.workspace = true rayon.workspace = true petgraph.workspace = true dashmap.workspace = true +globset.workspace = true [dev-dependencies] tempfile = "3" diff --git a/crates/diskern-core/README.md b/crates/diskern-core/README.md index a6f4a1f..f6ff8b9 100644 --- a/crates/diskern-core/README.md +++ b/crates/diskern-core/README.md @@ -26,11 +26,11 @@ scanner ──► index ──► dedup ──► graph ──► rules + risk | ---------------------------- | ------------------------------------------------------ | | [`scanner`](src/scanner.rs) | Parallel filesystem walk (jwalk) with live progress | | [`dedup`](src/dedup.rs) | Duplicate detection — BLAKE3, size-collision gated | -| [`graph`](src/graph.rs) | Reference graph (what depends on what) | +| [`graph`](src/graph.rs) | Reference graph — which projects depend on which stores | | [`rules`](src/rules.rs) | Deterministic rules DB ([`rules/base.json`](rules/base.json)) | | [`risk`](src/risk.rs) | Turns rule matches + evidence into a verdict | | [`report`](src/report.rs) | Aggregates findings into a serializable report | -| [`actions`](src/actions.rs) | Quarantine (move-to-review-folder) — the only mutator | +| [`actions`](src/actions.rs) | Quarantine + manifest (move, restore, purge) — the only mutator | | [`ai`](src/ai.rs) | Optional narration layer (`--features ai`) | ## Develop diff --git a/crates/diskern-core/rules/base.json b/crates/diskern-core/rules/base.json index d8e6878..d391a38 100644 --- a/crates/diskern-core/rules/base.json +++ b/crates/diskern-core/rules/base.json @@ -1,93 +1,93 @@ { - "version": 2, + "version": 3, "rules": [ { "id": "windows-driverstore", - "patterns": ["/windows/system32/driverstore"], + "patterns": ["**/windows/system32/driverstore/**"], "category": "system_critical", "verdict": "protected", "description": "Windows driver store. Removing files here can make hardware unusable." }, { "id": "windows-winsxs", - "patterns": ["/windows/winsxs"], + "patterns": ["**/windows/winsxs/**"], "category": "system_critical", "verdict": "protected", "description": "Windows component store. Managed by the OS; never modify directly." }, { "id": "windows-installer-cache", - "patterns": ["/windows/installer/", "/package cache/"], + "patterns": ["**/windows/installer/**", "**/package cache/**"], "category": "system_critical", "verdict": "protected", "description": "Windows Installer cache. Windows needs these to uninstall, repair, or patch installed software; removing them strands every product that registered here." }, { "id": "chrome-cache", - "patterns": ["/google/chrome/user data/default/cache", "/.cache/google-chrome"], + "patterns": ["**/google/chrome/user data/*/cache/**", "**/.cache/google-chrome/**"], "category": "browser_cache", "verdict": "safe", "description": "Chrome browser cache. Fully regenerated on next use; no effect on installed applications or saved data." }, { "id": "firefox-cache", - "patterns": ["/.cache/mozilla/firefox", "/mozilla/firefox/profiles"], + "patterns": ["**/.cache/mozilla/firefox/**", "**/mozilla/firefox/profiles/*/cache2/**", "**/library/caches/firefox/profiles/*/cache2/**"], "category": "browser_cache", "verdict": "safe", - "description": "Firefox cache. Regenerated automatically." + "description": "Firefox network cache (cache2). Regenerated automatically. The rest of the profile — bookmarks, history, logins, cookies — is deliberately not covered." }, { "id": "rust-target", - "patterns": ["/target/debug", "/target/release"], + "patterns": ["**/target/debug/**", "**/target/release/**"], "category": "build_artifact", "verdict": "review", "description": "Rust build artifacts. Regenerable via cargo build, but rebuilding large projects takes time." }, { "id": "node-modules", - "patterns": ["/node_modules/"], + "patterns": ["**/node_modules/**"], "category": "build_artifact", "verdict": "review", "description": "Node.js dependencies. Regenerable via npm/pnpm install if a lockfile exists." }, { "id": "pip-cache", - "patterns": ["/.cache/pip", "/appdata/local/pip/cache"], + "patterns": ["**/.cache/pip/**", "**/appdata/local/pip/cache/**"], "category": "package_manager_cache", "verdict": "safe", "description": "pip download cache. Packages are re-downloaded on demand." }, { "id": "unix-system-logs", - "patterns": ["/var/log/"], + "patterns": ["/var/log/**", "/private/var/log/**"], "category": "log", "verdict": "review", "description": "System logs. Reclaimable once you no longer need them, but they are the first thing anyone asks for when something breaks." }, { "id": "macos-user-logs", - "patterns": ["/library/logs/"], + "patterns": ["**/library/logs/**"], "category": "log", "verdict": "review", "description": "Application logs written by macOS apps. Regenerated as the apps run; only worth keeping while you are chasing a bug." }, { "id": "windows-crash-logs", - "patterns": ["/appdata/local/crashdumps/", "/appdata/local/microsoft/windows/wer/"], + "patterns": ["**/appdata/local/crashdumps/**", "**/appdata/local/microsoft/windows/wer/**"], "category": "log", "verdict": "review", "description": "Windows crash dumps and error reports. Large, and only useful while investigating the crash that produced them." }, { "id": "installer-packages", - "patterns": [".dmg", ".msi", ".pkg"], + "patterns": ["**/*.dmg", "**/*.msi", "**/*.pkg"], "category": "installer", "verdict": "review", "description": "Installer package. Reclaimable, but you will have to download it again to reinstall or roll back." }, { "id": "temp-dirs", - "patterns": ["/tmp/", "/appdata/local/temp"], + "patterns": ["/tmp/**", "**/var/tmp/**", "**/appdata/local/temp/**"], "category": "temp_file", "verdict": "review", "description": "Temporary files. Usually safe once no program is using them." diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index 9aa1224..5d7f949 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -3,10 +3,60 @@ //! Design rule: Diskern never hard-deletes. Files are moved to a //! quarantine directory with a manifest recording original locations, so //! every action is reversible until the user explicitly purges quarantine. +//! +//! The manifest is what makes "reversible" outlive the process. It is a +//! JSON Lines file in the quarantine directory: one [`QuarantineRecord`] +//! per line, appended as files arrive, rewritten when one leaves. The +//! quarantine filenames cannot stand in for it — flattening a path maps +//! `/` and `_` onto the same character, so `1700000000_home_user_.cache_x` +//! has no single original it could be read back to. +//! +//! [`purge`] is the one operation here that deletes, and it deletes only +//! what the manifest says this crate put there. use crate::{GenomeError, Result, Verdict}; use serde::{Deserialize, Serialize}; +use std::io::Write; use std::path::{Path, PathBuf}; +use std::sync::{Mutex, MutexGuard}; + +/// Quarantine's record of itself, inside the quarantine directory. +pub const MANIFEST_NAME: &str = "manifest.jsonl"; + +/// Longest flattened name we will build. Filenames are capped at 255 +/// bytes on every filesystem Diskern targets, and a deep path flattens +/// past that easily; the tail is kept because that is the half that +/// distinguishes two files. +const MAX_FLAT_LEN: usize = 180; + +/// Serializes every manifest access in this process. +/// +/// `restore_from_manifest` and `purge` are read-modify-write: they read +/// the whole manifest and later rewrite it from what they read. A record +/// appended in between was erased by that rewrite, leaving the file in +/// quarantine with nothing recording where it belongs — invisible to +/// `list`, unrestorable, and not even removed by a later `purge`. +/// +/// The app makes that reachable: `quarantine_finding`, `restore_quarantined` +/// and `purge_quarantine` each run on their own blocking task, and nothing +/// in the UI stops a user quarantining one row while another row's restore +/// is still in flight. +/// +/// One lock for the process rather than one per directory: these +/// operations are rare, short, and a second quarantine directory in one +/// process isn't a thing that happens. +static MANIFEST: Mutex<()> = Mutex::new(()); + +/// A poisoned lock means an earlier holder panicked mid-operation. The +/// manifest on disk is either the old one or the new one — `write_manifest` +/// renames a complete file into place — so there is no torn state to +/// protect, and refusing to unlock would break quarantine for the rest of +/// the session. +fn manifest_lock() -> MutexGuard<'static, ()> { + MANIFEST + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) +} #[derive(Debug, Clone, Serialize, Deserialize)] pub struct QuarantineRecord { @@ -34,31 +84,292 @@ pub fn quarantine( std::fs::create_dir_all(quarantine_dir).map_err(|e| io_err(quarantine_dir, e))?; - // Flatten path into a unique quarantine filename. let stamp = now_epoch(); - let flat = file.to_string_lossy().replace(['/', '\\', ':'], "_"); - let dest = quarantine_dir.join(format!("{stamp}_{flat}")); + let record = QuarantineRecord { + original: file.to_path_buf(), + quarantined_to: unique_dest(quarantine_dir, stamp, file), + at_epoch: stamp, + }; + + // Encode before moving anything. `serde` refuses a `PathBuf` that + // isn't valid UTF-8, and Linux and macOS both allow filenames that + // aren't — a Latin-1 name out of an old archive is enough. Moving + // first and discovering that afterwards left the file in quarantine + // with no record of where it came from, under a flattened name that + // cannot be read back: the exact loss this manifest exists to stop. + let line = encode(&record)?; - // rename() fails across filesystems; fall back to copy+remove. - if std::fs::rename(file, &dest).is_err() { - std::fs::copy(file, &dest).map_err(|e| io_err(file, e))?; - std::fs::remove_file(file).map_err(|e| io_err(file, e))?; + move_file(file, &record.quarantined_to)?; + + // The move happened; the record must follow it or the move must not + // stand. Anything else strands the file. + let _guard = manifest_lock(); + if let Err(e) = append_line(quarantine_dir, &line) { + // Best effort, and the only sensible order: the original was + // sitting here a moment ago, so putting it back is the outcome + // closest to nothing having happened. + let _ = move_file(&record.quarantined_to, &record.original); + return Err(e); } + Ok(record) +} - Ok(QuarantineRecord { - original: file.to_path_buf(), - quarantined_to: dest, - at_epoch: stamp, - }) +/// A quarantine filename that no existing file already owns. +/// +/// Flattening is lossy on purpose (it only has to be readable; the +/// manifest is the source of truth), which means two different originals +/// can flatten to the same name inside the same second. Without this the +/// second `rename` would silently overwrite the first — quarantine losing +/// a file is the one failure this module cannot have. +fn unique_dest(quarantine_dir: &Path, stamp: i64, file: &Path) -> PathBuf { + let mut flat = file.to_string_lossy().replace(['/', '\\', ':'], "_"); + if flat.len() > MAX_FLAT_LEN { + // Byte-slice on a char boundary; a split mid-codepoint would panic. + let cut = flat + .char_indices() + .map(|(i, _)| i) + .find(|&i| flat.len() - i <= MAX_FLAT_LEN) + .unwrap_or(0); + flat = flat.split_off(cut); + } + + let base = format!("{stamp}_{flat}"); + let mut candidate = quarantine_dir.join(&base); + let mut n = 1u32; + while candidate.exists() { + candidate = quarantine_dir.join(format!("{base}.{n}")); + n += 1; + } + candidate } /// Restore a quarantined file to its original location. +/// +/// Refuses when something is already there. Both halves of `move_file` +/// replace an existing destination without asking, and the files most +/// likely to be quarantined are the ones most likely to come back: a +/// browser cache is `safe` precisely because the browser rebuilds it, so +/// "quarantine the cache, keep browsing, change your mind" ends with the +/// undo destroying the newer file. Overwriting a file the user did not +/// name is exactly what this module promises never to do, so an occupied +/// destination is an error the caller shows rather than a decision this +/// function makes. +/// +/// Leaves the manifest alone — see [`restore_from_manifest`] for the +/// version that also stops listing the file as quarantined. pub fn restore(record: &QuarantineRecord) -> Result<()> { + // symlink_metadata, not exists(): a broken symlink is something in + // the way too, and exists() follows the link and reports false. + if std::fs::symlink_metadata(&record.original).is_ok() { + return Err(GenomeError::Rules(format!( + "{} already exists; move or remove it and restore again — \ + refusing to overwrite it with the quarantined copy", + record.original.display() + ))); + } + if let Some(parent) = record.original.parent() { std::fs::create_dir_all(parent).map_err(|e| io_err(parent, e))?; } - std::fs::rename(&record.quarantined_to, &record.original) - .map_err(|e| io_err(&record.quarantined_to, e)) + move_file(&record.quarantined_to, &record.original) +} + +/// Everything the manifest says is currently in quarantine, oldest first. +/// +/// A quarantine directory with no manifest is empty, not broken: the app +/// resolves the path before anything has been quarantined into it. +pub fn list(quarantine_dir: &Path) -> Result> { + let _guard = manifest_lock(); + read_manifest(quarantine_dir) +} + +/// [`list`] without taking the lock — for callers that already hold it. +/// `Mutex` is not reentrant, so they must not go through `list`. +fn read_manifest(quarantine_dir: &Path) -> Result> { + let path = manifest_path(quarantine_dir); + let contents = match std::fs::read_to_string(&path) { + Ok(c) => c, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(vec![]), + Err(e) => return Err(io_err(&path, e)), + }; + + Ok(contents + .lines() + .filter(|line| !line.trim().is_empty()) + .filter_map(|line| match serde_json::from_str::(line) { + Ok(record) => Some(record), + // One torn line — a half-written append after a power cut — + // must not hide every other file from the restore list. + Err(e) => { + tracing::warn!(manifest = %path.display(), "skipping unreadable manifest line: {e}"); + None + } + }) + .collect()) +} + +/// Restore the file quarantined *to* `quarantined_to` and stop listing it. +/// +/// Addressed by quarantine path rather than by original path: the +/// quarantine path is the unique one. The same original can be quarantined +/// again after being restored, and both attempts are real records. +pub fn restore_from_manifest( + quarantine_dir: &Path, + quarantined_to: &Path, +) -> Result { + // Held across the read, the move and the rewrite: anything appended + // between the read and the rewrite would be erased by it. + let _guard = manifest_lock(); + let records = read_manifest(quarantine_dir)?; + let record = records + .iter() + .find(|r| r.quarantined_to == quarantined_to) + .cloned() + .ok_or_else(|| { + GenomeError::Rules(format!( + "{} is not listed in the quarantine manifest", + quarantined_to.display() + )) + })?; + + restore(&record)?; + + // Rewrite only after the move succeeded. Dropping the line first would + // strand the file in quarantine with nothing recording where it came + // from — the failure this manifest exists to prevent. + let remaining: Vec = records + .into_iter() + .filter(|r| r.quarantined_to != quarantined_to) + .collect(); + write_manifest(quarantine_dir, &remaining)?; + Ok(record) +} + +/// What a [`purge`] actually did. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct PurgeSummary { + pub files_removed: usize, + pub bytes_removed: u64, + /// Files the manifest listed that could not be removed, with the + /// reason. Reported rather than raised: one locked file shouldn't + /// abort the purge of everything else. + pub failed: Vec, +} + +/// Empty quarantine for good. This is the only deletion in Diskern, and +/// it deletes only the files the manifest says this crate moved here — +/// never whatever else happens to be sitting in the directory. +pub fn purge(quarantine_dir: &Path) -> Result { + let _guard = manifest_lock(); + let records = read_manifest(quarantine_dir)?; + let mut summary = PurgeSummary::default(); + let mut kept = Vec::new(); + + for record in records { + let size = std::fs::metadata(&record.quarantined_to) + .map(|m| m.len()) + .unwrap_or(0); + match std::fs::remove_file(&record.quarantined_to) { + Ok(()) => { + summary.files_removed += 1; + summary.bytes_removed += size; + } + // Already gone: the manifest was stale, and the goal (not + // there any more) is met. Anything else stays listed so the + // user can still restore it. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + summary.files_removed += 1; + } + Err(e) => { + summary + .failed + .push(format!("{}: {e}", record.quarantined_to.display())); + kept.push(record); + } + } + } + + write_manifest(quarantine_dir, &kept)?; + Ok(summary) +} + +/// Where the manifest lives for a given quarantine directory. +pub fn manifest_path(quarantine_dir: &Path) -> PathBuf { + quarantine_dir.join(MANIFEST_NAME) +} + +/// One manifest line. Fails on a path that is not valid UTF-8, which is +/// why every caller encodes before it moves anything. +fn encode(record: &QuarantineRecord) -> Result { + serde_json::to_string(record).map_err(|e| { + GenomeError::Rules(format!( + "cannot record {} in the quarantine manifest, so it will not be moved: {e}", + record.original.display() + )) + }) +} + +fn append_line(quarantine_dir: &Path, line: &str) -> Result<()> { + let path = manifest_path(quarantine_dir); + let mut file = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(&path) + .map_err(|e| io_err(&path, e))?; + writeln!(file, "{line}").map_err(|e| io_err(&path, e)) +} + +/// Rewrite the whole manifest. Via a temporary file in the same directory +/// so a crash mid-write leaves the old manifest intact rather than a +/// truncated one — the manifest is the only copy of where these files +/// belong. +fn write_manifest(quarantine_dir: &Path, records: &[QuarantineRecord]) -> Result<()> { + let path = manifest_path(quarantine_dir); + let tmp = path.with_extension("jsonl.tmp"); + + // The directory need not exist yet: `purge` on a quarantine nothing + // has been moved into reads an empty list and lands here, and failing + // that with "No such file or directory" reads like a disk fault + // rather than the no-op it is. + std::fs::create_dir_all(quarantine_dir).map_err(|e| io_err(quarantine_dir, e))?; + + let mut body = String::new(); + for record in records { + body.push_str(&encode(record)?); + body.push('\n'); + } + + std::fs::write(&tmp, body).map_err(|e| io_err(&tmp, e))?; + std::fs::rename(&tmp, &path).map_err(|e| io_err(&path, e)) +} + +/// Move a file, in the one direction this crate is allowed to move things. +/// +/// `rename` is the fast path and fails with `EXDEV` when the two paths are +/// on different filesystems — which is the normal case here, not an edge +/// one: quarantine lives in the app's local data directory, and the files +/// being quarantined come from wherever the user pointed the scan, which +/// may well be a second drive or a `/home` on its own partition. +/// +/// Both directions go through this. The quarantine path used to handle the +/// fallback and the restore path didn't, so the exact case the move +/// survived was the case the undo failed on. +fn move_file(from: &Path, to: &Path) -> Result<()> { + if std::fs::rename(from, to).is_ok() { + return Ok(()); + } + copy_then_remove(from, to) +} + +/// The fallback half of [`move_file`], separated so it can be tested +/// without two filesystems to hand. +/// +/// The remove comes last on purpose: if it fails, the file still exists in +/// both places, which is recoverable. Removing first and failing to copy +/// would not be. +fn copy_then_remove(from: &Path, to: &Path) -> Result<()> { + std::fs::copy(from, to).map_err(|e| io_err(from, e))?; + std::fs::remove_file(from).map_err(|e| io_err(from, e)) } fn io_err(path: &Path, source: std::io::Error) -> GenomeError { @@ -94,6 +405,39 @@ mod tests { assert!(f.exists()); } + /// Issue #42. `quarantine` handled the cross-filesystem case and + /// `restore` didn't, so quarantining a file from another mount worked + /// and undoing it returned EXDEV. Provoking a real EXDEV needs two + /// mounts, so this exercises the fallback both directions now share. + #[test] + fn the_cross_filesystem_fallback_moves_the_bytes() { + let dir = tempfile::tempdir().unwrap(); + let from = dir.path().join("from.txt"); + let to = dir.path().join("to.txt"); + std::fs::write(&from, b"data").unwrap(); + + copy_then_remove(&from, &to).unwrap(); + + assert!(!from.exists()); + assert_eq!(std::fs::read(&to).unwrap(), b"data"); + } + + #[test] + fn restore_recreates_a_directory_that_was_removed_meanwhile() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let nested = dir.path().join("a/b"); + std::fs::create_dir_all(&nested).unwrap(); + let f = nested.join("victim.txt"); + std::fs::write(&f, b"data").unwrap(); + + let rec = quarantine(&f, Verdict::Safe, &q).unwrap(); + std::fs::remove_dir_all(dir.path().join("a")).unwrap(); + + restore(&rec).unwrap(); + assert_eq!(std::fs::read(&f).unwrap(), b"data"); + } + #[test] fn refuses_protected() { let dir = tempfile::tempdir().unwrap(); @@ -102,4 +446,244 @@ mod tests { assert!(quarantine(&f, Verdict::Protected, dir.path()).is_err()); assert!(f.exists()); // untouched } + + /// Issue #43. The module promised every action stays reversible until + /// the user purges, and the only record of where a file came from was + /// the value `quarantine` returned — which the app dropped on the + /// floor. Reversibility has to survive the process exiting. + #[test] + fn a_record_outlives_the_process_that_made_it() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let f = dir.path().join("victim.txt"); + std::fs::write(&f, b"data").unwrap(); + + // Return value deliberately ignored, the way the app used to. + let _ = quarantine(&f, Verdict::Safe, &q).unwrap(); + + // Nothing carried over in memory: read it back off the disk. + let listed = list(&q).unwrap(); + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].original, f); + + restore_from_manifest(&q, &listed[0].quarantined_to).unwrap(); + assert_eq!(std::fs::read(&f).unwrap(), b"data"); + assert!(list(&q).unwrap().is_empty()); + } + + /// Flattening maps `/` and `_` onto the same character, so two + /// different originals can produce the same name inside one second. + /// Before `unique_dest` the second rename overwrote the first. + #[test] + fn two_files_that_flatten_alike_do_not_overwrite_each_other() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let a = dir.path().join("a_b"); + let b_dir = dir.path().join("a"); + std::fs::create_dir(&b_dir).unwrap(); + let b = b_dir.join("b"); + std::fs::write(&a, b"first").unwrap(); + std::fs::write(&b, b"second").unwrap(); + + let ra = quarantine(&a, Verdict::Safe, &q).unwrap(); + let rb = quarantine(&b, Verdict::Safe, &q).unwrap(); + + assert_ne!(ra.quarantined_to, rb.quarantined_to); + assert_eq!(std::fs::read(&ra.quarantined_to).unwrap(), b"first"); + assert_eq!(std::fs::read(&rb.quarantined_to).unwrap(), b"second"); + assert_eq!(list(&q).unwrap().len(), 2); + } + + #[test] + fn an_empty_quarantine_lists_nothing_rather_than_failing() { + let dir = tempfile::tempdir().unwrap(); + assert!(list(&dir.path().join("never-used")).unwrap().is_empty()); + } + + /// A half-written line after a power cut must not hide the records + /// either side of it. + #[test] + fn a_torn_manifest_line_is_skipped_not_fatal() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let f = dir.path().join("victim.txt"); + std::fs::write(&f, b"data").unwrap(); + quarantine(&f, Verdict::Safe, &q).unwrap(); + + let manifest = manifest_path(&q); + let good = std::fs::read_to_string(&manifest).unwrap(); + // A line cut off mid-value, the way an interrupted append leaves it. + std::fs::write( + &manifest, + format!("{{\"original\": \"/home/u/half-writ\n{good}"), + ) + .unwrap(); + + assert_eq!(list(&q).unwrap().len(), 1); + } + + #[test] + fn purge_removes_the_files_it_listed_and_nothing_else() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let f = dir.path().join("victim.txt"); + std::fs::write(&f, b"data").unwrap(); + let rec = quarantine(&f, Verdict::Safe, &q).unwrap(); + + // Something Diskern did not put here. + let stranger = q.join("not-ours.txt"); + std::fs::write(&stranger, b"leave me alone").unwrap(); + + let summary = purge(&q).unwrap(); + assert_eq!(summary.files_removed, 1); + assert_eq!(summary.bytes_removed, 4); + assert!(summary.failed.is_empty()); + + assert!(!rec.quarantined_to.exists()); + assert!(stranger.exists()); + assert!(list(&q).unwrap().is_empty()); + } + + #[test] + fn restoring_something_not_in_the_manifest_is_refused() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + std::fs::create_dir_all(&q).unwrap(); + assert!(restore_from_manifest(&q, &q.join("invented")).is_err()); + } + + /// A path serde cannot encode must stop the move, not follow it. + /// + /// Linux and macOS both allow filenames that are not valid UTF-8, and + /// a disk scanner meets them. Encoding after the move left the file in + /// quarantine, absent from the manifest, under a flattened name with + /// no way back — while the caller was told the operation failed. + #[cfg(unix)] + #[test] + fn a_path_that_cannot_be_recorded_is_not_moved() { + use std::os::unix::ffi::OsStrExt; + + 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. + let victim = dir + .path() + .join(std::ffi::OsStr::from_bytes(b"photo-\xff\xfe.dmg")); + std::fs::write(&victim, b"payload").unwrap(); + + let err = quarantine(&victim, Verdict::Review, &q).unwrap_err(); + assert!( + err.to_string().contains("will not be moved"), + "unexpected error: {err}" + ); + + // The file is exactly where it was, and quarantine is untouched. + assert_eq!(std::fs::read(&victim).unwrap(), b"payload"); + assert!(list(&q).unwrap().is_empty()); + let strays = std::fs::read_dir(&q).map(|d| d.count()).unwrap_or(0); + assert_eq!(strays, 0, "quarantine should hold no orphan"); + } + + /// Restoring must not destroy a file that came back on its own. + /// + /// The caches Diskern calls `safe` are the ones applications rebuild, + /// so this is the ordinary sequence, not a contrived one: quarantine + /// the cache, carry on using the app, then change your mind. + #[test] + fn restore_refuses_to_overwrite_something_already_there() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let original = dir.path().join("cache.dat"); + std::fs::write(&original, b"old-copy").unwrap(); + + let rec = quarantine(&original, Verdict::Safe, &q).unwrap(); + std::fs::write(&original, b"regenerated-by-the-app").unwrap(); + + let err = restore(&rec).unwrap_err(); + assert!(err.to_string().contains("already exists"), "{err}"); + + // Neither copy was destroyed: the new one is still in place and + // the quarantined one is still restorable once it is moved aside. + assert_eq!(std::fs::read(&original).unwrap(), b"regenerated-by-the-app"); + assert_eq!(std::fs::read(&rec.quarantined_to).unwrap(), b"old-copy"); + assert_eq!(list(&q).unwrap().len(), 1); + + std::fs::remove_file(&original).unwrap(); + restore(&rec).unwrap(); + assert_eq!(std::fs::read(&original).unwrap(), b"old-copy"); + } + + /// Nothing may end up in the quarantine directory without a manifest + /// line naming it. + /// + /// `restore_from_manifest` and `purge` rewrite the manifest from what + /// they read, so a record appended in between used to be erased — + /// stranding a file that `list` could not see, `restore` could not + /// reach and `purge` would not remove. Two threads quarantining while + /// a third restores reproduces it without the lock. + #[test] + fn concurrent_quarantines_and_restores_strand_no_file() { + let dir = tempfile::tempdir().unwrap(); + let q = dir.path().join("quarantine"); + let victims: Vec = (0..40) + .map(|i| { + let f = dir.path().join(format!("victim-{i}.dat")); + std::fs::write(&f, format!("{i}")).unwrap(); + f + }) + .collect(); + + std::thread::scope(|scope| { + for chunk in victims.chunks(20) { + let q = &q; + scope.spawn(move || { + for victim in chunk { + quarantine(victim, Verdict::Safe, q).unwrap(); + } + }); + } + // Restores racing the appends, on whatever is listed so far. + scope.spawn(|| { + for _ in 0..40 { + if let Some(record) = list(&q).unwrap().first() { + // Losing the race with another restore is fine; + // stranding a file is not. + let _ = restore_from_manifest(&q, &record.quarantined_to); + } + } + }); + }); + + let listed: std::collections::HashSet = list(&q) + .unwrap() + .into_iter() + .map(|r| r.quarantined_to) + .collect(); + let on_disk: Vec = std::fs::read_dir(&q) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.path()) + .filter(|p| p.file_name().is_some_and(|n| n != MANIFEST_NAME)) + .collect(); + + for path in &on_disk { + assert!( + listed.contains(path), + "{} is in quarantine with no manifest record", + path.display() + ); + } + assert_eq!(listed.len(), on_disk.len()); + } + + #[test] + fn purging_a_quarantine_nothing_was_moved_into_is_a_no_op() { + let dir = tempfile::tempdir().unwrap(); + let summary = purge(&dir.path().join("never-used")).unwrap(); + assert_eq!(summary.files_removed, 0); + assert_eq!(summary.bytes_removed, 0); + assert!(summary.failed.is_empty()); + } } diff --git a/crates/diskern-core/src/dedup.rs b/crates/diskern-core/src/dedup.rs index 38124a8..384f672 100644 --- a/crates/diskern-core/src/dedup.rs +++ b/crates/diskern-core/src/dedup.rs @@ -37,10 +37,42 @@ pub fn find_duplicates_cancellable( entries: &mut [FileEntry], cancelled: &AtomicBool, ) -> Option> { + find_duplicates_filtered(entries, |_, _| true, cancelled) +} + +/// [`find_duplicates_cancellable`], restricted to the entries `eligible` +/// accepts. +/// +/// Duplicate sets are an *offer*: "you are storing this three times, keep +/// one". An entry the user is never allowed to act on doesn't belong in +/// that offer, and hashing it is work spent to produce a number nobody can +/// use. `eligible` is where the caller says which entries those are; it is +/// called exactly once per entry, before any hashing. +/// +/// The index comes along because the interesting callers have already +/// worked something out per entry — `report` classifies first — and +/// looking that answer up by position beats re-deriving it or keeping a +/// set of paths the size of the disk. +pub fn find_duplicates_filtered( + entries: &mut [FileEntry], + eligible: F, + cancelled: &AtomicBool, +) -> Option> +where + F: Fn(usize, &FileEntry) -> bool, +{ + let keep: Vec = entries + .iter() + .enumerate() + .map(|(i, e)| eligible(i, e)) + .collect(); + // Stage 1: bucket by size. let mut by_size: HashMap> = HashMap::new(); for (i, e) in entries.iter().enumerate() { - by_size.entry(e.size).or_default().push(i); + if keep[i] { + by_size.entry(e.size).or_default().push(i); + } } let candidates: Vec = by_size .into_values() @@ -68,11 +100,14 @@ pub fn find_duplicates_cancellable( entries[i].hash = h; } - // Stage 3: bucket by hash. + // Stage 3: bucket by hash. Only stage-1 survivors carry one, but the + // eligibility check is repeated here so a caller reusing entries that + // already hold hashes from an earlier run can't smuggle them back in. let mut by_hash: HashMap<&str, Vec<&FileEntry>> = HashMap::new(); - for e in entries.iter() { - if let Some(h) = &e.hash { - by_hash.entry(h).or_default().push(e); + for (i, e) in entries.iter().enumerate() { + match &e.hash { + Some(h) if keep[i] => by_hash.entry(h).or_default().push(e), + _ => continue, } } @@ -126,6 +161,43 @@ mod tests { assert_eq!(sets[0].wasted, 10); } + #[test] + fn ineligible_entries_form_no_sets() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("a"), b"same-bytes").unwrap(); + std::fs::write(dir.path().join("b"), b"same-bytes").unwrap(); + std::fs::write(dir.path().join("keep-a"), b"other-byte").unwrap(); + std::fs::write(dir.path().join("keep-b"), b"other-byte").unwrap(); + + let opts = ScanOptions { + roots: vec![dir.path().to_path_buf()], + ..Default::default() + }; + let mut entries = scan(&opts, Arc::new(ScanProgress::default())).unwrap(); + let never = AtomicBool::new(false); + let sets = find_duplicates_filtered( + &mut entries, + |_, e| { + e.path + .file_name() + .is_some_and(|n| n.to_string_lossy().starts_with("keep")) + }, + &never, + ) + .unwrap(); + + assert_eq!(sets.len(), 1); + assert!(sets[0].paths.iter().all(|p| p + .file_name() + .is_some_and(|n| n.to_string_lossy().starts_with("keep")))); + // Excluded entries are never hashed, so they cost nothing either. + for entry in &entries { + if entry.path.file_name().is_some_and(|n| n == "a") { + assert!(entry.hash.is_none()); + } + } + } + #[test] fn stops_when_cancelled() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/diskern-core/src/graph.rs b/crates/diskern-core/src/graph.rs index 306a1e6..6178d45 100644 --- a/crates/diskern-core/src/graph.rs +++ b/crates/diskern-core/src/graph.rs @@ -8,10 +8,27 @@ //! link them to the dependency stores they reference. Everything else //! (dynamic linking, registry, plists) comes later. +use crate::FileEntry; use petgraph::graph::{DiGraph, NodeIndex}; use serde::{Deserialize, Serialize}; -use std::collections::HashMap; -use std::path::PathBuf; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicBool, Ordering}; + +/// A file whose presence makes the directory holding it a project root, +/// and the stores a project of that kind owns. +/// +/// Root-level stores only, in v0. `__pycache__` and nested `node_modules` +/// exist at every depth and belong to whatever encloses them, which is a +/// containment question rather than a reference one. +const PROJECTS: &[(&str, ProjectKind, &[&str])] = &[ + ("cargo.toml", ProjectKind::Cargo, &["target"]), + ("package.json", ProjectKind::Npm, &["node_modules"]), + ("pyproject.toml", ProjectKind::Python, &[".venv", "venv"]), +]; + +/// Directory names that are dependency stores wherever they appear. +const STORE_NAMES: &[&str] = &["target", "node_modules", ".venv", "venv"]; #[derive(Debug, Clone, Serialize, Deserialize)] pub enum Node { @@ -20,7 +37,7 @@ pub enum Node { DependencyStore(PathBuf), // e.g. a node_modules dir, ~/.cargo/registry } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub enum ProjectKind { Cargo, Npm, @@ -45,6 +62,99 @@ pub struct ImpactGraph { } impl ImpactGraph { + /// Build the graph from one pass over the scanned entries. + /// + /// Two things come out of that pass: which directories are project + /// roots (they hold a marker file), and which are dependency stores + /// (an entry lives under one). A `References` edge is added for each + /// project to the store it owns. + /// + /// A project whose own store wasn't scanned is linked to the nearest + /// enclosing project's store of the same kind instead. That is not a + /// guess: npm workspaces hoist dependencies to the repository root and + /// Cargo workspaces share one `target/`, so the members really do + /// reference it — and it is where the "referenced by 3 projects" + /// number comes from rather than always being 1. + pub fn from_entries(entries: &[FileEntry]) -> Self { + static NEVER: AtomicBool = AtomicBool::new(false); + Self::from_entries_cancellable(entries, &NEVER) + .expect("a run that cannot be cancelled cannot stop early") + } + + /// [`from_entries`], abandoned as soon as `cancelled` is set. + /// + /// This walks every entry's ancestors, which is a second or so per + /// million entries — long enough that a Cancel arriving during it + /// would otherwise do nothing until the stage after it started. The + /// walk and the hashing pass both stop promptly; this has to as well, + /// or it just moves the dead spot. + pub fn from_entries_cancellable(entries: &[FileEntry], cancelled: &AtomicBool) -> Option { + // A set of kinds per root, not one: a directory holding both a + // `Cargo.toml` and a `package.json` is both projects, and it is a + // shape that turns up constantly — any Rust binary with a web + // front end, this repository's own `app/` among them. Keeping one + // kind meant whichever marker the walk happened to yield last + // won, so one of `target/` and `node_modules/` came back + // unreferenced, and *which* one changed with directory order. + // + // Ordered maps, so the graph a given scan produces is the same + // graph every time. Verdicts are meant to be deterministic, and a + // reference count feeds straight into one. + let mut roots: BTreeMap> = BTreeMap::new(); + let mut stores: HashSet = HashSet::new(); + + for entry in entries { + if cancelled.load(Ordering::Relaxed) { + return None; + } + let store = enclosing_store(&entry.path); + if let Some(store) = &store { + stores.insert(store.clone()); + } + + // A marker inside a dependency store marks nothing: every npm + // package ships a package.json, and there are tens of + // thousands of them under one node_modules. + if store.is_some() { + continue; + } + let Some(kind) = marker_kind(&entry.path) else { + continue; + }; + if let Some(dir) = entry.path.parent() { + roots.entry(dir.to_path_buf()).or_default().insert(kind); + } + } + + let mut graph = Self::default(); + for (root, kinds) in &roots { + if cancelled.load(Ordering::Relaxed) { + return None; + } + for kind in kinds { + let owned = stores_of(root, *kind, &stores); + let targets = if owned.is_empty() { + shared_store(root, *kind, &roots, &stores) + } else { + owned + }; + + for store in targets { + // Keyed by path, so a root that is two kinds is still + // one node with one edge per store — and one project + // for anything counting references. + let from = graph.node(Node::ProjectRoot { + path: root.clone(), + kind: *kind, + }); + let to = graph.node(Node::DependencyStore(store)); + graph.graph.add_edge(from, to, Edge::References); + } + } + } + Some(graph) + } + pub fn node(&mut self, node: Node) -> NodeIndex { let key = match &node { Node::File(p) | Node::DependencyStore(p) => p.clone(), @@ -60,13 +170,261 @@ impl ImpactGraph { /// How many project roots reference this path (directly, v0)? /// This number feeds risk::downgrade — the "breaks 17 projects" number. + /// + /// Answers for the nearest enclosing node as well as for the path + /// itself, because findings are files and the thing projects reference + /// is the store above them: nothing points at + /// `proj/node_modules/react/index.js`, but three projects may well + /// point at the `proj/node_modules` it sits in. pub fn referencing_projects(&self, path: &std::path::Path) -> usize { - let Some(&ix) = self.index.get(path) else { - return 0; - }; - self.graph - .neighbors_directed(ix, petgraph::Direction::Incoming) - .filter(|&n| matches!(self.graph[n], Node::ProjectRoot { .. })) - .count() + for ancestor in path.ancestors() { + let Some(&ix) = self.index.get(ancestor) else { + continue; + }; + return self + .graph + .neighbors_directed(ix, petgraph::Direction::Incoming) + .filter(|&n| matches!(self.graph[n], Node::ProjectRoot { .. })) + .count(); + } + 0 + } +} + +/// The outermost dependency store this path sits inside, if any. +/// +/// Outermost, not nearest: `proj/node_modules/a/node_modules/b` belongs to +/// `proj/node_modules`, which is the store a project actually references. +fn enclosing_store(path: &Path) -> Option { + let mut found = None; + let mut current = path.parent(); + while let Some(dir) = current { + if dir + .file_name() + .and_then(|n| n.to_str()) + .is_some_and(|n| STORE_NAMES.contains(&n)) + { + found = Some(dir.to_path_buf()); + } + current = dir.parent(); + } + found +} + +/// Which kind of project a file marks, if it marks one. +fn marker_kind(path: &Path) -> Option { + let name = path.file_name()?.to_str()?.to_ascii_lowercase(); + PROJECTS + .iter() + .find(|(marker, _, _)| *marker == name) + .map(|(_, kind, _)| *kind) +} + +/// The stores this project owns that the scan actually saw. +fn stores_of(root: &Path, kind: ProjectKind, seen: &HashSet) -> Vec { + PROJECTS + .iter() + .filter(|(_, k, _)| *k == kind) + .flat_map(|(_, _, names)| names.iter()) + .map(|name| root.join(name)) + .filter(|store| seen.contains(store)) + .collect() +} + +/// The store an enclosing project of the same kind owns — what a workspace +/// member uses when the dependencies were hoisted above it. +fn shared_store( + root: &Path, + kind: ProjectKind, + roots: &BTreeMap>, + seen: &HashSet, +) -> Vec { + for ancestor in root.ancestors().skip(1) { + if !roots + .get(ancestor) + .is_some_and(|kinds| kinds.contains(&kind)) + { + continue; + } + let stores = stores_of(ancestor, kind, seen); + if !stores.is_empty() { + return stores; + } + } + vec![] +} + +#[cfg(test)] +mod tests { + use super::*; + + fn entries(paths: &[&str]) -> Vec { + paths + .iter() + .map(|p| FileEntry { + path: PathBuf::from(p), + size: 1, + modified: None, + accessed: None, + is_symlink: false, + hash: None, + }) + .collect() + } + + #[test] + fn a_project_references_the_store_beside_it() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/home/u/proj/package.json", + "/home/u/proj/src/index.js", + "/home/u/proj/node_modules/react/index.js", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/home/u/proj/node_modules/react/index.js")), + 1 + ); + // The project's own source is referenced by nothing. + assert_eq!( + graph.referencing_projects(Path::new("/home/u/proj/src/index.js")), + 0 + ); + } + + /// An abandoned store — no project alongside it — is the case this + /// whole module exists to tell apart from the one above. + #[test] + fn an_unreferenced_store_is_referenced_by_nobody() { + let graph = + ImpactGraph::from_entries(&entries(&["/home/u/old-thing/node_modules/react/index.js"])); + + assert_eq!( + graph.referencing_projects(Path::new("/home/u/old-thing/node_modules/react/index.js")), + 0 + ); + } + + /// Every npm package ships a package.json. Treating those as project + /// roots would make one node_modules look like ten thousand projects. + #[test] + fn a_package_json_inside_node_modules_is_not_a_project() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/home/u/proj/package.json", + "/home/u/proj/node_modules/react/package.json", + "/home/u/proj/node_modules/react/node_modules/loose/package.json", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/home/u/proj/node_modules/react/package.json")), + 1 + ); + } + + /// The README's headline evidence. npm workspaces hoist dependencies + /// to the repository root, so all three members really do reference + /// the one store. + #[test] + fn hoisted_workspace_members_all_reference_the_shared_store() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/repo/package.json", + "/repo/packages/a/package.json", + "/repo/packages/b/package.json", + "/repo/node_modules/react/index.js", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/repo/node_modules/react/index.js")), + 3 + ); + } + + /// A member with its own store uses that one, not the root's. + #[test] + fn a_member_with_its_own_store_does_not_borrow_the_roots() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/repo/package.json", + "/repo/packages/a/package.json", + "/repo/packages/a/node_modules/x/index.js", + "/repo/node_modules/react/index.js", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/repo/packages/a/node_modules/x/index.js")), + 1 + ); + assert_eq!( + graph.referencing_projects(Path::new("/repo/node_modules/react/index.js")), + 1 + ); + } + + #[test] + fn cargo_workspace_members_share_one_target() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/repo/Cargo.toml", + "/repo/crates/core/Cargo.toml", + "/repo/crates/cli/Cargo.toml", + "/repo/target/debug/app", + ])); + + assert_eq!( + graph.referencing_projects(Path::new("/repo/target/debug/app")), + 3 + ); + } + + /// A directory can be more than one kind of project, and both of its + /// stores are then referenced. Keeping a single kind per root meant + /// whichever marker the walk yielded last won, so one store came back + /// unreferenced — and which one depended on directory order, which + /// makes the verdict non-deterministic. Both orders are asserted. + #[test] + fn a_root_that_is_two_projects_references_both_of_its_stores() { + let both = [ + "/proj/Cargo.toml", + "/proj/package.json", + "/proj/target/debug/app", + "/proj/node_modules/react/index.js", + ]; + let reversed: Vec<&str> = both.iter().copied().rev().collect(); + + for order in [both.to_vec(), reversed] { + let graph = ImpactGraph::from_entries(&entries(&order)); + assert_eq!( + graph.referencing_projects(Path::new("/proj/target/debug/app")), + 1, + "target/ in order {order:?}" + ); + assert_eq!( + graph.referencing_projects(Path::new("/proj/node_modules/react/index.js")), + 1, + "node_modules/ in order {order:?}" + ); + } + } + + /// A root that is two kinds is still one project, so a store it shares + /// with siblings must not be counted twice for it. + #[test] + fn a_two_kind_root_counts_once_against_a_shared_store() { + let graph = ImpactGraph::from_entries(&entries(&[ + "/repo/Cargo.toml", + "/repo/package.json", + "/repo/node_modules/react/index.js", + ])); + assert_eq!( + graph.referencing_projects(Path::new("/repo/node_modules/react/index.js")), + 1 + ); + } + + #[test] + fn a_cancelled_build_stops_instead_of_finishing() { + let cancelled = AtomicBool::new(true); + assert!(ImpactGraph::from_entries_cancellable( + &entries(&["/repo/package.json", "/repo/node_modules/react/index.js"]), + &cancelled, + ) + .is_none()); } } diff --git a/crates/diskern-core/src/report.rs b/crates/diskern-core/src/report.rs index 6ca3099..1275eef 100644 --- a/crates/diskern-core/src/report.rs +++ b/crates/diskern-core/src/report.rs @@ -1,10 +1,31 @@ //! Assembles scanner + dedup + rules + risk into Findings — the single //! structure both the CLI and the Tauri UI render. -use crate::{dedup, risk, rules::RulesDb, Category, FileEntry, Finding, Verdict}; +use crate::{dedup, graph, risk, rules::RulesDb, Category, FileEntry, Finding, Verdict}; use serde::{Deserialize, Serialize}; +use std::collections::HashSet; use std::sync::atomic::{AtomicBool, Ordering}; +/// Knobs for [`build_with`] that are about the report, not the walk. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ReportOptions { + /// Files strictly smaller than this take no part in duplicate + /// detection. This lived on `ScanOptions` as `min_file_size` and was + /// applied inside the walk, which meant raising it to speed dedup up + /// also hid every small file from the rules engine, the risk model, + /// the report and `files_scanned` — none of which the name promised. + pub dedup_min_size: u64, +} + +impl Default for ReportOptions { + fn default() -> Self { + // 1, not 0: a zero-byte file is identical to every other zero-byte + // file, so a default of 0 makes one enormous duplicate set worth + // nothing. They still reach the report as findings. + Self { dedup_min_size: 1 } + } +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Report { pub findings: Vec, @@ -23,8 +44,18 @@ pub fn build(entries: Vec, rules: &RulesDb) -> Report { /// stopped early, so there is no report to show — not an error, just the /// user's answer arriving before ours. pub fn build_cancellable( + entries: Vec, + rules: &RulesDb, + cancelled: &AtomicBool, +) -> Option { + build_with(entries, rules, &ReportOptions::default(), cancelled) +} + +/// [`build_cancellable`] with the report knobs spelled out. +pub fn build_with( mut entries: Vec, rules: &RulesDb, + opts: &ReportOptions, cancelled: &AtomicBool, ) -> Option { let now = std::time::SystemTime::now() @@ -32,45 +63,84 @@ pub fn build_cancellable( .map(|d| d.as_secs() as i64) .unwrap_or(0); - let duplicate_sets = dedup::find_duplicates_cancellable(&mut entries, cancelled)?; + // The graph stage. Project roots and the dependency stores they point + // at, worked out from the same entries the rest of the pipeline sees, + // so a `node_modules` three live projects depend on can be told apart + // from an abandoned one. + let impact = graph::ImpactGraph::from_entries_cancellable(&entries, cancelled)?; + + // Classify before dedup, not after. A file nothing will act on has no + // business in a duplicate set — the set is an offer to keep one copy + // and drop the rest, and dropping a driver store copy is not on offer + // — and hashing it is time spent producing a number nobody can use. + // + // Classification is cheap per entry, but a home directory is millions + // of them, so it happens once and the answer is kept. + let mut verdicts: Vec = Vec::with_capacity(entries.len()); + for entry in &entries { + if cancelled.load(Ordering::Relaxed) { + return None; + } + let (category, base, rule) = rules.classify(&entry.path); + // Evidence can only make a verdict more cautious, never less. + let referenced_by = impact.referencing_projects(&entry.path); + let verdict = risk::downgrade(base, referenced_by); + verdicts.push(Classified { + category, + verdict, + rule, + referenced_by, + }); + } + + let duplicate_sets = dedup::find_duplicates_filtered( + &mut entries, + |i, e| e.size >= opts.dedup_min_size && is_actionable(verdicts[i].verdict), + cancelled, + )?; let files_scanned = entries.len() as u64; let mut findings = Vec::new(); - for entry in entries { - // Classification is cheap per entry, but a home directory is - // millions of them — cheap times millions is still a wait. + for (entry, class) in entries.into_iter().zip(verdicts) { if cancelled.load(Ordering::Relaxed) { return None; } - let (category, verdict, rule) = rules.classify(&entry.path); // Unknown + unremarkable files aren't findings; don't drown the user. - if category == Category::Unknown { + if class.category == Category::Unknown { continue; } - let assessment = risk::assess(&entry, verdict, now); - let mut reasons: Vec = rule + let assessment = risk::assess(&entry, class.verdict, now); + let mut reasons: Vec = class + .rule .map(|r| vec![format!("matched rule {}: {}", r.id, r.description)]) .unwrap_or_default(); + if class.referenced_by > 0 { + reasons.push(format!( + "referenced by {} project{}", + class.referenced_by, + if class.referenced_by == 1 { "" } else { "s" } + )); + } reasons.extend(assessment.reasons); findings.push(Finding { - reclaimable: if verdict == Verdict::Protected { - 0 - } else { + // Bytes nothing will ever offer to move are not reclaimable. + reclaimable: if is_actionable(class.verdict) { entry.size + } else { + 0 }, entry, - category, - verdict, + category: class.category, + verdict: class.verdict, risk_score: assessment.score, reasons, }); } - let total_reclaimable = findings.iter().map(|f| f.reclaimable).sum::() - + duplicate_sets.iter().map(|d| d.wasted).sum::(); + let total_reclaimable = total_reclaimable(&findings, &duplicate_sets); findings.sort_by_key(|f| std::cmp::Reverse(f.reclaimable)); @@ -81,3 +151,380 @@ pub fn build_cancellable( files_scanned, }) } + +/// Whether anything will ever offer to move this file. +/// +/// `actions::quarantine` refuses Protected and Risky, and the UI renders +/// no action for either. One definition, used in both places it matters: +/// what counts towards the reclaimable headline, and what takes part in +/// dedup. Splitting them let Risky bytes back into the total through the +/// duplicate half after they had been taken out of the findings half. +fn is_actionable(verdict: Verdict) -> bool { + match verdict { + Verdict::Safe | Verdict::Review => true, + Verdict::Risky | Verdict::Protected => false, + } +} + +/// What the pipeline worked out about one entry before findings are built. +struct Classified<'a> { + category: Category, + /// After [`risk::downgrade`], not the rule's base verdict. + verdict: Verdict, + rule: Option<&'a crate::rules::Rule>, + referenced_by: usize, +} + +/// The two halves of the report overlap, so they can't just be added. +/// +/// A finding offers the file's own bytes. A duplicate set offers the +/// copies beyond the first. A file is often both: two identical 1 GB +/// installers under `/tmp` are two findings at 1 GB each *and* a duplicate +/// set with 1 GB wasted, and summing those said 3 GB when 2 GB is +/// everything there is. +/// +/// So findings are counted in full, and a duplicate set adds only the +/// redundant copies nobody has counted yet. +fn total_reclaimable(findings: &[Finding], duplicate_sets: &[dedup::DuplicateSet]) -> u64 { + let counted: HashSet<&std::path::Path> = findings + .iter() + .filter(|f| f.reclaimable > 0) + .map(|f| f.entry.path.as_path()) + .collect(); + + let from_findings: u64 = findings.iter().map(|f| f.reclaimable).sum(); + let from_duplicates: u64 = duplicate_sets + .iter() + .map(|set| { + // One copy always stays; that is what makes the rest redundant. + let redundant = set.paths.len().saturating_sub(1); + let already = set + .paths + .iter() + .filter(|p| counted.contains(p.as_path())) + .count(); + set.size * redundant.saturating_sub(already) as u64 + }) + .sum(); + + from_findings + from_duplicates +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::scanner::{scan, ScanOptions, ScanProgress}; + use std::sync::Arc; + + fn temp_rules() -> RulesDb { + RulesDb::new( + 1, + vec![ + crate::rules::Rule { + id: "test-protected".into(), + patterns: vec!["**/dk-sys/**".into()], + category: Category::SystemCritical, + verdict: Verdict::Protected, + description: "test".into(), + }, + crate::rules::Rule { + id: "test-temp".into(), + patterns: vec!["**/dk-scratch/**".into()], + category: Category::TempFile, + verdict: Verdict::Review, + description: "test".into(), + }, + ], + ) + } + + fn scan_dir(dir: &std::path::Path) -> Vec { + let opts = ScanOptions { + roots: vec![dir.to_path_buf()], + ..Default::default() + }; + scan(&opts, Arc::new(ScanProgress::default())).unwrap() + } + + /// Issue #47. `min_file_size` was applied inside the walk, so a file + /// under it never reached the rules engine, the risk model, the report + /// or `files_scanned` — the default of 1 quietly dropped every + /// zero-byte file. The knob is about dedup and now only affects dedup. + #[test] + fn the_dedup_minimum_does_not_hide_files_from_the_report() { + let dir = tempfile::tempdir().unwrap(); + let tmp = dir.path().join("dk-scratch"); + std::fs::create_dir(&tmp).unwrap(); + std::fs::write(tmp.join("empty.log"), b"").unwrap(); + std::fs::write(tmp.join("small-a"), b"tiny").unwrap(); + std::fs::write(tmp.join("small-b"), b"tiny").unwrap(); + + let rules = temp_rules(); + let entries = scan_dir(dir.path()); + let never = AtomicBool::new(false); + + // Every file is scanned and classified, whatever the dedup floor. + let big_floor = ReportOptions { + dedup_min_size: 1_000_000, + }; + let report = build_with(entries.clone(), &rules, &big_floor, &never).unwrap(); + assert_eq!(report.files_scanned, 3); + assert_eq!(report.findings.len(), 3); + // ...but nothing is small enough to be staged for dedup. + assert!(report.duplicate_sets.is_empty()); + + // With the default floor the two identical files do pair up. + let report = build_with(entries, &rules, &ReportOptions::default(), &never).unwrap(); + assert_eq!(report.files_scanned, 3); + assert_eq!(report.duplicate_sets.len(), 1); + } + + /// Issue #44. Two identical files under `/tmp` are two findings *and* + /// one duplicate set. Adding both halves counted the same bytes twice: + /// 4 + 4 + 4 = 12 where 8 is everything on the disk. + #[test] + fn duplicated_findings_are_not_counted_twice() { + let dir = tempfile::tempdir().unwrap(); + let tmp = dir.path().join("dk-scratch"); + std::fs::create_dir(&tmp).unwrap(); + std::fs::write(tmp.join("a.iso"), b"same").unwrap(); + std::fs::write(tmp.join("b.iso"), b"same").unwrap(); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &never, + ) + .unwrap(); + + assert_eq!(report.findings.len(), 2); + assert_eq!(report.duplicate_sets.len(), 1); + assert_eq!(report.duplicate_sets[0].wasted, 4); + // Both copies are already offered as findings, so the duplicate + // set adds nothing on top of them. + assert_eq!(report.total_reclaimable, 8); + } + + /// A duplicate whose copies are not findings still contributes — the + /// fix must not swing the other way and undercount. + #[test] + fn duplicates_outside_the_findings_still_count() { + let dir = tempfile::tempdir().unwrap(); + let docs = dir.path().join("docs"); + std::fs::create_dir(&docs).unwrap(); + std::fs::write(docs.join("a.txt"), b"same").unwrap(); + std::fs::write(docs.join("b.txt"), b"same").unwrap(); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &never, + ) + .unwrap(); + + assert!(report.findings.is_empty()); // unknown category, dropped + assert_eq!(report.duplicate_sets.len(), 1); + assert_eq!(report.total_reclaimable, 4); // one redundant copy + } + + /// Half in, half out: one copy is an actionable finding, the other is + /// unclassified user data. Acting on everything frees one copy's worth. + #[test] + fn a_duplicate_shared_with_a_finding_counts_once() { + let dir = tempfile::tempdir().unwrap(); + let tmp = dir.path().join("dk-scratch"); + let docs = dir.path().join("docs"); + std::fs::create_dir(&tmp).unwrap(); + std::fs::create_dir(&docs).unwrap(); + std::fs::write(tmp.join("a.iso"), b"same").unwrap(); + std::fs::write(docs.join("keep.iso"), b"same").unwrap(); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &never, + ) + .unwrap(); + + assert_eq!(report.findings.len(), 1); + assert_eq!(report.duplicate_sets.len(), 1); + assert_eq!(report.total_reclaimable, 4); + } + + /// The other half of #44: `find_duplicates` ran over every entry, + /// including ones nothing will act on, so their bytes contributed + /// `wasted` to a total the user can never do anything with. + #[test] + fn protected_files_form_no_duplicate_sets() { + let dir = tempfile::tempdir().unwrap(); + let sys = dir.path().join("dk-sys"); + std::fs::create_dir(&sys).unwrap(); + std::fs::write(sys.join("a.dll"), b"same").unwrap(); + std::fs::write(sys.join("b.dll"), b"same").unwrap(); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &never, + ) + .unwrap(); + + assert_eq!(report.findings.len(), 2); + assert!(report + .findings + .iter() + .all(|f| f.verdict == Verdict::Protected)); + assert!(report.duplicate_sets.is_empty()); + assert_eq!(report.total_reclaimable, 0); + } + + /// Issue #48. The graph stage was in the pipeline diagram and not in + /// the pipeline: `risk::downgrade` had no callers anywhere, so a + /// `node_modules` three live projects depend on got the same verdict + /// and the same reasons as an abandoned one. + #[test] + fn a_referenced_store_is_more_cautious_than_an_abandoned_one() { + let dir = tempfile::tempdir().unwrap(); + let live = dir.path().join("live"); + let dead = dir.path().join("dead"); + std::fs::create_dir_all(live.join("node_modules/react")).unwrap(); + std::fs::create_dir_all(dead.join("node_modules/react")).unwrap(); + std::fs::write(live.join("package.json"), b"{}").unwrap(); + std::fs::write(live.join("node_modules/react/index.js"), b"live").unwrap(); + std::fs::write(dead.join("node_modules/react/index.js"), b"dead").unwrap(); + + let rules = RulesDb::new( + 1, + vec![crate::rules::Rule { + id: "test-node-modules".into(), + patterns: vec!["**/node_modules/**".into()], + category: Category::BuildArtifact, + verdict: Verdict::Review, + description: "test".into(), + }], + ); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &rules, + &ReportOptions::default(), + &never, + ) + .unwrap(); + + let find = |needle: &str| { + report + .findings + .iter() + .find(|f| f.entry.path.to_string_lossy().contains(needle)) + .unwrap() + .clone() + }; + + let referenced = find("live/node_modules"); + assert_eq!(referenced.verdict, Verdict::Risky); + assert!(referenced + .reasons + .iter() + .any(|r| r == "referenced by 1 project")); + // Nothing offers to move a risky file, so its bytes are not on + // offer either. + assert_eq!(referenced.reclaimable, 0); + + let abandoned = find("dead/node_modules"); + assert_eq!(abandoned.verdict, Verdict::Review); + assert!(!abandoned + .reasons + .iter() + .any(|r| r.starts_with("referenced by"))); + assert_eq!(abandoned.reclaimable, 4); + } + + /// The graph stage is a full pass over the entries in front of every + /// other cancellation check, so it has to answer a cancel itself — + /// otherwise it just moves the dead spot the last round removed. + #[test] + fn a_cancel_during_the_graph_stage_stops_the_report() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("a.txt"), b"x").unwrap(); + + let cancelled = AtomicBool::new(true); + assert!(build_with( + scan_dir(dir.path()), + &temp_rules(), + &ReportOptions::default(), + &cancelled, + ) + .is_none()); + } + + /// Risky bytes were taken out of the findings half of the headline and + /// then walked back in through the duplicate half: `total_reclaimable` + /// treats a zero-reclaimable finding as "nobody counted this yet", so + /// a duplicate set of Risky copies added its full `wasted`. + #[test] + fn risky_duplicates_do_not_return_to_the_headline() { + let dir = tempfile::tempdir().unwrap(); + for project in ["live1", "live2"] { + let root = dir.path().join(project); + std::fs::create_dir_all(root.join("node_modules/react")).unwrap(); + std::fs::write(root.join("package.json"), b"{}").unwrap(); + std::fs::write(root.join("node_modules/react/index.js"), b"identical").unwrap(); + } + + let rules = RulesDb::new( + 1, + vec![crate::rules::Rule { + id: "test-node-modules".into(), + patterns: vec!["**/node_modules/**".into()], + category: Category::BuildArtifact, + verdict: Verdict::Review, + description: "test".into(), + }], + ); + + let never = AtomicBool::new(false); + let report = build_with( + scan_dir(dir.path()), + &rules, + &ReportOptions::default(), + &never, + ) + .unwrap(); + + let risky: Vec<&Finding> = report + .findings + .iter() + .filter(|f| f.verdict == Verdict::Risky) + .collect(); + assert_eq!(risky.len(), 2, "both node_modules copies are referenced"); + assert!(risky.iter().all(|f| f.reclaimable == 0)); + + // Nothing the app refuses to act on may reach a duplicate set... + for set in &report.duplicate_sets { + for path in &set.paths { + assert!( + !path.to_string_lossy().contains("node_modules"), + "{} is risky and should not be offered as a duplicate", + path.display() + ); + } + } + // ...so the headline is the one duplicate pair the user really + // can act on — the two identical `package.json` files, which are + // unclassified and so not findings, but are still two copies of + // the same two bytes. Before the fix the risky pair added its own + // 9 bytes on top, promising space the app refuses to free. + assert_eq!(report.duplicate_sets.len(), 1); + assert_eq!(report.total_reclaimable, 2); + } +} diff --git a/crates/diskern-core/src/rules.rs b/crates/diskern-core/src/rules.rs index 7f2ccce..2cc81dc 100644 --- a/crates/diskern-core/src/rules.rs +++ b/crates/diskern-core/src/rules.rs @@ -5,15 +5,23 @@ //! patterns and yields a Category + base Verdict. The risk module may //! DOWNGRADE a verdict (Safe -> Review) based on evidence; it may never //! upgrade one (Risky -> Safe). Protected is final. +//! +//! Patterns are globs, matched against the normalized path (lowercased, +//! `\` rewritten to `/`) with `*` stopping at a path separator. That +//! anchoring is the point: a plain substring test made `/tmp/` fire on +//! `/home/user/tmp/tax-return.pdf`, which is user data, not scratch space. use crate::{Category, Verdict}; +use globset::{Glob, GlobBuilder, GlobSet, GlobSetBuilder}; use serde::{Deserialize, Serialize}; +use std::sync::OnceLock; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Rule { pub id: String, - /// Substring / suffix patterns matched against the normalized path. - /// TODO: replace with proper glob matching (globset crate). + /// Globs matched against the normalized path. `*` matches within one + /// path component, `**` spans components, so `**/node_modules/**` + /// reaches any depth while `/tmp/**` stays at the filesystem root. pub patterns: Vec, pub category: Category, pub verdict: Verdict, @@ -21,10 +29,32 @@ pub struct Rule { pub description: String, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Serialize, Deserialize)] pub struct RulesDb { pub version: u32, pub rules: Vec, + /// One compiled matcher per rule, in rule order. Built on the first + /// `classify` and reused after: a home directory is millions of + /// entries, and compiling a glob per entry would cost more than the + /// walk that found them. + /// + /// Skipped by serde and rebuilt on demand, so a db that arrives over + /// the wire behaves exactly like one built here. + #[serde(skip)] + matchers: OnceLock>, +} + +/// Hand-written because `OnceLock` isn't `Clone`. A clone starts +/// with an empty cache rather than sharing one — the rules are the value, +/// the compiled form is an optimization. +impl Clone for RulesDb { + fn clone(&self) -> Self { + Self { + version: self.version, + rules: self.rules.clone(), + matchers: OnceLock::new(), + } + } } impl RulesDb { @@ -35,17 +65,70 @@ impl RulesDb { .expect("embedded rules db must parse") } + /// Build a db from rules held in memory (tests, future remote rule + /// updates). Same matching path as [`RulesDb::embedded`]. + pub fn new(version: u32, rules: Vec) -> Self { + Self { + version, + rules, + matchers: OnceLock::new(), + } + } + /// First matching rule wins; order in the db is priority order. /// Protected rules are listed first for exactly that reason. pub fn classify(&self, path: &std::path::Path) -> (Category, Verdict, Option<&Rule>) { - let p = path.to_string_lossy().replace('\\', "/").to_lowercase(); - for rule in &self.rules { - if rule.patterns.iter().any(|pat| p.contains(pat.as_str())) { + let p = normalize(path); + let candidate = std::path::Path::new(&p); + for (rule, matcher) in self.rules.iter().zip(self.matchers()) { + if matcher.is_match(candidate) { return (rule.category, rule.verdict, Some(rule)); } } (Category::Unknown, Verdict::Review, None) } + + fn matchers(&self) -> &[GlobSet] { + self.matchers + .get_or_init(|| self.rules.iter().map(compile).collect()) + } +} + +/// Lowercase, `/`-separated. Windows hands us `C:\Users\...`; the rules +/// are written once, in one shape, and the path is bent to fit them. +pub(crate) fn normalize(path: &std::path::Path) -> String { + path.to_string_lossy().replace('\\', "/").to_lowercase() +} + +/// `literal_separator` is the whole reason this module compiles globs at +/// all: without it `*` swallows `/`, and `**/target/*/**` would reach back +/// across directories the rule never named. +fn build_glob(pattern: &str) -> std::result::Result { + GlobBuilder::new(pattern).literal_separator(true).build() +} + +/// A pattern that doesn't compile is dropped, not fatal. The alternative — +/// panicking inside `classify` — would take the whole scan down over one +/// bad line in a rules file that may not even be ours. `patterns_compile` +/// below keeps the shipped db honest. +fn compile(rule: &Rule) -> GlobSet { + let mut builder = GlobSetBuilder::new(); + for pattern in &rule.patterns { + match build_glob(pattern) { + Ok(glob) => { + builder.add(glob); + } + Err(e) => tracing::warn!( + rule = %rule.id, + pattern = %pattern, + "ignoring rule pattern that is not a valid glob: {e}" + ), + } + } + builder.build().unwrap_or_else(|e| { + tracing::warn!(rule = %rule.id, "rule matches nothing: {e}"); + GlobSet::empty() + }) } #[cfg(test)] @@ -123,6 +206,98 @@ mod tests { } } + /// Every shipped pattern has to compile, because `compile` drops the + /// ones that don't. A protected rule that silently matches nothing is + /// the worst failure this module has. + #[test] + fn every_shipped_pattern_is_a_valid_glob() { + for rule in &RulesDb::embedded().rules { + for pattern in &rule.patterns { + assert!( + build_glob(pattern).is_ok(), + "{}: {pattern} is not a valid glob", + rule.id + ); + } + } + } + + /// Issue #41. Under substring matching every one of these matched a + /// rule written for somewhere else on the disk, and `review` is an + /// actionable verdict — the app offered to move them. + #[test] + fn rules_do_not_reach_outside_the_paths_they_name() { + let db = RulesDb::embedded(); + for path in [ + "/home/user/tmp/tax-return.pdf", // not /tmp + "/home/user/var/log/notes.txt", // not /var/log + "/home/user/Downloads/holiday.dmgx", // not a .dmg + "/home/user/mytmp/scratch.bin", + ] { + let (cat, _, rule) = db.classify(std::path::Path::new(path)); + assert_eq!(cat, Category::Unknown, "{path} matched {rule:?}"); + } + } + + /// The same rules still have to fire where they were meant to. + #[test] + fn rules_still_reach_the_paths_they_do_name() { + let db = RulesDb::embedded(); + for (path, expected) in [ + ("/tmp/build-9a2f/out.o", Category::TempFile), + ("/var/tmp/systemd-private/x", Category::TempFile), + ( + "C:\\Users\\x\\AppData\\Local\\Temp\\a.tmp", + Category::TempFile, + ), + ( + "/home/u/proj/node_modules/react/index.js", + Category::BuildArtifact, + ), + ("/home/u/proj/target/debug/app", Category::BuildArtifact), + ( + "/home/u/.cache/pip/wheels/a.whl", + Category::PackageManagerCache, + ), + ] { + let (cat, _, _) = db.classify(std::path::Path::new(path)); + assert_eq!(cat, expected, "{path}"); + } + } + + /// Issue #40. `/mozilla/firefox/profiles` covered the whole profile + /// directory, so bookmarks, history, logins and cookies were listed + /// under "Safe to remove" and `quarantine_finding` accepted them. + /// Only cache2 is cache; everything else in the profile is user data. + #[test] + fn firefox_profile_data_is_not_safe_to_remove() { + let db = RulesDb::embedded(); + for path in [ + "C:\\Users\\x\\AppData\\Roaming\\Mozilla\\Firefox\\Profiles\\ab12.default\\logins.json", + "C:\\Users\\x\\AppData\\Roaming\\Mozilla\\Firefox\\Profiles\\ab12.default\\places.sqlite", + "C:\\Users\\x\\AppData\\Roaming\\Mozilla\\Firefox\\Profiles\\ab12.default\\cookies.sqlite", + "/home/u/.mozilla/firefox/ab12.default/key4.db", + ] { + let (_, verdict, rule) = db.classify(std::path::Path::new(path)); + assert_ne!(verdict, Verdict::Safe, "{path} matched {rule:?}"); + } + } + + /// The cache the rule is actually named after still classifies. + #[test] + fn firefox_cache_is_still_safe_to_remove() { + let db = RulesDb::embedded(); + for path in [ + "/home/u/.cache/mozilla/firefox/ab12.default/cache2/entries/A1B2", + "C:\\Users\\x\\AppData\\Local\\Mozilla\\Firefox\\Profiles\\ab12.default\\cache2\\entries\\A1B2", + "/Users/x/Library/Caches/Firefox/Profiles/ab12.default/cache2/entries/A1B2", + ] { + let (cat, verdict, _) = db.classify(std::path::Path::new(path)); + assert_eq!(cat, Category::BrowserCache, "{path}"); + assert_eq!(verdict, Verdict::Safe, "{path}"); + } + } + /// An installed application's own repair binary is not a reclaimable /// download. `unknown` is the right answer: report::build drops those, /// so it never reaches the user as an actionable row. diff --git a/crates/diskern-core/src/scanner.rs b/crates/diskern-core/src/scanner.rs index 3c18867..570429d 100644 --- a/crates/diskern-core/src/scanner.rs +++ b/crates/diskern-core/src/scanner.rs @@ -13,10 +13,12 @@ use std::sync::Arc; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ScanOptions { pub roots: Vec, - /// Glob-style excludes (e.g. "/proc", "C:\\Windows\\WinSxS"). + /// Directories never walked, given as paths rather than globs + /// (e.g. "/proc", "C:\\Windows\\WinSxS"). Matched on whole path + /// components after the same normalization the rules database uses, + /// so case and separator style don't have to line up with the root. pub excludes: Vec, pub follow_symlinks: bool, // default false — symlink loops are real - pub min_file_size: u64, // skip tiny files for dedup purposes } impl Default for ScanOptions { @@ -25,7 +27,6 @@ impl Default for ScanOptions { roots: vec![], excludes: default_excludes(), follow_symlinks: false, - min_file_size: 1, } } } @@ -78,7 +79,9 @@ fn walk_root( progress: &ScanProgress, out: &mut Vec, ) -> Result<()> { - let excludes = opts.excludes.clone(); + // 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(); let walker = jwalk::WalkDir::new(root) .follow_links(opts.follow_symlinks) @@ -102,9 +105,6 @@ fn walk_root( } let Ok(meta) = entry.metadata() else { continue }; let size = meta.len(); - if size < opts.min_file_size { - continue; - } progress.files_seen.fetch_add(1, Ordering::Relaxed); progress.bytes_seen.fetch_add(size, Ordering::Relaxed); @@ -121,9 +121,68 @@ fn walk_root( Ok(()) } +/// Lowercased, `/`-separated, no trailing separator. An exclude written +/// `C:\\Windows\\WinSxS` has to match a root the user typed as +/// `c:\\windows\\winsxs`. +/// +/// ASCII case folding, not Unicode: these are directory names like +/// `Windows` and `System`, and it lets [`is_excluded`] fold the path a +/// character at a time instead of building a lowercased copy of it. +fn normalize_exclude(exclude: &str) -> String { + let normalized = exclude.replace('\\', "/").to_ascii_lowercase(); + let trimmed = normalized.trim_end_matches('/'); + // "/" itself trims to empty; keep it as the root rather than a prefix + // that matches every path. + if trimmed.is_empty() { + normalized + } else { + trimmed.to_string() + } +} + +/// True when `path` *is* an excluded directory or sits inside one. +/// +/// Compared on whole path components. A raw `starts_with` on the string +/// made `/run` exclude `/runtime-data` as well, because "/run" is a prefix +/// of "/runtime-data" in characters but not in directories. +/// +/// This runs for every child of every directory the walk opens, so it +/// normalizes nothing: it folds the path as it compares and stops at the +/// first character that differs — which, for a path not under an exclude, +/// is almost always the first or second. Building a normalized copy per +/// entry cost two allocations each and measured 2.4x slower than the +/// plain `starts_with` it replaced. fn is_excluded(path: &Path, excludes: &[String]) -> bool { - let p = path.to_string_lossy(); - excludes.iter().any(|ex| p.starts_with(ex.as_str())) + if excludes.is_empty() { + return false; + } + let path = path.to_string_lossy(); + excludes.iter().any(|ex| is_within(&path, ex)) +} + +/// Is `path`, folded as it goes, the already-normalized `exclude` itself +/// or something inside it? +fn is_within(path: &str, exclude: &str) -> bool { + let mut chars = path.chars(); + for expected in exclude.chars() { + match chars.next() { + Some(c) if fold(c) == expected => {} + _ => return false, + } + } + // Ends exactly there, or carries on at a component boundary — which a + // trailing separator on the path itself also satisfies. + match chars.next() { + None => true, + Some(sep) => sep == '/' || sep == '\\', + } +} + +fn fold(c: char) -> char { + match c { + '\\' => '/', + c => c.to_ascii_lowercase(), + } } fn to_epoch(t: std::time::SystemTime) -> Option { @@ -136,6 +195,57 @@ fn to_epoch(t: std::time::SystemTime) -> Option { mod tests { use super::*; + #[test] + fn excludes_match_whole_components_not_characters() { + let excludes = ["/run".to_string()]; + assert!(is_excluded(Path::new("/run"), &excludes)); + assert!(is_excluded(Path::new("/run/user/1000/x"), &excludes)); + // The bug: a character-prefix compare skipped this too. + assert!(!is_excluded(Path::new("/runtime-data/x"), &excludes)); + assert!(!is_excluded(Path::new("/runner"), &excludes)); + } + + #[test] + fn excludes_survive_a_differently_cased_or_separated_root() { + let excludes = [normalize_exclude("C:\\Windows\\WinSxS")]; + assert!(is_excluded( + Path::new("c:/windows/winsxs/component/x.dll"), + &excludes + )); + assert!(is_excluded( + Path::new("C:\\Windows\\WinSxS\\x.dll"), + &excludes + )); + assert!(!is_excluded( + Path::new("c:/windows/winsxs-backup/x.dll"), + &excludes + )); + } + + #[test] + fn nothing_is_excluded_when_there_are_no_excludes() { + assert!(!is_excluded(Path::new("/proc/1/maps"), &[])); + } + + #[test] + fn a_trailing_separator_on_the_path_still_matches() { + let excludes = [normalize_exclude("/run")]; + assert!(is_excluded(Path::new("/run/"), &excludes)); + } + + #[test] + fn an_exclude_longer_than_the_path_does_not_match() { + let excludes = [normalize_exclude("/proc/self/fd")]; + assert!(!is_excluded(Path::new("/proc"), &excludes)); + } + + #[test] + fn a_trailing_separator_on_an_exclude_changes_nothing() { + let excludes = [normalize_exclude("/proc/")]; + assert!(is_excluded(Path::new("/proc/1/maps"), &excludes)); + assert!(!is_excluded(Path::new("/process-data/x"), &excludes)); + } + #[test] fn scans_a_temp_tree() { let dir = tempfile::tempdir().unwrap(); diff --git a/docs/DEPENDENCY-AUTOMATION.md b/docs/DEPENDENCY-AUTOMATION.md index f54ed6a..03260b1 100644 --- a/docs/DEPENDENCY-AUTOMATION.md +++ b/docs/DEPENDENCY-AUTOMATION.md @@ -35,6 +35,53 @@ The job then fails, which is what auto-fix watches for. Both labels are created by the workflow if the repository doesn't have them yet, so this needs no manual setup. +## The ignore list + +Some advisories cannot be fixed here. They arrive through `tauri`, and +clearing them needs a release Tauri has not made yet. Those are listed, +with a reason each, in [`.cargo/audit.toml`](../.cargo/audit.toml). + +The whole list today is one dependency: Tauri v2 renders on Linux through +webkit2gtk-4.1, which links the **gtk-rs GTK3 bindings**. Those bindings +were archived upstream in 2024 — ten crates, plus `glib`'s unsound +`VariantStrIter` (fixed in the 0.19 line the GTK3 bindings never moved +to), plus `proc-macro-error` via `glib-macros`. The five `unic-*` crates +come the same way, via `urlpattern` in `tauri-utils`. Confirm any of it +with: + +``` +cargo tree -i gtk --target x86_64-unknown-linux-gnu +``` + +None of them is a *vulnerability*. `cargo audit` reports zero of those +against this lockfile; all seventeen are informational — sixteen +unmaintained, one unsound. + +**Why ignore rather than leave them red.** The audit job fails while +advisories stand, and auto-fix opens a PR when a version clears one. +Neither has anywhere to go here: there is no newer version, so the audit +stays red forever and the auto-fix PR is empty every week. A check that +is always red stops being read, and then the eighteenth advisory — the +one that *is* a vulnerability, in a crate we chose — arrives into a run +nobody looks at. The list buys back a green baseline. + +**What it does not do.** `ignore` suppresses exactly the ids named. A new +advisory against any of these same crates still fails the run, as does +any advisory anywhere else in the tree. The ids are also printed into the +job summary on every run, so a suppression cannot quietly outlive its +reason. + +**Reviewing it.** Re-check when Tauri ships a GTK4/webkit6 Linux runtime, +and otherwise every few months. To test whether an entry can go, delete +the line and run `cargo audit`: if it stays quiet, the advisory is gone +and so should the line be. + +Two places consume the list. `cargo audit` reads `.cargo/audit.toml` +itself. `rustsec/audit-check`, which runs on pull requests, does not — it +only takes an `ignore` input — so the workflow greps the ids out of the +file and passes them in, rather than keeping a second list to drift from +the first. + ## Auto-fix Runs `cargo audit fix` (raises a version requirement when clearing the diff --git a/docs/RULES.md b/docs/RULES.md index eb1731e..153143b 100644 --- a/docs/RULES.md +++ b/docs/RULES.md @@ -10,7 +10,7 @@ time from [`crates/diskern-core/rules/base.json`](../crates/diskern-core/rules/b ```json { "id": "chrome-cache", - "patterns": ["/google/chrome/user data/default/cache", "/.cache/google-chrome"], + "patterns": ["**/google/chrome/user data/*/cache/**", "**/.cache/google-chrome/**"], "category": "browser_cache", "verdict": "safe", "description": "Chrome browser cache. Fully regenerated on next use." @@ -20,7 +20,7 @@ time from [`crates/diskern-core/rules/base.json`](../crates/diskern-core/rules/b | Field | Meaning | | ------------- | -------------------------------------------------------------------- | | `id` | Stable, kebab-case, unique. Shown to users as evidence. | -| `patterns` | Substrings matched against the normalized path (lowercased, `/`-separated). | +| `patterns` | Globs matched against the normalized path (lowercased, `/`-separated). | | `category` | What the file *is* — `browser_cache`, `build_artifact`, `log`, … | | `verdict` | Base safety verdict (see below). | | `description` | Plain-language explanation shown to the user. | @@ -42,19 +42,34 @@ evidence (e.g. recently-accessed files), never less. - **First match wins** — order in the file is priority order, which is why `protected` rules are listed first. - Paths that match no rule get `unknown` / `review` — never `safe`. -- Patterns are plain substrings matched anywhere in the path, so a rule - cannot say "this extension, but only under Downloads". Where that - matters, write the half that is safe on its own (the extension) and - keep the verdict at `review`. Proper glob matching is - [issue #41](https://github.com/Coding-Moves/diskern/issues/41). - -Because matches can land anywhere in a path, **rule order is a safety -property**. `installer-packages` matches `.msi` anywhere — including -`C:\Windows\Installer`, the cache Windows needs to uninstall, repair or -patch installed software, and the `Package Cache` folders that serve the -same purpose for .NET and Visual Studio. `review` is an *actionable* -verdict in the app, so without `windows-installer-cache` listed above -`installer-packages`, the UI would offer to quarantine them. +- Patterns are globs, matched against the whole normalized path: + - `*` matches within one path component and stops at `/` + - `**` spans components + - a pattern that starts with `/` is anchored at the filesystem root + +That anchoring is what keeps a rule inside the directory it names. +`/tmp/**` is the root's scratch directory; it does not reach +`/home/user/tmp/tax-return.pdf`. `**/node_modules/**` still matches at +any depth, because that is what the rule means. + +Write patterns to end in `/**` when the rule is about a directory, and +as `**/*.ext` when it is about an extension. A directory pattern without +the trailing `/**` matches the directory entry itself — and the scanner +only ever classifies files, so it would match nothing. + +A pattern that isn't a valid glob is dropped with a warning rather than +taken down the scan, so **a typo makes a rule match nothing**. For a +`protected` rule that fails open, which is why +`every_shipped_pattern_is_a_valid_glob` in +[`rules.rs`](../crates/diskern-core/src/rules.rs) exists. + +**Rule order is a safety property.** `installer-packages` matches +`**/*.msi` anywhere on the disk — including `C:\Windows\Installer`, the +cache Windows needs to uninstall, repair or patch installed software, and +the `Package Cache` folders that serve the same purpose for .NET and +Visual Studio. `review` is an *actionable* verdict in the app, so without +`windows-installer-cache` listed above `installer-packages`, the UI would +offer to quarantine them. When adding a broad rule: @@ -75,7 +90,7 @@ rules PRs are very welcome. Guidelines: 1. Be conservative: when in doubt, use `review`, not `safe`. 2. Patterns should be specific enough not to match user data - (e.g. `/target/debug`, not `/target`). + (e.g. `**/target/debug/**`, not `**/target/**`). 3. Write the `description` for end users: what it is, why it's safe (or not), what happens after removal. 4. Add a test in [`rules.rs`](../crates/diskern-core/src/rules.rs) if the