diff --git a/crates/soar-cli/src/health.rs b/crates/soar-cli/src/health.rs index 9c85945c..075b27bb 100644 --- a/crates/soar-cli/src/health.rs +++ b/crates/soar-cli/src/health.rs @@ -99,6 +99,19 @@ pub async fn display_health(ctx: &SoarContext) -> SoarResult<()> { Ok(()) } +pub fn remove_broken_symlinks(ctx: &SoarContext) -> SoarResult<()> { + let removed = health::remove_broken_symlinks(ctx)?; + + if removed.is_empty() { + info!("No broken symlinks found."); + } + for path in &removed { + info!("Removed broken symlink: {}", path.display()); + } + + Ok(()) +} + pub async fn remove_broken_packages(ctx: &SoarContext) -> SoarResult<()> { let report = health::remove_broken_packages(ctx).await?; diff --git a/crates/soar-cli/src/main.rs b/crates/soar-cli/src/main.rs index 5a5d34d3..9c1e9e53 100644 --- a/crates/soar-cli/src/main.rs +++ b/crates/soar-cli/src/main.rs @@ -13,7 +13,7 @@ use apply::apply_packages; use clap::{CommandFactory, Parser}; use cli::Args; use download::{DownloadContext, create_regex_patterns, download}; -use health::{display_health, remove_broken_packages}; +use health::{display_health, remove_broken_packages, remove_broken_symlinks}; use indicatif::ProgressBar; use install::install_packages; use json2db::json_to_db; @@ -29,7 +29,7 @@ use soar_config::config::{ use soar_core::{ SoarResult, error::{ErrorContext, SoarError}, - utils::{cleanup_cache, remove_broken_symlinks, setup_required_paths}, + utils::{cleanup_cache, setup_required_paths}, }; use soar_dl::http_client::configure_http_client; use soar_events::EventSinkHandle; @@ -673,7 +673,7 @@ async fn handle_cli() -> SoarResult<()> { cleanup_cache()?; } if unspecified || broken_symlinks { - remove_broken_symlinks()?; + remove_broken_symlinks(&ctx)?; } if unspecified || broken { remove_broken_packages(&ctx).await?; diff --git a/crates/soar-cli/src/use.rs b/crates/soar-cli/src/use.rs index f1839ecd..ba587001 100644 --- a/crates/soar-cli/src/use.rs +++ b/crates/soar-cli/src/use.rs @@ -50,12 +50,13 @@ pub async fn use_alternate_package(ctx: &SoarContext, name: &str) -> SoarResult< ); } - if variants.len() == 1 { - return Ok(()); - } - - let selection = get_valid_selection(variants.len())?; - switch::switch_variant(ctx, name, selection).await?; + // A sole variant can still be unlinked, and switching to it links it again. + let selection = match variants.as_slice() { + [only] if only.is_active => return Ok(()), + [_] => 0, + _ => get_valid_selection(variants.len())?, + }; + switch::switch_variant(ctx, variants[selection].package.id).await?; info!("Switched to {}", variants[selection].package.pkg_name); diff --git a/crates/soar-core/src/package/install.rs b/crates/soar-core/src/package/install.rs index 5286681d..14559055 100644 --- a/crates/soar-core/src/package/install.rs +++ b/crates/soar-core/src/package/install.rs @@ -3,7 +3,7 @@ use std::{ env, fs, io::{Read, Write}, os::unix::fs::PermissionsExt, - path::{Path, PathBuf}, + path::{Component, Path, PathBuf}, process::Command, thread::sleep, time::Duration, @@ -316,7 +316,7 @@ fn is_safe_relative(path: &str) -> bool { && !Path::new(path).is_absolute() && Path::new(path) .components() - .all(|c| matches!(c, std::path::Component::Normal(_))) + .all(|c| matches!(c, Component::Normal(_))) } /// Every regular file under `dir`, skipping the staging directory. @@ -428,16 +428,17 @@ fn is_elf(path: &Path) -> bool { } /// Early validation of relative paths before download. -/// Rejects paths containing `..` or absolute paths. +/// Rejects absolute paths and paths with a `..` component. fn validate_relative_path(relative_path: &str, path_type: &str) -> SoarResult<()> { - if Path::new(relative_path).is_absolute() { + let path = Path::new(relative_path); + if path.is_absolute() { return Err(SoarError::Custom(format!( "{} '{}' must be a relative path, not absolute", path_type, relative_path ))); } - if relative_path.contains("..") { + if path.components().any(|c| c == Component::ParentDir) { return Err(SoarError::Custom(format!( "{} '{}' contains path traversal components", path_type, relative_path @@ -1613,7 +1614,7 @@ mod tests { use super::{ Package, apply_file_layout, calculate_checksum, clear_for_promotion, install_extras, - relative_to, resolve_source, + relative_to, resolve_source, validate_relative_path, }; /// Runs `install_extras`, which never waits on anything, to completion. @@ -1679,6 +1680,19 @@ mod tests { ); } + #[test] + fn extract_paths_reject_only_parent_components() { + for ok in ["foo..bar", "a/..b", "..a/b", "./bin", "a/b"] { + assert!(validate_relative_path(ok, "extract_root").is_ok(), "{ok}"); + } + for bad in ["..", "../x", "a/../b", "a/..", "/abs"] { + assert!( + validate_relative_path(bad, "extract_root").is_err(), + "{bad}" + ); + } + } + #[test] fn alias_in_another_directory_climbs_out() { assert_eq!( diff --git a/crates/soar-core/src/package/url.rs b/crates/soar-core/src/package/url.rs index a4bf652a..53705a05 100644 --- a/crates/soar-core/src/package/url.rs +++ b/crates/soar-core/src/package/url.rs @@ -49,8 +49,7 @@ impl UrlPackage { /// - `ghcr.io/org/repo@sha256:digest` /// - `ghcr.io/org/repo` (implies :latest) pub fn is_ghcr(input: &str) -> bool { - let input = input.trim().to_lowercase(); - input.starts_with("ghcr.io/") + strip_ghcr_prefix(input.trim()).is_some() } /// Check if input is either a URL or GHCR reference. @@ -128,17 +127,12 @@ impl UrlPackage { ) -> SoarResult { let reference = reference.trim(); - if !Self::is_ghcr(reference) { + let Some(path) = strip_ghcr_prefix(reference) else { return Err(SoarError::Custom(format!( "Invalid GHCR reference: {}", reference ))); - } - - let path = reference - .strip_prefix("ghcr.io/") - .or_else(|| reference.strip_prefix("GHCR.IO/")) - .unwrap_or(reference); + }; let (package, tag) = if let Some((pkg, digest)) = path.split_once('@') { (pkg, digest.to_string()) @@ -165,7 +159,7 @@ impl UrlPackage { Self::validate_names(&pkg_name, pkg_id.as_deref(), pkg_family.as_deref())?; Ok(Self { - url: reference.to_string(), + url: format!("ghcr.io/{path}"), pkg_id, pkg_family, pkg_name, @@ -294,6 +288,15 @@ impl UrlPackage { } } +/// The rest of `input` after a `ghcr.io/` prefix in any letter case. +fn strip_ghcr_prefix(input: &str) -> Option<&str> { + const PREFIX: &str = "ghcr.io/"; + input + .get(..PREFIX.len()) + .filter(|p| p.eq_ignore_ascii_case(PREFIX)) + .map(|_| &input[PREFIX.len()..]) +} + /// Extract the family from a URL: host plus the first two path segments. /// /// Examples: @@ -685,6 +688,18 @@ mod tests { assert!(pkg.is_ghcr); } + #[test] + fn ghcr_prefix_is_case_insensitive() { + let pkg = + UrlPackage::from_ghcr("Ghcr.Io/pkgforge/soar:v0.8.1", None, None, None, None).unwrap(); + + assert_eq!(pkg.url, "ghcr.io/pkgforge/soar:v0.8.1"); + assert_eq!(pkg.pkg_name, "soar"); + assert_eq!(pkg.pkg_family.as_deref(), Some("pkgforge.soar")); + assert!(UrlPackage::is_ghcr("gHcR.iO/org/repo")); + assert!(!UrlPackage::is_ghcr("ghcr.i")); + } + #[test] fn test_ghcr_with_digest() { let ghcr = "ghcr.io/org/repo@sha256:deadbeef1234567890"; diff --git a/crates/soar-core/src/utils.rs b/crates/soar-core/src/utils.rs index 2f8c700d..0e4caee9 100644 --- a/crates/soar-core/src/utils.rs +++ b/crates/soar-core/src/utils.rs @@ -7,10 +7,6 @@ use std::{ }; use soar_config::config::get_config; -use soar_utils::{ - error::FileSystemResult, - fs::{safe_remove, walk_dir}, -}; use tracing::info; use crate::error::{ErrorContext, SoarError}; @@ -58,33 +54,6 @@ pub fn cleanup_cache() -> Result<()> { Ok(()) } -fn remove_action(path: &Path) -> FileSystemResult<()> { - if path.is_symlink() && !path.exists() { - safe_remove(path)?; - info!("Removed broken symlink: {}", path.display()); - } - Ok(()) -} - -/// Removes broken symlinks from bin, desktop, and icons directories. -pub fn remove_broken_symlinks() -> Result<()> { - let mut soar_files_action = |path: &Path| -> FileSystemResult<()> { - if let Some(filename) = path.file_stem().and_then(|s| s.to_str()) - && filename.ends_with("-soar") - { - return remove_action(path); - } - Ok(()) - }; - - let config = get_config(); - walk_dir(&config.get_bin_path()?, &mut remove_action)?; - walk_dir(&config.get_desktop_path()?, &mut soar_files_action)?; - walk_dir(config.get_icons_path(), &mut soar_files_action)?; - - Ok(()) -} - /// Gets the extract directory path for a given base directory. pub fn get_extract_dir>(base_dir: P) -> PathBuf { let base_dir = base_dir.as_ref(); diff --git a/crates/soar-operations/src/health.rs b/crates/soar-operations/src/health.rs index 287c3ac9..81c574c0 100644 --- a/crates/soar-operations/src/health.rs +++ b/crates/soar-operations/src/health.rs @@ -1,4 +1,7 @@ -use std::path::PathBuf; +use std::{ + io::ErrorKind, + path::{Path, PathBuf}, +}; use soar_core::{SoarResult, package::remove::PackageRemover}; use soar_db::repository::core::CoreRepository; @@ -128,15 +131,26 @@ pub fn remove_broken_symlinks(ctx: &SoarContext) -> SoarResult> { let broken = get_broken_symlinks(ctx)?; let mut removed = Vec::new(); - for path in &broken { - if std::fs::remove_file(path).is_ok() { - removed.push(path.clone()); + for path in broken { + // The link may have been repaired since the scan. + if is_dangling(&path) && std::fs::remove_file(&path).is_ok() { + removed.push(path); } } Ok(removed) } +/// Whether `path` is a symlink whose target does not exist. +/// +/// Any other failure to read the target, such as a permission error, leaves +/// the link alone, since its target may well be there. +fn is_dangling(path: &Path) -> bool { + path.is_symlink() + && std::fs::metadata(path) + .is_err_and(|e| matches!(e.kind(), ErrorKind::NotFound | ErrorKind::NotADirectory)) +} + fn get_broken_packages(ctx: &SoarContext) -> SoarResult> { let diesel_db = ctx.diesel_core_db()?; let broken = diesel_db.with_conn(CoreRepository::list_broken)?; @@ -161,22 +175,19 @@ fn get_broken_symlinks(ctx: &SoarContext) -> SoarResult> { // something is wrong when nothing is. let bin_path = config.get_bin_path()?; if bin_path.is_dir() { - walk_dir( - &bin_path, - &mut |path: &std::path::Path| -> FileSystemResult<()> { - if !path.exists() { - broken.push(path.to_path_buf()); - } - Ok(()) - }, - )?; + walk_dir(&bin_path, &mut |path: &Path| -> FileSystemResult<()> { + if is_dangling(path) { + broken.push(path.to_path_buf()); + } + Ok(()) + })?; } let desktop_path = config.get_desktop_path()?; - let mut soar_check = |path: &std::path::Path| -> FileSystemResult<()> { + let mut soar_check = |path: &Path| -> FileSystemResult<()> { if let Some(filename) = path.file_stem().and_then(|s| s.to_str()) && filename.ends_with("-soar") - && !path.exists() + && is_dangling(path) { broken.push(path.to_path_buf()); } @@ -193,3 +204,38 @@ fn get_broken_symlinks(ctx: &SoarContext) -> SoarResult> { Ok(broken) } + +#[cfg(test)] +mod tests { + use std::{ + fs, + os::unix::fs::{PermissionsExt, symlink}, + }; + + use super::*; + + #[test] + fn only_links_to_missing_targets_are_dangling() { + let dir = tempfile::tempdir().unwrap(); + let p = |name: &str| dir.path().join(name); + fs::write(p("file"), "").unwrap(); + symlink(p("file"), p("good")).unwrap(); + symlink(p("gone"), p("missing")).unwrap(); + symlink(p("file/below"), p("through-file")).unwrap(); + + assert!(!is_dangling(&p("file"))); + assert!(!is_dangling(&p("good"))); + assert!(is_dangling(&p("missing"))); + assert!(is_dangling(&p("through-file"))); + + fs::create_dir(p("locked")).unwrap(); + fs::write(p("locked/target"), "").unwrap(); + symlink(p("locked/target"), p("unreadable")).unwrap(); + fs::set_permissions(p("locked"), fs::Permissions::from_mode(0o000)).unwrap(); + // Root reads through any permission, so the case cannot arise there. + if fs::metadata(p("unreadable")).is_err() { + assert!(!is_dangling(&p("unreadable"))); + } + fs::set_permissions(p("locked"), fs::Permissions::from_mode(0o755)).unwrap(); + } +} diff --git a/crates/soar-operations/src/progress.rs b/crates/soar-operations/src/progress.rs index a3355f0a..13f0c34d 100644 --- a/crates/soar-operations/src/progress.rs +++ b/crates/soar-operations/src/progress.rs @@ -18,7 +18,7 @@ const PROGRESS_INTERVAL: Duration = Duration::from_millis(100); /// `progress_callback` parameter. /// /// soar-dl reports every read, so chunk progress is passed on at most once -/// per [`PROGRESS_INTERVAL`], plus the chunk that completes the download. +/// per `PROGRESS_INTERVAL`, plus the chunk that completes the download. pub fn create_progress_bridge( events: EventSinkHandle, op_id: OperationId, diff --git a/crates/soar-operations/src/switch.rs b/crates/soar-operations/src/switch.rs index 14350fff..dcabc47b 100644 --- a/crates/soar-operations/src/switch.rs +++ b/crates/soar-operations/src/switch.rs @@ -18,6 +18,9 @@ use crate::{ }; /// List all installed variants (pkg_ids) for a given package name. +/// +/// Installs that never finished are left out, since there is nothing to +/// switch to. pub fn list_variants(ctx: &SoarContext, name: &str) -> SoarResult> { debug!(name = name, "listing variants"); let diesel_db = ctx.diesel_core_db()?; @@ -29,7 +32,7 @@ pub fn list_variants(ctx: &SoarContext, name: &str) -> SoarResult SoarResult SoarResult<()> { - debug!(name = name, index = selected_index, "switching variant"); +pub async fn switch_variant(ctx: &SoarContext, id: u64) -> SoarResult<()> { + debug!(id, "switching variant"); let diesel_db = ctx.diesel_core_db()?; - let packages = diesel_db.with_conn(|conn| { - CoreRepository::list_filtered( - conn, - None, - Some(name), - None, - None, - None, - None, - None, - Some(SortDirection::Asc), - ) - })?; - - let selected_package = packages - .into_iter() - .nth(selected_index) - .ok_or_else(|| SoarError::Custom("Invalid variant index".into()))?; + let row_id = + i32::try_from(id).map_err(|_| SoarError::Custom(format!("Invalid variant id {id}")))?; + let selected_package = diesel_db + .with_conn(|conn| CoreRepository::find_by_id_with_portable(conn, row_id))? + .filter(|p| p.is_installed) + .ok_or_else(|| SoarError::Custom("The selected variant is no longer installed".into()))?; let pkg_name = &selected_package.pkg_name; let pkg_id = selected_package.pkg_id.as_deref(); @@ -124,7 +111,7 @@ pub async fn switch_variant( .query_repo(&selected_package.repo_name, |conn| { MetadataRepository::find_filtered( conn, - Some(name), + Some(&selected_package.pkg_name), selected_package.pkg_id.as_deref(), selected_package.pkg_family.as_deref(), None, @@ -179,3 +166,85 @@ pub async fn switch_variant( Ok(()) } + +#[cfg(test)] +mod tests { + use std::{fs, os::unix::fs::PermissionsExt, path::Path, sync::Arc}; + + use soar_config::config::Config; + use soar_db::models::core::NewPackage; + use soar_events::NullSink; + + use super::*; + + fn context(root: &Path) -> SoarContext { + let mut config = Config::default_config::<&str>(&[]); + config.db_path = Some(root.join("db").display().to_string()); + config.bin_path = Some(root.join("bin").display().to_string()); + fs::create_dir_all(root.join("bin")).unwrap(); + SoarContext::new(config, Arc::new(NullSink)) + } + + fn add(ctx: &SoarContext, root: &Path, repo: &str, installed: bool) -> u64 { + let dir = root.join("packages").join(repo); + fs::create_dir_all(dir.join("bin")).unwrap(); + let bin = dir.join("bin/foo"); + fs::write(&bin, "#!/bin/sh\n").unwrap(); + fs::set_permissions(&bin, fs::Permissions::from_mode(0o755)).unwrap(); + let dir = dir.display().to_string(); + let id = ctx + .diesel_core_db() + .unwrap() + .with_conn(|conn| { + CoreRepository::insert( + conn, + &NewPackage { + repo_name: repo, + pkg_id: Some(repo), + pkg_name: "foo", + version: "1", + installed_path: &dir, + installed_date: "2026-10-19 00:00:00", + profile: "default", + is_installed: installed, + unlinked: true, + ..Default::default() + }, + ) + }) + .unwrap(); + id as u64 + } + + #[tokio::test] + async fn switching_follows_the_row_id_and_skips_unfinished_installs() { + let root = tempfile::tempdir().unwrap(); + let ctx = context(root.path()); + let first = add(&ctx, root.path(), "first", true); + let pending = add(&ctx, root.path(), "pending", false); + let last = add(&ctx, root.path(), "last", true); + + let listed: Vec = list_variants(&ctx, "foo") + .unwrap() + .iter() + .map(|v| v.package.id) + .collect(); + assert_eq!(listed, [first, last]); + + assert!(switch_variant(&ctx, pending).await.is_err()); + assert!(switch_variant(&ctx, last + (1 << 32)).await.is_err()); + + switch_variant(&ctx, last).await.unwrap(); + let active: Vec = list_variants(&ctx, "foo") + .unwrap() + .iter() + .filter(|v| v.is_active) + .map(|v| v.package.id) + .collect(); + assert_eq!(active, [last]); + assert_eq!( + fs::read_link(root.path().join("bin/foo")).unwrap(), + root.path().join("packages/last/bin/foo") + ); + } +}