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

Option to use devtools with csp #18140

Conversation

ruslan-shuster
Copy link

Summary

Currently, REACT_DEVTOOLS_GLOBAL_HOOK is being injected to the window through adding <script> tag to the web page, since it's the easiest cross-platform way to interact with web page environment. However, there are some users who restrict possible sources for scripts via Content Security Policy, but still want their web page to interact with react-devtools. The solution proposed here uses the possibility to whitelist webhook mounting script by its hash (see more here). Solves the issue described here.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9ffef21:

Sandbox Source
zen-pike-ixx08 Configuration

@sizebot
Copy link

sizebot commented Feb 26, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 9ffef21

@sizebot
Copy link

sizebot commented Feb 26, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 9ffef21

@gaearon
Copy link
Collaborator

gaearon commented Feb 27, 2020

Can we link to some way for the person to verify the hash actually matches our code?

@ruslan-shuster
Copy link
Author

Can we link to some way for the person to verify the hash actually matches our code?

There is some problem with it, because the code is being constructed from different pieces concatenated together:

. If I include the reference to this line to the README, it still gives nothing to the users, because they cannot just see the code which was used to generate the hash.

We may also try to generate the global hook code string at the build time, and then import and use it... This way it will be easier to refer to it. What do you think?

@bvaughn
Copy link
Contributor

bvaughn commented Mar 4, 2020

Can we link to some way for the person to verify the hash actually matches our code?

This was the concern I raised.

It looks like the hook source changes pretty infrequently, but we are unlikely to remember to update a README like this if we change it down the road.

Option 1: ReactJS.org

Maybe we could store the hash at build time like @ruslan-shuster says, and add an endpoint to reactjs.org that pulls it from GitHub (like it pulls the error codes) and displays some copy-pasteable thing for people to add?

We'd still have to remember to update reactjs.org to do this, but that probably gets updated enough on its own that the window of time when the two were out of sync wouldn't be too big.

Option 2: README

There's also the fact that rollouts to different browser stores take wildly different times. Firefox usually approves new extensions within minutes, where as Chrome now takes weeks. (So if we changed the hook, there would be a period of time when our messaging would be wrong one way or another.)

We could have the README show different hashes by versions. If we stored the most recently built-and-published hash, then the DevTools build script could print a big bold reminder (if it ever changes) letting person know to add a new entry to the README.

Option 3: Add the hash to DevTools setting UI

DevTools settings UI currently shows the revision it was built from inline:
Screen Shot 2020-03-04 at 9 50 14 AM

We could add another section that shows the CSP hash needed. Then our README could just say "look at DevTools to find the hash you should add for the current version". That way, at least, we could just auto-update this value anytime we build and release a new version (and we'd never have to remember to update external documentation).

I think option 3 seems the most promising. What do you think?

@ruslan-shuster
Copy link
Author

@bvaughn the third one indeed seems the most elegant and convenient. I'll start working on it and see how it looks. I think I should use some crypto library to generate sha256, since JS does not have such functionality in the standard library. Do you have any preferences on this one? Maybe Facebook has something open-sourced?

@bvaughn
Copy link
Contributor

bvaughn commented Mar 5, 2020

Cool. Thanks @ruslan-shuster.

I think Node's built-in crypto module should be capable of doing this so maybe we can just use it?

@ruslan-shuster
Copy link
Author

Thanks @bvaughn, it might work!

The only problem is that the hook resides in the esm-style module, and the build is running as cjs. Any common pattern how to overcome it? I was trying to find something like this in the React code base, but I did not succeed with that. I used https://www.npmjs.com/package/esm-to-cjs for similar case in other projects, but I'm pretty sure you don't want to introduce such dependency.

As I understand it, we need to import the hook code from injectGlobalHook at the build stage, generate a hash and export it as a 'process.env.GLOBAL_HOOK_HASH', so that we can use it just the same as build version.

@bvaughn
Copy link
Contributor

bvaughn commented Apr 7, 2020

Sorry @ruslan-shuster, I missed this comment somehow.

As I understand it, we need to import the hook code from injectGlobalHook at the build stage, generate a hash and export it as a 'process.env.GLOBAL_HOOK_HASH', so that we can use it just the same as build version.

That sounds right to me.

Our build script already does this and stores the artifact at e.g. packages/react-devtools-extensions/chrome/build/unpacked/build/injectGlobalHook.js

Can we just use that? 😄

@ruslan-shuster
Copy link
Author

@bvaughn I don't see the way how we could use it from the packages/react-devtools-extensions/webpack.config.js. We cannot import the file you've mentioned for two reasons:

  1. It is generated by the build, thus does not exist before the build, and this is exactly when we need to generate a hash from the hook.
  2. The name of the hook variable is being minified, so we cannot rely on it.

I assume that the variable holding the code of the hook will reside packages/react-devtools-extensions/src/injectGlobalHook.js, and should be somehow imported at the build phase from here.

@bvaughn
Copy link
Contributor

bvaughn commented Apr 15, 2020

  1. It is generated by the build, thus does not exist before the build...

Why can't we run part of the build twice? (Or split it into steps?)

Unfortunately I don't have time to dig into this and think about it right now. Too much context switching 😦 I'm sorry!

@stale
Copy link

stale bot commented Jul 18, 2020

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jul 18, 2020
@stale
Copy link

stale bot commented Jul 25, 2020

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

@stale stale bot closed this Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants