fix: parse options.env more similarly to process.env

[thecodrr]

This fix attempts to parse options.env in a similar way to how it is parsed in child_process.spawn, namely by doing a simple sorted case-insensitive lookup for path and pathext.

The intent is to support folks who pass in to opts { env: ...process.env}, as this removes the built in case insensitivity that is present on the original process.env object.

[wraithgar]

Is this handling the case where env is passed in on a Windows system that is using Path? Won't process.env.PATH match that anyways? I'm a bit confused where the performance hit is here.

[thecodrr]

@wraithgar process.env.PATH is not equal to process.env.Path. When Path is available, PATH is undefined.

[wraithgar]

We have always operated under the assumption that the windows environment variables were case insensitive.

The docs at https://nodejs.org/api/process.html#process_process_env even say:

On Windows operating systems, environment variables are case-insensitive.

So, process.env.Path is identical to process.env.PATH but only when accessing it through process.env. If something were to copy process.env into a new novel object, then it of course would only be accessible via the original name from when it was accessed from process.env.

I think the answer to my first question then is "yes" this is handling the case where options.env has been copied from the environment, and this case insensitivity is now lost. This is quite a subtle bug and would definitely benefit from some sort of comment to prevent future accidental removal of that code.

This will also need a test added for coverage purposes.

[wraithgar]

If process.env.PATH truly were empty on a windows system that used Path that would be news to a lot of people. Are you sure you're not looking at a copy of env?

[thecodrr]

If process.env.PATH truly were empty on a windows system that used Path that would be news to a lot of people. Are you sure you're not looking at a copy of env?

You are right. I was looking at options.env not process.env which is different. However, on Windows machines if you spread process.env it uses the case that is present in the system environment variables. On my machine, that happens to be Path hence the bug.

[wraithgar]

Phew perfect, I think we understand the bug now. An affordance in that conditional to look for Path would help a lot of folks who spread env out. The test should be relatively straightforward too I hope.

[thecodrr]

@wraithgar or maybe we can add a case-insensitive lookup function for finding env variables or would that be unnecessary? That would handle all sorts of weird pAtH casings.

[wraithgar]

Given as how we are trying to interpret opts.env identically to child_process I don't think that your suggestion is too far fetched.

It looks like node does special parsing of this parameter itself to account for this special quirk of environment variables

https://github.com/nodejs/node/blob/65e70bf54e7ff4704d244c55ad319571fdd438ae/lib/child_process.js#L673-L694

  let envKeys = [];
  // Prototype values are intentionally included.
  for (const key in env) {
    ArrayPrototypePush(envKeys, 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 SafeSet();
    envKeys = ArrayPrototypeFilter(
      ArrayPrototypeSort(envKeys),
      (key) => {
        const uppercaseKey = StringPrototypeToUpperCase(key);
        if (sawKey.has(uppercaseKey)) {
          return false;
        }
        sawKey.add(uppercaseKey);
        return true;
      },
    );
  }

This same logic may need to apply to both the path and pathext parameters we try to pull from opts.env

[thecodrr]

@wraithgar what would be the best way to write a test case for this? Can we convert pathToInitial into an exported function for test purposes?

[wraithgar]

No that would just be testing how the code is written, not what it ultimately does.

There is a test in test/shell.js that passes in env. We can duplicate that and edit the env parameter to cover the use cases we're wanting to test.

In order to match node the keys in env should probably be sorted.

[thecodrr]

@wraithgar should be ready