fix: oob string read when parsing node_options (#46246)

* fix: oob string read when parsing node_options

Co-authored-by: deepak1556 <[email protected]>

* chore: re-enable test

Co-authored-by: deepak1556 <[email protected]>

* fix: missing linux server env for tests

Co-authored-by: deepak1556 <[email protected]>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: deepak1556 <[email protected]>

shell/common/node_bindings.cc

@@ -365,6 +365,7 @@ void SetNodeOptions(base::Environment* env) {
   if (env->HasVar("NODE_OPTIONS")) {
     if (electron::fuses::IsNodeOptionsEnabled()) {
       std::string options;
+      std::string result_options;
       env->GetVar("NODE_OPTIONS", &options);
       const std::vector<std::string_view> parts = base::SplitStringPiece(
           options, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
@@ -379,18 +380,20 @@ void SetNodeOptions(base::Environment* env) {
           // 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());
+          continue;
         } else if (disallowed.contains(option)) {
           // 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());
+          continue;
         }
+        result_options.append(part);
+        result_options.append(" ");
       }
 
       // overwrite new NODE_OPTIONS without unsupported variables
-      env->SetVar("NODE_OPTIONS", options);
+      env->SetVar("NODE_OPTIONS", result_options);
     } else {
       LOG(WARNING) << "NODE_OPTIONS ignored due to disabled nodeOptions fuse.";
       env->UnSetVar("NODE_OPTIONS");

spec/crash-spec.ts

@@ -10,9 +10,14 @@ const fixturePath = path.resolve(__dirname, 'fixtures', 'crash-cases');
 
 let children: cp.ChildProcessWithoutNullStreams[] = [];
 
-const runFixtureAndEnsureCleanExit = async (args: string[]) => {
+const runFixtureAndEnsureCleanExit = async (args: string[], customEnv: NodeJS.ProcessEnv) => {
   let out = '';
-  const child = cp.spawn(process.execPath, args);
+  const child = cp.spawn(process.execPath, args, {
+    env: {
+      ...process.env,
+      ...customEnv
+    }
+  });
   children.push(child);
   child.stdout.on('data', (chunk: Buffer) => {
     out += chunk.toString();
@@ -70,11 +75,16 @@ describe('crash cases', () => {
     ifit(shouldRunCase(crashCase))(`the "${crashCase}" case should not crash`, () => {
       const fixture = path.resolve(fixturePath, crashCase);
       const argsFile = path.resolve(fixture, 'electron.args');
+      const envFile = path.resolve(fixture, 'electron.env.json');
       const args = [fixture];
+      let env = process.env;
       if (fs.existsSync(argsFile)) {
         args.push(...fs.readFileSync(argsFile, 'utf8').trim().split('\n'));
       }
-      return runFixtureAndEnsureCleanExit(args);
+      if (fs.existsSync(envFile)) {
+        env = JSON.parse(fs.readFileSync(envFile, 'utf8'));
+      }
+      return runFixtureAndEnsureCleanExit(args, env);
     });
   }
 });

spec/fixtures/crash-cases/node-options-parsing/electron.env.json

@@ -0,0 +1,3 @@
+{
+  "NODE_OPTIONS": "--allow-addons --enable-fips --allow-addons --enable-fips"
+}

spec/fixtures/crash-cases/node-options-parsing/index.js

@@ -0,0 +1,20 @@
+const { app, utilityProcess } = require('electron');
+
+const path = require('node:path');
+
+app.whenReady().then(() => {
+  if (process.env.NODE_OPTIONS &&
+      process.env.NODE_OPTIONS.trim() === '--allow-addons --allow-addons') {
+    const child = utilityProcess.fork(path.join(__dirname, 'options.js'), [], {
+      stdio: 'inherit',
+      env: {
+        NODE_OPTIONS: `--allow-addons --require ${path.join(__dirname, 'preload.js')} --enable-fips --allow-addons --enable-fips`,
+        ELECTRON_ENABLE_STACK_DUMPING: 'true'
+      }
+    });
+
+    child.once('exit', (code) => code ? app.exit(1) : app.quit());
+  } else {
+    app.exit(1);
+  }
+});

spec/fixtures/crash-cases/node-options-parsing/options.js

@@ -0,0 +1,8 @@
+const path = require('node:path');
+
+if (process.env.NODE_OPTIONS &&
+    process.env.NODE_OPTIONS.trim() === `--allow-addons --require ${path.join(__dirname, 'preload.js')} --allow-addons`) {
+  process.exit(0);
+} else {
+  process.exit(1);
+}

spec/node-spec.ts

@@ -705,9 +705,7 @@ describe('node feature', () => {
       expect(code).to.equal(1);
     });
 
-    // TODO(deepak1556): will be enabled in follow-up PR
-    // https://github.com/electron/electron/pull/46210
-    it.skip('does not allow --require in utility process of packaged apps', async () => {
+    it('does not allow --require in utility process of packaged apps', async () => {
       const appPath = path.join(fixtures, 'apps', 'node-options-utility-process');
       // App should exit with code 1.
       const child = childProcess.spawn(process.execPath, [appPath], {