process: generate list of allowed env flags programmatically

Avoids having a separate, second source of truth on this matter. Backport-PR-URL: #22847 PR-URL: #22638 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
nodejs · Sep 20, 2018 · 4ab9d6f · 4ab9d6f
1 parent ed142e1
commit 4ab9d6f
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 92 deletions.
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -620,9 +620,45 @@
     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 = [];
+    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;
+        }
+        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(' <arg>'))
+          canonical = canonical.substr(0, canonical.length - 4);
+        allowedNodeEnvironmentFlags.push(canonical);
+      }
+    }
 
     const trimLeadingDashes = (flag) => replace(flag, leadingDashesRegex, '');
 
@@ -660,6 +696,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
@@ -597,68 +597,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
@@ -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<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

diff --git 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<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;
 

diff --git 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',
@@ -50,7 +51,7 @@ require('../common');
 // assert all "canonical" flags begin with dash(es)
 {
   process.allowedNodeEnvironmentFlags.forEach((flag) => {
-    assert(/^--?[a-z8_-]+$/.test(flag), `Unexpected format for flag ${flag}`);
+    assert(/^--?[a-z28_-]+$/.test(flag), `Unexpected format for flag ${flag}`);
   });
 }