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 all commits
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;
}
}

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 '_')
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 @@ -16,7 +16,7 @@ const jsPrimitives = {
const jsGlobalObjectsUrl = `${jsDocPrefix}Reference/Global_Objects/`;
const jsGlobalTypes = [
'Array', 'ArrayBuffer', 'DataView', 'Date', 'Error', 'EvalError', 'Function',
'Object', 'Promise', 'RangeError', 'ReferenceError', 'RegExp',
'Object', 'Promise', 'RangeError', 'ReferenceError', 'RegExp', 'Set',
'SharedArrayBuffer', 'SyntaxError', 'TypeError', 'TypedArray', 'URIError',
'Uint8Array',
];
Expand Down