From 605a35c36ba16e77e178396f4453b8da7c59c4f9 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Thu, 22 Oct 2026 12:21:21 -0400 Subject: [PATCH] [img] hardening --- apps/api/src/routes/avatar.ts | 2 +- apps/api/src/routes/images.ts | 5 +- apps/api/src/test/integration/api.test.ts | 14 ++---- apps/img/src/img.app.ts | 39 +++++++++++++--- apps/img/src/test/integration/api.test.ts | 46 +++++++++++++++++++ apps/storage/src/test/integration/api.test.ts | 2 +- packages/hono-helpers/src/helpers/image.ts | 20 +++----- 7 files changed, 94 insertions(+), 34 deletions(-) diff --git a/apps/api/src/routes/avatar.ts b/apps/api/src/routes/avatar.ts index 694b043..fe75309 100644 --- a/apps/api/src/routes/avatar.ts +++ b/apps/api/src/routes/avatar.ts @@ -482,7 +482,7 @@ export const avatarRoutes = new Hono({ strict: false }) '`avatar-item//-thumb.png` and `avatar-item//-design.png`; those ' + 'keys are the `ThumbnailImageFilename` / `DesignFilename` on the row. Each file is ' + 'identified by its bytes — the client posts them under `.bin` names — and must be a ' + - 'PNG, JPEG, GIF, WebP or BMP; the key’s extension and the stored content-type come ' + + 'PNG or JPEG; the key’s extension and the stored content-type come ' + 'from that signature, and anything else is refused with a 400 before either file is ' + 'written.\n\n' + '`Price` is capped at 1000 tokens (`MAX_PLAYER_ITEM_PRICE`); one above it, or below ' + diff --git a/apps/api/src/routes/images.ts b/apps/api/src/routes/images.ts index 38e08fa..c2d2265 100644 --- a/apps/api/src/routes/images.ts +++ b/apps/api/src/routes/images.ts @@ -220,10 +220,9 @@ export const imageRoutes = new Hono({ strict: false }) '`savedImageType` of 4 (profile thumbnail) additionally becomes the account’s ' + 'avatar, persisted on the account row.\n\n' + 'The file is identified by its bytes, not by the part’s filename or content-type: ' + - 'it must be a PNG, JPEG, GIF, WebP or BMP, and the stored key’s extension and ' + + 'it must be a PNG or JPEG, and the stored key’s extension and ' + 'content-type come from that signature. Anything else is refused with a 400 — the ' + - '`img` worker serves the object back under its stored type, so an HTML body under ' + - 'an image name would otherwise be a page on the image origin.', + '`img` worker serves only those two formats, so it would never display.', security: AUTHED, requestBody: form(UploadImageRequest, 'The image file plus its metadata'), responses: { diff --git a/apps/api/src/test/integration/api.test.ts b/apps/api/src/test/integration/api.test.ts index 9a8b752..7412943 100644 --- a/apps/api/src/test/integration/api.test.ts +++ b/apps/api/src/test/integration/api.test.ts @@ -5017,14 +5017,10 @@ describe('custom avatar items', () => { // Both uploads landed in the image bucket under those keys. const thumb = await env.IMAGES.get(body.Value.ThumbnailImageFilename as string) - expect(new Uint8Array((await thumb!.arrayBuffer()) as ArrayBuffer)).toEqual( - new Uint8Array([1, 2, 3]) - ) + expect(new Uint8Array((await thumb!.arrayBuffer()) as ArrayBuffer)).toEqual(PNG_BYTES) expect(thumb!.httpMetadata?.contentType).toBe('image/png') const design = await env.IMAGES.get(body.Value.DesignFilename as string) - expect(new Uint8Array((await design!.arrayBuffer()) as ArrayBuffer)).toEqual( - new Uint8Array([4, 5, 6]) - ) + expect(new Uint8Array((await design!.arrayBuffer()) as ArrayBuffer)).toEqual(PNG_BYTES) // The row is the record as JSON, with the queried fields generated off it. const row = await env.DB.prepare( @@ -5384,14 +5380,14 @@ describe('custom avatar items', () => { expect(badThumbnail.status).toBe(400) expect(await badThumbnail.json()).toMatchObject({ Success: false, - Error: 'thumbnailImage is not a PNG, JPEG, GIF, WebP or BMP image', + Error: 'thumbnailImage is not a PNG or JPEG image', }) const badDesign = await upload(PNG_BYTES, html) expect(badDesign.status).toBe(400) expect(await badDesign.json()).toMatchObject({ Success: false, - Error: 'design is not a PNG, JPEG, GIF, WebP or BMP image', + Error: 'design is not a PNG or JPEG image', }) const row = await env.DB.prepare( @@ -6974,7 +6970,7 @@ describe('images', () => { body: fd, }) expect(res.status).toBe(400) - expect(await res.json()).toEqual({ error: 'image is not a PNG, JPEG, GIF, WebP or BMP image' }) + expect(await res.json()).toEqual({ error: 'image is not a PNG or JPEG image' }) expect((await env.IMAGES.list()).objects).toHaveLength(objectsBefore) }) diff --git a/apps/img/src/img.app.ts b/apps/img/src/img.app.ts index 8f6c281..7019f4b 100644 --- a/apps/img/src/img.app.ts +++ b/apps/img/src/img.app.ts @@ -54,6 +54,18 @@ function isPng(bytes: Uint8Array): boolean { return PNG_SIGNATURE.every((b, i) => bytes[i] === b) } +/** The only content-types served from R2. */ +const SERVED_CONTENT_TYPES = new Set(['image/png', 'image/jpeg']) + +/** + * The stored content-type, bare and lower-cased, when it is one this worker serves — + * otherwise `undefined`. + */ +function servedContentType(stored: string | undefined): string | undefined { + const type = stored?.split(';')[0]?.trim().toLowerCase() + return type && SERVED_CONTENT_TYPES.has(type) ? type : undefined +} + /** A requested transform, from `?width=`/`?height=`/`?cropSquare=1`. At least one applies. */ interface Transform { width?: number @@ -529,21 +541,36 @@ app.get( // bytes that are not the range asked for — ask R2 for the range only when we are // going to hand its bytes straight back. R2 parses the header itself; see // writeContentRange() below for why it is never answered with a bare 200. - const range = needsBody(transform, signing) ? undefined : c.req.raw.headers + const buffered = needsBody(transform, signing) + const range = buffered ? undefined : c.req.raw.headers const object = await bucket.get(objectKey, { ...(ifNoneMatch ? { onlyIf: { etagDoesNotMatch: ifNoneMatch } } : {}), ...(range ? { range } : {}), }) - if (!object) { - // Missing from both static and R2 → serve the bundled DefaultProfileImage.jpg - // static asset so clients still get a valid image instead of a 404. Honour - // `?sig=p1` the same way so the fallback is signed like any other image. + + // Missing from both static and R2, or not a PNG/JPEG → serve the bundled + // DefaultProfileImage.jpg static asset so clients still get a valid image instead + // of a 404. Honour `?sig=p1` the same way so the fallback is signed like any other. + const serveFallback = async () => { const asset = await c.env.ASSETS.fetch(new URL(FALLBACK_ASSET_PATH, c.req.url)) return serveStaticAsset(c.env, asset, transform, signing) } + if (!object) return serveFallback() + + // Only a PNG or JPEG is ever served, judged by the object's stored content-type. + // Uploads store the type their bytes name (`sniffImage` in every writer), so the + // stored type is trusted rather than re-reading the bytes here; anything else — + // an older upload stored under the uploader's `text/html` claim, say — is treated + // as missing. + const storedType = servedContentType(object.httpMetadata?.contentType) + if (!storedType) { + if ('body' in object) await object.body.cancel() + return serveFallback() + } const headers = new Headers() object.writeHttpMetadata(headers) + headers.set('content-type', storedType) headers.set('etag', object.httpEtag) headers.set('cache-control', CACHE_CONTROL) @@ -554,7 +581,7 @@ app.get( // ever rides a response that actually carries bytes. applyStubSignature(headers, signing) - if (needsBody(transform, signing)) { + if (buffered) { const bytes = await object.arrayBuffer() return finalizeImage(c.env, bytes, headers, transform, signing) } diff --git a/apps/img/src/test/integration/api.test.ts b/apps/img/src/test/integration/api.test.ts index e22fd1a..7ac952a 100644 --- a/apps/img/src/test/integration/api.test.ts +++ b/apps/img/src/test/integration/api.test.ts @@ -107,6 +107,52 @@ describe('img endpoints', () => { expect(notModified.headers.get('x-content-type-options')).toBe('nosniff') }) + // Only an object stored as PNG or JPEG is ever served. Anything else gets the + // fallback, as though the key were missing. + it('serves the fallback, not the body, for an object not stored as PNG or JPEG', async () => { + const fallback = new Uint8Array( + await (await SELF.fetch(`${ORIGIN}/does-not-exist-anywhere.jpg`)).arrayBuffer() + ) + const junk = [ + { key: 'gate/page.jpg', body: '', type: 'text/html' }, + { key: 'gate/octet.jpg', body: IMAGE_BYTES, type: 'application/octet-stream' }, + { key: 'gate/anim.gif', body: 'GIF89a\x01\x00\x01\x00', type: 'image/gif' }, + ] + for (const { key, body, type } of junk) { + await env.IMAGES.put(key, body, { httpMetadata: { contentType: type } }) + for (const query of ['', '?width=128', '?sig=p1']) { + const res = await SELF.fetch(`${ORIGIN}/${key}${query}`) + expect(res.status, key + query).toBe(200) + expect(res.headers.get('content-type'), key + query).toBe('image/jpeg') + if (query === '') expect(new Uint8Array(await res.arrayBuffer()), key).toEqual(fallback) + else await res.arrayBuffer() + } + } + + // A range into a non-image is refused too, not just the whole-object read. + const ranged = await SELF.fetch(`${ORIGIN}/gate/page.jpg`, { headers: { range: 'bytes=6-20' } }) + expect(ranged.status).toBe(200) + expect(new Uint8Array(await ranged.arrayBuffer())).toEqual(fallback) + }) + + // The served type is the stored one, normalised — never a parameter riding on it. + it('serves a stored image type bare, and a range of it', async () => { + await env.IMAGES.put('gate/params.jpg', IMAGE_BYTES, { + httpMetadata: { contentType: 'IMAGE/JPEG; charset=utf-8' }, + }) + const res = await SELF.fetch(`${ORIGIN}/gate/params.jpg`) + expect(res.status).toBe(200) + expect(res.headers.get('content-type')).toBe('image/jpeg') + expect(new Uint8Array(await res.arrayBuffer())).toEqual(IMAGE_BYTES) + + const ranged = await SELF.fetch(`${ORIGIN}/gate/params.jpg`, { + headers: { range: 'bytes=4-7' }, + }) + expect(ranged.status).toBe(206) + expect(ranged.headers.get('content-type')).toBe('image/jpeg') + expect(new Uint8Array(await ranged.arrayBuffer())).toEqual(IMAGE_BYTES.slice(4, 8)) + }) + it('streams an image stored in R2 with its content type', async () => { const res = await SELF.fetch(`${ORIGIN}/${R2_KEY}`) expect(res.status).toBe(200) diff --git a/apps/storage/src/test/integration/api.test.ts b/apps/storage/src/test/integration/api.test.ts index 4e91877..03c4cb4 100644 --- a/apps/storage/src/test/integration/api.test.ts +++ b/apps/storage/src/test/integration/api.test.ts @@ -189,7 +189,7 @@ it('POST /upload refuses an Image (FileType 3) whose bytes are not an image', as body: form, }) expect(res.status).toBe(400) - expect(await res.json()).toEqual({ error: 'file is not a PNG, JPEG, GIF, WebP or BMP image' }) + expect(await res.json()).toEqual({ error: 'file is not a PNG or JPEG image' }) }) it('POST /upload stores an Image (FileType 3) under the type its bytes say, not the claim', async () => { diff --git a/packages/hono-helpers/src/helpers/image.ts b/packages/hono-helpers/src/helpers/image.ts index bfcb9d1..6a9c63f 100644 --- a/packages/hono-helpers/src/helpers/image.ts +++ b/packages/hono-helpers/src/helpers/image.ts @@ -22,9 +22,11 @@ interface Signature extends SniffedImage { parts: Array<{ offset: number; bytes: number[] }> } -const ascii = (s: string): number[] => Array.from(s, (ch) => ch.charCodeAt(0)) - -/** The formats an upload may be — the set the `img` worker decodes and resizes. */ +/** + * The formats an upload may be, and the only ones the `img` worker serves — the two it + * re-encodes to and the only two the client has been seen to post. Keep upload and serve + * in step: a format accepted here but not served there uploads fine and never displays. + */ const SIGNATURES: Signature[] = [ { contentType: 'image/png', @@ -36,20 +38,10 @@ const SIGNATURES: Signature[] = [ extension: '.jpg', parts: [{ offset: 0, bytes: [0xff, 0xd8, 0xff] }], }, - { contentType: 'image/gif', extension: '.gif', parts: [{ offset: 0, bytes: ascii('GIF8') }] }, - { - contentType: 'image/webp', - extension: '.webp', - parts: [ - { offset: 0, bytes: ascii('RIFF') }, - { offset: 8, bytes: ascii('WEBP') }, - ], - }, - { contentType: 'image/bmp', extension: '.bmp', parts: [{ offset: 0, bytes: ascii('BM') }] }, ] /** The formats `sniffImage` accepts, for an error message. */ -export const SNIFFED_IMAGE_FORMATS = 'PNG, JPEG, GIF, WebP or BMP' +export const SNIFFED_IMAGE_FORMATS = 'PNG or JPEG' /** * Identify an image by its signature, or `undefined` when the bytes are not one of the