-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -416,6 +416,55 @@ generate a core file. | |
|
||
This feature is not available in [`Worker`][] threads. | ||
|
||
## process.allowedNodeEnvironmentFlags | ||
<!-- YAML | ||
added: REPLACEME | ||
--> | ||
|
||
* {Set} | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could also choose to throw here, which would be consistent with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, not a bad idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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[] = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to fit with other constant naming conventions, should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good question! 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don’t think we’ve use the |
||
// 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 '_') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry; missed this. You're right, the Instead, the API would be a function which returns So, instead of if (process.allowedEnvironmentNodeFlags.includes(arg)) {
// do stuff
} We'd have: if (process.isEnvironmentNodeFlag(arg)) {
// do stuff
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. V8 flags are even trickier — there is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...which alternatively can also be spelled as |
||
const char* const v8_environment_flags[] = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. likewise, |
||
"--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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was having trouble getting my code to compile using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That might be an issue of missing includes ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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) | ||
|
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); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this just go ahead and add Map, WeakSet, WeakMap, Symbol, etc? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = { | ||
|
There was a problem hiding this comment.
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):
node/tools/doc/type-parser.js
Lines 17 to 22 in 44d04a8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok