From 749e16f099061d37cace66d1489fcee0cd909d51 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sat, 7 Jun 2025 22:27:37 -0700 Subject: [PATCH 1/2] Fix #119 --- src/extensions/parser.rs | 6 +- src/extensions/session.rs | 5 +- src/util/sanitizers.rs | 196 +++++++++++++++++++++++++++++++++++++- 3 files changed, 201 insertions(+), 6 deletions(-) diff --git a/src/extensions/parser.rs b/src/extensions/parser.rs index d7b6384..05f3482 100644 --- a/src/extensions/parser.rs +++ b/src/extensions/parser.rs @@ -22,6 +22,7 @@ use crate::{ sum::Sum, }, error::LogriaError, + sanitizers::sanitize_filename, }, }; @@ -54,9 +55,10 @@ impl ExtensionMethods for Parser { /// Create parser file from a Parser struct fn save(self, file_name: &str) -> Result<(), LogriaError> { let parser_json = serde_json::to_string_pretty(&self).unwrap(); - let path = format!("{}/{}", patterns(), file_name); + let sanitized_filename = sanitize_filename(file_name); + let path = format!("{}/{}", patterns(), sanitized_filename); - match write(format!("{}/{}", patterns(), file_name), parser_json) { + match write(&path, parser_json) { Ok(()) => Ok(()), Err(why) => Err(LogriaError::CannotWrite(path, ::to_string(&why))), } diff --git a/src/extensions/session.rs b/src/extensions/session.rs index 3bbd97a..bbc4037 100644 --- a/src/extensions/session.rs +++ b/src/extensions/session.rs @@ -11,7 +11,7 @@ use serde::{Deserialize, Serialize}; use crate::{ constants::{cli::excludes::SESSION_FILE_EXCLUDES, directories::sessions}, extensions::extension::ExtensionMethods, - util::error::LogriaError, + util::{error::LogriaError, sanitizers::sanitize_filename}, }; #[derive(Eq, Hash, PartialEq, Serialize, Deserialize, Debug)] @@ -39,7 +39,8 @@ impl ExtensionMethods for Session { /// Create session file from a Session struct fn save(self, file_name: &str) -> Result<(), LogriaError> { let session_json = serde_json::to_string_pretty(&self).unwrap(); - let path = format!("{}/{}", sessions(), file_name); + let sanitized_filename = sanitize_filename(file_name); + let path = format!("{}/{}", sessions(), sanitized_filename); match write(&path, session_json) { Ok(()) => Ok(()), Err(why) => Err(LogriaError::CannotWrite(path, ::to_string(&why))), diff --git a/src/util/sanitizers.rs b/src/util/sanitizers.rs index 5101399..8ce524c 100644 --- a/src/util/sanitizers.rs +++ b/src/util/sanitizers.rs @@ -1,8 +1,93 @@ +use std::{borrow::Cow, cmp::max, str::from_utf8}; + use regex::bytes::Regex; -use std::{cmp::max, str::from_utf8}; use crate::constants::cli::patterns::ANSI_COLOR_PATTERN; +/// Sanitize a filename by replacing or removing characters that are not allowed in filenames +/// across different operating systems (Windows, macOS, Linux) +/// +/// Uses `Cow` to avoid unnecessary allocations when the filename is already valid. +pub fn sanitize_filename(filename: &str) -> Cow { + // Characters that are not allowed in filenames on Windows, macOS, or Linux + // Windows: < > : " | ? * \ / + // Also includes control characters (0-31) and DEL (127) + // Reserved names on Windows: CON, PRN, AUX, NUL, COM1-9, LPT1-9 + + // Check if we need to trim leading/trailing whitespace and dots + let trimmed = filename.trim_matches(|c: char| c.is_whitespace() || c == '.'); + let needs_trimming = trimmed.len() != filename.len(); + + // Check if any characters need to be replaced + let has_invalid_chars = trimmed.chars().any(|c| match c { + '<' | '>' | ':' | '"' | '|' | '?' | '*' | '\\' | '/' => true, + c if c.is_control() => true, + _ => false, + }); + + // Check if it's a Windows reserved name + let upper_name = trimmed.to_uppercase(); + let reserved_names = [ + "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", + "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + ]; + let is_reserved = reserved_names.contains(&upper_name.as_str()); + + // Check if it's empty after trimming + let is_empty = trimmed.is_empty(); + + // Check if it's too long + let is_too_long = trimmed.len() > 255; + + // If no changes are needed, return the original string (no allocation) + if !needs_trimming && !has_invalid_chars && !is_reserved && !is_empty && !is_too_long { + return Cow::Borrowed(filename); + } + + // Use Cow to minimize allocations during the sanitization process + let mut result: Cow = if needs_trimming { + Cow::Owned(trimmed.to_string()) + } else { + Cow::Borrowed(filename) + }; + + // Replace invalid characters if needed + if has_invalid_chars { + result = Cow::Owned( + result + .chars() + .map(|c| match c { + // Windows/general forbidden characters + '<' | '>' | ':' | '"' | '|' | '?' | '*' | '\\' | '/' => '_', + // Control characters and DEL + c if c.is_control() => '_', + // Keep valid characters + c => c, + }) + .collect::(), + ); + } + + // Handle Windows reserved names by appending underscore + if is_reserved { + result = Cow::Owned(format!("{}_", result)); + } + + // Handle empty filename + if is_empty { + result = Cow::Borrowed("unnamed_session"); + } + + // Limit length to 255 characters (common filesystem limit) + if result.len() > 255 { + let mut truncated = result.into_owned(); + truncated.truncate(255); + result = Cow::Owned(truncated); + } + + result +} + pub struct LengthFinder { color_pattern: Regex, } @@ -34,7 +119,8 @@ impl LengthFinder { #[cfg(test)] mod tests { - use crate::util::sanitizers::LengthFinder; + use std::borrow::Cow; + use crate::util::sanitizers::{LengthFinder, sanitize_filename}; #[test] fn test_length_clean() { @@ -81,4 +167,110 @@ mod tests { assert_eq!(rows, 1); assert_eq!(length, 6); } + + #[test] + fn test_sanitize_filename_clean() { + assert_eq!(sanitize_filename("normal_filename"), "normal_filename"); + assert_eq!(sanitize_filename("file.txt"), "file.txt"); + assert_eq!(sanitize_filename("file_123"), "file_123"); + } + + #[test] + fn test_sanitize_filename_invalid_chars() { + assert_eq!(sanitize_filename("file<>name"), "file__name"); + assert_eq!(sanitize_filename("file:name"), "file_name"); + assert_eq!(sanitize_filename("file\"name"), "file_name"); + assert_eq!(sanitize_filename("file|name"), "file_name"); + assert_eq!(sanitize_filename("file?name"), "file_name"); + assert_eq!(sanitize_filename("file*name"), "file_name"); + assert_eq!(sanitize_filename("file\\name"), "file_name"); + assert_eq!(sanitize_filename("file/name"), "file_name"); + } + + #[test] + fn test_sanitize_filename_control_chars() { + assert_eq!(sanitize_filename("file\x00name"), "file_name"); + assert_eq!(sanitize_filename("file\x1fname"), "file_name"); + assert_eq!(sanitize_filename("file\x7fname"), "file_name"); + } + + #[test] + fn test_sanitize_filename_trim() { + assert_eq!(sanitize_filename(" filename "), "filename"); + assert_eq!(sanitize_filename("..filename.."), "filename"); + assert_eq!(sanitize_filename("\tfilename\t"), "filename"); + } + + #[test] + fn test_sanitize_filename_reserved_names() { + assert_eq!(sanitize_filename("CON"), "CON_"); + assert_eq!(sanitize_filename("con"), "con_"); + assert_eq!(sanitize_filename("PRN"), "PRN_"); + assert_eq!(sanitize_filename("COM1"), "COM1_"); + assert_eq!(sanitize_filename("LPT1"), "LPT1_"); + } + + #[test] + fn test_sanitize_filename_empty() { + assert_eq!(sanitize_filename(""), "unnamed_session"); + assert_eq!(sanitize_filename(" "), "unnamed_session"); + assert_eq!(sanitize_filename("..."), "unnamed_session"); + } + + #[test] + fn test_sanitize_filename_long() { + let long_name = "a".repeat(300); + let sanitized = sanitize_filename(&long_name); + assert_eq!(sanitized.len(), 255); + } + + #[test] + fn test_sanitize_filename_no_allocation_needed() { + // These should not require any modifications, demonstrating Cow optimization + let clean_names = vec![ + "normal_filename", + "file.txt", + "file_123", + "valid-name.log", + "test_file_2024.json" + ]; + + for name in clean_names { + let result = sanitize_filename(name); + assert_eq!(result, name); + // The function should return efficiently without unnecessary string operations + } + } + + #[test] + fn test_sanitize_filename_cow_borrowed_optimization() { + // Test that clean filenames return Cow::Borrowed (no allocation) + let clean_name = "clean_filename.txt"; + let result = sanitize_filename(clean_name); + + // Verify the result is correct + assert_eq!(result, clean_name); + + // Verify it's a borrowed reference (no allocation) + match result { + Cow::Borrowed(_) => {}, // This is what we want + Cow::Owned(_) => panic!("Expected Cow::Borrowed for clean filename, got Cow::Owned"), + } + } + + #[test] + fn test_sanitize_filename_cow_owned_when_modified() { + // Test that modified filenames return Cow::Owned (allocation only when needed) + let dirty_name = "dirty<>filename.txt"; + let result = sanitize_filename(dirty_name); + + // Verify the result is sanitized + assert_eq!(result, "dirty__filename.txt"); + + // Verify it's an owned string (allocation was necessary) + match result { + Cow::Owned(_) => {}, // This is what we want when modifications are needed + Cow::Borrowed(_) => panic!("Expected Cow::Owned for modified filename, got Cow::Borrowed"), + } + } } From 9d5a038c5337bbf5f1ec03434671f12095463c42 Mon Sep 17 00:00:00 2001 From: Christopher Sardegna Date: Sat, 7 Jun 2025 23:09:33 -0700 Subject: [PATCH 2/2] Reduce scope of sanitization routine, since input is controlled --- src/util/sanitizers.rs | 178 ++++++----------------------------------- 1 file changed, 26 insertions(+), 152 deletions(-) diff --git a/src/util/sanitizers.rs b/src/util/sanitizers.rs index 8ce524c..5bac030 100644 --- a/src/util/sanitizers.rs +++ b/src/util/sanitizers.rs @@ -1,91 +1,32 @@ -use std::{borrow::Cow, cmp::max, str::from_utf8}; +use std::{cmp::max, collections::HashSet, str::from_utf8, sync::LazyLock}; use regex::bytes::Regex; use crate::constants::cli::patterns::ANSI_COLOR_PATTERN; -/// Sanitize a filename by replacing or removing characters that are not allowed in filenames -/// across different operating systems (Windows, macOS, Linux) -/// -/// Uses `Cow` to avoid unnecessary allocations when the filename is already valid. -pub fn sanitize_filename(filename: &str) -> Cow { - // Characters that are not allowed in filenames on Windows, macOS, or Linux - // Windows: < > : " | ? * \ / - // Also includes control characters (0-31) and DEL (127) - // Reserved names on Windows: CON, PRN, AUX, NUL, COM1-9, LPT1-9 - - // Check if we need to trim leading/trailing whitespace and dots - let trimmed = filename.trim_matches(|c: char| c.is_whitespace() || c == '.'); - let needs_trimming = trimmed.len() != filename.len(); - - // Check if any characters need to be replaced - let has_invalid_chars = trimmed.chars().any(|c| match c { - '<' | '>' | ':' | '"' | '|' | '?' | '*' | '\\' | '/' => true, - c if c.is_control() => true, - _ => false, - }); - - // Check if it's a Windows reserved name - let upper_name = trimmed.to_uppercase(); - let reserved_names = [ - "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", - "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", - ]; - let is_reserved = reserved_names.contains(&upper_name.as_str()); - - // Check if it's empty after trimming - let is_empty = trimmed.is_empty(); - - // Check if it's too long - let is_too_long = trimmed.len() > 255; - - // If no changes are needed, return the original string (no allocation) - if !needs_trimming && !has_invalid_chars && !is_reserved && !is_empty && !is_too_long { - return Cow::Borrowed(filename); - } - - // Use Cow to minimize allocations during the sanitization process - let mut result: Cow = if needs_trimming { - Cow::Owned(trimmed.to_string()) - } else { - Cow::Borrowed(filename) - }; - - // Replace invalid characters if needed - if has_invalid_chars { - result = Cow::Owned( - result - .chars() - .map(|c| match c { - // Windows/general forbidden characters - '<' | '>' | ':' | '"' | '|' | '?' | '*' | '\\' | '/' => '_', - // Control characters and DEL - c if c.is_control() => '_', - // Keep valid characters - c => c, - }) - .collect::(), - ); - } - - // Handle Windows reserved names by appending underscore - if is_reserved { - result = Cow::Owned(format!("{}_", result)); - } - - // Handle empty filename - if is_empty { - result = Cow::Borrowed("unnamed_session"); - } - - // Limit length to 255 characters (common filesystem limit) - if result.len() > 255 { - let mut truncated = result.into_owned(); - truncated.truncate(255); - result = Cow::Owned(truncated); - } - - result +/// Characters disallowed in a filename +static FILENAME_DISALLOWED_CHARS: LazyLock> = + LazyLock::new(|| HashSet::from(['*', '"', '/', '\\', '<', '>', ':', '|', '?', '.'])); +/// The character to replace disallowed chars with +const FILENAME_REPLACEMENT_CHAR: char = '_'; + +/// Remove unsafe chars in [this list](FILENAME_DISALLOWED_CHARS). +/// +/// Does not need to use a `Cow` for optimization because the source is always generated based on chat data +/// so there is no opportunity for the original input to be passed in from another borrow. +pub fn sanitize_filename(filename: &str) -> String { + filename + .trim() + .chars() + .map(|letter| { + if letter.is_control() || FILENAME_DISALLOWED_CHARS.contains(&letter) { + FILENAME_REPLACEMENT_CHAR + } else { + letter + } + }) + .take(255) + .collect() } pub struct LengthFinder { @@ -119,7 +60,6 @@ impl LengthFinder { #[cfg(test)] mod tests { - use std::borrow::Cow; use crate::util::sanitizers::{LengthFinder, sanitize_filename}; #[test] @@ -171,7 +111,7 @@ mod tests { #[test] fn test_sanitize_filename_clean() { assert_eq!(sanitize_filename("normal_filename"), "normal_filename"); - assert_eq!(sanitize_filename("file.txt"), "file.txt"); + assert_eq!(sanitize_filename("file.txt"), "file_txt"); assert_eq!(sanitize_filename("file_123"), "file_123"); } @@ -197,80 +137,14 @@ mod tests { #[test] fn test_sanitize_filename_trim() { assert_eq!(sanitize_filename(" filename "), "filename"); - assert_eq!(sanitize_filename("..filename.."), "filename"); + assert_eq!(sanitize_filename("..filename.."), "__filename__"); assert_eq!(sanitize_filename("\tfilename\t"), "filename"); } - #[test] - fn test_sanitize_filename_reserved_names() { - assert_eq!(sanitize_filename("CON"), "CON_"); - assert_eq!(sanitize_filename("con"), "con_"); - assert_eq!(sanitize_filename("PRN"), "PRN_"); - assert_eq!(sanitize_filename("COM1"), "COM1_"); - assert_eq!(sanitize_filename("LPT1"), "LPT1_"); - } - - #[test] - fn test_sanitize_filename_empty() { - assert_eq!(sanitize_filename(""), "unnamed_session"); - assert_eq!(sanitize_filename(" "), "unnamed_session"); - assert_eq!(sanitize_filename("..."), "unnamed_session"); - } - #[test] fn test_sanitize_filename_long() { let long_name = "a".repeat(300); let sanitized = sanitize_filename(&long_name); assert_eq!(sanitized.len(), 255); } - - #[test] - fn test_sanitize_filename_no_allocation_needed() { - // These should not require any modifications, demonstrating Cow optimization - let clean_names = vec![ - "normal_filename", - "file.txt", - "file_123", - "valid-name.log", - "test_file_2024.json" - ]; - - for name in clean_names { - let result = sanitize_filename(name); - assert_eq!(result, name); - // The function should return efficiently without unnecessary string operations - } - } - - #[test] - fn test_sanitize_filename_cow_borrowed_optimization() { - // Test that clean filenames return Cow::Borrowed (no allocation) - let clean_name = "clean_filename.txt"; - let result = sanitize_filename(clean_name); - - // Verify the result is correct - assert_eq!(result, clean_name); - - // Verify it's a borrowed reference (no allocation) - match result { - Cow::Borrowed(_) => {}, // This is what we want - Cow::Owned(_) => panic!("Expected Cow::Borrowed for clean filename, got Cow::Owned"), - } - } - - #[test] - fn test_sanitize_filename_cow_owned_when_modified() { - // Test that modified filenames return Cow::Owned (allocation only when needed) - let dirty_name = "dirty<>filename.txt"; - let result = sanitize_filename(dirty_name); - - // Verify the result is sanitized - assert_eq!(result, "dirty__filename.txt"); - - // Verify it's an owned string (allocation was necessary) - match result { - Cow::Owned(_) => {}, // This is what we want when modifications are needed - Cow::Borrowed(_) => panic!("Expected Cow::Owned for modified filename, got Cow::Borrowed"), - } - } }