diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 55f6830..0422e12 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -233,6 +233,26 @@ pub async fn upload( ))); } + // Images only: refuse anything the compression worker could never decode, reading just + // the header. Without this the upload is accepted with a 201 and then silently + // soft-deleted minutes later when the worker gives up — the guest sees the photo + // 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/") && crate::services::imaging::exceeds_decode_budget(&temp_abs) { + let mp = crate::services::imaging::megapixels(&temp_abs); + tracing::info!( + %mime, megapixels = ?mp, + "rejecting an image that exceeds the decode budget at admission" + ); + let _ = tokio::fs::remove_file(&temp_abs).await; + let detail = mp.map_or(String::new(), |mp| format!(" (ca. {mp:.0} Megapixel)")); + return Err(AppError::BadRequest(format!( + "Bild hat zu viele Bildpunkte{detail} und kann nicht verarbeitet werden. \ + Bitte verkleinere es und lade es erneut hoch." + ))); + } + // Per-user storage quota — dynamic formula based on available disk space and the // number of active uploaders. Gated by master + per-area toggles so the admin can // disable it on trusted instances. diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index 0ad11d7..eba9f43 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -74,6 +74,11 @@ impl CompressionWorker { // an ENOSPC spike while several guests upload at once, a momentary DB-pool // exhaustion, a panic inside the image codec — and the give-up path is // user-visible data loss, so it is worth a few seconds to avoid entering it. + // + // But only for failures that CAN clear. An image that exceeds the decode budget, + // is corrupt, or is in an unsupported format fails identically on every attempt, + // so retrying it just burns 2s + 4s of backoff and writes three near-identical + // warnings before reaching the same conclusion. Give up on those immediately. let mut attempt = 1u32; let outcome = loop { match worker @@ -81,7 +86,10 @@ impl CompressionWorker { .await { Ok(v) => break Ok(v), - Err(e) if attempt < Self::MAX_PROCESS_ATTEMPTS => { + Err(e) + if attempt < Self::MAX_PROCESS_ATTEMPTS + && !crate::services::imaging::is_permanent_image_error(&e) => + { tracing::warn!( error = ?e, %upload_id, attempt, "compression attempt failed; retrying" diff --git a/backend/src/services/imaging.rs b/backend/src/services/imaging.rs index 6854c12..0266a5c 100644 --- a/backend/src/services/imaging.rs +++ b/backend/src/services/imaging.rs @@ -34,11 +34,32 @@ fn decode_limits() -> image::Limits { limits } -/// Decode an image from disk with decompression-bomb limits applied and its EXIF -/// orientation baked into the pixels. +/// True when re-running the exact same work on the exact same bytes cannot possibly +/// succeed, so retrying only burns wall-clock and log noise. /// -/// Blocking — call inside `spawn_blocking`. -pub fn decode_oriented(path: &Path) -> Result { +/// Deliberately narrow. Only the `ImageError` variants that are a property of the *input* +/// count: the file will not shrink, gain codec support, or un-corrupt itself between +/// attempts. `IoError` is excluded on purpose — an ENOSPC while writing a derivative, or +/// EMFILE under load, is exactly the transient case the retry exists for. +pub fn is_permanent_image_error(err: &anyhow::Error) -> bool { + err.chain().any(|cause| { + matches!( + cause.downcast_ref::(), + Some( + image::ImageError::Limits(_) + | image::ImageError::Unsupported(_) + | image::ImageError::Decoding(_) + ) + ) + }) +} + +/// Build a decoder for `path` with the budget enforced, WITHOUT reading any pixels. +/// +/// Single source of truth for "may this image be decoded at all": both the upload +/// admission check and the compression worker go through here, so they cannot disagree +/// about what is acceptable. +fn decoder_within_budget(path: &Path) -> Result { let mut reader = image::ImageReader::open(path) .context("failed to open image")? .with_guessed_format() @@ -64,6 +85,51 @@ pub fn decode_oriented(path: &Path) -> Result { decoder .set_limits(limits) .context("image too large to decode within the memory budget")?; + Ok(decoder) +} + +/// Megapixels an image would decode to, or `None` if its header can't be read. Used only +/// to put a concrete number in the message the guest sees. +pub fn megapixels(path: &Path) -> Option { + let reader = image::ImageReader::open(path) + .ok()? + .with_guessed_format() + .ok()?; + let (w, h) = reader.into_dimensions().ok()?; + Some(f64::from(w) * f64::from(h) / 1_000_000.0) +} + +/// 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 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 +/// orientation baked into the pixels. +/// +/// Blocking — call inside `spawn_blocking`. +pub fn decode_oriented(path: &Path) -> Result { + let mut decoder = decoder_within_budget(path)?; // Cheap, and it happens BEFORE any pixels are read: an oversized image costs a header // parse, not an allocation. @@ -108,6 +174,77 @@ mod tests { ); } + #[test] + fn an_oversized_image_is_a_permanent_failure() { + // The retry loop must not burn 2s + 4s of backoff on this: the file will not shrink + // between attempts, so all three attempts reach the identical conclusion. + let err = decode_oriented(Path::new(HUGE)) + .map(|img| (img.width(), img.height())) + .expect_err("fixture must exceed the budget"); + assert!( + is_permanent_image_error(&err), + "a Limits error can never succeed on retry: {err:#}" + ); + } + + #[test] + fn a_plain_io_error_is_not_permanent() { + // The mirror that keeps the classifier honest. ENOSPC while writing a derivative, or + // EMFILE under load, is exactly what the retry exists for — misclassifying those as + // permanent would turn a transient blip back into the data loss round 1 fixed. + let err = decode_oriented(Path::new("/nonexistent/definitely-not-here.jpg")) + .map(|img| (img.width(), img.height())) + .expect_err("a missing file must error"); + assert!( + !is_permanent_image_error(&err), + "an IO error must stay retryable: {err:#}" + ); + } + + #[test] + 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!( + 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!( + !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 diff --git a/e2e/specs/02-upload/oversized-image.spec.ts b/e2e/specs/02-upload/oversized-image.spec.ts index 3e4654a..678ed57 100644 --- a/e2e/specs/02-upload/oversized-image.spec.ts +++ b/e2e/specs/02-upload/oversized-image.spec.ts @@ -1,24 +1,26 @@ /** - * Regression guard — an image that would blow the decode budget must be refused, not - * allocated, and the container must survive it. + * Regression guard — an image that would blow the decode budget must be refused at the + * door, with a reason the guest can act on, and must never allocate. * - * The compression worker sets `max_alloc = 256 MiB`, but that budget was inert: reading the - * EXIF orientation tag requires `ImageReader::into_decoder()`, which skips the - * `limits.reserve(decoder.total_bytes())` that `decode()` performs, and nothing else enforces - * it (the JPEG decoder's `set_limits` only checks support and dimensions). So the only real - * bound was the 12000px per-axis cap — leaving 12000x12000 decodable at 412 MiB, and two - * concurrent decodes at 824 MiB against a 1 GiB container. + * Two defects met here. * - * That mattered acutely because bumping DERIVATIVES_REV makes the first boot after a deploy - * re-decode the whole gallery two at a time: an OOM kill there restarts the container, which - * re-runs the backfill — a boot loop. + * 1. The budget was inert. `max_alloc = 256 MiB` was set, but reading the EXIF orientation + * tag requires `ImageReader::into_decoder()`, which skips the + * `limits.reserve(decoder.total_bytes())` that `decode()` performs — and nothing else + * enforces it (the JPEG decoder's `set_limits` only checks support and dimensions). The + * only real bound was the 12000px per-axis cap, leaving two concurrent decodes at + * 824 MiB against a 1 GiB container. This suite could not have caught it either, because + * the e2e app container had NO memory limit while production is capped at 1 GiB; that cap + * is now mirrored in docker-compose.test.yml so these assertions mean something. * - * This suite could never have caught it, because until now the e2e app container had NO - * memory limit at all while production is capped at 1 GiB. The cap is mirrored in - * docker-compose.test.yml so this test means something. + * 2. Even with the budget restored, the upload was ACCEPTED with a 201 and then silently + * soft-deleted minutes later when the worker gave up — the photo simply vanished, with at + * best a vague "could not be processed". Admission now runs the same budget check against + * the header, so the guest is told immediately and told why. * * Fixture: 11000x9000 = 99 MP, 568 KiB on disk. Deliberately UNDER the per-axis cap, so the - * axis check cannot be what rejects it — 283 MiB decoded against a 256 MiB budget. + * axis check cannot be what rejects it — 283 MiB decoded against a 256 MiB budget. A fixture + * at 13000px would pass this test against a build with no budget at all. */ import { test, expect } from '../../fixtures/test'; import { uploadRaw } from '../../helpers/upload-client'; @@ -30,35 +32,29 @@ const HUGE = join(process.cwd(), 'fixtures', 'media', 'huge-99mp.jpg'); const SAMPLE = join(process.cwd(), 'fixtures', 'media', 'sample.jpg'); test.describe('Upload — an oversized image is refused, not allocated', () => { - test('a 99 MP upload fails compression gracefully and the backend stays up', async ({ - guest, - db, - }) => { - test.setTimeout(90_000); + test('a 99 MP upload is rejected at admission with a readable reason', async ({ guest, db }) => { + test.setTimeout(60_000); const g = await guest('BombThrower'); + const before = await db.countUploadsForUser(g.userId); - // The upload itself is accepted — 568 KiB is well within the body cap. The rejection - // happens in the compression worker, where the decode budget lives. const res = await uploadRaw(g.jwt, readFileSync(HUGE), { filename: 'huge.jpg', contentType: 'image/jpeg', caption: 'zu gross', }); - expect(res.status, 'a 568 KiB file is a legitimate upload').toBe(201); - const { id } = (await res.json()) as { id: string }; - // It must land in 'failed', not 'done' — and must get there, rather than the container - // dying mid-decode and leaving it stuck in 'processing' forever. - await expect - .poll(() => db.compressionStatus(id), { timeout: 60_000, intervals: [500] }) - .toBe('failed'); + // 4xx, not 201-then-vanish. The queue classifies this as terminal, so the guest gets the + // message rather than watching the photo disappear. + expect(res.status, 'an undecodable image must be refused at the door').toBe(400); + const body = (await res.json()) as { message?: string }; + expect(body.message ?? '', 'the reason must be actionable, not generic').toMatch(/bildpunkte/i); + expect(body.message ?? '', 'and should name the size so it is obvious why').toMatch(/99/); - // The whole point: the process is still alive. An OOM kill would have taken the backend - // down here, and Docker would have restarted it. - const health = await fetch(`${BASE}/health`); - expect(health.status, 'the backend must have survived the oversized decode').toBe(200); + // Nothing was stored — no row to soft-delete later, no orphaned file to sweep. + expect(await db.countUploadsForUser(g.userId)).toBe(before); - // And it is still doing useful work afterwards — not wedged or restarting. + // The backend never allocated: it is still alive and still doing useful work. + expect((await fetch(`${BASE}/health`)).status).toBe(200); const ok = await uploadRaw(g.jwt, readFileSync(SAMPLE), { filename: 'after.jpg', contentType: 'image/jpeg', @@ -68,27 +64,37 @@ test.describe('Upload — an oversized image is refused, not allocated', () => { await expect.poll(() => db.compressionStatus(after.id), { timeout: 30_000 }).toBe('done'); }); - test('two oversized uploads at once still leave the container alive', async ({ guest, db }) => { - // The concurrent case is the one that actually OOM'd: `compression_concurrency` is 2, so - // two decodes overlap. Under the old behaviour this pair peaked near the container cap. - test.setTimeout(90_000); + test('a burst of oversized uploads leaves the container alive', async ({ guest }) => { + // The concurrent case is the one that OOM'd: `compression_concurrency` is 2, so decodes + // overlapped. Four at once is comfortably past that, and must still cost only header + // reads. + test.setTimeout(60_000); const g = await guest('BombThrower2'); const bytes = readFileSync(HUGE); - const [a, b] = await Promise.all([ - uploadRaw(g.jwt, bytes, { filename: 'huge-a.jpg', contentType: 'image/jpeg' }), - uploadRaw(g.jwt, bytes, { filename: 'huge-b.jpg', contentType: 'image/jpeg' }), - ]); - expect([a.status, b.status]).toEqual([201, 201]); - const ids = [((await a.json()) as { id: string }).id, ((await b.json()) as { id: string }).id]; + const results = await Promise.all( + Array.from({ length: 4 }, (_, i) => + uploadRaw(g.jwt, bytes, { filename: `huge-${i}.jpg`, contentType: 'image/jpeg' }) + ) + ); + expect(results.map((r) => r.status)).toEqual([400, 400, 400, 400]); - for (const id of ids) { - await expect - .poll(() => db.compressionStatus(id), { timeout: 60_000, intervals: [500] }) - .toBe('failed'); - } + expect( + (await fetch(`${BASE}/health`)).status, + 'concurrent oversized uploads must not kill the backend' + ).toBe(200); + }); - const health = await fetch(`${BASE}/health`); - expect(health.status, 'two concurrent oversized decodes must not kill the backend').toBe(200); + test('an ordinary photo is unaffected by the admission check', async ({ guest, db }) => { + // The mirror that keeps the check honest: a budget that rejected everything would pass + // both tests above. + const g = await guest('NormalShooter'); + const res = await uploadRaw(g.jwt, readFileSync(SAMPLE), { + filename: 'normal.jpg', + contentType: 'image/jpeg', + }); + expect(res.status).toBe(201); + const { id } = (await res.json()) as { id: string }; + await expect.poll(() => db.compressionStatus(id), { timeout: 30_000 }).toBe('done'); }); });