Fourth round: reject at the door what the worker can only fail on, and stop retrying errors that cannot succeed — while keeping IoError retryable so ENOSPC still gets another attempt. Squashed from 2 commits, original messages preserved below. ──────── fix(upload): refuse undecodable images at the door, and stop retrying them Two halves of the same complaint: an oversized photo was accepted with a 201 and then silently soft-deleted minutes later, after the worker had burned six seconds of backoff re-reaching a conclusion it could not change. Admission. The compression budget now runs at upload time, against the header only, so a guest is told immediately and told why: "Bild hat zu viele Bildpunkte (ca. 99 Megapixel) und kann nicht verarbeitet werden. Bitte verkleinere es und lade es erneut hoch." instead of watching the photo vanish behind a vague "could not be processed" — which arrived only if they happened to still be on the feed with that card loaded. Nothing is stored, so there is no row to soft-delete and no orphan for the sweep to reclaim. Admission and the worker share ONE function (`decoder_within_budget`), so they cannot drift apart and start disagreeing about what is acceptable — a photo accepted at the door and rejected by the worker would be worse than either behaviour alone. The worker keeps its own check: the backfill decodes files that predate this check, and defence in depth is the whole reason the budget exists. Retries. The loop retried every failure, including ones that are a property of the input. An image over the budget, a corrupt file, an unsupported format: each fails identically on all three attempts, so the only effect was 2s + 4s of sleep and three near-identical warnings before the same outcome. `is_permanent_image_error` classifies the `ImageError` variants that cannot change between attempts — Limits, Unsupported, Decoding — and the loop gives up on those at once. `IoError` is deliberately excluded: an ENOSPC while writing a derivative is exactly the transient case the retry exists for, and misclassifying it would turn a blip back into the data loss round 1 fixed. Measured: retry log lines went from 3 per oversized upload to 0. Tests: unit tests for both sides of the classifier (a Limits error is permanent, a missing file is not) and for admission agreeing with the decoder on accept AND reject. The e2e spec is rewritten for the new contract — 400 with an actionable message, nothing stored, backend alive after a burst of four — plus a mirror asserting an ordinary photo still uploads and processes, since a budget that rejected everything would satisfy the other two. ──────── 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 <noreply@anthropic.com>
101 lines
4.7 KiB
TypeScript
101 lines
4.7 KiB
TypeScript
/**
|
|
* 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.
|
|
*
|
|
* Two defects met here.
|
|
*
|
|
* 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.
|
|
*
|
|
* 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. 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';
|
|
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 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);
|
|
|
|
const res = await uploadRaw(g.jwt, readFileSync(HUGE), {
|
|
filename: 'huge.jpg',
|
|
contentType: 'image/jpeg',
|
|
caption: 'zu gross',
|
|
});
|
|
|
|
// 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/);
|
|
|
|
// Nothing was stored — no row to soft-delete later, no orphaned file to sweep.
|
|
expect(await db.countUploadsForUser(g.userId)).toBe(before);
|
|
|
|
// 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',
|
|
});
|
|
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('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 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]);
|
|
|
|
expect(
|
|
(await fetch(`${BASE}/health`)).status,
|
|
'concurrent oversized uploads 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');
|
|
});
|
|
});
|