facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.71k stars 26.85k forks source link

Add an ID to the run time script #5288

Open Timer opened 6 years ago

Timer commented 6 years ago

https://github.com/facebook/create-react-app/issues/5144#issuecomment-426980892

PerfectPixel commented 6 years ago

@Timer To bring forth the arguments from #5144 , let me elaborate on a few issues we encountered:

I understand that adding a postbuild script is not difficult. We already have to add it to four repositories, I guess there are companies out there that use more. Furthermore, patching itself is not difficult, getting it right is.

Example: myHTML.replace(/(<script>(?:.*?)<\/script>)/, `<script src="${manifest['runtime~main.js']}"></script>`). Adding an ID will help us to find the correct <script>...</script> so it is better than nothing. If for some reason the runtime script contains itself a </script> like let script = "<script src=" + src + "></script>" this will break.

The solid solution is to parse the index.html as HTML, find the correct node, replace it, convert the content to a string and write it back. Then, I would say, we are no longer in the realm of an easy solution.

If more scripts are inlined, this solution will break as well.

I completely understand the desire to not create additional environment variables or configuration options as they surely add complexity. But in this case it is essentially punishing users that try to maintain a high security standard - regardless if they want to do it or are even (legally) obligated to do so).

I can absolutely understand that saving one additional request for a small file is worth it. But while going from <script src="/static/js/runtime~main.js"></script> to an inlined script is very easy, the other way around becomes rather fragile.

I do not intend to press this any further, I just do not think this is really a small edge-case.

Thank you for your time & feedback đź‘Ť

mgol commented 6 years ago

Just for comparison, Angular CLI has always extracted the script to a separate file precisely for this issue. Before v6 it was even called somewhat confusingly inline.bundle.js.

peterbe commented 6 years ago

This is a slightly off-topic comment but I do wonder if it's even a good thing to inline the JavaScript at all in the HTTP2 era. It makes it impossible to make the JS code async and if the .js file is served on the same domain there is extremely little overhead to serving it as a file over HTTP2. The HTML can be parsed sooner because it's smaller which means it can get started, in parallel, to download (and start parsing) other assets. Would love to see a simple option to disable all inlining.

Also, because https://github.com/facebook/create-react-app/issues/5144 is now closed I'll post it here: If others are struggling with busted Content Security Polices as of this recent change, you might get some inspirational ideas from this blog post: https://www.peterbe.com/plog/inline-scripts-in-create-react-app-2.0-and-csp-nonces

edmorley commented 6 years ago

If others are struggling with busted Content Security Polices as of this recent change, you might get some inspirational ideas from this blog post:

I could be wrong, but I believe that using a static nonce-<hash> like that is insecure (the spec says it must be unique each time). The generated hash needs to be put into a '<hash-algorithm>-<base64-value>' attribute instead. See: https://github.com/facebook/create-react-app/issues/5144#issuecomment-425261243 https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#Sources

peterbe commented 6 years ago

@edmorley You're right. I rushed that. Fixed my blog post now.

But also, I changed my mind about bothering anyway. I took @PerfectPixel 's heed and wrote a script that "uninlines" the inline script tag. Now my index.html is 1420 bytes (676 gzipped) instead of 2496 bytes (1194 gzipped). It's so small that it's really hard to measure any big difference and the context is a fun little side-project but I wanted to use this to put thoughts into code.

0xdeafcafe commented 6 years ago

Thanks @peterbe, that looks quite helpful. Although it seems quite insane the hoops you have to jump through just to get back to where we were with CRA1. This change only encourages a less secure web - with less experienced/lazy devs most likely to just turn off CSP or allow unsafe-inline...

peterbe commented 6 years ago

@Timer Where should we redirect our issues about A) an option to disable inlining small scripts and B) lobby to have that option "on" by default.

Timer commented 6 years ago

This issue, @peterbe.

gaearon commented 6 years ago

I say let’s add a way to opt out. https://github.com/facebook/create-react-app/issues/5309#issuecomment-427838480

I still think the majority would be better served by the inlined script though.

Timer commented 6 years ago

We can close this if we're going to add an environment variable. edit: actually it might still be useful, tagged for later milestone

jvatic commented 4 years ago

Any update on this? I'd like to see the webpack config for inlining the runtime chunk be opt-in. IMHO having this inlined is a premature optimization at the cost of easily serving the app with a sane CSP.

The only other option to prevent having unsafe-inline in the CSP, besides ejecting and removing this portion of the config, is to add a cryptographic hash of each runtime script to the CSP (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#Sources). Neither option seems to be a sane default.

kddnewton commented 4 years ago

@jvatic you can opt out with INLINE_RUNTIME_CHUNK=false in your .env.

dimaqq commented 4 years ago

Or add a config override, like draft.plugins = draft.plugins.filter(p => p.constructor.name !== "InlineChunkHtmlPlugin");

caub commented 2 years ago

a little ugly, but it's probably fine to do:

function serveApp(res) {
  const nonce = crypto.randomBytes(16).toString('base64');
  res.set({
    'Content-Security-Policy': `....; script-src 'nonce-${nonce}'`,
    // ..
  });
  return fs.promises.readFile(path.join(__dirname, 'static', 'myapp', 'index.html'), 'utf8')
    .then(indexHtml => res.send(indexHtml.replace(/<script>/g, `<script nonce="${nonce}">`)));
}

(coming from #5144 nonce issue)

edit: actually using INLINE_RUNTIME_CHUNK=false is better (seen it in https://deploy-preview-27627--material-ui.netlify.app/guides/content-security-policy/#create-react-app-cra)

caub commented 2 years ago

Re #5144 the problem is not only for the runtime script, but also for lazy-loaded scripts, what can we do with react-scripts?