Fix broken ICO + restore animation for WebP and APNG sources#4
Fix broken ICO + restore animation for WebP and APNG sources#4adityaverm-a wants to merge 5 commits into
Conversation
Two failure modes converge on broken .ico requests today. When origin sends Content-Type: image/x-icon the fetcher rejects with HTTP 415 because ICO is missing from isValidImageContentType. When origin sends a generic content-type the bytes pass through, Sharp has no ICO decoder, and ErrorMapper falls through to ImageProcessingError(500, 'ProcessingFailure'). Both paths reach end users as broken icons. ICO is a multi-resolution container, not a single-frame raster — Sharp can't decode it and re-encoding would destroy the bundled sizes anyway. Serve the source bytes verbatim. - origin-fetcher: whitelist image/x-icon and image/vnd.microsoft.icon; add ICO magic bytes (00 00 01 00) to validateImageMagicNumbers so content-type/buffer mismatches are still caught. - image-processor: after fetch, if sourceImageContentType is an ICO type, short-circuit the transformation pipeline — set response.contentType to the source type, log an ico_passthrough event, return the buffer unchanged. Same shape as the existing empty-transformations branch. - auto-optimizer: skip the format optimization for ICO sources so it doesn't appear in transformation logs as if it were applied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sharp's animated:true flag was previously gated on image/gif alone, so
the ANIM chunk in animated WebP and the acTL chunk in APNG were ignored
and animation was silently stripped on first encode. As a side effect,
the Q50 animated-WebP cap added recently for the optimization sweep
never fired on real WebP sources — metadata.pages was always 1.
Broaden the trigger:
ANIMATION_CAPABLE_SOURCE_TYPES = { image/gif, image/webp, image/png }
Two implementation details that matter:
1. The metadata probe must be animation-aware. sharp(buf).metadata()
without animated:true returns pages=1 for APNG (the acTL chunk
isn't surfaced). The existing pages<=1 reset would then incorrectly
fire and undo the fix. Compute isExpectedToBeAnimated first, then
pass it into the metadata probe.
2. The auto-optimizer guard needs to widen alongside. The old
sourceIsGif check is generalized to any animation-capable source —
when client only accepts a non-animation-capable target (jpeg, etc.)
we skip the format conversion so any animation in the source
survives. Image-processor verifies multi-frame downstream; static
sources still emit the source format unchanged via the same
pages<=1 reset path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes production issues in the image transformation pipeline by (1) supporting ICO sources safely (bypass Sharp and serve bytes verbatim) and (2) preserving animation for animation-capable source containers (WebP/APNG) by making Sharp metadata probing and instantiation animation-aware and widening auto-optimizer guards accordingly.
Changes:
- Add ICO support in origin fetching (Content-Type allowlist + ICO magic bytes validation) and bypass Sharp processing for ICO in the processor.
- Preserve animation for WebP/APNG by treating
image/webpandimage/pngas animation-capable sources and probing metadata withanimated: true. - Update auto-optimizer to skip format conversion for ICO sources and to conservatively avoid converting animation-capable sources to non-animation-capable targets; expand/adjust tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts | Generalizes animation-related format-selection guard and skips format optimization for ICO sources. |
| source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts | Adds/updates tests for generalized animation guard and ICO passthrough behavior. |
| source/container/src/services/image-processing/origin-fetcher.ts | Accepts ICO Content-Types and validates ICO magic bytes. |
| source/container/src/services/image-processing/origin-fetcher.test.ts | Adds tests for ICO Content-Type acceptance and ICO magic-number validation/mismatch behavior. |
| source/container/src/services/image-processing/image-processor.service.ts | Adds ICO passthrough and makes metadata probing/Sharp instantiation animation-aware for WebP/APNG. |
| source/container/src/services/image-processing/image-processor.service.test.ts | Adds tests for ICO passthrough and broadened animation-capable source detection (incl. single-frame WebP/PNG regression coverage). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous animation fix (broadening isExpectedToBeAnimated to include image/png) is necessary but not sufficient for APNG. Live testing on a real animated PNG (158x99, 13 frames, 383KB) showed the output came back as a 100x63 single-frame static PNG with the acTL chunk stripped. Sharp 0.34.x cannot read APNG as multi-frame nor write animated PNG output — animated:true on a PNG input yields pages=1, and the PNG encoder has no APNG mode. Any transformation silently strips animation. Add a buffer sniff for the acTL chunk on PNG sources. If detected, short-circuit the transformation pipeline and serve the source bytes verbatim — same shape as the ICO passthrough. We lose the ability to resize APNG, but preserve animation which is the user-visible bug. Static PNGs are untouched: they don't contain acTL, so the sniff returns false and the request continues through Sharp normally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR review flagged that downstream guards (ICO passthrough, animation trigger, FORMAT_MAPPING, contentTypeToFormat) use exact-match Set / object key comparisons against sourceImageContentType. If an origin sends `image/png; charset=utf-8` or `IMAGE/WEBP`, those guards silently miss — re-introducing the bugs we just fixed (ICO 500s and animation stripping). Normalize at the two boundaries where origin headers enter the pipeline: - connection-manager: strip parameters + trim + lowercase before assigning to imageRequest.sourceImageContentType. Restructured so normalization runs first, then validateContentType operates on the normalized form — this also fixes uppercase Content-Types (`IMAGE/PNG`) which previously slipped past the case-sensitive startsWith check and got rejected as INVALID_FORMAT. - origin-fetcher.validateImageMagicNumbers: strip parameters before the contentTypeToFormat lookup so magic-number validation isn't silently skipped for parameterized headers. Tests cover parameterized, uppercase, and whitespace-padded variants across both HTTP and S3 paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
source/container/src/services/image-processing/origin-fetcher.ts:201
- WebP magic-number validation currently treats any RIFF container (
52494646/ "RIFF") as WebP. RIFF is a generic container (e.g. WAV/AVI), so a non-WebP RIFF payload labeledimage/webpwill incorrectly pass magic-number validation and fail later in Sharp (often as a 500). Consider validating the WebP fourCC at offset 8 ("WEBP") before classifying RIFF as WebP.
const fileHeader = buffer.subarray(0, 4).toString('hex').toUpperCase();
let detectedFormat: string | undefined;
for (const [magic, format] of Object.entries(magicToFormat)) {
if (fileHeader.startsWith(magic)) {
… (exact match) Two correctness tighten-ups from PR review: 1. isAnimatedPng: replace 4KB substring scan for 'acTL' with a proper chunk-stream walk. The substring approach could false-positive on a tEXt chunk containing 'acTL' as its keyword/data, or on coincidental bytes in compressed IDAT data — silently routing a resizable static PNG into the passthrough path. The new parser walks length/type/data chunks, returns true only when an actual acTL chunk type appears before the first IDAT, and is bounded against truncation / length-field corruption. 2. isValidImageContentType: replace `validTypes.some(includes)` with `Set.has(normalized)`. Substring matching was overly permissive — `application/image/jpeg`, `image/pngfoo`, and similar impostors all slipped through. Normalizes Content-Type (strip params + lowercase) once and requires exact set membership. Tests added for both: - PNG with tEXt-chunk-acTL-as-data → not animated (false-positive guard) - PNG with acTL trailing IDAT → not animated (spec compliance) - Truncated buffer → not animated (robustness) - application/image/jpeg, image/pngfoo, etc. → rejected - parameterized & whitespace-padded valid types → accepted - unknown image subtypes (image/svg+xml, image/bmp) → rejected Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| // ICONDIR header: reserved(2)=0, type(2)=1 (icon) — 00 00 01 00 little-endian. | ||
| '00000100': 'ico' | ||
| }; |
| i += 12 + length; | ||
| // Guard against truncation / length-field corruption causing infinite loop. | ||
| if (length > end || i < 0) return false; |
Summary
Three production bugs reported on
image-transformation.punchh.iocome down to two root causes — both inimage-processor/origin-fetcher. Two commits.Root causes:
origin-fetcher.tswhitelists eight image MIME types inisValidImageContentType();image/x-iconis absent. When origin sends that type the fetcher returns HTTP 415, more commonly S3 serves.icowith a generic content-type, the bytes pass through, Sharp has no ICO decoder, andErrorMapper.mapError()falls through to a defaultImageProcessingError(500, 'ProcessingFailure', ...)— the 500 we see in production.image-processor.service.tswas instantiating Sharp withanimated: trueonly when source content-type wasimage/gif. Animated WebP (ANIM chunk) and APNG (acTL chunk) were read single-frame, so animation was silently stripped on first encode. Side effect: the Q50 animated-WebP cap added in the optimization sweep (#2, commit52bfe6d) never fired on real WebP sources becausemetadata.pageswas always 1.Fixes:
4a7cec5— BUG-01: serve ICO sources verbatim, bypassing SharpICO is a multi-resolution container, not a single-frame raster. Sharp can't decode it and re-encoding would discard the bundled sizes anyway — serve the source bytes verbatim.
origin-fetcher.ts: whitelistimage/x-iconandimage/vnd.microsoft.icon; add ICO magic bytes (00 00 01 00) tovalidateImageMagicNumbersso content-type/buffer mismatches are still caught.image-processor.service.ts: after fetch, ifsourceImageContentTypeis an ICO type, short-circuit the transformation pipeline — setresponse.contentTypeto the source type, log anico_passthroughevent, return the buffer unchanged. Same shape as the existing empty-transformations branch.auto-optimizer.ts: skip the format optimization for ICO sources so it doesn't appear in transformation logs as if it were applied.Unblocks: every
.icoURL currently 500-ing (e.g./static/par-favicon.ico,/development/redeemables/.../631addc5537f2622da5e5c13718223267097c82e.ico).0cdbcba— BUG-02 + BUG-03: preserve animation for WebP and APNGBroaden
isExpectedToBeAnimated:Two implementation subtleties:
The metadata probe must be animation-aware.
sharp(buf).metadata()withoutanimated: truereturnspages: 1for APNG (theacTLchunk isn't surfaced). The existingpages <= 1reset would then incorrectly fire and undo the fix. ComputeisExpectedToBeAnimatedfirst, then pass it into the metadata probe.The auto-optimizer guard widens alongside. The old
sourceIsGifcheck generalizes to any animation-capable source — when client only accepts a non-animation-capable target (jpeg, etc.) we skip the format conversion so any animation in the source survives. The image-processor verifies multi-frame downstream; static sources still emit the source format unchanged via the samepages <= 1reset path.Unblocks: animated WebP at
/development/redeemables/.../1e60f883869f60656cbd9868ca16c9f95ff8114a.webpand APNG at/development/subscription_plans/.../83d8b62a7733dec4275e3062c6a24f20a3c1e02a.png. Also activates the dormant Q50 animated-WebP cap from PR #2.Test plan
origin-fetcher.test.ts— 19/19 passing (3 new: accepts ICO content-types; ICO magic bytes accepted with both ICO content types; mismatch caught when ICO bytes are labeled asimage/png)image-processor.service.test.ts— 30/30 passing (3 new for ICO passthrough; 8 new for animation-capable source detection including single-frame WebP/PNG regression coverage)auto-optimizer.test.ts— 25/25 passing (2 new for ICO passthrough; 5 new for generalized animation guard covering WebP and PNG; one existing test revised because thenon-GIF sourcesassertion no longer reflects current behavior)edit-applicator.test.ts— 27/27 passing (regression check)par-favicon.icoreturns 200 withContent-Type: image/x-icon; confirm the animated WebP and APNG redeemables animate in browser; confirm a static PNG/WebP still produces output (reset path)operation: ico_passthroughlog line for unexpected volume🤖 Generated with Claude Code