From aec3668e1cb41f846fa8dfebd42ab58b9c870dec Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 1 Sep 2018 00:09:46 +0200 Subject: [PATCH 1/4] process: generate list of allowed env flags programmatically Avoids having a separate, second source of truth on this matter. --- lib/internal/bootstrap/node.js | 42 +++++++++++++++++++++-- src/node.cc | 62 ---------------------------------- src/node_config.cc | 17 ---------- src/node_internals.h | 5 --- 4 files changed, 39 insertions(+), 87 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 069c8bc9975fa1..fd29c9e0fdded5 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -663,9 +663,42 @@ const test = Function.call.bind(RegExp.prototype.test); const { - allowedV8EnvironmentFlags, - allowedNodeEnvironmentFlags - } = process.binding('config'); + getOptions, + types: { kV8Option }, + envSettings: { kAllowedInEnvironment } + } = internalBinding('options'); + const { options, aliases } = getOptions(); + const allowedV8EnvironmentFlags = + [...options].filter(([name, info]) => + info.type === kV8Option && + info.envVarSettings === kAllowedInEnvironment) + .map(([name]) => name); + const allowedNodeEnvironmentFlags = + [...options].filter(([name, info]) => + info.type !== kV8Option && + info.envVarSettings === kAllowedInEnvironment) + .map(([name]) => name); + + for (const [ from, expansion ] of aliases) { + let isAccepted = true; + for (const to of expansion) { + if (!to.startsWith('-')) continue; + if (aliases.get(to)) { + expansion.push(...aliases.get(to)); + continue; + } + isAccepted = options.get(to).envVarSettings === kAllowedInEnvironment; + if (!isAccepted) break; + } + if (isAccepted) { + let canonical = from; + if (canonical.endsWith('=')) + canonical = canonical.substr(0, canonical.length - 1); + if (canonical.endsWith(' ')) + canonical = canonical.substr(0, canonical.length - 4); + allowedNodeEnvironmentFlags.push(canonical); + } + } const trimLeadingDashes = (flag) => replace(flag, leadingDashesRegex, ''); @@ -703,6 +736,9 @@ // 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. + // TODO(addaleax): It might be more flexible to run the option parser + // on a dummy option set and see whether it rejects the argument or + // not. if (typeof key === 'string') { key = replace(key, trailingValuesRegex, ''); if (test(leadingDashesRegex, key)) { diff --git a/src/node.cc b/src/node.cc index 3597571cdffd4c..0049b8cb265095 100644 --- a/src/node.cc +++ b/src/node.cc @@ -590,68 +590,6 @@ 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[] = { - // 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[] = { - "--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); -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) diff --git a/src/node_config.cc b/src/node_config.cc index 080d8550665ad3..e9561be9d7f1fa 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -5,7 +5,6 @@ namespace node { -using v8::Array; using v8::Boolean; using v8::Context; using v8::Integer; @@ -137,22 +136,6 @@ static void Initialize(Local target, READONLY_PROPERTY(debug_options_obj, "inspectorEnabled", Boolean::New(isolate, debug_options->inspector_enabled)); - - Local 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 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 diff --git a/src/node_internals.h b/src/node_internals.h index edfc00e53f13a7..5cf606ddeecd72 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -180,11 +180,6 @@ extern bool v8_initialized; extern Mutex per_process_opts_mutex; extern std::shared_ptr 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; From f05091c651cce940b15fdce75f6738ca19794f11 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 2 Sep 2018 15:13:38 +0200 Subject: [PATCH 2/4] fixup! process: generate list of allowed env flags programmatically --- lib/internal/bootstrap/node.js | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index fd29c9e0fdded5..33ee33c6ed1e3e 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -668,23 +668,26 @@ envSettings: { kAllowedInEnvironment } } = internalBinding('options'); const { options, aliases } = getOptions(); - const allowedV8EnvironmentFlags = - [...options].filter(([name, info]) => - info.type === kV8Option && - info.envVarSettings === kAllowedInEnvironment) - .map(([name]) => name); - const allowedNodeEnvironmentFlags = - [...options].filter(([name, info]) => - info.type !== kV8Option && - info.envVarSettings === kAllowedInEnvironment) - .map(([name]) => name); + + const allowedV8EnvironmentFlags = []; + const allowedNodeEnvironmentFlags = []; + for (const [name, info] of options) { + if (info.envVarSettings === kAllowedInEnvironment) { + if (info.type === kV8Option) { + allowedV8EnvironmentFlags.push(name); + } else { + allowedNodeEnvironmentFlags.push(name); + } + } + } for (const [ from, expansion ] of aliases) { let isAccepted = true; for (const to of expansion) { if (!to.startsWith('-')) continue; - if (aliases.get(to)) { - expansion.push(...aliases.get(to)); + const recursiveExpansion = aliases.get(to); + if (recursiveExpansion) { + expansion.push(...recursiveExpansion); continue; } isAccepted = options.get(to).envVarSettings === kAllowedInEnvironment; From 0002b1ef0eddc9592d7702c4cecb58f0108b4d56 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 1 Sep 2018 00:38:09 +0200 Subject: [PATCH 3/4] lib: generate allowedNodeEnvironmentFlags lazily --- lib/internal/bootstrap/node.js | 207 ++++++++++++++++++--------------- 1 file changed, 111 insertions(+), 96 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 33ee33c6ed1e3e..b3629af8120cbb 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -662,117 +662,132 @@ const has = Function.call.bind(Set.prototype.has); const test = Function.call.bind(RegExp.prototype.test); - const { - getOptions, - types: { kV8Option }, - envSettings: { kAllowedInEnvironment } - } = internalBinding('options'); - const { options, aliases } = getOptions(); - - const allowedV8EnvironmentFlags = []; - const allowedNodeEnvironmentFlags = []; - for (const [name, info] of options) { - if (info.envVarSettings === kAllowedInEnvironment) { - if (info.type === kV8Option) { - allowedV8EnvironmentFlags.push(name); - } else { - allowedNodeEnvironmentFlags.push(name); + const get = () => { + const { + getOptions, + types: { kV8Option }, + envSettings: { kAllowedInEnvironment } + } = internalBinding('options'); + const { options, aliases } = getOptions(); + + const allowedV8EnvironmentFlags = []; + const allowedNodeEnvironmentFlags = []; + for (const [name, info] of options) { + if (info.envVarSettings === kAllowedInEnvironment) { + if (info.type === kV8Option) { + allowedV8EnvironmentFlags.push(name); + } else { + allowedNodeEnvironmentFlags.push(name); + } } } - } - for (const [ from, expansion ] of aliases) { - let isAccepted = true; - for (const to of expansion) { - if (!to.startsWith('-')) continue; - const recursiveExpansion = aliases.get(to); - if (recursiveExpansion) { - expansion.push(...recursiveExpansion); - continue; + for (const [ from, expansion ] of aliases) { + let isAccepted = true; + for (const to of expansion) { + if (!to.startsWith('-')) continue; + const recursiveExpansion = aliases.get(to); + if (recursiveExpansion) { + expansion.push(...recursiveExpansion); + continue; + } + isAccepted = options.get(to).envVarSettings === kAllowedInEnvironment; + if (!isAccepted) break; + } + if (isAccepted) { + let canonical = from; + if (canonical.endsWith('=')) + canonical = canonical.substr(0, canonical.length - 1); + if (canonical.endsWith(' ')) + canonical = canonical.substr(0, canonical.length - 4); + allowedNodeEnvironmentFlags.push(canonical); } - isAccepted = options.get(to).envVarSettings === kAllowedInEnvironment; - if (!isAccepted) break; - } - if (isAccepted) { - let canonical = from; - if (canonical.endsWith('=')) - canonical = canonical.substr(0, canonical.length - 1); - if (canonical.endsWith(' ')) - canonical = canonical.substr(0, canonical.length - 4); - allowedNodeEnvironmentFlags.push(canonical); } - } - 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; - } + 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)) + ); - delete() { - // noop, `Set` API compatible - return false; - } + class NodeEnvironmentFlagsSet extends Set { + constructor(...args) { + super(...args); - clear() { - // noop - } + // the super constructor consumes `add`, but + // disallow any future adds. + this.add = () => this; + } - 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. - // TODO(addaleax): It might be more flexible to run the option parser - // on a dummy option set and see whether it rejects the argument or - // not. - if (typeof key === 'string') { - key = replace(key, trailingValuesRegex, ''); - if (test(leadingDashesRegex, key)) { - return has(this, key) || - has(v8Flags, - replace( + delete() { + // noop, `Set` API compatible + return false; + } + + clear() { + // noop + } + + 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. + // TODO(addaleax): It might be more flexible to run the option parser + // on a dummy option set and see whether it rejects the argument or + // not. + if (typeof key === 'string') { + key = replace(key, trailingValuesRegex, ''); + if (test(leadingDashesRegex, key)) { + return has(this, key) || + has(v8Flags, replace( - key, - leadingDashesRegex, - '' - ), - replaceDashesRegex, - '_' - ) - ); + replace( + key, + leadingDashesRegex, + '' + ), + replaceDashesRegex, + '_' + ) + ); + } + return has(nodeFlags, key) || + has(v8Flags, replace(key, replaceDashesRegex, '_')); } - return has(nodeFlags, key) || - has(v8Flags, replace(key, replaceDashesRegex, '_')); + return false; } - return false; } - } - Object.freeze(NodeEnvironmentFlagsSet.prototype.constructor); - Object.freeze(NodeEnvironmentFlagsSet.prototype); + Object.freeze(NodeEnvironmentFlagsSet.prototype.constructor); + Object.freeze(NodeEnvironmentFlagsSet.prototype); + + return process.allowedNodeEnvironmentFlags = Object.freeze( + new NodeEnvironmentFlagsSet( + allowedNodeEnvironmentFlags.concat(allowedV8EnvironmentFlags) + )); + }; - process.allowedNodeEnvironmentFlags = Object.freeze( - new NodeEnvironmentFlagsSet( - allowedNodeEnvironmentFlags.concat(allowedV8EnvironmentFlags) - ) - ); + Object.defineProperty(process, 'allowedNodeEnvironmentFlags', { + get, + set(value) { + Object.defineProperty(this, 'allowedNodeEnvironmentFlags', { + value, + configurable: true, + enumerable: true, + writable: true + }); + }, + enumerable: true, + configurable: true + }); } startup(); From 1f37916ad3d6e70aa0e5ca1e398ec8fa20a64ede Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 5 Sep 2018 12:51:22 +0200 Subject: [PATCH 4/4] fixup! process: generate list of allowed env flags programmatically --- test/parallel/test-process-env-allowed-flags.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-process-env-allowed-flags.js b/test/parallel/test-process-env-allowed-flags.js index 39ad09a6c35b10..b853bdd539852e 100644 --- a/test/parallel/test-process-env-allowed-flags.js +++ b/test/parallel/test-process-env-allowed-flags.js @@ -6,8 +6,6 @@ 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', @@ -17,8 +15,11 @@ require('../common'); '-r', 'r', '--stack-trace-limit=100', - '--stack-trace-limit=-=xX_nodejs_Xx=-' - ]; + '--stack-trace-limit=-=xX_nodejs_Xx=-', + ].concat(process.config.variables.v8_enable_inspector ? [ + '--inspect-brk', + 'inspect-brk', + ] : []); const badFlags = [ '--inspect_brk',