diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index d578769..0422e12 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -239,12 +239,10 @@ pub async fn upload( // vanish with, at best, a vague "could not be processed". Rejecting here gives them a // reason at the door that they can act on, and it uses the SAME budget the worker // enforces, so admission and processing cannot disagree. - if mime.starts_with("image/") - && let Err(e) = crate::services::imaging::probe_decodable(&temp_abs) - { + if mime.starts_with("image/") && crate::services::imaging::exceeds_decode_budget(&temp_abs) { let mp = crate::services::imaging::megapixels(&temp_abs); tracing::info!( - error = ?e, %mime, megapixels = ?mp, + %mime, megapixels = ?mp, "rejecting an image that exceeds the decode budget at admission" ); let _ = tokio::fs::remove_file(&temp_abs).await; diff --git a/backend/src/services/imaging.rs b/backend/src/services/imaging.rs index dc78b47..0266a5c 100644 --- a/backend/src/services/imaging.rs +++ b/backend/src/services/imaging.rs @@ -99,13 +99,29 @@ pub fn megapixels(path: &Path) -> Option { Some(f64::from(w) * f64::from(h) / 1_000_000.0) } -/// Reject an image the compression worker could never process, reading only its header. +/// True when an image cannot be decoded specifically because it would exceed the memory +/// budget — read from the header, no pixels touched. /// -/// Called at upload admission so the guest is told at the door, with a reason they can act -/// on, instead of the upload being accepted with a 201 and then silently soft-deleted -/// minutes later when the worker gives up on it. -pub fn probe_decodable(path: &Path) -> Result<()> { - decoder_within_budget(path).map(|_| ()) +/// Called at upload admission so a guest who sends a 100 MP photo is told at the door, with +/// a reason they can act on, instead of the upload being accepted with a 201 and then +/// silently soft-deleted minutes later when the worker gives up on it. +/// +/// Deliberately narrow: ONLY the budget. A corrupt, truncated or unsupported file also +/// fails to build a decoder, but rejecting those here would change a contract the +/// adversarial suite pins on purpose — acceptance follows the magic bytes, and a payload +/// with a valid JPEG header is accepted regardless of what follows it. Those go to the +/// compression worker as before, which handles them gracefully and (since the retry +/// classifier) no longer burns backoff on them. +pub fn exceeds_decode_budget(path: &Path) -> bool { + match decoder_within_budget(path) { + Ok(_) => false, + Err(e) => e.chain().any(|cause| { + matches!( + cause.downcast_ref::(), + Some(image::ImageError::Limits(_)) + ) + }), + } } /// Decode an image from disk with decompression-bomb limits applied and its EXIF @@ -186,23 +202,49 @@ mod tests { } #[test] - fn probe_agrees_with_the_decoder_on_both_sides() { - // Admission and processing must never disagree — a photo accepted at the door and - // then rejected by the worker is the exact failure this pair exists to prevent. + fn admission_rejects_only_the_over_budget_case() { + // Admission and processing must agree about SIZE — a photo accepted at the door and + // then rejected by the worker for being too big is the failure this pair prevents. assert!( - probe_decodable(Path::new(HUGE)).is_err(), - "probe must reject what the decoder rejects" + exceeds_decode_budget(Path::new(HUGE)), + "admission must reject what the decoder rejects for size" ); let ordinary = concat!( env!("CARGO_MANIFEST_DIR"), "/../e2e/fixtures/media/portrait-exif6.jpg" ); assert!( - probe_decodable(Path::new(ordinary)).is_ok(), - "probe must accept what the decoder accepts" + !exceeds_decode_budget(Path::new(ordinary)), + "admission must accept an ordinary photo" ); } + #[test] + fn admission_does_not_reject_a_merely_undecodable_file() { + // The narrowing that keeps the adversarial contract intact: a payload with valid + // JPEG magic bytes and nothing behind them cannot be decoded, but acceptance follows + // the magic bytes by design (07-adversarial/file-upload-attacks). It is the worker's + // job to fail it, not admission's — admission is only the resource guard. + let dir = std::env::temp_dir().join("eventsnap-imaging-test"); + std::fs::create_dir_all(&dir).expect("tmp dir"); + let stub = dir.join("magic-only.jpg"); + let mut bytes = vec![0u8; 1024]; + bytes[..3].copy_from_slice(&[0xFF, 0xD8, 0xFF]); + std::fs::write(&stub, &bytes).expect("write stub"); + + assert!( + !exceeds_decode_budget(&stub), + "a corrupt file is not an over-budget file" + ); + assert!( + decode_oriented(&stub) + .map(|i| (i.width(), i.height())) + .is_err(), + "...but it must still fail in the worker" + ); + let _ = std::fs::remove_file(&stub); + } + #[test] fn still_decodes_an_ordinary_photo_and_applies_orientation() { // The guard must not have become a blanket refusal. This fixture is 40x20 stored with