🐛 Fixed SVG favicons not rendering in bookmark cards - #29732
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 3m 17s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 18s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 19s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 41s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 16s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 33s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 16s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 8s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-04 07:02:54 UTC
WalkthroughThe oEmbed image service detects SVG images by extension or content. It rejects oversized and gzip-compressed SVGs. It rasterizes accepted SVGs to 256×256 PNGs with a 10-second timeout. Stored SVG results use the Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
319b3ce to
34f6282
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/lib/image/sanitize-svg.js (1)
43-46: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider reusing one JSDOM window across both helpers.
Each call to
sanitizeSvgContentandisRenderableSvgDocumentbuilds a new JSDOM window. The oEmbed path calls both for a single icon, so one bookmark request now builds two windows on the request thread. A lazily created module-level window and DOMPurify instance removes that cost. Keep therequirecalls lazy so boot time is unaffected.This is optional. The per-call construction matches the previous behavior in
upload.js, so it is not a regression.♻️ Proposed lazy shared window
+let cachedWindow; + +const getWindow = () => { + if (!cachedWindow) { + const {JSDOM} = require('jsdom'); + cachedWindow = new JSDOM('').window; + } + return cachedWindow; +};Also applies to: 76-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/lib/image/sanitize-svg.js` around lines 43 - 46, Optionally update the SVG helper initialization so jsdom and DOMPurify are loaded lazily once, then reuse a module-level JSDOM window and DOMPurify instance across sanitizeSvgContent and isRenderableSvgDocument. Preserve lazy require behavior so application boot time remains unaffected and both helpers share the same cached instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ghost/core/core/server/lib/image/sanitize-svg.js`:
- Around line 43-46: Optionally update the SVG helper initialization so jsdom
and DOMPurify are loaded lazily once, then reuse a module-level JSDOM window and
DOMPurify instance across sanitizeSvgContent and isRenderableSvgDocument.
Preserve lazy require behavior so application boot time remains unaffected and
both helpers share the same cached instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a0c1b121-d733-4925-90bf-06dd6590bcbb
📒 Files selected for processing (8)
.changeset/smart-icons-render.mdghost/core/core/server/adapters/storage/S3Storage.tsghost/core/core/server/lib/image/sanitize-svg.jsghost/core/core/server/services/oembed/oembed-service.jsghost/core/core/server/web/api/middleware/upload.jsghost/core/test/unit/server/adapters/storage/s3-storage.test.tsghost/core/test/unit/server/services/oembed/oembed-service.test.jspackages/adapters/storage-base/src/base.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29732 +/- ##
==========================================
+ Coverage 75.43% 75.45% +0.02%
==========================================
Files 1612 1612
Lines 141929 141970 +41
Branches 17549 17565 +16
==========================================
+ Hits 107060 107127 +67
- Misses 33797 33798 +1
+ Partials 1072 1045 -27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
34f6282 to
0558355
Compare
ref https://linear.app/ghost/issue/HKG-1836/ Bookmark icons and thumbnails are stored without a content type, so GCS serves them as an octet-stream. Browsers sniff raster formats and render them anyway, but they do not sniff SVG, so SVG favicons show as broken images. Converting them to PNG on the way in fixes that without any storage change, since a PNG is sniffed like every other raster favicon already is. It also means we never store a document that can carry script under our own origin — these bytes come from whatever site the author bookmarked. Local storage types a file by its extension, so the URL's extension triggers conversion in its own right: everything that could be stored under a `.svg` name is converted or not stored at all, which makes a missed content sniff harmless rather than a stored script. Rasterizing untrusted input needs bounding, and the obvious bounds do not hold. `timeout` does not constrain librsvg, which renders during load before libvips checks its deadline. Input size alone does not either, because filter primitives cost output-area time. Neither says anything about the output, where a few hundred bytes of extreme aspect ratio rendered to tens of megabytes. And gzip hides its size from a byte cap entirely — 10KB of `.svgz` inflated to 1.1MB and took 47s — so compressed input is rejected rather than converted. Input is capped at 32KB, output is fixed at 256x256, and the worst case inside those is 0.8s.
0558355 to
f2830ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/test/unit/server/services/oembed/oembed-service.test.js`:
- Around line 834-842: Update processImageFromUrl so the rasterized PNG call to
saveRaw passes image/png as its third argument, then extend the SVG test for
saveRaw to assert that content type while preserving the existing path and
PNG-content assertions.
- Around line 883-892: Decode the URL pathname before extracting the filename in
the image-processing path, ensuring percent-encoded extensions such as %2E are
recognized by path.basename/path.extname and routed through shouldRasterize.
Extend the oEmbed service tests around the existing case-insensitive SVG test to
cover favicon%2ESVG and assert the saved output ends with .png.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 50443947-e386-4880-a0fb-f59054bfb05e
📒 Files selected for processing (2)
ghost/core/core/server/services/oembed/oembed-service.jsghost/core/test/unit/server/services/oembed/oembed-service.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/oembed/oembed-service.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR addresses bookmark-card SVG favicons rendering as broken images when stored via saveRaw() without a content-type (e.g., on GCS defaulting to application/octet-stream). Instead of plumbing MIME type through storage, it rasterizes SVG bookmark images to 256×256 PNGs before storing, which browsers reliably render via sniffing and avoids storing executable SVG under the site origin.
Changes:
- Detect SVGs by extension (
.svg/.svgz, case-insensitive) and by lightweight content sniffing, then convert to PNG via@tryghost/image-transformbeforeimageStore.saveRaw(). - Add guardrails: reject oversized SVG inputs (>32KB) and gzipped inputs (SVGZ/gzip magic) with a specific validation error.
- Add unit test coverage for conversion, bounds, extension/case handling, and rejection paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ghost/core/core/server/services/oembed/oembed-service.js | Adds SVG detection + guarded rasterization to PNG in processImageFromUrl() before storing bookmark icons/thumbnails. |
| ghost/core/test/unit/server/services/oembed/oembed-service.test.js | Adds a focused test suite validating PNG conversion, 256×256 bounding, case/extension invariants, and rejection of gzip/oversized SVG inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ref https://linear.app/ghost/issue/ONC-1930/substack-favicon-broken
Bookmark card icons are stored without a content type, so GCS serves them as an
octet-stream. Browsers sniff raster formats and render them anyway, but they do
not sniff SVG, so SVG favicons in bookmark cards show as broken images.
Reported via escalation.
Why this path and not image cards
Image cards go through
save(), which has always passedfile.typefrom theupload middleware. Bookmark icons go through
saveRaw()— they are fetchedserver-side rather than uploaded — and
saveRaw()never set a type. Same classof bug as #26637, on the half of the storage interface that fix did not cover.
What this does
Converts bookmark SVGs to PNG before storing them, using the conversion
handle-image-sizes.jsalready applies when it converts an SVG. Nothing elsechanges — no storage interface change, no content type, no changeset.
A PNG needs no content type to render: browsers sniff it, which is exactly why
every PNG and JPG favicon already renders fine from the same octet-stream that
breaks SVG. Converting also means we never store a document that can carry
script under our own origin, and these bytes come from whatever site an author
bookmarked.
Local storage types a file by its extension, so the URL's extension triggers
conversion in its own right. Everything that could be stored under a
.svgname is converted or not stored at all, which makes a missed content sniff
harmless rather than a stored script — on
mainan SVG favicon is alreadyserved as
image/svg+xmlfrom the site's own origin, unsanitized.Review confirmed no SSRF, no local file read and no XXE reach librsvg, that the
PNG output carries no metadata from the source, and that the naming invariant
survives case variants, percent-encoding, double extensions, query strings and
gzip.
Bounds
Rasterizing untrusted input needs bounding, and the obvious bounds do not hold.
Each of these was measured, and each was found by building the shape designed to
defeat the previous bound:
.svgz, 10KB inflating to 1.1MBtimeoutdoes not constrain librsvg — it renders during load, before libvipschecks its deadline
time rather than parse time
256x256, matching the
iconentry inimageOptimization.internalImageSizesrather than converted
Known limitations
broken. A header-only backfill is not safe for the SVGs, since those bytes
were never converted; re-fetching is the right remediation, and it mints a new
URL so the bookmark cards in post content need updating too.
Not visible at the 20px a bookmark icon renders at.
already crops with
object-fit: cover, and a wider target is most of the DoSexposure above.
DEFAULT_BOOKMARK_ICON..html/.xhtmlfrom an attacker's URL are still stored verbatim andserved executable from the site's own origin on self-hosted. Unchanged from
main and out of scope here — needs its own fix.
external-media-inlinerhas the same missing content type and is untouched.Testing
50 oembed unit tests. Every guard has a test that fails when the guard is
removed, including the extension match being case-insensitive, which is what
the naming invariant rests on.