From a2e4d5266dd982f04cedf9829255a2e797f87a8d Mon Sep 17 00:00:00 2001 From: jdalton Date: Fri, 24 Jul 2026 14:45:37 -0400 Subject: [PATCH] fix(optimize): preserve inherited NODE_OPTIONS in agent installer runAgentInstall set the child's NODE_OPTIONS to only our own Node flags (harden / no-warnings / disable-sigusr1). A child process' NODE_OPTIONS REPLACES the parent's rather than extending it, so any NODE_OPTIONS the user configured globally was silently dropped when `socket optimize` ran the package-manager install. Merge the inherited process.env.NODE_OPTIONS ahead of our added flags via a new pure `mergeNodeOptions` helper in util/process/cmd.mts, unit-tested in isolation. The value is left unquoted (it is assigned to an env var, not passed through a shell). Same class of NODE_OPTIONS-clobber bug as the v1.x shadow-npm fix for #1160/#1036, but a different code path: this is the `socket optimize` agent installer on main, not the `socket npm` shadow wrapper. On main `socket npm` hands off to Socket Firewall and no longer builds Node flags itself, so this is the remaining place on main that needed it. --- .../src/commands/optimize/agent-installer.mts | 6 ++- packages/cli/src/util/process/cmd.mts | 22 ++++++++++ .../optimize/agent-installer.test.mts | 34 +++++++++++++++ .../cli/test/unit/util/process/cmd.test.mts | 41 +++++++++++++++++++ 4 files changed, 101 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/optimize/agent-installer.mts b/packages/cli/src/commands/optimize/agent-installer.mts index b48da6bff9..2e28949bc6 100644 --- a/packages/cli/src/commands/optimize/agent-installer.mts +++ b/packages/cli/src/commands/optimize/agent-installer.mts @@ -22,7 +22,7 @@ import { WIN32 } from '@socketsecurity/lib-stable/constants/platform' import { getOwn } from '@socketsecurity/lib-stable/objects/inspect' import { spawn } from '@socketsecurity/lib-stable/process/spawn/child' -import { cmdFlagsToString } from '../../util/process/cmd.mts' +import { mergeNodeOptions } from '../../util/process/cmd.mts' import type { EnvDetails } from '../../util/ecosystem/environment.mjs' import type { SpinnerInstance } from '@socketsecurity/lib-stable/spinner/types' @@ -91,7 +91,9 @@ export function runAgentInstall( ...process.env, // Set CI mode for pnpm to ensure consistent behavior. ...(isPnpm ? { CI: '1' } : {}), - NODE_OPTIONS: cmdFlagsToString([ + // Merge our flags into any inherited NODE_OPTIONS instead of replacing + // it, so a user's globally-configured NODE_OPTIONS is preserved. + NODE_OPTIONS: mergeNodeOptions(process.env['NODE_OPTIONS'], [ ...(skipNodeHardenFlags ? [] : getNodeHardenFlags()), ...getNodeNoWarningsFlags(), ...getNodeDisableSigusr1Flags(), diff --git a/packages/cli/src/util/process/cmd.mts b/packages/cli/src/util/process/cmd.mts index 6430f0b2d6..53f5d06a3e 100644 --- a/packages/cli/src/util/process/cmd.mts +++ b/packages/cli/src/util/process/cmd.mts @@ -146,3 +146,25 @@ export function filterFlags( export function isHelpFlag(cmdArg: string): boolean { return helpFlags.has(cmdArg) } + +/** + * Merge Node flags into a NODE_OPTIONS value without clobbering an inherited + * one. + * + * A child process' NODE_OPTIONS env var REPLACES (does not extend) the + * parent's, so setting it to only our own flags silently drops any NODE_OPTIONS + * the user configured globally. This joins, in order, the caller's existing + * NODE_OPTIONS ahead of the flags we add so both are honoured. + * + * The value is intentionally not quoted: it is assigned directly to an env var + * (not passed through a shell), and consumers that re-tokenize NODE_OPTIONS on + * whitespace (e.g. Next.js) mishandle embedded quotes. + */ +export function mergeNodeOptions( + envNodeOptions: string | undefined, + addedFlags: string[] | readonly string[], +): string { + return [envNodeOptions, cmdFlagsToString(addedFlags)] + .filter(Boolean) + .join(' ') +} diff --git a/packages/cli/test/unit/commands/optimize/agent-installer.test.mts b/packages/cli/test/unit/commands/optimize/agent-installer.test.mts index 89ee310540..6b27db30a9 100644 --- a/packages/cli/test/unit/commands/optimize/agent-installer.test.mts +++ b/packages/cli/test/unit/commands/optimize/agent-installer.test.mts @@ -43,6 +43,9 @@ vi.mock(import('../../../../src/util/process/cmd.mts'), () => ({ .map(([k, v]) => `--${k}=${String(v)}`) .join(' '), ), + mergeNodeOptions: vi.fn((envNodeOptions, addedFlags) => + [envNodeOptions, ...(addedFlags || [])].filter(Boolean).join(' '), + ), })) vi.mock( @@ -109,6 +112,37 @@ describe('agent installer utilities', () => { ) }) + it('preserves an inherited NODE_OPTIONS instead of clobbering it', async () => { + const { spawn } = vi.mocked( + await import('@socketsecurity/lib-stable/process/spawn/child'), + ) + spawn.mockReturnValue(Promise.resolve({ status: 0 }) as unknown) + + const pkgEnvDetails = { + agent: 'npm', + agentExecPath: '/usr/bin/npm', + pkgPath: '/test/project', + agentVersion: { major: 10, minor: 0, patch: 0 }, + } as unknown + + const originalNodeOptions = process.env['NODE_OPTIONS'] + process.env['NODE_OPTIONS'] = '--max-old-space-size=4096' + try { + await runAgentInstall(pkgEnvDetails) + } finally { + if (originalNodeOptions === undefined) { + delete process.env['NODE_OPTIONS'] + } else { + process.env['NODE_OPTIONS'] = originalNodeOptions + } + } + + const spawnEnv = ( + spawn.mock.calls[0]![2] as { env: Record } + ).env + expect(spawnEnv['NODE_OPTIONS']).toContain('--max-old-space-size=4096') + }) + it('uses spawn for pnpm agent', async () => { const { spawn } = vi.mocked( await import('@socketsecurity/lib-stable/process/spawn/child'), diff --git a/packages/cli/test/unit/util/process/cmd.test.mts b/packages/cli/test/unit/util/process/cmd.test.mts index 08198a0389..0f86517abb 100644 --- a/packages/cli/test/unit/util/process/cmd.test.mts +++ b/packages/cli/test/unit/util/process/cmd.test.mts @@ -31,6 +31,7 @@ import { cmdPrefixMessage, filterFlags, isHelpFlag, + mergeNodeOptions, } from '../../../../src/util/process/cmd.mts' describe('cmd utilities', () => { @@ -99,6 +100,46 @@ describe('cmd utilities', () => { }) }) + describe('mergeNodeOptions', () => { + const addedFlags = [ + '--disable-warning=ExperimentalWarning', + '--no-warnings', + ] + + it('prepends the inherited NODE_OPTIONS ahead of added flags', () => { + const result = mergeNodeOptions('--max-old-space-size=4096', addedFlags) + expect(result).toBe( + '--max-old-space-size=4096 --disable-warning=ExperimentalWarning --no-warnings', + ) + }) + + it('returns only the added flags when NODE_OPTIONS is undefined', () => { + const result = mergeNodeOptions(undefined, addedFlags) + expect(result).toBe('--disable-warning=ExperimentalWarning --no-warnings') + }) + + it('drops an empty NODE_OPTIONS without adding a stray separator', () => { + const result = mergeNodeOptions('', addedFlags) + expect(result).toBe('--disable-warning=ExperimentalWarning --no-warnings') + }) + + it('preserves the inherited NODE_OPTIONS when there are no added flags', () => { + expect(mergeNodeOptions('--enable-source-maps', [])).toBe( + '--enable-source-maps', + ) + }) + + it('does not wrap or quote the merged value', () => { + const result = mergeNodeOptions('--enable-source-maps', addedFlags) + expect(result).not.toContain("'") + expect(result).not.toContain('"') + }) + + it('returns an empty string when nothing is provided', () => { + expect(mergeNodeOptions(undefined, [])).toBe('') + }) + }) + describe('isHelpFlag', () => { it('identifies --help flag', () => { expect(isHelpFlag('--help')).toBe(true)