From 4a7cec5f398c2813012a6a4eab6883755be8a5e0 Mon Sep 17 00:00:00 2001 From: adityaverm-a Date: Tue, 2 Jun 2026 13:06:10 +0530 Subject: [PATCH 1/5] feat: serve ICO sources verbatim, bypassing Sharp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../image-processor.service.test.ts | 74 +++++++++++++++++++ .../image-processor.service.ts | 25 ++++++- .../image-processing/origin-fetcher.test.ts | 22 ++++++ .../image-processing/origin-fetcher.ts | 17 ++++- .../auto-optimization/auto-optimizer.test.ts | 22 ++++++ .../auto-optimization/auto-optimizer.ts | 15 +++- 6 files changed, 168 insertions(+), 7 deletions(-) diff --git a/source/container/src/services/image-processing/image-processor.service.test.ts b/source/container/src/services/image-processing/image-processor.service.test.ts index fe0ddb81d..c6b1dd776 100644 --- a/source/container/src/services/image-processing/image-processor.service.test.ts +++ b/source/container/src/services/image-processing/image-processor.service.test.ts @@ -302,6 +302,80 @@ describe('ImageProcessorService', () => { }); }); + describe('ICO passthrough', () => { + // Minimal valid ICONDIR: reserved(2)=0, type(2)=1, count(2)=0 — plus a couple + // trailing bytes to be over the 4-byte minimum. The processor must not call + // Sharp on this, so the buffer never needs to be a real ICO. + const ICO_HEADER = Buffer.from([0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0xAB, 0xCD]); + + it('should pass ICO buffer through unchanged when transformations are present', async () => { + jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ + buffer: ICO_HEADER, + metadata: { size: ICO_HEADER.length, format: 'x-icon' } + }); + + const request: ImageProcessingRequest = { + requestId: 'test-ico-passthrough', + timestamp: Date.now(), + origin: { url: 'https://example.com/favicon.ico' }, + sourceImageContentType: 'image/x-icon', + // Policy might have asked for resize/format conversion — must be ignored for ICO. + transformations: [{ type: 'resize', value: { width: 50 }, source: 'url' }], + response: { headers: {} } + }; + + const result = await service.process(request); + + expect(result).toBe(ICO_HEADER); + expect(request.response.contentType).toBe('image/x-icon'); + expect(request.timings.imageProcessing.transformationApplicationMs).toBe(0); + }); + + it('should pass ICO buffer through unchanged for image/vnd.microsoft.icon', async () => { + jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ + buffer: ICO_HEADER, + metadata: { size: ICO_HEADER.length, format: 'vnd.microsoft.icon' } + }); + + const request: ImageProcessingRequest = { + requestId: 'test-ico-vnd-passthrough', + timestamp: Date.now(), + origin: { url: 'https://example.com/favicon.ico' }, + sourceImageContentType: 'image/vnd.microsoft.icon', + transformations: [{ type: 'resize', value: { width: 50 }, source: 'url' }], + response: { headers: {} } + }; + + const result = await service.process(request); + + expect(result).toBe(ICO_HEADER); + expect(request.response.contentType).toBe('image/vnd.microsoft.icon'); + }); + + it('should still process non-ICO sources normally', async () => { + // Regression guard: the ICO short-circuit must only fire for ICO content types. + jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ + buffer: TEST_JPEG_BUFFER, + metadata: { size: TEST_JPEG_BUFFER.length, format: 'jpeg' } + }); + + const request: ImageProcessingRequest = { + requestId: 'test-not-ico', + timestamp: Date.now(), + origin: { url: 'https://example.com/image.jpg' }, + sourceImageContentType: 'image/jpeg', + transformations: [{ type: 'resize', value: { width: 50 }, source: 'url' }], + response: { headers: {} } + }; + + const result = await service.process(request); + + // Must be a transformed Sharp output, not the original buffer. + expect(result).not.toBe(TEST_JPEG_BUFFER); + expect(request.response.contentType).toMatch(/^image\//); + }); + }); + describe('animated GIF handling', () => { it('should re-instantiate with animated=false for single-frame GIF', async () => { jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ diff --git a/source/container/src/services/image-processing/image-processor.service.ts b/source/container/src/services/image-processing/image-processor.service.ts index 6afc21edb..ba65782ba 100644 --- a/source/container/src/services/image-processing/image-processor.service.ts +++ b/source/container/src/services/image-processing/image-processor.service.ts @@ -17,6 +17,12 @@ export class ImageProcessorService { this.originFetcher = new OriginFetcher(); } + private static readonly ICO_CONTENT_TYPES = new Set(['image/x-icon', 'image/vnd.microsoft.icon']); + + private static isIcoContentType(contentType: string | undefined): boolean { + return !!contentType && ImageProcessorService.ICO_CONTENT_TYPES.has(contentType.toLowerCase()); + } + public static getInstance(): ImageProcessorService { if (!ImageProcessorService.instance) { ImageProcessorService.instance = new ImageProcessorService(); @@ -43,7 +49,24 @@ export class ImageProcessorService { imageRequest.timings.imageProcessing.transformationApplicationMs = 0; return imageBuffer; } - + + // ICO is a multi-resolution container, not a single-frame raster. Sharp has + // no ICO decoder and re-encoding would discard the bundled sizes anyway — + // serve the source bytes verbatim regardless of what transformations the + // policy requested. + if (ImageProcessorService.isIcoContentType(imageRequest.sourceImageContentType)) { + imageRequest.response.contentType = imageRequest.sourceImageContentType; + imageRequest.timings.imageProcessing.transformationApplicationMs = 0; + console.log(JSON.stringify({ + requestId: imageRequest.requestId, + component: 'ImageProcessor', + operation: 'ico_passthrough', + contentType: imageRequest.sourceImageContentType, + sizeBytes: imageBuffer.length + })); + return imageBuffer; + } + // Extract source dimensions to validate auto-resize transformations const metadata = await sharp(imageBuffer).metadata(); this.preventAutoUpscaling(imageRequest, metadata.width); diff --git a/source/container/src/services/image-processing/origin-fetcher.test.ts b/source/container/src/services/image-processing/origin-fetcher.test.ts index d2b2ce068..6b5c835d3 100644 --- a/source/container/src/services/image-processing/origin-fetcher.test.ts +++ b/source/container/src/services/image-processing/origin-fetcher.test.ts @@ -27,6 +27,11 @@ describe('OriginFetcher', () => { it('should handle case insensitive content types', () => { expect(fetcher['isValidImageContentType']('IMAGE/JPEG')).toBe(true); }); + + it('should accept ICO content types', () => { + expect(fetcher['isValidImageContentType']('image/x-icon')).toBe(true); + expect(fetcher['isValidImageContentType']('image/vnd.microsoft.icon')).toBe(true); + }); }); describe('error handling', () => { @@ -101,6 +106,23 @@ describe('OriginFetcher', () => { expect(() => fetcher['validateImageMagicNumbers'](malformedPngBuffer, 'image/png', 'https://example.com/test.png')) .toThrow('Invalid image file'); }); + + it('should accept ICO magic numbers with image/x-icon content-type', () => { + // ICONDIR header: 00 00 01 00 + const icoBuffer = Buffer.from([0x00, 0x00, 0x01, 0x00]); + expect(() => fetcher['validateImageMagicNumbers'](icoBuffer, 'image/x-icon', 'https://example.com/test.ico')).not.toThrow(); + }); + + it('should accept ICO magic numbers with image/vnd.microsoft.icon content-type', () => { + const icoBuffer = Buffer.from([0x00, 0x00, 0x01, 0x00]); + expect(() => fetcher['validateImageMagicNumbers'](icoBuffer, 'image/vnd.microsoft.icon', 'https://example.com/test.ico')).not.toThrow(); + }); + + it('should reject content-type mismatch when ICO bytes are served as image/png', () => { + const icoBuffer = Buffer.from([0x00, 0x00, 0x01, 0x00]); + expect(() => fetcher['validateImageMagicNumbers'](icoBuffer, 'image/png', 'https://example.com/test.png')) + .toThrow('Content-Type mismatch'); + }); }); describe('fetchImage', () => { diff --git a/source/container/src/services/image-processing/origin-fetcher.ts b/source/container/src/services/image-processing/origin-fetcher.ts index 845afa5f5..0b1c41fcd 100644 --- a/source/container/src/services/image-processing/origin-fetcher.ts +++ b/source/container/src/services/image-processing/origin-fetcher.ts @@ -148,13 +148,18 @@ export class OriginFetcher { private isValidImageContentType(contentType: string): boolean { const validTypes = [ 'image/jpeg', - 'image/jpg', + 'image/jpg', 'image/png', 'image/webp', 'image/gif', 'image/tiff', 'image/avif', 'image/heif', + // ICO is accepted but bypasses the Sharp pipeline downstream (see + // ImageProcessorService — Sharp has no ICO decoder, and ICO is a + // multi-resolution bundle that must not be re-encoded). + 'image/x-icon', + 'image/vnd.microsoft.icon', ]; return validTypes.some(type => contentType.toLowerCase().includes(type)); } @@ -169,11 +174,13 @@ export class OriginFetcher { const magicToFormat = { 'FFD8FF': 'jpeg', - '89504E47': 'png', + '89504E47': 'png', '47494638': 'gif', '52494646': 'webp', '49492A00': 'tiff', - '4D4D002A': 'tiff' + '4D4D002A': 'tiff', + // ICONDIR header: reserved(2)=0, type(2)=1 (icon) — 00 00 01 00 little-endian. + '00000100': 'ico' }; const contentTypeToFormat = { @@ -182,7 +189,9 @@ export class OriginFetcher { 'image/jpeg': 'jpeg', 'image/jpg': 'jpeg', 'image/tiff': 'tiff', - 'image/gif': 'gif' + 'image/gif': 'gif', + 'image/x-icon': 'ico', + 'image/vnd.microsoft.icon': 'ico' }; const fileHeader = buffer.subarray(0, 4).toString('hex').toUpperCase(); diff --git a/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts b/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts index 513725e41..1a80efb5d 100644 --- a/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts +++ b/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts @@ -122,6 +122,28 @@ describe('applyAutoOptimizations', () => { expect(result[0]).toEqual({ type: 'format', value: 'jpeg', source: 'auto' }); }); + it('should skip format conversion when source is ICO (image/x-icon)', () => { + // ICO passes through downstream — picking a format here is wasted work + // and pollutes transformation logs. + mockPolicy.outputs = [{ type: 'format', value: 'auto' }]; + mockRequest.headers = { 'dit-accept': 'image/webp,image/jpeg' }; + const imageRequest = { sourceImageContentType: 'image/x-icon' } as ImageProcessingRequest; + + const result = applyAutoOptimizations(baseTransformations, mockRequest as Request, mockPolicy, imageRequest); + + expect(result).toHaveLength(0); + }); + + it('should skip format conversion when source is ICO (image/vnd.microsoft.icon)', () => { + mockPolicy.outputs = [{ type: 'format', value: 'auto' }]; + mockRequest.headers = { 'dit-accept': 'image/webp,image/jpeg' }; + const imageRequest = { sourceImageContentType: 'image/vnd.microsoft.icon' } as ImageProcessingRequest; + + const result = applyAutoOptimizations(baseTransformations, mockRequest as Request, mockPolicy, imageRequest); + + expect(result).toHaveLength(0); + }); + }); diff --git a/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts b/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts index e12f7955d..7f49e1454 100644 --- a/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts +++ b/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts @@ -8,6 +8,10 @@ import { ImageProcessingRequest } from '../../../types/image-processing-request' const FORMAT_PRIORITY = ['webp', 'avif', 'jpeg', 'png', 'heif', 'tiff', 'raw', 'gif']; // TODO, DISCUSS WITH TEAM FOR OPTIMAL FORMAT PRIORITIY LIST const ANIMATION_CAPABLE_FORMATS = new Set(['webp', 'avif', 'gif']); +// Source content-types that Sharp cannot decode and must pass through unchanged. +// The image-processor short-circuits these, so any format optimization picked +// here would be discarded — skip it for log/metric clarity. +const PASSTHROUGH_SOURCE_CONTENT_TYPES = new Set(['image/x-icon', 'image/vnd.microsoft.icon']); const FORMAT_MAPPING: Record = { 'image/webp': 'webp', 'image/png': 'png', @@ -69,11 +73,18 @@ function getFormatOptimizations(req: Request, formatConfig: any, imageRequest?: .map(mimeType => FORMAT_MAPPING[mimeType]); const selectedFormat = FORMAT_PRIORITY.find(format => compatibleFormats.includes(format)); - + if (!selectedFormat) { return []; } - + + // ICO sources pass through unchanged downstream (Sharp can't decode them and + // re-encoding would destroy the multi-resolution bundle). Skip the format + // optimization so it doesn't appear in transformation logs as if it were applied. + if (PASSTHROUGH_SOURCE_CONTENT_TYPES.has(imageRequest?.sourceImageContentType)) { + return []; + } + // Skip format conversion if source is a GIF and selected format cannot carry animation const sourceIsGif = imageRequest?.sourceImageContentType === 'image/gif'; if (sourceIsGif && !ANIMATION_CAPABLE_FORMATS.has(selectedFormat)) { From 0cdbcba299a7c25a52d2d2c8a1cea151dc5752ca Mon Sep 17 00:00:00 2001 From: adityaverm-a Date: Tue, 2 Jun 2026 13:19:31 +0530 Subject: [PATCH 2/5] feat: preserve animation for WebP and APNG sources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../image-processor.service.test.ts | 85 ++++++++++++++++++- .../image-processor.service.ts | 27 +++++- .../auto-optimization/auto-optimizer.test.ts | 52 +++++++++++- .../auto-optimization/auto-optimizer.ts | 18 +++- 4 files changed, 172 insertions(+), 10 deletions(-) diff --git a/source/container/src/services/image-processing/image-processor.service.test.ts b/source/container/src/services/image-processing/image-processor.service.test.ts index c6b1dd776..457f2e3bc 100644 --- a/source/container/src/services/image-processing/image-processor.service.test.ts +++ b/source/container/src/services/image-processing/image-processor.service.test.ts @@ -10,16 +10,28 @@ import sharp from 'sharp'; let TEST_JPEG_BUFFER: Buffer; let TEST_GIF_BUFFER: Buffer; +let TEST_WEBP_BUFFER: Buffer; +let TEST_PNG_BUFFER: Buffer; beforeAll(async () => { // Generate valid test images using Sharp TEST_JPEG_BUFFER = await sharp({ create: { width: 100, height: 100, channels: 3, background: { r: 255, g: 0, b: 0 } } }).jpeg().toBuffer(); - + TEST_GIF_BUFFER = await sharp({ create: { width: 100, height: 100, channels: 3, background: { r: 0, g: 0, b: 255 } } }).gif().toBuffer(); + + // Single-frame WebP/PNG — used to exercise the "animation-capable source type + // but pages<=1" reset path under the broadened isExpectedToBeAnimated check. + TEST_WEBP_BUFFER = await sharp({ + create: { width: 100, height: 100, channels: 3, background: { r: 0, g: 255, b: 0 } } + }).webp().toBuffer(); + + TEST_PNG_BUFFER = await sharp({ + create: { width: 100, height: 100, channels: 3, background: { r: 128, g: 128, b: 128 } } + }).png().toBuffer(); }); describe('ImageProcessorService', () => { @@ -376,6 +388,77 @@ describe('ImageProcessorService', () => { }); }); + describe('animation-capable source detection', () => { + // isAnimationCapableSource is the gate that decides whether Sharp is + // instantiated with animated:true. Without animated:true, the ANIM (WebP) + // and acTL (APNG) chunks are ignored and animation is silently lost. + it('should classify GIF as animation-capable', () => { + expect(ImageProcessorService['isAnimationCapableSource']('image/gif')).toBe(true); + }); + + it('should classify WebP as animation-capable (covers animated WebP)', () => { + expect(ImageProcessorService['isAnimationCapableSource']('image/webp')).toBe(true); + }); + + it('should classify PNG as animation-capable (covers APNG)', () => { + expect(ImageProcessorService['isAnimationCapableSource']('image/png')).toBe(true); + }); + + it('should classify JPEG as not animation-capable', () => { + expect(ImageProcessorService['isAnimationCapableSource']('image/jpeg')).toBe(false); + }); + + it('should be case-insensitive', () => { + expect(ImageProcessorService['isAnimationCapableSource']('IMAGE/WEBP')).toBe(true); + }); + + it('should handle undefined content type', () => { + expect(ImageProcessorService['isAnimationCapableSource'](undefined)).toBe(false); + }); + + it('should process a single-frame WebP source (animated reset path)', async () => { + // Regression guard: with isExpectedToBeAnimated=true for WebP, a static + // WebP must still reach the reset path (pages<=1) and produce output. + jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ + buffer: TEST_WEBP_BUFFER, + metadata: { size: TEST_WEBP_BUFFER.length, format: 'webp' } + }); + + const request: ImageProcessingRequest = { + requestId: 'test-static-webp', + timestamp: Date.now(), + origin: { url: 'https://example.com/image.webp' }, + sourceImageContentType: 'image/webp', + transformations: [{ type: 'resize', value: { width: 50 }, source: 'url' }], + response: { headers: {} } + }; + + const result = await service.process(request); + expect(result).toBeInstanceOf(Buffer); + expect(request.response.contentType).toMatch(/^image\//); + }); + + it('should process a single-frame PNG source (animated reset path)', async () => { + jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ + buffer: TEST_PNG_BUFFER, + metadata: { size: TEST_PNG_BUFFER.length, format: 'png' } + }); + + const request: ImageProcessingRequest = { + requestId: 'test-static-png', + timestamp: Date.now(), + origin: { url: 'https://example.com/image.png' }, + sourceImageContentType: 'image/png', + transformations: [{ type: 'resize', value: { width: 50 }, source: 'url' }], + response: { headers: {} } + }; + + const result = await service.process(request); + expect(result).toBeInstanceOf(Buffer); + expect(request.response.contentType).toMatch(/^image\//); + }); + }); + describe('animated GIF handling', () => { it('should re-instantiate with animated=false for single-frame GIF', async () => { jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ diff --git a/source/container/src/services/image-processing/image-processor.service.ts b/source/container/src/services/image-processing/image-processor.service.ts index ba65782ba..1895bf406 100644 --- a/source/container/src/services/image-processing/image-processor.service.ts +++ b/source/container/src/services/image-processing/image-processor.service.ts @@ -18,11 +18,24 @@ export class ImageProcessorService { } private static readonly ICO_CONTENT_TYPES = new Set(['image/x-icon', 'image/vnd.microsoft.icon']); + // Content-types whose containers can carry multiple frames. WebP advertises + // animation via an ANIM chunk; PNG via the acTL chunk (APNG). Sharp needs + // animated: true at instantiation to surface those frames — without it the + // ANIM/acTL chunks are ignored and we silently drop animation on first encode. + private static readonly ANIMATION_CAPABLE_SOURCE_TYPES = new Set([ + 'image/gif', + 'image/webp', + 'image/png', + ]); private static isIcoContentType(contentType: string | undefined): boolean { return !!contentType && ImageProcessorService.ICO_CONTENT_TYPES.has(contentType.toLowerCase()); } + private static isAnimationCapableSource(contentType: string | undefined): boolean { + return !!contentType && ImageProcessorService.ANIMATION_CAPABLE_SOURCE_TYPES.has(contentType.toLowerCase()); + } + public static getInstance(): ImageProcessorService { if (!ImageProcessorService.instance) { ImageProcessorService.instance = new ImageProcessorService(); @@ -67,13 +80,20 @@ export class ImageProcessorService { return imageBuffer; } + // Whether to read the source as multi-frame. Must be decided before the + // metadata probe — Sharp only surfaces ANIM (WebP) / acTL (APNG) frame + // counts when instantiated with animated: true, so probing without the + // flag would report pages=1 for APNG and trigger an incorrect reset + // downstream. + const isExpectedToBeAnimated = ImageProcessorService.isAnimationCapableSource(imageRequest.sourceImageContentType); + // Extract source dimensions to validate auto-resize transformations - const metadata = await sharp(imageBuffer).metadata(); + const metadata = await sharp(imageBuffer, { animated: isExpectedToBeAnimated }).metadata(); this.preventAutoUpscaling(imageRequest, metadata.width); - + // We need to map Transformations to Edits before Sharp image instantiation because it influences whether or not we strip or keep metadata const imageEdits = await TransformationMapper.mapToImageEdits(imageRequest.transformations); - + console.log(JSON.stringify({ requestId: imageRequest.requestId, component: 'TransformationMapper', @@ -82,7 +102,6 @@ export class ImageProcessorService { editCount: Object.keys(imageEdits).length })); - const isExpectedToBeAnimated = imageRequest.sourceImageContentType == 'image/gif'; let sharpOptions = { failOnError: true, animated: isExpectedToBeAnimated diff --git a/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts b/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts index 1a80efb5d..2320f602c 100644 --- a/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts +++ b/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.test.ts @@ -111,15 +111,63 @@ describe('applyAutoOptimizations', () => { expect(result[0]).toEqual({ type: 'format', value: 'avif', source: 'auto' }); }); - it('should not restrict format selection for non-GIF sources', () => { + it('should not restrict format selection for non-animation-capable sources', () => { + // JPEG is single-frame by definition — the animation guard should not fire. + mockPolicy.outputs = [{ type: 'format', value: 'auto' }]; + mockRequest.headers = { 'dit-accept': 'image/jpeg' }; + const imageRequest = { sourceImageContentType: 'image/jpeg' } as ImageProcessingRequest; + + const result = applyAutoOptimizations(baseTransformations, mockRequest as Request, mockPolicy, imageRequest); + + // jpeg→jpeg hits the same-format short-circuit. + expect(result).toHaveLength(0); + }); + + it('should skip format conversion when source is WebP and selected format is not animation-capable', () => { + // WebP container can carry animation (ANIM chunk). Block jpeg selection so + // any animation in the source survives. + mockPolicy.outputs = [{ type: 'format', value: 'auto' }]; + mockRequest.headers = { 'dit-accept': 'image/jpeg' }; + const imageRequest = { sourceImageContentType: 'image/webp' } as ImageProcessingRequest; + + const result = applyAutoOptimizations(baseTransformations, mockRequest as Request, mockPolicy, imageRequest); + + expect(result).toHaveLength(0); + }); + + it('should allow WebP → AVIF (both animation-capable)', () => { + mockPolicy.outputs = [{ type: 'format', value: 'auto' }]; + mockRequest.headers = { 'dit-accept': 'image/avif' }; + const imageRequest = { sourceImageContentType: 'image/webp' } as ImageProcessingRequest; + + const result = applyAutoOptimizations(baseTransformations, mockRequest as Request, mockPolicy, imageRequest); + + expect(result).toHaveLength(1); + expect(result[0]).toEqual({ type: 'format', value: 'avif', source: 'auto' }); + }); + + it('should skip format conversion when source is PNG and selected format is not animation-capable', () => { + // PNG container can carry animation (acTL chunk → APNG). Block jpeg + // selection so APNG animation survives — static PNGs still pass through + // unchanged when the image-processor sees pages <= 1. mockPolicy.outputs = [{ type: 'format', value: 'auto' }]; mockRequest.headers = { 'dit-accept': 'image/jpeg' }; const imageRequest = { sourceImageContentType: 'image/png' } as ImageProcessingRequest; const result = applyAutoOptimizations(baseTransformations, mockRequest as Request, mockPolicy, imageRequest); + expect(result).toHaveLength(0); + }); + + it('should allow PNG → WebP (animation-capable)', () => { + mockPolicy.outputs = [{ type: 'format', value: 'auto' }]; + mockRequest.headers = { 'dit-accept': 'image/webp' }; + const imageRequest = { sourceImageContentType: 'image/png' } as ImageProcessingRequest; + + const result = applyAutoOptimizations(baseTransformations, mockRequest as Request, mockPolicy, imageRequest); + expect(result).toHaveLength(1); - expect(result[0]).toEqual({ type: 'format', value: 'jpeg', source: 'auto' }); + expect(result[0]).toEqual({ type: 'format', value: 'webp', source: 'auto' }); }); it('should skip format conversion when source is ICO (image/x-icon)', () => { diff --git a/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts b/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts index 7f49e1454..ab9a33dcb 100644 --- a/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts +++ b/source/container/src/services/transformation-resolver/auto-optimization/auto-optimizer.ts @@ -8,6 +8,12 @@ import { ImageProcessingRequest } from '../../../types/image-processing-request' const FORMAT_PRIORITY = ['webp', 'avif', 'jpeg', 'png', 'heif', 'tiff', 'raw', 'gif']; // TODO, DISCUSS WITH TEAM FOR OPTIMAL FORMAT PRIORITIY LIST const ANIMATION_CAPABLE_FORMATS = new Set(['webp', 'avif', 'gif']); +// Source content-types whose container can carry multi-frame animation. We +// can't tell pre-fetch whether the file actually is animated, so we behave +// conservatively: if the client only accepts a non-animation-capable target +// (e.g. jpeg), skip the format conversion so any possible animation survives. +// Matches the set in ImageProcessorService.ANIMATION_CAPABLE_SOURCE_TYPES. +const ANIMATION_CAPABLE_SOURCE_CONTENT_TYPES = new Set(['image/gif', 'image/webp', 'image/png']); // Source content-types that Sharp cannot decode and must pass through unchanged. // The image-processor short-circuits these, so any format optimization picked // here would be discarded — skip it for log/metric clarity. @@ -85,9 +91,15 @@ function getFormatOptimizations(req: Request, formatConfig: any, imageRequest?: return []; } - // Skip format conversion if source is a GIF and selected format cannot carry animation - const sourceIsGif = imageRequest?.sourceImageContentType === 'image/gif'; - if (sourceIsGif && !ANIMATION_CAPABLE_FORMATS.has(selectedFormat)) { + // Skip format conversion when the source container can carry animation (gif, + // webp, png/APNG) and the selected target cannot. We can't distinguish + // static-vs-animated pre-fetch, so we err on the side of preserving any + // possible animation; the image-processor verifies multi-frame downstream + // and a static source still emits the source format unchanged. + if ( + ANIMATION_CAPABLE_SOURCE_CONTENT_TYPES.has(imageRequest?.sourceImageContentType) && + !ANIMATION_CAPABLE_FORMATS.has(selectedFormat) + ) { return []; } From 5be97b874b5893467d030ccfb35208a52af1a0d9 Mon Sep 17 00:00:00 2001 From: adityaverm-a Date: Tue, 2 Jun 2026 14:09:20 +0530 Subject: [PATCH 3/5] fix: passthrough APNG sources, Sharp can't preserve animation on output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../image-processor.service.test.ts | 80 +++++++++++++++++++ .../image-processor.service.ts | 30 +++++++ 2 files changed, 110 insertions(+) diff --git a/source/container/src/services/image-processing/image-processor.service.test.ts b/source/container/src/services/image-processing/image-processor.service.test.ts index 457f2e3bc..f1792224e 100644 --- a/source/container/src/services/image-processing/image-processor.service.test.ts +++ b/source/container/src/services/image-processing/image-processor.service.test.ts @@ -388,6 +388,86 @@ describe('ImageProcessorService', () => { }); }); + describe('APNG passthrough', () => { + // Minimal-but-valid leading PNG signature + IHDR + acTL chunk. The pipeline + // only sniffs the prefix — the chunks past acTL don't have to decode. + // PNG signature: 89 50 4E 47 0D 0A 1A 0A + // IHDR chunk: length(4)=0x0D type(4)='IHDR' data(13) crc(4) — 25 bytes + // acTL chunk: length(4)=0x08 type(4)='acTL' data(8) crc(4) — 20 bytes + const APNG_HEADER = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), // PNG sig + Buffer.from([0x00, 0x00, 0x00, 0x0d]), // IHDR length + Buffer.from('IHDR', 'ascii'), + Buffer.alloc(13 + 4), // IHDR data + crc (dummy) + Buffer.from([0x00, 0x00, 0x00, 0x08]), // acTL length + Buffer.from('acTL', 'ascii'), + Buffer.alloc(8 + 4), // acTL data + crc (dummy) + ]); + const STATIC_PNG_HEADER = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + Buffer.from([0x00, 0x00, 0x00, 0x0d]), + Buffer.from('IHDR', 'ascii'), + Buffer.alloc(13 + 4), + // No acTL chunk — straight to IDAT in real files + ]); + + it('should detect acTL chunk in APNG buffer', () => { + expect(ImageProcessorService['isAnimatedPng'](APNG_HEADER)).toBe(true); + }); + + it('should return false for a static PNG buffer (no acTL chunk)', () => { + expect(ImageProcessorService['isAnimatedPng'](STATIC_PNG_HEADER)).toBe(false); + }); + + it('should pass APNG buffer through unchanged when transformations are present', async () => { + jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ + buffer: APNG_HEADER, + metadata: { size: APNG_HEADER.length, format: 'png' } + }); + + const request: ImageProcessingRequest = { + requestId: 'test-apng-passthrough', + timestamp: Date.now(), + origin: { url: 'https://example.com/animated.png' }, + sourceImageContentType: 'image/png', + // Even with a resize requested, we serve the source verbatim — Sharp + // would strip animation on output otherwise. + transformations: [{ type: 'resize', value: { width: 50 }, source: 'url' }], + response: { headers: {} } + }; + + const result = await service.process(request); + + expect(result).toBe(APNG_HEADER); + expect(request.response.contentType).toBe('image/png'); + expect(request.timings.imageProcessing.transformationApplicationMs).toBe(0); + }); + + it('should still process static PNG through Sharp', async () => { + // Regression guard: only APNG short-circuits; static PNG must reach the + // Sharp pipeline. + jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ + buffer: TEST_PNG_BUFFER, + metadata: { size: TEST_PNG_BUFFER.length, format: 'png' } + }); + + const request: ImageProcessingRequest = { + requestId: 'test-static-png-not-passthrough', + timestamp: Date.now(), + origin: { url: 'https://example.com/static.png' }, + sourceImageContentType: 'image/png', + transformations: [{ type: 'resize', value: { width: 50 }, source: 'url' }], + response: { headers: {} } + }; + + const result = await service.process(request); + + // Must be the Sharp output, not the original buffer. + expect(result).not.toBe(TEST_PNG_BUFFER); + expect(request.response.contentType).toBe('image/png'); + }); + }); + describe('animation-capable source detection', () => { // isAnimationCapableSource is the gate that decides whether Sharp is // instantiated with animated:true. Without animated:true, the ANIM (WebP) diff --git a/source/container/src/services/image-processing/image-processor.service.ts b/source/container/src/services/image-processing/image-processor.service.ts index 1895bf406..bfd2be15e 100644 --- a/source/container/src/services/image-processing/image-processor.service.ts +++ b/source/container/src/services/image-processing/image-processor.service.ts @@ -36,6 +36,16 @@ export class ImageProcessorService { return !!contentType && ImageProcessorService.ANIMATION_CAPABLE_SOURCE_TYPES.has(contentType.toLowerCase()); } + // APNG is a PNG with an 'acTL' chunk preceding the first IDAT. Sharp 0.34.x + // cannot read APNG as multi-frame nor write animated PNG output, so any + // pipeline operation silently strips animation. We sniff a bounded prefix of + // the buffer — acTL is always near the top of the file in practice. + private static isAnimatedPng(buffer: Buffer): boolean { + const SCAN_LIMIT = 4096; + const head = buffer.subarray(0, Math.min(buffer.length, SCAN_LIMIT)); + return head.indexOf('acTL', 0, 'ascii') !== -1; + } + public static getInstance(): ImageProcessorService { if (!ImageProcessorService.instance) { ImageProcessorService.instance = new ImageProcessorService(); @@ -80,6 +90,26 @@ export class ImageProcessorService { return imageBuffer; } + // APNG passthrough. Sharp can neither read APNG as multi-frame nor write + // animated PNG output, so any transformation would silently strip + // animation. Serve the source bytes verbatim — we lose the ability to + // resize APNG, but preserve animation which is the user-visible bug. + if ( + imageRequest.sourceImageContentType?.toLowerCase() === 'image/png' && + ImageProcessorService.isAnimatedPng(imageBuffer) + ) { + imageRequest.response.contentType = imageRequest.sourceImageContentType; + imageRequest.timings.imageProcessing.transformationApplicationMs = 0; + console.log(JSON.stringify({ + requestId: imageRequest.requestId, + component: 'ImageProcessor', + operation: 'apng_passthrough', + contentType: imageRequest.sourceImageContentType, + sizeBytes: imageBuffer.length + })); + return imageBuffer; + } + // Whether to read the source as multi-frame. Must be decided before the // metadata probe — Sharp only surfaces ANIM (WebP) / acTL (APNG) frame // counts when instantiated with animated: true, so probing without the From 0f709617e53aa7aa84812bb835d12f4afcdc967d Mon Sep 17 00:00:00 2001 From: adityaverm-a Date: Tue, 2 Jun 2026 15:00:25 +0530 Subject: [PATCH 4/5] fix: normalize Content-Type with parameters and varying case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../image-processing/origin-fetcher.test.ts | 21 ++++++++++ .../image-processing/origin-fetcher.ts | 6 ++- .../connection-manager.test.ts | 38 ++++++++++++++++++- .../connection-manager/connection-manager.ts | 27 +++++++++---- 4 files changed, 82 insertions(+), 10 deletions(-) diff --git a/source/container/src/services/image-processing/origin-fetcher.test.ts b/source/container/src/services/image-processing/origin-fetcher.test.ts index 6b5c835d3..20b066651 100644 --- a/source/container/src/services/image-processing/origin-fetcher.test.ts +++ b/source/container/src/services/image-processing/origin-fetcher.test.ts @@ -123,6 +123,27 @@ describe('OriginFetcher', () => { expect(() => fetcher['validateImageMagicNumbers'](icoBuffer, 'image/png', 'https://example.com/test.png')) .toThrow('Content-Type mismatch'); }); + + it('should still apply magic-number validation when Content-Type includes parameters', () => { + // Without parameter stripping, the contentTypeToFormat lookup returns + // undefined for `image/png; charset=utf-8` and validation is silently + // skipped — letting a content/buffer mismatch slip through to Sharp. + const pngBuffer = Buffer.from([0x89, 0x50, 0x4E, 0x47]); + expect(() => fetcher['validateImageMagicNumbers'](pngBuffer, 'image/png; charset=utf-8', 'https://example.com/test.png')) + .not.toThrow(); + }); + + it('should reject content-type mismatch when JPEG bytes are served as parameterized image/png', () => { + const jpegBuffer = Buffer.from([0xFF, 0xD8, 0xFF, 0xE0]); + expect(() => fetcher['validateImageMagicNumbers'](jpegBuffer, 'image/png; charset=utf-8', 'https://example.com/test.png')) + .toThrow('Content-Type mismatch'); + }); + + it('should treat uppercase parameterized Content-Type as a known format', () => { + const pngBuffer = Buffer.from([0x89, 0x50, 0x4E, 0x47]); + expect(() => fetcher['validateImageMagicNumbers'](pngBuffer, 'IMAGE/PNG; charset=utf-8', 'https://example.com/test.png')) + .not.toThrow(); + }); }); describe('fetchImage', () => { diff --git a/source/container/src/services/image-processing/origin-fetcher.ts b/source/container/src/services/image-processing/origin-fetcher.ts index 0b1c41fcd..5497b053c 100644 --- a/source/container/src/services/image-processing/origin-fetcher.ts +++ b/source/container/src/services/image-processing/origin-fetcher.ts @@ -205,7 +205,11 @@ export class OriginFetcher { } if (contentType) { - const expectedFormat = contentTypeToFormat[contentType.toLowerCase()]; + // Strip parameters (`image/png; charset=utf-8` → `image/png`) before + // the lookup. Without this, parameterized headers silently bypass magic- + // number validation, allowing mismatched buffers through. + const bareContentType = contentType.split(';')[0].trim().toLowerCase(); + const expectedFormat = contentTypeToFormat[bareContentType]; // If no expectedFormat found, skip magic number validation if (expectedFormat) { if (!detectedFormat) { diff --git a/source/container/src/services/request-resolver/connection-manager/connection-manager.test.ts b/source/container/src/services/request-resolver/connection-manager/connection-manager.test.ts index 012ca54b1..ecee1018b 100644 --- a/source/container/src/services/request-resolver/connection-manager/connection-manager.test.ts +++ b/source/container/src/services/request-resolver/connection-manager/connection-manager.test.ts @@ -91,7 +91,10 @@ describe('ConnectionManager', () => { }); }); - it('should accept image content type with charset', async () => { + it('should normalize content-type with parameters before storing on imageRequest', async () => { + // Downstream consumers (image-processor ICO/animation guards, auto-optimizer + // FORMAT_MAPPING) use exact-match lookups. Parameters and casing on the raw + // header would silently bypass those guards — normalize at this boundary. mockFetch.mockResolvedValue({ ok: true, status: 200, @@ -99,7 +102,38 @@ describe('ConnectionManager', () => { }); await connectionManager.validateOriginUrl('https://example.com/image.jpg', mockImageRequest); - expect(mockImageRequest.sourceImageContentType).toBe('image/jpeg; charset=utf-8'); + expect(mockImageRequest.sourceImageContentType).toBe('image/jpeg'); + }); + + it('should normalize uppercase content-type to lowercase', async () => { + mockFetch.mockResolvedValue({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': 'IMAGE/PNG' }) + }); + + await connectionManager.validateOriginUrl('https://example.com/image.png', mockImageRequest); + expect(mockImageRequest.sourceImageContentType).toBe('image/png'); + }); + + it('should normalize content-type with extra whitespace and parameters', async () => { + mockFetch.mockResolvedValue({ + ok: true, + status: 200, + headers: new Headers({ 'content-type': ' image/x-icon ; charset=binary ' }) + }); + + await connectionManager.validateOriginUrl('https://example.com/favicon.ico', mockImageRequest); + expect(mockImageRequest.sourceImageContentType).toBe('image/x-icon'); + }); + + it('should normalize content-type from S3 with parameters', async () => { + (S3UrlHelper.isS3Url as jest.Mock).mockReturnValue(true); + (S3UrlHelper.parseS3Url as jest.Mock).mockReturnValue({ bucket: 'test-bucket', key: 'test-key.png' }); + mockS3Send.mockResolvedValue({ ContentType: 'image/webp; charset=utf-8' }); + + await connectionManager.validateOriginUrl('https://bucket.s3.amazonaws.com/key.png', mockImageRequest); + expect(mockImageRequest.sourceImageContentType).toBe('image/webp'); }); it('should pass clientHeaders to fetch', async () => { diff --git a/source/container/src/services/request-resolver/connection-manager/connection-manager.ts b/source/container/src/services/request-resolver/connection-manager/connection-manager.ts index 011265645..8cb0b7837 100644 --- a/source/container/src/services/request-resolver/connection-manager/connection-manager.ts +++ b/source/container/src/services/request-resolver/connection-manager/connection-manager.ts @@ -12,12 +12,25 @@ export class ConnectionManager { private static readonly TIMEOUT_MS = 5000; private readonly s3Client = new S3Client(getOptions()); + // Validates a Content-Type that has already been normalized (stripped of + // parameters + lowercased). Operates on the normalized form so uppercase + // and parameterized headers are accepted instead of incorrectly rejected. private validateContentType(contentType: string | undefined): void { - if (!contentType?.split(';')[0].trim().startsWith('image/')) { + if (!contentType?.startsWith('image/')) { throw new ConnectionError('Invalid content type', `Origin does not serve image content. Content-Type: ${contentType}`, 400, 'INVALID_FORMAT'); } } + // Origin Content-Type can include parameters (e.g. `image/jpeg; charset=utf-8`) + // and varying case. Downstream lookups use exact-match Set / object key + // comparisons, so we strip the parameter list, trim, and lowercase here at + // the boundary — every consumer sees a single canonical form. + private static normalizeContentType(contentType: string | undefined): string | undefined { + if (!contentType) return undefined; + const bare = contentType.split(';')[0].trim().toLowerCase(); + return bare || undefined; + } + private async validateS3Origin(url: string, imageRequest: ImageProcessingRequest): Promise { try { const { bucket, key } = S3UrlHelper.parseS3Url(url); @@ -36,10 +49,10 @@ export class ConnectionManager { const command = new HeadObjectCommand(commandInput); const response = await this.s3Client.send(command); - const contentType = response.ContentType; - this.validateContentType(contentType); + const normalized = ConnectionManager.normalizeContentType(response.ContentType); + this.validateContentType(normalized); if (imageRequest) { - imageRequest.sourceImageContentType = contentType; + imageRequest.sourceImageContentType = normalized; } } catch (error: any) { const statusCode = error?.$metadata?.httpStatusCode; @@ -87,10 +100,10 @@ export class ConnectionManager { throw new ConnectionError('Origin validation failed', `Origin returned status ${status} for ${url}`, 502, 'BAD_GATEWAY'); } - const contentType = response.headers.get('content-type') ?? undefined; - this.validateContentType(contentType); + const normalized = ConnectionManager.normalizeContentType(response.headers.get('content-type') ?? undefined); + this.validateContentType(normalized); if (imageRequest) { - imageRequest.sourceImageContentType = contentType; + imageRequest.sourceImageContentType = normalized; } } catch (error) { clearTimeout(timeoutId); From 80722f91bbaba38c87455eb9c8221625c1c2fd5e Mon Sep 17 00:00:00 2001 From: adityaverm-a Date: Tue, 2 Jun 2026 15:16:41 +0530 Subject: [PATCH 5/5] fix: tighten isAnimatedPng (chunk parser) and isValidImageContentType (exact match) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../image-processor.service.test.ts | 51 +++++++++++++++++++ .../image-processor.service.ts | 28 ++++++++-- .../image-processing/origin-fetcher.test.ts | 19 +++++++ .../image-processing/origin-fetcher.ts | 37 ++++++++------ 4 files changed, 114 insertions(+), 21 deletions(-) diff --git a/source/container/src/services/image-processing/image-processor.service.test.ts b/source/container/src/services/image-processing/image-processor.service.test.ts index f1792224e..d769b15fd 100644 --- a/source/container/src/services/image-processing/image-processor.service.test.ts +++ b/source/container/src/services/image-processing/image-processor.service.test.ts @@ -411,6 +411,36 @@ describe('ImageProcessorService', () => { // No acTL chunk — straight to IDAT in real files ]); + // PNG with a tEXt chunk that uses 'acTL' as its keyword content. A naive + // substring scan would false-positive here; the chunk parser must not. + // tEXt chunk: length(4) + 'tEXt' + data(length) + crc(4) + const PNG_WITH_TEXT_ACTL_KEYWORD = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + Buffer.from([0x00, 0x00, 0x00, 0x0d]), + Buffer.from('IHDR', 'ascii'), + Buffer.alloc(13 + 4), + // tEXt chunk carrying the literal bytes 'acTL' as data + Buffer.from([0x00, 0x00, 0x00, 0x04]), // tEXt length = 4 bytes + Buffer.from('tEXt', 'ascii'), + Buffer.from('acTL', 'ascii'), // 4 bytes of data — the false-positive trap + Buffer.alloc(4), // crc + ]); + + // IHDR -> IDAT -> trailing 'acTL' bytes. APNG spec requires acTL to appear + // BEFORE the first IDAT — the parser must stop scanning at IDAT. + const PNG_WITH_ACTL_AFTER_IDAT = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + Buffer.from([0x00, 0x00, 0x00, 0x0d]), + Buffer.from('IHDR', 'ascii'), + Buffer.alloc(13 + 4), + Buffer.from([0x00, 0x00, 0x00, 0x08]), + Buffer.from('IDAT', 'ascii'), + Buffer.alloc(8 + 4), + Buffer.from([0x00, 0x00, 0x00, 0x08]), + Buffer.from('acTL', 'ascii'), + Buffer.alloc(8 + 4), + ]); + it('should detect acTL chunk in APNG buffer', () => { expect(ImageProcessorService['isAnimatedPng'](APNG_HEADER)).toBe(true); }); @@ -419,6 +449,27 @@ describe('ImageProcessorService', () => { expect(ImageProcessorService['isAnimatedPng'](STATIC_PNG_HEADER)).toBe(false); }); + it('should not false-positive when acTL appears as tEXt chunk data', () => { + // A naive substring scan over the buffer would find "acTL" and incorrectly + // mark the file as animated. + expect(ImageProcessorService['isAnimatedPng'](PNG_WITH_TEXT_ACTL_KEYWORD)).toBe(false); + }); + + it('should stop scanning at IDAT (acTL after IDAT is spec-invalid)', () => { + expect(ImageProcessorService['isAnimatedPng'](PNG_WITH_ACTL_AFTER_IDAT)).toBe(false); + }); + + it('should handle truncated buffers without infinite looping', () => { + // Length field claims more bytes than exist — must terminate, not loop or throw. + const truncated = Buffer.concat([ + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), + Buffer.from([0xff, 0xff, 0xff, 0xff]), // length claims 4GB + Buffer.from('IHDR', 'ascii'), + Buffer.alloc(2), // truncated data + ]); + expect(ImageProcessorService['isAnimatedPng'](truncated)).toBe(false); + }); + it('should pass APNG buffer through unchanged when transformations are present', async () => { jest.spyOn(service['originFetcher'], 'fetchImage').mockResolvedValue({ buffer: APNG_HEADER, diff --git a/source/container/src/services/image-processing/image-processor.service.ts b/source/container/src/services/image-processing/image-processor.service.ts index bfd2be15e..917df6187 100644 --- a/source/container/src/services/image-processing/image-processor.service.ts +++ b/source/container/src/services/image-processing/image-processor.service.ts @@ -38,12 +38,30 @@ export class ImageProcessorService { // APNG is a PNG with an 'acTL' chunk preceding the first IDAT. Sharp 0.34.x // cannot read APNG as multi-frame nor write animated PNG output, so any - // pipeline operation silently strips animation. We sniff a bounded prefix of - // the buffer — acTL is always near the top of the file in practice. + // pipeline operation silently strips animation. + // + // We walk the actual chunk stream rather than substring-sniffing. A naive + // indexOf('acTL') can false-positive on a tEXt chunk that happens to use + // 'acTL' as a keyword, or on coincidental bytes inside compressed IDAT data — + // and would then silently passthrough a perfectly resizable static PNG. private static isAnimatedPng(buffer: Buffer): boolean { - const SCAN_LIMIT = 4096; - const head = buffer.subarray(0, Math.min(buffer.length, SCAN_LIMIT)); - return head.indexOf('acTL', 0, 'ascii') !== -1; + const PNG_SIGNATURE_LENGTH = 8; + // Bound the walk to a sane limit; acTL must appear before IDAT in real + // APNG files, which in practice puts it well within the first few KB. + const SCAN_LIMIT = 64 * 1024; + const end = Math.min(buffer.length, SCAN_LIMIT); + let i = PNG_SIGNATURE_LENGTH; + // Each chunk: length(4) + type(4) + data(length) + crc(4) + while (i + 8 <= end) { + const length = buffer.readUInt32BE(i); + const type = buffer.toString('ascii', i + 4, i + 8); + if (type === 'acTL') return true; + if (type === 'IDAT') return false; // per spec, acTL must precede IDAT + i += 12 + length; + // Guard against truncation / length-field corruption causing infinite loop. + if (length > end || i < 0) return false; + } + return false; } public static getInstance(): ImageProcessorService { diff --git a/source/container/src/services/image-processing/origin-fetcher.test.ts b/source/container/src/services/image-processing/origin-fetcher.test.ts index 20b066651..ecf29550b 100644 --- a/source/container/src/services/image-processing/origin-fetcher.test.ts +++ b/source/container/src/services/image-processing/origin-fetcher.test.ts @@ -32,6 +32,25 @@ describe('OriginFetcher', () => { expect(fetcher['isValidImageContentType']('image/x-icon')).toBe(true); expect(fetcher['isValidImageContentType']('image/vnd.microsoft.icon')).toBe(true); }); + + it('should accept parameterized content types (strip params)', () => { + expect(fetcher['isValidImageContentType']('image/jpeg; charset=utf-8')).toBe(true); + expect(fetcher['isValidImageContentType'](' image/png ; q=0.9 ')).toBe(true); + expect(fetcher['isValidImageContentType']('image/x-icon; charset=binary')).toBe(true); + }); + + it('should reject substring-match impostors that the previous includes() check would allow', () => { + // Substring matching previously accepted these; exact-match closes the hole. + expect(fetcher['isValidImageContentType']('application/image/jpeg')).toBe(false); + expect(fetcher['isValidImageContentType']('image/pngfoo')).toBe(false); + expect(fetcher['isValidImageContentType']('image/jpegabc')).toBe(false); + expect(fetcher['isValidImageContentType']('foo image/png bar')).toBe(false); + }); + + it('should reject unknown image subtypes', () => { + expect(fetcher['isValidImageContentType']('image/svg+xml')).toBe(false); + expect(fetcher['isValidImageContentType']('image/bmp')).toBe(false); + }); }); describe('error handling', () => { diff --git a/source/container/src/services/image-processing/origin-fetcher.ts b/source/container/src/services/image-processing/origin-fetcher.ts index 5497b053c..eab1778c7 100644 --- a/source/container/src/services/image-processing/origin-fetcher.ts +++ b/source/container/src/services/image-processing/origin-fetcher.ts @@ -145,23 +145,28 @@ export class OriginFetcher { + // ICO is accepted but bypasses the Sharp pipeline downstream (see + // ImageProcessorService — Sharp has no ICO decoder, and ICO is a + // multi-resolution bundle that must not be re-encoded). + private static readonly VALID_IMAGE_CONTENT_TYPES = new Set([ + 'image/jpeg', + 'image/jpg', + 'image/png', + 'image/webp', + 'image/gif', + 'image/tiff', + 'image/avif', + 'image/heif', + 'image/x-icon', + 'image/vnd.microsoft.icon', + ]); + + // Strip Content-Type parameters and lowercase, then exact-match against the + // allowlist. Substring matching (the previous behavior) would let through + // `application/image/jpeg`, `image/pngfoo`, and other malformed types. private isValidImageContentType(contentType: string): boolean { - const validTypes = [ - 'image/jpeg', - 'image/jpg', - 'image/png', - 'image/webp', - 'image/gif', - 'image/tiff', - 'image/avif', - 'image/heif', - // ICO is accepted but bypasses the Sharp pipeline downstream (see - // ImageProcessorService — Sharp has no ICO decoder, and ICO is a - // multi-resolution bundle that must not be re-encoded). - 'image/x-icon', - 'image/vnd.microsoft.icon', - ]; - return validTypes.some(type => contentType.toLowerCase().includes(type)); + const bare = contentType.split(';')[0].trim().toLowerCase(); + return OriginFetcher.VALID_IMAGE_CONTENT_TYPES.has(bare); } private validateImageMagicNumbers(buffer: Buffer, contentType: string | undefined, url: string): void {