Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stbeldarborge
Copy link

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.

image
image

@stbeldarborge
Copy link
Author

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.

@DCMattyG DCMattyG self-requested a review November 20, 2024 15:26
@DCMattyG DCMattyG self-assigned this Nov 20, 2024
@DCMattyG
Copy link
Contributor

Hi @stbeldarborge, pardon my delay in response. I have a few questions about this as I'm unfamiliar with this process...

  1. As this package is provided by Google, what is the realistic concern that an invalid package would be distributed?
  2. How often does this need to be regenerated, especially considering I have no control over that CSS package?
  3. If it does need to be regenerated, what happens to customers whom have an older version of the index.html file?

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!

@nilsel
Copy link

nilsel commented Nov 22, 2024

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.
The @font-face -> src URL looks like it has a version-string in the path (v142).

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).
You will get enhanced privacy too, by not loading resources from Google servers 😸

I see you also have @mui/icons-material in [ui/package.json - this package seems to offer the same icons as the icon-font imported in index.html. I see they are being used in the React-app at least.

I don't know the details of this project so there might be a good reason for also loading these via index.html (for usage outside of React perhaps)?

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 😅

@DCMattyG
Copy link
Contributor

DCMattyG commented Nov 22, 2024

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:

  • After adding this hash, if Google updates their CSS, the Azure IPAM product would fail for all users because the hash would no longer match
  • I could copy the Google CSS into the project locally, but then I become responsible for something Google was already doing and may or may not violate some licensing

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.

@stbeldarborge
Copy link
Author

stbeldarborge commented Nov 25, 2024

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
https://developers.google.com/fonts/docs/material_icons#setup_method_2_self_hosting

@nilsel
Copy link

nilsel commented Nov 25, 2024

@DCMattyG no worries 😄

After adding this hash, if Google updates their CSS, the Azure IPAM product would fail for all users because the hash would no longer match

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:
Google checks the user-agent string, and serves different CSS based on what browser is requesting the CSS.

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 -webkit-font-smoothing and Firefox gets -moz-osx-font-smoothing for example). Also the character sets in the font-file returned may be different.

I could copy the Google CSS into the project locally, but then I become responsible for something Google was already doing and may or may not violate some licensing

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
There is still a chance this can be resolved! ✨
The React-app uses the @mui/icons-material package - I can see the icons are being imported and used in the app as inline SVG in the form of <IconName />. I cannot find any usage of the actual icon-font searching this repo (no occurrences of material-icon css classname that I can find).

The SVG-code for the icons can be found in ui/node_modules/@mui/icons-material/*.js, wrapped in JS.

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. 😅
All the font-nuances I mentioned can be disregarded if this is the case.

Try removing the <link rel="stylesheet" href="https://fonts.googleapis.com ... from index.html and see if things still work.

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 <IconName /> tags still work as before.
As long as the app is using icons the "@mui-way" (via import and <IconName /> tag), and not the icon-font approach (that would look something like: <span class="material-icon">icon_name</span>) it should be fine to remove.
Might just get lucky here 😸

Related:
This issue in Google Fonts repo has some details for why Google does not prioritise SRI on their font-serving CDN.

@stbeldarborge
Copy link
Author

do you have any automated way to test this @DCMattyG ? that it builds and deploys to a test webapp ?

@DCMattyG
Copy link
Contributor

@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 <Icon> method which requires the CSS file, but I've since moved away from this and it's highly likely that the CSS link in question doesn't serve any real purpose at this point in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants