From ceb68939a7fba7e0838f83fda142fb68dc62cff7 Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 29 Jul 2026 07:55:56 +0200 Subject: [PATCH] fix(upload): narrow the admission check to the memory budget only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admission check I just added rejected ANY image the decoder couldn't build — corrupt, truncated, or unsupported, not only over-budget. That broke two adversarial tests, and they were right to break. 07-adversarial/file-upload-attacks pins, deliberately, that acceptance follows the MAGIC BYTES: a payload whose first three bytes are a JPEG header is accepted regardless of what follows, because the security property under test is that the client-declared Content-Type has no influence. Both failing cases upload 1024 bytes of JPEG magic followed by zeros. Rejecting those at admission is a different, broader contract than the one asked for, and rewriting an adversarial test to match new behaviour is precisely the thing that needs justifying rather than doing quietly. So admission now checks only what it was meant to: `exceeds_decode_budget` returns true solely for `ImageError::Limits`. A corrupt file goes to the compression worker exactly as before — which handles it gracefully and, since the retry classifier in the previous commit, no longer burns backoff on it. The resource guard is the part that had to move earlier; nothing else did. Tests: the size agreement between admission and the worker is still asserted in both directions, plus a new one writing a magic-bytes-only stub and asserting admission accepts it WHILE the worker still rejects it — pinning the boundary between the two checks so a future widening fails here rather than in the adversarial suite. Co-Authored-By: Claude Opus 5 (1M context) --- backend/src/handlers/upload.rs | 6 +-- backend/src/services/imaging.rs | 68 ++++++++++++++++++++++++++------- 2 files changed, 57 insertions(+), 17 deletions(-) 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