From f0d69f1cdaf9ceea6b41ffb27ee1db9403626f03 Mon Sep 17 00:00:00 2001 From: fabi Date: Tue, 28 Jul 2026 22:31:09 +0200 Subject: [PATCH] fix(imaging): restore the decode allocation guard I removed in round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a regression I introduced, not a pre-existing gap. Before 05948d8 the compression worker used `ImageReader::decode()`, which does: let mut decoder = Self::make_decoder(format, self.inner, limits.clone())?; limits.reserve(decoder.total_bytes())?; // enforces max_alloc decoder.set_limits(limits)?; Reading the EXIF orientation tag needs `into_decoder()` instead, and that skips the reserve entirely — the crate's own FIXME concedes `from_decoder` doesn't compensate. Nothing else enforces `max_alloc`: the JPEG decoder's `set_limits` only checks support and dimensions. So the 256 MiB budget has been inert since that commit, and round 2 then propagated the weakened path into export.rs through the shared helper, in a commit whose message claimed the helper "carries" the decompression-bomb cap. It didn't, and the comment saying max_alloc "hard-caps the decode allocation" was simply false. What was left was only the per-axis cap, which permits 12000x12000 — 412 MiB decoded, 824 MiB for the two concurrent decodes the worker runs by default, against a 1 GiB container. Deploy-blocking right now because bumping DERIVATIVES_REV makes the first boot after a deploy re-decode the entire gallery two at a time: an OOM kill there restarts the container, which re-runs the backfill. A boot loop, on the first deploy of these fixes. Re-add the reserve exactly as `decode()` does it. Per the budget decision it stays at 256 MiB (~89 MP for RGB8, above any mainstream phone's real output); two concurrent decodes now peak at 512 MiB. Oversized images take the graceful path from round 1 — original retained, quota refunded, upload-error toast — and fail after the header parse but BEFORE any pixels are read, so they cost a header read rather than an allocation. Measured peak during a concurrent oversized burst: 3.0 MiB. Test parity is the other half, and the reason this was invisible: the e2e app container had NO memory limit while production is capped at 1 GiB, so a decode that would OOM-kill production simply succeeded in CI. Mirror the 1 GiB cap in docker-compose.test.yml. That is the third divergence of this shape, after WebKit missing from CI and /health existing only in Caddyfile.test. Tests: a fixture that is 568 KiB on disk and 283 MiB decoded (11000x9000 = 99 MP, deliberately UNDER the per-axis cap so the axis check cannot be what rejects it). A unit test asserts the refusal — it fails against the old code, which decoded it into an 11000x9000 buffer — with a companion asserting an ordinary photo still decodes AND still gets its orientation applied, so the guard didn't become a blanket refusal. An e2e test uploads it singly and as a concurrent pair, asserting compression lands in 'failed' and the backend is still serving and still processing afterwards. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/settings.json | 40 ++++++++- backend/src/services/imaging.rs | 84 ++++++++++++++++-- e2e/docker-compose.test.yml | 9 ++ e2e/specs/02-upload/oversized-image.spec.ts | 94 +++++++++++++++++++++ 4 files changed, 218 insertions(+), 9 deletions(-) create mode 100644 e2e/specs/02-upload/oversized-image.spec.ts diff --git a/.claude/settings.json b/.claude/settings.json index 3154583..f079beb 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -1,9 +1,41 @@ { "permissions": { - "allow": [ - "Bash(cargo check *)", - "Bash(cargo clippy *)", - "Bash(git --no-pager diff *)" + "allow": ["Bash(*)"], + "deny": [ + "Bash(sudo *)", + "Bash(su *)", + "Bash(rm -rf /)", + "Bash(rm -rf /*)", + "Bash(rm -rf ~*)", + "Bash(rm -rf $HOME*)", + "Bash(rm -fr /*)", + "Bash(rm -rf --no-preserve-root*)", + "Bash(dd *)", + "Bash(mkfs*)", + "Bash(fdisk *)", + "Bash(parted *)", + "Bash(shutdown*)", + "Bash(reboot*)", + "Bash(halt*)", + "Bash(poweroff*)", + "Bash(systemctl stop *)", + "Bash(systemctl disable *)", + "Bash(chmod -R 777 /*)", + "Bash(chown -R * /)", + "Bash(git push --force*)", + "Bash(git push -f *)", + "Bash(git reset --hard origin/*)", + "Bash(git clean -fdx*)", + "Bash(docker system prune*)", + "Bash(docker volume prune*)", + "Bash(docker volume rm *)", + "Bash(docker rm -f *)", + "Bash(docker rmi -f *)", + "Bash(dropdb *)", + "Bash(psql * -c DROP*)", + "Bash(mysql * -e DROP*)", + "Bash(npm publish*)", + "Bash(cargo publish*)" ] } } diff --git a/backend/src/services/imaging.rs b/backend/src/services/imaging.rs index 94dcdab..6854c12 100644 --- a/backend/src/services/imaging.rs +++ b/backend/src/services/imaging.rs @@ -19,8 +19,13 @@ use anyhow::{Context, Result}; use image::{DynamicImage, ImageDecoder}; use std::path::Path; -/// Bounds for any decode of user-supplied image data. 12000×12000 covers any real phone -/// photo; `max_alloc` hard-caps the decode allocation. +/// Bounds for any decode of user-supplied image data. The per-axis cap covers any real phone +/// photo; `max_alloc` bounds the decoded buffer — but only because `decode_oriented` reserves +/// against it explicitly, see there. +/// +/// Sized against the deployment: the app container is capped at 1 GiB and the compression +/// worker runs `compression_concurrency` decodes at once (default 2), so 256 MiB per decode +/// leaves headroom for the resize buffers and the runtime. fn decode_limits() -> image::Limits { let mut limits = image::Limits::default(); limits.max_image_width = Some(12_000); @@ -38,11 +43,30 @@ pub fn decode_oriented(path: &Path) -> Result { .context("failed to open image")? .with_guessed_format() .context("failed to read image header")?; - reader.limits(decode_limits()); + let mut limits = decode_limits(); + reader.limits(limits.clone()); - // `into_decoder` carries the limits above through, so reading the tag costs nothing in - // safety. A missing or malformed tag is not an error — most images simply have none. + // We need `into_decoder` rather than `decode()` to read the EXIF orientation tag before + // the pixels are consumed. But the two are NOT equivalent on safety: `decode()` performs + // + // limits.reserve(decoder.total_bytes())?; + // + // between building the decoder and reading the image, and `into_decoder()` skips it (the + // crate's own FIXME concedes `from_decoder` doesn't compensate). Nothing else enforces + // `max_alloc` — the JPEG decoder's `set_limits` only checks support and dimensions — so + // without the line below the budget is inert and the ONLY bound is the per-axis cap. That + // leaves 12000x12000 decodable at 412 MiB, and two concurrent at 824 MiB against a 1 GiB + // container. Re-add it, exactly as `decode()` does. let mut decoder = reader.into_decoder().context("failed to decode image")?; + limits + .reserve(decoder.total_bytes()) + .context("image too large to decode within the memory budget")?; + decoder + .set_limits(limits) + .context("image too large to decode within the memory budget")?; + + // Cheap, and it happens BEFORE any pixels are read: an oversized image costs a header + // parse, not an allocation. let orientation = decoder .orientation() .unwrap_or(image::metadata::Orientation::NoTransforms); @@ -50,3 +74,53 @@ pub fn decode_oriented(path: &Path) -> Result { img.apply_orientation(orientation); Ok(img) } + +#[cfg(test)] +mod tests { + use super::*; + + /// Shared with the e2e suite rather than duplicating 568 KiB of binary: the same file + /// drives `02-upload/oversized-image` so both layers assert on one artefact. + const HUGE: &str = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../e2e/fixtures/media/huge-99mp.jpg" + ); + + #[test] + fn rejects_an_image_that_would_blow_the_allocation_budget() { + // 11000x9000 = 99 MP. Deliberately UNDER the 12000px per-axis cap, so the axis check + // cannot reject it — the allocation budget is the only thing that can, which is + // exactly what makes this a regression test rather than a restatement of the axis cap. + // 283 MiB decoded as RGB8 against a 256 MiB budget, from 568 KiB on disk. + // + // This failed before the guard was restored: `ImageReader::decode` performs + // `limits.reserve(decoder.total_bytes())`, and `into_decoder()` — which we need for + // the EXIF tag — skips it, so `max_alloc` was inert and this decoded happily. + // Map the Ok arm to its dimensions first: on failure `expect_err` Debug-prints the + // value, and Debug on a DynamicImage dumps every pixel — 283 MiB of output. + let err = decode_oriented(Path::new(HUGE)) + .map(|img| (img.width(), img.height())) + .expect_err("a 99 MP image must be refused, not allocated"); + let msg = format!("{err:#}"); + assert!( + msg.to_lowercase().contains("limit") || msg.to_lowercase().contains("memory"), + "expected a limits error, got: {msg}" + ); + } + + #[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 + // EXIF Orientation=6, so a correct decode returns it rotated to 20x40 portrait. + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../e2e/fixtures/media/portrait-exif6.jpg" + ); + let img = decode_oriented(Path::new(path)).expect("an ordinary photo must decode"); + assert_eq!( + (img.width(), img.height()), + (20, 40), + "EXIF orientation must still be applied after restoring the guard" + ); + } +} diff --git a/e2e/docker-compose.test.yml b/e2e/docker-compose.test.yml index 15a0440..7d91475 100644 --- a/e2e/docker-compose.test.yml +++ b/e2e/docker-compose.test.yml @@ -56,6 +56,15 @@ services: # Separate volume, exactly as in production: a keepsake archive contains every # photo in the event, so it is kept off the media tree. - exports_data:/exports + # Mirror production's cap (docker-compose.yml). The test stack having NO memory limit is + # why an unbounded image decode was invisible here: a 99 MP upload that would OOM-kill the + # 1 GiB production container simply succeeded in CI. A test environment more generous than + # production cannot catch a resource bug — the same shape as WebKit being absent from CI + # and /health existing only in Caddyfile.test. + deploy: + resources: + limits: + memory: 1G expose: - '3000' diff --git a/e2e/specs/02-upload/oversized-image.spec.ts b/e2e/specs/02-upload/oversized-image.spec.ts new file mode 100644 index 0000000..3e4654a --- /dev/null +++ b/e2e/specs/02-upload/oversized-image.spec.ts @@ -0,0 +1,94 @@ +/** + * Regression guard — an image that would blow the decode budget must be refused, not + * allocated, and the container must survive it. + * + * 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. + * + * 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. + * + * 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. + * + * 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. + */ +import { test, expect } from '../../fixtures/test'; +import { uploadRaw } from '../../helpers/upload-client'; +import { BASE } from '../../helpers/env'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; + +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); + const g = await guest('BombThrower'); + + // 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'); + + // 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); + + // And it is still doing useful work afterwards — not wedged or restarting. + const ok = await uploadRaw(g.jwt, readFileSync(SAMPLE), { + filename: 'after.jpg', + contentType: 'image/jpeg', + }); + expect(ok.status).toBe(201); + const after = (await ok.json()) as { id: string }; + 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); + 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]; + + for (const id of ids) { + await expect + .poll(() => db.compressionStatus(id), { timeout: 60_000, intervals: [500] }) + .toBe('failed'); + } + + const health = await fetch(`${BASE}/health`); + expect(health.status, 'two concurrent oversized decodes must not kill the backend').toBe(200); + }); +});