Fixed YouTube bookmark metadata#29114
Conversation
|
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds allowlisted oEmbed enrichment for bookmark cards and combines it with fetched page metadata. Bookmark image processing now handles missing URLs safely, uses shared defaults, and conditionally processes icons and thumbnails. Failed page requests can return enrichment-only bookmark data when a title is available. Tests cover provider metadata, fallback paths, timeout behavior, missing images, and exclusion of provider embed HTML. 🚥 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.
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.
715b168 to
201ce2f
Compare
|
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. |
ref TryGhost#24741 Limited the oEmbed bookmark shortcut to YouTube so other providers keep their richer page metadata while YouTube retains its provider-specific fallback.
ref TryGhost#24741 Known providers can expose accurate structured media metadata even when their pages return generic content or block server-side requests, so bookmarks now enrich page metadata without discarding useful page fields.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/oembed/oembed-service.js (1)
442-488: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winThumbnail fallback still leaves raw provider URL exposed on processing failure.
When
processImageFromUrlfails formetadata.thumbnail(Lines 472-479), the.catchonly logs and leavesmetadata.thumbnailpointing at the raw, unprocessed provider URL — inconsistent with the icon path (Lines 460-471), which falls back toDEFAULT_BOOKMARK_ICONon failure. This was already flagged in a prior review and remains unresolved in this revision. The "process image, fall back on failure" pattern is also still duplicated between the icon and thumbnail branches here.♻️ Suggested fix
- if (metadata.thumbnail) { - await this.processImageFromUrl(metadata.thumbnail, 'thumbnail') - .then((processedImageUrl) => { - metadata.thumbnail = processedImageUrl; - }).catch((err) => { - logging.error(err); - }); - } + metadata.thumbnail = await this.processImageFromUrl(metadata.thumbnail, 'thumbnail') + .catch((err) => { + logging.error(err); + return null; + });🤖 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 442 - 488, Update the thumbnail handling in buildBookmarkData so a processImageFromUrl failure replaces metadata.thumbnail with DEFAULT_BOOKMARK_ICON instead of retaining the raw provider URL. Reuse the existing icon/thumbnail image-processing fallback pattern where practical, while preserving successful processed URLs and existing logging 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.
Outside diff comments:
In `@ghost/core/core/server/services/oembed/oembed-service.js`:
- Around line 442-488: Update the thumbnail handling in buildBookmarkData so a
processImageFromUrl failure replaces metadata.thumbnail with
DEFAULT_BOOKMARK_ICON instead of retaining the raw provider URL. Reuse the
existing icon/thumbnail image-processing fallback pattern where practical, while
preserving successful processed URLs and existing logging behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ca2ea1d-6000-4313-acc2-b577cbdab90b
📒 Files selected for processing (2)
ghost/core/core/server/services/oembed/oembed-service.jsghost/core/test/unit/server/services/oembed/oembed-service.test.js
- simplified the enrichment timeout to the got-style {request: ms} shape only;
no caller passes a bare number, and the dual-shape handling widened the
public JSDoc for a case that never occurs
- always create the abort signal so a zero/negative timeout can't leave the
enrichment request unbounded while Promise.allSettled waits on it
- merged two near-identical YouTube bookmark tests into the TryGhost#24741 regression
test, keeping the union of their assertions (description passthrough, icon
fallback, thumbnail processing call, embed-HTML leak check)
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 55s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 16s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 6s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 40s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 31s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | <1s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 5s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 22s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-22 01:26:38 UTC
no ref - the test's page used <link rel="thumbnail">, which metascraper never picks up, so the service always called processImageFromUrl(undefined); the test only passed because the stub returned a fake stored URL for any input, including undefined - the missing-image guard added for TryGhost#24741 now skips absent thumbnails, exposing the artifact; use og:image so the test exercises the real scrape-and-store path
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29114 +/- ##
==========================================
- Coverage 74.57% 74.56% -0.01%
==========================================
Files 1609 1609
Lines 141363 141454 +91
Branches 17258 17269 +11
==========================================
+ Hits 105423 105477 +54
- Misses 34909 34913 +4
- Partials 1031 1064 +33
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:
|
2 participants
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: