fix(upload): narrow the admission check to the memory budget only

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>
This commit is contained in:
fabi
2026-07-29 07:55:56 +02:00
parent 674ea87bbd
commit ceb68939a7
2 changed files with 57 additions and 17 deletions

View File

@@ -239,12 +239,10 @@ pub async fn upload(
// vanish with, at best, a vague "could not be processed". Rejecting here gives them a // 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 // reason at the door that they can act on, and it uses the SAME budget the worker
// enforces, so admission and processing cannot disagree. // enforces, so admission and processing cannot disagree.
if mime.starts_with("image/") if mime.starts_with("image/") && crate::services::imaging::exceeds_decode_budget(&temp_abs) {
&& let Err(e) = crate::services::imaging::probe_decodable(&temp_abs)
{
let mp = crate::services::imaging::megapixels(&temp_abs); let mp = crate::services::imaging::megapixels(&temp_abs);
tracing::info!( tracing::info!(
error = ?e, %mime, megapixels = ?mp, %mime, megapixels = ?mp,
"rejecting an image that exceeds the decode budget at admission" "rejecting an image that exceeds the decode budget at admission"
); );
let _ = tokio::fs::remove_file(&temp_abs).await; let _ = tokio::fs::remove_file(&temp_abs).await;

View File

@@ -99,13 +99,29 @@ pub fn megapixels(path: &Path) -> Option<f64> {
Some(f64::from(w) * f64::from(h) / 1_000_000.0) 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 /// Called at upload admission so a guest who sends a 100 MP photo is told at the door, with
/// on, instead of the upload being accepted with a 201 and then silently soft-deleted /// a reason they can act on, instead of the upload being accepted with a 201 and then
/// minutes later when the worker gives up on it. /// silently soft-deleted minutes later when the worker gives up on it.
pub fn probe_decodable(path: &Path) -> Result<()> { ///
decoder_within_budget(path).map(|_| ()) /// 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 /// Decode an image from disk with decompression-bomb limits applied and its EXIF
@@ -186,23 +202,49 @@ mod tests {
} }
#[test] #[test]
fn probe_agrees_with_the_decoder_on_both_sides() { fn admission_rejects_only_the_over_budget_case() {
// Admission and processing must never disagree — a photo accepted at the door and // Admission and processing must agree about SIZE — a photo accepted at the door and
// then rejected by the worker is the exact failure this pair exists to prevent. // then rejected by the worker for being too big is the failure this pair prevents.
assert!( assert!(
probe_decodable(Path::new(HUGE)).is_err(), exceeds_decode_budget(Path::new(HUGE)),
"probe must reject what the decoder rejects" "admission must reject what the decoder rejects for size"
); );
let ordinary = concat!( let ordinary = concat!(
env!("CARGO_MANIFEST_DIR"), env!("CARGO_MANIFEST_DIR"),
"/../e2e/fixtures/media/portrait-exif6.jpg" "/../e2e/fixtures/media/portrait-exif6.jpg"
); );
assert!( assert!(
probe_decodable(Path::new(ordinary)).is_ok(), !exceeds_decode_budget(Path::new(ordinary)),
"probe must accept what the decoder accepts" "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] #[test]
fn still_decodes_an_ordinary_photo_and_applies_orientation() { fn still_decodes_an_ordinary_photo_and_applies_orientation() {
// The guard must not have become a blanket refusal. This fixture is 40x20 stored with // The guard must not have become a blanket refusal. This fixture is 40x20 stored with