Skip to content
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

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

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

roryabraham
Copy link
Contributor

Details

This fixes CORS in a way that's similar to #7665, but specifically for local development using the web proxy.

Fixed Issues

$ n/a – coming from multiple reports in slack:

Tests

  1. Ensure that, if you have a .env file, USE_NRGOK is false and USE_WEB_PROXY is true.
  2. Run npm run desktop
  3. Verify that you can log out and log back into production Expensify accounts without seeing any CORS errors.
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Desktop

No CORS errors:

image

@roryabraham roryabraham self-assigned this Feb 10, 2022
@roryabraham roryabraham requested a review from a team as a code owner February 10, 2022 23:30
@roryabraham
Copy link
Contributor Author

No need to CP this – it only affects dev (specifically dev w/ web proxy)

@roryabraham roryabraham requested review from a team and removed request for a team February 11, 2022 00:07
@MelvinBot MelvinBot requested review from nickmurray47 and removed request for a team February 11, 2022 01:16
Copy link
Contributor

@nickmurray47 nickmurray47 left a comment

Choose a reason for hiding this comment

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

LGTM

@roryabraham roryabraham merged commit 3590200 into main Feb 11, 2022
@roryabraham roryabraham deleted the Rory-FixCORSForWebProxy branch February 11, 2022 04:09
@OSBotify
Copy link
Contributor

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

Comment on lines +181 to +187
if (ELECTRON_ENVIRONMENT.isDev()) {
const dotenv = require('dotenv');
const path = require('path');
const devEnvConfig = dotenv.config({path: path.resolve(__dirname, '../.env')}).parsed;

if (devEnvConfig.USE_WEB_PROXY === 'true') {
browserWindow.webContents.session.webRequest.onHeadersReceived(validDestinationFilters, (details, callback) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've just tested this now and the app crashes / stays on white screen
This block fails with:

[Main] 04:19:38.521 › (node:97605) UnhandledPromiseRejectionWarning: TypeError: Cannot read properties of undefined (reading 'USE_WEB_PROXY')
[Main]     at /Users/kidroca/Desktop/Freelance/Expensify.cash/desktop/main.js:186:34

External contributors are instructed not to have .env files - zero config
The proxy.js script actually checks for explicitly having USE_WEB_PROXY set to false - it always starts the server otherwise

App/web/proxy.js

Lines 3 to 8 in 394c301

require('dotenv').config();
if (process.env.USE_WEB_PROXY === 'false') {
process.stdout.write('Skipping proxy as USE_WEB_PROXY was set to false.\n');
process.exit();
}

I think we should do the same thing here - run the logic unless this value exists and it's false

Copy link
Contributor

Choose a reason for hiding this comment

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

Another way is maybe PING the proxy server if it responds then it's running - use it, otherwise do nothing

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 think we should do the same thing here - run the logic unless this value exists and it's false

This sounds correct! This was reported in Slack today and I meant to fix it but unfortunately did not get around to it. Feel free to submit a PR if you don't want to wait until Monday.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.1.39-0 🚀

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

@roryabraham roryabraham mentioned this pull request Feb 18, 2022
14 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants