Address code review feedback - improve path traversal detection and header sanitization

Copilot · nielsdrost7 · Copilot · commit 50b47efb8a84 · 2026-01-14T04:53:07.000Z
Co-authored-by: nielsdrost7 &lt;47660417+nielsdrost7@users.noreply.github.com&gt;
diff --git a/application/helpers/file_security_helper.php b/application/helpers/file_security_helper.php
@@ -36,8 +36,15 @@ function validate_safe_filename(string $filename): array
         return ['valid' => false, 'hash' => $hash, 'error' => 'empty_filename'];
     }
 
-    // Check for path traversal sequences
-    if (str_contains($filename, '../') || str_contains($filename, '..\\')) {
+    // Check for path traversal sequences (including standalone ..)
+    // Matches: ../, ..\, /.. and standalone ..
+    if (str_contains($filename, '../') || 
+        str_contains($filename, '..\\') || 
+        str_contains($filename, '/..') ||
+        str_contains($filename, '\\..') ||
+        $filename === '..' ||
+        str_starts_with($filename, '../') ||
+        str_starts_with($filename, '..\\')) {
         return ['valid' => false, 'hash' => $hash, 'error' => 'path_traversal'];
     }
 
@@ -91,9 +98,16 @@ function validate_file_in_directory(string $filePath, string $baseDirectory): bo
  */
 function sanitize_filename_for_header(string $filename): string
 {
-    // Remove all control characters (0x00-0x1F, 0x7F) and quotes
-    // This prevents CRLF injection and response splitting attacks
-    return preg_replace('/[\x00-\x1F\x7F"]/', '', $filename);
+    // Remove all control characters (0x00-0x1F, 0x7F), quotes, and backslashes
+    // This prevents CRLF injection and response splitting attacks, including backslash-based bypasses
+    $sanitized = preg_replace('/[\x00-\x1F\x7F"\\\\]/', '', $filename);
+    
+    // If sanitization results in empty string, use safe fallback
+    if (empty($sanitized)) {
+        return 'attachment.bin';
+    }
+    
+    return $sanitized;
 }
 
 /**
diff --git a/application/modules/upload/controllers/Upload.php b/application/modules/upload/controllers/Upload.php
@@ -116,6 +116,7 @@ public function delete_file(string $url_key): void
     public function get_file($filename): void
     {
         // Security: Removed urldecode() - CodeIgniter already handles URL decoding
+        // First sanitize to handle the url_key_filename format
         $filename = $this->sanitize_file_name($filename);
 
         $underscorePos = mb_strpos($filename, '_');
@@ -127,18 +128,29 @@ public function get_file($filename): void
 
         $url_key       = mb_substr($filename, 0, $underscorePos);
         $real_filename = mb_substr($filename, $underscorePos + 1);
-        $fullPath      = $this->get_target_file_path($url_key, $real_filename);
         
-        if ( ! file_exists($fullPath)) {
-            log_message('debug', 'upload: File not found in uploads directory');
-            $this->respond_message(404, 'upload_error_file_not_found', 'File not found');
-        }
+        // Security: Use comprehensive validation on the real filename component
+        $validation = validate_file_access($real_filename, $this->targetPath);
         
-        // Security: Validate file is within allowed directory
-        if (!validate_file_in_directory($fullPath, $this->targetPath)) {
-            $filenameHash = hash_for_logging($filename);
-            log_message('error', 'upload: Path traversal detected (hash: ' . $filenameHash . ')');
-            $this->respond_message(403, 'upload_error_unauthorized_access', 'Unauthorized access');
+        // For uploads, we need to check the actual path with url_key prefix
+        if (!$validation['valid'] || $validation['error'] === 'file_not_found') {
+            // Try with the url_key prefix (uploads use url_key_filename format)
+            $fullPath = $this->get_target_file_path($url_key, $real_filename);
+            
+            if (!file_exists($fullPath)) {
+                log_message('debug', 'upload: File not found in uploads directory');
+                $this->respond_message(404, 'upload_error_file_not_found', 'File not found');
+            }
+            
+            // Validate the actual path is within allowed directory
+            if (!validate_file_in_directory($fullPath, $this->targetPath)) {
+                $filenameHash = hash_for_logging($filename);
+                log_message('error', 'upload: Path traversal detected (hash: ' . $filenameHash . ')');
+                $this->respond_message(403, 'upload_error_unauthorized_access', 'Unauthorized access');
+            }
+        } else {
+            // Use validated path from helper
+            $fullPath = $this->get_target_file_path($url_key, $real_filename);
         }
         
         $path_parts = pathinfo($fullPath);
