-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exclude 'remixicon.symbol.svg' from asset pipeline #5878
Exclude 'remixicon.symbol.svg' from asset pipeline #5878
Conversation
Interesting. In Alchemy we have the same issue and we tried to fix it with a preload tag
I have no assets host that I can test this with, though I trust your approach. But, can we try to preload the asset and verify if it works as well or not? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to think this approach is fine (rather than preloads) because:
- It's in the admin and serving one asset in the admin from the app server isn't a performance concern.
- Keeping preloads in sync with usage seems like a maintenance burden. It would be pretty easy for people to change something and miss that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solidus_installer
step will pass if you rebase this off main (which has a fix fpr that step merged now) :)
Referencing SVG icons from an external host using `<use xlink:href="">` is not supported, for CORS (cross-origin request scripting) security reasons. This doesn't come up often in development but can rear its head in a production deployment. To avoid this, we'll always provide an empty host. This results in the generated paths being protocol-prefixed relative URLs (i.e. "http:/assets/solidus_admin/remixicon.symbol.svg") but this seems to work OK.
a60443d
to
7e432d8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5878 +/- ##
==========================================
+ Coverage 87.88% 89.58% +1.70%
==========================================
Files 477 783 +306
Lines 11659 17991 +6332
==========================================
+ Hits 10246 16117 +5871
- Misses 1413 1874 +461 ☔ View full report in Codecov by Sentry. |
Addresses #5657 - when a Solidus store has
config.action_controller.asset_host
configured to point to a different hostname than the Rails application (i.e. an assets CDN, in production), the admin menu icons do not render as expected.This is because for security reasons, referencing SVG icons from an external file (using
<use xlink:href="">
) is not possible when the asset is served from a different domain to the one requesting it.Providing an empty
host
option allows us to skip Rails' built-in asset-path helper methods that are falling back to the globalasset_host
config, and correct the issue.Note: this change does result in the generated asset path being a protocol-prefixed relative URL (i.e.
"http:/assets/solidus_admin/remixicon.symbol-[SHA].svg"
), but this seems to work OK from my testing.