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

Add an ID to the run time script #5288

Open
Timer opened this issue Oct 4, 2018 · 15 comments
Open

Add an ID to the run time script #5288

Timer opened this issue Oct 4, 2018 · 15 comments
Milestone

Comments

@Timer
Copy link
Contributor

Timer commented Oct 4, 2018

#5144 (comment)

@PerfectPixel
Copy link

@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
Copy link

mgol commented Oct 5, 2018

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
Copy link

peterbe commented Oct 5, 2018

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 #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
Copy link

edmorley commented Oct 5, 2018

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:
#5144 (comment)
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#Sources

@peterbe
Copy link

peterbe commented Oct 5, 2018

@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
Copy link
Contributor

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
Copy link

peterbe commented Oct 8, 2018

@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
Copy link
Contributor Author

Timer commented Oct 8, 2018

This issue, @peterbe.

@gaearon
Copy link
Contributor

gaearon commented Oct 8, 2018

I say let’s add a way to opt out. #5309 (comment)

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

@Timer
Copy link
Contributor Author

Timer commented Oct 8, 2018

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

@Timer Timer closed this as completed Oct 8, 2018
@Timer Timer reopened this Oct 8, 2018
@Timer Timer modified the milestones: 2.0.5, 2.x Oct 8, 2018
@iansu iansu modified the milestones: 2.x, 3.x Mar 10, 2019
@jvatic
Copy link

jvatic commented Jan 24, 2020

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
Copy link

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

@dimaqq
Copy link

dimaqq commented May 12, 2020

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

@caub
Copy link

caub commented Dec 22, 2021

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
Copy link

caub commented Feb 24, 2022

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

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

No branches or pull requests