🐛 Fixed bookmark favicon storage on object-storage adapters#27670
🐛 Fixed bookmark favicon storage on object-storage adapters#27670
Conversation
ref https://linear.app/ghost/issue/ONC-1673 processImageFromUrl was joining the absolute getContentPath('images') with the imageType and passing the result to store.generateUnique. Object-storage adapters (e.g. S3) build keys via path.posix.join, which preserves an absolute second argument and produces a malformed bucket key embedding the local filesystem prefix. The exists() probe then targeted a different key than the eventual saveRaw, defeating uniqueness checks and -- depending on bucket policy -- throwing on the HEAD request, which surfaced as the bookmark favicon fallback users were reporting. Pass the relative imageType directly so probe and write target the same key. Also converted the six oembed logging.error sites to structured logging with event names and bookmark/icon/thumbnail URL context so failures can be attributed to specific embeds in Elasticsearch.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
ref https://linear.app/tryghost/issue/ONC-1673 The previous fix patched the bookmark favicon caller, but the underlying storage adapters silently tolerated absolute, prefixed, and traversing paths — masking similar bugs elsewhere. Introduced a shared assertCanonicalRelativePath validator used by LocalStorageBase and S3Storage at all mutating entry points, removed the tolerance helpers that papered over bad inputs, and migrated the remaining non-canonical callers (importer image handler, external media inliner, handle-image-sizes middleware, getLocalImagesStoragePath) so the contract holds end-to-end. exists() stays lenient (returns false on malformed input) since it has a natural negative answer.
rob-ghost
force-pushed
the
fix/onc-1673-bookmark-favicons
branch
from
b88ef5f to
5768626
Compare
|
Closing in favour of #27671, which lands the same user-visible fix (favicons restored) with a much smaller diff by absorbing the multi-shape inputs at the adapter layer rather than tightening the contract and migrating callers. The strict approach in this PR is still defensible — it makes the storage adapter contract explicit and would prevent future callers from reintroducing similar bugs — but the cost (validator module, file-vs-directory distinction, three caller migrations, ~25 test fixtures touched) outweighs the benefit at this point. Re-opening this if we decide later that the permissive contract is causing problems. |
1 participant
Problem
Bookmark cards stopped rendering favicons on Pro because the bookmark fetcher passed an absolute filesystem path to the storage adapter. Investigation revealed the bug was the user-visible symptom of a wider design issue: the storage adapter contract was permissive — accepting absolute paths, paths already prefixed with the storage root, decorative leading slashes, etc. — and its consumers (the bookmark fetcher, the importer image handler, the external-media-inliner, and the image-sizes middleware) each violated it in different ways. The two adapters (LocalStorageBase and S3Storage) tolerated the violations differently, so callers got away with it on local dev while producing malformed bucket keys on object-storage backends.
Solution
Tighten the contract: both adapters now require canonical relative paths via a shared
assertCanonicalRelativePathvalidator. Migrate all four callers to pass the canonical shape. Drops the silent tolerance helpers fromLocalStorageBase. Mutating methods (save/saveRaw/delete/read) throw on bad input;exists()is lenient and returnsfalsesince it's a query, not a mutation.The empty string is the only "non-relative-looking" input the validator accepts, and only for
targetDirarguments — it represents the storage root itself, which the importer needs for files at the top of an import zip.Migration
Forward-only. No existing storage objects move or rename; URLs already stored in posts continue to resolve to whatever object they referenced before. New writes from previously-broken callers (importer + media-inliner) now land at the keys their referencing URLs expect.
Comparison with #27671
This is the strict half of a strict-vs-permissive design debate. PR #27671 is the permissive alternative — same user-visible fix, but the adapter absorbs the multi-shape inputs instead of forcing each caller to be correct. Both PRs are open so the contract direction can be picked deliberately.
ref https://linear.app/ghost/issue/ONC-1673