addons-code-manager
addons-code-manager copied to clipboard
mozilla
Serve external assets with SRI
In case of the CDN being taken over, the external statics (as allowed by CSP) could be modified.
It would be good to serve the static assets with Sub-Resource Integrity (SRI) to prevent any content that doesn't match the hash from being loaded.
┆Issue is synchronized with this Jira Task
@willdurand how much effort is there in enabling this? Just thinking it would be nice to have this before this starts being used in production.
This is not easy it seems because CRA does not support SRI. It used to support SRI (https://github.com/facebook/create-react-app/pull/1176) but it was removed because of obscure reasons (https://github.com/facebook/create-react-app/issues/1231).
Let's start with a CRA issue: https://github.com/facebook/create-react-app/issues/7006
The CRA docs suggests to fork CRA to add extra features. While we should not be doing this, I believe it would allow us to implement SRI easily by forking.
I worked on a POC that works and that is ready for a review already. It consists of:
- a new CRA fork that can be moved under the
mozillaorg as I renamed it toamo-create-react-app. Here is the state of the fork: https://github.com/willdurand/amo-create-react-app/pull/1. The branch starts from thev3.0.0tag, which is what we are currently using in code-manager. I followed this guide: https://auth0.com/blog/how-to-configure-create-react-app/ (linked in the official CRA docs) - a patch in code-manager, which mostly fixes the local prod env scripts and requires the fork: https://github.com/mozilla/addons-code-manager/compare/sri
Here is the output of code-manager with the patch mentioned above (yarn start-local-dev):
<!doctype html>
<html lang="en">
<head>
<meta charset="utf-8" />
<link rel="shortcut icon" href="http://localhost:3000/static/favicon.ico" />
<meta name="viewport" content="width=device-width,initial-scale=1,shrink-to-fit=no" />
<meta name="theme-color" content="#000000" />
<title>Addons Code Manager</title>
<link href="http://localhost:3000/static/css/2.272e98f1.chunk.css" rel="stylesheet" integrity="sha256-Or/xqhCYwSq+JSWAFqSoSKm10qN5QjtPfcAYSBMir/I= sha384-BD660je9iq5ZlRVjCqFvaAhyd+qfaNDoQOnMq5u7IefbmBwHVvB89o56Dzi1FCVW" crossorigin="anonymous">
<link href="http://localhost:3000/static/css/main.67403a33.chunk.css" rel="stylesheet" integrity="sha256-19Ykwh2ZVXrjf9zsGk5Zq1+rnFUBLjR8NSw8R0RPhxE= sha384-twWaR6jzh0NgiDRCwkXR6WTXskLLu7cXU+dQnXqf5LvwvJb/xsH8IYxILsbSrSBQ" crossorigin="anonymous">
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root" data-auth-token="TOKEN_HA_HA_HA"></div>
<script src="http://localhost:3000/static/js/runtime~main.f9585000.js" integrity="sha256-MSix49EiuGxIuDlIG9X/KUEprfk6kiTmFPkHE21vj0A= sha384-UkvrbuwAsSbGkaxMkocskpPMec1pG7C9kIGcY7OMBve1W0oR+rrN3d1e4iP+BHnm" crossorigin="anonymous"></script>
<script src="http://localhost:3000/static/js/2.e33cbe1b.chunk.js" integrity="sha256-L0d6CyLwvXW5D4Hr/pic9JWw81ejLLPSFwoo/qA/m6o= sha384-a1TlhbIXD2gCvrS9xUPDlFlGHBNnoNeBVst/OswwMXWVboiPRMkQuDdllhf7A2Pv" crossorigin="anonymous"></script>
<script src="http://localhost:3000/static/js/main.d5581f45.chunk.js" integrity="sha256-k09inONb8K3l3CmESh6mXkSYTVlet2gYs6qStPbkPws= sha384-37cxnhwky0Hj3KpoTORKAg4X5KTNaDVUrsA+CH1IAMfHbq6cYJkCV30JPDKSN8Qy" crossorigin="anonymous"></script>
</body>
</html>
Things to do/consider if we go this way:
- add new owner to the
amo-react-scriptsnpm package (currently owned by me, @willdurand) - automate the release process to easily publish new versions to npm (similar to our eslint configs, etc.)
- move
willdurand/amo-create-react-appto the Mozilla org - how do we maintain the fork?
- how do we maintain the fork?
I (@willdurand) think it should not be a problem because there are not a lot of CRA updates but https://github.com/facebook/create-react-app/issues/7006 is what we need (so we need to convince the CRA team to add this feature, again)
@kumar303 @bobsilverberg How do you feel about taking this direction? At least until we get an answer in https://github.com/facebook/create-react-app/issues/7006 and also considering that this issue might block the "go live" (deploying to prod).
@willdurand It seems like a better option than ejecting, but also seems not ideal. I guess our hope is that we fork and then they accept a patch which fixes the issue, and then we can go back to the main repo and delete our fork?
I don't fully understand SRI and why it is so important. Is it definitely a blocker for going live to prod with code manager? Also, what are our motivations for wanting to go live to prod with code manager? Do we expect reviewers to start using it for actual reviews? I guess what I'm asking is: how important is it to go live to prod with code manager soon, rather than waiting to see what happens with SRI and CRA?
I don't fully understand SRI and why it is so important.
@bobsilverberg SRI protects against modification of files by anything serving static resources which in our case is the CDN. Without SRI to exploit this you'd need to have access to the CDN and be able to write a modification to a file.
With SRI, the webpage served by the app defines a hash for a static file and if the hash doesn't match the content served it is rejected by the browser and not executed.
@psiinon do you have a view on whether lack of SRI needs to block releasing this to production? My take would be it's an extra layer of safety, and as such it probably doesn't need to be a blocker, but if we can add it we should look to do so as soon as reasonably possible.
I should also add that we do not have a lot of other options with CRA.
There is a file generated by CRA during the build but its content will need extra manipulation before being able to iterate over the generated JS/CSS files if we want to handle the SRI stuff in the server code, which will also require a new index.html template that will completely override the CRA/webpack build step (and we need to think about how it would work for NODE_ENV=development). Maintaining a fork with a +8/-4 patch seems way easier than that to me, especially since the patch does the right thing (i.e. hooking into the webpack build step), at least until it becomes a CRA official feature.
@muffinresearch in this case its our CDN (right?) so I dont think its a blocker. SRI is a great protection when using files on other peoples CDNs where you have less control. Maintaining SRIs can actually be a pain, ie to update them when the files validly change.
I advise to just keep an eye on facebook/create-react-app#7006 (the request for this feature in CRA) and possibly spend time on a patch for it, although let's prioritize that work before starting it.
SRI is not super crucial for our app since the CDN is fully under our control and our ops team uses best practices when handling the credentials. The need for SRI doesn't seem like a compelling enough reason to publish and maintain our own fork of CRA. Using a fork would add extra steps to the upgrade process (including regression testing) so I'd like to avoid it.
@muffinresearch in this case its our CDN (right?) so I dont think its a blocker. SRI is a great protection when using files on other peoples CDNs where you have less control. Maintaining SRIs can actually be a pain, ie to update them when the files validly change.
Yep the CDN. The way this would work would be fully automated (assuming Create-react-app support) so maintenance of the hashes shouldn't be an issue.