Closed davemfish closed 2 years ago
I could use some help deciding what is appropriate for our use-case because I don't know much on these topics.
Notes from Electron Security Tutorial:https://www.electronjs.org/docs/tutorial/security
Take away from below notes:
In fact, the most popular Electron apps (Atom, Slack, Visual Studio Code, etc) display primarily local content (or trusted, secure remote content without Node integration) – if your application executes code from an online source, it is your responsibility to ensure that the code is not malicious.
By updating Electron to the latest version, you ensure that critical vulnerabilities (such as nodeIntegration bypasses) are already patched and cannot be exploited in your application.
Common web vulnerabilities, such as Cross-Site Scripting (XSS), have a higher security impact on Electron applications hence it is highly recommended to adopt secure software development best practices and perform security testing.
⚠️ Under no circumstances should you load and execute remote code with Node.js integration enabled. Instead, use only local files (packaged together with your application) to execute Node.js code. To display remote content, use the
tag or BrowserView, make sure to disable the nodeIntegration and enable contextIsolation.
@emlys I did a little reading about this. I still don't think I understand it 100%, but here's my take on what/how we might need to refactor in order to comply with electron's best-practices of contextIsolation: true
. Writing it down to help myself understand...
If all the Node API functionality needs to happen in the main
process, then we'll need to look across all our front-end js & jsx files for all uses of node modules like fs
, path
, os
, child_process
and probably a few others and replace those API calls with send
or invoke
IPC calls to functions running in the main process. So there will be a lot more EventListener-type logic. And we'll probably want to keep that well organized across different modules that can be imported by main.js
- so that main.js
doesn't get too cluttered.
The ipcRenderer
API itself probably needs to go in the preload
script, like you were showing me. I was stuck on the idea that the preload script might need to expose all those other node APIs too (fs, path
, etc), but actually I think not, since we will move all those calls out of the renderer and into main. So I think those rather brief preload script examples we've been seeing are probably what we will need.
Then I was stuck on, "how can we import React
and react-bootstrap
etc if the renderer does not have Node's require
API. And I think the answer there is, when we use babel (or webpack) to transpile, it presumably produces JS that runs in a browser without Node's require
.
If we do a refactor like this, I think the vast majority of our tests should continue working without needing any changes themselves. But there probably exceptions, like the tests that mock the spawned
invest subprocess.
I'm a little less intimidated by all this now than when we talked earlier, but it's definitely a fair bit of work. So I think we should decide as a team when the right time to do this is, before/after the beta milestone.
I also agree with all @dcdenu4 's points above. So I do feel safe releasing our app without these changes. At a minimum though we ought to make sure we aren't loading any code that hasn't been packaged with the binary (e.g. stylesheets from a CDN, etc).
We'll discuss and decide what the right milestone is. Might depend on how urgently we want to do a beta release.
thanks for diving into this @davemfish! I had not realized we could avoid exposing all the node modules fs
, path
, etc. by invoke
ing them through ipcRenderer
. That does make it somewhat less intimidating. I agree it's fine to put off for now and we can decide together when makes sense to make the transition.
As part of these changes, we'll need to start using webpack
instead of or in addition to babel
to package the front-end code. Right now, we have babel
generating node-compliant javascript, but for the front-end parts of the app, we then rely on electron's nodeIntegration
to run that nodejs javascript in the browser.
In order to set nodeIntegration: false
and contextIsolation: true
, we'll need to generate browser-compliant javascript, and webpack is the tool for that.
I think we're quite close to meeting all the requirements needed to flip the switch on these webPreferences
webPreferences: {
contextIsolation: false, // should go true
nodeIntegration: true, // should go false
preload: path.join(__dirname, '..', 'preload.js'),
},
One more todo: remove use of crypto
node module in the renderer. Should be very easy because we can use window.crypto
.
We may or may not be doing things the "right way" regarding the options enabled in our app's renderer process. If a rendererer process ever renders third-party content (e.g. from a website on the internet) then these things would be a big deal. Since we don't do that, I'm not sure if they are a big deal or not.
Right now, we give the renderer access to the local node runtime. That allows for using
require
in theindex.html
to load the whole front-end. See the isntantiation ofnew BrowserWindow
inmain.js
.In addition to
nodeIntegration
, there's the related issue ofenableRemoteModule
, mentioned in natcap/invest-workbench#45 , as well asworldSafeExecuteJavaScript
andcontextIsolation
, which Electron warns about in when the app starts up.Some resources: https://stackoverflow.com/questions/63427191/security-warning-in-the-console-of-browserwindow-electron-9-2-0 https://stackoverflow.com/questions/44391448/electron-require-is-not-defined https://www.electronjs.org/docs/tutorial/security