Fixed media inliner to store correct relative paths with CDN storage#26791
Merged
Fixed media inliner to store correct relative paths with CDN storage#26791
Conversation
vershwal
force-pushed
the
fix/s3-storage-relative-paths
branch
from
d6c67bb to
e210a19
Compare
Contributor
WalkthroughstoreMediaLocally was changed to return a transform-ready URL directly, and calling sites now use that returned value as the inlined source instead of prepending a GHOST_URL prefix. Unit tests were added for CDN storage adapter behavior and for handling storage.saveRaw returning relative local paths, verifying correct inlined URLs and field values. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect media paths produced during external media inlining when using CDN-backed storage adapters (e.g., S3) by converting returned CDN URLs back into relative /content/... paths before callers prepend __GHOST_URL__.
Changes:
- Update
ExternalMediaInliner.storeMediaLocally()to strip configured CDN base URLs fromsaveRaw()return values. - Add unit tests covering CDN-URL and relative-path
saveRaw()behavior for the external media inliner. - Add an oEmbed unit test covering
saveRaw()returning a relative path (local storage behavior).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ghost/core/core/server/services/media-inliner/external-media-inliner.js | Adds CDN base URL stripping logic to ensure returned paths are compatible with __GHOST_URL__ prefixing. |
| ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js | Adds CDN-configured test cases to verify correct __GHOST_URL__/content/... storage for both CDN URL and relative-path saveRaw() outputs. |
| ghost/core/test/unit/server/services/oembed/oembed-service.test.js | Adds coverage for local-storage-style relative path return from saveRaw() in oEmbed image processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/core/server/services/media-inliner/external-media-inliner.js (1)
150-164: Prefer adapter-owned URL normalization here.
saveRaw()is storage-specific, and the storage contract already exposesurlToPath(url). Rebuilding that mapping fromurls:image|media|filesduplicates adapter logic and can drift for backends whose public URL shape does not exactly match those config values. If possible, normalize absolutesaveRaw()results through the adapter instead of hardcoding config-prefix stripping here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/media-inliner/external-media-inliner.js` around lines 150 - 164, The current manual stripping of config-based CDN prefixes duplicates adapter logic; instead, call the storage adapter's urlToPath(url) (e.g. storageAdapter.urlToPath(filePath) or the same adapter instance used by saveRaw()) to convert absolute URLs into relative paths returned by saveRaw(), and use the returned path if non-null; keep the existing config-prefix stripping as a fallback (use variables filePath and cdnBaseUrls as before) so behavior is preserved for adapters that don't implement urlToPath().ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js (1)
994-1126: Add explicitmedia/filesCDN cases to this suite.These tests only exercise
/content/images, so the newurls:mediaandurls:filesbranches are still unproven. Parameterizing the storage path and expected output across all three prefixes would make the regression coverage match the implementation claim.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js` around lines 994 - 1126, The tests only cover the /content/images branch; update the four test cases (those that construct ExternalMediaInliner and call inlineContent/inlineFields) to run for each CDN prefix (image, media, files) by parameterizing the storagePath/staticFileURLPrefix/getTargetDir/getUniqueFileName and expected output; specifically, iterate over an array of prefixes (e.g. '/content/images', '/content/media', '/content/files') and for each create a storage stub returned by getMediaStorage (the existing sinon.stub().withArgs('.gif').returns({...})) with storagePath and staticFileURLPrefix set to the current prefix and getUniqueFileName returning `${prefix}/unique-*.jpg` (and adjust path.relative stub to match), then assert the result equals `__GHOST_URL__/${prefix.replace(/^\/+/,'')}/unique-*.jpg`; apply this parameterization to both inlineContent tests and both inlineFields tests so all three URL config branches are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/media-inliner/external-media-inliner.js`:
- Around line 150-164: The current manual stripping of config-based CDN prefixes
duplicates adapter logic; instead, call the storage adapter's urlToPath(url)
(e.g. storageAdapter.urlToPath(filePath) or the same adapter instance used by
saveRaw()) to convert absolute URLs into relative paths returned by saveRaw(),
and use the returned path if non-null; keep the existing config-prefix stripping
as a fallback (use variables filePath and cdnBaseUrls as before) so behavior is
preserved for adapters that don't implement urlToPath().
In
`@ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js`:
- Around line 994-1126: The tests only cover the /content/images branch; update
the four test cases (those that construct ExternalMediaInliner and call
inlineContent/inlineFields) to run for each CDN prefix (image, media, files) by
parameterizing the
storagePath/staticFileURLPrefix/getTargetDir/getUniqueFileName and expected
output; specifically, iterate over an array of prefixes (e.g. '/content/images',
'/content/media', '/content/files') and for each create a storage stub returned
by getMediaStorage (the existing sinon.stub().withArgs('.gif').returns({...}))
with storagePath and staticFileURLPrefix set to the current prefix and
getUniqueFileName returning `${prefix}/unique-*.jpg` (and adjust path.relative
stub to match), then assert the result equals
`__GHOST_URL__/${prefix.replace(/^\/+/,'')}/unique-*.jpg`; apply this
parameterization to both inlineContent tests and both inlineFields tests so all
three URL config branches are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8be777d7-231c-46c4-9181-9a5feb3d2344
📒 Files selected for processing (3)
ghost/core/core/server/services/media-inliner/external-media-inliner.jsghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.jsghost/core/test/unit/server/services/oembed/oembed-service.test.js
allouis
reviewed
Mar 12, 2026
Collaborator
allouis
left a comment
allouis
left a comment
There was a problem hiding this comment.
I think we can return urlUtils.toTransformReady(filePath) here instead 🤔
vershwal
force-pushed
the
fix/s3-storage-relative-paths
branch
from
e210a19 to
f8b2cf8
Compare
- When using CDN storage (S3Storage), saveRaw() returns a full CDN URL (e.g., https://storage.ghost.is/.../content/images/photo.jpg) instead of a relative path like local storage does (/content/images/photo.jpg) - The media inliner's storeMediaLocally() passed this CDN URL directly to callers, which prepended __GHOST_URL__ to it, producing malformed database entries like __GHOST_URL__https://storage.ghost.is/... - This broke image references in posts after running content imports on CDN-configured sites - Used urlUtils.toTransformReady() in storeMediaLocally() to convert the storage adapter's return value into a __GHOST_URL__-prefixed relative path, which correctly handles both CDN URLs and local paths — callers no longer manually prepend __GHOST_URL__ - The oembed service (the only other saveRaw caller) was verified to not have this issue — it returns the URL as-is to the admin client, which flows through the model pipeline where formatOnWrite handles the CDN-to-__GHOST_URL__ transformation correctly
vershwal
force-pushed
the
fix/s3-storage-relative-paths
branch
from
f8b2cf8 to
26884dc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js (1)
999-1009: Differentiate the CDN bases per asset type.All three config keys are set to the same URL, and every case writes under
/content/images. That means a regression which only stripsurls:imagewould still pass here, sourls:mediaandurls:filesare not independently covered.Also applies to: 1017-1135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js` around lines 999 - 1009, The test's beforeEach currently sets the same CDN_BASE_URL for all asset types which masks regressions; update the setup in beforeEach to use distinct base URLs for each asset type (e.g., CDN_IMAGE_BASE, CDN_MEDIA_BASE, CDN_FILES_BASE) when calling configUtils.set('urls:image'...), configUtils.set('urls:media'...), configUtils.set('urls:files'...) and when assigning urlUtils._assetBaseUrls, and adjust any expectations/assertions in the tests that rely on specific upload paths so they assert different output paths for image, media and files; apply the same change to the other test blocks referenced (around 1017-1135) to ensure independent coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js`:
- Around line 1033-1039: The test stubs call getMediaStorage with the wrong
extension ('.gif') so they never match the code path that uses media.extension
('.jpg'); update the sinon.stub().withArgs('.gif') occurrences to
withArgs('.jpg') for the blocks around test cases that generate JPG URLs/targets
(the getMediaStorage stub used in the test file, and the similar blocks at the
locations flagged: the blocks at ~1033, ~1063, ~1090, ~1119), ensuring the
returned getUniqueFileName/saveRaw values remain JPG-based and the rest of the
test expectations remain unchanged.
---
Nitpick comments:
In
`@ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js`:
- Around line 999-1009: The test's beforeEach currently sets the same
CDN_BASE_URL for all asset types which masks regressions; update the setup in
beforeEach to use distinct base URLs for each asset type (e.g., CDN_IMAGE_BASE,
CDN_MEDIA_BASE, CDN_FILES_BASE) when calling configUtils.set('urls:image'...),
configUtils.set('urls:media'...), configUtils.set('urls:files'...) and when
assigning urlUtils._assetBaseUrls, and adjust any expectations/assertions in the
tests that rely on specific upload paths so they assert different output paths
for image, media and files; apply the same change to the other test blocks
referenced (around 1017-1135) to ensure independent coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e326ca3-ee21-4b6d-a867-928f64e1ecda
📒 Files selected for processing (3)
ghost/core/core/server/services/media-inliner/external-media-inliner.jsghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.jsghost/core/test/unit/server/services/oembed/oembed-service.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/test/unit/server/services/oembed/oembed-service.test.js
- ghost/core/core/server/services/media-inliner/external-media-inliner.js
Member
Author
|
Thanks for the suggestion @allouis , |
allouis
approved these changes
Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/HKG-1648