diff --git a/crates/lanspread-peer/ARCHITECTURE.md b/crates/lanspread-peer/ARCHITECTURE.md index 2fc92b4..f7fabe1 100644 --- a/crates/lanspread-peer/ARCHITECTURE.md +++ b/crates/lanspread-peer/ARCHITECTURE.md @@ -185,10 +185,12 @@ Reserved per-game paths: - `.lanspread_owned` inside `.local.*` directories proves Lanspread ownership when the current intent is `None`. -Downloaded-file removal is not an uninstall transaction. It removes the whole -game root only for a catalog ID that is a single direct child of the configured -game directory, has a regular root-level `version.ini`, and has no `local/`, -`.local.installing/`, or `.local.backup/` path. +Downloaded-file removal is not an uninstall transaction. For a catalog ID that +is a single direct child of the configured game directory, it requires a valid +ownership record and regular root-level `version.ini`, and refuses `local/`, +`.local.installing/`, or `.local.backup/`. It journals an empty pending +generation, deletes only committed downloader-owned files and the sentinel, and +keeps the game root plus every unknown file or directory. Recovery reads app-state `install_intent.json` and combines the recorded intent with the observed `local/`, `.local.installing/`, and `.local.backup/` state. @@ -214,6 +216,8 @@ Most scans become O(number of game dirs), with full recursion only when needed. `ValidatedDownloadManifest` before any destination mutation. It contains canonical game-root-relative paths and rejects aliases, reserved state, shape conflicts, and bounded-size violations as one unit. +- A new manifest target that already exists without prior committed ownership is + rejected before the sentinel, ownership journal, or payload is mutated. - Download mutation holds a capability handle for the direct catalog game root. Directory components and final files are reopened relative to that handle without following links or Windows reparse points; chunk writes and checks use diff --git a/crates/lanspread-peer/README.md b/crates/lanspread-peer/README.md index b6b7aef..bbf7a17 100644 --- a/crates/lanspread-peer/README.md +++ b/crates/lanspread-peer/README.md @@ -127,22 +127,22 @@ as the single source of truth for whether a download is still running. ### Install Transactions -Install, update, uninstall, downloaded-file removal, and startup recovery live -under `src/install/`. Install-side operation intent is stored atomically under -the configured peer state directory, at `games//install_intent.json`. -Game roots still use Lanspread-owned `.local.installing/` and `.local.backup/` +Install, update, uninstall, and install-side startup recovery live under +`src/install/`. Install-side operation intent is stored atomically under the +configured peer state directory, at `games//install_intent.json`. Game +roots still use Lanspread-owned `.local.installing/` and `.local.backup/` directories marked by `.lanspread_owned`. Startup recovery combines the recorded intent with the observed filesystem state and only deletes reserved directories -when intent or marker ownership proves they belong to Lanspread. Downloaded-file -removal is deliberately separate from uninstall: it only accepts catalog IDs -that are direct children of the configured game directory, refuses installed or -in-flight roots, and deletes the whole game root only after finding a regular -root-level `version.ini` sentinel. +when intent or marker ownership proves they belong to Lanspread. Download provenance is stored separately at `games//download_ownership.json` in the peer state directory. It is bound to the canonical configured games directory so switching library roots -cannot make an old record authorize deletion in a different tree. +cannot make an old record authorize deletion in a different tree. Downloaded- +file removal is deliberately separate from uninstall: it refuses installed or +in-flight roots, journals an empty pending generation, and deletes only the +regular sentinel plus paths proven by the last committed ownership set. Unknown +files, directories, and the game root remain untouched. Legacy launcher-owned files in game directories are migrated by a dedicated pre-start phase. Normal install, recovery, scan, and transfer paths use only the diff --git a/crates/lanspread-peer/src/download/confined_fs.rs b/crates/lanspread-peer/src/download/confined_fs.rs index 35ce9cf..eec6604 100644 --- a/crates/lanspread-peer/src/download/confined_fs.rs +++ b/crates/lanspread-peer/src/download/confined_fs.rs @@ -142,7 +142,8 @@ impl ConfinedGameRoot { } for parent in parent_directories { let parent = ValidatedDownloadPath::from_ownership(&parent)?; - root.open_directory_blocking(&parent, false)?.sync_all()?; + let directory = root.open_directory_blocking(&parent, false)?; + sync_directory_handle(&directory)?; } sync_directory_handle(&root.inner.game_root)?; Ok(()) @@ -164,6 +165,20 @@ impl ConfinedGameRoot { .await? } + pub(super) async fn reject_existing_unowned_files( + &self, + paths: Vec, + ) -> eyre::Result<()> { + let root = self.clone(); + tokio::task::spawn_blocking(move || { + for path in &paths { + root.reject_existing_unowned_file_blocking(path)?; + } + Ok(()) + }) + .await? + } + pub(super) async fn root_regular_file_exists(&self, name: &'static str) -> eyre::Result { let root = self.clone(); tokio::task::spawn_blocking( @@ -182,6 +197,18 @@ impl ConfinedGameRoot { .await? } + pub(super) async fn root_entry_exists(&self, name: &'static str) -> eyre::Result { + let root = self.clone(); + tokio::task::spawn_blocking(move || { + match fs::stat(&root.inner.game_root, Path::new(name), FollowSymlinks::No) { + Ok(_) => Ok(true), + Err(error) if error.kind() == ErrorKind::NotFound => Ok(false), + Err(error) => Err(error.into()), + } + }) + .await? + } + pub(super) async fn create_new_root_file(&self, name: &'static str) -> eyre::Result { let root = self.clone(); tokio::task::spawn_blocking(move || root.create_new_root_file_blocking(name)).await? @@ -190,6 +217,20 @@ impl ConfinedGameRoot { pub(super) async fn remove_root_file_if_exists(&self, name: &'static str) -> eyre::Result<()> { let root = self.clone(); tokio::task::spawn_blocking(move || { + #[cfg(windows)] + match root.inspect_root_regular_file_blocking(name) { + Ok(file) => { + make_windows_file_removable(&root.inner.game_root, Path::new(name), &file)? + } + Err(error) + if error + .downcast_ref::() + .is_some_and(|error| error.kind() == ErrorKind::NotFound) => + { + return Ok(()); + } + Err(error) => return Err(error), + } match fs::remove_file(&root.inner.game_root, Path::new(name)) { Ok(()) => { sync_directory_handle(&root.inner.game_root)?; @@ -305,24 +346,56 @@ impl ConfinedGameRoot { return Ok(()); } - // Opening and inspecting the same object before unlink catches Windows - // reparse points that are not reported as ordinary symlinks. - let file = match inspect_regular_file_at(&parent, leaf) { - Ok(file) => file, - Err(error) if is_preservable_shape_error(&error) => { - log::warn!( - "Preserving owned path whose final object changed at {}/{}", - self.inner.display_path.display(), - path.canonical() - ); + // Windows has non-symlink reparse points that require same-handle + // inspection. On Unix, opening merely to unlink would incorrectly make + // cleanup depend on the owned file's read permission. + #[cfg(windows)] + { + let file = match inspect_regular_file_at(&parent, leaf) { + Ok(file) => file, + Err(error) if is_preservable_shape_error(&error) => { + log::warn!( + "Preserving owned path whose final object changed at {}/{}", + self.inner.display_path.display(), + path.canonical() + ); + return Ok(()); + } + Err(error) => return Err(error), + }; + make_windows_file_removable(&parent, Path::new(leaf), &file)?; + drop(file); + } + fs::remove_file(&parent, Path::new(leaf))?; + sync_directory_handle(&parent)?; + Ok(()) + } + + fn reject_existing_unowned_file_blocking( + &self, + path: &ValidatedDownloadPath, + ) -> eyre::Result<()> { + let (parent, leaf) = match self.open_parent_blocking(path, false) { + Ok(value) => value, + Err(error) + if error + .downcast_ref::() + .is_some_and(|error| error.kind() == ErrorKind::NotFound) => + { return Ok(()); } Err(error) => return Err(error), }; - drop(file); - fs::remove_file(&parent, Path::new(leaf))?; - sync_directory_handle(&parent)?; - Ok(()) + + match fs::stat(&parent, Path::new(leaf), FollowSymlinks::No) { + Err(error) if error.kind() == ErrorKind::NotFound => Ok(()), + Err(error) => Err(error.into()), + Ok(_) => eyre::bail!( + "refusing to replace untracked file at {}/{}", + self.inner.display_path.display(), + path.canonical() + ), + } } fn open_parent_for_cleanup_blocking<'a>( @@ -410,6 +483,7 @@ fn open_regular_file_at(parent: &File, leaf: &str, create: bool) -> eyre::Result Ok(file) } +#[cfg(windows)] fn inspect_regular_file_at(parent: &File, leaf: &str) -> eyre::Result { let options = inspection_file_options(); let file = fs::open(parent, Path::new(leaf), &options)?; @@ -477,6 +551,20 @@ fn reject_windows_reparse(metadata: &std::fs::Metadata, display: &str) -> std::i Ok(()) } +#[cfg(windows)] +fn make_windows_file_removable(parent: &File, path: &Path, file: &File) -> std::io::Result<()> { + let mut permissions = file.metadata()?.permissions(); + if permissions.readonly() { + permissions.set_readonly(false); + fs::set_symlink_permissions( + parent, + path, + cap_primitives::fs::Permissions::from_std(permissions), + )?; + } + Ok(()) +} + #[cfg(not(windows))] #[allow(clippy::unnecessary_wraps)] const fn reject_windows_reparse( @@ -486,11 +574,15 @@ const fn reject_windows_reparse( Ok(()) } +#[cfg(windows)] fn is_preservable_shape_error(error: &eyre::Report) -> bool { error.downcast_ref::().is_some_and(|error| { matches!( error.kind(), - ErrorKind::NotFound | ErrorKind::NotADirectory | ErrorKind::InvalidInput + ErrorKind::NotFound + | ErrorKind::NotADirectory + | ErrorKind::InvalidInput + | ErrorKind::FilesystemLoop ) }) } @@ -668,4 +760,25 @@ mod tests { assert!(!payload.exists()); } + + #[cfg(unix)] + #[tokio::test] + async fn cleanup_does_not_require_read_access_to_owned_files() { + use std::os::unix::fs::PermissionsExt as _; + + let games = TempDir::new("lanspread-confined-mode-zero-cleanup"); + let root = ConfinedGameRoot::open_or_create(games.path(), "game") + .await + .expect("game root should open"); + let payload = games.game_root().join("payload.bin"); + std::fs::write(&payload, b"owned").expect("payload should be written"); + std::fs::set_permissions(&payload, std::fs::Permissions::from_mode(0o000)) + .expect("payload permissions should be removed"); + + root.remove_owned_regular_files(vec![path("payload.bin")]) + .await + .expect("mode-zero owned file should be removable by its parent owner"); + + assert!(!payload.exists()); + } } diff --git a/crates/lanspread-peer/src/download/manifest.rs b/crates/lanspread-peer/src/download/manifest.rs index 5783c61..c31347f 100644 --- a/crates/lanspread-peer/src/download/manifest.rs +++ b/crates/lanspread-peer/src/download/manifest.rs @@ -382,7 +382,7 @@ enum EntryShape { Directory, } -fn validate_game_id(game_id: &str) -> eyre::Result<()> { +pub(super) fn validate_game_id(game_id: &str) -> eyre::Result<()> { if game_id.contains('/') || game_id.contains('\\') { eyre::bail!("catalog game ID must be one path component: {game_id}"); } @@ -406,7 +406,7 @@ fn validate_protocol_game_id( Ok(()) } -fn canonical_games_folder(games_folder: &Path) -> eyre::Result { +pub(super) fn canonical_games_folder(games_folder: &Path) -> eyre::Result { if !games_folder.is_absolute() { eyre::bail!( "configured games directory must be absolute: {}", @@ -485,7 +485,10 @@ fn validate_component(component: &str) -> eyre::Result<()> { }) { eyre::bail!("download path component is not portable: {component}"); } - let device_stem = component.split('.').next().unwrap_or_default(); + // Match the Windows path backend's device-name normalization. Windows + // ignores trailing Unicode whitespace in the stem before the first dot, + // and reserves the zero-numbered serial/parallel aliases as well. + let device_stem = component.split('.').next().unwrap_or_default().trim_end(); if is_windows_device_name(device_stem) { eyre::bail!("download path uses a Windows device name: {component}"); } @@ -507,7 +510,7 @@ fn is_windows_device_name(stem: &str) -> bool { .strip_prefix("COM") .or_else(|| upper.strip_prefix("LPT")) .is_some_and(|number| { - (number.len() == 1 && matches!(number.as_bytes()[0], b'1'..=b'9')) + (number.len() == 1 && number.as_bytes()[0].is_ascii_digit()) || matches!(number, "¹" | "²" | "³") }) } @@ -812,8 +815,12 @@ mod tests { let temp = TempDir::new("lanspread-manifest-portability"); for path in [ "game/.ſync/state", + "game/COM0", + "game/LPT0.txt", "game/COM¹.txt", "game/LPT³.log", + "game/CON .txt", + "game/CON\u{00a0}", "game/LOCAL~1/save.dat", ] { let descriptions = vec![file(path, 1), file("game/version.ini", 8)]; @@ -985,6 +992,8 @@ mod tests { vec![file("game/version.ini", 8), file("other/local/save.dat", 1)], vec![file("game/version.ini", 8), file("game/local/save.dat", 1)], vec![file("game/version.ini", 8), file("game/.sync/state", 1)], + vec![file("game/version.ini", 8), file("game/COM0", 1)], + vec![file("game/version.ini", 8), file("game/CON\u{00a0}", 1)], vec![file("game/version.ini", 8), file("game/../escape", 1)], vec![ file("game/version.ini", 8), diff --git a/crates/lanspread-peer/src/download/mod.rs b/crates/lanspread-peer/src/download/mod.rs index 6afad8b..5a2cea3 100644 --- a/crates/lanspread-peer/src/download/mod.rs +++ b/crates/lanspread-peer/src/download/mod.rs @@ -13,4 +13,6 @@ mod version_ini; pub(crate) use manifest::{ValidatedDownloadManifest, validate_protocol_v7_descriptions}; pub(crate) use orchestrator::download_game_files; -pub(crate) use ownership::{clear_download_ownership, recover_incomplete_download}; +#[cfg(test)] +pub(crate) use ownership::seed_download_ownership_for_test; +pub(crate) use ownership::{recover_incomplete_download, remove_downloaded_payload}; diff --git a/crates/lanspread-peer/src/download/ownership.rs b/crates/lanspread-peer/src/download/ownership.rs index b78e050..6db95ae 100644 --- a/crates/lanspread-peer/src/download/ownership.rs +++ b/crates/lanspread-peer/src/download/ownership.rs @@ -17,6 +17,8 @@ use super::{ MAX_DOWNLOAD_RELATIVE_PATH_BYTES, ValidatedDownloadManifest, ValidatedDownloadPath, + canonical_games_folder, + validate_game_id, validate_owned_file_path, }, version_ini::{ @@ -25,7 +27,10 @@ use super::{ restore_unjournaled_version_ini_transaction, }, }; -use crate::state_paths::{download_ownership_path, download_ownership_tmp_path}; +use crate::{ + game_paths::{BACKUP_DIR, INSTALLING_DIR, LOCAL_DIR, VERSION_INI}, + state_paths::{download_ownership_path, download_ownership_tmp_path}, +}; const OWNERSHIP_SCHEMA_VERSION: u32 = 1; const MAX_OWNERSHIP_RECORD_BYTES: u64 = 128 * 1024 * 1024; @@ -122,21 +127,29 @@ impl DownloadOwnershipTransaction { let record_path = download_ownership_path(state_dir, manifest.game_id()); let tmp_path = download_ownership_tmp_path(state_dir, manifest.game_id()); - let previous = match load_record(&record_path, manifest.game_id(), &games_folder_key).await - { - LoadedOwnership::Valid(record) => record.committed_files.into_iter().collect(), - LoadedOwnership::Missing | LoadedOwnership::Invalid => { - let baseline = - DownloadOwnershipRecord::empty(manifest.game_id(), &games_folder_key); - require_durable_record( - write_record(&record_path, &tmp_path, &baseline).await?, - "download ownership baseline", - )?; - BTreeSet::new() - } - }; + let (previous, needs_baseline) = + match load_record(&record_path, manifest.game_id(), &games_folder_key).await { + LoadedOwnership::Valid(record) => { + (record.committed_files.into_iter().collect(), false) + } + LoadedOwnership::Missing | LoadedOwnership::Invalid => (BTreeSet::new(), true), + }; let current = manifest.owned_file_paths().into_iter().collect(); validate_generation_aliases(&previous, ¤t)?; + let untracked_targets = current + .difference(&previous) + .map(|path| ValidatedDownloadPath::from_ownership(path)) + .collect::>>()?; + game_root + .reject_existing_unowned_files(untracked_targets) + .await?; + if needs_baseline { + let baseline = DownloadOwnershipRecord::empty(manifest.game_id(), &games_folder_key); + require_durable_record( + write_record(&record_path, &tmp_path, &baseline).await?, + "download ownership baseline", + )?; + } Ok(Self { record_path, @@ -149,6 +162,73 @@ impl DownloadOwnershipTransaction { }) } + async fn prepare_removal( + games_folder: &Path, + state_dir: &Path, + game_id: &str, + ) -> eyre::Result> { + validate_game_id(game_id)?; + let games_folder_path = games_folder.to_path_buf(); + let games_folder = + tokio::task::spawn_blocking(move || canonical_games_folder(&games_folder_path)) + .await??; + let games_folder_key = games_folder_key(&games_folder); + let record_path = download_ownership_path(state_dir, game_id); + let tmp_path = download_ownership_tmp_path(state_dir, game_id); + let Some(game_root) = ConfinedGameRoot::open_existing(&games_folder, game_id).await? else { + let empty = DownloadOwnershipRecord::empty(game_id, &games_folder_key); + require_durable_record( + write_record(&record_path, &tmp_path, &empty).await?, + "absent downloaded-game ownership", + )?; + return Ok(None); + }; + + recover_incomplete_download_with_root(&game_root, state_dir, game_id, &games_folder_key) + .await?; + let record = match load_record(&record_path, game_id, &games_folder_key).await { + LoadedOwnership::Valid(record) => record, + LoadedOwnership::Missing => { + eyre::bail!( + "cannot safely remove downloaded files for {game_id}: the ownership record is missing; move or delete the legacy game folder manually" + ); + } + LoadedOwnership::Invalid => { + eyre::bail!( + "cannot safely remove downloaded files for {game_id}: the ownership record is invalid; move or delete the game folder manually" + ); + } + }; + if record.pending_files.is_some() { + eyre::bail!("download ownership recovery did not settle for {game_id}"); + } + if !game_root.root_regular_file_exists(VERSION_INI).await? { + if record.committed_files.is_empty() { + return Ok(None); + } + eyre::bail!("download sentinel is missing for {game_id}"); + } + for (name, label) in [ + (LOCAL_DIR, "local install"), + (INSTALLING_DIR, "install staging"), + (BACKUP_DIR, "install backup"), + ] { + if game_root.root_entry_exists(name).await? { + eyre::bail!("refusing to remove downloaded files for {game_id} with {label}"); + } + } + + Ok(Some(Self { + record_path, + tmp_path, + game_id: game_id.to_owned(), + games_folder_key, + game_root, + previous: record.committed_files.into_iter().collect(), + current: BTreeSet::new(), + })) + } + /// Publishes the proposed set after the old sentinel has been parked. pub(super) async fn journal_pending(&self) -> eyre::Result { let record = DownloadOwnershipRecord { @@ -203,6 +283,60 @@ impl DownloadOwnershipTransaction { } } +/// Removes only files proven to belong to a completed peer download. +/// +/// An empty pending ownership generation is the durable removal intent. That +/// lets normal startup recovery finish an interrupted removal without ever +/// recursively deleting the game root or unknown files. +pub(crate) async fn remove_downloaded_payload( + games_folder: &Path, + state_dir: &Path, + game_id: &str, +) -> eyre::Result<()> { + let Some(transaction) = + DownloadOwnershipTransaction::prepare_removal(games_folder, state_dir, game_id).await? + else { + return Ok(()); + }; + + if let Err(error) = + super::version_ini::begin_version_ini_transaction(&transaction.game_root).await + { + if let Err(restore_error) = + restore_unjournaled_version_ini_transaction(&transaction.game_root).await + { + return Err(error.wrap_err(format!( + "sentinel parking failed and rollback also failed: {restore_error}" + ))); + } + return Err(error); + } + match transaction.journal_pending().await { + Ok(OwnershipJournalPublication::Durable) => {} + Ok(OwnershipJournalPublication::NeedsRecovery(error)) => { + return Err(eyre::eyre!( + "download removal ownership was renamed but its durability could not be established: {error}" + )); + } + Err(error) => { + if let Err(restore_error) = + restore_unjournaled_version_ini_transaction(&transaction.game_root).await + { + return Err(error.wrap_err(format!( + "removal ownership journal failed and sentinel restore also failed: {restore_error}" + ))); + } + return Err(error); + } + } + + // From the durable empty pending generation onward, recovery must roll the + // removal forward. Never restore the sentinel on a later failure. + transaction.remove_stale().await?; + discard_version_ini_transaction(&transaction.game_root).await?; + transaction.finalize().await +} + /// Recovers the ownership/version transaction for one inactive game root. pub(crate) async fn recover_incomplete_download( game_root: &Path, @@ -298,19 +432,6 @@ async fn recover_incomplete_download_with_root( Ok(()) } -/// Clears provenance after an explicit downloaded-game removal succeeds. -pub(crate) async fn clear_download_ownership(state_dir: &Path, game_id: &str) -> eyre::Result<()> { - let path = download_ownership_path(state_dir, game_id); - remove_file_if_exists(&download_ownership_tmp_path(state_dir, game_id)).await?; - remove_file_if_exists(&path).await?; - if let Err(error) = sync_parent_dir(&path) - && error.kind() != ErrorKind::NotFound - { - return Err(error.into()); - } - Ok(()) -} - fn validate_file_set(paths: &[String]) -> eyre::Result<()> { if paths.len() > MAX_DOWNLOAD_MANIFEST_ENTRIES { eyre::bail!("download ownership has too many paths"); @@ -507,6 +628,34 @@ async fn sweep_tmp_file(path: &Path) { } } +#[cfg(test)] +pub(crate) async fn seed_download_ownership_for_test( + state_dir: &Path, + games_folder: &Path, + game_id: &str, + committed_files: &[&str], +) { + let games_folder = canonical_games_folder(games_folder).expect("games folder should resolve"); + let record = DownloadOwnershipRecord { + schema_version: OWNERSHIP_SCHEMA_VERSION, + game_id: game_id.to_owned(), + games_folder_key: games_folder_key(&games_folder), + committed_files: committed_files.iter().map(ToString::to_string).collect(), + pending_files: None, + }; + require_durable_record( + write_record( + &download_ownership_path(state_dir, game_id), + &download_ownership_tmp_path(state_dir, game_id), + &record, + ) + .await + .expect("test ownership should publish"), + "test ownership", + ) + .expect("test ownership should be durable"); +} + #[cfg(unix)] fn games_folder_key(path: &Path) -> String { use std::os::unix::ffi::OsStrExt; @@ -707,6 +856,38 @@ mod tests { assert!(valid.validate("game", "other-root").is_err()); } + #[tokio::test] + async fn untracked_exact_manifest_target_is_rejected_without_mutation() { + let games = TempDir::new("lanspread-ownership-untracked-games"); + let state = TempDir::new("lanspread-ownership-untracked-state"); + let root = games.game_root(); + write_file(&root.join(VERSION_INI), b"20240101"); + write_file(&root.join("archive.eti"), b"user-bytes"); + write_file(&root.join("notes.txt"), b"user-note"); + let manifest = manifest(games.path(), &["archive.eti"]); + let confined_root = confined_root(&manifest).await; + + let error = DownloadOwnershipTransaction::prepare(state.path(), &manifest, &confined_root) + .await + .expect_err("an untracked exact target must be preserved"); + + assert!(error.to_string().contains("untracked file")); + assert_eq!( + std::fs::read(root.join(VERSION_INI)).expect("sentinel should remain readable"), + b"20240101" + ); + assert_eq!( + std::fs::read(root.join("archive.eti")).expect("target should remain readable"), + b"user-bytes" + ); + assert_eq!( + std::fs::read(root.join("notes.txt")).expect("user note should remain readable"), + b"user-note" + ); + assert!(!root.join(VERSION_DISCARDED_FILE).exists()); + assert!(!download_ownership_path(state.path(), "game").exists()); + } + #[tokio::test] async fn replacement_and_abort_touch_only_explicitly_owned_paths() { let games = TempDir::new("lanspread-ownership-replace-games"); @@ -716,7 +897,6 @@ mod tests { ("keep.eti", b"keep".as_slice()), ("stale.eti", b"old".as_slice()), ("nested/stale.bin", b"old".as_slice()), - ("new.eti", b"new".as_slice()), ("notes.txt", b"user".as_slice()), ("local/save.dat", b"save".as_slice()), ] { @@ -780,6 +960,144 @@ mod tests { assert!(record.pending_files.is_none()); } + #[tokio::test] + async fn removal_deletes_only_owned_payload_and_keeps_an_empty_journal() { + let games = TempDir::new("lanspread-ownership-remove-games"); + let state = TempDir::new("lanspread-ownership-remove-state"); + let root = games.game_root(); + for (path, bytes) in [ + (VERSION_INI, b"20240101".as_slice()), + ("archive.eti", b"owned".as_slice()), + ("nested/owned.bin", b"owned".as_slice()), + ("notes.txt", b"user".as_slice()), + ("nested/user.txt", b"user".as_slice()), + ] { + write_file(&root.join(path), bytes); + } + seed_record( + state.path(), + games.path(), + &["archive.eti", "nested/owned.bin"], + None, + ) + .await; + + remove_downloaded_payload(games.path(), state.path(), "game") + .await + .expect("owned download should be removable"); + + assert!(root.is_dir()); + assert!(!root.join(VERSION_INI).exists()); + assert!(!root.join("archive.eti").exists()); + assert!(!root.join("nested/owned.bin").exists()); + assert_eq!( + std::fs::read(root.join("notes.txt")).expect("user file should remain"), + b"user" + ); + assert_eq!( + std::fs::read(root.join("nested/user.txt")).expect("nested user file should remain"), + b"user" + ); + let record = read_valid_record(state.path(), games.path()).await; + assert!(record.committed_files.is_empty()); + assert!(record.pending_files.is_none()); + + remove_downloaded_payload(games.path(), state.path(), "game") + .await + .expect("completed removal should be idempotent"); + assert_eq!( + std::fs::read(root.join("notes.txt")).expect("retry must preserve user file"), + b"user" + ); + } + + #[tokio::test] + async fn removal_without_trustworthy_ownership_is_zero_mutation() { + for invalid_record in [None, Some(b"not-json".as_slice())] { + let games = TempDir::new("lanspread-ownership-remove-untracked-games"); + let state = TempDir::new("lanspread-ownership-remove-untracked-state"); + let root = games.game_root(); + write_file(&root.join(VERSION_INI), b"20240101"); + write_file(&root.join("archive.eti"), b"ambiguous"); + write_file(&root.join("notes.txt"), b"user"); + if let Some(bytes) = invalid_record { + write_file(&download_ownership_path(state.path(), "game"), bytes); + } + + let error = remove_downloaded_payload(games.path(), state.path(), "game") + .await + .expect_err("ambiguous payload must not be removed"); + + assert!(error.to_string().contains("cannot safely remove")); + assert_eq!( + std::fs::read(root.join(VERSION_INI)).expect("sentinel should remain"), + b"20240101" + ); + assert_eq!( + std::fs::read(root.join("archive.eti")).expect("payload should remain"), + b"ambiguous" + ); + assert_eq!( + std::fs::read(root.join("notes.txt")).expect("user file should remain"), + b"user" + ); + assert!(!root.join(VERSION_DISCARDED_FILE).exists()); + } + } + + #[tokio::test] + async fn removal_refuses_install_state_before_parking_the_sentinel() { + for protected in [LOCAL_DIR, INSTALLING_DIR, BACKUP_DIR] { + let games = TempDir::new("lanspread-ownership-remove-installed-games"); + let state = TempDir::new("lanspread-ownership-remove-installed-state"); + let root = games.game_root(); + write_file(&root.join(VERSION_INI), b"20240101"); + write_file(&root.join("archive.eti"), b"owned"); + write_file(&root.join(protected).join("canary"), b"protected"); + seed_record(state.path(), games.path(), &["archive.eti"], None).await; + + assert!( + remove_downloaded_payload(games.path(), state.path(), "game") + .await + .is_err() + ); + assert!(root.join(VERSION_INI).is_file()); + assert!(root.join("archive.eti").is_file()); + assert_eq!( + std::fs::read(root.join(protected).join("canary")) + .expect("protected state should remain"), + b"protected" + ); + assert!(!root.join(VERSION_DISCARDED_FILE).exists()); + } + } + + #[tokio::test] + async fn recovery_rolls_a_durable_removal_intent_forward() { + let games = TempDir::new("lanspread-ownership-remove-recovery-games"); + let state = TempDir::new("lanspread-ownership-remove-recovery-state"); + let root = games.game_root(); + write_file(&root.join(VERSION_DISCARDED_FILE), b"20240101"); + write_file(&root.join("archive.eti"), b"owned"); + write_file(&root.join("notes.txt"), b"user"); + seed_record(state.path(), games.path(), &["archive.eti"], Some(&[])).await; + + recover_incomplete_download(&root, state.path(), "game") + .await + .expect("removal should recover"); + + assert!(!root.join(VERSION_INI).exists()); + assert!(!root.join(VERSION_DISCARDED_FILE).exists()); + assert!(!root.join("archive.eti").exists()); + assert_eq!( + std::fs::read(root.join("notes.txt")).expect("user file should remain"), + b"user" + ); + let record = read_valid_record(state.path(), games.path()).await; + assert!(record.committed_files.is_empty()); + assert!(record.pending_files.is_none()); + } + #[tokio::test] async fn recovery_handles_every_durable_journal_state() { // Crash after parking the old sentinel but before publishing pending. @@ -945,6 +1263,32 @@ mod tests { assert!(!root.join("archive.eti").exists()); } + #[tokio::test] + async fn committed_path_ownership_survives_external_root_recreation() { + let games = TempDir::new("lanspread-ownership-recreated-root-games"); + let state = TempDir::new("lanspread-ownership-recreated-root-state"); + let root = games.game_root(); + write_file(&root.join(VERSION_INI), b"20240101"); + write_file(&root.join("archive.eti"), b"old-owned"); + seed_record(state.path(), games.path(), &["archive.eti"], None).await; + + std::fs::remove_dir_all(&root).expect("old game root should be removed externally"); + write_file(&root.join(VERSION_INI), b"20240101"); + write_file(&root.join("archive.eti"), b"replacement"); + + let manifest = manifest(games.path(), &["archive.eti"]); + let confined_root = confined_root(&manifest).await; + DownloadOwnershipTransaction::prepare(state.path(), &manifest, &confined_root) + .await + .expect("path ownership deliberately survives local root replacement"); + + assert_eq!( + std::fs::read(root.join("archive.eti")).expect("prepare must not mutate the target"), + b"replacement" + ); + assert!(root.join(VERSION_INI).is_file()); + } + #[tokio::test] async fn corrupt_or_wrong_root_state_never_authorizes_payload_deletion() { let games_a = TempDir::new("lanspread-ownership-binding-a"); diff --git a/crates/lanspread-peer/src/handlers.rs b/crates/lanspread-peer/src/handlers.rs index 132a423..24b7f32 100644 --- a/crates/lanspread-peer/src/handlers.rs +++ b/crates/lanspread-peer/src/handlers.rs @@ -19,8 +19,8 @@ use crate::{ context::{Ctx, OperationGuard, OperationKind}, download::{ ValidatedDownloadManifest, - clear_download_ownership, download_game_files, + remove_downloaded_payload, validate_protocol_v7_descriptions, }, events, @@ -1097,11 +1097,7 @@ async fn run_remove_downloaded_operation( ctx.active_operations.clone(), tx_notify_ui.clone(), ); - let result = async { - install::remove_downloaded(&game_dir, &id).await?; - clear_download_ownership(ctx.state_dir.as_ref(), &id).await - } - .await; + let result = remove_downloaded_payload(&game_dir, ctx.state_dir.as_ref(), &id).await; match result { Ok(()) => { @@ -2288,6 +2284,13 @@ mod tests { write_file(&root.join("game.eti"), b"archive"); let ctx = test_ctx(temp.path().to_path_buf()); + crate::download::seed_download_ownership_for_test( + ctx.state_dir.as_ref(), + temp.path(), + "game", + &["game.eti"], + ) + .await; let (tx, mut rx) = mpsc::unbounded_channel(); let catalog = ctx.catalog.read().await.clone(); let scan = scan_local_library(temp.path(), ctx.state_dir.as_ref(), &catalog) @@ -2311,7 +2314,9 @@ mod tests { recv_event(&mut rx).await, PeerEvent::RemoveDownloadedGameFinished { id } if id == "game" )); - assert!(!root.exists()); + assert!(root.is_dir()); + assert!(!root.join("version.ini").exists()); + assert!(!root.join("game.eti").exists()); assert!(ctx.active_operations.read().await.is_empty()); } diff --git a/crates/lanspread-peer/src/install/mod.rs b/crates/lanspread-peer/src/install/mod.rs index e0c404b..0e45d3e 100644 --- a/crates/lanspread-peer/src/install/mod.rs +++ b/crates/lanspread-peer/src/install/mod.rs @@ -1,9 +1,7 @@ pub(crate) mod intent; -mod remove; mod transaction; pub mod unpack; -pub use remove::remove_downloaded; pub(crate) use transaction::root_eti_archives; pub use transaction::{ StreamedInstallTransaction, diff --git a/crates/lanspread-peer/src/install/remove.rs b/crates/lanspread-peer/src/install/remove.rs deleted file mode 100644 index 14b11f2..0000000 --- a/crates/lanspread-peer/src/install/remove.rs +++ /dev/null @@ -1,209 +0,0 @@ -use std::{ - ffi::OsStr, - io::ErrorKind, - path::{Component, Path, PathBuf}, -}; - -use eyre::{WrapErr, bail}; - -use crate::game_paths::{BACKUP_DIR, INSTALLING_DIR, LOCAL_DIR, VERSION_INI}; - -/// Remove the downloaded files for an uninstalled game root. -/// -/// This is intentionally stricter than the scanner: callers must pass a catalog -/// id that is a single path component, the target must be a direct child of the -/// configured game directory, and the root must still look like a downloaded -/// but uninstalled game immediately before recursive deletion. -pub async fn remove_downloaded(game_dir: &Path, id: &str) -> eyre::Result<()> { - validate_game_id(id)?; - - let game_dir = canonical_game_dir(game_dir).await?; - let game_root = game_dir.join(id); - let Some(root_metadata) = symlink_metadata_if_exists(&game_root).await? else { - return Ok(()); - }; - - if root_metadata.file_type().is_symlink() { - bail!( - "refusing to remove symlink game root {}", - game_root.display() - ); - } - if !root_metadata.is_dir() { - bail!( - "refusing to remove non-directory game root {}", - game_root.display() - ); - } - - let game_root = tokio::fs::canonicalize(&game_root) - .await - .wrap_err_with(|| format!("failed to canonicalize {}", game_root.display()))?; - ensure_direct_child(&game_dir, &game_root, id)?; - ensure_downloaded_uninstalled_root(&game_root).await?; - - tokio::fs::remove_dir_all(&game_root) - .await - .wrap_err_with(|| format!("failed to remove downloaded game {}", game_root.display())) -} - -fn validate_game_id(id: &str) -> eyre::Result<()> { - let mut components = Path::new(id).components(); - match (components.next(), components.next()) { - (Some(Component::Normal(_)), None) => Ok(()), - _ => bail!("refusing to remove invalid game id {id:?}"), - } -} - -async fn canonical_game_dir(game_dir: &Path) -> eyre::Result { - let game_dir = tokio::fs::canonicalize(game_dir) - .await - .wrap_err_with(|| format!("failed to canonicalize game dir {}", game_dir.display()))?; - let metadata = tokio::fs::metadata(&game_dir).await?; - if !metadata.is_dir() { - bail!("game dir is not a directory: {}", game_dir.display()); - } - Ok(game_dir) -} - -fn ensure_direct_child(game_dir: &Path, game_root: &Path, id: &str) -> eyre::Result<()> { - if game_root == game_dir - || game_root.parent() != Some(game_dir) - || game_root.file_name() != Some(OsStr::new(id)) - { - bail!( - "refusing to remove game root outside direct game-dir child: {}", - game_root.display() - ); - } - Ok(()) -} - -async fn ensure_downloaded_uninstalled_root(game_root: &Path) -> eyre::Result<()> { - let version_path = game_root.join(VERSION_INI); - let version_metadata = tokio::fs::symlink_metadata(&version_path) - .await - .wrap_err_with(|| format!("download sentinel is missing: {}", version_path.display()))?; - if version_metadata.file_type().is_symlink() || !version_metadata.is_file() { - bail!( - "refusing to remove game without a regular version.ini sentinel: {}", - game_root.display() - ); - } - - ensure_absent(&game_root.join(LOCAL_DIR), "local install").await?; - ensure_absent(&game_root.join(INSTALLING_DIR), "install staging").await?; - ensure_absent(&game_root.join(BACKUP_DIR), "install backup").await -} - -async fn ensure_absent(path: &Path, label: &str) -> eyre::Result<()> { - if symlink_metadata_if_exists(path).await?.is_some() { - bail!( - "refusing to remove game root with {label}: {}", - path.display() - ); - } - Ok(()) -} - -async fn symlink_metadata_if_exists(path: &Path) -> eyre::Result> { - match tokio::fs::symlink_metadata(path).await { - Ok(metadata) => Ok(Some(metadata)), - Err(err) if err.kind() == ErrorKind::NotFound => Ok(None), - Err(err) => Err(err.into()), - } -} - -#[cfg(test)] -mod tests { - use std::path::Path; - - use super::*; - use crate::test_support::TempDir; - - fn write_file(path: &Path, bytes: &[u8]) { - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).expect("parent dir should be created"); - } - std::fs::write(path, bytes).expect("file should be written"); - } - - #[tokio::test] - async fn remove_downloaded_deletes_only_requested_game_root() { - let temp = TempDir::new("lanspread-remove-download"); - let root = temp.game_root(); - let sibling = temp.path().join("sibling"); - write_file(&root.join("version.ini"), b"20250101"); - write_file(&root.join("game.eti"), b"archive"); - write_file(&sibling.join("version.ini"), b"20250101"); - - remove_downloaded(temp.path(), "game") - .await - .expect("downloaded game root should be removed"); - - assert!(!root.exists()); - assert!(sibling.join("version.ini").is_file()); - } - - #[tokio::test] - async fn remove_downloaded_refuses_installed_game() { - let temp = TempDir::new("lanspread-remove-installed"); - let root = temp.game_root(); - write_file(&root.join("version.ini"), b"20250101"); - write_file(&root.join("local").join("payload.txt"), b"installed"); - - let err = remove_downloaded(temp.path(), "game") - .await - .expect_err("installed game must not be removed"); - - assert!(err.to_string().contains("local install")); - assert!(root.join("version.ini").is_file()); - assert!(root.join("local").join("payload.txt").is_file()); - } - - #[tokio::test] - async fn remove_downloaded_refuses_missing_download_sentinel() { - let temp = TempDir::new("lanspread-remove-missing-sentinel"); - let root = temp.game_root(); - write_file(&root.join("game.eti"), b"archive"); - - let err = remove_downloaded(temp.path(), "game") - .await - .expect_err("undownloaded game root must not be removed"); - - assert!(err.to_string().contains("download sentinel is missing")); - assert!(root.join("game.eti").is_file()); - } - - #[tokio::test] - async fn remove_downloaded_rejects_path_traversal_id() { - let temp = TempDir::new("lanspread-remove-traversal"); - let root = temp.game_root(); - write_file(&root.join("version.ini"), b"20250101"); - - let err = remove_downloaded(temp.path(), "../game") - .await - .expect_err("path traversal id must be rejected"); - - assert!(err.to_string().contains("invalid game id")); - assert!(root.join("version.ini").is_file()); - } - - #[cfg(unix)] - #[tokio::test] - async fn remove_downloaded_refuses_symlink_game_root() { - use std::os::unix::fs::symlink; - - let temp = TempDir::new("lanspread-remove-symlink"); - let outside = temp.path().join("outside"); - write_file(&outside.join("version.ini"), b"20250101"); - symlink(&outside, temp.game_root()).expect("symlink should be created"); - - let err = remove_downloaded(temp.path(), "game") - .await - .expect_err("symlink game root must be rejected"); - - assert!(err.to_string().contains("symlink game root")); - assert!(outside.join("version.ini").is_file()); - } -} diff --git a/organize/decision-tracking/PEER-AUTH-REFACTOR-DECISIONS.md b/organize/decision-tracking/PEER-AUTH-REFACTOR-DECISIONS.md index 2086d5c..f88fb0b 100644 --- a/organize/decision-tracking/PEER-AUTH-REFACTOR-DECISIONS.md +++ b/organize/decision-tracking/PEER-AUTH-REFACTOR-DECISIONS.md @@ -114,27 +114,31 @@ Alternatives: promotion point, but can require another full game's worth of disk and a large multi-file transaction mechanism. -## 2026-08-09 — Manifest target paths become download-owned +## 2026-08-09 — Reject untracked exact manifest targets -**TL;DR:** Once a validated download transaction starts, every regular file path -in that manifest is treated as downloader-owned for abort and recovery. Unknown -paths outside the manifest remain untouched. +**TL;DR:** Before parking `version.ini` or preparing storage, reject a download +when a regular-file destination already exists but is absent from the last +committed ownership set. Previously owned paths may be replaced; every unknown +path remains untouched. Without prior ownership state, an existing file at an exact manifest target is -ambiguous. Treating the authoritative target namespace as download-owned keeps -legacy upgrades working and makes partial-transfer cleanup deterministic. A -user-created extra file is preserved, but a user file placed at an exact package -path may be replaced or removed by the download. +ambiguous. Automatically claiming it would make a failed first or post-upgrade +download truncate and then delete potentially user-owned bytes. Failing closed +preserves the file, but a legacy tree without an ownership journal may require +the user to move or remove conflicting package files before its first update. +Phase 2 can later recognize intact catalog bytes by their trusted local hashes +without weakening this boundary. Alternatives: -- Reject every untracked existing target. This preserves ambiguous files, but - prevents the first post-upgrade update of existing legacy downloads. +- Treat every validated manifest target as download-owned. This keeps legacy + upgrades seamless, but can destroy an unrelated user file on failure or + cancellation. - Snapshot and restore untracked targets. This preserves their bytes, but adds unbounded backup space and another crash-consistent transaction. -- Leave pre-existing untracked targets after abort. This avoids deletion, but - the transfer may already have truncated or partially overwritten them, so it - does not actually preserve their contents. +- Stage every payload in a second game tree and promote it atomically. This + avoids touching collisions during transfer, but can require another full + game's worth of disk and a cross-platform multi-file promotion protocol. ## 2026-08-09 — Ownership-record rename is its publication point @@ -201,6 +205,66 @@ Alternatives: but expands the crash protocol and still needs a policy for pre-existing manifest directories. +## 2026-08-09 — Remove downloads as an empty ownership generation + +**TL;DR:** “Remove downloaded files” parks `version.ini`, durably journals an +empty pending generation, removes only the last committed file set, discards the +sentinel, and keeps a valid empty ownership record. It never recursively deletes +the game root. + +The existing pending-generation recovery already gives this operation a clean +crash protocol. Before the empty generation is durable, recovery restores the +sentinel and no payload has been touched. Afterwards, recovery idempotently +finishes deleting only proven-owned files and finalizes the empty record. +Unknown files and directories remain. A legacy, corrupt, or wrongly bound record +fails closed because paths and sizes alone cannot distinguish package bytes from +user bytes; Phase 2 can later adopt an intact legacy tree by checking it against +trusted catalog hashes. + +Alternatives: + +- Recursively delete the game root, then clear the ledger. This frees every + byte, but destroys unknown files and leaves stale deletion authority if the + process crashes between those operations. +- Delete the sentinel and leave every ambiguous payload file. This preserves + user bytes, but reports a misleading successful removal while reclaiming + almost no space. +- Infer ownership from the current remote manifest or filename extensions. This + is convenient for legacy trees, but lets untrusted or incomplete metadata + authorize deletion. + +## 2026-08-09 — Ownership follows committed paths, not inode generations + +**TL;DR:** A successfully committed download path remains downloader-owned until +a later download or explicit removal releases it. The ledger does not persist +platform-specific inode or file-ID generations. + +This is enough for the stated remote-peer threat model: a peer cannot replace a +victim filesystem object except through the one admitted download operation, and +untracked exact targets are rejected before that operation starts. A local actor +who replaces an already owned path makes the replacement subject to later +owned-path cleanup. That local-filesystem race is outside the plan's threat +model, but the consequence is recorded because the provenance boundary is path +based rather than object based. + +The same rule applies if someone deletes and recreates the entire game root at +the same configured path outside Lanspread: the last committed relative paths +remain owned. The application cannot distinguish that replacement without a +separate root-generation marker or platform file identity. + +Alternatives: + +- Persist inode/file-ID generations and delete only the exact recorded object. + This detects replacement, but creates a platform-specific schema and does not + survive ordinary copy/restore workflows consistently. +- Hash every owned file before cleanup. Phase 2 catalog hashes can identify + intact package content, but always rereading multi-gigabyte payloads solely + for deletion adds significant latency and still needs a policy for modified + downloader-owned files. +- Never delete a previously owned path automatically. This preserves every local + replacement but makes stale cleanup and “Remove downloaded files” unable to + reclaim ordinary package data. + ## 2026-08-09 — A baseline record distinguishes new and legacy scratch **TL;DR:** Before parking an existing `version.ini`, the new downloader durably @@ -299,15 +363,14 @@ Alternatives: **TL;DR:** Reject links and reparse points that can redirect path resolution, but do not reject an otherwise regular manifest target merely because it has multiple hard links. The threat model excludes an attacker controlling the -victim filesystem, and exact manifest target paths already become download-owned -when a transaction starts. +victim filesystem, and only previously recorded download-owned targets may be +replaced by a transaction. A hard link cannot be selected or created by a remote description outside the -validated game-relative namespace. A local user can make the same inode visible -under another name, but that is local filesystem manipulation rather than a -peer-controlled path escape. This choice inherits the documented consequence -that replacing an exact ambiguous manifest target may affect another local name -for those bytes. +validated game-relative namespace. A local user can make an already owned inode +visible under another name, but that is local filesystem manipulation rather +than a peer-controlled path escape. New untracked manifest targets are rejected +before mutation instead of being claimed automatically. Alternatives: