From 85b7aa361b1011853fdf5813ee88641ef27892c3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 10 May 2026 18:03:02 -0400 Subject: [PATCH 1/2] Ignore top_level.txt entries in uninstall that are not valid Python identifiers --- Cargo.lock | 1 + Cargo.toml | 1 + crates/uv-install-wheel/Cargo.toml | 1 + crates/uv-install-wheel/src/uninstall.rs | 144 +++++++++++++---------- crates/uv-installer/src/uninstall.rs | 8 +- crates/uv/tests/it/pip_uninstall.rs | 63 +++++++++- 6 files changed, 153 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d6c03e7ddcb10..c6c45119aed52 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6644,6 +6644,7 @@ dependencies = [ "sha2", "thiserror 2.0.18", "tracing", + "unicode-ident", "uv-distribution-filename", "uv-flags", "uv-fs", diff --git a/Cargo.toml b/Cargo.toml index 1a4d0aef15650..0bcdb049a8e81 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -275,6 +275,7 @@ tracing-durations-export = { version = "0.3.0", features = ["plot"] } tracing-subscriber = { version = "0.3.18" } # Default feature set for uv_build, uv activates extra features tracing-test = { version = "0.2.5" } tracing-tree = { version = "0.4.0" } +unicode-ident = { version = "1.0.24" } unicode-width = { version = "0.2.0" } unscanny = { version = "0.1.0" } url = { version = "2.5.2", features = ["serde"] } diff --git a/crates/uv-install-wheel/Cargo.toml b/crates/uv-install-wheel/Cargo.toml index 36664566fa64c..bed04cfdd323b 100644 --- a/crates/uv-install-wheel/Cargo.toml +++ b/crates/uv-install-wheel/Cargo.toml @@ -45,6 +45,7 @@ serde_json = { workspace = true } sha2 = { workspace = true } thiserror = { workspace = true } tracing = { workspace = true } +unicode-ident = { workspace = true } walkdir = { workspace = true } [target.'cfg(target_os = "windows")'.dependencies] diff --git a/crates/uv-install-wheel/src/uninstall.rs b/crates/uv-install-wheel/src/uninstall.rs index 5e1a2a200b8a8..91d6ffdcd2519 100644 --- a/crates/uv-install-wheel/src/uninstall.rs +++ b/crates/uv-install-wheel/src/uninstall.rs @@ -160,6 +160,7 @@ pub fn uninstall_wheel( } static WARNED_FOR_PACKAGE: OnceLock>> = OnceLock::new(); +static WARNED_FOR_EGG_TOP_LEVEL_PACKAGE: OnceLock>> = OnceLock::new(); /// Check if the path is inside the venv or a system interpreter path, and warn if it isn't. /// @@ -210,14 +211,44 @@ fn is_path_in_scheme( } } +/// Check that a `top_level.txt` entry names a single top-level module or package. +/// +/// Unlike wheel `RECORD` entries, egg `top_level.txt` entries are Python identifiers, not paths. +/// Treating them as paths can make uninstall delete directories outside `site-packages`. +fn is_valid_top_level_entry(entry: &str, distribution: impl Display) -> bool { + if is_python_identifier(entry) { + true + } else { + if WARNED_FOR_EGG_TOP_LEVEL_PACKAGE + .get_or_init(|| Mutex::new(HashSet::new())) + .lock() + .expect("The mutex is broken, did some other thread panic?") + .insert(distribution.to_string()) + { + warn_user!( + "Invalid `top_level.txt` entry in {} that is not a top-level module or package, skipping: {}", + distribution, + entry + ); + } + false + } +} + +fn is_python_identifier(entry: &str) -> bool { + let mut chars = entry.chars(); + let Some(first) = chars.next() else { + return false; + }; + + (first == '_' || unicode_ident::is_xid_start(first)) + && chars.all(|character| character == '_' || unicode_ident::is_xid_continue(character)) +} + /// Uninstall the egg represented by the `.egg-info` directory. /// /// See: -pub fn uninstall_egg( - egg_info: &Path, - distribution: impl Display, - layout: &Layout, -) -> Result { +pub fn uninstall_egg(egg_info: &Path, distribution: impl Display) -> Result { let mut file_count = 0usize; let mut dir_count = 0usize; @@ -268,12 +299,12 @@ pub fn uninstall_egg( // Remove everything in `top_level.txt`. for entry in top_level { - let path = dist_location.join(&entry); - - if !is_path_in_scheme(&entry, dist_location, &distribution, layout) { + if !is_valid_top_level_entry(&entry, &distribution) { continue; } + let path = dist_location.join(&entry); + // Remove as a directory. match fs_err::remove_dir_all(&path) { Ok(()) => { @@ -435,7 +466,21 @@ mod tests { use uv_pypi_types::Scheme; use crate::Layout; - use crate::uninstall::{uninstall_egg, uninstall_wheel}; + use crate::uninstall::{is_python_identifier, uninstall_egg, uninstall_wheel}; + + #[test] + fn test_top_level_entry_python_identifier() { + assert!(is_python_identifier("package")); + assert!(is_python_identifier("_package")); + assert!(is_python_identifier("pkg_123")); + assert!(is_python_identifier("\u{00e9}clair")); + + assert!(!is_python_identifier("")); + assert!(!is_python_identifier("1package")); + assert!(!is_python_identifier("package-name")); + assert!(!is_python_identifier("package.name")); + assert!(!is_python_identifier("../package")); + } /// Uninstall must not remove files outside the install scheme. #[test] @@ -473,7 +518,7 @@ mod tests { let metadata = dist_info.child("METADATA"); metadata.touch().unwrap(); - // Something that looks sufficiently like a Unix venv. + // Something that looks sufficiently like a Unix environment. let layout = Layout { sys_executable: venv.path().join("bin/python"), python_version: (3, 13), @@ -500,12 +545,23 @@ mod tests { fn test_uninstall_egg_info_path_traversal() { let venv = assert_fs::TempDir::new().unwrap(); let site_packages = venv.child("lib/python3.12/site-packages"); - let outside_dir = assert_fs::TempDir::new().unwrap(); - // Create a directory outside site-packages that a malicious top_level.txt might target. - let target_dir = outside_dir.child("traversal_target"); + // Create directories outside site-packages, but inside the environment. Egg uninstall should + // still reject them, even though wheel RECORD entries may target other install-scheme + // directories. + let target_dir = venv.child("traversal_target"); let target_file = target_dir.child("secret.txt"); target_file.write_str("I should not be deleted").unwrap(); + let absolute_target_dir = venv.child("absolute_target"); + let absolute_target_file = absolute_target_dir.child("secret.txt"); + absolute_target_file + .write_str("I should not be deleted") + .unwrap(); + let invalid_name_dir = site_packages.child("invalid-name"); + let invalid_name_file = invalid_name_dir.child("secret.txt"); + invalid_name_file + .write_str("I should not be deleted") + .unwrap(); // Build a relative traversal path from site-packages to the target directory. let egg_info = site_packages.child("evilpkg-0.1.0.egg-info"); @@ -513,35 +569,31 @@ mod tests { let target_path = pathdiff::diff_paths(target_dir.path(), site_packages.path()).unwrap(); assert!(site_packages.join(&target_path).exists()); - // Create a fake egg-info directory with a top_level.txt containing a path traversal entry. + // Create a fake egg-info directory with `top_level.txt` entries containing relative and + // absolute paths. egg_info .child("top_level.txt") - .write_str(&format!("evilpkg\n{}\n", target_path.display())) + .write_str(&format!( + "evilpkg\n{}\n{}\ninvalid-name\n", + target_path.display(), + absolute_target_dir.path().display() + )) .unwrap(); // Also create the legitimate package directory so uninstall can remove it. let init_py = site_packages.child("evilpkg").child("__init__.py"); init_py.touch().unwrap(); - // Something that looks sufficiently like a Unix venv. - let layout = Layout { - sys_executable: venv.path().join("bin/python"), - python_version: (3, 13), - os_name: "posix".to_string(), - scheme: Scheme { - purelib: site_packages.to_path_buf(), - platlib: site_packages.to_path_buf(), - scripts: venv.path().join("bin"), - data: venv.path().to_path_buf(), - include: venv.path().join("include/python3.12"), - }, - }; - - uninstall_egg(egg_info.path(), "evilpkg 0.1.0", &layout).unwrap(); + uninstall_egg(egg_info.path(), "evilpkg 0.1.0").unwrap(); - // The regular package directory has been removed, while the directory outside the scheme still exists. + // The regular package directory has been removed, while the directories outside + // site-packages still exist. assert!(target_dir.exists()); assert!(target_file.exists()); + assert!(absolute_target_dir.exists()); + assert!(absolute_target_file.exists()); + assert!(invalid_name_dir.exists()); + assert!(invalid_name_file.exists()); assert!(!init_py.exists()); assert!(!egg_info.exists()); } @@ -571,20 +623,7 @@ mod tests { egg_info.create_dir_all().unwrap(); egg_info.child("top_level.txt").write_str("\n").unwrap(); - let layout = Layout { - sys_executable: venv.path().join("bin/python"), - python_version: (3, 13), - os_name: "posix".to_string(), - scheme: Scheme { - purelib: site_packages.to_path_buf(), - platlib: site_packages.to_path_buf(), - scripts: venv.path().join("bin"), - data: venv.path().to_path_buf(), - include: venv.path().join("include/python3.12"), - }, - }; - - uninstall_egg(egg_info.path(), "emptypkg 0.1.0", &layout).unwrap(); + uninstall_egg(egg_info.path(), "emptypkg 0.1.0").unwrap(); // The egg-info is gone, but the rest of site-packages (including the sibling // package) survives. @@ -628,20 +667,7 @@ mod tests { .write_str("\npkg_a\n \r\npkg_b\n\n") .unwrap(); - let layout = Layout { - sys_executable: venv.path().join("bin/python"), - python_version: (3, 13), - os_name: "posix".to_string(), - scheme: Scheme { - purelib: site_packages.to_path_buf(), - platlib: site_packages.to_path_buf(), - scripts: venv.path().join("bin"), - data: venv.path().to_path_buf(), - include: venv.path().join("include/python3.12"), - }, - }; - - uninstall_egg(egg_info.path(), "mixedpkg 0.1.0", &layout).unwrap(); + uninstall_egg(egg_info.path(), "mixedpkg 0.1.0").unwrap(); // The two named packages are gone, the egg-info is gone, and site-packages plus // the sibling survive. diff --git a/crates/uv-installer/src/uninstall.rs b/crates/uv-installer/src/uninstall.rs index eace930091c9b..3c203334099c9 100644 --- a/crates/uv-installer/src/uninstall.rs +++ b/crates/uv-installer/src/uninstall.rs @@ -13,11 +13,9 @@ pub async fn uninstall( InstalledDistKind::Registry(_) | InstalledDistKind::Url(_) => Ok( uv_install_wheel::uninstall_wheel(dist.install_path(), &dist, &layout)?, ), - InstalledDistKind::EggInfoDirectory(_) => Ok(uv_install_wheel::uninstall_egg( - dist.install_path(), - &dist, - &layout, - )?), + InstalledDistKind::EggInfoDirectory(_) => { + Ok(uv_install_wheel::uninstall_egg(dist.install_path(), &dist)?) + } InstalledDistKind::LegacyEditable(dist) => { Ok(uv_install_wheel::uninstall_legacy_editable(&dist.egg_link)?) } diff --git a/crates/uv/tests/it/pip_uninstall.rs b/crates/uv/tests/it/pip_uninstall.rs index e475ac5be99e2..6a0e2f015a4ef 100644 --- a/crates/uv/tests/it/pip_uninstall.rs +++ b/crates/uv/tests/it/pip_uninstall.rs @@ -535,7 +535,7 @@ fn uninstall_record_path_traversal() -> Result<()> { // Build the relative traversal path from site-packages to a target file outside // site-packages but inside the test temp dir. RECORD uses forward slashes, even on - // Windows, and the venv layout (and thus the traversal depth) differs by platform, + // Windows, and the environment layout (and thus the traversal depth) differs by platform, // so we construct the path manually and filter the leading `../` sequence out of the // snapshot above. let target_file = context.temp_dir.child("traversal_target.txt"); @@ -580,6 +580,67 @@ fn uninstall_record_path_traversal() -> Result<()> { Ok(()) } +/// Egg `top_level.txt` entries must be top-level names, not paths. +#[test] +fn uninstall_egg_info_top_level_path_traversal() -> Result<()> { + // The traversal-depth count differs between Unix (`.venv/lib/pythonX.Y/site-packages`) + // and Windows (`.venv/Lib/site-packages`), so normalize the `../` sequence in the warning. + let context = uv_test::test_context!("3.12") + .with_filter((r"(\.\./)+traversal_target", "[..]/traversal_target")); + + let site_packages = ChildPath::new(context.site_packages()); + + // Manually create a `name-version.egg-info` directory, which is recognized by the shared egg + // filename parser. + let egg_info = site_packages.child("evilpkg-0.1.0.egg-info"); + egg_info.create_dir_all()?; + + // The traversal target is outside site-packages but inside the environment, so a wheel RECORD entry + // could validly target this scheme area. An egg `top_level.txt` entry must not. + let target_dir = context.venv.child("traversal_target"); + let target_file = target_dir.child("secret.txt"); + target_file.write_str("I should not be deleted")?; + let invalid_name_dir = site_packages.child("invalid-name"); + let invalid_name_file = invalid_name_dir.child("secret.txt"); + invalid_name_file.write_str("I should not be deleted")?; + + let depth = context + .site_packages() + .strip_prefix(context.venv.path())? + .components() + .count(); + let traversal_entry = format!("{}traversal_target", "../".repeat(depth)); + assert!(context.site_packages().join(&traversal_entry).exists()); + + egg_info + .child("top_level.txt") + .write_str(&format!("evilpkg\n{traversal_entry}\ninvalid-name\n"))?; + + let init_py = site_packages.child("evilpkg").child("__init__.py"); + init_py.touch()?; + + uv_snapshot!(context.filters(), context.pip_uninstall() + .arg("evilpkg"), @" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + warning: Invalid `top_level.txt` entry in evilpkg==0.1.0 that is not a top-level module or package, skipping: [..]/traversal_target + Uninstalled 1 package in [TIME] + - evilpkg==0.1.0 + "); + + assert!(target_dir.exists()); + assert!(target_file.exists()); + assert!(invalid_name_dir.exists()); + assert!(invalid_name_file.exists()); + assert!(!init_py.exists()); + assert!(!egg_info.exists()); + + Ok(()) +} + /// `--yes` is accepted for `pip uninstall` compatibility, but emits a warning. #[test] fn yes_flag() { From 0bf76d588c352e8cf5fe107747502fb774fd3322 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 11 May 2026 18:54:55 -0400 Subject: [PATCH 2/2] Accept arbitrary, but reject paths --- Cargo.lock | 1 - Cargo.toml | 1 - crates/uv-install-wheel/Cargo.toml | 1 - crates/uv-install-wheel/src/uninstall.rs | 71 ++++++++---------------- crates/uv/tests/it/pip_uninstall.rs | 7 +-- 5 files changed, 23 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c6c45119aed52..d6c03e7ddcb10 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6644,7 +6644,6 @@ dependencies = [ "sha2", "thiserror 2.0.18", "tracing", - "unicode-ident", "uv-distribution-filename", "uv-flags", "uv-fs", diff --git a/Cargo.toml b/Cargo.toml index 0bcdb049a8e81..1a4d0aef15650 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -275,7 +275,6 @@ tracing-durations-export = { version = "0.3.0", features = ["plot"] } tracing-subscriber = { version = "0.3.18" } # Default feature set for uv_build, uv activates extra features tracing-test = { version = "0.2.5" } tracing-tree = { version = "0.4.0" } -unicode-ident = { version = "1.0.24" } unicode-width = { version = "0.2.0" } unscanny = { version = "0.1.0" } url = { version = "2.5.2", features = ["serde"] } diff --git a/crates/uv-install-wheel/Cargo.toml b/crates/uv-install-wheel/Cargo.toml index bed04cfdd323b..36664566fa64c 100644 --- a/crates/uv-install-wheel/Cargo.toml +++ b/crates/uv-install-wheel/Cargo.toml @@ -45,7 +45,6 @@ serde_json = { workspace = true } sha2 = { workspace = true } thiserror = { workspace = true } tracing = { workspace = true } -unicode-ident = { workspace = true } walkdir = { workspace = true } [target.'cfg(target_os = "windows")'.dependencies] diff --git a/crates/uv-install-wheel/src/uninstall.rs b/crates/uv-install-wheel/src/uninstall.rs index 91d6ffdcd2519..6853bb951c876 100644 --- a/crates/uv-install-wheel/src/uninstall.rs +++ b/crates/uv-install-wheel/src/uninstall.rs @@ -159,7 +159,7 @@ pub fn uninstall_wheel( }) } -static WARNED_FOR_PACKAGE: OnceLock>> = OnceLock::new(); +static WARNED_FOR_RECORD_ENTRY_PACKAGE: OnceLock>> = OnceLock::new(); static WARNED_FOR_EGG_TOP_LEVEL_PACKAGE: OnceLock>> = OnceLock::new(); /// Check if the path is inside the venv or a system interpreter path, and warn if it isn't. @@ -195,7 +195,7 @@ fn is_path_in_scheme( } else { // A package that does this is malformed to the point of being a risk to the user, be // annoying about it, but only once per package. - if WARNED_FOR_PACKAGE + if WARNED_FOR_RECORD_ENTRY_PACKAGE .get_or_init(|| Mutex::new(HashSet::new())) .lock() .expect("The mutex is broken, did some other thread panic?") @@ -213,10 +213,11 @@ fn is_path_in_scheme( /// Check that a `top_level.txt` entry names a single top-level module or package. /// -/// Unlike wheel `RECORD` entries, egg `top_level.txt` entries are Python identifiers, not paths. -/// Treating them as paths can make uninstall delete directories outside `site-packages`. +/// Unlike wheel `RECORD` entries, egg `top_level.txt` entries refer to direct children of the +/// egg's base location, not arbitrary paths. Treating them as paths can make uninstall delete +/// directories outside `site-packages`. fn is_valid_top_level_entry(entry: &str, distribution: impl Display) -> bool { - if is_python_identifier(entry) { + if is_safe_top_level_entry(entry) { true } else { if WARNED_FOR_EGG_TOP_LEVEL_PACKAGE @@ -235,14 +236,8 @@ fn is_valid_top_level_entry(entry: &str, distribution: impl Display) -> bool { } } -fn is_python_identifier(entry: &str) -> bool { - let mut chars = entry.chars(); - let Some(first) = chars.next() else { - return false; - }; - - (first == '_' || unicode_ident::is_xid_start(first)) - && chars.all(|character| character == '_' || unicode_ident::is_xid_continue(character)) +fn is_safe_top_level_entry(entry: &str) -> bool { + !entry.is_empty() && entry != "." && entry != ".." && !entry.contains(['/', '\\']) } /// Uninstall the egg represented by the `.egg-info` directory. @@ -466,20 +461,18 @@ mod tests { use uv_pypi_types::Scheme; use crate::Layout; - use crate::uninstall::{is_python_identifier, uninstall_egg, uninstall_wheel}; + use crate::uninstall::{is_safe_top_level_entry, uninstall_egg, uninstall_wheel}; #[test] - fn test_top_level_entry_python_identifier() { - assert!(is_python_identifier("package")); - assert!(is_python_identifier("_package")); - assert!(is_python_identifier("pkg_123")); - assert!(is_python_identifier("\u{00e9}clair")); - - assert!(!is_python_identifier("")); - assert!(!is_python_identifier("1package")); - assert!(!is_python_identifier("package-name")); - assert!(!is_python_identifier("package.name")); - assert!(!is_python_identifier("../package")); + fn test_top_level_entry_safe_name() { + assert!(is_safe_top_level_entry("package")); + + assert!(!is_safe_top_level_entry("")); + assert!(!is_safe_top_level_entry(".")); + assert!(!is_safe_top_level_entry("..")); + assert!(!is_safe_top_level_entry("../package")); + assert!(!is_safe_top_level_entry("package/name")); + assert!(!is_safe_top_level_entry(r"package\name")); } /// Uninstall must not remove files outside the install scheme. @@ -552,32 +545,16 @@ mod tests { let target_dir = venv.child("traversal_target"); let target_file = target_dir.child("secret.txt"); target_file.write_str("I should not be deleted").unwrap(); - let absolute_target_dir = venv.child("absolute_target"); - let absolute_target_file = absolute_target_dir.child("secret.txt"); - absolute_target_file - .write_str("I should not be deleted") - .unwrap(); - let invalid_name_dir = site_packages.child("invalid-name"); - let invalid_name_file = invalid_name_dir.child("secret.txt"); - invalid_name_file - .write_str("I should not be deleted") - .unwrap(); - // Build a relative traversal path from site-packages to the target directory. let egg_info = site_packages.child("evilpkg-0.1.0.egg-info"); egg_info.create_dir_all().unwrap(); let target_path = pathdiff::diff_paths(target_dir.path(), site_packages.path()).unwrap(); assert!(site_packages.join(&target_path).exists()); - // Create a fake egg-info directory with `top_level.txt` entries containing relative and - // absolute paths. + // Create a fake egg-info directory with a path traversal entry in `top_level.txt`. egg_info .child("top_level.txt") - .write_str(&format!( - "evilpkg\n{}\n{}\ninvalid-name\n", - target_path.display(), - absolute_target_dir.path().display() - )) + .write_str(&format!("evilpkg\n{}\n", target_path.display())) .unwrap(); // Also create the legitimate package directory so uninstall can remove it. @@ -586,14 +563,10 @@ mod tests { uninstall_egg(egg_info.path(), "evilpkg 0.1.0").unwrap(); - // The regular package directory has been removed, while the directories outside - // site-packages still exist. + // The regular package directory has been removed, while the directory outside + // site-packages still exists. assert!(target_dir.exists()); assert!(target_file.exists()); - assert!(absolute_target_dir.exists()); - assert!(absolute_target_file.exists()); - assert!(invalid_name_dir.exists()); - assert!(invalid_name_file.exists()); assert!(!init_py.exists()); assert!(!egg_info.exists()); } diff --git a/crates/uv/tests/it/pip_uninstall.rs b/crates/uv/tests/it/pip_uninstall.rs index 6a0e2f015a4ef..07d02d9cabe62 100644 --- a/crates/uv/tests/it/pip_uninstall.rs +++ b/crates/uv/tests/it/pip_uninstall.rs @@ -600,9 +600,6 @@ fn uninstall_egg_info_top_level_path_traversal() -> Result<()> { let target_dir = context.venv.child("traversal_target"); let target_file = target_dir.child("secret.txt"); target_file.write_str("I should not be deleted")?; - let invalid_name_dir = site_packages.child("invalid-name"); - let invalid_name_file = invalid_name_dir.child("secret.txt"); - invalid_name_file.write_str("I should not be deleted")?; let depth = context .site_packages() @@ -614,7 +611,7 @@ fn uninstall_egg_info_top_level_path_traversal() -> Result<()> { egg_info .child("top_level.txt") - .write_str(&format!("evilpkg\n{traversal_entry}\ninvalid-name\n"))?; + .write_str(&format!("evilpkg\n{traversal_entry}\n"))?; let init_py = site_packages.child("evilpkg").child("__init__.py"); init_py.touch()?; @@ -633,8 +630,6 @@ fn uninstall_egg_info_top_level_path_traversal() -> Result<()> { assert!(target_dir.exists()); assert!(target_file.exists()); - assert!(invalid_name_dir.exists()); - assert!(invalid_name_file.exists()); assert!(!init_py.exists()); assert!(!egg_info.exists());