Fixed YouTube bookmark metadata#29114
Conversation
Fixes TryGhost#24741 Why: YouTube pages can return generic localized metadata to server-side bookmark requests even when the allowlisted oEmbed endpoint has the video details. What: Use known-provider oEmbed metadata for explicit bookmark cards and skip missing image URLs before processing them.
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
WalkthroughChangesThis change modifies the oEmbed service's bookmark card metadata handling. A Sequence Diagram(s)(See hidden review stack artifact for diagram.) Related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/oembed/oembed-service.test.js (1)
315-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a test for the known-provider failure/fallback path.
Coverage is good for the success case and the missing-image case, but there's no test exercising
fetchBookmarkDataFromKnownProviderwhenknownProviderthrows (or returns no title) to confirmfetchOembedDataFromUrlcorrectly falls back to page scraping for a bookmark request.🤖 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/test/unit/server/services/oembed/oembed-service.test.js` around lines 315 - 329, Add a unit test for the bookmark fallback path in oembed-service.test.js that exercises fetchOembedDataFromUrl when fetchBookmarkDataFromKnownProvider fails or returns no title, so it falls back to page scraping for bookmark requests. Use the existing oembedService and stub fetchBookmarkDataFromKnownProvider to throw or return an incomplete result, then assert the fallback result comes from page scraping and that bookmark metadata is still populated correctly. Keep the test alongside the existing fetchBookmarkData coverage and reference fetchOembedDataFromUrl and fetchBookmarkDataFromKnownProvider to locate the behavior.
🤖 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/core/server/services/oembed/oembed-service.js`:
- Around line 192-244: The thumbnail image handling in
fetchBookmarkDataFromKnownProvider and fetchBookmarkData is inconsistent with
the icon fallback and can leave raw provider URLs exposed when
processImageFromUrl fails. Update the thumbnail processing path so failures
clear or replace metadata.thumbnail with a safe fallback instead of keeping the
original URL, matching the DEFAULT_BOOKMARK_ICON behavior used for icons. While
touching fetchBookmarkData and fetchBookmarkDataFromKnownProvider, consider
extracting the repeated “process image with fallback” logic into a shared helper
to remove duplication and keep the thumbnail/icon behavior consistent.
---
Nitpick comments:
In `@ghost/core/test/unit/server/services/oembed/oembed-service.test.js`:
- Around line 315-329: Add a unit test for the bookmark fallback path in
oembed-service.test.js that exercises fetchOembedDataFromUrl when
fetchBookmarkDataFromKnownProvider fails or returns no title, so it falls back
to page scraping for bookmark requests. Use the existing oembedService and stub
fetchBookmarkDataFromKnownProvider to throw or return an incomplete result, then
assert the fallback result comes from page scraping and that bookmark metadata
is still populated correctly. Keep the test alongside the existing
fetchBookmarkData coverage and reference fetchOembedDataFromUrl and
fetchBookmarkDataFromKnownProvider to locate the behavior.
🪄 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
Run ID: 11974741-5297-4601-918a-7c0105cc664a
📒 Files selected for processing (2)
ghost/core/core/server/services/oembed/oembed-service.jsghost/core/test/unit/server/services/oembed/oembed-service.test.js
| /** | ||
| * Build bookmark metadata from a known oEmbed provider without exposing the | ||
| * provider-supplied HTML that embed cards use. | ||
| * | ||
| * @param {string} url | ||
| * @returns {Promise<Object|undefined>} | ||
| */ | ||
| async fetchBookmarkDataFromKnownProvider(url) { | ||
| const {url: providerUrl, provider} = findUrlWithProvider(url); | ||
| if (!provider) { | ||
| return; | ||
| } | ||
|
|
||
| let oembed; | ||
| try { | ||
| oembed = await this.knownProvider(providerUrl); | ||
| } catch { | ||
| // Known-provider metadata is an enhancement for explicit bookmark | ||
| // cards. If it fails, fall back to scraping the page as before. | ||
| return; | ||
| } | ||
|
|
||
| if (!oembed.title) { | ||
| return; | ||
| } | ||
|
|
||
| const metadata = { | ||
| url, | ||
| title: oembed.title, | ||
| description: oembed.description || null, | ||
| author: oembed.author_name || null, | ||
| publisher: oembed.provider_name || null, | ||
| thumbnail: oembed.thumbnail_url || (oembed.type === 'photo' ? oembed.url : null), | ||
| icon: DEFAULT_BOOKMARK_ICON | ||
| }; | ||
|
|
||
| if (metadata.thumbnail) { | ||
| await this.processImageFromUrl(metadata.thumbnail, 'thumbnail') | ||
| .then((processedImageUrl) => { | ||
| metadata.thumbnail = processedImageUrl; | ||
| }).catch((err) => { | ||
| logging.error(err); | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| version: '1.0', | ||
| type: 'bookmark', | ||
| url, | ||
| metadata | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Thumbnail fallback inconsistency + duplicated image-processing pattern.
When processImageFromUrl fails for the thumbnail (Line 228-235 here, and the equivalent Line 469-476 in fetchBookmarkData), the .catch only logs the error and leaves metadata.thumbnail pointing at the raw, unprocessed provider/third-party URL. This is inconsistent with the icon handling, which explicitly falls back to DEFAULT_BOOKMARK_ICON on failure, and it partially undercuts this PR's stated goal of not exposing provider-supplied content directly — a bookmark card can end up hot-linking an untrusted external image URL instead of a locally-stored copy.
Additionally, the "process image, fall back on missing/failure" pattern is now duplicated three times (icon in fetchBookmarkData, thumbnail in fetchBookmarkData, thumbnail in fetchBookmarkDataFromKnownProvider). Consider extracting a small shared helper.
♻️ Suggested helper + fallback fix
+ async _processBookmarkImage(imageUrl, imageType, fallback = null) {
+ if (!imageUrl) {
+ return fallback;
+ }
+ try {
+ return await this.processImageFromUrl(imageUrl, imageType);
+ } catch (err) {
+ logging.error(err);
+ return fallback;
+ }
+ }
+
async fetchBookmarkDataFromKnownProvider(url) {
...
- if (metadata.thumbnail) {
- await this.processImageFromUrl(metadata.thumbnail, 'thumbnail')
- .then((processedImageUrl) => {
- metadata.thumbnail = processedImageUrl;
- }).catch((err) => {
- logging.error(err);
- });
- }
+ metadata.thumbnail = await this._processBookmarkImage(metadata.thumbnail, 'thumbnail');Based on the PR's own rationale of not exposing provider-supplied content raw, applying the same null-fallback used for icons to thumbnails would close this gap.
Also applies to: 228-235, 469-476
🤖 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/services/oembed/oembed-service.js` around lines 192 -
244, The thumbnail image handling in fetchBookmarkDataFromKnownProvider and
fetchBookmarkData is inconsistent with the icon fallback and can leave raw
provider URLs exposed when processImageFromUrl fails. Update the thumbnail
processing path so failures clear or replace metadata.thumbnail with a safe
fallback instead of keeping the original URL, matching the DEFAULT_BOOKMARK_ICON
behavior used for icons. While touching fetchBookmarkData and
fetchBookmarkDataFromKnownProvider, consider extracting the repeated “process
image with fallback” logic into a shared helper to remove duplication and keep
the thumbnail/icon behavior consistent.
1 participant
Fixes #24741
Why:
YouTube bookmark requests can receive generic page metadata (and no thumbnail) from the fetched page, even though YouTube's allowlisted oEmbed endpoint has the video title, author, provider, and thumbnail. Missing image URLs were also still sent through bookmark image processing, which produced
Failed to parse URL from nulllogs.What:
Why Ghost users or developers need this:
YouTube bookmark cards now use video metadata instead of a generic YouTube card and avoid noisy null-image URL errors.
Tests:
pnpm test:single test/unit/server/services/oembed/oembed-service.test.jspnpm exec eslint core/server/services/oembed/oembed-service.js test/unit/server/services/oembed/oembed-service.test.jsgit diff --checkPlease check your PR against these items: