fix(core): preview externally hosted URLs in storage properties - #948
Merged
Conversation
A string property with a `storage` config crashed the preview with `FirebaseError: Firebase Storage: Invalid URL` whenever the saved value was an externally hosted URL (e.g. `https://abc.com/abc.png`) rather than a path in the configured bucket. Two separate defects combined to cause this. 1. `StorageThumbnail` ignored its own `storeUrl` prop. The prop was declared and compared in the `areEqual` memo comparator, but never read in the component body, so every value was unconditionally sent to `storage.getDownloadURL()` even when it already was a fully resolved download URL. The component now detects values that are already absolute `http(s)` URLs (or that come from a `storeUrl: true` property) and previews them directly, without going through the storage source at all. Since there is no storage metadata, and therefore no `contentType`, for such values, the preview type is inferred from the file extension, falling back to `"file"` as before. Storage paths keep resolving exactly as they did, including the `fileNotFound` -> "File not found" view for objects missing from the bucket. 2. `useFirebaseStorageSource.getDownloadURL()` had no pass-through for external URLs. `gs://` URLs were special-cased, but any other absolute URL was handed straight to `ref()`, which throws `storage/invalid-url` for anything it cannot parse as a Firebase Storage location. That error is now caught, and an absolute `http(s)` URL is returned as-is instead of propagating. Rather than matching hosts, this defers to Firebase's own URL parser: the set of URLs `ref()` accepts depends on the configured storage host, so pointing the SDK at the storage emulator makes production `firebasestorage.googleapis.com` URLs invalid too. That is exactly the reported scenario, and a static host allowlist would not have covered it. Firebase download URLs that `ref()` does accept still resolve through it, so `getMetadata()` keeps reporting their real content type, and genuinely missing objects still yield `fileNotFound`. The URL helpers are extracted as pure functions in `util/urls.ts` and covered by unit tests. Fixes #550 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes crashes when FireCMS previews string storage fields whose stored value is an externally hosted http(s) URL (including emulator-vs-production Firebase download URL mismatches), by avoiding invalid Firebase Storage ref() parsing and adding URL-based preview inference.
Changes:
- Add shared URL helpers to detect absolute
http(s)URLs and infer preview type from file extensions. - Update
StorageThumbnailto bypass storage resolution for absolute URLs / stored URLs and render previews directly. - Patch Firebase storage source
getDownloadURLto pass through absolute URLs when Firebase throwsstorage/invalid-url.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/firecms_core/src/util/urls.ts | Introduces URL detection + extension/preview-type inference helpers. |
| packages/firecms_core/src/util/index.ts | Re-exports the new URL utilities. |
| packages/firecms_core/src/util/tests/urls.test.ts | Adds unit tests covering URL detection, extension parsing, and preview inference. |
| packages/firecms_core/src/preview/components/StorageThumbnail.tsx | Avoids calling storage resolution for absolute URLs and infers preview type from URL extension. |
| packages/firebase_firecms/src/hooks/useFirebaseStorageSource.ts | Catches storage/invalid-url for absolute URLs and returns them as-is instead of throwing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * previewed as they are, and must not be handed over to the storage source, | ||
| * which can only resolve paths of its own bucket. | ||
| */ | ||
| const valueIsUrl = storeUrl || isAbsoluteHttpUrl(storagePathOrDownloadUrl); |
Comment on lines
73
to
+75
| if (!storagePathOrDownloadUrl) return null; | ||
|
|
||
| if (valueIsUrl) { |
Comment on lines
57
to
59
| useEffect(() => { | ||
| if (!storagePathOrDownloadUrl) | ||
| if (!storagePathOrDownloadUrl || valueIsUrl) | ||
| return; |
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.
Fixes #550
A string property with a
storageconfig crashed the preview withFirebaseError: Firebase Storage: Invalid URLwhenever the stored value was an externally hosted URL rather than a path in the configured bucket. Two separate defects combined.Defect 1 —
StorageThumbnailignored its ownstoreUrlpropThe prop was declared and compared in the
areEqualmemo comparator, but never read in the component body, so every value was unconditionally sent tostorage.getDownloadURL()even when it already was a resolved download URL.Defect 2 — no pass-through for external URLs
getDownloadURL()special-casedgs://but handed any other absolute URL straight toref(), which throwsstorage/invalid-urlfor anything it cannot parse as a Storage location.Why this does not use a host allowlist
The obvious fix is to match
firebasestorage.googleapis.com/storage.googleapis.com. That would be wrong.Location.makeFromUrl(url, host)builds its regexes from the runtimestorage.host, so when the SDK points at the storage emulator, production Firebase download URLs arestorage/invalid-urltoo:That is exactly the reporter's "emulator against a live project's storage" scenario, so an allowlist would still have crashed. This defers to Firebase's own parser and catches
storage/invalid-url, gated on^https?://so a malformed path still throws. It needs no maintenance as Firebase adds hosts (e.g.*.firebasestorage.app) and cannot regress any URLref()accepts today.The two layers compose rather than duplicate: the core fix means the storage source is never consulted for an absolute URL; the Firebase fix covers every other
getDownloadURLcaller.Tests
New pure helpers in
util/urls.tswith 21 unit tests (external vs Firebase vsgs://vs paths vsdata:; extension extraction through query strings and hashes; preview-type inference).@firecms/core: 258/261 passing (was 237/240) — 3 pre-existing failures unchanged.Behaviour change worth reviewing
storeUrl: truethumbnails no longer make agetDownloadURL+getMetadataround-trip — a real perf win in list views — but preview type now comes from the file extension rather thancontentType. FireCMS's own upload path preserves extensions, and.pdf/.zipmap to"file"exactly asapplication/*did, so no visible change is expected; an extension-less stored filename would degrade from image preview to file link.Separate pre-existing bug found, left alone
The
errorstate inStorageThumbnailis also dead —setErroris passed to.catch()buterroris never read in the render, so a failed resolution shows the skeleton forever.Not verified
The end-to-end preview needs a browser and a real Firebase project. The render branch is verified by typecheck and reading, plus a functional probe of the patched
getDownloadURLcontrol flow against the real SDK.🤖 Generated with Claude Code