diff --git a/REPORT.md b/REPORT.md new file mode 100644 index 0000000..50d4355 --- /dev/null +++ b/REPORT.md @@ -0,0 +1,932 @@ +# 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 new file mode 100644 index 0000000..e098166 --- /dev/null +++ b/TODO.md @@ -0,0 +1,383 @@ +# 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/main.rs b/eh/src/main.rs index 6369154..13c29f4 100644 --- a/eh/src/main.rs +++ b/eh/src/main.rs @@ -36,6 +36,12 @@ 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 9896e18..f452296 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) } -fn validate_nix_args(args: &[String]) -> Result<()> { +pub fn validate_nix_args(args: &[String]) -> Result<()> { const DANGEROUS_PATTERNS: &[&str] = &[ ";", "&&", "||", "|", "`", "$(", "${", ">", "<", ">>", "<<", "2>", "2>>", ];