lashman/pixstrip
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Fix 30 critical and high severity bugs from audit passes 6-8

Browse Source 
Critical fixes:
- Prevent path traversal via rename templates (sanitize_filename)
- Prevent input == output data loss (paths_are_same check)
- Undo now uses actual executor output paths instead of scanning directory
- Filter empty paths from output_files (prevents trashing CWD on undo)
- Sanitize URL download filenames to prevent path traversal writes

High severity fixes:
- Fix EXIF orientation 5/7 transforms per spec
- Atomic file creation in find_unique_path (TOCTOU race)
- Clean up 0-byte placeholder files on encoding failure
- Cap canvas padding to 10000px, total dimensions to 65535
- Clamp crop dimensions to minimum 1px
- Clamp DPI to 65535 before u16 cast in JPEG encoder
- Force pixel path for non-JPEG/TIFF metadata stripping
- Fast path now applies regex find/replace on rename stem
- Add output_dpi to needs_pixel_processing check
- Cap watermark image scale dimensions to 16384
- Cap template counter padding to 10
- Cap URL download size to 100MB
- Fix progress bar NaN when total is zero
- Fix calculate_eta underflow when current > total
- Fix loaded.len()-1 underflow in preview callbacks
- Replace ListItem downcast unwrap with if-let
- Fix resize preview division by zero on degenerate images
- Clamp rename cursor position to prevent overflow panic
- Watch mode: skip output dirs to prevent infinite loop
- Watch mode: drop tx sender so channel closes on exit
- Watch mode: add delay for partially-written files
- Watch mode: warn and skip unmatched files instead of wrong preset
- Clean temp download directory on app close
- Replace action downcast unwrap with checked if-let
- Add BatchResult.output_files for accurate undo tracking
This commit is contained in:
lashman
2026-03-07 20:49:10 +02:00
parent b432cc7431
commit adef810691
14 changed files with 207 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
pixstrip-core/src/encoder.rs
View File

@@ -86,8 +86,8 @@ impl OutputEncoder {
if self.options.output_dpi > 0 {
comp.set_pixel_density(mozjpeg::PixelDensity {
unit: mozjpeg::PixelDensityUnit::Inches,
x: self.options.output_dpi as u16,
y: self.options.output_dpi as u16,
x: self.options.output_dpi.min(65535) as u16,
y: self.options.output_dpi.min(65535) as u16,
});
}

144
pixstrip-core/src/executor.rs
View File

@@ -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())

14
pixstrip-core/src/operations/adjustments.rs
View File

@@ -68,12 +68,12 @@ fn crop_to_aspect_ratio(img: DynamicImage, w_ratio: f64, h_ratio: f64) -> Dynami
let (crop_w, crop_h) = if current_ratio > target_ratio {
// Image is wider than target, crop width
let new_w = (ih as f64 * target_ratio) as u32;
(new_w.min(iw), ih)
let new_w = (ih as f64 * target_ratio).round().max(1.0) as u32;
(new_w.min(iw).max(1), ih)
} else {
// Image is taller than target, crop height
let new_h = (iw as f64 / target_ratio) as u32;
(iw, new_h.min(ih))
let new_h = (iw as f64 / target_ratio).round().max(1.0) as u32;
(iw, new_h.min(ih).max(1))
};
let x = iw.saturating_sub(crop_w) / 2;
@@ -193,9 +193,11 @@ fn apply_sepia(img: DynamicImage) -> DynamicImage {
}
fn add_canvas_padding(img: DynamicImage, padding: u32) -> DynamicImage {
// Cap padding to prevent unreasonable memory allocation
let padding = padding.min(10_000);
let (w, h) = (img.width(), img.height());
let new_w = w.saturating_add(padding.saturating_mul(2));
let new_h = h.saturating_add(padding.saturating_mul(2));
let new_w = w.saturating_add(padding.saturating_mul(2)).min(65_535);
let new_h = h.saturating_add(padding.saturating_mul(2)).min(65_535);
let mut canvas = RgbaImage::from_pixel(new_w, new_h, Rgba([255, 255, 255, 255]));

1
pixstrip-core/src/operations/rename.rs
View File

@@ -31,6 +31,7 @@ pub fn apply_template_full(
if let Some(end) = result[start..].find('}') {
let spec = &result[start + 9..start + end];
if let Ok(padding) = spec.parse::<usize>() {
let padding = padding.min(10);
let counter_str = format!("{:0>width$}", counter, width = padding);
result = format!("{}{}{}", &result[..start], counter_str, &result[start + end + 1..]);
}

18
pixstrip-core/src/operations/watermark.rs
View File

@@ -363,12 +363,8 @@ fn apply_tiled_image_watermark(
reason: format!("Failed to load watermark image: {}", e),
})?;
let wm_width = (watermark.width() as f32 * scale) as u32;
let wm_height = (watermark.height() as f32 * scale) as u32;
if wm_width == 0 || wm_height == 0 {
return Ok(img);
}
let wm_width = ((watermark.width() as f32 * scale) as u32).clamp(1, 16384);
let wm_height = ((watermark.height() as f32 * scale) as u32).clamp(1, 16384);
let mut watermark = watermark.resize_exact(wm_width, wm_height, image::imageops::FilterType::Lanczos3);
if let Some(rot) = rotation {
@@ -429,13 +425,9 @@ fn apply_image_watermark(
reason: format!("Failed to load watermark image: {}", e),
})?;
// Scale the watermark
let wm_width = (watermark.width() as f32 * scale) as u32;
let wm_height = (watermark.height() as f32 * scale) as u32;
if wm_width == 0 || wm_height == 0 {
return Ok(img);
}
// Scale the watermark (capped to prevent OOM on extreme scale values)
let wm_width = ((watermark.width() as f32 * scale) as u32).clamp(1, 16384);
let wm_height = ((watermark.height() as f32 * scale) as u32).clamp(1, 16384);
let mut watermark = watermark.resize_exact(
wm_width,

1
pixstrip-core/src/pipeline.rs
View File

@@ -80,6 +80,7 @@ impl ProcessingJob {
|| self.convert.is_some()
|| self.compress.is_some()
|| self.watermark.is_some()
|| self.output_dpi > 0
}
pub fn output_path_for(