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.

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

options.env.PATH is used to find binaries instead of the calling process.env.PATH #366

Closed
mvachhar opened this issue Aug 30, 2019 · 13 comments
Labels

Comments

@mvachhar
Copy link

It seems that there is no way to pass an empty environment to execa and use PATH to find the binary.

If one does

execa("ls", [], {extendEnv: false})

execa will throw ENOENT because there is no PATH environment variable in env. However, with vanilla node:

cp = require("child_process");
cp.spawn("ls", { env: {} });

everything is fine.

It appears that this difference is due to the fact that spawn (IMHO) does the correct thing and uses the calling processes PATH environment variable to find the underlying binary. However execa is using the env option, which is intended to be the environment for the spawned process, not the parent process.

This makes it impossible to use the parent path to find the binary but give the child an empty environment (which is important to avoid leaking secrets stored in the environment, for example). Moreover, someone may want a different path for the child vs. the parent, which is also impossible to achieve.

A little bit of digging seems to indicate that the underlying problem is actually in cross-spawn itself, though it is unclear to me if this is the desired behavior or not for their package. Again, it probably shouldn't be the desired behavior, but to each their own.

@ehmicky
Copy link
Collaborator

ehmicky commented Aug 30, 2019

Hi @mvachhar,

Would execa('ls', [], {extendEnv: false, env:{PATH:process.env.PATH}}) work?

@mvachhar
Copy link
Author

I've used that to work around my particular issue at the moment. However, what if you want the child to have a different PATH than the parent, and in particular, the child PATH does not contain the right directory to find the binary to spawn on the parent. Or, you want to tightly control the path in the spawned process so that it doesn't accidentally run binaries you didn't intend.

In these cases this is still an issue. It is why libc exec and nodejs spawn both use the process.env.PATH to find the binary, not options.env.PATH

@ehmicky
Copy link
Collaborator

ehmicky commented Aug 30, 2019

what if you want the child to have a different PATH than the parent

I am not sure I am understanding. If the child needs to have a different PATH than the parent, shouldn't the parent explicitly pass which PATH this should be? If not, how would the child know which PATH to use?

@mvachhar
Copy link
Author

mvachhar commented Aug 30, 2019

Exactly. Suppose we have the following. The parent PATH is /usr/bin:/home/mvachhar/bin. The program to execute is myProg that is in /home/mvachhar/bin/. However, it should have its PATH be /usr/bin only. Now, if we do

execa("myProg", [], { extendEnv: false, env: { PATH: "/usr/bin" })

We will get ENOENT since myProg is in /home/manishv/bin on the parent and the child path does not contain this directory in the path. Does that make sense?

@mvachhar
Copy link
Author

Oh, and just to be clear in my prior comment, UNIX exec and nodejs child_process.spawn don't have this issue because they use the parent process.env.PATH to find myProg but exec myProg with options.env.PATH

@ehmicky
Copy link
Collaborator

ehmicky commented Aug 30, 2019

Yes that makes sense now. @sindresorhus what do you think?

@sindresorhus
Copy link
Owner

Yeah, seems like a valid bug.

@mvachhar
Copy link
Author

mvachhar commented Sep 5, 2019 via email

@ehmicky
Copy link
Collaborator

ehmicky commented Sep 6, 2019

I cannot reproduce this bug.

await execa('ls', [], {extendEnv:false})

Returns:

{
  command: 'ls',
  exitCode: 0,
  exitCodeName: 'SUCCESS',
  stdout: 'index.d.ts\n' +
    'index.js\n' +
    'index.test-d.ts\n' +
    'lib\n' +
    'license\n' +
    'media\n' +
    'node_modules\n' +
    'package.json\n' +
    'readme.md\n' +
    'test',
  stderr: '',
  all: undefined,
  failed: false,
  timedOut: false,
  isCanceled: false,
  killed: false
}

Are you using the latest version of execa?

@mvachhar
Copy link
Author

mvachhar commented Sep 6, 2019

On the latest version (2.0.4) you have to do

execa("ls", [], {extendEnv: false, env: { PATH:null } });

but, essentially the same problem. I haven't looked at the latest code, but I suspect it is still a cross-spawn issue at heart.

@ehmicky
Copy link
Collaborator

ehmicky commented Sep 6, 2019

This is normal that you get ENOENT if you set PATH to null. By "normal", I mean this is core Node.js behavior, not execa.

Try this:

const { execFile } = require('child_process')
const { promisify } = require('util')
const pExecFile = promisify(execFile)
await pExecFile('ls', { env: { PATH: null } })

You get:

Thrown:
Error: spawn ls ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:264:19)
    at onErrorNT (internal/child_process.js:456:16)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  errno: 'ENOENT',
  code: 'ENOENT',
  syscall: 'spawn ls',
  path: 'ls',
  spawnargs: [],
  cmd: 'ls',
  stdout: '',
  stderr: ''
}

@mvachhar
Copy link
Author

mvachhar commented Sep 6, 2019

Ok, so execa is mimicking the behavior of child_process.spawn here, which itself is broken, IMHO, but that isn't going to get fixed. There is some wierd stuff going on with env that I don't understand.

For example

import { execFileSync } = require('child_process');
execFileSync("/usr/bin/env", [], { env: { } }).toString(); // Returns empty string, so no environment as expected
execFileSync("ls", [], { env: { } }).toString(); // Works as expected
execFileSync("/usr/bin/env, [], { env: { PATH: null } }).toString(); //Returns 'PATH=null\n'
execFileSync("ls", [], { env: { PATH: null } }).toString(); //ENOENT

So, my original test comparison to child_process was flawed. I have no idea why ls can be found with no PATH set, but can't when PATH is explicitly null. What is node using to find ls when the env is empty? Why does that get overridden when the child environment has more information?

In any case, it isn't worth digging into the node.js source to find it, it is what it is. Sorry for the false alarm.

@mvachhar mvachhar closed this as completed Sep 6, 2019
@ehmicky
Copy link
Collaborator

ehmicky commented Sep 6, 2019

execa is mimicking the behavior of child_process.spawn here

Just for precision: we are forwarding to it, not mimicking it. execa is just a wrapper around child_process.spawn().

I think it would be worth posting that issue on the Node.js repository though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants