[Future-waiting on Electron framework] Implement sandbox

[reZach]

Sandbox will be a property on the BrowserWindow in the future (?) hopefully. When it is in place, enhance the template to use it. Using sandbox will force ipc communication between the renderer and main processes. Even though that's how the template is built now, it feels better to force our decisions by the framework than by our own designs.

When this is implemented, copy the documentation from here and write up some documentation to verify we are in sandbox mode.

[kewde]

Sandboxing will means deprecrating the IPC architecture in favor of ProtocolHandlers. I've read reports that IPC can fail under high loads, see slack.engineering

[reZach]

@kewde Are you implying that there has been a push/announcement from the Electron team that protocol handlers are the best way moving forward, or is this just an assumption? I would be most comfortable moving in the direction that the Electron team recommends.

[lacymorrow]

The sandbox property seems to be implemented in Electron, is there a reason it isn't being used here yet?

[reZach]

@lacymorrow Where are you seeing this being implemented? I still see that it is experimental: https://www.electronjs.org/docs/api/sandbox-option#status

[kewde]

It's implemented & available in many releases, just marked as experimental.

[b-zurg]

@kewde It think that's his whole point. I agree. Something so explicitly marked as experimental is likely not production-ready.

[kewde]

I have used it in production for over two years and haven't had any issues. At this point I'm willing to believe it's marked as experimental just because they don't want to claim it is secure.

[lacymorrow]

Yep, it's in production builds is my point. I understand the argument for waiting until it's not labeled experimental, however.

[reZach]

@lacymorrow , @kewde , @b-zurg - I've commented on this electron issue and hopefully we can get an answer from the team on the status of this feature.

[reZach]

@lacymorrow @kewde @b-zurg it looks like it's officially supported, that's good. There's one barrier as of today to update this template to support sandbox mode and that's to change secure-electron-store (a dependency), to be sandbox-friendly. The other dependencies I have built should work but I know for sure the store dependency needs to be updated

The problem is this line which is used to get initial store data synchronously for users who need access to their store data when their app loads.

I am open to some help, if any of you have time.

[kewde]

I would suggest using moving away from any IPC-based design as it does not scale very well for large applications.

The protocol handlers are likely a better option as they use the same network stack as a normal HTTP request (as far as I understand). https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-url-request.md

An asynchronous call can be made synchronous so I don't think there is much of an issue there.

[reZach]

@kewde Understood, I believe you made that point with regards to IPC earlier in this issue. What particular approach in here are you recommending?

It's not just that, but the store module requires fs to be passed in. So it's ripping that out, and then making the context binding async-supported (is it possible? not entirely sure) - if that's even the route to go as you describe.

[kewde]

I referred to the article to explain that IPC does not perform well for a lot of data, therefore protocol handlers are a better alternative.

Write a ProtocolHandler for example "securefs://". Make the main application listen on for example "securefs://command". Process HTTP request sent to that URL.

POST securefs://command
{
  function: "writeFileSync",
  args: [
    "path",
    "data"
   ]
}

I do not recommend JSON as the actual underlying protocol IDL. It's better to pass binary messages, because JSON can not carry binary data (only when BASE64 encoded).

Then rewrite the Store to use the protocol handlers.

[kewde]

My 2 cents, do the above but write a generic variant.

in main process:

function whitelistFS(call, args) {
  if (call !== "writeFileSync){
    return false;
  }
  return true;
}

const fs = require("fs");
const crypto = require("crypto");
securecomm.attach(fs, whitelistFS)
securecomm.attach(crypto, ...you get it ....)

Every request passed to "securecomm://fs" will go through whitelistFS. If whitelistFS returns false, reject the call and return a 400. If it returns true, execute for module.call(args), e.g: fs.writeFileSync(path, data). Blacklist everything as the default behavior.

[reZach]

I referred to the article to explain that IPC does not perform well for a lot of data, therefore protocol handlers are a better alternative.

Write a ProtocolHandler for example "securefs://". Make the main application listen on for example "securefs://command". Process HTTP request sent to that URL.

POST securefs://command
{
  function: "writeFileSync",
  args: [
    "path",
    "data"
   ]
}

I do not recommend JSON as the actual underlying protocol IDL. It's better to pass binary messages, because JSON can not carry binary data (only when BASE64 encoded).

Then rewrite the Store to use the protocol handlers.

How high performance are you thinking of when you made this comment. Are you desiring to make apps in this way or to make this template more performant?

[reZach]

@kewde What I was trying to tease out is that my intention is to keep this repository aligned with the methods the Electron team recommends, instead of rolling my own protocol, unless of course that change is minimal and isn't a large or unfamiliar or frequently touched change.

I'd be more than fine you taking this repo and making a higher-performance fork of it if you'd like.

[reZach]

Edit, misunderstood. Re-opening.

[roryabraham]

Hi @reZach 👋 Thanks for this very helpful repo! I see that Electron's BrowserWindow now takes a boolean sandbox parameter, but I have been really struggling to get it to work. Maybe you can help?

Background

A simplified version of my configuration (using Electron 17.0.0) is something like this:

1. A shared file with plain JS stores custom events:

     const ELECTRON_EVENTS = {
         MY_EVENT: 'my-event',
         MY_OTHER_EVENT: 'my-other-event',
     };
   
     module.exports = ELECTRON_EVENTS;

2. A preload script uses the contextBridge API to facilitate IPC:

     const _ = require('underscore');
     const {
         contextBridge,
         ipcRenderer,
     } = require('electron');
     const ELECTRON_EVENTS = require('./ELECTRON_EVENTS');
   
     const WHITELIST_CHANNELS_RENDERER_TO_MAIN = [
         ELECTRON_EVENTS.MY_EVENT,
     ];
   
     const WHITELIST_CHANNELS_MAIN_TO_RENDERER = [
         ELECTRON_EVENTS.MY_OTHER_EVENT,
     ];
   
     contextBridge.exposeInMainWorld('electron', {
         send: (channel, data) => {
             if (!_.contains(WHITELIST_CHANNELS_RENDERER_TO_MAIN, channel)) {
                 // eslint-disable-next-line no-console
                 console.warn(`Attempt to send data across electron context bridge with invalid channel ${channel}`);
                 return;
             }
   
             ipcRenderer.send(channel, data);
         },
         on: (channel, func) => {
             if (!_.contains(WHITELIST_CHANNELS_MAIN_TO_RENDERER, channel)) {
                 // eslint-disable-next-line no-console
                 console.warn(`Attempt to send data across electron context bridge with invalid channel ${channel}`);
                 return;
             }
   
             // Deliberately strip event as it includes `sender`
             ipcRenderer.on(channel, (event, ...args) => func(...args));
         },
     });

3. The Electron main process loads my app in a BrowserWindow like so:

     const browserWindow = new BrowserWindow({
         webPreferences: {
             preload: `${__dirname}/contextBridge.js`,
             contextIsolation: true,
         },
     });

And that's all working as expected (thanks to the guidance from this repo 🙏 ).

The problem

If I enable sandbox on the BrowserWindow object, I get the following errors:

Unable to load preload script: /Users/roryabraham/Expensidev/App/desktop/contextBridge.js
(anonymous) @ VM4 sandbox_bundle:93
VM4 sandbox_bundle:93 Error: module not found: underscore
    at preloadRequire (VM4 sandbox_bundle:93:1417)
    at <anonymous>:2:13
    at runPreloadScript (VM4 sandbox_bundle:93:2244)
    at Object.<anonymous> (VM4 sandbox_bundle:93:2511)
    at Object../lib/sandboxed_renderer/init.ts (VM4 sandbox_bundle:93:2667)
    at __webpack_require__ (VM4 sandbox_bundle:1:170)
    at VM4 sandbox_bundle:1:1242
    at ___electron_webpack_init__ (VM4 sandbox_bundle:1:1320)
    at VM4 sandbox_bundle:160:455

What I've tried to fix this

Remove my use of underscore – this leads to the following error:

VM4 sandbox_bundle:93 Error: module not found: ./ELECTRON_EVENTS
    at preloadRequire (VM4 sandbox_bundle:93:1417)
    at <anonymous>:7:25
    at runPreloadScript (VM4 sandbox_bundle:93:2244)
    at Object.<anonymous> (VM4 sandbox_bundle:93:2511)
    at Object../lib/sandboxed_renderer/init.ts (VM4 sandbox_bundle:93:2667)
    at __webpack_require__ (VM4 sandbox_bundle:1:170)
    at VM4 sandbox_bundle:1:1242
    at ___electron_webpack_init__ (VM4 sandbox_bundle:1:1320)
    at VM4 sandbox_bundle:160:455
(anonymous) @ VM4 sandbox_bundle:93

Okay, so that lead me to the following realization (from the Electron docs):

Because the require function is a polyfill with limited functionality, you will not be able to use CommonJS modules to separate your preload script into multiple files. If you need to split your preload code, use a bundler such as webpack or Parcel.

So that tells me I need to use webpack to precompile the preload script (since I'm already using webpack for my app).

Use webpack to bundle the preload script with my local code

So I implement something like this to do that:

webpack.preload.js

const path = require('path');

module.exports = {
    entry: {
        preload: './desktop/contextBridge.js',
    },
    output: {
        filename: '[name].bundle.js',
        path: path.resolve(__dirname, '../../dist'),
    },
    target: 'node',
};

Running that webpack script in isolation seems to work, so then I used webpack-merge to merge it with my main webpack config:

const {merge} = require('webpack-merge');
const preloadConfig = require('./webpack.preload.js');

let webpackConfig = {
    // Here's my main app webpack config
    ...
    ...

    devServer: {
        ...
        ...
        // Ensure that webpack-dev-server makes the preload bundle available on disk
        writeToDisk: filepath => /preload.bundle.js$/.test(filepath),
    }
};

webpackConfig = merge(webpackConfig, preloadConfig);

Then (in Electron main process) point my preload at the bundled file:

// Verify with a console log that the file exists:
console.log('RORY_DEBUG', fs.readdirSync(`${__dirname}/../dist`));
console.log('RORY_DEBUG', fs.existsSync(`${__dirname}/../dist/preload.bundle.js)`));

const browserWindow = new BrowserWindow({
    webPreferences: {
        preload: `${__dirname}/../dist/preload.bundle.js`,
        contextIsolation: true,
    },
});

Sadly, after doing all that, I still get the following errors:

VM113 renderer_init:73 Unable to load preload script: /Users/roryabraham/Expensidev/App/desktop/../dist/preload.bundle.js
(anonymous) @ VM113 renderer_init:73
VM113 renderer_init:73 Error: Cannot find module '/Users/roryabraham/Expensidev/App/desktop/../dist/preload.bundle.js'
    at Module._resolveFilename (node:internal/modules/cjs/loader:940:15)
    at Function.o._resolveFilename (node:electron/js2c/renderer_init:33:1095)
    at Module._load (node:internal/modules/cjs/loader:785:27)
    at Function.c._load (node:electron/js2c/asar_bundle:5:13331)
    at Function.o._load (node:electron/js2c/renderer_init:33:356)
    at Object.<anonymous> (node:electron/js2c/renderer_init:73:2203)
    at Object../lib/renderer/init.ts (node:electron/js2c/renderer_init:73:2330)
    at __webpack_require__ (node:electron/js2c/renderer_init:1:170)
    at node:electron/js2c/renderer_init:1:1242
    at ___electron_webpack_init__ (node:electron/js2c/renderer_init:1:1310)

Looking at this, I determined that what was probably happening was that Electron was creating my BrowserWindow before webpack was done compiling.

Make sure the webpack build is done before running Electron

Next, I made sure that the webpack build was done before running Electron (first confirmed by viewing my app in the browser, then running Electron). Still, I get the following:

Unable to load preload script
Uncaught ReferenceError: require is not defined

I also saw the following errors:

Error: Electron failed to install correctly

Interestingly, it looks like I might an invalid path for my preload bundle?

Conclusion

My brain hurts ... help? 🙃 🙏

[reZach]

@roryabraham Thanks! I'm glad this repo helped you along. In my limited understanding of your problem, I'd recommend keeping only IPC channels defined in the preload.js. Everything that should be required should be in your main script. This of course means you have to manually type out whitelisted channels in the preload file. I think that at the very least should get you past your "Uncaught ReferenceError: require is not defined" errors. (It also might remove the "Error: Electron failed to install correctly" error which might be a show-this-error-if-any-errors-occur-on-init and not representative of an actual/other problem).