Skip to content

fix(#479): bound what a cover decode may allocate, and what a download may buffer - #486

Merged
guycorbaz merged 1 commit into
mainfrom
fix/479-bound-image-decoding
Sep 10, 2026
Merged

guycorbaz merged 1 commit into
mainfrom
fix/479-bound-image-decoding

Conversation

@guycorbaz

Copy link
Copy Markdown
Owner

Closes #479.

The hole

CoverService::process_and_save_bytes decoded with whatever limits the image crate defaults to — 512 MiB. The 10 MiB guard beside it bounds the compressed input, which says nothing about memory: a redundant PNG or WebP of a couple of hundred bytes can declare an RGBA surface of several gigabytes. On the Docker deployment the OOM killer takes the container down, and every in-flight request with it.

Both entry points funnel through this one function: the manual upload (Librarian+, so realistically a mistake or a stolen session) and the provider download, which is the one that matters — those bytes come from Open Library, BnF Couvertures, Inventaire or Google Books, no authentication is involved, and the bulk cover-refetch walks the whole catalogue in a row.

The change

Decode budget. 64 MiB of allocation plus a strict 10 000 px per-side cap. ImageReader::decode computes the output size from the declared dimensions and checks it before allocating, for every format, so a bomb is refused rather than swallowed. 64 MiB is ~16 megapixels of RGBA: a 4032×3024 phone photo still passes, and everything here ends up resized to 400 px wide anyway. The issue suggested measuring against the largest legitimate cover in production; covers are stored already resized, so the original dimensions aren't recorded anywhere — the ceiling is set from what a camera produces instead, with the dimension cap as the cheap early rejection.

Its own error message. The crate says "Memory limit exceeded", which reads like a server fault. From where the librarian sits the file is simply too big to process, and the message now says so and what to do about it. English like its sibling "Image too large: N bytes" — the handler prefixes both with the localized copy.

One layer up, the same shape of hole (beyond the issue's text, flagged here so it is easy to challenge): response.bytes() buffered the entire body after checking Content-Length. That header is the server's claim, not a fact — absent under chunked transfer, and free to lie. A provider that streams without announcing a length could make us buffer indefinitely, which is the same failure the issue is about with one fewer step. The body is now read chunk by chunk and abandoned the moment it crosses 10 MiB.

Coverage

Unit tests in src/services/cover.rs:

  • a hand-built PNG that declares 8000×8000 in 200-odd bytes → refused by the allocation budget (this is the issue's crafted-input test);
  • one declaring 40 000 px on a side → refused by the strict dimension cap, from the header;
  • a 2000×3000 cover → still accepted, so the budget doesn't cost real covers;
  • a chunked, never-ending response served from a local TcpListener → the download is cut off with the size-cap error (no HTTP-mock dependency added; tokio was already there).

E2E (tests/e2e/specs/journeys/cover-upload-limits.spec.ts): a librarian uploads the bomb through the real modal, sees the refusal in the modal's error region, and /health still answers — which is the whole point of the issue.

Testing

  • cargo clippy --all-targets -- -D warnings — clean.
  • cargo test --lib — 1175 passed.
  • cargo test --tests --no-fail-fast — no failures.
  • CI=true npx playwright test --workers=2 on a freshly reset stack — 333 passed, 2 skipped, 0 failed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01D5cV9QRqQAFpiNKZwZfoNX

…d may buffer

`process_and_save_bytes` decoded with whatever limits the `image` crate
defaults to — 512 MiB. The 10 MiB guard next to it bounds the
*compressed* input, which says nothing about memory: a redundant PNG or
WebP of a couple of hundred bytes can declare an RGBA surface of several
gigabytes. On the Docker deployment the OOM killer takes the container
down and every in-flight request with it.

Both entry points funnel through this function — the manual upload
(Librarian+, so a mistake or a stolen session) and the provider download,
which is the one that matters: those bytes come from Open Library, BnF,
Inventaire or Google Books, nobody authenticates, and the bulk refetch
walks the whole catalogue in a row.

The decode now runs under an explicit budget: 64 MiB of allocation
(~16 megapixels of RGBA — a 4032x3024 phone photo still passes, and
everything ends up resized to 400 px wide anyway) plus a strict 10 000 px
per-side cap. `ImageReader::decode` sizes the output from the declared
dimensions and checks it before allocating, for every format, so a bomb
is refused rather than swallowed. A limit failure gets its own message:
the crate's "Memory limit exceeded" reads like a server fault, when from
where the librarian sits the file is simply too big to process.

The download side had the same shape of hole one layer up:
`response.bytes()` buffered the whole body after checking
`Content-Length`, which is the server's claim, not a fact — absent under
chunked transfer, and free to lie. The body is now read chunk by chunk
and abandoned the moment it crosses 10 MiB.

Tests: a hand-built PNG that declares 8000x8000 in 200-odd bytes is
refused by the allocation budget, one declaring 40 000 px per side by the
dimension cap, a 2000x3000 cover still goes through, and a chunked
never-ending response served from a local socket is cut off. The E2E
journey uploads the bomb through the modal and asserts the librarian sees
the refusal — and that /health still answers, which is the whole point.

Refs #479

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D5cV9QRqQAFpiNKZwZfoNX
@guycorbaz
guycorbaz marked this pull request as ready for review September 10, 2026 23:23
@guycorbaz
guycorbaz merged commit 90cc95b into main Sep 10, 2026
8 checks passed
@guycorbaz
guycorbaz deleted the fix/479-bound-image-decoding branch September 10, 2026 23:23
guycorbaz added a commit that referenced this pull request Sep 11, 2026
Cut the v1.19.0 release documentation: version bump in Cargo.toml /
Cargo.lock, and every version-bearing surface required by Foundation
Rule 19 brought to 1.19.0.

- Manual EN/FR: title-page version, install snippets, a "What's new in
  1.19.0" section in chapter 8 (the Restore button that never worked,
  the seeded accounts leaving no trace, the bounded cover decode, and
  an explicit "nothing to do on upgrade"); the release-notes section
  now states that the PDFs are attached to the release rather than
  promising it once CI is wired. PDFs rebuilt.
- README: status line, live-install label, image-size badge, current-
  release paragraph.
- ROADMAP: current-stable header, a v1.19.0 shipped section, and the
  "merged but not released" block retired now that it has shipped.
- docs/dockerhub-overview.md: tags list.
- website/: index (nav badge, hero, JSON-LD softwareVersion, the hero
  paragraph rewritten around this release), about (nav badge), roadmap
  (meta descriptions, JSON-LD, nav badge, both narrative paragraphs),
  sitemap lastmod — stale since May, and the surface Rule 19 names as
  the easiest to forget.
- sprint-status.yaml: last_updated header.

No source change. The release carries #478, #480 and #479 (merged in
#485, #484 and #486), the supply-chain CI work (#481), the network-
posture documentation (#482), the community-health files (#477) and the
documentation audit (#487). No migration.


Claude-Session: https://claude.ai/code/session_01D5cV9QRqQAFpiNKZwZfoNX

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CR] Bound image decoding so a compressed cover cannot exhaust memory

1 participant