🐛 Fixed bookmark card favicons not loading after the first save

[allouis]

ref https://linear.app/ghost/issue/ONC-1788/bookmark-card-favicon-fetching-broken

Problem

#27926 switched oEmbed image storage to content-addressed filenames (name-<sha256>.ext) to kill the per-write uniqueness walk that didn't scale on GCS.

Side effect: re-bookmarking a URL whose favicon was already stored hashes to the same path, so saveRaw attempts an in-place overwrite. Production object storage deliberately rejects overwrites (create-only, anti-corruption), so the save throws and the bookmark card silently falls back to the placeholder icon. Invisible in development because local disk overwrites silently.

Fix

Name the file with a random suffix instead of a content hash:

const uniqueFileName = `${name}-${crypto.randomUUID()}${ext}`;
const targetPath = path.join(imageType, uniqueFileName);
return store.saveRaw(imageBuffer, targetPath);

Every save lands on a fresh key, so saveRaw always creates and never overwrites — the no-overwrite restriction is sidestepped entirely. No exists-check, no catch, no URL reconstruction.

Why not check-then-reuse the existing file

That was the previous approach in this PR (check exists, reuse if present). It works for the write, but to return the icon URL for an already-stored file you have to reconstruct it (urlUtils.urlFor) rather than take it from saveRaw. We can't guarantee that reconstruction matches what the storage adapter actually returns (it depends on urls:image config reproducing the adapter's CDN + per-tenant key scheme), and the adapter interface is a public extension point so we can't add a getDownloadUrl to fetch it. A wrong reconstructed URL would silently re-break favicons. A unique key avoids the whole question: saveRaw always runs and returns the adapter's own URL.

Why a random key is safe re: the original incident

The scaling incident #27926 fixed was the generateUnique walk — sequential HEADs probing for a free favicon-N.png. A random key is unique without probing, so it avoids the walk just as well as a content hash did. No regression there.

Trade-off (deliberate)

We lose the within-site dedup the content hash gave us: re-bookmarking the same favicon now stores another copy instead of reusing one. For favicons (tiny) this is acceptable. Note that under create-only these duplicates can't be reclaimed (no delete), so it's a slow, bounded, append-only growth — flagged here so it's a conscious choice, not an accident.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e042c629-f739-4f0c-ab17-59fbb0fcdfb9

📥 Commits

Reviewing files that changed from the base of the PR and between 8ba93e3 and d5e832b.

📒 Files selected for processing (2)

• ghost/core/core/server/services/oembed/oembed-service.js
• ghost/core/test/unit/server/services/oembed/oembed-service.test.js

---

Walkthrough

This PR changes how the oEmbed service generates filenames for stored images. The processImageFromUrl function now uses crypto.randomUUID() instead of SHA-256 hashing the image buffer. This makes each image storage operation generate a unique filename, even when processing identical image bytes. The test suite is updated to verify UUID-based naming patterns, confirm that each call produces a distinct filename, and ensure that the storage layer is not probed for existing files.

Possibly related PRs

• TryGhost/Ghost#27926: Prior update to image filename generation in oembed-service.js and corresponding test adjustments.

Suggested labels

needs:review

Suggested reviewers

• vershwal

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title references fixing bookmark card favicons, but the actual changes affect oEmbed image storage naming (UUID vs SHA-256 hashing), which is a broader infrastructure change not specific to favicons.	Consider a more precise title like 'Use random UUID for oEmbed image filenames instead of content hash' to accurately reflect the implementation change.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is directly related to the changeset, providing context on the problem (content-addressed filenames causing overwrite issues), the fix (random UUID suffixes), and trade-offs.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fabien-onc-1788-bookmark-card-favicon-fetching-broken

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[rob-ghost]

I believe this solves the issue in a tactical way 👍🏻 I do have some questions / feedback.

For favicons (tiny) this is acceptable

If I'm not mistaken this is both favicons and thumbnails, so the size impact is larger than stated. Might still be fine, but I want us to be clear about the cost of the duplication.

I'm also getting a bit of a 🚩 on the overall design. My thinking:

The fact that we can't overwrite files is a policy decision on our specific adapter. That policy now leaks into the client: because we know we can't overwrite, the OEmbed service has to work around it. That's information leakage through the interface; our internal policy has reached into a storage concern that self-hosters (raw files, or backends that overwrite happily) don't share.

The real fix is to change the adapter so the policy stays behind the port. We can't add a required method without breaking self-hosters, but we can do it non-breakingly: add a method that just takes the buffer and a logical path and returns a URL: cacheImage(buffer, path) with a default implementation that delegates to today's create-and-uniquify behaviour (so every existing adapter is unchanged), overridden in our adapter to hash-and-reuse instead of overwriting.

The key point is that how the bytes are made unique or deduped; content hash, overwrite, random suffix, whatever, is the adapter's business, not the caller's. The SHA is an implementation detail of our adapter's dedupe, so it shouldn't appear at the call site at all. The caller just hands over the bytes and a name; identity, overwrite policy, and URL construction all sit behind the port.

I'd deliberately not fold this into saveRaw itself, that would change behaviour for every saveRaw caller, whereas a new method means the only thing that changes is the favicon/thumbnail path. saveRaw and its other callers stay exactly as they are.

Happy with this change as it is, but I think there's potential for a more principled fix if we have the appetite for it!

[allouis]

If I'm not mistaken this is both favicons and thumbnails, so the size impact is larger than stated. Might still be fine, but I want us to be clear about the cost of the duplication.

Yes you're right, I should have been more clear about that! Though it's worth mentioning that this is how it functioned before the previous "fix", so I feel fine about the change!

RE: The policy leaking - yes I do agree. There's also the idea of adding a flag to saveRaw like deduplicate or isCAS or something 🤔 I think I would prefer a new (non-required) method though.

I'd like to merge this fix as is, to close off the immediate problem - but I'm going to take a note to look at an alternative, and I'll get back to you whether I go through with it or not.