From 2453c9443628a6d7d5d786a9be7271d8287d8a5c Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Mon, 20 Jul 2026 18:14:09 +0200 Subject: [PATCH] fix: make installs atomic with their metadata; safer asset matching and launch paths Three audit findings on the install pipeline: - Cancelling a download during verify/extract detached the blocking install task (it runs to completion) while dropping the completion message, so the version/asset records - previously written in the UI handler - were never saved: an installed binary with no version file is silently excluded from every future update check, and a filePattern install became an orphaned binary the app no longer saw as installed. The records are now written inside the blocking step, atomically with the promotion, by download_release_asset itself (the parameters moved into an AssetInstall struct while touching the signature). - find_asset_by_pattern now prefers an exact name match (so 'app-macos' resolves even though 'app-macos-x86' contains it) and ignores non-installable siblings (.sig/.sha256/.yml/...): the day a repo signs its releases the way Colony itself does, every substring pattern would otherwise turn ambiguous and break installs. - installed_app_path now applies the same traversal guard as the install path to the manifest-supplied binary/file name: a hostile manifest could otherwise point the Launch button at an arbitrary executable outside the app's install dir. Two new tests cover the exact-match and signature-sibling cases. --- src/download.rs | 53 +++++++++++++++++++++++++++++++++++---------- src/github.rs | 54 +++++++++++++++++++++++++++++++++++++++++++--- src/persistence.rs | 12 +++++++++++ src/update.rs | 33 +++++++++++++++------------- 4 files changed, 123 insertions(+), 29 deletions(-) diff --git a/src/download.rs b/src/download.rs index 4cde81f..2119f00 100644 --- a/src/download.rs +++ b/src/download.rs @@ -136,7 +136,7 @@ fn verify_sha256(path: &std::path::Path, expected_hex: &str) -> Result<()> { /// Ensure a filename is a single normal path component (no `..`, no path /// separators, not absolute) before it is joined into a destination directory. /// Shared by archive extraction and raw-asset download to block path traversal. -fn ensure_safe_component(name: &str) -> Result<()> { +pub(crate) fn ensure_safe_component(name: &str) -> Result<()> { let p = std::path::Path::new(name); anyhow::ensure!( p.components().count() == 1 @@ -248,20 +248,39 @@ fn extract_binary_from_archive( } } -/// Download a release asset to `//`. -/// If `expected_sha256` is provided, verifies the file integrity after download. -/// If `binary_name` is provided, treats the downloaded file as an archive and -/// extracts the named binary from it (supports .zip and .tar.gz). -/// Returns the final path on success. +/// Everything needed to install one resolved release asset. +pub struct AssetInstall { + pub repo_name: String, + /// The resolved (never "latest") release tag being installed. + pub tag: String, + /// The resolved asset name to download. + pub filename: String, + /// When set, the download is an archive and this named binary is extracted. + pub binary_name: Option, + /// When set, the download is integrity-checked against this hex digest. + pub expected_sha256: Option, + /// True when `filename` was resolved from a filePattern: the name is then + /// persisted next to the binary so `installed_app_path` can find the + /// install again. + pub record_asset: bool, +} + +/// Download a release asset to `//`, +/// verify/extract it, atomically promote it into place, and record the +/// installed version. Returns the final path on success. pub async fn download_release_asset( token: Option, - repo_name: String, - tag: String, - filename: String, - binary_name: Option, - expected_sha256: Option, + install: AssetInstall, progress_tx: Option>, ) -> Result { + let AssetInstall { + repo_name, + tag, + filename, + binary_name, + expected_sha256, + record_asset, + } = install; // The manifest-supplied filename becomes a local path — guard it against // traversal (`../`, absolute paths) before joining, mirroring the archive // `binary` guard. @@ -324,6 +343,18 @@ pub async fn download_release_asset( std::fs::set_permissions(&final_path, perms)?; } + // Record WHAT is installed here, atomically with the install + // itself - not in the UI message handler. A cancel mid-install + // drops the awaiting future but this blocking task runs to + // completion: writing the version from the handler meant an + // installed binary with no version file, silently excluded from + // every future update check (or, in the filePattern case, an + // orphaned binary the app no longer even sees as installed). + crate::persistence::save_installed_version(&repo_name, &tag)?; + if record_asset { + crate::persistence::save_installed_asset(&repo_name, &filename)?; + } + Ok(final_path) }) .await? diff --git a/src/github.rs b/src/github.rs index aaf8283..f768fce 100644 --- a/src/github.rs +++ b/src/github.rs @@ -28,8 +28,8 @@ pub use crate::download::{apply_launcher_update, download_launcher_asset, downlo pub use crate::persistence::{ colony_apps_dir, colony_data_dir, installed_app_path, load_favorites, load_installed_version, load_preferences, load_repos_cache, load_scan_cache, read_repo_doc, - save_favorites, save_installed_asset, save_installed_version, save_preferences, - save_repos_cache, save_scan_cache, CachedApp, UserPreferences, + save_favorites, save_preferences, save_repos_cache, save_scan_cache, CachedApp, + UserPreferences, }; use crate::persistence::save_repo_doc; use crate::persistence::save_repo_icon; @@ -679,11 +679,29 @@ pub async fn fetch_release_info( /// Find an asset whose name contains the given pattern (case-insensitive). /// Returns an error if zero or multiple assets match. +/// Metadata companions published alongside release binaries (signatures, +/// checksums, updater manifests). Never installable, so they are excluded from +/// pattern matching - otherwise `app-linux.sig` would make the pattern +/// "linux" ambiguous the day a repo starts signing its releases (Colony's own +/// releases already ship `.sig` siblings). +const NON_INSTALLABLE_SUFFIXES: &[&str] = + &[".sig", ".asc", ".sha256", ".sha256sum", ".txt", ".yml", ".yaml", ".json"]; + pub fn find_asset_by_pattern(assets: &[String], pattern: &str) -> Result { let pattern_lower = pattern.to_lowercase(); + // An exact (case-insensitive) name match always wins: it cannot be + // ambiguous, and it lets a manifest pin `app-macos` even though + // `app-macos-x86` also contains that pattern as a substring. + if let Some(exact) = assets.iter().find(|n| n.to_lowercase() == pattern_lower) { + return Ok(exact.clone()); + } let matches: Vec<&String> = assets .iter() - .filter(|name| name.to_lowercase().contains(&pattern_lower)) + .filter(|name| { + let lower = name.to_lowercase(); + lower.contains(&pattern_lower) + && !NON_INSTALLABLE_SUFFIXES.iter().any(|s| lower.ends_with(s)) + }) .collect(); match matches.len() { 0 => anyhow::bail!("No release asset matching pattern '{pattern}'"), @@ -1043,6 +1061,36 @@ mod tests { assert!(result.unwrap_err().to_string().contains("Ambiguous")); } + #[test] + fn find_asset_by_pattern_exact_name_beats_substring_overlap() { + // "app-macos" is a substring of "app-macos-x86": an exact name match + // must win instead of erroring as ambiguous, so Apple Silicon + // manifests can pin the shorter asset name. + let assets = vec!["app-macos".to_string(), "app-macos-x86".to_string()]; + assert_eq!( + find_asset_by_pattern(&assets, "app-macos").unwrap(), + "app-macos" + ); + assert_eq!( + find_asset_by_pattern(&assets, "app-macos-x86").unwrap(), + "app-macos-x86" + ); + } + + #[test] + fn find_asset_by_pattern_ignores_signature_and_checksum_siblings() { + // The day a repo signs its releases (like Colony itself), every binary + // grows a .sig sibling containing the same name: the pattern must + // keep resolving to the binary, not error as ambiguous. + let assets = vec![ + "app-linux".to_string(), + "app-linux.sig".to_string(), + "app-linux.sha256".to_string(), + "latest-linux.yml".to_string(), + ]; + assert_eq!(find_asset_by_pattern(&assets, "linux").unwrap(), "app-linux"); + } + #[test] fn parse_colony_manifest_missing_required_field() { // category is required, so missing it should fail diff --git a/src/persistence.rs b/src/persistence.rs index 3e8179f..3ffd3cd 100644 --- a/src/persistence.rs +++ b/src/persistence.rs @@ -79,6 +79,18 @@ pub fn installed_app_path(repo: &ColonyRepo) -> Option { // filePattern was used — check saved resolved asset name load_installed_asset(&repo.name)? }; + // The filename comes straight from the repo's own manifest: the same + // traversal guard as the install path applies, or a hostile manifest + // could point the Launch button at an arbitrary executable on disk + // (e.g. `binary: "../../somewhere/else"`). + if crate::download::ensure_safe_component(&filename).is_err() { + tracing::warn!( + repo = %repo.name, + %filename, + "refusing manifest launch path outside the app's install dir" + ); + return None; + } let path = colony_apps_dir().ok()?.join(&repo.name).join(&filename); if path.exists() { Some(path) diff --git a/src/update.rs b/src/update.rs index 1479aa9..7d8cec4 100644 --- a/src/update.rs +++ b/src/update.rs @@ -470,23 +470,26 @@ impl App { (tag, f) }; + // The version/asset records are written by + // download_release_asset itself, inside the + // blocking install step: writing them here (or + // in DownloadCompleted) meant a cancel landing + // mid-install detached the blocking task and + // left an installed binary with no metadata. let path = github::download_release_asset( token, - repo_name.clone(), - resolved_tag.clone(), - resolved_file.clone(), - binary, - expected_sha256, + crate::download::AssetInstall { + repo_name: repo_name.clone(), + tag: resolved_tag.clone(), + filename: resolved_file.clone(), + binary_name: binary, + expected_sha256, + record_asset: file_pattern.is_some(), + }, Some(progress_tx), ) .await?; - // Save resolved asset name when using filePattern (for installed_app_path) - if file_pattern.is_some() { - let _ = - github::save_installed_asset(&repo_name, &resolved_file); - } - Ok((path, dl_repo, resolved_tag)) }, |result: Result<_, anyhow::Error>| { @@ -525,10 +528,10 @@ impl App { self.is_downloading = false; self.download_abort = None; match result { - Ok((path, repo_name, tag)) => { - if let Err(e) = github::save_installed_version(&repo_name, &tag) { - tracing::warn!("Failed to save version info: {e}"); - } + // Version/asset records were written atomically with the + // install (inside download_release_asset), so the tag is + // no longer needed here. + Ok((path, repo_name, _tag)) => { // The just-installed version IS the one the badge was // advertising: clear it, or the card keeps showing // "Update vX -> vX" until the next global check.