fix: filter invalid env keys before forking utility processes - #333333
Open
LI BINGZHEN (peaceaddpaasion) wants to merge 4 commits into
Open
LI BINGZHEN (peaceaddpaasion) wants to merge 4 commits into
LI BINGZHEN (peaceaddpaasion) wants to merge 4 commits into
Conversation
When a parent process has environment variables whose names Node.js treats as invalid (e.g. a leading digit or chars outside [A-Za-z0-9_]), forking a utility process throws 'TypeError: Invalid value for env', breaking the extension host and leaving commands/auth providers unregistered. In createEnv() the environment is built by implicitly inheriting the whole parent process.env (when no explicit env is provided). We strip any key that does not match a valid name before passing it to utilityProcess.fork().
- Move the Node env-key legality predicate out of the UtilityProcess class into src/vs/platform/utilityProcess/common/envKey.ts so it can be shared and unit-tested in isolation. - Add envKey.test.ts covering valid names, leading digits and illegal chars (incl. real-world CJK/CLion/IntelliJ/PATH(x86) cases). - utilityProcess.ts now imports the shared predicate; behavior unchanged (filter Node-invalid keys before fork to avoid 'Invalid value for env').
Author
|
@microsoft-github-policy-service agree |
Joaquín Ruales (jruales)
assigned
Robo (deepak1556) and unassigned
Joaquín Ruales (jruales)
Author
|
Joaquín Ruales (@jruales) Robo (@deepak1556) I synced the PR branch with upstream main, which created a new merge commit and dismissed all prior approvals.There are no actual code changes in this update; only the merge from main.Could you kindly re-review and re-approve this PR when you have time? Thanks a lot. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The utility process extension host inherits the entire environment block via the default generic env processing. On Windows it is possible for the current process environment to contain keys that Node.js considers illegal (for example a variable whose name starts with a digit, often left behind by installers or tampered environments). Because the inherited block is passed on to the forked utility process, the utility process fails to launch with Node \TypeError: Invalid value for env, and the whole extension-host stack silently dies while the front-end UI stays up (built-in commands \command not found, GitHub auth provider never registers, Copilot
o bundle location, logs never initialize).
This makes a trivially-poisoned environment take down all utility processes with no crash surface and no diagnostic trail.
Changes
The patch is minimal and purely defensive; legal keys are preserved unchanged.
Why not reuse
emoveDangerousEnvVariables?
emoveDangerousEnvVariables\ targets specific high-risk keys/values inside an already-valid key namespace (e.g. \NODE_OPTIONS). This fix addresses a different layer: the key itself is not a valid identifier, which Node hard-fails on and which no value-stripping can repair. They are complementary, so a separate pure predicate keeps the boundary check explicit.
Testing
Related
Closes nothing; complements the research issue #333316