win, child_process: sanitize env variables #35210

bzoz · 2020-09-15T14:43:30Z

On Windows environment variables are case-insensitive. When spawning
child process certain apps can get confused if some of the variables are
duplicated.

This adds a step on Windows to normalizeSpawnArguments that removes such
duplicates, keeping only the first (in lexicographic order) entry in the
env key of options. This is partly already done for the PATH entry.

Fixes: #35129

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax · 2020-09-15T14:58:34Z

doc/api/child_process.md

@@ -43,6 +43,10 @@ the first one case-insensitively matching `PATH` to perform command lookup.
 This may lead to issues on Windows when passing objects to `env` option that
 have multiple variants of `PATH` variable.

+On Windows Node.js will sanitize the `env` by removing case-insensitive
+duplicates. Only first (in lexicographic order) entry will be passed to the
+child process.


In JS object syntax, including object spread, it’s the last key that wins, so I it might be a bit more intuitive to use that?

It chooses the first one for PATH (the doc about this is one line above this change), so I've kept that.

IIRC the PATH deduplication was done in libuv. Should we be doing the deduplication all in one place?

lexicographic order is correct in order to match the expectation of platform APIs like GetEnvironmentVariable which stops at the first lexicographic match

@richardlau I expect this change to break stuff. It probably should be semver-major, which is kinda impossible to do in libuv.

mscdex · 2020-09-15T15:22:38Z

lib/child_process.js

+    // keeping only the first one (in lexicographic order)
+    const sawKey = new Set();
+    envKeys = envKeys.sort().filter((key) => {
+      const upercaseKey = key.toUpperCase();


Suggested change

const upercaseKey = key.toUpperCase();

const uppercaseKey = key.toUpperCase();

mscdex · 2020-09-15T15:23:04Z

lib/child_process.js

+    const sawKey = new Set();
+    envKeys = envKeys.sort().filter((key) => {
+      const upercaseKey = key.toUpperCase();
+      if (sawKey.has(upercaseKey)) {


Suggested change

if (sawKey.has(upercaseKey)) {

if (sawKey.has(uppercaseKey)) {

mscdex · 2020-09-15T15:23:09Z

lib/child_process.js

+      if (sawKey.has(upercaseKey)) {
+        return false;
+      }
+      sawKey.add(upercaseKey);


Suggested change

sawKey.add(upercaseKey);

sawKey.add(uppercaseKey);

nodejs-github-bot · 2020-09-23T08:37:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33208/

bzoz · 2020-09-23T08:37:45Z

Updated&rebased, PTAL.

nodejs-github-bot · 2020-09-24T09:04:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33226/

nodejs-github-bot · 2020-09-24T13:28:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33232/

bzoz · 2020-09-24T13:54:11Z

Rebased, CI is green.

On Windows environment variables are case-insensitive. When spawning child process certain apps can get confused if some of the variables are duplicated. This adds a step on Windows to normalizeSpawnArguments that removes such duplicates, keeping only the first (in lexicographic order) entry in the env key of options. This is partly already done for the PATH entry. Fixes: nodejs#35129

nodejs-github-bot · 2020-09-24T14:16:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33233/

nodejs-github-bot · 2020-09-24T16:03:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33234/

On Windows environment variables are case-insensitive. When spawning child process certain apps can get confused if some of the variables are duplicated. This adds a step on Windows to normalizeSpawnArguments that removes such duplicates, keeping only the first (in lexicographic order) entry in the env key of options. This is partly already done for the PATH entry. Fixes: #35129 PR-URL: #35210 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

bzoz · 2020-09-24T16:26:06Z

Landed in abd8cdf

Notable changes: Deprecations and Removals: - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **timers**: introduce timers/promises (James M Snell) (#33950) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) npm 7 (#35631): Node.js 15 comes with a new major release of npm, npm 7. npm 7 comes with many new features - including npm workspaces and a new package-lock.json format. npm 7 also includes yarn.lock file support. One of the big changes in npm 7 is that peer dependencies are now installed by default. Throw On Unhandled Rejections (#33021): As of Node.js 15, the default mode for `unhandledRejection` is changed to `throw` (from `warn`). In `throw` mode, if an `unhandledRejection` hook is not set, the `unhandledRejection` is raised as an uncaught exception. Users that have an `unhandledRejection` hook should see no change in behavior, and it’s still possible to switch modes using the `--unhandled-rejections=mode` process flag. QUIC (#32379): Node.js 15 comes with experimental support QUIC, which can be enabled by compiling Node.js with the `--experimental-quic` configuration flag. The Node.js QUIC implementation is exposed by the core `net` module. V8 8.6 (#35415): The V8 JavaScript engine has been updated to V8 8.6 (V8 8.4 is the latest available in Node.js 14). Along with performance tweaks and improvements the V8 update also brings the following language features: * `Promise.any()` (from V8 8.5) * `AggregateError` (from V8 8.5) * `String.prototype.replaceAll()` (from V8 8.5) * Logical assignment operators `&&=`, `||=`, and `??=` (from V8 8.5) Other Notable Changes: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **dns**: add dns/promises alias (shisama) (#32953) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **stream**: construct (Robert Nagy) (#29656) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) Semver-Major Commits: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **build**: reset embedder string to "-node.0" (Michaël Zasso) (#35415) - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **crypto**: refactoring internals, add WebCrypto (James M Snell) (#35093) - **crypto**: move node\_crypto files to src/crypto (James M Snell) (#35093) - **deps**: V8: cherry-pick d76abfed3512 (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 717543bbf0ef (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 6be2f6e26e8d (Michaël Zasso) (#35415) - **deps**: fix V8 build issue with inline methods (Jiawen Geng) (#35415) - **deps**: fix platform-embedded-file-writer-win for ARM64 (Michaël Zasso) (#35415) - **deps**: update V8 postmortem metadata script (Colin Ihrig) (#35415) - **deps**: update V8 to 8.6.395 (Michaël Zasso) (#35415) - **deps**: upgrade npm to 7.0.0 (Myles Borins) (#35631) - **deps**: update npm to 7.0.0-rc.3 (Myles Borins) (#35474) - **deps**: V8: cherry-pick 0d6debcc5f08 (Gus Caplan) (#33600) - **dns**: add dns/promises alias (shisama) (#32953) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **doc**: update support macos version for 15.x (Ash Cripps) (#35022) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http**: fixed socket.setEncoding fatal error (iskore) (#33405) - **http**: emit 'error' on aborted server request (Robert Nagy) (#33172) - **http**: cleanup end argument handling (Robert Nagy) (#31818) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **http2**: add `invalidheaders` test (Pranshu Srivastava) (#33161) - **http2**: refactor state code validation for the http2Stream class (rickyes) (#33535) - **http2**: header field valid checks (Pranshu Srivastava) (#33193) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: remove ERR\_INVALID\_OPT\_VALUE and ERR\_INVALID\_OPT\_VALUE\_ENCODING (Denys Otrishko) (#34682) - **lib**: handle one of args case in ERR\_MISSING\_ARGS (Denys Otrishko) (#34022) - **lib**: remove NodeError from the prototype of errors with code (Michaël Zasso) (#33857) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: check args in net.connect() and socket.connect() calls (Denys Otrishko) (#34022) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **process**: update v8 fast api calls usage (Maya Lekova) (#35415) - **process**: change default --unhandled-rejections=throw (Dan Fabulich) (#33021) - **process**: use v8 fast api calls for hrtime (Gus Caplan) (#33600) - **process**: delay throwing an error using `throwDeprecation` (Ruben Bridgewater) (#32312) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: implement NodePlatform::PostJob (Clemens Backes) (#35415) - **src**: update NODE\_MODULE\_VERSION to 88 (Michaël Zasso) (#35415) - **src**: error reporting on CPUUsage (Yash Ladha) (#34762) - **src**: use node:moduleName as builtin module filename (Michaël Zasso) (#35498) - **src**: enable wasm trap handler on windows (Gus Caplan) (#35033) - **src**: update NODE\_MODULE\_VERSION to 86 (Michaël Zasso) (#33579) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **src**: remove \_third\_party\_main support (Anna Henningsen) (#33971) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **src**: remove unused CancelPendingDelayedTasks (Anna Henningsen) (#32859) - **stream**: try to wait for flush to complete before 'finish' (Robert Nagy) (#34314) - **stream**: cleanup and fix Readable.wrap (Robert Nagy) (#34204) - **stream**: add promises version to utility functions (rickyes) (#33991) - **stream**: fix writable.end callback behavior (Robert Nagy) (#34101) - **stream**: construct (Robert Nagy) (#29656) - **stream**: write should throw on unknown encoding (Robert Nagy) (#33075) - **stream**: fix \_final and 'prefinish' timing (Robert Nagy) (#32780) - **stream**: simplify Transform stream implementation (Robert Nagy) (#32763) - **stream**: use callback to properly propagate error (Robert Nagy) (#29179) - **test**: update tests after increasing typed array size to 4GB (Kim-Anh Tran) (#35415) - **test**: fix tests for npm 7.0.0 (Myles Borins) (#35631) - **test**: fix test suite to work with npm 7 (Myles Borins) (#35474) - **test**: update WPT harness and tests (Michaël Zasso) (#33770) - **timers**: introduce timers/promises (James M Snell) (#33950) - **tools**: disable x86 safe exception handlers in V8 (Michaël Zasso) (#35415) - **tools**: update V8 gypfiles for 8.6 (Ujjwal Sharma) (#35415) - **tools**: update V8 gypfiles for 8.5 (Ujjwal Sharma) (#35415) - **url**: file URL path normalization (Daijiro Wachi) (#35477) - **url**: verify domain is not empty after "ToASCII" (Michaël Zasso) (#33770) - **url**: remove U+0000 case in the fragment state (Michaël Zasso) (#33770) - **url**: remove gopher from special schemes (Michaël Zasso) (#33325) - **url**: forbid lt and gt in url host code point (Yash Ladha) (#33328) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) - **win, child_process**: sanitize env variables (Bartosz Sosnowski) (#35210) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) - **worker**: set trackUnmanagedFds to true by default (Anna Henningsen) (#34394) - **worker**: rename error code to be more accurate (Anna Henningsen) (#33872) PR-URL: #35014

On Windows environment variables are case-insensitive. When spawning child process certain apps can get confused if some of the variables are duplicated. This adds a step on Windows to normalizeSpawnArguments that removes such duplicates, keeping only the first (in lexicographic order) entry in the env key of options. This is partly already done for the PATH entry. Fixes: nodejs#35129 PR-URL: nodejs#35210 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

gentunian · 2021-06-03T13:21:05Z

@bzoz will this backported to previous version of nodejs?

bl-ue · 2021-06-03T13:32:19Z

Which version would you like it backported to, @gentunian?

gentunian · 2021-06-03T14:01:14Z

@bl-ue 12.17.0 specifically, but I think that probably 12 and 14 would be nice.

richardlau · 2021-06-03T14:03:22Z

This was marked semver-major so will not be backported.

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Sep 15, 2020

bzoz mentioned this pull request Sep 15, 2020

Node must de-duplicate environment variables before calling CreateProcess (and upon env.extend()) #35129

Closed

addaleax approved these changes Sep 15, 2020

View reviewed changes

mscdex reviewed Sep 15, 2020

View reviewed changes

asklar approved these changes Sep 15, 2020

View reviewed changes

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 16, 2020

lundibundi approved these changes Sep 16, 2020

View reviewed changes

bzoz force-pushed the win-only-one-env-pair branch from 3723f53 to 209901b Compare September 23, 2020 08:37

bzoz force-pushed the win-only-one-env-pair branch from 209901b to 0762b6f Compare September 24, 2020 08:56

bzoz force-pushed the win-only-one-env-pair branch from 0762b6f to 2df8258 Compare September 24, 2020 14:15

bzoz closed this Sep 24, 2020

BethGriggs mentioned this pull request Oct 14, 2020

2020-10-20, Version 15.0.0 (Current) #35014

Merged

gentunian mentioned this pull request Jun 3, 2021

Invalid command line switch for "cmd.exe" System.argumentException: Item has already been added. (PATH vs Path) cmake-js/cmake-js#232

Closed

gentunian mentioned this pull request Jun 3, 2021

add workaround for windows case sensitive issue cmake-js/cmake-js#243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

win, child_process: sanitize env variables #35210

win, child_process: sanitize env variables #35210

bzoz commented Sep 15, 2020

addaleax Sep 15, 2020

bzoz Sep 15, 2020

richardlau Sep 15, 2020

asklar Sep 15, 2020

bzoz Sep 16, 2020

mscdex Sep 15, 2020

mscdex Sep 15, 2020

mscdex Sep 15, 2020

nodejs-github-bot commented Sep 23, 2020

bzoz commented Sep 23, 2020

nodejs-github-bot commented Sep 24, 2020

nodejs-github-bot commented Sep 24, 2020

bzoz commented Sep 24, 2020

nodejs-github-bot commented Sep 24, 2020

nodejs-github-bot commented Sep 24, 2020

bzoz commented Sep 24, 2020

gentunian commented Jun 3, 2021

bl-ue commented Jun 3, 2021

gentunian commented Jun 3, 2021

richardlau commented Jun 3, 2021

	const upercaseKey = key.toUpperCase();
	const uppercaseKey = key.toUpperCase();

	if (sawKey.has(upercaseKey)) {
	if (sawKey.has(uppercaseKey)) {

win, child_process: sanitize env variables #35210

win, child_process: sanitize env variables #35210

Conversation

bzoz commented Sep 15, 2020

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 23, 2020

bzoz commented Sep 23, 2020

nodejs-github-bot commented Sep 24, 2020

nodejs-github-bot commented Sep 24, 2020

bzoz commented Sep 24, 2020

nodejs-github-bot commented Sep 24, 2020

nodejs-github-bot commented Sep 24, 2020

bzoz commented Sep 24, 2020

gentunian commented Jun 3, 2021

bl-ue commented Jun 3, 2021

gentunian commented Jun 3, 2021

richardlau commented Jun 3, 2021