Skip to content

fix: ignore all NODE_ envs from foreign parent in node process #40770

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

Jump to bottom
Merged
zcbenz merged 2 commits into from
Jan 4, 2024
Merged

fix: ignore all NODE_ envs from foreign parent in node process #40770

zcbenz merged 2 commits into from
Jan 4, 2024

Conversation

@zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Dec 15, 2023

Description of Change

This is a followup to #40579.

@deepak1556 found that it is possible to work around the protection of NODE_OPTIONS by using another env NODE_REPL_EXTERNAL_MODULE, we think it is better just removing all envs start with NODE_ in case there are new creative ways invented to work around the protection.

Note that NODE_REPL_EXTERNAL_MODULE only works in repl mode so only the ELECTRON_RUN_AS_NODE processes are affected.

Checklist

Release Notes

Notes: Unset all Node envs in node process when parent is a foreign process.

@zcbenz zcbenz added semver/patch backwards-compatible bug fixes target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. labels Dec 15, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Dec 15, 2023
deepak1556
deepak1556 reviewed Dec 15, 2023
View reviewed changes
deepak1556
deepak1556 approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Thanks!

nornagon
nornagon reviewed Dec 15, 2023
View reviewed changes
shell/app/node_main.cc
auto os_env = base::Environment::Create();
bool node_options_enabled = electron::fuses::IsNodeOptionsEnabled();
if (!node_options_enabled) {
os_env->UnSetVar("NODE_OPTIONS");
Copy link
Contributor

@nornagon nornagon Dec 15, 2023

Choose a reason for hiding this comment

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

Node already has kDisableNodeOptionsEnv, which we enable. It seems like a Node bug that there are workarounds for this? Can we fix this in Node?

Copy link
Contributor Author

@zcbenz zcbenz Dec 18, 2023

Choose a reason for hiding this comment

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

kDisableNodeOptionsEnv means disabling the NODE_OPTIONS env, it is probably arguable whether it should disable other envs too, but anyway I'll bring this up to Node and will patch upstream if they agree so.

In the long term I think what we really need is a way to guarantee there is no way to inject code into an app, but I don't have a good idea how to implement it. I'll discuss with Node team too.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Dec 16, 2023
@zcbenz zcbenz mentioned this pull request Dec 20, 2023
Closed
@zcbenz
Copy link
Contributor Author

zcbenz commented Dec 20, 2023

If no opposition I would like to merge this before fixing the upstream Node, since the protection introduced by #40579 is half-baked now, and this change only affects people who do things like env ELECTRON_RUN_AS_NODE=1 /Applications/VS Code.app.

ckerr
ckerr reviewed Jan 3, 2024
View reviewed changes
@zcbenz zcbenz merged commit dfce1a9 into main Jan 4, 2024
@zcbenz zcbenz deleted the node-env-check branch January 4, 2024 07:34
@release-clerk
Copy link

release-clerk bot commented Jan 4, 2024

Release Notes Persisted

Unset all Node envs in node process when parent is a foreign process.

@trop
Copy link
Contributor

trop bot commented Jan 4, 2024

I was unable to backport this PR to "26-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/26-x-y and removed target/26-x-y PR should also be added to the "26-x-y" branch. labels Jan 4, 2024
This was referenced Jan 4, 2024
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Jan 4, 2024

I have automatically backported this PR to "27-x-y", please check out #40879

@trop trop bot added in-flight/27-x-y and removed target/27-x-y PR should also be added to the "27-x-y" branch. labels Jan 4, 2024
@trop
Copy link
Contributor

trop bot commented Jan 4, 2024

I have automatically backported this PR to "28-x-y", please check out #40880

@trop
Copy link
Contributor

trop bot commented Jan 4, 2024

I have automatically backported this PR to "29-x-y", please check out #40881

@trop trop bot added in-flight/28-x-y in-flight/29-x-y and removed target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. labels Jan 4, 2024
@zcbenz zcbenz mentioned this pull request Jan 4, 2024
Merged
@trop
Copy link
Contributor

trop bot commented Jan 4, 2024

@zcbenz has manually backported this PR to "26-x-y", please check out #40882

@trop trop bot added in-flight/26-x-y merged/27-x-y PR was merged to the "27-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. and removed needs-manual-bp/26-x-y in-flight/27-x-y in-flight/29-x-y in-flight/28-x-y in-flight/26-x-y labels Jan 4, 2024
This was referenced Jan 23, 2024
Closed
Merged
forivall added a commit to forivall/vscode that referenced this pull request Feb 5, 2024
@forivall
Clear out hijackable options in darwin/bin/code.sh
8c2c427 
Fixes microsoft#204005

Electron will log a warning when it is launched with these options, see electron/electron#40770 for more context.
@forivall forivall mentioned this pull request Feb 5, 2024
Closed
@deepak1556 deepak1556 mentioned this pull request Feb 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ckerr ckerr ckerr left review comments

@deepak1556 deepak1556 deepak1556 approved these changes

@codebytere codebytere codebytere approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

+1 more reviewer

@nornagon nornagon nornagon left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. semver/patch backwards-compatible bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zcbenz @ckerr @nornagon @deepak1556 @codebytere