diff --git a/CHANGELOG.md b/CHANGELOG.md index c18aaf47..996cf34b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Windows User PATH pollution from acceptance tests**: `PathRestoreGuard` sets + a test-only flag that makes `persist_path_dir` / Unix `persist_user_path` + skip durable User PATH, `~/.local/bin/nu`, and shell-profile writes while the + guard is held (it does not rewrite the Windows User PATH registry on drop, so + concurrent external PATH edits are preserved). `numan setup nu` / `use` also + refuse to persist paths under the system temp folder (fail closed if the temp + root cannot be canonicalized), so tempfile fixtures like `Temp\.tmp*\off` + cannot land on PATH again. If you already have those leftovers, clean User + PATH (PowerShell): + + ```powershell + $userPath = [Environment]::GetEnvironmentVariable('Path', 'User') + $cleaned = ($userPath -split ';' | Where-Object { + $_ -and ($_ -notmatch '(?i)\\Temp\\.tmp.*\\(off|existing-nu)$') + }) -join ';' + [Environment]::SetEnvironmentVariable('Path', $cleaned, 'User') + ``` + + Then open a new terminal and confirm with `$env:Path -split ';'`. + - **`numan setup nu`**: official Nushell 0.114.x release archives exceed the old 256 MiB extract cap (~279 MiB uncompressed on linux-gnu). Bootstrap now extracts only the `nu` binary (skipping bundled plugins) and raises the diff --git a/src/nu/bootstrap.rs b/src/nu/bootstrap.rs index f0443ea6..5bc8f179 100644 --- a/src/nu/bootstrap.rs +++ b/src/nu/bootstrap.rs @@ -435,6 +435,21 @@ pub fn persist_user_path(binary: &Path) -> Result<()> { } #[cfg(unix)] { + // Same test harness skip as `persist_path_dir_*`: PathRestoreGuard sets + // this so ignored acceptance tests cannot leave a dangling + // `~/.local/bin/nu` or shell-profile export after a tempfile fixture. + if std::env::var_os("NUMAN_TEST_NO_PERSIST_USER_PATH").is_some() { + return Ok(()); + } + // Match Windows `persist_path_dir` temp refuse: never durable-link a + // tempfile-rooted binary into `~/.local/bin/nu`. + if path_is_under_temp_dir(binary) { + bail!( + "Refusing to add temporary directory '{}' to the user PATH. \ + Install or register a stable Nushell location instead.", + binary.display() + ); + } persist_user_path_unix(binary)?; ensure_local_bin_on_path()?; Ok(()) @@ -569,7 +584,21 @@ pub fn register_existing_nu(binary: &Path, options: &NuSetupOptions) -> Result

Result<()> { + // Test harness sets this while PathRestoreGuard is held so ignored + // acceptance tests cannot permanently pollute the developer User PATH. + if std::env::var_os("NUMAN_TEST_NO_PERSIST_USER_PATH").is_some() { + return Ok(()); + } let dir = normalize_path_entry(dir); + // Refuse tempfile roots: test fixtures and one-off extracts must not land + // on the durable User PATH (seen as Temp\.tmp*\off / existing-nu leaks). + if path_is_under_temp_dir(&dir) { + bail!( + "Refusing to add temporary directory '{}' to the user PATH. \ + Install or register a stable Nushell location instead.", + dir.display() + ); + } let dir_str = dir .to_str() .with_context(|| format!("PATH entry '{}' is not valid UTF-8", dir.display()))?; @@ -586,6 +615,28 @@ fn persist_path_dir_windows(dir: &Path) -> Result<()> { Ok(()) } +fn path_is_under_temp_dir(dir: &Path) -> bool { + path_is_under_temp_dir_with(dir, &std::env::temp_dir()) +} + +/// Returns true when `dir` sits under `temp_raw`, failing closed if either +/// path cannot be canonicalized (lexical `starts_with` fallback). +fn path_is_under_temp_dir_with(dir: &Path, temp_raw: &Path) -> bool { + let Ok(temp) = temp_raw.canonicalize() else { + // Fail closed: an unresolvable temp root must still refuse lexical + // children (same fallback as an uncanonicalizable `dir`). + return match dir.canonicalize() { + Ok(d) => d.starts_with(temp_raw), + Err(_) => dir.starts_with(temp_raw), + }; + }; + let Ok(dir) = dir.canonicalize() else { + // If the dir vanished, still treat literal temp prefixes as unsafe. + return dir.starts_with(temp_raw); + }; + dir.starts_with(&temp) +} + #[cfg(unix)] fn shell_escape_for_double_quotes(value: &str) -> String { let mut escaped = String::with_capacity(value.len()); @@ -603,6 +654,16 @@ fn shell_escape_for_double_quotes(value: &str) -> String { #[cfg(unix)] fn persist_path_dir_unix(dir: &Path) -> Result<()> { + if std::env::var_os("NUMAN_TEST_NO_PERSIST_USER_PATH").is_some() { + return Ok(()); + } + if path_is_under_temp_dir(dir) { + bail!( + "Refusing to add temporary directory '{}' to the user PATH. \ + Install or register a stable Nushell location instead.", + dir.display() + ); + } let dir_str = dir .to_str() .with_context(|| format!("PATH entry '{}' is not valid UTF-8", dir.display()))?; @@ -1099,6 +1160,98 @@ mod tests { ); } + #[test] + fn persist_path_dir_refuses_temp_directories() { + use crate::util::test_paths::PathRestoreGuard; + // Hold the PATH mutex so concurrent tests do not race on the env flag. + let _guard = PathRestoreGuard::new(); + let dir = TempDir::new().unwrap(); + let nested = dir.path().join("off"); + std::fs::create_dir_all(&nested).unwrap(); + // Clear the test harness skip flag so we exercise the production refuse. + std::env::remove_var("NUMAN_TEST_NO_PERSIST_USER_PATH"); + let err = persist_path_dir(&nested).expect_err("temp dirs must not be persisted"); + let msg = format!("{err:#}"); + assert!( + msg.contains("temporary directory") || msg.contains("Refusing"), + "unexpected error: {msg}" + ); + } + + #[cfg(unix)] + #[test] + fn persist_user_path_honors_test_no_persist_flag() { + use crate::util::test_paths::PathRestoreGuard; + let _guard = PathRestoreGuard::new(); + let home = TempDir::new().unwrap(); + let original_home = std::env::var_os("HOME"); + std::env::set_var("HOME", home.path()); + + let binary = home.path().join("fixture-nu"); + std::fs::write(&binary, b"fake").unwrap(); + persist_user_path(&binary).expect("flag must no-op durable Unix PATH writes"); + + assert!( + !home.path().join(".local").join("bin").join("nu").exists(), + "must not create ~/.local/bin/nu while PathRestoreGuard is held" + ); + // No shell-profile export either. + for name in [".zshrc", ".bashrc", ".profile"] { + assert!( + !home.path().join(name).exists(), + "must not create {name} while PathRestoreGuard is held" + ); + } + + match original_home { + Some(h) => std::env::set_var("HOME", h), + None => std::env::remove_var("HOME"), + } + } + + #[cfg(unix)] + #[test] + fn persist_user_path_refuses_temp_binaries_without_flag() { + use crate::util::test_paths::PathRestoreGuard; + let _guard = PathRestoreGuard::new(); + std::env::remove_var("NUMAN_TEST_NO_PERSIST_USER_PATH"); + let dir = TempDir::new().unwrap(); + let binary = dir.path().join("nu"); + std::fs::write(&binary, b"fake").unwrap(); + let err = persist_user_path(&binary).expect_err("temp binaries must not be persisted"); + let msg = format!("{err:#}"); + assert!( + msg.contains("temporary directory") || msg.contains("Refusing"), + "unexpected error: {msg}" + ); + } + + #[test] + fn path_is_under_temp_dir_fails_closed_when_temp_uncanonicalizable() { + // A temp root that does not exist cannot be canonicalized; the helper + // must still refuse lexical children (fail closed), not return false. + let missing_temp = + std::env::temp_dir().join(format!("numan-missing-temp-root-{}", std::process::id())); + assert!( + !missing_temp.exists(), + "precondition: missing temp root must not exist" + ); + let nested = missing_temp.join("off"); + assert!( + path_is_under_temp_dir_with(&nested, &missing_temp), + "lexical child of an uncanonicalizable temp root must be refused" + ); + let outside = PathBuf::from(if cfg!(windows) { + r"C:\Windows\System32" + } else { + "/usr/bin" + }); + assert!( + !path_is_under_temp_dir_with(&outside, &missing_temp), + "unrelated paths must not match an uncanonicalizable temp root" + ); + } + #[test] fn nu_release_size_cap_exceeds_known_official_archive() { // Nu 0.114.1 x86_64-unknown-linux-gnu was ~279 MiB uncompressed and diff --git a/src/util/test_paths.rs b/src/util/test_paths.rs index e4b9333e..ee39d10c 100644 --- a/src/util/test_paths.rs +++ b/src/util/test_paths.rs @@ -4,6 +4,14 @@ //! any PATH mutation with [`PathRestoreGuard`] so concurrent tests cannot race //! on the process-global environment and so a developer shell is not left with //! a poisoned PATH after the test binary exits. +//! +//! On Windows (and Unix), the guard sets a test-only +//! `NUMAN_TEST_NO_PERSIST_USER_PATH` flag so production +//! [`crate::nu::bootstrap::persist_path_dir`] / `persist_user_path` skip durable +//! User PATH, `~/.local/bin/nu`, and shell-profile writes while the guard is +//! held. The guard does **not** rewrite the Windows User PATH registry value on +//! drop: a full snapshot restore would overwrite legitimate PATH changes made +//! by the user or another process during the test. use std::ffi::OsString; use std::sync::{Mutex, MutexGuard}; @@ -16,6 +24,9 @@ static PATH_MUTEX: Mutex<()> = Mutex::new(()); /// restores it on drop. Acquires a shared process-wide mutex so callers /// never need a separate `Mutex` for PATH serialization. /// +/// Also sets `NUMAN_TEST_NO_PERSIST_USER_PATH` for the guard lifetime so durable +/// PATH persistence is suppressed (and restored to its prior value on drop). +/// /// Use this around any test that mutates PATH so real-Nu runs from a /// developer terminal are not poisoned by the test process, and so /// parallel ignored acceptance tests cannot overwrite each other's PATH. @@ -29,10 +40,12 @@ static PATH_MUTEX: Mutex<()> = Mutex::new(()); /// ```text /// let _path_guard = PathRestoreGuard::new(); /// // mutate PATH... -/// // drop restores original PATH (or unsets it if it was unset) +/// // drop restores the original process PATH /// ``` pub struct PathRestoreGuard { original: Option, + /// Pre-existing `NUMAN_TEST_NO_PERSIST_USER_PATH` value (or `None` if absent). + previous_no_persist: Option, _lock: MutexGuard<'static, ()>, } @@ -41,8 +54,17 @@ impl PathRestoreGuard { let lock = PATH_MUTEX .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); + // Capture then set so Drop can restore a pre-existing value (or remove + // only when the variable was originally absent). + let previous_no_persist = std::env::var_os("NUMAN_TEST_NO_PERSIST_USER_PATH"); + // Block durable PATH writes for the duration of the guard. + // Production `persist_path_dir` / `persist_user_path` check this; + // without it, ignored acceptance tests permanently pollute developer + // User PATH / shell profiles with tempfile fixture dirs. + std::env::set_var("NUMAN_TEST_NO_PERSIST_USER_PATH", "1"); Self { original: std::env::var_os("PATH"), + previous_no_persist, _lock: lock, } } @@ -54,6 +76,10 @@ impl Drop for PathRestoreGuard { Some(path) => std::env::set_var("PATH", path), None => std::env::remove_var("PATH"), } + match self.previous_no_persist.as_ref() { + Some(prev) => std::env::set_var("NUMAN_TEST_NO_PERSIST_USER_PATH", prev), + None => std::env::remove_var("NUMAN_TEST_NO_PERSIST_USER_PATH"), + } } } @@ -62,3 +88,65 @@ impl Default for PathRestoreGuard { Self::new() } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Like [`PathRestoreGuard::new`], but installs `previous` under the PATH + /// mutex before capturing so parallel tests cannot race the set/capture window. + fn guard_with_previous_no_persist(previous: Option<&str>) -> PathRestoreGuard { + let lock = PATH_MUTEX + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + match previous { + Some(v) => std::env::set_var("NUMAN_TEST_NO_PERSIST_USER_PATH", v), + None => std::env::remove_var("NUMAN_TEST_NO_PERSIST_USER_PATH"), + } + let previous_no_persist = std::env::var_os("NUMAN_TEST_NO_PERSIST_USER_PATH"); + std::env::set_var("NUMAN_TEST_NO_PERSIST_USER_PATH", "1"); + PathRestoreGuard { + original: std::env::var_os("PATH"), + previous_no_persist, + _lock: lock, + } + } + + #[test] + fn path_restore_guard_preserves_no_persist_flag_across_drop() { + { + let guard = guard_with_previous_no_persist(Some("preexisting")); + assert_eq!( + std::env::var("NUMAN_TEST_NO_PERSIST_USER_PATH").as_deref(), + Ok("1") + ); + drop(guard); + // Re-acquire so parallel PathRestoreGuard users cannot flip the flag + // between Drop restore and our assertion. + let _lock = PATH_MUTEX + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + assert_eq!( + std::env::var("NUMAN_TEST_NO_PERSIST_USER_PATH").as_deref(), + Ok("preexisting") + ); + std::env::remove_var("NUMAN_TEST_NO_PERSIST_USER_PATH"); + } + + { + let guard = guard_with_previous_no_persist(None); + assert_eq!( + std::env::var("NUMAN_TEST_NO_PERSIST_USER_PATH").as_deref(), + Ok("1") + ); + drop(guard); + let _lock = PATH_MUTEX + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + assert!( + std::env::var_os("NUMAN_TEST_NO_PERSIST_USER_PATH").is_none(), + "flag must be removed when it was originally absent" + ); + } + } +}