From 53432c31c473c6a2b038bd29dc98d868e8e740c6 Mon Sep 17 00:00:00 2001 From: Brian Bornino Date: Sun, 12 Jul 2026 07:34:36 -0700 Subject: [PATCH 1/2] fix(worker): handle data: URL images in processImage favicons and og:image banners are sometimes embedded directly as data: URLs (inline SVG or base64 raster) rather than linked, and processImage handed those straight to fetchAsBot, which has no data: scheme handling and fails with a null body. Adds an early branch, keyed off url.protocol === "data:", that decodes the payload directly instead of making a network request. Parses the data:[;base64], shape from url.href rather than url.pathname, since the WHATWG URL parser treats data: as an opaque-path scheme and silently truncates url.pathname at an unescaped `#`. SVG payloads reuse the existing svgo.optimize + s3 upload logic; raster payloads are base64-decoded into a Buffer, wrapped in a Readable, and fed through the existing sharp metadata/resize/upload pipeline. The compareLastModified freshness check is skipped for data: URLs (no HTTP resource to compare against) - safe since the upload key is already an md5 hash of url.href, so identical embedded images naturally collide onto the same key. Fixes #28 --- .../url-metadata/utils/processImage.test.ts | 121 ++++++++++ .../tasks/url-metadata/utils/processImage.ts | 215 ++++++++++++++---- 2 files changed, 286 insertions(+), 50 deletions(-) create mode 100644 apps/worker/src/tasks/url-metadata/utils/processImage.test.ts diff --git a/apps/worker/src/tasks/url-metadata/utils/processImage.test.ts b/apps/worker/src/tasks/url-metadata/utils/processImage.test.ts new file mode 100644 index 00000000..f340af64 --- /dev/null +++ b/apps/worker/src/tasks/url-metadata/utils/processImage.test.ts @@ -0,0 +1,121 @@ +import crypto from "crypto"; +import { s3 } from "@playfulprogramming/s3"; +import { type Mock } from "vitest"; +import { mockEndpoint } from "../../../../test-utils/server.ts"; +import { processImages } from "./processImage.ts"; + +function md5(value: string): string { + return crypto.createHash("md5").update(value).digest("hex"); +} + +async function readStreamToString(readable: NodeJS.ReadableStream) { + const chunks: Buffer[] = []; + for await (const chunk of readable) { + chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); + } + return Buffer.concat(chunks).toString("utf-8"); +} + +const onePixelPngBase64 = + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR42mMAAQAABQABoIJXOQAAAABJRU5ErkJggg=="; + +test("decodes an inline percent-encoded SVG data URL without a network request", async () => { + // Matches the shape from issue #28: an SVG data URL whose payload has an + // unescaped `#`. The WHATWG URL parser splits that off into url.hash, so + // url.pathname alone silently loses everything after it - the fix has to + // parse from url.href instead. + const dataUrl = new URL( + "data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 10 10'%3E%3Ccircle fill='#ff0000' cx='5' cy='5' r='5'/%3E%3C/svg%3E", + ); + + const result = await processImages( + [dataUrl], + 24, + "test-bucket", + "remote-icon", + "job-1", + new AbortController().signal, + ); + + const expectedKey = `remote-icon-${md5(dataUrl.href)}.svg`; + expect(result).toEqual({ key: expectedKey }); + + expect(s3.upload).toBeCalledTimes(1); + expect(s3.upload).toBeCalledWith( + "test-bucket", + expectedKey, + "job-1", + expect.anything(), + "image/svg+xml", + ); + + const uploadedStream = (s3.upload as Mock).mock.calls[0][3]; + const uploadedSvg = await readStreamToString(uploadedStream); + expect(uploadedSvg).toContain(" { + const dataUrl = new URL(`data:image/png;base64,${onePixelPngBase64}`); + + const result = await processImages( + [dataUrl], + 896, + "test-bucket", + "remote-banner", + "job-2", + new AbortController().signal, + ); + + const expectedKey = `remote-banner-${md5(dataUrl.href)}.png`; + expect(result).toEqual({ + key: expectedKey, + width: 1, + height: 1, + }); + + expect(s3.upload).toBeCalledWith( + "test-bucket", + expectedKey, + "job-2", + expect.anything(), + "image/png", + ); +}); + +test("still fetches and processes an http(s) image over the network", async () => { + const url = new URL("https://example.test/banner.png"); + mockEndpoint({ + path: url, + body: Uint8Array.from(Buffer.from(onePixelPngBase64, "base64")).buffer, + }); + + const result = await processImages( + [url], + 896, + "test-bucket", + "remote-banner", + "job-3", + new AbortController().signal, + ); + + const expectedKey = `remote-banner-${md5(url.href)}.png`; + expect(result).toEqual({ + key: expectedKey, + width: 1, + height: 1, + }); + + expect(s3.upload).toBeCalledWith( + "test-bucket", + expectedKey, + "job-3", + expect.anything(), + "image/png", + ); +}); diff --git a/apps/worker/src/tasks/url-metadata/utils/processImage.ts b/apps/worker/src/tasks/url-metadata/utils/processImage.ts index 21dca95a..ded33904 100644 --- a/apps/worker/src/tasks/url-metadata/utils/processImage.ts +++ b/apps/worker/src/tasks/url-metadata/utils/processImage.ts @@ -47,57 +47,35 @@ async function compareLastModified( return false; } -async function processImage( +async function uploadSvg( + svg: string, + bucket: string, + uploadKey: string, + tag: string | undefined, +): Promise { + const optimizedSvg = svgo.optimize(svg, { multipass: true }).data; + await s3.upload( + bucket, + uploadKey, + tag, + stream.Readable.from([optimizedSvg]), + "image/svg+xml", + ); +} + +async function uploadRasterImage( + body: NodeJS.ReadableStream, url: URL, + urlHash: string, width: number, bucket: string, key: string, - tag?: string, - signal?: AbortSignal, + tag: string | undefined, + signal: AbortSignal | undefined, + // undefined for sources with no HTTP resource to compare a last-modified + // header against (e.g. data: URLs) - always upload in that case + request: Dispatcher.ResponseData | undefined, ): Promise { - const request = await fetchAsBot({ url, method: "GET", signal }).catch( - (e) => { - console.error(`Error fetching ${url}`, e); - if (e instanceof DOMException && e.name === "TimeoutError") { - throw e; - } - return undefined; - }, - ); - const body = request?.body; - if (!body) { - console.error(`Request body for ${url} is null`); - return undefined; - } - - const urlHash = crypto.createHash("md5").update(url.href).digest("hex"); - - const isSvg = - request.headers["content-type"]?.includes("image/svg") || - (!("content-type" in request.headers) && - path.extname(url.pathname) === ".svg"); - - if (isSvg) { - const uploadKey = `${key}-${urlHash}.svg`; - - if (await compareLastModified(request, bucket, uploadKey, signal)) { - console.log(`Skipping ${uploadKey}, as it has already been stored.`); - await body.dump(); - } else { - const svg = await body.text(); - const optimizedSvg = svgo.optimize(svg, { multipass: true }).data; - await s3.upload( - bucket, - uploadKey, - tag, - stream.Readable.from([optimizedSvg]), - "image/svg+xml", - ); - } - - return { key: uploadKey }; - } - const pipeline = sharp(); const metadataStream = body.pipe(pipeline); const metadata = await Promise.race([ @@ -107,7 +85,6 @@ async function processImage( if (!metadata || !metadata.format) { console.error(`Image format for ${url} could not be found.`); - await body.dump(); return undefined; } @@ -119,7 +96,11 @@ async function processImage( ? Math.round(metadata.height * (transformWidth / metadata.width)) : undefined; - if (await compareLastModified(request, bucket, uploadKey, signal)) { + const alreadyStored = + request !== undefined && + (await compareLastModified(request, bucket, uploadKey, signal)); + + if (alreadyStored) { console.log(`Skipping ${uploadKey}, as it has already been stored.`); metadataStream.destroy(); } else { @@ -135,8 +116,6 @@ async function processImage( ); } - await body.dump(); - return { key: uploadKey, width: transformWidth, @@ -144,6 +123,142 @@ async function processImage( }; } +interface ParsedDataUrl { + mediaType: string; + isBase64: boolean; + payload: string; +} + +// The WHATWG URL parser treats `data:` as an opaque-path scheme, so an +// unescaped `#` in the payload gets split off into url.hash and url.pathname +// alone silently loses everything after it. url.href keeps the full string +// intact, so parse the data:[;base64], shape from there. +function parseDataUrl(url: URL): ParsedDataUrl | undefined { + const href = url.href; + const commaIndex = href.indexOf(","); + if (!href.startsWith("data:") || commaIndex === -1) { + return undefined; + } + + const meta = href.slice("data:".length, commaIndex); + const payload = href.slice(commaIndex + 1); + const isBase64 = /;base64$/i.test(meta); + const mediaType = + (isBase64 ? meta.slice(0, -";base64".length) : meta) || + "text/plain;charset=US-ASCII"; + + return { mediaType, isBase64, payload }; +} + +async function processDataUrlImage( + url: URL, + width: number, + bucket: string, + key: string, + tag: string | undefined, + signal: AbortSignal | undefined, +): Promise { + const parsed = parseDataUrl(url); + if (!parsed) { + console.error(`Unable to parse data URL ${url}`); + return undefined; + } + + const urlHash = crypto.createHash("md5").update(url.href).digest("hex"); + const isSvg = parsed.mediaType.includes("image/svg"); + + if (isSvg) { + const svg = parsed.isBase64 + ? Buffer.from(parsed.payload, "base64").toString("utf-8") + : decodeURIComponent(parsed.payload); + + const uploadKey = `${key}-${urlHash}.svg`; + await uploadSvg(svg, bucket, uploadKey, tag); + return { key: uploadKey }; + } + + const buffer = parsed.isBase64 + ? Buffer.from(parsed.payload, "base64") + : Buffer.from(decodeURIComponent(parsed.payload), "utf-8"); + + const body = stream.Readable.from(buffer); + + return uploadRasterImage( + body, + url, + urlHash, + width, + bucket, + key, + tag, + signal, + undefined, + ); +} + +async function processImage( + url: URL, + width: number, + bucket: string, + key: string, + tag?: string, + signal?: AbortSignal, +): Promise { + if (url.protocol === "data:") { + return processDataUrlImage(url, width, bucket, key, tag, signal); + } + + const request = await fetchAsBot({ url, method: "GET", signal }).catch( + (e) => { + console.error(`Error fetching ${url}`, e); + if (e instanceof DOMException && e.name === "TimeoutError") { + throw e; + } + return undefined; + }, + ); + const body = request?.body; + if (!body) { + console.error(`Request body for ${url} is null`); + return undefined; + } + + const urlHash = crypto.createHash("md5").update(url.href).digest("hex"); + + const isSvg = + request.headers["content-type"]?.includes("image/svg") || + (!("content-type" in request.headers) && + path.extname(url.pathname) === ".svg"); + + if (isSvg) { + const uploadKey = `${key}-${urlHash}.svg`; + + if (await compareLastModified(request, bucket, uploadKey, signal)) { + console.log(`Skipping ${uploadKey}, as it has already been stored.`); + await body.dump(); + } else { + const svg = await body.text(); + await uploadSvg(svg, bucket, uploadKey, tag); + } + + return { key: uploadKey }; + } + + const result = await uploadRasterImage( + body, + url, + urlHash, + width, + bucket, + key, + tag, + signal, + request, + ); + await body.dump(); + return result; +} + export async function processImages( urls: URL[], width: number, From 3e31f1d9c8aeabb9fa41b91d7688aec90e1f927a Mon Sep 17 00:00:00 2001 From: Brian Bornino Date: Tue, 18 Aug 2026 06:08:02 -0700 Subject: [PATCH 2/2] refactor(worker): flatten processImage into a single readImage-based flow Unify the data: URL and HTTP fetch upload paths behind one readImage() source reader, removing the near-duplicate SVG/raster branching and simplifying compareLastModified to take a plain Date. --- .../url-metadata/utils/processImage.test.ts | 14 +- .../tasks/url-metadata/utils/processImage.ts | 288 ++++++++++-------- 2 files changed, 167 insertions(+), 135 deletions(-) diff --git a/apps/worker/src/tasks/url-metadata/utils/processImage.test.ts b/apps/worker/src/tasks/url-metadata/utils/processImage.test.ts index f340af64..488fc21f 100644 --- a/apps/worker/src/tasks/url-metadata/utils/processImage.test.ts +++ b/apps/worker/src/tasks/url-metadata/utils/processImage.test.ts @@ -1,6 +1,6 @@ import crypto from "crypto"; import { s3 } from "@playfulprogramming/s3"; -import { type Mock } from "vitest"; +import { vi } from "vitest"; import { mockEndpoint } from "../../../../test-utils/server.ts"; import { processImages } from "./processImage.ts"; @@ -28,6 +28,16 @@ test("decodes an inline percent-encoded SVG data URL without a network request", "data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 10 10'%3E%3Ccircle fill='#ff0000' cx='5' cy='5' r='5'/%3E%3C/svg%3E", ); + // setup.ts's default s3.upload mock drains the file stream itself (to let + // sharp's resize pipeline settle), which leaves nothing to read back from + // mock.calls afterward - capture the content as it's uploaded instead. + let uploadedSvg = ""; + vi.mocked(s3.upload).mockImplementationOnce( + async (_bucket, _key, _tag, file) => { + uploadedSvg = await readStreamToString(file as NodeJS.ReadableStream); + }, + ); + const result = await processImages( [dataUrl], 24, @@ -49,8 +59,6 @@ test("decodes an inline percent-encoded SVG data URL without a network request", "image/svg+xml", ); - const uploadedStream = (s3.upload as Mock).mock.calls[0][3]; - const uploadedSvg = await readStreamToString(uploadedStream); expect(uploadedSvg).toContain(", + lastModified: Date, bucket: string, key: string, signal?: AbortSignal, ): Promise { - // If there is a last-modified header, compare it to the header from S3 - const lastModified = request.headers["last-modified"]?.toString(); - if (lastModified) { - const existingFile = await fetchAsBot({ - url: new URL(`${bucket}/${key}`, env.S3_PUBLIC_URL), - method: "HEAD", - skipRobotsCheck: true, - signal, - }).catch(() => undefined); - - if (existingFile && existingFile.statusCode == 200) { - const modS3 = existingFile.headers["last-modified"]?.toString(); - const modExternal = lastModified; - - if (modS3 && modExternal) { - return new Date(modS3) > new Date(modExternal); - } else { - console.error("File exists in S3, but has no last-modified header."); - return false; - } + const existingFile = await fetchAsBot({ + url: new URL(`${bucket}/${key}`, env.S3_PUBLIC_URL), + method: "HEAD", + skipRobotsCheck: true, + signal, + }).catch(() => undefined); + + if (existingFile && existingFile.statusCode == 200) { + const modS3 = existingFile.headers["last-modified"]?.toString(); + + if (modS3) { + return new Date(modS3) > lastModified; + } else { + console.error("File exists in S3, but has no last-modified header."); + return false; } } return false; } +async function readStreamToBuffer(readable: stream.Readable): Promise { + const chunks: Buffer[] = []; + for await (const chunk of readable) { + chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); + } + return Buffer.concat(chunks); +} + async function uploadSvg( - svg: string, + body: stream.Readable, bucket: string, uploadKey: string, tag: string | undefined, -): Promise { +): Promise { + const svg = (await readStreamToBuffer(body)).toString("utf-8"); const optimizedSvg = svgo.optimize(svg, { multipass: true }).data; await s3.upload( bucket, @@ -61,65 +74,44 @@ async function uploadSvg( stream.Readable.from([optimizedSvg]), "image/svg+xml", ); + return { key: uploadKey }; } -async function uploadRasterImage( - body: NodeJS.ReadableStream, - url: URL, - urlHash: string, +function computeRasterDimensions( + image: Pick, width: number, - bucket: string, - key: string, - tag: string | undefined, - signal: AbortSignal | undefined, - // undefined for sources with no HTTP resource to compare a last-modified - // header against (e.g. data: URLs) - always upload in that case - request: Dispatcher.ResponseData | undefined, -): Promise { - const pipeline = sharp(); - const metadataStream = body.pipe(pipeline); - const metadata = await Promise.race([ - setTimeout(10 * 1000).then(() => undefined), - pipeline.metadata().catch(() => undefined), - ]); - - if (!metadata || !metadata.format) { - console.error(`Image format for ${url} could not be found.`); - return undefined; - } - - const uploadKey = `${key}-${urlHash}.${metadata.format}`; - - const transformWidth = Math.min(width, metadata.width || width); +): { width: number; height?: number } { + const transformWidth = Math.min(width, image.width || width); const transformHeight = - metadata.height && metadata.width - ? Math.round(metadata.height * (transformWidth / metadata.width)) + image.height && image.width + ? Math.round(image.height * (transformWidth / image.width)) : undefined; - const alreadyStored = - request !== undefined && - (await compareLastModified(request, bucket, uploadKey, signal)); + return { width: transformWidth, height: transformHeight }; +} - if (alreadyStored) { - console.log(`Skipping ${uploadKey}, as it has already been stored.`); - metadataStream.destroy(); - } else { - const transformer = sharp().resize(transformWidth); - const transformerStream = metadataStream.pipe(transformer); - - await s3.upload( - bucket, - uploadKey, - tag, - transformerStream, - `image/${metadata.format}`, - ); - } +async function uploadRasterImage( + image: ReadImageResult, + dimensions: { width: number; height?: number }, + bucket: string, + uploadKey: string, + tag: string | undefined, +): Promise { + const transformer = sharp().resize(dimensions.width); + const transformerStream = image.body.pipe(transformer); + + await s3.upload( + bucket, + uploadKey, + tag, + transformerStream, + `image/${image.format}`, + ); return { key: uploadKey, - width: transformWidth, - height: transformHeight, + width: dimensions.width, + height: dimensions.height, }; } @@ -150,64 +142,59 @@ function parseDataUrl(url: URL): ParsedDataUrl | undefined { return { mediaType, isBase64, payload }; } -async function processDataUrlImage( +async function readRasterMetadata( + body: stream.Readable, url: URL, - width: number, - bucket: string, - key: string, - tag: string | undefined, - signal: AbortSignal | undefined, -): Promise { +): Promise { + const pipeline = sharp(); + const metadataStream = body.pipe(pipeline); + const metadata = await Promise.race([ + setTimeout(10 * 1000).then(() => undefined), + pipeline.metadata().catch(() => undefined), + ]); + + if (!metadata || !metadata.format) { + console.error(`Image format for ${url} could not be found.`); + return undefined; + } + + return { + body: metadataStream, + format: metadata.format, + width: metadata.width, + height: metadata.height, + }; +} + +async function readDataUrlImage( + url: URL, +): Promise { const parsed = parseDataUrl(url); if (!parsed) { console.error(`Unable to parse data URL ${url}`); return undefined; } - const urlHash = crypto.createHash("md5").update(url.href).digest("hex"); const isSvg = parsed.mediaType.includes("image/svg"); - if (isSvg) { const svg = parsed.isBase64 ? Buffer.from(parsed.payload, "base64").toString("utf-8") : decodeURIComponent(parsed.payload); - const uploadKey = `${key}-${urlHash}.svg`; - await uploadSvg(svg, bucket, uploadKey, tag); - return { key: uploadKey }; + return { body: stream.Readable.from([svg]), format: "svg" }; } const buffer = parsed.isBase64 ? Buffer.from(parsed.payload, "base64") : Buffer.from(decodeURIComponent(parsed.payload), "utf-8"); - const body = stream.Readable.from(buffer); - - return uploadRasterImage( - body, - url, - urlHash, - width, - bucket, - key, - tag, - signal, - undefined, - ); + return readRasterMetadata(stream.Readable.from(buffer), url); } -async function processImage( +async function readFetchedImage( url: URL, - width: number, - bucket: string, - key: string, - tag?: string, - signal?: AbortSignal, -): Promise { - if (url.protocol === "data:") { - return processDataUrlImage(url, width, bucket, key, tag, signal); - } - + signal: AbortSignal | undefined, +): Promise { const request = await fetchAsBot({ url, method: "GET", signal }).catch( (e) => { console.error(`Error fetching ${url}`, e); @@ -223,7 +210,10 @@ async function processImage( return undefined; } - const urlHash = crypto.createHash("md5").update(url.href).digest("hex"); + const lastModifiedHeader = request.headers["last-modified"]?.toString(); + const lastModified = lastModifiedHeader + ? new Date(lastModifiedHeader) + : undefined; const isSvg = request.headers["content-type"]?.includes("image/svg") || @@ -231,32 +221,66 @@ async function processImage( path.extname(url.pathname) === ".svg"); if (isSvg) { - const uploadKey = `${key}-${urlHash}.svg`; + return { body, format: "svg", lastModified }; + } - if (await compareLastModified(request, bucket, uploadKey, signal)) { - console.log(`Skipping ${uploadKey}, as it has already been stored.`); - await body.dump(); - } else { - const svg = await body.text(); - await uploadSvg(svg, bucket, uploadKey, tag); - } + const raster = await readRasterMetadata(body, url); + if (!raster) { + return undefined; + } + + return { ...raster, lastModified }; +} - return { key: uploadKey }; +async function readImage( + url: URL, + signal: AbortSignal | undefined, +): Promise { + if (url.protocol === "data:") { + return readDataUrlImage(url); } - const result = await uploadRasterImage( - body, - url, - urlHash, - width, - bucket, - key, - tag, - signal, - request, - ); - await body.dump(); - return result; + return readFetchedImage(url, signal); +} + +async function processImage( + url: URL, + width: number, + bucket: string, + key: string, + tag?: string, + signal?: AbortSignal, +): Promise { + const image = await readImage(url, signal); + if (!image) { + return undefined; + } + + const urlHash = crypto.createHash("md5").update(url.href).digest("hex"); + const uploadKey = `${key}-${urlHash}.${image.format}`; + + const alreadyStored = + image.lastModified !== undefined && + (await compareLastModified(image.lastModified, bucket, uploadKey, signal)); + + if (alreadyStored) { + console.log(`Skipping ${uploadKey}, as it has already been stored.`); + } + + if (image.format === "svg") { + if (alreadyStored) { + image.body.destroy(); + return { key: uploadKey }; + } + return uploadSvg(image.body, bucket, uploadKey, tag); + } + + const dimensions = computeRasterDimensions(image, width); + if (alreadyStored) { + image.body.destroy(); + return { key: uploadKey, ...dimensions }; + } + return uploadRasterImage(image, dimensions, bucket, uploadKey, tag); } export async function processImages(