From 6f3c9fcf99dc6bdd2ccd9af5b15db89b4db4ec37 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Tue, 25 Mar 2025 08:14:33 -0500 Subject: [PATCH] fix: oob string read when parsing node_options (#46247) * fix: oob string read when parsing node_options Co-authored-by: deepak1556 * chore: re-enable test Co-authored-by: deepak1556 * fix: missing linux server env for tests Co-authored-by: deepak1556 --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: deepak1556 --- shell/common/node_bindings.cc | 9 ++++++--- spec/crash-spec.ts | 16 ++++++++++++--- .../node-options-parsing/electron.env.json | 3 +++ .../crash-cases/node-options-parsing/index.js | 20 +++++++++++++++++++ .../node-options-parsing/options.js | 8 ++++++++ .../node-options-parsing/preload.js | 0 spec/node-spec.ts | 4 +--- 7 files changed, 51 insertions(+), 9 deletions(-) create mode 100644 spec/fixtures/crash-cases/node-options-parsing/electron.env.json create mode 100644 spec/fixtures/crash-cases/node-options-parsing/index.js create mode 100644 spec/fixtures/crash-cases/node-options-parsing/options.js create mode 100644 spec/fixtures/crash-cases/node-options-parsing/preload.js diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 423aafae6eb2..8db9da3646a4 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -394,6 +394,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 parts = base::SplitStringPiece( options, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); @@ -408,18 +409,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"); diff --git a/spec/crash-spec.ts b/spec/crash-spec.ts index 253537b43b1e..1bffcd83219d 100644 --- a/spec/crash-spec.ts +++ b/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); }); } }); diff --git a/spec/fixtures/crash-cases/node-options-parsing/electron.env.json b/spec/fixtures/crash-cases/node-options-parsing/electron.env.json new file mode 100644 index 000000000000..49c7c0010caf --- /dev/null +++ b/spec/fixtures/crash-cases/node-options-parsing/electron.env.json @@ -0,0 +1,3 @@ +{ + "NODE_OPTIONS": "--allow-addons --enable-fips --allow-addons --enable-fips" +} diff --git a/spec/fixtures/crash-cases/node-options-parsing/index.js b/spec/fixtures/crash-cases/node-options-parsing/index.js new file mode 100644 index 000000000000..1a3017817483 --- /dev/null +++ b/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); + } +}); diff --git a/spec/fixtures/crash-cases/node-options-parsing/options.js b/spec/fixtures/crash-cases/node-options-parsing/options.js new file mode 100644 index 000000000000..391dbecfc7eb --- /dev/null +++ b/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); +} diff --git a/spec/fixtures/crash-cases/node-options-parsing/preload.js b/spec/fixtures/crash-cases/node-options-parsing/preload.js new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/spec/node-spec.ts b/spec/node-spec.ts index f6df35db98f5..420698f88c70 100644 --- a/spec/node-spec.ts +++ b/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], {