diff --git a/.gitignore b/.gitignore index 15ab3a1..809cc8f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,2 @@ target/ bin/ -completions/ diff --git a/Cargo.lock b/Cargo.lock index bd32f1d..d195e8b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -78,7 +78,7 @@ checksum = "b94f61472cee1439c0b966b47e3aca9ae07e45d070759512cd390ea2bebc6675" [[package]] name = "eh" -version = "0.1.3" +version = "0.1.2" dependencies = [ "clap", "regex", @@ -438,7 +438,7 @@ checksum = "f17a85883d4e6d00e8a97c586de764dabcc06133f7f1d55dce5cdc070ad7fe59" [[package]] name = "xtask" -version = "0.1.3" +version = "0.1.2" dependencies = [ "clap", "clap_complete", diff --git a/Cargo.toml b/Cargo.toml index 9cc10d5..dda7e17 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ edition = "2024" license = "MPL-2.0" readme = true rust-version = "1.89" -version = "0.1.3" +version = "0.1.2" [workspace.dependencies] clap = { default-features = false, features = [ "std", "help", "derive" ], version = "4.5.51" } diff --git a/REPORT.md b/REPORT.md deleted file mode 100644 index 50d4355..0000000 --- a/REPORT.md +++ /dev/null @@ -1,932 +0,0 @@ -# Code Review Report - -**Review Date**: 2025-11-15\ -**Reviewed Files**: eh/src/main.rs, eh/src/lib.rs, eh/src/command.rs, -eh/src/error.rs, eh/src/run.rs, eh/src/shell.rs, eh/src/util.rs, -eh/src/build.rs, eh/Cargo.toml, Cargo.toml, xtask/src/main.rs\ -**Total Issues Found**: 12 - ---- - -## Executive Summary - -I conducted a comprehensive code review of the "eh" ergonomic Nix helper -project, examining all 11 Rust source files totaling 4,159 lines of code. The -codebase is well-structured with clear separation of concerns, implementing a -command-line tool that wraps Nix commands with intelligent retry logic for hash -mismatches and license issues. The architecture uses traits for extensibility -and follows Rust best practices for error handling. - -The review identified 12 issues across severity levels: 3 critical issues -related to potential security vulnerabilities and error handling, 5 major issues -concerning code robustness and maintainability, and 4 minor issues related to -code quality and documentation. The most significant concerns involve -insufficient input validation in multicall scenarios, potential resource leaks -in command execution, and inconsistent error handling patterns. - ---- - -## Critical Issues (Severity: High) - -### Issue #1: Insufficient Input Validation in Multicall Dispatch - -**File**: `eh/src/main.rs`\ -**Lines**: 34-70\ -**Severity**: High\ -**Category**: Security - -**Problem Description**: The `dispatch_multicall` function processes -command-line arguments without validation before passing them to Nix commands. -This could allow injection of dangerous shell commands through specially crafted -arguments when the tool is invoked as multicall binaries (nr, ns, nb). - -**Current Code**: - -```rust -fn dispatch_multicall( - app_name: &str, - args: std::env::Args, -) -> Option> { - let rest: Vec = args.collect(); - let hash_extractor = util::RegexHashExtractor; - let fixer = util::DefaultNixFileFixer; - let classifier = util::DefaultNixErrorClassifier; - - match app_name { - "nr" => { - Some(run::handle_nix_run( - &rest, - &hash_extractor, - &fixer, - &classifier, - )) - }, - "ns" => { - Some(shell::handle_nix_shell( - &rest, - &hash_extractor, - &fixer, - &classifier, - )) - }, - "nb" => { - Some(build::handle_nix_build( - &rest, - &hash_extractor, - &fixer, - &classifier, - )) - }, - _ => None, - } -} -``` - -**Recommended Fix**: - -```rust -fn dispatch_multicall( - app_name: &str, - args: std::env::Args, -) -> Option> { - let rest: Vec = args.collect(); - - // Validate arguments before processing - if let Err(e) = util::validate_nix_args(&rest) { - return Some(Err(e)); - } - - let hash_extractor = util::RegexHashExtractor; - let fixer = util::DefaultNixFileFixer; - let classifier = util::DefaultNixErrorClassifier; - - match app_name { - "nr" => { - Some(run::handle_nix_run( - &rest, - &hash_extractor, - &fixer, - &classifier, - )) - }, - "ns" => { - Some(shell::handle_nix_shell( - &rest, - &hash_extractor, - &fixer, - &classifier, - )) - }, - "nb" => { - Some(build::handle_nix_build( - &rest, - &hash_extractor, - &fixer, - &classifier, - )) - }, - _ => None, - } -} -``` - -**Rationale**: Adding input validation in the multicall dispatch ensures that -dangerous arguments are caught early, regardless of how the tool is invoked. -This prevents command injection attacks that could bypass the validation in -`handle_nix_with_retry`. - -**Impact if Unfixed**: Attackers could execute arbitrary commands by passing -malicious arguments to multicall binaries, potentially leading to system -compromise. - ---- - -### Issue #2: Resource Leak in Non-Interactive Command Execution - -**File**: `eh/src/command.rs`\ -**Lines**: 151-179\ -**Severity**: High\ -**Category**: Performance/Resource Management - -**Problem Description**: The `run_with_logs` method creates child processes with -piped stdout/stderr but does not properly handle the case where the child -process hangs or produces infinite output, potentially causing resource -exhaustion. - -**Current Code**: - -```rust -loop { - let mut did_something = false; - - match stdout.read(&mut out_buf) { - Ok(0) => {}, - Ok(n) => { - interceptor.on_stdout(&out_buf[..n]); - out_queue.push_back(Vec::from(&out_buf[..n])); - did_something = true; - }, - Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => {}, - Err(e) => return Err(EhError::Io(e)), - } - - match stderr.read(&mut err_buf) { - Ok(0) => {}, - Ok(n) => { - interceptor.on_stderr(&err_buf[..n]); - err_queue.push_back(Vec::from(&err_buf[..n])); - did_something = true; - }, - Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => {}, - Err(e) => return Err(EhError::Io(e)), - } - - if !did_something && child.try_wait()?.is_some() { - break; - } -} -``` - -**Recommended Fix**: - -```rust -use std::time::{Duration, Instant}; - -let start_time = Instant::now(); -const TIMEOUT: Duration = Duration::from_secs(300); // 5 minute timeout - -loop { - let mut did_something = false; - - // Check for timeout - if start_time.elapsed() > TIMEOUT { - let _ = child.kill(); - return Err(EhError::CommandFailed { - command: format!("nix {} timed out after 5 minutes", self.subcommand), - }); - } - - match stdout.read(&mut out_buf) { - Ok(0) => {}, - Ok(n) => { - interceptor.on_stdout(&out_buf[..n]); - out_queue.push_back(Vec::from(&out_buf[..n])); - did_something = true; - }, - Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => {}, - Err(e) => return Err(EhError::Io(e)), - } - - match stderr.read(&mut err_buf) { - Ok(0) => {}, - Ok(n) => { - interceptor.on_stderr(&err_buf[..n]); - err_queue.push_back(Vec::from(&err_buf[..n])); - did_something = true; - }, - Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => {}, - Err(e) => return Err(EhError::Io(e)), - } - - if !did_something && child.try_wait()?.is_some() { - break; - } - - // Prevent busy waiting - if !did_something { - std::thread::sleep(Duration::from_millis(10)); - } -} -``` - -**Rationale**: Adding a timeout prevents infinite loops from hanging the -application, and a small sleep prevents CPU spinning when no data is available. - -**Impact if Unfixed**: Malicious or buggy Nix commands could cause the -application to hang indefinitely, consuming system resources and potentially -causing denial of service. - ---- - -### Issue #3: Inconsistent Error Code Mapping - -**File**: `eh/src/error.rs`\ -**Lines**: 40-47\ -**Severity**: High\ -**Category**: Logic/Error Handling - -**Problem Description**: The `exit_code` method returns the same exit code (1) -for most error types, making it impossible to distinguish between different -failure modes in scripts that depend on the tool. - -**Current Code**: - -```rust -impl EhError { - #[must_use] - pub const fn exit_code(&self) -> i32 { - match self { - Self::ProcessExit { code } => *code, - Self::NixCommandFailed(_) => 1, - Self::CommandFailed { .. } => 1, - _ => 1, - } - } -} -``` - -**Recommended Fix**: - -```rust -impl EhError { - #[must_use] - pub const fn exit_code(&self) -> i32 { - match self { - Self::ProcessExit { code } => *code, - Self::NixCommandFailed(_) => 2, - Self::CommandFailed { .. } => 3, - Self::HashExtractionFailed => 4, - Self::NoNixFilesFound => 5, - Self::HashFixFailed { .. } => 6, - Self::InvalidInput { .. } => 7, - Self::Io(_) => 8, - Self::Regex(_) => 9, - Self::Utf8(_) => 10, - } - } -} -``` - -**Rationale**: Unique exit codes for each error type allow scripts and -automation to handle different failure scenarios appropriately. - -**Impact if Unfixed**: Scripts cannot distinguish between different types of -failures, making error handling and recovery impossible. - ---- - -## Major Issues (Severity: Medium) - -### Issue #4: Missing Error Context in Hash Extraction - -**File**: `eh/src/util.rs`\ -**Lines**: 24-40\ -**Severity**: Medium\ -**Category**: Error Handling - -**Problem Description**: The `extract_hash` method returns `None` without -providing information about why hash extraction failed, making debugging -difficult. - -**Current Code**: - -```rust -impl HashExtractor for RegexHashExtractor { - fn extract_hash(&self, stderr: &str) -> Option { - let patterns = [ - r"got:\s+(sha256-[a-zA-Z0-9+/=]+)", - r"actual:\s+(sha256-[a-zA-Z0-9+/=]+)", - r"have:\s+(sha256-[a-zA-Z0-9+/=]+)", - ]; - for pattern in &patterns { - if let Ok(re) = Regex::new(pattern) - && let Some(captures) = re.captures(stderr) - && let Some(hash) = captures.get(1) - { - return Some(hash.as_str().to_string()); - } - } - None - } -} -``` - -**Recommended Fix**: - -```rust -impl HashExtractor for RegexHashExtractor { - fn extract_hash(&self, stderr: &str) -> Option { - let patterns = [ - r"got:\s+(sha256-[a-zA-Z0-9+/=]+)", - r"actual:\s+(sha256-[a-zA-Z0-9+/=]+)", - r"have:\s+(sha256-[a-zA-Z0-9+/=]+)", - ]; - for pattern in &patterns { - if let Ok(re) = Regex::new(pattern) { - if let Some(captures) = re.captures(stderr) { - if let Some(hash) = captures.get(1) { - tracing::debug!("Extracted hash '{}' using pattern '{}'", hash.as_str(), pattern); - return Some(hash.as_str().to_string()); - } - } - } else { - tracing::warn!("Invalid regex pattern: {}", pattern); - } - } - tracing::debug!("No hash found in stderr output"); - None - } -} -``` - -**Rationale**: Adding debug logging helps diagnose why hash extraction failed, -which is crucial for troubleshooting Nix build issues. - -**Impact if Unfixed**: Users cannot determine why hash fixing failed, making it -difficult to resolve Nix build problems. - ---- - -### Issue #5: Unsafe File Path Handling - -**File**: `eh/src/util.rs`\ -**Lines**: 120-133\ -**Severity**: Medium\ -**Category**: Security - -**Problem Description**: The `fix_hash_in_file` method uses -`unwrap_or(Path::new("."))` for the parent directory, which could lead to -writing files to unintended locations. - -**Current Code**: - -```rust -let temp_file = - NamedTempFile::new_in(file_path.parent().unwrap_or(Path::new(".")))?; -``` - -**Recommended Fix**: - -```rust -let parent_dir = file_path.parent() - .ok_or_else(|| EhError::HashFixFailed { - path: file_path.to_string_lossy().to_string(), - })?; -let temp_file = NamedTempFile::new_in(parent_dir)?; -``` - -**Rationale**: Explicitly handling the case where a file has no parent directory -prevents unintended file creation in the current directory. - -**Impact if Unfixed**: Temporary files could be created in unexpected locations, -potentially causing security issues or file system pollution. - ---- - -### Issue #6: Inefficient Regex Compilation in Loop - -**File**: `eh/src/util.rs`\ -**Lines**: 85-102\ -**Severity**: Medium\ -**Category**: Performance - -**Problem Description**: Regex patterns are compiled for every file that needs -hash fixing, even though the patterns are constant. - -**Current Code**: - -```rust -fn fix_hash_in_file(&self, file_path: &Path, new_hash: &str) -> Result { - // Pre-compile regex patterns once to avoid repeated compilation - let patterns: Vec<(Regex, String)> = [ - (r#"hash\s*=\s*"[^"]*""#, format!(r#"hash = "{new_hash}""#)), - ( - r#"sha256\s*=\s*"[^"]*""#, - format!(r#"sha256 = "{new_hash}""#), - ), - ( - r#"outputHash\s*=\s*"[^"]*""#, - format!(r#"outputHash = "{new_hash}""#), - ), - ] - .into_iter() - .map(|(pattern, replacement)| { - Regex::new(pattern) - .map(|re| (re, replacement)) - .map_err(EhError::Regex) - }) - .collect::>>()?; -``` - -**Recommended Fix**: - -```rust -// Add lazy_static or once_cell at the top of the file -use once_cell::sync::Lazy; -use std::sync::Arc; - -static HASH_PATTERNS: Lazy> = Lazy::new(|| { - vec![ - (Regex::new(r#"hash\s*=\s*"[^"]*""#).unwrap(), r#"hash = "{hash}""#), - (Regex::new(r#"sha256\s*=\s*"[^"]*""#).unwrap(), r#"sha256 = "{hash}""#), - (Regex::new(r#"outputHash\s*=\s*"[^"]*""#).unwrap(), r#"outputHash = "{hash}""#), - ] -}); - -fn fix_hash_in_file(&self, file_path: &Path, new_hash: &str) -> Result { - // Use pre-compiled patterns - let patterns: Vec<(Regex, String)> = HASH_PATTERNS - .iter() - .map(|(re, template)| { - let replacement = template.replace("{hash}", new_hash); - (re.clone(), replacement) - }) - .collect(); -``` - -**Rationale**: Pre-compiling regex patterns once and reusing them significantly -improves performance when fixing multiple files. - -**Impact if Unfixed**: Performance degradation when processing many Nix files, -especially in large projects. - ---- - -### Issue #7: Missing Error Handling in Completion Generation - -**File**: `xtask/src/main.rs`\ -**Lines**: 164-168\ -**Severity**: Medium\ -**Category**: Error Handling - -**Problem Description**: The completion generation code creates symlinks without -proper error handling for the case where the target file doesn't exist. - -**Current Code**: - -```rust -#[cfg(unix)] -{ - let canonical_target = fs::canonicalize(&completion_file)?; - std::os::unix::fs::symlink(&canonical_target, &symlink_path)?; - println!("completion symlink created: {}", symlink_path.display()); -} -``` - -**Recommended Fix**: - -```rust -#[cfg(unix)] -{ - match fs::canonicalize(&completion_file) { - Ok(canonical_target) => { - if let Err(e) = std::os::unix::fs::symlink(&canonical_target, &symlink_path) { - eprintln!("warning: failed to create symlink {}: {}", symlink_path.display(), e); - // Fall back to copying - if let Err(e) = fs::copy(&completion_file, &symlink_path) { - eprintln!("error: failed to copy completion file: {}", e); - return Err(e.into()); - } - println!("completion copy created: {}", symlink_path.display()); - } else { - println!("completion symlink created: {}", symlink_path.display()); - } - }, - Err(e) => { - eprintln!("error: completion file not found: {}", e); - return Err(e.into()); - } - } -} -``` - -**Rationale**: Proper error handling ensures the completion generation process -is robust and provides meaningful error messages. - -**Impact if Unfixed**: Completion generation could fail silently or with unclear -error messages, making it difficult for users to troubleshoot. - ---- - -### Issue #8: Inconsistent Tracing Configuration - -**File**: `eh/src/main.rs`\ -**Lines**: 14-20\ -**Severity**: Medium\ -**Category**: Configuration - -**Problem Description**: The tracing configuration is hardcoded and doesn't -respect environment variables or provide flexibility for different output -formats. - -**Current Code**: - -```rust -let format = tracing_subscriber::fmt::format() - .with_level(true) // don't include levels in formatted output - .with_target(true) // don't include targets - .with_thread_ids(false) // include the thread ID of the current thread - .with_thread_names(false) // include the name of the current thread - .compact(); // use the `Compact` formatting style. -tracing_subscriber::fmt().event_format(format).init(); -``` - -**Recommended Fix**: - -```rust -use tracing_subscriber::{fmt, EnvFilter}; - -let filter = EnvFilter::try_from_default_env() - .unwrap_or_else(|_| EnvFilter::new("eh=info")); - -let format = fmt::format() - .with_level(true) - .with_target(true) - .with_thread_ids(false) - .with_thread_names(false) - .compact(); - -fmt() - .event_format(format) - .with_env_filter(filter) - .init(); -``` - -**Rationale**: Respecting environment variables allows users to control log -levels and output without code changes, making debugging easier. - -**Impact if Unfixed**: Users cannot customize logging behavior, making it -difficult to debug issues in different environments. - ---- - -## Minor Issues (Severity: Low) - -### Issue #9: Inconsistent Comment Style - -**File**: `eh/src/command.rs`\ -**Lines**: 57, 67, 82, 88, 94\ -**Severity**: Low\ -**Category**: Code Style - -**Problem Description**: Some methods have -`#[allow(dead_code, reason = "FIXME")]` attributes while others have -`#[must_use]` attributes, creating inconsistent documentation style. - -**Current Code**: - -```rust -#[allow(dead_code, reason = "FIXME")] -pub fn args(mut self, args: I) -> Self -where - I: IntoIterator, - S: Into, -{ - self.args.extend(args.into_iter().map(Into::into)); - self -} - -#[must_use] -pub fn args_ref(mut self, args: &[String]) -> Self { - self.args.extend(args.iter().cloned()); - self -} -``` - -**Recommended Fix**: - -```rust -#[must_use] -pub fn args(mut self, args: I) -> Self -where - I: IntoIterator, - S: Into, -{ - self.args.extend(args.into_iter().map(Into::into)); - self -} - -#[must_use] -pub fn args_ref(mut self, args: &[String]) -> Self { - self.args.extend(args.iter().cloned()); - self -} -``` - -**Rationale**: Consistent use of `#[must_use]` attributes improves API clarity -and prevents unused method calls. - -**Impact if Unfixed**: Minor inconsistency in code style, but no functional -impact. - ---- - -### Issue #10: Missing Documentation for Public Traits - -**File**: `eh/src/util.rs`\ -**Lines**: 17-19, 42-46, 139-141\ -**Severity**: Low\ -**Category**: Documentation - -**Problem Description**: Public traits lack documentation comments, making it -difficult for users to understand their purpose and usage. - -**Current Code**: - -```rust -pub trait HashExtractor { - fn extract_hash(&self, stderr: &str) -> Option; -} - -pub trait NixFileFixer { - fn fix_hash_in_files(&self, new_hash: &str) -> Result; - fn find_nix_files(&self) -> Result>; - fn fix_hash_in_file(&self, file_path: &Path, new_hash: &str) -> Result; -} - -pub trait NixErrorClassifier { - fn should_retry(&self, stderr: &str) -> bool; -} -``` - -**Recommended Fix**: - -```rust -/// Extracts Nix output hashes from error messages. -/// -/// This trait is used to parse Nix error output and extract -/// the correct hash values when hash mismatches occur. -pub trait HashExtractor { - /// Attempts to extract a hash from the given stderr output. - /// - /// # Arguments - /// * `stderr` - The stderr output from a failed Nix command - /// - /// # Returns - /// * `Some(String)` - The extracted hash if found - /// * `None` - No hash could be extracted - fn extract_hash(&self, stderr: &str) -> Option; -} - -/// Fixes hash values in Nix configuration files. -/// -/// This trait provides methods to locate Nix files and update -/// hash values when they change due to source modifications. -pub trait NixFileFixer { - /// Fixes hash values in all discovered Nix files. - /// - /// # Arguments - /// * `new_hash` - The new hash value to set - /// - /// # Returns - /// * `Ok(true)` - At least one file was modified - /// * `Ok(false)` - No files needed modification - /// * `Err(EhError)` - An error occurred during processing - fn fix_hash_in_files(&self, new_hash: &str) -> Result; - - /// Discovers all Nix files in the current directory tree. - /// - /// # Returns - /// * `Ok(Vec)` - List of found Nix files - /// * `Err(EhError::NoNixFilesFound)` - No Nix files found - fn find_nix_files(&self) -> Result>; - - /// Fixes hash values in a specific Nix file. - /// - /// # Arguments - /// * `file_path` - Path to the Nix file to modify - /// * `new_hash` - The new hash value to set - /// - /// # Returns - /// * `Ok(true)` - The file was modified - /// * `Ok(false)` - The file didn't need modification - /// * `Err(EhError)` - An error occurred during processing - fn fix_hash_in_file(&self, file_path: &Path, new_hash: &str) -> Result; -} - -/// Classifies Nix errors to determine if retry is appropriate. -/// -/// This trait analyzes Nix error output to determine whether -/// a command should be retried with different flags or parameters. -pub trait NixErrorClassifier { - /// Determines if a command should be retried based on error output. - /// - /// # Arguments - /// * `stderr` - The stderr output from a failed Nix command - /// - /// # Returns - /// * `true` - The command should be retried - /// * `false` - The command should not be retried - fn should_retry(&self, stderr: &str) -> bool; -} -``` - -**Rationale**: Comprehensive documentation makes the API easier to understand -and use correctly. - -**Impact if Unfixed**: Users may struggle to understand how to implement or use -these traits correctly. - ---- - -### Issue #11: Unused Variable in Test - -**File**: `eh/src/util.rs`\ -**Lines**: 481-482\ -**Severity**: Low\ -**Category**: Code Quality - -**Problem Description**: The test extracts file permissions but never uses them, -creating dead code. - -**Current Code**: - -```rust -// Get original permissions -let original_metadata = std::fs::metadata(file_path).unwrap(); -let _original_permissions = original_metadata.permissions(); -``` - -**Recommended Fix**: - -```rust -// Get original permissions -let original_metadata = std::fs::metadata(file_path).unwrap(); -let original_permissions = original_metadata.permissions(); - -// Test hash replacement -let fixer = DefaultNixFileFixer; -let result = fixer.fix_hash_in_file(file_path, "sha256-newhash").unwrap(); - -assert!(result, "Hash replacement should succeed"); - -// Verify file still exists and permissions are preserved -let new_metadata = std::fs::metadata(file_path).unwrap(); -assert!( - new_metadata.is_file(), - "File should still exist after replacement" -); -assert_eq!( - original_permissions, - new_metadata.permissions(), - "File permissions should be preserved" -); -``` - -**Rationale**: Actually using the permissions check makes the test meaningful -and verifies that file permissions are preserved. - -**Impact if Unfixed**: The test doesn't actually verify what it claims to test. - ---- - -### Issue #12: Inconsistent Error Message Formatting - -**File**: `eh/src/util.rs`\ -**Lines**: 265-269, 284-289, 305-309\ -**Severity**: Low\ -**Category**: Code Style - -**Problem Description**: Warning messages use inconsistent formatting and some -have unnecessary line breaks in the string literals. - -**Current Code**: - -```rust -warn!( - "{}", - Paint::yellow( - "⚠ Unfree package detected, retrying with NIXPKGS_ALLOW_UNFREE=1..." - ) -); -warn!( - "{}", - Paint::yellow( - "⚠ Insecure package detected, retrying with \ - NIXPKGS_ALLOW_INSECURE=1..." - ) -); -warn!( - "{}", - Paint::yellow( - "⚠ Broken package detected, retrying with NIXPKGS_ALLOW_BROKEN=1..." - ) -); -``` - -**Recommended Fix**: - -```rust -warn!( - "{}", - Paint::yellow("⚠ Unfree package detected, retrying with NIXPKGS_ALLOW_UNFREE=1...") -); -warn!( - "{}", - Paint::yellow("⚠ Insecure package detected, retrying with NIXPKGS_ALLOW_INSECURE=1...") -); -warn!( - "{}", - Paint::yellow("⚠ Broken package detected, retrying with NIXPKGS_ALLOW_BROKEN=1...") -); -``` - -**Rationale**: Consistent formatting makes the code more readable and -maintainable. - -**Impact if Unfixed**: Minor inconsistency in code style, but no functional -impact. - ---- - -## Code Quality Observations - -### Positive Patterns - -1. **Good Separation of Concerns**: The codebase properly separates command - execution, error handling, and utility functions into distinct modules. -2. **Trait-Based Design**: The use of traits for `HashExtractor`, - `NixFileFixer`, and `NixErrorClassifier` provides good extensibility. -3. **Comprehensive Testing**: The `util.rs` module includes thorough unit tests - covering edge cases and error conditions. -4. **Atomic File Operations**: The hash fixing uses temporary files and atomic - operations to prevent corruption. - -### Areas for Improvement - -1. **Configuration Management**: Consider using a configuration file for default - values and timeouts instead of hardcoding them. -2. **Logging Strategy**: Implement structured logging with consistent log levels - throughout the application. -3. **Error Recovery**: Add more sophisticated error recovery mechanisms for - transient failures. - ---- - -## Summary Statistics - -- Critical Issues: 3 -- Major Issues: 5 -- Minor Issues: 4 -- Files Affected: 7 -- Total Lines Reviewed: 4,159 - ---- - -## Prioritized Action Plan - -1. **Fix input validation in multicall dispatch** - `eh/src/main.rs:34-70` - (Critical - Security) -2. **Add timeout protection to command execution** - `eh/src/command.rs:151-179` - (Critical - Resource Management) -3. **Implement unique error exit codes** - `eh/src/error.rs:40-47` (Critical - - Error Handling) -4. **Add debug logging to hash extraction** - `eh/src/util.rs:24-40` (Major - - Debugging) -5. **Fix unsafe file path handling** - `eh/src/util.rs:120-133` (Major - - Security) -6. **Optimize regex compilation** - `eh/src/util.rs:85-102` (Major - - Performance) -7. **Improve completion generation error handling** - - `xtask/src/main.rs:164-168` (Major - Robustness) -8. **Make tracing configuration flexible** - `eh/src/main.rs:14-20` (Major - - Configuration) -9. **Standardize method attributes** - `eh/src/command.rs:57,67,82,88,94` - (Minor - Style) -10. **Add trait documentation** - `eh/src/util.rs:17-19,42-46,139-141` (Minor - - Documentation) -11. **Fix unused variable in test** - `eh/src/util.rs:481-482` (Minor - Test - Quality) -12. **Standardize warning message formatting** - - `eh/src/util.rs:265-269,284-289,305-309` (Minor - Style) - diff --git a/TODO.md b/TODO.md deleted file mode 100644 index e098166..0000000 --- a/TODO.md +++ /dev/null @@ -1,383 +0,0 @@ -# TODO List: Codebase Improvements - -## 1. Extract Binary Name Dispatch Logic (Medium Priority) - -**Context**: Code duplication in main.rs:42-65 where the same trait -instantiation pattern repeats 3 times for nr/ns/nb dispatch. - -**Pseudocode**: - -```rust -fn dispatch_multicall_command(app_name: &str, args: Vec) -> Result { - let hash_extractor = util::RegexHashExtractor; - let fixer = util::DefaultNixFileFixer; - let classifier = util::DefaultNixErrorClassifier; - - match app_name { - "nr" => run::handle_nix_run(&args, &hash_extractor, &fixer, &classifier), - "ns" => shell::handle_nix_shell(&args, &hash_extractor, &fixer, &classifier), - "nb" => build::handle_nix_build(&args, &hash_extractor, &fixer, &classifier), - _ => Err(EhError::UnknownCommand(app_name.to_string())) - } -} -``` - -## 2. Replace Magic Buffer Sizes (Low Priority) - -**Context**: Hardcoded `[0u8; 4096]` in command.rs:119-120 should be named -constants for maintainability. - -**Pseudocode**: - -```rust -const DEFAULT_BUFFER_SIZE: usize = 4096; - -// Replace: -let mut out_buf = [0u8; 4096]; -let mut err_buf = [0u8; 4096]; - -// With: -let mut out_buf = [0u8; DEFAULT_BUFFER_SIZE]; -let mut err_buf = [0u8; DEFAULT_BUFFER_SIZE]; -``` - -## 3. Standardize Error Handling (Medium Priority) - -**Context**: Inconsistent error handling patterns - some functions use `?` while -others manually handle errors. - -**Pseudocode**: - -```rust -// Instead of manual error handling: -match some_operation() { - Ok(result) => result, - Err(e) => return Err(EhError::from(e)) -} - -// Use consistent ? operator: -let result = some_operation()?; -``` - -## 4. Replace Manual Directory Traversal (High Priority) - -**Context**: Manual stack-based traversal in util.rs:55-75 is inefficient and -error-prone compared to walkdir crate. - -**Pseudocode**: - -```rust -// Add to Cargo.toml: -walkdir = "2.4" - -// Replace find_nix_files implementation: -use walkdir::WalkDir; - -fn find_nix_files(&self) -> Result> { - let files: Vec = WalkDir::new(".") - .into_iter() - .filter_map(|entry| entry.ok()) - .filter(|entry| { - entry.file_type().is_file() && - entry.path().extension() - .map(|ext| ext.eq_ignore_ascii_case("nix")) - .unwrap_or(false) - }) - .map(|entry| entry.path().to_path_buf()) - .collect(); - - if files.is_empty() { - Err(EhError::NoNixFilesFound) - } else { - Ok(files) - } -} -``` - -## 5. Eliminate Unnecessary String Allocations (Medium Priority) - -**Context**: Multiple string clones in retry logic, especially args cloning in -util.rs:152-200. - -**Pseudocode**: - -```rust -// Instead of cloning args repeatedly: -.handle_nix_with_retry("run", &args, ...) - -// Use references and avoid cloning: -.handle_nix_with_retry("run", args, ...) - -// In retry logic, reuse the same command builder: -let mut cmd = NixCommand::new(subcommand) - .print_build_logs(true) - .args(args); // Pass by reference instead of cloning -``` - -## 6. Convert to Async/Await (Low Priority) - -**Context**: Blocking I/O operations could benefit from async for better -concurrency. - -**Pseudocode**: - -```rust -// Add to Cargo.toml: -tokio = { version = "1.0", features = ["full"] } - -// Convert command execution: -pub async fn run_with_logs_async( - &self, - mut interceptor: I, -) -> Result { - let mut cmd = tokio::process::Command::new("nix"); - // ... rest of implementation using async I/O -} -``` - -## 7. Add Comprehensive Unit Tests (High Priority) - -**Context**: Current tests are basic integration tests. Need unit tests for core -logic components. - -**Pseudocode**: - -```rust -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_hash_extraction_patterns() { - let extractor = RegexHashExtractor; - let test_cases = vec![ - ("got: sha256-abc123", Some("sha256-abc123".to_string())), - ("actual: sha256-def456", Some("sha256-def456".to_string())), - ("no hash here", None), - ]; - - for (input, expected) in test_cases { - assert_eq!(extractor.extract_hash(input), expected); - } - } - - #[test] - fn test_nix_file_fixer_replacements() { - // Test each pattern replacement individually - let test_content = r#" - hash = "oldhash" - sha256 = "oldsha256" - outputHash = "oldoutput" - "#; - - let expected = r#" - hash = "newhash" - sha256 = "newhash" - outputHash = "newhash" - "#; - - // Test replacement logic - } -} -``` - -## 8. Create Mock Nix Files for Testing (High Priority) - -**Context**: Tests rely on external network dependencies. Should use mock files -instead. - -**Pseudocode**: - -```rust -#[cfg(test)] -mod mock_tests { - use tempfile::TempDir; - use std::fs::File; - use std::io::Write; - - fn create_mock_nix_file(dir: &TempDir, content: &str) -> PathBuf { - let file_path = dir.path().join("test.nix"); - let mut file = File::create(&file_path).unwrap(); - file.write_all(content.as_bytes()).unwrap(); - file_path - } - - #[test] - fn test_hash_fixing_with_mock_files() { - let temp_dir = TempDir::new().unwrap(); - let mock_content = r#" - stdenv.mkDerivation { - name = "test"; - src = fetchurl { - url = "https://example.com"; - hash = "sha256-oldhash"; - }; - } - "#; - - let file_path = create_mock_nix_file(&temp_dir, mock_content); - - // Test hash fixing logic with mock file - let fixer = DefaultNixFileFixer; - let fixed = fixer.fix_hash_in_file(&file_path, "sha256-newhash").unwrap(); - assert!(fixed); - - // Verify the content was updated correctly - let updated_content = std::fs::read_to_string(&file_path).unwrap(); - assert!(updated_content.contains("sha256-newhash")); - } -} -``` - -## 9. Add File Size Limits (Medium Priority) - -**Context**: File operations read entire files into memory without size limits. - -**Pseudocode**: - -```rust -const MAX_FILE_SIZE: usize = 10 * 1024 * 1024; // 10MB limit - -fn fix_hash_in_file(&self, file_path: &Path, new_hash: &str) -> Result { - let metadata = fs::metadata(file_path)?; - if metadata.len() as usize > MAX_FILE_SIZE { - return Err(EhError::FileTooLarge { - path: file_path.to_string_lossy().to_string(), - size: metadata.len(), - }); - } - - // Continue with existing logic... -} - -// Add to EhError enum: -#[error("File too large: {path} ({size} bytes)")] -FileTooLarge { path: String, size: u64 }, -``` - -## 10. Implement Input Validation (Medium Priority) - -**Context**: Need better input sanitization to prevent command injection. - -**Pseudocode**: - -```rust -fn validate_nix_args(args: &[String]) -> Result<()> { - const DANGEROUS_PATTERNS: &[&str] = &[ - ";", "&&", "||", "|", "`", "$(", "${", - ">", "<", ">>", "<<", "2>", "2>>" - ]; - - for arg in args { - for pattern in DANGEROUS_PATTERNS { - if arg.contains(pattern) { - return Err(EhError::InvalidInput { - input: arg.clone(), - reason: format!("contains potentially dangerous pattern: {}", pattern), - }); - } - } - } - Ok(()) -} - -// Add to EhError enum: -#[error("Invalid input: {input} - {reason}")] -InvalidInput { input: String, reason: String }, -``` - -## 11. Make Configuration External (Low Priority) - -**Context**: Hardcoded retry logic, hash patterns, and error classifications -should be configurable. - -**Pseudocode**: - -```rust -#[derive(Debug, Clone, Deserialize)] -pub struct EhConfig { - pub hash_patterns: Vec, - pub max_retries: u32, - pub file_size_limit: Option, - pub allowed_packages: Option>, -} - -impl Default for EhConfig { - fn default() -> Self { - Self { - hash_patterns: vec![ - r"got:\s+(sha256-[a-zA-Z0-9+/=]+)".to_string(), - r"actual:\s+(sha256-[a-zA-Z0-9+/=]+)".to_string(), - r"have:\s+(sha256-[a-zA-Z0-9+/=]+)".to_string(), - ], - max_retries: 3, - file_size_limit: Some(10 * 1024 * 1024), - allowed_packages: None, - } - } -} - -// Load from config file or environment: -fn load_config() -> EhConfig { - if let Ok(config_path) = std::env::var("EH_CONFIG_FILE") { - // Load from file - } else { - EhConfig::default() - } -} -``` - -## 12. Implement Dependency Injection (Low Priority) - -**Context**: Tight coupling makes testing difficult. Use dependency injection -for better testability. - -**Pseudocode**: - -```rust -pub struct EhApp { - hash_extractor: H, - file_fixer: F, - error_classifier: C, -} - -impl EhApp -where - H: HashExtractor, - F: NixFileFixer, - C: NixErrorClassifier, -{ - pub fn new(hash_extractor: H, file_fixer: F, error_classifier: C) -> Self { - Self { - hash_extractor, - file_fixer, - error_classifier, - } - } - - pub fn handle_command(&self, subcommand: &str, args: &[String]) -> Result { - handle_nix_with_retry( - subcommand, - args, - &self.hash_extractor, - &self.file_fixer, - &self.error_classifier, - subcommand == "run" || subcommand == "shell", - ) - } -} - -// Usage in production: -let app = EhApp::new( - RegexHashExtractor, - DefaultNixFileFixer, - DefaultNixErrorClassifier, -); - -// Usage in tests: -let mock_extractor = MockHashExtractor::new(); -let mock_fixer = MockNixFileFixer::new(); -let mock_classifier = MockNixErrorClassifier::new(); -let app = EhApp::new(mock_extractor, mock_fixer, mock_classifier); -``` - diff --git a/eh/src/command.rs b/eh/src/command.rs index 7eace98..74ca3f1 100644 --- a/eh/src/command.rs +++ b/eh/src/command.rs @@ -2,7 +2,6 @@ use std::{ collections::VecDeque, io::{self, Read, Write}, process::{Command, ExitStatus, Output, Stdio}, - time::{Duration, Instant}, }; use crate::error::{EhError, Result}; @@ -28,9 +27,6 @@ impl LogInterceptor for StdIoInterceptor { /// Default buffer size for reading command output const DEFAULT_BUFFER_SIZE: usize = 4096; -/// Default timeout for command execution -const DEFAULT_TIMEOUT: Duration = Duration::from_secs(300); // 5 minutes - /// Builder and executor for Nix commands. pub struct NixCommand { subcommand: String, @@ -151,19 +147,10 @@ impl NixCommand { let mut out_queue = VecDeque::new(); let mut err_queue = VecDeque::new(); - let start_time = Instant::now(); loop { let mut did_something = false; - // Check for timeout - if start_time.elapsed() > DEFAULT_TIMEOUT { - let _ = child.kill(); - return Err(EhError::CommandFailed { - command: format!("nix {} timed out after 5 minutes", self.subcommand), - }); - } - match stdout.read(&mut out_buf) { Ok(0) => {}, Ok(n) => { @@ -189,11 +176,6 @@ impl NixCommand { if !did_something && child.try_wait()?.is_some() { break; } - - // Prevent busy waiting when no data is available - if !did_something { - std::thread::sleep(Duration::from_millis(10)); - } } let status = child.wait()?; diff --git a/eh/src/error.rs b/eh/src/error.rs index 173f047..b9c2bfb 100644 --- a/eh/src/error.rs +++ b/eh/src/error.rs @@ -40,15 +40,9 @@ impl EhError { pub const fn exit_code(&self) -> i32 { match self { Self::ProcessExit { code } => *code, - Self::NixCommandFailed(_) => 2, - Self::CommandFailed { .. } => 3, - Self::HashExtractionFailed => 4, - Self::NoNixFilesFound => 5, - Self::HashFixFailed { .. } => 6, - Self::InvalidInput { .. } => 7, - Self::Io(_) => 8, - Self::Regex(_) => 9, - Self::Utf8(_) => 10, + Self::NixCommandFailed(_) => 1, + Self::CommandFailed { .. } => 1, + _ => 1, } } } diff --git a/eh/src/main.rs b/eh/src/main.rs index 370049f..6369154 100644 --- a/eh/src/main.rs +++ b/eh/src/main.rs @@ -36,12 +36,6 @@ fn dispatch_multicall( args: std::env::Args, ) -> Option> { let rest: Vec = args.collect(); - - // Validate arguments before processing - if let Err(e) = util::validate_nix_args(&rest) { - return Some(Err(e)); - } - let hash_extractor = util::RegexHashExtractor; let fixer = util::DefaultNixFileFixer; let classifier = util::DefaultNixErrorClassifier; diff --git a/eh/src/util.rs b/eh/src/util.rs index f452296..9896e18 100644 --- a/eh/src/util.rs +++ b/eh/src/util.rs @@ -174,7 +174,7 @@ fn pre_evaluate(_subcommand: &str, args: &[String]) -> Result { Ok(false) } -pub fn validate_nix_args(args: &[String]) -> Result<()> { +fn validate_nix_args(args: &[String]) -> Result<()> { const DANGEROUS_PATTERNS: &[&str] = &[ ";", "&&", "||", "|", "`", "$(", "${", ">", "<", ">>", "<<", "2>", "2>>", ]; diff --git a/eh/tests/basic.rs b/eh/tests/basic.rs new file mode 100644 index 0000000..7380b3f --- /dev/null +++ b/eh/tests/basic.rs @@ -0,0 +1,234 @@ +use std::{fs, process::Command}; + +use eh::util::{ + DefaultNixErrorClassifier, + DefaultNixFileFixer, + HashExtractor, + NixErrorClassifier, + NixFileFixer, + RegexHashExtractor, +}; +use tempfile::TempDir; + +#[test] +fn test_hash_extraction_from_real_nix_errors() { + // Test hash extraction from actual Nix error messages + let extractor = RegexHashExtractor; + + let test_cases = [ + ( + r#"error: hash mismatch in fixed-output derivation '/nix/store/xxx-foo.drv': + specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= + got: sha256-BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB="#, + Some("sha256-BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB=".to_string()), + ), + ( + "actual: sha256-abc123def456", + Some("sha256-abc123def456".to_string()), + ), + ("have: sha256-xyz789", Some("sha256-xyz789".to_string())), + ("no hash here", None), + ]; + + for (input, expected) in test_cases { + assert_eq!(extractor.extract_hash(input), expected); + } +} + +#[test] +fn test_error_classification_for_retry_logic() { + // Test that the classifier correctly identifies errors that should be retried + let classifier = DefaultNixErrorClassifier; + + // These should trigger retries + let retry_cases = [ + "Package 'discord-1.0.0' has an unfree license ('unfree'), refusing to \ + evaluate.", + "Package 'openssl-1.1.1' has been marked as insecure, refusing to \ + evaluate.", + "Package 'broken-1.0' has been marked as broken, refusing to evaluate.", + "hash mismatch in fixed-output derivation\ngot: sha256-newhash", + ]; + + for error in retry_cases { + assert!(classifier.should_retry(error), "Should retry: {}", error); + } + + // These should NOT trigger retries + let no_retry_cases = [ + "build failed", + "random error", + "permission denied", + "network error", + ]; + + for error in no_retry_cases { + assert!( + !classifier.should_retry(error), + "Should not retry: {}", + error + ); + } +} + +#[test] +fn test_hash_fixing_in_nix_files() { + // Test that hash fixing actually works on real Nix files + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let fixer = DefaultNixFileFixer; + + // Create a mock Nix file with various hash formats + let nix_content = r#" +stdenv.mkDerivation { + name = "test-package"; + src = fetchurl { + url = "https://example.com.tar.gz"; + hash = "sha256-oldhash123"; + }; + + buildInputs = [ fetchurl { + url = "https://deps.com.tar.gz"; + sha256 = "sha256-oldhash456"; + }]; + + outputHash = "sha256-oldhash789"; +} +"#; + + let file_path = temp_dir.path().join("test.nix"); + fs::write(&file_path, nix_content).expect("Failed to write test file"); + + // Test hash replacement + let new_hash = "sha256-newhashabc"; + let was_fixed = fixer + .fix_hash_in_file(&file_path, new_hash) + .expect("Failed to fix hash"); + + assert!(was_fixed, "File should have been modified"); + + let updated_content = + fs::read_to_string(&file_path).expect("Failed to read updated file"); + + // All hash formats should be updated + assert!(updated_content.contains(&format!(r#"hash = "{}""#, new_hash))); + assert!(updated_content.contains(&format!(r#"sha256 = "{}""#, new_hash))); + assert!(updated_content.contains(&format!(r#"outputHash = "{}""#, new_hash))); + + // Old hashes should be gone + assert!(!updated_content.contains("oldhash123")); + assert!(!updated_content.contains("oldhash456")); + assert!(!updated_content.contains("oldhash789")); +} + +#[test] +fn test_multicall_binary_dispatch() { + // Test that multicall binaries work without needing actual Nix evaluation + let commands = [("nb", "build"), ("nr", "run"), ("ns", "shell")]; + + for (binary_name, _expected_command) in &commands { + // Test that the binary starts and handles invalid arguments gracefully + let output = Command::new("timeout") + .args(["5", "cargo", "run", "--bin", "eh", "--"]) + .env("CARGO_BIN_NAME", binary_name) + .arg("invalid-package-ref") + .output() + .expect("Failed to execute command"); + + // Should fail gracefully (not panic or hang) + assert!( + output.status.code().is_some(), + "{} should exit with a code", + binary_name + ); + + // Should show an error message, not crash + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("Error:") + || stderr.contains("error:") + || stderr.contains("failed"), + "{} should show error for invalid package", + binary_name + ); + } +} + +#[test] +fn test_invalid_expression_handling() { + // Test that invalid Nix expressions fail fast with proper error messages + let invalid_refs = [ + "invalid-flake-ref", + "nonexistent-package", + "file:///nonexistent/path", + ]; + + for invalid_ref in invalid_refs { + let output = Command::new("timeout") + .args([ + "10", + "cargo", + "run", + "--bin", + "eh", + "--", + "build", + invalid_ref, + ]) + .output() + .expect("Failed to execute command"); + + // Should fail with a proper error, not hang or crash + assert!( + !output.status.success(), + "Invalid ref '{}' should fail", + invalid_ref + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("Error:") + || stderr.contains("error:") + || stderr.contains("failed"), + "Should show error message for invalid ref '{}': {}", + invalid_ref, + stderr + ); + } +} + +#[test] +fn test_nix_file_discovery() { + // Test that the fixer can find Nix files in a directory structure + let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let fixer = DefaultNixFileFixer; + + // Create directory structure with Nix files + fs::create_dir_all(temp_dir.path().join("subdir")) + .expect("Failed to create subdir"); + + let files = [ + ("test.nix", "stdenv.mkDerivation { name = \"test\"; }"), + ("subdir/other.nix", "pkgs.hello"), + ("not-nix.txt", "not a nix file"), + ("default.nix", "import ./test.nix"), + ]; + + for (path, content) in files { + fs::write(temp_dir.path().join(path), content) + .expect("Failed to write file"); + } + + // Change to temp dir for file discovery + let original_dir = + std::env::current_dir().expect("Failed to get current dir"); + std::env::set_current_dir(temp_dir.path()) + .expect("Failed to change directory"); + + let found_files = fixer.find_nix_files().expect("Failed to find Nix files"); + + // Should find 3 .nix files (not the .txt file) + assert_eq!(found_files.len(), 3, "Should find exactly 3 .nix files"); + + // Restore original directory + std::env::set_current_dir(original_dir).expect("Failed to restore directory"); +} diff --git a/xtask/src/main.rs b/xtask/src/main.rs index ad449c4..327ba0d 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -162,8 +162,7 @@ fn generate_completions( #[cfg(unix)] { - let canonical_target = fs::canonicalize(&completion_file)?; - std::os::unix::fs::symlink(&canonical_target, &symlink_path)?; + std::os::unix::fs::symlink(&completion_file, &symlink_path)?; println!("completion symlink created: {}", symlink_path.display()); }