From 0fb250e28cbd5a87d741bf1c5d8c296e21ee862a Mon Sep 17 00:00:00 2001 From: perf3ct Date: Mon, 11 Aug 2025 01:13:29 +0000 Subject: [PATCH] feat(security): this was just pain --- src/utils/security.rs | 145 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 129 insertions(+), 16 deletions(-) diff --git a/src/utils/security.rs b/src/utils/security.rs index 3a78fce..db8525b 100644 --- a/src/utils/security.rs +++ b/src/utils/security.rs @@ -2,7 +2,7 @@ use anyhow::Result; use std::path::{Path, PathBuf, Component}; -use tracing::warn; +use tracing::{warn, debug}; /// Validate and sanitize file paths to prevent path traversal attacks pub fn validate_and_sanitize_path(input_path: &str) -> Result { @@ -171,28 +171,54 @@ pub fn validate_path_within_base(path: &str, base_dir: &str) -> Result<()> { let path_buf = PathBuf::from(path); let base_buf = PathBuf::from(base_dir); - // Canonicalize if possible, but don't fail if paths don't exist yet - let canonical_path = path_buf.canonicalize().unwrap_or_else(|_| { - // If canonicalization fails, do our best with normalization - normalize_path(&path_buf) + // Convert both paths to absolute paths for consistent comparison + let current_dir = std::env::current_dir().unwrap_or_default(); + + let absolute_base = if base_buf.is_absolute() { + base_buf + } else { + current_dir.join(&base_buf) + }; + + let absolute_path = if path_buf.is_absolute() { + path_buf + } else { + current_dir.join(&path_buf) + }; + + // Try to canonicalize both paths, with consistent fallback behavior + let canonical_base = absolute_base.canonicalize().unwrap_or_else(|_| { + // If canonicalization fails, use the absolute path through normalize_path + normalize_path(&absolute_base) }); - let canonical_base = base_buf.canonicalize().unwrap_or_else(|_| { - normalize_path(&base_buf) - }); + let canonical_path = if absolute_path.exists() { + // If the file exists, canonicalize it + absolute_path.canonicalize().unwrap_or_else(|_| normalize_path(&absolute_path)) + } else { + // If file doesn't exist, try to canonicalize its parent directory and append the filename + if let Some(parent) = absolute_path.parent() { + if let Some(filename) = absolute_path.file_name() { + let canonical_parent = parent.canonicalize().unwrap_or_else(|_| normalize_path(parent)); + canonical_parent.join(filename) + } else { + normalize_path(&absolute_path) + } + } else { + normalize_path(&absolute_path) + } + }; // Add debug logging to diagnose path validation issues - eprintln!("DEBUG: Path validation:"); - eprintln!(" Input path: '{}'", path); - eprintln!(" Input base: '{}'", base_dir); - eprintln!(" Canonical path: '{}'", canonical_path.display()); - eprintln!(" Canonical base: '{}'", canonical_base.display()); - eprintln!(" Starts with check: {}", canonical_path.starts_with(&canonical_base)); + debug!("Path validation: input_path='{}', base_dir='{}'", path, base_dir); + debug!("Path validation: absolute_path='{}', absolute_base='{}'", absolute_path.display(), absolute_base.display()); + debug!("Path validation: canonical_path='{}', canonical_base='{}'", canonical_path.display(), canonical_base.display()); + debug!("Path validation: starts_with_check={}", canonical_path.starts_with(&canonical_base)); if !canonical_path.starts_with(&canonical_base) { return Err(anyhow::anyhow!( - "Path '{}' is not within allowed base directory '{}' (failed after {:?})", - path, base_dir, std::time::Instant::now().elapsed() + "Path '{}' is not within allowed base directory '{}'", + path, base_dir )); } @@ -236,4 +262,91 @@ mod tests { assert_eq!(sanitize_filename(" report.pdf "), "report.pdf"); assert_eq!(sanitize_filename("file:name|test.doc"), "file_name_test.doc"); } + + #[test] + fn test_validate_path_within_base() { + use std::fs; + + // Setup test directories + let test_base = "test_uploads_validation"; + let test_docs = format!("{}/documents", test_base); + + // Clean up any existing test directories + fs::remove_dir_all(test_base).unwrap_or(()); + + // Test 1: Neither base nor parent exists + let result = validate_path_within_base( + "./test_uploads_validation/documents/test.txt", + "./test_uploads_validation" + ); + assert!(result.is_ok(), "Should allow paths within base even when directories don't exist"); + + // Test 2: Base exists but parent doesn't (the problematic case) + fs::create_dir_all(test_base).unwrap(); + let result = validate_path_within_base( + "./test_uploads_validation/documents/test.txt", + "./test_uploads_validation" + ); + assert!(result.is_ok(), "Should allow subdirectory paths when base exists but parent doesn't"); + + // Test 3: Both base and parent exist + fs::create_dir_all(&test_docs).unwrap(); + let result = validate_path_within_base( + "./test_uploads_validation/documents/test.txt", + "./test_uploads_validation" + ); + assert!(result.is_ok(), "Should allow paths when both base and parent exist"); + + // Test 4: Path outside base directory should fail + let result = validate_path_within_base( + "../outside.txt", + "./test_uploads_validation" + ); + assert!(result.is_err(), "Should reject paths outside base directory"); + + // Test 5: Absolute paths + let current_dir = std::env::current_dir().unwrap(); + let abs_base = current_dir.join(test_base); + let abs_path = abs_base.join("documents/test.txt"); + + let result = validate_path_within_base( + &abs_path.to_string_lossy(), + &abs_base.to_string_lossy() + ); + assert!(result.is_ok(), "Should handle absolute paths correctly"); + + // Test 6: Mixed absolute and relative paths + let result = validate_path_within_base( + &abs_path.to_string_lossy(), + "./test_uploads_validation" + ); + assert!(result.is_ok(), "Should handle mixed absolute/relative paths"); + + // Clean up + fs::remove_dir_all(test_base).unwrap_or(()); + } + + #[test] + fn test_validate_path_within_base_traversal_attempts() { + use std::fs; + + let test_base = "test_security_validation"; + fs::create_dir_all(test_base).unwrap_or(()); + + // Test various path traversal attempts + let traversal_attempts = vec![ + "../../../etc/passwd", + "./test_security_validation/../../../etc/passwd", + "test_security_validation/../outside.txt", + "./test_security_validation/documents/../../outside.txt", + ]; + + for attempt in traversal_attempts { + let result = validate_path_within_base(attempt, "./test_security_validation"); + assert!(result.is_err(), "Should reject path traversal attempt: {}", attempt); + } + + // Clean up + fs::remove_dir_all(test_base).unwrap_or(()); + } } \ No newline at end of file