Fix rich text image display with S3-compatible storage#2835
Fix rich text image display with S3-compatible storage#2835dilberryhoundog wants to merge 1 commit intobasecamp:mainfrom
Conversation
Fix service worker CORS credentials to work with cross-origin Active Storage redirects to S3/R2/GCS/Azure. The turbo-offline service worker inherits credentials: include from the original request, which fails after the cross-origin redirect because S3-compatible stores cannot return Access-Control-Allow-Credentials: true. Using credentials: same-origin sends credentials to Rails for auth but strips them on the cross-origin redirect. Fix ActionText attachment URLs missing account prefix for multi-tenant routing. to_rich_text_attributes generates blob URLs without the account script_name prefix, so AccountSlug::Extractor cannot route them. Adding script_name: Current.account&.slug produces correct tenant-prefixed paths. Nil-safe when no account is set.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes rich-text image rendering and offline caching in self-hosted deployments that use cross-origin Active Storage redirects (S3-compatible providers), while preserving multi-tenant path-based routing.
Changes:
- Forces the PWA service worker to use
credentials: "same-origin"so cross-origin redirects to storage don’t requireAccess-Control-Allow-Credentials. - Adds an account slug
script_nameprefix to ActionText attachment URLs so multi-tenant routing can resolve blob/representation redirect paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| config/initializers/active_storage.rb | Prefixes ActionText attachment relative URLs with the tenant/account path segment via script_name. |
| app/views/pwa/service_worker.js.erb | Updates turbo-offline fetch options to avoid sending credentials on cross-origin redirects to blob storage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # path (for portability across host name changes, beta environments, etc). | ||
| def to_rich_text_attributes(*) | ||
| super.merge url: Rails.application.routes.url_helpers.polymorphic_url(self, only_path: true) | ||
| super.merge url: Rails.application.routes.url_helpers.polymorphic_url(self, only_path: true, script_name: Current.account&.slug) |
There was a problem hiding this comment.
script_name is expected to be a path prefix (typically starting with /). Passing a bare slug (e.g., \"123\") can produce malformed paths (missing separator) depending on how Rails concatenates script_name + path. Consider normalizing to script_name: \"/#{Current.account.slug}\" (or using a helper that already returns a leading-slash prefix) so generated URLs are reliably /123/....
| super.merge url: Rails.application.routes.url_helpers.polymorphic_url(self, only_path: true, script_name: Current.account&.slug) | |
| slug = Current.account&.slug | |
| super.merge url: Rails.application.routes.url_helpers.polymorphic_url( | |
| self, | |
| only_path: true, | |
| script_name: (slug.present? ? "/#{slug.delete_prefix("/")}" : nil) | |
| ) |
| maxEntrySize: 2 * 1024 * 1024, // 2MB covers about 95% of all Fizzy blobs | ||
| maxEntries: 500, | ||
| fetchOptions: { mode: "cors" } | ||
| fetchOptions: { mode: "cors", credentials: "same-origin" } |
There was a problem hiding this comment.
This change is subtle but security/CORS-sensitive (it intentionally strips credentials on cross-origin redirects). Adding an inline comment explaining the redirect/CORS rationale here would help prevent future regressions (e.g., someone reverting to include while debugging auth).
| fetchOptions: { mode: "cors", credentials: "same-origin" } | |
| fetchOptions: { | |
| mode: "cors", | |
| // Intentionally avoid `include`: with CORS requests, `same-origin` strips | |
| // credentials on cross-origin redirects, which is the safer behavior here. | |
| credentials: "same-origin" | |
| } |
Summary
Problem
Two issues affecting self-hosted deployments using S3-compatible storage (R2, MinIO, etc.):
Service worker CORS failure: The
turbo-offlineservice worker inheritscredentials: "include"from the original request when fetching Active Storage redirect URLs. After following the cross-origin redirect to cloud storage, the browser requiresAccess-Control-Allow-Credentials: truein the response, which S3-compatible stores cannot return with specific origin CORS policies.ActionText attachment URLs missing account prefix:
to_rich_text_attributesgenerates blob URLs without the accountscript_nameprefix (e.g.,/rails/active_storage/blobs/redirect/...instead of/1234567/rails/active_storage/blobs/redirect/...). TheAccountSlug::Extractormiddleware can't route these, returning 404. The editor (Lexxy) uses these URLs as<img src>when re-editing rich text, so images appear broken in the editor despite displaying correctly on the show view.Root cause: upstream commit
235890e66switched ActionText attachment URLs from absolute to relative for portability, but the relative URLs lack thescript_nameprefix needed by multi-tenant routing.Changes
app/views/pwa/service_worker.js.erb(line 46):Sends credentials to Rails (needed for auth) but strips them on cross-origin redirects to cloud storage.
config/initializers/active_storage.rb(line 17):Adds account slug prefix so URLs route correctly through the multi-tenant middleware. Nil-safe — when
Current.accountis nil, behaves identically to before.Safety
AccountTenantedmixin serializes/restoresCurrent.accountCurrent.account&.slugreturns nil, no behavior changescript_name: account.slugwithpolymorphic_urlReproduction
MULTI_TENANT=false), but still uses path-based account slug routingNote
Existing ActionText content with broken URLs will fix itself when re-saved.