Don’t run binaries that just happen to be in PATH

We only want our own managed crates.io installed binaries thanks.

Author: mxcl

README.md

@@ -93,7 +93,6 @@ You can override the default by setting:
    for already-installed binaries.
 
 2. **Binary lookup is restricted to**:
-   - Binaries already on your `PATH` (via `which`)
    - The `cargox` install directory only
 
 3. **Environment isolation**: When installing packages, `cargox` removes all

TESTING.md

@@ -50,29 +50,15 @@ Verifies that when no override is set, we use platform-appropriate XDG directori
 
 ### 3. Directory Isolation Tests
 
-These tests ensure we don't check or use standard Cargo/system directories:
+Directory isolation is enforced by refusing to execute binaries outside the sandboxed install directory (see the Execution Guard tests below).
 
-#### `candidate_bin_dirs_only_checks_cargox_dir`
-**Critical sandboxing test**: Verifies that:
-- Only 2 directories are checked (our install dir and install dir/bin)
-- No standard directories like `~/.cargo/bin` are included
-- All checked directories contain "cargox" in the path
+### 4. Execution Guard Tests
 
-#### `candidate_bin_dirs_ignores_cargo_env_vars`
-**Critical sandboxing test**: Verifies that even when Cargo-related environment variables are set, they are completely ignored:
-- `CARGO_INSTALL_ROOT` - ignored
-- `CARGO_HOME` - ignored
-- `BINSTALL_INSTALL_PATH` - ignored
+#### `allows_binaries_inside_install_dir`
+Unit test in `executor.rs` that ensures binaries located inside the sandboxed install directory are allowed to run.
 
-This ensures complete isolation from the user's Cargo configuration.
-
-### 4. Binary Resolution Tests
-
-#### `resolve_binary_path_checks_which_first`
-Verifies that we check `PATH` first via the `which` crate, allowing users to override with binaries already in their PATH.
-
-#### `resolve_binary_path_falls_back_to_cargox_dir`
-Verifies that after checking PATH, we fall back to our sandboxed directory.
+#### `rejects_binaries_outside_install_dir`
+Unit test in `executor.rs` that ensures we refuse to execute binaries that live outside the sandboxed install directory, preventing delegation to system-wide paths.
 
 ### 5. Environment Sanitization Tests
 
@@ -88,11 +74,6 @@ The function removes these variables before calling `cargo install` or `cargo-bi
 - `RUSTUP_HOME`
 - `RUSTUP_TOOLCHAIN`
 
-### 6. Override Tests
-
-#### `cargox_install_dir_override_is_respected`
-Verifies that users can override the install directory with `CARGOX_INSTALL_DIR` and that the override is properly respected.
-
 ## Running Tests
 
 ```bash

src/cli.rs

@@ -3,9 +3,9 @@ use clap::Parser;
 use std::env;
 use std::ffi::OsString;
 
-/// Run Cargo binaries on demand
+/// Run Cargo binaries on demand, installing them via `cargo-binstall` when missing.
 #[derive(Parser, Debug)]
-#[command(name = "cargox", author, version, about = "Run Cargo binaries on demand", long_about = None, arg_required_else_help = true)]
+#[command(name = "cargox", author, version, about, long_about = None, arg_required_else_help = true)]
 pub struct Cli {
     /// Crate to run, optionally suffixed with `@version`
     #[arg(value_name = "crate[@version]")]

src/executor.rs

@@ -1,9 +1,12 @@
-use anyhow::{Context, Result};
+use crate::paths::get_install_dir;
+use anyhow::{Context, Result, anyhow};
 use std::ffi::OsString;
 use std::path::Path;
 use std::process::{Command, ExitStatus};
 
 pub fn execute_binary(binary_path: &Path, args: &[OsString]) -> Result<ExitStatus> {
+    ensure_within_install_dir(binary_path)?;
+
     let mut cmd = Command::new(binary_path);
     cmd.args(args);
 
@@ -13,3 +16,59 @@ pub fn execute_binary(binary_path: &Path, args: &[OsString]) -> Result<ExitStatu
 
     Ok(status)
 }
+
+fn ensure_within_install_dir(binary_path: &Path) -> Result<()> {
+    let install_dir = get_install_dir()?;
+    ensure_binary_within_dir(binary_path, &install_dir)
+}
+
+fn ensure_binary_within_dir(binary_path: &Path, install_dir: &Path) -> Result<()> {
+    let install_dir = install_dir
+        .canonicalize()
+        .with_context(|| format!("failed to canonicalize {}", install_dir.display()))?;
+    let binary = binary_path.canonicalize().with_context(|| {
+        format!(
+            "failed to canonicalize binary path {}",
+            binary_path.display()
+        )
+    })?;
+
+    if !binary.starts_with(&install_dir) {
+        return Err(anyhow!(
+            "refusing to execute binary outside install dir: {}",
+            binary.display()
+        ));
+    }
+
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::fs;
+    use tempfile::tempdir;
+
+    #[test]
+    fn allows_binaries_inside_install_dir() {
+        let temp = tempdir().unwrap();
+        let bin_dir = temp.path().join("bin");
+        fs::create_dir_all(&bin_dir).unwrap();
+        let binary_path = bin_dir.join("tool");
+        fs::write(&binary_path, b"#!/bin/sh\n").unwrap();
+
+        let result = ensure_binary_within_dir(&binary_path, temp.path());
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn rejects_binaries_outside_install_dir() {
+        let temp = tempdir().unwrap();
+        let outside = tempdir().unwrap();
+        let binary_path = outside.path().join("tool");
+        fs::write(&binary_path, b"#!/bin/sh\n").unwrap();
+
+        let result = ensure_binary_within_dir(&binary_path, temp.path());
+        assert!(result.is_err());
+    }
+}

src/main.rs

@@ -4,6 +4,8 @@ mod installer;
 mod paths;
 mod registry;
 mod target;
+#[cfg(test)]
+mod test_support;
 mod versions;
 
 use std::path::PathBuf;
@@ -15,14 +17,12 @@ use semver::{Version, VersionReq};
 use cli::Cli;
 use executor::execute_binary;
 use installer::ensure_installed;
-use paths::resolve_binary_path;
 use registry::{fetch_highest_matching_version, fetch_latest_version};
 use target::{Target, VersionSpec, parse_spec};
 use versions::{find_installed_version, latest_installed, versioned_binary_path};
 
 enum RunPlan {
     UseInstalled { path: PathBuf },
-    UseSystem { path: PathBuf },
     InstallAndRun { version: Version },
 }
 
@@ -71,10 +71,6 @@ fn resolve_unspecified(target: &Target, cli: &Cli) -> Result<RunPlan> {
                 path: installed.path,
             });
         }
-
-        if let Ok(path) = resolve_binary_path(&target.binary) {
-            return Ok(RunPlan::UseSystem { path });
-        }
     }
 
     let version = fetch_latest_version(&target.crate_name)?;
@@ -116,7 +112,6 @@ fn resolve_requirement(target: &Target, cli: &Cli, requirement: &VersionReq) ->
 fn execute_plan(plan: &RunPlan, target: &Target, cli: &Cli) -> Result<ExitStatus> {
     match plan {
         RunPlan::UseInstalled { path } => execute_binary(path, &cli.args),
-        RunPlan::UseSystem { path } => execute_binary(path, &cli.args),
         RunPlan::InstallAndRun { version } => {
             ensure_installed(target, cli, version)?;
             let binary_path = versioned_binary_path(&target.binary, version)?;

src/paths.rs

@@ -33,40 +33,6 @@ pub fn get_install_dir() -> Result<PathBuf> {
     Err(anyhow!("unable to determine install directory"))
 }
 
-pub fn resolve_binary_path(name: &str) -> Result<PathBuf> {
-    if let Ok(path) = which::which(name) {
-        return Ok(path);
-    }
-
-    for dir in candidate_bin_dirs() {
-        let candidate = dir.join(name);
-        if candidate.exists() {
-            return Ok(candidate);
-        }
-        #[cfg(windows)]
-        {
-            let exe_candidate = candidate.with_extension("exe");
-            if exe_candidate.exists() {
-                return Ok(exe_candidate);
-            }
-        }
-    }
-
-    Err(anyhow!("cannot find binary path"))
-}
-
-fn candidate_bin_dirs() -> Vec<PathBuf> {
-    let mut dirs = Vec::new();
-
-    // Only check our sandboxed install directory
-    if let Ok(install_dir) = get_install_dir() {
-        dirs.push(install_dir.join("bin"));
-        dirs.push(install_dir);
-    }
-
-    dirs
-}
-
 fn home_dir() -> Option<PathBuf> {
     env::var_os("HOME")
         .or_else(|| env::var_os("USERPROFILE"))
@@ -76,9 +42,11 @@ fn home_dir() -> Option<PathBuf> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::test_support::env_lock;
 
     #[test]
     fn get_install_dir_respects_cargox_install_dir() {
+        let _guard = env_lock().lock().unwrap();
         let temp = tempfile::tempdir().unwrap();
         let custom_path = temp.path().to_path_buf();
 
@@ -95,6 +63,7 @@ mod tests {
 
     #[test]
     fn get_install_dir_ignores_cargo_install_root() {
+        let _guard = env_lock().lock().unwrap();
         let temp = tempfile::tempdir().unwrap();
         let cargo_root = temp.path().join("cargo_root");
 
@@ -114,6 +83,7 @@ mod tests {
 
     #[test]
     fn get_install_dir_uses_xdg_directories() {
+        let _guard = env_lock().lock().unwrap();
         // Clear any override env vars
         unsafe {
             env::remove_var("CARGOX_INSTALL_DIR");
@@ -134,129 +104,4 @@ mod tests {
         #[cfg(target_os = "windows")]
         assert!(result.to_string_lossy().contains("AppData"));
     }
-
-    #[test]
-    fn candidate_bin_dirs_only_checks_cargox_dir() {
-        // Clear any override env vars
-        unsafe {
-            env::remove_var("CARGOX_INSTALL_DIR");
-        }
-
-        let dirs = candidate_bin_dirs();
-
-        // Should only have 2 entries: install_dir/bin and install_dir
-        assert_eq!(dirs.len(), 2);
-
-        // Both should contain "cargox"
-        for dir in &dirs {
-            assert!(dir.to_string_lossy().contains("cargox"));
-        }
-
-        // Should NOT contain any standard cargo directories
-        let dir_strings: Vec<String> = dirs
-            .iter()
-            .map(|d| d.to_string_lossy().to_string())
-            .collect();
-
-        for dir_str in &dir_strings {
-            assert!(
-                !dir_str.contains("/.cargo/bin"),
-                "Should not check ~/.cargo/bin, found: {dir_str}"
-            );
-            assert!(
-                !dir_str.contains("\\.cargo\\bin"),
-                "Should not check ~/.cargo/bin, found: {dir_str}"
-            );
-        }
-    }
-
-    #[test]
-    fn candidate_bin_dirs_ignores_cargo_env_vars() {
-        let temp = tempfile::tempdir().unwrap();
-
-        // Set various cargo-related env vars
-        unsafe {
-            env::set_var("CARGO_INSTALL_ROOT", temp.path().join("cargo_root"));
-            env::set_var("CARGO_HOME", temp.path().join("cargo_home"));
-            env::set_var("BINSTALL_INSTALL_PATH", temp.path().join("binstall"));
-        }
-
-        let dirs = candidate_bin_dirs();
-
-        // Clean up
-        unsafe {
-            env::remove_var("CARGO_INSTALL_ROOT");
-            env::remove_var("CARGO_HOME");
-            env::remove_var("BINSTALL_INSTALL_PATH");
-        }
-
-        // None of the candidate dirs should match the env var paths
-        let dir_strings: Vec<String> = dirs
-            .iter()
-            .map(|d| d.to_string_lossy().to_string())
-            .collect();
-
-        for dir_str in &dir_strings {
-            assert!(
-                !dir_str.contains("cargo_root"),
-                "Should ignore CARGO_INSTALL_ROOT, found: {dir_str}"
-            );
-            assert!(
-                !dir_str.contains("cargo_home"),
-                "Should ignore CARGO_HOME, found: {dir_str}"
-            );
-            assert!(
-                !dir_str.contains("binstall"),
-                "Should ignore BINSTALL_INSTALL_PATH, found: {dir_str}"
-            );
-        }
-    }
-
-    #[test]
-    fn resolve_binary_path_checks_which_first() {
-        // This test verifies that we check PATH first via which
-        // We use a binary that's definitely on PATH (like "ls" on Unix or "cmd" on Windows)
-        #[cfg(unix)]
-        let result = resolve_binary_path("ls");
-
-        #[cfg(windows)]
-        let result = resolve_binary_path("cmd");
-
-        // Should find the system binary via which
-        assert!(result.is_ok());
-    }
-
-    #[test]
-    fn resolve_binary_path_falls_back_to_cargox_dir() {
-        // Clear any override env vars
-        unsafe {
-            env::remove_var("CARGOX_INSTALL_DIR");
-        }
-
-        // Try to find a binary that doesn't exist
-        let result = resolve_binary_path("this_binary_definitely_does_not_exist_12345");
-
-        // Should fail because it's not in PATH and not in our install dir
-        assert!(result.is_err());
-    }
-
-    #[test]
-    fn cargox_install_dir_override_is_respected() {
-        let temp = tempfile::tempdir().unwrap();
-        let custom_dir = temp.path().join("my_custom_cargox");
-
-        unsafe {
-            env::set_var("CARGOX_INSTALL_DIR", &custom_dir);
-        }
-
-        let dirs = candidate_bin_dirs();
-
-        unsafe {
-            env::remove_var("CARGOX_INSTALL_DIR");
-        }
-
-        // Should use the custom directory
-        assert!(dirs.iter().any(|d| d == &custom_dir.join("bin")));
-        assert!(dirs.iter().any(|d| d == &custom_dir));
-    }
 }

src/test_support.rs

@@ -0,0 +1,6 @@
+use std::sync::{Mutex, OnceLock};
+
+pub fn env_lock() -> &'static Mutex<()> {
+    static LOCK: OnceLock<Mutex<()>> = OnceLock::new();
+    LOCK.get_or_init(|| Mutex::new(()))
+}