feat: add fuses for NODE_OPTIONS and --inspect (#30420)

* feat: add fuses for NODE_OPTIONS and --inspect

* chore: add node patch to ensure NODE_OPTIONS are never parsed when fuse is disabledd

* chore: fix lint

* chore: flip boolean logic

* chore: update patches

* chore: add trailing _ to static member

* Update add_should_read_node_options_from_env_option_to_disable_node_options.patch

* chore: update patches

Co-authored-by: Samuel Attard <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: Samuel Attard <[email protected]>

build/fuses/fuses.json5

@@ -3,5 +3,7 @@
   "_schema": "0 == off, 1 == on, r == removed fuse",
   "_version": 1,
   "run_as_node": "1",
-  "cookie_encryption": "0"
+  "cookie_encryption": "0",
+  "node_options": "1",
+  "node_cli_inspect": "1"
 }

patches/node/.patches

@@ -25,3 +25,4 @@ src_allow_embedders_to_provide_a_custom_pageallocator_to.patch
 fix_crypto_tests_to_run_with_bssl.patch
 fix_account_for_debugger_agent_race_condition.patch
 fix_use_new_v8_error_message_property_access_format.patch
+add_should_read_node_options_from_env_option_to_disable_node_options.patch

patches/node/add_should_read_node_options_from_env_option_to_disable_node_options.patch

@@ -0,0 +1,68 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Samuel Attard <[email protected]>
+Date: Wed, 21 Jul 2021 13:40:59 -0700
+Subject: add should_read_node_options_from_env option to disable NODE_OPTIONS
+ parsing at runtime
+
+We can remove the NODE_OPTIONS environment variable but it in theory could be injected / re-inserted at runtime and be used for workers.  In order to ensure the fuse is respected we need a hard runtime toggle for NODE_OPTION support.
+
+diff --git a/src/env.cc b/src/env.cc
+index 1cc7da1ce15f43905ce607adcc1a23ed9d92948a..16af6aec3791df1363682f1ed024c52208b9a8f6 100644
+--- a/src/env.cc
++++ b/src/env.cc
+@@ -329,6 +329,9 @@ std::string GetExecPath(const std::vector<std::string>& argv) {
+   return exec_path;
+ }
+ 
++/* static */
++bool Environment::should_read_node_options_from_env_ = true;
++
+ Environment::Environment(IsolateData* isolate_data,
+                          Isolate* isolate,
+                          const std::vector<std::string>& args,
+diff --git a/src/env.h b/src/env.h
+index 45210f074a0ca4d57f9fdc5019e8e82540b28b72..c0da6c53028bc9459065caf25c1221f556b22d68 100644
+--- a/src/env.h
++++ b/src/env.h
+@@ -1143,6 +1143,8 @@ class Environment : public MemoryRetainer {
+   inline double trigger_async_id();
+   inline double get_default_trigger_async_id();
+ 
++  static bool should_read_node_options_from_env_;
++
+   // List of id's that have been destroyed and need the destroy() cb called.
+   inline std::vector<double>* destroy_async_id_list();
+ 
+diff --git a/src/node.cc b/src/node.cc
+index 6302bb925339d709a54151a8fc8b58f1a2253881..48a9eedfaf6350fc05fb104a1f44e85b75878f9a 100644
+--- a/src/node.cc
++++ b/src/node.cc
+@@ -882,7 +882,7 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
+ #if !defined(NODE_WITHOUT_NODE_OPTIONS)
+   std::string node_options;
+ 
+-  if (credentials::SafeGetenv("NODE_OPTIONS", &node_options)) {
++  if (Environment::should_read_node_options_from_env_ && credentials::SafeGetenv("NODE_OPTIONS", &node_options)) {
+     std::vector<std::string> env_argv =
+         ParseNodeOptionsEnvVar(node_options, errors);
+ 
+diff --git a/src/node_worker.cc b/src/node_worker.cc
+index 43a3862cc69dc39b95b329f713691b19de3b8d7e..7782e8488f797a8cf83b1e02f012797ef256afe7 100644
+--- a/src/node_worker.cc
++++ b/src/node_worker.cc
+@@ -457,6 +457,7 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
+     });
+ 
+ #ifndef NODE_WITHOUT_NODE_OPTIONS
++    if (Environment::should_read_node_options_from_env_) {
+     MaybeLocal<String> maybe_node_opts =
+         env_vars->Get(isolate, OneByteString(isolate, "NODE_OPTIONS"));
+     Local<String> node_opts;
+@@ -487,6 +488,7 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
+         return;
+       }
+     }
++    }
+ #endif
+   }
+ 

shell/common/node_bindings.cc

@@ -25,6 +25,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_paths.h"
 #include "electron/buildflags/buildflags.h"
+#include "electron/fuses.h"
 #include "shell/browser/api/electron_api_app.h"
 #include "shell/common/api/electron_bindings.h"
 #include "shell/common/electron_command_line.h"
@@ -188,16 +189,26 @@ void ErrorMessageListener(v8::Local<v8::Message> message,
   }
 }
 
+const std::unordered_set<base::StringPiece, base::StringPieceHash>
+GetAllowedDebugOptions() {
+  if (electron::fuses::IsNodeCliInspectEnabled()) {
+    // Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode
+    return {
+        "--inspect",          "--inspect-brk",
+        "--inspect-port",     "--debug",
+        "--debug-brk",        "--debug-port",
+        "--inspect-brk-node", "--inspect-publish-uid",
+    };
+  }
+  // If node CLI inspect support is disabled, allow no debug options.
+  return {};
+}
+
 // Initialize Node.js cli options to pass to Node.js
 // See https://nodejs.org/api/cli.html#cli_options
 void SetNodeCliFlags() {
-  // Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode
-  const std::unordered_set<base::StringPiece, base::StringPieceHash> allowed = {
-      "--inspect",          "--inspect-brk",
-      "--inspect-port",     "--debug",
-      "--debug-brk",        "--debug-port",
-      "--inspect-brk-node", "--inspect-publish-uid",
-  };
+  const std::unordered_set<base::StringPiece, base::StringPieceHash> allowed =
+      GetAllowedDebugOptions();
 
   const auto argv = base::CommandLine::ForCurrentProcess()->argv();
   std::vector<std::string> args;
@@ -231,7 +242,7 @@ void SetNodeCliFlags() {
   } else if (!errors.empty()) {
     LOG(ERROR) << err_str << base::JoinString(errors, " ");
   }
-}  // namespace
+}
 
 // Initialize NODE_OPTIONS to pass to Node.js
 // See https://nodejs.org/api/cli.html#cli_node_options_options
@@ -246,34 +257,39 @@ void SetNodeOptions(base::Environment* env) {
                                                      "--http-parser"};
 
   if (env->HasVar("NODE_OPTIONS")) {
-    std::string options;
-    env->GetVar("NODE_OPTIONS", &options);
-    std::vector<std::string> parts = base::SplitString(
-        options, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
-
-    bool is_packaged_app = electron::api::App::IsPackaged();
-
-    for (const auto& part : parts) {
-      // Strip off values passed to individual NODE_OPTIONs
-      std::string option = part.substr(0, part.find('='));
-
-      if (is_packaged_app &&
-          allowed_in_packaged.find(option) == allowed_in_packaged.end()) {
-        // Explicitly disallow majority of NODE_OPTIONS in packaged apps
-        LOG(ERROR) << "Most NODE_OPTIONs are not supported in packaged apps."
-                   << " See documentation for more details.";
-        options.erase(options.find(option), part.length());
-      } else if (disallowed.find(option) != disallowed.end()) {
-        // Remove NODE_OPTIONS specifically disallowed for use in Node.js
-        // through Electron owing to constraints like BoringSSL.
-        LOG(ERROR) << "The NODE_OPTION " << option
-                   << " is not supported in Electron";
-        options.erase(options.find(option), part.length());
+    if (electron::fuses::IsNodeOptionsEnabled()) {
+      std::string options;
+      env->GetVar("NODE_OPTIONS", &options);
+      std::vector<std::string> parts = base::SplitString(
+          options, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+
+      bool is_packaged_app = electron::api::App::IsPackaged();
+
+      for (const auto& part : parts) {
+        // Strip off values passed to individual NODE_OPTIONs
+        std::string option = part.substr(0, part.find('='));
+
+        if (is_packaged_app &&
+            allowed_in_packaged.find(option) == allowed_in_packaged.end()) {
+          // Explicitly disallow majority of NODE_OPTIONS in packaged apps
+          LOG(ERROR) << "Most NODE_OPTIONs are not supported in packaged apps."
+                     << " See documentation for more details.";
+          options.erase(options.find(option), part.length());
+        } else if (disallowed.find(option) != disallowed.end()) {
+          // Remove NODE_OPTIONS specifically disallowed for use in Node.js
+          // through Electron owing to constraints like BoringSSL.
+          LOG(ERROR) << "The NODE_OPTION " << option
+                     << " is not supported in Electron";
+          options.erase(options.find(option), part.length());
+        }
       }
-    }
 
-    // overwrite new NODE_OPTIONS without unsupported variables
-    env->SetVar("NODE_OPTIONS", options);
+      // overwrite new NODE_OPTIONS without unsupported variables
+      env->SetVar("NODE_OPTIONS", options);
+    } else {
+      LOG(ERROR) << "NODE_OPTIONS have been disabled in this app";
+      env->SetVar("NODE_OPTIONS", "");
+    }
   }
 }
 
@@ -364,6 +380,8 @@ void NodeBindings::Initialize() {
 
   auto env = base::Environment::Create();
   SetNodeOptions(env.get());
+  node::Environment::should_read_node_options_from_env_ =
+      fuses::IsNodeOptionsEnabled();
 
   std::vector<std::string> argv = {"electron"};
   std::vector<std::string> exec_argv;