refactor(uploads): centralize image decode behind safe helpers + grep-ban (LC-206) #272

Merged

longjacksonle merged 1 commit from refactor/lc-206-image-decode-safe-helper into main 2026-05-30 03:09:06 +02:00

Conversation 0 Commits 1 Files changed 8 +426 -62

longjacksonle commented 2026-05-30 03:08:14 +02:00

Owner

Copy link

Framing: spike + audit clarified the failure mode

LC-206 was filed as "uploads run the decoder without a catch_unwind boundary while the avatar path runs with one." Verifying that premise first (the LC-205 discipline) found it wrong about WHY, not about whether the work should happen. All four decoder call sites already sat inside tokio::task::spawn_blocking, and panic=unwind is confirmed in both Cargo profiles, so tokio's JoinError::is_panic() already caught every decoder panic today. There was never a missing panic boundary and never a live exploit. The asymmetry instinct was correct; the threat model attached to it (live panic exposure) was not. The work is the same; the framing shifts from urgency to hygiene. Real wins retained: retire four copies of the same spawn_blocking + JoinError boilerplate, prevent a future un-spawn_blocking'd fifth caller structurally, and close the validation gap (the decoder was only ever spike-validated at the 1 MiB avatar cap, never the 10 MiB upload cap).

10 MiB spike result (the gate) - clean, Outcome A

Re-ran the LC-78-AVATAR-PROXY hostile corpus scaled to the upload path's 10 MiB ceiling, isolated in catch_unwind, not through any live route.

Input shape	Size	10 MiB result	Panic
Random bytes, no format	10 MiB	clean Err	none
PNG sig + garbage	10 MiB	clean Err	none
JPEG SOI + garbage	10 MiB	clean Err	none
JPEG SOI + 0xFF run	10 MiB	clean Err	none
GIF header + garbage	10 MiB	clean Err	none
WebP lying RIFF size + garbage	10 MiB	clean Err	none
Pixel-bomb IHDR 65535x65535	tiny	clean Err	none
JPEG SOF0 65500x65500 truncated	tiny	clean Err	none
Large valid 3000x3000 RGBA PNG	36 MB decoded	DECODED ok	none

Zero panics. Promoted into spike_image_decoder_hostile_corpus.rs so the validation is permanent. The large-valid row carries a caveat comment: it tests allocation-at-scale (36 MB decode buffer), not compression ratio; the high-ratio decompression-bomb vector is covered by the pixel-bomb row, which image 0.25 rejects at header-read before allocating. Without that comment a future reader could misread "large valid PNG worked" as "decompression bombs are fine."

Change

Two thin public async entry points over one shared internal panic boundary (LC-152 outbound_get/outbound_post shape):

• process_image_safely(path, mime) -> ProcessedImage and process_preview_safely(path, mime) -> Vec<u8> share run_blocking, which folds a JoinError into SafeImageError::Panic and a normal failure into SafeImageError::Pipeline.
• Wraps the whole pipeline (decode + re-encode), not decode-only: re-encode is also CPU work that belongs off the async runtime.
• All four callers migrated, behavior preserved exactly: uploads 400-on-decode / 500-else; attachments fold to one ImagePipeline drop; bridge-avatar keeps the decode-vs-panic log distinction; admin regen warns-and-continues on a pipeline error and propagates 500 on a panic (the old ?-on-JoinError). The thumbnail semaphore stays at the call sites that acquired it (bridge-avatar deliberately does not), so throttling is unchanged.

Grep-ban (lc206_no_raw_image_decode_in_src.rs, LC-152 shape)

Raw decoder entry points (ImageReader::open/new, GifDecoder::new, image::open/load, image::io::Reader) and the run_blocking panic-boundary seam are forbidden in src/ outside the one full-path allow-list entry src/uploads/pipeline.rs. run_blocking is banned exactly as hard as the raw entry points (same posture LC-152 takes with outbound_unchecked): a #[doc(hidden)] seam is still a bypass if src/ can call it, since a direct caller would replicate the boilerplate the helpers exist to retire. The sync process_image / preview_from_path stay pub as the pure-pipeline unit-test seam; visibility is not the enforcement, the grep-ban is. A meta-test asserts pipeline.rs still contains a banned token so the exception cannot silently go dead.

Killer test pair (image_decode_safe_boundary.rs)

A synthetic panic through run_blocking maps to SafeImageError::Panic (proves the boundary catches regardless of source, durable across an image-crate bump); a malformed image maps to SafeImageError::Pipeline(Decode), NOT Panic (proves the two failure modes stay distinguishable, which the bridge-avatar logging relies on). Both assert the specific variant, never bare is_err().

Out of scope (confirmed)

No upstream image-rs report (nothing panicked). No security-release question (no shipped live vector - the premise correction makes this clear). No image::* broad ban, no decode-only boundary, no helper expansion to resize/convert. ProcessedImage stays the common struct.

Test plan

just check clean both modes. Full suite green both modes: standalone and saas, 128 binaries each, zero failures. No pre-existing failures observed in either run (the lc77_webhook_render_fixture golden-fixture failures and the routes_uploads concurrent-load flake noted in prior PRs did not reproduce here; neither is touched by this change).

🤖 Generated with Claude Code

## Framing: spike + audit clarified the failure mode LC-206 was filed as "uploads run the decoder without a `catch_unwind` boundary while the avatar path runs with one." Verifying that premise first (the LC-205 discipline) found it wrong about WHY, not about whether the work should happen. All four decoder call sites already sat inside `tokio::task::spawn_blocking`, and `panic=unwind` is confirmed in both Cargo profiles, so tokio's `JoinError::is_panic()` already caught every decoder panic today. There was never a missing panic boundary and never a live exploit. The asymmetry instinct was correct; the threat model attached to it (live panic exposure) was not. The work is the same; the framing shifts from urgency to hygiene. Real wins retained: retire four copies of the same spawn_blocking + JoinError boilerplate, prevent a future un-`spawn_blocking`'d fifth caller structurally, and close the validation gap (the decoder was only ever spike-validated at the 1 MiB avatar cap, never the 10 MiB upload cap). ## 10 MiB spike result (the gate) - clean, Outcome A Re-ran the LC-78-AVATAR-PROXY hostile corpus scaled to the upload path's 10 MiB ceiling, isolated in `catch_unwind`, not through any live route. | Input shape | Size | 10 MiB result | Panic | |---|---|---|---| | Random bytes, no format | 10 MiB | clean Err | none | | PNG sig + garbage | 10 MiB | clean Err | none | | JPEG SOI + garbage | 10 MiB | clean Err | none | | JPEG SOI + 0xFF run | 10 MiB | clean Err | none | | GIF header + garbage | 10 MiB | clean Err | none | | WebP lying RIFF size + garbage | 10 MiB | clean Err | none | | Pixel-bomb IHDR 65535x65535 | tiny | clean Err | none | | JPEG SOF0 65500x65500 truncated | tiny | clean Err | none | | Large valid 3000x3000 RGBA PNG | 36 MB decoded | DECODED ok | none | Zero panics. Promoted into `spike_image_decoder_hostile_corpus.rs` so the validation is permanent. The large-valid row carries a caveat comment: it tests allocation-at-scale (36 MB decode buffer), not compression ratio; the high-ratio decompression-bomb vector is covered by the pixel-bomb row, which `image 0.25` rejects at header-read before allocating. Without that comment a future reader could misread "large valid PNG worked" as "decompression bombs are fine." ## Change Two thin public async entry points over one shared internal panic boundary (LC-152 `outbound_get`/`outbound_post` shape): - `process_image_safely(path, mime) -> ProcessedImage` and `process_preview_safely(path, mime) -> Vec<u8>` share `run_blocking`, which folds a `JoinError` into `SafeImageError::Panic` and a normal failure into `SafeImageError::Pipeline`. - Wraps the whole pipeline (decode + re-encode), not decode-only: re-encode is also CPU work that belongs off the async runtime. - All four callers migrated, behavior preserved exactly: uploads 400-on-decode / 500-else; attachments fold to one `ImagePipeline` drop; bridge-avatar keeps the decode-vs-panic log distinction; admin regen warns-and-continues on a pipeline error and propagates 500 on a panic (the old `?`-on-JoinError). The thumbnail semaphore stays at the call sites that acquired it (bridge-avatar deliberately does not), so throttling is unchanged. ### Grep-ban (`lc206_no_raw_image_decode_in_src.rs`, LC-152 shape) Raw decoder entry points (`ImageReader::open/new`, `GifDecoder::new`, `image::open/load`, `image::io::Reader`) and the `run_blocking` panic-boundary seam are forbidden in `src/` outside the one full-path allow-list entry `src/uploads/pipeline.rs`. `run_blocking` is banned exactly as hard as the raw entry points (same posture LC-152 takes with `outbound_unchecked`): a `#[doc(hidden)]` seam is still a bypass if `src/` can call it, since a direct caller would replicate the boilerplate the helpers exist to retire. The sync `process_image` / `preview_from_path` stay `pub` as the pure-pipeline unit-test seam; visibility is not the enforcement, the grep-ban is. A meta-test asserts `pipeline.rs` still contains a banned token so the exception cannot silently go dead. ### Killer test pair (`image_decode_safe_boundary.rs`) A synthetic panic through `run_blocking` maps to `SafeImageError::Panic` (proves the boundary catches regardless of source, durable across an image-crate bump); a malformed image maps to `SafeImageError::Pipeline(Decode)`, NOT `Panic` (proves the two failure modes stay distinguishable, which the bridge-avatar logging relies on). Both assert the specific variant, never bare `is_err()`. ## Out of scope (confirmed) No upstream `image-rs` report (nothing panicked). No security-release question (no shipped live vector - the premise correction makes this clear). No `image::*` broad ban, no decode-only boundary, no helper expansion to resize/convert. `ProcessedImage` stays the common struct. ## Test plan `just check` clean both modes. Full suite green both modes: standalone and saas, 128 binaries each, zero failures. No pre-existing failures observed in either run (the `lc77_webhook_render_fixture` golden-fixture failures and the `routes_uploads` concurrent-load flake noted in prior PRs did not reproduce here; neither is touched by this change). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

longjacksonle added 1 commit 2026-05-30 03:08:15 +02:00

refactor(uploads): centralize image decode behind safe helpers + grep-ban (LC-206) ... b0b81ade3e

Spike + audit clarified the failure mode. LC-206 was filed as "uploads run the image decoder without a catch_unwind boundary while the avatar path runs with one." Verification found that premise wrong about WHY, not about whether the work should happen: all four decoder call sites (uploads, email-ingress attachments, bridge-avatar fetch, admin thumbnail regen) already sat inside `tokio::task::spawn_blocking`, and with `panic=unwind` confirmed in both Cargo profiles, tokio's `JoinError::is_panic()` already caught every decoder panic. So there was never a missing panic boundary or a live exploit; the asymmetry instinct was correct but the threat model attached to it (live panic exposure) was not. The real wins are dedup of four near-identical spawn_blocking blocks, structural prevention against a future un-spawn_blocking'd fifth caller, and closing the validation gap (the decoder was only ever spike-validated at the 1 MiB avatar cap, never the 10 MiB upload cap). Framing shifts from urgency to hygiene.

10 MiB spike: re-ran the LC-78-AVATAR-PROXY hostile corpus scaled to the upload path's 10 MiB ceiling (random bytes, per-format header + garbage, WebP lying-RIFF size, JPEG 0xFF run, JPEG SOF0 huge-dims truncated, pixel-bomb dimension overflow, large valid 3000x3000 PNG). All clean, zero panics. Promoted into `spike_image_decoder_hostile_corpus.rs` so the validation is permanent. The large-valid-PNG row carries a caveat comment: it tests allocation-at-scale (36 MB decode buffer), not compression ratio; the high-ratio decompression-bomb vector is covered by the pixel-bomb row, which image 0.25 rejects at header-read before allocating.

Helper shape mirrors LC-152's outbound_get/outbound_post over a shared internal: two thin public async entry points `process_image_safely(path, mime) -> ProcessedImage` and `process_preview_safely(path, mime) -> Vec<u8>` share one internal `run_blocking` panic boundary that folds a `JoinError` into `SafeImageError::Panic` and a normal failure into `SafeImageError::Pipeline`. The helper wraps the whole pipeline (decode + re-encode), not decode-only: re-encode is also CPU work that belongs off the async runtime. All four callers migrated; behavior preserved exactly (uploads 400-on-decode / 500-else, attachments fold to one drop, bridge-avatar decode-vs-panic log distinction, admin warn-and-continue on pipeline error / propagate-500 on panic). The thumbnail semaphore stays at the call sites that acquired it (bridge-avatar deliberately does not), so throttling is unchanged.

Grep-ban (`lc206_no_raw_image_decode_in_src.rs`, LC-152 shape): raw decoder entry points (ImageReader::open/new, GifDecoder::new, image::open/load, image::io::Reader) and the `run_blocking` panic-boundary seam are forbidden in src/ outside the single full-path allow-list entry `src/uploads/pipeline.rs`. run_blocking is banned exactly as hard as the raw entry points, the same posture LC-152 takes with outbound_unchecked: a #[doc(hidden)] seam is still a bypass if src/ can call it, because a direct caller would replicate the spawn_blocking boilerplate the helpers exist to retire. The sync process_image / preview_from_path stay pub as the pure-pipeline unit-test seam; visibility is not the enforcement, the grep-ban is. A meta-test asserts pipeline.rs still contains a banned token so the exception cannot silently go dead.

Killer test pair (`image_decode_safe_boundary.rs`): a synthetic panic through run_blocking maps to SafeImageError::Panic (proves the boundary catches regardless of source, durable across an image-crate bump), and a malformed image maps to SafeImageError::Pipeline(Decode), NOT Panic (proves the two failure modes stay distinguishable, which the bridge-avatar logging relies on). Both assert the specific variant, never bare is_err().

Out of scope, confirmed: no upstream image-rs report (nothing panicked), no security-release question (no shipped live vector), no `image::*` broad ban, no decode-only boundary, no helper expansion to resize/convert, ProcessedImage stays the common struct.

Validation: `just check` clean both modes; full suite green both modes (standalone and saas, 128 binaries each, zero failures).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

longjacksonle merged commit a8454a7f74 into main 2026-05-30 03:09:06 +02:00

longjacksonle deleted branch refactor/lc-206-image-decode-safe-helper 2026-05-30 03:09:06 +02:00

longjacksonle referenced this pull request from a commit 2026-05-30 03:09:08 +02:00

Merge pull request 'refactor(uploads): centralize image decode behind safe helpers + grep-ban (LC-206)' (#272) from refactor/lc-206-image-decode-safe-helper into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

psa-systems/lets-chat!272

No description provided.