ceb68939a7
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) <noreply@anthropic.com>
264 lines
12 KiB
Rust
264 lines
12 KiB
Rust
//! Shared image decoding.
|
||
//!
|
||
//! Exists so there is exactly ONE way to turn a file on disk into a `DynamicImage` in this
|
||
//! codebase. Two properties have to hold everywhere an image is decoded, and both were
|
||
//! previously re-derived per call site — which is how they drifted apart:
|
||
//!
|
||
//! - **EXIF orientation must be applied.** Phones do not rotate sensor data; they record how
|
||
//! the camera was held in a tag and store the pixels as shot. `image::open` and
|
||
//! `ImageReader::decode` both hand back the raw pixels and ignore that tag, and re-encoding
|
||
//! to JPEG writes no EXIF, so the derivative is permanently sideways while the untouched
|
||
//! original still renders upright. The compression worker was fixed; the export worker was
|
||
//! not, so every portrait photo came out sideways in the keepsake's HTML viewer.
|
||
//! - **Decode limits must be set.** The upload body cap bounds the file on disk, but a small
|
||
//! file can decode to enormous dimensions (a ~1 MB image expanding to 50k×50k px), OOM-ing
|
||
//! the box. `image::open` applies NO limits at all, so the export path was also decoding
|
||
//! arbitrary user-supplied images unbounded.
|
||
|
||
use anyhow::{Context, Result};
|
||
use image::{DynamicImage, ImageDecoder};
|
||
use std::path::Path;
|
||
|
||
/// 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);
|
||
limits.max_image_height = Some(12_000);
|
||
limits.max_alloc = Some(256 * 1024 * 1024);
|
||
limits
|
||
}
|
||
|
||
/// 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.
|
||
///
|
||
/// 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::<image::ImageError>(),
|
||
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<impl image::ImageDecoder> {
|
||
let mut reader = image::ImageReader::open(path)
|
||
.context("failed to open image")?
|
||
.with_guessed_format()
|
||
.context("failed to read image header")?;
|
||
let mut limits = decode_limits();
|
||
reader.limits(limits.clone());
|
||
|
||
// 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")?;
|
||
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<f64> {
|
||
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::<image::ImageError>(),
|
||
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<DynamicImage> {
|
||
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.
|
||
let orientation = decoder
|
||
.orientation()
|
||
.unwrap_or(image::metadata::Orientation::NoTransforms);
|
||
let mut img = DynamicImage::from_decoder(decoder).context("failed to decode image")?;
|
||
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 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
|
||
// 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"
|
||
);
|
||
}
|
||
}
|