[$250] Desktop - Console error when opening the app

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue found when executing PR #12296

Action Performed:

1. Open app
2. Check the console

Expected Result:

Should be no console error

Actual Result:

Error in console

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Desktop App

Version Number: 1.2.24.0

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5806549_12296_desktop1.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @flaviadefaria (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[Santhosh-Sellavel]

@kbecciv
Note: This happens on production as well
#12296 is pending production deployment

[melvin-bot]

@flaviadefaria Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

Current assignee @flaviadefaria is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[melvin-bot]

Triggered auto assignment to @Luke9389 (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[flaviadefaria]

Adding the external label. I'll create the UW post tomorrow.

[smrutiparida]

Proposal:

The issue was introduced during upgrading Electron version from from 18.3.15 to 21.0.0 by me. I certainly missed it in checking all aspects in the PR ##11836

In a feature change released in Electron V20 electron/electron#28496 (comment) Sandbox setting was defaulted to value "true" from "false" based on nodeIntegration value set in the app code.

In that change, sandbox is enabled by default when nodeIntegration is false or unspecified (i.e. this PR). Hence our present sandbox:true where as previously it was sandbox:false.

Below change shall remove the error.

Screenshot:

A similar issue where i read about the fix is discussed here - electron/forge#2931

Please review @flaviadefaria @kbecciv

[flaviadefaria]

@kbecciv @Luke9389 what are your thoughts on the solution @smrutiparida posted above?

[Santhosh-Sellavel]

Why do we need to set nodeIntegration: true, what does it do other than remove the warning?

[smrutiparida]

Why do we need to set nodeIntegration: true, what does it do other than remove the warning?

Starting from Electron 20, the sandbox is enabled by default for renderer processes. When we run mainWindow as a separate process, the process is sandboxed. The sandbox limits the harm that malicious code can cause by limiting access to most system resources — sandboxed processes can only freely use CPU cycles and memory.

However running a preload script contextBridge.js require access to nodeJS modules from the javascript code. This was not a concern since sandbox used to be false by default for Electron <= 20

Starting from Electron 20, by doing nodeIntegration: true we set sandbox: false. I assumed this is safe which I after reading further thinks are not and need to be well tested. The APIs available in sandbox option to the preload script are more limited than nodeIntegration. Hence instead, I suggest only do sandbox:false now.

sandbox:false shall enable the Node.js engine and make NodeJS's features such as NodeJS's implementation of module, exports, and require to be accessible directly from the HTML/Javascript page's script context.

The Electron FAQ here explains it https://www.electronjs.org/docs/latest/faq#i-can-not-use-jqueryrequirejsmeteorangularjs-in-electron
More - https://www.electronjs.org/docs/latest/tutorial/sandbox#disabling-the-sandbox-for-a-single-process
https://www.electronjs.org/docs/latest/api/browser-window
electron/forge@6e0a6248

@Santhosh-Sellavel

[Santhosh-Sellavel]

I suggest only do sandbox:false now.

Seems straightforward, so looks good to me! Please have a look at this when you have time @Luke9389!

[puneetlath]

@Luke9389 this is one of the oldest issues in the /App repo. To help us clear out the large backlog of bugs, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is
• For any that can't, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #bug-zero

[smrutiparida]

Not yet. Need a day more.

[Santhosh-Sellavel]

Seems like a simple and straightforward change, can you let us know what's causing the delay?

[smrutiparida]

I need to create the screenshots in all devices of the errors not coming along with iOS. which is taking some time

[Santhosh-Sellavel]

Again we are specifically modifying desktop-only code, so I think we don't need it for other platforms, only on this PR as we didn't touch native src code. Please include that in your PR description.

cc: @Luke9389

[iwiznia]

Sorry, not very clear, did we hire @smrutiparida? If so, can someone assign them so the help wanted label is removed?

[Santhosh-Sellavel]

@flaviadefaria please assign @smrutiparida issue.

@smrutiparida Any update on the PR?

[melvin-bot]

📣 @smrutiparida You have been assigned to this job by @flaviadefaria!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[flaviadefaria]

Assigned @smrutiparida to the issue 👍

[flaviadefaria]

It seems like the PR is still being reviewed.

[smrutiparida]

I have applied in upwork for the job.

[melvin-bot]

@smrutiparida, @Luke9389, @flaviadefaria, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Santhosh-Sellavel]

PR is merged

[flaviadefaria]

Heads up I'm heading OoO for a week so in case this needs to be paid out before I come back then please reassign or ask someone else in the team to process the payment. Thanks!

[JmillsExpensify]

Still working on getting the linked PR deployed.

[flaviadefaria]

It looks like the PR was deployed so I'll proceed to paying the contributors. Let me know in case I'm missing something.

[flaviadefaria]

Offers sent in UW!

[Luke9389]

Thanks @flaviadefaria

[flaviadefaria]

Contributors paid, thanks!