GeezerVison

Couple of discussion points on this:

1. For the blacklisted options like --tls-cipher-list, should be there an explicit opt in that would allow those to work. For instance NODEOPT="--tls-cipher-list" node --ignore-nodeopt-blacklist foo ... or do we simply always blacklist those items for sake of simplicity?

2. There needs to be a simple way for embedders like Electron to switch off NODEOPT support (a compile time flag should work fine for this)

3. I still very much want to see NODEOPT to not be passed down to child_process.fork() processes by default.

@jasnell

1. There are no other options like --tls-cipher-list, its one of a kind, all the other blacklisted items are pretty clearly not the kind of thing that should be in an env var. Having to specify a CLI option to enable an env var isn't something I've heard a use-case for, I think we should wait until someone asks for it and makes a case.

2. Good idea, I'll add a compile time flag.

3. Stripping some env vars is something that needs more discussion, I'll follow up when I get out of this meeting.

For (1) .. good point :-) The only other one that I would definitely rule out, however, is the --security-revert= flag. It's a purposefully undocumented flag that is a very special case. It's use needs to be explicitly opt-in in every situation.

Right, forgot --security-revert (I'd read through the code and it didn't do anything, there aren't any reverts at the moment, so I forgot). See c0836e5

+1 ... yeah, --security-revert is a special snowflake that is reset with every semver-major. We haven't had any CVE's that needed to be included in this since 4.x I think so it's a non-op at the current time. Hopefully it'll be able to remain a non-op forever :-)

On stripping of NODEOPT env var during spawn: I think it takes away from the usefullness of NODEOPT (specifically, it means we can't use NODEOPT="-r node-report" npm install ... to debug install scripts), intrudes confusingly into the child_process API, and goes against the basic philosophy of environment, which is that environment is inherited by children.

However, I'd rather have at least a one-level NODEOPT with this confusing caveat than none at all.

@jasnell, can you make a case for why you want this non-inheritable env var behaviour? And what specifically you propose?

For example:

require('child_process').exec('node -p process.env.NODE_PATH', function(err, out) {
  console.log(err || out);                                                       
});
require('child_process').exec('node -p process.env.NODEOPT', function(err, out) {
  console.log(err || out);                                                       
});

That NODE_PATH=/some/path NODEOPT=--prof node echo.js should print:

/some/path

instead of

/some/path
--prof

I would find confusing, is that what you propose?

And what do you propose that the following code would print?

process.env.NODEOPT = '-r node-report';
require('child_process').exec('node -p process.env.NODEOPT', function(err, out) {
  console.log(err || out);
});

I would expect it to print the value of NODEOPT, do you propose it print something else?

@sam-github ... the concern is largely about backwards compatibility and the possibility of NODEOPT settings altering the behavior of child processes in ways that could break.

For instance, imagine a child process that uses fs.readFileSync() internally to read a file and return it's contents to the parent. The parent is written to scan stdout / stderr to determine whatever status it needs to know. The child process is launched specifically assuming that flags like --trace-sync-io are not passed (because it is not passed in the args). Using NODEOPT=--trace-sync-io, however, causes a number of warnings to be printed to stderr in the child process potentially causing existing code to break.

(Essentially, passing NODEOPT automatically to child processes breaks the existing API contract around args passing to forked child processes.)

My proposal is that NODEOPT would be simply removed from the env passed to the child process unless an option is set, e.g. fork(path, {nodeopt: true}).

But don't we already have that issue with existing environment variables, e.g. NODE_DEBUG?

NODE_TTY_UNSAFE_ASYNC is also inherited, and modifies API contract in ways that would break child processes, there seems to be some precedence for these "use at own risk" env vars.

If there is widespread concern, I'd rather disable V8 options that output to stdio then break the inheritance, it leaves the behaviour much more predictable.

NODE_TTY_UNSAFE_ASYNC is in the process of being removed entirely, however. There is also extremely little evidence that it's actually used by anyone.

NODE_DEBUG isn't getting removed, and has the same characteristics as you describe for NODEOPT=--trace-sync-io: it can be useful, it is inherited, and it will (unsprisingly) break any code that doesn't expect extra output.

I would be much more comfortable if this was a whitelist instead of a blacklist. It would prevent accidentally harmful flags from being exposed.

NODE_DEBUG has been around since v0.1.32 and is well established as an environment variable. It predates the child_process API and it's side effects and inheritance by spawned child processes is well known. It also does not alter arbitrary command line arguments passed into the child process. There is no backwards compatibility concern there and therefore is not directly comparable.

@jasnell , what about if we have NODEOPT and NODEOPT_INHERIT and let people choose the behavior they want. NODEOPT_INHERIT would be passed to child processes while NODEOPT would not. In that way you can use the environment to set options and chose if they are inherited or not. I think having the ability to pass on options to child processes is important and this would be one way to let the end user make a specific choice.

@jasnell Before I implement this, lets be really clear:

My proposal is that NODEOPT would be simply removed from the env passed to the child process unless an option is set, e.g. fork(path, {nodeopt: true}).

child_process.spawn(process.execPath, {env: {NODEOPT='xxx'}}, function(...

You propose NODEOPT to not be set in the child process's env?

@bmeck, Do you mean for node options, or V8 options? I can switch it around to opt-in for node, I think there is a way I can do that, but there are 417 V8 options as of this moment on master, and they all look like the kinds of things someone might want to set via env to tune performance or diagnose mis-performance, and there don't appear to be anything we'd want to blacklist.

@sam-github ... no, if NODEOPT is explicitly passed in the call to fork like in your example, it would be used. What I'm saying is that env is not explicitly given and the default process.env is used, then it's NODEOPT would not be inherited.

In other words... NODEOPT is used:

child_process.spawn(process.execPath, {env: {NODEOPT:'...'}}, () => {});

process.env.NODEOPT is used:

child_process.spawn(process.execPath, {nodeopt: true}, () => {});

process.env.NODEOPT is not used:

child_process.spawn(process.execPath, () => {});

@mhdawson ... as long as there is an explicit opt-in for the options to be used in child processes I'm fine with however it happens. A second environment variable would not be my first choice tho but I could live with it.

Note: that if we went with the nodeopt:true option to spawn/fork now, we can always choose to switch the default later (making the option a non-op) if it can be shown that inheriting the NODEOPT env does not break anyone in practice.

@jasnell @mhdawson If the main risk you have in mind is security, it won’t matter whether you pass one or two environment variables.

(But as I’ve said before, using environment variables that are not inherited by child processes seems pointless by definition.)

@sam-github I am definitely thinking about more than just node arguments. Things like --harmony-* would prevent some polyfill detection and use incomplete/broken v8 implementations if they are passed.

Another note: the documentation on this should need to be clear that:

(a) command line arguments passed directly on the command line take precedence over those passed in NODEOPT and
(b) Child processing inheriting NODEOPT would not inherit arguments passed on the command line.

To illustrate the kind of impact this has consider the following example:

$ NODEOPT="--icu-data-dir=a" ./node --icu-data-dir=b
> process.binding('config');
{ hasIntl: true, hasSmallICU: true, icuDataDir: 'b' }
> var m = child_process.spawn('./node', ['-pe', 'console.log(process.binding("config"))'], {stdio:['inherit','inherit', 'inherit']})
undefined
> { hasIntl: true, hasSmallICU: true, icuDataDir: 'a' }

To be certain, this is not a criticism, just something that should be noted. (This is also true when using the NODE_ICU_DATA environment variable)

Note that the current CLI/env documentation (https://nodejs.org/dist/latest-v7.x/docs/api/cli.html) currently does not explain that arguments would take precedence over env vars.

I agree with @addaleax that environment variables should be passed to child processes by default.

Another test case that will need be looked at:

NODEOPT="--inspect=localhost:1234" ./node
Debugger listening on port 1234.
Warning: This is an experimental feature and could change at any time.
To start debugging, open the following URL in Chrome:
    chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=localhost:1234/f8884219-6fd2-442e-aa5e-257cb686e3a5
> var m = child_process.spawn('./node', {stdio:['inherit', 'inherit', 'inherit']})
undefined
> Starting inspector on localhost:1234 failed: address already in use