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

process: add allowedNodeEnvironmentFlags property #19335

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,55 @@ generate a core file.

This feature is not available in [`Worker`][] threads.

## process.allowedNodeEnvironmentFlags
<!-- YAML
added: REPLACEME
-->

* {Set}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type should be added to this list (in ABC order):

const jsGlobalTypes = [
'Array', 'ArrayBuffer', 'DataView', 'Date', 'Error', 'EvalError', 'Function',
'Object', 'Promise', 'RangeError', 'ReferenceError', 'RegExp',
'SharedArrayBuffer', 'SyntaxError', 'TypeError', 'TypedArray', 'URIError',
'Uint8Array',
];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


The `process.allowedNodeEnvironmentFlags` property is a special,
read-only `Set` of flags allowable within the [`NODE_OPTIONS`][]
environment variable.

`process.allowedNodeEnvironmentFlags` extends `Set`, but overrides
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an implementation detail that the person reading the docs doesn't really need to know, I'd just concentrate on describing the behavior of the api, e.g., how to use .has() to check for the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this qualifier was necessary since the "type" of this API member is Set. Users expecting it to work exactly like a Set will be disappointed. Perhaps the "type" should just be something generic like "object" to avoid unwanted expectations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mentioning that it's a Set subclass is important, and is more than just an implementation detail - it's the observable interface.

`Set.prototype.has` to recognize several different possible flag
representations. `process.allowedNodeEnvironmentFlags.has()` will
return `true` in the following cases:

- Flags may omit leading single (`-`) or double (`--`) dashes; e.g.,
`inspect-brk` for `--inspect-brk`, or `r` for `-r`.
- Flags passed through to V8 (as listed in `--v8-options`) may replace
one or more *non-leading* dashes for an underscore, or vice-versa;
e.g., `--perf_basic_prof`, `--perf-basic-prof`, `--perf_basic-prof`,
etc.
- Flags may contain one or more equals (`=`) characters; all
characters after and including the first equals will be ignored;
e.g., `--stack-trace-limit=100`.
- Flags *must* be allowable within [`NODE_OPTIONS`][].

When iterating over `process.allowedNodeEnvironmentFlags`, flags will
appear only *once*; each will begin with one or more dashes. Flags
passed through to V8 will contain underscores instead of non-leading
dashes:

```js
process.allowedNodeEnvironmentFlags.forEach((flag) => {
// -r
// --inspect-brk
// --abort_on_uncaught_exception
// ...
});
```

The methods `add()`, `clear()`, and `delete()` of
`process.allowedNodeEnvironmentFlags` do nothing, and will fail
silently.

If Node.js was compiled *without* [`NODE_OPTIONS`][] support (shown in
[`process.config`][]), `process.allowedNodeEnvironmentFlags` will
contain what *would have* been allowable.

## process.arch
<!-- YAML
added: v0.5.0
Expand Down Expand Up @@ -2061,8 +2110,10 @@ cases:
[`end()`]: stream.html#stream_writable_end_chunk_encoding_callback
[`net.Server`]: net.html#net_class_net_server
[`net.Socket`]: net.html#net_class_net_socket
[`NODE_OPTIONS`]: cli.html#cli_node_options_options
[`os.constants.dlopen`]: os.html#os_dlopen_constants
[`process.argv`]: #process_process_argv
[`process.config`]: #process_process_config
[`process.execPath`]: #process_process_execpath
[`process.exit()`]: #process_process_exit_code
[`process.exitCode`]: #process_process_exitcode
Expand Down
89 changes: 89 additions & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,8 @@

perf.markMilestone(NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE);

setupAllowedFlags();

// There are various modes that Node can run in. The most common two
// are running from a script and running the REPL - but there are a few
// others like the debugger or running --eval arguments. Here we decide
Expand Down Expand Up @@ -631,5 +633,92 @@
new vm.Script(source, { displayErrors: true, filename });
}

function setupAllowedFlags() {
// This builds process.allowedNodeEnvironmentFlags
// from data in the config binding

const replaceDashesRegex = /-/g;
const leadingDashesRegex = /^--?/;
const trailingValuesRegex = /=.*$/;

// Save references so user code does not interfere
const replace = Function.call.bind(String.prototype.replace);
const has = Function.call.bind(Set.prototype.has);
const test = Function.call.bind(RegExp.prototype.test);

const {
allowedV8EnvironmentFlags,
allowedNodeEnvironmentFlags
} = process.binding('config');

const trimLeadingDashes = (flag) => replace(flag, leadingDashesRegex, '');

// Save these for comparison against flags provided to
// process.allowedNodeEnvironmentFlags.has() which lack leading dashes.
// Avoid interference w/ user code by flattening `Set.prototype` into
// each object.
const [nodeFlags, v8Flags] = [
allowedNodeEnvironmentFlags, allowedV8EnvironmentFlags
].map((flags) => Object.defineProperties(
new Set(flags.map(trimLeadingDashes)),
Object.getOwnPropertyDescriptors(Set.prototype))
);

class NodeEnvironmentFlagsSet extends Set {
constructor(...args) {
super(...args);

// the super constructor consumes `add`, but
// disallow any future adds.
this.add = () => this;
}

delete() {
// noop, `Set` API compatible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, not a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return false;
}

clear() {
// noop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}

has(key) {
// This will return `true` based on various possible
// permutations of a flag, including present/missing leading
// dash(es) and/or underscores-for-dashes in the case of V8-specific
// flags. Strips any values after `=`, inclusive.
if (typeof key === 'string') {
key = replace(key, trailingValuesRegex, '');
if (test(leadingDashesRegex, key)) {
return has(this, key) ||
has(v8Flags,
replace(
replace(
key,
leadingDashesRegex,
''
),
replaceDashesRegex,
'_'
)
);
}
return has(nodeFlags, key) ||
has(v8Flags, replace(key, replaceDashesRegex, '_'));
}
return false;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’ll also want to freeze your subclass’ prototype, and the constructor.


Object.freeze(NodeEnvironmentFlagsSet.prototype.constructor);
Object.freeze(NodeEnvironmentFlagsSet.prototype);

process.allowedNodeEnvironmentFlags = Object.freeze(
new NodeEnvironmentFlagsSet(
allowedNodeEnvironmentFlags.concat(allowedV8EnvironmentFlags)
)
);
}

startup();
});
62 changes: 62 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,68 @@ const char* signo_string(int signo) {
}
}

// These are all flags available for use with NODE_OPTIONS.
//
// Disallowed flags:
// These flags cause Node to do things other than run scripts:
// --version / -v
// --eval / -e
// --print / -p
// --check / -c
// --interactive / -i
// --prof-process
// --v8-options
// These flags are disallowed because security:
// --preserve-symlinks
const char* const environment_flags[] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to fit with other constant naming conventions, should this be kEnvironmentFlags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question! 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think we’ve use the kSomething notation for non-primitives in C++ so far. This can stay as-is, if you like (ditto below)

// Node options, sorted in `node --help` order for ease of comparison.
"--enable-fips",
"--experimental-modules",
"--experimenatl-repl-await",
"--experimental-vm-modules",
"--experimental-worker",
"--force-fips",
"--icu-data-dir",
"--inspect",
"--inspect-brk",
"--inspect-port",
"--loader",
"--napi-modules",
"--no-deprecation",
"--no-force-async-hooks-checks",
"--no-warnings",
"--openssl-config",
"--pending-deprecation",
"--redirect-warnings",
"--require",
"--throw-deprecation",
"--tls-cipher-list",
"--trace-deprecation",
"--trace-event-categories",
"--trace-event-file-pattern",
"--trace-events-enabled",
"--trace-sync-io",
"--trace-warnings",
"--track-heap-objects",
"--use-bundled-ca",
"--use-openssl-ca",
"--v8-pool-size",
"--zero-fill-buffers",
"-r"
};

// V8 options (define with '_', which allows '-' or '_')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't affect what the consumers of the API receive, does it?
It looks like the consumers need to process _ as [\-_] on their end, is that planned?

Copy link
Contributor Author

@boneskull boneskull Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry; missed this. You're right, the Array is just not useful for these. I don't really see a "good" way around this using a data structure.

Instead, the API would be a function which returns true or false if a value matches an allowed NODE_OPTIONS environment flag. That may be all we need here--Mocha certainly doesn't need to iterate over a list--but I'm curious if @benjamingr, @jdalton or @ljharb thinks that would be a sufficient API.

So, instead of

if (process.allowedEnvironmentNodeFlags.includes(arg)) {
  // do stuff
}

We'd have:

if (process.isEnvironmentNodeFlag(arg)) {
  // do stuff
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm not opposed to changing this. I'd rather this PR land with wider approval than just land ASAP.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine either way - although I tend to lean towards the list and guiding users to consume it the way @ChALkeR requested.

I think the a motivation for me other than API support for this API other than making libraries easier to write is the ability to determine allowed flags across node versions.

Hopefully, when we add flags tools I use would be able to pick up on them - I am not as-concerned with consumers processing - and _ but I defer to @ChALkeR

Copy link
Member

@ChALkeR ChALkeR Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V8 flags are even trickier — there is a --no-something flag for every boolean --something flag in --v8-options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...which alternatively can also be spelled as --nosomething.

const char* const v8_environment_flags[] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise, kV8EnvironmentFlags (unless another reviewer more familiar with the C++ codebase has said otherwise).

"--abort_on_uncaught_exception",
"--max_old_space_size",
"--perf_basic_prof",
"--perf_prof",
"--stack_trace_limit",
};

int v8_environment_flags_count = arraysize(v8_environment_flags);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like a bit of a micro-optimization to store this value, and it puts more variables in global space; I'd just use arraysize() in both places where this variable is used personally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having trouble getting my code to compile using arraysize() within node_config.cc. I'll dig up the error...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding node_internals.h to node_config.cc results in:

../src/node_internals.h:239:18: note: candidate template ignored: could not match 'const T [N]' against 'const char *const []'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some chatter about this in IRC, we came to the conclusion that we don't know why arraysize isn't working in node_config.cc. I'm going to leave it as-is for now.

int environment_flags_count = arraysize(environment_flags);

// Look up environment variable unless running as setuid root.
bool SafeGetenv(const char* key, std::string* text) {
#if !defined(__CloudABI__) && !defined(_WIN32)
Expand Down
17 changes: 17 additions & 0 deletions src/node_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

namespace node {

using v8::Array;
using v8::Boolean;
using v8::Context;
using v8::Integer;
Expand Down Expand Up @@ -132,6 +133,22 @@ static void Initialize(Local<Object> target,
READONLY_PROPERTY(debug_options_obj,
"inspectorEnabled",
Boolean::New(isolate, debug_options->inspector_enabled));

Local<Array> environmentFlags = Array::New(env->isolate(),
environment_flags_count);
READONLY_PROPERTY(target, "allowedNodeEnvironmentFlags", environmentFlags);
for (int i = 0; i < environment_flags_count; ++i) {
environmentFlags->Set(i, OneByteString(env->isolate(),
environment_flags[i]));
}

Local<Array> v8EnvironmentFlags = Array::New(env->isolate(),
v8_environment_flags_count);
READONLY_PROPERTY(target, "allowedV8EnvironmentFlags", v8EnvironmentFlags);
for (int i = 0; i < v8_environment_flags_count; ++i) {
v8EnvironmentFlags->Set(i, OneByteString(env->isolate(),
v8_environment_flags[i]));
}
} // InitConfig

} // namespace node
Expand Down
5 changes: 5 additions & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,11 @@ extern bool v8_initialized;

extern std::shared_ptr<PerProcessOptions> per_process_opts;

extern const char* const environment_flags[];
extern int environment_flags_count;
extern const char* const v8_environment_flags[];
extern int v8_environment_flags_count;

// Forward declaration
class Environment;

Expand Down
74 changes: 74 additions & 0 deletions test/parallel/test-process-env-allowed-flags.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
'use strict';

const assert = require('assert');
require('../common');

// assert legit flags are allowed, and bogus flags are disallowed
{
const goodFlags = [
'--inspect-brk',
'inspect-brk',
'--perf_basic_prof',
'--perf-basic-prof',
'perf-basic-prof',
'--perf_basic-prof',
'perf_basic-prof',
'perf_basic_prof',
'-r',
'r',
'--stack-trace-limit=100',
'--stack-trace-limit=-=xX_nodejs_Xx=-'
];

const badFlags = [
'--inspect_brk',
'INSPECT-BRK',
'--INSPECT-BRK',
'--r',
'-R',
'---inspect-brk',
'--cheeseburgers'
];

goodFlags.forEach((flag) => {
assert.strictEqual(
process.allowedNodeEnvironmentFlags.has(flag),
true,
`flag should be in set: ${flag}`
);
});

badFlags.forEach((flag) => {
assert.strictEqual(
process.allowedNodeEnvironmentFlags.has(flag),
false,
`flag should not be in set: ${flag}`
);
});
}

// assert all "canonical" flags begin with dash(es)
{
process.allowedNodeEnvironmentFlags.forEach((flag) => {
assert.strictEqual(/^--?[a-z8_-]+$/.test(flag), true);
});
}

// assert immutability of process.allowedNodeEnvironmentFlags
{
assert.strictEqual(Object.isFrozen(process.allowedNodeEnvironmentFlags),
true);

process.allowedNodeEnvironmentFlags.add('foo');
assert.strictEqual(process.allowedNodeEnvironmentFlags.has('foo'), false);
process.allowedNodeEnvironmentFlags.forEach((flag) => {
assert.strictEqual(flag === 'foo', false);
});

process.allowedNodeEnvironmentFlags.clear();
assert.strictEqual(process.allowedNodeEnvironmentFlags.size > 0, true);

const size = process.allowedNodeEnvironmentFlags.size;
process.allowedNodeEnvironmentFlags.delete('-r');
assert.strictEqual(process.allowedNodeEnvironmentFlags.size, size);
}
2 changes: 1 addition & 1 deletion tools/doc/type-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

];

const customTypesMap = {
Expand Down