Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,9 @@ jobs:

- name: clippy (app only — privhelper is Linux/macOS)
run: cargo clippy -p freeyourdisk --all-targets -- -D warnings

# Run the cross-platform core-trash tests on REAL Windows — including the
# zone-canonicalization regression guard for the elevated cleanup path. The
# clippy gate above is compile-only and cannot catch this runtime bug.
- name: test core-trash (Windows runtime)
run: cargo test -p core-trash

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Canonicalize core-trash fixtures before enabling Windows tests

This new Windows step runs the entire core-trash test suite, but several in-zone tests still build zones with Zones(vec![dir.path().to_path_buf()]) (for example crates/core-trash/src/lib.rs:199-204 and 293-312). On Windows, validate() canonicalizes the candidate parent before starts_with, and the new regression comment documents that non-canonical zones do not match the resulting \\?\ paths, so these existing tests fail on windows-latest as soon as this job is enabled. Canonicalize the fixture zones or narrow the Windows test before adding this CI gate.

Useful? React with 👍 / 👎.

1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions crates/core-trash/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ repository.workspace = true
core-ipc = { path = "../core-ipc" }
trash = { workspace = true }
thiserror = { workspace = true }
# Normalizes Windows canonical paths (strips the \\?\ verbatim prefix) so zone
# membership (starts_with) works; on Linux/macOS it delegates to fs::canonicalize.
dunce = "1"

[dev-dependencies]
tempfile = { workspace = true }
130 changes: 123 additions & 7 deletions crates/core-trash/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,16 @@ pub struct Zones(pub Vec<PathBuf>);

impl Zones {
fn contains(&self, path: &Path) -> bool {
self.0.iter().any(|zone| path.starts_with(zone))
// Compare against each zone's *canonical* form so a plain zone matches a
// canonicalized candidate. `validate` resolves candidates to their real
// location (dunce strips Windows' \\?\ prefix and resolves macOS
// /tmp→/private/tmp); the zone must be resolved the same way or
// starts_with never matches. Falls back to the literal zone if it cannot
// be resolved (e.g. it does not exist on disk).
self.0.iter().any(|zone| {
let zone_real = dunce::canonicalize(zone).unwrap_or_else(|_| zone.clone());
path.starts_with(&zone_real)
})
}
}

Expand All @@ -41,11 +50,11 @@ pub fn validate(path: &Path, zones: &Zones) -> Result<PathBuf, TrashError> {
let lexical = match (path.parent(), path.file_name()) {
(Some(parent), Some(name)) => {
let parent_real =
fs::canonicalize(parent).map_err(|e| TrashError::Io(e.to_string()))?;
dunce::canonicalize(parent).map_err(|e| TrashError::Io(e.to_string()))?;
parent_real.join(name)
}
// path is "/" or ends with "..": resolve it whole.
_ => fs::canonicalize(path).map_err(|e| TrashError::Io(e.to_string()))?,
_ => dunce::canonicalize(path).map_err(|e| TrashError::Io(e.to_string()))?,
};

if !zones.contains(&lexical) {
Expand All @@ -56,7 +65,7 @@ pub fn validate(path: &Path, zones: &Zones) -> Result<PathBuf, TrashError> {
.map(|m| m.file_type().is_symlink())
.unwrap_or(false);
if is_symlink {
let real = fs::canonicalize(path).map_err(|e| TrashError::Io(e.to_string()))?;
let real = dunce::canonicalize(path).map_err(|e| TrashError::Io(e.to_string()))?;
if !zones.contains(&real) {
return Err(TrashError::SymlinkEscape(path.to_path_buf()));
}
Expand Down Expand Up @@ -98,6 +107,42 @@ pub fn delete_permanent(paths: &[PathBuf], zones: &Zones) -> ExecutionReport {
execute(paths, zones, |real| remove(real).map_err(|e| e.to_string()))
}

/// Validate every path in `plan` against `zones` (all-or-nothing) and, if none
/// escape, execute the deletion. `Ok` = executed report; `Err` = refusal report
/// (nothing deleted). Shared by the privileged helper (Linux/macOS) and the
/// Windows elevated executor so the allowlist has a single source of truth.
pub fn execute_root_plan(
plan: &core_ipc::DeletionPlan,
zones: &Zones,
) -> Result<core_ipc::ExecutionReport, core_ipc::ExecutionReport> {
let paths: Vec<PathBuf> = plan.items.iter().map(|item| item.path.clone()).collect();

let refusals: Vec<core_ipc::ItemError> = paths
.iter()
.filter_map(|path| match validate(path, zones) {
Ok(_) => None,
Err(err) => Some(core_ipc::ItemError {
path: path.clone(),
message: err.to_string(),
}),
})
.collect();

if !refusals.is_empty() {
return Err(core_ipc::ExecutionReport {
freed_bytes: 0,
deleted_count: 0,
errors: refusals,
});
}

let report = match plan.destination {
core_ipc::Destination::Trash => to_trash(&paths, zones),
core_ipc::Destination::Permanent => delete_permanent(&paths, zones),
};
Ok(report)
}

fn execute(
paths: &[PathBuf],
zones: &Zones,
Expand Down Expand Up @@ -138,10 +183,14 @@ mod tests {

#[test]
fn validate_rejects_outside_zone() {
let dir = tempfile::tempdir().unwrap();
let zones = Zones(vec![dir.path().to_path_buf()]);
// Cross-platform: a real file in a different tempdir is out of zone.
let zone_dir = tempfile::tempdir().unwrap();
let outside_dir = tempfile::tempdir().unwrap();
let outside = outside_dir.path().join("victim.txt");
std::fs::write(&outside, b"x").unwrap();
let zones = Zones(vec![zone_dir.path().to_path_buf()]);
assert!(matches!(
validate(Path::new("/etc/passwd"), &zones),
validate(&outside, &zones),
Err(TrashError::OutsideZone(_))
));
}
Expand Down Expand Up @@ -209,4 +258,71 @@ mod tests {
assert_eq!(report.errors.len(), 1);
assert!(victim.exists());
}

#[test]
fn execute_root_plan_refuses_whole_batch_on_escape() {
let zone = tempfile::tempdir().unwrap();
let inside = zone.path().join("junk.tmp");
std::fs::write(&inside, b"x").unwrap();
let outside = tempfile::tempdir().unwrap();
let escape = outside.path().join("keep.txt");
std::fs::write(&escape, b"important").unwrap();

let zones = Zones(vec![zone.path().to_path_buf()]);
let plan = core_ipc::DeletionPlan {
items: vec![
core_ipc::ScanItem {
id: inside.to_string_lossy().into_owned(),
path: inside.clone(),
size_bytes: 1,
last_access: None,
kind: core_ipc::ItemKind::File,
requires_root: true,
},
core_ipc::ScanItem {
id: escape.to_string_lossy().into_owned(),
path: escape.clone(),
size_bytes: 9,
last_access: None,
kind: core_ipc::ItemKind::File,
requires_root: true,
},
],
destination: core_ipc::Destination::Permanent,
total_bytes: 10,
requires_root: true,
};

let result = execute_root_plan(&plan, &zones);
assert!(result.is_err(), "any escape must refuse the whole batch");
assert!(inside.exists(), "nothing deleted on refusal");
assert!(
escape.exists(),
"the out-of-zone file must never be touched"
);
}

#[test]
fn execute_root_plan_deletes_when_all_in_zone() {
let zone = tempfile::tempdir().unwrap();
let f = zone.path().join("junk.tmp");
std::fs::write(&f, vec![0u8; 50]).unwrap();
let zones = Zones(vec![zone.path().to_path_buf()]);
let plan = core_ipc::DeletionPlan {
items: vec![core_ipc::ScanItem {
id: f.to_string_lossy().into_owned(),
path: f.clone(),
size_bytes: 50,
last_access: None,
kind: core_ipc::ItemKind::File,
requires_root: true,
}],
destination: core_ipc::Destination::Permanent,
total_bytes: 50,
requires_root: true,
};
let result = execute_root_plan(&plan, &zones);
assert!(result.is_ok());
assert!(!f.exists(), "in-zone file deleted");
}
}
43 changes: 13 additions & 30 deletions crates/privhelper/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
//!
//! Exit codes: 0 = success, 2 = invalid input, 3 = a path was refused.

use core_ipc::{DeletionPlan, Destination, ExecutionReport, InstallReport, ItemError, SmartInfo};
use core_trash::{delete_permanent, to_trash, validate, Zones};
use core_ipc::{DeletionPlan, ExecutionReport, InstallReport, SmartInfo};
use core_trash::Zones;
use std::io::{Read, Write};
use std::path::PathBuf;
use std::process::{Command, ExitCode};
Expand Down Expand Up @@ -249,33 +249,16 @@ fn main() -> ExitCode {
};

let zones = Zones(ROOT_ZONES.iter().map(PathBuf::from).collect());
let paths: Vec<PathBuf> = plan.items.iter().map(|item| item.path.clone()).collect();

// Pre-validate every path; refuse the whole batch on any escape.
let refusals: Vec<ItemError> = paths
.iter()
.filter_map(|path| match validate(path, &zones) {
Ok(_) => None,
Err(err) => Some(ItemError {
path: path.clone(),
message: err.to_string(),
}),
})
.collect();

if !refusals.is_empty() {
write_report(&ExecutionReport {
freed_bytes: 0,
deleted_count: 0,
errors: refusals,
});
return ExitCode::from(3);
// Validation + execution is the shared single source of truth (also used by
// the Windows elevated executor). All-or-nothing: any escape refuses the batch.
match core_trash::execute_root_plan(&plan, &zones) {
Ok(report) => {
write_report(&report);
ExitCode::SUCCESS
}
Err(refusal) => {
write_report(&refusal);
ExitCode::from(3)
}
}

let report = match plan.destination {
Destination::Trash => to_trash(&paths, &zones),
Destination::Permanent => delete_permanent(&paths, &zones),
};
write_report(&report);
ExitCode::SUCCESS
}
Loading
Loading