-
Notifications
You must be signed in to change notification settings - Fork 100
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
feat: add subresource integrity #334
base: main
Are you sure you want to change the base?
Conversation
An alternative could be to copy the contents into a local CSS file and reference that instead, if we're worried the upstream CSS could change (and thus mismatch the hash)... Either solution should improve the security by not being vulnerable to upstream exploits. 🙂 @DCMattyG let me know if you want me to update the PR with that solution instead. |
Hi @stbeldarborge, pardon my delay in response. I have a few questions about this as I'm unfamiliar with this process...
I'm just trying to understand the residual impact of implementing this and why this is a concern, especially for this specific package. Appreciate the additional details! |
Google regularly update their fonts, so this CSS and font-file will probably change in the future (probably when people least expect it). There are regular commits to the Material Icons repo. I suggest copying the CSS and the font-file into the project as @stbeldarborge suggests. The Apache 2.0 license probably permits this (beware: I am no expert on licensing).
Edit: I see now from the MUI docs that this CSS import in index.html is required by @mui/icons-material. Sorry for jumping in unannounced - I'm a font-nerd and could not help myself 😅 |
Thanks @nilsel, I really appreciate the insight here but I'm still trying to wrap my head around the value of implementing this. This is my current understanding:
So you see, I'm trying to weigh in if this change makes an overall positive or negative impact on the product, because as I understand it right now there appears to be more downside than upside. Apologies if I'm being a bit dense, I just like to "measure twice, cut once" as they say. |
Thanks for weighing in @nilsel. As you're a font nerd and think the best solution is to self host, I'll change the PR. :) I'm not knowledgeable about fonts.. I just want IPAM to score better on security testing tools like securityscorecard which checks for subresource integrity. 😅 EDIT: okay this was harder than it seems.. please commit to this and help me out @nilsel ? :D |
@DCMattyG no worries 😄
True, the app would probably run, but without any icons, and a security-error in the devtools console and network tab. But there's another dealbreaker for SRI here I just remembered: This is done to optimise both the css and font-files served, with some font-features removed for platforms that do not support it (Safari will get
Yes, copying into the project would work, we will lose some of the optimisations (and backwards-compabilty for older browsers that Google offers). The license should be fine, they do encourage self-hosting if needed. But having another license to parse and track (plus another repo if updates are needed) is extra work. However... @stbeldarborge The SVG-code for the icons can be found in These icons are part of the npm-package and are bundled with the JS for the app when imported, so I don't think the font-file is needed at all. 😅 Try removing the I tried this locally (removed the Google fonts link, commented out the auth-part and added some icons to a page just to test), and the Related: |
do you have any automated way to test this @DCMattyG ? that it builds and deploys to a test webapp ? |
@stbeldarborge, when a PR is submitted (and approved by a maintainer) a series of GitHub actions takes place which does indeed deploy the full application and test it. However, these tests are limited to API functionality for now as writing UI tests can be complicated and I probably need some assistance from someone proficient in tooling such as Selenium (or equivalent). That said, the UI isn't so complicated right now and I can fairly easily verify if there are any missing or incorrect icons as a result of this change. I also 100% agree with @nilsel. At one point in time, I did use the |
This fixes an unsafe implementation of a subresource. Ref https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity
The hash has been properly generated, see below screenshots.