From 13fc1dba62233fde0878b2a95778c78b986d9a8c Mon Sep 17 00:00:00 2001 From: Rabindra Dhakal Date: Thu, 16 Jul 2026 22:35:56 +0545 Subject: [PATCH 1/2] fix(security): validate repository names to block path traversal --- Cargo.lock | 1 + crates/soar-config/src/config.rs | 41 +++++++++++++++++++++++++++++- crates/soar-config/src/error.rs | 21 ++++++++++++--- crates/soar-db/Cargo.toml | 1 + crates/soar-db/src/models/types.rs | 28 +------------------- crates/soar-operations/src/repo.rs | 24 ++++++++++++++--- crates/soar-utils/src/path.rs | 41 +++++++++++++++++++++++++++++- 7 files changed, 121 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 928455c8d..c3852fbc1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2296,6 +2296,7 @@ dependencies = [ "serde", "serde_json", "soar-registry", + "soar-utils", "thiserror", "tracing", ] diff --git a/crates/soar-config/src/config.rs b/crates/soar-config/src/config.rs index 98bf0696f..3fe6decac 100644 --- a/crates/soar-config/src/config.rs +++ b/crates/soar-config/src/config.rs @@ -8,7 +8,7 @@ use std::{ use documented::{Documented, DocumentedFields}; use serde::{Deserialize, Serialize}; use soar_utils::{ - path::{resolve_path, xdg_config_home, xdg_data_home}, + path::{is_safe_component, resolve_path, xdg_config_home, xdg_data_home}, system::platform, }; use toml_edit::DocumentMut; @@ -428,6 +428,11 @@ impl Config { let mut seen_repos = HashSet::new(); for repo in &mut self.repositories { + // The name becomes a directory under the repositories path, so it + // must not be able to escape it. + if !is_safe_component(&repo.name) { + return Err(ConfigError::InvalidRepositoryName(repo.name.clone())); + } if repo.name == "local" { return Err(ConfigError::ReservedRepositoryName); } @@ -674,6 +679,40 @@ mod tests { assert!(matches!(result, Err(ConfigError::MissingDefaultProfile(_)))); } + fn repo_named(name: &str) -> Repository { + Repository { + name: name.to_string(), + url: "https://example.com".to_string(), + desktop_integration: None, + pubkey: None, + enabled: Some(true), + signature_verification: None, + sync_interval: None, + } + } + + #[test] + fn test_config_resolve_rejects_traversal_repo_name() { + for name in ["..", "a/b", "/home/user/Documents", "", "."] { + let mut config = Config::default_config::<&str>(&[]); + config.repositories.push(repo_named(name)); + + let result = config.resolve(); + assert!( + matches!(result, Err(ConfigError::InvalidRepositoryName(_))), + "name {name:?} should be rejected" + ); + } + } + + #[test] + fn test_config_resolve_accepts_normal_repo_name() { + let mut config = Config::default_config::<&str>(&[]); + config.repositories.push(repo_named("my-repo")); + + assert!(config.resolve().is_ok()); + } + #[test] fn test_config_resolve_reserved_repo_name() { let mut config = Config::default_config::<&str>(&[]); diff --git a/crates/soar-config/src/error.rs b/crates/soar-config/src/error.rs index c39030c19..27b04a919 100644 --- a/crates/soar-config/src/error.rs +++ b/crates/soar-config/src/error.rs @@ -60,9 +60,24 @@ pub enum ConfigError { )] MissingProfile(String), - #[error("Invalid repository name: {0}")] - #[diagnostic(code(soar_config::invalid_repository))] - InvalidRepository(String), + #[error("Repository '{0}' is not configured")] + #[diagnostic( + code(soar_config::repository_not_found), + help("Run `soar repo list` to see the configured repositories.") + )] + RepositoryNotFound(String), + + #[error("Repository name '{0}' is not a valid directory name")] + #[diagnostic( + code(soar_config::invalid_repository_name), + help( + "A repository's data is stored in a directory named after it, under the \ + repositories path, so the name must be a single path component. It cannot be \ + empty, contain '/', be '.' or '..', or be an absolute path. Try a plain name, \ + e.g. `soar repo add personal https://personal.repo`." + ) + )] + InvalidRepositoryName(String), #[error("Invalid repository URL: {0}")] #[diagnostic(code(soar_config::invalid_repository_url))] diff --git a/crates/soar-db/Cargo.toml b/crates/soar-db/Cargo.toml index 50acba81c..a3b11dcca 100644 --- a/crates/soar-db/Cargo.toml +++ b/crates/soar-db/Cargo.toml @@ -21,5 +21,6 @@ regex = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } soar-registry = { workspace = true } +soar-utils = { workspace = true } thiserror = { workspace = true } tracing = { workspace = true } diff --git a/crates/soar-db/src/models/types.rs b/crates/soar-db/src/models/types.rs index 66e0b26e0..e75a17dd8 100644 --- a/crates/soar-db/src/models/types.rs +++ b/crates/soar-db/src/models/types.rs @@ -1,17 +1,5 @@ -use std::path::{Component, Path}; - use serde::{Deserialize, Serialize}; - -/// Returns `true` if `name` is a single, normal path component that is safe to -/// join onto a directory. -/// -/// Rejects empty strings, absolute paths, `.`/`..`, and any value containing a -/// path separator. Used to keep untrusted `provides` metadata from escaping the -/// bin directory when it is turned into a symlink path. -pub fn is_safe_component(name: &str) -> bool { - let mut components = Path::new(name).components(); - matches!(components.next(), Some(Component::Normal(_))) && components.next().is_none() -} +use soar_utils::path::is_safe_component; #[derive(Debug, Clone, Deserialize, Serialize)] pub enum ProvideStrategy { @@ -137,20 +125,6 @@ mod tests { assert_eq!(p.bin_symlink_names(), vec!["clipcat"]); } - #[test] - fn test_is_safe_component() { - assert!(is_safe_component("clipcat")); - assert!(is_safe_component("clip-cat.1")); - - assert!(!is_safe_component("")); - assert!(!is_safe_component(".")); - assert!(!is_safe_component("..")); - assert!(!is_safe_component("a/b")); - assert!(!is_safe_component("../etc")); - assert!(!is_safe_component("../../home/user/.bashrc")); - assert!(!is_safe_component("/etc/passwd")); - } - #[test] fn test_provide_is_safe() { assert!(PackageProvide::from_string("clipcatd").is_safe()); diff --git a/crates/soar-operations/src/repo.rs b/crates/soar-operations/src/repo.rs index edb4c4220..7b8fec60d 100644 --- a/crates/soar-operations/src/repo.rs +++ b/crates/soar-operations/src/repo.rs @@ -54,7 +54,7 @@ impl SoarContext { .iter_mut() .find(|r| r.name == name) .ok_or_else(|| { - soar_config::error::ConfigError::InvalidRepository(name.to_string()) + soar_config::error::ConfigError::RepositoryNotFound(name.to_string()) })?; if let Some(url) = update.url { @@ -89,7 +89,7 @@ impl SoarContext { .iter() .position(|r| r.name == name) .ok_or_else(|| { - soar_config::error::ConfigError::InvalidRepository(name.to_string()) + soar_config::error::ConfigError::RepositoryNotFound(name.to_string()) })?; let repo = config.repositories.remove(idx); @@ -97,8 +97,24 @@ impl SoarContext { // Clean up the repository's data directory if let Ok(repo_path) = repo.get_path() { if repo_path.exists() { - fs::remove_dir_all(&repo_path).with_context(|| { - format!("removing repository data at {}", repo_path.display()) + // Canonicalize before deleting: `Path::parent` is lexical, so + // a name like ".." would pass a parent check while still + // resolving outside the repositories directory. + let repos_dir = config.get_repositories_path()?; + let repos_dir = repos_dir.canonicalize().with_context(|| { + format!("canonicalizing repositories dir {}", repos_dir.display()) + })?; + let target = repo_path.canonicalize().with_context(|| { + format!("canonicalizing repository path {}", repo_path.display()) + })?; + if target == repos_dir || !target.starts_with(&repos_dir) { + return Err(soar_config::error::ConfigError::InvalidRepositoryName( + repo.name.clone(), + ) + .into()); + } + fs::remove_dir_all(&target).with_context(|| { + format!("removing repository data at {}", target.display()) })?; } } diff --git a/crates/soar-utils/src/path.rs b/crates/soar-utils/src/path.rs index 09aee3149..4173e2642 100644 --- a/crates/soar-utils/src/path.rs +++ b/crates/soar-utils/src/path.rs @@ -1,10 +1,34 @@ -use std::{env, path::PathBuf}; +use std::{ + env, + path::{Component, Path, PathBuf}, +}; use crate::{ error::{PathError, PathResult}, system::get_username, }; +/// Returns `true` if `name` is a single, normal path component that is safe to +/// join onto a directory. +/// +/// Rejects empty strings, absolute paths, `.`/`..`, and any value containing a +/// path separator. Use this for untrusted values that become a path component, +/// such as repository names or package `provides` entries, where a join must +/// not escape its base directory. +/// +/// # Example +/// +/// ``` +/// use soar_utils::path::is_safe_component; +/// +/// assert!(is_safe_component("soarpkgs")); +/// assert!(!is_safe_component("../etc")); +/// ``` +pub fn is_safe_component(name: &str) -> bool { + let mut components = Path::new(name).components(); + matches!(components.next(), Some(Component::Normal(_))) && components.next().is_none() +} + /// Resolves a path string that may contain environment variables /// /// This method expands environment variables in the format `$VAR` or `${VAR}`, resolves tilde @@ -236,6 +260,21 @@ fn expand_env_var(var_name: &str, result: &mut String, original: &str) -> PathRe #[cfg(test)] mod tests { + #[test] + fn test_is_safe_component() { + assert!(super::is_safe_component("clipcat")); + assert!(super::is_safe_component("clip-cat.1")); + assert!(super::is_safe_component("soarpkgs")); + + assert!(!super::is_safe_component("")); + assert!(!super::is_safe_component(".")); + assert!(!super::is_safe_component("..")); + assert!(!super::is_safe_component("a/b")); + assert!(!super::is_safe_component("../etc")); + assert!(!super::is_safe_component("../../home/user/.bashrc")); + assert!(!super::is_safe_component("/etc/passwd")); + } + use std::env; use serial_test::serial; From 5df0ea3883a6c8cb0ab0176c8303437a8e68a1d2 Mon Sep 17 00:00:00 2001 From: Rabindra Dhakal Date: Thu, 16 Jul 2026 22:49:19 +0545 Subject: [PATCH 2/2] fix --- crates/soar-operations/src/repo.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/soar-operations/src/repo.rs b/crates/soar-operations/src/repo.rs index 7b8fec60d..85a4d7bd3 100644 --- a/crates/soar-operations/src/repo.rs +++ b/crates/soar-operations/src/repo.rs @@ -113,9 +113,20 @@ impl SoarContext { ) .into()); } - fs::remove_dir_all(&target).with_context(|| { - format!("removing repository data at {}", target.display()) - })?; + // Remove the configured path rather than the canonicalized + // target: when the repo dir is a symlink, drop the link + // itself instead of the directory it points at. + let meta = fs::symlink_metadata(&repo_path) + .with_context(|| format!("reading metadata for {}", repo_path.display()))?; + if meta.file_type().is_symlink() { + fs::remove_file(&repo_path).with_context(|| { + format!("removing repository symlink at {}", repo_path.display()) + })?; + } else { + fs::remove_dir_all(&repo_path).with_context(|| { + format!("removing repository data at {}", repo_path.display()) + })?; + } } }