process: add allowedNodeEnvironmentFlags property

[boneskull]

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

• [x] make -j4 test (UNIX), or vcbuild test (Windows) passes
• [x] tests and/or benchmarks are included
• [x] documentation is changed or added
• [x] commit message follows commit guidelines

[gibfahn]

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 (https://github.com/nodejs/node/pull/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]

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 https://github.com/nodejs/node/issues/17740#issuecomment-352555197 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]

What's the use case for this?

[gibfahn]

What's the use case for this?

@jasnell that's covered in the linked issue (https://github.com/nodejs/node/issues/17740).

[vkurchatkin]

-1. The justification seems pretty unconvincing.

[Trott]

-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]

@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]

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.

[boneskull]

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:

1. CLI apps can match against /--v8-.+/ and pass those through. There's no such easy regex for matching Node.js flags.
2. --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"
3. 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]

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]

@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]

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]

(a friendly nag at @addaleax)

[addaleax]

@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]

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]

Maybe better to read flag-definitions.h directly?

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

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

A couple of solutions/workarounds:

1. Petition V8 for an API that lets you query the flags at runtime.
2. Two-stage build: parse output of node --v8-options and compile that into the stage 2 build
3. 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]

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?

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

[ljharb]

@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]

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

[ljharb]

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

[boneskull]

I've made some modifications:

• renaming of property to allowedEnvironmentNodeFlags (cc @addaleax)
• cross-reference list of flags with NODE_OPTIONS to ensure I wasn't missing anything (cc @gibfahn)
• wrote a list of explicitly excluded flags and justification

Insofar as petitioning V8 (as suggested by @bnoordhuis), I had hoped it would be obvious how to do such a thing, but I was mistaken. How should I do this? Mailing list? Bug tracker? Any other suggestions on how to frame the request?

[Trott]

@nodejs/process

[gibfahn]

Insofar as petitioning V8 (as suggested by @bnoordhuis), I had hoped it would be obvious how to do such a thing, but I was mistaken. How should I do this? Mailing list? Bug tracker? Any other suggestions on how to frame the request?

Sounds like a question for @nodejs/v8

[hashseed]

Having briefly read this thread and related issue, I have to reply with a firm no wrt exposing a list of V8 flags through an API that Node.js users would rely on. Here are some reasons:

• Unless explicitly stated otherwise, V8 flags are not safe to play around with. To paraphrase the issue title, they should not belong in a list of allowed flags. The fact that some frameworks rely on V8's internal flags should not be encouraged.
• V8 flags are not considered part of the a public API. In fact, they are neither thread safe nor safe to use across multiple isolates. Most of them are either debugging help, a way to test incomplete features, or heuristic settings.
• V8 developers love the flexibility of being able to introduce new flags, no strings attached. Having Node.js depend on V8's internal flags would ruin this freedom.
• I personally have seen very bad examples of how V8 flags have been used. --allow-natives-syntax, --use-strict, --always-opt, and most of the experimental --harmony-XX flags for incomplete language features, just to name a few.
• Custom flags that are not the shipping configuration do not get sufficient test coverage. You could run into stability issues or worse.

That being said, there are a few flags that are safe to use, and probably won't change much over the years. E.g. --no-opt, --prof, --trace-deopt, etc. But I think it's overkill to introduce a way to feature-detect them.

[ljharb]

@hashseed isn't that list of "bad examples" a contradiction to your claim that V8 devs have the flexibility to add new flags without consequence?

[jasnell]

it's that list of "bad examples" a contradiction ...

I wouldn't think so if you view the flags as experimental and debugging switches as opposed to Things-To-Use-In-Production, which is the distinction that I believe @hashseed is making here.

What I could see as a possible option here... is making process.allowedEnvironmentNodeFlags([options]) a method with an includev8: true option that would include a whitelisted subset of known-safe v8 flags. It's not a great option by any stretch, but it is an option.

[hashseed]

V8 flags fundamentally differ from Node flags in their purpose. I'll admit that this difference is not obvious and it's unfortunate that users have been misunderstanding the purpose of V8's flags, but that doesn't mean we need to encourage these uses.

[boneskull]

who is to whitelist the subset of v8 flags? I don’t believe Node should be in the business of determining which are safe.

I’d like to remind us that listing any v8 flags is beyond the original scope, and adding them may only be for the sake of completionism instead of any real use case.

[gibfahn]

@bnoordhuis @ljharb :

Looking at this thread, it looks like you were the ones interested in having the V8 flags included. Given the feedback from @hashseed , do you still think this PR should include the V8 flags?

[ljharb]

I do. Anything users can do to change the runtime is something i need to detect; for example, some v8 flags make new core modules available. @hashseed’s concern seems to be users using the flags; making them easier to detect at runtime doesn’t seem to me to conflict with that.

[boneskull]

I withdraw my previous comment, as it sounds like @ljharb has a use case.

[jdalton]

@ljharb Can you expand on your use case. The project this bit can be helpful with, how you're working around the lack of this now, etc.

[ljharb]

For one, the resolve module currently has a long hardcoded list of core modules with quasi semver ranges, to be able to determine what a core module is (for linters, bundlers, module loaders, etc). Since there are some core modules that are only available when a flag is set, i currently have to report them as either always core, or never core. I would prefer to report them as core only when they are requireable as such - which would necessitate me checking the flags. A reliable and ergonomic way to do this would be nice, as i think parsing argv is too much complexity for a small leaf package like resolve.

[boneskull]

@hashseed,

I understand that the v8 team discourages users from actually using these flags, but at the same time, we can't pretend they don't exist. Since they exist, users will use them. As it currently stands, a userland module couldn't efficiently warn a user about consuming flags they shouldn't:

$ node --some-v8-flag script.js
WARNING: "--some-v8-flag" is unsupported!!

A brittle workaround is to parse the output of v8's --help. But nobody expects the "help" output of their executable to be parsed by a machine! :smile:

[hashseed]

I'm not pretending that users are not going to use these flags. I'm merely advising not to encourage users to use them. Offering an API as requested sounds like encouragement to me. That would give the appearance and false sense that their usage is tested and supported.

Since every V8 flag should be considered unsupported by default, I don't really see the benefit of providing an API to get these unsupported flags. In this use case

$ node --some-v8-flag script.js

there should always be a warning unless the flag is in a very small and manually curated set of flags that have been explicitly confirmed to be safe. I agree that this set could be curated by V8 and exposed via API, but in that case, don't be surprised if that list is very short.

[ljharb]

If they’re not safe, then I’d argue they shouldn’t be there at all. Users always do whatever they’re allowed to do - the only way to prevent usage is to actually remove the capability.

[hashseed]

Then I would ask Node to not call v8::V8::SetFlagsFromString or filter its input. That API was originally intended for V8's developer shell for testing.

[boneskull]

any change to Node.js' behavior around passing v8 flags should probably be tackled in a separate issue...

[boneskull]

Since every V8 flag should be considered unsupported by default, I don't really see the benefit of providing an API to get these unsupported flags. In this use case

$ node --some-v8-flag script.js

there should always be a warning unless the flag is in a very small and manually curated set of flags that have been explicitly confirmed to be safe.

I'm confused; Node & userland can't warn a user about using v8 flags if we don't know what the v8 flags are. This is why we want the list of flags...

[hashseed]

I agree.

Let me recap on the V8 flags issue. The use case is to be able to tell whether flags from CLI should be passed through to Node and V8. For this, you need a list of supported flags.

What I'm saying is that V8 flags should by default be considered unsupported. If you expect that V8 provide an API to list supported flags, and that this API would return the same set of flags that d8 --help lists, then I have to reject because these flags do not in fact belong on thay list. Just because they are already in use does not mean they are well tested.

You are essentially asking the manufacturer of Qtips to list cleaning your ear as a safe use case on the packaging. Yes, people use Qtips to clean their ears. No, it's not safe to do so.

[hashseed]

I'm confused; Node & userland can't warn a user about using v8 flags if we don't know what the v8 flags are. This is why we want the list of flags...

The way I understood this is that you want the list of supported flags to include V8 flags. If you know what flags are safe, can't you tell that all remaining flags are unsafe?

[ljharb]

This feature is called "allowed", not "supported" - inclusion in this list imo doesn't imply any support, nor any safety, just that it's a flag that does something.

[boneskull]

The way I understood this is that you want the list of supported flags to include V8 flags. If you know what flags are safe, can't you tell that all remaining flags are unsafe?

Hmm, this is the source of the confusion. I don't care about which v8 flags are "safe" (I assume none of them!). I only care that they are flags, (i.e. listed in v8's help output).

Maybe I should have been more clear in my terminology... "supported" isn't a good word. I'm not asking v8 to "support" any flags; rather just provide the list of flags as they appear in the 'help' output--in a machine-readable manner.

Yes "allowed" is correct

[BridgeAR]

Then I would ask Node to not call v8::V8::SetFlagsFromString or filter its input. That API was originally intended for V8's developer shell for testing.

This might be a solution to this problem. If we pass through only a subset of possible V8 flags, all others would definitely not be "allowed" and would not have to be listed anywhere.

@hashseed if you could add a API to provide a "safe" list of V8 flags that may be used by end users, we could make all other V8 flags no-ops. That would be a breaking change but I guess it is the best way forward.

Any further ideas?

[hashseed]

With such an API, changes to V8 flags considered to be safe to use would constitute breaking changes, right?

So far some breaking changes in V8 required a very long time for deprecation, e.g. removal of debug context. I wonder whether that would apply to flag changes too. If yes, I can imagine that V8 would be very hesitant to include flags in this list.

[ljharb]

Arguably they’re already breaking changes, with or without an explicit list. If you can decide arbitrarily that the current set of flags aren’t considered part of your api, i don’t see why you couldn’t arbitrarily decide that the list of “safe” flags also isn’t part of it.

[hashseed]

Given the lack of an API to provide a list of safe flags, all of them are unsafe. Arguing about whether changes to unsafe flags constitute a breaking change is a bit pointless imo. I don't want to repeat the arguments I brought up in above comments.

I'm just pointing out that these considerations do not encourage adding many flags to such a list. The number of flags I would vouch for to stay stable and do not open an unmaintainable API surface probably does not exceed a dozen. At this point this API might become useless, since I doubt that a PR to ignore all unlisted flags is going to be popular.

[jasnell]

Avoiding SetFlagsFromString entirely is not currently feasible as we use it specifically during bootstrap to temporarily enable --allow-natives-syntax to gain access to things that are not available otherwise. We then use it again to switch that back off also doing bootstrap.

[hashseed]

That's... horrible. That means bootstrapping relies on V8's internal implementation details. Runtime functions exposed via --allow-natives-syntax may change any time without any warning. Considering that we are moving away from JS-implemented builtins, removing runtime functions or changing their semantics is a very real possibility. Can you point me to where these are used so that we can work on a fix?

[jasnell]

Do a quick search in src/node.cc and you'll find it.