Skip to content

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

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.

Sign up for GitHub

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

Merged
tgolen merged 8 commits into from
Feb 10, 2022
+78 −13
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2561aea
Set up isProduction flag in Electron main process
roryabraham Feb 9, 2022
c77bbef
Fix CORS issues in electron application
roryabraham Feb 9, 2022
759af3a
Fix JS style
roryabraham Feb 9, 2022
d3e7dd5
Move ENVIRONMENT constant to a CJS module
roryabraham Feb 10, 2022
3cf3eb9
Create separate module for ELECTRON_ENVIRONMENT
roryabraham Feb 10, 2022
e47cc83
Correct comment to use PROD instead of production
roryabraham Feb 10, 2022
5d7c549
Add more detailed comment to explain CORS fix
roryabraham Feb 10, 2022
0424b45
Don't export getEnvironment
roryabraham Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/electron.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
const ENVIRONMENT = require('../src/CONST/ENVIRONMENT');

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

@tgolen tgolen Feb 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@neil-marcellini neil-marcellini Feb 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@roryabraham roryabraham Feb 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@neil-marcellini neil-marcellini Feb 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me.

},
mac: {
category: 'public.app-category.finance',
Expand Down
30 changes: 30 additions & 0 deletions desktop/ELECTRON_ENVIRONMENT.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// This variable is injected into package.json by electron-builder via the extraMetadata field (specified in electron.config.js)
// It will be `PROD` on production, `STG` on staging, and `undefined` on dev (because dev doesn't use electron-builder)
const {electronEnvironment} = require('../package.json');
const ENVIRONMENT = require('../src/CONST/ENVIRONMENT');

/**
* @returns {String} – One of ['PROD', 'STG', 'DEV']
*/
function getEnvironment() {
// If we are on dev, then the NODE_ENV environment variable will be present (set by the executing shell in start.js)
if (process.env.NODE_ENV === 'development') {
return ENVIRONMENT.DEV;
}

// Otherwise, use the environment injected into package.json by electron-builder
return electronEnvironment;
}

function isDev() {
return getEnvironment() === ENVIRONMENT.DEV;
}

function isProd() {
return getEnvironment() === ENVIRONMENT.PRODUCTION;
}

module.exports = {
isDev,
isProd,
};
42 changes: 36 additions & 6 deletions desktop/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ const serve = require('electron-serve');
const contextMenu = require('electron-context-menu');
const {autoUpdater} = require('electron-updater');
const log = require('electron-log');
const ELECTRON_ENVIRONMENT = require('./ELECTRON_ENVIRONMENT');
const ELECTRON_EVENTS = require('./ELECTRON_EVENTS');
const checkForUpdates = require('../src/libs/checkForUpdates');

const isDev = process.env.NODE_ENV === 'development';
const port = process.env.PORT || 8080;

/**
Expand All @@ -41,7 +41,7 @@ autoUpdater.logger.transports.file.level = 'info';
_.assign(console, log.functions);

// setup Hot reload
if (isDev) {
if (ELECTRON_ENVIRONMENT.isDev()) {
try {
require('electron-reloader')(module, {
watchRenderer: false,
Expand Down Expand Up @@ -124,12 +124,12 @@ const electronUpdater = browserWindow => ({
});

const mainWindow = (() => {
const loadURL = isDev
const loadURL = ELECTRON_ENVIRONMENT.isDev()
? win => win.loadURL(`http://localhost:${port}`)
: serve({directory: `${__dirname}/../dist`});

// Prod and staging set the icon in the electron-builder config, so only update it here for dev
if (isDev) {
if (ELECTRON_ENVIRONMENT.isDev()) {
app.dock.setIcon(`${__dirname}/icon-dev.png`);
app.setName('New Expensify');
}
Expand All @@ -147,8 +147,38 @@ const mainWindow = (() => {
titleBarStyle: 'hidden',
});

/*
* The default origin of our Electron app is app://- instead of https://new.expensify.com or https://staging.new.expensify.com
* This causes CORS errors because the referer and origin headers are wrong and the API responds with an Access-Control-Allow-Origin that doesn't match app://-
*
* To fix this, we'll:
*
* 1. Modify headers on any outgoing requests to match the origin of our corresponding web environment.
* 2. Modify the Access-Control-Allow-Origin header of the response to match the "real" origin of our Electron app.
*/
if (!ELECTRON_ENVIRONMENT.isDev()) {
const newDotURL = ELECTRON_ENVIRONMENT.isProd() ? 'https://new.expensify.com' : 'https://staging.new.expensify.com';

// Modify the origin and referer 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

@tgolen tgolen Feb 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@neil-marcellini neil-marcellini Feb 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 roryabraham Feb 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tgolen tgolen Feb 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@roryabraham roryabraham Feb 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

details.requestHeaders.origin = newDotURL;
// eslint-disable-next-line no-param-reassign
details.requestHeaders.referer = newDotURL;
callback({requestHeaders: details.requestHeaders});
});

// Modify access-control-allow-origin header for the response
browserWindow.webContents.session.webRequest.onHeadersReceived(validDestinationFilters, (details, callback) => {
// eslint-disable-next-line no-param-reassign
details.responseHeaders['access-control-allow-origin'] = ['app://-'];
callback({responseHeaders: details.responseHeaders});
});
}

// Prod and staging overwrite the app name in the electron-builder config, so only update it here for dev
if (isDev) {
if (ELECTRON_ENVIRONMENT.isDev()) {
browserWindow.setTitle('New Expensify');
}

Expand Down Expand Up @@ -286,7 +316,7 @@ const mainWindow = (() => {

// Start checking for JS updates
.then((browserWindow) => {
if (isDev) {
if (ELECTRON_ENVIRONMENT.isDev()) {
return;
}

Expand Down
5 changes: 5 additions & 0 deletions src/CONST/ENVIRONMENT.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = {
DEV: 'DEV',
STAGING: 'STG',
PRODUCTION: 'PROD',
};
11 changes: 4 additions & 7 deletions src/CONST.js → src/CONST/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import lodashGet from 'lodash/get';

@tgolen tgolen Feb 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@roryabraham roryabraham Feb 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import Config from 'react-native-config';
import * as Url from './libs/Url';
import ENVIRONMENT from './ENVIRONMENT';
import * as Url from '../libs/Url';

const CLOUDFRONT_URL = 'https://d2k5nsl2zxldvw.cloudfront.net';
const ACTIVE_ENVIRONMENT_NEW_EXPENSIFY_URL = Url.addTrailingForwardSlash(lodashGet(Config, 'EXPENSIFY_URL_CASH', 'https://new.expensify.com'));
Expand Down Expand Up @@ -390,12 +391,6 @@ const CONST = {
ADMIN: 'admin@expensify.com',
},

ENVIRONMENT: {
DEV: 'DEV',
STAGING: 'STG',
PRODUCTION: 'PROD',
},

// Used to delay the initial fetching of reportActions when the app first inits or reconnects (e.g. returning
// from backgound). The times are based on how long it generally seems to take for the app to become interactive
// in each scenario.
Expand Down Expand Up @@ -614,4 +609,6 @@ const CONST = {
},
};

CONST.ENVIRONMENT = ENVIRONMENT;

export default CONST;