fix(worker): handle data: URL images in processImage - #194
Conversation
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:<mediatype>[;base64],<payload> 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 playfulprogramming#28
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| ); | ||
| } | ||
|
|
||
| async function processImage( |
There was a problem hiding this comment.
This looks good overall, though I have an organizational nit - processImage and processDataImage have almost the same execution flow (computing a URL hash, branching if the image is an svg, otherwise passing into uploadRasterImage). That makes this a bit confusing to follow.
I find that early returns are great for asserting simple preconditions, but the url.protocol === "data:" / isSvg branches are more involved.
It would be nice to flatten this logic and have one readImage(url, signal) that returns a consistent { body: ReadableStream, format: 'svg' | 'png' | 'jpeg' | 'webp' | 'gif' | ..., lastModified?: Date }
readImage can then parse data URLs, detect SVGs, or otherwise fetch an HTTP response & process sharp().metadata() to construct that result.
processImage then uses that result and has one consistent upload implementation - creating urlHash, determining the uploadKey, checking lastModified if available, then passing the stream to uploadSvg or uploadRasterImage accordingly.
That should eliminate a lot of the duplication here.
…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.
2 participants
What
processImageassumed every image URL it received was fetchable over HTTP. Favicons andog:imagebanners are sometimes embedded directly asdata:URLs instead — inline SVG text or base64-encoded raster data — and those got handed straight tofetchAsBot, which has no handling for thedata:scheme and fails with a null body.This adds an early branch in
processImage, keyed offurl.protocol === "data:", that decodes the payload directly instead of making a network request.How
data:<mediatype>[;base64],<payload>shape fromurl.href, noturl.pathname. The WHATWG URL parser treatsdata:as an opaque-path scheme, so an unescaped#in the payload gets silently split off intourl.hash—url.pathnamealone loses everything after it. Covered by the first test below.content-typeheader, which doesn't exist fordata:URLs).svgo.optimize+image/svg+xmlupload logic as the fetch path.Buffer, wrapped in aReadable, and fed through the same sharp metadata/resize/upload pipeline already used for fetched images.compareLastModifiedfreshness check is skipped entirely fordata:URLs — there's no HTTP resource orlast-modifiedheader to compare against. This is safe and idempotent: the upload key is already an md5 hash ofurl.href, and for adata:URL thathrefincludes the actual payload, so identical embedded images naturally hash to the same key and a re-upload is a harmless no-op.uploadSvg,uploadRasterImage) out of the originalprocessImagebody — same steps, same order, same conditionals, just callable from both the fetch branch and the newdata:branch. Flagging this explicitly since it's a judgment call between two things you might want independently; happy to switch to a literal copy-paste (zero-diff on the fetch path) if you'd rather.Tests
New test file (none existed for
processImage.tsbefore):apps/worker/src/tasks/url-metadata/utils/processImage.test.ts#in the payload — this , since it's the case whereurl.pathnametruncates andurl.hrefdoesn't.I don't have the literal text of the issue's reproduced error, so the SVG test uses a representative data URL rather than a verbatim copy — let me know if the real repro string differs meaningfully.
Closes #28