Do not leak VSCode related environment variables to debug targets

[weinand]

Leaking the ELECTRON_NO_ASAR environment variable makes Electron based apps started from VS Code fail (because they can no longer load scripts from *.asar archives).

For more details see #30105

[bpasero]

@weinand can we start slowly and remove it from the place where we launch an application from debug? I do not see an easy way in the extension host to avoid leaking this variable anywhere.

[weinand]

@bpasero leakingELECTRON_NO_ASAR from the extension host is probably not the problem because we do not launch debug adapters from the EH. We start DAs from the renderer.

I think we should remove ELECTRON_NO_ASAR in the same place where we remove the ELECTRON_RUN_AS_NODE: https://github.com/Microsoft/vscode/blob/37223d05a7389525990d9c94e2a93dc84beaad9a/src/vs/base/node/stdForkStart.js#L152
@alexandrudima is this correct?

For our debug extensions we will start to support that environment variables can be removed, so this will give users at least a workaround for the problem.

[bpasero]

@weinand that is a bit different though. our motivation for actually setting ELECTRON_NO_ASAR as environment variable in forked processes is so that we ensure NO process spawned from any process gets into the business of being penalised for ASAR lookups. We actually WANT this variable to leak through all processes because ASAR is never used.

Can we add code somewhere where we spawn a process from debug to have this variable removed?

This issue would go away if we switched to using a standalone node.js executable instead of going through Electron.

[weinand]

Sure, we can remove the ELECTRON_NO_ASAR in all our debug extensions.
But the same needs to be done for every extension that runs an external program (because potentially the program could be based on Electron).

I wonder whether Electron always reads the ELECTRON_NO_ASAR from process.env or whether it caches the value. In the latter case we could remove ELECTRON_NO_ASAR after Electron has cached it.

[weinand]

yes, it seems so: from https://electron.atom.io/releases:

The check for disabling ASAR support via the ELECTRON_NO_ASAR environment variable is now cached for better performance. #7978

[bpasero]

@weinand it is a tradeoff, there are 2 cases:

• an extension forks a process via node and every file system access will execute ASAR code even though the extension is unlikely to use ASAR
• an extension forks a process to run electron and fails due to the environment variable

I believe that the former case is the more common case compared to the latter. If we remove ELECTRON_NO_ASAR from the extension host, every extension pays the extra cost on each file system access.

[weinand]

I'm not asking for removing ELECTRON_NO_ASAR from the extension host (so no fs code should suffer).
I'm only asking for not leaking ELECTRON_NO_ASAR to sub-processes unnecessarily (because most of them are not based on electron anyway).

Basically I try to find an "opt-in" approach instead of an "opt-out" approach.

So whenever we fork electron with ELECTRON_RUN_AS_NODE=1 we want to add the ELECTRON_NO_ASAR as well (because that's the only combination where ELECTRON_NO_ASAR makes sense). After the fork, ELECTRON_NO_ASAR could be deleted from the env (because it is cached and all fs operation will continue to use it anyway).

(BTW, I noticed in electron/electron@bea1a06 that a process.noAsar takes precedence over the ELECTRON_NO_ASAR. So may be we could just set process.noAsar in https://github.com/Microsoft/vscode/blob/37223d05a7389525990d9c94e2a93dc84beaad9a/src/vs/base/node/stdForkStart.js which would probably prevent the leaking)

[bpasero]

After discussion we think a good solution would be if the debug adapter could get access to the actual shell environment that we resolve right on startup and then use that environment when spawning the debug target. This would ensure that the target sees the exact same environment that it would see if it was spawned from the terminal.

[bpasero]

@weinand I found getShellEnvironment() which unfortunately is not implemented for Windows, so we are missing this for that OS. It also seems we never use this if VSCode is launched from the command line. So I think what needs to happen here is:

• implement it for windows (maybe @Tyriar has this code already for the integrated terminal)
• wire this into a service

[johannesegger]

I'm having a similar issue originally posted at the wrong repository it seems.

When debugging a C# app that starts an electron-based app via the .NET API Process.Start, the UI of the electron app doesn't show up. But this only happens when debugging through what the omnisharp extension calls internalConsole (output is printed to the VSCode Debug Console). If ELECTRON_RUN_AS_NODE is removed from the environment before starting the electron app the UI does show up, so everything works.

Although I didn't try I would assume that all electron-based child processes started by a debugged app - be it C# or anything else - have this issue.

[bpasero]

@weinand I feel this either needs a plan what to be done or close as we have not looked into this for almost 4 years.

[connor4312]

Taking care of this for (js) debugging, imo other debuggers should sanitize in the same way if they inherit process.env (which they may not want to do, in linked issue)