process: add allowedNodeEnvironmentFlags property #19335

boneskull · 2018-03-13T23:28:36Z

process.allowedNodeEnvironmentFlags lists all allowable flags within the NODE_OPTIONS environment variable.

UPDATE Aug 17 2018: I've modified this PR to provide a friendlier API, as described in this comment.

This change addresses #17740 as far as I need it to. The whitelist of NODE_OPTIONS-able flags (not counting v8 flags) correspond to the set of flags a CLI application may want to pass along to a spawned node process. Put another way, other Node.js-specific flags are essentially useless to CLI apps wrapping node.

I changed whitelist to be more obvious about what it's for, since it's no longer within a function.
I also changed whitelist to a vector to mimic the existing global static variables in node.cc (I didn't see a const char * [] anywhere; don't know if this matters)
Added a test
Added documentation

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

bnoordhuis

I see two issues with this PR:

It doesn't address V8 flags, and
The whitelist is only a subset of the flags.

bnoordhuis · 2018-03-14T04:53:22Z

src/node.cc

@@ -564,6 +564,47 @@ const char *signo_string(int signo) {
  }
 }

+static std::vector<std::string> environment_flags = {


std::vector is kinda wasteful for this. Can you change it back to const char*[]? Or better, const char* const [].

bnoordhuis · 2018-03-14T04:57:02Z

src/node.cc

@@ -3078,6 +3119,14 @@ void SetupProcessObject(Environment* env,
  process->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "execArgv"),
               exec_arguments);

+  // process.envFlags
+  Local<Array> env_flags = Array::New(env->isolate());


Preallocate the array by passing the size as the second argument.

will do. I found another place where it wasn't preallocated (involving another vector, preload_modules, so this is why I removed it.

gibfahn · 2018-03-14T10:43:25Z

The whitelist of NODE_OPTIONS-able flags (not counting v8 flags) correspond to the set of flags a CLI application may want to pass along to a spawned node process. Put another way, other Node.js-specific flags are essentially useless to CLI apps wrapping node.

I'm not sure this is true. The original idea was for NODE_OPTIONS to be able to pass through any flags except a few that didn't make sense, but it was changed to a whitelist due to security concerns (#12028).

It seems quite likely to me that there are flags that cli apps like mocha might want to pass to node that are not in the NODE_OPTIONS whitelist.

EDIT: I'm not sure what the best solution to this is, but maybe it would be enough to just expose all of the command line flags, and then filter the ones you don't need in mocha.

gibfahn · 2018-03-14T11:18:50Z

It doesn't address V8 flags

What would be your preferred solution to this @bnoordhuis ? Presumably the best solution would be to add a similar option to V8 that returns a list of flags, and then expose those flags in a similar way. Things probably start to get complex though, we'd probably want process.flags.node, process.flags.v8, and maybe process.flags.both (not to mention maybe process.flags.nodeOptions for the NODE_OPTIONS whitelist).

I guess alternatively we could do what you suggested in #17740 (comment) and parse the output of d8 --help when building node or something?

Either way, as long as we allow for the option of adding V8 options in the future, I don't see why we'd need to add V8 options at the same time as node options.

Am I right in thinking that all V8 options start with --v8? If so then with this config variable you can easily work out whether a flag should be forwarded to node or not

jasnell · 2018-03-14T11:55:23Z

What's the use case for this?

gibfahn · 2018-03-14T13:15:09Z

What's the use case for this?

@jasnell that's covered in the linked issue (#17740).

vkurchatkin · 2018-03-14T13:27:50Z

-1. The justification seems pretty unconvincing.

Trott · 2018-03-14T14:54:29Z

-1. The justification seems pretty unconvincing.

@vkurchatkin Can you provide a little more detail? Is the use case invalid? Or is this something that doesn't need to be in core? Or...what's unconvincing?

vkurchatkin · 2018-03-14T15:15:54Z

@Trott In the issue @boneskull admits, that something like --node-flags= would solve the problem. That's something that I would do without a second thought.

Yes, I would say that the use case is invalid, in my opinion. Even if it wasn't, just one pretty specific use case doesn't warrant such an addition.

gibfahn · 2018-03-14T15:31:37Z

Okay, so thinking about this further (and talking to @boneskull), if the purpose of this option is to allow people who write cli apps to know which flags a user might want to pass through to node, then having the list of relevant flags match the list in NODE_OPTIONS might make sense. It depends whether there's a difference between the two lists.

richardlau · 2018-03-14T18:36:40Z

doc/api/process.md

@@ -916,6 +916,63 @@ console.log(process.env.test);
 // => 1
 ```

+## process.envFlags
+<!-- YAML
+added: v10.0.0


This should be added: REPLACEME. The releaser will substitute the actual version when making the release.

I figured that wasn't right, but didn't know what to put. Is it documented somewhere?

@boneskull Heh, yes. 😄 I introduced these things, and I agree that this should be documented somewhere, but there’s one question that has always stopped me from doing it myself until now:

Where would people look for such documentation?

CONTRIBUTING.md perhaps?

If we could catch this on landing (i.e. with nodejs/node-core-utils#193) then I think that would make a big difference, although documenting is a good idea too.

boneskull · 2018-03-15T21:20:37Z

This would be useful for any CLI app which chooses to "pass through" Node.js options. It provides a better UX than having to execute a script via node <flags> /path/to/executable <more flags>. It's also easier to stomach for those users who are new to the command line. Likewise, it's a better UX than prefixing each with --node-.

If the above seems too particular or unnecessary, I hope reviewers can briefly put themselves in the shoes of maintainers, contributors, and consumers of userland CLI apps. In other words, please don't dismiss it just because it's not something which you often encounter or struggle with.

The reason this does not include "all" flags (v8 flags nor non-NODE_OPTIONS-flags) is threefold:

CLI apps can match against /--v8-.+/ and pass those through. There's no such easy regex for matching Node.js flags.
--version, --help and its ilk (flags not appearing in NODE_OPTIONS) are essentially useless, as they fundamentally change node's behavior to something other than "execute this script"
It will add more complexity / overhead to gather the v8 flags, whether at compile or runtime

Then, I don't know of a use case (this does not imply there isn't one, of course) for "all" flags unless besides the human need to be completionist. Collect 'em all, etc. 😉

ljharb · 2018-03-15T22:47:35Z

One reason for "all" flags is, then I don't have to know to pass through v8 options - I just have this list as a single source of truth.

boneskull · 2018-03-15T23:21:36Z

@ljharb This is true.

Entertaining the idea further, if they were to be added:

If I were to parse d8 --help (or whatever node --v8-options calls) at build time, what would be the preferred way to pull that output in? Generate a text file and parse it? Generate a header file? #define a bunch of stuff?

If we do do that, it means we're parsing output intended to be human-readable. IMO, this isn't too terribly kind to the v8 team. The output will also vary by architecture.

Maybe better to read flag-definitions.h directly?

jasnell · 2018-03-16T14:28:39Z

I'd like to get @addaleax's take on this. I know she's been giving some thought to improved handling of the command line arguments here recently and may have some ideas on how to best proceed here.

boneskull · 2018-03-26T21:15:59Z

(a friendly nag at @addaleax)

addaleax · 2018-03-26T21:54:20Z

@boneskull Sorry, kinda missed the ping here. I don’t have strong opinions on this, and I don't think the kind of refactor I'm having in mind would necessarily change the API for this feature.

I would, however, appreciate a more expressive name than envFlags -- maybe something like process.allowedEnvironmentNodeFlags? It's verbose but it gets to the point of what this array means.

boneskull · 2018-03-27T00:42:23Z

I'll go ahead and change the name.

Can I please have some guidance as asked in this comment? I would like to see if I can pull the v8 flags in as well, but I'm unsure of how others would approach this.

bnoordhuis · 2018-03-27T09:21:10Z

Maybe better to read flag-definitions.h directly?

That won't work (reliably) for the following reasons:

its implementation changes over time (maintenance hassle)
its content changes based on V8's build flags, which are different from node's build flags
it gives the wrong answers when linking against a shared library build of V8

A couple of solutions/workarounds:

Petition V8 for an API that lets you query the flags at runtime.
Two-stage build: parse output of node --v8-options and compile that into the stage 2 build
Hack: redirect stderr with e.g. fmemopen() and call v8::V8::SetFlagsFromString("--help").

(2) and (3) are still prone to break when the format changes; (1) is arguably the best option.

boneskull · 2018-03-27T19:46:03Z

I'd rather not introduce (2) or (3) if it's prone to break, unless there are some guarantees from V8 that it won't. I'll follow up there.

@ljharb Does this sound reasonable?

We exposed process.allowedEnvironmentNodeFlags (roughly as this PR is written)
Petition V8 for runtime access to flags
Add the V8 flags as e.g. process.allowedV8Flags when/if runtime access to V8 flags lands

ljharb · 2018-03-27T20:14:51Z

@boneskull seems like a good plan. Any chance they could be an object mapping arg names to provided values rather than just a list of allowed names?

boneskull · 2018-03-27T21:04:22Z

@ljharb That sounds like just cross-referencing with process.execArgv...?

ljharb · 2018-03-27T21:28:28Z

fair, it just seems like it’d be nice to avoid the extra step.

boneskull · 2018-08-22T16:24:43Z

failure is tools/doc/type-parser.js doesn't recognize Set

addaleax · 2018-08-23T23:40:13Z

src/node.cc

+  "--stack_trace_limit",
+};
+
+int v8_environment_flags_count = arraysize(v8_environment_flags);


That might be an issue of missing includes (node_internals.h?)

`process.allowedNodeEnvironmentFlags` provides an API to validate and list flags as specified in `NODE_OPTIONS` from user code. Refs: nodejs#17740 Signed-off-by: Christopher Hiller <[email protected]>

boneskull · 2018-08-24T17:56:56Z

@Trott It's not clear to me what needs to happen to get this merged. wait 72h?

I've addressed recent comments, and the lone rejection was withdrawn. It doesn't seem--for lack of a better word--fair to require every approver to re-approve this...

ljharb

<3 JS robustness LGTM

ljharb · 2018-08-24T18:05:09Z

lib/internal/bootstrap/node.js

+      }
+
+      delete() {
+        // noop, `Set` API compatible


you could also choose to throw here, which would be consistent with delete on a frozen object in strict mode.

hmm, not a bad idea.

I'm going to leave these as-is, I think

ljharb · 2018-08-24T18:05:42Z

lib/internal/bootstrap/node.js

+      }
+
+      clear() {
+        // noop


ljharb · 2018-08-24T18:06:44Z

tools/doc/type-parser.js

@@ -18,7 +18,7 @@ const jsGlobalTypes = [
  'Array', 'ArrayBuffer', 'DataView', 'Date', 'Error', 'EvalError', 'Function',
  'Object', 'Promise', 'RangeError', 'ReferenceError', 'RegExp',
  'SharedArrayBuffer', 'SyntaxError', 'TypeError', 'TypedArray', 'URIError',
-  'Uint8Array',
+  'Uint8Array', 'Set'


should this just go ahead and add Map, WeakSet, WeakMap, Symbol, etc?

@ljharb We had all common globals here but unused ones were purged in 6ac3c44

@boneskull Nit: These types are sorted alphabetically, so the Set should go after 'RegExp'.

that seems like it serves only to frustrate contributors in the future when they use normal parts of the language :-/ cc @Trott

Trott · 2018-08-24T22:30:13Z

It doesn't seem--for lack of a better word--fair to require every approver to re-approve this...

It's not required. I was just suggesting that, given the changes, a few re-approvals would be good.

I think this is good to land if CI is green. I'll alphabetize that one entry in type-parser.js while landing. As far as @ljharb's comment that we should add Map etc., I don't dispute it, but that can certainly happen in another PR. (Maybe there's a good way to generate them dynamically rather than having them hard-coded. But again, outside the scope of this PR for sure.)

Trott · 2018-08-24T22:34:44Z

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

`process.allowedNodeEnvironmentFlags` provides an API to validate and list flags as specified in `NODE_OPTIONS` from user code. Refs: nodejs#17740 Signed-off-by: Christopher Hiller <[email protected]> PR-URL: nodejs#19335 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Sam Ruby <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Trott · 2018-08-25T01:17:16Z

Landed in 80143f6.

`process.allowedNodeEnvironmentFlags` provides an API to validate and list flags as specified in `NODE_OPTIONS` from user code. Refs: #17740 Signed-off-by: Christopher Hiller <[email protected]> PR-URL: #19335 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Sam Ruby <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22394 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22392 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716

`process.allowedNodeEnvironmentFlags` provides an API to validate and list flags as specified in `NODE_OPTIONS` from user code. Refs: #17740 Signed-off-by: Christopher Hiller <[email protected]> PR-URL: #19335 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Sam Ruby <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22407 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22515 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 13, 2018

vsemozhetbyt added cli Issues and PRs related to the Node.js command line interface. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 13, 2018

bnoordhuis reviewed Mar 14, 2018

View reviewed changes

richardlau reviewed Mar 14, 2018

View reviewed changes

jdalton mentioned this pull request Mar 15, 2018

module: add builtinModules #16386

Closed

3 tasks

boneskull force-pushed the env-flags branch 2 times, most recently from 7e1a4b8 to 3b4574f Compare March 15, 2018 23:24

boneskull changed the title ~~process: add envFlags property~~ process: add allowedEnvironmentNodeFlags property Apr 9, 2018

addaleax approved these changes Aug 23, 2018

View reviewed changes

boneskull force-pushed the env-flags branch 2 times, most recently from 88a9662 to 330f96d Compare August 23, 2018 23:55

process: add allowedNodeEnvironmentFlags property

7830f6e

`process.allowedNodeEnvironmentFlags` provides an API to validate and list flags as specified in `NODE_OPTIONS` from user code. Refs: nodejs#17740 Signed-off-by: Christopher Hiller <[email protected]>

boneskull force-pushed the env-flags branch from 330f96d to 7830f6e Compare August 23, 2018 23:56

ljharb approved these changes Aug 24, 2018

View reviewed changes

Trott added 2 commits August 24, 2018 15:31

squash! alphabetize

f154a52

squash! restore trailing comma

9a0565d

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 25, 2018

Trott closed this Aug 25, 2018

plroebuck mentioned this pull request Sep 5, 2018

Documented some cli options mochajs/mocha#3447

Closed

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

richardlau mentioned this pull request Sep 29, 2018

enhancement: provide list of allowed flags of current executable #17740

Closed

joyeecheung mentioned this pull request Aug 1, 2024

Programmatically expose whether a cli flag takes an argument or not #54144

Open

process: add allowedNodeEnvironmentFlags property #19335

process: add allowedNodeEnvironmentFlags property #19335

Conversation

boneskull commented Mar 13, 2018 • edited Loading

Checklist

bnoordhuis left a 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

gibfahn commented Mar 14, 2018 • edited Loading

gibfahn commented Mar 14, 2018

jasnell commented Mar 14, 2018

gibfahn commented Mar 14, 2018

vkurchatkin commented Mar 14, 2018

Trott commented Mar 14, 2018

vkurchatkin commented Mar 14, 2018

gibfahn commented Mar 14, 2018

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

boneskull commented Mar 15, 2018

ljharb commented Mar 15, 2018

boneskull commented Mar 15, 2018 • edited Loading

jasnell commented Mar 16, 2018

boneskull commented Mar 26, 2018

addaleax commented Mar 26, 2018

boneskull commented Mar 27, 2018

bnoordhuis commented Mar 27, 2018

boneskull commented Mar 27, 2018 • edited Loading

ljharb commented Mar 27, 2018

boneskull commented Mar 27, 2018

ljharb commented Mar 27, 2018

boneskull commented Aug 22, 2018

Choose a reason for hiding this comment

boneskull commented Aug 24, 2018

ljharb left a 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

Choose a reason for hiding this comment

Trott commented Aug 24, 2018 • edited Loading

Trott commented Aug 24, 2018

Trott commented Aug 25, 2018

boneskull commented Mar 13, 2018 •

edited

Loading

gibfahn commented Mar 14, 2018 •

edited

Loading

boneskull commented Mar 15, 2018 •

edited

Loading

boneskull commented Mar 27, 2018 •

edited

Loading

Trott commented Aug 24, 2018 •

edited

Loading