From af8bd5a17780ae47c9aebc547666a4191ba01772 Mon Sep 17 00:00:00 2001 From: Alex Smolya Date: Sun, 6 Sep 2026 22:48:10 +0200 Subject: [PATCH] fix(app): enforce report-backed quarantine authority --- CHANGELOG.md | 3 + Cargo.lock | 1 + app/README.md | 6 +- app/src-tauri/Cargo.toml | 3 + app/src-tauri/src/commands.rs | 379 ++++++++++++++++++++++++----- app/src-tauri/src/lib.rs | 2 +- app/src/App.jsx | 14 +- crates/diskern-core/src/actions.rs | 295 +++++++++++++++++++++- crates/diskern-core/src/lib.rs | 16 ++ docs/ARCHITECTURE.md | 18 +- 10 files changed, 660 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96bfe77..e9a958b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,9 @@ All notable changes to Diskern are documented here. The format follows ### Changed +- Desktop quarantine now uses the exact finding from the completed backend + report, with graph-aware verdicts, generation-scoped report authority and + fail-closed stale-finding checks; frontend verdict values are never trusted - 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 diff --git a/Cargo.lock b/Cargo.lock index 62ce792..07f5ce3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -839,6 +839,7 @@ dependencies = [ "tauri-plugin-dialog", "tauri-plugin-process", "tauri-plugin-updater", + "tempfile", ] [[package]] diff --git a/app/README.md b/app/README.md index 3890d96..c58038d 100644 --- a/app/README.md +++ b/app/README.md @@ -30,7 +30,7 @@ a live file counter. | --- | --- | | `start_scan` | Read-only scan; returns a report, or `null` if cancelled | | `cancel_scan` | Stops the scan in flight | -| `quarantine_finding` | Moves one file to quarantine, re-classifying server-side first | +| `quarantine_finding` | Moves one exact report finding to quarantine; the backend owns the verdict and rejects stale or superseded report state | | `list_quarantine` | Everything currently quarantined, read from the manifest | | `restore_quarantined` | Puts one file back where it came from | | `purge_quarantine` | Empties quarantine for good — the only deletion in the app | @@ -38,7 +38,9 @@ a live file counter. Everything except `quarantine_finding` and `purge_quarantine` is read-only. Quarantine is manifest-backed, so what was moved in one session is still restorable in the next; the Quarantine panel renders -before any scan has been run for exactly that reason. +before any scan has been run for exactly that reason. A completed scan is the +user-review snapshot for quarantine: starting or cancelling another scan +invalidates that snapshot, and a new scan is required before acting again. ## Updater diff --git a/app/src-tauri/Cargo.toml b/app/src-tauri/Cargo.toml index dfaeaaa..c966eab 100644 --- a/app/src-tauri/Cargo.toml +++ b/app/src-tauri/Cargo.toml @@ -21,3 +21,6 @@ tauri = { version = "2", features = [] } tauri-plugin-updater = "2" tauri-plugin-dialog = "2" tauri-plugin-process = "2" + +[dev-dependencies] +tempfile = "3" diff --git a/app/src-tauri/src/commands.rs b/app/src-tauri/src/commands.rs index 482e8d9..6b1b440 100644 --- a/app/src-tauri/src/commands.rs +++ b/app/src-tauri/src/commands.rs @@ -1,30 +1,140 @@ -use diskern_core::{actions, report, rules::RulesDb, scanner, GenomeError, Verdict}; +use diskern_core::{actions, report, rules::RulesDb, scanner, GenomeError}; use serde::Serialize; use std::path::PathBuf; -use std::sync::atomic::Ordering; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex, MutexGuard}; use std::time::Duration; use tauri::{Emitter, State, Window}; -/// Handle on the scan that is running right now, if any. +/// Backend-owned scan/report authority. /// -/// The engine has always supported cancellation — `ScanProgress` carries the -/// flag and the walk checks it on every entry — but `start_scan` created its -/// `ScanProgress` as a local, so nothing outside that one call could ever -/// reach it. This is the shared slot that makes it reachable. +/// A completed report is a review snapshot. Starting or cancelling a scan +/// invalidates the previous snapshot, and only the scan that owns the current +/// generation may publish. The mutex protects short state transitions only; +/// filesystem scanning and quarantine I/O happen outside it. #[derive(Default)] -pub struct ActiveScan(Mutex>>); - -impl ActiveScan { - /// A poisoned lock means some earlier holder panicked while swapping an - /// `Option`. There is no invariant here worth protecting, and refusing - /// to unlock would leave cancellation permanently broken for the rest of - /// the session — so recover the value instead. - fn slot(&self) -> MutexGuard<'_, Option>> { +pub struct ScanAuthority(Mutex); + +#[derive(Default)] +struct AuthorityState { + next_generation: u64, + active: Option, + report: Option, + epoch: Arc, +} + +struct ActiveGeneration { + generation: u64, + progress: Arc, +} + +struct CompletedReport { + generation: u64, + report: Arc, +} + +struct ActionLease { + generation: u64, + report: Arc, + epoch: Arc, +} + +impl ActionLease { + fn is_current(&self) -> bool { + self.epoch.load(Ordering::Acquire) == self.generation + } +} + +impl ScanAuthority { + fn slot(&self) -> MutexGuard<'_, AuthorityState> { self.0 .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) } + + fn begin(&self, progress: Arc) -> u64 { + let mut state = self.slot(); + if let Some(previous) = state.active.take() { + previous.progress.cancel(); + } + state.next_generation = state.next_generation.wrapping_add(1); + let generation = state.next_generation; + state.epoch.store(generation, Ordering::Release); + state.report = None; + state.active = Some(ActiveGeneration { + generation, + progress, + }); + generation + } + + fn publish(&self, generation: u64, report: report::Report) -> bool { + let mut state = self.slot(); + if state + .active + .as_ref() + .is_some_and(|active| active.generation == generation) + && state.epoch.load(Ordering::Acquire) == generation + { + state.report = Some(CompletedReport { + generation, + report: Arc::new(report), + }); + state.active = None; + true + } else { + false + } + } + + fn abandon(&self, generation: u64) { + let mut state = self.slot(); + if state + .active + .as_ref() + .is_some_and(|active| active.generation == generation) + { + state.active = None; + state.report = None; + } + } + + fn finish_active(&self, generation: u64) { + let mut state = self.slot(); + if state + .active + .as_ref() + .is_some_and(|active| active.generation == generation) + { + state.active = None; + } + } + + fn cancel(&self) -> bool { + let mut state = self.slot(); + let Some(active) = state.active.take() else { + return false; + }; + active.progress.cancel(); + state + .epoch + .store(active.generation.wrapping_add(1), Ordering::Release); + state.report = None; + true + } + + fn lease(&self) -> Option { + let state = self.slot(); + if state.active.is_some() { + return None; + } + let completed = state.report.as_ref()?; + Some(ActionLease { + generation: completed.generation, + report: completed.report.clone(), + epoch: state.epoch.clone(), + }) + } } #[derive(Clone, Serialize)] @@ -35,7 +145,7 @@ struct ScanProgressPayload { /// 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. +/// authority 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 — @@ -46,12 +156,17 @@ struct ScanProgressPayload { struct ScanRun<'a> { stop: Arc, ticker: Option>, - state: &'a ActiveScan, - progress: Arc, + state: &'a ScanAuthority, + generation: u64, } impl<'a> ScanRun<'a> { - fn start(window: &Window, state: &'a ActiveScan, progress: Arc) -> Self { + fn start( + window: &Window, + state: &'a ScanAuthority, + generation: u64, + progress: Arc, + ) -> Self { let stop = Arc::new(std::sync::atomic::AtomicBool::new(false)); // A plain OS thread keeps this independent of whatever async @@ -76,7 +191,7 @@ impl<'a> ScanRun<'a> { stop, ticker: Some(ticker), state, - progress, + generation, } } } @@ -88,17 +203,9 @@ impl Drop for ScanRun<'_> { 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; - } + // Clear only if the slot still belongs to this generation. A newer + // scan may already own it. + self.state.finish_active(self.generation); } } @@ -115,18 +222,16 @@ impl Drop for ScanRun<'_> { #[tauri::command] pub async fn start_scan( window: Window, - state: State<'_, ActiveScan>, + state: State<'_, Arc>, roots: Vec, ) -> Result, String> { let progress = Arc::new(scanner::ScanProgress::default()); - // Publish the handle before the walk starts. The guard is a temporary - // here on purpose — holding it across the await below would make this - // future non-Send. - *state.slot() = Some(progress.clone()); + let authority = state.inner().clone(); + let generation = authority.begin(progress.clone()); // 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 run = ScanRun::start(&window, &authority, generation, progress.clone()); let progress_for_scan = progress.clone(); let joined = tauri::async_runtime::spawn_blocking(move || { @@ -151,8 +256,6 @@ pub async fn start_scan( }) .await; - drop(run); - // One final snapshot so the UI's last-seen count matches the real total. let _ = window.emit( "scan-progress", @@ -162,7 +265,26 @@ pub async fn start_scan( }, ); - joined.map_err(|e| e.to_string())? + let joined = joined.map_err(|e| e.to_string())?; + let result = match joined { + Ok(Some(report)) => { + if authority.publish(generation, report.clone()) { + Ok(Some(report)) + } else { + Err("scan was superseded by a newer scan".to_string()) + } + } + Ok(None) => { + authority.abandon(generation); + Ok(None) + } + Err(error) => { + authority.abandon(generation); + Err(error) + } + }; + drop(run); + result } /// Stop the scan that is currently running. @@ -174,43 +296,51 @@ pub async fn start_scan( /// Read-only, like `start_scan`: setting the flag makes the walk return /// early. Nothing is written, moved, or deleted. #[tauri::command] -pub fn cancel_scan(state: State<'_, ActiveScan>) -> bool { - match state.slot().as_ref() { - Some(progress) => { - progress.cancel(); - true - } - None => false, - } +pub fn cancel_scan(state: State<'_, Arc>) -> bool { + state.cancel() } -/// 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. +/// 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` takes only a path lookup +/// key and obtains the finding from the current backend report lease. The +/// report-bound core action then applies the graph-aware verdict and fresh +/// static-rule tightening before moving anything. #[tauri::command] pub async fn quarantine_finding( + state: State<'_, Arc>, path: PathBuf, quarantine_dir: PathBuf, ) -> Result { + let authority = state.inner().clone(); tauri::async_runtime::spawn_blocking(move || { - let (_, verdict, _) = RulesDb::embedded().classify(&path); - if matches!(verdict, Verdict::Protected | Verdict::Risky) { - return Err(format!( - "{} is classified {verdict:?}; action refused", - path.display() - )); - } - actions::quarantine(&path, verdict, &quarantine_dir).map_err(|e| e.to_string()) + quarantine_from_authoritative(&authority, &path, &quarantine_dir) }) .await .map_err(|e| e.to_string())? } +fn quarantine_from_authoritative( + authority: &ScanAuthority, + path: &std::path::Path, + quarantine_dir: &std::path::Path, +) -> Result { + let lease = authority.lease().ok_or_else(|| { + "no current completed scan report; refusing to quarantine — scan again".to_string() + })?; + if !lease.is_current() { + return Err("scan report was superseded; refusing to quarantine — scan again".into()); + } + actions::quarantine_finding_with_authorization( + path, + &lease.report, + &RulesDb::embedded(), + quarantine_dir, + || lease.is_current(), + ) + .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: @@ -258,3 +388,124 @@ pub async fn purge_quarantine(quarantine_dir: PathBuf) -> Result report::Report { + report::Report { + findings: vec![], + duplicate_sets: vec![], + total_reclaimable: 0, + files_scanned: 0, + } + } + + fn report_for(path: &Path, verdict: Verdict) -> report::Report { + let metadata = std::fs::symlink_metadata(path).unwrap(); + let epoch = |time: std::time::SystemTime| { + time.duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() as i64 + }; + report::Report { + findings: vec![Finding { + entry: FileEntry { + path: path.to_path_buf(), + size: metadata.len(), + modified: metadata.modified().ok().map(epoch), + accessed: metadata.accessed().ok().map(epoch), + is_symlink: metadata.file_type().is_symlink(), + hash: None, + }, + category: Category::TempFile, + verdict, + risk_score: 0.5, + reasons: vec!["backend test".into()], + reclaimable: metadata.len(), + }], + duplicate_sets: vec![], + total_reclaimable: metadata.len(), + files_scanned: 1, + } + } + + #[test] + fn no_completed_report_fails_closed() { + let authority = ScanAuthority::default(); + let error = quarantine_from_authoritative( + &authority, + Path::new("/not-in-a-report"), + Path::new("/not-created"), + ) + .unwrap_err(); + assert!(error.contains("no current completed scan report")); + } + + #[test] + fn only_the_current_generation_can_publish() { + let authority = ScanAuthority::default(); + let first_progress = Arc::new(scanner::ScanProgress::default()); + let first = authority.begin(first_progress.clone()); + let second_progress = Arc::new(scanner::ScanProgress::default()); + let second = authority.begin(second_progress); + + assert!(first_progress.cancelled.load(Ordering::Acquire)); + assert!(!authority.publish(first, empty_report())); + assert!(authority.publish(second, empty_report())); + assert!(authority.lease().is_some()); + } + + #[test] + fn cancellation_invalidates_the_report_authority() { + let authority = ScanAuthority::default(); + let progress = Arc::new(scanner::ScanProgress::default()); + authority.begin(progress.clone()); + assert!(authority.cancel()); + assert!(progress.cancelled.load(Ordering::Acquire)); + assert!(authority.lease().is_none()); + assert!(!authority.cancel()); + } + + #[test] + fn an_invalidated_action_lease_cannot_move_a_file() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("cache.bin"); + let quarantine_dir = dir.path().join("quarantine"); + std::fs::write(&target, b"cache").unwrap(); + let authority = ScanAuthority::default(); + let generation = authority.begin(Arc::new(scanner::ScanProgress::default())); + authority.publish(generation, report_for(&target, Verdict::Review)); + let lease = authority.lease().unwrap(); + + authority.begin(Arc::new(scanner::ScanProgress::default())); + let result = actions::quarantine_finding_with_authorization( + &target, + &lease.report, + &RulesDb::embedded(), + &quarantine_dir, + || lease.is_current(), + ); + assert!(result.is_err()); + assert!(target.exists()); + assert!(!quarantine_dir.exists()); + } + + #[test] + fn risky_report_is_refused_without_a_frontend_verdict() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("dependency.bin"); + let quarantine_dir = dir.path().join("quarantine"); + std::fs::write(&target, b"keep").unwrap(); + let authority = ScanAuthority::default(); + let generation = authority.begin(Arc::new(scanner::ScanProgress::default())); + authority.publish(generation, report_for(&target, Verdict::Risky)); + + let result = quarantine_from_authoritative(&authority, &target, &quarantine_dir); + assert!(result.is_err()); + assert!(target.exists()); + } +} diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index dc719ee..5a69c03 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -5,7 +5,7 @@ mod commands; pub fn run() { tauri::Builder::default() - .manage(commands::ActiveScan::default()) + .manage(std::sync::Arc::new(commands::ScanAuthority::default())) .setup(|app| { // Updater: desktop only, checked from the frontend after launch. #[cfg(desktop)] diff --git a/app/src/App.jsx b/app/src/App.jsx index bf4522f..2a77f70 100644 --- a/app/src/App.jsx +++ b/app/src/App.jsx @@ -467,6 +467,12 @@ export default function App() { } if (!folder) return; + // Starting a scan invalidates the previous backend report authority. + // Clear the view at the same time so no stale row remains actionable. + setReport(null); + setScannedFolder(null); + setQuarantinedPaths(new Set()); + setReclaimed(0); setLiveProgress({ files_seen: 0, bytes_seen: 0 }); setScanning(true); setCancelling(false); @@ -480,16 +486,14 @@ export default function App() { try { const result = await invoke("start_scan", { roots: [folder] }); - // null means the scan was cancelled. Keep whatever report was already - // on screen rather than blanking the view. + // null means the scan was cancelled. The backend has no completed + // report authority after a cancellation, and the view was cleared when + // this scan began. if (result === null) { setNotice("Scan cancelled. Scanning is read-only — nothing was moved or deleted."); } else { setReport(result); setScannedFolder(folder); - // Fresh scan — clear any prior session's quarantine bookkeeping. - setQuarantinedPaths(new Set()); - setReclaimed(0); } } catch (e) { setError(String(e)); diff --git a/crates/diskern-core/src/actions.rs b/crates/diskern-core/src/actions.rs index f965935..15a1eba 100644 --- a/crates/diskern-core/src/actions.rs +++ b/crates/diskern-core/src/actions.rs @@ -14,7 +14,7 @@ //! [`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 crate::{report, rules::RulesDb, FileEntry, GenomeError, Result, Verdict}; use serde::{Deserialize, Serialize}; use std::io::Write; use std::path::{Path, PathBuf}; @@ -114,6 +114,93 @@ pub fn quarantine( Ok(record) } +/// Quarantine a finding selected from a backend-owned report. +/// +/// `path` is an exact path lookup key. It is deliberately not canonicalized: +/// a symlink or other alias must not borrow the authorization of a scanned +/// object. The report verdict includes graph evidence from the completed scan; +/// a fresh static rule may only tighten it. The report metadata is checked +/// without using access time, since reading a file can legitimately update +/// atime without changing the object. +pub fn quarantine_finding( + path: &Path, + report: &report::Report, + rules: &RulesDb, + quarantine_dir: &Path, +) -> Result { + quarantine_finding_with_authorization(path, report, rules, quarantine_dir, || true) +} + +/// Report-bound quarantine with a final authority check supplied by the +/// caller. Tauri uses this to reject a lease invalidated by a newer scan +/// without holding the report mutex over filesystem I/O. +pub fn quarantine_finding_with_authorization( + path: &Path, + report: &report::Report, + rules: &RulesDb, + quarantine_dir: &Path, + still_authorized: F, +) -> Result +where + F: Fn() -> bool, +{ + let finding = report + .findings + .iter() + .find(|finding| finding.entry.path == path) + .ok_or_else(|| { + GenomeError::Rules(format!( + "{} is not present in the authoritative report; scan again", + path.display() + )) + })?; + + let (_, fresh_verdict, _) = rules.classify(path); + let effective_verdict = finding.verdict.strictest(fresh_verdict); + if matches!(effective_verdict, Verdict::Risky | Verdict::Protected) { + return Err(GenomeError::Rules(format!( + "{} is classified {effective_verdict:?}; action refused", + path.display() + ))); + } + + ensure_entry_is_current(&finding.entry)?; + if !still_authorized() { + return Err(GenomeError::Rules( + "scan report was superseded; refusing to quarantine — scan again".into(), + )); + } + + quarantine(&finding.entry.path, effective_verdict, quarantine_dir) +} + +/// Check the metadata captured by the scanner, excluding atime. +/// +/// Size, modification time, and symlink-ness catch ordinary replacement and +/// mutation. They are not a universal filesystem identity proof: platforms +/// with coarse timestamps and an attacker able to replace a path in the final +/// rename window remain outside this bounded path-based guarantee. +fn ensure_entry_is_current(entry: &FileEntry) -> Result<()> { + let metadata = std::fs::symlink_metadata(&entry.path).map_err(|e| io_err(&entry.path, e))?; + let modified = metadata.modified().ok().and_then(to_epoch); + let is_symlink = metadata.file_type().is_symlink(); + + if metadata.len() != entry.size || modified != entry.modified || is_symlink != entry.is_symlink + { + return Err(GenomeError::Rules(format!( + "{} changed since the report was built; scan again before quarantining", + entry.path.display() + ))); + } + Ok(()) +} + +fn to_epoch(t: std::time::SystemTime) -> Option { + t.duration_since(std::time::UNIX_EPOCH) + .ok() + .map(|d| d.as_secs() as i64) +} + /// A quarantine filename that no existing file already owns. /// /// Flattening is lossy on purpose (it only has to be readable; the @@ -389,6 +476,53 @@ fn now_epoch() -> i64 { #[cfg(test)] mod tests { use super::*; + use std::sync::Arc; + + fn entry_for(path: &Path) -> FileEntry { + let metadata = std::fs::symlink_metadata(path).unwrap(); + let epoch = |time: std::time::SystemTime| { + time.duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() as i64 + }; + FileEntry { + path: path.to_path_buf(), + size: metadata.len(), + modified: metadata.modified().ok().map(epoch), + accessed: metadata.accessed().ok().map(epoch), + is_symlink: metadata.file_type().is_symlink(), + hash: None, + } + } + + fn report_for(path: &Path, verdict: Verdict) -> report::Report { + report::Report { + findings: vec![crate::Finding { + entry: entry_for(path), + category: crate::Category::TempFile, + verdict, + risk_score: 0.5, + reasons: vec!["test finding".into()], + reclaimable: path.metadata().unwrap().len(), + }], + duplicate_sets: vec![], + total_reclaimable: path.metadata().unwrap().len(), + files_scanned: 1, + } + } + + fn rules_with_verdict(path_pattern: &str, verdict: Verdict) -> RulesDb { + RulesDb::new( + 1, + vec![crate::rules::Rule { + id: "test-rule".into(), + patterns: vec![path_pattern.into()], + category: crate::Category::TempFile, + verdict, + description: "test rule".into(), + }], + ) + } #[test] fn quarantine_and_restore_roundtrip() { @@ -405,6 +539,165 @@ mod tests { assert!(f.exists()); } + #[test] + fn referenced_node_modules_finding_cannot_be_quarantined() { + let dir = tempfile::tempdir().unwrap(); + let project = dir.path().join("project"); + let target = project.join("node_modules/react/index.js"); + std::fs::create_dir_all(target.parent().unwrap()).unwrap(); + std::fs::write(project.join("package.json"), b"{}").unwrap(); + std::fs::write(&target, b"live dependency").unwrap(); + + let rules = rules_with_verdict("**/node_modules/**", Verdict::Review); + let entries = crate::scanner::scan( + &crate::scanner::ScanOptions { + roots: vec![dir.path().to_path_buf()], + ..Default::default() + }, + Arc::new(crate::scanner::ScanProgress::default()), + ) + .unwrap(); + let report = crate::report::build(entries, &rules); + let finding = report + .findings + .iter() + .find(|finding| finding.entry.path == target) + .unwrap(); + assert_eq!(finding.verdict, Verdict::Risky); + assert!( + quarantine_finding(&target, &report, &rules, &dir.path().join("quarantine")).is_err() + ); + assert!(target.exists()); + } + + #[test] + fn an_authoritative_review_finding_can_be_quarantined() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("cache.bin"); + let quarantine_dir = dir.path().join("quarantine"); + std::fs::write(&target, b"cache").unwrap(); + let report = report_for(&target, Verdict::Review); + let rules = rules_with_verdict("**/cache.bin", Verdict::Review); + + let record = quarantine_finding(&target, &report, &rules, &quarantine_dir).unwrap(); + assert!(!target.exists()); + assert!(record.quarantined_to.exists()); + } + + #[test] + fn a_path_missing_from_the_authoritative_report_fails_closed() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("not-in-report.bin"); + let quarantine_dir = dir.path().join("quarantine"); + std::fs::write(&target, b"keep").unwrap(); + let report = report::Report { + findings: vec![], + duplicate_sets: vec![], + total_reclaimable: 0, + files_scanned: 1, + }; + let rules = rules_with_verdict("**/*.bin", Verdict::Review); + + let result = quarantine_finding(&target, &report, &rules, &quarantine_dir); + assert!(result.is_err()); + assert!(target.exists()); + assert!(!quarantine_dir.exists()); + } + + #[test] + fn fresh_rules_only_tighten_the_report_verdict() { + let cases = [ + (Verdict::Risky, Verdict::Review, "report-risky"), + (Verdict::Review, Verdict::Protected, "report-review"), + (Verdict::Safe, Verdict::Risky, "report-safe"), + ]; + + for (report_verdict, fresh_verdict, name) in cases { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join(name); + std::fs::write(&target, b"data").unwrap(); + let report = report_for(&target, report_verdict); + let rules = rules_with_verdict("**/*", fresh_verdict); + + let result = + quarantine_finding(&target, &report, &rules, &dir.path().join("quarantine")); + assert!(result.is_err(), "{report_verdict:?} + {fresh_verdict:?}"); + assert!(target.exists()); + } + } + + #[test] + fn reading_a_finding_does_not_invalidate_it() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("cache.bin"); + let quarantine_dir = dir.path().join("quarantine"); + std::fs::write(&target, b"cache").unwrap(); + let report = report_for(&target, Verdict::Review); + let rules = rules_with_verdict("**/cache.bin", Verdict::Review); + + assert_eq!(std::fs::read(&target).unwrap(), b"cache"); + quarantine_finding(&target, &report, &rules, &quarantine_dir).unwrap(); + assert!(!target.exists()); + } + + #[test] + fn a_modified_finding_fails_closed() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("cache.bin"); + std::fs::write(&target, b"before").unwrap(); + let report = report_for(&target, Verdict::Review); + let rules = rules_with_verdict("**/cache.bin", Verdict::Review); + std::fs::write(&target, b"after and different").unwrap(); + + let result = quarantine_finding(&target, &report, &rules, &dir.path().join("quarantine")); + assert!(result.is_err()); + assert!(target.exists()); + } + + #[test] + #[cfg(unix)] + fn an_unscanned_symlink_alias_cannot_borrow_authorization() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("cache.bin"); + let alias = dir.path().join("alias.bin"); + std::fs::write(&target, b"cache").unwrap(); + symlink(&target, &alias).unwrap(); + let report = report_for(&target, Verdict::Review); + let rules = rules_with_verdict("**/*.bin", Verdict::Review); + + let result = quarantine_finding(&alias, &report, &rules, &dir.path().join("quarantine")); + assert!(result.is_err()); + assert!(target.exists()); + assert!(alias.exists()); + } + + #[test] + fn completed_report_is_the_review_snapshot_until_a_new_scan() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("node_modules/abandoned/index.js"); + std::fs::create_dir_all(target.parent().unwrap()).unwrap(); + std::fs::write(&target, b"dependency").unwrap(); + let rules = rules_with_verdict("**/node_modules/**", Verdict::Review); + let entries = crate::scanner::scan( + &crate::scanner::ScanOptions { + roots: vec![dir.path().to_path_buf()], + ..Default::default() + }, + Arc::new(crate::scanner::ScanProgress::default()), + ) + .unwrap(); + let report = crate::report::build(entries, &rules); + assert_eq!(report.findings[0].verdict, Verdict::Review); + + // A changed graph is not silently treated as a newly reviewed report. + // The completed report remains the user's snapshot; the UI must scan + // again before a new graph becomes authoritative. + std::fs::write(dir.path().join("package.json"), b"{}").unwrap(); + quarantine_finding(&target, &report, &rules, &dir.path().join("quarantine")).unwrap(); + } + /// 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 diff --git a/crates/diskern-core/src/lib.rs b/crates/diskern-core/src/lib.rs index 25a5013..8f95649 100644 --- a/crates/diskern-core/src/lib.rs +++ b/crates/diskern-core/src/lib.rs @@ -83,6 +83,22 @@ pub enum Verdict { Protected, } +impl Verdict { + /// Return the more restrictive of two safety decisions. + /// + /// Keep this explicit instead of coupling action safety to the declaration + /// order of the enum. A future insertion or reordering must not silently + /// make a fresh defense-in-depth rule less restrictive. + pub const fn strictest(self, other: Self) -> Self { + match (self, other) { + (Self::Protected, _) | (_, Self::Protected) => Self::Protected, + (Self::Risky, _) | (_, Self::Risky) => Self::Risky, + (Self::Review, _) | (_, Self::Review) => Self::Review, + (Self::Safe, Self::Safe) => Self::Safe, + } + } +} + /// One finding = one row the user sees. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Finding { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 65ee1ea..cb806ee 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -71,10 +71,20 @@ an informational score and per-file evidence, and each entry becomes a and adds only the duplicate copies nothing has counted yet. Acting on a finding is a separate call: -[`actions::quarantine`](../crates/diskern-core/src/actions.rs) is the -only function in the crate that writes, it refuses `Risky` and -`Protected`, and it records every move in a manifest so a restore -survives the process exiting. +[`actions::quarantine_finding`](../crates/diskern-core/src/actions.rs) is the +report-bound safety entry point. It looks up an exact path in the completed +report, combines the report's graph-aware verdict with a fresh static-rule +verdict by taking the stricter result, checks the report's size, modification +time and symlink state without using access time, and then delegates to +[`actions::quarantine`](../crates/diskern-core/src/actions.rs). Missing or +changed findings fail closed. The desktop backend invalidates the report when +a newer scan starts, and a generation lease prevents a superseded scan from +publishing or continuing an action. A completed report is a user-review +snapshot: filesystem graph changes made without a new scan are not silently +treated as a new report, so the user must scan again before relying on them. +The final path-based move still has an ordinary OS-level TOCTOU window; the +metadata check is a bounded stale-report defense, not a universal filesystem +identity proof. ## Adding a feature