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.

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
merged 8 commits into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
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,
Copy link
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.

Copy link
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)

Copy link
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.

Copy link
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
31 changes: 31 additions & 0 deletions desktop/ELECTRON_ENVIRONMENT.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// 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 = {
getEnvironment,
isDev,
isProd,
};
33 changes: 27 additions & 6 deletions desktop/main.js
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,29 @@ const mainWindow = (() => {
titleBarStyle: 'hidden',
});

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const validDestinationFilters = {urls: ['https://*.expensify.com/*']};
browserWindow.webContents.session.webRequest.onBeforeSendHeaders(validDestinationFilters, (details, callback) => {
// eslint-disable-next-line no-param-reassign
Copy link
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}?

Copy link
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.

Copy link
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.

Copy link
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.

Copy link
Contributor Author

@roryabraham roryabraham Feb 10, 2022

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 +307,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
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import lodashGet from 'lodash/get';
Copy link
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)

Copy link
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: '[email protected]',
},

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;