Fix path traversal, encoding, and edge case bugs

pixstrip-core/src/executor.rs

@@ -33,6 +33,8 @@ pub struct BatchResult {
     pub total_output_bytes: u64,
     pub errors: Vec<(String, String)>,
     pub elapsed_ms: u64,
+    /// Actual output file paths written by the executor (only successfully written files).
+    pub output_files: Vec<String>,
 }
 
 pub struct PipelineExecutor {
@@ -98,6 +100,7 @@ impl PipelineExecutor {
                 total_output_bytes: 0,
                 errors: Vec::new(),
                 elapsed_ms: 0,
+                output_files: Vec::new(),
             });
         }
 
@@ -122,6 +125,7 @@ impl PipelineExecutor {
         let output_bytes = AtomicU64::new(0);
         let cancelled = AtomicBool::new(false);
         let errors: Mutex<Vec<(String, String)>> = Mutex::new(Vec::new());
+        let written_files: Mutex<Vec<String>> = Mutex::new(Vec::new());
 
         // Channel for progress updates from worker threads to the callback
         let (tx, rx) = std::sync::mpsc::channel::<ProgressUpdate>();
@@ -141,6 +145,7 @@ impl PipelineExecutor {
             let output_bytes_ref = &output_bytes;
             let cancelled_ref = &cancelled;
             let errors_ref = &errors;
+            let written_ref = &written_files;
 
             let worker = scope.spawn(move || {
                 pool.install(|| {
@@ -189,10 +194,15 @@ impl PipelineExecutor {
                         });
 
                         match Self::process_single_static(job, source, &loader, &encoder, idx) {
-                            Ok((in_size, out_size)) => {
+                            Ok((in_size, out_size, out_path)) => {
                                 succeeded_ref.fetch_add(1, Ordering::Relaxed);
                                 input_bytes_ref.fetch_add(in_size, Ordering::Relaxed);
                                 output_bytes_ref.fetch_add(out_size, Ordering::Relaxed);
+                                if !out_path.as_os_str().is_empty() {
+                                    if let Ok(mut wf) = written_ref.lock() {
+                                        wf.push(out_path.display().to_string());
+                                    }
+                                }
                             }
                             Err(e) => {
                                 failed_ref.fetch_add(1, Ordering::Relaxed);
@@ -231,6 +241,7 @@ impl PipelineExecutor {
             total_output_bytes: output_bytes.load(Ordering::Relaxed),
             errors: errors.into_inner().unwrap_or_default(),
             elapsed_ms: start.elapsed().as_millis() as u64,
+            output_files: written_files.into_inner().unwrap_or_default(),
         })
     }
 
@@ -256,6 +267,7 @@ impl PipelineExecutor {
             total_output_bytes: 0,
             errors: Vec::new(),
             elapsed_ms: 0,
+            output_files: Vec::new(),
         };
 
         for (i, source) in job.sources.iter().enumerate() {
@@ -291,10 +303,13 @@ impl PipelineExecutor {
             });
 
             match Self::process_single_static(job, source, &loader, &encoder, i) {
-                Ok((input_size, output_size)) => {
+                Ok((input_size, output_size, out_path)) => {
                     result.succeeded += 1;
                     result.total_input_bytes += input_size;
                     result.total_output_bytes += output_size;
+                    if !out_path.as_os_str().is_empty() {
+                        result.output_files.push(out_path.display().to_string());
+                    }
                 }
                 Err(e) => {
                     result.failed += 1;
@@ -316,14 +331,24 @@ impl PipelineExecutor {
         loader: &ImageLoader,
         encoder: &OutputEncoder,
         index: usize,
-    ) -> std::result::Result<(u64, u64), PixstripError> {
+    ) -> std::result::Result<(u64, u64, std::path::PathBuf), PixstripError> {
         let input_size = std::fs::metadata(&source.path)
             .map(|m| m.len())
             .unwrap_or(0);
 
+        // Metadata stripping via little_exif only works for JPEG/TIFF.
+        // For other formats, force the pixel path so re-encoding strips metadata.
+        let metadata_needs_reencode = job.metadata.as_ref().is_some_and(|m| {
+            !matches!(m, crate::operations::MetadataConfig::KeepAll)
+                && !matches!(
+                    source.original_format,
+                    Some(ImageFormat::Jpeg) | Some(ImageFormat::Tiff)
+                )
+        });
+
         // Fast path: if no pixel processing needed (rename-only or rename+metadata),
         // just copy the file instead of decoding/re-encoding.
-        if !job.needs_pixel_processing() {
+        if !job.needs_pixel_processing() && !metadata_needs_reencode {
             let output_path = if let Some(ref rename) = job.rename {
                 let stem = source.path.file_stem().and_then(|s| s.to_str()).unwrap_or("output");
                 let ext = source.path.extension().and_then(|e| e.to_str()).unwrap_or("jpg");
@@ -333,8 +358,12 @@ impl PipelineExecutor {
                         .ok()
                         .and_then(|r| r.with_guessed_format().ok())
                         .and_then(|r| r.into_dimensions().ok());
+                    // Apply regex find/replace on stem (matching the pixel path)
+                    let working_stem = crate::operations::rename::apply_regex_replace(
+                        stem, &rename.regex_find, &rename.regex_replace,
+                    );
                     let new_name = crate::operations::rename::apply_template_full(
-                        template, stem, ext,
+                        template, &working_stem, ext,
                         rename.counter_start.saturating_add(index as u32),
                         dims, Some(ext), Some(&source.path), None,
                     );
@@ -348,18 +377,26 @@ impl PipelineExecutor {
                     } else {
                         new_name
                     };
-                    job.output_dir.join(new_name)
+                    job.output_dir.join(sanitize_filename(&new_name))
                 } else {
                     let new_name = rename.apply_simple(stem, ext, (index as u32).saturating_add(1));
-                    job.output_dir.join(new_name)
+                    job.output_dir.join(sanitize_filename(&new_name))
                 }
             } else {
                 job.output_path_for(source, None)
             };
 
+            // Prevent overwriting the source file
+            if paths_are_same(&source.path, &output_path) {
+                return Err(PixstripError::Processing {
+                    operation: "output".into(),
+                    reason: format!("output path is the same as source: {}", source.path.display()),
+                });
+            }
+
             let output_path = match job.overwrite_behavior {
                 crate::operations::OverwriteAction::Skip if output_path.exists() => {
-                    return Ok((input_size, 0));
+                    return Ok((input_size, 0, std::path::PathBuf::new()));
                 }
                 crate::operations::OverwriteAction::AutoRename if output_path.exists() => {
                     find_unique_path(&output_path)
@@ -368,10 +405,16 @@ impl PipelineExecutor {
             };
 
             if let Some(parent) = output_path.parent() {
-                std::fs::create_dir_all(parent).map_err(PixstripError::Io)?;
+                std::fs::create_dir_all(parent).map_err(|e| {
+                    cleanup_placeholder(&output_path);
+                    PixstripError::Io(e)
+                })?;
             }
 
-            std::fs::copy(&source.path, &output_path).map_err(PixstripError::Io)?;
+            std::fs::copy(&source.path, &output_path).map_err(|e| {
+                cleanup_placeholder(&output_path);
+                PixstripError::Io(e)
+            })?;
 
             // Metadata handling on the copy
             if let Some(ref meta_config) = job.metadata {
@@ -391,7 +434,7 @@ impl PipelineExecutor {
             }
 
             let output_size = std::fs::metadata(&output_path).map(|m| m.len()).unwrap_or(0);
-            return Ok((input_size, output_size));
+            return Ok((input_size, output_size, output_path));
         }
 
         // Load image
@@ -494,21 +537,29 @@ impl PipelineExecutor {
                 } else {
                     new_name
                 };
-                job.output_dir.join(new_name)
+                job.output_dir.join(sanitize_filename(&new_name))
             } else {
                 let new_name = rename.apply_simple(stem, ext, (index as u32).saturating_add(1));
-                job.output_dir.join(new_name)
+                job.output_dir.join(sanitize_filename(&new_name))
             }
         } else {
             job.output_path_for(source, Some(output_format))
         };
 
+        // Prevent overwriting the source file
+        if paths_are_same(&source.path, &output_path) {
+            return Err(PixstripError::Processing {
+                operation: "output".into(),
+                reason: format!("output path is the same as source: {}", source.path.display()),
+            });
+        }
+
         // Handle overwrite behavior
         let output_path = match job.overwrite_behavior {
             crate::operations::OverwriteAction::Skip => {
                 if output_path.exists() {
                     // Return 0 bytes written - file was skipped
-                    return Ok((input_size, 0));
+                    return Ok((input_size, 0, std::path::PathBuf::new()));
                 }
                 output_path
             }
@@ -524,16 +575,25 @@ impl PipelineExecutor {
 
         // Ensure output directory exists
         if let Some(parent) = output_path.parent() {
-            std::fs::create_dir_all(parent).map_err(PixstripError::Io)?;
+            std::fs::create_dir_all(parent).map_err(|e| {
+                cleanup_placeholder(&output_path);
+                PixstripError::Io(e)
+            })?;
         }
 
         // Watermark (after compress settings determined, before encode)
         if let Some(ref config) = job.watermark {
-            img = apply_watermark(img, config)?;
+            img = apply_watermark(img, config).map_err(|e| {
+                cleanup_placeholder(&output_path);
+                e
+            })?;
         }
 
         // Encode and save
-        encoder.encode_to_file(&img, &output_path, output_format, quality)?;
+        encoder.encode_to_file(&img, &output_path, output_format, quality).map_err(|e| {
+            cleanup_placeholder(&output_path);
+            e
+        })?;
 
         // Metadata handling: re-encoding strips all EXIF by default.
         // KeepAll: copy everything back from source.
@@ -563,7 +623,7 @@ impl PipelineExecutor {
             .map(|m| m.len())
             .unwrap_or(0);
 
-        Ok((input_size, output_size))
+        Ok((input_size, output_size, output_path))
     }
 }
 
@@ -614,14 +674,44 @@ fn auto_orient_from_exif(
         2 => img.fliph(),                                  // Flipped horizontal
         3 => img.rotate180(),                              // Rotated 180
         4 => img.flipv(),                                  // Flipped vertical
-        5 => img.rotate90().fliph(),                       // Transposed
+        5 => img.fliph().rotate270(),                      // Transposed
         6 => img.rotate90(),                               // Rotated 90 CW
-        7 => img.rotate270().fliph(),                      // Transverse
+        7 => img.fliph().rotate90(),                       // Transverse
         8 => img.rotate270(),                              // Rotated 270 CW
         _ => img,
     }
 }
 
+/// Strip path separators and parent-directory components from a user-provided filename.
+/// Prevents path traversal attacks via rename templates (e.g., "../../etc/passwd").
+fn sanitize_filename(name: &str) -> String {
+    let path = std::path::Path::new(name);
+    path.file_name()
+        .and_then(|f| f.to_str())
+        .unwrap_or("output")
+        .to_string()
+}
+
+/// Check if two paths refer to the same file (resolves symlinks and relative paths).
+fn paths_are_same(a: &std::path::Path, b: &std::path::Path) -> bool {
+    match (a.canonicalize(), b.canonicalize()) {
+        (Ok(ca), Ok(cb)) => ca == cb,
+        _ => {
+            // If canonicalize fails (file doesn't exist yet), compare components directly
+            a.as_os_str() == b.as_os_str()
+        }
+    }
+}
+
+/// Remove a 0-byte placeholder file created by find_unique_path on error.
+fn cleanup_placeholder(path: &std::path::Path) {
+    if let Ok(meta) = std::fs::metadata(path) {
+        if meta.len() == 0 {
+            let _ = std::fs::remove_file(path);
+        }
+    }
+}
+
 fn find_unique_path(path: &std::path::Path) -> std::path::PathBuf {
     let stem = path
         .file_stem()
@@ -633,13 +723,21 @@ fn find_unique_path(path: &std::path::Path) -> std::path::PathBuf {
         .unwrap_or("bin");
     let parent = path.parent().unwrap_or_else(|| std::path::Path::new("."));
 
+    // Use O_CREAT | O_EXCL via create_new() to atomically reserve the path,
+    // preventing TOCTOU races when multiple threads auto-rename concurrently.
     for i in 1u32..10000 {
         let candidate = parent.join(format!("{}_{}.{}", stem, i, ext));
-        if !candidate.exists() {
-            return candidate;
+        match std::fs::OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(&candidate)
+        {
+            Ok(_) => return candidate,
+            Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue,
+            Err(_) => continue,
         }
     }
-    // Extremely unlikely fallback
+    // Extremely unlikely fallback - use timestamp for uniqueness
     parent.join(format!("{}_{}.{}", stem, std::time::SystemTime::now()
         .duration_since(std::time::UNIX_EPOCH)
         .map(|d| d.as_millis())