Skip to content
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

win, child_process: sanitize env variables #35210

Closed
wants to merge 1 commit into from
Closed

win, child_process: sanitize env variables #35210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/api/child_process.md
View file
Open in desktop
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


The [`child_process.spawn()`][] method spawns the child process asynchronously,
without blocking the Node.js event loop. The [`child_process.spawnSync()`][]
function provides equivalent functionality in a synchronous manner that blocks
Expand Down
20 changes: 20 additions & 0 deletions lib/child_process.js
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ const {
ObjectDefineProperty,
ObjectPrototypeHasOwnProperty,
Promise,
Set,
} = primordials;

const {
Expand Down Expand Up @@ -524,8 +525,27 @@ function normalizeSpawnArguments(file, args, options) {
env.NODE_V8_COVERAGE = process.env.NODE_V8_COVERAGE;
}

let envKeys = [];
// Prototype values are intentionally included.
for (const key in env) {
envKeys.push(key);
}

if (process.platform === 'win32') {
// On Windows env keys are case insensitive. Filter out duplicates,
// keeping only the first one (in lexicographic order)
const sawKey = new Set();
envKeys = envKeys.sort().filter((key) => {
const uppercaseKey = key.toUpperCase();
if (sawKey.has(uppercaseKey)) {
return false;
}
sawKey.add(uppercaseKey);
return true;
});
}

for (const key of envKeys) {
const value = env[key];
if (value !== undefined) {
envPairs.push(`${key}=${value}`);
Expand Down
11 changes: 10 additions & 1 deletion test/parallel/test-child-process-env.js
View file
Open in desktop
Expand Up @@ -36,7 +36,9 @@ const env = {
'HELLO': 'WORLD',
'UNDEFINED': undefined,
'NULL': null,
'EMPTY': ''
'EMPTY': '',
'duplicate': 'lowercase',
'DUPLICATE': 'uppercase',
};
Object.setPrototypeOf(env, {
'FOO': 'BAR'
Expand Down Expand Up @@ -65,4 +67,11 @@ child.stdout.on('end', mustCall(() => {
assert.ok(!response.includes('UNDEFINED=undefined'));
assert.ok(response.includes('NULL=null'));
assert.ok(response.includes(`EMPTY=${os.EOL}`));
if (isWindows) {
assert.ok(response.includes('DUPLICATE=uppercase'));
assert.ok(!response.includes('duplicate=lowercase'));
} else {
assert.ok(response.includes('DUPLICATE=uppercase'));
assert.ok(response.includes('duplicate=lowercase'));
}
}));