[CP Stg] Fix CORS errors in desktop application #7665

roryabraham · 2022-02-09T22:38:51Z

Details

Fixes CORS errors which were introduced by enabling web-security in this PR

Fixed Issues

$ #7659

Tests

Comment out these lines in the Electron config.
Run a local desktop production build: export SHOULD_DEPLOY_PRODUCTION=true && npm run desktop-build.
Once that completes, install the app. Verify that the build completes successfully and that you can log in and out.
Open the JS console (OPT+CMD+i), verify that there are no errors.
Uninstall the locally-build production desktop app by dragging the application to the trash.
Run a local desktop staging build: export SHOULD_DEPLOY_PRODUCTION=false && npm run desktop-build-staging
Repeat steps 3-5 with the locally built staging app.

Verify that no errors appear in the JS console

QA Steps

Verify that you can log out and back in.

Verify that no errors appear in the JS console

Tested On

Screenshots

Desktop

github-actions · 2022-02-09T22:39:39Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

roryabraham · 2022-02-09T22:42:52Z

Also see this SO regarding the isProduction bit, I explained in a bit more detail there.

neil-marcellini · 2022-02-09T22:53:52Z

👀

desktop/main.js

tgolen · 2022-02-09T23:04:12Z

desktop/main.js

@@ -147,6 +150,27 @@ const mainWindow = (() => {
                titleBarStyle: 'hidden',
            });

+            if (!isDev) {
+                const newDotURL = isProduction ? 'https://new.expensify.com' : 'https://staging.new.expensify.com';


It feels like it would be cleaner to have a single variable called env that would be either dev, staging, or production. Is there anyway we can use the ENVIRONMENT variable from the .env files?

I agree one env variable would be cleaner.

It feels like it would be cleaner to have a single variable called env that would be either dev, staging, or production.

Let me look into this.

Is there anyway we can use the ENVIRONMENT variable from the .env files?

Maybe, but the .env files are consumed using react-native-config and/or react-web-config. This is the main process, running in an Electron context, more akin to a Node.js environment than a web environment. It might be possible to use dotenv separately to consume the .env files, but we'd still need a way to tell Electron which environment it's running in.

Is there anyway we can use the ENVIRONMENT variable from the .env files?

Expanding on this some more, we'd have no way of knowing which .env file to use. For the main web app, we have a separate webpack config that specifies this for us. However, the code in desktop/main.js isn't touched by webpack. Still thinking of how this can be cleaned up.

tgolen · 2022-02-09T23:06:43Z

desktop/main.js

+                // Modify the request origin for requests sent to our API
+                const validDestinationFilters = {urls: ['https://*.expensify.com/*']};
+                browserWindow.webContents.session.webRequest.onBeforeSendHeaders(validDestinationFilters, (details, callback) => {
+                    // eslint-disable-next-line no-param-reassign


I think you could get around this by using const newDetails = {...details}?

I think const newRequestHeaders = {...details.requestHeaders} would be better since that is all that is being modified and const newDetails = {...details} would create a shallow copy AFAIK.

I actually took one out of your playbook from this PR ... this code will be executed before and after every api request, so performance matters. Using the spread operator would cause us to perform an unnecessary shallow copy of all the headers for every request twice per request, which seems less good than just overwriting a couple properties.

Yeah, that's probably a good call. I had also thought of that, and the fact that this is done on every request is a good reason to keep it very performant. The only caveat I will say is that in my PR, I think the spread operator is not performant because of how Babel chooses to transpile it into _spreadOperator(). Since this is in Node.js and nothing is transpiled, I would think it's much more performant.

Something else to consider. In my PR, since there was a reducer used along with the spread operator, it could have been compounding the problem. This is mentioned specifically in this SO: https://stackoverflow.com/questions/55843097/does-spread-operator-affect-performance

beware of using spread with an array.reduce(). I suspect it leads to O(n^2) behavior or worse.

So, I'm not really sure that using a spread operator here is such a bad thing.

Thanks, I wasn't aware of that context, especially regarding Babel. Very good to know about watching out for a spread operator in reducers! (maybe we could create a lint rule for that?)

So, I'm not really sure that using a spread operator here is such a bad thing.

That makes sense – I agree it might not create a performance problem to use the spread operator here. But still I would lean towards leaving this as-is because I think it will be "technically" more performant in a very performance-critical place, which seems worth the couple eslint-disables to me. I don't feel as strongly anymore though, so I'll change it if you prefer.

neil-marcellini

The local production and staging builds test well for me. After some tweaks it should be good.

neil-marcellini · 2022-02-09T23:24:29Z

desktop/main.js

@@ -147,6 +150,27 @@ const mainWindow = (() => {
                titleBarStyle: 'hidden',
            });

+            if (!isDev) {
+                const newDotURL = isProduction ? 'https://new.expensify.com' : 'https://staging.new.expensify.com';


I agree one env variable would be cleaner.

neil-marcellini · 2022-02-09T23:29:18Z

desktop/main.js

+                // Modify the request origin for requests sent to our API
+                const validDestinationFilters = {urls: ['https://*.expensify.com/*']};
+                browserWindow.webContents.session.webRequest.onBeforeSendHeaders(validDestinationFilters, (details, callback) => {
+                    // eslint-disable-next-line no-param-reassign


I think const newRequestHeaders = {...details.requestHeaders} would be better since that is all that is being modified and const newDetails = {...details} would create a shallow copy AFAIK.

roryabraham · 2022-02-10T03:40:56Z

I cleaned up the environment stuff a bit by creating a separate module for ELECTRON_ENVIRONMENT. Hopefully that helps make things clearer. I'm open to suggestions for further improvements, but I'm at a loss for new ideas at the moment.

tgolen · 2022-02-10T14:14:25Z

src/CONST/index.js

@@ -1,6 +1,7 @@
 import lodashGet from 'lodash/get';


Why did the name of this file change? I don't think it should be changed. The only time we should ever have an index.js is when it's in a directory used to load platform specific files (eg. index.js, index.website.js, index.ios.js)

This got renamed because of CJS and ESM modules not playing nice together. I wanted to use CONST.ENVIRONMENT from the ELECTRON_ENVIRONMENT CJS module, but can't require an ESM module from Node.js. So we end up with a setup like this:

src/CONST/ ---------| ENVIRONMENT.js // CJS module with just CONST.ENVIRONMENT ---------| index.js // ESM module with all our existing CONST stuff. This imports ENVIRONMENT.js as a CJS module and exports it as an ESM module desktop/ELECTRON_ENVIRONMENT.js // This imports src/CONST/ENVIRONMENT.js directly.

The only time we should ever have an index.js is when it's in a directory used to load platform specific files (eg. index.js, index.website.js, index.ios.js)

By moving CONST.js to CONST/index.js, we don't have to change any imports. We can still import it like:

import CONST from '../CONST';

And JS will automatically grab index.js from the CONST directory.

If you want me to revert this, I could just create a separate constant directly in the ELECTRON_ENVIRONMENT file and not worry about importing CONST.ENVIRONMENT into the CJS module.

tgolen · 2022-02-10T14:20:48Z

desktop/main.js

+                // Modify the request origin for requests sent to our API
+                const validDestinationFilters = {urls: ['https://*.expensify.com/*']};
+                browserWindow.webContents.session.webRequest.onBeforeSendHeaders(validDestinationFilters, (details, callback) => {
+                    // eslint-disable-next-line no-param-reassign


Yeah, that's probably a good call. I had also thought of that, and the fact that this is done on every request is a good reason to keep it very performant. The only caveat I will say is that in my PR, I think the spread operator is not performant because of how Babel chooses to transpile it into _spreadOperator(). Since this is in Node.js and nothing is transpiled, I would think it's much more performant.

Something else to consider. In my PR, since there was a reducer used along with the spread operator, it could have been compounding the problem. This is mentioned specifically in this SO: https://stackoverflow.com/questions/55843097/does-spread-operator-affect-performance

beware of using spread with an array.reduce(). I suspect it leads to O(n^2) behavior or worse.

So, I'm not really sure that using a spread operator here is such a bad thing.

tgolen · 2022-02-10T14:22:11Z

desktop/main.js

+            if (!ELECTRON_ENVIRONMENT.isDev()) {
+                const newDotURL = ELECTRON_ENVIRONMENT.isProd() ? 'https://new.expensify.com' : 'https://staging.new.expensify.com';
+
+                // Modify the request origin for requests sent to our API


Can you please add more to this comment to explain "why?". Currently, it is only explaining "what".

Added a comment above this block. Give it a read and let me know if you think it's clear enough, or if it leaves you with more questions.

desktop/ELECTRON_ENVIRONMENT.js

tgolen · 2022-02-10T14:28:51Z

config/electron.config.js

 module.exports = {
    appId: 'com.expensifyreactnative.chat',
    productName: 'New Expensify',
    extraMetadata: {
        main: './desktop/main.js',
+        electronEnvironment: process.env.SHOULD_DEPLOY_PRODUCTION ? ENVIRONMENT.PRODUCTION : ENVIRONMENT.STAGING,


This is where it still remains a bit weird for me. The electronEnvironment variable will only contain PROD or STAG. The development environment comes from process.env.NODE_ENV. Can't they all come from the same place? Like, can this code here be something like...

let env = process.env.NODE_ENV; if (env !== 'development') { env = process.env.SHOULD_DEPLOY_PRODUCTION ? ENVIRONMENT.PRODUCTION : ENVIRONMENT.STAGING; } else { env = ENVIRONMENT.DEVELOPMENT; } // ... electronEnvironment: env,

However, even in that code... if process.env.NODE_ENV is not development, then what is it? Maybe this logic can be cleaned up a little more.

The goal here would be that getEnvironment() (below) would only be looking at a single place to determine the environment.

That sounds like a good idea. As I understand it process.env.NODE_ENV would be undefined outside of development because:

function getEnvironment() { // If we are on dev, then the NODE_ENV environment variable will be present (set by the executing shell in start.js)

Can't they all come from the same place? ... The goal here would be that getEnvironment() (below) would only be looking at a single place to determine the environment.

I would love to achieve this goal, but I don't see how. You see, electron.config.js would actually be better-named electron-builder.config.js, because it's not actually used by Electron itself – only electron-builder. So there would never be a time where process.env.NODE_ENV === 'development' in electron.config.js.

When we run the development app here, we can easily set an environment variable such as NODE_ENV in the runtime, and that will be available to the Electron main process. However, when we bundle an app using electron-builder (or really, create any executable file), we can't "bake" an environment variable in AFAIK. If I were to do something like this:

export NODE_ENV=production && electron-builder --config config/electron.config.js

Then when I install and run the bundled Electron app, and try to access process.env.NODE_ENV, it will be undefined. You have to set the environment variable from the shell or environment that runs the executable.

However, electron-builder does provide a weird workaround. You can set data in the extraMetadata field in the configuration file, and then electron-builder will inject those variables into your package.json, which is available to the application at runtime.

That makes sense to me.

neil-marcellini

Aside from some small details I think this looks good.

neil-marcellini · 2022-02-10T16:52:46Z

config/electron.config.js

 module.exports = {
    appId: 'com.expensifyreactnative.chat',
    productName: 'New Expensify',
    extraMetadata: {
        main: './desktop/main.js',
+        electronEnvironment: process.env.SHOULD_DEPLOY_PRODUCTION ? ENVIRONMENT.PRODUCTION : ENVIRONMENT.STAGING,


That sounds like a good idea. As I understand it process.env.NODE_ENV would be undefined outside of development because:

function getEnvironment() { // If we are on dev, then the NODE_ENV environment variable will be present (set by the executing shell in start.js)

tgolen

Changes here are OK. There might still be some cleanup stuff for this, but let's get that deploy blocker fixed for now.

[CP Stg] Fix CORS errors in desktop application (cherry picked from commit a344c05)

OSBotify · 2022-02-10T21:05:22Z

🚀 Cherry-picked to staging by @tgolen in version: 1.1.38-2 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2022-02-15T18:50:34Z

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

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

roryabraham added 2 commits February 9, 2022 13:03

Set up isProduction flag in Electron main process

2561aea

Fix CORS issues in electron application

c77bbef

roryabraham added the CP Staging label Feb 9, 2022

roryabraham requested a review from a team February 9, 2022 22:38

roryabraham self-assigned this Feb 9, 2022

MelvinBot requested review from danieldoglas and removed request for a team February 9, 2022 22:39

Fix JS style

759af3a

roryabraham requested a review from a team as a code owner February 9, 2022 22:44

MelvinBot requested review from neil-marcellini and removed request for a team February 9, 2022 22:44

tgolen requested changes Feb 9, 2022

View reviewed changes

neil-marcellini reviewed Feb 9, 2022

View reviewed changes

roryabraham added 3 commits February 9, 2022 17:08

Move ENVIRONMENT constant to a CJS module

d3e7dd5

Create separate module for ELECTRON_ENVIRONMENT

3cf3eb9

Correct comment to use PROD instead of production

e47cc83

roryabraham requested review from tgolen and neil-marcellini February 10, 2022 04:36

roryabraham mentioned this pull request Feb 10, 2022

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

Closed

49 tasks

kidroca mentioned this pull request Feb 10, 2022

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

Closed

tgolen requested changes Feb 10, 2022

View reviewed changes

neil-marcellini reviewed Feb 10, 2022

View reviewed changes

roryabraham added 2 commits February 10, 2022 10:28

Add more detailed comment to explain CORS fix

5d7c549

Don't export getEnvironment

0424b45

tgolen approved these changes Feb 10, 2022

View reviewed changes

tgolen merged commit a344c05 into main Feb 10, 2022

tgolen deleted the Rory-FixDesktopCORS branch February 10, 2022 19:54

OSBotify pushed a commit that referenced this pull request Feb 10, 2022

Merge pull request #7665 from Expensify/Rory-FixDesktopCORS

6d127aa

[CP Stg] Fix CORS errors in desktop application (cherry picked from commit a344c05)

OSBotify mentioned this pull request Feb 10, 2022

🍒 Cherry pick PR #7665 to staging 🍒 #7683

Merged

This was referenced Feb 10, 2022

[No QA] Fix desktop CORS issue when using web proxy #7693

Merged

[No QA] Simplify electron-builder configs #7684

Merged

OSBotify mentioned this pull request Feb 16, 2022

Deploy Checklist: New Expensify 2022-02-15 #7759

Closed

85 tasks

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

tienifr mentioned this pull request Nov 22, 2023

[$1000] Desktop-"Hmm.. it's not here" page appears when the app auto closes to update #28495

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CP Stg] Fix CORS errors in desktop application #7665

[CP Stg] Fix CORS errors in desktop application #7665

roryabraham commented Feb 9, 2022

github-actions bot commented Feb 9, 2022

roryabraham commented Feb 9, 2022

neil-marcellini commented Feb 9, 2022

tgolen Feb 9, 2022

neil-marcellini Feb 9, 2022

roryabraham Feb 10, 2022

roryabraham Feb 10, 2022

tgolen Feb 9, 2022

neil-marcellini Feb 9, 2022

roryabraham Feb 10, 2022

tgolen Feb 10, 2022

roryabraham Feb 10, 2022 •

edited

Loading

neil-marcellini left a comment

neil-marcellini Feb 9, 2022

neil-marcellini Feb 9, 2022

roryabraham commented Feb 10, 2022 •

edited

Loading

tgolen Feb 10, 2022

roryabraham Feb 10, 2022

tgolen Feb 10, 2022

tgolen Feb 10, 2022

roryabraham Feb 10, 2022

tgolen Feb 10, 2022

neil-marcellini Feb 10, 2022

roryabraham Feb 10, 2022

neil-marcellini Feb 10, 2022

neil-marcellini left a comment

neil-marcellini Feb 10, 2022

tgolen left a comment

OSBotify commented Feb 10, 2022

OSBotify commented Feb 15, 2022

[CP Stg] Fix CORS errors in desktop application #7665

[CP Stg] Fix CORS errors in desktop application #7665

Conversation

roryabraham commented Feb 9, 2022

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Desktop

github-actions bot commented Feb 9, 2022

roryabraham commented Feb 9, 2022

neil-marcellini commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham commented Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

OSBotify commented Feb 10, 2022

OSBotify commented Feb 15, 2022

roryabraham Feb 10, 2022 •

edited

Loading

roryabraham commented Feb 10, 2022 •

edited

Loading