Security best practices and guidelines for the Jarvy CLI codebase - a cross-platform development environment provisioning tool that executes system commands with elevated privileges
This skill provides comprehensive security guidelines for developing and maintaining the Jarvy CLI codebase. Jarvy is a cross-platform tool that provisions development environments by executing package manager commands (brew, apt, dnf, winget, etc.) with potential privilege escalation.
Jarvy operates in a high-risk security context:
sudo on POSIX systems for package installationJarvy passes user-provided tool names and versions to package manager commands. A malicious jarvy.toml could attempt command injection.
Always use Command::new() with separate arguments - NEVER shell string interpolation:
// CORRECT: Arguments are passed as separate array elements
// Each argument is escaped by the OS, preventing injection
use std::process::Command;
pub fn run(cmd: &str, args: &[&str]) -> Result<Output, InstallError> {
Command::new(cmd)
.args(args) // Safe: each arg is a separate element
.output()
.map_err(|e| /* handle error */)
}
// Usage - safe even with untrusted input
run("brew", &["install", user_provided_package_name])?;
run("apt-get", &["install", "-y", package_name])?;
run("winget", &["install", "-e", "--id", package_id])?;
// DANGEROUS: Never concatenate user input into shell commands
let cmd = format!("brew install {}", user_input); // VULNERABLE
Command::new("sh").args(["-c", &cmd]).output()?; // VULNERABLE
// DANGEROUS: Using shell expansion with user data
let script = format!("curl ... | sh -s -- {}", user_version); // VULNERABLE
For package managers that use IDs, hardcode known-safe values:
// CORRECT: Package IDs are compile-time constants
const GIT_WINGET_ID: &str = "Git.Git";
const RUSTUP_WINGET_ID: &str = "Rustlang.Rustup";
fn install_windows() -> Result<(), InstallError> {
run("winget", &["install", "-e", "--id", GIT_WINGET_ID])?;
Ok(())
}
Validate tool names against an allowlist before any operations:
// CORRECT: Only process tools registered in the known registry
pub fn add(name: &str, version: &str) -> Result<(), InstallError> {
let key = name.to_ascii_lowercase(); // Normalize
let map = registry().read().expect("registry rwlock poisoned");
if let Some(handler) = map.get(&key) {
// Only known tools can be processed
let f = *handler;
drop(map);
f(version)
} else {
Err(InstallError::Parse("unknown tool")) // Reject unknown tools
}
}
Version hints should be validated to prevent injection via version parameters:
// CORRECT: Validate version strings contain only expected characters
fn validate_version_hint(hint: &str) -> bool {
// Allow: digits, dots, hyphens, plus signs (semver compatible)
hint.chars().all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '+'))
}
// CORRECT: Use version for comparison only, not command construction
pub fn cmd_satisfies(cmd: &str, min_prefix: &str) -> bool {
if let Ok(out) = Command::new(cmd).arg("--version").output() {
let s = String::from_utf8_lossy(&out.stdout);
return s.contains(min_prefix); // Read-only comparison
}
false
}
Improper sudo handling can lead to privilege escalation attacks or unintended system modifications.
// CORRECT: Sudo is a conscious decision, not automatic
pub fn run_maybe_sudo(use_sudo: bool, cmd: &str, args: &[&str]) -> Result<Output, InstallError> {
match current_os() {
Os::Windows => run(cmd, args), // No sudo on Windows
Os::Linux | Os::Macos => {
if use_sudo {
// Prepend sudo as a separate command
let mut all = Vec::with_capacity(1 + args.len());
all.push(cmd);
all.extend_from_slice(args);
run("sudo", &all) // sudo is the command, original cmd is first arg
} else {
run(cmd, args)
}
}
}
}
// CORRECT: Check if sudo is available before attempting
pub fn has(cmd: &str) -> bool {
Command::new(cmd)
.arg("--version")
.output()
.map(|o| o.status.success())
.unwrap_or(false)
}
// CORRECT: Allow users to control sudo behavior via config
pub fn plan_sudo_attempts(use_sudo: Option<bool>, sudo_available: bool) -> Vec<bool> {
match use_sudo {
Some(flag) => vec![flag], // User explicitly set preference
None => {
if sudo_available {
vec![false, true] // Try without first, then with
} else {
vec![false] // No sudo available
}
}
}
}
// DANGEROUS: Always using sudo without user consent
fn install_package(pkg: &str) -> Result<(), Error> {
run("sudo", &["apt-get", "install", pkg])?; // WRONG: No user control
Ok(())
}
// DANGEROUS: Sudo in shell strings
let cmd = "sudo apt-get install -y package"; // WRONG: Shell injection risk
Command::new("sh").args(["-c", cmd]).output()?;
// CORRECT: Distinct error types for permission issues
#[derive(thiserror::Error, Debug)]
pub enum InstallError {
#[error("invalid permissions: {0}")]
InvalidPermissions(&'static str),
// ...
}
// CORRECT: Map permission errors to specific exit codes
match e.kind() {
std::io::ErrorKind::PermissionDenied => {
InstallError::InvalidPermissions("operation requires elevated privileges")
}
// ...
}
User-specified config file paths could access sensitive system files.
use std::path::Path;
// CORRECT: Canonicalize and validate paths
fn safe_read_config(path: &str) -> Result<String, ConfigError> {
let path = Path::new(path);
// Canonicalize to resolve symlinks and ../ sequences
let canonical = path.canonicalize()
.map_err(|_| ConfigError::InvalidPath)?;
// Ensure file has expected extension
if canonical.extension() != Some(std::ffi::OsStr::new("toml")) {
return Err(ConfigError::InvalidExtension);
}
std::fs::read_to_string(canonical)
.map_err(ConfigError::IoError)
}
// CORRECT: Global config is always in a known location
pub fn initialize() -> CliConfig {
let home_dir = dirs::home_dir().expect("Failed to get home directory");
let jarvy_dir = home_dir.join(".jarvy"); // Fixed subdirectory
let config_file_path = jarvy_dir.join("config.toml"); // Fixed filename
// Only read from this specific path
let config_content = fs::read_to_string(&config_file_path).unwrap_or_default();
// ...
}
// DANGEROUS: Directly using user path without validation
fn read_config(path: &str) -> String {
fs::read_to_string(path).unwrap() // WRONG: No validation
}
// DANGEROUS: String concatenation for paths
let path = format!("{}/{}", user_dir, user_filename); // WRONG: Path injection
Telemetry could leak PII, credentials, or sensitive system information.
// CORRECT: Use hardware-based anonymous fingerprint
fn get_hwid_fingerprint() -> Result<String, Box<dyn std::error::Error>> {
let mut builder = IdBuilder::new(Encryption::SHA256);
// Components that identify the machine, not the user
builder
.add_component(HWIDComponent::SystemID)
.add_component(HWIDComponent::CPUCores)
.add_component(HWIDComponent::OSName)
.add_component(HWIDComponent::DriveSerial);
// Salted hash - cannot be reversed to original values
const SALT: &str = "9f86d081884c7d659a2feaa0c55ad015...";
builder.build(SALT)
}
// CORRECT: Allow users to disable telemetry
pub fn init(enable_analytics: bool, distinct_id: String) {
// Environment variable override
let env_disable = std::env::var("JARVY_ANALYTICS").ok();
let enabled = match env_disable.as_deref() {
Some("0") | Some("false") => false, // Honor user preference
_ => enable_analytics,
};
// Clear documentation about telemetry
println!(r"
Jarvy collects telemetry data to help us improve your experience.
The data collected is anonymized...
If you wish to opt-out, add to ~/.jarvy/config.toml:
[settings]
telemetry = false
");
}
// CORRECT: Only collect necessary, non-sensitive data
pub fn capture(event: &str, mut properties: serde_json::Map<String, serde_json::Value>) {
// Safe to collect: OS type, shell type, CLI version
properties.entry("os".to_string())
.or_insert(serde_json::Value::String(detect_os()));
properties.entry("shell".to_string())
.or_insert(serde_json::Value::String(detect_shell()));
properties.entry("version".to_string())
.or_insert(serde_json::Value::String(env!("CARGO_PKG_VERSION").to_string()));
// DO NOT collect: usernames, paths containing usernames, environment variables
// containing secrets, command arguments that might contain credentials
}
// DANGEROUS: Collecting potentially sensitive data
properties.insert("username", std::env::var("USER").unwrap()); // WRONG: PII
properties.insert("home_dir", dirs::home_dir().display()); // WRONG: Contains username
properties.insert("full_command", std::env::args().collect()); // WRONG: May contain secrets
properties.insert("env_vars", std::env::vars().collect()); // WRONG: Contains secrets
Network requests could be intercepted (MITM) or directed to malicious endpoints.
// CORRECT: Use HTTPS endpoints with proper certificate validation
// ureq performs certificate validation by default
let response = ureq::post(&format!("{}/capture/", c.host)) // host should be https://
.header("Content-Type", "application/json")
.send_json(payload)?;
// CORRECT: Hardcode HTTPS URLs for installers
const RUSTUP_URL: &str = "https://sh.rustup.rs";
run("bash", &["-lc",
"curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y"
])?;
// CORRECT: Enforce modern TLS in curl commands
"curl --proto '=https' --tlsv1.2 -sSf https://example.com"
// ^^^^^^^^^^^^^^^ only HTTPS
// ^^^^^^^^^^^ TLS 1.2 minimum
// DANGEROUS: HTTP endpoints
let host = "http://api.example.com"; // WRONG: No TLS
// DANGEROUS: Disabling certificate verification
// (ureq doesn't easily allow this, but some HTTP clients do)
// DANGEROUS: Downloading scripts without verification
run("bash", &["-c", "curl http://example.com/script.sh | bash"])?; // WRONG: HTTP + piped execution
Parsing command output incorrectly can lead to logic errors or information disclosure.
// CORRECT: Use lossy conversion to handle malformed output safely
if !out.status.success() {
return Err(InstallError::CommandFailed {
cmd: cmd.to_string(),
code: out.status.code(),
stderr: String::from_utf8_lossy(&out.stderr).into(), // Safe conversion
});
}
// CORRECT: Version checking with contains (not parsing)
pub fn cmd_satisfies(cmd: &str, min_prefix: &str) -> bool {
if let Ok(out) = Command::new(cmd).arg("--version").output() {
let s = String::from_utf8_lossy(&out.stdout);
return s.contains(min_prefix); // Simple substring check
}
false
}
// CORRECT: Handle parsing failures gracefully
let config: CliConfig = {
let config_content = fs::read_to_string(&config_file_path).unwrap_or_default();
if config_content.trim().is_empty() {
CliConfig::default() // Safe fallback
} else {
toml::from_str(&config_content).unwrap_or_default() // Safe fallback on parse error
}
};
// DANGEROUS: Assuming output is valid UTF-8
let output = String::from_utf8(out.stdout).unwrap(); // WRONG: Can panic
// DANGEROUS: Complex regex on untrusted output without limits
let re = Regex::new(r"version (\d+)\.(\d+)\.(\d+)").unwrap();
// Consider: What if output is 10MB of garbage?
// DANGEROUS: Executing parsed output
let version = parse_version(&output);
run("install", &[&format!("package@{}", version)])?; // WRONG: Trusting parsed data
Downloading and executing scripts from the internet is inherently dangerous.
// CORRECT: Pin to specific versions when downloading scripts
const NVM_VERSION: &str = "v0.39.7";
const NVM_INSTALL_URL: &str =
"https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.7/install.sh";
fn install_posix() -> Result<(), InstallError> {
run("bash", &["-lc",
&format!("curl -fsSL {} | bash", NVM_INSTALL_URL)
])?;
Ok(())
}
// CORRECT: Only use official, well-known installer URLs
// rustup: https://sh.rustup.rs (official Rust project)
// nvm: https://raw.githubusercontent.com/nvm-sh/nvm/... (official repo)
// Homebrew: https://raw.githubusercontent.com/Homebrew/install/...
// Document the trust chain in comments
/// Install Rust via rustup using the official installer.
/// Source: https://rustup.rs (maintained by Rust project)
fn install_unix() -> Result<(), InstallError> {
run("bash", &["-lc",
"curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y"
])
}
// DANGEROUS: User-provided URLs
fn install_from_url(url: &str) -> Result<(), Error> {
run("bash", &["-c", &format!("curl {} | bash", url)])?; // WRONG
Ok(())
}
// DANGEROUS: Unpinned versions
"curl -fsSL https://example.com/install.sh | bash" // WRONG: Gets latest, could change
Detailed error messages can leak sensitive system information.
// CORRECT: Error types that don't leak paths or system details
#[derive(thiserror::Error, Debug)]
pub enum InstallError {
#[error("unsupported platform")]
Unsupported,
#[error("prerequisite missing: {0}")]
Prereq(&'static str), // Static string, not dynamic path
#[error("invalid permissions: {0}")]
InvalidPermissions(&'static str),
#[error("command failed: {cmd} (code: {code:?})\n{stderr}")]
CommandFailed {
cmd: String, // Command name only
code: Option<i32>,
stderr: String, // May contain sensitive info - log carefully
},
}
// CORRECT: Sanitize errors before sending to telemetry
pub fn capture_exception(
message: &str,
exception_type: &str,
stack_trace: Option<String>,
context: serde_json::Map<String, serde_json::Value>,
) {
// Filter or redact sensitive information from stack traces
let safe_stack = stack_trace.map(|s| redact_paths(&s));
// Don't include full file paths in telemetry
let mut safe_context = context.clone();
safe_context.remove("full_path");
safe_context.remove("home_directory");
// Send sanitized data
capture("$exception", props);
}
// DANGEROUS: Including full paths in errors
eprintln!("Failed to read: {}", full_path); // WRONG: Exposes filesystem structure
// DANGEROUS: Including environment in errors
eprintln!("Error with env: {:?}", std::env::vars()); // WRONG: Leaks secrets
// DANGEROUS: Stack traces to users
panic!("Unexpected error: {:?}", e); // WRONG: May contain sensitive info
cargo audit and cargo deny regularly# Security-relevant dependencies in Jarvy:
# ureq - HTTP client
# - Uses rustls by default (no OpenSSL dependency issues)
# - Certificate validation enabled by default
ureq = { version = "3.1.2", features = ["json"] }
# serde/toml - Config parsing
# - Well-audited, widely used
# - Deserialize untrusted input with care
serde = { version = "1.0.204", features = ["derive"] }
toml = "0.9.5"
# machineid-rs - Hardware fingerprinting
# - Review what data it collects
# - Ensure it doesn't collect PII
machineid-rs = "1.2"
// CORRECT: Disable dangerous operations in tests
pub fn run(cmd: &str, args: &[&str]) -> Result<Output, InstallError> {
// Fast tests skip external commands entirely
if std::env::var_os("JARVY_FAST_TEST").is_some() {
return Err(InstallError::Prereq("skipped in fast test mode"));
}
// Unit tests don't run external commands by default
#[cfg(test)]
{
if std::env::var_os("JARVY_RUN_EXTERNAL_CMDS_IN_TEST").is_none() {
return Err(InstallError::Prereq("external commands disabled in tests"));
}
}
// Actual command execution...
}
// CORRECT: Test mode avoids interactive prompts
fn user_select() {
if std::env::var("JARVY_TEST_MODE").as_deref() == Ok("1") {
println!("TEST: user_select invoked");
return; // Skip interactive UI
}
// ...
}
When reviewing PRs, verify:
Command::new() calls use argument arrays, not concatenated stringsIf you discover a security vulnerability in Jarvy: