Secure our Electron implementation #7567

roryabraham · 2022-02-04T17:56:50Z

Details

This PR implements many of the security best practices outlined in Electron's documentation. There is a lot of context behind why these changes are a good idea, but this is one of the best resources I've found with a good summary: https://github.com/reZach/secure-electron-template/blob/master/docs/newtoelectron.md.

TL;DR we're completely separating the website code (renderer process) from the powerful Node.js and Electron API's, and any communication between the renderer and main processes (which can use those APIs) has to happen through whitelisted channels. There's also some additional context in https://github.com/Expensify/Expensify/issues/193886

Fun fact: A random side-effect of this PR is that if you run npm run desktop, you can also view the renderer process of the desktop app in your browser by going to localhost:PORT. This is almost the same as running npm run web, with the exception of index.desktop.js files will be used instead of index.website.js.

Remaining TODOS:

Enable sandbox mode (I couldn't get this working, posted a comment here).
Run a local production build and verify it works as expected.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/193886
$ https://github.com/Expensify/Expensify/issues/166153

Tests / QA steps.

Run the desktop app.
Verify that you can sign in and sign out.
Paste an external link in the chat, verify that clicking that link opens it in your default browser.
Verify that you can right-click in the compose bar to expose a context menu.
In the app menu, click on View Keyboard Shortcuts. Verify the keyboard shortcuts modal opens. This verifies that events from the main process are being sent to the renderer process correctly.
Run the web app (or just open the desktop app in your browser as noted in the Fun Fact above) and sign into a different account.
Make sure the desktop app has a chat open with a different user than the one you're signed into web as.
Send a message from the web account to the desktop account.
Verify that the desktop app's unread count increases. This verifies that events from the renderer process are being sent to the main process correctly.
(dev only) Run a local production build and repeat the test steps in there.

Verify that no new errors appear in the JS console (Be extra on the lookout for this with this PR)

Tested On

Screenshots

Desktop

Successful local production build

botify · 2022-02-04T17:56:52Z

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

roryabraham · 2022-02-04T18:30:40Z

config/webpack/webpack.common.js

@@ -146,8 +146,4 @@ const webpackConfig = {
    },
 };

-if (platform === 'desktop') {
-    webpackConfig.target = 'electron-renderer';


This is no longer needed – because we're using context isolation, the renderer process (our app that's bundled by webpack) is just like a regular web app without any direct access to Electron or Node APIs, except those defined in the contextBridge

roryabraham · 2022-02-04T18:31:49Z

desktop/main.js

@@ -145,7 +141,8 @@ const mainWindow = (() => {
                width: 1200,
                height: 900,
                webPreferences: {
-                    nodeIntegration: true,
+                    preload: `${__dirname}/contextBridge.js`,


These lines right here are the most significant changes

roryabraham · 2022-02-04T18:32:44Z

desktop/main.js

-                // make sure local urls stay in electron perimeter
-                if (url.substr(0, 'file://'.length) === 'file://') {
-                    return;
+            browserWindow.webContents.setWindowOpenHandler(({url}) => {


The new-window event was deprecated and replaced by this setWindowOpenHandler API in Electron 12.

roryabraham · 2022-02-04T18:33:46Z

desktop/main.js

+                const denial = {action: 'deny'};
+
+                // Make sure local urls stay in electron perimeter
+                if (url.substr(0, 'file://'.length).toLowerCase() === 'file://') {


Note: it used to be possible to open apps and files (bypassing the blacklist we had here), simply by using a URL like File:// or fiLE://.

desktop/contextBridge.js

marcaaron

Ooh nice changes!

desktop/contextBridge.js

neil-marcellini

The tests work well and these look like good changes based on the Electron recommendations.

AndrewGable · 2022-02-07T21:17:22Z

Is merging blocked on this remaining todo? Enable sandbox mode ?

roryabraham · 2022-02-07T21:51:25Z

Is merging blocked on this remaining todo? Enable sandbox mode ?

Nope. We can address that in a follow-up.

OSBotify · 2022-02-08T01:48:26Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-02-09T18:05:25Z

🚀 Deployed to staging by @AndrewGable in version: 1.1.38-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-02-15T18:51:07Z

🚀 Deployed to production by @Julesssss in version: 1.1.38-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham added 11 commits February 2, 2022 16:35

Make file url blacklist case-insensitive

1130e03

Disable node-integration and enable context isolation

ea9962d

Delete unused markdown file

0b4d4bd

Rename window.electronContextBridge to window.electron

a6f9119

Fix context bridge whitelist

c1c5ca9

Rename preload to contextBridge

e5c7140

Upgrade to electron 12.x.x

66c3706

Upgrade electron to 17.0.0

1968707

Enable chromium sandbox

2a8fa20

Enable web-security

3535094

Disable sandbox

9f88a52

roryabraham self-assigned this Feb 4, 2022

Add SHOW_KEYBOARD_SHORTCUTS_MODAL to whitelist

4705676

roryabraham commented Feb 4, 2022

View reviewed changes

roryabraham added 2 commits February 4, 2022 11:23

Update the desktop README

2e085c9

Patch README html

fb6d3f6

roryabraham commented Feb 4, 2022

View reviewed changes

desktop/contextBridge.js Outdated Show resolved Hide resolved

Implement sendSync

cec59b0

roryabraham marked this pull request as ready for review February 4, 2022 19:54

roryabraham requested a review from a team as a code owner February 4, 2022 19:54

MelvinBot requested review from neil-marcellini and removed request for a team February 4, 2022 19:55

roryabraham requested review from marcaaron and AndrewGable February 4, 2022 19:55

marcaaron reviewed Feb 4, 2022

View reviewed changes

desktop/contextBridge.js Outdated Show resolved Hide resolved

Exit both renderer and main processes together

cb23086

roryabraham added 3 commits February 4, 2022 14:52

Throw an error instead of logging a warning

eb2fa40

Add JSDocs in ContextBridge

903dc0f

Add comment to contextBridge explaining how to access those methods

263ef8d

neil-marcellini approved these changes Feb 7, 2022

View reviewed changes

AndrewGable approved these changes Feb 8, 2022

View reviewed changes

AndrewGable merged commit 95c0292 into main Feb 8, 2022

AndrewGable deleted the Rory-SecureElectronImplementation branch February 8, 2022 01:47

OSBotify mentioned this pull request Feb 9, 2022

Deploy Checklist: New Expensify 2022-02-09 #7655

Closed

49 tasks

roryabraham mentioned this pull request Feb 11, 2022

[$1000] Desktop app is crashing #7642

Closed

roryabraham mentioned this pull request Feb 23, 2022

[HOLD for payment 2022-03-02] Conversation - A single message was sent 100s of times #7666

Closed

roryabraham mentioned this pull request Jul 8, 2022

[HOLD for payment 2022-08-11] [$250] Electron log file not working #9794

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure our Electron implementation #7567

Secure our Electron implementation #7567

roryabraham commented Feb 4, 2022 •

edited

Loading

botify commented Feb 4, 2022

roryabraham Feb 4, 2022

roryabraham Feb 4, 2022

roryabraham Feb 4, 2022

roryabraham Feb 4, 2022

marcaaron left a comment

neil-marcellini left a comment

AndrewGable commented Feb 7, 2022

roryabraham commented Feb 7, 2022

OSBotify commented Feb 8, 2022

OSBotify commented Feb 9, 2022

OSBotify commented Feb 15, 2022

Secure our Electron implementation #7567

Secure our Electron implementation #7567

Conversation

roryabraham commented Feb 4, 2022 • edited Loading

Details

Fixed Issues

Tests / QA steps.

Tested On

Screenshots

Desktop

botify commented Feb 4, 2022

roryabraham Feb 4, 2022

Choose a reason for hiding this comment

roryabraham Feb 4, 2022

Choose a reason for hiding this comment

roryabraham Feb 4, 2022

Choose a reason for hiding this comment

roryabraham Feb 4, 2022

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

AndrewGable commented Feb 7, 2022

roryabraham commented Feb 7, 2022

OSBotify commented Feb 8, 2022

OSBotify commented Feb 9, 2022

OSBotify commented Feb 15, 2022

roryabraham commented Feb 4, 2022 •

edited

Loading